Skip to content

fix(runtime): deduplicate in-flight context compaction - #4624

Open
testikun wants to merge 4 commits into
apache:mainfrom
testikun:codex/issue-4360-delegated
Open

fix(runtime): deduplicate in-flight context compaction#4624
testikun wants to merge 4 commits into
apache:mainfrom
testikun:codex/issue-4360-delegated

Conversation

@testikun

@testikun testikun commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • coalesce identical concurrent history compaction requests at the effective-history summarizer/provider-call boundary
  • preserve per-turn checkpoint and ledger attribution while sharing one physical summarizer call
  • keep the existing context-budget high-water trigger, provider overflow recovery, and fail-open/no-op behavior

Verification

  • npm --workspace @maka/runtime run build
  • node --test packages/runtime/dist/__tests__/ai-sdk-backend.test.js — 221 passed
  • npm 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 — passed
  • Full workspace and Host lifecycle suites were not run locally, per the requested test scope.

Refs #4360

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 3, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 hasActiveRunssession_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.

@testikun

testikun commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

已处理并重新提交。

  • 已将并发去重下沉到 effective-history summarizer/provider-call 边界。
  • 每个并发 Turn 保留自己的 checkpoint 和 ledger attribution。
  • 修正 run headers 与 fingerprint 的过滤一致性。
  • 并发测试现在在 Promise 完成后验证只发生一次 summarizer 调用,同时验证两次独立 checkpoint 记录。
  • 已将 issue 关联改为 Refs #4360,避免错误关闭原 issue。
  • Runtime 构建通过;针对性 ai-sdk-backend 单文件测试 221 项全部通过。
  • 按本地测试范围约束,未运行完整 Host 生命周期套件。

@testikun
testikun force-pushed the codex/issue-4360-delegated branch from d526b2a to 30f405a Compare September 3, 2026 14:39

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

now: monotonicClock(),
contextBudget: {
name: 'in-flight-dedup-test',
maxHistoryEstimatedTokens: 10_000,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: this is the line CI dies on. ContextBudgetPolicy.maxHistoryEstimatedTokens was removed outright by #4559, so on the merge ref this is error TS2353 and the whole test job fails at Build. The rebase is not mechanical: the policy no longer carries a token ceiling at all, and this branch still reads policy.maxHistoryEstimatedTokens in ai-sdk-compaction.ts (:435, :468). After the rebase the test needs a different way to make compaction fire, through policy.historyCompact.

}
}

function hasSameEffectiveCoverage(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: this is a second predicate for "may this checkpoint replace the current one", next to canReplaceHistoryCompactCheckpoint. Two costs. First, it hand-enumerates the checkpoint's fields, so any field added to HistoryCompactCheckpoint later is silently treated as not mattering. Second, it exists only because the two checkpoints are genuinely different: checkpointId hashes highWaterSeq, which defaults to max(now, maxEventTs) (history-compact-checkpoint.ts:293-309), so the rider's id differs from the one the first turn already reported in its complete diagnostic boundaryIds. Line 101 then flips the session pointer to the rider's checkpoint, and the next compaction chains previousCheckpointId to that one, orphaning the first id.

Smaller fix that needs no new predicate: when the durable checkpoint already covers the rider's fold, return through the existing already_compacted outcome with the durable checkpointId. One checkpoint, one id, and canReplaceHistoryCompactCheckpoint stays the only authority.

input: Omit<BackendCompactHistoryInput, 'runId'> & { runId: string | undefined },
automaticMemoryBoundary?: HistoryCompactMemoryExtractionBoundary,
): Promise<AiSdkCompactHistoryResult> {
return this.compactHistoryOnce(input, automaticMemoryBoundary);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: compactHistory is now a pure forward to compactHistoryOnce with no added behavior. Rename compactHistoryOnce back to compactHistory and drop the wrapper.

...(input.runtimeContextRunHeaders
? { runHeaders: input.runtimeContextRunHeaders }
? {
runHeaders: input.runtimeContextRunHeaders.filter((run) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: this filter and the foldedRunIds filter in summarizeWithFailureCircuit (:559-561) are the same rule over the same set, since foldedRuntimeEvents is coveredRuntimeEvents. Keeping it here is the right call, so the one in the fingerprint can go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants