fix(runtime): retreat a rejected fold to a span the provider has accepted - #4667
Conversation
fd26f65 to
bcd8a5c
Compare
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
acceptedInputBoundary() proves only that some prior model request accepted this prefix, not that the current summarizer model/connection did. Session history can span runs on different routes (the compaction path already carries runtimeContextRunHeaders for that distinction), but this scan uses only role === 'model'; after a model/connection switch the single retreat can therefore target a span never accepted by the current summarizer and fail open unnecessarily. Derive the boundary from the latest response on the same effective route and add a mixed-route session test.
bcd8a5c to
378f987
Compare
|
You are right, and the claim in the code comment was stronger than what the code proved. Fixed on
Two regressions:
Still no schema or epoch change: the boundary is read from the ledger and its run headers. |
…pted When the summarizer's own provider rejects a fold as too large, the planner halved the covered range and tried again. Halving is a guess in both directions: it can discard verbatim history the summarizer would have taken, and it can still be too large, paying another round trip to find out. There is a boundary that needs no guessing. The last accepted request's input covered everything before the newest model reply began; that span was accepted by this model on this connection, so it is provably within the provider's capacity. The fold retreats to it once. A rejection of that span too is the provider saying this fold cannot be made, and the fold fails open with the summarizer's own reason rather than a span-selection one. The boundary is read from the ledger rather than persisted: the newest model reply is the end of the proven span whether or not it sits at the tail, so a turn's first request finds the previous turn's reply. A ledger with no model reply has nothing proven and gets no retreat, because inventing a boundary is the guess this change removes. Refs apache#4559, apache#4634 Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
378f987 to
a45346c
Compare
|
Nice change, and the proven boundary is a much better idea than halving. One thing after the merge, only because a release is close: the retreat is wired into one of the two production call sites, and the one it misses is the one The five added lines land at That path did retreat before. Its coverage gate is just Four entries reach it: CLI The fix is the two lines already at ...(input.runtimeContextRunHeaders ? { runHeaders: input.runtimeContextRunHeaders } : {}),
acceptedRoute: {
modelId: this.input.modelId,
...(this.targetConnectionId !== undefined ? { connectionId: this.targetConnectionId } : {}),
},Worth one test driving Two things I checked and they are fine. The Static read of AI-assisted review: drafted with Maka. I verified the call sites, the gates and the entry points myself. 简体中文改得挺好,用被证明过的边界替代折半是更对的思路。合并之后才提一句,只因为发版临近:退避接到了两个生产调用点里的一个,而漏掉的那个正是 加的五行落在 这条路改动前是有退避的。它的覆盖闸只是 四个入口会走到:CLI 修法就是 建议补一条驱动 两件我核过、没问题的事。 |
|
Confirmed, and it is a regression this PR introduced rather than a gap it left: the standalone site retreated before, because its coverage gate admitted every halving step. Fixed in #4671, with the two values that were already in hand there. Your point about the test is the one that matters most: the planner tests hand I also confirmed the mid-turn observation. Thank you for tracing the four entry points and for saying which parts you had checked and which were a static read. |
|
Severities for the comment above, which I should have included with it. P1 — the standalone retreat is unwired. Normal supported operation: any P2 — the new planner tests prove no production obligation. They pair P2 — P3 — mid-turn cannot reach a retreat either, since No finding on the The P1 is the one worth a decision before the cut: land the two lines, or revert this for the release. 简体中文上面那条评论的分级,应该跟着一起给的。 P1 —— standalone 的退避没接线。 正常支持路径:任何 P2 —— 新加的 planner 测试没有证明任何生产义务。 它们把 P2 —— P3 —— mid-turn 同样到不了退避,因为 无发现: 发版前需要拍板的只有那条 P1:补上两行,或者这次先 revert。 |
Astro-Han
left a comment
There was a problem hiding this comment.
Anchoring the graded points to the lines they belong to, since a follow-up PR is the likely shape here.
| orderedEvents, | ||
| headAnchor: { runtimeEventId: state.headAnchor.id, turnId }, | ||
| runHeaders: state.priorRunHeaders, | ||
| acceptedRoute: { |
There was a problem hiding this comment.
P1 These two lines are the whole wiring, and they only reach the mid-turn and pre-turn call site. The sibling call at :365 (phase: 'standalone') passes neither, so acceptedInputBoundary returns undefined on its first line and the first input_too_large fails open there, where halving previously walked down until a span was accepted (that path's coverage gate is just coveredCount > 0, so every halving step passed it).
Four entries reach it: CLI /compact, Desktop sessions:compact, supervisor-wake sub-agent compaction (agent-graph-supervisor-wake.ts:111), and the pre-turn fallback at ai-sdk-backend.ts:3426. The first three end failed with a context_compaction_failed_open note; the fourth sends the oversized history and the turn dies with context_overflow. No flag or fallback. And the standalone first attempt covers the entire prior session (reserveTailEvents: 0), the span most likely to be rejected, so it is the ordinary long-session case.
Both values are in hand at :365, where input.runtimeContextRunHeaders is already used ten lines below:
...(input.runtimeContextRunHeaders ? { runHeaders: input.runtimeContextRunHeaders } : {}),
acceptedRoute: {
modelId: this.input.modelId,
...(this.targetConnectionId !== undefined ? { connectionId: this.targetConnectionId } : {}),
},P3, separately: state.priorRunHeaders excludes the current turn by construction (prior-run-context.ts:66-71 filters run.turnId !== currentTurnId), so no current-turn reply can be on route and the proven index always lands at or below headAnchorIndex, while the mid_turn gate wants strictly above it. So mid-turn cannot reach a retreat either. That gate is unchanged from before and halving usually undershot it too, so this is a pre-existing limit rather than something this PR broke. A follow-up, not a fix under time pressure. If you do pursue it, the current run is the route by construction, so a synthetic header for input.origin.runId or a currentRunId field treated as on-route would make the comment at history-compaction.ts:239-246 true.
| input.runHeaders ?? [], | ||
| input.acceptedRoute, | ||
| ); | ||
| if (proven === undefined || proven >= boundary.coveredCount) { |
There was a problem hiding this comment.
Context for the P1 above. This early exit is correct and it does report the summarizer's own reason, which was the point. It is just reached unconditionally on the standalone path, because acceptedInputBoundary returns undefined whenever acceptedRoute is absent and :365 never passes one.
| let attempts = 0; | ||
| const retreated = await planHistoryCompaction( | ||
| planInput({ | ||
| phase: 'standalone', |
There was a problem hiding this comment.
P2 These cases pair phase: 'standalone' with a hand-supplied acceptedRoute, and no production caller produces that combination: the standalone site passes neither field, and the site that passes them is mid-turn or pre-turn. So they prove the boundary arithmetic but not one production obligation, and they would stay green if both call sites were deleted. That is what let the wiring gap through.
One case driving AiSdkCompaction.compactHistory rather than planHistoryCompaction directly, asserting a second summarizer call after one input_too_large, closes it. The overflow-reactive-recovery.test.ts assertion is the only production-wired one today, and it covers the one phase where the wiring happens to work.
| owner, immutable request snapshots remain enforced at AgentRun acceptance and backend dispatch, | ||
| and SessionEvent-to-RuntimeEvent conversion remains a pure mapper. | ||
| - Retired the Task Ledger domain: SessionTodo is now the sole authority for in-session work items, and the operational-state schema drops the `workflow_task_ledger_events` table on first open. **Unfinished Tasks are not migrated and are permanently deleted.** This affects workspaces last opened by `v0.1.0` through `v0.1.11`, `cli-v0.1.0-beta.1`, `v0.2.0-incubating-rc1`, or a `v0.2.0-dev` build; those releases wrote Tasks to a table that no shipped build ever bridged into SessionTodo. Before opening such a workspace with this build, finish or export the Tasks you still need, or copy the workspace's `runtime.sqlite` aside — the migration removes the only live copy, so afterwards recovery requires a backup made in advance. | ||
| - A compaction rejected as too large for the summarizer's own window now retreats to the span the last accepted request's input covered, instead of halving the covered range. That span is the newest reply this route produced, found through the run headers, so it was accepted by this model on this connection and is provably within capacity; halving can overshoot (discarding verbatim history for nothing) or undershoot (paying another round trip), and a span another route accepted proves nothing at all. One retreat, then the fold fails open and the provider decides. |
There was a problem hiding this comment.
P2 This is true for step-0 recovery and not for /compact, supervisor-wake compaction, or the pre-turn fallback, where there are zero retreats. Land the wiring rather than rewording the line, since rewording would document the gap.
…eat (#4671) #4667 wired the proven-boundary retreat into the mid-turn and pre-turn call site and missed the standalone one, which is the site manual compaction uses. Without `runHeaders` and `acceptedRoute`, `acceptedInputBoundary` returns nothing on its first line, so the first `input_too_large` fails open. That path retreated before: its coverage gate admitted every halving step, so the loop walked down until a span was accepted. Four entries reach it: CLI `/compact`, Desktop `sessions:compact`, sub-agent compaction from supervisor wake, and the pre-turn fallback. Its first attempt covers the whole prior session with no reserved tail, which is the span most likely to be rejected, so the regression landed on the ordinary long session rather than an edge. The first three entries reported failure with a `context_compaction_failed_open` note; the fourth sent the oversized history and the turn died with `context_overflow`. The fix passes the same two values the other call site already passes, so there is one rule and two call sites rather than two rules. The test drives `compactHistory` rather than the planner: the planner tests hand the route in directly, so they would have stayed green with both call sites deleted, which is exactly how this got through. Mid-turn still cannot reach a retreat, because `priorRunHeaders` excludes the current turn so the proven index lands at or below `headAnchorIndex` while the gate wants it above. That is a pre-existing limit rather than something #4667 changed, and it needs its own change to the gate, so it is not in this PR. No protocol or schema change. Refs #4559, #4667 Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
Summary
PR 2 of the series on #4559, following the merged #4653. It is the compaction module's failure path and nothing else: 4 files, no protocol change.
When the summarizer's own provider rejects a fold as too large, the planner halved the covered range and tried again. Halving is a guess in both directions. It can overshoot, discarding verbatim history the summarizer would have accepted, and it can undershoot, paying another provider round trip to find that out. The loop then exited through span selection, so the diagnostic reported a span problem for what was a provider verdict.
There is a boundary that needs no guessing. The last request this route had accepted covered everything before the newest reply this route produced. The run headers name that reply, so the span was accepted by this model on this connection and is provably within the provider's capacity; a span some other model accepted proves nothing about this summarizer's window. The fold retreats to it once; a rejection of that span too is the provider saying this fold cannot be made, and the fold fails open with the summarizer's own reason.
The boundary is read from the ledger and its run headers rather than persisted, so there is no schema or epoch change. The newest model reply ends the proven span whether or not it sits at the tail: at a turn's first request the newest events are the user's message and its tool results, and the span still ends where the previous turn's reply began.
Refs #4559, #4634
What the retreat leaves behind, and for how long
The retreat keeps the newest reply out of the fold, so that reply stays in the request as raw text. It does not stay there: the next fold covers it, rolling the checkpoint forward, because by then a newer reply ends the proven span. The test "a later fold rolls over the reply the retreat left verbatim" pins that, and it bounds the leftover to one send.
I had planned a watermark here — fold that reply separately when it exceeds 24,000 tokens — and this measurement is why it is not in this PR. Its whole benefit is inside the one send where the leftover is large enough to keep the request over the line, and its cost is a second summarizer call and a second checkpoint write inside a transaction that writes one. If a session is found where that single send matters, it is worth revisiting with the evidence; on the current evidence it is complexity for a case the next fold already resolves.
Also not here: "compact and retry" for an unrecognised rejection (#4623).
Verification
Every local gate clean. Runtime suites: history compaction 21/21, overflow recovery 50/50, mid-turn capacity 73/73, checkpoint and summarizer suites unchanged and green.
runtime-hostprotocol and composition 28/28 (that suite times out under parallel load on my machine and passes on its own; CI runs it serially). The epoch guard confirms no protocol change against the base.Self-review
refs.stepIdto find the newest reply. Tests showed that field is only set on function-call events, so a plain text reply left no boundary and the retreat would have silently never fired — worse than halving. The role-based rule replaced it.step-0 overflow recovery gates reasoning on retry and durable reloadpreviously relied on two halving retreats to keep the reasoning tail out of the fold. It now rejects once and the proven boundary leaves that tail verbatim, so the reasoning-gating assertions it exists for are unchanged.AI use
Select exactly one:
Tool(s) and scope: Claude Code — implementation; reviewed and verified by the author.
Checklist
Does this PR entail a change in behavior?
https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J