From 662a755ebb88b8a222132d99fc98ff25e90df4f8 Mon Sep 17 00:00:00 2001 From: JongKyung Lee Date: Mon, 19 Oct 2026 17:19:33 +0900 Subject: [PATCH 1/5] fix(global): preserve existing binaries when installation fails Capture binary files and ownership configs before changing them, then restore their original state on failure instead of removing every declared binary. Keep backups when restoration fails. Add regression coverage for existing files, replaced npm links, partial installs, and failed restoration. --- .../snapshots.toml | 39 ++ ...stall_rollback_preserves_existing_files.md | 85 ++++ ...env_install_rollback_restores_npm_owner.md | 93 ++++ .../src/commands/global/install.rs | 409 +++++++++++++----- 4 files changed, 515 insertions(+), 111 deletions(-) create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_preserves_existing_files.md create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_restores_npm_owner.md diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots.toml b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots.toml index 16764e7f01..3b33693d96 100644 --- a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots.toml +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots.toml @@ -12,3 +12,42 @@ steps = [ { argv = ["vp", "remove", "-g", "env-binary-conflict-pkg-b"], comment = "Cleanup", continue-on-failure = true }, { argv = ["vpt", "stat-file", "$VP_HOME/bins/env-binary-conflict-cli.json", "--assert", "missing"], comment = "Bin config should be deleted", continue-on-failure = true }, ] + +[[case]] +name = "env_install_rollback_preserves_existing_files" +vp = "global" +skip-platforms = ["windows"] +steps = [ + ["vpt", "json-edit", "env-binary-conflict-pkg-a/package.json", "bin", "{\"aaa-rollback-cli\":\"./cli.js\",\"zzz-rollback-cli\":\"./cli.js\"}"], + ["vpt", "write-file", "$VP_HOME/bin/aaa-rollback-cli", "#!/bin/sh\necho original-first\n"], + ["vpt", "write-file", "$VP_HOME/bin/zzz-rollback-cli", "#!/bin/sh\necho original-unattempted\n"], + ["vpt", "chmod", "+x", "$VP_HOME/bin/aaa-rollback-cli"], + ["vpt", "chmod", "+x", "$VP_HOME/bin/zzz-rollback-cli"], + { argv = ["vp", "install", "-g", "./env-binary-conflict-pkg-a"], comment = "A regular file prevents shim creation without authorizing rollback to delete existing files", continue-on-failure = true }, + { argv = ["vpt", "stat-file", "$VP_HOME/bin/aaa-rollback-cli", "--assert", "file"], continue-on-failure = true }, + ["aaa-rollback-cli"], + ["vpt", "stat-file", "$VP_HOME/bin/zzz-rollback-cli", "--assert", "file"], + ["zzz-rollback-cli"], + ["vpt", "stat-file", "$VP_HOME/packages/env-binary-conflict-pkg-a.json", "--assert", "missing"], + ["vpt", "stat-file", "$VP_HOME/bins/aaa-rollback-cli.json", "--assert", "missing"], + ["vpt", "stat-file", "$VP_HOME/bins/zzz-rollback-cli.json", "--assert", "missing"], +] + +[[case]] +name = "env_install_rollback_restores_npm_owner" +vp = "global" +skip-platforms = ["windows"] +steps = [ + { tty = false, argv = ["npm", "install", "-g", "./env-binary-conflict-pkg-a"] }, + ["vpt", "print-file", "$VP_HOME/bins/env-binary-conflict-cli.json"], + ["vpt", "json-edit", "env-binary-conflict-pkg-a/package.json", "bin.zzz-rollback-cli", "./cli.js"], + ["vpt", "write-file", "$VP_HOME/bin/zzz-rollback-cli", "#!/bin/sh\necho original-blocker\n"], + ["vpt", "chmod", "+x", "$VP_HOME/bin/zzz-rollback-cli"], + { argv = ["vp", "install", "-g", "./env-binary-conflict-pkg-a"], comment = "A later collision must restore the original npm link and ownership", continue-on-failure = true }, + { argv = ["vpt", "stat-file", "$VP_HOME/bin/env-binary-conflict-cli", "--assert", "symlink"], continue-on-failure = true }, + ["env-binary-conflict-cli"], + ["vpt", "print-file", "$VP_HOME/bins/env-binary-conflict-cli.json"], + ["zzz-rollback-cli"], + ["vpt", "stat-file", "$VP_HOME/packages/env-binary-conflict-pkg-a.json", "--assert", "missing"], + ["vpt", "stat-file", "$VP_HOME/bins/zzz-rollback-cli.json", "--assert", "missing"], +] diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_preserves_existing_files.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_preserves_existing_files.md new file mode 100644 index 0000000000..085c09c887 --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_preserves_existing_files.md @@ -0,0 +1,85 @@ +# env_install_rollback_preserves_existing_files + +## `vpt json-edit env-binary-conflict-pkg-a/package.json bin '{"aaa-rollback-cli":"./cli.js","zzz-rollback-cli":"./cli.js"}'` + +``` +``` + +## `vpt write-file $VP_HOME/bin/aaa-rollback-cli '#'\!'/bin/sh +echo original-first +'` + +``` +``` + +## `vpt write-file $VP_HOME/bin/zzz-rollback-cli '#'\!'/bin/sh +echo original-unattempted +'` + +``` +``` + +## `vpt chmod +x $VP_HOME/bin/aaa-rollback-cli` + +``` +``` + +## `vpt chmod +x $VP_HOME/bin/zzz-rollback-cli` + +``` +``` + +## `vp install -g ./env-binary-conflict-pkg-a` + +A regular file prevents shim creation without authorizing rollback to delete existing files + +**Exit code:** 1 + +``` +VITE+ - The Unified Toolchain for the Web + +info: Installing 1 global package with Node.js +error: Failed to install env-binary-conflict-pkg-a: Command execution failed: File exists (os error 17) +``` + +## `vpt stat-file $VP_HOME/bin/aaa-rollback-cli --assert file` + +``` +/.vite-plus/bin/aaa-rollback-cli: file +``` + +## `aaa-rollback-cli` + +``` +original-first +``` + +## `vpt stat-file $VP_HOME/bin/zzz-rollback-cli --assert file` + +``` +/.vite-plus/bin/zzz-rollback-cli: file +``` + +## `zzz-rollback-cli` + +``` +original-unattempted +``` + +## `vpt stat-file $VP_HOME/packages/env-binary-conflict-pkg-a.json --assert missing` + +``` +/.vite-plus/packages/env-binary-conflict-pkg-a.json: missing +``` + +## `vpt stat-file $VP_HOME/bins/aaa-rollback-cli.json --assert missing` + +``` +/.vite-plus/bins/aaa-rollback-cli.json: missing +``` + +## `vpt stat-file $VP_HOME/bins/zzz-rollback-cli.json --assert missing` + +``` +/.vite-plus/bins/zzz-rollback-cli.json: missing +``` diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_restores_npm_owner.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_restores_npm_owner.md new file mode 100644 index 0000000000..33f2a0592b --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_restores_npm_owner.md @@ -0,0 +1,93 @@ +# env_install_rollback_restores_npm_owner + +## `npm install -g ./env-binary-conflict-pkg-a` + +``` + +added 1 package in +Linked 'env-binary-conflict-cli' to /.vite-plus/bin/env-binary-conflict-cli +``` + +## `vpt print-file $VP_HOME/bins/env-binary-conflict-cli.json` + +``` +{ + "name": "env-binary-conflict-cli", + "package": "env-binary-conflict-pkg-a", + "version": "", + "nodeVersion": "", + "source": "npm" +} +``` + +## `vpt json-edit env-binary-conflict-pkg-a/package.json bin.zzz-rollback-cli ./cli.js` + +``` +``` + +## `vpt write-file $VP_HOME/bin/zzz-rollback-cli '#'\!'/bin/sh +echo original-blocker +'` + +``` +``` + +## `vpt chmod +x $VP_HOME/bin/zzz-rollback-cli` + +``` +``` + +## `vp install -g ./env-binary-conflict-pkg-a` + +A later collision must restore the original npm link and ownership + +**Exit code:** 1 + +``` +VITE+ - The Unified Toolchain for the Web + +info: Installing 1 global package with Node.js +error: Failed to install env-binary-conflict-pkg-a: Command execution failed: File exists (os error 17) +``` + +## `vpt stat-file $VP_HOME/bin/env-binary-conflict-cli --assert symlink` + +``` +/.vite-plus/bin/env-binary-conflict-cli: symlink +``` + +## `env-binary-conflict-cli` + +``` +Hello from pkg-a! +``` + +## `vpt print-file $VP_HOME/bins/env-binary-conflict-cli.json` + +``` +{ + "name": "env-binary-conflict-cli", + "package": "env-binary-conflict-pkg-a", + "version": "", + "nodeVersion": "", + "source": "npm" +} +``` + +## `zzz-rollback-cli` + +``` +original-blocker +``` + +## `vpt stat-file $VP_HOME/packages/env-binary-conflict-pkg-a.json --assert missing` + +``` +/.vite-plus/packages/env-binary-conflict-pkg-a.json: missing +``` + +## `vpt stat-file $VP_HOME/bins/zzz-rollback-cli.json --assert missing` + +``` +/.vite-plus/bins/zzz-rollback-cli.json: missing +``` diff --git a/crates/vp_global_cli/src/commands/global/install.rs b/crates/vp_global_cli/src/commands/global/install.rs index de1cc3c60c..9da38841bf 100644 --- a/crates/vp_global_cli/src/commands/global/install.rs +++ b/crates/vp_global_cli/src/commands/global/install.rs @@ -429,57 +429,29 @@ pub async fn install( metadata.install_id = install_id.clone(); metadata.version_spec = update_version_spec(spec); - let mut finalized = true; - for bin_name in &stale_bin_names { - let result = async { - remove_package_shim(&bin_dir, bin_name).await?; - BinConfig::delete(bin_name).await?; - Ok::<(), Error>(()) - } - .await; - - if let Err(error) = result.map_err(|error| package_error(&package_name, error)) { - restore_previous_install_state( - &bin_dir, - &package_name, - previous_metadata.as_ref(), - &bin_names, - ) - .await; + let mut rollback = match BinRollback::new() { + Ok(rollback) => rollback, + Err(error) => { let _ = cleanup_failed_install(&install_dir).await; if first_error.is_none() { - first_error = Some(error); + first_error = Some(package_error(&package_name, error)); } - finalized = false; - break; + continue; } - } - - if !finalized { - continue; - } - - // 5.4 Activate the new installation through metadata. - if let Err(error) = - metadata.save().await.map_err(|error| package_error(&package_name, error)) - { - restore_previous_install_state( - &bin_dir, - &package_name, - previous_metadata.as_ref(), - &bin_names, - ) - .await; - let _ = cleanup_failed_install(&install_dir).await; - if first_error.is_none() { - first_error = Some(error); + }; + let result = async { + for bin_name in &stale_bin_names { + rollback.capture_bin(&bin_dir, bin_name).await?; + remove_package_shim(&bin_dir, bin_name).await?; + BinConfig::delete(bin_name).await?; } - continue; - } - // 5.5 Expose each binary and record its ownership. - for bin_name in &bin_names { - let result = async { + // 5.4 Activate the new installation through metadata. + metadata.save().await?; + + // 5.5 Expose each binary and record its ownership. + for bin_name in &bin_names { + rollback.capture_bin(&bin_dir, bin_name).await?; create_package_shim(&bin_dir, bin_name, &package_name).await?; BinConfig::new( bin_name.clone(), @@ -488,28 +460,19 @@ pub async fn install( node_version.clone(), ) .save() - .await + .await?; } - .await; + Ok::<(), Error>(()) + } + .await; - if let Err(error) = result.map_err(|error| package_error(&package_name, error)) { - restore_previous_install_state( - &bin_dir, - &package_name, - previous_metadata.as_ref(), - &bin_names, - ) + if let Err(error) = result { + restore_previous_install_state(&package_name, previous_metadata.as_ref(), rollback) .await; - let _ = cleanup_failed_install(&install_dir).await; - if first_error.is_none() { - first_error = Some(error); - } - finalized = false; - break; + let _ = cleanup_failed_install(&install_dir).await; + if first_error.is_none() { + first_error = Some(package_error(&package_name, error)); } - } - - if !finalized { continue; } @@ -654,57 +617,148 @@ async fn restore_package_metadata(package_name: &str, previous_metadata: Option< } } -async fn restore_previous_install_state( - bin_dir: &AbsolutePath, - package_name: &str, - previous_metadata: Option<&PackageMetadata>, - current_bin_names: &[String], -) { - restore_package_metadata(package_name, previous_metadata).await; +/// Save only paths an installation is about to change, including their original owners. +struct BinRollback { + directory: tempfile::TempDir, + files: Vec<(AbsolutePathBuf, Option)>, +} - let previous_bin_names = previous_metadata - .map(|metadata| metadata.bins.iter().cloned().collect::>()) - .unwrap_or_default(); - let bin_names = - current_bin_names.iter().chain(previous_bin_names.iter()).cloned().collect::>(); - - for bin_name in bin_names { - let result = if previous_bin_names.contains(&bin_name) { - async { - create_package_shim(bin_dir, &bin_name, package_name).await?; - if let Some(metadata) = previous_metadata { - BinConfig::new( - bin_name.clone(), - package_name.to_string(), - metadata.version.clone(), - metadata.platform.node.clone(), - ) - .save() - .await +impl BinRollback { + fn new() -> Result { + Ok(Self { directory: tempfile::tempdir()?, files: Vec::new() }) + } + + async fn capture_bin(&mut self, bin_dir: &AbsolutePath, bin_name: &str) -> Result<(), Error> { + #[cfg(unix)] + let suffixes = [""]; + #[cfg(windows)] + let suffixes = [".exe", ".shim", ".cmd", ".ps1", ""]; + for suffix in suffixes { + self.capture(bin_dir.join(format!("{bin_name}{suffix}"))).await?; + } + self.capture(BinConfig::path(bin_name)?).await + } + + async fn capture(&mut self, path: AbsolutePathBuf) -> Result<(), Error> { + if self.files.iter().any(|(saved, _)| *saved == path) { + return Ok(()); + } + let backup = match tokio::fs::symlink_metadata(&path).await { + Ok(metadata) => { + let backup = + AbsolutePathBuf::new(self.directory.path().join(self.files.len().to_string())) + .expect("temporary directory is absolute"); + if metadata.file_type().is_symlink() { + let target = tokio::fs::read_link(&path).await?; + #[cfg(unix)] + tokio::fs::symlink(target, &backup).await?; + #[cfg(windows)] + tokio::fs::symlink_file(target, &backup).await?; + } else if metadata.is_file() { + tokio::fs::copy(&path, &backup).await?; } else { - Ok(()) + return Err(Error::Other( + format!("Cannot replace non-file {}", path.as_path().display()).into(), + )); } + Some(backup) } - .await - } else { - async { - remove_package_shim(bin_dir, &bin_name).await?; - BinConfig::delete(&bin_name).await - } - .await + Err(error) if error.kind() == std::io::ErrorKind::NotFound => None, + Err(error) => return Err(error.into()), }; + self.files.push((path, backup)); + Ok(()) + } - if let Err(error) = result { - tracing::warn!( - "Failed to restore '{}' binary state for global package '{}': {}", - bin_name, - package_name, - error - ); + async fn restore(self) { + let mut failed = false; + for (path, backup) in self.files.into_iter().rev() { + let result = async { + if let Some(backup) = &backup { + let metadata = tokio::fs::symlink_metadata(backup).await?; + if metadata.file_type().is_symlink() { + if tokio::fs::read_link(&path).await.ok() + == Some(tokio::fs::read_link(backup).await?) + { + return Ok::<(), std::io::Error>(()); + } + } else if let Ok(current) = tokio::fs::symlink_metadata(&path).await { + if current.is_file() + && current.permissions() == metadata.permissions() + && tokio::fs::read(&path).await? == tokio::fs::read(backup).await? + { + return Ok(()); + } + } + } + + #[cfg(windows)] + { + crate::commands::env::remove_or_rename_to_old(&path).await; + match tokio::fs::symlink_metadata(&path).await { + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => return Err(error), + Ok(_) => { + return Err(std::io::Error::other( + "Could not remove replacement binary", + )); + } + } + } + #[cfg(unix)] + match tokio::fs::remove_file(&path).await { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => return Err(error), + } + if let Some(backup) = &backup { + if tokio::fs::symlink_metadata(backup).await?.file_type().is_symlink() { + let target = tokio::fs::read_link(backup).await?; + #[cfg(unix)] + tokio::fs::symlink(target, &path).await?; + #[cfg(windows)] + tokio::fs::symlink_file(target, &path).await?; + } else { + tokio::fs::copy(backup, &path).await?; + } + } + Ok(()) + } + .await; + if let Err(error) = result { + failed = true; + if let Some(backup) = backup { + output::warn(&format!( + "Failed to restore {} from {}: {error}", + path.as_path().display(), + backup.as_path().display() + )); + } else { + output::warn(&format!( + "Failed to remove new binary {}: {error}", + path.as_path().display() + )); + } + } + } + if failed { + output::warn(&format!( + "Could not fully restore global binaries. Backups kept at {}", + self.directory.keep().display() + )); } } } +async fn restore_previous_install_state( + package_name: &str, + previous_metadata: Option<&PackageMetadata>, + rollback: BinRollback, +) { + restore_package_metadata(package_name, previous_metadata).await; + rollback.restore().await; +} + async fn cleanup_failed_install(install_dir: &AbsolutePathBuf) -> Result<(), Error> { remove_dir_all_if_exists(install_dir).await } @@ -1593,6 +1647,23 @@ mod tests { "npm".to_string(), ); previous_metadata.install_id = "#123e4567-e89b-42d3-a456-426614174000".to_string(); + previous_metadata.save().await.unwrap(); + for bin_name in &previous_metadata.bins { + create_package_shim(&bin_dir, bin_name, "test-package").await.unwrap(); + BinConfig::new( + bin_name.clone(), + "test-package".into(), + "1.0.0".into(), + "20.0.0".into(), + ) + .save() + .await + .unwrap(); + } + let mut rollback = BinRollback::new().unwrap(); + rollback.capture_bin(&bin_dir, "drop").await.unwrap(); + remove_package_shim(&bin_dir, "drop").await.unwrap(); + BinConfig::delete("drop").await.unwrap(); let mut new_metadata = PackageMetadata::new( "test-package".to_string(), @@ -1607,6 +1678,7 @@ mod tests { new_metadata.save().await.unwrap(); for bin_name in ["keep", "new"] { + rollback.capture_bin(&bin_dir, bin_name).await.unwrap(); create_package_shim(&bin_dir, bin_name, "test-package").await.unwrap(); BinConfig::new( bin_name.to_string(), @@ -1619,13 +1691,8 @@ mod tests { .unwrap(); } - restore_previous_install_state( - &bin_dir, - "test-package", - Some(&previous_metadata), - &new_metadata.bins, - ) - .await; + restore_previous_install_state("test-package", Some(&previous_metadata), rollback) + .await; let restored = PackageMetadata::load("test-package").await.unwrap().unwrap(); assert_eq!(restored.install_id, previous_metadata.install_id); @@ -1647,6 +1714,126 @@ mod tests { .await; } + #[tokio::test] + async fn test_rollback_restores_regular_wrappers_and_sidecars() { + let temp = tempfile::TempDir::new().unwrap(); + let bin_dir = AbsolutePathBuf::new(temp.path().to_path_buf()).unwrap(); + let mut rollback = BinRollback::new().unwrap(); + for suffix in [".exe", ".shim", ".cmd", ".ps1", ""] { + let path = bin_dir.join(format!("tool{suffix}")); + std::fs::write(&path, b"original contents").unwrap(); + rollback.capture(path.clone()).await.unwrap(); + std::fs::write(&path, b"replacement contents").unwrap(); + rollback.capture(path.clone()).await.unwrap(); + std::fs::remove_file(&path).unwrap(); + } + rollback.restore().await; + for suffix in [".exe", ".shim", ".cmd", ".ps1", ""] { + assert_eq!( + std::fs::read(bin_dir.join(format!("tool{suffix}"))).unwrap(), + b"original contents" + ); + } + } + + #[tokio::test] + async fn test_rollback_keeps_backup_when_restoration_fails() { + let temp = tempfile::TempDir::new().unwrap(); + let parent = temp.path().join("parent"); + std::fs::create_dir(&parent).unwrap(); + let path = AbsolutePathBuf::new(parent.join("tool")).unwrap(); + std::fs::write(&path, b"original contents").unwrap(); + let mut rollback = BinRollback::new().unwrap(); + rollback.capture(path.clone()).await.unwrap(); + let backup = rollback.files[0].1.clone().unwrap(); + let backup_dir = rollback.directory.path().to_path_buf(); + std::fs::remove_file(&path).unwrap(); + std::fs::remove_dir(&parent).unwrap(); + std::fs::write(&parent, b"blocks restoration").unwrap(); + + rollback.restore().await; + + assert_eq!(std::fs::read(&backup).unwrap(), b"original contents"); + assert_eq!(std::fs::read(&parent).unwrap(), b"blocks restoration"); + std::fs::remove_dir_all(backup_dir).unwrap(); + } + + #[cfg(unix)] + #[tokio::test] + async fn test_rollback_preserves_existing_and_unattempted_binaries() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::TempDir::new().unwrap(); + vp_shared::EnvConfig::with_vars_async([(env_vars::VP_HOME, temp.path())], |_| async { + let bin_dir = get_bin_dir().unwrap(); + std::fs::create_dir_all(&bin_dir).unwrap(); + let bins = vec!["collision".to_string(), "untouched".to_string()]; + for name in &bins { + std::fs::write(bin_dir.join(name), b"original executable").unwrap(); + std::fs::set_permissions( + bin_dir.join(name), + std::fs::Permissions::from_mode(0o755), + ) + .unwrap(); + } + + let mut rollback = BinRollback::new().unwrap(); + rollback.capture_bin(&bin_dir, &bins[0]).await.unwrap(); + assert!(create_package_shim(&bin_dir, &bins[0], "new-package").await.is_err()); + restore_previous_install_state("new-package", None, rollback).await; + + for name in &bins { + assert_eq!(std::fs::read(bin_dir.join(name)).unwrap(), b"original executable"); + assert_eq!( + std::fs::metadata(bin_dir.join(name)).unwrap().permissions().mode() & 0o777, + 0o755 + ); + } + }) + .await; + } + + #[cfg(unix)] + #[tokio::test] + async fn test_rollback_restores_replaced_npm_link_and_owner() { + let temp = tempfile::TempDir::new().unwrap(); + vp_shared::EnvConfig::with_vars_async([(env_vars::VP_HOME, temp.path())], |_| async { + let bin_dir = get_bin_dir().unwrap(); + std::fs::create_dir_all(&bin_dir).unwrap(); + let old_target = temp.path().join("original-cli.js"); + std::fs::write(&old_target, b"original npm binary").unwrap(); + std::os::unix::fs::symlink(&old_target, bin_dir.join("replaced")).unwrap(); + BinConfig::new_npm("replaced".into(), "test-package".into(), "22.18.0".into()) + .save() + .await + .unwrap(); + let config_path = BinConfig::path("replaced").unwrap(); + let old_config = std::fs::read(&config_path).unwrap(); + std::fs::write(bin_dir.join("collision"), b"original executable").unwrap(); + + let mut rollback = BinRollback::new().unwrap(); + rollback.capture_bin(&bin_dir, "replaced").await.unwrap(); + create_package_shim(&bin_dir, "replaced", "test-package").await.unwrap(); + BinConfig::new( + "replaced".into(), + "test-package".into(), + "2.0.0".into(), + "22.18.0".into(), + ) + .save() + .await + .unwrap(); + rollback.capture_bin(&bin_dir, "collision").await.unwrap(); + assert!(create_package_shim(&bin_dir, "collision", "test-package").await.is_err()); + restore_previous_install_state("test-package", None, rollback).await; + + assert_eq!(std::fs::read_link(bin_dir.join("replaced")).unwrap(), old_target); + assert_eq!(std::fs::read(config_path).unwrap(), old_config); + assert_eq!(std::fs::read(bin_dir.join("collision")).unwrap(), b"original executable"); + }) + .await; + } + #[tokio::test] async fn test_cleanup_stale_installations_supports_all_layouts() { use tempfile::TempDir; From 791bfa88bdf10cecd292327ee0f654507f3a57b3 Mon Sep 17 00:00:00 2001 From: JongKyung Lee Date: Mon, 19 Oct 2026 17:54:38 +0900 Subject: [PATCH 2/5] fix(global): create rollback storage only when needed Defer temporary directory creation until an existing path needs a backup so fresh global installations do not fail when temporary storage is unavailable. Cover packages without binaries and fresh binary installations with a read-only temporary directory. --- .../snapshots.toml | 16 ++++++ .../env_install_without_backup_storage.md | 57 +++++++++++++++++++ .../src/commands/global/install.rs | 40 ++++++------- 3 files changed, 89 insertions(+), 24 deletions(-) create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_without_backup_storage.md diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots.toml b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots.toml index 3b33693d96..9aea01a566 100644 --- a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots.toml +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots.toml @@ -13,6 +13,22 @@ steps = [ { argv = ["vpt", "stat-file", "$VP_HOME/bins/env-binary-conflict-cli.json", "--assert", "missing"], comment = "Bin config should be deleted", continue-on-failure = true }, ] +[[case]] +name = "env_install_without_backup_storage" +vp = "global" +skip-platforms = ["windows"] +steps = [ + ["vpt", "mkdir", "readonly-tmp"], + ["vpt", "chmod", "555", "readonly-tmp"], + ["vpt", "json-edit", "env-binary-conflict-pkg-a/package.json", "bin", "{}"], + { argv = ["vp", "install", "-g", "./env-binary-conflict-pkg-a"], envs = [["TMPDIR", "${workspace}/readonly-tmp"]], comment = "A package without binaries needs no rollback storage" }, + ["vpt", "stat-file", "$VP_HOME/packages/env-binary-conflict-pkg-a.json", "--assert", "file"], + { argv = ["vp", "install", "-g", "./env-binary-conflict-pkg-b"], envs = [["TMPDIR", "${workspace}/readonly-tmp"]], comment = "New binaries need no backup of existing files" }, + ["env-binary-conflict-cli"], + ["vpt", "stat-file", "$VP_HOME/bins/env-binary-conflict-cli.json", "--assert", "file"], +] +after = [["vpt", "chmod", "755", "readonly-tmp"]] + [[case]] name = "env_install_rollback_preserves_existing_files" vp = "global" diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_without_backup_storage.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_without_backup_storage.md new file mode 100644 index 0000000000..91e07e40bd --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_without_backup_storage.md @@ -0,0 +1,57 @@ +# env_install_without_backup_storage + +## `vpt mkdir readonly-tmp` + +``` +``` + +## `vpt chmod 555 readonly-tmp` + +``` +``` + +## `vpt json-edit env-binary-conflict-pkg-a/package.json bin {}` + +``` +``` + +## `TMPDIR=${workspace}/readonly-tmp vp install -g ./env-binary-conflict-pkg-a` + +A package without binaries needs no rollback storage + +``` +VITE+ - The Unified Toolchain for the Web + +info: Installing 1 global package with Node.js + Installed env-binary-conflict-pkg-a 1.0.0 +``` + +## `vpt stat-file $VP_HOME/packages/env-binary-conflict-pkg-a.json --assert file` + +``` +/.vite-plus/packages/env-binary-conflict-pkg-a.json: file +``` + +## `TMPDIR=${workspace}/readonly-tmp vp install -g ./env-binary-conflict-pkg-b` + +New binaries need no backup of existing files + +``` +VITE+ - The Unified Toolchain for the Web + +info: Installing 1 global package with Node.js + Installed env-binary-conflict-pkg-b 2.0.0 + Bins: env-binary-conflict-cli +``` + +## `env-binary-conflict-cli` + +``` +Hello from pkg-b! +``` + +## `vpt stat-file $VP_HOME/bins/env-binary-conflict-cli.json --assert file` + +``` +/.vite-plus/bins/env-binary-conflict-cli.json: file +``` diff --git a/crates/vp_global_cli/src/commands/global/install.rs b/crates/vp_global_cli/src/commands/global/install.rs index 9da38841bf..27a731ca26 100644 --- a/crates/vp_global_cli/src/commands/global/install.rs +++ b/crates/vp_global_cli/src/commands/global/install.rs @@ -429,16 +429,7 @@ pub async fn install( metadata.install_id = install_id.clone(); metadata.version_spec = update_version_spec(spec); - let mut rollback = match BinRollback::new() { - Ok(rollback) => rollback, - Err(error) => { - let _ = cleanup_failed_install(&install_dir).await; - if first_error.is_none() { - first_error = Some(package_error(&package_name, error)); - } - continue; - } - }; + let mut rollback = BinRollback::default(); let result = async { for bin_name in &stale_bin_names { rollback.capture_bin(&bin_dir, bin_name).await?; @@ -618,16 +609,13 @@ async fn restore_package_metadata(package_name: &str, previous_metadata: Option< } /// Save only paths an installation is about to change, including their original owners. +#[derive(Default)] struct BinRollback { - directory: tempfile::TempDir, + directory: Option, files: Vec<(AbsolutePathBuf, Option)>, } impl BinRollback { - fn new() -> Result { - Ok(Self { directory: tempfile::tempdir()?, files: Vec::new() }) - } - async fn capture_bin(&mut self, bin_dir: &AbsolutePath, bin_name: &str) -> Result<(), Error> { #[cfg(unix)] let suffixes = [""]; @@ -645,8 +633,12 @@ impl BinRollback { } let backup = match tokio::fs::symlink_metadata(&path).await { Ok(metadata) => { + let directory = match &self.directory { + Some(directory) => directory, + None => self.directory.insert(tempfile::tempdir()?), + }; let backup = - AbsolutePathBuf::new(self.directory.path().join(self.files.len().to_string())) + AbsolutePathBuf::new(directory.path().join(self.files.len().to_string())) .expect("temporary directory is absolute"); if metadata.file_type().is_symlink() { let target = tokio::fs::read_link(&path).await?; @@ -741,10 +733,10 @@ impl BinRollback { } } } - if failed { + if failed && let Some(directory) = self.directory { output::warn(&format!( "Could not fully restore global binaries. Backups kept at {}", - self.directory.keep().display() + directory.keep().display() )); } } @@ -1660,7 +1652,7 @@ mod tests { .await .unwrap(); } - let mut rollback = BinRollback::new().unwrap(); + let mut rollback = BinRollback::default(); rollback.capture_bin(&bin_dir, "drop").await.unwrap(); remove_package_shim(&bin_dir, "drop").await.unwrap(); BinConfig::delete("drop").await.unwrap(); @@ -1718,7 +1710,7 @@ mod tests { async fn test_rollback_restores_regular_wrappers_and_sidecars() { let temp = tempfile::TempDir::new().unwrap(); let bin_dir = AbsolutePathBuf::new(temp.path().to_path_buf()).unwrap(); - let mut rollback = BinRollback::new().unwrap(); + let mut rollback = BinRollback::default(); for suffix in [".exe", ".shim", ".cmd", ".ps1", ""] { let path = bin_dir.join(format!("tool{suffix}")); std::fs::write(&path, b"original contents").unwrap(); @@ -1743,10 +1735,10 @@ mod tests { std::fs::create_dir(&parent).unwrap(); let path = AbsolutePathBuf::new(parent.join("tool")).unwrap(); std::fs::write(&path, b"original contents").unwrap(); - let mut rollback = BinRollback::new().unwrap(); + let mut rollback = BinRollback::default(); rollback.capture(path.clone()).await.unwrap(); let backup = rollback.files[0].1.clone().unwrap(); - let backup_dir = rollback.directory.path().to_path_buf(); + let backup_dir = rollback.directory.as_ref().unwrap().path().to_path_buf(); std::fs::remove_file(&path).unwrap(); std::fs::remove_dir(&parent).unwrap(); std::fs::write(&parent, b"blocks restoration").unwrap(); @@ -1777,7 +1769,7 @@ mod tests { .unwrap(); } - let mut rollback = BinRollback::new().unwrap(); + let mut rollback = BinRollback::default(); rollback.capture_bin(&bin_dir, &bins[0]).await.unwrap(); assert!(create_package_shim(&bin_dir, &bins[0], "new-package").await.is_err()); restore_previous_install_state("new-package", None, rollback).await; @@ -1811,7 +1803,7 @@ mod tests { let old_config = std::fs::read(&config_path).unwrap(); std::fs::write(bin_dir.join("collision"), b"original executable").unwrap(); - let mut rollback = BinRollback::new().unwrap(); + let mut rollback = BinRollback::default(); rollback.capture_bin(&bin_dir, "replaced").await.unwrap(); create_package_shim(&bin_dir, "replaced", "test-package").await.unwrap(); BinConfig::new( From 288eb4b897e996161ac26148a6c06a68124f6ba4 Mon Sep 17 00:00:00 2001 From: JongKyung Lee Date: Mon, 19 Oct 2026 19:21:19 +0900 Subject: [PATCH 3/5] fix(global): avoid loading rollback files into memory Reject foreign regular files at new shim paths before activation and restore backups without comparing full file contents. Keep lazy disk backups so permissions survive rollback and failed restorations retain recoverable files. Cover permission restoration and preserve CLI coverage for existing files and npm ownership after installation failures. --- .../snapshots.toml | 7 +- ...stall_rollback_preserves_existing_files.md | 2 +- ...env_install_rollback_restores_npm_owner.md | 17 +-- .../src/commands/global/install.rs | 114 ++++++++++-------- 4 files changed, 76 insertions(+), 64 deletions(-) diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots.toml b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots.toml index 9aea01a566..276ef52cfc 100644 --- a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots.toml +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots.toml @@ -57,13 +57,12 @@ steps = [ { tty = false, argv = ["npm", "install", "-g", "./env-binary-conflict-pkg-a"] }, ["vpt", "print-file", "$VP_HOME/bins/env-binary-conflict-cli.json"], ["vpt", "json-edit", "env-binary-conflict-pkg-a/package.json", "bin.zzz-rollback-cli", "./cli.js"], - ["vpt", "write-file", "$VP_HOME/bin/zzz-rollback-cli", "#!/bin/sh\necho original-blocker\n"], - ["vpt", "chmod", "+x", "$VP_HOME/bin/zzz-rollback-cli"], - { argv = ["vp", "install", "-g", "./env-binary-conflict-pkg-a"], comment = "A later collision must restore the original npm link and ownership", continue-on-failure = true }, + ["vpt", "mkdir", "$VP_HOME/bin/zzz-rollback-cli"], + { argv = ["vp", "install", "-g", "./env-binary-conflict-pkg-a"], comment = "A failure after replacing the npm link must restore the link and its ownership", continue-on-failure = true }, { argv = ["vpt", "stat-file", "$VP_HOME/bin/env-binary-conflict-cli", "--assert", "symlink"], continue-on-failure = true }, ["env-binary-conflict-cli"], ["vpt", "print-file", "$VP_HOME/bins/env-binary-conflict-cli.json"], - ["zzz-rollback-cli"], + ["vpt", "stat-file", "$VP_HOME/bin/zzz-rollback-cli", "--assert", "dir"], ["vpt", "stat-file", "$VP_HOME/packages/env-binary-conflict-pkg-a.json", "--assert", "missing"], ["vpt", "stat-file", "$VP_HOME/bins/zzz-rollback-cli.json", "--assert", "missing"], ] diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_preserves_existing_files.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_preserves_existing_files.md index 085c09c887..fff8cbb85d 100644 --- a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_preserves_existing_files.md +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_preserves_existing_files.md @@ -39,7 +39,7 @@ A regular file prevents shim creation without authorizing rollback to delete exi VITE+ - The Unified Toolchain for the Web info: Installing 1 global package with Node.js -error: Failed to install env-binary-conflict-pkg-a: Command execution failed: File exists (os error 17) +error: Failed to install env-binary-conflict-pkg-a: /.vite-plus/bin/aaa-rollback-cli already exists and is not managed by Vite+ ``` ## `vpt stat-file $VP_HOME/bin/aaa-rollback-cli --assert file` diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_restores_npm_owner.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_restores_npm_owner.md index 33f2a0592b..5c99fac992 100644 --- a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_restores_npm_owner.md +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/env_install_binary_conflict/snapshots/env_install_rollback_restores_npm_owner.md @@ -25,21 +25,14 @@ Linked 'env-binary-conflict-cli' to /.vite-plus/bin/env-binary-conflict-cl ``` ``` -## `vpt write-file $VP_HOME/bin/zzz-rollback-cli '#'\!'/bin/sh -echo original-blocker -'` - -``` -``` - -## `vpt chmod +x $VP_HOME/bin/zzz-rollback-cli` +## `vpt mkdir $VP_HOME/bin/zzz-rollback-cli` ``` ``` ## `vp install -g ./env-binary-conflict-pkg-a` -A later collision must restore the original npm link and ownership +A failure after replacing the npm link must restore the link and its ownership **Exit code:** 1 @@ -47,7 +40,7 @@ A later collision must restore the original npm link and ownership VITE+ - The Unified Toolchain for the Web info: Installing 1 global package with Node.js -error: Failed to install env-binary-conflict-pkg-a: Command execution failed: File exists (os error 17) +error: Failed to install env-binary-conflict-pkg-a: Cannot replace non-file /.vite-plus/bin/zzz-rollback-cli ``` ## `vpt stat-file $VP_HOME/bin/env-binary-conflict-cli --assert symlink` @@ -74,10 +67,10 @@ Hello from pkg-a! } ``` -## `zzz-rollback-cli` +## `vpt stat-file $VP_HOME/bin/zzz-rollback-cli --assert dir` ``` -original-blocker +/.vite-plus/bin/zzz-rollback-cli: dir ``` ## `vpt stat-file $VP_HOME/packages/env-binary-conflict-pkg-a.json --assert missing` diff --git a/crates/vp_global_cli/src/commands/global/install.rs b/crates/vp_global_cli/src/commands/global/install.rs index 27a731ca26..344d223ae3 100644 --- a/crates/vp_global_cli/src/commands/global/install.rs +++ b/crates/vp_global_cli/src/commands/global/install.rs @@ -415,6 +415,23 @@ pub async fn install( continue; } }; + // Reject foreign files at new shim paths before activating the installation. + if let Some(path) = bin_names.iter().find_map(|bin_name| foreign_bin(&bin_dir, bin_name)) { + let _ = cleanup_failed_install(&install_dir).await; + if first_error.is_none() { + first_error = Some(package_error( + &package_name, + Error::Other( + format!( + "{} already exists and is not managed by Vite+", + path.as_path().display() + ) + .into(), + ), + )); + } + continue; + } let metadata_version = installed_version.as_deref().unwrap_or("unknown"); let mut metadata = PackageMetadata::new( @@ -608,6 +625,20 @@ async fn restore_package_metadata(package_name: &str, previous_metadata: Option< } } +/// The regular file at `bin_name`'s shim path when Vite+ does not own it. +fn foreign_bin(bin_dir: &AbsolutePath, bin_name: &str) -> Option { + if is_protected_shim(bin_name, true) { + return None; + } + let path = if cfg!(windows) { + bin_dir.join(format!("{bin_name}.exe")) + } else { + bin_dir.join(bin_name) + }; + let metadata = std::fs::symlink_metadata(path.as_path()).ok()?; + (metadata.is_file() && !is_vp_shim_target(&path)).then_some(path) +} + /// Save only paths an installation is about to change, including their original owners. #[derive(Default)] struct BinRollback { @@ -628,7 +659,7 @@ impl BinRollback { } async fn capture(&mut self, path: AbsolutePathBuf) -> Result<(), Error> { - if self.files.iter().any(|(saved, _)| *saved == path) { + if self.files.iter().any(|(captured, _)| *captured == path) { return Ok(()); } let backup = match tokio::fs::symlink_metadata(&path).await { @@ -668,19 +699,11 @@ impl BinRollback { let result = async { if let Some(backup) = &backup { let metadata = tokio::fs::symlink_metadata(backup).await?; - if metadata.file_type().is_symlink() { - if tokio::fs::read_link(&path).await.ok() + if metadata.file_type().is_symlink() + && tokio::fs::read_link(&path).await.ok() == Some(tokio::fs::read_link(backup).await?) - { - return Ok::<(), std::io::Error>(()); - } - } else if let Ok(current) = tokio::fs::symlink_metadata(&path).await { - if current.is_file() - && current.permissions() == metadata.permissions() - && tokio::fs::read(&path).await? == tokio::fs::read(backup).await? - { - return Ok(()); - } + { + return Ok::<(), std::io::Error>(()); } } @@ -1728,6 +1751,27 @@ mod tests { } } + #[tokio::test] + async fn test_rollback_restores_changed_regular_files() { + let temp = tempfile::TempDir::new().unwrap(); + let path = AbsolutePathBuf::new(temp.path().join("tool")).unwrap(); + std::fs::write(&path, b"original contents").unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + let permissions = std::fs::metadata(&path).unwrap().permissions(); + let mut rollback = BinRollback::default(); + rollback.capture(path.clone()).await.unwrap(); + std::fs::write(&path, b"replacement contents").unwrap(); + + rollback.restore().await; + + assert_eq!(std::fs::read(&path).unwrap(), b"original contents"); + assert_eq!(std::fs::metadata(&path).unwrap().permissions(), permissions); + } + #[tokio::test] async fn test_rollback_keeps_backup_when_restoration_fails() { let temp = tempfile::TempDir::new().unwrap(); @@ -1751,38 +1795,18 @@ mod tests { } #[cfg(unix)] - #[tokio::test] - async fn test_rollback_preserves_existing_and_unattempted_binaries() { - use std::os::unix::fs::PermissionsExt; - + #[test] + fn test_foreign_bin_detects_only_unowned_regular_files() { let temp = tempfile::TempDir::new().unwrap(); - vp_shared::EnvConfig::with_vars_async([(env_vars::VP_HOME, temp.path())], |_| async { - let bin_dir = get_bin_dir().unwrap(); - std::fs::create_dir_all(&bin_dir).unwrap(); - let bins = vec!["collision".to_string(), "untouched".to_string()]; - for name in &bins { - std::fs::write(bin_dir.join(name), b"original executable").unwrap(); - std::fs::set_permissions( - bin_dir.join(name), - std::fs::Permissions::from_mode(0o755), - ) - .unwrap(); - } + let bin_dir = AbsolutePathBuf::new(temp.path().to_path_buf()).unwrap(); + std::fs::write(bin_dir.join("foreign"), b"original executable").unwrap(); + std::os::unix::fs::symlink(temp.path().join("cli.js"), bin_dir.join("npm-link")).unwrap(); + std::fs::write(bin_dir.join("node"), b"protected shim").unwrap(); - let mut rollback = BinRollback::default(); - rollback.capture_bin(&bin_dir, &bins[0]).await.unwrap(); - assert!(create_package_shim(&bin_dir, &bins[0], "new-package").await.is_err()); - restore_previous_install_state("new-package", None, rollback).await; - - for name in &bins { - assert_eq!(std::fs::read(bin_dir.join(name)).unwrap(), b"original executable"); - assert_eq!( - std::fs::metadata(bin_dir.join(name)).unwrap().permissions().mode() & 0o777, - 0o755 - ); - } - }) - .await; + assert_eq!(foreign_bin(&bin_dir, "foreign"), Some(bin_dir.join("foreign"))); + assert_eq!(foreign_bin(&bin_dir, "npm-link"), None); + assert_eq!(foreign_bin(&bin_dir, "missing"), None); + assert_eq!(foreign_bin(&bin_dir, "node"), None); } #[cfg(unix)] @@ -1801,7 +1825,6 @@ mod tests { .unwrap(); let config_path = BinConfig::path("replaced").unwrap(); let old_config = std::fs::read(&config_path).unwrap(); - std::fs::write(bin_dir.join("collision"), b"original executable").unwrap(); let mut rollback = BinRollback::default(); rollback.capture_bin(&bin_dir, "replaced").await.unwrap(); @@ -1815,13 +1838,10 @@ mod tests { .save() .await .unwrap(); - rollback.capture_bin(&bin_dir, "collision").await.unwrap(); - assert!(create_package_shim(&bin_dir, "collision", "test-package").await.is_err()); restore_previous_install_state("test-package", None, rollback).await; assert_eq!(std::fs::read_link(bin_dir.join("replaced")).unwrap(), old_target); assert_eq!(std::fs::read(config_path).unwrap(), old_config); - assert_eq!(std::fs::read(bin_dir.join("collision")).unwrap(), b"original executable"); }) .await; } From be182726c48c69047c6c81307897dea1a85a9183 Mon Sep 17 00:00:00 2001 From: JongKyung Lee Date: Mon, 19 Oct 2026 19:50:34 +0900 Subject: [PATCH 4/5] fix(global): keep protected shims untouched during rollback Skip shim capture for protected stale bin names while retaining BinConfig backups. Add regression coverage for file identity preservation and config restoration after a later failure. --- .../src/commands/global/install.rs | 64 ++++++++++++++++++- 1 file changed, 62 insertions(+), 2 deletions(-) diff --git a/crates/vp_global_cli/src/commands/global/install.rs b/crates/vp_global_cli/src/commands/global/install.rs index 344d223ae3..d6c86904e5 100644 --- a/crates/vp_global_cli/src/commands/global/install.rs +++ b/crates/vp_global_cli/src/commands/global/install.rs @@ -652,8 +652,10 @@ impl BinRollback { let suffixes = [""]; #[cfg(windows)] let suffixes = [".exe", ".shim", ".cmd", ".ps1", ""]; - for suffix in suffixes { - self.capture(bin_dir.join(format!("{bin_name}{suffix}"))).await?; + if !is_protected_shim(bin_name, false) { + for suffix in suffixes { + self.capture(bin_dir.join(format!("{bin_name}{suffix}"))).await?; + } } self.capture(BinConfig::path(bin_name)?).await } @@ -1729,6 +1731,64 @@ mod tests { .await; } + #[tokio::test] + async fn test_rollback_preserves_protected_shims_and_restores_bin_config() { + let temp = tempfile::TempDir::new().unwrap(); + vp_shared::EnvConfig::with_vars_async([(env_vars::VP_HOME, temp.path())], |_| async { + let bin_dir = get_bin_dir().unwrap(); + std::fs::create_dir_all(&bin_dir).unwrap(); + #[cfg(unix)] + let suffixes = [""]; + #[cfg(windows)] + let suffixes = [".exe", ".shim", ".cmd", ".ps1", ""]; + for suffix in suffixes { + let path = bin_dir.join(format!("node{suffix}")); + std::fs::write(&path, b"protected shim").unwrap(); + std::fs::hard_link(&path, temp.path().join(format!("original-node{suffix}"))) + .unwrap(); + } + BinConfig::new( + "node".into(), + "legacy-package".into(), + "1.0.0".into(), + "22.18.0".into(), + ) + .save() + .await + .unwrap(); + let config_path = BinConfig::path("node").unwrap(); + let original_config = std::fs::read(&config_path).unwrap(); + let stale_bins = + stale_bin_names_for_package(None, "legacy-package", &[]).await.unwrap(); + assert_eq!(stale_bins, ["node"]); + + let mut rollback = BinRollback::default(); + for bin_name in stale_bins { + rollback.capture_bin(&bin_dir, &bin_name).await.unwrap(); + remove_package_shim(&bin_dir, &bin_name).await.unwrap(); + BinConfig::delete(&bin_name).await.unwrap(); + } + std::fs::create_dir(bin_dir.join(format!("blocked{}", std::env::consts::EXE_SUFFIX))) + .unwrap(); + assert!(rollback.capture_bin(&bin_dir, "blocked").await.is_err()); + rollback.restore().await; + + assert_eq!(std::fs::read(&config_path).unwrap(), original_config); + for suffix in suffixes { + let path = bin_dir.join(format!("node{suffix}")); + assert_eq!(std::fs::read(&path).unwrap(), b"protected shim"); + assert!( + same_file::is_same_file( + &path, + temp.path().join(format!("original-node{suffix}")) + ) + .unwrap() + ); + } + }) + .await; + } + #[tokio::test] async fn test_rollback_restores_regular_wrappers_and_sidecars() { let temp = tempfile::TempDir::new().unwrap(); From 48928582cbc69d640ca10f7b33af687881b288aa Mon Sep 17 00:00:00 2001 From: JongKyung Lee Date: Mon, 19 Oct 2026 20:05:05 +0900 Subject: [PATCH 5/5] fix(global): exclude incomplete captures from rollback Discard rollback entries added by a failed capture_bin call because no changes to those paths have started. Preserve earlier backups so prior changes still roll back. Add regression coverage for untouched hard links and restoration of earlier binaries and ownership configs. --- .../src/commands/global/install.rs | 61 ++++++++++++++++--- 1 file changed, 53 insertions(+), 8 deletions(-) diff --git a/crates/vp_global_cli/src/commands/global/install.rs b/crates/vp_global_cli/src/commands/global/install.rs index d6c86904e5..b0ea93064b 100644 --- a/crates/vp_global_cli/src/commands/global/install.rs +++ b/crates/vp_global_cli/src/commands/global/install.rs @@ -648,16 +648,25 @@ struct BinRollback { impl BinRollback { async fn capture_bin(&mut self, bin_dir: &AbsolutePath, bin_name: &str) -> Result<(), Error> { - #[cfg(unix)] - let suffixes = [""]; - #[cfg(windows)] - let suffixes = [".exe", ".shim", ".cmd", ".ps1", ""]; - if !is_protected_shim(bin_name, false) { - for suffix in suffixes { - self.capture(bin_dir.join(format!("{bin_name}{suffix}"))).await?; + let captured_len = self.files.len(); + let result = async { + #[cfg(unix)] + let suffixes = [""]; + #[cfg(windows)] + let suffixes = [".exe", ".shim", ".cmd", ".ps1", ""]; + if !is_protected_shim(bin_name, false) { + for suffix in suffixes { + self.capture(bin_dir.join(format!("{bin_name}{suffix}"))).await?; + } } + self.capture(BinConfig::path(bin_name)?).await + } + .await; + if result.is_err() { + // The caller has not changed these paths when capture fails. + self.files.truncate(captured_len); } - self.capture(BinConfig::path(bin_name)?).await + result } async fn capture(&mut self, path: AbsolutePathBuf) -> Result<(), Error> { @@ -1789,6 +1798,42 @@ mod tests { .await; } + #[tokio::test] + async fn test_failed_capture_preserves_untouched_files_and_prior_backups() { + let temp = tempfile::TempDir::new().unwrap(); + vp_shared::EnvConfig::with_vars_async([(env_vars::VP_HOME, temp.path())], |_| async { + let bin_dir = get_bin_dir().unwrap(); + std::fs::create_dir_all(&bin_dir).unwrap(); + let changed = bin_dir.join(format!("changed{}", std::env::consts::EXE_SUFFIX)); + std::fs::write(&changed, b"original contents").unwrap(); + BinConfig::new("changed".into(), "package".into(), "1.0.0".into(), "22.18.0".into()) + .save() + .await + .unwrap(); + let config_path = BinConfig::path("changed").unwrap(); + let original_config = std::fs::read(&config_path).unwrap(); + let mut rollback = BinRollback::default(); + rollback.capture_bin(&bin_dir, "changed").await.unwrap(); + std::fs::write(&changed, b"replacement contents").unwrap(); + BinConfig::delete("changed").await.unwrap(); + + let untouched = bin_dir.join(format!("untouched{}", std::env::consts::EXE_SUFFIX)); + std::fs::write(&untouched, b"untouched contents").unwrap(); + let original = temp.path().join("original-untouched"); + std::fs::hard_link(&untouched, &original).unwrap(); + std::fs::create_dir(BinConfig::path("untouched").unwrap()).unwrap(); + assert!(rollback.capture_bin(&bin_dir, "untouched").await.is_err()); + + rollback.restore().await; + + assert_eq!(std::fs::read(&changed).unwrap(), b"original contents"); + assert_eq!(std::fs::read(&config_path).unwrap(), original_config); + assert_eq!(std::fs::read(&untouched).unwrap(), b"untouched contents"); + assert!(same_file::is_same_file(&untouched, &original).unwrap()); + }) + .await; + } + #[tokio::test] async fn test_rollback_restores_regular_wrappers_and_sidecars() { let temp = tempfile::TempDir::new().unwrap();