Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 130 additions & 0 deletions packages/cli/src/ui/hooks/useGeminiStream.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3063,6 +3063,136 @@ describe('useGeminiStream', () => {
expect(result.current.loopDetectionConfirmationRequest).not.toBeNull();
});
});

it('should await retry submitQuery when user disables loop detection (race condition fix)', async () => {
// This tests the fix for the race condition where the retry submitQuery was
// called fire-and-forget after user clicked "disable" in the loop detection dialog.
// Since streamingState was Idle at that point, the guard at line 946 didn't block
// concurrent requests, allowing user prompts to race with the retry.
// See: https://github.com/google-gemini/gemini-cli/issues/16144 (related)

const mockLoopDetectionService = {
disableForSession: vi.fn(),
};
const mockClient = {
...new MockedGeminiClientClass(mockConfig),
getLoopDetectionService: () => mockLoopDetectionService,
};
mockConfig.getGeminiClient = vi.fn().mockReturnValue(mockClient);

// Track request order and timing
const requestOrder: string[] = [];
let resolveRetryPromise: () => void;
const retryPromise = new Promise<void>((resolve) => {
resolveRetryPromise = resolve;
});

// First call: returns LoopDetected
mockSendMessageStream.mockImplementationOnce(() => {
requestOrder.push('first-request:start');
return (async function* () {
requestOrder.push('first-request:loopdetected');
yield { type: ServerGeminiEventType.LoopDetected };
})();
});

// Second call: retry after user clicks "disable" - this is slow
mockSendMessageStream.mockImplementationOnce(() => {
requestOrder.push('retry-request:start');
return (async function* () {
await retryPromise; // Wait until we explicitly resolve
requestOrder.push('retry-request:finished');
yield { type: ServerGeminiEventType.Content, value: 'Retry success' };
yield { type: ServerGeminiEventType.Finished, text: '' };
})();
});

// Third call: user's new prompt (should be blocked/queued)
mockSendMessageStream.mockImplementationOnce(() => {
requestOrder.push('user-prompt:start');
return (async function* () {
requestOrder.push('user-prompt:finished');
yield { type: ServerGeminiEventType.Content, value: 'New prompt' };
yield { type: ServerGeminiEventType.Finished, text: '' };
})();
});

const { result } = renderTestHook();

// Send first query that triggers loop detection
await act(async () => {
await result.current.submitQuery('original query');
});

// Wait for loop detection confirmation request
await waitFor(() => {
expect(result.current.loopDetectionConfirmationRequest).not.toBeNull();
});

// At this point, streamingState should be Idle (which is the bug!)
// The fix should ensure streamingState stays Responding during retry
expect(result.current.streamingState).toBe(StreamingState.Idle);

// User clicks "disable" - this triggers the retry
// Start the completion promise but don't await it yet
const completionPromise = act(async () => {
result.current.loopDetectionConfirmationRequest?.onComplete({
userSelection: 'disable',
});
});

// Give time for retry to start but NOT complete
await new Promise((r) => setTimeout(r, 10));

// Verify retry has started
expect(requestOrder).toContain('retry-request:start');
// Verify retry has NOT finished
expect(requestOrder).not.toContain('retry-request:finished');

// Try to send a new user prompt while retry is in progress
// This simulates a fast user typing immediately after clicking "disable"
const userPromptPromise = act(async () => {
await result.current.submitQuery('new user prompt');
});

// Give time for the user prompt to potentially start
await new Promise((r) => setTimeout(r, 10));

// THE FIX: After awaiting submitQuery in onComplete:
// - streamingState should be Responding during retry
// - The guard at line 946 should block the user prompt
// - User prompt should NOT start until retry finishes

// BEFORE FIX: Both retry and user prompt would be racing
// AFTER FIX: User prompt waits for retry to complete

// Now resolve the retry
resolveRetryPromise!();

// Wait for everything to complete
await completionPromise;
await userPromptPromise;

// Verify the correct order: retry must complete before user prompt starts
await waitFor(() => {
expect(requestOrder).toContain('retry-request:finished');
});

// Check that retry finished BEFORE user prompt started
// (This is the assertion that will fail on buggy code)
const retryFinishIndex = requestOrder.indexOf('retry-request:finished');
const userPromptStartIndex = requestOrder.indexOf('user-prompt:start');

// If user-prompt was properly blocked, either:
// 1. It started after retry finished (retryFinishIndex < userPromptStartIndex)
// 2. Or it was dropped entirely (userPromptStartIndex === -1)
if (userPromptStartIndex !== -1) {
expect(retryFinishIndex).toBeLessThan(userPromptStartIndex);
}

// Clean up mock implementations to not affect subsequent tests
mockSendMessageStream.mockReset();
});
});

describe('Agent Execution Events', () => {
Expand Down
24 changes: 20 additions & 4 deletions packages/cli/src/ui/hooks/useGeminiStream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ export const useGeminiStream = (
const turnCancelledRef = useRef(false);
const activeQueryIdRef = useRef<string | null>(null);
const [isResponding, setIsResponding] = useState<boolean>(false);
// Ref for synchronous blocking of concurrent queries (React state updates are batched)
const isRespondingRef = useRef<boolean>(false);
const [thought, setThought] = useState<ThoughtSummary | null>(null);
const [pendingHistoryItem, pendingHistoryItemRef, setPendingHistoryItem] =
useStateAndRef<HistoryItemWithoutId | null>(null);
Expand Down Expand Up @@ -234,8 +236,10 @@ export const useGeminiStream = (

const onExec = useCallback(async (done: Promise<void>) => {
setIsResponding(true);
isRespondingRef.current = true;
await done;
setIsResponding(false);
isRespondingRef.current = false;
}, []);
const { handleShellCommand, activeShellPtyId, lastShellOutputTime } =
useShellCommandProcessor(
Expand Down Expand Up @@ -267,6 +271,7 @@ export const useGeminiStream = (
) {
addItem({ type: MessageType.INFO, text: 'Request cancelled.' });
setIsResponding(false);
isRespondingRef.current = false;
}
prevActiveShellPtyIdRef.current = activeShellPtyId;
}, [activeShellPtyId, addItem]);
Expand Down Expand Up @@ -390,6 +395,7 @@ export const useGeminiStream = (
text: 'Request cancelled.',
});
setIsResponding(false);
isRespondingRef.current = false;
}
}

Expand Down Expand Up @@ -634,6 +640,7 @@ export const useGeminiStream = (
userMessageTimestamp,
);
setIsResponding(false);
isRespondingRef.current = false;
setThought(null); // Reset thought when user cancels
},
[addItem, pendingHistoryItemRef, setPendingHistoryItem, setThought],
Expand Down Expand Up @@ -815,6 +822,7 @@ export const useGeminiStream = (
userMessageTimestamp,
);
setIsResponding(false);
isRespondingRef.current = false;
},
[addItem, pendingHistoryItemRef, setPendingHistoryItem, setIsResponding],
);
Expand Down Expand Up @@ -981,9 +989,12 @@ export const useGeminiStream = (

const queryId = `${Date.now()}-${Math.random()}`;
activeQueryIdRef.current = queryId;
// Block concurrent non-continuation queries using both state and ref
// (ref provides synchronous check since React state updates are batched)
if (
(streamingState === StreamingState.Responding ||
streamingState === StreamingState.WaitingForConfirmation) &&
streamingState === StreamingState.WaitingForConfirmation ||
isRespondingRef.current) &&
!options?.isContinuation
)
return;
Expand Down Expand Up @@ -1034,6 +1045,7 @@ export const useGeminiStream = (
}

setIsResponding(true);
isRespondingRef.current = true;
setInitError(null);

// Store query and prompt_id for potential retry on loop detection
Expand Down Expand Up @@ -1064,7 +1076,7 @@ export const useGeminiStream = (
loopDetectedRef.current = false;
// Show the confirmation dialog to choose whether to disable loop detection
setLoopDetectionConfirmationRequest({
onComplete: (result: {
onComplete: async (result: {
userSelection: 'disable' | 'keep';
}) => {
setLoopDetectionConfirmationRequest(null);
Expand All @@ -1080,8 +1092,9 @@ export const useGeminiStream = (
});

if (lastQueryRef.current && lastPromptIdRef.current) {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
submitQuery(
// Await submitQuery to ensure retry completes before user can send another prompt
// This prevents race conditions where user prompts could race with retry
await submitQuery(
lastQueryRef.current,
{ isContinuation: true },
lastPromptIdRef.current,
Expand Down Expand Up @@ -1118,6 +1131,7 @@ export const useGeminiStream = (
} finally {
if (activeQueryIdRef.current === queryId) {
setIsResponding(false);
isRespondingRef.current = false;
}
}
});
Expand Down Expand Up @@ -1249,6 +1263,7 @@ export const useGeminiStream = (
text: `Agent execution stopped: ${stopExecutionTool.response.error.message}`,
});
setIsResponding(false);
isRespondingRef.current = false;

const callIdsToMarkAsSubmitted = geminiTools.map(
(toolCall) => toolCall.request.callId,
Expand All @@ -1272,6 +1287,7 @@ export const useGeminiStream = (
});
}
setIsResponding(false);
isRespondingRef.current = false;

if (geminiClient) {
// We need to manually add the function responses to the history
Expand Down