fix(session): order messages by (time_created, id) so an id wrap can't wedge a session - #2125
Open
JinyuXiang-Mimo wants to merge 3 commits into
Open
fix(session): order messages by (time_created, id) so an id wrap can't wedge a session#2125JinyuXiang-Mimo wants to merge 3 commits into
JinyuXiang-Mimo wants to merge 3 commits into
Conversation
…t wedge a session Message ids are not a usable clock. `Identifier.create` packs `Date.now() * 0x1000 + counter` into 6 bytes, so the sortable prefix wraps every 2^36 ms (~2.18 years). The last boundary was 1786706395136 (2026-08-14 11:19:55 UTC), which made every post-wrap id (`msg_000…`) sort BEFORE the whole pre-wrap history (`msg_fff…`). Any session with history from before that instant went silently dead: a new user prompt sorted ahead of everything, `lastMainMessageID` returned a days-old message as the tail, the run loop saw no new work and exited at step 0. The message row was stored, no provider call was ever made, and nothing surfaced to the user. Fix the ordering rather than the encoding: `time_created` is an independent integer column, unaffected by the 48-bit packing, so it becomes the primary sort key and the id only breaks ties inside the same millisecond — the exact ambiguity the counter exists to resolve. This needs no migration and unwedges existing sessions immediately. Widening the encoded value is still worth doing for the ~Oct 2028 boundary, but it would NOT restore order on its own: pre-wrap `ff…` ids keep sorting above freshly minted ones. Sites corrected (each verified for intent before changing): - session.ts `lastMainMessageID`: `desc(id)` → `desc(time_created), desc(id)`. This is the one that wedged the loop. - session.ts `fork`: truncation cutoff; an inverted compare forks an empty session or copies all of it. - revert.ts `cleanup`/`revert`: these DELETE the messages they classify as "after" the revert point — inverted, they take the history instead of the tail. - prompt.ts: auto-continue-on-length, the step>1 user-prompt reminder scan, and the fork-agent watermark filter. - history/backfill.ts: `gt(id, cursor)` walk → composite (time_created, id) cursor, otherwise post-wrap parts are never indexed into FTS. `usageRecovered` in prompt.ts is deliberately NOT converted to a compare. `msgs` comes from `filterCompacted`, which walks newest-first and stops at the first checkpoint/compaction marker, so the slice holds at most one marker and it is always msgs[0]. "A marker exists in this slice" already means "this turn was rebuilt". Both orderings that look right there are wrong — a position compare never fires because nothing follows the marker, and a time compare never fires because the marker carries the synthetic time `boundary.time.created + 1`. The old `id >` worked only by accident (a fresh id sorts highest) and stops working across a wrap. Either mistake un-guards the overflow path and rebuilds the same turn twice, which is how the regression showed up in auto-overflow-writer-first.test.ts; the reasoning is now recorded at the call site. Adds test/session/message-id-wrap-order.test.ts, using real ids from an affected session. One case asserts the encoder still wraps, so widening the encoding later fails loudly here instead of leaving this fix looking unmotivated.
…mpare Missed in the previous commit. `classifyAssistantStep` is called from all three runLoop classification sites — the same loop that commit was fixing — and both of its staleness guards still compared raw ids: - #3a text-form tool call (`classify.ts:74`) - XiaomiMiMo#4 stale assistant predating the current user turn (`classify.ts:90`) Semantically identical to the `lastUser.id < lastAssistant.id` check in prompt.ts that was already converted; guard XiaomiMiMo#4's own comment ("Stale assistant predating the current user turn") states the intent is chronological. Across an id wrap a newer assistant carries a smaller id, so XiaomiMiMo#4 judges a live reply stale and re-loops instead of terminating, and #3a skips detection and falls through to the unconditional tool-calls continue at XiaomiMiMo#3, losing the text-tool-call retry. The test fixtures needed changing too, and this is the more interesting half: every `userInfo`/`assistantInfo` hardcoded `time: { created: 0 }` and expressed order purely through ids ("m-1" vs "m-2"). Converting the guards without touching them would have left all 31 cases tied on time and silently falling back to the id tie-break — green, but no longer exercising the ordering they claim to. `created` now mirrors the id's numeric suffix so the fixtures order under compare the way they read. Adds 4 cases using real straddling ids, covering both directions: a fresh post-wrap turn must not be judged stale, a genuinely stale pre-wrap turn must still be, and text-tool-call detection must survive. Verified they fail against the bare-id version and pass against this one. Leaves the two `orderBy(PartTable.id)` sites alone: those order parts within a single message, which are minted in one wrap epoch, so their relative order holds.
Member
Author
|
Good catch — confirmed and fixed in 792cdfb. Both guards were converted:
The fixtures were the interesting halfConverting the guards alone would have looked green and proved nothing. Every Added 4 cases using real straddling ids, covering both directions so the fix can't be a one-way overcorrection:
Verified by reverting RescanRe-swept the package for id-vs-id comparisons and drizzle-level Two
|
…y id The server-side fix was not enough: the TUI keeps its own copy of the message list and has its own ordering, which still keyed on the id string. Two visible symptoms in any session whose history straddles the 2026-08-14T11:19:55Z wrap: 1. New messages rendered at the TOP of the transcript. The store keeps each agent bucket sorted and used Binary.search over the id order to pick the splice index for an incoming message (sync.tsx "message.updated"). A post-wrap id sorts below the whole pre-wrap history, so every new message landed at index 0. 2. History was DELETED. Right after the insert, the >100 trim takes `list[0]` as "oldest", shifts it off and deletes its parts. With new messages arriving at index 0 the trim ate real history instead. Observed on a session created 2026-07-25 whose `message` rows now start 2026-08-14 21:11 — the surviving 8730 pre-wrap rows in history_fts show what was dropped. Binary.search compares id strings and cannot express (time.created, id), so this adds `compareMessages` (client mirror of MessageV2.compare) plus two helpers: - `searchMessages(list, id, probe?)` — bisects on the chronological order for inserts/updates. Callers holding only an id (message.removed) pass no probe and get a linear identity scan: without a time there is nothing to bisect on, and a wrong bisect there splices out an unrelated message. - `compareToMarker(list, msg, markerID)` — for the revert point and the pending watermark, which the session holds as a bare id with no time. It resolves the marker through the list, and returns undefined when the marker is outside the loaded window so callers decide explicitly instead of defaulting it to one end. Sites converted: the message.updated insert/update, message.removed, the initial load (now sorts defensively so the store's invariant comes from its own code rather than from a property of the endpoint), selectMessages' newest-bucket pick, and six revert/queue comparisons in routes/session/index.tsx. `pending` now carries the message instead of its id, which removes a marker lookup entirely. Left alone deliberately: Binary.search on parts (keyed within one message, so one wrap epoch) and on the session/permission lists (sorted and searched by the same id key — self-consistent lookups, not chronological). select-messages.test.ts fixtures gained a `time.created`; without one every bucket ties and the test stops exercising the choice it asserts. New message-order-wrap.test.ts covers both symptoms with real straddling ids, including that repeated appends keep the genuinely oldest message at index 0 — the invariant the trim depends on. Verified 7 of its 12 cases fail against the id-keyed ordering. Full TUI suite: 209 pass.
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
1 participant
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.
Summary
Sessions with history from before 2026-08-14 11:19:55 UTC silently stop responding: a new prompt is stored, no provider call is ever made, and nothing surfaces to the user.
Identifier.createpacksDate.now() * 0x1000 + counterinto 6 bytes, so the sortable id prefix wraps every2^36 ms(~2.18 years). The last boundary was1786706395136, which makes every post-wrap id (msg_000…) sort before the entire pre-wrap history (msg_fff…).Session.lastMainMessageIDthen returns a days-old message as the tail, the run loop sees no new work, and exits at step 0.Observed on a real session — three user messages stored on Aug 14, zero assistant replies, last processed turn Aug 06:
Approach
Fix the ordering, not the encoding.
time_createdis an independent integer column, unaffected by the 48-bit packing, so it becomes the primary sort key; the id only breaks ties within the same millisecond — exactly the ambiguity the counter exists to resolve.This needs no migration and unwedges existing sessions immediately. Widening the encoded value is still worth doing for the ~Oct 2028 boundary, but it would not restore order on its own: pre-wrap
ff…ids keep sorting above freshly minted ones. A follow-up can widen the encoding; this PR makes correctness independent of it.Adds
MessageV2.compare(a, b)as the single chronological comparator and routes the affected call sites through it.Sites corrected
Each was checked for intent before changing — several encode "later in the stored sequence" rather than "later in time".
session.tslastMainMessageIDdesc(id)→desc(time_created), desc(id)session.tsforkrevert.tscleanup/revertprompt.ts×3step > 1user-prompt reminder scan, the fork-agent watermark filterhistory/backfill.tsgt(id, cursor)walk → composite(time_created, id)cursor; otherwise post-wrap parts are never indexed into FTSOne site deliberately left alone
usageRecoveredinprompt.tsis not converted to a compare.msgscomes fromfilterCompacted, which walks newest-first and stops at the first checkpoint/compaction marker — so the slice holds at most one marker and it is alwaysmsgs[0]. "A marker exists in this slice" already means "this turn was rebuilt".Both orderings that look correct there are wrong: a position compare never fires (nothing follows the marker), and a time compare never fires either (the marker carries the synthetic time
boundary.time.created + 1, so it sorts earlier thanlastFinished). The oldid >worked only by accident — a freshly minted id sorts highest — and stops working across a wrap.Getting this wrong un-guards the overflow path and rebuilds the same turn twice; that is how it surfaced in
auto-overflow-writer-first.test.tsduring development. The reasoning is now recorded at the call site so the next reader doesn't "fix" it.Testing
bun typecheck— clean (all 12 packages)test/session/message-id-wrap-order.test.ts— 7 cases, using real ids from an affected session. One case asserts the encoder still wraps, so a later widening fails loudly here rather than leaving this fix looking unmotivated.test/session/— 917 pass. 4 failures are unrelated: 2 fail identically on a clean baseline (checkpoint-child-sessionT10,prune.test.tsrecovery gate), and 2 are full-run timeout flakes (a beforeEach/afterEach hook timed out) that pass 66/66 in isolation on both baseline and this branch. Before this change the same suite had 7 failures.revert-compact,fork-prefix-invariant,checkpoint-fork-mode,main-runloop-history-invariant, and all oftest/history/— 52 pass, 0 fail.Note
The next wrap is ~Oct 2028. Two follow-ups worth considering: widen the encoded value, and assert the invariant (
new user message id < session max id) so "silently ignored prompt" stops being a possible failure mode at all — that, more than the ordering bug, is what made this expensive to diagnose.