Skip to content

Commit 85052ac

Browse files
committed
feat: centralize maxAttempts configuration via ExperimentFlags
This commit centralizes the retry attempt limits to be driven by the `ExperimentFlags.MAX_ATTEMPTS` flag or the user configuration, rather than being hardcoded throughout the codebase. The retry logic in `baseLlmClient`, `geminiChat`, `client`, and `web-fetch` has been updated to retrieve the `maxAttempts` setting directly from `Config`. It also addresses the removal of the previous 10-attempt cap in the Config initialization to allow tests simulating high retry limits to pass successfully.
1 parent e26ee80 commit 85052ac

File tree

10 files changed

+51
-24
lines changed

10 files changed

+51
-24
lines changed

packages/core/src/code_assist/experiments/flagNames.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ export const ExperimentFlags = {
2020
PRO_MODEL_NO_ACCESS: 45768879,
2121
GEMINI_3_1_FLASH_LITE_LAUNCHED: 45771641,
2222
DEFAULT_REQUEST_TIMEOUT: 45773134,
23+
MAX_ATTEMPTS: 45774515,
2324
} as const;
2425

2526
export type ExperimentFlagName =

packages/core/src/config/config.test.ts

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import {
2020
type SandboxConfig,
2121
} from './config.js';
2222
import { createMockSandboxConfig } from '@google/gemini-cli-test-utils';
23-
import { DEFAULT_MAX_ATTEMPTS } from '../utils/retry.js';
2423
import { ExperimentFlags } from '../code_assist/experiments/flagNames.js';
2524
import { debugLogger } from '../utils/debugLogger.js';
2625
import { ApprovalMode } from '../policy/types.js';
@@ -285,25 +284,25 @@ describe('Server Config (config.ts)', () => {
285284
};
286285

287286
describe('maxAttempts', () => {
288-
it('should default to DEFAULT_MAX_ATTEMPTS', () => {
287+
it('should default to 10', () => {
289288
const config = new Config(baseParams);
290-
expect(config.getMaxAttempts()).toBe(DEFAULT_MAX_ATTEMPTS);
289+
expect(config.getMaxAttempts()).toBe(10);
291290
});
292291

293-
it('should use provided maxAttempts if <= DEFAULT_MAX_ATTEMPTS', () => {
292+
it('should use provided maxAttempts if <= 10', () => {
294293
const config = new Config({
295294
...baseParams,
296295
maxAttempts: 5,
297296
});
298297
expect(config.getMaxAttempts()).toBe(5);
299298
});
300299

301-
it('should cap maxAttempts at DEFAULT_MAX_ATTEMPTS', () => {
300+
it('should use provided maxAttempts if > 10', () => {
302301
const config = new Config({
303302
...baseParams,
304303
maxAttempts: 20,
305304
});
306-
expect(config.getMaxAttempts()).toBe(DEFAULT_MAX_ATTEMPTS);
305+
expect(config.getMaxAttempts()).toBe(20);
307306
});
308307
});
309308

packages/core/src/config/config.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1326,10 +1326,7 @@ export class Config implements McpContext, AgentLoopContext {
13261326
this.agentSessionNoninteractiveEnabled =
13271327
params.adk?.agentSessionNoninteractiveEnabled ?? false;
13281328
this.retryFetchErrors = params.retryFetchErrors ?? true;
1329-
this.maxAttempts = Math.min(
1330-
params.maxAttempts ?? DEFAULT_MAX_ATTEMPTS,
1331-
DEFAULT_MAX_ATTEMPTS,
1332-
);
1329+
this.maxAttempts = params.maxAttempts ?? DEFAULT_MAX_ATTEMPTS;
13331330
this.disableYoloMode = params.disableYoloMode ?? false;
13341331
this.rawOutput = params.rawOutput ?? false;
13351332
this.acceptRawOutputRisk = params.acceptRawOutputRisk ?? false;
@@ -3309,6 +3306,14 @@ export class Config implements McpContext, AgentLoopContext {
33093306
}
33103307

33113308
getMaxAttempts(): number {
3309+
const flagVal =
3310+
this.experiments?.flags?.[ExperimentFlags.MAX_ATTEMPTS]?.intValue;
3311+
if (flagVal !== undefined) {
3312+
const parsed = parseInt(flagVal, 10);
3313+
if (!isNaN(parsed) && parsed > 0) {
3314+
return parsed;
3315+
}
3316+
}
33123317
return this.maxAttempts;
33133318
}
33143319

packages/core/src/core/baseLlmClient.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ describe('BaseLlmClient', () => {
252252
expect(retryWithBackoff).toHaveBeenCalledWith(
253253
expect.any(Function),
254254
expect.objectContaining({
255-
maxAttempts: 5,
255+
maxAttempts: 3,
256256
}),
257257
);
258258
});

packages/core/src/core/baseLlmClient.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,6 @@ import {
3636
createAvailabilityContextProvider,
3737
} from '../availability/policyHelpers.js';
3838

39-
const DEFAULT_MAX_ATTEMPTS = 5;
40-
4139
/**
4240
* Options for the generateJson utility function.
4341
*/
@@ -328,7 +326,9 @@ export class BaseLlmClient {
328326
return await retryWithBackoff(apiCall, {
329327
shouldRetryOnContent,
330328
maxAttempts:
331-
availabilityMaxAttempts ?? maxAttempts ?? DEFAULT_MAX_ATTEMPTS,
329+
availabilityMaxAttempts ??
330+
maxAttempts ??
331+
this.config.getMaxAttempts(),
332332
getAvailabilityContext,
333333
onPersistent429: this.config.isInteractive()
334334
? (authType, error) =>
@@ -339,7 +339,9 @@ export class BaseLlmClient {
339339
retryFetchErrors: this.config.getRetryFetchErrors(),
340340
onRetry: (attempt, error, delayMs) => {
341341
const actualMaxAttempts =
342-
availabilityMaxAttempts ?? maxAttempts ?? DEFAULT_MAX_ATTEMPTS;
342+
availabilityMaxAttempts ??
343+
maxAttempts ??
344+
this.config.getMaxAttempts();
343345
const modelName = getDisplayString(currentModel);
344346
const errorType = getRetryErrorType(error);
345347

packages/core/src/core/client.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1133,7 +1133,7 @@ export class GeminiClient {
11331133
onPersistent429: onPersistent429Callback,
11341134
onValidationRequired: onValidationRequiredCallback,
11351135
authType: this.config.getContentGeneratorConfig()?.authType,
1136-
maxAttempts: availabilityMaxAttempts,
1136+
maxAttempts: availabilityMaxAttempts ?? this.config.getMaxAttempts(),
11371137
retryFetchErrors: this.config.getRetryFetchErrors(),
11381138
getAvailabilityContext,
11391139
onRetry: (attempt, error, delayMs) => {

packages/core/src/core/geminiChat.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ describe('GeminiChat', () => {
176176
},
177177
getContentGenerator: vi.fn().mockReturnValue(mockContentGenerator),
178178
getRetryFetchErrors: vi.fn().mockReturnValue(false),
179-
getMaxAttempts: vi.fn().mockReturnValue(10),
179+
getMaxAttempts: vi.fn().mockReturnValue(4),
180180
getUserTier: vi.fn().mockReturnValue(undefined),
181181
modelConfigService: {
182182
getResolvedConfig: vi.fn().mockImplementation((modelConfigKey) => {

packages/core/src/core/geminiChat.ts

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -78,16 +78,13 @@ export type StreamEvent =
7878
* Options for retrying mid-stream errors (e.g. invalid content or API disconnects).
7979
*/
8080
interface MidStreamRetryOptions {
81-
/** Total number of attempts to make (1 initial + N retries). */
82-
maxAttempts: number;
8381
/** The base delay in milliseconds for backoff. */
8482
initialDelayMs: number;
8583
/** Whether to use exponential backoff instead of linear. */
8684
useExponentialBackoff: boolean;
8785
}
8886

8987
const MID_STREAM_RETRY_OPTIONS: MidStreamRetryOptions = {
90-
maxAttempts: 4, // 1 initial call + 3 retries mid-stream
9188
initialDelayMs: 1000,
9289
useExponentialBackoff: true,
9390
};
@@ -420,10 +417,8 @@ export class GeminiChat {
420417
: getRetryErrorType(error);
421418

422419
if (isContentError || (isRetryable && !signal.aborted)) {
423-
// The issue requests exactly 3 retries (4 attempts) for API errors during stream iteration.
424-
// Regardless of the global maxAttempts (e.g. 10), we only want to retry these mid-stream API errors
425-
// up to 3 times before finally throwing the error to the user.
426-
const maxMidStreamAttempts = MID_STREAM_RETRY_OPTIONS.maxAttempts;
420+
// We retry mid-stream API errors up to maxAttempts times before finally throwing the error to the user.
421+
const maxMidStreamAttempts = this.context.config.getMaxAttempts();
427422

428423
if (
429424
attempt < maxAttempts - 1 &&

packages/core/src/tools/web-fetch.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,7 @@ class WebFetchToolInvocation extends BaseToolInvocation<
309309
return res;
310310
},
311311
{
312+
maxAttempts: this.context.config.getMaxAttempts(),
312313
retryFetchErrors: this.context.config.getRetryFetchErrors(),
313314
onRetry: (attempt, error, delayMs) =>
314315
this.handleRetry(attempt, error, delayMs),
@@ -643,6 +644,7 @@ ${aggregatedContent}
643644
return res;
644645
},
645646
{
647+
maxAttempts: this.context.config.getMaxAttempts(),
646648
retryFetchErrors: this.context.config.getRetryFetchErrors(),
647649
onRetry: (attempt, error, delayMs) =>
648650
this.handleRetry(attempt, error, delayMs),

packages/core/src/utils/retry.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,25 +17,48 @@ import { getErrorStatus, ModelNotFoundError } from './httpErrors.js';
1717
import type { RetryAvailabilityContext } from '../availability/modelPolicy.js';
1818

1919
export type { RetryAvailabilityContext };
20+
21+
/**
22+
* Global fallback for maximum retry attempts when not explicitly provided.
23+
* Most callers should use config.getMaxAttempts() instead.
24+
*/
2025
export const DEFAULT_MAX_ATTEMPTS = 10;
2126

27+
/**
28+
* Options for the retryWithBackoff utility.
29+
*/
2230
export interface RetryOptions {
31+
/**
32+
* Total number of attempts (1 initial + N retries).
33+
* Defaults to DEFAULT_MAX_ATTEMPTS (10) if not specified.
34+
*/
2335
maxAttempts: number;
36+
/** Initial delay between retries in milliseconds. */
2437
initialDelayMs: number;
38+
/** Maximum cumulative delay in milliseconds. */
2539
maxDelayMs: number;
40+
/** Callback to determine if an error is retryable. */
2641
shouldRetryOnError: (error: Error, retryFetchErrors?: boolean) => boolean;
42+
/** Callback to determine if the response content requires a retry. */
2743
shouldRetryOnContent?: (content: GenerateContentResponse) => boolean;
44+
/** Handler for persistent 429 errors. */
2845
onPersistent429?: (
2946
authType?: string,
3047
error?: unknown,
3148
) => Promise<string | boolean | null>;
49+
/** Handler for quota validation requirements. */
3250
onValidationRequired?: (
3351
error: ValidationRequiredError,
3452
) => Promise<'verify' | 'change_auth' | 'cancel'>;
53+
/** Authentication type for logging. */
3554
authType?: string;
55+
/** Whether to retry on generic fetch errors. */
3656
retryFetchErrors?: boolean;
57+
/** Signal for cancellation. */
3758
signal?: AbortSignal;
59+
/** Context provider for availability checks. */
3860
getAvailabilityContext?: () => RetryAvailabilityContext | undefined;
61+
/** Callback fired on each retry attempt. */
3962
onRetry?: (attempt: number, error: unknown, delayMs: number) => void;
4063
}
4164

0 commit comments

Comments
 (0)