From fa31d9ec356c935eefabb040e2c5a77f5e9bba64 Mon Sep 17 00:00:00 2001 From: Darien Kindlund Date: Fri, 24 Jul 2026 23:09:02 -0400 Subject: [PATCH] fix(session): order messages by time so the run loop can terminate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `latest()` and the run loop's exit test both compare message ids as plain strings. That holds for ids from Identifier.ascending(), which embed a timestamp, but nothing enforces it for sessions that arrive via `opencode import`. When an imported id sorts above the live conversation, `latest()` returns the wrong newest message, the exit test never passes, and the loop re-requests forever. Each continuation ends on an assistant message, so providers that disallow prefill reject it: This model does not support assistant message prefill. The conversation must end with a user message. The user sees a generic "Unexpected server error"; the response that triggered the retry was a clean `stop_reason: "end_turn"` with `message_stop`. Failure is probabilistic in session length — roughly 2.6% of random hex ids sort above a freshly minted native id, so ~8% of 3-message sessions and essentially 100% of 400-message ones are affected. That matches what I saw in practice, including a small session that failed only intermittently. Order by `time.created` and keep the id as the tiebreaker, in `latest()` (user, assistant, finished, and the `tasks` filter, which made the same assumption) and in the loop's exit test. Behaviour is unchanged for natively minted ids, where time and id order already agree. Adds a regression test that fails on the current code and passes with the fix. Refs #38791 --- packages/opencode/src/session/message-v2.ts | 21 ++++- packages/opencode/src/session/prompt.ts | 13 +++- .../test/message-latest-ordering.test.ts | 78 +++++++++++++++++++ 3 files changed, 107 insertions(+), 5 deletions(-) create mode 100644 packages/opencode/test/message-latest-ordering.test.ts diff --git a/packages/opencode/src/session/message-v2.ts b/packages/opencode/src/session/message-v2.ts index 1bea9f52c3ec..3acf7cfca502 100644 --- a/packages/opencode/src/session/message-v2.ts +++ b/packages/opencode/src/session/message-v2.ts @@ -582,18 +582,31 @@ export const filterCompactedEffect = Effect.fnUntraced(function* (sessionID: Ses // assistant doesn't get mistaken for the most recent turn. tasks are // compaction/subtask parts attached to user messages newer than the latest // finished assistant — i.e. unprocessed work. +// Order by wall-clock first, id second. Ids minted by Identifier.ascending() +// embed a timestamp and so sort chronologically on their own, but sessions can +// also arrive via `opencode import` from third-party writers whose ids carry no +// ordering. A single such id sorting above the live conversation used to make +// the newest message unreachable here, so the run loop's exit test could never +// pass and opencode re-requested until the provider rejected the +// assistant-terminated conversation. `time.created` is authoritative; the id +// stays as the tiebreaker for messages minted within the same millisecond. +function isNewer(a: { id: string; time: { created: number } }, b: { id: string; time: { created: number } }) { + if (a.time.created !== b.time.created) return a.time.created > b.time.created + return a.id > b.id +} + export function latest(msgs: WithParts[]) { let user: User | undefined let assistant: Assistant | undefined let finished: Assistant | undefined for (const msg of msgs) { const info = msg.info - if (info.role === "user" && (!user || info.id > user.id)) user = info - if (info.role === "assistant" && (!assistant || info.id > assistant.id)) assistant = info - if (info.role === "assistant" && info.finish && (!finished || info.id > finished.id)) finished = info + if (info.role === "user" && (!user || isNewer(info, user))) user = info + if (info.role === "assistant" && (!assistant || isNewer(info, assistant))) assistant = info + if (info.role === "assistant" && info.finish && (!finished || isNewer(info, finished))) finished = info } const tasks = msgs.flatMap((m) => - finished && m.info.id <= finished.id + finished && !isNewer(m.info, finished) ? [] : m.parts.filter((p): p is CompactionPart | SubtaskPart => p.type === "compaction" || p.type === "subtask"), ) diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index eb116f6b960f..3d9f902d6951 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -1108,11 +1108,22 @@ const layer = Layer.effect( (part) => part.type === "tool" && !part.metadata?.providerExecuted && !isOrphanedInterruptedTool(part), ) ?? false + // Compare by wall-clock, not by raw id. Ids only sort chronologically + // when minted by Identifier.ascending(), which is not guaranteed for + // sessions brought in through `opencode import`; an unordered id here + // used to keep this test permanently false, so the loop never exited. + // The id remains the tiebreaker within a single millisecond. + const assistantFollowsUser = + !!lastAssistant && + (lastUser.time.created !== lastAssistant.time.created + ? lastUser.time.created < lastAssistant.time.created + : lastUser.id < lastAssistant.id) + if ( lastAssistant?.finish && !["tool-calls"].includes(lastAssistant.finish) && !hasToolCalls && - lastUser.id < lastAssistant.id + assistantFollowsUser ) { const orphan = lastAssistantMsg?.parts.find( (part): part is SessionV1.ToolPart => part.type === "tool" && isOrphanedInterruptedTool(part), diff --git a/packages/opencode/test/message-latest-ordering.test.ts b/packages/opencode/test/message-latest-ordering.test.ts new file mode 100644 index 000000000000..64078f5e0da9 --- /dev/null +++ b/packages/opencode/test/message-latest-ordering.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, test } from "bun:test" + +import { MessageV2 } from "../src/session/message-v2" + +/** + * `latest()` used to pick the newest user/assistant message by comparing ids as + * plain strings. That holds for ids minted by Identifier.ascending(), which + * embed a timestamp, but not for sessions written by anything else — e.g. a + * third-party importer emitting uuids. A single imported message whose id + * happens to sort high then masquerades as the newest message, the run loop's + * exit test (`lastUser.id < lastAssistant.id`) never passes, and opencode + * re-requests until the provider rejects the assistant-terminated conversation. + */ +function msg(id: string, role: "user" | "assistant", created: number, finish?: string) { + return { + info: { + id, + sessionID: "ses_test", + role, + time: { created }, + ...(role === "assistant" ? { finish } : {}), + }, + parts: [], + } as unknown as MessageV2.WithParts +} + +describe("MessageV2.latest ordering", () => { + test("uses chronology, not raw id sort, to find the newest messages", () => { + // An imported history whose ids do not sort chronologically: the FIRST + // message carries the highest-sorting id. + const msgs = [ + msg("msg_ffffffffffffffffffffffff", "assistant", 1_000, "stop"), // oldest, id sorts highest + msg("msg_0000000000000000000000aa", "user", 2_000), + msg("msg_0000000000000000000000bb", "assistant", 3_000, "stop"), // newest in time + ] + + const { user, assistant } = MessageV2.latest(msgs) + + expect(user?.id).toBe("msg_0000000000000000000000aa") + // Raw id sort would return the 1_000ms message here. + expect(assistant?.id).toBe("msg_0000000000000000000000bb") + expect(assistant?.time.created).toBe(3_000) + }) + + test("the newest assistant follows the newest user, so the run loop can exit", () => { + const msgs = [ + msg("msg_ffffffffffffffffffffffff", "assistant", 1_000, "stop"), + msg("msg_0000000000000000000000aa", "user", 2_000), + msg("msg_0000000000000000000000bb", "assistant", 3_000, "stop"), + ] + + const { user, assistant } = MessageV2.latest(msgs) + + // What runLoop asserts before breaking out of the loop. + expect(user!.time.created).toBeLessThan(assistant!.time.created) + }) + + test("still orders correctly for natively minted, time-sortable ids", () => { + const msgs = [ + msg("msg_f90250c84002aaaaaaaaaaaaaa", "user", 1_000), + msg("msg_f90253552001bbbbbbbbbbbbbb", "assistant", 2_000, "stop"), + ] + + const { user, assistant } = MessageV2.latest(msgs) + + expect(user?.id).toBe("msg_f90250c84002aaaaaaaaaaaaaa") + expect(assistant?.id).toBe("msg_f90253552001bbbbbbbbbbbbbb") + }) + + test("ties on timestamp fall back to id order", () => { + const msgs = [ + msg("msg_aaa", "assistant", 5_000, "stop"), + msg("msg_bbb", "assistant", 5_000, "stop"), + ] + + expect(MessageV2.latest(msgs).assistant?.id).toBe("msg_bbb") + }) +})