feat: add retry utility for network operations - #2
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR adds a retry utility with backoff and a Retry-After header parser. There is an off-by-one bug in the retry loop (<= instead of <) that causes one more attempt than maxAttempts specifies. Additionally, parseRetryAfter lacks input validation and can return NaN, and RetryOptions uses number instead of the safer number | undefined pattern for the partial merge. Risk level: medium — the off-by-one is a correctness bug that will surprise callers.
4 finding(s): 1 error, 2 warning, 1 info
[ERROR] correctness
File: src/utils/retry.ts:18
Off-by-one error: the loop condition i <= opts.maxAttempts iterates maxAttempts+1 times. For maxAttempts=3, the function body runs 4 times (i=0,1,2,3), meaning 4 attempts instead of the documented 3.
Suggestion: Change the loop condition to i < opts.maxAttempts so it runs exactly maxAttempts times.
[WARNING] error-handling
File: src/utils/retry.ts:34
parseRetryAfter returns NaN (a negative number in practice) when the header value is not a valid number or parseable date. Callers receive a useless delay value with no indication of failure.
Suggestion: Return a sensible default (e.g., 0 or throw) when parsing fails, or wrap the date parsing in a try-catch and validate with isNaN before returning.
[WARNING] error-handling
File: src/utils/retry.ts:35
When new Date(header) produces an invalid date, date.getTime() returns NaN, so the return value becomes NaN. Subtracting Date.now() from NaN yields NaN.
Suggestion: After creating the Date, check isNaN(date.getTime()) and fall back to a default or throw an error.
[INFO] correctness
File: src/utils/retry.ts:37
parseRetryAfter can return a negative number if the parsed date is in the past. A negative delay passed to setTimeout will fire immediately, which may not be the desired behavior.
Suggestion: Clamp the return value to a minimum of 0: return Math.max(0, date.getTime() - Date.now()).
Reviewed by pi-relay · View full run
| export async function retry<T>(fn: () => Promise<T>, options: Partial<RetryOptions> = {}): Promise<T> { | ||
| const opts = { ...DEFAULT_OPTIONS, ...options }; | ||
| let lastError: any; | ||
| let delay = opts.delayMs; |
There was a problem hiding this comment.
[ERROR] correctness
Off-by-one error: the loop condition i <= opts.maxAttempts iterates maxAttempts+1 times. For maxAttempts=3, the function body runs 4 times (i=0,1,2,3), meaning 4 attempts instead of the documented 3.
Suggestion: Change the loop condition to i < opts.maxAttempts so it runs exactly maxAttempts times.
| } | ||
|
|
||
| export function parseRetryAfter(header: string): number { | ||
| const val = parseInt(header); |
There was a problem hiding this comment.
[WARNING] error-handling
parseRetryAfter returns NaN (a negative number in practice) when the header value is not a valid number or parseable date. Callers receive a useless delay value with no indication of failure.
Suggestion: Return a sensible default (e.g., 0 or throw) when parsing fails, or wrap the date parsing in a try-catch and validate with isNaN before returning.
|
|
||
| export function parseRetryAfter(header: string): number { | ||
| const val = parseInt(header); | ||
| if (val) return val * 1000; |
There was a problem hiding this comment.
[WARNING] error-handling
When new Date(header) produces an invalid date, date.getTime() returns NaN, so the return value becomes NaN. Subtracting Date.now() from NaN yields NaN.
Suggestion: After creating the Date, check isNaN(date.getTime()) and fall back to a default or throw an error.
| const val = parseInt(header); | ||
| if (val) return val * 1000; | ||
| const date = new Date(header); | ||
| return date.getTime() - Date.now(); |
There was a problem hiding this comment.
[INFO] correctness
parseRetryAfter can return a negative number if the parsed date is in the past. A negative delay passed to setTimeout will fire immediately, which may not be the desired behavior.
Suggestion: Clamp the return value to a minimum of 0: return Math.max(0, date.getTime() - Date.now()).
![pi-relay[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
AI Review: Fixes AppliedReviewed and found 4 issue(s). All addressed. Changes MadeFixed in src/utils/retry.ts:
Verification passed. |
|
Test PR — verified the review → findings → fix → push flow. Closing. |
1 participant
Adds a generic retry helper with configurable backoff and a Retry-After header parser.