From 8b1aa7fa41bce38e49b74a43354df2ff4ab641f5 Mon Sep 17 00:00:00 2001 From: JongKyung Lee Date: Thu, 22 Oct 2026 10:40:31 +0900 Subject: [PATCH 1/3] fix(global): serialize package activation and cleanup Protect ownership checks, activation, rollback, and stale installation cleanup with a shared global bin lock so concurrent installs cannot delete the active package. Use the same lock for removal and npm link updates while keeping npm preparation parallel and user prompts outside the lock. Cover concurrent installation, forced replacement, lock contention and process termination, and ownership changes while an npm link prompt is open. --- .../global_install_concurrent/prepare.mjs | 20 +++ .../global_install_concurrent/snapshots.toml | 8 + .../snapshots/global_install_concurrent.md | 10 ++ .../global_install_concurrent/verify.mjs | 64 ++++++++ .../src/commands/global/bin_lock.rs | 154 ++++++++++++++++++ .../src/commands/global/install.rs | 139 ++++++++++++++-- .../vp_global_cli/src/commands/global/mod.rs | 1 + crates/vp_global_cli/src/shim/dispatch.rs | 102 +++++++++++- 8 files changed, 481 insertions(+), 17 deletions(-) create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/prepare.mjs create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/snapshots.toml create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/snapshots/global_install_concurrent.md create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/verify.mjs create mode 100644 crates/vp_global_cli/src/commands/global/bin_lock.rs diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/prepare.mjs b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/prepare.mjs new file mode 100644 index 0000000000..9447fcb902 --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/prepare.mjs @@ -0,0 +1,20 @@ +import { access, writeFile } from 'node:fs/promises'; +import { join } from 'node:path'; +import { setTimeout } from 'node:timers/promises'; + +const { VP_TEST_BARRIER: barrier, VP_TEST_PARTICIPANT: participant } = process.env; +if (barrier) { + await writeFile(join(barrier, participant), 'ready'); + const other = participant === 'first' ? 'second' : 'first'; + const deadline = Date.now() + 20_000; + while (true) { + try { + await access(join(barrier, other)); + break; + } catch (error) { + if (error.code !== 'ENOENT') throw error; + if (Date.now() >= deadline) throw new Error('npm preparation was serialized'); + await setTimeout(10); + } + } +} diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/snapshots.toml b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/snapshots.toml new file mode 100644 index 0000000000..242b0fd652 --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/snapshots.toml @@ -0,0 +1,8 @@ +[[case]] +name = "global_install_concurrent" +vp = "global" +comment = "Concurrent npm preparation must remain parallel, and activation must leave an executable active installation. Forced replacement must not deadlock." +env = { npm_config_offline = "true", npm_config_audit = "false" } +steps = [ + { argv = ["node", "verify.mjs"], tty = false }, +] diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/snapshots/global_install_concurrent.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/snapshots/global_install_concurrent.md new file mode 100644 index 0000000000..a169d48ffa --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/snapshots/global_install_concurrent.md @@ -0,0 +1,10 @@ +# global_install_concurrent + +Concurrent npm preparation must remain parallel, and activation must leave an executable active installation. Forced replacement must not deadlock. + +## `node verify.mjs` + +``` +Concurrent preparation completed; the active installation and command survived. +Forced replacement and removal completed without deadlock. +``` diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/verify.mjs b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/verify.mjs new file mode 100644 index 0000000000..d79a0b6a40 --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/global_install_concurrent/verify.mjs @@ -0,0 +1,64 @@ +import assert from 'node:assert/strict'; +import { spawn } from 'node:child_process'; +import { mkdir, readFile, readdir, writeFile } from 'node:fs/promises'; +import { join, resolve } from 'node:path'; + +const home = process.env.VP_HOME; +const executable = (name) => join(home, 'bin', name + (process.platform === 'win32' ? '.exe' : '')); +const vp = executable('vp'); +const packages = join(home, 'packages'); +const name = 'global-lock-demo'; +const barrier = resolve('barrier'); +await mkdir(barrier); + +async function makePackage(directory, packageName, version) { + await mkdir(directory); + await writeFile(join(directory, 'package.json'), JSON.stringify({ + name: packageName, + version, + bin: { [name]: 'cli.js' }, + scripts: { postinstall: 'node ../prepare.mjs' }, + })); + await writeFile(join(directory, 'cli.js'), `#!/usr/bin/env node\nconsole.log('${version}');\n`); +} + +function run(binary, args, env = {}) { + return new Promise((resolve, reject) => { + const child = spawn(binary, args, { + env: { ...process.env, ...env }, + stdio: ['ignore', 'pipe', 'pipe'], + timeout: 30_000, + killSignal: 'SIGKILL', + }); + let output = ''; + child.stdout.on('data', (chunk) => { output += chunk; }); + child.stderr.on('data', (chunk) => { output += chunk; }); + child.on('error', reject); + child.on('close', (code) => { + if (code === 0) resolve(output.trim()); + else reject(new Error(`${args.join(' ')} exited ${code}: ${output}`)); + }); + }); +} + +await makePackage('first', name, '1.0.0'); +await makePackage('second', name, '2.0.0'); +await Promise.all(['first', 'second'].map((participant) => run(vp, ['install', '-g', `./${participant}`], { + VP_TEST_BARRIER: barrier, + VP_TEST_PARTICIPANT: participant, +}))); + +const metadata = JSON.parse(await readFile(join(packages, `${name}.json`), 'utf8')); +const installations = (await readdir(join(packages, name), { withFileTypes: true })) + .filter((entry) => entry.isDirectory()).map((entry) => entry.name); +assert.deepEqual(installations, [metadata.installId]); +assert.equal(await run(executable(name), []), metadata.version); +console.log('Concurrent preparation completed; the active installation and command survived.'); + +await makePackage('replacement', 'global-lock-replacement', '3.0.0'); +await run(vp, ['install', '-g', '--force', './replacement']); +assert.equal(await run(executable(name), []), '3.0.0'); +await assert.rejects(readFile(join(packages, `${name}.json`)), { code: 'ENOENT' }); +await run(vp, ['uninstall', '-g', 'global-lock-replacement']); +await assert.rejects(readFile(join(packages, 'global-lock-replacement.json')), { code: 'ENOENT' }); +console.log('Forced replacement and removal completed without deadlock.'); diff --git a/crates/vp_global_cli/src/commands/global/bin_lock.rs b/crates/vp_global_cli/src/commands/global/bin_lock.rs new file mode 100644 index 0000000000..30d61a702c --- /dev/null +++ b/crates/vp_global_cli/src/commands/global/bin_lock.rs @@ -0,0 +1,154 @@ +//! Serialize global bin ownership checks, activation, rollback, and cleanup. + +use std::{ + fs::{File, OpenOptions, TryLockError}, + time::{Duration, Instant}, +}; + +use crate::{commands::env::config::get_bin_dir, error::Error}; + +const WAIT: Duration = Duration::from_mins(5); +const POLL_INTERVAL: Duration = Duration::from_millis(50); + +fn open_lock_file() -> Result { + let bin_dir = get_bin_dir()?; + std::fs::create_dir_all(&bin_dir)?; + // Keep the file in place so all writers lock the same inode. + Ok(OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .open(bin_dir.join(".vp-global-bin.lock"))?) +} + +fn try_lock(file: &File, deadline: Instant) -> Result { + match file.try_lock() { + Ok(()) => Ok(true), + Err(TryLockError::WouldBlock) if Instant::now() < deadline => Ok(false), + Err(TryLockError::WouldBlock) => Err(Error::Other( + "Timed out waiting for another global package operation to finish.".into(), + )), + Err(TryLockError::Error(error)) => Err(error.into()), + } +} + +/// Hold the returned file until all shared-state changes and cleanup finish. +pub(crate) async fn acquire() -> Result { + let file = open_lock_file()?; + let deadline = Instant::now() + WAIT; + while !try_lock(&file, deadline)? { + tokio::time::sleep(POLL_INTERVAL).await; + } + Ok(file) +} + +/// Synchronous counterpart for npm shim dispatch, outside prompts and npm execution. +pub(crate) fn acquire_sync() -> Result { + let file = open_lock_file()?; + let deadline = Instant::now() + WAIT; + while !try_lock(&file, deadline)? { + std::thread::sleep(POLL_INTERVAL); + } + Ok(file) +} + +#[cfg(test)] +mod tests { + use vp_shared::{EnvConfig, env_vars}; + + use super::*; + + #[tokio::test] + async fn async_wait_does_not_block_and_drop_releases_lock() { + let temp = tempfile::tempdir().unwrap(); + EnvConfig::with_vars_async([(env_vars::VP_HOME, temp.path())], |_| async { + let first = acquire_sync().unwrap(); + let second = acquire(); + tokio::pin!(second); + assert!(tokio::time::timeout(Duration::from_millis(150), &mut second).await.is_err()); + drop(first); + let second = + tokio::time::timeout(Duration::from_secs(5), second).await.unwrap().unwrap(); + let contender = open_lock_file().unwrap(); + assert!(try_lock(&contender, Instant::now()).is_err()); + drop(second); + assert!(try_lock(&contender, Instant::now()).unwrap()); + }) + .await; + } + + #[test] + fn lock_child_process() { + let Some(marker_dir) = std::env::var_os("VP_TEST_GLOBAL_LOCK_MARKERS") else { return }; + let marker_dir = std::path::PathBuf::from(marker_dir); + std::fs::write(marker_dir.join("started"), b"").unwrap(); + let _lock = acquire_sync().unwrap(); + std::fs::write(marker_dir.join("acquired"), b"").unwrap(); + let mut byte = [0]; + std::io::Read::read(&mut std::io::stdin(), &mut byte).unwrap(); + } + + #[tokio::test] + async fn serializes_processes_and_releases_lock_after_kill() { + let temp = tempfile::tempdir().unwrap(); + let home = temp.path().join("home"); + let spawn = |name: &str| { + let markers = temp.path().join(name); + std::fs::create_dir_all(&markers).unwrap(); + let child = tokio::process::Command::new(std::env::current_exe().unwrap()) + .args(["--exact", "commands::global::bin_lock::tests::lock_child_process"]) + .env(env_vars::VP_HOME, &home) + .env("VP_TEST_GLOBAL_LOCK_MARKERS", &markers) + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::null()) + .kill_on_drop(true) + .spawn() + .unwrap(); + (child, markers) + }; + let wait_for = |path: std::path::PathBuf| async move { + tokio::time::timeout(Duration::from_secs(10), async { + while !path.exists() { + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .unwrap(); + }; + let (mut first, first_markers) = spawn("first"); + wait_for(first_markers.join("acquired")).await; + let (mut second, second_markers) = spawn("second"); + wait_for(second_markers.join("started")).await; + assert!( + tokio::time::timeout( + Duration::from_millis(150), + wait_for(second_markers.join("acquired")) + ) + .await + .is_err() + ); + first.kill().await.unwrap(); + wait_for(second_markers.join("acquired")).await; + drop(second.stdin.take()); + assert!( + tokio::time::timeout(Duration::from_secs(5), second.wait()) + .await + .unwrap() + .unwrap() + .success() + ); + } + + #[test] + fn contention_times_out_without_taking_lock() { + let temp = tempfile::tempdir().unwrap(); + EnvConfig::with_vars([(env_vars::VP_HOME, temp.path())], |_| { + let _first = acquire_sync().unwrap(); + let second = open_lock_file().unwrap(); + let error = try_lock(&second, Instant::now()).unwrap_err(); + assert!(error.to_string().contains("Timed out waiting")); + assert!(matches!(second.try_lock(), Err(TryLockError::WouldBlock))); + }); + } +} diff --git a/crates/vp_global_cli/src/commands/global/install.rs b/crates/vp_global_cli/src/commands/global/install.rs index 7e753f801e..94f8bdfaca 100644 --- a/crates/vp_global_cli/src/commands/global/install.rs +++ b/crates/vp_global_cli/src/commands/global/install.rs @@ -26,7 +26,7 @@ use crate::{ package_metadata::{PackageMetadata, is_legacy_install_id, is_nested_install_id}, }, global::{ - LEGACY_PACKAGE_MANAGER_PACKAGES, is_local_package_spec, parse_package_spec, + LEGACY_PACKAGE_MANAGER_PACKAGES, bin_lock, is_local_package_spec, parse_package_spec, update_version_spec, }, }, @@ -274,7 +274,6 @@ pub async fn install( progress.finish_and_clear(); // 5. Finalize installed packages. - let mut bin_owners = HashMap::::new(); for (index, (package_name, Package { spec, install })) in packages.into_iter().enumerate() { let lock_file = install_locks.remove(&package_name); let Some(InstalledPackage { @@ -288,6 +287,18 @@ pub async fn install( continue; }; + // Re-read ownership under the shared lock and hold it through rollback and cleanup. + let activation_lock = match bin_lock::acquire().await { + Ok(lock) => lock, + Err(error) => { + let _ = cleanup_failed_install(&install_dir).await; + if first_error.is_none() { + first_error = Some(package_error(&package_name, error)); + } + continue; + } + }; + // Load previous metadata once for stale-bin detection below. let previous_metadata = match PackageMetadata::load(&package_name).await { Ok(metadata) => metadata, @@ -337,13 +348,6 @@ pub async fn install( // 5.1 Detect binary ownership conflicts before writing metadata. for bin_name in &bin_names { - if let Some(owner) = bin_owners.get(bin_name) - && owner != &package_name - { - conflicts.push((bin_name.clone(), owner.clone())); - continue; - } - match BinConfig::load(bin_name).await { Ok(Some(config)) => { if config.package != package_name { @@ -376,7 +380,7 @@ pub async fn install( "Uninstalling {} (conflicts with {})...", pkg, package_name )); - if let Err(error) = Box::pin(uninstall(&pkg, false)).await { + if let Err(error) = uninstall_locked(&pkg, false).await { let _ = cleanup_failed_install(&install_dir).await; if first_error.is_none() { first_error = Some(package_error(&package_name, error)); @@ -484,13 +488,10 @@ pub async fn install( continue; } - for bin_name in &bin_names { - bin_owners.insert(bin_name.clone(), package_name.clone()); - } - // 5.6 Remove stale installations for this package. cleanup_stale_installations(&package_name, &install_id).await; drop(lock_file); + drop(activation_lock); // 5.7 Print success message output::raw(&format!( @@ -1071,6 +1072,12 @@ async fn stale_bin_names_for_package( /// 1. Try to use PackageMetadata for binary list /// 2. Fallback to scanning BinConfig files for orphaned binaries pub async fn uninstall(package_name: &str, dry_run: bool) -> Result<(), Error> { + let _lock = bin_lock::acquire().await?; + uninstall_locked(package_name, dry_run).await +} + +// Called directly by forced activation, which already holds the global bin lock. +async fn uninstall_locked(package_name: &str, dry_run: bool) -> Result<(), Error> { if is_local_package_spec(package_name) { // We can't resolve local packages for uninstall, follow npm's behavior return Err(Error::Other( @@ -1402,6 +1409,110 @@ mod tests { trampoline } + #[tokio::test] + async fn test_uninstall_waits_for_global_activation() { + let temp = tempfile::tempdir().unwrap(); + vp_shared::EnvConfig::with_vars_async([(env_vars::VP_HOME, temp.path())], |_| async { + let metadata = PackageMetadata::new( + "test-package".into(), + "1.0.0".into(), + "22.18.0".into(), + None, + vec![], + HashSet::new(), + "npm".into(), + ); + metadata.save().await.unwrap(); + let bin_dir = get_bin_dir().unwrap(); + std::fs::create_dir_all(&bin_dir).unwrap(); + let lock = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .open(bin_dir.join(".vp-global-bin.lock")) + .unwrap(); + lock.lock().unwrap(); + + let removal = uninstall("test-package", false); + tokio::pin!(removal); + assert!(tokio::time::timeout(Duration::from_millis(150), &mut removal).await.is_err()); + assert!(PackageMetadata::load("test-package").await.unwrap().is_some()); + drop(lock); + tokio::time::timeout(Duration::from_secs(5), removal).await.unwrap().unwrap(); + assert!(PackageMetadata::load("test-package").await.unwrap().is_none()); + }) + .await; + } + + #[cfg(unix)] + #[tokio::test] + async fn test_concurrent_installs_prepare_outside_lock_and_keep_active_installation() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().unwrap(); + vp_shared::EnvConfig::with_vars_async([(env_vars::VP_HOME, temp.path())], |env| async move { + // A local npm stand-in exercises activation without downloads or lifecycle scripts. + let node_bin = env.dirs.data.join("js_runtime/node/22.18.0/bin"); + std::fs::create_dir_all(&node_bin).unwrap(); + std::fs::write(node_bin.join("node"), b"").unwrap(); + let npm = node_bin.join("npm"); + std::fs::write(&npm, r#"#!/bin/sh +set -eu +package="$npm_config_prefix/lib/node_modules/test-package" +mkdir -p "$package" +printf '%s' '{"name":"test-package","version":"1.0.0","bin":{"test-cli":"cli"}}' > "$package/package.json" +printf '#!/bin/sh\nprintf working' > "$package/cli" +touch "$npm_config_prefix/prepared" +"#).unwrap(); + std::fs::set_permissions(&npm, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let lock = bin_lock::acquire().await.unwrap(); + let specs = vec!["test-package".to_string()]; + let options = || InstallOptions { + node_version: Some("22.18.0"), + force: false, + ignore_scripts: true, + concurrency: 5, + update: false, + }; + let mut installs = Box::pin(async { + tokio::join!(install(&specs, options()), install(&specs, options())) + }); + let package_dir = PackageMetadata::installation_dir_for("test-package", "").unwrap(); + let prepared = async { + loop { + let count = std::fs::read_dir(&package_dir).into_iter().flatten().flatten() + .filter(|entry| entry.path().join("prepared").exists()).count(); + if count == 2 { + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }; + tokio::select! { + _ = &mut installs => panic!("installation finished while activation was locked"), + result = tokio::time::timeout(Duration::from_secs(10), prepared) => result.unwrap(), + } + assert!(tokio::time::timeout(Duration::from_millis(150), &mut installs).await.is_err()); + assert!(PackageMetadata::load("test-package").await.unwrap().is_none()); + drop(lock); + let (first, second) = tokio::time::timeout(Duration::from_secs(10), installs).await.unwrap(); + first.unwrap(); + second.unwrap(); + let active = PackageMetadata::load("test-package").await.unwrap().unwrap(); + let installation = active.installation_dir().unwrap(); + let binary = get_node_modules_dir(&installation, "test-package").join("cli"); + let output = Command::new(binary.as_path()).output().await.unwrap(); + assert!(output.status.success()); + assert_eq!(output.stdout, b"working"); + let installations: Vec<_> = std::fs::read_dir(&package_dir).unwrap() + .map(Result::unwrap).filter(|entry| entry.file_type().unwrap().is_dir()).collect(); + assert_eq!(installations.len(), 1); + assert_eq!(installations[0].path(), installation.as_path()); + }).await; + } + #[test] fn test_default_shims_are_protected() { for shim in crate::shim::DEFAULT_SHIM_TOOLS.iter().chain([&"vp"]) { diff --git a/crates/vp_global_cli/src/commands/global/mod.rs b/crates/vp_global_cli/src/commands/global/mod.rs index 3beef79e6e..5e851fa2e8 100644 --- a/crates/vp_global_cli/src/commands/global/mod.rs +++ b/crates/vp_global_cli/src/commands/global/mod.rs @@ -12,6 +12,7 @@ use vt_path::{AbsolutePathBuf, current_dir}; use crate::{commands::env::config::resolve_version, error::Error}; +pub(crate) mod bin_lock; pub mod install; pub mod outdated; pub mod packages; diff --git a/crates/vp_global_cli/src/shim/dispatch.rs b/crates/vp_global_cli/src/shim/dispatch.rs index 526b99b2b3..97555c7642 100644 --- a/crates/vp_global_cli/src/shim/dispatch.rs +++ b/crates/vp_global_cli/src/shim/dispatch.rs @@ -21,7 +21,10 @@ use crate::{ package_manager, package_metadata::PackageMetadata, }, - global::install::{extract_binaries, is_protected_shim}, + global::{ + bin_lock, + install::{extract_binaries, is_protected_shim}, + }, }, error::Error, }; @@ -234,6 +237,13 @@ fn check_npm_global_install_result( } let is_interactive = vp_shared::is_stdin_terminal(); + let lock = match bin_lock::acquire_sync() { + Ok(lock) => lock, + Err(error) => { + output::error(&error.to_string()); + return; + } + }; // (bin_name, source_path, package_name) let mut missing_bins: Vec<(String, AbsolutePathBuf, String)> = Vec::new(); let mut managed_conflicts: Vec<(String, String)> = Vec::new(); @@ -326,6 +336,9 @@ fn check_npm_global_install_result( } } + // Never hold the lock while waiting for a user to answer the prompt. + drop(lock); + // Deduplicate by bin_name so that when two packages declare the same binary, // only the last one is linked (matching npm's "last writer wins" behavior). let missing_bins = dedup_missing_bins(missing_bins); @@ -364,8 +377,49 @@ fn check_npm_global_install_result( }; if should_link { - for (bin_name, source_path, package_name) in &missing_bins { - create_bin_link(&bin_dir, bin_name, source_path, package_name, node_version); + link_missing_npm_bins(&bin_dir, &missing_bins, node_version); + } +} + +fn link_missing_npm_bins( + bin_dir: &AbsolutePath, + missing_bins: &[(String, AbsolutePathBuf, String)], + node_version: &str, +) { + let _lock = match bin_lock::acquire_sync() { + Ok(lock) => lock, + Err(error) => { + output::error(&error.to_string()); + return; + } + }; + for (bin_name, source_path, package_name) in missing_bins { + // A different operation may have claimed this name while the prompt was open. + match BinConfig::load_sync(bin_name) { + Ok(Some(config)) if config.source == BinSource::Vp => { + output::warn(&vt_str::format!( + "Skipped '{bin_name}': managed by `vp install -g {}`.", + config.package + )); + continue; + } + Err(error) => { + output::error(&error.to_string()); + continue; + } + _ => {} + } + if std::fs::symlink_metadata(bin_dir.join(bin_name)).is_ok() { + continue; + } + #[cfg(windows)] + if ["exe", "cmd"].iter().any(|extension| { + std::fs::symlink_metadata(bin_dir.join(format!("{bin_name}.{extension}"))).is_ok() + }) { + continue; + } + if source_path.as_path().exists() { + create_bin_link(bin_dir, bin_name, source_path, package_name, node_version); } } } @@ -469,6 +523,13 @@ fn remove_npm_global_uninstall_links( npm_prefix: &AbsolutePath, ) { let Ok(bin_dir) = config::get_bin_dir() else { return }; + let _lock = match bin_lock::acquire_sync() { + Ok(lock) => lock, + Err(error) => { + output::error(&error.to_string()); + return; + } + }; for (bin_name, package_name, package_dir) in bin_entries { // Skip protected shims: a stale Npm BinConfig must not let @@ -1829,6 +1890,41 @@ mod tests { }); } + #[test] + fn test_npm_link_rechecks_owner_after_prompt() { + let temp = TempDir::new().unwrap(); + vp_shared::EnvConfig::with_vars([(env_vars::VP_HOME, temp.path())], |_| { + let bin_dir = config::get_bin_dir().unwrap(); + std::fs::create_dir_all(&bin_dir).unwrap(); + let source = bin_dir.join("source.js"); + std::fs::write(&source, "console.log('test');").unwrap(); + let candidates = vec![("test-cli".into(), source, "npm-package".into())]; + + // Another activation claims the name while npm's link prompt is open. + BinConfig::new( + "test-cli".into(), + "managed-package".into(), + "1.0.0".into(), + "22.18.0".into(), + ) + .save_sync() + .unwrap(); + link_missing_npm_bins(&bin_dir, &candidates, "22.18.0"); + assert_eq!( + BinConfig::load_sync("test-cli").unwrap().unwrap().package, + "managed-package" + ); + for name in ["test-cli", "test-cli.cmd", "test-cli.exe"] { + assert!(std::fs::symlink_metadata(bin_dir.join(name)).is_err()); + } + + BinConfig::delete_sync("test-cli").unwrap(); + link_missing_npm_bins(&bin_dir, &candidates, "22.18.0"); + assert_eq!(BinConfig::load_sync("test-cli").unwrap().unwrap().package, "npm-package"); + assert!(bin_dir.join("test-cli").as_path().exists()); + }); + } + #[test] fn test_npm_uninstall_preserves_links_until_package_is_removed() { let temp = TempDir::new().unwrap(); From 7d724b0f9b60332a26701e7317ebdd761dd1a3af Mon Sep 17 00:00:00 2001 From: JongKyung Lee Date: Thu, 22 Oct 2026 11:42:47 +0900 Subject: [PATCH 2/3] fix(implode): remove the global package lock Remove the global bin lock from split installations, including when it is the only remaining managed artifact. Preserve unrelated files in the shared bin directory and cover both removal paths in the existing regression test. --- crates/vp_global_cli/src/commands/implode.rs | 27 ++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/crates/vp_global_cli/src/commands/implode.rs b/crates/vp_global_cli/src/commands/implode.rs index 71796fa508..f016de22b4 100644 --- a/crates/vp_global_cli/src/commands/implode.rs +++ b/crates/vp_global_cli/src/commands/implode.rs @@ -78,7 +78,9 @@ pub fn execute(yes: bool) -> Result { } } - if !delete_set.iter().any(|root| root.as_path().exists()) { + let global_bin_lock = dirs.bin.join(".vp-global-bin.lock"); + if !delete_set.iter().any(|root| root.as_path().exists()) && !global_bin_lock.as_path().exists() + { if homebrew { output::info("No Vite+-managed data directories exist."); print_homebrew_uninstall_notice(); @@ -129,6 +131,13 @@ pub fn execute(yes: bool) -> Result { // Remove vp-owned shim files from the (potentially shared) bin directory, // then the owned roots. remove_shim_files(dirs); + match std::fs::remove_file(global_bin_lock.as_path()) { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => { + output::warn(&vt_str::format!("Failed to remove global package lock: {e}")); + } + } for root in &delete_set { if root.as_path().exists() { #[cfg(unix)] @@ -1545,7 +1554,7 @@ mod tests { } #[test] - fn execute_normalizes_category_roots_before_deduplication() { + fn execute_removes_split_roots_and_global_bin_lock() { let temp_dir = tempfile::tempdir().unwrap(); let home = temp_dir.path().join("home"); let bin = temp_dir.path().join("bin"); @@ -1572,13 +1581,27 @@ mod tests { ("USERPROFILE", Some(home.as_os_str())), ], |_| { + let lock_path = bin.join(".vp-global-bin.lock"); + drop(crate::commands::global::bin_lock::acquire_sync().unwrap()); + assert!(lock_path.is_file()); + let unrelated = bin.join("unrelated-tool"); + std::fs::write(&unrelated, b"keep").unwrap(); + let result = execute(true).unwrap(); assert!(result.success()); + assert!(!lock_path.exists(), "global bin lock must be removed"); + assert_eq!(std::fs::read(&unrelated).unwrap(), b"keep"); assert!(!data.exists(), "data root must be removed"); assert!( !normalized_cache.exists(), "lexically distinct cache root must be removed" ); + + // A failed uninstall can create only the lock, without any data roots. + drop(crate::commands::global::bin_lock::acquire_sync().unwrap()); + assert!(execute(true).unwrap().success()); + assert!(!lock_path.exists(), "a lone global bin lock must also be removed"); + assert_eq!(std::fs::read(&unrelated).unwrap(), b"keep"); }, ); } From a02eaa57c557e0155dbc18545b8b0f6f2e8c2b47 Mon Sep 17 00:00:00 2001 From: JongKyung Lee Date: Thu, 22 Oct 2026 11:52:58 +0900 Subject: [PATCH 3/3] Revert "fix(implode): remove the global package lock" This reverts commit 7d724b0f9b60332a26701e7317ebdd761dd1a3af. --- crates/vp_global_cli/src/commands/implode.rs | 27 ++------------------ 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/crates/vp_global_cli/src/commands/implode.rs b/crates/vp_global_cli/src/commands/implode.rs index f016de22b4..71796fa508 100644 --- a/crates/vp_global_cli/src/commands/implode.rs +++ b/crates/vp_global_cli/src/commands/implode.rs @@ -78,9 +78,7 @@ pub fn execute(yes: bool) -> Result { } } - let global_bin_lock = dirs.bin.join(".vp-global-bin.lock"); - if !delete_set.iter().any(|root| root.as_path().exists()) && !global_bin_lock.as_path().exists() - { + if !delete_set.iter().any(|root| root.as_path().exists()) { if homebrew { output::info("No Vite+-managed data directories exist."); print_homebrew_uninstall_notice(); @@ -131,13 +129,6 @@ pub fn execute(yes: bool) -> Result { // Remove vp-owned shim files from the (potentially shared) bin directory, // then the owned roots. remove_shim_files(dirs); - match std::fs::remove_file(global_bin_lock.as_path()) { - Ok(()) => {} - Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} - Err(e) => { - output::warn(&vt_str::format!("Failed to remove global package lock: {e}")); - } - } for root in &delete_set { if root.as_path().exists() { #[cfg(unix)] @@ -1554,7 +1545,7 @@ mod tests { } #[test] - fn execute_removes_split_roots_and_global_bin_lock() { + fn execute_normalizes_category_roots_before_deduplication() { let temp_dir = tempfile::tempdir().unwrap(); let home = temp_dir.path().join("home"); let bin = temp_dir.path().join("bin"); @@ -1581,27 +1572,13 @@ mod tests { ("USERPROFILE", Some(home.as_os_str())), ], |_| { - let lock_path = bin.join(".vp-global-bin.lock"); - drop(crate::commands::global::bin_lock::acquire_sync().unwrap()); - assert!(lock_path.is_file()); - let unrelated = bin.join("unrelated-tool"); - std::fs::write(&unrelated, b"keep").unwrap(); - let result = execute(true).unwrap(); assert!(result.success()); - assert!(!lock_path.exists(), "global bin lock must be removed"); - assert_eq!(std::fs::read(&unrelated).unwrap(), b"keep"); assert!(!data.exists(), "data root must be removed"); assert!( !normalized_cache.exists(), "lexically distinct cache root must be removed" ); - - // A failed uninstall can create only the lock, without any data roots. - drop(crate::commands::global::bin_lock::acquire_sync().unwrap()); - assert!(execute(true).unwrap().success()); - assert!(!lock_path.exists(), "a lone global bin lock must also be removed"); - assert_eq!(std::fs::read(&unrelated).unwrap(), b"keep"); }, ); }