fix(desktop): stop reporting transient token-refresh failures as expired sessions - #73844
fix(desktop): stop reporting transient token-refresh failures as expired sessions#73844Sora-bluesky wants to merge 4 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the native-refresh error shape and covering the ticket and REST sibling paths. The current-main premise is confirmed: apps/desktop/electron/main.ts:6311, :7570, and :10131 swallow refresh failures, while fetchJson emits untagged Error("401: …") at :4214-4216.
Problems
apps/desktop/electron/native-access-token.ts:31makesauthEpochglobal, although refresh flights are scoped by base URL. An explicit auth change for gateway A invalidates an in-flight refresh for gateway B;:66/:74returnnulland discard B's valid rotation. This is reachable because Desktop supports distinct per-profile remote hosts (apps/desktop/electron/connection-config.ts:377-380).
Suggested changes
- Keep an epoch per normalized base URL, pass the affected URL to
invalidateExplicitAuthChange, and add a two-gateway regression test proving invalidating A does not cancel B's refresh.
Automated hermes-sweeper review.
| deps: NativeAccessTokenCoordinatorDeps | ||
| ): NativeAccessTokenCoordinator { | ||
| const refreshFlights = new Map<string, Promise<string | null>>() | ||
| let authEpoch = 0 |
There was a problem hiding this comment.
authEpoch is global while refreshFlights is per base URL. A login/logout for remote gateway A will make an in-flight refresh for independent gateway B return null at lines 66/74 and drop its rotated token. Please scope the epoch and invalidation to normalized base URL, with a two-host regression test.
|
Confirmed and fixed in 397805ccb. The epoch check sits above The epoch is now per normalized gateway and One thing your report and my first fix both missed. The logout handler bumped the epoch unconditionally while On reachability, Tests now drive the real IPC handlers rather than the coordinator alone. Deleting either production invalidation call fails them; with the previous tests both deletions stayed green. |
|
Production confirmation for the second defect you describe, the one where the dead-RT 401 branch never matches. I hit it last week and the logs show it clearly. Setup: Hermes Desktop in remote mode against a self-hosted gateway, native (cookieless) sign-in, OIDC provider is Authelia 4.39.20 with rotating refresh tokens and reuse detection. My refresh token chain was genuinely revoked provider-side, which is the case the branch exists for. The provider's refresh-token table shows the newest two entries as The desktop never cleared those tokens.
That matches your reading of the code. Worth noting for anyone reading this thread that the revocation itself has a separate cause: concurrent refreshes replaying the same rotating token, tracked in #55712, with #55717 and #71548 coalescing the cookie and native paths respectively. This PR is the other half, what happens once the chain is already dead, and my logs show both mechanisms firing one after the other. Happy to test this branch against a rotating-RT provider with reuse detection if that is useful. I can reproduce the dead-chain state on demand. |
|
@oliver-mee thanks — this is the production trace the branch was missing, and it matches the code on current
The branch currently conflicts with |
7f5b8f9 to
78806d1
Compare
|
@oliver-mee rebased onto |
78806d1 to
7a961e6
Compare
…red sessions ensureNativeAccessToken splits refresh failures on purpose: a dead refresh token clears the stored tokens and resolves null, a transient failure throws so the tokens survive for a retry. mintGatewayWsTicket erased that split with .catch(() => null), so with a native (cookieless) sign-in a network blip fell through to the cookie path, 401ed, and was misreported as an expired session. Two adjacent defects surfaced while fixing it. The dead-RT 401 branch never matched in production because fetchJson rejects with a message-prefixed Error and no statusCode property; the new isNativeRefreshAuthRejection predicate recognizes both shapes. And cookie and native sessions can coexist, so a native-path failure now falls back to the cookie session and only propagates the original native error when that fallback also fails. mintGatewayWsTicket moved to connection-config.ts as a dependency-injected helper (the resolveTestWsUrl pattern) so all of this is unit-testable; main.ts keeps a same-named adapter. Fixes mechanism 2 of NousResearch#73722. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rors Two failure modes in the native OAuth path: - ensureNativeAccessToken could run concurrently, so two callers sent the same rotating refresh token; the loser's 401 then cleared the winner's freshly stored tokens and forced a re-login. Refreshes now share one in-flight promise per gateway (single-flight), a 401 only clears tokens when the stored refresh token is still the one that was sent (compare-and-clear), and an explicit login/logout bumps an auth epoch that makes a stale in-flight refresh discard its result. - both REST call sites swallowed every ensure error into the cookie fallback, so a transient refresh timeout with an empty cookie jar surfaced as a false 401. The fallback now keeps the original native error and rethrows it when the cookie path also fails auth, mirroring the WS-ticket-minting pattern. Extracted into native-access-token.ts / oauth-rest-request.ts with behavioral tests for the race, the stale-loser 401, the logout epoch, and both transient outcomes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The refresh coordinator kept a single authEpoch while refresh flights are keyed per gateway, so an explicit auth change on one gateway cancelled an in-flight refresh on another. The epoch check runs before storeTokens, so the other gateway's rotation was discarded after the server had already consumed its refresh token: the next refresh looked like token reuse and logged that gateway out for real. It also dropped ensure() to null, which is the branch this PR exists to keep out of the "session expired" path. The epoch is now per normalized gateway. invalidateExplicitAuthChange takes the gateway it applies to, and the logout handler calls it in the same if (baseUrl) block that clears that gateway's native tokens, so invalidation and token clearing share one condition. A logout with no URL clears the shared cookie partition and no longer touches native epochs, since it does not change any native tokens. There is no clear-all native-token operation, so the global epoch is gone rather than left as an unused path. Tests drive the real IPC handlers: a URL-bearing logout must stop its own gateway's stale rotation from being stored and must leave another gateway's refresh alone, a no-URL logout must not disturb either, and an explicit login must invalidate its own older flight. Deleting either production invalidation call fails these. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A bare `88,547` sat on line 2 of native-access-token.test.ts. It parses as a comma expression so vitest ran fine, but tsc rejects it (TS2695) and check:lint failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7a961e6 to
f547562
Compare
What does this PR do?
Fixes mechanism 2 of #73722: a transient failure while refreshing the native OAuth access token was reported as "Your remote gateway session has expired", sending the user to re-sign-in for a network blip.
ensureNativeAccessToken()is deliberate about the split: a 401 on refresh means the refresh token is dead, so it clears the stored tokens and resolvesnull; a transient failure (timeout, 5xx, network) throws so the tokens survive for a retry. The call site inmintGatewayWsTicket()erased that distinction with.catch(() => null). With a native (cookieless) sign-in there is no cookie session to fall back to, so the fallback leg 401s andgatewayTicketFailure()classifies the whole thing as an expired session. The reporter's desktop.log shows it live: liveness probe drops, the next resolve says "expired", and the retry seconds later connects with the same credentials.Two adjacent defects surfaced while fixing it, and both are covered here because a naive fix (just dropping the catch) would regress them:
ensureNativeAccessTokencheckserror.statusCode === 401, butfetchJsonrejects with a message-prefixedError("401: …")and nostatusCodeproperty, and the server's dead-RT branch answers 401{"error":"session_expired"}. So a genuinely dead refresh token always took the throw path, and only the swallowed catch kept the reauth flow working by accident. The newisNativeRefreshAuthRejection()predicate recognizes both shapes, making the intended "dead RT → clear tokens → cookie fallback → sign-in" flow real.The end state for each input: dead refresh token → sign-in prompt (as intended); transient failure with a live cookie → connects through the cookie (as baseline did); transient failure with no cookie → retryable connectivity error ("Could not reach the remote Hermes gateway while refreshing its WebSocket ticket. Try reconnecting.") instead of a false sign-in demand, which is the mechanism-2 fix itself.
Testability
mintGatewayWsTicketlived inmain.ts, which importselectronandnode-ptyat module scope and is unreachable from the test suite. The function moved toconnection-config.tsas a dependency-injected helper, the same patternresolveTestWsUrlalready uses there, andmain.tskeeps a same-named adapter so all three call sites (freshGatewayWsUrl,buildRemoteConnection, the connection test'smintTicketinjection) are untouched.Tests
New cases in
connection-config.test.ts:Error("401: …"), nostatusCode) is recognized as an auth rejection, plus negative cases for transient shapes79/79 in
connection-config.test.ts,tsc --noEmitclean.Relation to #73821
Independent fixes for the two mechanisms in #73722. #73821 handles the renderer's boot path; this PR handles the main process's ticket minting. Either lands without the other.