Repository navigation
Conversation
getAccessToken only refreshed once the token had expired, so every caller that asked for a token on the same shared event (event-site asks on every real time push, in every open tab) refreshed on the same second, which showed up as synchronized spikes on POST /oauth2/token. A still valid token inside its last ACCESS_TOKEN_REFRESH_AHEAD_SECS (capped to a quarter of its lifetime) is now returned right away and refreshed once per tab after a random delay in [0, ACCESS_TOKEN_REFRESH_SPREAD_MS), under the same cross tab lock. The refresh is skipped if another tab already stored a newer token, makes a single attempt, and on a rejected refresh token restores the session clearing flag so the hard expiry path keeps handling the logout. Expired tokens are still refreshed synchronously.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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.
🟡 Changes recommended
A queued background refresh can bypass a subsequently registered access-token resolver.
1 open finding
What changed in this PR
Adds proactive, randomized access-token refresh to reduce IDP request spikes.
Changes:
- Schedules near-expiry refreshes under a cross-tab lock.
- Preserves synchronous expiry handling and resolver precedence.
- Adds comprehensive background-refresh tests.
| File | Description |
|---|---|
src/components/security/methods.js |
Implements locked background token refresh. |
src/components/security/__tests__/methods.test.js |
Tests refresh timing, failures, and edge cases. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const _backgroundRefresh = async () => { | ||
| const authInfo = getAuthInfo(); | ||
| if (!authInfo || !authInfo.accessToken || !authInfo.refreshToken) return; |
There was a problem hiding this comment.
Fixed in 50f4584. _backgroundRefresh checks the shared resolver slot under the lock and returns without touching authInfo or calling the IDP when a resolver has been registered after the refresh was scheduled. I kept setAccessTokenResolver unchanged since the check covers the pending timer. Covered by "skips a pending background refresh once an access token resolver is registered".
- Store the refreshed token only if the stored session is still the one the background refresh started from (same refresh token and issue time), so a logout or a new login during the request is not undone. - Stop scheduling background refreshes for a refresh token the IDP rejected; the hard expiry path handles the logout. Transient errors stay retryable. - Skip a pending background refresh once an access token resolver is registered.
ref: https://app.clickup.com/t/86bcfte4c
getAccessToken only refreshed the access token once it had expired. event-site calls it from withRealTimeUpdates processUpdates on every real time push, in every open tab, so every client whose token had expired refreshed on the same second. On 2026-10-08 that showed up on the IDP as 1-2 second spikes of up to 68 POST /oauth2/token requests right after summit-api published schedule changes, even with the access token lifetime jitter already in place.
Changes
_getAccessToken: a still valid token inside its lastACCESS_TOKEN_REFRESH_AHEAD_SECS(300s, capped to a quarter of the token lifetime) is returned right away and a background refresh is scheduled once per tab after a random delay in[0, ACCESS_TOKEN_REFRESH_SPREAD_MS)(30s). Expired tokens are still refreshed synchronously, as before.getAccessToken(extracted intowithAccessTokenLock), re-readsauthInfoand skips the call if another tab already stored a newer token.processRefreshTokentakes awithRetryflag): retrying with backoff would hold the lock for every tab while the current token is still valid. Transient errors are only logged.refreshAccessTokensetsclearing_session_state, which makesinitLoginskip the re-login. Nobody handles that error in the background, so the previous flag value is restored and the logout stays with the hard expiry path, which surfaces the error to the caller.accessTokenUpdatedAt), so a logout (LOGOUT_USER) or a new login (onUserAuth) during the request is not undone.setAccessTokenResolverkeeps precedence: when a resolver is registeredgetAccessTokendelegates to it and none of this runs. A background refresh scheduled before a resolver is registered is skipped.Testing
getAccessToken background refreshtests inmethods.test.js(fake timers): outside the window, inside the window with the random delay, single refresh per tab, another tab already refreshed, hard expiry, transient failure without retry, rejected refresh token, short lifetime cap, no refresh token, implicit flow, rejected refresh token not retried, transient failure retried, logout and new login during the request, resolver registered after scheduling. 5 of them fail against the previousmethods.js, and each review fix has a test that fails when that fix is removed.yarn jest: 18 suites, 203 tests pass.yarn buildOK on Node 18.15.0 (.nvmrc; node-sass does not build on Node 22).