Cross-tab login/refresh race can still restore the wrong session (#393 follow-up) #438

What

login() doesn't participate in the cross-tab cookie lock (AUTH_COOKIE_LOCK, #169) that refreshTokens()/changePassword() already use. Follow-up to #393, whose fix (revokeSupersededCookie() always revoking on a stale flight — landed, single-tab-correct) doesn't close this: it's a different hazard, cross-tab specific.

The gap, precisely

sessionGeneration and the access-token store are per-tab module state (web/src/api/client.ts). When a refresh in Tab A races a login in Tab B:

• Tab B's login() bumps Tab B's own sessionGeneration — invisible to Tab A.
• Tab A's in-flight refresh, on completion, checks sessionGeneration !== generation against Tab A's own counter — which was never touched, so the check is false. Tab A's refresh looks like a completely normal, uncontested completion from Tab A's own perspective.
• Tab A's refresh's Set-Cookie can therefore land after Tab B's login's Set-Cookie (real network arrival order), and fix(spa): a superseded refresh can land its Set-Cookie over a newer login (#310 follow-up) #393's fix never even triggers — there's no "stale" branch to detect it in.
• Result: reloading Tab B can restore the previous user's session. Exactly the wrong-session hole fix(spa): a superseded refresh can land its Set-Cookie over a newer login (#310 follow-up) #393 fixed for the single-tab case, still open cross-tab.

(Raised by a codex review on PR #433, commit 4ca374c.)

Why the obvious fix (login joins the lock) isn't a clean patch

Tried this directly — login() wrapped in the same withAuthCookieLock(...) refresh/changePassword use, timeout matching REFRESH_TIMEOUT_MS. Mechanically it works and forces correct ordering: any in-flight refresh (same tab or another) fully settles — cookie applied — before the login request is even sent, so login's Set-Cookie is provably the last write.

Six existing tests needed restructuring for the new queuing (all straightforward — resolve the parked gate before awaiting login(), not after), and that part is done and was green.

But this surfaces a second, deeper timing bug: supersededToken() (called from currentAccessToken()'s catch block when a parked request's refresh comes back StaleSessionError) does a synchronous getAccessToken() check and assumes "a newer login already committed its token by the time we get here." That assumption held when login() ran independent of any lock (it completed essentially immediately). Once login() queues behind the same lock as the refresh that just failed, its actual completion can land after supersededToken()'s synchronous peek — so the parked request sees no token yet and wrongly 401s instead of waiting for the login that's about to supersede it.

Caught this via 3 failing tests (not by inspection) — e.g. "a late-FAILING obsolete refresh does not clear or corrupt a newer session" started getting ApiError 401 NoSession instead of the expected successful retry on the newer login's token.

What a real fix needs

Not just "make login join the lock" — that alone trades a cross-tab hole for a same-tab timing bug. Needs, together:

1. login() takes AUTH_COOKIE_LOCK (as above).
2. A way for a parked caller to await the in-flight login rather than just peek at getAccessToken() once — mirroring how refreshInFlight already lets concurrent callers share one refresh. Likely a new loginInFlight: Promise<void> | null (or similar) that supersededToken()/currentAccessToken() can await before re-checking the token.
3. Re-verify the full #310/#169 describe blocks in client.test.ts again against that combined change — the six tests already identified as needing restructuring, plus whatever the supersededToken() fix itself touches.

Why this wasn't pushed through in the same PR

This is the same code path that shipped a real regression once already (PR #390 — an abort-based fix for the sibling hazard, reverted for causing a spurious 401). Given that history, landing #393's single-tab fix (already correct, tested, closes the more common case) separately from this cross-tab work — which needs its own careful test pass — matches this repo's own delivery discipline (one structural intent at a time) rather than risking a second regression on the same file bundled into an unrelated PR.

Scope

• login() takes the shared cookie lock (mechanical part, already prototyped — see PR fix(web): always revoke a stale flight's cookie, not just when logged out (#393) #433's history for the working diff and the six restructured tests).
• Add an awaitable in-flight-login handle; supersededToken() awaits it before its token check.
• Re-verify client.test.ts's #310/#169 blocks pass with both changes together.
• Consider whether changePassword()'s own stale-generation handling has the same synchronous-peek assumption anywhere.