-
Notifications
You must be signed in to change notification settings - Fork 10.8k
fix(cli): prevent message queue interleaving by awaiting tool result submission #17199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @imadraude, 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 addresses a critical timing issue in the Gemini CLI where user messages could appear out of order when the AI model was processing tool calls. The core problem stemmed from unawaited asynchronous calls, leading to an inconsistent state that allowed the message queue to inject messages prematurely. The fix ensures that all relevant asynchronous operations complete before the system processes new user input, thereby maintaining the correct conversational flow and preventing message interleaving. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses a race condition where queued messages could appear out of order during tool execution. The fix involves correctly awaiting the recursive submitQuery calls, ensuring the application state remains consistent throughout the asynchronous operations. The changes are well-targeted, and the necessary adjustments to test mocks have been made. The fix appears correct and complete.
4170635 to
b4c42b1
Compare
|
I have reset the branch to its initial state to remove unintended merge commits from |
|
Looks like there is an existing PR for this that tries to solve the same race condition: #17057 Although the account that created the other PR seems like someones bot 😆 I am not sure how your test update is testing the race condition? The other PR is a bit overly complex for the fix as well i believe, but it is definitely good to test. In my eyes isn't this the proper fix? (from other users PR)
Awaiting the submit, and then marking as submitted... Please give me details on your approach here as I compare the two solutions 😄 |
|
Thanks for the questions, @jackwotherspoon! It's great to compare approaches and ensure we have the most robust solution. You've hit on the core of the fix, and the image you provided from the other PR actually illustrates the same fundamental principle that this PR implements. The key issue was indeed unawaited asynchronous calls, specifically How this PR addresses the race condition:
By awaiting How the test updates verify the fix:The updates to
These test updates don't directly test the race condition itself in terms of timing, but they enable the existing and new tests to run successfully against the modified asynchronous logic. The original tests for In essence, both this PR and the one you linked are aiming for the same solution: ensuring that the |
|
Updated the PR to await submitQuery before marking tools as submitted to ensure state consistency. Added regression tests to verify the fix and filtered out already-submitted tools in both the main and loop detection flows. This should be all set now. |
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly identifies and fixes a race condition by awaiting submitQuery calls, preventing message queue interleaving. The changes are logical and the new tests provide good coverage for the fix. However, I've identified a significant issue with the new implementation where tools are marked as submitted even if the submitQuery call fails internally (e.g., due to a network error), which can lead to lost conversational context. I've provided a detailed comment with a suggested refactoring to address this. This should be addressed to ensure the reliability of tool call handling.
| await submitQuery( | ||
| responsesToSend, | ||
| { | ||
| isContinuation: true, | ||
| }, | ||
| prompt_ids[0], | ||
| ); | ||
|
|
||
| markToolsAsSubmitted(callIdsToMarkAsSubmitted); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While awaiting submitQuery is a great step towards fixing the race condition, calling markToolsAsSubmitted unconditionally afterwards can lead to data loss. If submitQuery encounters an error internally (like a network issue), it will handle the error and not re-throw, but the tool results will not have been successfully sent to the model. This implementation would then incorrectly mark the tools as submitted, leading to lost context in the conversation.
A more robust approach is to move the responsibility of marking tools as submitted into submitQuery itself, so it's only done upon successful completion of the stream.
This would involve a few changes:
-
Modify
submitQueryto accept an optionaltoolCallIdsToMarkarray in its options. -
In
handleCompletedTools, pass the IDs tosubmitQueryand remove themarkToolsAsSubmittedcall.// in handleCompletedTools await submitQuery( responsesToSend, { isContinuation: true, toolCallIdsToMark: callIdsToMarkAsSubmitted, }, prompt_ids[0], );
-
Inside
submitQuery, callmarkToolsAsSubmittedonly after the stream processing is fully successful and not in a loop detection path. Also, passtoolCallIdsToMarkalong in the recursivesubmitQuerycall within the loop detection logic.// in submitQuery // ... try { // ... if (loopDetectedRef.current) { // ... setLoopDetectionConfirmationRequest({ onComplete: async (result) => { if (result.userSelection === 'disable') { // ... await submitQuery( lastQueryRef.current, { isContinuation: true, toolCallIdsToMark: options?.toolCallIdsToMark }, lastPromptIdRef.current, ); } }, }); } else { // No loop detected, request was successful. Mark tools as submitted. if (options?.toolCallIdsToMark) { markToolsAsSubmitted(options.toolCallIdsToMark); } } } catch (e) { // On error, tools are not marked as submitted. } // ...
Ensure tools are only marked as submitted if submitQuery succeeds, preventing lost conversational context on network errors.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses a critical race condition where queued messages could interleave with tool call responses. The root cause was identified as unawaited submitQuery calls, which has been fixed by adding await and adjusting the function signatures to support asynchronous operations. The changes are well-reasoned and correctly implemented. Key improvements include:
- Awaiting
submitQuerycalls withinhandleCompletedToolsand the loop detection logic, ensuring proper execution order. - Conditionally calling
markToolsAsSubmittedonly after a successful submission, which resolves the race condition. - Adding a defensive check in
handleCompletedToolsto prevent resubmission of already processed tool calls, which fixes a potential 400 error. - Updating the dependency array for the
handleCompletedToolshook to prevent stale closures.
The accompanying test changes are thorough, with new tests specifically validating the race condition fix and the prevention of tool resubmission. The overall changes are excellent and significantly improve the robustness of tool handling in the CLI.
|
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! |

Summary
This PR fixes a bug where queued messages could interleave with Gemini's response when tool calls were involved.
Details
The issue was caused by not awaiting the recursive
submitQuerycalls inuseGeminiStream.ts. When a tool call completed,submitQuerywas called withoutawait, causing theisRespondingstate to momentarily flip tofalse(Idle). This allowed theuseMessageQueuehook to trigger and inject queued messages before the model's next turn could start.Changes:
awaittosubmitQuerycalls inhandleCompletedToolsand loop detection logic.onCompletecallback async to supportawait.useGeminiStream.test.tsxby providing a concreteValidationRequiredErrorclass andgetCurrentSequenceModelmethod in the@google/gemini-cli-coremock.Related Issues
None.
How to Validate
@include GEMINI.md).Alternatively, run the updated tests:
npm test -w @google/gemini-cli -- src/ui/hooks/useGeminiStream.test.tsxPre-Merge Checklist
Fixes #17282