From f1e1c1711af68eb89cc5d8a82bc3cfb5db4e0063 Mon Sep 17 00:00:00 2001 From: Steven Lee Date: Fri, 26 Jun 2026 20:03:26 +0000 Subject: [PATCH 1/7] Serialize shared MCP OAuth stores --- codex-rs/rmcp-client/Cargo.toml | 5 + codex-rs/rmcp-client/src/oauth.rs | 68 +++- codex-rs/rmcp-client/src/oauth/store_lock.rs | 139 ++++++++ .../src/oauth/tests/store_lock_tests.rs | 301 ++++++++++++++++++ 4 files changed, 499 insertions(+), 14 deletions(-) create mode 100644 codex-rs/rmcp-client/src/oauth/store_lock.rs create mode 100644 codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs diff --git a/codex-rs/rmcp-client/Cargo.toml b/codex-rs/rmcp-client/Cargo.toml index 517f7270bbe0..ac5ab05dd215 100644 --- a/codex-rs/rmcp-client/Cargo.toml +++ b/codex-rs/rmcp-client/Cargo.toml @@ -84,5 +84,10 @@ keyring = { workspace = true, features = ["windows-native"] } [target.'cfg(any(target_os = "freebsd", target_os = "openbsd"))'.dependencies] keyring = { workspace = true, features = ["sync-secret-service"] } +# This test is compiled through `#[path]` inside the inline `oauth::tests` module. Cargo-shear +# cannot resolve that nested module path and otherwise reports the linked file as unlinked. +[package.metadata.cargo-shear] +ignored-paths = ["src/oauth/tests/store_lock_tests.rs"] + [lib] doctest = false diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index d0005d20a6d0..b3bd3b4cc450 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -16,6 +16,8 @@ //! //! If the keyring is not available or fails, we fall back to CODEX_HOME/.credentials.json which is consistent with other coding CLI agents. +mod store_lock; + use anyhow::Context; use anyhow::Error; use anyhow::Result; @@ -49,6 +51,10 @@ use std::time::SystemTime; use std::time::UNIX_EPOCH; use tracing::warn; +use self::store_lock::OAuthStore; +use self::store_lock::OAuthStoreLock; +use self::store_lock::OAuthStoreLockFailure; + use codex_keyring_store::DefaultKeyringStore; use codex_keyring_store::KeyringStore; use rmcp::transport::auth::AuthorizationManager; @@ -220,6 +226,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(), @@ -314,6 +321,29 @@ 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, + )?; + } + + let key = compute_store_key(server_name, &tokens.url)?; + if let Err(error) = delete_oauth_tokens_from_file(&key) { + warn!("failed to remove OAuth tokens from fallback storage: {error:?}"); + } + Ok(()) +} + +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(), @@ -323,14 +353,8 @@ fn save_oauth_tokens_to_secrets_keyring( ); let secret_name = compute_secret_name(server_name, &tokens.url)?; manager - .set(&SecretScope::Global, &secret_name, &serialized) - .context("failed to write OAuth tokens to encrypted storage")?; - - let key = compute_store_key(server_name, &tokens.url)?; - if let Err(error) = delete_oauth_tokens_from_file(&key) { - warn!("failed to remove OAuth tokens from fallback storage: {error:?}"); - } - Ok(()) + .set(&SecretScope::Global, &secret_name, serialized) + .context("failed to write OAuth tokens to encrypted storage") } fn save_oauth_tokens_with_keyring_with_fallback_to_file( @@ -341,6 +365,11 @@ fn save_oauth_tokens_with_keyring_with_fallback_to_file Result<()> { match save_oauth_tokens_with_keyring(keyring_store, keyring_backend_kind, server_name, tokens) { Ok(()) => Ok(()), + // A store lock failure means another process may be updating the selected aggregate + // authority, or that coordination itself is unavailable. It is not evidence that the + // keyring backend is unavailable, so treating it as a File-fallback signal could leave a + // newer fallback token hidden behind a stale Secrets entry. + Err(error) if error.downcast_ref::().is_some() => Err(error), Err(error) => { let message = error.to_string(); warn!("falling back to file storage for OAuth tokens: {message}"); @@ -430,6 +459,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(), @@ -592,7 +622,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); }; @@ -635,8 +666,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 @@ -664,7 +700,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), }; @@ -750,7 +787,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, @@ -826,6 +863,9 @@ mod tests { use codex_keyring_store::tests::MockKeyringStore; + #[path = "store_lock_tests.rs"] + mod store_lock_tests; + struct TempCodexHome { _guard: MutexGuard<'static, ()>, _dir: tempfile::TempDir, @@ -964,7 +1004,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); @@ -1076,7 +1116,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/store_lock.rs b/codex-rs/rmcp-client/src/oauth/store_lock.rs new file mode 100644 index 000000000000..5c38277052c0 --- /dev/null +++ b/codex-rs/rmcp-client/src/oauth/store_lock.rs @@ -0,0 +1,139 @@ +//! Cross-process serialization for MCP OAuth stores shared by multiple credentials. +//! +//! File and Secrets each keep credentials for multiple MCP servers in one aggregate document. +//! Their lock therefore protects the complete read-modify-write operation. Direct keyring entries +//! are already stored independently per credential and do not use this 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_LOCK_DIR: &str = "mcp-oauth-locks"; +const STORE_LOCK_ACQUIRE_TIMEOUT: Duration = Duration::from_secs(60); +const STORE_LOCK_RETRY_SLEEP: Duration = Duration::from_millis(50); +// Tests listen for this event so they prove a contender reached the real WouldBlock branch. +const LOCK_CONTENTION_EVENT_TARGET: &str = "codex_rmcp_client::oauth::store_lock::contention"; + +/// Marks aggregate-store coordination failures in an [`anyhow::Error`] chain. +/// +/// Auto may fall back when the configured keyring backend is unavailable, but it must surface a +/// lock failure. Falling back while another process owns the aggregate-store lock could leave the +/// newer credential in File while a stale Secrets entry remains preferred. +#[derive(Debug, thiserror::Error)] +#[error("failed to acquire MCP OAuth {store} aggregate-store lock")] +pub(super) struct OAuthStoreLockFailure { + store: &'static str, +} + +#[derive(Clone, Copy)] +pub(super) enum OAuthStore { + File, + Secrets, +} + +impl OAuthStore { + fn lock_filename(self) -> &'static str { + match self { + Self::File => "file-store.lock", + Self::Secrets => "secrets-store.lock", + } + } + + fn description(self) -> &'static str { + match self { + Self::File => "fallback file", + Self::Secrets => "encrypted secrets", + } + } +} + +/// Serializes one complete operation on an aggregate OAuth credential store. +pub(super) struct OAuthStoreLock { + _file: File, +} + +impl OAuthStoreLock { + pub(super) fn acquire(store: OAuthStore) -> Result { + let codex_home = find_codex_home().context(OAuthStoreLockFailure { + store: store.description(), + })?; + Self::acquire_in(&codex_home, store, STORE_LOCK_ACQUIRE_TIMEOUT).context( + OAuthStoreLockFailure { + store: store.description(), + }, + ) + } + + 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_LOCK_DIR).join(store.lock_filename()) +} diff --git a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs new file mode 100644 index 000000000000..2368c0809fbd --- /dev/null +++ b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs @@ -0,0 +1,301 @@ +use std::sync::mpsc; +use std::time::Duration; + +use anyhow::Result; +use tracing::Event; +use tracing::Id; +use tracing::Metadata; +use tracing::Subscriber; +use tracing::span::Attributes; +use tracing::span::Record; +use tracing::subscriber::Interest; + +use super::MockKeyringStore; +use super::TempCodexHome; +use super::assert_tokens_match_without_expiry; +use super::sample_tokens; +use crate::oauth::OAuthStore; +use crate::oauth::OAuthStoreLock; +use crate::oauth::OAuthStoreLockFailure; +use crate::oauth::fallback_file_path; +use crate::oauth::load_oauth_tokens_from_file; +use crate::oauth::load_oauth_tokens_from_keyring; +use crate::oauth::save_oauth_tokens_to_file; +use crate::oauth::save_oauth_tokens_to_file_unlocked; +use crate::oauth::save_oauth_tokens_to_secrets_keyring_unlocked; +use crate::oauth::save_oauth_tokens_with_keyring; +use crate::oauth::save_oauth_tokens_with_keyring_with_fallback_to_file; +use codex_config::types::AuthKeyringBackendKind; + +const STORE_LOCK_CONTENTION_EVENT_TARGET: &str = "codex_rmcp_client::oauth::store_lock::contention"; + +#[test] +fn auto_secrets_lock_failure_does_not_fall_back_to_file() -> Result<()> { + let env = TempCodexHome::new(); + // Make only the lock directory invalid. The fallback file remains writable, so this test + // would succeed and leave File credentials behind if Auto mistook coordination failure for + // keyring unavailability. + std::fs::write(env.path().join("mcp-oauth-locks"), "not a directory")?; + let keyring_store = MockKeyringStore::default(); + let tokens = sample_tokens(); + + let error = save_oauth_tokens_with_keyring_with_fallback_to_file( + &keyring_store, + AuthKeyringBackendKind::Secrets, + &tokens.server_name, + &tokens, + ) + .expect_err("aggregate-store lock failure must abort Auto persistence"); + + assert!(error.downcast_ref::().is_some()); + assert!(!fallback_file_path()?.exists()); + Ok(()) +} + +struct LockContentionSubscriber { + contended_tx: mpsc::Sender<()>, +} + +impl Subscriber for LockContentionSubscriber { + fn enabled(&self, metadata: &Metadata<'_>) -> bool { + metadata.target() == STORE_LOCK_CONTENTION_EVENT_TARGET + } + + fn register_callsite(&self, metadata: &'static Metadata<'static>) -> Interest { + if self.enabled(metadata) { + Interest::always() + } else { + Interest::never() + } + } + + fn max_level_hint(&self) -> Option { + Some(tracing::level_filters::LevelFilter::DEBUG) + } + + fn new_span(&self, _span: &Attributes<'_>) -> Id { + Id::from_u64(/*u*/ 1) + } + + fn record(&self, _span: &Id, _values: &Record<'_>) {} + + fn record_follows_from(&self, _span: &Id, _follows_from: &Id) {} + + fn event(&self, event: &Event<'_>) { + if self.enabled(event.metadata()) { + self.contended_tx + .send(()) + .expect("signal actual OAuth store lock contention"); + } + } + + fn enter(&self, _span: &Id) {} + + fn exit(&self, _span: &Id) {} +} + +fn complete_after_store_lock_contention( + codex_home: &std::path::Path, + store: OAuthStore, + operation: impl FnOnce() -> Result + Send + 'static, +) -> Result +where + T: Send + 'static, +{ + let held_lock = + OAuthStoreLock::acquire_in(codex_home, store, Duration::from_millis(/*millis*/ 100))?; + let (contended_tx, contended_rx) = mpsc::channel(); + let (result_tx, result_rx) = mpsc::channel(); + let worker = std::thread::spawn(move || { + tracing::subscriber::with_default(LockContentionSubscriber { contended_tx }, || { + result_tx + .send(operation()) + .expect("send contending OAuth store operation result"); + }); + }); + + // This event is emitted only after `try_lock()` returns WouldBlock, so the test fails if the + // operation stops acquiring the aggregate-store lock. + contended_rx.recv_timeout(Duration::from_secs(/*secs*/ 1))?; + drop(held_lock); + let result = result_rx.recv_timeout(Duration::from_secs(/*secs*/ 10))??; + worker + .join() + .expect("contending OAuth store worker should finish"); + Ok(result) +} + +#[test] +fn file_store_lock_preserves_updates_for_different_servers() -> Result<()> { + let env = TempCodexHome::new(); + let first = sample_tokens(); + let mut second = sample_tokens(); + second.server_name = "second-server".to_string(); + second.url = "https://second.example.test".to_string(); + + let held_lock = OAuthStoreLock::acquire_in( + env.path(), + OAuthStore::File, + Duration::from_millis(/*millis*/ 100), + )?; + let (contended_tx, contended_rx) = mpsc::channel(); + let (result_tx, result_rx) = mpsc::channel(); + let second_for_writer = second.clone(); + let writer = std::thread::spawn(move || { + tracing::subscriber::with_default(LockContentionSubscriber { contended_tx }, || { + result_tx + .send(save_oauth_tokens_to_file(&second_for_writer)) + .expect("send writer result"); + }); + }); + + contended_rx.recv_timeout(Duration::from_secs(/*secs*/ 1))?; + save_oauth_tokens_to_file_unlocked(&first)?; + drop(held_lock); + result_rx.recv_timeout(Duration::from_secs(/*secs*/ 10))??; + writer.join().expect("file store writer should finish"); + + let loaded_first = load_oauth_tokens_from_file(&first.server_name, &first.url)? + .expect("first server tokens should remain stored"); + let loaded_second = load_oauth_tokens_from_file(&second.server_name, &second.url)? + .expect("second server tokens should be stored"); + assert_tokens_match_without_expiry(&loaded_first, &first); + assert_tokens_match_without_expiry(&loaded_second, &second); + Ok(()) +} + +#[test] +fn file_store_load_and_delete_observe_aggregate_lock() -> Result<()> { + let env = TempCodexHome::new(); + let tokens = sample_tokens(); + save_oauth_tokens_to_file(&tokens)?; + + let server_name = tokens.server_name.clone(); + let url = tokens.url.clone(); + let loaded = complete_after_store_lock_contention(env.path(), OAuthStore::File, move || { + load_oauth_tokens_from_file(&server_name, &url) + })? + .expect("file credentials should remain readable after contention"); + assert_tokens_match_without_expiry(&loaded, &tokens); + + let key = crate::oauth::compute_store_key(&tokens.server_name, &tokens.url)?; + let removed = complete_after_store_lock_contention(env.path(), OAuthStore::File, move || { + crate::oauth::delete_oauth_tokens_from_file(&key) + })?; + assert!(removed); + assert!(load_oauth_tokens_from_file(&tokens.server_name, &tokens.url)?.is_none()); + Ok(()) +} + +#[test] +fn secrets_store_lock_preserves_updates_for_different_servers() -> Result<()> { + let env = TempCodexHome::new(); + let keyring_store = MockKeyringStore::default(); + let first = sample_tokens(); + let mut second = sample_tokens(); + second.server_name = "second-server".to_string(); + second.url = "https://second.example.test".to_string(); + + let held_lock = OAuthStoreLock::acquire_in( + env.path(), + OAuthStore::Secrets, + Duration::from_millis(/*millis*/ 100), + )?; + let (contended_tx, contended_rx) = mpsc::channel(); + let (result_tx, result_rx) = mpsc::channel(); + let store_for_writer = keyring_store.clone(); + let second_for_writer = second.clone(); + let writer = std::thread::spawn(move || { + tracing::subscriber::with_default(LockContentionSubscriber { contended_tx }, || { + result_tx + .send(save_oauth_tokens_with_keyring( + &store_for_writer, + AuthKeyringBackendKind::Secrets, + &second_for_writer.server_name, + &second_for_writer, + )) + .expect("send writer result"); + }); + }); + + contended_rx.recv_timeout(Duration::from_secs(/*secs*/ 1))?; + let first_serialized = serde_json::to_string(&first)?; + save_oauth_tokens_to_secrets_keyring_unlocked( + &keyring_store, + &first.server_name, + &first, + &first_serialized, + )?; + drop(held_lock); + result_rx.recv_timeout(Duration::from_secs(/*secs*/ 10))??; + writer.join().expect("secrets store writer should finish"); + + let loaded_first = load_oauth_tokens_from_keyring( + &keyring_store, + AuthKeyringBackendKind::Secrets, + &first.server_name, + &first.url, + )? + .expect("first server tokens should remain stored"); + let loaded_second = load_oauth_tokens_from_keyring( + &keyring_store, + AuthKeyringBackendKind::Secrets, + &second.server_name, + &second.url, + )? + .expect("second server tokens should be stored"); + assert_tokens_match_without_expiry(&loaded_first, &first); + assert_tokens_match_without_expiry(&loaded_second, &second); + Ok(()) +} + +#[test] +fn secrets_store_load_and_delete_observe_aggregate_lock() -> Result<()> { + let env = TempCodexHome::new(); + let keyring_store = MockKeyringStore::default(); + let tokens = sample_tokens(); + save_oauth_tokens_with_keyring( + &keyring_store, + AuthKeyringBackendKind::Secrets, + &tokens.server_name, + &tokens, + )?; + + let store_for_load = keyring_store.clone(); + let server_name = tokens.server_name.clone(); + let url = tokens.url.clone(); + let loaded = + complete_after_store_lock_contention(env.path(), OAuthStore::Secrets, move || { + load_oauth_tokens_from_keyring( + &store_for_load, + AuthKeyringBackendKind::Secrets, + &server_name, + &url, + ) + })? + .expect("encrypted credentials should remain readable after contention"); + assert_tokens_match_without_expiry(&loaded, &tokens); + + let store_for_delete = keyring_store.clone(); + let server_name = tokens.server_name.clone(); + let url = tokens.url.clone(); + let removed = + complete_after_store_lock_contention(env.path(), OAuthStore::Secrets, move || { + crate::oauth::delete_oauth_tokens_from_secrets_keyring( + &store_for_delete, + &server_name, + &url, + ) + })?; + assert!(removed); + assert!( + load_oauth_tokens_from_keyring( + &keyring_store, + AuthKeyringBackendKind::Secrets, + &tokens.server_name, + &tokens.url, + )? + .is_none() + ); + Ok(()) +} From 8f1d2f1ab61bca589f2982886e344a2424bbba72 Mon Sep 17 00:00:00 2001 From: Steven Lee Date: Fri, 26 Jun 2026 22:49:46 +0000 Subject: [PATCH 2/7] Reject Auto fallback on OAuth store lock failures --- codex-rs/rmcp-client/src/oauth.rs | 12 ++++-- .../src/oauth/tests/store_lock_tests.rs | 41 ++++++++++++++++--- 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index b3bd3b4cc450..9a66d0f602c5 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -179,6 +179,11 @@ fn load_oauth_tokens_from_keyring_with_fallback_to_file Ok(Some(tokens)), Ok(None) => load_oauth_tokens_from_file(server_name, url), + // A store lock failure means the configured aggregate authority could be changing, or + // that coordination itself is unavailable. It is not evidence that the keyring backend + // is unavailable, so consulting File here could replay credentials hidden behind a + // newer Secrets entry. This is the load-side counterpart of the save guard below. + Err(error) if error.downcast_ref::().is_some() => Err(error), Err(error) => { warn!("failed to read OAuth tokens from keyring: {error}"); load_oauth_tokens_from_file(server_name, url) @@ -365,10 +370,9 @@ fn save_oauth_tokens_with_keyring_with_fallback_to_file Result<()> { match save_oauth_tokens_with_keyring(keyring_store, keyring_backend_kind, server_name, tokens) { Ok(()) => Ok(()), - // A store lock failure means another process may be updating the selected aggregate - // authority, or that coordination itself is unavailable. It is not evidence that the - // keyring backend is unavailable, so treating it as a File-fallback signal could leave a - // newer fallback token hidden behind a stale Secrets entry. + // As on load, a store lock failure is a coordination failure rather than evidence that + // the keyring backend is unavailable. Falling back could leave a newer File token hidden + // behind a stale Secrets entry. Err(error) if error.downcast_ref::().is_some() => Err(error), Err(error) => { let message = error.to_string(); diff --git a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs index 2368c0809fbd..edfff1171196 100644 --- a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs +++ b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs @@ -20,6 +20,7 @@ use crate::oauth::OAuthStoreLockFailure; use crate::oauth::fallback_file_path; use crate::oauth::load_oauth_tokens_from_file; use crate::oauth::load_oauth_tokens_from_keyring; +use crate::oauth::load_oauth_tokens_from_keyring_with_fallback_to_file; use crate::oauth::save_oauth_tokens_to_file; use crate::oauth::save_oauth_tokens_to_file_unlocked; use crate::oauth::save_oauth_tokens_to_secrets_keyring_unlocked; @@ -30,12 +31,14 @@ use codex_config::types::AuthKeyringBackendKind; const STORE_LOCK_CONTENTION_EVENT_TARGET: &str = "codex_rmcp_client::oauth::store_lock::contention"; #[test] -fn auto_secrets_lock_failure_does_not_fall_back_to_file() -> Result<()> { +fn auto_save_secrets_lock_failure_does_not_fall_back_to_file() -> Result<()> { let env = TempCodexHome::new(); - // Make only the lock directory invalid. The fallback file remains writable, so this test - // would succeed and leave File credentials behind if Auto mistook coordination failure for - // keyring unavailability. - std::fs::write(env.path().join("mcp-oauth-locks"), "not a directory")?; + let lock_dir = env.path().join("mcp-oauth-locks"); + std::fs::create_dir_all(&lock_dir)?; + // Break only the Secrets lock path. The distinct File lock remains usable, so Auto would + // successfully write fallback credentials if it mistook coordination failure for backend + // unavailability. + std::fs::create_dir(lock_dir.join("secrets-store.lock"))?; let keyring_store = MockKeyringStore::default(); let tokens = sample_tokens(); @@ -49,6 +52,34 @@ fn auto_secrets_lock_failure_does_not_fall_back_to_file() -> Result<()> { assert!(error.downcast_ref::().is_some()); assert!(!fallback_file_path()?.exists()); + save_oauth_tokens_to_file(&tokens)?; + let loaded = load_oauth_tokens_from_file(&tokens.server_name, &tokens.url)? + .expect("fallback File should remain independently writable"); + assert_tokens_match_without_expiry(&loaded, &tokens); + Ok(()) +} + +#[test] +fn auto_load_secrets_lock_failure_does_not_fall_back_to_file() -> Result<()> { + let env = TempCodexHome::new(); + let keyring_store = MockKeyringStore::default(); + let tokens = sample_tokens(); + save_oauth_tokens_to_file(&tokens)?; + + let lock_dir = env.path().join("mcp-oauth-locks"); + std::fs::create_dir(lock_dir.join("secrets-store.lock"))?; + let error = load_oauth_tokens_from_keyring_with_fallback_to_file( + &keyring_store, + AuthKeyringBackendKind::Secrets, + &tokens.server_name, + &tokens.url, + ) + .expect_err("aggregate-store lock failure must abort Auto resolution"); + + assert!(error.downcast_ref::().is_some()); + let loaded = load_oauth_tokens_from_file(&tokens.server_name, &tokens.url)? + .expect("fallback File should remain independently readable"); + assert_tokens_match_without_expiry(&loaded, &tokens); Ok(()) } From b5f1164e5fea075af5f329b4c709302c3c6d5940 Mon Sep 17 00:00:00 2001 From: Steven Lee Date: Sat, 27 Jun 2026 17:50:46 +0000 Subject: [PATCH 3/7] Clarify MCP OAuth store lock preconditions --- codex-rs/rmcp-client/src/oauth.rs | 15 +++++++++++---- .../src/oauth/tests/store_lock_tests.rs | 8 ++++---- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index 9a66d0f602c5..7478a593a4c0 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -320,6 +320,9 @@ fn save_oauth_tokens_to_direct_keyring( } } +/// Saves one credential while holding the Secrets aggregate-store lock across the mutation. +/// +/// The Secrets lock is released before fallback File cleanup to preserve aggregate-lock ordering. fn save_oauth_tokens_to_secrets_keyring( keyring_store: &K, server_name: &str, @@ -328,7 +331,7 @@ fn save_oauth_tokens_to_secrets_keyring( 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( + save_oauth_tokens_to_secrets_keyring_with_lock_held( keyring_store, server_name, tokens, @@ -343,7 +346,8 @@ fn save_oauth_tokens_to_secrets_keyring( Ok(()) } -fn save_oauth_tokens_to_secrets_keyring_unlocked( +/// Writes one credential to Secrets. The caller must hold the Secrets aggregate-store lock. +fn save_oauth_tokens_to_secrets_keyring_with_lock_held( keyring_store: &K, server_name: &str, tokens: &StoredOAuthTokens, @@ -669,12 +673,15 @@ 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) + save_oauth_tokens_to_file_with_lock_held(tokens) } -fn save_oauth_tokens_to_file_unlocked(tokens: &StoredOAuthTokens) -> Result<()> { +/// Updates the fallback File. The caller must hold the File aggregate-store lock. +fn save_oauth_tokens_to_file_with_lock_held(tokens: &StoredOAuthTokens) -> Result<()> { let key = compute_store_key(&tokens.server_name, &tokens.url)?; let mut store = read_fallback_file_unlocked()?.unwrap_or_default(); diff --git a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs index edfff1171196..1936483a49de 100644 --- a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs +++ b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs @@ -22,8 +22,8 @@ use crate::oauth::load_oauth_tokens_from_file; use crate::oauth::load_oauth_tokens_from_keyring; use crate::oauth::load_oauth_tokens_from_keyring_with_fallback_to_file; use crate::oauth::save_oauth_tokens_to_file; -use crate::oauth::save_oauth_tokens_to_file_unlocked; -use crate::oauth::save_oauth_tokens_to_secrets_keyring_unlocked; +use crate::oauth::save_oauth_tokens_to_file_with_lock_held; +use crate::oauth::save_oauth_tokens_to_secrets_keyring_with_lock_held; use crate::oauth::save_oauth_tokens_with_keyring; use crate::oauth::save_oauth_tokens_with_keyring_with_fallback_to_file; use codex_config::types::AuthKeyringBackendKind; @@ -181,7 +181,7 @@ fn file_store_lock_preserves_updates_for_different_servers() -> Result<()> { }); contended_rx.recv_timeout(Duration::from_secs(/*secs*/ 1))?; - save_oauth_tokens_to_file_unlocked(&first)?; + save_oauth_tokens_to_file_with_lock_held(&first)?; drop(held_lock); result_rx.recv_timeout(Duration::from_secs(/*secs*/ 10))??; writer.join().expect("file store writer should finish"); @@ -251,7 +251,7 @@ fn secrets_store_lock_preserves_updates_for_different_servers() -> Result<()> { contended_rx.recv_timeout(Duration::from_secs(/*secs*/ 1))?; let first_serialized = serde_json::to_string(&first)?; - save_oauth_tokens_to_secrets_keyring_unlocked( + save_oauth_tokens_to_secrets_keyring_with_lock_held( &keyring_store, &first.server_name, &first, From 9b7f3b3abbc350b17b350879be2c575be2c25058 Mon Sep 17 00:00:00 2001 From: Steven Lee Date: Mon, 6 Jul 2026 21:20:06 +0000 Subject: [PATCH 4/7] Address OAuth store lock review comments --- codex-rs/rmcp-client/Cargo.toml | 4 - codex-rs/rmcp-client/src/oauth.rs | 3 - codex-rs/rmcp-client/src/oauth/store_lock.rs | 114 ++++++---- .../src/oauth/tests/store_lock_tests.rs | 204 +++++++++++++++++- 4 files changed, 274 insertions(+), 51 deletions(-) diff --git a/codex-rs/rmcp-client/Cargo.toml b/codex-rs/rmcp-client/Cargo.toml index ac5ab05dd215..0ab5bf076971 100644 --- a/codex-rs/rmcp-client/Cargo.toml +++ b/codex-rs/rmcp-client/Cargo.toml @@ -84,10 +84,6 @@ keyring = { workspace = true, features = ["windows-native"] } [target.'cfg(any(target_os = "freebsd", target_os = "openbsd"))'.dependencies] keyring = { workspace = true, features = ["sync-secret-service"] } -# This test is compiled through `#[path]` inside the inline `oauth::tests` module. Cargo-shear -# cannot resolve that nested module path and otherwise reports the linked file as unlinked. -[package.metadata.cargo-shear] -ignored-paths = ["src/oauth/tests/store_lock_tests.rs"] [lib] doctest = false diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index 7478a593a4c0..ff2e7af32b37 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -874,9 +874,6 @@ mod tests { use codex_keyring_store::tests::MockKeyringStore; - #[path = "store_lock_tests.rs"] - mod store_lock_tests; - struct TempCodexHome { _guard: MutexGuard<'static, ()>, _dir: tempfile::TempDir, diff --git a/codex-rs/rmcp-client/src/oauth/store_lock.rs b/codex-rs/rmcp-client/src/oauth/store_lock.rs index 5c38277052c0..b99858a7f45d 100644 --- a/codex-rs/rmcp-client/src/oauth/store_lock.rs +++ b/codex-rs/rmcp-client/src/oauth/store_lock.rs @@ -7,12 +7,12 @@ use std::fs; use std::fs::File; use std::fs::OpenOptions; +use std::io; 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; @@ -28,12 +28,46 @@ const LOCK_CONTENTION_EVENT_TARGET: &str = "codex_rmcp_client::oauth::store_lock /// lock failure. Falling back while another process owns the aggregate-store lock could leave the /// newer credential in File while a stale Secrets entry remains preferred. #[derive(Debug, thiserror::Error)] -#[error("failed to acquire MCP OAuth {store} aggregate-store lock")] -pub(super) struct OAuthStoreLockFailure { - store: &'static str, +pub(super) enum OAuthStoreLockFailure { + #[error("failed to resolve CODEX_HOME for MCP OAuth {store} aggregate-store lock")] + CodexHome { + store: OAuthStore, + #[source] + source: io::Error, + }, + #[error("failed to create MCP OAuth {store} aggregate-store lock directory {}", path.display())] + CreateDir { + store: OAuthStore, + path: PathBuf, + #[source] + source: io::Error, + }, + #[error("failed to open MCP OAuth {store} aggregate-store lock {}", path.display())] + Open { + store: OAuthStore, + path: PathBuf, + #[source] + source: io::Error, + }, + #[error( + "timed out after {acquire_timeout:?} waiting for MCP OAuth {store} aggregate-store lock {}", + path.display() + )] + Timeout { + store: OAuthStore, + path: PathBuf, + acquire_timeout: Duration, + }, + #[error("failed to lock MCP OAuth {store} aggregate-store lock {}", path.display())] + Lock { + store: OAuthStore, + path: PathBuf, + #[source] + source: io::Error, + }, } -#[derive(Clone, Copy)] +#[derive(Clone, Copy, Debug)] pub(super) enum OAuthStore { File, Secrets, @@ -46,11 +80,13 @@ impl OAuthStore { Self::Secrets => "secrets-store.lock", } } +} - fn description(self) -> &'static str { +impl std::fmt::Display for OAuthStore { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Self::File => "fallback file", - Self::Secrets => "encrypted secrets", + Self::File => f.write_str("fallback file"), + Self::Secrets => f.write_str("encrypted secrets"), } } } @@ -62,14 +98,14 @@ pub(super) struct OAuthStoreLock { impl OAuthStoreLock { pub(super) fn acquire(store: OAuthStore) -> Result { - let codex_home = find_codex_home().context(OAuthStoreLockFailure { - store: store.description(), - })?; - Self::acquire_in(&codex_home, store, STORE_LOCK_ACQUIRE_TIMEOUT).context( - OAuthStoreLockFailure { - store: store.description(), - }, - ) + // This lock intentionally follows the existing local File/Secrets credential-store + // authority. Those stores are CODEX_HOME-backed today: if CODEX_HOME is unset they use + // the default home (`~/.codex`), and if an embedder has no local home/filesystem authority + // those stores already cannot operate. A future provider-backed credential store should + // provide its own matching lock authority instead of using this local path. + let codex_home = find_codex_home() + .map_err(|source| OAuthStoreLockFailure::CodexHome { store, source })?; + Self::acquire_in(&codex_home, store, STORE_LOCK_ACQUIRE_TIMEOUT) } pub(super) fn acquire_in( @@ -79,7 +115,11 @@ impl OAuthStoreLock { ) -> Result { let path = oauth_store_lock_path(codex_home, store); if let Some(parent) = path.parent() { - fs::create_dir_all(parent)?; + fs::create_dir_all(parent).map_err(|source| OAuthStoreLockFailure::CreateDir { + store, + path: parent.to_path_buf(), + source, + })?; } let file = OpenOptions::new() @@ -88,12 +128,10 @@ impl OAuthStoreLock { .create(true) .truncate(false) .open(&path) - .with_context(|| { - format!( - "failed to open MCP OAuth {} store lock {}", - store.description(), - path.display() - ) + .map_err(|source| OAuthStoreLockFailure::Open { + store, + path: path.clone(), + source, })?; let started = Instant::now(); let mut reported_contention = false; @@ -102,17 +140,18 @@ impl OAuthStoreLock { 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() - ); + return Err(OAuthStoreLockFailure::Timeout { + store, + path, + acquire_timeout, + } + .into()); } Err(std::fs::TryLockError::WouldBlock) => { if !reported_contention { tracing::debug!( target: LOCK_CONTENTION_EVENT_TARGET, - store = store.description(), + store = %store, lock_path = %path.display(), "waiting for another process to finish updating MCP OAuth store state" ); @@ -121,13 +160,12 @@ impl OAuthStoreLock { 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() - ) - }); + return Err(OAuthStoreLockFailure::Lock { + store, + path, + source: io::Error::from(error), + } + .into()); } } } @@ -137,3 +175,7 @@ impl OAuthStoreLock { fn oauth_store_lock_path(codex_home: &Path, store: OAuthStore) -> PathBuf { codex_home.join(OAUTH_LOCK_DIR).join(store.lock_filename()) } + +#[cfg(test)] +#[path = "tests/store_lock_tests.rs"] +mod tests; diff --git a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs index 1936483a49de..d59c8c4f72d3 100644 --- a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs +++ b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs @@ -1,7 +1,26 @@ +use std::path::Path; +use std::process::Command; +use std::sync::Mutex; +use std::sync::MutexGuard; +use std::sync::OnceLock; +use std::sync::PoisonError; use std::sync::mpsc; use std::time::Duration; +use std::time::Instant; +use anyhow::Context; use anyhow::Result; +use codex_config::types::AuthKeyringBackendKind; +use codex_keyring_store::tests::MockKeyringStore; +use oauth2::AccessToken; +use oauth2::RefreshToken; +use oauth2::Scope; +use oauth2::TokenResponse; +use oauth2::basic::BasicTokenType; +use pretty_assertions::assert_eq; +use rmcp::transport::auth::OAuthTokenResponse; +use rmcp::transport::auth::VendorExtraTokenFields; +use tempfile::tempdir; use tracing::Event; use tracing::Id; use tracing::Metadata; @@ -10,13 +29,11 @@ use tracing::span::Attributes; use tracing::span::Record; use tracing::subscriber::Interest; -use super::MockKeyringStore; -use super::TempCodexHome; -use super::assert_tokens_match_without_expiry; -use super::sample_tokens; -use crate::oauth::OAuthStore; -use crate::oauth::OAuthStoreLock; -use crate::oauth::OAuthStoreLockFailure; +use super::OAuthStore; +use super::OAuthStoreLock; +use super::OAuthStoreLockFailure; +use crate::oauth::StoredOAuthTokens; +use crate::oauth::WrappedOAuthTokenResponse; use crate::oauth::fallback_file_path; use crate::oauth::load_oauth_tokens_from_file; use crate::oauth::load_oauth_tokens_from_keyring; @@ -26,10 +43,181 @@ use crate::oauth::save_oauth_tokens_to_file_with_lock_held; use crate::oauth::save_oauth_tokens_to_secrets_keyring_with_lock_held; use crate::oauth::save_oauth_tokens_with_keyring; use crate::oauth::save_oauth_tokens_with_keyring_with_fallback_to_file; -use codex_config::types::AuthKeyringBackendKind; const STORE_LOCK_CONTENTION_EVENT_TARGET: &str = "codex_rmcp_client::oauth::store_lock::contention"; +struct TempCodexHome { + _guard: MutexGuard<'static, ()>, + _dir: tempfile::TempDir, +} + +impl TempCodexHome { + fn new() -> Self { + static LOCK: OnceLock> = OnceLock::new(); + let guard = LOCK + .get_or_init(Mutex::default) + .lock() + .unwrap_or_else(PoisonError::into_inner); + let dir = tempdir().expect("create CODEX_HOME temp dir"); + unsafe { + std::env::set_var("CODEX_HOME", dir.path()); + } + Self { + _guard: guard, + _dir: dir, + } + } + + fn path(&self) -> &Path { + self._dir.path() + } +} + +impl Drop for TempCodexHome { + fn drop(&mut self) { + unsafe { + std::env::remove_var("CODEX_HOME"); + } + } +} + +fn assert_tokens_match_without_expiry(actual: &StoredOAuthTokens, expected: &StoredOAuthTokens) { + assert_eq!(actual.server_name, expected.server_name); + assert_eq!(actual.url, expected.url); + assert_eq!(actual.client_id, expected.client_id); + assert_eq!(actual.expires_at, expected.expires_at); + assert_token_response_match_without_expiry(&actual.token_response, &expected.token_response); +} + +fn assert_token_response_match_without_expiry( + actual: &WrappedOAuthTokenResponse, + expected: &WrappedOAuthTokenResponse, +) { + let actual_response = &actual.0; + let expected_response = &expected.0; + + assert_eq!( + actual_response.access_token().secret(), + expected_response.access_token().secret() + ); + assert_eq!(actual_response.token_type(), expected_response.token_type()); + assert_eq!( + actual_response.refresh_token().map(RefreshToken::secret), + expected_response.refresh_token().map(RefreshToken::secret), + ); + assert_eq!(actual_response.scopes(), expected_response.scopes()); + assert_eq!( + actual_response.extra_fields().0, + expected_response.extra_fields().0 + ); + assert_eq!( + actual_response.expires_in().is_some(), + expected_response.expires_in().is_some() + ); +} + +fn sample_tokens() -> StoredOAuthTokens { + let mut response = OAuthTokenResponse::new( + AccessToken::new("access-token".to_string()), + BasicTokenType::Bearer, + VendorExtraTokenFields::default(), + ); + response.set_refresh_token(Some(RefreshToken::new("refresh-token".to_string()))); + response.set_scopes(Some(vec![ + Scope::new("scope-a".to_string()), + Scope::new("scope-b".to_string()), + ])); + let expires_in = Duration::from_secs(3600); + response.set_expires_in(Some(&expires_in)); + let expires_at = crate::oauth::compute_expires_at_millis(&response); + + StoredOAuthTokens { + server_name: "test-server".to_string(), + url: "https://example.test".to_string(), + client_id: "client-id".to_string(), + token_response: WrappedOAuthTokenResponse(response), + expires_at, + } +} + +const LOCK_HOLDER_CHILD_TEST: &str = + "oauth::store_lock::tests::store_lock_is_released_when_holder_process_exits_child"; +const LOCK_HOLDER_READY_PATH_ENV: &str = "CODEX_OAUTH_STORE_LOCK_CHILD_READY_PATH"; + +#[test] +fn store_lock_is_released_when_holder_process_exits() -> Result<()> { + let env = TempCodexHome::new(); + let ready_file = env.path().join("lock-holder-ready"); + let mut child = Command::new(std::env::current_exe()?) + .arg("--exact") + .arg(LOCK_HOLDER_CHILD_TEST) + .arg("--ignored") + .env("CODEX_HOME", env.path()) + .env(LOCK_HOLDER_READY_PATH_ENV, &ready_file) + .spawn() + .context("spawn OAuth store lock holder test process")?; + + let test_result = (|| -> Result<()> { + let started = Instant::now(); + while !ready_file.exists() { + if started.elapsed() > Duration::from_secs(/*secs*/ 5) { + anyhow::bail!("timed out waiting for child process to acquire OAuth store lock"); + } + std::thread::sleep(Duration::from_millis(/*millis*/ 20)); + } + + let error = match OAuthStoreLock::acquire_in( + env.path(), + OAuthStore::File, + Duration::from_millis(/*millis*/ 100), + ) { + Ok(_) => { + anyhow::bail!("live holder process should keep the OAuth store lock unavailable") + } + Err(error) => error, + }; + assert!(matches!( + error.downcast_ref::(), + Some(OAuthStoreLockFailure::Timeout { .. }) + )); + + child + .kill() + .context("kill OAuth store lock holder process")?; + let status = child + .wait() + .context("wait for killed OAuth store lock holder process")?; + assert!(!status.success()); + let _lock = OAuthStoreLock::acquire_in( + env.path(), + OAuthStore::File, + Duration::from_secs(/*secs*/ 1), + )?; + Ok(()) + })(); + + if let Ok(None) = child.try_wait() { + let _ = child.kill(); + let _ = child.wait(); + } + + test_result +} + +#[test] +#[ignore = "child process for store_lock_is_released_when_holder_process_exits"] +fn store_lock_is_released_when_holder_process_exits_child() -> Result<()> { + let ready_file = match std::env::var_os(LOCK_HOLDER_READY_PATH_ENV) { + Some(path) => std::path::PathBuf::from(path), + None => return Ok(()), + }; + let _lock = OAuthStoreLock::acquire(OAuthStore::File)?; + std::fs::write(ready_file, b"ready")?; + loop { + std::thread::sleep(Duration::from_secs(/*secs*/ 60)); + } +} + #[test] fn auto_save_secrets_lock_failure_does_not_fall_back_to_file() -> Result<()> { let env = TempCodexHome::new(); From b21923d7774e7b5bf6f5d98ab27d0621efcd8ed9 Mon Sep 17 00:00:00 2001 From: Steven Lee Date: Mon, 6 Jul 2026 21:42:31 +0000 Subject: [PATCH 5/7] Share OAuth test CODEX_HOME guard --- codex-rs/rmcp-client/src/oauth.rs | 47 +++---------------- .../rmcp-client/src/oauth/test_support.rs | 46 ++++++++++++++++++ .../src/oauth/tests/store_lock_tests.rs | 42 +---------------- 3 files changed, 53 insertions(+), 82 deletions(-) create mode 100644 codex-rs/rmcp-client/src/oauth/test_support.rs diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index ff2e7af32b37..46e6279a161e 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -18,6 +18,10 @@ mod store_lock; +#[cfg(test)] +#[path = "oauth/test_support.rs"] +mod test_support; + use anyhow::Context; use anyhow::Error; use anyhow::Result; @@ -862,52 +866,13 @@ fn sha_256_prefix(value: &Value) -> Result { mod tests { use super::*; use anyhow::Result; + use codex_keyring_store::tests::MockKeyringStore; use codex_secrets::compute_keyring_account; use keyring::Error as KeyringError; use pretty_assertions::assert_eq; use std::sync::Arc; - use std::sync::Mutex; - use std::sync::MutexGuard; - use std::sync::OnceLock; - use std::sync::PoisonError; - use tempfile::tempdir; - - use codex_keyring_store::tests::MockKeyringStore; - struct TempCodexHome { - _guard: MutexGuard<'static, ()>, - _dir: tempfile::TempDir, - } - - impl TempCodexHome { - fn new() -> Self { - static LOCK: OnceLock> = OnceLock::new(); - let guard = LOCK - .get_or_init(Mutex::default) - .lock() - .unwrap_or_else(PoisonError::into_inner); - let dir = tempdir().expect("create CODEX_HOME temp dir"); - unsafe { - std::env::set_var("CODEX_HOME", dir.path()); - } - Self { - _guard: guard, - _dir: dir, - } - } - - fn path(&self) -> &std::path::Path { - self._dir.path() - } - } - - impl Drop for TempCodexHome { - fn drop(&mut self) { - unsafe { - std::env::remove_var("CODEX_HOME"); - } - } - } + use super::test_support::TempCodexHome; #[test] fn load_oauth_tokens_reads_from_keyring_when_available() -> Result<()> { diff --git a/codex-rs/rmcp-client/src/oauth/test_support.rs b/codex-rs/rmcp-client/src/oauth/test_support.rs new file mode 100644 index 000000000000..6ca2f510f5fa --- /dev/null +++ b/codex-rs/rmcp-client/src/oauth/test_support.rs @@ -0,0 +1,46 @@ +use std::path::Path; +use std::sync::Mutex; +use std::sync::MutexGuard; +use std::sync::OnceLock; +use std::sync::PoisonError; + +use tempfile::tempdir; + +/// Serializes tests that mutate process-wide CODEX_HOME. +/// +/// Keep OAuth tests on this one guard instead of defining per-module helpers; otherwise +/// concurrently running test modules can point File/Secrets storage at different homes. +pub(super) struct TempCodexHome { + _guard: MutexGuard<'static, ()>, + _dir: tempfile::TempDir, +} + +impl TempCodexHome { + pub(super) fn new() -> Self { + static LOCK: OnceLock> = OnceLock::new(); + let guard = LOCK + .get_or_init(Mutex::default) + .lock() + .unwrap_or_else(PoisonError::into_inner); + let dir = tempdir().expect("create CODEX_HOME temp dir"); + unsafe { + std::env::set_var("CODEX_HOME", dir.path()); + } + Self { + _guard: guard, + _dir: dir, + } + } + + pub(super) fn path(&self) -> &Path { + self._dir.path() + } +} + +impl Drop for TempCodexHome { + fn drop(&mut self) { + unsafe { + std::env::remove_var("CODEX_HOME"); + } + } +} diff --git a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs index d59c8c4f72d3..aafc362c204b 100644 --- a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs +++ b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs @@ -1,9 +1,4 @@ -use std::path::Path; use std::process::Command; -use std::sync::Mutex; -use std::sync::MutexGuard; -use std::sync::OnceLock; -use std::sync::PoisonError; use std::sync::mpsc; use std::time::Duration; use std::time::Instant; @@ -20,7 +15,6 @@ use oauth2::basic::BasicTokenType; use pretty_assertions::assert_eq; use rmcp::transport::auth::OAuthTokenResponse; use rmcp::transport::auth::VendorExtraTokenFields; -use tempfile::tempdir; use tracing::Event; use tracing::Id; use tracing::Metadata; @@ -43,44 +37,10 @@ use crate::oauth::save_oauth_tokens_to_file_with_lock_held; use crate::oauth::save_oauth_tokens_to_secrets_keyring_with_lock_held; use crate::oauth::save_oauth_tokens_with_keyring; use crate::oauth::save_oauth_tokens_with_keyring_with_fallback_to_file; +use crate::oauth::test_support::TempCodexHome; const STORE_LOCK_CONTENTION_EVENT_TARGET: &str = "codex_rmcp_client::oauth::store_lock::contention"; -struct TempCodexHome { - _guard: MutexGuard<'static, ()>, - _dir: tempfile::TempDir, -} - -impl TempCodexHome { - fn new() -> Self { - static LOCK: OnceLock> = OnceLock::new(); - let guard = LOCK - .get_or_init(Mutex::default) - .lock() - .unwrap_or_else(PoisonError::into_inner); - let dir = tempdir().expect("create CODEX_HOME temp dir"); - unsafe { - std::env::set_var("CODEX_HOME", dir.path()); - } - Self { - _guard: guard, - _dir: dir, - } - } - - fn path(&self) -> &Path { - self._dir.path() - } -} - -impl Drop for TempCodexHome { - fn drop(&mut self) { - unsafe { - std::env::remove_var("CODEX_HOME"); - } - } -} - fn assert_tokens_match_without_expiry(actual: &StoredOAuthTokens, expected: &StoredOAuthTokens) { assert_eq!(actual.server_name, expected.server_name); assert_eq!(actual.url, expected.url); From ceef76befcbba57447e4e7a7e311f7fbac159df8 Mon Sep 17 00:00:00 2001 From: Steven Lee Date: Tue, 7 Jul 2026 00:19:50 +0000 Subject: [PATCH 6/7] Address OAuth lock review nits --- codex-rs/rmcp-client/Cargo.toml | 1 - codex-rs/rmcp-client/src/oauth/store_lock.rs | 58 ++++++++++---------- 2 files changed, 29 insertions(+), 30 deletions(-) diff --git a/codex-rs/rmcp-client/Cargo.toml b/codex-rs/rmcp-client/Cargo.toml index 0ab5bf076971..517f7270bbe0 100644 --- a/codex-rs/rmcp-client/Cargo.toml +++ b/codex-rs/rmcp-client/Cargo.toml @@ -84,6 +84,5 @@ keyring = { workspace = true, features = ["windows-native"] } [target.'cfg(any(target_os = "freebsd", target_os = "openbsd"))'.dependencies] keyring = { workspace = true, features = ["sync-secret-service"] } - [lib] doctest = false diff --git a/codex-rs/rmcp-client/src/oauth/store_lock.rs b/codex-rs/rmcp-client/src/oauth/store_lock.rs index b99858a7f45d..f6c359c5d370 100644 --- a/codex-rs/rmcp-client/src/oauth/store_lock.rs +++ b/codex-rs/rmcp-client/src/oauth/store_lock.rs @@ -22,6 +22,35 @@ const STORE_LOCK_RETRY_SLEEP: Duration = Duration::from_millis(50); // Tests listen for this event so they prove a contender reached the real WouldBlock branch. const LOCK_CONTENTION_EVENT_TARGET: &str = "codex_rmcp_client::oauth::store_lock::contention"; +#[derive(Clone, Copy, Debug)] +pub(super) enum OAuthStore { + File, + Secrets, +} + +impl OAuthStore { + fn lock_filename(self) -> &'static str { + match self { + Self::File => "file-store.lock", + Self::Secrets => "secrets-store.lock", + } + } +} + +impl std::fmt::Display for OAuthStore { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::File => f.write_str("fallback file"), + Self::Secrets => f.write_str("encrypted secrets"), + } + } +} + +/// Serializes one complete operation on an aggregate OAuth credential store. +pub(super) struct OAuthStoreLock { + _file: File, +} + /// Marks aggregate-store coordination failures in an [`anyhow::Error`] chain. /// /// Auto may fall back when the configured keyring backend is unavailable, but it must surface a @@ -67,35 +96,6 @@ pub(super) enum OAuthStoreLockFailure { }, } -#[derive(Clone, Copy, Debug)] -pub(super) enum OAuthStore { - File, - Secrets, -} - -impl OAuthStore { - fn lock_filename(self) -> &'static str { - match self { - Self::File => "file-store.lock", - Self::Secrets => "secrets-store.lock", - } - } -} - -impl std::fmt::Display for OAuthStore { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::File => f.write_str("fallback file"), - Self::Secrets => f.write_str("encrypted secrets"), - } - } -} - -/// Serializes one complete operation on an aggregate OAuth credential store. -pub(super) struct OAuthStoreLock { - _file: File, -} - impl OAuthStoreLock { pub(super) fn acquire(store: OAuthStore) -> Result { // This lock intentionally follows the existing local File/Secrets credential-store From d5eb78ec3d3095cfe7bd8ee1987ec6933678cacf Mon Sep 17 00:00:00 2001 From: Steven Lee Date: Tue, 7 Jul 2026 03:10:15 +0000 Subject: [PATCH 7/7] Keep OAuth lock impl next to its type --- codex-rs/rmcp-client/src/oauth/store_lock.rs | 90 ++++++++++---------- 1 file changed, 45 insertions(+), 45 deletions(-) diff --git a/codex-rs/rmcp-client/src/oauth/store_lock.rs b/codex-rs/rmcp-client/src/oauth/store_lock.rs index f6c359c5d370..5f83ebb8a63f 100644 --- a/codex-rs/rmcp-client/src/oauth/store_lock.rs +++ b/codex-rs/rmcp-client/src/oauth/store_lock.rs @@ -51,51 +51,6 @@ pub(super) struct OAuthStoreLock { _file: File, } -/// Marks aggregate-store coordination failures in an [`anyhow::Error`] chain. -/// -/// Auto may fall back when the configured keyring backend is unavailable, but it must surface a -/// lock failure. Falling back while another process owns the aggregate-store lock could leave the -/// newer credential in File while a stale Secrets entry remains preferred. -#[derive(Debug, thiserror::Error)] -pub(super) enum OAuthStoreLockFailure { - #[error("failed to resolve CODEX_HOME for MCP OAuth {store} aggregate-store lock")] - CodexHome { - store: OAuthStore, - #[source] - source: io::Error, - }, - #[error("failed to create MCP OAuth {store} aggregate-store lock directory {}", path.display())] - CreateDir { - store: OAuthStore, - path: PathBuf, - #[source] - source: io::Error, - }, - #[error("failed to open MCP OAuth {store} aggregate-store lock {}", path.display())] - Open { - store: OAuthStore, - path: PathBuf, - #[source] - source: io::Error, - }, - #[error( - "timed out after {acquire_timeout:?} waiting for MCP OAuth {store} aggregate-store lock {}", - path.display() - )] - Timeout { - store: OAuthStore, - path: PathBuf, - acquire_timeout: Duration, - }, - #[error("failed to lock MCP OAuth {store} aggregate-store lock {}", path.display())] - Lock { - store: OAuthStore, - path: PathBuf, - #[source] - source: io::Error, - }, -} - impl OAuthStoreLock { pub(super) fn acquire(store: OAuthStore) -> Result { // This lock intentionally follows the existing local File/Secrets credential-store @@ -172,6 +127,51 @@ impl OAuthStoreLock { } } +/// Marks aggregate-store coordination failures in an [`anyhow::Error`] chain. +/// +/// Auto may fall back when the configured keyring backend is unavailable, but it must surface a +/// lock failure. Falling back while another process owns the aggregate-store lock could leave the +/// newer credential in File while a stale Secrets entry remains preferred. +#[derive(Debug, thiserror::Error)] +pub(super) enum OAuthStoreLockFailure { + #[error("failed to resolve CODEX_HOME for MCP OAuth {store} aggregate-store lock")] + CodexHome { + store: OAuthStore, + #[source] + source: io::Error, + }, + #[error("failed to create MCP OAuth {store} aggregate-store lock directory {}", path.display())] + CreateDir { + store: OAuthStore, + path: PathBuf, + #[source] + source: io::Error, + }, + #[error("failed to open MCP OAuth {store} aggregate-store lock {}", path.display())] + Open { + store: OAuthStore, + path: PathBuf, + #[source] + source: io::Error, + }, + #[error( + "timed out after {acquire_timeout:?} waiting for MCP OAuth {store} aggregate-store lock {}", + path.display() + )] + Timeout { + store: OAuthStore, + path: PathBuf, + acquire_timeout: Duration, + }, + #[error("failed to lock MCP OAuth {store} aggregate-store lock {}", path.display())] + Lock { + store: OAuthStore, + path: PathBuf, + #[source] + source: io::Error, + }, +} + fn oauth_store_lock_path(codex_home: &Path, store: OAuthStore) -> PathBuf { codex_home.join(OAUTH_LOCK_DIR).join(store.lock_filename()) }