From 28880dc5a0e1ab2c8db0ad67c09d3ea6ba84e704 Mon Sep 17 00:00:00 2001 From: panteliselef Date: Wed, 19 Mar 2025 14:59:33 +0200 Subject: [PATCH 1/9] chore(clerk-js): Improve session refresh retry logic --- .changeset/chubby-lies-report.md | 6 +++++ .../src/core/auth/AuthCookieService.ts | 11 ++++++-- .../src/core/auth/SessionCookiePoller.ts | 16 +++++++----- packages/clerk-js/src/core/auth/safeLock.ts | 2 +- packages/clerk-js/src/core/clerk.ts | 20 ++++++++++++--- packages/clerk-js/src/core/fapiClient.ts | 25 +++++++++++-------- .../clerk-js/src/core/resources/Session.ts | 12 ++++++++- packages/shared/src/retry.ts | 4 +-- 8 files changed, 69 insertions(+), 27 deletions(-) create mode 100644 .changeset/chubby-lies-report.md diff --git a/.changeset/chubby-lies-report.md b/.changeset/chubby-lies-report.md new file mode 100644 index 00000000000..7c523ff5303 --- /dev/null +++ b/.changeset/chubby-lies-report.md @@ -0,0 +1,6 @@ +--- +'@clerk/clerk-js': minor +'@clerk/shared': minor +--- + +Improve session refresh retry logic. diff --git a/packages/clerk-js/src/core/auth/AuthCookieService.ts b/packages/clerk-js/src/core/auth/AuthCookieService.ts index 0f070cd6860..1a23c0a8fa9 100644 --- a/packages/clerk-js/src/core/auth/AuthCookieService.ts +++ b/packages/clerk-js/src/core/auth/AuthCookieService.ts @@ -109,11 +109,18 @@ export class AuthCookieService { this.devBrowser.clear(); } - private startPollingForToken() { + public startPollingForToken() { if (!this.poller) { this.poller = new SessionCookiePoller(); + this.poller.startPollingForSessionToken(() => this.refreshSessionToken()); + } + } + + public stopPollingForToken() { + if (this.poller) { + this.poller.stopPollingForSessionToken(); + this.poller = null; } - this.poller.startPollingForSessionToken(() => this.refreshSessionToken()); } private refreshTokenOnFocus() { diff --git a/packages/clerk-js/src/core/auth/SessionCookiePoller.ts b/packages/clerk-js/src/core/auth/SessionCookiePoller.ts index 4497b87c28d..5a5fde162bf 100644 --- a/packages/clerk-js/src/core/auth/SessionCookiePoller.ts +++ b/packages/clerk-js/src/core/auth/SessionCookiePoller.ts @@ -10,19 +10,23 @@ export class SessionCookiePoller { private workerTimers = createWorkerTimers(); private timerId: ReturnType | null = null; - public startPollingForSessionToken(cb: () => unknown): void { + public startPollingForSessionToken(cb: () => Promise): void { if (this.timerId) { return; } - this.timerId = this.workerTimers.setInterval(() => { - void this.lock.acquireLockAndRun(cb); - }, INTERVAL_IN_MS); + const run = async () => { + await this.lock.acquireLockAndRun(cb); + this.timerId = this.workerTimers.setTimeout(run, INTERVAL_IN_MS); + }; + + void run(); } public stopPollingForSessionToken(): void { - if (this.timerId) { - this.workerTimers.clearInterval(this.timerId); + // Note: `timerId` can be 0. + if (this.timerId != null) { + this.workerTimers.clearTimeout(this.timerId); this.timerId = null; } } diff --git a/packages/clerk-js/src/core/auth/safeLock.ts b/packages/clerk-js/src/core/auth/safeLock.ts index baf2c07511d..405190a73ff 100644 --- a/packages/clerk-js/src/core/auth/safeLock.ts +++ b/packages/clerk-js/src/core/auth/safeLock.ts @@ -9,7 +9,7 @@ export function SafeLock(key: string) { await lock.releaseLock(key); }); - const acquireLockAndRun = async (cb: () => unknown) => { + const acquireLockAndRun = async (cb: () => Promise) => { if ('locks' in navigator && isSecureContext) { const controller = new AbortController(); const lockTimeout = setTimeout(() => controller.abort(), 4999); diff --git a/packages/clerk-js/src/core/clerk.ts b/packages/clerk-js/src/core/clerk.ts index 58ee4533f35..95253727795 100644 --- a/packages/clerk-js/src/core/clerk.ts +++ b/packages/clerk-js/src/core/clerk.ts @@ -1682,8 +1682,8 @@ export class Clerk implements ClerkInterface { } return this.setActive({ session: null }); } catch (err) { - // Handle the 403 Forbidden - if (err.status === 403) { + // Clerk is already attempting to sign out the user, respect the sign-out when client fails to re-load. + if (isClerkAPIResponseError(err) && err.status > 400) { return this.setActive({ session: null }); } else { throw err; @@ -2067,8 +2067,20 @@ export class Clerk implements ClerkInterface { this.updateClient(localClient); - // Always grab a fresh token - await this.session?.getToken({ skipCache: true }); + /** + * In most scenarios we want the poller to stop while we are fetching a fresh token during an outage. + * We want to avoid having the below `getToken()` retrying at the same time as the poller. + */ + this.#authService?.stopPollingForToken(); + + // Attempt to grab a fresh token + await this.session + ?.getToken({ skipCache: true }) + // If the token fetch fails, let Clerk be marked as loaded and leave it up to the poller. + .catch(() => null) + .finally(() => { + this.#authService?.startPollingForToken(); + }); // Allows for Clerk to be marked as loaded with the client and session created from the JWT. return null; diff --git a/packages/clerk-js/src/core/fapiClient.ts b/packages/clerk-js/src/core/fapiClient.ts index f996ccd8db4..32e52272c62 100644 --- a/packages/clerk-js/src/core/fapiClient.ts +++ b/packages/clerk-js/src/core/fapiClient.ts @@ -232,17 +232,20 @@ export function createFapiClient(options: FapiClientOptions): FapiClient { try { if (beforeRequestCallbacksResult) { const maxTries = requestOptions?.fetchMaxTries ?? (isBrowserOnline() ? 4 : 11); - response = - // retry only on GET requests for safety - overwrittenRequestMethod === 'GET' - ? await retry(() => fetch(urlStr, fetchOpts), { - initialDelay: 500, - maxDelayBetweenRetries: 3000, - shouldRetry: (_: unknown, iterationsCount: number) => { - return iterationsCount < maxTries; - }, - }) - : await fetch(urlStr, fetchOpts); + response = await retry(() => fetch(urlStr, fetchOpts), { + // This retry handles only network errors, not 4xx or 5xx responses, + // so we want to try once immediately to handle simple network blips. + // Since fapiClient is responsible for the network layer only, + // callers need to use their own retry logic where needed. + retryImmediately: true, + // And then exponentially back off with a max delay of 3 seconds. + initialDelay: 700, + maxDelayBetweenRetries: 5000, + shouldRetry: (_: unknown, iterations: number) => { + // We want to retry only GET requests, as other methods are not idempotent. + return overwrittenRequestMethod === 'GET' && iterations < maxTries; + }, + }); } else { response = new Response('{}', requestInit); // Mock an empty json response } diff --git a/packages/clerk-js/src/core/resources/Session.ts b/packages/clerk-js/src/core/resources/Session.ts index 61c42a2d237..6216dea7066 100644 --- a/packages/clerk-js/src/core/resources/Session.ts +++ b/packages/clerk-js/src/core/resources/Session.ts @@ -92,8 +92,18 @@ export class Session extends BaseResource implements SessionResource { }; getToken: GetToken = async (options?: GetTokenOptions): Promise => { + // This will retry the getToken call if it fails with a non-4xx error + // We're going to trigger 8 retries in the span of ~3 minutes, + // Example delays: 3s, 5s, 13s, 19s, 26s, 34s, 43s, 50s, total: ~193s return retry(() => this._getToken(options), { - shouldRetry: (error: unknown, currentIteration: number) => !is4xxError(error) && currentIteration < 4, + factor: 1.55, + retryImmediately: false, + initialDelay: 3 * 1000, + maxDelayBetweenRetries: 50 * 1_000, + jitter: false, + shouldRetry: (error, iterationsCount) => { + return !is4xxError(error) && iterationsCount <= 8; + }, }); }; diff --git a/packages/shared/src/retry.ts b/packages/shared/src/retry.ts index 8b98bab0874..68603e61cff 100644 --- a/packages/shared/src/retry.ts +++ b/packages/shared/src/retry.ts @@ -29,7 +29,7 @@ type RetryOptions = Partial<{ /** * Controls whether the helper should retry the operation immediately once before applying exponential backoff. * The delay for the immediate retry is 100ms. - * @default true + * @default false */ retryImmediately: boolean; /** @@ -44,7 +44,7 @@ const defaultOptions: Required = { maxDelayBetweenRetries: 0, factor: 2, shouldRetry: (_: unknown, iteration: number) => iteration < 5, - retryImmediately: true, + retryImmediately: false, jitter: true, }; From 5a4f823c2881501858d43523b6777d13ff1ca3fe Mon Sep 17 00:00:00 2001 From: panteliselef Date: Thu, 20 Mar 2025 15:47:15 +0200 Subject: [PATCH 2/9] improve handleUnauthenticated catch --- packages/clerk-js/src/core/clerk.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/clerk-js/src/core/clerk.ts b/packages/clerk-js/src/core/clerk.ts index 840009a397d..df0e25c3f5f 100644 --- a/packages/clerk-js/src/core/clerk.ts +++ b/packages/clerk-js/src/core/clerk.ts @@ -1692,8 +1692,11 @@ export class Clerk implements ClerkInterface { } return this.setActive({ session: null }); } catch (err) { - // Clerk is already attempting to sign out the user, respect the sign-out when client fails to re-load. - if (isClerkAPIResponseError(err) && err.status > 400) { + // `/client` can fail with either a 401, a 403, 500 or network errors. + // 401 is already handled internally in our fetcher. + // 403 means that the client is blocked, signing out the user is the only option. + // 500 means that the client is not working, signing out the user is the only option, since the intention was to sign out the user. + if (isClerkAPIResponseError(err) && [403, 500].includes(err.status)) { return this.setActive({ session: null }); } else { throw err; From be509e9421c05773d421b71bebd9d4389b43f965 Mon Sep 17 00:00:00 2001 From: panteliselef Date: Fri, 21 Mar 2025 18:18:53 +0200 Subject: [PATCH 3/9] Update packages/clerk-js/src/core/fapiClient.ts --- packages/clerk-js/src/core/fapiClient.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/clerk-js/src/core/fapiClient.ts b/packages/clerk-js/src/core/fapiClient.ts index 32e52272c62..7aae4e0b107 100644 --- a/packages/clerk-js/src/core/fapiClient.ts +++ b/packages/clerk-js/src/core/fapiClient.ts @@ -238,7 +238,7 @@ export function createFapiClient(options: FapiClientOptions): FapiClient { // Since fapiClient is responsible for the network layer only, // callers need to use their own retry logic where needed. retryImmediately: true, - // And then exponentially back off with a max delay of 3 seconds. + // And then exponentially back off with a max delay of 5 seconds. initialDelay: 700, maxDelayBetweenRetries: 5000, shouldRetry: (_: unknown, iterations: number) => { From 49200e1c68028282631c1d3fef28bfaa9fdbed2c Mon Sep 17 00:00:00 2001 From: panteliselef Date: Fri, 21 Mar 2025 18:30:17 +0200 Subject: [PATCH 4/9] update bundlewatch --- packages/clerk-js/bundlewatch.config.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/clerk-js/bundlewatch.config.json b/packages/clerk-js/bundlewatch.config.json index 189f6279cb6..469d0c9e408 100644 --- a/packages/clerk-js/bundlewatch.config.json +++ b/packages/clerk-js/bundlewatch.config.json @@ -1,7 +1,7 @@ { "files": [ { "path": "./dist/clerk.js", "maxSize": "580kB" }, - { "path": "./dist/clerk.browser.js", "maxSize": "78.6kB" }, + { "path": "./dist/clerk.browser.js", "maxSize": "78.8kB" }, { "path": "./dist/clerk.headless.js", "maxSize": "55KB" }, { "path": "./dist/ui-common*.js", "maxSize": "94KB" }, { "path": "./dist/vendors*.js", "maxSize": "30KB" }, From 05fb33e44175f0308d840264dec0e258ac479094 Mon Sep 17 00:00:00 2001 From: panteliselef Date: Fri, 21 Mar 2025 18:34:03 +0200 Subject: [PATCH 5/9] update changeset --- .changeset/five-towns-start.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/five-towns-start.md diff --git a/.changeset/five-towns-start.md b/.changeset/five-towns-start.md new file mode 100644 index 00000000000..7103589fd52 --- /dev/null +++ b/.changeset/five-towns-start.md @@ -0,0 +1,5 @@ +--- +'@clerk/shared': minor +--- + +Set `retryImmediately: false` as the default for `retry()`. From 24c37356c1937d82d2bfe5d1b81cfcf77fc8019c Mon Sep 17 00:00:00 2001 From: panteliselef Date: Fri, 21 Mar 2025 18:37:08 +0200 Subject: [PATCH 6/9] update changeset --- .changeset/chubby-lies-report.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.changeset/chubby-lies-report.md b/.changeset/chubby-lies-report.md index 7c523ff5303..3fb11a56c62 100644 --- a/.changeset/chubby-lies-report.md +++ b/.changeset/chubby-lies-report.md @@ -1,6 +1,8 @@ --- '@clerk/clerk-js': minor -'@clerk/shared': minor --- -Improve session refresh retry logic. +Improve session refresh logic. +- Switched from interval-based polling to timeout-based polling, ensuring retries for a `getToken()` call complete before the next poll begins. +- `Clerk.handleUnauthenticated()` now sets the session to null when a `/client` request returns a `500` status code, preventing infinite request loops. +- Improved error handling: If the `/client` request fails during initialization, the poller stops, a dummy client is created, a manual request to `/tokens` is attempted, and polling resumes. From a295c095d71b68cd8dcf841eb7fb2b1a12b071fe Mon Sep 17 00:00:00 2001 From: panteliselef Date: Thu, 27 Mar 2025 13:34:51 +0200 Subject: [PATCH 7/9] noop on network errors --- packages/clerk-js/src/core/auth/AuthCookieService.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/clerk-js/src/core/auth/AuthCookieService.ts b/packages/clerk-js/src/core/auth/AuthCookieService.ts index 1a23c0a8fa9..6fd67cae50e 100644 --- a/packages/clerk-js/src/core/auth/AuthCookieService.ts +++ b/packages/clerk-js/src/core/auth/AuthCookieService.ts @@ -1,6 +1,7 @@ import { createCookieHandler } from '@clerk/shared/cookie'; import { setDevBrowserJWTInURL } from '@clerk/shared/devBrowser'; -import { is4xxError, isClerkAPIResponseError } from '@clerk/shared/error'; +import { is4xxError, isClerkAPIResponseError, isClerkRuntimeError } from '@clerk/shared/error'; +import { noop } from '@clerk/shared/utils'; import type { Clerk, InstanceType } from '@clerk/types'; import { clerkCoreErrorTokenRefreshFailed, clerkMissingDevBrowserJwt } from '../errors'; @@ -180,16 +181,18 @@ export class AuthCookieService { } private handleGetTokenError(e: any) { - //throw if not a clerk error - if (!isClerkAPIResponseError(e)) { + //throw if not a clerk api error (aka fapi error) and not a network error + if (!isClerkAPIResponseError(e) && !isClerkRuntimeError(e)) { clerkCoreErrorTokenRefreshFailed(e.message || e); } //sign user out if a 4XX error if (is4xxError(e)) { - void this.clerk.handleUnauthenticated(); + void this.clerk.handleUnauthenticated().catch(noop); return; } + // Treat any other error as a noop + // TODO(debug-logs): Once debug logs is available log this error. } /** From 28fe0688767665a6a2bbeec7102bd8ad63e1cad3 Mon Sep 17 00:00:00 2001 From: panteliselef Date: Thu, 27 Mar 2025 15:33:03 +0200 Subject: [PATCH 8/9] address comment --- packages/clerk-js/src/core/resources/Session.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/clerk-js/src/core/resources/Session.ts b/packages/clerk-js/src/core/resources/Session.ts index 6216dea7066..85c98c30830 100644 --- a/packages/clerk-js/src/core/resources/Session.ts +++ b/packages/clerk-js/src/core/resources/Session.ts @@ -97,7 +97,6 @@ export class Session extends BaseResource implements SessionResource { // Example delays: 3s, 5s, 13s, 19s, 26s, 34s, 43s, 50s, total: ~193s return retry(() => this._getToken(options), { factor: 1.55, - retryImmediately: false, initialDelay: 3 * 1000, maxDelayBetweenRetries: 50 * 1_000, jitter: false, From 5b382a30e54d5afefe3bf4a9a62c75f35763a3b5 Mon Sep 17 00:00:00 2001 From: panteliselef Date: Thu, 27 Mar 2025 15:56:50 +0200 Subject: [PATCH 9/9] protect from multiple invocations until timerId is populated. --- packages/clerk-js/src/core/auth/SessionCookiePoller.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/clerk-js/src/core/auth/SessionCookiePoller.ts b/packages/clerk-js/src/core/auth/SessionCookiePoller.ts index 5a5fde162bf..91e8040f79d 100644 --- a/packages/clerk-js/src/core/auth/SessionCookiePoller.ts +++ b/packages/clerk-js/src/core/auth/SessionCookiePoller.ts @@ -9,13 +9,16 @@ export class SessionCookiePoller { private lock = SafeLock(REFRESH_SESSION_TOKEN_LOCK_KEY); private workerTimers = createWorkerTimers(); private timerId: ReturnType | null = null; + // Disallows for multiple `startPollingForSessionToken()` calls before `callback` is executed. + private initiated = false; public startPollingForSessionToken(cb: () => Promise): void { - if (this.timerId) { + if (this.timerId || this.initiated) { return; } const run = async () => { + this.initiated = true; await this.lock.acquireLockAndRun(cb); this.timerId = this.workerTimers.setTimeout(run, INTERVAL_IN_MS); }; @@ -29,5 +32,6 @@ export class SessionCookiePoller { this.workerTimers.clearTimeout(this.timerId); this.timerId = null; } + this.initiated = false; } }