fix(cli): race conditions in useGeminiStream parallel tool handling #17057
Conversation
…lel tool failures After multi-step MCP tools like sequentialthinking completed, subsequent parallel tool calls failed with 400 error about function response part count mismatch. Root cause: handleCompletedTools captured stale toolCalls reference, making the responseSubmittedToGemini check ineffective. Fix: Add toolCalls to useCallback dependency array and check responseSubmittedToGemini before including tools in the response batch. Fixes google-gemini#16135
Adds test verifying that tools with responseSubmittedToGemini=true are filtered out and not resubmitted, preventing the unrecoverable 400 error that corrupts the entire session.
Race condition: markToolsAsSubmitted() was called BEFORE submitQuery() completed, allowing user prompts to race ahead of tool responses. The fire-and-forget pattern (eslint-disable @typescript-eslint/no-floating-promises) caused streamingState to become Idle before the API received tool responses, creating a window where new prompts could jump the queue. Fix: await submitQuery() completion, THEN call markToolsAsSubmitted(). Smoking gun: useGeminiStream.ts:1278-1292 (895845a) Fixes google-gemini#16144, google-gemini#16216, google-gemini#16068, google-gemini#15239, google-gemini#16135
Verifies that markToolsAsSubmitted is called AFTER submitQuery completes, not before. This prevents user prompts from racing ahead of tool responses. References: google-gemini#16144
Summary of ChangesHello @jw409, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on resolving critical race conditions within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request provides clear and effective fixes for two race conditions in useGeminiStream.ts. The changes to address the stale closure in handleCompletedTools and to correctly await the submitQuery before updating state are well-implemented. The accompanying regression tests are thorough and accurately verify that the race conditions are resolved. The code is high-quality, and I have no further suggestions.
gemini-cli
bot
added
priority/p2
|
Fixes #17067 |
| // Check if we've already submitted this tool call. | ||
| // We need to look up the tracked version because the incoming 'tc' | ||
| // comes directly from the scheduler core and lacks the | ||
| // 'responseSubmittedToGemini' flag. | ||
| const trackedToolCall = toolCalls.find( | ||
| (t) => t.request.callId === tc.request.callId, | ||
| ); | ||
| if (trackedToolCall?.responseSubmittedToGemini) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Happy Path (Works Fine)
t=0 Tool A and Tool B start
t=1 Tool A completes
→ handleCompletedTools([A]) fires
→ Submits A, marks A as submitted
→ React re-renders
→ useCallback recreates handleCompletedTools with fresh toolCalls
t=2 Tool B completes (after React re-rendered)
→ NEW handleCompletedTools([A, B]) fires
→ Callback sees toolCalls where A.responseSubmittedToGemini = true
→ Filters out A, only submits B ✓
Unhappy Path (Race Condition)
t=0 Tool A and Tool B start
t=1 Tool A completes
→ handleCompletedTools([A]) fires
→ Submits A, calls markToolsAsSubmitted(["A"])
→ State update is QUEUED (async)
t=1.001 Tool B completes BEFORE React re-renders
→ SAME OLD handleCompletedTools([A, B]) fires
→ This callback has a STALE CLOSURE - it captured toolCalls
from before A was marked as submitted
→ Callback sees A.responseSubmittedToGemini = false (stale!)
→ Submits A again 💥400 error
The Core Issue: Stale Closure
The useCallback captures toolCalls at creation time. If toolCalls wasn't in the dependency array (part of the fix), the callback never gets recreated with fresh state - it always sees the original toolCalls.
Even with toolCalls in deps, there's still a window between:
- markToolsAsSubmitted() being called
- React re-rendering
- New callback being created
If another tool completes in that window, the old callback runs with stale state.
In practice on modern computers in unloaded state, the happy path happens the overwhelming majority of the time. If my system is swapping to death because I'm out of ram, or a ran a make -j 32 on an 8 core system and it's pegged, this could happen.
It's an edge case optimization for correctness, and I will readily admit I don't understand the nuances of the react ui loop and how it interacts with the various scheduling layers. I'm trying to help fix all these 400 parallel tool calling bugs, and this is one of the PR's I've split out from the original larger pr that was muddled, but in my mind they are all related at treating the same symptom.
|
Hi there! Thank you for your contribution to Gemini CLI. We really appreciate the time and effort you've put into this pull request. To keep our backlog manageable and ensure we're focusing on current priorities, we are closing pull requests that haven't seen maintainer activity for 30 days. Currently, the team is prioritizing work associated with 🔒 maintainer only or help wanted issues. If you believe this change is still critical, please feel free to comment with updated details. Otherwise, we encourage contributors to focus on open issues labeled as help wanted. Thank you for your understanding! |
4 participants
Summary
Fixes race conditions in
useGeminiStream.tsthat cause unrecoverable 400 errors during parallel function calling. This PR is extracted from #16146 per reviewer feedback requesting separation of concerns. It contains ONLY theuseGeminiStream.tsfixes and their tests. ## Single Concern: useGeminiStream Race Conditions All changes are inpackages/cli/src/ui/hooks/: -useGeminiStream.ts(2 fixes) -useGeminiStream.test.tsx(2 regression tests) ### Fix 1: Stale closure inhandleCompletedToolsProblem:handleCompletedToolscaptured staletoolCallsstate, causing already-submitted tools to be resubmitted when the callback fired. Fix: AddtoolCallsto dependency array and filter out tools whereresponseSubmittedToGemini=true. ### Fix 2: Fire-and-forgetsubmitQueryProblem:markToolsAsSubmitted()was called BEFOREsubmitQuery()completed. This setstreamingStatetoIdleprematurely, allowing user prompts to race ahead of tool responses. Fix:await submitQuery()completion, THEN callmarkToolsAsSubmitted(). ## What's NOT in this PR (vs #16146) Per reviewer request, these were removed to separate concerns: -local-executor.tsresponse orderingmcp-client.test.tsOAuth fixgenerateContentResponseUtilities.tsbinary contentresponseSubmittedToGemini=truetools are filtered - [x] Added regression test: verifiesmarkToolsAsSubmittedwaits forsubmitQuery- [x] Runnpm run preflightFixes #17067