diff --git a/crates/vite_global_cli/src/commands/global/install.rs b/crates/vite_global_cli/src/commands/global/install.rs index 2c41a6f203..f750221a4d 100644 --- a/crates/vite_global_cli/src/commands/global/install.rs +++ b/crates/vite_global_cli/src/commands/global/install.rs @@ -2,6 +2,7 @@ use std::{ collections::{HashMap, HashSet}, + fs::{File, OpenOptions, TryLockError}, io::{IsTerminal, Read, Write}, process::Stdio, time::Duration, @@ -18,13 +19,13 @@ use vite_path::{AbsolutePath, AbsolutePathBuf, current_dir}; use vite_shared::{format_path_prepended, output}; #[cfg(test)] -use crate::commands::env::package_metadata::{INSTALL_ID_LENGTH, is_install_id}; +use crate::commands::env::package_metadata::INSTALL_ID_LENGTH; use crate::{ commands::{ env::{ bin_config::BinConfig, config::{get_bin_dir, get_node_modules_dir, resolve_version, resolve_version_alias}, - package_metadata::{INSTALL_ID_PREFIX, PackageMetadata}, + package_metadata::{INSTALL_ID_PREFIX, PackageMetadata, is_install_id}, }, global::{CORE_SHIMS, is_local_package_spec, parse_package_spec}, }, @@ -191,6 +192,7 @@ pub async fn install( let mut package_names = package_names.iter(); let mut installs = FuturesUnordered::new(); + let mut install_locks = HashMap::::new(); let mut first_error = None; let mut stop_scheduling = false; loop { @@ -211,9 +213,10 @@ pub async fn install( } match installs.next().await { - Some((package_name, Ok(installed_package))) => { + Some((package_name, Ok((installed_package, lock_file)))) => { progress.inc(1); - packages.get_mut(&package_name).unwrap().install = Some(installed_package) + packages.get_mut(&package_name).unwrap().install = Some(installed_package); + install_locks.insert(package_name, lock_file); } Some((package_name, Err(error))) => { stop_scheduling = true; @@ -229,6 +232,7 @@ pub async fn install( // 4. 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 { installed_version, mut bin_names, @@ -488,8 +492,9 @@ pub async fn install( bin_owners.insert(bin_name.clone(), package_name.clone()); } - // 4.6 Remove only the installation that this operation replaced. - cleanup_previous_installation(previous_metadata.as_ref(), &install_id).await; + // 4.6 Remove stale installations for this package. + cleanup_stale_installations(&package_name, &install_id).await; + drop(lock_file); // 4.7 Print success message output::success(&format!( @@ -521,10 +526,11 @@ async fn install_one( package_spec: &str, npm_path: &AbsolutePathBuf, node_bin_dir: &AbsolutePathBuf, -) -> Result { +) -> Result<(InstalledPackage, File), Error> { // 1. Create an immutable install directory. let install_id = new_install_id(); let install_dir = PackageMetadata::installation_dir_for(package_name, &install_id)?; + let lock_file = lock_install_dir(&install_dir)?; tokio::fs::create_dir_all(&install_dir).await?; // 2. Run npm install with prefix set to the final installation directory. @@ -596,7 +602,10 @@ async fn install_one( } } - Ok(InstalledPackage { installed_version, bin_names, js_bins, install_id, install_dir }) + Ok(( + InstalledPackage { installed_version, bin_names, js_bins, install_id, install_dir }, + lock_file, + )) } fn new_install_id() -> String { @@ -668,35 +677,168 @@ async fn cleanup_failed_install(install_dir: &AbsolutePathBuf) -> Result<(), Err remove_dir_all_if_exists(install_dir).await } -async fn cleanup_previous_installation( - previous_metadata: Option<&PackageMetadata>, - current_install_id: &str, -) { - let Some(previous_metadata) = previous_metadata else { +async fn remove_dir_all_if_exists(path: &AbsolutePathBuf) -> Result<(), Error> { + match tokio::fs::remove_dir_all(path).await { + Ok(()) => Ok(()), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(error) => Err(error.into()), + } +} + +fn install_dir_lock_path(install_dir: &AbsolutePathBuf) -> Result { + let parent = install_dir.as_path().parent().ok_or_else(|| { + Error::ConfigError( + format!( + "Global package installation path has no parent: {}", + install_dir.as_path().display() + ) + .into(), + ) + })?; + let file_name = + install_dir.as_path().file_name().and_then(|name| name.to_str()).ok_or_else(|| { + Error::ConfigError( + format!( + "Global package installation path has no file name: {}", + install_dir.as_path().display() + ) + .into(), + ) + })?; + AbsolutePathBuf::new(parent.join(format!("{file_name}.lock"))).ok_or_else(|| { + Error::ConfigError( + format!( + "Invalid global package installation lock path for {}", + install_dir.as_path().display() + ) + .into(), + ) + }) +} + +fn open_install_dir_lock_file( + install_dir: &AbsolutePathBuf, +) -> Result<(AbsolutePathBuf, File), Error> { + let lock_path = install_dir_lock_path(install_dir)?; + if let Some(parent) = lock_path.as_path().parent() { + std::fs::create_dir_all(parent)?; + } + + let lock_file = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .open(lock_path.as_path())?; + + Ok((lock_path, lock_file)) +} + +fn lock_install_dir(install_dir: &AbsolutePathBuf) -> Result { + let (_, lock_file) = open_install_dir_lock_file(install_dir)?; + lock_file.lock()?; + Ok(lock_file) +} + +fn try_lock_install_dir( + install_dir: &AbsolutePathBuf, +) -> Result, Error> { + let (lock_path, lock_file) = open_install_dir_lock_file(install_dir)?; + match lock_file.try_lock() { + Ok(()) => Ok(Some((lock_path, lock_file))), + Err(TryLockError::WouldBlock) => Ok(None), + Err(TryLockError::Error(error)) => Err(error.into()), + } +} + +async fn cleanup_stale_installations(package_name: &str, current_install_id: &str) { + let Ok(stale_dirs) = stale_installation_dirs(package_name, current_install_id).await else { return; }; - if previous_metadata.install_id == current_install_id { - return; + + for install_dir in stale_dirs { + let (lock_path, lock_file) = match try_lock_install_dir(&install_dir) { + Ok(Some(lock)) => lock, + Ok(None) => continue, + Err(error) => { + tracing::warn!( + "Failed to lock stale global package installation at {}: {}", + install_dir.as_path().display(), + error + ); + continue; + } + }; + + if let Err(error) = remove_dir_all_if_exists(&install_dir).await { + tracing::warn!( + "Failed to remove stale global package installation at {}: {}", + install_dir.as_path().display(), + error + ); + continue; + } + + drop(lock_file); + if let Err(error) = tokio::fs::remove_file(&lock_path).await { + tracing::warn!( + "Failed to remove stale global package installation lock at {}: {}", + lock_path.as_path().display(), + error + ); + } } +} - let Ok(previous_install_dir) = previous_metadata.installation_dir() else { - return; +async fn stale_installation_dirs( + package_name: &str, + current_install_id: &str, +) -> Result, Error> { + let legacy_install_dir = PackageMetadata::installation_dir_for(package_name, "")?; + let Some(parent) = legacy_install_dir.as_path().parent() else { + return Ok(Vec::new()); }; - if let Err(error) = remove_dir_all_if_exists(&previous_install_dir).await { - tracing::warn!( - "Failed to remove replaced global package installation at {}: {}", - previous_install_dir.as_path().display(), - error - ); + if !tokio::fs::try_exists(parent).await.unwrap_or(false) { + return Ok(Vec::new()); } + + let Some(base_name) = legacy_install_dir.as_path().file_name().and_then(|name| name.to_str()) + else { + return Ok(Vec::new()); + }; + + let mut stale_dirs = Vec::new(); + let mut entries = tokio::fs::read_dir(parent).await?; + while let Some(entry) = entries.next_entry().await? { + if !entry.file_type().await?.is_dir() { + continue; + } + + let file_name = entry.file_name(); + let Some(file_name) = file_name.to_str() else { + continue; + }; + let Some(install_id) = stale_install_id(base_name, file_name) else { + continue; + }; + if install_id == current_install_id { + continue; + } + if let Some(path) = AbsolutePathBuf::new(entry.path()) { + stale_dirs.push(path); + } + } + + Ok(stale_dirs) } -async fn remove_dir_all_if_exists(path: &AbsolutePathBuf) -> Result<(), Error> { - match tokio::fs::remove_dir_all(path).await { - Ok(()) => Ok(()), - Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()), - Err(error) => Err(error.into()), +fn stale_install_id<'a>(base_name: &str, file_name: &'a str) -> Option<&'a str> { + if file_name == base_name { + return Some(""); } + + let install_id = file_name.strip_prefix(base_name)?; + if is_install_id(install_id) { Some(install_id) } else { None } } async fn stale_bin_names_for_package( @@ -1253,57 +1395,6 @@ mod tests { assert!(!package_dir.as_path().exists(), "identified package directory should be removed"); } - #[tokio::test] - async fn test_cleanup_previous_installation_removes_only_replaced_install() { - use tempfile::TempDir; - use vite_path::AbsolutePathBuf; - - let temp_dir = TempDir::new().unwrap(); - let temp_path = temp_dir.path().to_path_buf(); - let _env_guard = vite_shared::EnvConfig::test_guard( - vite_shared::EnvConfig::for_test_with_home(&temp_path), - ); - - let legacy_package_dir = - AbsolutePathBuf::new(temp_path.join("packages").join("@scope").join("pkg")).unwrap(); - let current_install_id = "#123e4567-e89b-42d3-a456-426614174000"; - let old_install_id = "#987e6543-e21b-42d3-a456-426614174000"; - let stale_install_id = "#987e6543-e21b-42d3-b456-426614174000"; - let current_install = AbsolutePathBuf::new( - legacy_package_dir.as_path().with_file_name(format!("pkg{current_install_id}")), - ) - .unwrap(); - let old_install = AbsolutePathBuf::new( - legacy_package_dir.as_path().with_file_name(format!("pkg{old_install_id}")), - ) - .unwrap(); - let stale_install = AbsolutePathBuf::new( - legacy_package_dir.as_path().with_file_name(format!("pkg{stale_install_id}")), - ) - .unwrap(); - tokio::fs::create_dir_all(¤t_install).await.unwrap(); - tokio::fs::create_dir_all(&old_install).await.unwrap(); - tokio::fs::create_dir_all(&stale_install).await.unwrap(); - tokio::fs::write(current_install.join("marker").as_path(), "current").await.unwrap(); - - let mut previous_metadata = PackageMetadata::new( - "@scope/pkg".to_string(), - "1.0.0".to_string(), - "22.0.0".to_string(), - None, - vec![], - HashSet::new(), - "npm".to_string(), - ); - previous_metadata.install_id = old_install_id.to_string(); - - cleanup_previous_installation(Some(&previous_metadata), current_install_id).await; - - assert!(current_install.join("marker").as_path().exists()); - assert!(!old_install.as_path().exists()); - assert!(stale_install.as_path().exists()); - } - #[tokio::test] #[cfg_attr(windows, serial_test::serial)] async fn test_restore_previous_install_state_removes_partial_new_bins() { diff --git a/packages/cli/snap-tests-global/env-install-interrupt/check-stale-packages.js b/packages/cli/snap-tests-global/env-install-interrupt/check-stale-packages.js new file mode 100644 index 0000000000..05343d9654 --- /dev/null +++ b/packages/cli/snap-tests-global/env-install-interrupt/check-stale-packages.js @@ -0,0 +1,35 @@ +const fs = require('fs'); +const path = require('path'); + +const packageBase = 'long-time-install-package'; +const scopeDir = path.join(process.env.VP_HOME, 'packages', '@scope'); +const metadataPath = path.join(scopeDir, `${packageBase}.json`); +const metadata = JSON.parse(fs.readFileSync(metadataPath, 'utf8')); +const activeDir = `${packageBase}${metadata.installId}`; +const expectStale = process.argv.includes('--expect-stale'); + +const packageDirs = fs + .readdirSync(scopeDir, { withFileTypes: true }) + .filter((entry) => { + if (!entry.isDirectory()) { + return false; + } + if (entry.name === packageBase) { + return true; + } + return /^long-time-install-package#[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/.test( + entry.name, + ); + }) + .map((entry) => entry.name) + .sort(); + +const hasIdentifiedStale = packageDirs.some((name) => name !== packageBase && name !== activeDir); + +console.log( + hasIdentifiedStale ? 'interrupted stale package exists' : 'interrupted stale package removed', +); + +if (expectStale !== hasIdentifiedStale) { + process.exit(1); +} diff --git a/packages/cli/snap-tests-global/env-install-interrupt/snap.txt b/packages/cli/snap-tests-global/env-install-interrupt/snap.txt index ee9e7d0415..dc771162da 100644 --- a/packages/cli/snap-tests-global/env-install-interrupt/snap.txt +++ b/packages/cli/snap-tests-global/env-install-interrupt/snap.txt @@ -11,3 +11,14 @@ info: Installing 1 global package with Node.js > long-time-install-package # Original package should be still runnable long-time-install-package + +> node check-stale-packages.js --expect-stale # Interrupted reinstall should leave stale package +interrupted stale package exists + +> vp install -g ./long-time-install-package # Successful reinstall should clean stale packages +info: Installing 1 global package with Node.js +✓ Installed @scope/long-time-install-package + Bins: long-time-install-package + +> node check-stale-packages.js +interrupted stale package removed diff --git a/packages/cli/snap-tests-global/env-install-interrupt/steps.json b/packages/cli/snap-tests-global/env-install-interrupt/steps.json index 24ad83f2b3..06302a3206 100644 --- a/packages/cli/snap-tests-global/env-install-interrupt/steps.json +++ b/packages/cli/snap-tests-global/env-install-interrupt/steps.json @@ -4,7 +4,10 @@ "vp install -g ./long-time-install-package", "long-time-install-package", "node test-reinstall-interrupt.js # Reinstall but interrupt", - "long-time-install-package # Original package should be still runnable" + "long-time-install-package # Original package should be still runnable", + "node check-stale-packages.js --expect-stale # Interrupted reinstall should leave stale package", + "vp install -g ./long-time-install-package # Successful reinstall should clean stale packages", + "node check-stale-packages.js" ], "after": ["vp remove -g @scope/long-time-install-package"] }