Skip to content

Add retry utility with exponential backoff#22

Open
fantaize wants to merge 4 commits intomasterfrom
test/review-body
Open

Add retry utility with exponential backoff#22
fantaize wants to merge 4 commits intomasterfrom
test/review-body

Conversation

@fantaize
Copy link
Copy Markdown
Owner

Simple retry helper for wrapping flaky async operations.

Copy link
Copy Markdown

@fantaize-code-reviewer fantaize-code-reviewer bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 issue in 1m 39s.

  1. src/utils/retry.ts:11 — The loop condition attempt <= maxAttempts causes the function to invoke fn() a total of maxAttempts + 1 times, not maxAttempts. With the default maxAttempts = 3, callers receive 4 call attempts instead of 3, violating the contract implied by the parameter name. The fix is to change the condition to attempt < maxAttempts. (issue)

Copy link
Copy Markdown

@fantaize-code-reviewer fantaize-code-reviewer bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 issue in 1m 16s.

  1. src/utils/retry.ts:16 — The condition if (attempt < maxAttempts) on line 16 is tautologically true because the for loop's own condition (attempt < maxAttempts) guarantees attempt is always strictly less than maxAttempts while inside the loop body. The intended guard was if (attempt < maxAttempts - 1), which would skip the sleep after the last attempt. As written, with the defaults of maxAttempts = 3 and baseDelayMs = 1000, callers always wait an unnecessary extra 4 seconds (1000 * 2² = 4000ms) after the final failure before the error is propagated. The fix is to change the condition to attempt < maxAttempts - 1. (issue)

Copy link
Copy Markdown

@fantaize-code-reviewer fantaize-code-reviewer bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 issue in 2m 5s.

  1. src/utils/retry.ts:23 — When maxAttempts is 0 (or any non-positive value), the for loop body never executes, so lastError stays undefined. The subsequent throw lastError statement then throws undefined instead of an Error. Any caller that does catch (err) { err.message } will receive a secondary TypeError: Cannot read properties of undefined, and callers that do catch (err) { if (err instanceof Error) { ... } } will silently skip the error-handling branch. The fix is to guard throw lastError — for example, throw lastError ?? new Error('retry called with maxAttempts <= 0') — or validate that maxAttempts >= 1 at the top of the function. (issue)

Copy link
Copy Markdown

@fantaize-code-reviewer fantaize-code-reviewer bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found in 1m 49s — the changes look good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant