-
Notifications
You must be signed in to change notification settings - Fork 8.6k
fix(core): add retry logic for specific fetch errors #11066
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 @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 significantly enhances the resilience of the Gemini CLI by implementing a configurable retry mechanism for specific network-related fetch errors. By introducing a new CLI flag and integrating the logic into the core retry utility, the CLI can now gracefully handle transient network connectivity issues, leading to a more stable and reliable user experience during API interactions. 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 introduces retry logic for specific 'fetch failed' errors, controlled by a new --retry-fetch-errors
flag. The implementation correctly threads the new configuration from the CLI down to the retryWithBackoff
utility. However, there are two high-severity issues that should be addressed. First, the retry condition relies on a hardcoded error message string, which is brittle and could lead to silent failures if the error format changes. Second, the new test case intended to validate this feature mocks the retryWithBackoff
function, meaning the new retry logic is not actually being tested. The test should be updated to test the real implementation or a new unit test should be added.
Size Change: +926 B (+0.01%) Total Size: 17.8 MB ℹ️ View Unchanged
|
}); | ||
|
||
describe('Fetch error retries', () => { | ||
const fetchErrorMsg = 'exception TypeError: fetch failed sending request'; |
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.
Can we specify the fetch error with a Type rather than a String?
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.
The SDK throws away error details: https://github.com/googleapis/js-genai/blob/58133cdbc2971c09c5ba7358da82b18b25e160eb/src/_api_client.ts#L600
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.
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.
Thanks for the context, SandyTao520. Given that the SDK discards error details, using error.message.includes()
for the specific fetch error message is a pragmatic and reasonable approach to handle these transient network issues. If the SDK evolves to expose more granular error types in the future, we can revisit this to use a more robust type-based check. The linked issue also provides good background on the problem.
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.
Approved after these comments are addressed.
I've addressed the comments by moving the retry-fetch-errors from a flag to a setting, and updating the test to use the actual retryWithBackoff implementation. |
TLDR
Adds configuration and logic to retry API calls when a specific "fetch failed" error occurs, addressing transient network issues.
Dive Deeper
Introduces a
--retry-fetch-errors
CLI flag (and corresponding config) that, when enabled, allowsgeminiChat
to retry requests that fail with "exception TypeError: fetch failed sending request". This is handled within the existingretryWithBackoff
utility in@google/gemini-cli-core
.Reviewer Test Plan
Run the CLI with
--retry-fetch-errors
in an environment with unstable network connectivity or mock network failures to verify retries occur (disconnect network or set GOOGLE_GEMINI_BASE_URL=http://localhost:9999).Testing Matrix
Linked issues / bugs