-
Notifications
You must be signed in to change notification settings - Fork 434
fix(runtime): deduplicate in-flight context compaction #4624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4464b93
d91ea2d
c31abf9
30f405a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -215,7 +215,17 @@ export class AiSdkCompaction { | |
| providerReasoningReplayEventIds: ReadonlySet<string>, | ||
| ) => Promise<ModelMessage[]>; | ||
| private readonly canReplayProviderNative: (plan: RuntimeEventModelReplayPlan) => boolean; | ||
| private historyCompactAbortController: AbortController | null = null; | ||
| private readonly historyCompactAbortControllers = new Set<AbortController>(); | ||
| /** | ||
| * Exact duplicate summary inputs share one physical provider call. The map | ||
| * lives at the summarizer boundary, where the existing effective-history | ||
| * fingerprint already describes the request that spends provider budget. | ||
| * Each caller still completes its own checkpoint/turn bookkeeping. | ||
| */ | ||
| private readonly inFlightHistorySummaries = new Map< | ||
| string, | ||
| Promise<string | HistoryCompactProviderState | undefined> | ||
| >(); | ||
| /** | ||
| * Session-scoped circuit for exact malformed compaction inputs. A retry or | ||
| * regeneration on the same backend must not dispatch the same doomed call; | ||
|
|
@@ -287,15 +297,22 @@ export class AiSdkCompaction { | |
|
|
||
| /** Abort an in-flight manual history compaction (called by AiSdkBackend.stop). */ | ||
| public abortHistoryCompact(): void { | ||
| this.historyCompactAbortController?.abort(); | ||
| for (const controller of this.historyCompactAbortControllers) controller.abort(); | ||
| } | ||
|
|
||
| public async compactHistory( | ||
| input: Omit<BackendCompactHistoryInput, 'runId'> & { runId: string | undefined }, | ||
| automaticMemoryBoundary?: HistoryCompactMemoryExtractionBoundary, | ||
| ): Promise<AiSdkCompactHistoryResult> { | ||
| return this.compactHistoryOnce(input, automaticMemoryBoundary); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: |
||
| } | ||
|
|
||
| private async compactHistoryOnce( | ||
| input: Omit<BackendCompactHistoryInput, 'runId'> & { runId: string | undefined }, | ||
| automaticMemoryBoundary?: HistoryCompactMemoryExtractionBoundary, | ||
| ): Promise<AiSdkCompactHistoryResult> { | ||
| const historyCompactAbortController = new AbortController(); | ||
| this.historyCompactAbortController = historyCompactAbortController; | ||
| this.historyCompactAbortControllers.add(historyCompactAbortController); | ||
| try { | ||
| const policy = this.input.contextBudget; | ||
| const summarizer = this.input.summarizeHistoryCompact; | ||
|
|
@@ -399,7 +416,11 @@ export class AiSdkCompaction { | |
| source: { | ||
| foldedRuntimeEvents: [...coveredRuntimeEvents], | ||
| ...(input.runtimeContextRunHeaders | ||
| ? { runHeaders: input.runtimeContextRunHeaders } | ||
| ? { | ||
| runHeaders: input.runtimeContextRunHeaders.filter((run) => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: this filter and the |
||
| coveredRuntimeEvents.some((event) => event.runId === run.runId), | ||
| ), | ||
| } | ||
| : {}), | ||
| }, | ||
| newlyFoldedRuntimeEvents: [...newlyFoldedRuntimeEvents], | ||
|
|
@@ -501,9 +522,7 @@ export class AiSdkCompaction { | |
| }), | ||
| }; | ||
| } finally { | ||
| if (this.historyCompactAbortController === historyCompactAbortController) { | ||
| this.historyCompactAbortController = null; | ||
| } | ||
| this.historyCompactAbortControllers.delete(historyCompactAbortController); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -546,8 +565,13 @@ export class AiSdkCompaction { | |
| const priorFailure = this.malformedSummaryFailures.get(fingerprint); | ||
| if (priorFailure) throw new HistoryCompactSummarizerError(priorFailure); | ||
|
|
||
| const existing = this.inFlightHistorySummaries.get(fingerprint); | ||
| if (existing) return existing; | ||
|
|
||
| const pending = Promise.resolve().then(() => summarizer(input)); | ||
| this.inFlightHistorySummaries.set(fingerprint, pending); | ||
| try { | ||
| return await Promise.resolve(summarizer(input)); | ||
| return await pending; | ||
| } catch (error) { | ||
| if ( | ||
| error instanceof HistoryCompactSummarizerError && | ||
|
|
@@ -562,6 +586,10 @@ export class AiSdkCompaction { | |
| } | ||
| } | ||
| throw error; | ||
| } finally { | ||
| if (this.inFlightHistorySummaries.get(fingerprint) === pending) { | ||
| this.inFlightHistorySummaries.delete(fingerprint); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,9 +86,17 @@ export class HistoryCompactCheckpointCoordinator { | |
| .catch(() => {}) | ||
| .then(async () => { | ||
| const durableCheckpoint = await this.load(sessionId); | ||
| if (!canReplaceHistoryCompactCheckpoint(durableCheckpoint, checkpoint)) { | ||
| const sameEffectiveCheckpoint = hasSameEffectiveCoverage(durableCheckpoint, checkpoint); | ||
| if ( | ||
| !sameEffectiveCheckpoint && | ||
| !canReplaceHistoryCompactCheckpoint(durableCheckpoint, checkpoint) | ||
| ) { | ||
| throw new Error('History compact checkpoint was superseded before persistence'); | ||
| } | ||
| // A concurrent caller may have received the same effective checkpoint | ||
| // from the summarizer coalescer. Keep its run-local ledger event too so | ||
| // the rider Turn retains provenance even though the session checkpoint | ||
| // itself is already current. | ||
| await run.recordHistoryCompactCheckpoint(checkpoint); | ||
| this.checkpoints.set(sessionId, checkpoint); | ||
| this.scheduleCleanup(sessionId, checkpoint); | ||
|
|
@@ -145,3 +153,35 @@ export class HistoryCompactCheckpointCoordinator { | |
| this.cleanups.set(sessionId, tracked); | ||
| } | ||
| } | ||
|
|
||
| function hasSameEffectiveCoverage( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Smaller fix that needs no new predicate: when the durable checkpoint already covers the rider's fold, return through the existing |
||
| current: HistoryCompactCheckpoint | undefined, | ||
| candidate: HistoryCompactCheckpoint, | ||
| ): boolean { | ||
| if (!current) return false; | ||
| const currentContent = checkpointContent(current); | ||
| const candidateContent = checkpointContent(candidate); | ||
| return ( | ||
| current.sessionId === candidate.sessionId && | ||
| current.version === candidate.version && | ||
| current.highWaterName === candidate.highWaterName && | ||
| current.phase === candidate.phase && | ||
| current.coverage.eventCount === candidate.coverage.eventCount && | ||
| current.coverage.turnCount === candidate.coverage.turnCount && | ||
| current.coverage.sourceDigest === candidate.coverage.sourceDigest && | ||
| current.coverage.through.runId === candidate.coverage.through.runId && | ||
| current.coverage.through.turnId === candidate.coverage.through.turnId && | ||
| current.coverage.through.runtimeEventId === candidate.coverage.through.runtimeEventId && | ||
| JSON.stringify(current.source) === JSON.stringify(candidate.source) && | ||
| JSON.stringify(current.headAnchor) === JSON.stringify(candidate.headAnchor) && | ||
| JSON.stringify(current.memoryExtractionBoundary) === | ||
| JSON.stringify(candidate.memoryExtractionBoundary) && | ||
| JSON.stringify(currentContent) === JSON.stringify(candidateContent) | ||
| ); | ||
| } | ||
|
|
||
| function checkpointContent(checkpoint: HistoryCompactCheckpoint): unknown { | ||
| return checkpoint.version === 2 | ||
| ? { summary: checkpoint.summary, summaryFormat: checkpoint.summaryFormat } | ||
| : { providerState: checkpoint.providerState }; | ||
| } | ||
There was a problem hiding this comment.
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.maxHistoryEstimatedTokenswas removed outright by #4559, so on the merge ref this iserror TS2353and the wholetestjob fails at Build. The rebase is not mechanical: the policy no longer carries a token ceiling at all, and this branch still readspolicy.maxHistoryEstimatedTokensinai-sdk-compaction.ts(:435,:468). After the rebase the test needs a different way to make compaction fire, throughpolicy.historyCompact.