fix(auth): apply a refreshed operation token to the client, not just the replayed request

[dawsontoth]

A memoized direct-connect client kept re-sending the Bearer it was built with, so once the operation token expired every poll tick paid a guaranteed 401 plus a replay, indefinitely and silently — shouldKeepEvent drops 401s. RUM caught 87 of them against one customer instance in 18.6 minutes on the browse-data view. Fixes #1700.

The first commit armed the client's default header after a refresh. Review (thanks @kriszyp) showed that was too narrow, and the second commit replaces it: direct-connect clients now read the Bearer from the store on every send, so one refresh advances every client a page holds rather than each one paying its own 401. Recovery is scoped to a connection generation so a mint racing a disconnect/reconnect can neither overwrite the new identity's token nor replay the old request as the new user, and the refresh exchange opts out of the token lifecycle — it was getting this same 401 interceptor and re-entering the mint awaiting it, a promise cycle that hung every later request for that entity. That last one also fixes #1701.

For the human reviewer

1. One judgment call is genuinely open, and it is the one I did not want to make alone (send-time-store-bearer-vs-client-invalidation). The generation is stamped at a request's first send, not at client construction. Retries are therefore scoped correctly, but a fresh request from a client retained across a reconnect adopts the new connection's identity. Codex has now asked three times for construction-time stamping; the Harper adjudicator rejected it in an earlier round on the grounds that clients are entity-scoped (useInstanceClientIdParams memoizes on route params), so pinning at construction leaves a still-mounted page's only client permanently cancelled after any reconnect until remount. I tested that objection rather than taking either side on faith: with construction-time stamping a mounted page still worked, because a first send takes the stamp branch and never compares — so the proposed fix does not close the closure case it is motivated by, and only changes what retries compare against. Round 9 named a concrete caller worth your eye, useRollingConfigUpdate.tsx:68, which builds clients once and awaits a restart between operations. Deciding this needs a product call on whether a reconnect should invalidate in-flight workflows; I would rather you rule than have me pick.

2. A superseded retry is cancelled rather than sent (cancel-shape-err-canceled), with a hand-built AxiosError carrying ERR_CANCELED. describeAuthFailure already classifies that code, and nothing in src calls axios.isCancel. CanceledError would read better; it is a one-line change if you prefer it.

3. A signed-out retained client sends no Authorization at all (signed-out-send-vs-cancel) — the constructor-baked header is deleted rather than left to ride out a JWT the store discarded but the instance still honours for ~a day. The alternative is cancelling the request outright; I chose the unauthenticated 401 because it fails closed at the server and keeps the error shape callers already handle.

4. The best-effort logout client clears request interceptors in bulk (logout-client-bulk-interceptor-clear), here. Sign-out deliberately builds the client before clearing the store, and the send-time read defeated that — the logout was going out unauthenticated, leaving the token valid at the instance, which inverts the point of signing out. Clearing the interceptor restores the baked header. A narrower opt-out flag would be less blunt.

Not decided here: whether token freshness should live in this interceptor at all, or in a per-entity credential holder that clients consult. That is the framing question the review kept returning to, and it is bigger than this fix.

Verification

Route: integration tests driving real axios, the real getInstanceClient, and the real store, with one fake instance serving every client — including the refresh client the store builds internally, which no per-client hook can reach.

Nine behaviours are pinned and every one is mutation-checked — each guard deleted individually turns a test red, so none of them is decorative: refresh propagation to retained peers; generation-scoped retry cancellation; no deadlock when the refresh token is also rejected; a superseded refresh not evicting the live one; the commit guard discarding a mint whose connection was replaced; own-entry-only eviction; the post-sign-out header strip; and the logout still presenting its credential.

Fails on base — the original reproduction against origin/stage:

AssertionError: expected [ 'Bearer stale-token', …(2) ] to have a length of 1 but got 3

Full gate on Node 24.20.0: vitest run 341 files / 2,977 passed (11 skipped); tsc -b, oxlint, dprint check all exit 0.

Not verified in a browser: reproducing the original needs a direct-connect instance whose operation token expires mid-session (~1 day), which the preview cannot stage.

Complexity: complicated

_{Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; blocked=cursor-grok(failed); declined=cursor-composer; rounds=10 @ 378aa83}

_{Human-Review-Need: 4 (decisions: send-time-store-bearer-vs-client-invalidation, cancel-shape-err-canceled, signed-out-send-vs-cancel, logout-client-bulk-interceptor-clear) @ 378aa83}

[gemini-code-assist]

Code Review

This pull request updates the token recovery interceptor (curryRecoverExpiredOperationToken) to write the recovered token to the client's default headers, preventing subsequent requests from failing with a 401. It also adds a check to reject the request if the store no longer holds the recovered token, and includes comprehensive unit and integration tests. The feedback suggests adding an additional unit test to verify that the interceptor immediately replays the request using the store's token without triggering a recovery flow if a newer token is already present in the store.

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	62.51%	8677 / 13880
🔵	Statements	62.86%	9275 / 14754
🔵	Functions	55.17%	2180 / 3951
🔵	Branches	56.11%	6258 / 11153

File Coverage

File	Stmts	Branches	Functions	Lines	Uncovered Lines
Changed Files
src/config/getInstanceClient.ts	82.85%	85.45%	100%	82.85%	40-47
src/features/auth/store/authStore.ts	75.17%	63.08%	80%	75.61%	104, 190-211, 227-236, 246, 267, 291-302, 309-318, 383-386, 625, 640-646, 680, 684-691, 703, 716-744
src/integrations/api/retryExpiredOperationToken.ts	100%	100%	100%	100%

Generated in workflow #1900 for commit 378aa83 by the Vitest Coverage Report Action

[cb1kenobi]

The stale-client 401 loop is fixed by writing the recovered Bearer onto client defaults after the store still holds that exact token. Sequential requests after the first refresh no longer send the expired token, and a mint that lands after sign-out is not applied. No new blocking defect showed up on the changed interceptor lines; the leftover design questions and the refresh-token deadlock are already documented or filed.

—
Reviewed 4b05406

[cb1kenobi]

The fix writes the recovered operation token onto the direct-connect client's default Authorization header, so a memoized client stops re-sending the expired Bearer on every poll after the first refresh. A store-agreement check keeps a token minted after sign-out or a mode flip from arming the client, failing the request instead. Tests drive the real axios client and store, and the changed interceptor is fully covered with CI green. No blocking defect on the changed lines; the remaining design questions and the pre-existing refresh-token deadlock are already documented in the PR body or filed as issue #1701.

—
Reviewed 4b05406

[kriszyp]

Looks good
_{🤖 Reviewed with Codex}

[kriszyp]

This equality is not an auth-session guard. If A's recovery is in flight, A disconnects, and B reconnects to the same entity, src/features/auth/store/authStore.ts:430-434 checks only mode === 'direct'; A's completion can overwrite B's token, this equality passes, and the old request replays. Conversely, an old A request whose 401 arrives after B connects can refresh B's credential and replay A's operation as B. Please capture a per-connection generation or stable session identity when creating/sending the client, and require it both when committing the recovered token and before updating defaults or replaying. The discriminating case is drop-and-reestablish, not only the tested drop-to-undefined case.

[dawsontoth]

Done — added a per-entity connection generation, bumped on every establish, flag-off, teardown and global clear. updateDirectOperationToken now requires the generation as well as mode === 'direct', so a mint that lands after a drop-and-reestablish is discarded rather than overwriting B's token, and each request carries the generation it was sent under so a superseded one is never replayed. Two further races fell out of it and are fixed too: the in-flight refresh map is cleared on a bump (otherwise B's first 401 joins A's doomed mint), and the finally only evicts its own entry (otherwise A settling evicts B's and spawns a duplicate concurrent mint). The drop-and-reestablish case you named is the one under test, not just drop-to-undefined.

One piece I deliberately did not decide: the generation is stamped at a request's first send, so retries are scoped but a fresh request from a client retained across a reconnect still adopts the new identity. Construction-time stamping has been proposed and rejected across review rounds, and when I tested it, it did not actually close that case while it does risk cancelling a still-mounted page's client after every reconnect. It is the first item under "For the human reviewer" — I'd rather you rule on it.

🤖 Addressed by Claude Code

[cb1kenobi]

The change fixes a real bug: a memoized direct-connect client kept sending its expired token forever, causing an endless 401-and-retry loop. Writing the refreshed token onto the client's default headers stops that, and a store-agreement check prevents a token minted after sign-out from being used. The interceptor is fully covered by tests that drive the real axios client and store, and CI is green. The remaining design questions and a pre-existing refresh-token deadlock were already raised inline and are tracked separately.

—
Reviewed 4b05406

[cb1kenobi]

Would throw do the same thing?

Suggested change

	return Promise.reject(error);
	throw error;

[dawsontoth]

Yes — identical here. The handler is async, so throw error and return Promise.reject(error) both reject the returned promise with the same object; confirmed rather than assumed:

[{"s":"rejected","same":true},{"s":"rejected","same":true}]

I left it as-is only for symmetry: this handler has three rejection paths (lines 20, 25, 30) and the sibling retryGatewayErrors.ts has three more, all spelled return Promise.reject(error). Flipping just this one would make it the odd one out. Happy to switch all six to throw in a follow-up commit if you prefer that spelling — say the word and I'll do the set, not the single line.

🤖 Addressed by Claude Code

[cb1kenobi]

Retained direct clients now use the current token, while generation checks prevent cross-session replays. Refresh requests bypass recovery to avoid the prior deadlock, and no blocking defects were confirmed.

—
Reviewed 378aa83

[cb1kenobi]

A memoized direct-connect client kept sending the token it was built with, so every poll tick after expiry paid a 401 plus a replay. This reads the Bearer from the store on every send and scopes recovery to a connection generation, so one refresh advances all of a page's clients and a mint racing a reconnect can't overwrite the new identity's token. Interceptor ordering, generation propagation through axios config merging, and every store write path check out, and the integration tests genuinely discriminate. The four flagged design decisions need a human product call, not a code fix.

—
Reviewed 378aa83

[cb1kenobi]

No blocking defects were confirmed on the changed lines. Existing discussion already covers the remaining design tradeoffs.

—
Reviewed 378aa83