From a3ae1b377db2717103b3b2b1ea78dbfb3f2e3079 Mon Sep 17 00:00:00 2001 From: Jinyu Xiang Date: Sat, 15 Aug 2026 01:36:06 +0800 Subject: [PATCH 1/3] fix(session): order messages by (time_created, id) so an id wrap can't wedge a session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/opencode/src/history/backfill.ts | 23 ++++-- packages/opencode/src/session/message-v2.ts | 21 ++++++ packages/opencode/src/session/prompt.ts | 38 +++++++--- packages/opencode/src/session/revert.ts | 16 +++-- packages/opencode/src/session/session.ts | 14 +++- .../session/message-id-wrap-order.test.ts | 72 +++++++++++++++++++ 6 files changed, 165 insertions(+), 19 deletions(-) create mode 100644 packages/opencode/test/session/message-id-wrap-order.test.ts diff --git a/packages/opencode/src/history/backfill.ts b/packages/opencode/src/history/backfill.ts index f7314ece1..1afc0ac5b 100644 --- a/packages/opencode/src/history/backfill.ts +++ b/packages/opencode/src/history/backfill.ts @@ -1,5 +1,5 @@ import { Context, Effect, Layer } from "effect" -import { and, asc, desc, eq, gt, sql } from "drizzle-orm" +import { and, asc, desc, eq, gt, or, sql } from "drizzle-orm" import { Database } from "../storage" import { Config } from "../config" import { PartTable, SessionTable } from "../session/session.sql" @@ -49,7 +49,12 @@ function scanSession( enabled: ReadonlySet, ) { return Effect.gen(function* () { - let cursor = "" + // Composite (time_created, id) cursor. Part ids share the message-id clock, + // which wraps every ~2.18 years (see MessageV2.compare), so a bare + // `gt(id, cursor)` walk stops dead at a wrap boundary: every post-wrap part + // has a SMALLER id than the pre-wrap tail and would never be visited, leaving + // the newest history permanently unindexed. + let cursor: { time: number; id: string } | undefined while (true) { const parts = Database.use((db) => db @@ -58,18 +63,26 @@ function scanSession( .where( and( eq(PartTable.session_id, session.id as any), - gt(PartTable.id, cursor as any), + ...(cursor + ? [ + or( + gt(PartTable.time_created, cursor.time), + and(eq(PartTable.time_created, cursor.time), gt(PartTable.id, cursor.id as any)), + )!, + ] + : []), sql`NOT EXISTS (SELECT 1 FROM history_fts WHERE history_fts.part_id = ${PartTable.id})`, ), ) - .orderBy(asc(PartTable.id)) + .orderBy(asc(PartTable.time_created), asc(PartTable.id)) .limit(BATCH) .all(), ) if (parts.length === 0) return yield* writeBatch(parts, session.project_id, resolver, enabled) - cursor = parts[parts.length - 1]!.id + const last = parts[parts.length - 1]! + cursor = { time: last.time_created, id: last.id } yield* Effect.sleep("10 millis") } }) diff --git a/packages/opencode/src/session/message-v2.ts b/packages/opencode/src/session/message-v2.ts index 949bae61f..578e6f7f0 100644 --- a/packages/opencode/src/session/message-v2.ts +++ b/packages/opencode/src/session/message-v2.ts @@ -607,6 +607,27 @@ const part = (row: typeof PartTable.$inferSelect) => const older = (row: Cursor) => or(lt(MessageTable.time_created, row.time), and(eq(MessageTable.time_created, row.time), lt(MessageTable.id, row.id))) +/** + * Chronological order of two messages: `< 0` if a precedes b, `> 0` if a follows + * b, `0` if they are the same message. + * + * 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 wrap was 2026-08-14 12:39:55 UTC and the + * next is ~Oct 2028. Across a wrap, a NEWER message gets a SMALLER id — post-wrap + * ids restart near `msg_000…` while pre-wrap ids sit near `msg_fff…`. Any bare + * `a.id > b.id` therefore inverts for every session whose history straddles a + * boundary, which silently wedged the session loop (a fresh user prompt sorted + * before all history, so the loop saw no new work and exited at step 0). + * + * `time.created` is an independent integer column and is not affected, so it is + * the primary key of the order; the id only breaks ties inside the same + * millisecond, which is exactly the ambiguity the counter was added to resolve. + */ +export function compare(a: { id: string; time: { created: number } }, b: { id: string; time: { created: number } }) { + return a.time.created !== b.time.created ? a.time.created - b.time.created : a.id < b.id ? -1 : a.id > b.id ? 1 : 0 +} + function hydrate(rows: (typeof MessageTable.$inferSelect)[]) { const ids = rows.map((row) => row.id) const partByMessage = new Map() diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index 1696ecb37..578803f63 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -3283,13 +3283,27 @@ NOTE: At any point in time through this workflow you should feel free to ask the } if (!lastUser) throw new Error("No user message found in stream. This should never happen.") + // Deliberately NOT MessageV2.compare, and not a raw id compare either. + // + // `msgs` comes from filterCompacted (message-v2.ts:1077), which walks + // newest-first and STOPS at the first checkpoint/compaction marker, then + // reverses. So the slice contains at most one marker and it is always + // msgs[0] — the boundary the slice starts at. "A marker exists in this + // slice" is therefore exactly "this turn's context was already rebuilt", + // with no ordering question to get wrong. + // + // The two orderings that look plausible here are both wrong: a position + // compare against lastFinished never fires (the marker precedes every + // message in the slice, so nothing follows it), and a time compare never + // fires either, because the marker carries the SYNTHETIC time + // `boundary.time.created + 1` (checkpoint.ts:1599) and so sorts earlier + // than lastFinished rather than later. The original `id > lastFinished.id` + // only worked by accident — a freshly minted marker id happens to sort + // highest, which stops being true across an id wrap (see MessageV2.compare). + // Either mistake un-guards the overflow path and rebuilds the turn twice. const usageRecovered = !!lastFinished && - msgs.some( - (msg) => - msg.info.id > lastFinished.id && - msg.parts.some((part) => part.type === "checkpoint" || part.type === "compaction"), - ) + msgs.some((msg) => msg.parts.some((part) => part.type === "checkpoint" || part.type === "compaction")) // Per-user-message active recall reminder. Once the session has // any memory artifacts (memory dir populated OR tasks recorded), @@ -3340,7 +3354,7 @@ NOTE: At any point in time through this workflow you should feel free to ask the if ( lastAssistant?.finish === "length" && !hasToolCalls && - lastUser.id < lastAssistant.id && + MessageV2.compare(lastUser, lastAssistant) < 0 && (yield* autoContinueOutputLength({ lastUser, assistant: lastAssistant })) ) { continue @@ -3720,7 +3734,7 @@ NOTE: At any point in time through this workflow you should feel free to ask the if (step > 1 && lastFinished) { for (const m of msgs) { - if (m.info.role !== "user" || m.info.id <= lastFinished.id) continue + if (m.info.role !== "user" || MessageV2.compare(m.info, lastFinished) <= 0) continue for (const p of m.parts) { if (p.type !== "text" || p.ignored || p.synthetic) continue if (!p.text.trim()) continue @@ -3774,8 +3788,16 @@ NOTE: At any point in time through this workflow you should feel free to ask the .pipe(Effect.ignore) return "break" as const } + // Resolve the watermark to its message so the comparison can use + // (time.created, id) — a bare id compare inverts across an id wrap + // (see MessageV2.compare). If the watermark message is no longer in + // the list, fall back to the agent_id check alone, which the + // ForkContext.watermarkMsgID JSDoc documents as sufficient on its own. + const watermarkMsg = msgs.find((m) => m.info.id === forkCtx.watermarkMsgID)?.info const ownNew = msgs.filter( - (m) => m.info.id > forkCtx.watermarkMsgID && m.info.agentID === lastUser.agentID, + (m) => + m.info.agentID === lastUser.agentID && + (!watermarkMsg || MessageV2.compare(m.info, watermarkMsg) > 0), ) const ownNewModelMsgs = yield* MessageV2.toModelMessagesEffect(ownNew, model) const prebuiltSystem = forkCtx.system diff --git a/packages/opencode/src/session/revert.ts b/packages/opencode/src/session/revert.ts index 0387e213a..c1dae7cf8 100644 --- a/packages/opencode/src/session/revert.ts +++ b/packages/opencode/src/session/revert.ts @@ -74,7 +74,8 @@ export const layer = Layer.effect( if (session.revert?.snapshot) yield* snap.restore(session.revert.snapshot) yield* snap.revert(patches) if (rev.snapshot) rev.diff = yield* snap.diff(rev.snapshot as string) - const range = all.filter((msg) => msg.info.id >= rev!.messageID) + const revertPoint = all.find((msg) => msg.info.id === rev!.messageID)?.info + const range = revertPoint ? all.filter((msg) => MessageV2.compare(msg.info, revertPoint) >= 0) : [] const diffs = yield* summary.computeDiff({ messages: range }) yield* storage.write(["session_diff", input.sessionID], diffs).pipe(Effect.ignore) yield* bus.publish(Session.Event.Diff, { sessionID: input.sessionID, diff: diffs }) @@ -107,9 +108,16 @@ export const layer = Layer.effect( const messageID = session.revert.messageID const remove = [] as MessageV2.WithParts[] let target: MessageV2.WithParts | undefined - for (const msg of msgs) { - if (msg.info.id < messageID) continue - if (msg.info.id > messageID) { + // Order by (time.created, id): a bare id compare inverts across an id wrap + // (see MessageV2.compare), and this loop DELETES everything it classifies as + // "after" the revert point — an inverted compare would delete the session's + // history instead of the reverted tail. If the revert point is missing from + // the list there is nothing to anchor on, so remove nothing. + const anchor = msgs.find((msg) => msg.info.id === messageID)?.info + for (const msg of anchor ? msgs : []) { + const rel = MessageV2.compare(msg.info, anchor!) + if (rel < 0) continue + if (rel > 0) { remove.push(msg) continue } diff --git a/packages/opencode/src/session/session.ts b/packages/opencode/src/session/session.ts index 319436b9d..ecd6d2a05 100644 --- a/packages/opencode/src/session/session.ts +++ b/packages/opencode/src/session/session.ts @@ -664,9 +664,15 @@ export const layer: Layer.Layer() + // Fork cuts the history at input.messageID. Compare on (time.created, id) — + // a bare id compare inverts across an id wrap (see MessageV2.compare), which + // would either truncate at message 0 (empty fork) or never trigger (full + // copy). `messages` already returns chronological order, so the break is + // still a single forward scan. + const cutoff = input.messageID ? msgs.find((m) => m.info.id === input.messageID)?.info : undefined for (const msg of msgs) { - if (input.messageID && msg.info.id >= input.messageID) break + if (cutoff && MessageV2.compare(msg.info, cutoff) >= 0) break const newID = MessageID.ascending() idMap.set(msg.info.id, newID) @@ -811,7 +817,11 @@ export const layer: Layer.Layer ({ id, time: { created } }) + +describe("message id wraparound", () => { + test("encoder really does wrap at the 2^36 ms boundary", () => { + // Not asserting the bug is fixed in the encoder — asserting it EXISTS, so + // that widening the encoding later fails this test loudly rather than + // leaving the ordering fix looking pointless. + const before = Identifier.create("msg", "ascending", WRAP - 1) + const after = Identifier.create("msg", "ascending", WRAP + 1) + expect(before.slice(4, 16) > after.slice(4, 16)).toBe(true) + expect(after.slice(4, 8)).toBe("0000") + }) + + test("wrap period is ~2.18 years, next boundary ~Oct 2028", () => { + expect(PERIOD).toBe(68719476736) + expect(new Date(WRAP).toISOString()).toBe("2026-08-14T11:19:55.136Z") + expect(new Date(WRAP + PERIOD).getUTCFullYear()).toBe(2028) + }) + + test("compare orders across a wrap where bare id compare inverts", () => { + // Real ids: pre-wrap tail and post-wrap prompt from the wedged session. + const pre = msg("msg_fd708d21e001JXYNUE1Jba3VEw", 1786019107358) // Aug 06 + const post = msg("msg_0006f768700114fd6bDaDwzOWs", 1786713700254) // Aug 14 + + // The bug: lexicographic id order claims the Aug-14 message is older. + expect(post.id < pre.id).toBe(true) + // The fix: chronological order is correct. + expect(MessageV2.compare(pre, post)).toBeLessThan(0) + expect(MessageV2.compare(post, pre)).toBeGreaterThan(0) + }) + + test("compare handles the upstream-reported id pair too", () => { + const pre = msg("msg_ffd4c8ecc001nKKJtMDUGfajRL", 1786700000000) + const post = msg("msg_000bcb6970013Jm92xkWhlZiOA", 1786718762656) + expect(post.id < pre.id).toBe(true) + expect(MessageV2.compare(pre, post)).toBeLessThan(0) + }) + + test("id breaks ties within the same millisecond", () => { + const a = msg("msg_000aaa", 1786713700254) + const b = msg("msg_000bbb", 1786713700254) + expect(MessageV2.compare(a, b)).toBeLessThan(0) + expect(MessageV2.compare(b, a)).toBeGreaterThan(0) + }) + + test("compare is 0 only for the same message", () => { + const a = msg("msg_000aaa", 1786713700254) + expect(MessageV2.compare(a, { ...a })).toBe(0) + }) + + test("sorting a straddling session yields chronological order", () => { + const msgs = [ + msg("msg_0006f7687001", 1786713700254), // Aug 14, post-wrap + msg("msg_f4c12734b001", 1783687705445), // Jul 10, pre-wrap + msg("msg_fd708d21e001", 1786019107358), // Aug 06, pre-wrap + ] + expect([...msgs].sort(MessageV2.compare).map((m) => m.time.created)).toEqual([ + 1783687705445, 1786019107358, 1786713700254, + ]) + // Bare id sort puts the newest message first — the wedging behaviour. + expect([...msgs].sort((a, b) => (a.id < b.id ? -1 : 1))[0]!.time.created).toBe(1786713700254) + }) +}) From 792cdfba7d58d5994f88537914ec8ca95f5a0511 Mon Sep 17 00:00:00 2001 From: Jinyu Xiang Date: Sat, 15 Aug 2026 01:52:23 +0800 Subject: [PATCH 2/3] fix(session): route classify.ts staleness guards through MessageV2.compare MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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`) - #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 #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 #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 #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. --- packages/opencode/src/session/classify.ts | 10 ++- .../opencode/test/session/classify.test.ts | 67 +++++++++++++++++-- 2 files changed, 71 insertions(+), 6 deletions(-) diff --git a/packages/opencode/src/session/classify.ts b/packages/opencode/src/session/classify.ts index 48d251ba8..b7ae8e1c1 100644 --- a/packages/opencode/src/session/classify.ts +++ b/packages/opencode/src/session/classify.ts @@ -71,7 +71,7 @@ export function classifyAssistantStep(input: { if ( assistant.finish === "tool-calls" && !assistant.error && - input.lastUser.id < assistant.id && + MessageV2.compare(input.lastUser, assistant) < 0 && !input.parts.some((part) => part.type === "tool") && input.parts.some( (part) => @@ -87,7 +87,13 @@ export function classifyAssistantStep(input: { if (assistant.finish === "tool-calls") return { type: "continue" } // 4. Stale assistant predating the current user turn — don't terminate on it. - if (input.phase === "existing-assistant" && !(input.lastUser.id < assistant.id)) + // Ordered by (time.created, id) via MessageV2.compare, not by raw id: message + // ids encode a 48-bit timestamp that wraps every ~2.18 years, so across a wrap + // a NEWER assistant gets a SMALLER id and a bare compare calls a fresh reply + // "stale" (or a genuinely stale one fresh). Both misjudgements are load-bearing + // here and at #3a — this is the same guard prompt.ts applies before + // auto-continue, and it must agree with it. + if (input.phase === "existing-assistant" && !(MessageV2.compare(input.lastUser, assistant) < 0)) return { type: "continue" } // 5. Errored step — checked before content so an errored message that also diff --git a/packages/opencode/test/session/classify.test.ts b/packages/opencode/test/session/classify.test.ts index fd8207836..a3cfde24e 100644 --- a/packages/opencode/test/session/classify.test.ts +++ b/packages/opencode/test/session/classify.test.ts @@ -6,12 +6,22 @@ import { ProviderID, ModelID } from "../../src/provider/schema" const sessionID = SessionID.make("session") +// `created` mirrors the numeric suffix of `id` so fixtures order the same way +// under MessageV2.compare (time first, id only as a same-millisecond tie-break) +// as they read on the page. Leaving every fixture at created: 0 would collapse +// the suite onto the id tie-break and stop exercising the time ordering the +// staleness guards at classify.ts #3a/#4 actually use. +function seq(id: string) { + const n = Number(id.replace(/^\D*/, "")) + return Number.isNaN(n) ? 0 : n +} + function userInfo(id: string): MessageV2.User { return { id: MessageID.make(id), sessionID, role: "user", - time: { created: 0 }, + time: { created: seq(id) }, agent: "user", model: { providerID: ProviderID.make("test"), modelID: ModelID.make("test") }, tools: {}, @@ -27,7 +37,7 @@ function assistantInfo( id: MessageID.make(id), sessionID, role: "assistant", - time: { created: 0 }, + time: { created: seq(id) }, parentID: MessageID.make("m-parent"), modelID: "test", providerID: "test", @@ -61,7 +71,7 @@ function toolPart(messageID: string, opts?: { providerExecuted?: boolean }) { } as unknown as MessageV2.Part } -// User "m-1" precedes assistant "m-2" so the stale guard (lastUser.id < assistant.id) is satisfied. +// User "m-1" precedes assistant "m-2" so the stale guard (compare(lastUser, assistant) < 0) is satisfied. const lastUser = userInfo("m-1") describe("classifyAssistantStep", () => { @@ -274,7 +284,7 @@ describe("classifyAssistantStep", () => { ).toBe("invalid") }) - test("existing-assistant phase + stale assistant (lastUser.id >= assistant.id) => continue", () => { + test("existing-assistant phase + stale assistant (assistant predates current user) => continue", () => { // user "m-2" comes after assistant "m-1": assistant predates the current turn. expect( classifyAssistantStep({ @@ -409,4 +419,53 @@ describe("classifyAssistantStep", () => { expect(result.type).toBe("continue") }) }) + + // Message ids encode a 48-bit timestamp that wraps every ~2.18 years (last + // boundary 2026-08-14T11:19:55Z), so across a wrap a NEWER message carries a + // SMALLER id. Both staleness guards (#3a and #4) must follow time, not the id, + // or a live turn is judged stale and vice versa. Ids below are real ones from + // a session that straddled the boundary. + describe("staleness guards across an id wraparound", () => { + const preWrap = "msg_fd708d21e001JXYNUE1Jba3VEw" // 2026-08-06 + const postWrap = "msg_0006f768700114fd6bDaDwzOWs" // 2026-08-14 + const withTime = (info: T, created: number) => + ({ ...info, time: { created } }) as T + + test("id order is inverted for this pair — the premise of the cases below", () => { + expect(postWrap < preWrap).toBe(true) + }) + + test("fresh post-wrap assistant is not judged stale", () => { + const result = classifyAssistantStep({ + phase: "existing-assistant", + lastUser: withTime(userInfo(preWrap), 1786019107358), + assistant: withTime(assistantInfo(postWrap, { finish: "stop" }), 1786713700254), + parts: [textPart(postWrap, "fresh answer")], + }) + // A raw id compare reads postWrap < preWrap and returns "continue" here. + expect(result).toEqual({ type: "final" }) + }) + + test("genuinely stale pre-wrap assistant is still judged stale", () => { + const result = classifyAssistantStep({ + phase: "existing-assistant", + lastUser: withTime(userInfo(postWrap), 1786713700254), + assistant: withTime(assistantInfo(preWrap, { finish: "stop" }), 1786019107358), + parts: [textPart(preWrap, "old answer")], + }) + expect(result).toEqual({ type: "continue" }) + }) + + test("text-form tool call in a fresh post-wrap turn is still detected", () => { + const result = classifyAssistantStep({ + phase: "existing-assistant", + lastUser: withTime(userInfo(preWrap), 1786019107358), + assistant: withTime(assistantInfo(postWrap, { finish: "tool-calls" }), 1786713700254), + parts: [textPart(postWrap, 'ls')], + }) + // A raw id compare skips #3a and falls through to the unconditional + // tool-calls continue at #3, losing the text-tool-call retry. + expect(result.type).toBe("text-tool-call") + }) + }) }) From 1110eded72a9ae01172417b4edf84c8e85f6c402 Mon Sep 17 00:00:00 2001 From: Jinyu Xiang Date: Sat, 15 Aug 2026 02:43:51 +0800 Subject: [PATCH 3/3] fix(tui): order the client message store by (time.created, id), not by id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../opencode/src/cli/cmd/tui/context/sync.tsx | 102 ++++++++++++++++- .../src/cli/cmd/tui/routes/session/index.tsx | 52 +++++++-- .../test/cli/tui/message-order-wrap.test.ts | 104 ++++++++++++++++++ .../test/cli/tui/select-messages.test.ts | 7 +- 4 files changed, 247 insertions(+), 18 deletions(-) create mode 100644 packages/opencode/test/cli/tui/message-order-wrap.test.ts diff --git a/packages/opencode/src/cli/cmd/tui/context/sync.tsx b/packages/opencode/src/cli/cmd/tui/context/sync.tsx index a24660d5c..d4699d3a1 100644 --- a/packages/opencode/src/cli/cmd/tui/context/sync.tsx +++ b/packages/opencode/src/cli/cmd/tui/context/sync.tsx @@ -157,6 +157,85 @@ export function bucketMessages( return out } +/** + * Client-side mirror of MessageV2.compare — chronological order of two messages. + * + * Message ids are NOT a clock. The id encoder packs `Date.now() * 0x1000 + counter` + * into 6 bytes, so the sortable prefix wraps every 2^36 ms (~2.18 years); the last + * boundary was 2026-08-14T11:19:55Z. Across a wrap a NEWER message gets a SMALLER + * id, so `a.id < b.id` inverts for any session whose history straddles it. + * + * This matters more here than it looks: the store keeps each bucket sorted and uses + * Binary.search over that order both to locate an existing message and to pick the + * splice index for a new one. With ids as the key, a fresh message lands at index 0 + * — which is what put new messages at the top of the transcript — and the >100 + * trim then treats `list[0]` as "oldest" and deletes it along with its parts. + * + * `time.created` is an independent field, unaffected by the packing, so it is the + * primary key; the id only breaks ties inside the same millisecond. + */ +export function compareMessages( + a: { id: string; time: { created: number } }, + b: { id: string; time: { created: number } }, +) { + return a.time.created !== b.time.created ? a.time.created - b.time.created : a.id < b.id ? -1 : a.id > b.id ? 1 : 0 +} + +/** + * Compare a message against a MARKER id (revert point, pending watermark) held by + * the session rather than by a message object. + * + * Returns `< 0` if `msg` precedes the marker, `> 0` if it follows, `0` if it IS the + * marker. When the marker is not present in `list` there is no anchor to compare + * against and the caller gets `undefined` — callers decide what that means rather + * than silently treating the marker as position 0 or infinity. + * + * Resolving the marker through the list is what makes this wrap-safe: the marker id + * alone carries no time, and comparing id strings inverts across a wrap boundary + * (see compareMessages). + */ +export function compareToMarker( + list: M[], + msg: M, + markerID: string, +): number | undefined { + const marker = list.find((m) => m.id === markerID) + if (!marker) return undefined + return compareMessages(msg, marker) +} + +/** + * Locate `id` in a chronologically-sorted message list, or the index it should be + * inserted at. Same contract as Binary.search, but keyed on (time.created, id) — + * Binary.search compares id strings only and cannot express this order. + * + * `probe` is the incoming message when inserting. Lookups that only have an id + * (message.removed) pass none and fall back to a linear scan: without a time there + * is nothing to bisect on, and a wrong bisect here would splice out an unrelated + * message. + */ +export function searchMessages( + list: M[], + id: string, + probe?: { id: string; time: { created: number } }, +): { found: boolean; index: number } { + if (!probe) { + const index = list.findIndex((m) => m.id === id) + return index === -1 ? { found: false, index: list.length } : { found: true, index } + } + let left = 0 + let right = list.length + while (left < right) { + const mid = Math.floor((left + right) / 2) + if (compareMessages(list[mid], probe) < 0) left = mid + 1 + else right = mid + } + // `left` is the first element not ordered before probe — an exact id match can + // only be there, since compare is a total order and probe carries its own id. + if (left < list.length && list[left].id === id) return { found: true, index: left } + return { found: false, index: left } +} + /** * A `session.status` event is authoritative for the WHOLE status object. * @@ -193,7 +272,7 @@ export function nextSessionStatus(status: SessionStatus) { // read-only local-DB snapshot and they drift — this arm's population grew // 1294 → 1313 across this branch's own revisions — so trust the split's shape, // not the absolute numbers. -export function selectMessages( +export function selectMessages( buckets: Record | undefined, agentID: string, sessionID: string, @@ -202,7 +281,10 @@ export function selectMessages( if (buckets?.[sessionID]?.length) return buckets[sessionID] const newest = Object.entries(buckets ?? {}) .filter(([key, msgs]) => key !== "main" && msgs.length > 0) - .sort(([, a], [, b]) => (b.at(-1)?.id ?? "").localeCompare(a.at(-1)?.id ?? "")) + // "Newest bucket" by its last message's time, not its id — an id compare + // inverts across an id wrap (see compareMessages) and would pick the bucket + // that happens to hold a pre-wrap tail. + .sort(([, a], [, b]) => (b.at(-1)?.time.created ?? 0) - (a.at(-1)?.time.created ?? 0)) .at(0) return newest?.[1] ?? [] } @@ -567,7 +649,7 @@ export const { use: useSync, provider: SyncProvider } = createSimpleContext({ break } const messages = store.message[sid][aid] - const result = Binary.search(messages, event.properties.info.id, (m) => m.id) + const result = searchMessages(messages, event.properties.info.id, event.properties.info) if (result.found) { setStore("message", sid, aid, result.index, reconcile(event.properties.info)) break @@ -608,7 +690,7 @@ export const { use: useSync, provider: SyncProvider } = createSimpleContext({ if (!buckets) break for (const aid of Object.keys(buckets)) { const messages = buckets[aid] - const result = Binary.search(messages, event.properties.messageID, (m) => m.id) + const result = searchMessages(messages, event.properties.messageID) if (result.found) { setStore( "message", @@ -1005,8 +1087,16 @@ export const { use: useSync, provider: SyncProvider } = createSimpleContext({ draft.todo[sessionID] = todo.data ?? [] draft.task[sessionID] = task.data ?? [] const flat = (messages.data ?? []).map((x) => x.info) - // Server returns messages id-ordered and message.updated keeps that order; the footer's post-/rebuild pending-detection deliberately does NOT depend on it (it keys off checkpoint coveredUpTo, model.ts), so reordering here won't resurface the stale-context bug. - draft.message[sessionID] = bucketMessages(flat) + // Server returns messages in (time_created, id) order and + // message.updated keeps that order via searchMessages. Sorting here + // too is belt-and-braces: it makes the store's invariant hold from + // its own code rather than from a property of the endpoint, so a + // paging/ordering change server-side can't silently corrupt the + // splice indices the trim and message.removed both rely on. The + // footer's post-/rebuild pending-detection deliberately does NOT + // depend on this order (it keys off checkpoint coveredUpTo, model.ts), + // so ordering here won't resurface the stale-context bug. + draft.message[sessionID] = bucketMessages(flat.toSorted(compareMessages)) for (const message of messages.data ?? []) { draft.part[message.info.id] = message.parts } diff --git a/packages/opencode/src/cli/cmd/tui/routes/session/index.tsx b/packages/opencode/src/cli/cmd/tui/routes/session/index.tsx index 788132fc2..bdfeeb918 100644 --- a/packages/opencode/src/cli/cmd/tui/routes/session/index.tsx +++ b/packages/opencode/src/cli/cmd/tui/routes/session/index.tsx @@ -15,7 +15,7 @@ import { Dynamic } from "solid-js/web" import path from "path" import { useCurrentAgentID, useRoute, useRouteData } from "@tui/context/route" import { useProject } from "@tui/context/project" -import { selectMessages, useSync } from "@tui/context/sync" +import { compareMessages, compareToMarker, selectMessages, useSync } from "@tui/context/sync" import { useEvent } from "@tui/context/event" import { SplitBorder } from "@tui/component/border" import { Spinner } from "@tui/component/spinner" @@ -188,8 +188,11 @@ export function Session() { ) const disabled = createMemo(() => permissions().length > 0 || questions().length > 0) + // The in-flight assistant message itself, not just its id: `queued` compares + // against it chronologically (see compareMessages) and an id compare inverts + // across an id wrap, which would mark settled messages as queued. const pending = createMemo(() => { - return messages().findLast((x) => x.role === "assistant" && !x.time.completed)?.id + return messages().findLast((x) => x.role === "assistant" && !x.time.completed) }) const lastAssistant = createMemo(() => { @@ -733,7 +736,17 @@ export function Session() { const status = sync.data.session_status?.[route.sessionID] if (status?.type !== "idle") await sdk.client.session.abort({ sessionID: route.sessionID }).catch(() => {}) const revert = session()?.revert?.messageID - const message = messages().findLast((x) => (!revert || x.id < revert) && x.role === "user") + const all = messages() + // Ordered against the revert marker via compareToMarker, not by raw id: an + // id compare inverts across an id wrap and would undo the wrong turn. + // An unresolvable marker (outside the loaded window) yields undefined, which + // excludes the message rather than defaulting it into the "before" side. + const before = (x: (typeof all)[number]) => { + if (!revert) return true + const rel = compareToMarker(all, x, revert) + return rel !== undefined && rel < 0 + } + const message = all.findLast((x) => x.role === "user" && before(x)) if (!message) return void sdk.client.session .revert({ @@ -772,7 +785,13 @@ export function Session() { dialog.clear() const messageID = session()?.revert?.messageID if (!messageID) return - const message = messages().find((x) => x.role === "user" && x.id > messageID) + const all = messages() + // Ordered via compareToMarker, not by raw id (inverts across an id wrap). + const message = all.find((x) => { + if (x.role !== "user") return false + const rel = compareToMarker(all, x, messageID) + return rel !== undefined && rel > 0 + }) if (!message) { void sdk.client.session.unrevert({ sessionID: route.sessionID, @@ -1019,9 +1038,14 @@ export function Session() { category: "session", onSelect: (dialog) => { const revertID = session()?.revert?.messageID - const lastAssistantMessage = messages().findLast( - (msg) => msg.role === "assistant" && (!revertID || msg.id < revertID), - ) + const all = messages() + // Ordered via compareToMarker, not by raw id (inverts across an id wrap). + const lastAssistantMessage = all.findLast((msg) => { + if (msg.role !== "assistant") return false + if (!revertID) return true + const rel = compareToMarker(all, msg, revertID) + return rel !== undefined && rel < 0 + }) if (!lastAssistantMessage) { toast.show({ message: "No assistant messages found", variant: "error" }) dialog.clear() @@ -1225,7 +1249,13 @@ export function Session() { const revertRevertedMessages = createMemo(() => { const messageID = revertMessageID() if (!messageID) return [] - return messages().filter((x) => x.id >= messageID && x.role === "user") + const all = messages() + // Ordered via compareToMarker, not by raw id (inverts across an id wrap). + return all.filter((x) => { + if (x.role !== "user") return false + const rel = compareToMarker(all, x, messageID) + return rel !== undefined && rel >= 0 + }) }) const revert = createMemo(() => { @@ -1405,7 +1435,7 @@ export function Session() { ) })()} - = revert()!.messageID}> + = 0}> <> @@ -1521,7 +1551,7 @@ function UserMessage(props: { parts: Part[] onMouseUp: () => void index: number - pending?: string + pending?: { id: string; time: { created: number } } }) { const ctx = use() const local = useLocal() @@ -1562,7 +1592,7 @@ function UserMessage(props: { const { theme } = useTheme() const t = useLanguage().t const [hover, setHover] = createSignal(false) - const queued = createMemo(() => props.pending && props.message.id > props.pending) + const queued = createMemo(() => !!props.pending && compareMessages(props.message, props.pending) > 0) const color = createMemo(() => local.agent.color(props.message.agent)) const queuedFg = createMemo(() => selectedForeground(theme, color())) const metadataVisible = createMemo(() => queued() || ctx.showTimestamps()) diff --git a/packages/opencode/test/cli/tui/message-order-wrap.test.ts b/packages/opencode/test/cli/tui/message-order-wrap.test.ts new file mode 100644 index 000000000..9d0b04126 --- /dev/null +++ b/packages/opencode/test/cli/tui/message-order-wrap.test.ts @@ -0,0 +1,104 @@ +import { describe, test, expect } from "bun:test" +import { bucketMessages, compareMessages, compareToMarker, searchMessages, selectMessages } from "../../../src/cli/cmd/tui/context/sync" + +// Real ids from a session whose history straddled the 2026-08-14T11:19:55Z id wrap. +// Post-wrap ids restart near msg_000… while pre-wrap ids sit near msg_fff…, so a +// NEWER message sorts BEFORE the whole history under a raw id compare. +const JUL_10 = { id: "msg_f4c12734b001QCMWgJEWl8E2J3", time: { created: 1783687705445 } } +const AUG_06 = { id: "msg_fd708d21e001JXYNUE1Jba3VEw", time: { created: 1786019107358 } } +const AUG_14 = { id: "msg_0006f768700114fd6bDaDwzOWs", time: { created: 1786713700254 } } + +const m = (x: typeof JUL_10, extra?: Record) => ({ ...x, ...extra }) as any + +describe("TUI message ordering across an id wraparound", () => { + test("the premise: id order is inverted for these ids", () => { + expect(AUG_14.id < AUG_06.id).toBe(true) + expect(AUG_14.id < JUL_10.id).toBe(true) + }) + + test("compareMessages orders them chronologically", () => { + expect(compareMessages(JUL_10, AUG_06)).toBeLessThan(0) + expect(compareMessages(AUG_06, AUG_14)).toBeLessThan(0) + expect(compareMessages(AUG_14, JUL_10)).toBeGreaterThan(0) + expect([AUG_14, JUL_10, AUG_06].toSorted(compareMessages).map((x) => x.time.created)).toEqual([ + 1783687705445, 1786019107358, 1786713700254, + ]) + }) + + test("id still breaks ties inside one millisecond", () => { + const a = { id: "msg_000aaa", time: { created: 5 } } + const b = { id: "msg_000bbb", time: { created: 5 } } + expect(compareMessages(a, b)).toBeLessThan(0) + expect(compareMessages(b, a)).toBeGreaterThan(0) + expect(compareMessages(a, { ...a })).toBe(0) + }) + + // The bug that put new messages at the top of the transcript: the store keeps + // each bucket sorted and asks searchMessages where an incoming message goes. + describe("searchMessages picks the append index, not index 0", () => { + const history = [m(JUL_10), m(AUG_06)] + + test("a post-wrap message appends to the end", () => { + expect(searchMessages(history, AUG_14.id, AUG_14)).toEqual({ found: false, index: 2 }) + }) + + test("an existing message is found at its own index", () => { + expect(searchMessages(history, AUG_06.id, AUG_06)).toEqual({ found: true, index: 1 }) + expect(searchMessages(history, JUL_10.id, JUL_10)).toEqual({ found: true, index: 0 }) + }) + + test("an already-inserted post-wrap message is found, not duplicated", () => { + const withNew = [...history, m(AUG_14)] + expect(searchMessages(withNew, AUG_14.id, AUG_14)).toEqual({ found: true, index: 2 }) + }) + + test("id-only lookup (message.removed) finds by identity", () => { + const withNew = [...history, m(AUG_14)] + expect(searchMessages(withNew, AUG_14.id)).toEqual({ found: true, index: 2 }) + expect(searchMessages(withNew, "msg_absent")).toEqual({ found: false, index: 3 }) + }) + + // Regression guard for the data loss: the >100 trim drops list[0] and deletes + // its parts. Appending correctly is what keeps list[0] the genuinely oldest + // message; inserting at 0 made the trim delete the newest message instead. + test("repeated post-wrap appends keep the oldest message at index 0", () => { + const list = [m(JUL_10), m(AUG_06)] + for (let i = 0; i < 5; i++) { + const next = { id: `msg_0006f7687${i}`, time: { created: 1786713700254 + i } } + const at = searchMessages(list, next.id, next) + expect(at.found).toBe(false) + list.splice(at.index, 0, m(next)) + } + expect(list[0].id).toBe(JUL_10.id) + expect(list.at(-1)!.time.created).toBe(1786713700258) + expect(list.map((x) => x.time.created)).toEqual([...list].map((x) => x.time.created).toSorted((a, b) => a - b)) + }) + }) + + describe("compareToMarker resolves a session-held marker id", () => { + const list = [m(JUL_10), m(AUG_06), m(AUG_14)] + + test("orders messages around a post-wrap revert marker", () => { + expect(compareToMarker(list, m(JUL_10), AUG_14.id)).toBeLessThan(0) + expect(compareToMarker(list, m(AUG_14), AUG_14.id)).toBe(0) + }) + + test("orders messages around a pre-wrap revert marker", () => { + // A raw `id >= marker` compare calls the Aug-14 message "before" the Aug-06 + // marker, which is how /undo and the reverted-message strikethrough drifted. + expect(compareToMarker(list, m(AUG_14), AUG_06.id)).toBeGreaterThan(0) + expect(compareToMarker(list, m(JUL_10), AUG_06.id)).toBeLessThan(0) + }) + + test("returns undefined when the marker is outside the loaded window", () => { + expect(compareToMarker(list, m(AUG_14), "msg_notloaded")).toBeUndefined() + }) + }) + + test("selectMessages picks the newest bucket by time, not by id", () => { + // general-2's tail is post-wrap (smaller id, later time) — an id compare would + // pick general-1 and render the stale bucket. + const buckets = bucketMessages([m(AUG_06, { agentID: "general-1" }), m(AUG_14, { agentID: "general-2" })]) + expect(selectMessages(buckets, "main", "ses_actorhost").map((x: any) => x.id)).toEqual([AUG_14.id]) + }) +}) diff --git a/packages/opencode/test/cli/tui/select-messages.test.ts b/packages/opencode/test/cli/tui/select-messages.test.ts index 8e5f59f63..a648d793f 100644 --- a/packages/opencode/test/cli/tui/select-messages.test.ts +++ b/packages/opencode/test/cli/tui/select-messages.test.ts @@ -1,7 +1,12 @@ import { describe, test, expect } from "bun:test" import { bucketMessages, selectMessages } from "../../../src/cli/cmd/tui/context/sync" -const msg = (id: string, agentID?: string) => ({ id, agentID }) as any +// `time.created` mirrors the numeric suffix of `id`. selectMessages picks the +// newest bucket by its last message's TIME (ids wrap every ~2.18 years and invert +// across the boundary — see compareMessages), so a fixture without a time would +// leave every bucket tied and stop exercising the choice at all. +const msg = (id: string, agentID?: string) => + ({ id, agentID, time: { created: Number(id.replace(/^\D*/, "")) || 0 } }) as any describe("selectMessages", () => { test("renders the main bucket for a normal session", () => {