diff --git a/crates/vp_global_cli/src/commands/implode.rs b/crates/vp_global_cli/src/commands/implode.rs index c778b128b6..71796fa508 100644 --- a/crates/vp_global_cli/src/commands/implode.rs +++ b/crates/vp_global_cli/src/commands/implode.rs @@ -1198,7 +1198,8 @@ mod tests { #[cfg(windows)] fn locked_executable_child() { if std::env::var_os("VP_IMPLODE_LOCK_TEST").is_some() { - std::thread::sleep(std::time::Duration::from_secs(2)); + // Stay alive until the parent test closes stdin. + std::io::copy(&mut std::io::stdin(), &mut std::io::sink()).unwrap(); } } @@ -1211,7 +1212,8 @@ mod tests { let root = AbsolutePathBuf::new(root.into()).unwrap(); let cleanup_source = prepare_deferred_delete_source(std::slice::from_ref(&root)).unwrap(); remove_vite_plus_dir(&root, &cleanup_source).unwrap(); - std::thread::sleep(std::time::Duration::from_secs(2)); + // Stay alive until the parent test closes stdin. + std::io::copy(&mut std::io::stdin(), &mut std::io::sink()).unwrap(); } #[test] @@ -1287,6 +1289,7 @@ mod tests { let mut child = std::process::Command::new(&locked_executable) .args(["deferred_root_cleanup_child", "--nocapture"]) .env("VP_IMPLODE_ROOT_TEST", &original) + .stdin(std::process::Stdio::piped()) .spawn() .unwrap(); let trash_prefix = vt_str::format!("data.removing-{}-", child.id()); @@ -1312,6 +1315,7 @@ mod tests { let replacement = original.join("version/replacement.txt"); std::fs::create_dir_all(replacement.parent().unwrap()).unwrap(); std::fs::write(&replacement, b"keep").unwrap(); + drop(child.stdin.take()); assert!(child.wait().unwrap().success()); for _ in 0..150 { @@ -1348,9 +1352,9 @@ mod tests { let mut child = std::process::Command::new(&executable) .args(["locked_executable_child", "--nocapture"]) .env("VP_IMPLODE_LOCK_TEST", "1") + .stdin(std::process::Stdio::piped()) .spawn() .unwrap(); - std::thread::sleep(std::time::Duration::from_millis(250)); assert!(child.try_wait().unwrap().is_none(), "test executable must still be running"); assert!(std::fs::remove_file(&executable).is_err(), "running executable must be locked"); @@ -1362,6 +1366,7 @@ mod tests { std::fs::copy(std::env::current_exe().unwrap(), &executable).unwrap(); std::fs::write(&pointer, b"new-sidecar").unwrap(); + drop(child.stdin.take()); assert!(child.wait().unwrap().success()); for _ in 0..100 { if !deferred.executable.exists() && !deferred.pointer.exists() { diff --git a/crates/vp_global_cli/src/upgrade_check.rs b/crates/vp_global_cli/src/upgrade_check.rs index 164c26cd04..243251a879 100644 --- a/crates/vp_global_cli/src/upgrade_check.rs +++ b/crates/vp_global_cli/src/upgrade_check.rs @@ -675,14 +675,23 @@ mod tests { }) .await .expect("at least one registry request should start"); - tokio::time::sleep(Duration::from_millis(200)).await; + // The server never responds, so a check that sent a request cannot finish here. + tokio::time::timeout(Duration::from_secs(2), async { + while checks.iter().filter(|check| check.is_finished()).count() < checks.len() - 1 { + tokio::task::yield_now().await; + } + }) + .await + .expect("the other checks must back off while the first request is in flight"); + let in_flight = checks.iter().filter(|check| !check.is_finished()).count(); let observed_requests = request_count.load(Ordering::SeqCst); let cache_exists = home.path().join("cache").join(CACHE_FILE_NAME).exists(); for check in checks { check.abort(); } + assert_eq!(in_flight, 1, "the first registry request must still be in flight"); assert_eq!(observed_requests, 1, "concurrent checks must share one registry request"); assert!( cache_exists, diff --git a/crates/vp_setup/src/install.rs b/crates/vp_setup/src/install.rs index 49fbc8f04a..65626b0eea 100644 --- a/crates/vp_setup/src/install.rs +++ b/crates/vp_setup/src/install.rs @@ -589,23 +589,39 @@ pub async fn cleanup_old_versions( } } + for path in versions_to_remove(versions, max_keep, protected_versions) { + tracing::debug!("Cleaning up old version: {}", path.as_path().display()); + if let Err(e) = tokio::fs::remove_dir_all(&path).await { + tracing::warn!("Failed to remove {}: {}", path.as_path().display(), e); + } + } + + Ok(()) +} + +/// Select the version directories that [`cleanup_old_versions`] removes. +/// +/// Kept apart from the directory scan so tests can supply exact creation times, +/// which some filesystems record too coarsely (or not at all) to order fresh dirs. +fn versions_to_remove( + mut versions: Vec<(SystemTime, AbsolutePathBuf)>, + max_keep: usize, + protected_versions: &[&str], +) -> Vec { // Sort newest first (by creation time, matching install.sh) versions.sort_by_key(|b| std::cmp::Reverse(b.0)); // Remove versions beyond the keep limit, but never remove protected versions + let mut removable = Vec::new(); for (_time, path) in versions.into_iter().skip(max_keep) { let name = path.as_path().file_name().and_then(|n| n.to_str()).unwrap_or(""); if protected_versions.contains(&name) { tracing::debug!("Skipping protected version: {}", name); continue; } - tracing::debug!("Cleaning up old version: {}", path.as_path().display()); - if let Err(e) = tokio::fs::remove_dir_all(&path).await { - tracing::warn!("Failed to remove {}: {}", path.as_path().display(), e); - } + removable.push(path); } - - Ok(()) + removable } /// Read the previous version from `.previous-version` file. @@ -864,103 +880,102 @@ mod tests { assert!(!is_safe_tar_path(Path::new("/usr/bin/vp"))); } - #[tokio::test] - async fn test_cleanup_preserves_active_downgraded_version() { - let temp = tempfile::tempdir().unwrap(); - let install_dir = AbsolutePathBuf::new(temp.path().to_path_buf()).unwrap(); - - // Create 7 version directories with staggered creation times. - // Simulate: installed 0.1-0.7 in order, then rolled back to 0.2.0 - for v in ["0.1.0", "0.2.0", "0.3.0", "0.4.0", "0.5.0", "0.6.0", "0.7.0"] { - tokio::fs::create_dir(install_dir.join(v)).await.unwrap(); - // Small delay to ensure distinct creation times - tokio::time::sleep(std::time::Duration::from_millis(10)).await; - } - - // Simulate rollback: current points to 0.2.0 (low semver rank) - #[cfg(unix)] - tokio::fs::symlink("0.2.0", install_dir.join("current")).await.unwrap(); + /// Select cleanup candidates from dirs created one second apart, oldest first. + fn removed_versions( + created_oldest_first: &[&str], + max_keep: usize, + protected_versions: &[&str], + ) -> Vec { + let install_dir = AbsolutePathBuf::new(std::env::temp_dir()).unwrap(); + let versions = created_oldest_first + .iter() + .zip(0..) + .map(|(name, secs)| { + (UNIX_EPOCH + std::time::Duration::from_secs(secs), install_dir.join(name)) + }) + .collect(); + versions_to_remove(versions, max_keep, protected_versions) + .iter() + .map(|path| path.as_path().file_name().unwrap().to_os_string()) + .collect() + } - // Cleanup keeping top 5, with 0.2.0 protected (the active version) - cleanup_old_versions(&install_dir, 5, &["0.2.0"]).await.unwrap(); + #[test] + fn test_cleanup_preserves_active_downgraded_version() { + // Simulate: installed 0.1-0.7 in order, then rolled back to 0.2.0. + // Keep the top 5, with 0.2.0 protected (the active version). + let removed = removed_versions( + &["0.1.0", "0.2.0", "0.3.0", "0.4.0", "0.5.0", "0.6.0", "0.7.0"], + 5, + &["0.2.0"], + ); // 0.2.0 is the active version — it MUST survive cleanup - assert!( - tokio::fs::try_exists(install_dir.join("0.2.0")).await.unwrap(), - "Active version 0.2.0 was deleted by cleanup" - ); + assert_eq!(removed, ["0.1.0"], "Active version 0.2.0 must survive cleanup"); } - #[tokio::test] - async fn test_cleanup_sorts_by_creation_time_not_semver() { - let temp = tempfile::tempdir().unwrap(); - let install_dir = AbsolutePathBuf::new(temp.path().to_path_buf()).unwrap(); - - // Create versions in non-semver order with creation times: - // 0.5.0 (oldest), 0.1.0, 0.3.0, 0.7.0, 0.2.0, 0.6.0 (newest) - for v in ["0.5.0", "0.1.0", "0.3.0", "0.7.0", "0.2.0", "0.6.0"] { - tokio::fs::create_dir(install_dir.join(v)).await.unwrap(); - tokio::time::sleep(std::time::Duration::from_millis(10)).await; - } + #[test] + fn test_cleanup_sorts_by_creation_time_not_semver() { + // Creation order: 0.5.0 (oldest), 0.1.0, 0.3.0, 0.7.0, 0.2.0, 0.6.0 (newest) + let removed = + removed_versions(&["0.5.0", "0.1.0", "0.3.0", "0.7.0", "0.2.0", "0.6.0"], 4, &[]); // Keep top 4 by creation time → keep 0.6.0, 0.2.0, 0.7.0, 0.3.0 // Remove 0.1.0 and 0.5.0 (oldest by creation time) - cleanup_old_versions(&install_dir, 4, &[]).await.unwrap(); - - // The 4 newest by creation time should survive - assert!(tokio::fs::try_exists(install_dir.join("0.6.0")).await.unwrap()); - assert!(tokio::fs::try_exists(install_dir.join("0.2.0")).await.unwrap()); - assert!(tokio::fs::try_exists(install_dir.join("0.7.0")).await.unwrap()); - assert!(tokio::fs::try_exists(install_dir.join("0.3.0")).await.unwrap()); - - // The 2 oldest by creation time should be removed - assert!( - !tokio::fs::try_exists(install_dir.join("0.5.0")).await.unwrap(), - "0.5.0 (oldest by creation time) should have been removed" - ); - assert!( - !tokio::fs::try_exists(install_dir.join("0.1.0")).await.unwrap(), - "0.1.0 (second oldest by creation time) should have been removed" - ); + assert_eq!(removed, ["0.1.0", "0.5.0"]); } - #[tokio::test] - async fn test_cleanup_removes_old_forced_reinstall_dir() { - let temp = tempfile::tempdir().unwrap(); - let install_dir = AbsolutePathBuf::new(temp.path().to_path_buf()).unwrap(); - + #[test] + fn test_cleanup_removes_old_forced_reinstall_dir() { // Forced reinstall directories use semver build metadata. They should // still be eligible for normal old-version cleanup. let old_forced = "0.1.23+force.123.456"; - for v in [old_forced, "0.1.24", "0.1.25", "0.1.26", "0.1.27", "0.1.28"] { - tokio::fs::create_dir(install_dir.join(v)).await.unwrap(); - tokio::time::sleep(std::time::Duration::from_millis(10)).await; - } + let removed = removed_versions( + &[old_forced, "0.1.24", "0.1.25", "0.1.26", "0.1.27", "0.1.28"], + 5, + &[], + ); - cleanup_old_versions(&install_dir, 5, &[]).await.unwrap(); + assert_eq!(removed, [old_forced], "old forced reinstall directory should be removed"); + } + + #[test] + fn test_cleanup_preserves_protected_forced_reinstall_dir() { + let protected_forced = "0.1.23+force.123.456"; + let removed = removed_versions( + &[protected_forced, "0.1.24", "0.1.25", "0.1.26", "0.1.27", "0.1.28"], + 5, + &[protected_forced], + ); assert!( - !tokio::fs::try_exists(install_dir.join(old_forced)).await.unwrap(), - "old forced reinstall directory should be removed by cleanup" + removed.is_empty(), + "active forced reinstall directory should be preserved when protected" ); } #[tokio::test] - async fn test_cleanup_preserves_protected_forced_reinstall_dir() { + async fn test_cleanup_removes_only_unprotected_version_dirs() { let temp = tempfile::tempdir().unwrap(); let install_dir = AbsolutePathBuf::new(temp.path().to_path_buf()).unwrap(); - let protected_forced = "0.1.23+force.123.456"; - for v in [protected_forced, "0.1.24", "0.1.25", "0.1.26", "0.1.27", "0.1.28"] { - tokio::fs::create_dir(install_dir.join(v)).await.unwrap(); - tokio::time::sleep(std::time::Duration::from_millis(10)).await; + // Keeping no versions makes the outcome independent of creation order. + let forced = "0.1.23+force.123.456"; + for name in [forced, "0.1.24", "0.1.25", "current"] { + tokio::fs::create_dir(install_dir.join(name)).await.unwrap(); } - cleanup_old_versions(&install_dir, 5, &[protected_forced]).await.unwrap(); + cleanup_old_versions(&install_dir, 0, &["0.1.25"]).await.unwrap(); + assert!(!tokio::fs::try_exists(install_dir.join(forced)).await.unwrap()); + assert!(!tokio::fs::try_exists(install_dir.join("0.1.24")).await.unwrap()); assert!( - tokio::fs::try_exists(install_dir.join(protected_forced)).await.unwrap(), - "active forced reinstall directory should be preserved when protected" + tokio::fs::try_exists(install_dir.join("0.1.25")).await.unwrap(), + "protected version must survive cleanup" + ); + assert!( + tokio::fs::try_exists(install_dir.join("current")).await.unwrap(), + "non-version entries must survive cleanup" ); }