fix(session): order messages by time so the run loop can terminate - #38798
fix(session): order messages by time so the run loop can terminate#38798dkindlund wants to merge 2 commits into
Conversation
`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 anomalyco#38791
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
lexicographic ID comparison is fundamentally fragile It assumes.
None of these are guaranteed. |
…rdering filterCompacted emits messages out of chronological array order and mints fresh ids for rewritten messages. Add a regression test asserting latest() resolves the newest user/assistant/finished by time (id as same-ms tiebreaker), never by array position -- so a pre-compaction retained-tail assistant is not mistaken for the most recent turn. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks — useful framing. Mapping the four assumptions to what this PR actually changes: 1 & 4 (ULID / imported ids): agreed, that's the root cause — and it's exactly what this removes. 2 (same millisecond): already covered for natively-minted ids — 3 (compaction rewriting ids): compaction mints fresh ids via the same On the broader point: you're right that comparing message order to decide loop termination is a heuristic, not a proof. This PR makes the heuristic correct for every case the data can actually support. A fully ordering-independent exit (track that this iteration produced a finished assistant with no pending tool calls, rather than comparing last-user vs last-assistant) is a larger change — happy to open it as a follow-up so it can be reviewed on its own merits. This PR stays the minimal, behavior-preserving fix for the reported infinite loop, with the failing→passing regression test as the precondition. |
|
yw. appreciate the effort too. this patch helped validate my concerns, but it doesn't fix the issues i was having with opencode's message ordering. not implying this causes issues. for me, it behaves nearly identical to an unmodified version. root causes predate this. i just tested this pr on one of my rigs and here are my results ... tl;dr the 'exiting loop' strikes back like the imperial march. repeatedly. every test abruptly ended the runLoop prematurely. a bit more. it's like flying blind around here. there's virtually no telemetry in this patch (or the core) to track message state. knowing really is half the battle. model context windows are tiny (a 1M window really is still very tiny) and they rarely see the big picture without extremely (painstakingly) well written prompts that might as well be code because they're so detailed. imho of course. i used the same two initial prompts and same model (gemma4:26b) for all tests. tests setup is identical to other issues as well.
sequences obviously garbled. raw logs are attached. from a tui perspective, all tests get cut off mid-stream and results are not return (but are given) and maybe my analysis is wrong, i.e. 
opencode-pr38798-alice1.log |
Issue for this PR
Closes #38791
Type of change
What does this PR do?
latest()and the run loop's exit test find the newest message by comparing ids as plain strings. That works becauseIdentifier.ascending()puts a timestamp in the first 12 chars, so ids happen to sort chronologically. Nothing enforces that for sessions coming in throughopencode import.I hit this importing Claude Code sessions with a converter that emitted
msg_<uuid>. If any imported id sorts above the ids of the live conversation,latest()hands back that old message as the newest one.lastUser.id < lastAssistant.idis then false forever, the loop never breaks, and it keeps re-requesting. Each retry ends on an assistant message, which Anthropic rejects with "does not support assistant message prefill" — the user just sees "Unexpected server error".It looks flaky rather than broken because it depends on whether any random id happens to sort high: about 2.6% of them do, so a 3-message session usually survives and a 400-message one never does. That's what put me onto ids in the first place — the same small session passed once and failed once.
The fix is to compare
time.createdand fall back to the id when timestamps match. Nothing changes for normal sessions, since native ids already sort in timestamp order — it only differs when the ids were never chronological to begin with. I applied it inlatest()(user, assistant, finished, and thetasksfilter, which assumed the same thing) and in the loop's exit test.I did not touch the
finish === "stop"handling. Breaking out purely on the finish reason would also fix my case, but the comment right above says some providers returnstopwith tool calls still pending, so that seemed like a behaviour change rather than a bug fix.How did you verify your code works?
Added
packages/opencode/test/message-latest-ordering.test.tsand checked it fails first: 2 pass / 2 fail ondev, 4 pass / 0 fail with the change. The failing assertion is the loop's own precondition —latest()returned the 1000ms assistant instead of the 3000ms one.Before writing the test I confirmed the mechanism against a real provider by putting a proxy in front of it. The successful request came back
stop_reason: "end_turn"withmessage_stop, and opencode still issued another request one second later with the assistant reply appended. I also ruled out plugins (same failure under--pure), missingstep-start/step-finishparts, unpairedtool_use, and unsigned thinking blocks — and replaying the captured payload on its own returns 200, so the history itself was valid.Screenshots / recordings
N/A, not a UI change.
Checklist