From 9c2fd5a7c69a1582144a529f9b0a274467f1b0b9 Mon Sep 17 00:00:00 2001 From: Bryan Morgan Date: Thu, 26 Feb 2026 10:42:34 -0500 Subject: [PATCH] feat(core): add HTTP 499 to retryable errors and map to RetryableQuotaError (#20432) --- integration-tests/file-system.test.ts | 4 +-- integration-tests/write_file.test.ts | 4 +-- packages/core/src/utils/flashFallback.test.ts | 29 +++++++++++++++++++ .../core/src/utils/googleQuotaErrors.test.ts | 18 +++++++++++- packages/core/src/utils/googleQuotaErrors.ts | 18 ++++++------ packages/core/src/utils/retry.test.ts | 24 +++++++++++++++ packages/core/src/utils/retry.ts | 8 +++-- packages/sdk/src/agent.integration.test.ts | 6 ++-- 8 files changed, 92 insertions(+), 19 deletions(-) diff --git a/integration-tests/file-system.test.ts b/integration-tests/file-system.test.ts index bdcffedaf8..64481068c2 100644 --- a/integration-tests/file-system.test.ts +++ b/integration-tests/file-system.test.ts @@ -55,8 +55,8 @@ describe('file-system', () => { }); }); - it('should be able to write a file', async () => { - await rig.setup('should be able to write a file', { + it('should be able to write a hello world message to a file', async () => { + await rig.setup('should be able to write a hello world message to a file', { settings: { tools: { core: ['write_file', 'replace', 'read_file'] } }, }); rig.createFile('test.txt', ''); diff --git a/integration-tests/write_file.test.ts b/integration-tests/write_file.test.ts index 8069b1ca87..ece2a11aa4 100644 --- a/integration-tests/write_file.test.ts +++ b/integration-tests/write_file.test.ts @@ -22,8 +22,8 @@ describe('write_file', () => { afterEach(async () => await rig.cleanup()); - it('should be able to write a file', async () => { - await rig.setup('should be able to write a file', { + it('should be able to write a joke to a file', async () => { + await rig.setup('should be able to write a joke to a file', { settings: { tools: { core: ['write_file', 'read_file'] } }, }); const prompt = `show me an example of using the write tool. put a dad joke in dad.txt`; diff --git a/packages/core/src/utils/flashFallback.test.ts b/packages/core/src/utils/flashFallback.test.ts index ec95de94ef..af4a73c213 100644 --- a/packages/core/src/utils/flashFallback.test.ts +++ b/packages/core/src/utils/flashFallback.test.ts @@ -19,6 +19,7 @@ import { AuthType } from '../core/contentGenerator.js'; // Import the new types (Assuming this test file is in packages/core/src/utils/) import type { FallbackModelHandler } from '../fallback/types.js'; import type { GoogleApiError } from './googleErrors.js'; +import { type HttpError } from './httpErrors.js'; import { TerminalQuotaError } from './googleQuotaErrors.js'; vi.mock('node:fs'); @@ -106,6 +107,34 @@ describe('Retry Utility Fallback Integration', () => { expect(mockApiCall).toHaveBeenCalledTimes(3); }); + it('should trigger onPersistent429 when HTTP 499 persists through all retry attempts', async () => { + let fallbackCalled = false; + const mockError: HttpError = new Error('Simulated 499 error'); + mockError.status = 499; + + const mockApiCall = vi.fn().mockRejectedValue(mockError); // Always fail with 499 + + const mockPersistent429Callback = vi.fn(async (_authType?: string) => { + fallbackCalled = true; + // In a real scenario, this would change the model being called by mockApiCall + // or similar, but for the test we just need to see if it's called. + // We return null to stop retrying after the fallback attempt in this test. + return null; + }); + + const promise = retryWithBackoff(mockApiCall, { + maxAttempts: 2, + initialDelayMs: 1, + maxDelayMs: 10, + onPersistent429: mockPersistent429Callback, + authType: AuthType.LOGIN_WITH_GOOGLE, + }); + + await expect(promise).rejects.toThrow('Simulated 499 error'); + expect(fallbackCalled).toBe(true); + expect(mockPersistent429Callback).toHaveBeenCalledTimes(1); + }); + it('should not trigger onPersistent429 for API key users', async () => { const fallbackCallback = vi.fn(); diff --git a/packages/core/src/utils/googleQuotaErrors.test.ts b/packages/core/src/utils/googleQuotaErrors.test.ts index 06bde6444b..185f48e92a 100644 --- a/packages/core/src/utils/googleQuotaErrors.test.ts +++ b/packages/core/src/utils/googleQuotaErrors.test.ts @@ -81,7 +81,7 @@ describe('classifyGoogleError', () => { } }); - it('should return original error if code is not 429 or 503', () => { + it('should return original error if code is not 429, 499 or 503', () => { const apiError: GoogleApiError = { code: 500, message: 'Server error', @@ -95,6 +95,22 @@ describe('classifyGoogleError', () => { expect(result).not.toBeInstanceOf(RetryableQuotaError); }); + it('should return RetryableQuotaError for 499 Client Closed Request', () => { + const apiError: GoogleApiError = { + code: 499, + message: 'Client Closed Request', + details: [], + }; + vi.spyOn(errorParser, 'parseGoogleApiError').mockReturnValue(apiError); + const originalError = new Error('Client Closed Request'); + const result = classifyGoogleError(originalError); + expect(result).toBeInstanceOf(RetryableQuotaError); + if (result instanceof RetryableQuotaError) { + expect(result.cause).toBe(apiError); + expect(result.message).toBe('Client Closed Request'); + } + }); + it('should return TerminalQuotaError for daily quota violations in QuotaFailure', () => { const apiError: GoogleApiError = { code: 429, diff --git a/packages/core/src/utils/googleQuotaErrors.ts b/packages/core/src/utils/googleQuotaErrors.ts index 40c1c34361..a075b79b89 100644 --- a/packages/core/src/utils/googleQuotaErrors.ts +++ b/packages/core/src/utils/googleQuotaErrors.ts @@ -219,7 +219,7 @@ export function classifyGoogleError(error: unknown): unknown { if ( !googleApiError || - googleApiError.code !== 429 || + (googleApiError.code !== 429 && googleApiError.code !== 499) || googleApiError.details.length === 0 ) { // Fallback: try to parse the error message for a retry delay @@ -233,27 +233,27 @@ export function classifyGoogleError(error: unknown): unknown { return new RetryableQuotaError( errorMessage, googleApiError ?? { - code: 429, + code: status ?? 429, message: errorMessage, details: [], }, retryDelaySeconds, ); } - } else if (status === 429) { - // Fallback: If it is a 429 but doesn't have a specific "retry in" message, + } else if (status === 429 || status === 499) { + // Fallback: If it is a 429 or 499 but doesn't have a specific "retry in" message, // assume it is a temporary rate limit and retry after 5 sec (same as DEFAULT_RETRY_OPTIONS). return new RetryableQuotaError( errorMessage, googleApiError ?? { - code: 429, + code: status, message: errorMessage, details: [], }, ); } - return error; // Not a 429 error we can handle with structured details or a parsable retry message. + return error; // Not a retryable error we can handle with structured details or a parsable retry message. } const quotaFailure = googleApiError.details.find( @@ -353,15 +353,15 @@ export function classifyGoogleError(error: unknown): unknown { } } - // If we reached this point and the status is still 429, we return retryable. - if (status === 429) { + // If we reached this point and the status is still 429 or 499, we return retryable. + if (status === 429 || status === 499) { const errorMessage = googleApiError?.message || (error instanceof Error ? error.message : String(error)); return new RetryableQuotaError( errorMessage, googleApiError ?? { - code: 429, + code: status, message: errorMessage, details: [], }, diff --git a/packages/core/src/utils/retry.test.ts b/packages/core/src/utils/retry.test.ts index 43f038cfaa..f63a5ed723 100644 --- a/packages/core/src/utils/retry.test.ts +++ b/packages/core/src/utils/retry.test.ts @@ -158,6 +158,30 @@ describe('retryWithBackoff', () => { expect(mockFn).not.toHaveBeenCalled(); }); + it('should retry on HTTP 499 (Client Closed Request) error', async () => { + let attempts = 0; + const mockFn = vi.fn(async () => { + attempts++; + if (attempts === 1) { + const error: HttpError = new Error('Simulated 499 error'); + error.status = 499; + throw error; + } + return 'success'; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 2, + initialDelayMs: 10, + }); + + await vi.runAllTimersAsync(); + + const result = await promise; + expect(result).toBe('success'); + expect(mockFn).toHaveBeenCalledTimes(2); + }); + it('should use default shouldRetry if not provided, retrying on ApiError 429', async () => { const mockFn = vi.fn(async () => { throw new ApiError({ message: 'Too Many Requests', status: 429 }); diff --git a/packages/core/src/utils/retry.ts b/packages/core/src/utils/retry.ts index 17c4a656ed..50c992d6de 100644 --- a/packages/core/src/utils/retry.ts +++ b/packages/core/src/utils/retry.ts @@ -130,13 +130,17 @@ export function isRetryableError( if (error instanceof ApiError) { // Explicitly do not retry 400 (Bad Request) if (error.status === 400) return false; - return error.status === 429 || (error.status >= 500 && error.status < 600); + return ( + error.status === 429 || + error.status === 499 || + (error.status >= 500 && error.status < 600) + ); } // Check for status using helper (handles other error shapes) const status = getErrorStatus(error); if (status !== undefined) { - return status === 429 || (status >= 500 && status < 600); + return status === 429 || status === 499 || (status >= 500 && status < 600); } return false; diff --git a/packages/sdk/src/agent.integration.test.ts b/packages/sdk/src/agent.integration.test.ts index 1de8e52ac7..78229a81cc 100644 --- a/packages/sdk/src/agent.integration.test.ts +++ b/packages/sdk/src/agent.integration.test.ts @@ -144,14 +144,14 @@ describe('GeminiCliAgent Integration', () => { }); it('propagates errors from dynamic instructions', async () => { + const goldenFile = getGoldenPath('agent-static-instructions'); const agent = new GeminiCliAgent({ instructions: () => { throw new Error('Dynamic instruction failure'); }, model: 'gemini-2.0-flash', - fakeResponses: RECORD_MODE - ? undefined - : getGoldenPath('agent-dynamic-instructions'), + recordResponses: RECORD_MODE ? goldenFile : undefined, + fakeResponses: RECORD_MODE ? undefined : goldenFile, }); const session = agent.session();