fix(runtime): budget context compaction per accepted step, not per send - #5319
Conversation
df365cd to
295c3ad
Compare
A long turn that overflowed once, folded, and then kept working (ten image reads in the archived CAD run) overflowed again and died: the compaction module was gated by `compactionAttemptedThisSend`, a boolean spent by the first fold for the rest of the Turn. The bound that latch was introduced for (#4559, #4653) is the compact-and-resend spin with no progress in between, not the second overflow of a request the provider has since grown by accepting new steps. The send-level state carried four independent "what did I do at which step" markers (`compactionAttemptedThisSend`, `compactionAppliedThisSend`, `replacedStepNumber`, the turn-local `pruneAppliedAtStep`, plus the image omission map doubling as a suppressor) and every consumer enumerated them by hand. They are replaced by one record: `stepShaping`, the set of shapers (fold, prune, image omission) applied to the history since the last request the provider accepted, cleared by the turn at that acceptance after the consumers that judge the accepted request have read it. Reactive recovery enters once per step (refused when the step already holds a fold or an omission), so the resend's own rejection is terminal and a later accepted step earns a fresh attempt. The proactive projection folds whenever the accepted baseline is over the declared window; a local stopping rule keyed on the folded request's accepted input was tried and removed, because that input carries the reserved tail and the step's tool schemas and so is not the floor. `context_provider_dropping` compares input only when the step shaped nothing, which removes the false report after a reactive fold and the permanent suppression after an image omission. Generated-by: Claude Code
295c3ad to
4a52af6
Compare
jackwener
left a comment
There was a problem hiding this comment.
Independent agent review. Reviewed at 4a52af6206f2b099068cdb40500dba64f84967cb. I am an AI agent (executing seat @kabi-opus) publishing through a shared GitHub account; this is an automated review, not a human sign-off.
No P0–P2 findings. One wording nit at the end.
What I checked and why
Replacing four independent latches with one stepShaping set is the kind of change where the new code can read perfectly while a bound quietly disappears, so I went after the bounds rather than the shape.
1. The spin bound survives. The old latches existed to stop compact-and-resend with no progress in between (#4559, #4653). What now prevents it is that stepShaping.clear() sits inside if (providerOutcome.kind === 'completed') (ai-sdk-turn.ts:1872) — the same branch that does runtimeSteps += 1. A rejected request therefore never clears the set, so a step whose fold was spent stays spent and the resend's own rejection is terminal. I confirmed there is no continue/break/return/throw between the start of that branch and the clear, so every accepted step reaches it.
There is exactly one statement that could throw before the clear — await this.recordSystemNote('context_provider_dropping', …). Skipping the clear there is harmless, because that branch is gated on stepShaping.size === 0: if it runs at all, there is nothing to clear.
2. Read-before-clear holds. The context_provider_dropping comparison reads stepShaping.size === 0 at :1847, 25 lines before the clear. Clearing earlier would reintroduce the mirror image of the bug this PR fixes, so I treated the ordering as its own invariant rather than as a detail of the diff.
3. The stated cost bound rests on something narrower than the description says. The description explains the proactive path as having "no per-step gate (it runs once per step by construction)". Reading it, I expected the self-limit to be state.baselineTokens = undefined after a fold (ai-sdk-compaction.ts:941) — but that cannot be it: baselineTokens is unconditionally recomputed from options.completedSteps at :877 on every invocation of the hook, and a retry within a step has the same completedSteps, so the trigger would re-arm. The bound really does depend on the hook not being re-entered within a step, and the proactive path now has no stepShaping gate to fall back on if that assumption ever stops holding.
So I tested the assumption on the path most likely to break it — a retryable non-overflow failure forcing a resend inside one step, which the recent retry-classification change makes more common. Using this repository's own reactive fixture with script: ['tool', 'rateLimit', 'done'], contextWindow: 100, declareContextWindow: true, bigPriors: true:
doStreamCalls: 3, checkpoints: 1, summarizerCalls: 1, stopReason: end_turn
Three provider calls, one fold and one summarizer call. The bound holds on this path. I am reporting this as a verified non-finding rather than silently dropping it, because the reason it holds is not the reason the description gives.
4. projectionCheckpoint really is the old compactionAppliedThisSend. It is assigned at ai-sdk-compaction.ts:1188, immediately before the decision: 'compacted' return, so the write_failed and unmaterializable paths that return earlier never set it. On the merge base, compactionAppliedThisSend was set at both the proactive and the reactive site — so was every path that reaches this line, since both go through the shared core. The substitution preserves the meaning, including for context_overflow_after_compaction.
Ablations I ran myself
The description states each new regression was confirmed failing without the line it protects. I did not take that on trust and re-ran two, confirming each time that the ablation marker reached the emitted dist/ artifact before reading the result:
- Removing
state.stepShaping.add('fold')from the proactive path →a proactive fold spends the step, so the same step does not fold againfails. - Neutralising the
stepShaping.size === 0gate →a fold-shrunk retry input is not reported as provider droppingfails.
Both tests genuinely constrain their lines.
Other checks
Built from source at the reviewed SHA and ran the six suites the description names: 399 tests, 399 pass, 0 fail — matching the reported figure. No references to compactionAttemptedThisSend, compactionAppliedThisSend, replacedStepNumber or pruneAppliedAtStep remain anywhere in the tree, so this is a complete replacement rather than a partial one.
Nit, not a finding: the comment above the proactive call site says the stage maps "keep the raw projection on skip/fail", but ActiveRequestCompactionOutcome has only fail and compacted; the code correctly handles the two that exist. The comment describes a third case that cannot occur.
Boundaries
I did not run the full test suite or E2E — the description says the same, so that gap is shared rather than closed. I called no paid provider API. Every claim above is from source, local suites and the two ablations; the author's own results were context, not my evidence.
Code review and merge readiness are separate. At this SHA the two reported checks are green, mergeable=MERGEABLE, mergeStateStatus=BLOCKED (different fields, both can hold). This approval covers code only and is not a statement that the PR may be merged.
…haping record
Three pieces of per-step state each duplicated an authority that already
existed, so a reader had to check two places to know one fact.
`recoverFromOverflowError` re-classified the provider error it was handed.
`ModelAdapter.classifyError` returns `error.kind` unchanged for a ModelFailure,
and the only caller already passes the normalized failure and tests
`failure.kind === 'context_overflow'` a few lines below. Moving that test into
the caller's gate makes the overflow condition visible where the decision is
made and drops compaction's whole dependency on ModelAdapter.
`toolSchemaShrank` was a second, parallel record of "this request is not a pure
append of its predecessor" living beside `stepShaping`. It now records itself
as `'tools'` where the tool set is resolved, next to the request it describes,
and the silent-eviction check reads one set. `'tools'` deliberately does not
gate reactive recovery: that gate still asks only about fold and image
omission, which are the shapings a resend cannot repeat.
`previousCheckpoint` and `projectionCheckpoint` were assigned the same value on
every fold, so they could only ever disagree by a bug. The pre-turn seed keeps
its own write-once field and `previousCheckpoint` is now derived from the two,
which makes the roll-forward rule ("the fold of this send, else the one carried
in") readable at the field instead of inferable from two assignments.
Generated-by: Claude Code
`context_overflow_after_compaction` reads `projectionCheckpoint`, not "this step folded": the note is about what the Turn has folded. A send whose first overflow folded and was accepted, and whose later step overflows with its own fold failing open, writes the note even though that step reshaped nothing. Confirmed failing when the note is keyed on the step shaping record instead. The clear placement of the shaping record (after acceptance, not at the top of the step loop) is documented at the clear; the continuation-lane regression that would pin it needs a Responses transport stub in this suite and is not worth it. Generated-by: Claude Code
1c122c5 to
e76676c
Compare
Two of this PR's new tests assert cost-model consequences that an existing test already pins on the same production line, so they add runs without adding protection. `a declared window under the fold floor folds on every accepted step` asserts the per-step fold budget through a window the fold floor can never reach. The budget itself is the line, and `compacts at most once per step, and again once a step is accepted` fails on it already (revert the proactive hook to a per-send latch and only that test goes red). What the deleted test added beyond it was an unreachable-target scenario, not a distinct guarantee. `a later step escalates from the image omission to a fold, keeping the images omitted` asserts that the image-omission map survives into a later step's fold. The map merge it covers has no production line of its own that any revert can break: `a later step that overflows again folds again after real progress` pins the per-step reattempt, and `surfaces a retryable second overflow after the image-only retry without another loop` pins the omission spending its own step. Fixture options stay: every one this PR added still has a user (`usageByCall`/`toolSteps` in the per-step budget test, `inputTokensByCall` in the provider-dropping test, `failFirstCheckpointWrite` in the failed- proactive-fold test). Only the now-unreachable tail of `toolStepPath`'s path list is trimmed. Generated-by: Claude Code
Summary
A long turn that overflowed once, folded successfully, and then kept working (ten image reads in an archived CAD run: 732,789 → 327,635 → 1,393,264 input tokens) overflowed again and ended as a terminal error with
context_compactedandcontext_overflow_after_compactionon the trace. The compaction module was gated bycompactionAttemptedThisSend, a boolean the first fold spent for the rest of the Turn. The bound that latch exists for (#4559, #4653) is the compact-and-resend spin with no progress in between; it was also blocking the second overflow of a request the provider had since grown by accepting new steps.The underlying shape of the problem is that the send-level state carried four independent "what did I do at which step" markers (
compactionAttemptedThisSend,compactionAppliedThisSend,replacedStepNumber, the turn-localpruneAppliedAtStep, plusomittedImageToolResults.sizedoubling as a suppressor), and every consumer had to enumerate them by hand. This PR replaces them with one record onMidTurnCapacityCompactState:stepShaping: Set<'fold' | 'prune' | 'omit_images' | 'tools'>— what has reshaped the request since the last one the provider accepted. The turn clears it at that acceptance, after the consumers that judge the accepted request have read it; prune, the proactive fold, the reactive fold and image omission each record themselves; every consumer reads it. An empty set is the only proof that the next request is a pure append of its predecessor. Clearing at acceptance rather than at the top of the step loop matters for the continuation lane, where the ledger is re-projected (and may prune) after a step is accepted: that prune belongs to the next request.On top of it:
stepShapingalready holds a fold or an image omission, so the resend's own rejection is terminal, and a later step the provider accepted earns a fresh attempt. A proactive fold that was applied also blocks it; a proactive fold that failed without latching (write_failed,replacement_unmaterializable, ledger read) no longer spends the step, and the provider's rejection gets one reactive attempt.mainthe send folded once and then sent every later request over the declared window; here every accepted step that is still over it folds again, buying the step before it. With roll-forward intact one summarizer call is a few thousand tokens against main requests in the hundreds of thousands; when an active prune between two folds breaks roll-forward, that one fold re-summarizes the whole covered span, and a span the summarizer cannot take latchessummarizerFailurefor the Turn (History compaction always fails open on kimi-coding-plan/k3-256k: summarizer instruction as system prompt is ignored and breaks prefix cache #4634), which ends folding. Every fold is visible incompactionDecisions; the first one in a send writes thecontext_compactednote (feat(desktop): show context compaction in the task transcript #3587). No local estimate stands in for the provider's verdict (Context budget: stop estimating fit; the provider decides, the Maka window is a user target #4559).context_provider_droppingcompares input against the previous accepted request only whenstepShapingis empty. Onmain, an accepted retry after a reactive fold at step N>0 carried a smaller input and was reported as provider dropping, which then suppressed the real warning for the rest of the session; and any image omission disabled the check for the rest of the send. Both are gone.context_overflow_after_compactionreadsprojectionCheckpoint(a fold was applied in this send), the same meaningcompactionAppliedThisSendhad.omittedImageToolResultsinstead of replacing the map, so an earlier step's omission cannot resurface. The separate permanent "no fold after an omission" gate is gone. The merge is defensive and has no regression of its own: a second omission in one send needs the image budget to re-admit older images after a fold, which the current fixtures cannot produce.Three more duplicate authorities fall out of the same record:
toolSchemaShrankwas a second, parallel way of saying "this request is not a pure append of its predecessor". A shrinking tool set now records itself as'tools'where the tool set is resolved, and the silent-eviction check reads one set.'tools'deliberately does not gate reactive recovery: that gate still asks only about fold and image omission, the shapings a resend cannot repeat.AiSdkCompactionno longer takes aModelAdapter.recoverFromOverflowErrorre-classified the failure it was handed, butclassifyErrorreturnserror.kindunchanged for aModelFailureand the only caller already passes the normalized failure and testsfailure.kind === 'context_overflow'a few lines below. The test moved into the caller's gate, where the overflow condition is visible at the decision, and compaction's whole dependency on the adapter is gone.previousCheckpointandprojectionCheckpointwere assigned the same value on every fold, so they could only disagree by a bug. The pre-turn seed keeps a write-onceseedCheckpointandpreviousCheckpointis derived (projectionCheckpoint ?? seedCheckpoint), which puts the roll-forward rule at the field instead of in two assignments.Unchanged: the same rejected request is never compacted twice, completed tools are never re-executed, the durable checkpoint is still validated and persisted before the replacement is applied, the planner rolls forward from the previous checkpoint so a second fold covers the steps added since the first, and the summarizer failure circuit (
summarizerFailure, #4634) is still latched once for the whole Turn. Not changed here: the compaction threshold is still only the user-declared window (#4458, #4559), so without a declaration no proactive fold runs and recovery is reactive only.Refs #4559
Verification
npm --workspace @maka/runtime run build, then inpackages/runtime:node --test --test-concurrency=4onoverflow-reactive-recovery,mid-turn-capacity-backend,ai-sdk-backend,history-compact-mid-turn-checkpoint,context-budget-mid-turn-policy,provider-image-overflow-recovery: 397 pass, 0 fail.npm run formatandnpm run lint: no changes, no issues.a later step that overflows again folds again after real progress(['tool','overflow','tool','tool','overflow','done']): before, the fifth request was terminal (5 calls, 1 checkpoint); after, 6 calls, 2 checkpoints,end_turn, every tool executed once.a proactive fold spends the step, so the same step does not fold again: 2 calls, 1 checkpoint, terminalcontext_overflowwithcontext_overflow_after_compaction; without the shaping record, the rejected folded request is folded a second time.a failed proactive fold leaves the reactive attempt to the provider's rejection: first checkpoint write fails, the raw request is rejected, the reactive fold recovers toend_turn.a fold-shrunk retry input is not reported as provider dropping: before, the retry after a reactive fold wrotecontext_provider_dropping.an unfoldable overflow after an accepted fold still says the request was compacted: the first overflow folds and is accepted, a later step overflows and its own fold fails open. With the note gated onstepShaping.has('fold')instead ofprojectionCheckpointthe note is missing and the test fails.a second overflow after the single retry ends as a real error(same step, no progress) andsurfaces a retryable second overflow after the image-only retry without another loop.mid-turn-capacity-backend"compacts at most once per send" is now "once per step, and again once a step is accepted", with a fixture whose folded request fits the window and whose later step grows over it again.npm test, E2E.AI use
Select exactly one:
Tool(s) and scope: Claude Code (investigation, implementation, tests, adversarial review passes, this description); reviewed and directed by the author.
Checklist
Does this PR entail a change in behavior?