[rmcp-client] Serialize MCP OAuth refresh ownership

[stevenlee-oai]

Codex Thread 019edd6d-6f14-74e2-853c-345d1803d4a6

Important

This PR belongs to the superseded MCP OAuth stack. Please review and merge the replacement stack beginning with openai/codex#30292. This PR remains available only as historical/reference context.

Replacement review order:

1. openai/codex#30292 — shared File/Secrets store locking
2. openai/codex#30293 — authoritative serialized refresh
3. openai/codex#30294 — Codex-owned transport refresh and one-shot 401 recovery
4. openai/codex#30295 — login/logout transaction serialization
5. openai/codex#30296 — diagnostic-only Auto store drift reporting

This is part 1 of a five-PR stack that prevents concurrent MCP OAuth refreshes from replaying a rotating refresh token or overwriting newer credentials.

Review and merge as one stack. The five PRs are an atomic merge unit split only for reviewability. Each tip compiles and is testable, but implementation and regression tests are intentionally not duplicated across intermediate layers.

Review order

1. openai/codex#29017 — refresh ownership, authoritative reread, and lifecycle-local store pinning (this PR)
2. openai/codex#29019 — serialize login and logout with refresh
3. openai/codex#29021 — lock aggregate stores and observe Auto resolution drift
4. openai/codex#29018 — route all OAuth recovery through Codex
5. openai/codex#30089 — consolidated concurrency and transport regression tests

Why

OAuth providers may rotate the refresh token on every refresh. The provider request, authoritative credential reread, in-memory update, and durable write therefore have to behave as one serialized transaction. Otherwise two Codex processes can both consume token A, or a slower writer can overwrite the newer token B and force reauthentication.

Auto also currently chooses between keyring and File independently on each operation. That fallback is useful when a client starts, but switching stores during one refresh lifecycle can replay an older token from the other store. This layer resolves the source once for the RmcpClient lifecycle and then reads and writes only that source.

What changes

• Resolve Auto once at client construction using the existing keyring-first/File-fallback policy, and retain that concrete source across transport retries and session rebuilds.
• Serialize refresh plus persistence with a per-credential cross-process lock keyed consistently with compute_store_key(server_name, url).
• Reread the resolved store after taking the lock and adopt a newer winner before deciding whether to call the provider.
• Treat durable persistence as the commit point for refreshed credentials. The authorization-manager guard prevents the refreshed token from becoming request-authoritative until the selected store accepts it.
• If persistence fails, restore the previous request-only credentials, return the persistence error, and do not fall back to the other store or keep serving the unpersisted token.
• Emit stage-specific warnings plus one refresh-transaction failure summary inside the owned task, so caller cancellation cannot suppress the summary that identifies the server and refresh reason.
• Keep the provider request bounded at 45 seconds and lock acquisition bounded at 60 seconds.
• Own refresh plus persistence in a detached task once the provider request can begin, so caller cancellation cannot drop a successful response before the persistence decision completes.
• Extract store resolution, refresh locking, and persistence into focused modules.

Decisions encoded by this stack

• Credential-store authority is lifecycle-local, not durably persisted. Part 3 adds only a token-free diagnostic observation of resolution drift; it is never consulted to choose a store.
• Once a lifecycle resolves to Keyring or File, reread and refresh never hot-fallback to the other store.
• Provider timeout means “outcome unknown”: release the lock and allow a later serialized retry. We accept the residual provider-dependent grace-period risk instead of holding the lock indefinitely.
• A successful provider response finishes its persistence attempt even if the original caller stops waiting.
• Failed persistence is not retried in this stack. Codex restores the previous in-process credentials and surfaces the error from the transaction that failed instead of hiding the failure behind a temporarily usable, unpersisted token.
• The provider may already have consumed the previous refresh token, so a later attempt can require reauthorization. This fail-closed outcome is accepted for now; the code carries a TODO to add an explicit bounded retry or reconciliation policy if persistence failures prove common.
• OAuth time is separate from MCP handshake/tool deadlines; those deadlines still bound the MCP work and any public request replay.
• openai/codex#28647 and its branch remain untouched.

Review focus

Review the credential identity, lifecycle source pin, authoritative reread, cancellation boundary, persist-before-authority policy, and provider-timeout policy. Part 4 completes the ownership switch by withholding refresh capability from RMCP and routing all transport paths through Codex.

Non-goals

• Persisting or migrating an authoritative Auto backend selection.
• Retrying failed durable writes or quarantining provider-timeout outcomes.
• Changing OAuth metadata, scopes, app-server APIs, or upstream RMCP.

Validation

• just test -p codex-rmcp-client: 90 passed, 2 skipped.
• The full race and recovery matrix is consolidated in part 5 so it is reviewed once.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1328e80255

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[anp-oai]

This makes sense, but holding a synchronous mutex across an await boundary is a recipe for deadlocks. Can we avoid this?

[anp-oai]

I think we should assume that transient storage issues will happen in the field, doesn't need to be in this PR but could this be addressed in this stack?

[anp-oai]

This seems like it might invite bugs -- what would we need to do to fix it?