feat(cache): optimize provider cache economics - #624
Open
SantiagoDePolonia wants to merge 7 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds exact-cache JSON canonicalization and concurrent miss coalescing. It adds provider-native prompt-cache planning for chat and Responses requests, routing integration, Bedrock fallback handling, Gemini cached-content reuse, stable credential affinity, and cache documentation. ChangesCache behaviors
Pro compression documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant CachePlanner
participant Provider
Client->>Router: send chat or Responses request
Router->>CachePlanner: plan request after routing
CachePlanner-->>Router: return cloned request with cache metadata
Router->>Provider: forward planned request
Provider-->>Client: return response or stream
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Confidence Score: 4/5Not safe to merge until independently executed cacheable followers can persist their exact-cache result. The failure was reproduced with a targeted runtime test on the changed response-cache path. It affects concurrent identical requests when the first response is non-cacheable and a later follower is cacheable. Files Needing Attention: internal/responsecache/simple.go needs to capture and store the independently executed follower response rather than calling the provider callback directly.
 T-Rex LogsWhat T-Rex did
Comments Outside Diff (1)
Reviews (3): Last reviewed commit: "fix(cache): address planner review follo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
SantiagoDePolonia
force-pushed
the
feat/cache-economic-optimization
branch
from
July 31, 2026 14:41
e413b30 to
bfb4801
Compare
SantiagoDePolonia
force-pushed
the
feat/cache-economic-optimization
branch
from
August 1, 2026 11:23
bfb4801 to
93d990c
Compare
Contributor
Author
|
Rebased onto the updated #622 and added the provider cache-planner kill switch in 93d990c. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/providers/bedrock/chat.go (1)
156-182: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftHandle failed Bedrock cache checkpoints before retrying.
Bedrock prompt caching has model-specific support and minimum prefix thresholds, such as 1,536 tokens for Nova profiles. The planner uses a fixed 1,024-token minimum and
(len(json)+3)/4, and the Bedrock request path insertscachePointunconditionally. If Bedrock rejects the checkpoint, the request fails without fallback; detect the error and retry with cache points stripped, or scope cache points to models with a higher-confidence minimum.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providers/bedrock/chat.go` around lines 156 - 182, Update the Bedrock request flow around cache-point insertion and error handling to recover from rejected cache checkpoints: when a request fails because of a cache-point validation or support error, remove all generated cache-point blocks and retry once without them. Preserve the existing request behavior for successful cache-enabled calls and return the original failure for unrelated errors; use the visible cache-point handling in the message conversion path and its enclosing request method as the implementation anchors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/features/cache.mdx`:
- Around line 109-114: Update the Gemini cached-content documentation bullet to
state that the object has a 300-second TTL and is reused only when the native
Gemini API is enabled via USE_GOOGLE_GEMINI_NATIVE_API. Clearly distinguish the
default behavior from the override condition while keeping the explanation
concise and user-focused.
In `@internal/providers/bedrock/chat.go`:
- Around line 193-195: Update isGatewayCachePoint to decode the raw value from
fields.Lookup("_gomodel_cache_point") as a JSON boolean and return the decoded
result, accepting valid boolean representations regardless of raw formatting.
Preserve false behavior when the field is missing or cannot be decoded.
In `@internal/providers/cache_planner_test.go`:
- Around line 12-91: Add a table-driven test covering planResponses and
markOpenAIResponsesBreakpoint for string, []core.ContentPart, and []any content
shapes. Assert the OpenAI prompt_cache_key and explicit breakpoint, the
Anthropic cache directive, skipping plans below the provider minimum, and that
the original request remains unchanged after planning.
In `@internal/providers/cache_planner.go`:
- Around line 16-19: The cache-point marker is duplicated across packages,
creating an unsynchronized contract. In internal/providers/cache_planner.go
lines 16-19, define the marker as an exported constant in internal/core and
reference it from the cache planner; in internal/providers/bedrock/chat.go lines
193-195, replace the inline marker literal with the same shared constant.
- Around line 42-58: Update planChat to estimate the prefix size from message
text lengths before constructing prefixBody, returning the original request when
the estimate is below providerCacheMinimum; only marshal the tools and prior
messages after the threshold passes, preserving the existing cacheAffinityKey
input and cloneChatRequest flow.
- Around line 146-153: Extend hasCacheDirective and its caller to inspect the
prefix’s message ExtraFields and each content part’s ExtraFields, not only
request-level fields. Treat cache_control and prompt_cache_breakpoint, along
with the existing cache directive keys, as indicators and return true when any
are present so the planner does not add a duplicate directive.
- Around line 71-74: Update planChat so the Anthropic cache plan produced by
mergeCacheExtras is applied before hasCacheDirective rejects existing
cache_control, while preserving rejection of client-supplied directives. Ensure
the resulting planner-owned cache data is forwarded through forwardChatRequest
and remains consumable by adaptAnthropicCacheControl.
In `@internal/providers/gemini/gemini_test.go`:
- Around line 40-47: Update the httptest handler in the Gemini test to avoid
calling t.Fatalf from its server goroutine. Record an unexpected r.URL.Path for
assertion after the requests complete, or use t.Errorf followed by a plain
return, while preserving the existing path validation and response behavior.
- Around line 38-74: Expand TestPrepareCachedContentCreatesAndReusesObject into
table-driven cases covering creation errors, empty response names, entries
expiring within 15 seconds, and re-creation after expiry. Assert best-effort
failures preserve the original Contents, SystemInstruction, and Tools, while
expired entries trigger a new cache creation; retain the existing successful
creation-and-reuse assertions.
In `@internal/providers/gemini/gemini.go`:
- Around line 591-596: Bound growth of p.cacheObjects in the cache insertion
path around cachedContentObject: before or during inserting a new prefix, sweep
entries whose expiresAt is in the past when the map reaches a defined size
threshold. Preserve current lookup and expiry behavior, and ensure the cleanup
runs under p.cacheMu without leaving stale entries indefinitely.
- Around line 599-604: Update the cache-creation error path surrounding the
visible err check in the Gemini flow to log failures at debug or warn level,
including the model identifier and error reason. Preserve the existing
successful cached-prefix behavior, and do not include the cache key contents or
credentials in the log.
- Around line 565-598: Update prepareCachedContent and the cache lookup/storage
paths around cacheFlight, cachedContentObject, and cacheObjects so
cached-content entries are keyed by the active Gemini API key/project identity
in addition to the prompt cache key. Ensure both creation and reuse use this
scoped key, preventing requests under one credential context from referencing
resources created under another; alternatively disable cached-content reuse when
multiple rotating keys are configured.
In `@internal/providers/router.go`:
- Around line 581-598: In plannedChatRequest and plannedResponsesRequest, retain
the cachePlanner nil checks but remove the duplicated message-count, input type,
and length eligibility guards. After forwarding the request, delegate directly
to cachePlanner.planChat or planResponses so the planner owns eligibility and
planning order.
In `@internal/responsecache/exact_cache_test.go`:
- Around line 189-197: Update the concurrent test around driveHandleRequest and
wg.Go to avoid calling testing.T.Fatalf from worker goroutines: capture each
goroutine’s error/result and assert or fail from the test goroutine after
wg.Wait(). Replace the fixed time.Sleep synchronization with an atomic counter
or equivalent barrier that keeps the leader blocked until all follower requests
have entered the handler, ensuring the calls == 1 assertion is deterministic.
In `@internal/responsecache/simple.go`:
- Around line 111-124: Replace the blocking m.misses.Do call in the cache-miss
flow with m.misses.DoChan, then select between its result channel and
ex.Context().Done(). Return the context cancellation error immediately when the
follower disconnects or its deadline expires, while allowing the leader
operation to continue for other waiters and preserving the existing result/error
handling when the singleflight call completes.
---
Outside diff comments:
In `@internal/providers/bedrock/chat.go`:
- Around line 156-182: Update the Bedrock request flow around cache-point
insertion and error handling to recover from rejected cache checkpoints: when a
request fails because of a cache-point validation or support error, remove all
generated cache-point blocks and retry once without them. Preserve the existing
request behavior for successful cache-enabled calls and return the original
failure for unrelated errors; use the visible cache-point handling in the
message conversion path and its enclosing request method as the implementation
anchors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9350a263-a7da-42f8-a190-9695c8189cbd
📒 Files selected for processing (13)
.env.templatedocs/features/cache.mdxdocs/pro.mdxinternal/core/types.gointernal/providers/bedrock/chat.gointernal/providers/cache_planner.gointernal/providers/cache_planner_test.gointernal/providers/gemini/gemini.gointernal/providers/gemini/gemini_test.gointernal/providers/router.gointernal/responsecache/exact_cache_test.gointernal/responsecache/simple.gointernal/responsecache/stream_cache.go
SantiagoDePolonia
force-pushed
the
feat/cache-economic-optimization
branch
from
August 1, 2026 12:17
93d990c to
2547be9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/features/cache.mdx`:
- Around line 116-122: Update the provider prompt-cache planning documentation
near the PROVIDER_PROMPT_CACHE_PLANNER_ENABLED example to state that invalid
values retain the default enabled behavior. Clearly distinguish this from the
explicit value false, while keeping the existing concise default and override
guidance.
In `@internal/providers/bedrock/chat.go`:
- Around line 175-183: Update the assistant-message handling around
convertAssistantMessage and isGatewayCachePoint so the cache-point block is
appended only when blocks already contains at least one content block. Preserve
the existing empty-message behavior by allowing empty assistant messages to
reach appendOrMerge without adding a cache point.
In `@internal/providers/cache_planner_test.go`:
- Around line 57-82: Update TestNewCachePlanner_EnvironmentKillSwitch to unset
providerPromptCachePlannerEnabledEnv for the default-on case and call t.Setenv
only when a test value is explicitly declared, so the unset branch is exercised.
In TestCachePlannerHonorsMinimumAndClientDirective, set
providerPromptCachePlannerEnabledEnv to "true" before constructing the planner,
ensuring the assertions exercise the minimum threshold and client directive
logic rather than an ambient disabled configuration.
In `@internal/providers/cache_planner.go`:
- Around line 23-40: Update newCachePlanner to always return a non-nil
*cachePlanner containing an enabled bool that reflects the parsed environment
flag, including the disabled case. Modify planChat and planResponses to return
the request unchanged when the planner is disabled, while preserving existing
planning behavior when enabled.
- Around line 75-76: The Bedrock planner can mark a tool-result message that the
adapter ignores. Update the "bedrock" case in the cache planning flow and
markLastChatPrefix usage to restrict selection to roles handled by Bedrock's
convertMessages, while preserving the existing text/tool-call eligibility and
marker behavior.
In `@internal/providers/gemini/gemini_test.go`:
- Around line 62-68: Extend the cache test around prepareCachedContent to
exercise cacheFlight with concurrent calls using the same key: start N
goroutines against a creation handler that blocks until all callers arrive,
release them together, and assert that exactly one creation occurs. Ensure the
test synchronizes goroutine completion and is race-safe, while preserving the
existing sequential cache-hit coverage.
In `@internal/providers/gemini/gemini.go`:
- Around line 565-604: Update prepareCachedContent to add a short negative cache
entry when cached-content creation fails, and consult that entry to skip
repeated attempts for the same key. Use p.backend to restrict cached-content
creation to the implemented Gemini resource path, skipping unsupported backends
such as Vertex rather than posting to /cachedContents; preserve the existing
success caching behavior for supported Gemini requests.
In `@internal/responsecache/exact_cache_test.go`:
- Around line 216-223: The test
TestHashRequest_CanonicalizesJSONFormattingAndKeyOrder should become
table-driven and cover canonical-equivalent formatting/key order plus malformed
JSON, multiple JSON values, and distinct numeric spellings. Assert the
documented raw-byte fallback for invalid or multiple-value input and the
expected json.Number-preserving behavior for valid numeric variants, while
retaining the existing equivalent-object assertion.
In `@internal/responsecache/simple.go`:
- Around line 111-124: Update the miss handling around m.misses.Do and
ex.Capture so leader execution errors are stored in missResult rather than
returned directly through singleflight. Return the stored error only when the
result belongs to owner; when a follower receives a missResult containing an
error, invoke next() to execute independently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4434329e-0b8e-421c-9238-2d2baacd88e0
📒 Files selected for processing (13)
.env.templatedocs/features/cache.mdxdocs/pro.mdxinternal/core/types.gointernal/providers/bedrock/chat.gointernal/providers/cache_planner.gointernal/providers/cache_planner_test.gointernal/providers/gemini/gemini.gointernal/providers/gemini/gemini_test.gointernal/providers/router.gointernal/responsecache/exact_cache_test.gointernal/responsecache/simple.gointernal/responsecache/stream_cache.go
SantiagoDePolonia
force-pushed
the
feat/cache-economic-optimization
branch
2 times, most recently
from
August 1, 2026 16:12
6dc9466 to
6adc6a2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/providers/gemini/gemini.go`:
- Around line 553-555: The guard in the Gemini caching conversion must not
reject requests solely because body.Contents has fewer than two items when
SystemInstruction is present. Update the early-return condition in the relevant
caching function to allow a non-empty SystemInstruction with one remaining
content item, while preserving the existing nil-body and already-cached checks.
- Line 565: Update the cache creation closure in the cacheFlight.Do call to
derive a detached context with its own timeout instead of using the requesting
caller’s ctx, then pass that createCtx to p.nativeClient.Do. Ensure the timeout
context is canceled after creation and preserves the shared result for all
waiters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 11c40eed-61bd-49ec-8cd7-87bc51b02d19
📒 Files selected for processing (13)
.env.templatedocs/features/cache.mdxdocs/pro.mdxinternal/core/types.gointernal/providers/bedrock/chat.gointernal/providers/cache_planner.gointernal/providers/cache_planner_test.gointernal/providers/gemini/gemini.gointernal/providers/gemini/gemini_test.gointernal/providers/router.gointernal/responsecache/exact_cache_test.gointernal/responsecache/simple.gointernal/responsecache/stream_cache.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/providers/cache_planner.go`:
- Around line 122-144: The cloneChatRequest function currently loses
non-serialized fields during the JSON round trip. After unmarshalling the cloned
request, explicitly copy fields such as AnthropicMessagesRequest and
PromptCachePlan from req onto clone, while preserving the existing serialization
behavior for other fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 14d00085-ae06-4250-8ae9-73c33b4252fb
📒 Files selected for processing (13)
.env.templatedocs/features/cache.mdxdocs/pro.mdxinternal/core/types.gointernal/providers/bedrock/chat.gointernal/providers/cache_planner.gointernal/providers/cache_planner_test.gointernal/providers/gemini/gemini.gointernal/providers/gemini/gemini_test.gointernal/providers/router.gointernal/responsecache/exact_cache_test.gointernal/responsecache/simple.gointernal/responsecache/stream_cache.go
SantiagoDePolonia
force-pushed
the
feat/cache-economic-optimization
branch
from
August 1, 2026 17:00
6adc6a2 to
89b23f8
Compare
SantiagoDePolonia
force-pushed
the
feat/cache-economic-optimization
branch
from
August 1, 2026 17:36
89b23f8 to
0a2a57b
Compare
SantiagoDePolonia
changed the base branch from
feat/prompt-cache-affinity
to
main
August 1, 2026 17:36
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/responsecache/simple.go (1)
104-146: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
finishMissagainst a panic inex.Capture/next().In the leader branch,
m.finishMiss(key, call, data, err)runs only afterex.Capturereturns normally. Ifnext()panics,finishMissnever runs. The map entry forkeyinm.missesstays forever, andcall.donenever closes.Every later request that hashes to the same
keybecomes a follower. Each follower waits oncall.doneuntil its own context times out, then returns a context error instead of retrying independently. Thecall.err != nilretry path at Line 132 only fires afterclose(call.done), which never happens for a poisoned key.
golang.org/x/sync/singleflight, which this code replaces, guards against exactly this case: on panic, it recovers, propagates the panic to all waiters, and still removes the call from its internal map. The current implementation drops that guarantee.Wrap the leader branch so
finishMissalways executes, then re-panic to preserve the original failure signal.🔧 Proposed fix for panic safety in the leader branch
call, leader := m.joinMiss(key) if leader { - data, ok, err := ex.Capture("response cache: failed to capture cacheable response body", next) - if err == nil && ok { - m.enqueueWrite(cacheWriteJob{key: key, data: data}) - } else { - data = nil - } - m.finishMiss(key, call, data, err) - return err + var data []byte + var err error + finished := false + defer func() { + if finished { + return + } + r := recover() + m.finishMiss(key, call, nil, err) + if r != nil { + panic(r) + } + }() + var ok bool + data, ok, err = ex.Capture("response cache: failed to capture cacheable response body", next) + if err == nil && ok { + m.enqueueWrite(cacheWriteJob{key: key, data: data}) + } else { + data = nil + } + m.finishMiss(key, call, data, err) + finished = true + return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/responsecache/simple.go` around lines 104 - 146, Update the leader branch of simpleCacheMiddleware.StoreAfter so finishMiss always executes when ex.Capture or next panics, ensuring the miss entry is removed and call.done is closed. Add panic-safe cleanup around the capture/write flow, then re-panic with the original value after finishMiss so callers and followers observe the existing failure signal.internal/providers/cache_control.go (1)
47-73: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip the re-marshal for batch items without Anthropic cache control.
adaptAnthropicBatchCacheControlcallsjson.Marshal(adaptAnthropicCacheControl(chat, providerType))for every item once the provider doesn't accept Anthropic cache control, even when that specific item carries nocache_controldirective at all (in which caseadaptAnthropicCacheControlreturns the request unchanged). For large batches, this means decoding and re-encoding every item purely to reproduce the same bytes.Check
hasAnthropicCacheControl(chat)before re-marshaling, and reuseitem.Bodywhen it returns false.♻️ Proposed refactor
chat, ok := decoded.Request.(*core.ChatRequest) if !ok { return nil, core.NewInvalidRequestError( fmt.Sprintf("requests[%d]: Anthropic Message Batch item is not a chat completion", i), nil, ) } - body, err := json.Marshal(adaptAnthropicCacheControl(chat, providerType)) - if err != nil { - return nil, core.NewInvalidRequestError(fmt.Sprintf("requests[%d]: failed to encode adapted chat request", i), err) + if !hasAnthropicCacheControl(chat) { + continue + } + body, err := json.Marshal(adaptAnthropicCacheControl(chat, providerType)) + if err != nil { + return nil, core.NewInvalidRequestError(fmt.Sprintf("requests[%d]: failed to encode adapted chat request", i), err) } adapted.Requests[i].Body = body🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providers/cache_control.go` around lines 47 - 73, Update adaptAnthropicBatchCacheControl to check hasAnthropicCacheControl(chat) after decoding each batch item; when false, preserve item.Body directly in adapted.Requests[i].Body and skip adaptAnthropicCacheControl and json.Marshal. Continue adapting and marshaling only items that contain Anthropic cache control, while preserving the existing error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/pro.mdx`:
- Around line 57-66: Align the documented compression settings with the
implementation: either remove unsupported variables from docs/pro.mdx or wire
each named PRO_COMPRESSION_* setting through the configuration parser and
request rewriter, including defaults, units, normalization, scope, and bounded
frozen-epoch behavior. Add parser/config coverage to verify these settings match
the rewriter’s thresholds and per-epoch compaction semantics.
In `@internal/providers/cache_planner.go`:
- Around line 456-471: Update promptCacheProfileFor to recognize the normalized
provider type "bedrock-mantle" and return the same Bedrock cache profile,
preserving the cached OpenAI responses behavior and
supportsExplicitOpenAICache("gpt-5.6") semantics.
- Around line 428-454: Update the Anthropic branch of providerCacheMinimum to
match current model-generation requirements using exact platform model-ID
substring patterns: return 4096 for Haiku 4.5 and Opus 4.5/4.6, preserve the
appropriate 2048 minimum for Haiku generations that require it, and retain
existing 1024 behavior for older supported models. Ensure the conditions do not
classify Haiku 3 or 3.5 as 4096.
In `@internal/providers/gemini/gemini.go`:
- Around line 593-599: Update the cached-content request construction around
geminiCreateCachedContentRequest to derive the upstream TTL from geminiCacheTTL
instead of hardcoding "300s", converting the configured duration to the
provider’s seconds-formatted string with strconv as needed. Keep the upstream
TTL aligned with the local expiry fallback used later in the flow.
- Around line 679-684: Update planChat’s PromptCachePlan.Key derivation to
include a digest of the final content item in the cached prefix, matching the
boundary selected by useGeminiCachedPrefix. Preserve the existing Messages,
Tools, and credential components while ensuring requests with different boundary
content produce distinct cache keys.
- Around line 585-620: Re-read the clock immediately after the /cachedContents
call in the cached-content creation flow, before handling the result. Use this
post-call timestamp for the failure retryAfter values, the expiration freshness
check, and the stored successful geminiCacheObject; retain the initial timestamp
only for request setup and pre-call behavior.
In `@internal/responsecache/exact_cache_test.go`:
- Around line 38-43: The blockingMissExchange.Context method relies on
StoreAfter invoking ex.Context twice before the follower wait point. Add a short
comment immediately above Context documenting this coupling and noting that
changes to StoreAfter’s call count require updating the helper.
---
Outside diff comments:
In `@internal/providers/cache_control.go`:
- Around line 47-73: Update adaptAnthropicBatchCacheControl to check
hasAnthropicCacheControl(chat) after decoding each batch item; when false,
preserve item.Body directly in adapted.Requests[i].Body and skip
adaptAnthropicCacheControl and json.Marshal. Continue adapting and marshaling
only items that contain Anthropic cache control, while preserving the existing
error handling.
In `@internal/responsecache/simple.go`:
- Around line 104-146: Update the leader branch of
simpleCacheMiddleware.StoreAfter so finishMiss always executes when ex.Capture
or next panics, ensuring the miss entry is removed and call.done is closed. Add
panic-safe cleanup around the capture/write flow, then re-panic with the
original value after finishMiss so callers and followers observe the existing
failure signal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6c143831-3e04-4646-b1d8-ce71f269dd6e
📒 Files selected for processing (19)
.env.templatedocs/features/cache.mdxdocs/pro.mdxdocs/providers/gemini.mdxinternal/core/types.gointernal/providers/bedrock/bedrock_test.gointernal/providers/bedrock/chat.gointernal/providers/bedrock/chat_stream.gointernal/providers/cache_control.gointernal/providers/cache_planner.gointernal/providers/cache_planner_test.gointernal/providers/gemini/gemini.gointernal/providers/gemini/gemini_test.gointernal/providers/keyring.gointernal/providers/keyring_test.gointernal/providers/router.gointernal/responsecache/exact_cache_test.gointernal/responsecache/simple.gointernal/responsecache/stream_cache.go
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Comment on lines
+135
to
+136
| if len(call.data) == 0 { | ||
| return next() |
There was a problem hiding this comment.
When the miss leader returns a non-cacheable response, a waiting identical request correctly executes independently, but its successful cacheable response is never stored. The call.data == "" branch invokes next() directly, bypassing the response capture and asynchronous cache write used by the leader path. As a result, repeated identical requests continue reaching the provider until a later cacheable request happens to become the leader. Route this independent follower execution through StoreAfter (or shared capture-and-store logic) so it can populate the exact cache.
Artifacts
Response-cache coalescing reproduction source
- This targeted Go test blocks a non-cacheable miss leader, joins a cacheable follower on the same key, and then reads the exact cache; it defines the failing execution path.
PR-head response-cache coalescing reproduction output
- The current PR head ran the targeted Go test and failed because the cache remained empty after the cacheable follower completed, proving the missed cache write.
Contributor
Author
There was a problem hiding this comment.
Fixed in e523090. Independent followers now use the shared capture-and-store path, so a cacheable follower populates the exact cache after a non-cacheable leader. Added a regression test for this exact sequence.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/providers/cache_control.go (1)
55-71: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEvery batch item is decoded and re-encoded, including items that need no change.
adaptAnthropicCacheControlreturns the same pointer when the item carries nocache_control(line 19). Line 66 still marshals and line 70 still overwritesBody. Two effects follow for an Anthropic Messages batch routed to a provider that does not accept Anthropic cache control:
- The gateway re-serializes every item, so batch submission cost scales with the whole batch instead of with the affected items.
- An unchanged item's original bytes are replaced by the gateway's canonical encoding. Any request detail that
DecodeKnownBatchItemRequestdoes not model is lost from the stored body.Rewrite
Bodyonly when the adaptation changed the request.🐛 Proposed fix
- body, err := json.Marshal(adaptAnthropicCacheControl(chat, providerType)) + adaptedChat := adaptAnthropicCacheControl(chat, providerType) + if adaptedChat == chat { + continue + } + body, err := json.Marshal(adaptedChat) if err != nil { return nil, core.NewInvalidRequestError(fmt.Sprintf("requests[%d]: failed to encode adapted chat request", i), err) } adapted.Requests[i].Body = body🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providers/cache_control.go` around lines 55 - 71, Update the batch-item handling around adaptAnthropicCacheControl so Body is re-marshaled and overwritten only when adaptation returns a different request from the decoded chat request. Preserve each unchanged item's original Body bytes, while continuing to encode and store the adapted request for items requiring cache-control changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/providers/cache_planner.go`:
- Around line 174-181: Define the cache-directive key set once near the shared
cache-planning helpers, then reuse it in both hasCacheDirective and
isCacheDirectiveKey. Remove the duplicated key literals while preserving the
existing keys and lookup behavior so both functions always evaluate the same
directive set.
- Around line 433-459: Align the Bedrock Anthropic Sonnet 4.5 handling in
providerCacheMinimum with the Anthropic branch: verify the intended minimum and
update the Bedrock condition and its test expectation accordingly. If Bedrock
must remain at 4096, preserve that value but document the provider-specific
requirement and ensure the test explicitly reflects it.
- Around line 106-144: Update the entry guard in planResponses to return req
immediately for prompt-cache modes that this function cannot plan, including
promptCacheBedrock and promptCacheGemini. Perform this check before directive
scanning, prefix marshaling, and cloneResponsesRequest, while preserving the
existing OpenAI and Anthropic planning paths.
In `@internal/providers/gemini/gemini_test.go`:
- Around line 236-240: Update the Vertex negative test around
prepareCachedContent to use an httptest server as the provider’s HTTP endpoint
instead of http.DefaultClient and the real Gemini URL. Track requests received
by the test server and assert that none occurred, while preserving the existing
CachedContent assertion.
In `@internal/responsecache/simple.go`:
- Around line 129-136: Update the follower handling in the relevant
response-cache method around StoreAfter so call.err != nil invokes next()
directly instead of recursively calling StoreAfter; preserve the existing
behavior for non-empty cached data and empty call.data. Extend the leader-error
test to cover multiple concurrent followers and verify each independently
executes next() without retry-wave recursion.
- Around line 113-121: The leader path around ex.Capture must clean up an
in-flight call when next panics. Add deferred cleanup that invokes finishMiss
with the captured call state, removes the key, and closes call.done while
allowing the original panic to propagate; avoid duplicate completion on normal
return. Add a regression test that recovers the leader panic and verifies a
subsequent identical request proceeds.
In `@internal/responsecache/stream_cache.go`:
- Around line 65-75: Update canonicalJSONForCache to detect duplicate object
member names while decoding, returning the original body whenever duplicates are
found instead of canonicalizing it. Preserve existing handling for invalid or
trailing JSON, and add a table-driven regression case in exact_cache_test.go
covering duplicate keys producing no cache collision.
---
Outside diff comments:
In `@internal/providers/cache_control.go`:
- Around line 55-71: Update the batch-item handling around
adaptAnthropicCacheControl so Body is re-marshaled and overwritten only when
adaptation returns a different request from the decoded chat request. Preserve
each unchanged item's original Body bytes, while continuing to encode and store
the adapted request for items requiring cache-control changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b9ee01db-19a2-4625-a2c5-f042a98e9978
📒 Files selected for processing (19)
.env.templatedocs/features/cache.mdxdocs/pro.mdxdocs/providers/gemini.mdxinternal/core/types.gointernal/providers/bedrock/bedrock_test.gointernal/providers/bedrock/chat.gointernal/providers/bedrock/chat_stream.gointernal/providers/cache_control.gointernal/providers/cache_planner.gointernal/providers/cache_planner_test.gointernal/providers/gemini/gemini.gointernal/providers/gemini/gemini_test.gointernal/providers/keyring.gointernal/providers/keyring_test.gointernal/providers/router.gointernal/responsecache/exact_cache_test.gointernal/responsecache/simple.gointernal/responsecache/stream_cache.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/responsecache/stream_cache.go`:
- Around line 68-70: Update hasDuplicateJSONMemberNames to call
decoder.UseNumber() immediately after creating the JSON decoder and before
scanValue performs token scanning, preventing large numeric literals from
failing early. Add coverage in exact_cache_test.go for an oversized numeric
value followed by a duplicate model key, verifying the duplicate-preserving
fallback and request-key behavior remain correct.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 708dbe35-0e69-49f5-96fe-82e29cdb95ba
📒 Files selected for processing (8)
.env.templateinternal/providers/cache_planner.gointernal/providers/cache_planner_test.gointernal/providers/gemini/gemini_test.gointernal/providers/router.gointernal/responsecache/exact_cache_test.gointernal/responsecache/simple.gointernal/responsecache/stream_cache.go
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
Verification
go test ./...go test -race ./internal/responsecache ./internal/providers/gemini ./internal/providers/bedrockgo test ./tests/perf -run TestHotPathPerfGuard -count=1Stack
Depends on #622.
Summary by CodeRabbit
New Features
Improvements