diff --git a/src/errors/index.ts b/src/errors/index.ts index 45e89b7d..8d76762e 100644 --- a/src/errors/index.ts +++ b/src/errors/index.ts @@ -37,6 +37,7 @@ export { ProxyError, MigrationError, UserAbortError, + ValidationError, RetryableError, isCCSError, isRecoverableError, diff --git a/src/utils/__tests__/retry-strategy.test.ts b/src/utils/__tests__/retry-strategy.test.ts index 372babf5..58dd58e0 100644 --- a/src/utils/__tests__/retry-strategy.test.ts +++ b/src/utils/__tests__/retry-strategy.test.ts @@ -184,4 +184,31 @@ describe('withRetry', () => { expect(delay).toBeGreaterThanOrEqual(500); sleepSpy.mockRestore(); }); + + it('swallows onRetry callback errors and continues retrying', async () => { + let attempt = 0; + const fn = mock(() => { + attempt++; + if (attempt < 3) { + return Promise.reject(new RetryableError('fail')); + } + return Promise.resolve('ok'); + }); + const onRetry = mock(() => { + throw new Error('callback blew up'); + }); + + const result = await withRetry(fn, { maxRetries: 5, baseDelayMs: 1, onRetry }); + expect(result).toBe('ok'); + expect(fn).toHaveBeenCalledTimes(3); + expect(onRetry).toHaveBeenCalledTimes(2); + }); + + it('throws on negative baseDelayMs', async () => { + const fn = mock(() => Promise.resolve('ok')); + await expect(withRetry(fn, { maxRetries: 3, baseDelayMs: -1 })).rejects.toThrow( + 'baseDelayMs must be >= 0' + ); + expect(fn).not.toHaveBeenCalled(); + }); }); diff --git a/src/utils/retry-strategy.ts b/src/utils/retry-strategy.ts index 6e778862..50489b6b 100644 --- a/src/utils/retry-strategy.ts +++ b/src/utils/retry-strategy.ts @@ -16,13 +16,13 @@ export interface RetryOptions { maxRetries: number; /** Base delay in ms for the first retry (default: 1000) */ baseDelayMs: number; - /** Upper bound for the computed delay (default: 30000) */ + /** Upper bound for the computed backoff delay. Note: server-provided `retryAfter` (from RetryableError) takes precedence and may exceed this cap. */ maxDelayMs?: number; - /** Multiplier applied per attempt (default: 2) */ + /** Multiplier applied per attempt (default: 2). Values <1 produce degrowth. */ backoffMultiplier?: number; /** Override the default retryability check */ retryableCheck?: (error: unknown) => boolean; - /** Callback fired before each retry (not fired on initial call) */ + /** Callback fired before each retry. Errors thrown by this callback are swallowed to prevent aborting the retry loop. */ onRetry?: (error: Error, attempt: number) => void; } @@ -95,6 +95,9 @@ export async function withRetry(fn: () => Promise, options: RetryOptions): if (maxRetries < 0) { throw new Error('withRetry: maxRetries must be >= 0'); } + if (baseDelayMs < 0) { + throw new Error('withRetry: baseDelayMs must be >= 0'); + } const isRetryable = retryableCheck ?? defaultRetryableCheck; let lastError: unknown; @@ -116,7 +119,11 @@ export async function withRetry(fn: () => Promise, options: RetryOptions): } const err = error instanceof Error ? error : new Error(String(error)); - onRetry?.(err, attempt + 1); + try { + onRetry?.(err, attempt + 1); + } catch { + // Swallow callback errors — retry decision is already made + } const retryAfter = error instanceof RetryableError ? error.retryAfter : undefined; const delay = computeDelay(attempt, baseDelayMs, maxDelayMs, backoffMultiplier, retryAfter);