feat: enhance retry system with attempt tracking, retryCondition, and onRetry hook#549
feat: enhance retry system with attempt tracking, retryCondition, and onRetry hook#549productdevbook wants to merge 2 commits intounjs:mainfrom
Conversation
… onRetry hook - Add `FetchRetryState` to `FetchContext` with `attempt` and `limit` fields - Add `retryCondition` option for custom retry logic beyond status codes - Add `onRetry` hook called before each retry attempt (e.g., token refresh) - Fix retry not checking custom `retryStatusCodes` for client errors (unjs#495) - `retryDelay` callback now receives full context with retry state (unjs#536) Resolves unjs#536, unjs#503, unjs#358, unjs#495 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe diff adds a retry-state object ( Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Request
participant FetchHandler as Fetch Handler
participant Hooks as Hooks (onError/onRetry/onResponseError)
participant Retry as Retry Logic
Client->>FetchHandler: Send request with retry options
FetchHandler->>FetchHandler: Perform initial request
FetchHandler->>Retry: Error or retryable response observed
Retry->>Retry: Evaluate retryCondition OR retryStatusCodes
alt Should retry
Retry->>FetchHandler: Set context.retry { attempt, limit }
Retry->>Hooks: Call onRetry hooks with context.retry
Hooks->>Hooks: Optionally mutate request options
Retry->>Hooks: Compute delay via retryDelay(context)
Hooks-->>Retry: Return delay
Retry->>Retry: Wait delay
Retry->>FetchHandler: Re-invoke $fetchRaw with _retryState
FetchHandler->>FetchHandler: Execute retry request (loop as needed)
else Do not retry
FetchHandler-->>Client: Propagate error/response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fetch.ts`:
- Line 138: The initial retry state uses a default { attempt: 0, limit: 0 }
which misreports limit to hooks; change the initialization of retry (where you
currently set retry: _retryState ?? { attempt: 0, limit: 0 }) to compute the
limit from context.options.retry when _retryState is null/undefined (e.g., set
limit: context.options.retry ?? someFallback) so hooks like
onRequest/onResponseError see the real configured limit; update the assignment
that references _retryState and ensure any helper that reads retry still expects
{ attempt, limit } shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08ab19b1-b9b0-48e2-9f8f-d758208c90b4
📒 Files selected for processing (4)
src/fetch.tssrc/types.tstest/index.test.tstest/types.test-d.ts
…ct retry.limit init - Restore fallback to 500 for network errors (no response) to maintain backward-compatible retry behavior - Change retryCondition to replacement semantics: when provided, it decides whether to retry (replaces status code check entirely) - Initialize retry.limit correctly on first request (was 0, now computed) - Add tests for network error retry, retryCondition suppression, limit tracking Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/index.test.ts (1)
610-617: Reduce potential flakiness in network-error tests.Using
http://localhost:1can be environment-dependent. Consider creating a temporary local TCP server, capturing its ephemeral port, closing it, then using that closed port as the target URL for deterministic connection-refused behavior (Line 611 and Line 668).Also applies to: 666-680
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 610 - 617, The current tests use a hardcoded port (http://localhost:1) which is flaky; change both tests (the "retries network errors by default (no response)" test and the other test around the same area) to create a temporary TCP server via Node's net.createServer(), listen(0) to get an ephemeral port, capture server.address().port, close() the server to free the port, then use the closed port URL (e.g. http://127.0.0.1:${port}) as the $fetch target so the connection is deterministically refused; update the test setup where $fetch is called (the test name and the other nearby test referencing a closed-port connection) to use this pattern and clean up the server before asserting fetch call counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/index.test.ts`:
- Around line 610-617: The current tests use a hardcoded port
(http://localhost:1) which is flaky; change both tests (the "retries network
errors by default (no response)" test and the other test around the same area)
to create a temporary TCP server via Node's net.createServer(), listen(0) to get
an ephemeral port, capture server.address().port, close() the server to free the
port, then use the closed port URL (e.g. http://127.0.0.1:${port}) as the $fetch
target so the connection is deterministically refused; update the test setup
where $fetch is called (the test name and the other nearby test referencing a
closed-port connection) to use this pattern and clean up the server before
asserting fetch call counts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c68612f-3f0f-4360-8f22-1fa33baa3419
📒 Files selected for processing (3)
src/fetch.tssrc/types.tstest/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/types.ts
- src/fetch.ts
Summary
FetchRetryState({ attempt, limit }) toFetchContextfor retry trackingretryConditionoption for custom retry logic beyond status codesonRetryhook called before each retry (e.g., token refresh)retryStatusCodesfor client errorsChanges
src/types.tsFetchRetryStateinterface withattemptandlimitfieldsretryConditionoption:(context) => boolean | Promise<boolean>onRetryhook inFetchHookssrc/fetch.tsFetchRetryStateinstead of decrementingretrycountretryConditionevaluated alongsideretryStatusCodes(retries if either matches)onRetryhook called before each retry with full contextretryDelaycallback now receives context with retry stateUsage examples
Test plan
onResponseErrorcontextretryDelaycallbackretryConditioncallback triggers retryretryConditionworks alongsideretryStatusCodesonRetryhook called with correct attempt/limitonRetrycan modify request options (token refresh)retryConditionretryConditionreceives error context for network failuresFetchRetryState,retryCondition,onRetryResolves #536, #503, #358, #495
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests