-
Notifications
You must be signed in to change notification settings - Fork 431
feat(clerk-js): Proactive session token refresh #7317
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 6c3a5bf The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
📝 WalkthroughWalkthroughSession token handling was refactored to a stale-while-revalidate model with proactive background refresh. SessionTokenCache.get now returns a TokenCacheGetResult object with an 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@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: |
Ephem
left a comment
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.
I like the latest changes! Using the poller for the background revalidation makes sense to me. I think there's still some torny parts to untangle, and besides the latest comments, something still feels harder than it should be here, so I wonder if we've found the correct mental model for this yet?
I have a few early thoughts, let's chat!
|
|
||
| if (expiresSoon) { | ||
| // Token expired or dangerously close to expiration - force synchronous refresh | ||
| if (remainingTtl <= POLLER_INTERVAL_IN_MS / 1000) { |
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.
Default LEEWAY was 10s, and SYNC_LEEWAY was 5s, so if I'm reading this correctly, we used to sync fetch when less than 15s was remaining? Now this value is 5s?
I wonder if 5s is enough considering latency and all other factors?
However we change this it's a breaking change so let's remember to document it properly in the changelog when we've figured it out.
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.
Right. Before we would do a blocking fetch under the 15s, now we use the stale value when it's under 15s but more than 5 seconds. The idea there is that the poller would async fetch the token before we get to the 5s. If that doesn't occur then we would force a sync fetch.
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.
And yes, this is a breaking change so it's going to be part of Core 3
| } | ||
|
|
||
| return value.entry; | ||
| const effectiveLeeway = Math.max(leeway, MIN_REMAINING_TTL_IN_SECONDS); |
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.
This part is a bit weird. MIN_REMAINING_TTL_IN_SECONDS is 15s, and default LEEWAY is 10s, but the Math.max ensures we never go below 15s right?
If this is what we want(?), I think we should increase the LEEWAY to 15s as well, that way we document that "you can only raise this".
Thinking more on it, this PR now changes the public leewayInSeconds to only apply if you also use the internal refreshTokenOnFocus option? That seems off.
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.
🤔 In agreement here, this effectively forces leeway to always be equal to or greater than MIN_REMAINING_TTL_IN_SECONDS, do we want that value to be 5 or 15 going forward?
| try { | ||
| const token = await this.clerk.session.getToken(); | ||
| // Use refreshIfStale to fetch fresh token when cached token is within leeway period | ||
| const token = await this.clerk.session.getToken({ refreshIfStale: true }); |
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.
This also affects the refreshTokenOnFocus, but do we want that? Because those cases can be a race to finish setting the cookie before any external data fetching runs, it seems prudent to use the available token then?
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.
🤔
| // Prefer synchronous read to avoid microtask overhead when token is already resolved | ||
| const cachedToken = cacheResult.entry.resolvedToken ?? (await cacheResult.entry.tokenResolver); |
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.
As I mentioned in chat, when I made the comment about this I didn't think it through fully. To get the full benefits, we'd have to make sure _getToken/getToken itself to be synchronous when data is already available (or maybe more likely add the .status and .value/.reason fields to the promise).
This likely only makes sense if/when we decide to expose this promise to the end user so they can use it though.
I see you've used resolvedToken as the way to be able to have a background refetch running while still reading the currently cached token though so this still has immediate value. That was a bit unclear at first, but I think it makes sense, will need to think some more on it.
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.
Agreed. And yeah, right now the value is key to allow for returning the stale value, while we wait for the poller to do it's thing
d24d455 to
12a12f5
Compare
Replace the stale-while-revalidate approach with a timer-based proactive refresh mechanism. Instead of relying on getToken() calls to trigger background refresh, we now schedule a timer when tokens are cached that fires before the leeway period begins. Key changes: - Add onRefresh callback to TokenCacheEntry for proactive refresh - Schedule refresh timer at (TTL - leeway - 2s) = 43s for 60s tokens - Remove needsRefresh flag and refreshThreshold parameter from cache - Remove backgroundRefreshThreshold and refreshIfStale from GetTokenOptions - Update Session to pass onRefresh callback when caching tokens Benefits: - More consistent refresh regardless of getToken() call frequency - Zero risk: if timer doesn't fire, poller catches it as fallback - Fits naturally with existing cleanup timer pattern
|
!snapshot |
|
Hey @jacekradko - 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 |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.changeset/fresh-tigers-hunt.md:
- Around line 1-4: The changeset incorrectly marks a breaking change as "minor"
— reintroduce the removed public option and/or make leewayInSeconds optional on
the GetTokenOptions interface (or add it back with a deprecated comment) so
existing callers continue to compile, and update the .changeset entry to a major
bump; look for the GetTokenOptions declaration and the
.changeset/fresh-tigers-hunt.md file and either restore leewayInSeconds to the
public interface (or mark it optional/deprecated) and change the version line(s)
from minor to major.
🧹 Nitpick comments (1)
.changeset/fresh-tigers-hunt.md (1)
6-8: Add migration guidance forleewayInSecondsremoval.The changeset should include migration guidance for users currently passing
leewayInSeconds. Consider adding a note explaining that the option is no longer needed and will be ignored, or provide alternative configuration if applicable.📝 Suggested improvement
Add proactive session token refresh. Tokens are now automatically refreshed in the background before they expire, eliminating the need for manual refresh configuration. -Remove `leewayInSeconds` from `GetTokenOptions`. Token refresh timing is now handled automatically. +**BREAKING**: Remove `leewayInSeconds` from `GetTokenOptions`. Token refresh timing is now handled automatically. If you were previously passing `leewayInSeconds` to `getToken()`, you can safely remove this option—tokens will now be refreshed proactively in the background without additional configuration.
packages/upgrade/src/versions/core-3/changes/gettoken-stale-while-revalidate.md
Outdated
Show resolved
Hide resolved
| shouldDispatchTokenUpdate, | ||
| leewayInSeconds, | ||
| ), | ||
| }); |
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 are we triggering the same method here?
- Remove leewayInSeconds from GetTokenOptions type - Delete gettoken-leeway-minimum.md upgrade doc - Update changeset to major (breaking change) - Add comments explaining refreshLeadTime and onRefresh re-registration - Rename upgrade doc title to "proactive background refresh" - Simplify internal token cache to use default threshold
|
!allow-major |
Description
This PR implements proactive token refresh for session tokens. Instead of waiting for
getToken()calls to trigger refresh when a token is close to expiration, we schedule a timer when tokens are cached that fires proactively before the leeway period begins.Fixes: USER-4087
Dashboard testing PR: https://github.com/clerk/dashboard/pull/7990
How It Works
Key Behaviors
Benefits
getToken()call frequencyBroadcastChannelImplementation Details
Token Cache (
tokenCache.ts)onRefreshcallback on cache entries for proactive refresh(TTL - leeway - 2s)when tokens are cachedBroadcastChannelSession (
Session.ts)#refreshTokenInBackground(): Fires token refresh without blocking#backgroundRefreshInProgressSet prevents duplicate concurrent refreshesonRefreshcallback passed when caching tokensresolvedTokenon cache entries enables synchronous reads (avoids microtask overhead)Constants
Breaking Changes
leewayInSecondsoption: TheleewayInSecondsoption has been removed fromgetToken(). Token refresh timing is now handled automatically by the proactive refresh system. A codemod is available via@clerk/upgradeto remove this option from existing code.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Breaking Changes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.