Repository navigation
feat(model/gemini): add context cache lifecycle management for agent sessions - #1951
karthik-0306 wants to merge 7 commits into
Conversation
Resolves the conflict with google#1964 in session/vertexai/vertexai_test.go by keeping both sides' eventNeedsRawEvent cases: the transcription rows from upstream and the cache metadata row from this branch.
1a2fd28 to
8c2d019
Compare
karolpiotrowicz
left a comment
There was a problem hiding this comment.
Must change before merge:
- When an after-model callback fills in an empty Gemini reply on the turn that creates a cache, the session ends up naming a cache that the Gemini model code (
finish) has just deleted. Every later turn of that agent then sends the deleted cache name until the cache'sExpireTime, and the PR's own doc comment says the API rejects such a request. - Responses that reach the flow but are never stored delete a cache the session still names. The new cache created on that turn is recorded nowhere and stays on the Gemini API until its TTL runs out.
CreateHTTPOptionsis shared between the caller, the runner and every request, although the comments say the runner and the request processor each take a copy.- The behaviors under "Untested behavior" can change today without any test failing, so each needs a test.
A stored event can name a cache that was deleted
I reproduced this through runner.Run with an llmagent and the real Gemini model pointed at an httptest fake of the Gemini API:
- Turn 1 gets a normal reply that reports 5,000 prompt tokens.
- On turn 2 the prefix matches, so
preparecreatescachedContents/1. The model answers{"candidates":[{"finishReason":"STOP"}]}with no content. - An
AfterModelCallbackreplaces an empty reply with fallback text.
record puts the new cache's metadata on the empty response but leaves delivered false (gemini.go#L133-L139). keepCacheMetadata copies that metadata onto the callback's reply (base_flow.go#L1058), and the flow stores it. Then finish deletes the cache, because nothing was delivered (context_cache.go#L175-L188). Here is the API log from the run, with the fake answering 404 for a deleted cache:
POST /v1beta/cachedContents -> created cachedContents/1
POST /v1beta/models/test-model:generateContent cachedContent=cachedContents/1 -> 200, STOP with no content
DELETE /v1beta/cachedContents/1
turn 2 stored CacheMetadata{CacheName:"cachedContents/1" InvocationsUsed:1 ContentsCount:2}
POST /v1beta/models/test-model:generateContent cachedContent=cachedContents/1 -> 404 (turns 3, 4 and 5)
Without the callback, turn 3 creates a new cache and succeeds. A callback that edits the response in place and returns nil reaches the same state without keepCacheMetadata, so the problem is when delivery is decided, not that one helper. Whatever the fix, it needs to keep two properties. A cache is deleted only when no stored event names it. Every cache created on the Gemini API is either deleted or named by the event the session stores.
Responses that reach the flow but are never stored
Deciding delivery before the event is stored also leaks caches. I reproduced two cases with CacheIntervals: 1, where turn 3 replaces cache A with cache B. In the first, an after-model callback returns a reply with no content, which the flow drops. In the second, AppendEvent fails for the model event. In both, nothing is stored for turn 3. Cache A is deleted while the latest stored event still names it. Cache B is never recorded or deleted, so it stays on the Gemini API until its TTL runs out. An OnEvent plugin that returns a new event does the same, because fromPlugin keeps only Actions.Compaction (runner.go#L509-L514). The doc comment on generateWithContextCache names only post-processing and callback errors as outcomes the model cannot see (gemini.go#L119-L122), so these cases are missing from it too.
CreateHTTPOptions is shared, not copied
The comments at runner.go#L160-L163 and context_cache_processor.go#L56-L58 say the runner and the processor each take a copy. Both copies are one level deep, so every request shares the caller's *genai.HTTPOptions and its Headers map. I ran three cases:
- A header that I set on the caller's options after calling
runner.Newreached the next cache-create request. - A header that a
BeforeModelCallbackset onreq.CacheConfig.CreateHTTPOptions.Headersin one invocation ended up in the caller's map and on the next invocation's create request. - When a goroutine wrote the caller's header map while other turns were creating caches, the run produced
WARNING: DATA RACEinhttpOptions, thenfatal error: concurrent map iteration and map write, which ends the process.
TestNew_CopiesContextCache and TestContextCacheRequestProcessor_DoesNotShareState change only scalar fields, so they pass either way.
Untested behavior
Each change below leaves the whole suite green:
- Making a zero
CacheIntervalsmean 1000 instead of 10 (context_cache.go#L458-L463). - Dropping
resp.ErrorCode != "" || resp.Interruptedfrom the delivery check (gemini.go#L137). That clause is what keeps a stored response with an error code, such as a SAFETY finish, from deleting the cache it names. - Writing the metadata into the callback's own object instead of a copy in
keepCacheMetadata(base_flow.go#L1120-L1127). - Making
keepCacheMetadataoverwrite a replacement that already carries its own metadata. No test passes such a replacement, so nothing would catch it. - Removing the
Headers.Clone()inhttpOptions(context_cache.go#L198).TestGenerateContent_ContextCacheLeavesSharedOptionspasses options whoseHeadersis nil (context_cache_test.go#L667), so the clone never matters there. - Adding
omitemptyto thefingerprintJSON tag (cache.go#L140). adk-python's validator requires the key, so adk-python could not load cache metadata written without the key. - Letting an older prompt token count overwrite a newer one in
findCacheInfo(context_cache_processor.go#L114).
Not blocking
- The cache-creation request skips the googleapis/go-genai#931 workaround that
generateapplies. That workaround restores an emptytexton parts that carry only a thought signature. Compare gemini.go#L181 with context_cache.go#L350-L361. With a model part{Text: "", ThoughtSignature: ...}in the cached prefix, thegenerateContentbody carries"text": ""and thecachedContentsbody does not. I could not check whether the API rejects that create. If it does, caching never starts for a session whose cached prefix has a part that carries only a thought signature, and every model call retries the create. - The cache-creation request also leaves out a client-level
ExtrasRequestProvider, whichgeneratere-applies because genai drops the provider when it merges per-call options. A marker field that such a provider adds reached thegenerateContentbody but not thecachedContentsbody in my run. - The
breaking-changelabel you asked about for thecache_metadatacolumn is a maintainer decision, and this review does not settle it.
Address the review on google#1951. The model decided which cache to delete when its response left the model, before callbacks, plugins, the flow and the session write decided what gets stored. A callback filling in an empty reply then left the session naming a deleted cache, and a reply that was never stored deleted the cache the session still named while leaking the new one. Under a runner, each model call now records the caches it created or stopped using in a per-invocation ledger. When the invocation ends, the runner keeps every cache that an agent's latest stored record names and deletes every other tracked cache. Also: - Copy CreateHTTPOptions deeply in the runner and the request processor, so requests share no headers, extra body, timeout or retry options with the caller. - Send a request naming a cache the provider no longer holds again once without it. - Apply the client's ExtrasRequestProvider and the go-genai#931 workaround to cache creation. - Carry CacheMetadata onto callback replacements in inheritModelFields, replacing keepCacheMetadata. - Document context caching in the gemini package doc, and correct the adk-python compatibility note on CacheMetadata. - Add tests for every behavior the review found unpinned.
|
Thanks for the detailed repros, they made this easy to track down. You were right on all of it. The root problem was that the model deleted caches before anyone knew what would actually get stored. It doesn't do that anymore: each call just notes the caches it created or replaced, and the runner cleans up once the invocation ends, based on what the session really stored. That covers the empty-reply case, the dropped, failed and plugin-replaced replies, and both of your properties.
Each of your untested cases has a test now, and your repros are tests too. I checked that they fail on the old code the same way your runs did. Also, a correction: the "70 mutants" in my description was wrong. My test harness was broken and marked everything as failing. I fixed it, re-ran it with a no-op control, and updated the description. The breaking-change label for the column is your call. |
karolpiotrowicz
left a comment
There was a problem hiding this comment.
Must change before merge:
- With the database session service, a reply whose database write fails still decides which caches are deleted. The cache the database still names is deleted, and the new one stays on the provider until its TTL. This is the
AppendEventfailure case from my earlier review, still open for that service. - A callback that writes into a map nested in its request's
CreateHTTPOptions.ExtraBodychanges the caller's config and every later request, while the new comments say the copies share nothing. Details are on the line.
The rest of my earlier list is fixed. Your new tests fail on the previous head and pass on this one, and I re-ran the same checks on this head with clean results. Driving the empty-reply, streaming, early-stop and cancellation cases through runner.Run also keeps the provider's caches in step with the session.
A failed database write still decides what gets deleted
settleContextCaches judges against the invocation's own session object (runner.go#L563). The database service appends the event to that object before its transaction runs, and the event stays there when the transaction fails (service.go#L413-L423). I reproduced it through runner.Run with the database service on SQLite and CacheIntervals: 1. During turn 3's model call a second handle appended an event to the same session, so the database rejected turn 3's reply as stale:
turn 3: failed to add event to session: stale session error ...
database's latest record names cachedContents/1, live on the provider: cachedContents/2
turn 4: database's latest record names cachedContents/3, live on the provider: cachedContents/2, cachedContents/3
The same run without the second writer stays in step on every turn. With the default CacheIntervals, the next turn would reuse the cache the database names. That cache is gone, so the turn would pay one 404 and a resend. TestRunner_ContextCacheReplyNotStored can't catch this, because its wrapper fails before the in-memory service touches the session object (context_cache_test.go#L401-L406). The reason the settle comment gives, "so that the events it judges are the ones the session actually holds" (runner.go#L555-L556), doesn't hold for this service. Compaction re-reads the session before acting, for a related reason (runner.go#L329-L336).
| } | ||
| out := *o | ||
| out.Headers = o.Headers.Clone() | ||
| out.ExtraBody = maps.Clone(o.ExtraBody) |
There was a problem hiding this comment.
maps.Clone copies only the top level, so maps nested in ExtraBody stay shared by the caller, the runner and every request. A callback can write to them: a BeforeModelCallback that set labels["x"] on its request's copy changed the caller's map and the runner's copy. Let's copy nested values too, or narrow the "shares nothing" comments here, in runner.New and in the processor.
There was a problem hiding this comment.
Fixed in 76a138d. I copied the nested values: maps and slices inside ExtraBody are now copied, so a callback writing labels["x"] no longer touches the caller's map or the runner's. The comments now say what's still shared (functions, and maps inside structs). Added a test for the labels case, and one that fails if genai adds an HTTPOptions field the copy doesn't cover.
| // it is replaced. | ||
| // | ||
| // Under a runner, a cache is deleted only once the invocation that stopped | ||
| // using it has stored its events, and only if no stored record still names |
There was a problem hiding this comment.
Non-blocking: the runner keeps only the caches named by each agent's latest record, so a replaced cache is deleted even though older records still name it. Saying "only if the agent's latest record no longer names it" would match the code, and the same applies to the settleContextCaches comment.
There was a problem hiding this comment.
Done in 76a138d. The docs and comments now say a cache is deleted only if no agent's latest stored record names it, including the settleContextCaches comment.
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| api := &fakeCacheAPI{gone: map[string]bool{"cachedContents/gone": true}, goneStatus: tt.goneStatus, goneMessage: tt.goneMessage} |
There was a problem hiding this comment.
Optional: this fake fails before sending any chunk, so the !yielded guard has no test, and neither does the delete of the missing cache on the path without a ledger.
There was a problem hiding this comment.
Added both in 76a138d: a stream that fails after sending a chunk is not retried, and the missing cache gets deleted with and without a ledger.
Address the second review on google#1951. Cleanup judged against the invocation's own session object. The database service adds an event to that object before its write and leaves it there when the write fails, so a rejected reply could still decide which caches are deleted. Cleanup now reads the session as the store holds it, as compaction does. It reads only when a cache was created or replaced, gives up after a bound, and deletes nothing when it cannot read. Also copy maps and slices nested in CreateHTTPOptions.ExtraBody, so a callback writing to them no longer changes the caller's config or later requests, and say "latest stored record" where the docs said "stored records". Tests: the database stale-write case on SQLite, an unreadable and a hung store, no extra read on a turn that only reuses a cache, nested ExtraBody writes, a guard that fails when genai adds an HTTPOptions field the copy does not handle, a stream that fails after output, and the cleanup of a missing cache with and without a ledger.
|
On the first point, the database write failing: fixed in 76a138d. Cleanup now reads the session from the session service (same |
Closes #1937
What this adds
Explicit Gemini context caching, ported from adk-python's
ContextCacheConfig,ContextCacheRequestProcessorandGeminiContextCacheManager. Withrunner.Config.ContextCacheset, an agent's system instruction, tools and stable conversation prefix are stored once as a GeminiCachedContent. Later requests name the cache instead of resending them.What changes for existing users
session/databaseusers must add a column before upgrading, even with caching off.cache_metadatacolumn oneventsis part of every insert and select.database.AutoMigrateadds it, as the package doc already requires on every startup.cache_metadatacolumn first. Otherwise inserts intoeventsfail.breaking-changelabel or a release note.Everything else is inert unless
ContextCacheis set:CacheConfigis nil, and the new request and response fields are zero.TestGenerateContent_NoContextCacheandTestRunner_ContextCacheOffpin this.CacheMetadatais written withraw_event(eventNeedsRawEvent). Events without it keep the legacy wire format, so nothing changes with caching off.AfterModelcallback or plugin that replaces a response keeps the model'sCacheMetadataunless the replacement sets its own. A callback cannot clear it.adkrestmaps event fields explicitly (server/adkrest/internal/models/event.go). Code that marshals asession.Eventdirectly gets acacheMetadatakey when the field is set.How it works
model.ContextCacheConfig:CacheIntervals,TTL,MinTokensandCreateHTTPOptions. Zero fields take adk-python's defaults (10, 30m, 0).runner.NewcallsValidateand keeps its own copy of the config, with maps and slices nested inCreateHTTPOptions.ExtraBodycopied too. Each request gets a further copy.model.CacheMetadataonmodel.LLMResponse, so the session stores it with each event. Active means a cache exists. Fingerprint-only records the prefix the next request has to share before a cache is created. It encodes as adk-python does: snake_case keys, and times as float epoch seconds (cache_metadata.py:60-98), so sessions shared with adk-python read each other's records.LLMRequest.CacheConfig,CacheMetadata,PreviousPromptTokenCount: the flow sets these and the model acts on them.internal/llminternal/context_cache_processor.go), placed after contents as in adk-python's single flow (single_flow.py:67). It finds the agent's latest cache state and prompt size in the session, and counts a new invocation against an active cache (_cache.py:143-146).model/gemini/context_cache.go), followinggemini_context_cache_manager.py::310,:229).CacheIntervals, or its prefix changes (:261).MinTokensor the model's minimum: 2048 tokens for Gemini 2.5, 4096 for Gemini 3 (:41-42,:427-437).cachedContentand without the system instruction, tools, tool config and cached contents (:649).ContextCacheLedgerininternal/llminternal/context_cache_lifecycle.go): model calls record the caches they created or stopped using. When the invocation ends, the runner reads the session as its service holds it and deletes every recorded cache that no agent's latest stored record names. If that read fails or takes over 30s, nothing is deleted and the caches expire with their TTL. It reads only when a cache was created or replaced.cache_metadatacolumn insession/database, and the field inraw_eventfor Vertex AI. adk-python keeps it through its JSON event blobs (schemas/v1.py:222,vertex_ai_session_service.py:494).RunLiveis untouched, as in adk-python.Differences from adk-python
Raised on the issue (question 3):
_cache.py:127). A parallel worker runs copies of one agent side by side, each on its own branch. Matched on author alone, each copy reads its sibling's prefix as changed and deletes a cache the sibling is still using. The scope is the boundary the contents processor already draws around the history the cache holds.New in this PR, not discussed on the issue. Each is covered by its own tests and can be dropped:
2. Under a runner, a cache is deleted when the invocation ends, and only if no agent's latest stored record names it. The record is read from the session service, not from the invocation's own session object. adk-python deletes first (
gemini_context_cache_manager.py:149). A failed call, or a callback that replaces the reply, then leaves the session naming a deleted cache, and a later request whose prefix matches again is sent with that name and rejected. The stored session is read because the database service adds an event to the invocation's object before its write and leaves it there if the write fails. Without a runner, the model deletes on its own: a replaced cache after a complete response reached the caller, and a created cache when none did.3. A request that names a cache the provider no longer holds (404, or 403 naming the cached content) is sent once more without it, if nothing has been yielded yet. The missing cache is deleted, and the next request creates a new one. adk-python has no retry.
4. An
AfterModelreplacement keeps the model'sCacheMetadata, for the same reason as 2.5. A request that already names a cache in its
GenerateContentConfigis sent as it is. adk-python overwrites it (:669).Smaller:
6. The cached fields are left on the caller's request, and a modified copy is sent. adk-python edits it in place (
:649-677). The flow and its telemetry read the request after the call.7. Nothing is logged on a failed create or delete. ADK Go library code has no logger (AGENTS.md). As in adk-python, the request goes ahead uncached, and a cache that is not deleted expires with its TTL.
8. Zero config fields take the defaults, the Go zero-value convention. adk-python validates
ge=1/gt=0(context_cache_config.py:49-65).9.
CacheMetadata.ExpireSoonis left out. Nothing calls it, here or in adk-python.Notes for reviewers
model, becauseLLMRequestandLLMResponsereference them and every model implementation needs them. A separate package would only be imported back intomodel.gemini.NewModeland so send cache calls through the proxy. A proxy that does not routecachedContentsmakes each creation fail, and requests then go uncached.:532). adk-python counts tools with their null fields included, so the two can disagree about a prefix within a few percent of the model's minimum. The provider has the final word either way.model.ContextCacheConfig(withValidate),model.CacheMetadata(withActiveand the JSON methods), the threeLLMRequestfields,LLMResponse.CacheMetadata, andrunner.Config.ContextCache. All are additive.Follow-ups
agent_tool.py:294-307does. It is separable, and written.launcher.Configandadkrest.ServerConfigplumbing, asCompactionhas, soadk webusers can turn caching on.StaticInstructiononllmagent.Testing plan
The Gemini API is faked with
httptestrather than recorded, so no test makes a network call.model/cache_test.go: validation bounds; the JSON wire form both ways, against adk-python's exact output.internal/llminternal:ExtraBodymaps and slices, and a guard that fails ifgenaiadds anHTTPOptionsfield the copy does not handle.model/gemini/context_cache_test.go:runner/context_cache_test.go:runner.Run,llmagentand the Gemini model, under anllmagent, asequentialagentand aparallelagentroot; a callback rewriting the reply, caching off, validation and config copying;ExtraBodymap.session/database,session/vertexai:CacheMetadataround trips, and rows with no cache state.-race -shuffle=onon Go 1.26.10, andgolangci-lintv2.3.1 reports nothing in the changed files.gemini-3.5-flash) with a free-tier key, logging each call's method, path and status.POST /v1beta/cachedContents, holding the first exchange.429 RESOURCE_EXHAUSTED,TotalCachedContentStorageTokensPerModelFreeTier ... limit=0, requested=8406. Therequestedcount shows the API parsed the cache body.cachedContent, because that needs a billing-enabled key. If a reviewer has one,go run ./examples/contextcache/exercises that path in three turns.