test: replace sleeps in Rust tests with deterministic waits

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() {

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,

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<AbsolutePathBuf> {
     // 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<std::ffi::OsString> {
+        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"
         );
     }