-
Notifications
You must be signed in to change notification settings - Fork 346
chore(clerk-js): Improve session refresh retry logic #5397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(clerk-js): Improve session refresh retry logic #5397
Conversation
🦋 Changeset detectedLatest commit: 5b382a3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 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 Git ↗︎
|
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; | ||
}, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Credits to @nikosdouvlis
// 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; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Credits to @nikosdouvlis
}, INTERVAL_IN_MS); | ||
const run = async () => { | ||
await this.lock.acquireLockAndRun(cb); | ||
this.timerId = this.workerTimers.setTimeout(run, INTERVAL_IN_MS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Credits to @nikosdouvlis
/** | ||
* 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(); | ||
}); |
There was a problem hiding this comment.
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.
const run = async () => { | ||
await this.lock.acquireLockAndRun(cb); | ||
this.timerId = this.workerTimers.setTimeout(run, INTERVAL_IN_MS); | ||
}; | ||
|
||
void run(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reviewers:
This change allows for this behaviour
getToken -> 500 -> retry... retry.. retry... -> success/failure -> getToken
Until now, we had this one, which is no longer desired.
getToken -> 500 -> retry... retry.. retry... -> success/failure -> getToken
getToken -> 500 -> retry... retry.. retry... -> success/failure -> getToken
getToken -> 500 -> retry... retry.. retry... -> success/failure -> getToken
// `/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)) { |
There was a problem hiding this comment.
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.
…poller-when-signs-inout' into elef/sdki-949-startstop-session-poller-when-signs-inout
…signs-inout # Conflicts: # packages/clerk-js/bundlewatch.config.json
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
!snapshot |
Hey @panteliselef - 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/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact |
Description
This PR performs the following improvements related to how we are refreshing the session token.
getToken()
call to have concluded before attempting to poll again. Example here.Clerk.handleUnauthenticated()
will set session as null on 500 status code/client
response. Avoid infinite request loops, see here./client
fails on init, we stop the poller -> create the dummy client -> attempt manual request to/tokens
-> start polling again.retry
no longer fires immediately by default.Checklist
pnpm test
runs as expected.pnpm build
runs as expected.Type of change