From 665d653e5a679c981dc85737e60adb7f9b08f1f8 Mon Sep 17 00:00:00 2001 From: Steven Lee Date: Thu, 25 Jun 2026 21:01:44 +0000 Subject: [PATCH] Serialize shared MCP OAuth stores --- codex-rs/Cargo.lock | 2 + codex-rs/rmcp-client/Cargo.toml | 2 + codex-rs/rmcp-client/src/oauth.rs | 111 ++++++-- .../rmcp-client/src/oauth/resolution_state.rs | 251 ++++++++++++++++++ .../rmcp-client/src/oauth/resolved_store.rs | 105 +++++--- codex-rs/rmcp-client/src/oauth/store_lock.rs | 132 +++++++++ 6 files changed, 543 insertions(+), 60 deletions(-) create mode 100644 codex-rs/rmcp-client/src/oauth/resolution_state.rs create mode 100644 codex-rs/rmcp-client/src/oauth/store_lock.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index bc5953803ed7..96ff8cee507f 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3760,10 +3760,12 @@ dependencies = [ "codex-config", "codex-exec-server", "codex-keyring-store", + "codex-otel", "codex-protocol", "codex-secrets", "codex-utils-cargo-bin", "codex-utils-home-dir", + "codex-utils-path", "codex-utils-path-uri", "codex-utils-pty", "futures", diff --git a/codex-rs/rmcp-client/Cargo.toml b/codex-rs/rmcp-client/Cargo.toml index 517f7270bbe0..311a1dccac56 100644 --- a/codex-rs/rmcp-client/Cargo.toml +++ b/codex-rs/rmcp-client/Cargo.toml @@ -19,8 +19,10 @@ codex-api = { workspace = true } codex-config = { workspace = true } codex-exec-server = { workspace = true } codex-keyring-store = { workspace = true } +codex-otel = { workspace = true } codex-protocol = { workspace = true } codex-secrets = { workspace = true } +codex-utils-path = { workspace = true } codex-utils-path-uri = { workspace = true } codex-utils-pty = { workspace = true } codex-utils-home-dir = { workspace = true } diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index 17dfd5ed8b18..d4d4ad13ec0f 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -18,7 +18,9 @@ mod persistor; mod refresh_lock; +mod resolution_state; mod resolved_store; +mod store_lock; use anyhow::Context; use anyhow::Error; @@ -53,6 +55,11 @@ use std::time::SystemTime; use std::time::UNIX_EPOCH; use tracing::warn; +use self::resolution_state::StoreResolutionReason; +use self::resolution_state::record_store_resolution; +use self::store_lock::OAuthStore; +use self::store_lock::OAuthStoreLock; + use codex_keyring_store::DefaultKeyringStore; use codex_keyring_store::KeyringStore; use codex_utils_home_dir::find_codex_home; @@ -194,6 +201,7 @@ fn load_oauth_tokens_from_secrets_keyring( server_name: &str, url: &str, ) -> Result> { + let _store_lock = OAuthStoreLock::acquire(OAuthStore::Secrets)?; let codex_home = find_codex_home()?; let manager = SecretsManager::new_with_keyring_store_and_namespace( codex_home.to_path_buf(), @@ -276,21 +284,53 @@ fn save_oauth_tokens_with_keyring_store( store_mode: OAuthCredentialsStoreMode, keyring_backend_kind: AuthKeyringBackendKind, ) -> Result<()> { - match store_mode { - OAuthCredentialsStoreMode::Auto => save_oauth_tokens_with_keyring_with_fallback_to_file( - keyring_store, - keyring_backend_kind, - server_name, - tokens, - ), - OAuthCredentialsStoreMode::File => save_oauth_tokens_to_file(tokens), - OAuthCredentialsStoreMode::Keyring => save_oauth_tokens_with_keyring_and_cleanup_file( - keyring_store, - keyring_backend_kind, - server_name, - tokens, - ), - } + let (resolved_store, reason) = match store_mode { + OAuthCredentialsStoreMode::Auto => { + let resolved_store = save_oauth_tokens_with_keyring_with_fallback_to_file( + keyring_store, + keyring_backend_kind, + server_name, + tokens, + )?; + let reason = match resolved_store { + ResolvedOAuthCredentialStore::File => { + StoreResolutionReason::AutoSaveToFileAfterKeyringError + } + ResolvedOAuthCredentialStore::Keyring(_) => { + StoreResolutionReason::AutoSaveToKeyring + } + }; + (resolved_store, reason) + } + OAuthCredentialsStoreMode::File => { + save_oauth_tokens_to_file(tokens)?; + ( + ResolvedOAuthCredentialStore::File, + StoreResolutionReason::ConfiguredSave, + ) + } + OAuthCredentialsStoreMode::Keyring => { + save_oauth_tokens_with_keyring_and_cleanup_file( + keyring_store, + keyring_backend_kind, + server_name, + tokens, + )?; + ( + ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind), + StoreResolutionReason::ConfiguredSave, + ) + } + }; + record_store_resolution( + server_name, + &tokens.url, + store_mode, + keyring_backend_kind, + resolved_store, + reason, + ); + Ok(()) } fn save_oauth_tokens_with_keyring( @@ -319,6 +359,8 @@ fn save_oauth_tokens_with_keyring_and_cleanup_file Result<()> { + // Cross-store cleanup belongs to login-time store selection. Refresh persistence calls the + // raw keyring writer above so a client pinned to keyring never mutates fallback File state. save_oauth_tokens_with_keyring(keyring_store, keyring_backend_kind, server_name, tokens)?; let key = compute_store_key(server_name, &tokens.url)?; if let Err(error) = delete_oauth_tokens_from_file(&key) { @@ -354,6 +396,16 @@ fn save_oauth_tokens_to_secrets_keyring( tokens: &StoredOAuthTokens, ) -> Result<()> { let serialized = serde_json::to_string(tokens).context("failed to serialize OAuth tokens")?; + let _store_lock = OAuthStoreLock::acquire(OAuthStore::Secrets)?; + save_oauth_tokens_to_secrets_keyring_unlocked(keyring_store, server_name, tokens, &serialized) +} + +fn save_oauth_tokens_to_secrets_keyring_unlocked( + keyring_store: &K, + server_name: &str, + tokens: &StoredOAuthTokens, + serialized: &str, +) -> Result<()> { let codex_home = find_codex_home()?; let manager = SecretsManager::new_with_keyring_store_and_namespace( codex_home.to_path_buf(), @@ -363,7 +415,7 @@ fn save_oauth_tokens_to_secrets_keyring( ); let secret_name = compute_secret_name(server_name, &tokens.url)?; manager - .set(&SecretScope::Global, &secret_name, &serialized) + .set(&SecretScope::Global, &secret_name, serialized) .context("failed to write OAuth tokens to encrypted storage") } @@ -372,19 +424,20 @@ fn save_oauth_tokens_with_keyring_with_fallback_to_file Result<()> { +) -> Result { match save_oauth_tokens_with_keyring_and_cleanup_file( keyring_store, keyring_backend_kind, server_name, tokens, ) { - Ok(()) => Ok(()), + Ok(()) => Ok(ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind)), Err(error) => { let message = error.to_string(); warn!("falling back to file storage for OAuth tokens: {message}"); save_oauth_tokens_to_file(tokens) - .with_context(|| format!("failed to write OAuth tokens to keyring: {message}")) + .with_context(|| format!("failed to write OAuth tokens to keyring: {message}"))?; + Ok(ResolvedOAuthCredentialStore::File) } } } @@ -505,6 +558,7 @@ fn delete_oauth_tokens_from_secrets_keyring( server_name: &str, url: &str, ) -> Result { + let _store_lock = OAuthStoreLock::acquire(OAuthStore::Secrets)?; let codex_home = find_codex_home()?; let manager = SecretsManager::new_with_keyring_store_and_namespace( codex_home.to_path_buf(), @@ -539,7 +593,8 @@ struct FallbackTokenEntry { } fn load_oauth_tokens_from_file(server_name: &str, url: &str) -> Result> { - let Some(store) = read_fallback_file()? else { + let _store_lock = OAuthStoreLock::acquire(OAuthStore::File)?; + let Some(store) = read_fallback_file_unlocked()? else { return Ok(None); }; @@ -582,8 +637,13 @@ fn load_oauth_tokens_from_file(server_name: &str, url: &str) -> Result Result<()> { + let _store_lock = OAuthStoreLock::acquire(OAuthStore::File)?; + save_oauth_tokens_to_file_unlocked(tokens) +} + +fn save_oauth_tokens_to_file_unlocked(tokens: &StoredOAuthTokens) -> Result<()> { let key = compute_store_key(&tokens.server_name, &tokens.url)?; - let mut store = read_fallback_file()?.unwrap_or_default(); + let mut store = read_fallback_file_unlocked()?.unwrap_or_default(); let token_response = &tokens.token_response.0; let expires_at = tokens @@ -611,7 +671,8 @@ fn save_oauth_tokens_to_file(tokens: &StoredOAuthTokens) -> Result<()> { } fn delete_oauth_tokens_from_file(key: &str) -> Result { - let mut store = match read_fallback_file()? { + let _store_lock = OAuthStoreLock::acquire(OAuthStore::File)?; + let mut store = match read_fallback_file_unlocked()? { Some(store) => store, None => return Ok(false), }; @@ -697,7 +758,7 @@ fn fallback_file_path() -> Result { Ok(find_codex_home()?.join(FALLBACK_FILENAME).to_path_buf()) } -fn read_fallback_file() -> Result> { +fn read_fallback_file_unlocked() -> Result> { let path = fallback_file_path()?; let contents = match fs::read_to_string(&path) { Ok(contents) => contents, @@ -913,7 +974,7 @@ mod tests { let fallback_path = super::fallback_file_path()?; assert!(fallback_path.exists(), "fallback file should be created"); - let saved = super::read_fallback_file()?.expect("fallback file should load"); + let saved = super::read_fallback_file_unlocked()?.expect("fallback file should load"); let key = super::compute_store_key(&tokens.server_name, &tokens.url)?; let entry = saved.get(&key).expect("entry for key"); assert_eq!(entry.server_name, tokens.server_name); @@ -1025,7 +1086,7 @@ mod tests { &tokens, )?; - let saved = super::read_fallback_file()?.expect("fallback file should load"); + let saved = super::read_fallback_file_unlocked()?.expect("fallback file should load"); let key = super::compute_store_key(&tokens.server_name, &tokens.url)?; assert!(saved.contains_key(&key)); Ok(()) diff --git a/codex-rs/rmcp-client/src/oauth/resolution_state.rs b/codex-rs/rmcp-client/src/oauth/resolution_state.rs new file mode 100644 index 000000000000..d188c1d60e00 --- /dev/null +++ b/codex-rs/rmcp-client/src/oauth/resolution_state.rs @@ -0,0 +1,251 @@ +//! Best-effort diagnostics for changes in MCP OAuth store resolution. +//! +//! This state is deliberately observational, not authoritative. OAuth continues to resolve from +//! configuration and available credentials; failure to read, lock, or write this file must never +//! change which credential store is selected or whether an OAuth operation succeeds. +//! The last observation intentionally survives logout: logout removes credential authority, while +//! this token-free history lets a later login report that `Auto` selected a different store. A +//! successful later resolution or save replaces the observation for that credential identity. + +use std::collections::BTreeMap; +use std::fs; +use std::io::ErrorKind; +use std::path::Path; +use std::time::Duration; + +use anyhow::Context; +use anyhow::Result; +use codex_config::types::AuthKeyringBackendKind; +use codex_config::types::OAuthCredentialsStoreMode; +use codex_utils_home_dir::find_codex_home; +use codex_utils_path::write_atomically; +use serde::Deserialize; +use serde::Serialize; +use tracing::warn; + +use super::ResolvedOAuthCredentialStore; +use super::compute_store_key; +use super::store_lock::OAuthStore; +use super::store_lock::OAuthStoreLock; + +const RESOLUTION_STATE_FILENAME: &str = ".mcp-oauth-store-resolutions.json"; +const RESOLUTION_STATE_VERSION: u32 = 1; +const RESOLUTION_STATE_LOCK_TIMEOUT: Duration = Duration::from_millis(/*millis*/ 250); +const RESOLUTION_CHANGED_METRIC: &str = "codex.mcp.oauth.store_resolution_changed"; + +#[derive(Debug, Clone, Copy)] +pub(super) enum StoreResolutionReason { + AutoLoadFromKeyring, + AutoLoadFromFileAfterMissingKeyring, + AutoLoadFromFileAfterKeyringError, + AutoSaveToKeyring, + AutoSaveToFileAfterKeyringError, + ConfiguredLoad, + ConfiguredSave, +} + +impl StoreResolutionReason { + fn as_str(self) -> &'static str { + match self { + Self::AutoLoadFromKeyring => "auto_load_keyring", + Self::AutoLoadFromFileAfterMissingKeyring => "auto_load_file_keyring_missing", + Self::AutoLoadFromFileAfterKeyringError => "auto_load_file_keyring_error", + Self::AutoSaveToKeyring => "auto_save_keyring", + Self::AutoSaveToFileAfterKeyringError => "auto_save_file_keyring_error", + Self::ConfiguredLoad => "configured_load", + Self::ConfiguredSave => "configured_save", + } + } +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "lowercase")] +enum ObservedStore { + File, + Keyring, +} + +impl ObservedStore { + fn as_str(self) -> &'static str { + match self { + Self::File => "file", + Self::Keyring => "keyring", + } + } +} + +impl From for ObservedStore { + fn from(store: ResolvedOAuthCredentialStore) -> Self { + match store { + ResolvedOAuthCredentialStore::File => Self::File, + ResolvedOAuthCredentialStore::Keyring(_) => Self::Keyring, + } + } +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +struct StoreResolution { + store_mode: OAuthCredentialsStoreMode, + keyring_backend: AuthKeyringBackendKind, + resolved_store: ObservedStore, +} + +#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] +struct ResolutionState { + #[serde(default = "resolution_state_version")] + version: u32, + #[serde(default)] + resolutions: BTreeMap, +} + +impl Default for ResolutionState { + fn default() -> Self { + Self { + version: RESOLUTION_STATE_VERSION, + resolutions: BTreeMap::new(), + } + } +} + +fn resolution_state_version() -> u32 { + RESOLUTION_STATE_VERSION +} + +#[derive(Debug, PartialEq, Eq)] +struct StoreResolutionChange { + previous: ObservedStore, + current: ObservedStore, +} + +pub(super) fn record_store_resolution( + server_name: &str, + url: &str, + store_mode: OAuthCredentialsStoreMode, + keyring_backend: AuthKeyringBackendKind, + resolved_store: ResolvedOAuthCredentialStore, + reason: StoreResolutionReason, +) { + let result = match find_codex_home() { + Ok(codex_home) => record_store_resolution_in( + &codex_home, + server_name, + url, + store_mode, + keyring_backend, + resolved_store, + ), + Err(error) => Err(error.into()), + }; + + match result { + Ok(Some(change)) => { + warn!( + server_name, + previous_store = change.previous.as_str(), + resolved_store = change.current.as_str(), + resolution_reason = reason.as_str(), + "MCP OAuth Auto storage resolved differently than its previous use" + ); + if let Some(metrics) = codex_otel::global() { + let _ = metrics.counter( + RESOLUTION_CHANGED_METRIC, + /*inc*/ 1, + &[ + ("previous_store", change.previous.as_str()), + ("resolved_store", change.current.as_str()), + ("reason", reason.as_str()), + ], + ); + } + } + Ok(None) => {} + Err(error) => { + warn!( + server_name, + resolution_reason = reason.as_str(), + error = %error, + "failed to update MCP OAuth store resolution diagnostics" + ); + } + } +} + +fn record_store_resolution_in( + codex_home: &Path, + server_name: &str, + url: &str, + store_mode: OAuthCredentialsStoreMode, + keyring_backend: AuthKeyringBackendKind, + resolved_store: ResolvedOAuthCredentialStore, +) -> Result> { + // This short, best-effort lock serializes only the diagnostic map. It must not make an OAuth + // operation wait behind the 60-second credential-store lock budget. + let _lock = OAuthStoreLock::acquire_in( + codex_home, + OAuthStore::ResolutionState, + RESOLUTION_STATE_LOCK_TIMEOUT, + )?; + let path = codex_home.join(RESOLUTION_STATE_FILENAME); + let mut state = read_resolution_state(&path)?; + anyhow::ensure!( + state.version == RESOLUTION_STATE_VERSION, + "unsupported MCP OAuth store resolution state version {}", + state.version + ); + + let store_key = compute_store_key(server_name, url)?; + let current = StoreResolution { + store_mode, + keyring_backend, + resolved_store: resolved_store.into(), + }; + let previous = state.resolutions.get(&store_key).copied(); + if previous == Some(current) { + return Ok(None); + } + + state.resolutions.insert(store_key, current); + let serialized = serde_json::to_string(&state) + .context("failed to serialize MCP OAuth store resolution diagnostics")?; + write_atomically(&path, &serialized).with_context(|| { + format!( + "failed to write MCP OAuth store resolution diagnostics at {}", + path.display() + ) + })?; + + // Explicit File/Keyring configuration and keyring-backend changes are intentional authority + // changes, so they reset the comparison baseline. Only repeated Auto resolution under the + // same backend indicates the availability drift this diagnostic is meant to surface. + Ok(previous.and_then(|previous| { + (previous.store_mode == OAuthCredentialsStoreMode::Auto + && current.store_mode == OAuthCredentialsStoreMode::Auto + && previous.keyring_backend == current.keyring_backend + && previous.resolved_store != current.resolved_store) + .then_some(StoreResolutionChange { + previous: previous.resolved_store, + current: current.resolved_store, + }) + })) +} + +fn read_resolution_state(path: &Path) -> Result { + let contents = match fs::read_to_string(path) { + Ok(contents) => contents, + Err(error) if error.kind() == ErrorKind::NotFound => return Ok(ResolutionState::default()), + Err(error) => { + return Err(error).with_context(|| { + format!( + "failed to read MCP OAuth store resolution diagnostics at {}", + path.display() + ) + }); + } + }; + serde_json::from_str(&contents).with_context(|| { + format!( + "failed to parse MCP OAuth store resolution diagnostics at {}", + path.display() + ) + }) +} diff --git a/codex-rs/rmcp-client/src/oauth/resolved_store.rs b/codex-rs/rmcp-client/src/oauth/resolved_store.rs index b384cc536047..8dd5126de0ee 100644 --- a/codex-rs/rmcp-client/src/oauth/resolved_store.rs +++ b/codex-rs/rmcp-client/src/oauth/resolved_store.rs @@ -10,13 +10,17 @@ use tracing::warn; use super::StoredOAuthTokens; use super::load_oauth_tokens_from_file; use super::load_oauth_tokens_from_keyring; +use super::resolution_state::StoreResolutionReason; +use super::resolution_state::record_store_resolution; /// Concrete credential store resolved for one MCP OAuth client lifecycle. /// -/// This is intentionally not durable. `Auto` may resolve differently in a later process, but a -/// client that loaded credentials from one store must reread, refresh, persist, and remove only -/// through that store. A mid-lifecycle backend failure is unexpected and must return an error -/// rather than falling back to another possibly stale refresh token. +/// This selection is intentionally not persisted as credential authority. `Auto` may resolve +/// differently in a later process, but a client that loaded credentials from one store must +/// reread, refresh, persist, and remove only through that store. A separate best-effort diagnostic +/// record warns when later Auto resolution changes; it never influences this selection. A +/// mid-lifecycle backend failure is unexpected and must return an error rather than falling back +/// to another possibly stale refresh token. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum ResolvedOAuthCredentialStore { File, @@ -36,64 +40,95 @@ pub(crate) fn resolve_oauth_tokens( store_mode: OAuthCredentialsStoreMode, keyring_backend_kind: AuthKeyringBackendKind, ) -> Result> { - match store_mode { + let (resolved, reason) = match store_mode { OAuthCredentialsStoreMode::Auto => { // Auto remains keyring-first at lifecycle startup. The returned source is then pinned // by the client transport recipe and OAuth persistor so retries, recovery, and // refresh work cannot hot-switch stores. - // TODO(stevenlee): Different processes can still resolve Auto to different stores - // when keyring availability differs. Solving that safely requires durable backend - // selection or reconciliation of legacy entries and is intentionally outside this - // stack. + // Different processes can still resolve Auto to different stores when keyring + // availability differs. We persist only a diagnostic observation so that transition + // emits a warning and metric. Making it authoritative would require durable backend + // selection or reconciliation of legacy entries and remains intentionally out of + // scope. match load_oauth_tokens_from_keyring( keyring_store, keyring_backend_kind, server_name, url, ) { - Ok(Some(tokens)) => Ok(Some(ResolvedOAuthTokens { - tokens, - store: ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind), - })), - Ok(None) => Ok( + Ok(Some(tokens)) => ( + Some(ResolvedOAuthTokens { + tokens, + store: ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind), + }), + StoreResolutionReason::AutoLoadFromKeyring, + ), + Ok(None) => ( load_oauth_tokens_from_file(server_name, url)?.map(|tokens| { ResolvedOAuthTokens { tokens, store: ResolvedOAuthCredentialStore::File, } }), + StoreResolutionReason::AutoLoadFromFileAfterMissingKeyring, ), Err(error) => { warn!("failed to read OAuth tokens from keyring: {error}"); - Ok(load_oauth_tokens_from_file(server_name, url) - .with_context(|| { - format!("failed to read OAuth tokens from keyring: {error}") - })? - .map(|tokens| ResolvedOAuthTokens { - tokens, - store: ResolvedOAuthCredentialStore::File, - })) + ( + load_oauth_tokens_from_file(server_name, url) + .with_context(|| { + format!("failed to read OAuth tokens from keyring: {error}") + })? + .map(|tokens| ResolvedOAuthTokens { + tokens, + store: ResolvedOAuthCredentialStore::File, + }), + StoreResolutionReason::AutoLoadFromFileAfterKeyringError, + ) } } } - OAuthCredentialsStoreMode::File => Ok(load_oauth_tokens_from_file(server_name, url)?.map( - |tokens| ResolvedOAuthTokens { + OAuthCredentialsStoreMode::File => ( + load_oauth_tokens_from_file(server_name, url)?.map(|tokens| ResolvedOAuthTokens { tokens, store: ResolvedOAuthCredentialStore::File, - }, - )), - OAuthCredentialsStoreMode::Keyring => Ok(load_oauth_tokens_from_keyring( - keyring_store, - keyring_backend_kind, + }), + StoreResolutionReason::ConfiguredLoad, + ), + OAuthCredentialsStoreMode::Keyring => ( + load_oauth_tokens_from_keyring(keyring_store, keyring_backend_kind, server_name, url) + .with_context(|| "failed to read OAuth tokens from keyring".to_string())? + .map(|tokens| ResolvedOAuthTokens { + tokens, + store: ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind), + }), + StoreResolutionReason::ConfiguredLoad, + ), + }; + + // Explicit modes select their store even when no credential exists. Auto has no concrete + // selection until one store returns credentials or login successfully persists them. + let resolved_store = resolved + .as_ref() + .map(|resolved| resolved.store) + .or(match store_mode { + OAuthCredentialsStoreMode::Auto => None, + OAuthCredentialsStoreMode::File => Some(ResolvedOAuthCredentialStore::File), + OAuthCredentialsStoreMode::Keyring => { + Some(ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind)) + } + }); + if let Some(resolved_store) = resolved_store { + record_store_resolution( server_name, url, - ) - .with_context(|| "failed to read OAuth tokens from keyring".to_string())? - .map(|tokens| ResolvedOAuthTokens { - tokens, - store: ResolvedOAuthCredentialStore::Keyring(keyring_backend_kind), - })), + store_mode, + keyring_backend_kind, + resolved_store, + reason, + ); } + Ok(resolved) } pub(crate) fn load_oauth_tokens_from_store( diff --git a/codex-rs/rmcp-client/src/oauth/store_lock.rs b/codex-rs/rmcp-client/src/oauth/store_lock.rs new file mode 100644 index 000000000000..35b8db369486 --- /dev/null +++ b/codex-rs/rmcp-client/src/oauth/store_lock.rs @@ -0,0 +1,132 @@ +//! Cross-process serialization for MCP OAuth state shared by multiple credentials. +//! +//! The File and Secrets backends and the diagnostic resolution state each store a map containing +//! entries for multiple MCP servers. Their lock therefore protects the complete read-modify-write +//! operation, independently of the per-credential refresh transaction lock in `refresh_lock`. + +use std::fs; +use std::fs::File; +use std::fs::OpenOptions; +use std::path::Path; +use std::path::PathBuf; +use std::time::Duration; +use std::time::Instant; + +use anyhow::Context; +use anyhow::Result; +use codex_utils_home_dir::find_codex_home; + +const OAUTH_STORE_LOCK_DIR: &str = "mcp-oauth-refresh-locks"; +const STORE_LOCK_ACQUIRE_TIMEOUT: Duration = Duration::from_secs(60); +const STORE_LOCK_RETRY_SLEEP: Duration = Duration::from_millis(50); +// Keep this internal target stable so diagnostics and tests can distinguish actual WouldBlock +// contention from a worker that was merely descheduled before attempting the store operation. +const LOCK_CONTENTION_EVENT_TARGET: &str = "codex_rmcp_client::oauth::store_lock::contention"; + +#[derive(Clone, Copy)] +pub(super) enum OAuthStore { + File, + Secrets, + ResolutionState, +} + +impl OAuthStore { + fn lock_filename(self) -> &'static str { + match self { + Self::File => "file-store.lock", + Self::Secrets => "secrets-store.lock", + Self::ResolutionState => "resolution-state.lock", + } + } + + fn description(self) -> &'static str { + match self { + Self::File => "fallback file", + Self::Secrets => "encrypted secrets", + Self::ResolutionState => "store resolution diagnostics", + } + } +} + +/// Serializes access to stores that aggregate credentials for multiple MCP servers. +/// +/// A per-credential transaction lock may be acquired before this lock. Store operations must not +/// acquire a credential lock, and cross-store cleanup must happen after releasing the first store +/// lock. This ordering prevents deadlocks while keeping each aggregate read-modify-write atomic. +pub(super) struct OAuthStoreLock { + _file: File, +} + +impl OAuthStoreLock { + pub(super) fn acquire(store: OAuthStore) -> Result { + let codex_home = find_codex_home()?; + Self::acquire_in(&codex_home, store, STORE_LOCK_ACQUIRE_TIMEOUT) + } + + pub(super) fn acquire_in( + codex_home: &Path, + store: OAuthStore, + acquire_timeout: Duration, + ) -> Result { + let path = oauth_store_lock_path(codex_home, store); + if let Some(parent) = path.parent() { + fs::create_dir_all(parent)?; + } + + let file = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .open(&path) + .with_context(|| { + format!( + "failed to open MCP OAuth {} store lock {}", + store.description(), + path.display() + ) + })?; + let started = Instant::now(); + let mut reported_contention = false; + + loop { + match file.try_lock() { + Ok(()) => return Ok(Self { _file: file }), + Err(std::fs::TryLockError::WouldBlock) if started.elapsed() >= acquire_timeout => { + anyhow::bail!( + "timed out after {acquire_timeout:?} waiting for MCP OAuth {} store lock {}", + store.description(), + path.display() + ); + } + Err(std::fs::TryLockError::WouldBlock) => { + if !reported_contention { + tracing::debug!( + target: LOCK_CONTENTION_EVENT_TARGET, + store = store.description(), + lock_path = %path.display(), + "waiting for another process to finish updating MCP OAuth store state" + ); + reported_contention = true; + } + std::thread::sleep(STORE_LOCK_RETRY_SLEEP.min(acquire_timeout)); + } + Err(error) => { + return Err(std::io::Error::from(error)).with_context(|| { + format!( + "failed to lock MCP OAuth {} store lock {}", + store.description(), + path.display() + ) + }); + } + } + } + } +} + +fn oauth_store_lock_path(codex_home: &Path, store: OAuthStore) -> PathBuf { + codex_home + .join(OAUTH_STORE_LOCK_DIR) + .join(store.lock_filename()) +}