-
Notifications
You must be signed in to change notification settings - Fork 9.4k
fix(cli): prevent race condition when restoring prompt after context overflow #13473
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
Conversation
Summary of ChangesHello @SandyTao520, 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 resolves a critical race condition within the CLI that caused the input prompt to display an incorrect, outdated message after a context window overflow warning. By implementing a deferred restoration mechanism, the system now correctly waits for the latest user message state to synchronize before restoring the prompt, ensuring users always see their current input. 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
|
|
Size Change: +722 B (0%) Total Size: 21.1 MB ℹ️ View Unchanged
|
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 aims to fix a race condition when restoring a user's prompt after a context window overflow. The approach of introducing a pendingRestorePrompt state and a useEffect to handle the asynchronous nature of the state updates is sound. However, the implementation of the new useEffect hook introduces a potential deadlock. It relies on the userMessages state to be in sync with historyManager.history, but the effect that syncs them is asynchronous and lacks error handling. If that sync fails, the prompt restoration will never occur, leaving the UI in a stuck state. I've proposed a more robust implementation for the useEffect that removes this fragile dependency and makes the logic self-contained.
|
Great catch on the race condition! The solution correctly handles the asynchronous nature of the The implementation of The added regression test in Code quality looks good and follows the project patterns. Verification:
LGTM! 🚀 |
|
Thank you for the detailed analysis. While I acknowledge the theoretical risk if I prefer to maintain the current implementation to keep the restoration logic centralized within |
|
The current implementation effectively mitigates the race condition by waiting for the state to settle. However, I see an opportunity to simplify this significantly and remove the race condition entirely by passing the prompt text directly. In In const handleContextWindowWillOverflowEvent = useCallback(
(estimatedRequestTokenCount: number, remainingTokenCount: number) => {
// Pass the actual text that caused the overflow
const textToRestore = typeof lastQueryRef.current === 'string' ? lastQueryRef.current : undefined;
onCancelSubmit(textToRestore);
// ...In const onCancelSubmit = useCallback((restoredText?: string) => {
// ...
cancelHandlerRef.current(restoredText);
}, []);
// Update cancelHandlerRef to use the provided text directly
// instead of looking up userMessages.at(-1)This approach:
Would you consider this refactor? It would likely require updating the test case to verify that the passed text is restored, rather than simulating the history timing. |
jacob314
left a comment
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.

TLDR
Fixes a bug where the input prompt was restored with stale text (the previous message) instead of the current overflowing message due to a race condition between the
ContextWindowWillOverflowevent and theuserMessagesstate update.Dive Deeper
The
ContextWindowWillOverflowevent triggersonCancelSubmitsynchronously. However,userMessages(used to retrieve the text to restore) is updated asynchronously via auseEffectobserving history changes. This PR introduces apendingRestorePromptstate to defer the restoration untiluserMessagesreflects the latest history item.Reviewer Test Plan
ContextWindowWillOverflow.npm run test --workspace packages/cli -- src/ui/AppContainer.test.tsxTesting Matrix
Linked issues / bugs