fix(cli): clean stale global package installs

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::<String, File>::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::<String, String>::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<InstalledPackage, Error> {
+) -> 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<AbsolutePathBuf, Error> {
+    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<File, Error> {
+    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<Option<(AbsolutePathBuf, File)>, 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<Vec<AbsolutePathBuf>, 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(&current_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() {

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);
+}

packages/cli/snap-tests-global/env-install-interrupt/snap.txt

@@ -11,3 +11,14 @@ info: Installing 1 global package with Node.js <semver>
 
 > 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 <semver>
+✓ Installed @scope/long-time-install-package <semver>
+  Bins: long-time-install-package
+
+> node check-stale-packages.js
+interrupted stale package removed

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"]
 }