[rmcp-client] Coordinate MCP OAuth refresh across clients - #28647
[rmcp-client] Coordinate MCP OAuth refresh across clients#28647stevenlee-oai wants to merge 13 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cafc8a930
ℹ️ 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".
| if latest != snapshot { | ||
| if let Some(latest) = latest { | ||
| self.adopt_credentials(latest).await?; | ||
| } | ||
| return Ok(()); |
There was a problem hiding this comment.
expires_in differences
When a persisted token is inside the refresh skew but not yet expired, load_oauth_tokens_* recomputes expires_in from expires_at on every read, so rereading under the lock can make latest != snapshot solely because a second elapsed. This branch then assumes another client refreshed, adopts the same near-expiry credential, and returns without calling refresh_token; once the token expires, the transport can still auto-refresh outside this lock and reintroduce the rotating-refresh-token race this change is trying to prevent. Compare only stable credential fields or re-check token_needs_refresh(latest.expires_at) before skipping the refresh.
Useful? React with 👍 / 👎.
| Some(oauth_persistor), | ||
| ), | ||
| } => { | ||
| oauth_persistor.refresh_if_needed().await?; |
There was a problem hiding this comment.
For OAuth-backed streamable HTTP clients with expired persisted credentials, this await can perform provider discovery/token refresh I/O and can also wait indefinitely for another process to release the refresh lock, but it runs before service::serve_client(...) is wrapped in the caller's handshake timeout below. In that scenario initialize(..., Some(timeout)) no longer bounds startup, so a hung token endpoint or lock holder can hang the client instead of timing out; the refresh needs to share the same timeout/deadline as the initialize attempt.
Useful? React with 👍 / 👎.
| if latest.is_none() && snapshot.is_some() { | ||
| self.clear_manager_credentials().await; | ||
| let mut last_credentials = self.inner.last_credentials.lock().await; | ||
| *last_credentials = None; |
There was a problem hiding this comment.
In OAuthCredentialsStoreMode::Auto, load_oauth_tokens_from_keyring_with_fallback_to_file converts a keyring read error into a fallback-file lookup; if the credential was only in the keyring and that lookup returns None, this new branch treats the token as deliberately removed, clears the in-memory manager, and aborts instead of refreshing the still-available snapshot. A transient keyring outage during startup/proactive refresh can therefore force reauthorization even though the client already has the refresh token; preserve the keyring error or allow the snapshot refresh when Auto fallback is empty due to a keyring failure.
Useful? React with 👍 / 👎.
| Ok(find_codex_home()? | ||
| .join(REFRESH_LOCK_DIR) | ||
| .join(format!("{digest:x}.lock")) |
There was a problem hiding this comment.
For the default direct keyring backend, the OAuth credential is stored in the OS keyring by compute_store_key(server_name, url) and is not scoped by CODEX_HOME, but this new lock path is under find_codex_home(). Two Codex processes that use different CODEX_HOME values while sharing the same OS keyring entry will therefore take different lock files and can still refresh the same rotating refresh token concurrently; put the direct-keyring lock in a location scoped like the keyring credential, or include the home/profile in the credential key as well.
Useful? React with 👍 / 👎.
f589e88 to
c3cff7d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3cff7d5cb
ℹ️ 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".
| return Ok(()); | ||
| } | ||
|
|
||
| let _lock = |
There was a problem hiding this comment.
When a stored token is just outside this 30s skew, or crosses it between this check and the actual HTTP request, this returns before taking the cross-process lock. RMCP also refreshes tokens inside the request path when they fall under its 30s buffer, and the later persist_if_needed save is not protected by this lock, so two clients starting with the same near-expiry rotating refresh token can still refresh concurrently and leave one client with stale credentials. Consider acquiring the coordination lock before deciding to skip, or using a wider/authoritative preflight window that prevents RMCP's internal refresh path from running unlocked.
Useful? React with 👍 / 👎.
| Some(oauth_persistor), | ||
| ), | ||
| } => { | ||
| oauth_persistor.refresh_if_needed().await?; |
There was a problem hiding this comment.
For OAuth-backed streamable HTTP servers with expired or near-expired saved credentials, this refresh runs before the transport handshake is wrapped in the caller's startup_timeout_sec. If another Codex process holds the refresh lock or the token endpoint stalls, startup can now wait for the fixed OAuth lock/provider limits (60s/45s) even when the server is configured with a much smaller startup timeout, so bound this preflight by the remaining startup deadline too.
Useful? React with 👍 / 👎.
| timeout: Option<Duration>, | ||
| ) -> Result<ListToolsResult> { | ||
| self.refresh_oauth_if_needed().await; | ||
| self.refresh_oauth_if_needed().await?; |
There was a problem hiding this comment.
When a tool/resource operation supplies a timeout, the OAuth preflight happens before run_service_operation starts applying that timeout. With near-expiry credentials and either a contended refresh lock or a slow token endpoint, a call can wait for the fixed OAuth refresh waits before the configured tool_timeout_sec begins, so the per-tool limit is bypassed; pass the operation deadline through the refresh path as well.
Useful? React with 👍 / 👎.
| store_mode: OAuthCredentialsStoreMode, | ||
| keyring_backend_kind: AuthKeyringBackendKind, | ||
| ) -> Result<()> { | ||
| let _lock = RefreshCredentialLock::acquire_for_server(server_name, &tokens.url).await?; |
There was a problem hiding this comment.
In File mode (and Auto when falling back), this lock is keyed per server, but the subsequent save/delete rewrites the single shared .credentials.json map. Two processes updating different MCP servers therefore take different locks, both read the same old file, and the later write can drop the other server's refreshed/login/logout update; use a lock that covers the fallback credentials file whenever that backend is written.
Useful? React with 👍 / 👎.
878649a to
aca2273
Compare
|
@codex review |
aca2273 to
e37efba
Compare
efd9577 to
a180e13
Compare
a180e13 to
e1f541b
Compare
Codex Thread 019ed376-876c-7793-8da7-74eabb2ae263
Why
Multiple Codex clients can start from the same persisted MCP OAuth refresh token. If they refresh concurrently, a provider that rotates refresh tokens can accept one request and reject the other, leaving one client with stale credentials.
This race existed because RMCP could refresh from its in-memory credential independently of Codex's persistent store. A timing buffer alone cannot close that gap, especially when a provider omits
expires_inor an access token is rejected before its recorded expiry.File-backed stores have a second concurrency problem:
.credentials.jsonandsecrets/mcp_oauth.ageeach contain credentials for multiple MCP servers. Concurrent read-modify-write operations for different servers can overwrite each other's entries.This PR gives Codex sole ownership of MCP OAuth refresh and persistence, and coordinates both levels without holding a store-wide lock across browser or provider network requests.
Ownership Model
expires_at.refresh_token,expires_in, ortoken_received_at, so it cannot independently refresh or persist a rotated credential.OAuthPersistorowns proactive refresh, 401 recovery, refresh-token rotation, and persistence.AuthorizationManageronly while Codex holds both the manager mutex and the per-credential transaction lock. The manager is restored to an access-only credential before the mutex is released.Lock Model
There are two lock scopes:
Per-credential transaction lock
compute_store_key(server_name, url).Short store-wide file lock
.credentials.jsonand one forsecrets/mcp_oauth.age.Direct keyring storage remains per credential because each credential is a separate keyring item. Its fallback-file cleanup uses the File store lock.
Lock ordering is always per-credential first, then one store lock. The File and Secrets store locks are never held together.
Refresh And Recovery
Before an operation, Codex proactively refreshes credentials whose absolute
expires_atis within 60 seconds:Loading may recompute relative
expires_in, but refresh decisions use the authoritative absoluteexpires_at, so elapsed-time drift does not incorrectly skip a required refresh.If initialization or an MCP operation returns 401, Codex runs the same locked transaction and retries that request once. If another client already persisted a newer usable access token, the loser adopts it without another provider request. If the latest access token is still the rejected token, Codex forces a provider refresh even when expiry metadata is absent.
Missing persistent credentials clear the live manager and return authorization-required behavior instead of refreshing stale in-memory state. Lock, reread, adoption, persistence, and provider-refresh errors propagate; RMCP does not issue an independent refresh or retry.
In
Automode, a keyring read failure is distinct from a confirmed missing credential. Codex still uses an available fallback-file credential, but if the keyring read fails and no fallback exists, the refresh returns the storage error without clearing the live manager or using its stale refresh token.Login, Logout, And Active Threads
codex mcp logoutlocks before deleting credentials. A completed logout cannot be overwritten or resurrected by refresh.Bounded Waits
Non-Goals / Known Limits
config/mcpServer/reloador restart to load the newly authorized server and tools. A targeted refresh is intentionally separate from this credential-concurrency change so unrelated MCP sessions are not reconnected by a full-manager reload.codex mcp logoutis a separate process and cannot notify active app-server threads. Existing sessions likewise needconfig/mcpServer/reloador restart to immediately discard loaded clients and tools after CLI logout.CODEX_HOMEdirect-keyring coordination remains out of scope. File and Secrets locks naturally coordinate processes using the same backing store under oneCODEX_HOME.Verification
just test -p codex-rmcp-client(104 passed, 6 skipped)just fix -p codex-rmcp-clientFocused regressions cover:
expires_indrift without incorrectly skipping a required refresh;