fix(core): implement explicit 60s api timeout to prevent hangs #18031
Conversation
…TIMEDOUT on timeout.
Summary of ChangesHello @kevin-ramdass, 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 introduces a crucial reliability enhancement by implementing an explicit client-side timeout for API calls within the core 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://cdn.statically.io/img/avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an explicit client-side timeout for API calls, which is a great improvement to prevent the application from hanging. The implementation correctly uses AbortSignal.timeout and AbortSignal.any to combine signals. However, there is a critical flaw in where the timeout error is handled, which will prevent it from working as intended in the real application. I've left a detailed comment on the specific block of code, emphasizing the correct use of AbortSignal for cancellation and timeout handling.
| try { | ||
| return await this.config.getContentGenerator().generateContentStream( | ||
| { | ||
| model: modelToUse, | ||
| contents: contentsToUse, | ||
| config, | ||
| }, | ||
| prompt_id, | ||
| ); | ||
| } catch (error) { | ||
| if (timeoutSignal.aborted) { | ||
| const timeoutError = new Error( | ||
| `Request timed out after ${TIMEOUT_MS}ms`, | ||
| ); | ||
| (timeoutError as unknown as { code: string }).code = 'ETIMEDOUT'; | ||
| throw timeoutError; | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
This try...catch block incorrectly assumes the timeout error will be thrown during the creation of the async generator. However, generateContentStream returns a generator, and the underlying network request that can time out only happens when this generator is consumed.
The consumption happens later in processStreamResponse, so the timeout error will be thrown there and propagate to a catch block that doesn't have access to timeoutSignal. This means the timeout won't be correctly identified and retried.
The fix involves moving this try...catch logic to where the stream is consumed inside processStreamResponse. You'll need to pass timeoutSignal down to processStreamResponse to check timeoutSignal.aborted in the new catch block.
return await this.config.getContentGenerator().generateContentStream(
{
model: modelToUse,
contents: contentsToUse,
config,
},
prompt_id,
);References
- The comment highlights an issue where the
AbortSignal(referred to astimeoutSignal) is not being correctly leveraged for identifying and retrying timeout errors due to incorrecttry...catchplacement. This aligns with the principle of relying onAbortSignalfor cancellation and consistent timeout handling in asynchronous operations, rather than implementing separate or incorrectly placed timeout mechanisms.
|
Size Change: +1.23 kB (+0.01%) Total Size: 23.6 MB
ℹ️ View Unchanged
|
gemini-cli
bot
added
area/core
…ntent generation and update mock to return an AsyncGenerator.
1 participant
Summary
Implemented an explicit 60-second client-side timeout for API calls to prevent the default 5-minute hang when the backend/proxy is unresponsive.
Details
TIMEOUT_MS(60s) constant to packages/core/src/core/geminiChat.ts.generateContentStreamwithAbortSignal.timeoutmerged with the user's abort signal.ETIMEDOUTcode to ensure they are caught by retryWithBackoff and automatically retried.should throw ETIMEDOUT when request times outin geminiChat.test.ts.Related Issues
Fixes #18030
How to Validate
Run the specific unit test for this feature:
npm test -w @google/gemini-cli-core -- src/core/geminiChat.test.tsPre-Merge Checklist