feat(shared): Add cancellation and per-error delay options to retry#9182
feat(shared): Add cancellation and per-error delay options to retry#9182wobsoriano wants to merge 4 commits into
Conversation
Adds an optional signal (AbortSignal) option so callers can cancel retrying promptly, and allows initialDelay to be a function deriving the backoff base delay from the error that triggered the retry.
🦋 Changeset detectedLatest commit: eb3289e The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The `retry` helper now supports cancellation and error-dependent backoff. Added `signal` option for cancellation and modified `initialDelay` to accept a function for backoff base delay.
📝 WalkthroughWalkthroughThe shared ChangesRetry cancellation and dynamic backoff
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant retry
participant Callback
participant Delay
participant AbortSignal
Caller->>retry: Start retry with options
retry->>Callback: Execute attempt
Callback-->>retry: Throw error
retry->>Delay: Schedule error-derived backoff
AbortSignal-->>Delay: Abort with reason
Delay-->>retry: Reject pending delay
retry-->>Caller: Reject with abort reason
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/shared/src/retry.ts`:
- Around line 5-11: Update the JSDoc for the public initialDelay callback
parameter in retry configuration to explicitly state that iteration starts at 1,
clarifying its indexing semantics without changing the callback type or
behavior.
- Around line 138-142: Update the retry loop around the callback’s catch
handling to recheck signal cancellation immediately after the callback rejects,
before returning the original error or invoking retry hooks; when aborted, throw
abortReason(signal) to preserve the cancellation contract. Add a regression test
covering a callback that aborts the signal while running and then rejects.
🪄 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: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: fa8bdc4b-3b48-4f3e-8b60-0c1aca0aa78a
📒 Files selected for processing (3)
.changeset/shared-retry-cancellation.mdpackages/shared/src/__tests__/retry.spec.tspackages/shared/src/retry.ts
| * The base delay of the exponential backoff: either milliseconds, or a function | ||
| * deriving them from the error that triggered the retry and the iteration number. | ||
| * The exponential factor, max delay, and jitter still apply. | ||
| * | ||
| * @default 125 | ||
| */ | ||
| initialDelay: Milliseconds; | ||
| initialDelay: Milliseconds | ((error: unknown, iteration: number) => Milliseconds); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document that iteration starts at 1.
This new public callback parameter otherwise has ambiguous indexing semantics.
Proposed documentation update
- * deriving them from the error that triggered the retry and the iteration number.
+ * deriving them from the error that triggered the retry and the iteration number,
+ * starting at 1 for the first retry.The Docs team may also need to review this generated reference-doc change. As per path instructions, JSDoc on public/reference-facing APIs is customer-facing documentation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * The base delay of the exponential backoff: either milliseconds, or a function | |
| * deriving them from the error that triggered the retry and the iteration number. | |
| * The exponential factor, max delay, and jitter still apply. | |
| * | |
| * @default 125 | |
| */ | |
| initialDelay: Milliseconds; | |
| initialDelay: Milliseconds | ((error: unknown, iteration: number) => Milliseconds); | |
| /** | |
| * The base delay of the exponential backoff: either milliseconds, or a function | |
| * deriving them from the error that triggered the retry and the iteration number, | |
| * starting at 1 for the first retry. | |
| * The exponential factor, max delay, and jitter still apply. | |
| * | |
| * `@default` 125 | |
| */ | |
| initialDelay: Milliseconds | ((error: unknown, iteration: number) => Milliseconds); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/shared/src/retry.ts` around lines 5 - 11, Update the JSDoc for the
public initialDelay callback parameter in retry configuration to explicitly
state that iteration starts at 1, clarifying its indexing semantics without
changing the callback type or behavior.
Source: Path instructions
| while (true) { | ||
| if (signal?.aborted) { | ||
| throw abortReason(signal); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Recheck cancellation immediately after the callback rejects.
If the signal aborts while callback is running, the catch block can return the original error or invoke retry hooks before observing cancellation. This violates the documented guarantee that cancellation rejects with the abort reason.
Proposed fix
} catch (e) {
+ if (signal?.aborted) {
+ throw abortReason(signal);
+ }
+
iterations++;Add a regression test where the signal aborts during an active callback and that callback subsequently rejects.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while (true) { | |
| if (signal?.aborted) { | |
| throw abortReason(signal); | |
| } | |
| if (signal?.aborted) { | |
| throw abortReason(signal); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/shared/src/retry.ts` around lines 138 - 142, Update the retry loop
around the callback’s catch handling to recheck signal cancellation immediately
after the callback rejects, before returning the original error or invoking
retry hooks; when aborted, throw abortReason(signal) to preserve the
cancellation contract. Add a regression test covering a callback that aborts the
signal while running and then rejects.
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
wobsoriano
requested review from
a team and
mikepitre
and removed request for
a team
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| try { | ||
| return await callback(); |
There was a problem hiding this comment.
🟡 Medium src/retry.ts:144
If signal aborts while callback() is in flight and the callback then succeeds, retry returns the result normally instead of rejecting with the abort reason. This contradicts the documented contract that aborting rejects the returned promise. The loop awaits the callback at await callback() and returns immediately without rechecking signal.aborted, so a slow successful attempt resolves after cancellation. Recheck signal?.aborted after the await and throw abortReason(signal) before returning.
| try { | |
| return await callback(); | |
| try { | |
| const result = await callback(); | |
| if (signal?.aborted) { | |
| throw abortReason(signal); | |
| } | |
| return result; | |
| } catch (e) { |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/shared/src/retry.ts around lines 144-145:
If `signal` aborts while `callback()` is in flight and the callback then succeeds, `retry` returns the result normally instead of rejecting with the abort reason. This contradicts the documented contract that aborting rejects the returned promise. The loop awaits the callback at `await callback()` and returns immediately without rechecking `signal.aborted`, so a slow successful attempt resolves after cancellation. Recheck `signal?.aborted` after the await and throw `abortReason(signal)` before returning.
1 participant
Description
Follow-up to #9173, which added a hand-rolled exponential backoff to Expo's initialization resource recovery because the shared retry helper couldn't express two things it needed. This adds them:
signal?: AbortSignal: cancels retrying. Aborting rejects the returned promise with the signal's abort reason and immediately interrupts a pending backoff delay. It does not abort a callback that is already executing.initialDelaynow also accepts a function(error, iteration) => number, deriving the backoff base delay from the error that triggered the retry (e.g. 2s for network errors, 10s for 5xx). The exponential factor, cap, and jitter still apply.Both changes are optional and additive. Defaults and existing behavior are unchanged, and no current retry consumer is affected.
A follow-up PR will migrate the Expo recovery logic from #9173 onto this helper, gaining jitter on its retry delays.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
AbortSignal.Bug Fixes