From 4a86a74470ccc0bdbac85cf25c6e2cbee7840dec Mon Sep 17 00:00:00 2001 From: Liang Date: Tue, 20 Oct 2026 21:28:02 +0800 Subject: [PATCH 1/3] test(setup): order cleanup candidates without sleeps Split version selection out of cleanup_old_versions so tests pass explicit creation times instead of sleeping between create_dir calls, which left the order arbitrary on filesystems with coarse or missing birth times. One filesystem test still covers the directory scan with an order-independent outcome. --- crates/vp_setup/src/install.rs | 167 ++++++++++++++++++--------------- 1 file changed, 91 insertions(+), 76 deletions(-) diff --git a/crates/vp_setup/src/install.rs b/crates/vp_setup/src/install.rs index f512ab3e13..aecc71d5c5 100644 --- a/crates/vp_setup/src/install.rs +++ b/crates/vp_setup/src/install.rs @@ -522,23 +522,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. @@ -753,103 +769,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" ); } From b01426009c0e19bb33af08e1eaa644f10f5ec470 Mon Sep 17 00:00:00 2001 From: Liang Date: Tue, 20 Oct 2026 21:28:02 +0800 Subject: [PATCH 2/3] test(global): wait for upgrade checks to back off instead of sleeping Wait until every check except the one holding the registry request has returned, then assert that request is still in flight, instead of sleeping 200ms and sampling the request count. --- crates/vp_global_cli/src/upgrade_check.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) 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, From 2dde7d78b1827d7448f8b04e39f740f4a2625346 Mon Sep 17 00:00:00 2001 From: Liang Date: Tue, 20 Oct 2026 21:28:02 +0800 Subject: [PATCH 3/3] test(implode): keep Windows child processes alive until released The child processes slept for 2s while the parent polled for up to 5s and then asserted they were still running. They now block on stdin until the test closes it. --- crates/vp_global_cli/src/commands/implode.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) 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() {