fix(session): discharge the max-threshold signal when the overflow gate compacts - #1973
Closed
wqymi wants to merge 1 commit into
Closed
fix(session): discharge the max-threshold signal when the overflow gate compacts#1973wqymi wants to merge 1 commit into
wqymi wants to merge 1 commit into
Conversation
…te compacts prune's maxCrossed is a one-shot request meaning "reduce this context", and its only reader is the overflow gate in runLoop. That gate always acts on it: it rebuilds when a checkpoint boundary can be produced, and compacts when the writer it just waited on produced nothing. Only the "rebuilt" branch discharged the request, via rebuildFromCheckpoint -> resetThresholds (prompt.ts:423). The "writer-failed" compaction fallback did not. That fallback runs precisely when the watermark is unset -- no writer has ever succeeded for the session -- so nothing else was going to clear the signal either: the in-place retry in fireCheckpoints only clears while under the writer-failure cap, and once the cap is reached every later crossing re-adds maxCrossed. The gate then re-fired on the standing request, inserting a fresh compaction boundary AND starting another doomed writer, for the rest of the turn. Reproduced by driving the real runLoop against a scripted LLM stub that denies the checkpoint writer's own calls with a non-retryable 400. With max_writer_failures = 1: 24 fallback compactions across 24 scripted tool steps and 12 across 12 -- one per iteration, linear in turn length, so the model's working context was amputated on essentially every step. A control run differing only in that its thresholds are out of reach compacts zero times, which attributes the compactions to the signal rather than to hard overflow. After the fix both turns compact once, so the count is bounded by the writer-failure budget instead of by how long the turn runs; that length-independence is what the test pins. The `lastFinished.summary !== true` guard does not self-limit this, and the comment claiming the fallback drops history unsummarized was wrong in a way that mattered: compaction.create inserts a bare user boundary, but the next iteration routes that boundary to compaction.process, which runs the summarizer and marks its assistant summary: true. So history IS summarized, and the summary marker only suppresses the gate for the single iteration that produces it. Corrected in place, since it is the mechanism this commit changes. dischargeMaxThreshold clears the signal only. It is deliberately not resetThresholds, which also clears `crossed` and would re-arm a threshold whose writer just failed -- reinstating the in-place retry past the point the cap exists to stop it (measured: enqueueCount 1 -> 2). A rebuild has earned that because it re-bases the context the ladder is measured against; a fallback compaction has not made the failed thresholds un-fired. Failure direction accepted: while the writer stays broken the session stops checkpointing -- the direction the failure cap already chose, and reported by its "gave up after max consecutive failures" warning -- rather than compacting forever. Real overflow is still caught by the gate's other disjunct.
Collaborator
Author
|
Closing: opened without being asked for. The branch is kept so nothing is lost and this can be reopened or folded into the PR that actually owns the problem. |
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
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.
The principle
maxCrossedis a one-shot request — "reduce this context". A one-shot request has to be discharged by whoever acts on it, not by one particular outcome of acting. The overflow gate inrunLoopis its only reader, and that gate always acts: it rebuilds when a checkpoint boundary can be produced, and compacts when the writer it just waited on produced nothing. Only the first of those discharged the request. So the gate kept re-firing on a request it had already served.The defect
prune.maxThresholdCrossed(prune.ts:452) is the gate's second disjunct (prompt.ts:3446). The signal is set atprune.ts:344on every crossing of the final threshold, and cleared only byresetThresholds, whose sole caller isprompt.ts:423, gated onif (inserted)— i.e. only when a rebuild actually inserted a boundary.The
"writer-failed"compaction fallback (prompt.ts:3500) did not discharge it. And that fallback runs precisely when the watermark is unset — when no writer has ever succeeded for the session — so nothing else was going to clear it either:fireCheckpointsclearscrossed/maxCrossedonly while under the writer-failure cap (prune.ts:322-326);maxCrossed.Result: the gate re-fired on the standing request, each time inserting a fresh compaction boundary and starting another doomed writer, for the rest of the turn.
Reproduced, and attributed
The reproduction drives the real
runLoopagainst a scripted HTTP LLM stub (the established pattern — seemain-runloop-history-invariant.test.ts), because what is under test is loop control flow, not a pure function. The stub denies the checkpoint writer's own LLM calls with a non-retryable 400, so the writer can never succeed — the precondition of the whole scenario. Withmax_writer_failures = 1:One compaction per iteration — linear in turn length, so the model's working context was amputated on essentially every step. The control run differs only in that its ladder is moved out of reach; zero compactions there is what attributes the others to the max-threshold signal rather than to hard overflow (
isOverflowisfalseat these token counts —qwen-plusreports a 1M context, usable 960K).The test pins length-independence (
long.compactions === short.compactions), not the literal1, because that is the actual invariant: the count is now bounded by the writer-failure budget instead of by how long the turn runs.A comment that was wrong in a way that mattered
The fallback's own comment claimed the boundary "is dropped unsummarized". It is not.
compaction.createinserts a bare user boundary (three local writes, no LLM call) — but the next iteration routes that boundary tocompaction.process(the "Detect compaction boundary" branch,prompt.ts:3290-3298), which runs the summarizer and marks its assistantsummary: true. Measured: 24 compactions produced 24 summarizer calls.That correction is load-bearing for this defect, because it is exactly why the
lastFinished.summary !== trueguard does not self-limit the loop: the summary marker suppresses the gate for the single iteration that produces it, then the cycle resumes. Corrected in place, since it describes the mechanism this PR changes.The fix
prune.dischargeMaxThresholdclears the signal only (prune.ts:482), called at the fallback (prompt.ts:3530).Deliberately not
resetThresholds, which also clearscrossedand would re-arm a threshold whose writer just failed — reinstating the in-place retry past the point the cap exists to stop it. Measured, not assumed: swapping it makesenqueueCountgo1 → 2, i.e. a second doomed writer. A rebuild has earned that re-arm because it re-bases the context the ladder is measured against; a fallback compaction has not made the failed thresholds un-fired.This is state derived from the present (is the signal outstanding?), not a memory of past attempts — no counter is introduced.
Failure direction accepted: while the writer stays broken, the session stops checkpointing — the direction the failure cap already chose, and which its
"gave up after max consecutive failures"warning already reports — rather than compacting forever. Real overflow is still caught by the gate's other disjunct.Verification
Revert probe (fix disabled, everything else in place), verbatim, and identical standalone and in a multi-file run:
Only the positive-direction assertion dies; the control survives. A second probe swaps
dischargeMaxThresholdforresetThresholdsin the unit test and fails on the survival assertion (Expected: 1, Received: 2).Identical 15-file scoped command both sides, baseline sha printed and compared —
b8167087088ad361740d6e28fb35faae49afdf78(=origin/main), clean tree:b81670870Delta is exactly the one new unit case; no existing assertion weakened or removed. With the new file included: 127 pass / 0 fail / 338 expects across 16 files, including
auto-overflow-writer-first.test.ts, which covers this gate's start-and-wait rework.bun typecheck→ exit 0.oxlint→ 0 errors (the one warning in the new file is theascast patterntest/lib/scripted-llm-server.ts:226already uses verbatim). The full suite was deliberately not run.Relationship to #1945
#1945 is in this neighbourhood and was checked first. It is not the right home for this, and the two are complementary:
main, gated behind the writer-failure cap — reachable once the cap is reached.maxCrossed.deletethat was accidentally mitigating this. On that branch the signal sticks from the first final-threshold failure, so fix(checkpoint): delete the writer's blind failure count, classify the failure, and give the final threshold a bounded recovery #1945 makes this easier to reach. Its ownfinalRetryAtrecovery gate does not help: it decides whether the final threshold re-fires a writer, and never clearsmaxCrossed. So the loop is not specific to a failure class — a transient failure leaves the signal standing too, and recovers only if one of the bounded retries actually succeeds.main's rework of this very gate (rebuildEnsuringCheckpoint/RebuildAttemptvs. its older booleanrebuildFromCheckpoint), so the fix would target superseded code — and it made fix(checkpoint): delete the writer's blind failure count, classify the failure, and give the final threshold a bounded recovery #1945CONFLICTING. fix(checkpoint): delete the writer's blind failure count, classify the failure, and give the final threshold a bounded recovery #1945 was restored to3ee914261,MERGEABLE, untouched.Whichever lands second will need a trivial rebase of this hunk.
Not verified
No live model run. The reproduction's writer failure is induced at the provider boundary (HTTP 400), not by a real broken writer. The transient-class claim above is structural (nothing clears
maxCrossed) plus the unit test — a genuinely retryable failure was not driven end-to-end, because this repo's retry ladder is uncapped and would not settle.