Repository navigation
Turns carry a gapless per-session ordinal, backfilled from rowid - #75
Merged
Merged
Conversation
Ordering a session meant sorting by timestamp, which is not a total order over real data: on the author's 965,061-turn archive 19,160 adjacent turn pairs share a timestamp exactly, and 6,597 carry one earlier than the turn inserted before them. A sort with ties also leaves row order free to change between calls, so anything paginating a session's turns silently skips and repeats rows. Migration 008 adds turns.ordinal with UNIQUE(session_id, ordinal), plus sessions.last_entry_uuid, and backfills both from rowid order. The parser numbers turns as it walks the file, and sync derives the authoritative ordinal from the slice index — an adapter expresses a session's order as the order of ParsedSession.Turns and carries no position field, so there is no second number that could disagree. Ordered by rowid alone, not by (timestamp, rowid) as the issue first proposed. rowid is insertion order, insertion order is the order the parser walked the file, and file order is the ground truth. Verified read-only rather than assumed: of the 1,413 sessions whose source .jsonl is still on disk, ORDER BY rowid reproduced the file's line order for 1,412 and ORDER BY timestamp, rowid for only 1,336. One sequence per session over every turn type, so no consumer has to know which types participate; strictly per session, so a subagent transcript — its own session row since 007 — numbers from 0 with no special case. next_ordinal is deliberately not carried: MAX(ordinal) is an index seek against the new index, and a counter is one more thing to drift during the per-file replace sync performs. last_entry_uuid is read, not just stored. Sync checks it inside the transaction that is about to replace a session's turns: if the transcript just parsed does not contain the turn the previous sync left at the end of the sequence, the file was rewritten rather than appended to, and the replace is swapping the session's history for a different one. Neither a row count nor an mtime can see that difference. The replace still happens; doing it silently was the problem. Two things the constraint turned up: turns_au fired on every column, so the backfill's full-table UPDATE re-indexed content that had not changed. Measured on 965,000 turns: 9.2s and +377 MB of dead FTS segments, against 2.2s and +20 MB once the trigger is scoped to content. Nothing in the tree updates turns.content, so the wide form was never buying anything. InsertTurns is INSERT OR REPLACE, and SQLite resolves a REPLACE against a unique index by deleting the row it conflicts with — so a batch with unassigned ordinals landed as one surviving turn rather than an error. checkDistinctOrdinals makes that loud and names the two turns. MergeFrom computes ordinals for a pre-008 incoming archive, which is the main thing `ccvault import` is pointed at; without it every imported turn would land on DEFAULT 0 and the second one in a session would abort the merge. schema.sql also picks up the parent_session_id column and index that 007 added without updating the reference copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GetTurns already orders by ordinal; this is the consumer half. get_turns hands every turn its ordinal. offset counts rows in whatever a call returned — it shifts the moment a type filter is applied — while the ordinal names the turn's place in the session and does not, so it is the thing a caller resumes from. Its pagination was exposed to the same hazard PR #35 fixed on the sessions listing with an `s.id ASC` tiebreaker: it slices GetTurns' result by offset, and that result used to come back sorted on a key with 19,160 ties in the real archive, leaving SQLite free to order it differently between calls and the pagination free to skip and repeat rows. The two cross-session turn searches get a deterministic sort for the same reason. Ordinal is not an ordering key there — a position only means something inside one session — so they keep timestamp DESC and gain a `t.id ASC` tiebreaker, which is what decides whether a given turn falls inside LIMIT on repeat runs of the same query. The MCP and search projections now carry ordinal, so a turn that arrives from a search knows where it sits in its session. The sync and MCP tests are seeded with timestamps that both tie in pairs and run backwards across them, so every competing ordering — timestamp, id, timestamp-then-id — is wrong in a way the assertions see. Seeding ties alone would not have distinguished them: SQLite may break a tie in any order, which is the defect rather than a property to assert against. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cursor The tool description advertised the ordinal as "the cursor to resume from" while get_turns accepted only offset, so there was no way to actually use the thing it pointed at. after_ordinal closes that: it names a turn rather than counting rows, so it is unaffected by a type filter and still means the same place on the next call. Every response reports next_after_ordinal, whether or not there is more — a caller polling a live session wants to know where it got to even once it has caught up. get_session_summary now reports last_ordinal and last_entry_uuid, which is where SessionTurnCursor earns its place: a caller can find the end of a session without paging through it, and one holding both can tell "the session grew" from "the transcript was rewritten under me". Dropped SessionTurnCursorTx, which had no caller — sync reads the stored column through SessionLastEntryUUIDTx. While in getTurns: limit and offset arrive from an MCP caller and only had an upper clamp, so a negative value reached turns[offset:] / turns[:limit] and panicked the server, and limit 0 returned no turns while reporting has_more with next_offset unchanged — an endless loop for anything paging. Its siblings listSessions and listProjects already clamp both bounds. searchConversations at server.go:615 has the same unclamped slice and is left alone for a separate change. adapter.ParsedSession.Turns documents the ordering contract sync depends on. It was relied upon from the consumer side and stated nowhere on the producer side, and an adapter that returned turns sorted by timestamp would silently renumber the conversation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…stream SessionsRewrittenUpstream was counted but printed only under --verbose, while its two siblings — skipped lines and truncated raw_json — are both in the summary unconditionally. A diagnostic nobody sees is not a diagnostic, and this is the one that says a session's archived turns were swapped for a different set rather than extended. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 2 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (19)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #29.
Turns had no monotonic per-session position, so ordering a session meant sorting by
timestamp. That is not a total order over real data, and the archive says so:Beyond wrong order, a sort with ties is a sort that may come back differently next
time — so anything paginating a session's turns silently skips and repeats rows.
What landed
Migration 008 adds
turns.ordinalwithUNIQUE(session_id, ordinal)andsessions.last_entry_uuid, and backfills both. The parser numbers turns as it walksthe file; sync derives the authoritative ordinal from the slice index. One gapless
sequence per session over every turn type, so no consumer needs to know which
types take part; strictly per session, so a subagent transcript — its own session row
since #31 — numbers from 0 with no special case.
next_ordinalis deliberately not carried:MAX(ordinal)is an index seek against thenew index, and a counter is one more thing to drift during the per-file replace sync does.
The backfill orders by
rowidalone — verified, not assumedThe issue proposed
ORDER BY timestamp, rowid. That reintroduces the ties it exists tofix.
rowidis insertion order, insertion order is the order the parser walked the file,and file order is the ground truth.
Checked read-only against the real archive, comparing each ordering against the actual
.jsonlline order. Of the 1,413 sessions whose source file is still on disk:ORDER BY rowidORDER BY timestamp, rowidEvery one of the 77 timestamp-first failures is a session the issue's proposed ordering
would have gotten wrong. The single
rowidmiss had three turns rewritten by a laterpartial ingest, so the database no longer holds their original position in any column —
nothing recoverable in SQL, and the next sync of that file renumbers it correctly.
last_entry_uuidis read, not just storedSync checks it inside the transaction that is about to replace a session's turns. If the
transcript just parsed does not contain the turn the previous sync left at the end of the
sequence, the file was rewritten rather than appended to, and the replace is swapping
the session's history for a different one. Neither a row count nor an mtime can see that
difference. The replace still happens; doing it silently was the problem.
ccvault syncreports it alongside its sibling diagnostics.
get_turns had the pagination bug this issue predicts
It slices
GetTurns' result by offset, and that result was sorted on a key with 19,160ties — the same hazard PR #35 hit on the sessions listing and fixed with an
s.id ASCtiebreaker. Ordering on
ordinalcloses it.get_turnsnow also acceptsafter_ordinaland reportsnext_after_ordinal: a position names a turn, so unlikeoffsetit is unaffected by a type filter and still means the same place on a later call.get_session_summaryreportslast_ordinal/last_entry_uuidso a caller can find theend of a session without paging through it.
The two cross-session turn searches keep
timestamp DESC— a position means nothingacross sessions — and gain a
t.id ASCtiebreaker, which is what decides whether a giventurn falls inside
LIMITon repeat runs of the same query.Three things the constraint turned up
turns_aufired on every column, so the backfill's full-table UPDATE re-indexedcontent that had not changed. Measured on 965,000 turns: 9.2s and +377 MB of dead
FTS segments, against 2.2s and +20 MB once scoped to
content. Nothing in the treeupdates
turns.content, so the wide form was never buying anything.INSERT OR REPLACEturned an ordinal collision into silent row loss. SQLite resolvesa REPLACE against a unique index by deleting the conflicting row, so a batch with
unassigned ordinals landed as one surviving turn rather than an error.
checkDistinctOrdinalsmakes that loud and names the two turns.MergeFromwould have aborted on a pre-008 archive — the main thingccvault importis pointed at.
sharedColumnsdrops a column the incoming database lacks, so everyimported turn would land on
DEFAULT 0and the second one in a session would violate theindex. It now synthesizes positions from the incoming rowids.
Also:
getTurnsonly clamped the upper bound on caller-suppliedlimit/offset, so anegative value reached
turns[offset:]and panicked the server, andlimit: 0returned noturns while reporting
has_more— an endless loop for anything paging.How the backfill is proved non-vacuous
seedPre008applies only the migrations below 008, then asserts the columns do not yetexist before seeding — so nothing can write a correct value early — and reopens through
db.Opento let 008 run. The fixtures' timestamps contradict their insertion order, so theexpected ordinals are reachable only by ordering on
rowid. Mutating the migration toORDER BY timestamp, rowidfails the test on 5 of 6 turns; mutatingGetTurnsto eithertimestamp ASCortimestamp ASC, id ASCfails the MCP pagination tests.Rehearsed on the real CLI, too: synced a fixture with a binary built from
main(pre-008),then re-synced the same archive with this branch. Sync skipped both sessions as unchanged,
so the ordinals came from the migration backfill alone — and they reproduced file order
exactly across all five turn types, with
last_entry_uuidnaming the last line rather thanthe latest timestamp.
ccvault importof that pre-008 archive and the rewrite detection weredriven the same way.
Verification
go build ./...,go test ./...with a cleared cache (20/20 packages), andgo test -race ./internal/db/... ./internal/sync/... ./pkg/...all pass. Every CLI run used--data-dir/--configagainst a syntheticclaude_home; the real archive was byte-identicalbefore and after.
Out of scope, found along the way
searchConversations(internal/mcp/server.go:615) has the same unclamped negative-slicepanic
getTurnshad.parser.gosetssession.EndedAtfrom the last turn's timestamp rather thanMAX(timestamp), so under the skew documented aboveEndedAtcan precede a turn in thesession.
analytics.TurnRecordis declared, never written, and has no position column — if turnexport is ever wired up,
timestampis the only sequencing key it offers, the one theSQLite side just rejected.
SearchTurnsWithFiltershas no production callers;internal/searchcarries its own copy.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.