From b65f9366eeacf4f1b56e9daf54735b449e404814 Mon Sep 17 00:00:00 2001 From: ZachMurphy Date: Mon, 4 May 2026 16:05:49 -0700 Subject: [PATCH] Avoid whole plugin cache replacement on Windows --- codex-rs/core-plugins/src/store.rs | 131 +++++++++++++++++++++++ codex-rs/core-plugins/src/store_tests.rs | 69 ++++++++++++ 2 files changed, 200 insertions(+) diff --git a/codex-rs/core-plugins/src/store.rs b/codex-rs/core-plugins/src/store.rs index fe662a142e..a40115766a 100644 --- a/codex-rs/core-plugins/src/store.rs +++ b/codex-rs/core-plugins/src/store.rs @@ -260,6 +260,10 @@ fn replace_plugin_root_atomically( target_root: &Path, plugin_version: &str, ) -> Result<(), PluginStoreError> { + if cfg!(windows) { + return replace_plugin_root_on_windows(source, target_root, plugin_version); + } + let Some(parent) = target_root.parent() else { return Err(PluginStoreError::Invalid(format!( "plugin cache path has no parent: {}", @@ -322,6 +326,133 @@ fn replace_plugin_root_atomically( Ok(()) } +fn replace_plugin_root_on_windows( + source: &Path, + target_root: &Path, + plugin_version: &str, +) -> Result<(), PluginStoreError> { + let Some(parent) = target_root.parent() else { + return Err(PluginStoreError::Invalid(format!( + "plugin cache path has no parent: {}", + target_root.display() + ))); + }; + + fs::create_dir_all(parent) + .map_err(|err| PluginStoreError::io("failed to create plugin cache directory", err))?; + + let Some(plugin_dir_name) = target_root.file_name() else { + return Err(PluginStoreError::Invalid(format!( + "plugin cache path has no directory name: {}", + target_root.display() + ))); + }; + + let target_version_root = target_root.join(plugin_version); + let staged_dir = tempfile::Builder::new() + .prefix("plugin-install-") + .tempdir_in(parent) + .map_err(|err| { + PluginStoreError::io("failed to create temporary plugin cache directory", err) + })?; + let staged_root = staged_dir.path().join(plugin_dir_name); + let staged_version_root = staged_root.join(plugin_version); + copy_dir_recursive(source, &staged_version_root)?; + + if !target_root.exists() { + fs::rename(&staged_root, target_root) + .map_err(|err| PluginStoreError::io("failed to activate plugin cache entry", err))?; + return Ok(()); + } + + if !target_root.is_dir() { + return Err(PluginStoreError::Invalid(format!( + "plugin cache entry is not a directory: {}", + target_root.display() + ))); + } + + replace_plugin_version_root_atomically_on_windows(&staged_version_root, &target_version_root)?; + remove_stale_plugin_versions_best_effort(target_root, plugin_version); + + Ok(()) +} + +fn replace_plugin_version_root_atomically_on_windows( + staged_version_root: &Path, + target_version_root: &Path, +) -> Result<(), PluginStoreError> { + if !target_version_root.exists() { + fs::rename(staged_version_root, target_version_root) + .map_err(|err| PluginStoreError::io("failed to activate plugin cache version", err))?; + return Ok(()); + } + + let Some(parent) = target_version_root.parent() else { + return Err(PluginStoreError::Invalid(format!( + "plugin cache version path has no parent: {}", + target_version_root.display() + ))); + }; + let backup_dir = tempfile::Builder::new() + .prefix("plugin-version-backup-") + .tempdir_in(parent) + .map_err(|err| { + PluginStoreError::io( + "failed to create plugin cache version backup directory", + err, + ) + })?; + let Some(version_dir_name) = target_version_root.file_name() else { + return Err(PluginStoreError::Invalid(format!( + "plugin cache version path has no directory name: {}", + target_version_root.display() + ))); + }; + let backup_version_root = backup_dir.path().join(version_dir_name); + fs::rename(target_version_root, &backup_version_root) + .map_err(|err| PluginStoreError::io("failed to back up plugin cache version", err))?; + + if let Err(err) = fs::rename(staged_version_root, target_version_root) { + let rollback_result = fs::rename(&backup_version_root, target_version_root); + return match rollback_result { + Ok(()) => Err(PluginStoreError::io( + "failed to activate updated plugin cache version", + err, + )), + Err(rollback_err) => { + let backup_path = backup_dir.keep().join(version_dir_name); + Err(PluginStoreError::Invalid(format!( + "failed to activate updated plugin cache version at {}: {err}; failed to restore previous cache version (left at {}): {rollback_err}", + target_version_root.display(), + backup_path.display() + ))) + } + }; + } + + Ok(()) +} + +fn remove_stale_plugin_versions_best_effort(target_root: &Path, active_version: &str) { + let Ok(entries) = fs::read_dir(target_root) else { + return; + }; + + for entry in entries.filter_map(Result::ok) { + let Ok(file_type) = entry.file_type() else { + continue; + }; + if !file_type.is_dir() { + continue; + } + if entry.file_name() == std::ffi::OsStr::new(active_version) { + continue; + } + let _ = fs::remove_dir_all(entry.path()); + } +} + fn copy_dir_recursive(source: &Path, target: &Path) -> Result<(), PluginStoreError> { fs::create_dir_all(target) .map_err(|err| PluginStoreError::io("failed to create plugin target directory", err))?; diff --git a/codex-rs/core-plugins/src/store_tests.rs b/codex-rs/core-plugins/src/store_tests.rs index 0ba6b0d2c6..2e069400b6 100644 --- a/codex-rs/core-plugins/src/store_tests.rs +++ b/codex-rs/core-plugins/src/store_tests.rs @@ -183,6 +183,75 @@ fn install_uses_manifest_version_when_present() { assert!(installed_path.join(".codex-plugin/plugin.json").is_file()); } +#[cfg(windows)] +#[test] +fn windows_install_reuses_valid_existing_version_without_renaming_plugin_root() { + let tmp = tempdir().unwrap(); + write_plugin_with_version(tmp.path(), "sample-plugin", "sample-plugin", Some("1.0.0")); + let plugin_id = PluginId::new("sample-plugin".to_string(), "debug".to_string()).unwrap(); + let store = PluginStore::new(tmp.path().to_path_buf()); + + let first_result = store + .install( + AbsolutePathBuf::try_from(tmp.path().join("sample-plugin")).unwrap(), + plugin_id.clone(), + ) + .unwrap(); + let plugin_base_root = tmp.path().join("plugins/cache/debug/sample-plugin"); + let previous_dir = std::env::current_dir().unwrap(); + std::env::set_current_dir(&plugin_base_root).unwrap(); + let second_result = store.install( + AbsolutePathBuf::try_from(tmp.path().join("sample-plugin")).unwrap(), + plugin_id, + ); + std::env::set_current_dir(previous_dir).unwrap(); + let second_result = second_result.unwrap(); + + assert_eq!(second_result, first_result); +} + +#[cfg(windows)] +#[test] +fn windows_install_adds_new_version_without_renaming_plugin_root() { + let tmp = tempdir().unwrap(); + write_plugin_with_version(tmp.path(), "source-v1", "sample-plugin", Some("1.0.0")); + write_plugin_with_version(tmp.path(), "source-v2", "sample-plugin", Some("2.0.0")); + let plugin_id = PluginId::new("sample-plugin".to_string(), "debug".to_string()).unwrap(); + let store = PluginStore::new(tmp.path().to_path_buf()); + + store + .install( + AbsolutePathBuf::try_from(tmp.path().join("source-v1")).unwrap(), + plugin_id.clone(), + ) + .unwrap(); + let plugin_base_root = tmp.path().join("plugins/cache/debug/sample-plugin"); + let previous_dir = std::env::current_dir().unwrap(); + std::env::set_current_dir(&plugin_base_root).unwrap(); + let result = store.install( + AbsolutePathBuf::try_from(tmp.path().join("source-v2")).unwrap(), + plugin_id.clone(), + ); + std::env::set_current_dir(previous_dir).unwrap(); + let result = result.unwrap(); + + let installed_path = tmp.path().join("plugins/cache/debug/sample-plugin/2.0.0"); + assert_eq!( + result, + PluginInstallResult { + plugin_id, + plugin_version: "2.0.0".to_string(), + installed_path: AbsolutePathBuf::try_from(installed_path.clone()).unwrap(), + } + ); + assert!( + !tmp.path() + .join("plugins/cache/debug/sample-plugin/1.0.0") + .exists() + ); + assert!(installed_path.join(".codex-plugin/plugin.json").is_file()); +} + #[test] fn install_rejects_blank_manifest_version() { let tmp = tempdir().unwrap();