fix(core): Ensure zero-quota limits fail fast to prevent retry loop hang#27698
fix(core): Ensure zero-quota limits fail fast to prevent retry loop hang#27698luisfelipe-alt wants to merge 8 commits into
Conversation
The error classification logic was matching the "Please retry in..." string and returning a RetryableQuotaError even when the error explicitly stated "limit: 0". This forced retryWithBackoff into a futile 10-attempt loop, causing a 10-minute hang on unbilled free-tier accounts. This change intercepts "limit: 0" and "limit:0" errors in classifyGoogleError and classifies them as TerminalQuotaError immediately, bypassing the retryable classification and failing fast.
Summary of ChangesHello, 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 addresses a performance issue where the Gemini-CLI would incorrectly classify hard quota limits (limit: 0) as retryable errors. By intercepting these specific error messages before the retry logic evaluates them, the CLI now terminates immediately, significantly improving the user experience by avoiding unnecessary and futile retry attempts. 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 the 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 counterproductive. 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. Footnotes
|
|
📊 PR Size: size/M
|
There was a problem hiding this comment.
Code Review
This pull request introduces a check to classify Google API errors with a limit of 0 as terminal quota errors, even if they contain a retry message, and adds a corresponding unit test. The feedback suggests replacing the simple string inclusion check with a regular expression to avoid false positives on fractional or padded limits (e.g., 'limit: 0.5').
Applying changes suggested by bot Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a check to classify Google API errors with a limit of 0 as a TerminalQuotaError, preventing infinite retry loops when a quota is completely exhausted. It also adds a unit test to verify this behavior. The reviewer identified a critical issue where the check is placed inside a fallback block, meaning structured errors with details will bypass it and still be incorrectly classified as retryable. Additionally, the reviewer pointed out that the new test masks this issue by mocking the error parser to return null, and suggested moving the check earlier in the logic and adding a test case with structured RetryInfo details.
26dbf1c to
5c32c2b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Google quota error classification logic in packages/core/src/utils/googleQuotaErrors.ts by extracting the errorMessage variable to the top of the function and moving the universal limit: 0 check before the fallback block. It also adds corresponding unit tests in packages/core/src/utils/googleQuotaErrors.test.ts to ensure TerminalQuotaError is correctly returned when the limit is 0, even if retry information is present. There are no review comments provided, and I have no feedback to provide.
galz10
left a comment
There was a problem hiding this comment.
Code Review: PR #27698
Intent Summary: This PR introduces a fail-fast mechanism for Gemini API errors that indicate a zero-quota limit (limit: 0). By intercepting this specific error signature early, it correctly classifies it as a TerminalQuotaError and prevents the CLI from entering a futile, long-running retry loop.
🚨 Critical Concerns (P0/P1)
Action required before merging.
packages/core/src/utils/googleQuotaErrors.ts:244: The conditionstatus === undefinedpaired with the loose regex/limit:\s*0(?!\d|\.)/is dangerously greedy. BecauseclassifyGoogleErrortakeserror: unknown, any unrelated application error containing the string "limit: 0" (e.g.,new Error("Validation failed: array length limit: 0")) will lack astatusand be incorrectly swallowed and misclassified as aTerminalQuotaError(a Google API quota issue).Note: If you absolutely must catch zero-quota errors that lack an HTTP status, the regex must be significantly tightened to avoid false positives (e.g., `/Quota exceeded.limit:\s0/).if ( (status === 429 || status === 499 || status === 503) && /limit:\s*0(?!\d|\.)/.test(errorMessage) ) {
🧹 Refactoring & Nits (P2/P3)
Recommended improvements.
packages/core/src/utils/googleQuotaErrors.test.ts:832: AI Slop. Passing an emptynew Error()and relying purely on the mock makes the test non-intuitive. Pass a representative error to make the test self-documenting.const result = classifyGoogleError(new Error('Quota exceeded for limit: 0'));packages/core/src/utils/googleQuotaErrors.ts:238: Minor DRY violation. If you move theerrorMessagevariable definition to the very top of the function (before thestatus === 404check), you can reuse it for the 404 error fallback (errorMessage || 'Model not found'), removing duplicated message extraction logic.
📝 Metadata Review
Feedback on PR description or commit message clarity.
- The PR description is excellent. It clearly defines the bug, the exact fix, the impact, and provides explicit instructions for test validation.
…r for other status not only 409
5c32c2b to
737a5e4
Compare
Done, I have attended the comments:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors classifyGoogleError to extract a common errorMessage variable and introduces a universal check to return a TerminalQuotaError when a quota limit of 0 is detected, accompanied by new unit tests. The reviewer identified a high-severity issue in the regular expression used for this check, noting that .* does not match newline characters in JavaScript, which could lead to failures in production. They provided a code suggestion to use [\s\S]* instead for robust matching.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors Google quota error classification by extracting the error message extraction logic and introducing a universal check for "limit: 0" to classify errors as TerminalQuotaError, supported by new unit tests. However, a critical syntax error was introduced in the newly added if statement with a duplicate closing parenthesis and opening brace, which will cause compilation to fail.
|
si me pasa igual cuando entro con la cuenta en gemini cli hay que esperar como 10 min para que funcione |
0d323c5 to
5a54543
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a universal check for 'limit: 0' errors in classifyGoogleError to return a TerminalQuotaError early, along with corresponding unit tests. The review feedback highlights two critical issues: first, moving the errorMessage definition to the top of the function causes a regression in 404 error handling where plain object errors evaluate to '[object Object]' instead of falling back to 'Model not found'; second, the negative lookahead in the regex incorrectly rejects matches when 'limit: 0' is followed by a period at the end of a sentence, which can be resolved by updating the lookahead pattern to allow periods not followed by a digit.
1db28df to
89bbd6e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors classifyGoogleError in packages/core/src/utils/googleQuotaErrors.ts to centralize error message extraction and introduces a universal check for zero-limit quota errors (limit: 0) to classify them as TerminalQuotaError. It also updates the 404 status handling to trim error messages and adds comprehensive unit tests in googleQuotaErrors.test.ts to verify these changes under various error formats. There are no review comments, and I have no feedback to provide.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a universal 'limit: 0' check in classifyGoogleError to immediately classify such errors as TerminalQuotaError, alongside comprehensive unit tests to validate this behavior. It also refactors the error message extraction and trims 404 error messages. The feedback suggests removing the restrictive 'quota exceeded' substring checks from the new logic, as other variations like 'Rate limit exceeded' could also occur, and instead relying solely on the status codes and the 'limit: 0' regex.
Head branch was pushed to by a user without write access
0381e5d to
f476048
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors Google API error classification in googleQuotaErrors.ts by extracting a common errorMessage variable, trimming 404 error messages, and introducing a universal check to classify errors with limit: 0 as TerminalQuotaError. It also adds comprehensive unit tests covering these scenarios. The reviewer recommends removing status === undefined from the limit: 0 check to prevent non-HTTP errors from being misclassified, suggesting instead to restrict the check to HTTP statuses 429, 499, and 503, and updating the tests accordingly.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
f476048 to
3774315
Compare
|
/gemini review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a universal check for limit: 0 to classify certain errors as TerminalQuotaError and refactors classifyGoogleError to extract the error message earlier, supported by several new test cases. The review feedback identifies a critical issue where plain object errors (which are not instances of Error) will have their messages evaluated as "[object Object]", bypassing the limit: 0 check and obscuring error messages. A robust extraction of the message property from plain objects is suggested to resolve this.
25cbc30 to
f343b5e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Google error classification logic in packages/core/src/utils/googleQuotaErrors.ts by centralizing the extraction of errorMessage and introducing a universal check to classify limit: 0 errors (for status codes 429, 499, and 503) as TerminalQuotaError. It also adds comprehensive unit tests in googleQuotaErrors.test.ts to validate this new classification behavior under various conditions. There are no review comments, and I have no feedback to provide.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
30cda22 to
68e8736
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Google error classification logic in packages/core/src/utils/googleQuotaErrors.ts by introducing a helper function extractErrorMessage to consistently extract error messages from various error formats. It also implements a universal check to classify quota errors with a limit of 0 as TerminalQuotaError across status codes 429, 499, and 503, ensuring fractional limits (like 0.5) are still treated as retryable. Additionally, comprehensive unit tests have been added to validate these changes under different error structures and edge cases. No review comments were provided, so there is no further feedback to address.
Fail fast on zero-quota limits to prevent retry loop hang
Summary
This PR fixes a critical bug in the error classification logic where the Gemini-CLI gets trapped in a futile 10-attempt retry loop when hitting a hard quota limit of
0(e.g., on unbilled free-tier accounts). This bug causes the CLI to hang for up to 10 minutes before failing, severely degrading the user experience.Details
429error withlimit: 0and a"Please retry in 59.906331105s."message,classifyGoogleErrormatches the retry delay and incorrectly classifies it as aRetryableQuotaError.classifyGoogleError(insidepackages/core/src/utils/googleQuotaErrors.ts) to intercept any error message containing"limit: 0"or"limit:0"and immediately classify it as aTerminalQuotaError. This check is executed before the retry delay regex is evaluated.limit: 0) are unaffected and will still be retried with backoff as expected.Related Issues
How to Validate
Run the newly added unit test case to verify that
limit: 0errors are correctly classified asTerminalQuotaError:Expected Result: All 35 tests pass successfully, including the new test case
should return TerminalQuotaError when limit is 0 even if message contains "Please retry in Xs".Run the retry utility tests to ensure no regressions in the core retry logic:
Expected Result: All 41 tests pass successfully.
Pre-Merge Checklist