fix(compaction): let the core route claim the idle warm summary - #862
Merged
Conversation
On a new prompt the ordering is deterministic rather than a race: `_enforceCompactionBeforeProvider` runs before `emitBeforeAgentStart`, so the core route always reaches compaction first. Its `session_before_compact` handler called `invalidateSpeculativeCompaction()` as its very first statement, aborting and discarding the summary the idle warm-up had already paid for, then summarizing again while the user waited. PR #853 fixed the extension route; the warm-up stayed wasted whenever core won. Claim the job for the core route instead. The claim detaches it synchronously before any await so exactly one route owns it, and deliberately leaves the controller unaborted, which is what keeps the finished work alive. It holds only for an automatic compaction with no custom instructions, the same model, a valid warm anchor, and a boundary equal to core's own preparation; every other case falls through to the existing fresh generation untouched. Real-CLI QA reproduces the defect: on origin/main the scenario scores 7/9 with `warm_consumed=0`, and 9/9 with `warm_consumed=1` here.
Owner
Author
|
The claim deliberately detaches the warm job without aborting it, because aborting is what used to throw the finished summarization away. That left every non-consuming exit from the handler holding a live request nobody would read: the SDK-native lane rejection, the per-turn cap, the circuit breaker, a missing model, and the OpenAI remote path all returned while the warm generation kept streaming. Before the claim existed, `invalidateSpeculativeCompaction` aborted it on those paths. Release the claim on each of those exits, restoring the previous abort semantics for every path that does not consume the result.
Releasing the claim at each early return covered the paths that return, but not the ones that throw. `runOpenAiRemoteCompaction` can raise while preparing auth or transforming the request, and the handler then unwound with the warm request still streaming and no reference left to it, because the claim had already detached it from `speculativeJob`. Hold the claim in a try/finally instead, aborting unless the result was marked consumed. That covers returns and throws with one rule, so the five explicit release calls are gone.
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.
Problem
senpi pre-generates a summary while your session sits idle so you never wait for compaction. PR #853 taught the extension route to reuse it. The core route still threw it away.
The ordering is deterministic, not a race — on a new prompt
_enforceCompactionBeforeProviderruns beforeemitBeforeAgentStart, so core always reaches compaction first, and its handler calledinvalidateSpeculativeCompaction()as its first statement. Measured with a file probe on the real CLI:So the warm-up was aborted and re-billed in exactly the case it exists for.
Fix
Claim the job for the core route instead of invalidating it:
await, so exactly one route can own it.WarmAnchorSnapshot, and a boundary equal to core's ownpreparation.firstKeptEntryId. Every other case falls through to the existing fresh generation, unchanged.Manual compaction keeps its own instructions, a landed boundary still forces regeneration, and a mismatched cut is refused — each covered by a test.
Verification
Real-CLI A/B (
core-route-warm-handoff-qa.mjs, RPC + mock provider, sandboxed HOME) — a genuine defect reproduction:3358d48ffwarm_consumedexpected 'fresh summary…' to contain 'warm summary…'→ GREEN; 4 cases incl. three safety paths.test/compaction/346 passed (344 before).speculative-budget-handoff,required-compaction-deterministic-fallback,warm-summary-anchor,stale-warm-blocking-repro,metadata-side-effects,lifecycle,idle-compaction,speculative-abort-cancellation,suite/compaction-race,suite/agent-session-compaction— 88 tests.npm run checkexit 0.Scope note
An
AgentSession-level test cannot drive this:shouldRunIdleCompactionrequires a persistent mode (tui/rpc/app-server) and the vitest harness runs inprint, so the warm-up never starts there. The end-to-end proof is the RPC QA scenario above, which runs in a real persistent mode.Summary by cubic
Reuses the idle warm summary in the core route so users don’t wait for a second compaction. Previously core always invalidated the speculative job and regenerated; now it claims the job when safe, returns its result, and reliably aborts any unused claim, including on thrown exits.
session_before_compactcallsclaimWarmSummaryForCoreRoute()before invalidation. The claim detaches the job synchronously, logswarm_consumedwithroute: "core-route"on success, and uses a try/finally to abort the controller unless the result is consumed (covers early returns and exceptions).preparation.firstKeptEntryId. Otherwise it regenerates.core-route-warm-handoff.test.ts. Adds a real-CLI QA scenario to verifywarm_consumedand that core does not regenerate.Written for commit 4c91dfd. Summary will update on new commits.