fix(session): order messages by time so the run loop can terminate#38798
Open
dkindlund wants to merge 1 commit into
Open
fix(session): order messages by time so the run loop can terminate#38798dkindlund wants to merge 1 commit into
dkindlund wants to merge 1 commit 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
github-actions
Bot
added
needs:compliance
Contributor
|
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. |
github-actions
Bot
removed
needs:compliance
Contributor
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
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.
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