fix(session): prevent double auto-compaction from filterCompacted reorder#27545
Merged
Conversation
After PR #27145's tail-restoration reorder in filterCompacted, the returned array is [compaction-user, summary-assistant, ...tail..., continue-user] SessionPrompt.runLoop walks msgs backward to pick lastFinished and gates the overflow check on lastFinished.summary !== true. With the reordered layout the backward walk hits a finished assistant inside the retained tail before it reaches the summary, the guard fails open, and a second compaction.create fires immediately after the first summary completes. Observed in the wild as two auto compaction parts created within ~1s of each other. Failing test mirrors the runLoop walk on the output of filterCompacted and asserts that lastFinished is the summary, not the pre-compaction overflow assistant.
filterCompacted returns messages in model-consumption order [compaction-user, summary, ...retained tail..., continue-user] so the backward array walk in runLoop was picking a finished assistant from the retained tail (a pre-compaction overflow turn) as lastFinished instead of the freshly-completed summary. The overflow guard at prompt.ts:1722 (lastFinished.summary !== true && isOverflow(tokens)) then failed open on the old assistant's stale ~280K-token usage and fired compaction.create a second time immediately after the first summary completed. Sort msgs by id (MessageID is monotonic) before the backward walk so lastUser, lastAssistant, lastFinished, and tasks are derived in chronological order. msgs itself is left in model-consumption order so toModelMessagesEffect and handleSubtask still see the reorder intended by PR #27145.
kitlangton
changed the title
Collaborator
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // (packages/opencode/src/session/prompt.ts ~1660). The runLoop sorts msgs | ||
| // chronologically by id before walking so the model-consumption reorder | ||
| // performed by filterCompacted does not poison lastFinished. | ||
| const chronological = filtered |
Contributor
There was a problem hiding this comment.
Suggestion for the human to decide: this test now duplicates the SessionPrompt.runLoop sorting/walk logic instead of exercising the implementation that regressed. That is at odds with our testing guidance to test the actual implementation and not copy production logic into tests; it also means a future change that breaks the runLoop walk could leave this regression test passing. Could this cover the behavior through SessionPrompt.loop/runLoop, or alternatively extract the “latest chronological state” selection into a small tested helper used by runLoop?
Replaces slice+sort+reverse-walk with one forward pass tracking running max-id for lastUser, lastAssistant, lastFinished, and a flatMap for tasks. O(n), no allocation, no comparator, and the chronological intent is explicit at the comparison site instead of buried in a sort callback. Also fixes a latent dead-code check the rewrite obviated: `if (task && !lastFinished)` where task was always a (possibly empty) array, making the truthy half a no-op. Test refactored to assert the same chronological-latest invariant via reduce rather than mirroring the implementation's walk shape, so it stays a faithful regression contract if the implementation changes.
The chronological-vs-model-consumption order distinction was the source
of the original double-compaction bug, so it deserves a named function
the runLoop can call instead of inlining. MessageV2.latest returns
{ user, assistant, finished, tasks } picked by max id (MessageID is
monotonic).
Two benefits over the previous inline form:
- runLoop's binding setup is one line instead of 24, and the comment
explaining the model-consumption-order gotcha lives with the helper
rather than at every consumer.
- Test now exercises MessageV2.latest directly. Previously the test
re-implemented the selection logic, so a future change that broke
the runLoop walk could pass the regression test silently.
Adds a second unit test covering the other half of latest's contract
(a fresh compaction-user newer than the latest summary must surface in
tasks so processCompaction runs).
opencode-agent Bot
added a commit
that referenced
this pull request
May 14, 2026
…lterCompacted reorder
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
2 participants
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.
Summary
After #27145,
MessageV2.filterCompactedreturns messages reordered for model consumption:SessionPrompt.runLoopwalksmsgsbackward from the array end to picklastUser,lastAssistant,lastFinished, andtasks(packages/opencode/src/session/prompt.ts:1660-1668). The walk treats array order as chronological. With the reorder it isn't — the backward walk hits a finished assistant inside the retained tail (a pre-compaction overflow turn) before it reaches the summary at the front.lastFinishedthen hassummary === undefinedandtokens.total ≈ 280K, the overflow guard atprompt.ts:1722(lastFinished.summary !== true && isOverflow(...)) fails open on the stale tokens, andcompaction.createfires a second time as soon as the first summary completes.Observed in
opencode.dbas pairs ofauto: truecompaction parts ~1s–3m apart in multiple sessions post-2026-05-12, e.g.ses_1dcaf652cffeB0ZVK2zWAkxRSDat 08:01:35 → 08:02:41 today, where the second compaction summary started and then aborted.Fix
Sort
msgsbyid(monotonic) inside the walk and iterate the chronological view.msgsitself stays in model-consumption order sotoModelMessagesEffectandhandleSubtaskkeep receiving the layout #27145 intended.Smallest change that fixes the bug at the source. Alternatives considered:
filterCompactedinto model-message construction. Architecturally cleaner, butmsgsis consumed by bothtoModelMessagesEffect(prompt.ts:1827) andhandleSubtask→taskTool.execute(prompt.ts:1705), so the reorder would have to be threaded through both paths. Bigger blast radius than a regression fix should carry; that cleanup is its own change.[summary, tail]immediately before the continue prompt so the model attends to recent grounding.lastFinishedandlastAssistantalso feed the loop-exit check (prompt.ts:1682-1690) and the user-text wrap (prompt.ts:1803-1819). Both happen to coincidentally tolerate the wrong value today; better to fix the source.Commits
test(compaction): reproduce double auto-compaction trigger— failing regression test using the post-compaction message shape.fix(session): walk msgs chronologically when picking lastFinished— sort by id before the walk; updates the test to mirror the fixed walk so it stays a faithful regression contract.Test plan
bun run test test/session/message-v2.test.ts -t filterCompactedfails on the test-only commit, passes after the fixbun run test test/session/message-v2.test.ts test/session/compaction.test.ts test/session/prompt.test.ts— 138 pass, 0 failbun run typecheckclean