Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/chubby-lies-report.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@clerk/clerk-js': minor
---

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.
5 changes: 5 additions & 0 deletions .changeset/five-towns-start.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@clerk/shared': minor
---

Set `retryImmediately: false` as the default for `retry()`.
2 changes: 1 addition & 1 deletion packages/clerk-js/bundlewatch.config.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"files": [
{ "path": "./dist/clerk.js", "maxSize": "582.5kB" },
{ "path": "./dist/clerk.browser.js", "maxSize": "79.6kB" },
{ "path": "./dist/clerk.browser.js", "maxSize": "79.8kB" },
{ "path": "./dist/clerk.headless*.js", "maxSize": "55KB" },
{ "path": "./dist/ui-common*.js", "maxSize": "96KB" },
{ "path": "./dist/vendors*.js", "maxSize": "30KB" },
Expand Down
22 changes: 16 additions & 6 deletions packages/clerk-js/src/core/auth/AuthCookieService.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -109,11 +110,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() {
Expand Down Expand Up @@ -173,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.
}

/**
Expand Down
22 changes: 15 additions & 7 deletions packages/clerk-js/src/core/auth/SessionCookiePoller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,29 @@ export class SessionCookiePoller {
private lock = SafeLock(REFRESH_SESSION_TOKEN_LOCK_KEY);
private workerTimers = createWorkerTimers();
private timerId: ReturnType<typeof this.workerTimers.setInterval> | null = null;
// Disallows for multiple `startPollingForSessionToken()` calls before `callback` is executed.
private initiated = false;

public startPollingForSessionToken(cb: () => unknown): void {
if (this.timerId) {
public startPollingForSessionToken(cb: () => Promise<unknown>): void {
if (this.timerId || this.initiated) {
return;
}

this.timerId = this.workerTimers.setInterval(() => {
void this.lock.acquireLockAndRun(cb);
}, INTERVAL_IN_MS);
const run = async () => {
this.initiated = true;
await this.lock.acquireLockAndRun(cb);
this.timerId = this.workerTimers.setTimeout(run, INTERVAL_IN_MS);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Credits to @nikosdouvlis

};

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;
}
this.initiated = false;
}
}
2 changes: 1 addition & 1 deletion packages/clerk-js/src/core/auth/safeLock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export function SafeLock(key: string) {
await lock.releaseLock(key);
});

const acquireLockAndRun = async (cb: () => unknown) => {
const acquireLockAndRun = async (cb: () => Promise<unknown>) => {
if ('locks' in navigator && isSecureContext) {
const controller = new AbortController();
const lockTimeout = setTimeout(() => controller.abort(), 4999);
Expand Down
23 changes: 19 additions & 4 deletions packages/clerk-js/src/core/clerk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1745,8 +1745,11 @@ export class Clerk implements ClerkInterface {
}
return this.setActive({ session: null });
} catch (err) {
// Handle the 403 Forbidden
if (err.status === 403) {
// `/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)) {
Comment on lines +1748 to +1752
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the needed ?
When /client fails with 500, we attempt to read from __session and call /tokens. If /tokens returns with a 4xx handleUnauthencated gets called which tries to reload client which will fail with 500.

At this point the user cannot really do anything to resolve this, and clerk.js cannot auto recover. We set session: null to stop the poller from falling into an infinite loop.

return this.setActive({ session: null });
} else {
throw err;
Expand Down Expand Up @@ -2148,8 +2151,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();
});
Comment on lines +2154 to +2167
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid parallel retries from this explicit invocation and the poller.


// Allows for Clerk to be marked as loaded with the client and session created from the JWT.
return null;
Expand Down
25 changes: 14 additions & 11 deletions packages/clerk-js/src/core/fapiClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 5 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
}
Expand Down
11 changes: 10 additions & 1 deletion packages/clerk-js/src/core/resources/Session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,17 @@ export class Session extends BaseResource implements SessionResource {
};

getToken: GetToken = async (options?: GetTokenOptions): Promise<string | null> => {
// 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,
initialDelay: 3 * 1000,
maxDelayBetweenRetries: 50 * 1_000,
jitter: false,
shouldRetry: (error, iterationsCount) => {
return !is4xxError(error) && iterationsCount <= 8;
},
});
};

Expand Down
4 changes: 2 additions & 2 deletions packages/shared/src/retry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how come we changed the default here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We settled on it, as the best default to have.

*/
retryImmediately: boolean;
/**
Expand All @@ -44,7 +44,7 @@ const defaultOptions: Required<RetryOptions> = {
maxDelayBetweenRetries: 0,
factor: 2,
shouldRetry: (_: unknown, iteration: number) => iteration < 5,
retryImmediately: true,
retryImmediately: false,
jitter: true,
};

Expand Down