fix(runtime): deduplicate in-flight context compaction - #4624
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Before the code: I am not convinced this PR should exist in its current form, and I want to put that first because it changes what the useful next step is.
Why this PR?
Fixes #4360 is wrong. #4360 reports that manual compaction and context diagnostics are not exposed in the composer, and that compaction does not fire automatically at the high-water mark. This PR's own body says it will "preserve the existing context-budget high-water trigger", and the diff touches only ai-sdk-compaction.ts plus one test. Neither of the reporter's two problems is addressed, but merging this closes their issue. Please change it to Refs #4360 at minimum.
That leaves the question of what this PR does fix, and I could not find evidence that it happens. The two supported entries into compactHistory are the in-turn automatic path (ai-sdk-backend.ts:3426) and the manual path (runtime-kernel.ts:1040). The manual one is already refused by hasActiveRuns → session_busy (runtime-kernel.ts:965), and the host refuses a duplicate turn identity with operation_conflict (context-coordinator.ts:203). What remains is the window between that hasActiveRuns check and reserveGenerationRun, across one await readHeader. Narrow, and nothing in the PR shows it has ever been hit.
Even if it is hit, the consequence is bounded: one wasted summarizer call and a context_compaction_failed_open diagnostic. HistoryCompactCheckpointCoordinator already serializes checkpoint writes per session and rejects the superseded one, so no state is corrupted. Meanwhile the PR adds a resident per-session Map and a new fingerprint function and deletes nothing (the −1 is async being dropped from a signature).
So: is this worth having? I would say not as written. Either close it and reopen when there is a reproduction, or land the smaller version below. Both are better than merging a new resident authority for an unwitnessed race.
If it stays, the dedup belongs one layer down
summarizeWithFailureCircuit (ai-sdk-compaction.ts:557-574) already computes a sha256 fingerprint over connection, modelId, historyCompactRoute, contextBudget, inputBudget, previousCheckpoint, currentRunEventIds, sourceRunRoutes and the folded runtime events. That is the identity of one physical provider call, and it sits in the layer that spends the money. Coalescing there, keyed by that fingerprint, saves exactly what this PR says it wants to save, and historyCompactRequestKey plus the Map plus its re-derived filter(turnId) all stop being necessary.
Coalescing at the current layer also has a cost the fingerprint version does not. The rider turn receives {kind: 'compacted', checkpointId} and runtime-kernel.ts:1045-1075 emits token_usage and a complete carrying contextCompactionOutcome on its behalf, but only the first turn ran recorder(plan.checkpoint, input.turnId) (:492) and held the provider request tracker (:403-410). So the second run's ledger has no history_compact_checkpoint_recorded and no model call attempt for a compaction it just reported. Replay is fine, since the checkpoint is found per session, but turn-level provenance and usage attribution are not. The new test's assert.equal(recorded.length, 1) pins that as expected.
Two smaller things:
assert.equal(summarizeCalls, 1) runs in the same tick as the second compactHistory call, before it has awaited loadHistoryCompactCheckpoint and planHistoryCompaction, so it cannot have reached the counter yet. That assertion passes on main too. Moving it after await Promise.all([first, second]) makes it prove something.
The key filters runtimeContextRunHeaders down to folded run ids (:1667-1671), but :431-433 passes the unfiltered array to the summarizer. Two calls differing only in a header outside the folded prefix share a key while producing different summarizer input. Minor, but the key and the call it stands for come from two different derivations.
Next step
Fix the issue link first, that one is not optional. Then either attach a reproduction (which two entries, what ordering) or move the coalescing onto the existing fingerprint, where the diff gets smaller and the attribution problem disappears.
Evidence boundary: read at 6a3ad9a3 against base 9d4002b38, no build, no tests run, and I did not construct the race. Whether the window is reachable at all is inferred from the ordering of hasActiveRuns and reserveGenerationRun; if something above the host serializes per-session requests, it is not reachable and the finding is moot rather than smaller.
AI-assisted review: drafted with Maka.
d526b2a to
30f405a
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The five points from my last pass are addressed; the dedup now sits on the existing summarizer fingerprint, which also gets it the in-turn automatic path for free, and the Set<AbortController> fixes a controller that used to be dropped when two compactions overlapped. What blocks this now is the base, plus one new authority I would rather not merge.
P1: CI fails at the Build step, and the failing line is this PR's own new test. gh pr checks shows one test job dying with src/__tests__/ai-sdk-backend.test.ts(4472,9): error TS2353: 'maxHistoryEstimatedTokens' does not exist in type 'ContextBudgetPolicy'. #4559 removed that field from the policy entirely, so this is not a rename: the token-estimate fit decision it belonged to is gone ("whether a request fits is the provider's answer"), and policy.maxHistoryEstimatedTokens is still read in four places in the production code on this branch. Rebasing means rebuilding how the new test triggers compaction, against historyCompact rather than a token ceiling. Inline on the exact line.
P2: the summarizer coalescer shares one call's lifetime, not just its result. The fingerprint deliberately excludes abortSignal and providerRequestTracker, so the first caller owns the abort; a rider that nobody stopped can still see the shared promise reject and fail open, and its own historyCompactAbortController.signal.aborted check at :444 will not explain why. Evidence gap: I did not confirm a path that aborts one turn's compaction without aborting the others (stop() aborts every active scope, so it does not produce this), so if none exists this is P3 rather than P2. Either require the two callers' signals to be the same before sharing, or wrap the rider so only its own signal can fail it.
The rest is inline: one P2 on the new checkpoint predicate and two P3 simplifications.
Evidence boundary: read at 30f405a8 plus refs/pull/4624/merge and current origin/main (ab7b739260), CI log from run 33768133717. No build and no tests run locally, and I did not construct the race.
AI-assisted review: drafted with Maka.
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
3db6c4d to
6d0e2e2
Compare
|
Addressed the review findings on cb8ff3d.
Verification on the rebased head: Runtime typecheck, Biome, Runtime build, and the focused compaction tests passed (11 model-history tests, including the coalescing regression). |
Summary
Verification
apache/main(b39e8d36e) and resolved the compaction contract changesnpm --workspace @maka/core run buildnpm --workspace @maka/storage run buildnpm --workspace @maka/runtime run buildnode --test packages/runtime/dist/__tests__/ai-sdk-backend.test.js— 222 passednpm exec -- biome check packages/runtime/src/ai-sdk-compaction.ts packages/runtime/src/history-compact-checkpoint-coordinator.ts packages/runtime/src/__tests__/ai-sdk-backend.test.ts— passedRefs #4360