fix(runtime): retry mid-turn compaction after a no_safe_completed_span miss - #5792
Colafornia wants to merge 4 commits into
Conversation
…n miss state.summarizerFailure latched on every fail-open reason, including no_safe_completed_span — an outcome that never reaches the summarizer and only describes the event pool at that step. Once set, it short-circuited every later attempt for the rest of the turn, including the reactive overflow recovery that could have folded the grown ledger, so the turn could die on input capacity. Latch only outcomes that actually invoked the summarizer (provider_error, malformed_*, output_length, input_too_large, empty summary): the bounded-retry behavior from apache#4634 is unchanged, while a pool that grows a completed tool pair is re-evaluated on the next attempt. Fixes apache#5790 Generated-by: Devin Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
jackwener
left a comment
There was a problem hiding this comment.
Reviewed head 3ca4ea16cab22445a51b4108ab3c3cd9a54de7a2. The diagnosis is right: a plain no_safe_completed_span never reaches the summarizer, so it should not spend the Turn-wide latch. plan.reason has only two fail-open values (summarizer_failed, no_safe_completed_span), so the new condition is exhaustive. 1×P2 (inline).
P2: after the one-step retreat, no_safe_completed_span can follow a real summarizer call, and the latch no longer catches it.
- In
planHistoryCompaction(history-compaction.ts, around 380-395), a summarizer rejection with a proven accepted-input boundary setsmaxCoveredCount = provenandcontinues. - If the smaller window then has no safe coverage, the same call returns
{ reason: 'no_safe_completed_span' }(:316, or:464when the loop runs out) even though the summarizer was already dispatched and failed. - With this change that outcome does not latch. So every later step re-plans the same prefix, repeats the same doomed summarizer call, and retreats into the same miss. That is the repeated-summarizer-call loop #4634 bounded, and a slow provider that fails makes each iteration expensive.
- Suggested fix: have the planner report that it invoked the summarizer, and latch on that rather than on
reason. Alternatively, returnsummarizer_failed(with the originaldiagnosticReason) when the retreat itself finds no safe span. Please add a test: aninput_too_largerejection whose proven boundary leaves no safe coverage, then a second step, asserting that no second summarizer call is made.
The new tests cover the proactive trigger and the reactive-recovery path for the plain miss, and CI test is green. Not run locally: the suites. The P2 comes from reading the code paths above; I did not reproduce it end to end.
When an input_too_large rejection retreats to the proven boundary and the smaller window has no safe coverage, the summarizer was already dispatched and failed, so the result must not look like a pool that never reached it. Return summarizer_failed with input_too_large for that miss so the turn latch catches it instead of re-dispatching the same doomed call every step. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…overage The proven boundary a retreat targets is a prior-run reply, which always precedes the head anchor that mid_turn coverage must cover past — so the retreated cut could never fold and the miss disguised the summarizer failure. Mid_turn now reports input_too_large directly; the retreat and its post-retreat miss handling remain only where they can succeed (standalone/pre_turn). Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 826b419. The change lets a structural no_safe_completed_span retry when the event pool grows, while retaining the Turn-wide latch after a real summarizer failure. A later commit removes all mid-turn input_too_large retreats. I found one P2 regression in that removal, inline.
Node 24 build:test and the three focused compaction suites passed (160 tests). I also exercised the planner with a prior-run, same-turn model reply after the head anchor: it made one summarizer call and returned summarizer_failed without testing the proven smaller prefix. Fresh main 71bc045 merges cleanly and git diff --check passes. Current-head hosted test was queued at review time. I did not run the full suite or a real provider/handoff process; the concrete handoff path and synthetic rejection establish the reachable boundary, not its production frequency. Please address the inline regression before merge. This is not merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
jackwener
left a comment
There was a problem hiding this comment.
Approving at 826b41976134c2a2a80840d411a78993b88df1f0. The P2 from review 5339998409 is closed, and no P0–P2 remain.
a6b6c2227: once a retreat has happened (retreated = true), a smaller window with no safe coverage, or a loop that runs out, now returnssummarizer_failed/input_too_largeinstead ofno_safe_completed_span. The Turn latch therefore catches a failure the summarizer actually produced. A plain miss that never reached the summarizer still returnsno_safe_completed_span, which is #5790's fix.826b41976:mid_turnno longer retreats. This checks out: the mid-turn planner receivesstate.priorInvocations, soacceptedInputBoundarycan only land on a prior-run reply, which precedes the head anchor thatmid_turncoverage must pass. The retreated cut could never fold.input_too_largenow fails open assummarizer_faileddirectly and latches.standaloneandpre_turnkeep the retreat.- New tests cover the retreat-then-miss case in the planner and the mid-turn backend path.
CI test is green on this head.
Not run locally: the suites.
Handoff replay keeps the same logical turn's predecessor events and run record in the pool, so an on-route reply after the head anchor can prove a retreat boundary. Skipping the retreat for all mid_turn folds dropped that reachable fold and spent the Turn latch; restore the retreat and cover the handoff-shaped boundary in the planner tests. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Reviewed commit fa32a3f. The new delta removes the unconditional mid-turn failure on input_too_large (packages/runtime/src/history-compaction.ts:389-405) and restores the one-step proven-boundary retreat. The added planner regression covers a handoff predecessor reply after the current Turn's head anchor, asserting a second, smaller coverage attempt; the retreat-then-no-safe-span case still reports summarizer_failed, so the Turn latch bounds repeated calls. The earlier P2 is addressed. No new substantiated P0-P3 in the inspected change. Node 24 build:test and three focused compaction files passed (161 tests); current-head hosted test is green. Fresh main 2f32205 merges cleanly and git diff --check passes. I did not run the entire local suite or a real provider/handoff process. This is not merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Summary
Fixes #5790.
A mid-turn compaction attempt that finds nothing safe to fold (
no_safe_completed_span) never reaches the summarizer, yet it latchedstate.summarizerFailurefor the rest of the turn. Every later attempt short-circuited before re-reading the ledger — including the reactive recovery meant to rescue a real providercontext_overflow— so a turn could die on input capacity even though a grown pool would have folded.The latch now covers only outcomes that actually invoked the summarizer (
provider_error,malformed_*,output_length,input_too_large, empty summary), preserving the bounded-retry behavior added for #4634.no_safe_completed_spanis a property of the event pool at that step, so the next attempt re-reads the ledger and can fold once a completed tool pair appears.Verification
node --teston the rebuilt@maka/runtimedist:mid-turn-capacity-backend.test.js,overflow-reactive-recovery.test.js,history-compaction.test.js— 157 tests pass.npm run lintandnpm run format:checkpass repo-wide;tscpasses vianpm --workspace @maka/runtime run build.npm testmatrix across all workspaces, knip, and the desktop e2e budget check.AI use
Tool(s) and scope: Devin authored the fix and the tests.
Checklist
Does this PR entail a change in behavior?
中文版本
摘要
修复 #5790。
一次轮中压缩尝试如果找不到可安全折叠的区间(
no_safe_completed_span),并没有发起任何 summarizer 调用,却把state.summarizerFailure置位到本轮结束。之后的每次尝试都在重读账本前被短路——包括本应用于抢救真实 providercontext_overflow的响应式恢复——因此即使增长后的事件池本来可以折叠,turn 仍可能死于输入超限。现在只有真正调用过 summarizer 的结果才置位(
provider_error、malformed_*、output_length、input_too_large、空摘要),#4634 引入的有界重试行为不变。no_safe_completed_span只是该步事件池的属性,下一次尝试会重读账本,在出现完整的工具调用对后即可折叠。验证
@maka/runtimedist 上运行node --test:mid-turn-capacity-backend.test.js、overflow-reactive-recovery.test.js、history-compaction.test.js—— 157 个测试通过。npm run lint与npm run format:check仓库级通过;npm --workspace @maka/runtime run build的tsc通过。npm test矩阵、knip、desktop e2e 预算检查。