fix(session): ignore failed compaction summaries when reordering - #42283
Open
codeAnqiang-ma wants to merge 1 commit into
Open
fix(session): ignore failed compaction summaries when reordering#42283codeAnqiang-ma wants to merge 1 commit into
codeAnqiang-ma wants to merge 1 commit into
Conversation
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
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 #42282
Type of change
What does this PR do?
filterCompacteddecides "this compaction is complete" twice, with two different tests. The truncation loop requiressummary && finish && !error; the reordering branch underneath only checkssummary. A compaction whose summary turn failed lands between them: the loop keeps the full history, then the reorder returns[compaction..summary] + [tail..compaction] + [after summary]and drops everything beforetail_start_id. Since the summary failed there is nothing to replace that history, and the errored assistant message is dropped bytoModelMessagesEffectanyway.tail_start_idis persisted even on failure, so the loss repeats every turn.This makes the reordering branch use the same test as the loop. Successful compactions are unaffected — the reorder still fires, this only stops it firing on a summary the loop already considers incomplete.
Only the guard is changed. Tightening the
tail_start_idwrite incompaction.tswould stop new sessions from getting into this state, but sessions that already have the marker persisted still need this guard to recover, so it seemed like the right single place to fix.How did you verify your code works?
Added
keeps history before tail_start_id when the summary faileddirectly beside the existing success-path testretains original tail when compaction stores tail_start_id, reusing the sameaddUser/addAssistant/addCompactionParthelpers with an errored summary instead of a finished one.devit fails: expected all 8 messages, received 6 — the first turn's user and assistant messages are missing and the compaction pair is hoisted to the frontFrom
packages/opencode:bun test test/session/messages-pagination.test.ts— 52 pass, 0 failbun test test/session/message-v2.test.ts— 39 pass, 0 failbun test test/session/compaction.test.ts— 54 pass, 1 skip, 0 failbun test test/session/prompt.test.ts— 57 pass, 1 skip, 0 failbun test test/session/revert-compact.test.ts— 8 pass, 0 failbun test test/session/snapshot-tool-race.test.ts— 1 pass, 0 failbun typecheck, and the pre-pushbun turbo typecheck— 30/30 packagesoxlintwarning count unchanged from baseline on both filesI also drove the whole path end to end against the real
SessionCompaction.processwith persistence — three turns, then an auto-compaction whose summary request returns a non-retryable 401 — and confirmed the next turn sees 3 of 5 messages before the change and all 5 after. That one is slow and provider-shaped so I left it out of the suite; happy to add it if you'd like it committed.Did not run the rest of
packages/opencode.Screenshots / recordings
N/A — not a UI change.
Checklist
Disclosure: found and fixed with AI assistance; I reviewed and verified every claim locally.