Re-read OAuth state from disk across MCP sessions - #848
Conversation
The MCP OAuth provider snapshotted auth state once at construction, so a session that re-authenticated never published its fresh tokens to sibling sessions: they kept serving a stale in-memory mirror and needlessly triggered the browser flow after a 401. The SDK requires tokens(), clientInformation(), and codeVerifier() to be synchronous, so the getters now guard with statSync and only readFileSync the auth file (via a new loadAuthStateSync that mirrors loadAuthState's parsing) when its mtime or size changed. A vanished or unreadable file keeps the in-memory mirror instead of throwing from a sync getter.
The refresh guard recorded the file stamp before reading it, so a persistently unreadable auth file (e.g. chmod 000) wiped the in-memory mirror with empty state and never retried. The stamp is now committed only after a successful read, leaving it stale so the next getter call retries. loadAuthStateSync also now matches loadAuthState's error contract: missing (ENOENT) or corrupt files yield empty state, while other read errors propagate to the caller instead of being swallowed.
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Primary · Comment
Ready for human review. The branch stays within the OAuth provider/store boundary, preserves synchronous SDK getter contracts, and passes the repository pre-PR gate.
No residual findings.
Checks: bun run check (exit 0; 6,286 pass, 0 fail); reviewer-of-record log/stat/message audits; targeted OAuth tests (exit 0).
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Critic · Comment
The full origin/main...HEAD behavioral review is clean. Existing sibling providers observe newly saved tokens while missing or unreadable auth files preserve the in-memory mirror.
No findings.
A different-port sibling writing PKCE or DCR was replacing this session's live verifier and redirect. Failed disk reads also cleared good in-memory tokens because empty-on-corrupt was used as cache invalidation.
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Verdict: request changes.
CL-7527 is only half-met at a41fda5. Idle-sibling token reload and PKCE locality hold; DCR locality does not survive the completion mutation.
shouldAdoptClient is used only in refreshDurableFromDisk. apply() still unconditionally assignClient (src/mcp/oauth-provider.ts), and saveTokens / saveCodeVerifier both go through apply.
Trigger: Session A saveClient + saveCodeVerifier on port 62000, then sibling B constructs on 60435 during A's browser wait. B createOAuthProvider runs dropStaleClientRegistration and writes the file with no client. A's getters still hold client-on-62000. User finishes A's flow; SDK saveTokens. apply loads B's snapshot, persists tokens with no client, then assignClient deletes A's in-memory DCR. Next clientInformation() is undefined, so another browser flow. If B saveClients first, A's tokens land under B's client_id.
The getter-only tests never call apply on A after B writes, so they miss this.
Please apply the same adopt-client guard on apply (or keep this session matching client when the disk snapshot has none / a different-port client without completed tokens), and add a test that A saveTokens after B construct still keeps A's client_id / redirect_uris.
Should-fix: HEAD dropped the mtime/size stamp, so every SDK tokens() readFileSyncs. MCP StreamableHTTPClientTransport._commonHeaders calls tokens() on every send().
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Verdict: request changes.
CL-7527 is only half-met at a41fda5c. Idle-sibling token reload and PKCE locality hold; DCR locality does not survive the completion mutation.
shouldAdoptClient is used only in refreshDurableFromDisk. apply() still unconditionally assignClient (src/mcp/oauth-provider.ts), and saveTokens / saveCodeVerifier both go through apply.
Trigger: Session A saveClient + saveCodeVerifier on port 62000, then sibling B constructs on 60435 during A's browser wait. B createOAuthProvider runs dropStaleClientRegistration and writes the file with no client. A's getters still hold the 62000 client. User finishes A's flow; SDK saveTokens. apply loads B's snapshot, persists tokens with no client, then assignClient deletes A's in-memory DCR. Next clientInformation() is undefined, so another browser flow. If B saveClients first, A's tokens land under B's client_id.
The getter-only tests never call apply on A after B writes, so they miss this.
Please apply the same adopt-client guard on apply (or keep this session matching client when the disk snapshot has none / a different-port client without completed tokens), and add a test that A saveTokens after B construct still keeps A's client_id / redirect_uris.
Should-fix: HEAD dropped the mtime/size stamp, so every SDK tokens() hits readFileSync. MCP StreamableHTTPClientTransport._commonHeaders calls tokens() on every send().
apply() assigned disk client after every mutation, so a different-port sibling saveClient then this session's saveTokens replaced live DCR. Mutations now rewrite this session's matching client onto disk; getters still adopt completed sibling tokens.
Idle sibling tokens must still land on a session that already has matching live DCR, without going through apply. Same-port saveClient must replace this session's client even after a sibling wrote a different port.
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Verdict: approve (GitHub cannot approve own PR).
CL-7527 holds at 0c8cc4de. Sync SDK getters re-read auth state from disk when the file stamp changes; apply() keeps matching live DCR across sibling saveTokens; idle sibling tokens still adopt without going through apply.
CI is green (prettier, eslint, typecheck, build-and-test).
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Verdict: approve (GitHub cannot approve own PR).
CL-7527 holds at 0c8cc4d. Sync SDK getters re-read auth state from disk when the file stamp changes; apply() keeps matching live DCR across sibling saveTokens; idle sibling tokens still adopt without going through apply.
CI is green (prettier, eslint, typecheck, build-and-test).
1 participant
Summary
Verification
bun run checkpasses (6,286 tests, 0 failures)Fixes CL-7527