Skip to content

Serialize authoritative MCP OAuth refresh transactions#30416

Open
stevenlee-oai wants to merge 3 commits into
dev/stevenlee/mcp-oauth-independent-2-refresh-transactionfrom
dev/stevenlee/mcp-oauth-independent-3-refresh-transaction
Open

Serialize authoritative MCP OAuth refresh transactions#30416
stevenlee-oai wants to merge 3 commits into
dev/stevenlee/mcp-oauth-independent-2-refresh-transactionfrom
dev/stevenlee/mcp-oauth-independent-3-refresh-transaction

Conversation

@stevenlee-oai

@stevenlee-oai stevenlee-oai commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

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

Stack

Review and merge in order. Every layer is independently correct and documents its safe stopping point.

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

This PR is layer 3.

Why

Pinning one credential store prevents a client from hot-switching between stale authorities, but it does not stop two processes from refreshing the same rotating token concurrently. Both can send R1; one persists R2, while the other can replay R1 or overwrite the winner. Refresh must be one serialized read-refresh-write transaction based on a reread made after taking the credential lock.

What this PR does

  • Adds a bounded per-credential cross-process lock keyed by compute_store_key(server_name, url) under the active CODEX_HOME.
  • Rereads the lifecycle-pinned store after acquiring the lock, adopts a winner from another process, or performs one provider refresh and persists it before committing in-memory state.
  • Runs the refresh-and-persist transaction in an owned task so caller cancellation cannot stop persistence after the provider may have rotated the token.
  • Gives lock acquisition and the provider request independent bounds; refresh time is excluded from MCP handshake and operation budgets.
  • Preserves refresh tokens and scopes omitted from a successful provider response.
  • Propagates refresh failures from startup and public-operation preflight instead of continuing with stale credentials.
  • Treats credentials deleted while waiting for the transaction lock as an authentication-required result, rather than a generic refresh failure.
  • Adds focused tests for actual lock contention, one-provider-refresh winner adoption, caller cancellation, selected-store read failures, provider timeout, and omitted-field carry-forward.

Explicit decisions

  • Refresh always uses the already-resolved store from layer 2. A selected-store failure is returned; File and Keyring are never consulted as co-authoritative fallbacks.
  • Provider timeout means the outcome is unknown. The lock is released and a later serialized retry is permitted. Provider grace may recover; otherwise reauthorization is unavoidable. This residual risk is accepted instead of holding the lock indefinitely.
  • If persisting refreshed token B fails, the transaction logs the failing stage, restores the prior in-process credential, and returns an error. It does not serve unpersisted B or retry persistence yet; a TODO records that possible future policy.
  • Direct keyring coordination across different CODEX_HOME values remains out of scope until there is a safe cross-platform rendezvous.

Safe stopping point

This PR independently fixes startup and public-operation preflight refresh races. RMCP-owned GET/DELETE/server-response traffic and one-shot 401 recovery remain unchanged until layer 4.

Review size

The net layer is 8 files, +757/−43. About 430 additions are production code; the remainder is focused concurrency and failure-path coverage.

Validation

  • just test -p codex-rmcp-client (103 passed; expected environment skips)
  • cargo shear

@stevenlee-oai

Copy link
Copy Markdown
Contributor Author

@codex review

— Codex

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1df2d68e70

ℹ️ 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".

Comment thread codex-rs/rmcp-client/src/oauth/refresh_transaction.rs Outdated
@stevenlee-oai

Copy link
Copy Markdown
Contributor Author

@codex review

— Codex

@chatgpt-codex-connector

Copy link
Copy Markdown
Contributor

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: 65ac8434d8

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant