fix: deduplicate session restore race conditions on page reload#350
Merged
Conversation
- Staging vault login path performance baseline (3 scenarios): fresh vault, existing vault, session restore - Playwright + mock wallet perf script for reproducible measurement - Todo: deduplicate concurrent auth/refresh calls on page reload (6 simultaneous requests observed, 2 fail with 401) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: ba7fe295aa18
Renamed from auth-refresh-only to cover the full cascade: Layer 1 (root cause): auth/refresh token deduplication Layer 2 (downstream): vault init deduplication Both cause E2E test 3.7 to fail consistently on CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 12fa1b90966b
Two cascading race conditions broke session restore after page reload: Layer 1: Multiple React effects called authApi.refresh() concurrently. The first response rotated the refresh token, causing later concurrent calls to fail with 401. Fix: shared in-flight Promise in authApi.refresh(). Layer 2: Multiple successful refreshes led to concurrent initializeOrLoadVault() calls. Both saw getVault() → 404 and attempted vault init, hitting a duplicate key constraint on folder_ipns. Fix: module-level shared Promise in initializeOrLoadVault(). Both use the same deduplication pattern as PublishCoordinator. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 558aadead671
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
WalkthroughAdds shared-Promise deduplication for concurrent Changes
Sequence DiagramsequenceDiagram
participant Client
participant authApi
participant vaultHook
participant AuthSvr as Auth Server
participant VaultSvr as Vault Server
par Concurrent Refresh Calls
Client->>authApi: refresh() call 1
authApi->>authApi: check refreshPromise (null) / create & cache
authApi->>AuthSvr: POST /auth/refresh
Client->>authApi: refresh() call 2
authApi-->>Client: return cached promise
AuthSvr-->>authApi: token response
authApi->>authApi: clear refreshPromise = null
authApi-->>Client: resolve callers
end
par Concurrent Vault Init Calls
vaultHook->>vaultHook: initializeOrLoadVault() call 1
vaultHook->>vaultHook: check vaultInitPromise (null) / create & cache doInit()
vaultHook->>VaultSvr: getVault()
vaultHook->>vaultHook: initializeOrLoadVault() call 2
vaultHook-->>vaultHook: return cached promise
VaultSvr-->>vaultHook: vault response (data or 404)
vaultHook->>vaultHook: clear vaultInitPromise = null
vaultHook-->>callers: resolve callers
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: bd822024bd22
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 193448a5cdbe
![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
🧹 Nitpick comments (3)
tests/web-e2e/staging-perf-wallet.mjs (1)
45-56: Theasyncon response handlers is unused.The callback is marked
asyncbut neverawaits anything. This is harmless but slightly misleading.Remove unnecessary async
- page.on('response', async (resp) => { + page.on('response', (resp) => {Same applies to line 160.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/web-e2e/staging-perf-wallet.mjs` around lines 45 - 56, The response handler registered with page.on('response', async (resp) => { ... }) marks the callback as async but never awaits anything; remove the unnecessary async modifier from this handler (and the other similar response handler around line 160) so the callbacks are plain synchronous functions, keeping the logic that uses pendingRequests, pending.start, t0, and apiCalls unchanged..planning/todos/done/2026-03-24-session-restore-race-conditions.md (1)
52-64: Fix 1 file location is slightly inaccurate.The header says
api-config.tsbut the actual implementation is inapps/web/src/lib/api/auth.ts. This is a minor documentation inconsistency.Update header to match actual implementation
-### Fix 1: Auth/refresh deduplication (`api-config.ts`) +### Fix 1: Auth/refresh deduplication (`auth.ts`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/todos/done/2026-03-24-session-restore-race-conditions.md around lines 52 - 64, Update the documentation header to point to the actual implementation file: change the header from `api-config.ts` to `apps/web/src/lib/api/auth.ts` so it correctly references where refreshAccessToken, refreshPromise and doRefresh are defined; ensure the note or section title matches the file path and optionally mention the function names (refreshAccessToken, refreshPromise, doRefresh) to make locating the implementation unambiguous..planning/perf/staging-baseline-2026-03-24.md (1)
128-134: Re-measurement checklist item 5 should be updated post-merge.Line 134 states "Check auth/refresh race is still present (pre-existing, tracked separately)" — but this PR implements the fix. After merge, this item should confirm the race is resolved (single refresh call).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/perf/staging-baseline-2026-03-24.md around lines 128 - 134, Update the re-measurement checklist item that currently reads "Check auth/refresh race is still present (pre-existing, tracked separately)" to reflect the PR's fix: change it to confirm the race is resolved and that only a single refresh call occurs (e.g., "Confirm auth/refresh race resolved — single refresh call observed"); edit the checklist entry text in the file where the item string appears so post-merge validation checks for the resolved behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/web-e2e/staging-perf-wallet.mjs`:
- Around line 10-22: This file has Prettier formatting violations around the
mock account/transport block (symbols: privateKeyToAccount, account,
localTransport, custom, and the async request handler) and elsewhere; run the
formatter to fix spacing/linebreaks and ensure the switch/case and object
literals are properly formatted—execute `pnpm prettier --write
tests/web-e2e/staging-perf-wallet.mjs` (or apply the same Prettier rules) and
commit the updated file so the linter/CI passes.
---
Nitpick comments:
In @.planning/perf/staging-baseline-2026-03-24.md:
- Around line 128-134: Update the re-measurement checklist item that currently
reads "Check auth/refresh race is still present (pre-existing, tracked
separately)" to reflect the PR's fix: change it to confirm the race is resolved
and that only a single refresh call occurs (e.g., "Confirm auth/refresh race
resolved — single refresh call observed"); edit the checklist entry text in the
file where the item string appears so post-merge validation checks for the
resolved behavior.
In @.planning/todos/done/2026-03-24-session-restore-race-conditions.md:
- Around line 52-64: Update the documentation header to point to the actual
implementation file: change the header from `api-config.ts` to
`apps/web/src/lib/api/auth.ts` so it correctly references where
refreshAccessToken, refreshPromise and doRefresh are defined; ensure the note or
section title matches the file path and optionally mention the function names
(refreshAccessToken, refreshPromise, doRefresh) to make locating the
implementation unambiguous.
In `@tests/web-e2e/staging-perf-wallet.mjs`:
- Around line 45-56: The response handler registered with page.on('response',
async (resp) => { ... }) marks the callback as async but never awaits anything;
remove the unnecessary async modifier from this handler (and the other similar
response handler around line 160) so the callbacks are plain synchronous
functions, keeping the logic that uses pendingRequests, pending.start, t0, and
apiCalls unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ffb7e07f-949c-47c2-bad6-4cfadc9cf623
📒 Files selected for processing (5)
.planning/perf/staging-baseline-2026-03-24.md.planning/todos/done/2026-03-24-session-restore-race-conditions.mdapps/web/src/hooks/useAuth.tsapps/web/src/lib/api/auth.tstests/web-e2e/staging-perf-wallet.mjs
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses session-restore failures on page reload in the web app by deduplicating concurrent token refresh calls and concurrent vault initialization attempts, and adds a staging performance baseline script to measure login + session restore behavior.
Changes:
- Deduplicate
POST /auth/refreshcalls in the web auth API wrapper via a shared in-flight Promise. - Deduplicate
initializeOrLoadVault()calls via a module-level in-flight Promise to prevent double vault initialization on reload. - Add a Playwright-based staging performance baseline script and accompanying planning/baseline docs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/web-e2e/staging-perf-wallet.mjs | Adds a standalone Playwright script to baseline staging wallet login + reload/session-restore timing and API call waterfall. |
| apps/web/src/lib/api/auth.ts | Adds shared in-flight Promise to collapse concurrent refresh callers into a single request. |
| apps/web/src/hooks/useAuth.ts | Adds module-level shared in-flight Promise to dedupe vault init/load and avoid double-init DB constraint errors. |
| .planning/todos/done/2026-03-24-session-restore-race-conditions.md | Documents the identified races and the shared-Promise dedupe approach used to fix them. |
| .planning/perf/staging-baseline-2026-03-24.md | Captures pre-change staging baseline measurements for login and session restore. |
Comments suppressed due to low confidence (1)
apps/web/src/lib/api/auth.ts:76
- The shared
refreshPromiseis not cleared/invalidated onlogout(). If a refresh is in-flight when logout happens, callers can still await a stale refresh response, and a subsequent login in the same SPA session could also reuse that in-flight promise. Consider explicitly resetting the in-flight refresh state when logging out (and/or scoping it to the current session) so refresh results from a prior session can’t leak into a new one.
refresh: (): Promise<TokenResponseDto> => {
if (!refreshPromise) {
refreshPromise = authControllerRefresh({}).finally(() => {
refreshPromise = null;
});
}
return refreshPromise;
},
/**
* Logout and invalidate refresh token.
* Clears HTTP-only cookie on backend.
*/
logout: (): Promise<LogoutResponseDto> => authControllerLogout(),
- Remove unnecessary async on response event handlers - Fix todo header: api-config.ts → auth.ts (actual implementation file) - Update re-measurement checklist: confirm race resolved, not still present Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 7c8dd10ac5a6
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
auth.ts):authApi.refresh()now deduplicates concurrent callers via shared in-flight Promise — collapses 6 simultaneous refresh calls to 1useAuth.ts):initializeOrLoadVault()deduplicates at module level — prevents concurrent vault init attempts that hitduplicate key value violates unique constraint "UQ_folder_ipns_user_ipns"Root Cause
On page reload, multiple React effects fire simultaneously. Each calls
POST /auth/refresh, but the first response rotates the refresh token, causing later calls to fail with 401. Multiple successful refreshes then race to callinitializeOrLoadVault()— both seegetVault() → 404and attempt vault init, with the second hitting a DB unique constraint. The error cascades tocoreKitLogout(), breaking the session.Test plan
Page reload preserves session and reloads root folder) passes on CIPOST /auth/refreshcall on reload (browser devtools Network tab)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation