mirror of
https://github.com/tiennm99/ccs.git
synced 2026-07-16 06:16:37 +00:00
fix: round 2 red-team fixes — onRetry safety, validation, barrel
- retry-strategy: wrap onRetry in try/catch to prevent callback errors from aborting retries - retry-strategy: validate baseDelayMs >= 0 - retry-strategy: update JSDoc to clarify retryAfter/maxDelayMs interaction - errors/index.ts: add ValidationError to barrel - Tests: onRetry throw test, negative baseDelayMs test
This commit is contained in:
@@ -37,6 +37,7 @@ export {
|
||||
ProxyError,
|
||||
MigrationError,
|
||||
UserAbortError,
|
||||
ValidationError,
|
||||
RetryableError,
|
||||
isCCSError,
|
||||
isRecoverableError,
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<T>(fn: () => Promise<T>, 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<T>(fn: () => Promise<T>, 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);
|
||||
|
||||
Reference in New Issue
Block a user