fix(clerk-js): fail fast when the origin is slow at load#9065
Open
nikosdouvlis wants to merge 9 commits into
Open
fix(clerk-js): fail fast when the origin is slow at load#9065nikosdouvlis wants to merge 9 commits into
nikosdouvlis wants to merge 9 commits into
Conversation
When FAPI is slow or down, a cold clerk-js load hung for minutes: the /client fetch had no request timeout, and the load-failure recovery awaited getToken({ skipCache: true }), which runs a ~162s retry budget before Clerk marks itself loaded. During an outage the app sat unresponsive.
Bound the standard-browser load with a 5s timeout in two places: around the /client fetch, and around the recovery mint. On a slow or failed /client, recovery renders identity from the __session cookie, clears the session's token cache via the existing clearCache() so the mint bypasses the cache (no skipCache, so no force_origin), and awaits a fresh mint under the timeout, keeping the cookie identity if it times out. The on-timeout clear stops the poller from awaiting an abandoned in-flight mint.
Adds a timeLimit racer to @clerk/shared/utils.
🦋 Changeset detectedLatest commit: 51cf079 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.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a timeout-bounded Clerk load path, abort-aware client fetch propagation, JWT stub-user updates, and shared utility tests plus release notes. ChangesFail-fast initialization
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Contributor
API Changes Report
Summary
@clerk/sharedCurrent version: 4.25.0 Subpath
|
@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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/clerk-js/src/core/clerk.ts (2)
3184-3186: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon't swallow auth failures from the recovery mint.
The outer
/clientpath correctly rethrows 4xx at Lines 3184-3186, but the recoverysession.getToken()branch converts every failure intonull. If the cookie-backed session has already been revoked or expired, this preserves the cookie-derived identity and finishes load as signed-indegradedinstead of clearing auth state. Only timeout/network failures should be tolerated here; explicit auth failures need to sign the user out or bubble up.Also applies to: 3202-3214
🤖 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/clerk-js/src/core/clerk.ts` around lines 3184 - 3186, The recovery mint flow in clerk.ts is swallowing explicit auth failures by turning every session.getToken() error into null, which leaves revoked/expired cookie sessions looking signed-in. Update the recovery branch around session.getToken() so only timeout/network failures are tolerated and mapped to null; for auth-related failures, rethrow or sign the user out and clear the recovered identity. Use the existing is4xxError(e) handling in the /client path as the pattern, and apply the same distinction in the session.getToken() branch referenced by the affected recovery logic.
3162-3175: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrap
Environment.getInstance().fetch()with the initialization timeout
fetchMaxTriesonly limits retries here; it does not cap wall-clock time.Promise.allSettled([initEnvironmentPromise, initClient()])can still stall on a slow/environmentrequest, so this branch should use the sametimeLimit(INITIALIZATION_TIMEOUT_MS)guard as the client fetch.🤖 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/clerk-js/src/core/clerk.ts` around lines 3162 - 3175, The initialization flow in clerk.ts should apply the same wall-clock timeout to the Environment.getInstance().fetch() path as initClient(), since fetchMaxTries only limits retries and Promise.allSettled can still hang on a slow /environment request. Update the initEnvironmentPromise chain to wrap the fetch call with timeLimit(INITIALIZATION_TIMEOUT_MS) before the then/catch handling, keeping the existing updateEnvironment and fallback-to-SafeLocalStorage logic intact.
🧹 Nitpick comments (1)
packages/clerk-js/src/core/__tests__/clerk.test.ts (1)
830-834: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
anyin the session double.Line 830’s
session: anyremoves the compiler checks aroundid,getToken(), andclearCache(), so this suite can drift away from the recovery-path contract without noticing. A tiny test-local interface keeps the helper honest.As per coding guidelines, "Avoid
anytype - preferunknownwhen type is uncertain, then narrow with type guards."Suggested typing cleanup
+interface RecoverySessionDouble { + id: string; + status: 'active'; + user: Record<string, unknown>; + getToken(): Promise<string | null>; + clearCache(): void; +} + -const sessionClient = (session: any) => ({ +const sessionClient = (session: RecoverySessionDouble) => ({ signedInSessions: [session], lastActiveSessionId: session.id, });🤖 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/clerk-js/src/core/__tests__/clerk.test.ts` around lines 830 - 834, The session test double uses an untyped `any`, which bypasses compile-time checks for the recovery-path contract. Update the `sessionClient` helper in `clerk.test.ts` to use a small test-local interface or a narrowed `unknown` type that explicitly includes the `id`, `getToken()`, and `clearCache()` members, and keep `sessionlessClient` unchanged.Source: Coding guidelines
🤖 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/clerk-js/src/core/__tests__/clerk.test.ts`:
- Around line 838-848: The helper `pumpUntilSettled` in `clerk.test.ts` can
still hang because it always awaits `tracked` even after the timer budget is
exhausted. Update `pumpUntilSettled` so that if the `load()` promise has not
settled after the 20 `vi.advanceTimersByTimeAsync(5000)` iterations, it throws a
deterministic error instead of awaiting forever; keep the existing settled
tracking logic and only await `tracked` when the promise has already resolved or
rejected.
---
Outside diff comments:
In `@packages/clerk-js/src/core/clerk.ts`:
- Around line 3184-3186: The recovery mint flow in clerk.ts is swallowing
explicit auth failures by turning every session.getToken() error into null,
which leaves revoked/expired cookie sessions looking signed-in. Update the
recovery branch around session.getToken() so only timeout/network failures are
tolerated and mapped to null; for auth-related failures, rethrow or sign the
user out and clear the recovered identity. Use the existing is4xxError(e)
handling in the /client path as the pattern, and apply the same distinction in
the session.getToken() branch referenced by the affected recovery logic.
- Around line 3162-3175: The initialization flow in clerk.ts should apply the
same wall-clock timeout to the Environment.getInstance().fetch() path as
initClient(), since fetchMaxTries only limits retries and Promise.allSettled can
still hang on a slow /environment request. Update the initEnvironmentPromise
chain to wrap the fetch call with timeLimit(INITIALIZATION_TIMEOUT_MS) before
the then/catch handling, keeping the existing updateEnvironment and
fallback-to-SafeLocalStorage logic intact.
---
Nitpick comments:
In `@packages/clerk-js/src/core/__tests__/clerk.test.ts`:
- Around line 830-834: The session test double uses an untyped `any`, which
bypasses compile-time checks for the recovery-path contract. Update the
`sessionClient` helper in `clerk.test.ts` to use a small test-local interface or
a narrowed `unknown` type that explicitly includes the `id`, `getToken()`, and
`clearCache()` members, and keep `sessionlessClient` unchanged.
🪄 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: adbc0a12-845a-4bcc-8b5e-c59355326470
📒 Files selected for processing (6)
.changeset/clerkjs-fail-fast-slow-origin.mdpackages/clerk-js/src/core/__tests__/clerk.test.tspackages/clerk-js/src/core/clerk.tspackages/shared/src/utils/__tests__/timeLimit.test.tspackages/shared/src/utils/index.tspackages/shared/src/utils/timeLimit.ts
Member
Author
|
!snapshot |
This comment has been minimized.
This comment has been minimized.
Bound the load /client fetch and abort it on timeout instead of leaving it in flight. On a slow or failed /client, mint a fresh token first (clearCache + plain getToken, so no force_origin and the minter can still serve it during an origin outage) and derive the degraded identity from that token, falling back to the __session cookie identity only when the mint fails or times out. The mint is bounded by the same timeout. After the degraded load, retry /client once in the background with no timeout and apply it only if nothing else updated the client meanwhile, so a sign-out or a piggybacked client wins over a late response. Stamp the cookie-derived stub user with updated_at: 1 so listener memoization replaces it once the full user arrives, fixing useUser() returning the stub after recovery. Add a timeLimit util to @clerk/shared/utils that optionally aborts an AbortController on timeout.
Member
Author
|
!snapshot |
Member
Author
|
!snapshot |
Contributor
|
Hey @nikosdouvlis - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/astro@3.4.12-snapshot.v20260703092711 --save-exact
npm i @clerk/backend@3.11.0-snapshot.v20260703092711 --save-exact
npm i @clerk/chrome-extension@3.1.48-snapshot.v20260703092711 --save-exact
npm i @clerk/clerk-js@6.24.0-snapshot.v20260703092711 --save-exact
npm i @clerk/electron@0.0.9-snapshot.v20260703092711 --save-exact
npm i @clerk/electron-passkeys@0.0.4-snapshot.v20260703092711 --save-exact
npm i @clerk/eslint-plugin@0.2.1-snapshot.v20260703092711 --save-exact
npm i @clerk/expo@3.7.0-snapshot.v20260703092711 --save-exact
npm i @clerk/expo-passkeys@1.2.0-snapshot.v20260703092711 --save-exact
npm i @clerk/express@2.1.36-snapshot.v20260703092711 --save-exact
npm i @clerk/fastify@3.1.46-snapshot.v20260703092711 --save-exact
npm i @clerk/headless@0.0.7-snapshot.v20260703092711 --save-exact
npm i @clerk/hono@0.1.46-snapshot.v20260703092711 --save-exact
npm i @clerk/localizations@4.12.1-snapshot.v20260703092711 --save-exact
npm i @clerk/msw@0.0.43-snapshot.v20260703092711 --save-exact
npm i @clerk/nextjs@7.5.13-snapshot.v20260703092711 --save-exact
npm i @clerk/nuxt@2.6.12-snapshot.v20260703092711 --save-exact
npm i @clerk/react@6.11.4-snapshot.v20260703092711 --save-exact
npm i @clerk/react-router@3.5.5-snapshot.v20260703092711 --save-exact
npm i @clerk/shared@4.24.0-snapshot.v20260703092711 --save-exact
npm i @clerk/swingset@0.0.14-snapshot.v20260703092711 --save-exact
npm i @clerk/tanstack-react-start@1.4.13-snapshot.v20260703092711 --save-exact
npm i @clerk/testing@2.2.3-snapshot.v20260703092711 --save-exact
npm i @clerk/ui@1.24.2-snapshot.v20260703092711 --save-exact
npm i @clerk/upgrade@2.0.5-snapshot.v20260703092711 --save-exact
npm i @clerk/vue@2.4.11-snapshot.v20260703092711 --save-exact |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shared/src/utils/timeLimit.ts (1)
1-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing JSDoc on exported public utility.
timeLimitis a new shared, publicly exported utility (via@clerk/shared/utils) but has no JSDoc. The repo guideline requires documenting functions with@param/@returns/@throws, especially for public APIs.📝 Suggested JSDoc
+/** + * Races a value (or promise) against a timeout, rejecting if the timeout elapses first. + * + * `@param` value - The value or promise to race against the timeout. + * `@param` ms - The timeout duration in milliseconds. + * `@param` abortController - Optional controller aborted (with no reason) when the timeout wins the race. + * `@returns` A promise that resolves with `value` or rejects with a timeout `Error`. + * `@throws` {Error} When `ms` elapses before `value` settles. + */ export function timeLimit<T>( value: T | PromiseLike<T>, ms: number, abortController?: Pick<AbortController, 'abort'>, ): Promise<T> {🤖 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/utils/timeLimit.ts` around lines 1 - 25, The exported public utility timeLimit lacks the required JSDoc documentation; add a doc comment directly above the timeLimit function that explains its timeout behavior and includes `@param` entries for value, ms, and abortController, plus `@returns` for the raced promise and `@throws` only if applicable. Keep the documentation aligned with the function’s behavior in timeLimit so it is usable as a shared `@clerk/shared/utils` API.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/shared/src/utils/timeLimit.ts`:
- Around line 1-25: The exported public utility timeLimit lacks the required
JSDoc documentation; add a doc comment directly above the timeLimit function
that explains its timeout behavior and includes `@param` entries for value, ms,
and abortController, plus `@returns` for the raced promise and `@throws` only if
applicable. Keep the documentation aligned with the function’s behavior in
timeLimit so it is usable as a shared `@clerk/shared/utils` API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c6acc78a-1d98-48f6-8e80-ade33e212ddc
📒 Files selected for processing (6)
packages/clerk-js/src/core/__tests__/clerk.test.tspackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/resources/Base.tspackages/clerk-js/src/core/resources/Client.tspackages/shared/src/utils/__tests__/timeLimit.test.tspackages/shared/src/utils/timeLimit.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/clerk-js/src/core/resources/Client.ts
- packages/clerk-js/src/core/clerk.ts
- packages/clerk-js/src/core/tests/clerk.test.ts
nikosdouvlis
force-pushed
the
nikos/plat-2754-clerkjs-resiliency
branch
from
fd33d8a to
90ef111
Compare
Why: Follow-up fixes on the fail-fast slow-origin load path. The background /client retry could overwrite newer client state (a sign-out, or a mutation's piggybacked client) with a stale response, because Client.fetch() mutates the shared Client singleton in place before the generation guard runs. A throwing updateClient listener during degraded recovery could skip the poller restart and leave session-token refresh stopped until a full page reload. The timeLimit timeout error was passed as the AbortSignal reason, so a host fetch wrapper reading signal.reason could observe an internal Clerk error. The new fetch option `signal` also collided with Clerk's existing reactive signals concept. What changed: The background /client retry now reads raw JSON via Client._fetch and applies fromJSON only inside the generation check, so a superseded response never mutates the shared client. Degraded recovery wraps the identity rebuild in try/finally so startPollingForToken always runs. timeLimit aborts with no reason; the descriptive error stays only on the promise rejection. Renamed the fetch option to `abortSignal` and timeLimit's parameter to `abortController` - both are new on this branch, so there is no compatibility impact.
nikosdouvlis
force-pushed
the
nikos/plat-2754-clerkjs-resiliency
branch
from
90ef111 to
e0de525
Compare
Why: The degraded-load recovery carried a client-generation counter to discard a background /client response that raced a newer client update. It added an instance field, an increment in the hot updateClient path, and a low-level fetch that reached past the resource API. Dropped it: the background retry now applies the /client response directly, and the rare stale overwrite it guarded against is self-correcting via the poller. Also collapsed the mint fallback so recovery derives one client from a single jwt (minted, or the cookie jwt on mint failure) rather than branching on a separate localClient.
…d retry The "fails fast when the client fetch hangs" test made every mocked fetch hang, including the background /client retry, which left a forever-pending promise that stalled the test under CI. Scope the hang to the primary fetch and let the retry resolve. Also inline the session-cookie read at its use sites in the recovery path instead of hoisting a variable.
Compute the recovery jwt once (minted when a session exists, else the cookie) and build the client from it in a single place, instead of duplicating updateClient(createClientFromJwt(...)) across both branches.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
When Clerk's Frontend API is slow or unreachable, a cold Clerk.load() hangs for minutes. The /client request has no timeout, and on a 5xx or network error the recovery path awaits getToken({ skipCache: true }), which burns the full ~162s retry budget before Clerk is marked loaded. So during an origin outage the app sits there unresponsive instead of degrading and rendering.
What changed
Bound the browser load with a timeout in two spots: around the /client fetch and around the recovery token mint. On a slow or failed /client, recovery renders identity from the __session cookie, clears the session's token cache (via the existing clearCache()) so the mint bypasses the cache (no skipCache, so no force_origin), and keeps the cookie identity if the mint times out. Also adds a timeLimit util to @clerk/shared/utils.
One known gap: timeLimit stops waiting but doesn't cancel the request, so a timed-out recovery mint keeps retrying in the background and can briefly write a slightly stale token before the poller reconciles. Low severity and self-correcting, and the abortable-timeout follow-up plus the in-flight monotonic-session-token guards close it. This is phase 1, later phases cover fail-fast /environment and render-first init.
Summary by CodeRabbit