Add Session Minter support to the token fetch path - #835
Conversation
Native apps will feed their previous session token to Clerk's edge Session Minter, which mints the next token from it instead of reading the origin DB. For the edge to mint, the SDK has to send that previous token; today the tokens request is a bodyless POST that sends nothing the edge can use. The tokens request now attaches the previous session token as a token form field and maps skipCache to force_origin=true, both only when the environment's auth_config.session_minter flag is on, so the request is unchanged for instances that are not enrolled and for the templated route. The seed is the cached token for the session, falling back to the current client's lastActiveToken looked up by session id on a cold start, and is never sent empty. Because the previous token becomes the input to the next mint, a stale local token no longer self-corrects, and the pre-existing token cache had several races that only mattered once the token was a minting seed. The cache is made monotonic (an older oiat, or an unreadable JWT, never overwrites a newer entry, matching clerk-js pickFreshestJwt). Concurrent getToken calls dedupe through a shared task map, and a caller only joins an in-flight task started under the current invalidation generation, so a request begun before a sign-out or org switch can neither be joined nor write its result afterward. skipCache callers get their own task key and bypass the failure backoff, which is otherwise bounded and cleared on invalidation, all under the cache write lock so a stale outcome cannot open a window against post-switch state. A configuration epoch captured at admission and rechecked before caching discards a token minted under a configuration that reset or switchConfiguration has since replaced. The seed is kept out of debug body logs via a sensitive-request tag.
📝 WalkthroughWalkthroughSession token handling now supports session-minter requests, JWT freshness selection, generation-fenced caching, per-key exponential backoff, coroutine request deduplication, configuration-reset fencing, and cache cleanup during session changes and sign-out. ChangesSession token lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SessionTokenFetcher
participant SessionApi
participant SessionTokensCache
SessionTokenFetcher->>SessionTokensCache: read cached token and generation
SessionTokenFetcher->>SessionApi: request tokens or mintTokens
SessionApi-->>SessionTokenFetcher: return TokenResource
SessionTokenFetcher->>SessionTokensCache: conditionally cache token and record outcome
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| /** Returns the current invalidation counter for [sessionId]. */ | ||
| internal fun generation(sessionId: String): Generation = | ||
| Generation(global = globalGeneration.get(), session = sessionGenerations[sessionId] ?: 0L) |
There was a problem hiding this comment.
🟠 High session/SessionTokensCache.kt:20
generation() reads globalGeneration and sessionGenerations outside writeLock, so a concurrent token request can capture the new generation while the old token is still in the cache — before clear() has finished removing it. That response then passes the setTokenIfCurrent check and repopulates the token that sign-out or clear() intended to drop. Reading both counters inside synchronized(writeLock) closes the window.
| /** Returns the current invalidation counter for [sessionId]. */ | |
| internal fun generation(sessionId: String): Generation = | |
| Generation(global = globalGeneration.get(), session = sessionGenerations[sessionId] ?: 0L) | |
| /** Returns the current invalidation counter for [sessionId]. */ | |
| internal fun generation(sessionId: String): Generation = synchronized(writeLock) { | |
| Generation(global = globalGeneration.get(), session = sessionGenerations[sessionId] ?: 0L) | |
| } |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @source/api/src/main/kotlin/com/clerk/api/session/SessionTokensCache.kt around lines 20-22:
`generation()` reads `globalGeneration` and `sessionGenerations` outside `writeLock`, so a concurrent token request can capture the new generation while the old token is still in the cache — before `clear()` has finished removing it. That response then passes the `setTokenIfCurrent` check and repopulates the token that sign-out or `clear()` intended to drop. Reading both counters inside `synchronized(writeLock)` closes the window.
| ClerkLog.w("Discarding token for $cacheKey: the configuration was replaced mid-request") | ||
| null | ||
| } else { | ||
| when (tokensRequest) { |
There was a problem hiding this comment.
🟠 High session/SessionTokenFetcher.kt:292
requestToken returns tokensRequest.value after calling SessionTokensCache.setTokenIfCurrent, but setTokenIfCurrent silently discards the token when the session generation changed while the request was in flight — the method still returns the discarded token to its caller. So a token request racing sign-out or an organization switch still hands its original caller a token minted for the revoked or previous organization state, even though the cache write is correctly fenced. Recheck the generation before returning, or make setTokenIfCurrent report whether it accepted the token, and return null for stale responses.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @source/api/src/main/kotlin/com/clerk/api/session/SessionTokenFetcher.kt around line 292:
`requestToken` returns `tokensRequest.value` after calling `SessionTokensCache.setTokenIfCurrent`, but `setTokenIfCurrent` silently discards the token when the session generation changed while the request was in flight — the method still returns the discarded token to its caller. So a token request racing sign-out or an organization switch still hands its original caller a token minted for the revoked or previous organization state, even though the cache write is correctly fenced. Recheck the generation before returning, or make `setTokenIfCurrent` report whether it accepted the token, and return `null` for stale responses.
There was a problem hiding this comment.
🟠 High
A stale token from the old configuration can survive reset() and remain usable after re-initialization. SessionTokenFetcher.resetForNewConfiguration() bumps the configuration epoch only after SessionTokensCache.clear() and ClerkApi.reset() have already run, so a concurrent getToken admitted after the cache clears but before the epoch bump captures the new cache generation with the old epoch. If that request reaches the old Retrofit service and completes before the bump, setTokenIfCurrent accepts its token (the generation still matches), and the later epoch bump never clears it. The stale token then persists into the next configuration. Move the resetForNewConfiguration() call before SessionTokensCache.clear() and ClerkApi.reset(), so any request that could still reach the old service captured the old epoch and is discarded.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @source/api/src/main/kotlin/com/clerk/api/Clerk.kt around line 763:
A stale token from the old configuration can survive `reset()` and remain usable after re-initialization. `SessionTokenFetcher.resetForNewConfiguration()` bumps the configuration epoch only after `SessionTokensCache.clear()` and `ClerkApi.reset()` have already run, so a concurrent `getToken` admitted after the cache clears but before the epoch bump captures the new cache generation with the old epoch. If that request reaches the old Retrofit service and completes before the bump, `setTokenIfCurrent` accepts its token (the generation still matches), and the later epoch bump never clears it. The stale token then persists into the next configuration. Move the `resetForNewConfiguration()` call before `SessionTokensCache.clear()` and `ClerkApi.reset()`, so any request that could still reach the old service captured the old epoch and is discarded.
| generation: SessionTokensCache.Generation, | ||
| epoch: Long, | ||
| ): Deferred<TokenResource?> { | ||
| val started = |
There was a problem hiding this comment.
🟡 Medium session/SessionTokenFetcher.kt:206
sharedTask can start a duplicate token request when two callers race. A lazy Deferred is not isActive until start() runs, and start() is called after the task is published into tokenTasks at line 212. A concurrent caller entering compute in that interval sees isActive == false, replaces the task, and starts a second POST, defeating deduplication. Call start() before publishing the task into the map, or treat a non-completed, non-cancelled deferred as joinable.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @source/api/src/main/kotlin/com/clerk/api/session/SessionTokenFetcher.kt around line 206:
`sharedTask` can start a duplicate token request when two callers race. A lazy `Deferred` is not `isActive` until `start()` runs, and `start()` is called *after* the task is published into `tokenTasks` at line 212. A concurrent caller entering `compute` in that interval sees `isActive == false`, replaces the task, and starts a second POST, defeating deduplication. Call `start()` before publishing the task into the map, or treat a non-completed, non-cancelled deferred as joinable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
source/api/src/test/java/com/clerk/api/session/SessionTokenFetcherTest.kt (1)
738-744: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
returnsManyhere is pinned to the exact number ofgeneration()calls the implementation makes.The comment documents four specific call sites in order. Adding or removing a single
SessionTokensCache.generation(...)read in the fetcher silently shifts the sequence and this test starts asserting something else entirely. Consider driving the switch from a mutable holder (answers { currentGeneration }) flipped at the point the scenario needs it, so intent survives refactoring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/api/src/test/java/com/clerk/api/session/SessionTokenFetcherTest.kt` around lines 738 - 744, Update the generation stubbing in the test around SessionTokensCache.generation to use a mutable current-generation holder with an answers-based response, and flip that holder at the scenario’s switch point. Remove the exact-call-count returnsMany sequence while preserving the intended pre-switch and post-switch join behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/api/src/main/kotlin/com/clerk/api/auth/Auth.kt`:
- Around line 643-645: Update the session-removal flow around
removeSessionLocally and refreshClientAfterSessionMutation to pass the removed
sessionId as the dropped-token session identifier. Add a regression case
covering a refresh that reintroduces the removed session with a token, and
verify its persisted lastActiveToken is cleared.
In `@source/api/src/test/java/com/clerk/api/session/SessionTokenFetcherTest.kt`:
- Around line 102-107: Update tearDown() to call
SessionTokenFetcher.resetForNewConfiguration() so companion-level tokenTasks,
configurationEpoch, and sharedFetchScope state is reset between tests. Keep the
existing clock reset, scope cancellation, and mock cleanup intact.
---
Nitpick comments:
In `@source/api/src/test/java/com/clerk/api/session/SessionTokenFetcherTest.kt`:
- Around line 738-744: Update the generation stubbing in the test around
SessionTokensCache.generation to use a mutable current-generation holder with an
answers-based response, and flip that holder at the scenario’s switch point.
Remove the exact-call-count returnsMany sequence while preserving the intended
pre-switch and post-switch join behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 705699ef-b175-41d9-a12e-177c339243fd
📒 Files selected for processing (17)
source/api/src/main/kotlin/com/clerk/api/Clerk.ktsource/api/src/main/kotlin/com/clerk/api/Constants.ktsource/api/src/main/kotlin/com/clerk/api/auth/Auth.ktsource/api/src/main/kotlin/com/clerk/api/network/api/SessionApi.ktsource/api/src/main/kotlin/com/clerk/api/network/model/environment/AuthConfig.ktsource/api/src/main/kotlin/com/clerk/api/session/JwtFreshness.ktsource/api/src/main/kotlin/com/clerk/api/session/SessionTokenBackoff.ktsource/api/src/main/kotlin/com/clerk/api/session/SessionTokenFetcher.ktsource/api/src/main/kotlin/com/clerk/api/session/SessionTokensCache.ktsource/api/src/main/kotlin/com/clerk/api/signout/SignOutService.ktsource/api/src/test/java/com/clerk/api/auth/AuthTest.ktsource/api/src/test/java/com/clerk/api/network/api/SessionApiTest.ktsource/api/src/test/java/com/clerk/api/network/model/environment/AuthConfigSerializationTest.ktsource/api/src/test/java/com/clerk/api/session/SessionTokenBackoffTest.ktsource/api/src/test/java/com/clerk/api/session/SessionTokenFetcherTest.ktsource/api/src/test/java/com/clerk/api/session/SessionTokensCacheTest.ktsource/api/src/test/java/com/clerk/api/signout/SignOutServiceTest.kt
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/javascript(auto-detected) → reviewed against open PR#9284nikos/native-minter-hardeninginstead of the default branch
| SessionTokensCache.removeTokensForSession(sessionId) | ||
| removeSessionLocally(sessionId) | ||
| refreshClientAfterSessionMutation() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Clear the removed session’s persisted token during the refresh.
refreshClientAfterSessionMutation() can install a stale Client.get() response after the successful removal. Because no droppedTokenSessionId is provided here, a reintroduced removed session can retain lastActiveToken and seed a later mint request. Pass sessionId and add a regression case where the refresh returns the removed session with a token.
Proposed fix
- refreshClientAfterSessionMutation()
+ refreshClientAfterSessionMutation(droppedTokenSessionId = sessionId)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SessionTokensCache.removeTokensForSession(sessionId) | |
| removeSessionLocally(sessionId) | |
| refreshClientAfterSessionMutation() | |
| SessionTokensCache.removeTokensForSession(sessionId) | |
| removeSessionLocally(sessionId) | |
| refreshClientAfterSessionMutation(droppedTokenSessionId = sessionId) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/api/src/main/kotlin/com/clerk/api/auth/Auth.kt` around lines 643 -
645, Update the session-removal flow around removeSessionLocally and
refreshClientAfterSessionMutation to pass the removed sessionId as the
dropped-token session identifier. Add a regression case covering a refresh that
reintroduces the removed session with a token, and verify its persisted
lastActiveToken is cleared.
| @After | ||
| fun tearDown() { | ||
| clockMillis = 0L | ||
| fetchScope.cancel() | ||
| unmockkAll() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
tearDown leaves SessionTokenFetcher's companion state dirty between tests.
tokenTasks, configurationEpoch, and sharedFetchScope are all companion-level and survive across tests in the JVM. Several tests deliberately abandon in-flight callers (Lines 995, 1073) or bump the global epoch (Lines 1011, 1025, 1046), and nearly every test reuses the cache key "session_123". A leftover entry plus a relaxed-mock generation() is enough for a later test to join a previous test's task, which makes the suite order-dependent. Calling resetForNewConfiguration() in teardown clears the map and installs a fresh scope.
🧪 Proposed teardown fix
`@After`
fun tearDown() {
clockMillis = 0L
fetchScope.cancel()
+ // Clears the companion-level dedupe map and installs a fresh shared scope so leftover
+ // in-flight tasks cannot be joined by the next test.
+ SessionTokenFetcher.resetForNewConfiguration()
unmockkAll()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @After | |
| fun tearDown() { | |
| clockMillis = 0L | |
| fetchScope.cancel() | |
| unmockkAll() | |
| } | |
| `@After` | |
| fun tearDown() { | |
| clockMillis = 0L | |
| fetchScope.cancel() | |
| // Clears the companion-level dedupe map and installs a fresh shared scope so leftover | |
| // in-flight tasks cannot be joined by the next test. | |
| SessionTokenFetcher.resetForNewConfiguration() | |
| unmockkAll() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/api/src/test/java/com/clerk/api/session/SessionTokenFetcherTest.kt`
around lines 102 - 107, Update tearDown() to call
SessionTokenFetcher.resetForNewConfiguration() so companion-level tokenTasks,
configurationEpoch, and sharedFetchScope state is reset between tests. Keep the
existing clock reset, scope cancellation, and mock cleanup intact.
1 participant
Why
Native apps will feed their previous session token to Clerk's edge Session Minter, which mints the next token from it instead of reading the origin database. For the edge to mint, the SDK has to send that previous token, and today the tokens request is a bodyless POST that sends nothing the edge can use. This is the Android side of adding native Session Minter support.
What changed
The tokens request attaches the previous session token as a token form field and maps skipCache to force_origin=true, both only when the environment's auth_config.session_minter flag is on, so the request is unchanged for instances that are not enrolled and for the templated route. The seed is the cached token for the session, falling back to the current client's lastActiveToken looked up by session id on a cold start, and is never sent empty.
Because the previous token becomes the input to the next mint, a stale local token no longer self-corrects, and the existing token cache had several concurrency gaps that only mattered once the token was a minting seed. The cache is made monotonic (an older oiat, or an unreadable JWT, never overwrites a newer entry, matching clerk-js pickFreshestJwt). Concurrent getToken calls dedupe through a shared task map, and a caller only joins a task started under the current invalidation generation, so a request begun before a sign-out or org switch can neither be joined nor write its result after. skipCache callers get their own task key and bypass the failure backoff, which is otherwise bounded and cleared on invalidation under the cache write lock. A configuration epoch captured at admission and rechecked before caching discards a token minted under a configuration that reset has since replaced. The seed is kept out of debug body logs.
Related
Part of a four-PR set adding native Session Minter support: the edge worker is clerk/cloudflare-workers#2509, the web/Expo SDK is clerk/javascript#9284, and iOS is clerk/clerk-ios#535.
No JVM or Android toolchain was available in the authoring environment, so this could not be compiled or run locally; it was verified statically (including against the vendored kotlinx-coroutines and JWTDecode sources) and needs a green CI run before merge.
Note
Add Session Minter support to the token fetch path with deduplication, backoff, and generation fencing
mintTokensAPI endpoint and asessionMinterflag inAuthConfig; when enabled, non-templated token fetches call the minter path with an optional previous-token seed andforce_originforskipCacherequests.SessionTokenBackofffor per-cache-key exponential backoff (5s–60s) after repeated fetch failures; forced requests bypass backoff but still influence it.SessionTokensCacheso tokens fetched for invalidated sessions or replaced configurations are discarded rather than cached.SessionTokenFetcherinstances using a sharedConcurrentHashMap; forced fetches use a separate key to avoid joining plain in-flight requests.setActivesession switches, and nullifieslastActiveTokenon the in-memory client for the affected session.📊 Macroscope summarized b863b20. 10 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.
Summary by CodeRabbit