fix(gemini): tighten reactive CachedContent NOT_FOUND matcher#656
Open
sroussey wants to merge 1 commit into
Open
fix(gemini): tighten reactive CachedContent NOT_FOUND matcher#656sroussey wants to merge 1 commit into
sroussey wants to merge 1 commit into
Conversation
isGeminiCachedContentNotFoundError fell through to /NOT_FOUND|not.*found/i on
the error message, matching unrelated errors: 'model not found', 'file not
found', 'function name not found in declarations', a 404 on an unrelated
URL, plain new Error("NOT_FOUND"). When any of these surfaced on a request
that referenced cachedContent, the helper deleted the still-valid entry
(locally + best-effort server delete) and retried the request inline. If the
original error was model-not-found, the retry failed the same way — net
effect: 2x latency + billing, and every OTHER consumer of the shared
checkpoint id silently paid full re-encode cost on their next call.
Rewrite the matcher so a hit requires BOTH:
- a structured NOT_FOUND signal (status/code === 404 or "NOT_FOUND",
on the top-level error or the nested .error the Google GenAI SDK
sometimes wraps GAPI errors in), AND
- a scoped mention of the cache resource (cached[_ ]?content) in the
message.
When only a message signal is available, require the stricter scoped
pattern cached[_ ]?content(?:s/[\w-]+)?[^.\n]{0,120}(?:NOT_FOUND|not
found|does not exist) — a bare "NOT_FOUND" is deliberately not enough.
generateGeminiStreamWithCacheFallback now emits a debug log with
{status, code} when a checkpoint-referencing request fails but the error
did NOT match a CachedContent NOT_FOUND, so a genuine cache regression
stays diagnosable in ops logs.
Also expose the two helpers via _testOnly on @workglow/google-gemini/ai so
a colocated vitest can drive them without a client / real API call.
Tests in packages/test/src/test/ai-provider/Gemini_CachedContentFallback.
test.ts cover both true and false cases exhaustively (6 positive, 9
negative), plus four flow tests on the fallback helper: eviction+retry on
a scoped NOT_FOUND, and no-op on model-not-found / useCachedContent=false
/ missing checkpointId. 19 tests, all passing.
Co-Authored-By: Claude <noreply@anthropic.com>
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.
Summary
Follow-up on PR #641 (Add and harden AI provider cache checkpoints) addressing 1 of 2 HIGH-priority findings from a scheduled review of the PR.
The bug
isGeminiCachedContentNotFoundErrorfell through to/NOT_FOUND|not.*found/ion the error message. That unscoped substring matches many benign non-cache errors returned bygenerateContentStream:{ status: 400, message: "The requested model 'gemini-x-nope' was not found" }{ status: 400, message: "File 'files/abc' was not found." }{ message: "Function name 'foo' not found in declarations." }{ status: 404, message: "The requested URL /v1beta/... was not found." }new Error("NOT_FOUND")When any of these surfaced on a request that referenced
cachedContent, the helper deleted the still-valid entry (locally + best-effort server delete) and retried the request inline. If the original error was model-not-found, the retry failed the same way — net effect: 2× latency + billing, and every OTHER consumer of the shared checkpoint id silently paid full re-encode cost on their next call.The fix
Rewrite the matcher so a hit requires BOTH:
NOT_FOUNDsignal (status/code === 404or"NOT_FOUND", on the top-level error or the nested.errorthe Google GenAI SDK sometimes wraps GAPI errors in), AND/cached[_ ]?content/i) in the message.When only a message signal is available, require the stricter scoped pattern
cached[_ ]?content(?:s\/[\w-]+)?[^.\n]{0,120}(?:NOT_FOUND|not\s+found|does\s+not\s+exist)— a bare"NOT_FOUND"is deliberately not enough.generateGeminiStreamWithCacheFallbacknow emits a debug log with{status, code}when a checkpoint-referencing request fails but the error did NOT match a CachedContentNOT_FOUND, so a genuine cache regression stays diagnosable in ops logs.Also expose the two helpers via
_testOnlyon@workglow/google-gemini/aiso a colocated vitest can drive them without a client / real API call.Verification
Tests in
packages/test/src/test/ai-provider/Gemini_CachedContentFallback.test.tscover both true and false cases exhaustively:NOT_FOUND, nestederror.status, resource-path variants)new Error("NOT_FOUND"),null,undefined, plain string, message-only "not found" with no cache mention)useCachedContent=false/ missingcheckpointIdbun x vitest run packages/test/src/test/ai-provider/Gemini_CachedContentFallback.test.ts # 19 passDeferred
The second HIGH finding on PR #641 — LlamaCpp steal breaks
keepParentCheckpointsemantics (silent 10× perf regression on branched checkpoints) — needs a follow-up PR. The plan-of-record is Option A (parity with HFT): gate steal onsessionContext.supersedeParent === true, snapshotprefixTokenCountafter preload, and rewind viasequence.eraseContextTokenRangesafter non-superseding turns. That implementation requires carefulnode-llama-cppSDK feature detection (eraseContextTokenRanges/getStateData/loadStateDataavailability across the pinned version) that this PR intentionally does not attempt — a naive steal-gate change alone would corrupt concurrent consumers of a branched sequence. Tracking as a separate task.🤖 Generated with Claude Code
Generated by Claude Code