fix(clerk-js): Fix token cache refresh timer leak#8098
fix(clerk-js): Fix token cache refresh timer leak#8098jacekradko wants to merge 8 commits intomainfrom
Conversation
When set() was called multiple times for the same cache key, old refresh and expiration timers were never cancelled. This caused orphaned timers to accumulate — each independently firing onRefresh — making the poller appear erratic with accelerating token refresh requests. The root cause: both _updateClient (via Session constructor → #hydrateCache) and #refreshTokenInBackground call set() for the same key during a single refresh cycle. Each set() created new timers without clearing the old ones. After N cycles, N orphaned timers all fire at different offsets. Fix: clear existing timers at the start of setInternal before creating the new cache entry.
🦋 Changeset detectedLatest commit: c0f0f3d 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 |
|
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/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: |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughClears prior refresh and expiration timers before updating entries in 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/clerk-js/src/core/tokenCache.ts`:
- Around line 353-364: The cache set flow currently only clears timers on
existing entries but doesn't prevent a pending tokenResolver promise from later
installing orphaned refresh timers or invoking onRefresh after the key has been
overwritten; update the logic in the tokenCache set/update paths (the code that
constructs/assigns entries with tokenResolver, timeoutId, refreshTimeoutId and
that installs refresh timers and calls onRefresh) to attach a unique
marker/version id to each cache entry (e.g., entry.version or entry.resolverId)
when creating it and, before installing any timers or invoking onRefresh inside
tokenResolver.then/Promise continuations, verify the stored marker still matches
the current cache entry for that key—if it does not match, bail out and do not
set refreshTimeoutId or call onRefresh; apply the same guard wherever
tokenResolver continuations create timers (the other block that handles
refreshTimeoutId) so pending resolvers cannot schedule orphaned refreshes after
an overwrite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9af73399-f0a6-4ae4-810d-636541e51793
📒 Files selected for processing (4)
.changeset/fix-token-cache-timer-leak.mdintegration/tests/session-token-cache/refresh-timer-cleanup.test.tspackages/clerk-js/src/core/__tests__/tokenCache.test.tspackages/clerk-js/src/core/tokenCache.ts
| if (existing) { | ||
| if (existing.timeoutId !== undefined) { | ||
| clearTimeout(existing.timeoutId); | ||
| } | ||
| if (existing.refreshTimeoutId !== undefined) { | ||
| clearTimeout(existing.refreshTimeoutId); | ||
| } | ||
| } |
There was a problem hiding this comment.
NIT: I don't mind the verbosity, just noting that since clearTimeout(undefined) is a noop, this could be shortened for readability:
| if (existing) { | |
| if (existing.timeoutId !== undefined) { | |
| clearTimeout(existing.timeoutId); | |
| } | |
| if (existing.refreshTimeoutId !== undefined) { | |
| clearTimeout(existing.refreshTimeoutId); | |
| } | |
| } | |
| clearTimeout(existing?.timeoutId); | |
| clearTimeout(existing?.refreshTimeoutId); |
| entry.tokenResolver | ||
| .then(newToken => { | ||
| // If this entry was overwritten by a newer set() call while our promise | ||
| // was pending, bail out to avoid installing orphaned timers. | ||
| if (cache.get(key) !== value) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This makes sense and as long as tokenResolver doesn't have side effects (which it doesn't since that fetch has no client piggybacking), short circuiting here seems safe.
NIT: Moving
cache.set(key, value);to above
entry.tokenResolver
.then(newToken => {should be semantically the same, and could make this part a tiny bit clearer to read.
| const existing = cache.get(key); | ||
| if (existing) { | ||
| if (existing.timeoutId !== undefined) { | ||
| clearTimeout(existing.timeoutId); |
There was a problem hiding this comment.
If we only want to have one timer pending at any single time, would it make sense to move the timeoutId and refreshTimeoutId out into the parent scope and not saving them on the entries to have stricter guarantees?
| expect(result?.entry.tokenId).toBe('exp-overwrite'); | ||
| }); | ||
|
|
||
| it('does not accumulate refresh timers across multiple set() calls', async () => { |
There was a problem hiding this comment.
NIT: Is this test necessary? It seems implied by the cancels old refresh timer when set() is called again for the same key test.
| expect(onRefresh).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('simulates the hydration + background refresh double-set scenario', async () => { |
There was a problem hiding this comment.
Is this not the same test as cancels old refresh timer when set() is called again for the same key, with different names for the onRefresh callbacks?
I like that these names reflect a real scenario, so maybe merge, but use these names and point out it's one scenario that could happen?
| expect(backgroundRefresh).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('simulates multiple refresh cycles without timer accumulation', async () => { |
There was a problem hiding this comment.
This looks like cancels old refresh timer when set() is called again for the same key but 5 times in a row.
I think this is a keeper though since it might test for other scenarios of accumulation which is good.
| } | ||
| }); | ||
|
|
||
| it('set() with different key does not affect existing timers', async () => { |
There was a problem hiding this comment.
Hmm, what's the scenario when we should be refreshing multiple tokens in parallel in a single tab?
| for (let i = 0; i < 5; i++) { | ||
| await page.evaluate(async () => { | ||
| await (window as any).Clerk?.session?.touch(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
I think this is a multi-session app so it wont hit the 5s touch throttle? Might be worth a comment.
2 participants
Summary
SessionTokenCachethat caused token refresh requests to fire much earlier than expectedset()is called multiple times for the same cache key (from_updateClienthydration AND#refreshTokenInBackground), old timers were never cleared — each leaked timer independently firedonRefresh, accelerating the effective polling ratesetInternalbefore creating the new cache entrysession.touch()or any API response that triggers_updateClientTest plan
session.touch()Summary by CodeRabbit
Bug Fixes
Tests