fix: reset web search flag when recycling pooled gemini responses stream state - #5114
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesGemini web search stream recycling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 5/5This looks safe to merge.
Important Files Changed
Reviews (1): Last reviewed commit: "fix: reset web search flag when recyclin..." | Re-trigger Greptile |
Summary
Fixes #5113. The Gemini responses stream converter emits grounding metadata as a
web_search_calloutput item (queries, source list, lifecycle events) plus a rendered content message, guarded by aHasEmittedWebSearchflag on the pooledGeminiResponsesStreamStateso a stream emits the item only once.flush(), the reset bothacquireGeminiResponsesStreamStateandreleaseGeminiResponsesStreamStaterun when recycling a state object, clears every field of the struct except that flag: it was added with the web search tool support in #1371 afterflush()was last touched. The result is that only the first grounded streaming request served by a pooled object ever emits the web search lifecycle; every later grounded stream on a recycled object silently loses theweb_search_callitem, the sources, and the search suggestions block, both on the client SSE stream and in the accumulated message that feeds logging (items are appended onoutput_item.added). Vertex is affected too since it sharesHandleGeminiResponsesStream. The non-streaming path and the chat completions surface are unaffected.A CodeRabbit review comment on #2590 flagged this missing reset back in April with the same one-line suggestion, but that PR targets a different problem (safety filtering), is based on
main, has been inactive since April, and never picked the suggestion up, so this PR carries the fix ondevwith regression tests and a live reproduction.Changes
HasEmittedWebSearchinGeminiResponsesStreamState.flush(), alongside the other lifecycle flags, so a recycled state object emits web search events again. The flag keeps its in-stream deduplication role: it is still set after the first emission within a stream and only cleared when the state is recycled.dev: a direct flush contract test (a state with the flag set is fully reset), and a recycle test that replays the same grounded chunk sequence twice throughToBifrostResponsesStreamwith aflush()in between (equivalent to the reset both release and acquire run) and asserts the second stream produces the same event lifecycle as the first, including the completedweb_search_callitem with its queries and sources. Ondevthe second stream emits 10 events instead of 17, dropping the whole web search lifecycle and the rendered content pair.Type of change
Affected areas
How to test
Both tests fail on
devand pass with this change: the flush test because the flag survives the reset, the recycle test because the second grounded stream is missing all seven web-search-related events (output_item.added, the threeweb_search_call.*lifecycle events, theoutput_item.donecarrying queries and sources, and the rendered content added/done pair). The full./providers/gemini/suite is green, including the new tests under the race detector.Also verified end to end against a real Gemini backend (gemini-2.5-flash with the
google_searchtool, gateway run withGOMAXPROCS=1so pool reuse is deterministic for back-to-back requests): ondevthe first grounded streamed request carries 8web_search_calloccurrences on the client SSE plus the rendered content message, the second and third identical requests carry zero of each, and the logged accumulated message loses the item the same way, while the identical non-streamed request keeps the item every time. With this change all three streamed runs carry the full web search lifecycle, and non-streamed behavior is unchanged.Screenshots/Recordings
N/A
Breaking changes
The change is one flag reset on the pool recycle path. Streams without grounding are unchanged, and within a single stream the flag still deduplicates web search emission exactly as before.
Related issues
Fixes #5113
The missing reset was previously flagged in a review comment on #2590, which never picked it up.
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelines