mirror of
https://github.com/openai/codex.git
synced 2026-05-29 15:30:22 +00:00
fix(plugins): keep version upgrades additive (#23356)
## Why Windows can reject plugin cache upgrades when a running MCP server still has its working directory inside the currently active plugin version. The existing cache refresh path replaces `plugins/cache/<marketplace>/<plugin>` as a whole, so a live handle under the old version can make an otherwise ordinary version bump fail. This PR keeps the existing plugin-selection model intact while making version bumps less disruptive. ## What changed - When installing a new version beside an existing plugin cache root, move only the staged version directory into place instead of replacing the whole plugin root. - Best-effort prune older sibling version directories after the new version is activated. - Preserve the existing whole-root replacement path for first installs and same-version refreshes. - Add regression coverage for upgrading from `1.0.0` to `2.0.0` without replacing the plugin root. ## Verification - `cargo test -p codex-core-plugins install_with_new_version` - `cargo fmt --package codex-core-plugins --check`
This commit is contained in:
@@ -4,8 +4,10 @@ use codex_plugin::PluginId;
|
||||
use codex_plugin::validate_plugin_segment;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_plugins::find_plugin_manifest_path;
|
||||
use semver::Version;
|
||||
use serde::Deserialize;
|
||||
use serde_json::Value as JsonValue;
|
||||
use std::cmp::Ordering;
|
||||
use std::fs;
|
||||
use std::io;
|
||||
use std::path::Path;
|
||||
@@ -75,7 +77,7 @@ impl PluginStore {
|
||||
})
|
||||
.filter(|version| validate_plugin_version_segment(version).is_ok())
|
||||
.collect::<Vec<_>>();
|
||||
discovered_versions.sort_unstable();
|
||||
discovered_versions.sort_unstable_by(|left, right| compare_plugin_versions(left, right));
|
||||
if discovered_versions.is_empty() {
|
||||
None
|
||||
} else if discovered_versions
|
||||
@@ -286,6 +288,15 @@ fn replace_plugin_root_atomically(
|
||||
let staged_version_root = staged_root.join(plugin_version);
|
||||
copy_dir_recursive(source, &staged_version_root)?;
|
||||
|
||||
let target_version_root = target_root.join(plugin_version);
|
||||
if target_root.exists() && !target_version_root.exists() {
|
||||
fs::rename(&staged_version_root, &target_version_root).map_err(|err| {
|
||||
PluginStoreError::io("failed to activate updated plugin cache version", err)
|
||||
})?;
|
||||
remove_old_plugin_versions(target_root, plugin_version)?;
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
if target_root.exists() {
|
||||
let backup_dir = tempfile::Builder::new()
|
||||
.prefix("plugin-backup-")
|
||||
@@ -322,6 +333,52 @@ fn replace_plugin_root_atomically(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn remove_old_plugin_versions(
|
||||
target_root: &Path,
|
||||
plugin_version: &str,
|
||||
) -> Result<(), PluginStoreError> {
|
||||
let Ok(entries) = fs::read_dir(target_root) else {
|
||||
return Ok(());
|
||||
};
|
||||
|
||||
for entry in entries.filter_map(Result::ok) {
|
||||
let Ok(file_type) = entry.file_type() else {
|
||||
continue;
|
||||
};
|
||||
if !file_type.is_dir() {
|
||||
continue;
|
||||
}
|
||||
let Ok(version) = entry.file_name().into_string() else {
|
||||
continue;
|
||||
};
|
||||
if version == plugin_version || validate_plugin_version_segment(&version).is_err() {
|
||||
continue;
|
||||
}
|
||||
|
||||
if fs::remove_dir_all(entry.path()).is_err()
|
||||
&& old_plugin_version_would_stay_active(&version, plugin_version)
|
||||
{
|
||||
return Err(PluginStoreError::Invalid(format!(
|
||||
"failed to activate updated plugin cache version `{plugin_version}` while `{version}` remains active"
|
||||
)));
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn old_plugin_version_would_stay_active(old_version: &str, new_version: &str) -> bool {
|
||||
old_version == DEFAULT_PLUGIN_VERSION
|
||||
|| compare_plugin_versions(old_version, new_version).is_gt()
|
||||
}
|
||||
|
||||
fn compare_plugin_versions(left: &str, right: &str) -> Ordering {
|
||||
match (Version::parse(left), Version::parse(right)) {
|
||||
(Ok(left), Ok(right)) => left.cmp(&right),
|
||||
_ => left.cmp(right),
|
||||
}
|
||||
}
|
||||
|
||||
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))?;
|
||||
|
||||
@@ -247,7 +247,7 @@ fn active_plugin_version_prefers_default_local_version_when_multiple_versions_ex
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn active_plugin_version_returns_last_sorted_version_when_default_is_missing() {
|
||||
fn active_plugin_version_returns_latest_version_when_default_is_missing() {
|
||||
let tmp = tempdir().unwrap();
|
||||
write_plugin(
|
||||
&tmp.path().join("plugins/cache/debug"),
|
||||
@@ -268,6 +268,76 @@ fn active_plugin_version_returns_last_sorted_version_when_default_is_missing() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn active_plugin_version_compares_semver_versions_semantically() {
|
||||
let tmp = tempdir().unwrap();
|
||||
write_plugin(
|
||||
&tmp.path().join("plugins/cache/debug"),
|
||||
"sample-plugin/9.0.0",
|
||||
"sample-plugin",
|
||||
);
|
||||
write_plugin(
|
||||
&tmp.path().join("plugins/cache/debug"),
|
||||
"sample-plugin/10.0.0",
|
||||
"sample-plugin",
|
||||
);
|
||||
let store = PluginStore::new(tmp.path().to_path_buf());
|
||||
let plugin_id = PluginId::new("sample-plugin".to_string(), "debug".to_string()).unwrap();
|
||||
|
||||
assert_eq!(
|
||||
store.active_plugin_version(&plugin_id),
|
||||
Some("10.0.0".to_string())
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn install_with_new_version_keeps_existing_plugin_root_and_prunes_old_versions() {
|
||||
let tmp = tempdir().unwrap();
|
||||
let store = PluginStore::new(tmp.path().to_path_buf());
|
||||
let plugin_id = PluginId::new("sample-plugin".to_string(), "debug".to_string()).unwrap();
|
||||
|
||||
write_plugin_with_version(tmp.path(), "v1", "sample-plugin", Some("1.0.0"));
|
||||
store
|
||||
.install(
|
||||
AbsolutePathBuf::try_from(tmp.path().join("v1")).unwrap(),
|
||||
plugin_id.clone(),
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
write_plugin_with_version(tmp.path(), "v2", "sample-plugin", Some("2.0.0"));
|
||||
store
|
||||
.install(
|
||||
AbsolutePathBuf::try_from(tmp.path().join("v2")).unwrap(),
|
||||
plugin_id.clone(),
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(
|
||||
store.active_plugin_version(&plugin_id),
|
||||
Some("2.0.0".to_string())
|
||||
);
|
||||
assert!(
|
||||
tmp.path()
|
||||
.join("plugins/cache/debug/sample-plugin/2.0.0")
|
||||
.is_dir()
|
||||
);
|
||||
assert!(
|
||||
!tmp.path()
|
||||
.join("plugins/cache/debug/sample-plugin/1.0.0")
|
||||
.exists()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn old_plugin_version_would_stay_active_for_local_or_later_versions() {
|
||||
assert!(old_plugin_version_would_stay_active(
|
||||
DEFAULT_PLUGIN_VERSION,
|
||||
"1.0.0"
|
||||
));
|
||||
assert!(old_plugin_version_would_stay_active("10.0.0", "9.0.0"));
|
||||
assert!(!old_plugin_version_would_stay_active("1.0.0", "2.0.0"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn plugin_root_rejects_path_separators_in_key_segments() {
|
||||
let err = PluginId::parse("../../etc@debug").unwrap_err();
|
||||
|
||||
Reference in New Issue
Block a user