🤖 fix: commit compaction boundary after crash-recovery resume - #3972
🤖 fix: commit compaction boundary after crash-recovery resume#3972coadler wants to merge 2 commits into
Conversation
A crash mid-compaction leaves an orphaned assistant row; startup recovery appends a synthetic [CONTINUE] user sentinel after it. resolveCompactionRequest previously stopped scanning at that sentinel unless the current stream itself was a compaction send, so the resumed summary stream never correlated with the pending request and the boundary was silently never committed. Skip all synthetic user rows regardless of send options; correlation still stops at the first real user row. Co-authored-by: Mux <noreply@coder.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e15a26d0bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
Codex review follow-up: skipping every synthetic user row let a stale compaction request (left behind when a summary stream failed validation without committing a boundary) be claimed by an unrelated later turn whose history began with some other synthetic row, persisting that turn's response as a compaction boundary. Limit traversal to the three recognized producer shapes: file @-mention prompt snapshots, turn-start <system-file-update> notifications, and the crash-recovery [CONTINUE] sentinel. Co-authored-by: Mux <noreply@coder.com>
|
Addressed in cbd307d. Agreed — skipping every synthetic user row was too broad: a stale request left by a failed summary (no boundary committed) could be claimed by an unrelated later turn that began with some other synthetic row. Traversal now stops at any synthetic user row except the three recognized producer shapes:
Everything else — goal continuations, task wakes, real user rows — stops correlation. Added a regression test for the stale-request scenario. |
|
@codex review Please take another look. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This comment was marked as outdated.
This comment was marked as outdated.
1 participant
Summary
Keeps a pending compaction request correlated with its resumed stream after an app crash mid-compaction, so the compaction boundary is committed instead of silently dropped.
Background
When Xum crashes mid-compaction (for example on OOM), startup recovery finds a trailing non-partial assistant row and appends a synthetic
[CONTINUE]user sentinel before re-running the turn. The recovered stream then produces a valid summary, butresolveCompactionRequeststopped scanning history at that sentinel unless the current stream itself was a compaction send. The regenerated summary persisted as an ordinary assistant message and no compaction boundary was ever committed; the user had to run compaction again.Observed in workspace
dffb219cd6: attempt 3 completed at the provider level (finishReason: stop, 262k input tokens) but produced nocompaction.completedtimeline event.Implementation
resolveCompactionRequestnow skips any synthetic user row (metadata.synthetic === true) regardless of send options. Correlation still stops at the first real user row, so normal sends after a compaction request never mis-correlate.[CONTINUE], plus a guard that a real user message stops correlation.Validation
bun test src/node/services/agentSession.autoCompaction.test.ts src/node/services/compactionHandler.test.ts: 83 pass, 0 fail.make typecheck,make lint, andmake static-checkall pass.Risks
Low. The change only widens which synthetic rows are skipped during backward correlation; stopping at the first real user row is unchanged, so ordinary conversation flow cannot mis-correlate a stale compaction request. Worst case for a pathological history is correlating a summary stream with an older pending compaction request, which commits the boundary that request asked for anyway.
Generated with
xum• Model:openrouter:stealth/ox-alpha• Thinking:high• Cost:$0.01