fix: converge MRRT cache to single RT on concurrent refresh#1702
fix: converge MRRT cache to single RT on concurrent refresh#1702yogeshchoudhary147 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughMRRT refresh-token rotation now scopes cache updates by client ID and passes an MRRT flag to enable replacement across applicable entries. Tests cover MRRT propagation, audience isolation, evicted entries, and the updated method signature. ChangesMRRT refresh-token rotation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
__tests__/Auth0Client/onlineAccess.test.ts (1)
320-321: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the enabled-MRRT forwarding path.
This assertion only verifies
useMrrt === undefined. Add an integration case configured withuseMrrt: trueand assert thatupdateEntryreceivestrue; otherwise a regression inAuth0Clientforwarding could remain undetected.🤖 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 `@__tests__/Auth0Client/onlineAccess.test.ts` around lines 320 - 321, Add an integration test in the online access flow using configuration with useMrrt: true, then assert updateEntry receives true for that argument. Keep the existing undefined-case coverage and use the same Auth0Client test setup and updateEntry assertion path to verify enabled-MRRT forwarding.
🤖 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 `@src/cache/cache-manager.ts`:
- Around line 358-360: Restrict the unconditional MRRT update in the cache
update flow to entries belonging to the current clientId. Before applying the
useMrrt=true update to keys returned by getCacheKeys() or cache.allKeys(),
filter out keys associated with other client IDs while preserving updates for
the current client.
- Around line 355-360: Serialize the complete MRRT backfill loop, including all
per-key reads and writes, so concurrent getTokenSilently flows cannot interleave
updates. Add or reuse a shared mutex around the loop that performs the useMrrt
backfill in the cache manager, or replace it with an atomic batch update, while
preserving convergence to one shared rotated refresh token.
---
Nitpick comments:
In `@__tests__/Auth0Client/onlineAccess.test.ts`:
- Around line 320-321: Add an integration test in the online access flow using
configuration with useMrrt: true, then assert updateEntry receives true for that
argument. Keep the existing undefined-case coverage and use the same Auth0Client
test setup and updateEntry assertion path to verify enabled-MRRT forwarding.
🪄 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: 0dcc4f42-63e8-4097-9c66-e62105fd6147
📒 Files selected for processing (4)
__tests__/Auth0Client/onlineAccess.test.ts__tests__/cache/cache-manager.test.tssrc/Auth0Client.tssrc/cache/cache-manager.ts
2eb8302 to
f15b2a8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/cache/cache-manager.test.ts (1)
1008-1031: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftTest concurrent convergence, not only single-call overwrite.
This test proves MRRT replacement across audiences, but it cannot catch interleaving two refreshes across multiple keys. Add a deterministic fake-cache/barrier test that runs two MRRT updates concurrently and asserts all entries for
TEST_CLIENT_IDconverge to one token. The PR objective explicitly targets convergence during concurrentgetTokenSilently()calls.🤖 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 `@__tests__/cache/cache-manager.test.ts` around lines 1008 - 1031, Extend the cache-manager tests around updateEntry to add a deterministic fake-cache/barrier scenario that starts two concurrent MRRT updates for TEST_CLIENT_ID across multiple audience entries. Coordinate the interleaving explicitly, then assert every matching entry converges to a single refresh token, covering concurrent getTokenSilently-style updates rather than only sequential overwrite 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.
Nitpick comments:
In `@__tests__/cache/cache-manager.test.ts`:
- Around line 1008-1031: Extend the cache-manager tests around updateEntry to
add a deterministic fake-cache/barrier scenario that starts two concurrent MRRT
updates for TEST_CLIENT_ID across multiple audience entries. Coordinate the
interleaving explicitly, then assert every matching entry converges to a single
refresh token, covering concurrent getTokenSilently-style updates rather than
only sequential overwrite behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cb2ae7b9-d266-47e5-b6c2-2afa0cf3f2c3
📒 Files selected for processing (4)
__tests__/Auth0Client/onlineAccess.test.ts__tests__/cache/cache-manager.test.tssrc/Auth0Client.tssrc/cache/cache-manager.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Auth0Client/onlineAccess.test.ts
- src/Auth0Client.ts
f15b2a8 to
b8e173b
Compare
Problem
When MRRT (multi-resource refresh tokens) is enabled, concurrent
getTokenSilently()calls for different audiences can leave the cache in a diverged state.Steps to reproduce:
Auth0ClientwithuseRefreshTokens: trueand multiple resource audiences.getTokenSilently()calls for different audiences (e.g.api-1andapi-2) before either resolves./oauth/token.updateEntry(RT0, RT1-a)propagates RT1-a to all MRRT entries, including api-2.cacheManager.setwrites api-1's own entry with RT1-a.updateEntry(RT0, RT1-b)finds no entry with RT0 (already rotated to RT1-a), so it is a no-op. RT1-b is never propagated to api-1.cacheManager.setwrites api-2's own entry with RT1-b.Root cause: Each call does two writes —
updateEntrypropagates the new RT to other MRRT entries, andcacheManager.setwrites the caller's own entry. With concurrent calls, whichever call finishes second has itsupdateEntrysilently no-op (the old RT is already gone), so the new RT never propagates to the other audience entries. Both calls end up writing different RTs to their own entries leaving the cache in a diverged state.Fix
useMrrtparameter toCacheManager.updateEntry. Whentrue, instead of matchingoldRefreshToken, it unconditionally overwrites every entry that holds a RT. This way the last concurrent call to complete always wins and the client converges to a single shared RT.clientIdparameter toCacheManager.updateEntry. Without this, the unconditional overwrite would also update RT-bearing entries for otherAuth0Clientinstances sharing the same storage (e.g.localStorage). The loop now skips any key whose parsedclientIddoes not match the caller's.this.options.clientIdandthis.options.useMrrtat the call site inAuth0Client.Test plan
npx jest __tests__/cache/cache-manager.test.ts- new tests verify (1)useMrrt=trueoverwrites a forked RT unconditionally, (2) entries belonging to a differentclientIdare left untouched, and (3) entries evicted between key listing and fetching are skipped gracefullynpx jest __tests__/Auth0Client/onlineAccess.test.ts- existing spy assertion updated to reflect the new argumentsnpm test- full unit suite green