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 6 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: e841a9d 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
Suggested reviewers: 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
| 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Co-authored-by: macroscopeapp[bot] <170038800+macroscopeapp[bot]@users.noreply.github.com>
ApprovabilityVerdict: Approved Additive feature adding optional You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 150-151: Remove the duplicated catch clause in the retry logic so
the surrounding try statement has only one `catch (e)` block. Preserve the
existing catch body and error-handling behavior.
🪄 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: 103d6fe9-86f8-4115-80a0-a299e85963ce
📒 Files selected for processing (2)
packages/shared/src/__tests__/retry.spec.tspackages/shared/src/retry.ts
| } catch (e) { | ||
| } catch (e) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Duplicate catch block is a syntax error.
Line 150 and Line 151 both contain } catch (e) {, which is invalid JavaScript — a try block can only have one catch clause. This will cause a parse error and prevent the module from compiling. Biome also flags this: "Expected a statement but instead found 'catch (e)'."
This appears to be a merge artifact or copy-paste error. Remove the duplicate line.
🐛 Proposed fix
} catch (e) {
- } catch (e) {
if (signal?.aborted) {📝 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.
| } catch (e) { | |
| } catch (e) { | |
| } catch (e) { |
🧰 Tools
🪛 Biome (2.5.3)
[error] 151-151: Expected a statement but instead found 'catch (e)'.
(parse)
🤖 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 150 - 151, Remove the duplicated
catch clause in the retry logic so the surrounding try statement has only one
`catch (e)` block. Preserve the existing catch body and error-handling behavior.
Source: Linters/SAST tools
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
Tests