Align Android session token fetching with edge minter behavior - #827
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds session-minter configuration and form parameters, centralizes token fetching, introduces freshness-aware cache reconciliation, and resets shared fetch state during Clerk and device-token cleanup. ChangesSession token flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Session
participant SessionTokenFetcher
participant SessionTokensCache
participant SessionApi
Session->>SessionTokenFetcher: fetchToken
SessionTokenFetcher->>SessionTokensCache: hydrate and read cache
SessionTokenFetcher->>SessionApi: request token with session-minter fields
SessionApi-->>SessionTokenFetcher: return token
SessionTokenFetcher->>SessionTokensCache: storeIfFresher
SessionTokenFetcher-->>Session: return canonical token
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
6342742 to
b824409
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
source/api/src/main/kotlin/com/clerk/api/network/api/SessionApi.kt (1)
81-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the three new parameters in the KDoc.
The KDoc above this method documents only
sessionId. Add@paramentries fororganizationId,token, andforceOrigin, and state thattokenandforceOriginapply only when session minting is enabled.🤖 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/network/api/SessionApi.kt` around lines 81 - 86, Update the KDoc for the tokens method to document organizationId, token, and forceOrigin with `@param` entries. State that token and forceOrigin apply only when session minting is enabled, while preserving the existing sessionId documentation.source/api/src/test/java/com/clerk/api/session/TokenFreshnessTest.kt (1)
24-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
TokenFreshness.matches.The tests cover
pickFreshestand the cache operations.matcheshas no test.matchesgates cache hydration from the session snapshot inSessionTokenFetcher.fetchToken, so it is on the auth path. Add cases for a matchingsidand organization, a differentsid, a different organization, and an undecodable token.As per coding guidelines: "Add regression tests whenever altering auth flows, attestation, or persistence logic".
🤖 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/TokenFreshnessTest.kt` around lines 24 - 118, Add dedicated tests for TokenFreshness.matches covering matching sid and organization, mismatched sid, mismatched organization, and an undecodable token. Reuse the existing token and TokenResource helpers, assert true only for the matching case, and assert false for each mismatch or decode failure.Source: Coding guidelines
source/api/src/main/kotlin/com/clerk/api/session/SessionTokenFetcher.kt (1)
284-291: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueCache entries for previous organizations are never removed.
The non-template key now embeds
lastActiveOrganizationId. Each organization switch creates a new key.SessionTokensCachehas no eviction, so the entry for the previous organization stays in the map for the process lifetime and holds a JWT for an organization the user has left.The growth is bounded by the number of organizations per session, so the memory cost is small. The retained JWT is the more relevant point. Consider removing the stale key when the active organization changes.
🤖 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/session/SessionTokenFetcher.kt` around lines 284 - 291, Update the organization-switch handling around SessionTokensCache to remove the previous non-template cache entry when lastActiveOrganizationId changes. Preserve template-specific entries and the current cache key format from tokenCacheKey, while ensuring the stale organization JWT is evicted before or as the new organization entry is stored.source/api/src/test/java/com/clerk/api/session/SessionTokenFetcherTest.kt (2)
413-417: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis stub hides the canonical-token defect.
The
storeIfFresherstub echoessecondArg<TokenResource>(), soStoreResult.canonicalTokenalways equals the response token. The assertions at lines 432-433 therefore pass whether or not the production code returns the canonical token.See the finding on
source/api/src/main/kotlin/com/clerk/api/session/SessionTokenFetcher.ktlines 193-197. To cover it, make the stub return a canonical token that differs from the incoming token for one of the two calls, then assert that the caller receives the canonical token.Also applies to: 432-433
🤖 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 413 - 417, Update the storeIfFresher stub in SessionTokenFetcherTest so at least one invocation returns a StoreResult whose canonicalToken differs from secondArg<TokenResource>(), using distinct responses for the two calls as needed. Strengthen the assertions around the existing canonical-token checks at lines 432-433 to verify that SessionTokenFetcher returns the stubbed canonical token rather than the response token.
193-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
SessionTokenFetcher.reset().
reset()is new.Clerk.reset()andConfigurationManager.clearDeviceToken()both call it, and it cancels every pending deduplicated fetch. No test covers it. Add a case that starts a deduplicated fetch, callsreset(), and asserts that the waiting caller observes cancellation and that a later fetch registers a new task.As per coding guidelines: "Add regression tests whenever altering auth flows, attestation, or persistence logic".
🤖 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 193 - 231, Add a regression test covering SessionTokenFetcher.reset(): start a deduplicated token fetch with a waiting caller, invoke reset(), and assert the waiter observes cancellation. Then perform another fetch and verify it registers and executes as a new task rather than reusing the canceled deduplicated request.Source: Coding guidelines
source/api/src/main/kotlin/com/clerk/api/session/TokenFreshness.kt (1)
90-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
checkNotNullwith a structure that keeps the null check local.At line 95 both values are non-null because of the preceding branches, but the compiler cannot prove it, so
checkNotNullguards a state that a future edit could break silently. Comparing the two nullable values once in a localvalpair removes the runtime check.val existingOrigin = existing.jwt.originIssuedAt() val incomingOrigin = incoming.jwt.originIssuedAt() return when { existingOrigin != null && incomingOrigin != null -> when { existingOrigin > incomingOrigin -> existing.resource incomingOrigin > existingOrigin -> incoming.resource else -> pickByIssuedAt(existing, incoming, tieBreaker) } existingOrigin != null -> existing.resource incomingOrigin != null -> incoming.resource else -> pickByIssuedAt(existing, incoming, tieBreaker) }🤖 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/session/TokenFreshness.kt` around lines 90 - 98, Update the selection logic in TokenFreshness to compare existingOriginIssuedAt and incomingOriginIssuedAt through a local non-null branch rather than checkNotNull. Keep both nullable values in locals, handle the case where both are present with the existing ordering and tie-breaker behavior, and retain the existing-resource, incoming-resource, and pickByIssuedAt outcomes for one or neither value being present.source/api/src/main/kotlin/com/clerk/api/session/SessionTokensCache.kt (1)
22-49: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winJWT decoding now runs inside the
ConcurrentHashMap.computeremapping function.
TokenFreshness.pickFreshestdecodes two JWTs on everyhydrateandstoreIfFreshercall.ConcurrentHashMap.computeapplies the remapping function while it holds the lock for the key's bin. Base64 decoding and JSON parsing therefore run under that lock. The javadoc forcomputerequires the remapping function to be short and simple.Contention is limited to threads that use the same cache key. That is exactly the concurrent forced-refresh case this PR adds, because forced refreshes now bypass deduplication and can reach
storeIfFresherfor one key at the same time.Consider caching the decoded claims on
TokenResource, or precomputing the comparison values before enteringcompute, so the locked section only compares primitives.🤖 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/session/SessionTokensCache.kt` around lines 22 - 49, Move JWT freshness decoding out of the ConcurrentHashMap.compute remapping functions used by hydrate and storeIfFresher. Precompute the incoming token’s comparison values, and obtain any existing token’s cached comparison data without parsing while the key lock is held; update TokenResource or TokenFreshness as needed so compute only performs primitive freshness comparison and canonical-token selection.
🤖 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/session/SessionTokenFetcher.kt`:
- Around line 193-197: The fetchToken success path must return the canonical
token selected by SessionTokensCache.storeIfFresher, not the raw response token;
update SessionTokenFetcher.kt lines 193-197 to return
StoreResult.canonicalToken. In SessionTokenFetcherTest.kt lines 413-417, make
the storeIfFresher stub return a StoreResult with a canonicalToken different
from the supplied TokenResource and assert fetchToken returns that canonical
token.
- Around line 101-103: Update the skipCache path in SessionTokenFetcher so
concurrent forced refreshes for the same cache key are bounded rather than each
issuing an uncoordinated request. Preserve independent refreshes when no
matching forced refresh is in flight, while coalescing or gating refreshes that
overlap until the existing in-flight request is released; ensure callers do not
receive tokens superseded by a newer refresh.
- Around line 125-133: Update getToken to create the FetchContext via
makeFetchContext before performing the pending-session check, then validate
context.session.status rather than the caller-provided session.status. Preserve
the existing null return for pending sessions and use the resolved context for
the subsequent token fetch.
- Around line 105-122: The shared-fetch flow in SessionTokenFetcher must
distinguish an absent in-flight task from a completed task whose result is null.
Refactor the tokenTasks lookup/putIfAbsent handling to retain the selected
Deferred locally, await it directly, and return its nullable result without
Elvis fallback; only the caller that successfully registers the Deferred should
invoke fetchToken and remove that same task in finally, while existing waiters
must only await it.
In `@source/api/src/main/kotlin/com/clerk/api/session/TokenFreshness.kt`:
- Around line 38-50: Update TokenFreshness.matches so it returns false when
decode(token.jwt) fails or the sid claim is absent; only compare session and
organization identifiers after both values are available, preserving the
existing organization matching behavior for decodable tokens.
In `@source/api/src/test/java/com/clerk/api/session/SessionTokenFetcherTest.kt`:
- Line 74: Update the tearDown method in SessionTokenFetcherTest to call
SessionTokensCache.clear() during cleanup, alongside the existing unmockkAll()
handling. Keep the existing setup cleanup unchanged and ensure the shared cache
is cleared after each test.
---
Nitpick comments:
In `@source/api/src/main/kotlin/com/clerk/api/network/api/SessionApi.kt`:
- Around line 81-86: Update the KDoc for the tokens method to document
organizationId, token, and forceOrigin with `@param` entries. State that token and
forceOrigin apply only when session minting is enabled, while preserving the
existing sessionId documentation.
In `@source/api/src/main/kotlin/com/clerk/api/session/SessionTokenFetcher.kt`:
- Around line 284-291: Update the organization-switch handling around
SessionTokensCache to remove the previous non-template cache entry when
lastActiveOrganizationId changes. Preserve template-specific entries and the
current cache key format from tokenCacheKey, while ensuring the stale
organization JWT is evicted before or as the new organization entry is stored.
In `@source/api/src/main/kotlin/com/clerk/api/session/SessionTokensCache.kt`:
- Around line 22-49: Move JWT freshness decoding out of the
ConcurrentHashMap.compute remapping functions used by hydrate and
storeIfFresher. Precompute the incoming token’s comparison values, and obtain
any existing token’s cached comparison data without parsing while the key lock
is held; update TokenResource or TokenFreshness as needed so compute only
performs primitive freshness comparison and canonical-token selection.
In `@source/api/src/main/kotlin/com/clerk/api/session/TokenFreshness.kt`:
- Around line 90-98: Update the selection logic in TokenFreshness to compare
existingOriginIssuedAt and incomingOriginIssuedAt through a local non-null
branch rather than checkNotNull. Keep both nullable values in locals, handle the
case where both are present with the existing ordering and tie-breaker behavior,
and retain the existing-resource, incoming-resource, and pickByIssuedAt outcomes
for one or neither value being present.
In `@source/api/src/test/java/com/clerk/api/session/SessionTokenFetcherTest.kt`:
- Around line 413-417: Update the storeIfFresher stub in SessionTokenFetcherTest
so at least one invocation returns a StoreResult whose canonicalToken differs
from secondArg<TokenResource>(), using distinct responses for the two calls as
needed. Strengthen the assertions around the existing canonical-token checks at
lines 432-433 to verify that SessionTokenFetcher returns the stubbed canonical
token rather than the response token.
- Around line 193-231: Add a regression test covering
SessionTokenFetcher.reset(): start a deduplicated token fetch with a waiting
caller, invoke reset(), and assert the waiter observes cancellation. Then
perform another fetch and verify it registers and executes as a new task rather
than reusing the canceled deduplicated request.
In `@source/api/src/test/java/com/clerk/api/session/TokenFreshnessTest.kt`:
- Around line 24-118: Add dedicated tests for TokenFreshness.matches covering
matching sid and organization, mismatched sid, mismatched organization, and an
undecodable token. Reuse the existing token and TokenResource helpers, assert
true only for the matching case, and assert false for each mismatch or decode
failure.
🪄 Autofix
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: 028e30cc-6afa-4c8e-b979-b23712fd771f
📒 Files selected for processing (11)
source/api/src/main/kotlin/com/clerk/api/Clerk.ktsource/api/src/main/kotlin/com/clerk/api/configuration/ConfigurationManager.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/Session.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/session/TokenFreshness.ktsource/api/src/test/java/com/clerk/api/network/model/environment/AuthConfigTest.ktsource/api/src/test/java/com/clerk/api/session/SessionTokenFetcherTest.ktsource/api/src/test/java/com/clerk/api/session/TokenFreshnessTest.kt
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/javascript(auto-detected)clerk/dashboard(auto-detected)
Prevent requests from a reset Clerk runtime from mutating or returning session tokens, and reject malformed session snapshots during cache hydration. Document forced-refresh return semantics with regression coverage.
Return nullable results from an existing token task without triggering retries, and resolve session status from the current client snapshot. Clear shared token state after fetcher tests to keep the suite isolated.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
source/api/src/main/kotlin/com/clerk/api/session/SessionTokenFetcher.kt (1)
185-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicated runtime check.
Line 185 and Line 186 both call
isCurrentRuntime(context), soruntimeLockis acquired twice for one decision. A single guard expresses the same fencing and reduces nesting.♻️ Proposed simplification
- validCachedToken?.takeIf { isCurrentRuntime(context) } - ?: if (!isCurrentRuntime(context)) { - null - } else { - try { - reconcileTokenResponse(context, requestToken(context, options)) - } catch (e: CancellationException) { - throw e - } catch (e: Exception) { - ClerkLog.e("Failed to fetch token: ${e.message}") - null - } - } + if (!isCurrentRuntime(context)) { + null + } else { + validCachedToken + ?: try { + reconcileTokenResponse(context, requestToken(context, options)) + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + ClerkLog.e("Failed to fetch token: ${e.message}") + null + } + }🤖 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/session/SessionTokenFetcher.kt` around lines 185 - 197, Update the token-fetching expression around validCachedToken and isCurrentRuntime(context) to evaluate the runtime check only once, using a single guard before selecting the cached-token or request/reconcile path. Preserve the existing CancellationException propagation, general error logging, and null fallback behavior.source/api/src/test/java/com/clerk/api/session/SessionTokenFetcherTest.kt (1)
174-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the skip-cache test so it proves the bypass.
The test stubs
SessionTokensCache.getToken(cacheKey)to return null, so no cached token exists to bypass. ThecoVerify { SessionTokensCache.getToken(cacheKey) }assertion now passes becauserequestTokenreads the cache to select the previous token, not because the cache check ran.Return a valid cached token from the stub and assert that the network token is returned instead.
💚 Proposed test change
val options = GetTokenOptions(skipCache = true) val cacheKey = "session_123-organization-" + val cachedToken = mockk<TokenResource>(relaxed = true) + val networkToken = mockk<TokenResource>(relaxed = true) - coEvery { SessionTokensCache.getToken(cacheKey) } returns null + every { cachedToken.jwt } returns "valid.jwt.token" + every { mockJWT.expiresAt } returns Date(System.currentTimeMillis() + 120000) + coEvery { SessionTokensCache.getToken(cacheKey) } returns cachedToken coEvery { mockClerkApiService.tokens("session_123") } returns - ClerkResult.success(mockTokenResource) - coEvery { SessionTokensCache.storeIfFresher(cacheKey, mockTokenResource, any()) } returns - SessionTokensCache.StoreResult(mockTokenResource, true) + ClerkResult.success(networkToken) + coEvery { SessionTokensCache.storeIfFresher(cacheKey, networkToken, any()) } returns + SessionTokensCache.StoreResult(networkToken, true) // When val result = sessionTokenFetcher.getToken(mockSession, options) // Then - assertEquals(mockTokenResource, result) - coVerify { SessionTokensCache.getToken(cacheKey) } + assertEquals(networkToken, result) coVerify { mockClerkApiService.tokens("session_123") } - coVerify { SessionTokensCache.storeIfFresher(cacheKey, mockTokenResource, any()) } + coVerify { SessionTokensCache.storeIfFresher(cacheKey, networkToken, any()) }🤖 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 174 - 193, Update the test `getToken bypasses cached result when skipCache is true` to stub `SessionTokensCache.getToken(cacheKey)` with a valid cached token, then assert the returned result is the network token from `mockClerkApiService.tokens("session_123")` rather than the cached token. Keep verification that the network request and cache update occur, so the test proves skipCache bypasses the cached result.
🤖 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/session/SessionTokenFetcher.kt`:
- Around line 57-67: Update SessionTokenFetcher.reset() to complete each
deferred in tokenTasks with null rather than canceling it, so awaiters return
normally with a null token after the runtime reset. Preserve runtimeGeneration
advancement and task removal, and add a regression test covering two concurrent
non-forced requests where reset() causes the waiting caller to receive null.
---
Nitpick comments:
In `@source/api/src/main/kotlin/com/clerk/api/session/SessionTokenFetcher.kt`:
- Around line 185-197: Update the token-fetching expression around
validCachedToken and isCurrentRuntime(context) to evaluate the runtime check
only once, using a single guard before selecting the cached-token or
request/reconcile path. Preserve the existing CancellationException propagation,
general error logging, and null fallback behavior.
In `@source/api/src/test/java/com/clerk/api/session/SessionTokenFetcherTest.kt`:
- Around line 174-193: Update the test `getToken bypasses cached result when
skipCache is true` to stub `SessionTokensCache.getToken(cacheKey)` with a valid
cached token, then assert the returned result is the network token from
`mockClerkApiService.tokens("session_123")` rather than the cached token. Keep
verification that the network request and cache update occur, so the test proves
skipCache bypasses the cached result.
🪄 Autofix
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: e78ad45d-6e58-4fad-8822-e2eec5600803
📒 Files selected for processing (4)
source/api/src/main/kotlin/com/clerk/api/session/SessionTokenFetcher.ktsource/api/src/main/kotlin/com/clerk/api/session/TokenFreshness.ktsource/api/src/test/java/com/clerk/api/session/SessionTokenFetcherTest.ktsource/api/src/test/java/com/clerk/api/session/TokenFreshnessTest.kt
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/javascript(auto-detected)clerk/dashboard(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (1)
- source/api/src/test/java/com/clerk/api/session/TokenFreshnessTest.kt
Complete shared token tasks with null when the runtime resets so callers are released normally while late owner responses remain fenced. Add regression coverage for both callers.
Summary
session_minterenvironment flag and send organization, previous-token, and force-origin parameters for default token requestsWhy
Android did not yet implement the session-minter request and freshness behavior used by clerk-js and the corresponding iOS implementation. A late stale response could overwrite a newer canonical session token, and forced refreshes were deduplicated even though each request must reach the token endpoint.
pr-827-token-diagnostics-actual-test-run-v2.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Tests