-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(core): guard oversized resumed history sends #4531
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
Changes from all commits
f2a899a
b25e964
a171a5d
7dbe38f
99e7b77
7541e55
89f35d1
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 |
|---|---|---|
|
|
@@ -106,6 +106,38 @@ function isCompressionFailureStatus(status: CompressionStatus): boolean { | |
| ); | ||
| } | ||
|
|
||
| function shouldStopAfterHardRescue( | ||
| shouldForceFromHard: boolean, | ||
| hardLimit: number, | ||
| localPromptTokensAfterCompression: number, | ||
| ): boolean { | ||
| return shouldForceFromHard && localPromptTokensAfterCompression >= hardLimit; | ||
| } | ||
|
|
||
| function getHardRescueFailureMessage( | ||
| effectiveTokens: number, | ||
| hardLimit: number, | ||
| compressionInfo: ChatCompressionInfo, | ||
| localPromptTokensAfterCompression: number, | ||
| ): string { | ||
| const compressionStatus = | ||
| CompressionStatus[compressionInfo.compressionStatus] ?? | ||
| String(compressionInfo.compressionStatus); | ||
| const tokenCount = | ||
| compressionInfo.compressionStatus === CompressionStatus.COMPRESSED | ||
| ? Math.max( | ||
| compressionInfo.newTokenCount, | ||
| localPromptTokensAfterCompression, | ||
| ) | ||
| : Math.max(effectiveTokens, localPromptTokensAfterCompression); | ||
| return ( | ||
| `Context is too large to send safely after automatic compression. ` + | ||
| `Estimated prompt tokens: ${tokenCount}; hard limit: ${hardLimit}; ` + | ||
| `compression status: ${compressionStatus}. ` + | ||
| `Start a new session or reduce the resumed history before continuing.` | ||
| ); | ||
| } | ||
|
|
||
| export enum StreamEventType { | ||
| /** A regular content chunk from the API. */ | ||
| CHUNK = 'chunk', | ||
|
|
@@ -157,6 +189,11 @@ interface TryCompressOptions { | |
| * `getHistory(true)` clone per send. (review #4168 R1.3 / R1.4) | ||
| */ | ||
| precomputedEffectiveTokens?: number; | ||
| /** | ||
| * Delay writing the compression checkpoint until the caller has run any | ||
| * post-compression guards that may roll the in-memory chat state back. | ||
| */ | ||
| deferChatCompressionRecord?: boolean; | ||
| } | ||
|
|
||
| const INVALID_CONTENT_RETRY_OPTIONS: ContentRetryOptions = { | ||
|
|
@@ -1325,9 +1362,11 @@ export class GeminiChat { | |
| * | ||
| * Returns the compression info regardless of outcome. On a successful | ||
| * compaction (`COMPRESSED`), this method has already mutated the chat's | ||
| * history, recorded the event to `chatRecordingService` (if wired), and | ||
| * updated both the per-chat token count and (when wired) the global | ||
| * telemetry singleton. | ||
| * history, recorded the event to `chatRecordingService` (if wired and | ||
| * unless `options.deferChatCompressionRecord` is set), and updated both | ||
| * the per-chat token count and (when wired) the global telemetry singleton. | ||
| * Deferred callers are responsible for recording after their own | ||
| * post-compression guards pass. | ||
| */ | ||
| async tryCompress( | ||
| promptId: string, | ||
|
|
@@ -1352,10 +1391,12 @@ export class GeminiChat { | |
| }); | ||
|
|
||
| if (info.compressionStatus === CompressionStatus.COMPRESSED && newHistory) { | ||
| this.chatRecordingService?.recordChatCompression({ | ||
| info, | ||
| compressedHistory: newHistory, | ||
| }); | ||
| if (!options?.deferChatCompressionRecord) { | ||
| this.chatRecordingService?.recordChatCompression({ | ||
| info, | ||
| compressedHistory: newHistory, | ||
| }); | ||
| } | ||
| this.setHistory(newHistory); | ||
| debugLogger.debug('[FILE_READ_CACHE] clear after auto tryCompress'); | ||
| this.config.getFileReadCache().clear(); | ||
|
|
@@ -1525,9 +1566,13 @@ export class GeminiChat { | |
| imageTokenEstimate, | ||
| ); | ||
| const shouldForceFromHard = effectiveTokens >= hard; | ||
| const historyBeforeHardRescue = shouldForceFromHard | ||
| ? this.getHistoryShallow() | ||
| : undefined; | ||
| const lastPromptTokenCountBeforeHardRescue = this.lastPromptTokenCount; | ||
| if (shouldForceFromHard) { | ||
| debugLogger.warn( | ||
| `[compaction] hard-tier rescue triggered: effectiveTokens=${effectiveTokens}, hard=${hard}, consecutiveFailures=${this.consecutiveFailures}.`, | ||
| `[compaction] hard-tier rescue triggered: prompt_id=${prompt_id}, effectiveTokens=${effectiveTokens}, hard=${hard}, consecutiveFailures=${this.consecutiveFailures}.`, | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -1539,6 +1584,7 @@ export class GeminiChat { | |
| { | ||
| pendingUserMessage: userContent, | ||
| precomputedEffectiveTokens: effectiveTokens, | ||
| deferChatCompressionRecord: shouldForceFromHard, | ||
| // Hard-rescue is force=true to bypass the cheap-gate breaker | ||
| // but it's an AUTOMATIC trigger. Explicit trigger='auto' tells | ||
| // the service to skip the manual-only orphan-strip that would | ||
|
|
@@ -1550,6 +1596,66 @@ export class GeminiChat { | |
| }, | ||
| ); | ||
|
|
||
| const localPromptTokensAfterCompression = shouldForceFromHard | ||
| ? estimatePromptTokens( | ||
| this.lastPromptTokenCount > 0 ? [] : this.getHistoryShallow(true), | ||
| userContent, | ||
| this.lastPromptTokenCount, | ||
| imageTokenEstimate, | ||
| ) | ||
| : 0; | ||
| if ( | ||
|
Collaborator
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. [Suggestion] When compression succeeds ( This is a design tradeoff — the original history was already too large to send — but it's worth documenting explicitly. Consider either:
— qwen3.7-max via Qwen Code /review |
||
| shouldStopAfterHardRescue( | ||
| shouldForceFromHard, | ||
| hard, | ||
| localPromptTokensAfterCompression, | ||
| ) | ||
| ) { | ||
| const message = getHardRescueFailureMessage( | ||
| effectiveTokens, | ||
| hard, | ||
| compressionInfo, | ||
| localPromptTokensAfterCompression, | ||
| ); | ||
| if ( | ||
| compressionInfo.compressionStatus === CompressionStatus.COMPRESSED && | ||
| historyBeforeHardRescue | ||
| ) { | ||
|
Collaborator
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. [Suggestion] When compression succeeds (COMPRESSED) but the guard still rejects, On
— qwen3.7-max via Qwen Code /review |
||
| // Hard-rescue compression mutates in-memory history before this | ||
| // guard can compare the compressed prompt size. If the compressed | ||
| // prompt is still too large to send, restore the pre-compression | ||
| // state. The JSONL compression checkpoint is intentionally not | ||
| // written because the send is about to be rejected. | ||
| this.setHistory(historyBeforeHardRescue); | ||
| this.lastPromptTokenCount = lastPromptTokenCountBeforeHardRescue; | ||
| this.telemetryService?.setLastPromptTokenCount( | ||
| lastPromptTokenCountBeforeHardRescue, | ||
| ); | ||
| } | ||
| const compressionStatus = | ||
| CompressionStatus[compressionInfo.compressionStatus] ?? | ||
| String(compressionInfo.compressionStatus); | ||
| debugLogger.warn( | ||
| `[compaction] hard-tier rescue stopped oversized prompt: ` + | ||
| `prompt_id=${prompt_id}, effectiveTokens=${effectiveTokens}, ` + | ||
| `hard=${hard}, localPromptTokensAfterCompression=` + | ||
| `${localPromptTokensAfterCompression}, compressionStatus=` + | ||
| `${compressionStatus}, newTokenCount=` + | ||
| `${compressionInfo.newTokenCount}, consecutiveFailures=` + | ||
| `${this.consecutiveFailures}. ${message}`, | ||
| ); | ||
| throw new Error(message); | ||
| } | ||
| if ( | ||
| shouldForceFromHard && | ||
| compressionInfo.compressionStatus === CompressionStatus.COMPRESSED | ||
| ) { | ||
| this.chatRecordingService?.recordChatCompression({ | ||
| info: compressionInfo, | ||
| compressedHistory: this.getHistoryShallow(), | ||
| }); | ||
| } | ||
|
|
||
| // Add user content to history ONCE before any attempts. | ||
| this.history.push(userContent); | ||
| userContentAdded = true; | ||
|
|
||
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.
[Suggestion] The
tryCompressmethod-level JSDoc (lines 1361-1367) states unconditionally that onCOMPRESSEDthe method "recorded the event tochatRecordingService(if wired)" — but with this newdeferChatCompressionRecordoption, that's no longer true. When the option is set, recording is intentionally skipped insidetryCompressand deferred to the caller.Consider updating the JSDoc to note the exception, e.g.:
This prevents a future reader of the method signature from assuming the recording always happens inside
tryCompressand introducing a double-write.— qwen3.7-max via Qwen Code /review