fix: clamp negative maxRetries/retryBackoff to safe values

- maxRetries clamped to >= 0 (negative values treated as no retry)
- retryBackoff clamped to >= 1 (prevents zero/negative delay oscillation)
- retryDelayMs clamped to >= 0
- Add tests for negative maxRetries and negative backoff

Addresses Codex review P1 on #37
This commit is contained in:
JackChen 2026-04-03 14:05:20 +08:00
parent 972f9f0c74
commit 08cf01c6b4
2 changed files with 41 additions and 3 deletions

View File

@ -128,9 +128,9 @@ export async function executeWithRetry(
onRetry?: (data: { attempt: number; maxAttempts: number; error: string; nextDelayMs: number }) => void,
delayFn: (ms: number) => Promise<void> = sleep,
): Promise<AgentRunResult> {
const maxAttempts = (task.maxRetries ?? 0) + 1
const baseDelay = task.retryDelayMs ?? 1000
const backoff = task.retryBackoff ?? 2
const maxAttempts = Math.max(0, task.maxRetries ?? 0) + 1
const baseDelay = Math.max(0, task.retryDelayMs ?? 1000)
const backoff = Math.max(1, task.retryBackoff ?? 2)
let lastError: string = ''

View File

@ -275,4 +275,42 @@ describe('executeWithRetry', () => {
expect(mockDelay).toHaveBeenCalledWith(30_000) // capped
})
it('clamps negative maxRetries to 0 (single attempt)', async () => {
const run = vi.fn().mockRejectedValue(new Error('fail'))
const task = createTask({
title: 'Negative retry',
description: 'test',
maxRetries: -5,
})
// Manually set negative value since createTask doesn't validate
;(task as any).maxRetries = -5
const result = await executeWithRetry(run, task, undefined, noDelay)
expect(result.success).toBe(false)
expect(run).toHaveBeenCalledTimes(1) // exactly 1 attempt, no retries
})
it('clamps backoff below 1 to 1 (constant delay)', async () => {
const run = vi.fn()
.mockRejectedValueOnce(new Error('error'))
.mockResolvedValueOnce(SUCCESS_RESULT)
const task = createTask({
title: 'Bad backoff',
description: 'test',
maxRetries: 1,
retryDelayMs: 100,
retryBackoff: -2,
})
;(task as any).retryBackoff = -2
const mockDelay = vi.fn().mockResolvedValue(undefined)
await executeWithRetry(run, task, undefined, mockDelay)
// backoff clamped to 1, so delay = 100 * 1^0 = 100
expect(mockDelay).toHaveBeenCalledWith(100)
})
})