From ff19d53c4b7ed6113e364fb1012b6fb94f143bec Mon Sep 17 00:00:00 2001 From: xli-oai Date: Mon, 1 Jun 2026 17:00:33 -0700 Subject: [PATCH] use config write lock for specialized writers --- .../src/config/external_agent_config.rs | 76 ++++++++----------- .../app-server/src/config_manager_service.rs | 13 +++- codex-rs/config/src/marketplace_edit.rs | 55 +++++++++----- codex-rs/config/src/mcp_edit.rs | 20 +++-- codex-rs/config/src/plugin_edit.rs | 35 ++++----- 5 files changed, 108 insertions(+), 91 deletions(-) diff --git a/codex-rs/app-server/src/config/external_agent_config.rs b/codex-rs/app-server/src/config/external_agent_config.rs index 18c276f07d..118d778dfb 100644 --- a/codex-rs/app-server/src/config/external_agent_config.rs +++ b/codex-rs/app-server/src/config/external_agent_config.rs @@ -1,4 +1,5 @@ use codex_config::types::PluginConfig; +use codex_config::with_config_write_lock; use codex_core::config::Config; use codex_core::config::ConfigBuilder; use codex_core_plugins::PluginInstallRequest; @@ -793,30 +794,7 @@ impl ExternalAgentConfigService { return Ok(()); } - let Some(target_parent) = target_config.parent() else { - return Err(invalid_data_error("config target path has no parent")); - }; - fs::create_dir_all(target_parent)?; - if !target_config.exists() { - write_toml_file(&target_config, &migrated)?; - return Ok(()); - } - - let existing_raw = fs::read_to_string(&target_config)?; - let mut existing = if existing_raw.trim().is_empty() { - TomlValue::Table(Default::default()) - } else { - toml::from_str::(&existing_raw) - .map_err(|err| invalid_data_error(format!("invalid existing config.toml: {err}")))? - }; - - let changed = merge_missing_toml_values(&mut existing, &migrated)?; - if !changed { - return Ok(()); - } - - write_toml_file(&target_config, &existing)?; - Ok(()) + merge_missing_toml_file(&target_config, &migrated) } fn import_mcp_server_config(&self, cwd: Option<&Path>) -> io::Result<()> { @@ -847,26 +825,7 @@ impl ExternalAgentConfigService { return Ok(()); } - let Some(target_parent) = target_config.parent() else { - return Err(invalid_data_error("config target path has no parent")); - }; - fs::create_dir_all(target_parent)?; - if !target_config.exists() { - write_toml_file(&target_config, &migrated)?; - return Ok(()); - } - - let existing_raw = fs::read_to_string(&target_config)?; - let mut existing = if existing_raw.trim().is_empty() { - TomlValue::Table(Default::default()) - } else { - toml::from_str::(&existing_raw) - .map_err(|err| invalid_data_error(format!("invalid existing config.toml: {err}")))? - }; - if merge_missing_toml_values(&mut existing, &migrated)? { - write_toml_file(&target_config, &existing)?; - } - Ok(()) + merge_missing_toml_file(&target_config, &migrated) } fn import_subagents(&self, cwd: Option<&Path>) -> io::Result { @@ -1538,6 +1497,35 @@ fn merge_missing_toml_values(existing: &mut TomlValue, incoming: &TomlValue) -> } } +fn merge_missing_toml_file(target_config: &Path, incoming: &TomlValue) -> io::Result<()> { + with_config_write_lock(target_config, |write_paths| { + let Some(target_parent) = write_paths.write_path.parent() else { + return Err(invalid_data_error("config target path has no parent")); + }; + fs::create_dir_all(target_parent)?; + let Some(read_path) = write_paths.read_path.as_deref() else { + return write_toml_file(&write_paths.write_path, incoming); + }; + let existing_raw = match fs::read_to_string(read_path) { + Ok(raw) => raw, + Err(err) if err.kind() == io::ErrorKind::NotFound => { + return write_toml_file(&write_paths.write_path, incoming); + } + Err(err) => return Err(err), + }; + let mut existing = if existing_raw.trim().is_empty() { + TomlValue::Table(Default::default()) + } else { + toml::from_str::(&existing_raw) + .map_err(|err| invalid_data_error(format!("invalid existing config.toml: {err}")))? + }; + if merge_missing_toml_values(&mut existing, incoming)? { + write_toml_file(&write_paths.write_path, &existing)?; + } + Ok(()) + }) +} + fn write_toml_file(path: &Path, value: &TomlValue) -> io::Result<()> { let serialized = toml::to_string_pretty(value) .map_err(|err| invalid_data_error(format!("failed to serialize config.toml: {err}")))?; diff --git a/codex-rs/app-server/src/config_manager_service.rs b/codex-rs/app-server/src/config_manager_service.rs index 4b42c28aa9..6efaf79ad3 100644 --- a/codex-rs/app-server/src/config_manager_service.rs +++ b/codex-rs/app-server/src/config_manager_service.rs @@ -18,6 +18,7 @@ use codex_config::ConfigLayerStackOrdering; use codex_config::ConfigRequirementsToml; use codex_config::config_toml::ConfigToml; use codex_config::merge_toml_values; +use codex_config::with_config_write_lock; use codex_core::config::deserialize_config_toml_with_base; use codex_core::config::edit::ConfigEdit; use codex_core::config::edit::ConfigEditsBuilder; @@ -399,10 +400,14 @@ async fn create_empty_user_layer( } async fn write_empty_user_config(write_path: PathBuf) -> Result<(), ConfigManagerError> { - task::spawn_blocking(move || write_atomically(&write_path, "")) - .await - .map_err(|err| ConfigManagerError::anyhow("config persistence task panicked", err.into()))? - .map_err(|err| ConfigManagerError::io("failed to create empty user config.toml", err)) + task::spawn_blocking(move || { + with_config_write_lock(&write_path, |write_paths| { + write_atomically(&write_paths.write_path, "") + }) + }) + .await + .map_err(|err| ConfigManagerError::anyhow("config persistence task panicked", err.into()))? + .map_err(|err| ConfigManagerError::io("failed to create empty user config.toml", err)) } fn parse_value(value: JsonValue) -> Result, String> { diff --git a/codex-rs/config/src/marketplace_edit.rs b/codex-rs/config/src/marketplace_edit.rs index 2aad2486eb..0a2b42e345 100644 --- a/codex-rs/config/src/marketplace_edit.rs +++ b/codex-rs/config/src/marketplace_edit.rs @@ -9,6 +9,7 @@ use toml_edit::Value as TomlValue; use toml_edit::value; use crate::CONFIG_TOML_FILE; +use crate::with_config_write_lock; pub struct MarketplaceConfigUpdate<'a> { pub last_updated: &'a str, @@ -32,10 +33,17 @@ pub fn record_user_marketplace( update: &MarketplaceConfigUpdate<'_>, ) -> std::io::Result<()> { let config_path = codex_home.join(CONFIG_TOML_FILE); - let mut doc = read_or_create_document(&config_path)?; - upsert_marketplace(&mut doc, marketplace_name, update); - fs::create_dir_all(codex_home)?; - fs::write(config_path, doc.to_string()) + with_config_write_lock(&config_path, |write_paths| { + let mut doc = read_or_create_document( + write_paths + .read_path + .as_deref() + .unwrap_or(&write_paths.write_path), + )?; + upsert_marketplace(&mut doc, marketplace_name, update); + fs::create_dir_all(codex_home)?; + fs::write(&write_paths.write_path, doc.to_string()) + }) } pub fn remove_user_marketplace(codex_home: &Path, marketplace_name: &str) -> std::io::Result { @@ -48,24 +56,31 @@ pub fn remove_user_marketplace_config( marketplace_name: &str, ) -> std::io::Result { let config_path = codex_home.join(CONFIG_TOML_FILE); - let mut doc = match fs::read_to_string(&config_path) { - Ok(raw) => raw - .parse::() - .map_err(|err| std::io::Error::new(ErrorKind::InvalidData, err))?, - Err(err) if err.kind() == ErrorKind::NotFound => { - return Ok(RemoveMarketplaceConfigOutcome::NotFound); + with_config_write_lock(&config_path, |write_paths| { + let mut doc = match fs::read_to_string( + write_paths + .read_path + .as_deref() + .unwrap_or(&write_paths.write_path), + ) { + Ok(raw) => raw + .parse::() + .map_err(|err| std::io::Error::new(ErrorKind::InvalidData, err))?, + Err(err) if err.kind() == ErrorKind::NotFound => { + return Ok(RemoveMarketplaceConfigOutcome::NotFound); + } + Err(err) => return Err(err), + }; + + let outcome = remove_marketplace(&mut doc, marketplace_name); + if outcome != RemoveMarketplaceConfigOutcome::Removed { + return Ok(outcome); } - Err(err) => return Err(err), - }; - let outcome = remove_marketplace(&mut doc, marketplace_name); - if outcome != RemoveMarketplaceConfigOutcome::Removed { - return Ok(outcome); - } - - fs::create_dir_all(codex_home)?; - fs::write(config_path, doc.to_string())?; - Ok(RemoveMarketplaceConfigOutcome::Removed) + fs::create_dir_all(codex_home)?; + fs::write(&write_paths.write_path, doc.to_string())?; + Ok(RemoveMarketplaceConfigOutcome::Removed) + }) } fn read_or_create_document(config_path: &Path) -> std::io::Result { diff --git a/codex-rs/config/src/mcp_edit.rs b/codex-rs/config/src/mcp_edit.rs index 9f007de113..7006c46229 100644 --- a/codex-rs/config/src/mcp_edit.rs +++ b/codex-rs/config/src/mcp_edit.rs @@ -16,6 +16,7 @@ use crate::CONFIG_TOML_FILE; use crate::McpServerConfig; use crate::McpServerEnvVar; use crate::McpServerTransportConfig; +use crate::with_config_write_lock; pub async fn load_global_mcp_servers( codex_home: &Path, @@ -87,12 +88,19 @@ impl ConfigEditsBuilder { fn apply_blocking(self) -> std::io::Result<()> { let config_path = self.codex_home.join(CONFIG_TOML_FILE); - let mut doc = read_or_create_document(&config_path)?; - if let Some(servers) = self.mcp_servers.as_ref() { - replace_mcp_servers(&mut doc, servers); - } - fs::create_dir_all(&self.codex_home)?; - fs::write(config_path, doc.to_string()) + with_config_write_lock(&config_path, |write_paths| { + let mut doc = read_or_create_document( + write_paths + .read_path + .as_deref() + .unwrap_or(&write_paths.write_path), + )?; + if let Some(servers) = self.mcp_servers.as_ref() { + replace_mcp_servers(&mut doc, servers); + } + fs::create_dir_all(&self.codex_home)?; + fs::write(&write_paths.write_path, doc.to_string()) + }) } } diff --git a/codex-rs/config/src/plugin_edit.rs b/codex-rs/config/src/plugin_edit.rs index 63795cc6c6..8bc4e5be5e 100644 --- a/codex-rs/config/src/plugin_edit.rs +++ b/codex-rs/config/src/plugin_edit.rs @@ -2,7 +2,6 @@ use std::fs; use std::io::ErrorKind; use std::path::Path; -use codex_utils_path::resolve_symlink_write_paths; use codex_utils_path::write_atomically; use tokio::task; use toml_edit::DocumentMut; @@ -11,6 +10,7 @@ use toml_edit::Table as TomlTable; use toml_edit::value; use crate::CONFIG_TOML_FILE; +use crate::with_config_write_lock; #[derive(Debug, Clone, PartialEq, Eq)] pub enum PluginConfigEdit { @@ -56,22 +56,23 @@ fn apply_user_plugin_config_edits_blocking( } let config_path = codex_home.join(CONFIG_TOML_FILE); - let write_paths = resolve_symlink_write_paths(&config_path)?; - let mut doc = read_or_create_document(write_paths.read_path.as_deref())?; - let mut mutated = false; - for edit in edits { - mutated |= match edit { - PluginConfigEdit::SetEnabled { - plugin_key, - enabled, - } => set_plugin_enabled(&mut doc, &plugin_key, enabled), - PluginConfigEdit::Clear { plugin_key } => clear_plugin(&mut doc, &plugin_key), - }; - } - if !mutated { - return Ok(()); - } - write_atomically(&write_paths.write_path, &doc.to_string()) + with_config_write_lock(&config_path, |write_paths| { + let mut doc = read_or_create_document(write_paths.read_path.as_deref())?; + let mut mutated = false; + for edit in edits { + mutated |= match edit { + PluginConfigEdit::SetEnabled { + plugin_key, + enabled, + } => set_plugin_enabled(&mut doc, &plugin_key, enabled), + PluginConfigEdit::Clear { plugin_key } => clear_plugin(&mut doc, &plugin_key), + }; + } + if !mutated { + return Ok(()); + } + write_atomically(&write_paths.write_path, &doc.to_string()) + }) } fn read_or_create_document(config_path: Option<&Path>) -> std::io::Result {