fix: Add isRespondingRef for synchronous blocking and await the retry #20261
fix: Add isRespondingRef for synchronous blocking and await the retry #20261ishaanxgupta wants to merge 10 commits into
Conversation
Summary of ChangesHello @ishaanxgupta, 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 refines the state management for query submission and response handling, primarily by introducing a synchronous reference ( Highlights
Changelog
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 introduces a synchronous lock using useRef to prevent race conditions when submitting queries, which is a solid approach. The implementation is mostly correct, with the new setRespondingState helper being consistently applied. However, I've identified a critical issue where the new lock is not released in all code paths, which can lead to a deadlock and make the UI unresponsive. My review includes a comment detailing the scenarios and providing recommendations to fix it.
gemini-cli
Bot
added
priority/p1
|
@scidomino please have a look |
|
@ishaanxgupta Sorry, due to overwhelming demand, I am no longer reviewing external PRs unless:
|
|
Orb Code Review (powered by GLM-4.7 on Orb Cloud) SummaryThis PR addresses a race condition in synchronous query submission by introducing ArchitectureCore Fix (packages/cli/src/ui/hooks/useGeminiStream.ts):The Problem: // Before (race condition):
if (isRespondingRef.current || streamingState === StreamingState.Responding) {
return; // Check happens before ref is set
}
// ... later in submitQuery:
setIsResponding(true); // Async state update - not effective for same-tick blocksThe Solution:
const isRespondingRef = useRef(false);
const [isResponding, setIsResponding] = useState(false);
const setRespondingState = useCallback((value: boolean) => {
isRespondingRef.current = value; // Synchronous ref update
setIsResponding(value); // React state update
}, []);
if (
(isRespondingRef.current || streamingState === StreamingState.Responding ||
streamingState === StreamingState.WaitingForConfirmation) &&
!options?.isContinuation
) {
return;
}
+
+isRespondingRef.current = true; // Synchronous block for same-tick submissions
+setIsResponding(true);
// Before: blocked retries if turn cancelled or not responding
if (turnCancelledRef.current || !isRespondingRef.current) {
return;
}
// After: always show retry status
setRetryStatus(payload);Bundled Cleanup (massive scope):Deleted subsystems:
CLI startup simplification:
IssuesPR scope — critical — Bundles unrelated massive cleanup with focused bug fix The PR mixes two completely different changes:
Impact:
Recommendation: Split into two separate PRs:
packages/cli/src/ui/hooks/useGeminiStream.ts — good — Correct use of ref pattern The core fix correctly addresses the race condition: // Good: Synchronous ref update immediately after guard check
if (
(isRespondingRef.current || streamingState === StreamingState.Responding ||
streamingState === StreamingState.WaitingForConfirmation) &&
!options?.isContinuation
) {
return;
}
+
+isRespondingRef.current = true; // Blocks same-tick submissions
+setIsResponding(true);This ensures that even if React hasn't processed the state update yet, the ref check will block duplicate submissions. setRespondingState callback — minor — Could be inlined The // Current pattern:
const setRespondingState = useCallback((value: boolean) => {
isRespondingRef.current = value;
setIsResponding(value);
}, []);
// Simpler alternative (if only used once):
// Just set both directly where neededHowever, the callback pattern is fine if it's used in multiple places (the diff shows it's passed to handleRetryAttempt change — warning — May show retries when inappropriate Removing the // Before: blocked retries if not responding
if (turnCancelledRef.current || !isRespondingRef.current) {
return; // Don't show retry if we're not responding
}This change appears intentional to allow showing retry status, but it could lead to UI flicker or confusing retry notifications after the stream has completed. Consider adding back a more targeted check: if (turnCancelledRef.current) {
return; // Still block if turn was cancelled
}
// Allow retry notifications even if not currently responding
setRetryStatus(payload);Cleanup scope — warning — No backward compatibility assessment The massive cleanup removes:
Missing:
CLI startup change — warning — Removes auto memory configuration The parent/daemon process pattern that auto-configured memory ( // Before (removed):
async function getMemoryNodeArgs(): Promise<string[]> {
// ... calculated targetMaxOldSpaceSizeInMB ...
return [`--max-old-space-size=${targetMaxOldSpaceSizeInMB}`];
}
// After: Direct call to main()
import { main } from './src/gemini.js';
main().catch(...);Impact:
Recommendation: Either:
Testing — critical — No tests added for race condition fix The core fix addresses a race condition, but there are no tests added to verify:
Suggested test: it('should block same-tick submissions', async () => {
const { result } = renderHook(() => useGeminiStream(...));
const submitQuery = result.current.submitQuery;
// Submit query twice in same tick
submitQuery('query1');
submitQuery('query2');
await waitFor(() => expect(isRespondingRef.current).toBe(true));
// Verify only one submission was processed
expect(activeQueryIdRef.current).toBeDefined();
});Documentation — warning — No changelog or migration guide The PR removes entire features (ACP, gemma, perf tests) but adds no documentation about:
Cross-file Impact
AssessmentThe core fix (synchronous blocking with Required changes:
Once split into appropriate PRs:
This approach allows for:
|
|
Thank you for your interest in contributing to the project! We are closing this PR as the associated issue was closed. |
6 participants
Summary
Related Issues
fixes: #17071
Pre-Merge Checklist