fix(update): sweep what an earlier Windows update left behind

src/cli/update.rs

@@ -205,6 +205,9 @@ async fn run(args: &UpdateArgs) -> anyhow::Result<()> {
     let tmp = tempfile::tempdir()?;
     let new_binary = install(&base, &target, triple, tmp.path()).await?;
 
+    #[cfg(windows)]
+    sweep_self_replace_leftovers();
+
     // Atomically replace the running executable. `self_replace` handles the
     // Windows "can't overwrite a running .exe" case (rename-then-stage) and the
     // Unix in-place rename uniformly.
@@ -222,6 +225,85 @@ async fn run(args: &UpdateArgs) -> anyhow::Result<()> {
     Ok(())
 }
 
+/// Suffixes of the files `self-replace` creates on Windows: the old binary moved aside, and the
+/// copy of itself that deletes it after the updater exits.
+#[cfg(any(windows, test))]
+const SELF_REPLACE_SUFFIXES: [&str; 2] = [".__relocated__.exe", ".__selfdelete__.exe"];
+
+/// The random part `self-replace` puts in those names: 32 lowercase ASCII letters. Matching it
+/// exactly keeps the sweep from ever deleting a file somebody else named.
+#[cfg(any(windows, test))]
+fn is_generated_id(id: &str) -> bool {
+    id.len() == 32 && id.bytes().all(|b| b.is_ascii_lowercase())
+}
+
+/// Whether `name` is one of the files `self-replace` leaves behind for a binary called `exe_stem`
+/// (`.<stem>.<random>.__relocated__.exe` / `.__selfdelete__.exe`; the stem matches in any case).
+#[cfg(any(windows, test))]
+fn is_self_replace_leftover(name: &str, exe_stem: &str) -> bool {
+    // The stem is whatever casing the process was started with, which Windows does not keep
+    // stable between runs, so the one written by an earlier update may differ in case.
+    let Some(rest) = name
+        .strip_prefix('.')
+        .filter(|r| {
+            r.get(..exe_stem.len())
+                .is_some_and(|head| head.eq_ignore_ascii_case(exe_stem))
+        })
+        .and_then(|r| r[exe_stem.len()..].strip_prefix('.'))
+    else {
+        return false;
+    };
+    SELF_REPLACE_SUFFIXES
+        .iter()
+        .any(|suffix| rest.strip_suffix(suffix).is_some_and(is_generated_id))
+}
+
+/// Remove what an EARLIER `update` left in `dir`, ignoring every error.
+///
+/// On Windows the helper `self-replace` starts makes ONE attempt to delete the old binary once the
+/// updater has exited, and never retries: if something else (a virus scanner, say) holds the file
+/// at that moment, an 8 MB copy of the binary stays behind for good, once per update. Clearing
+/// them on the next update bounds the litter. A leftover whose helper is still running is locked
+/// and simply fails to delete, which is why errors are not reported. Returns how many were removed.
+#[cfg(any(windows, test))]
+fn sweep_self_replace_leftovers_in(dir: &Path, exe_stem: &str) -> usize {
+    let Ok(entries) = std::fs::read_dir(dir) else {
+        return 0;
+    };
+    entries
+        .flatten()
+        .filter(|e| is_self_replace_leftover(&e.file_name().to_string_lossy(), exe_stem))
+        .filter(|e| std::fs::remove_file(e.path()).is_ok())
+        .count()
+}
+
+/// Sweep both places `self-replace` puts them: the temp directory, and next to the binary (its
+/// fallback when the temp directory is on another volume).
+#[cfg(windows)]
+fn sweep_self_replace_leftovers() {
+    let Ok(exe) = std::env::current_exe() else {
+        return;
+    };
+    // `self-replace` names the files after the path the process was started with, but puts the
+    // fallback ones in the directory of the CANONICAL path: take each from where it takes it.
+    let Some(stem) = exe.file_stem().map(|s| s.to_string_lossy().into_owned()) else {
+        return;
+    };
+    let mut dirs = vec![std::env::temp_dir()];
+    dirs.extend(
+        exe.canonicalize()
+            .ok()
+            .and_then(|c| c.parent().map(Path::to_path_buf)),
+    );
+    let removed: usize = dirs
+        .iter()
+        .map(|d| sweep_self_replace_leftovers_in(d, &stem))
+        .sum();
+    if removed > 0 {
+        tracing::debug!("removed {removed} file(s) left by an earlier update");
+    }
+}
+
 /// `(os, arch, triple)` for every target `bugsee-cli` publishes a binary for.
 ///
 /// This MUST stay in lockstep with `[workspace.metadata.dist].targets` in
@@ -763,6 +845,94 @@ mod tests {
         );
     }
 
+    /// 32 lowercase letters, like the random part `self-replace` generates.
+    const ID_A: &str = "mmxodzqauiijizsertstseqdbgnfjuvy";
+    const ID_B: &str = "yfvcaxcanospydggtqjzznqkxikmtycu";
+
+    #[test]
+    fn only_the_files_self_replace_leaves_are_leftovers() {
+        let stem = "bugsee-cli";
+        assert!(is_self_replace_leftover(
+            &format!(".bugsee-cli.{ID_A}.__selfdelete__.exe"),
+            stem
+        ));
+        assert!(is_self_replace_leftover(
+            &format!(".bugsee-cli.{ID_B}.__relocated__.exe"),
+            stem
+        ));
+        // Windows does not keep the casing a process was started with stable.
+        assert!(is_self_replace_leftover(
+            &format!(".BUGSEE-CLI.{ID_A}.__relocated__.exe"),
+            stem
+        ));
+        assert!(is_self_replace_leftover(
+            &format!(".bugsee-cli.{ID_A}.__relocated__.exe"),
+            "Bugsee-CLI"
+        ));
+        let upper = ID_A.to_uppercase();
+        let short = &ID_A[..31];
+        let long = format!("{ID_A}a");
+        let digit = format!("{}1", &ID_A[..31]);
+        for other in [
+            "bugsee-cli.exe".to_string(),
+            ".bugsee-cli.__relocated__.exe".to_string(), // no random part
+            format!(".bugsee-cli.{ID_A}.__temp__.exe"),
+            format!(".other.{ID_A}.__relocated__.exe"), // someone else's binary
+            format!("bugsee-cli.{ID_A}.__relocated__.exe"), // not hidden
+            ".bugsee-cli-update-check".to_string(),
+            format!(".bugsee-clix.{ID_A}.__relocated__.exe"),
+            // a random part that is not what self-replace generates
+            ".bugsee-cli.backup.__relocated__.exe".to_string(),
+            format!(".bugsee-cli.{short}.__relocated__.exe"),
+            format!(".bugsee-cli.{long}.__relocated__.exe"),
+            format!(".bugsee-cli.{digit}.__relocated__.exe"),
+            format!(".bugsee-cli.{upper}.__relocated__.exe"),
+        ] {
+            assert!(!is_self_replace_leftover(&other, stem), "{other}");
+        }
+    }
+
+    #[test]
+    fn the_sweep_removes_leftovers_and_nothing_else() {
+        let tmp = tempfile::tempdir().unwrap();
+        let mine = [
+            format!(".bugsee-cli.{ID_A}.__relocated__.exe"),
+            format!(".bugsee-cli.{ID_B}.__selfdelete__.exe"),
+        ];
+        let dir_named_like_one = format!(".bugsee-cli.{}.__relocated__.exe", "z".repeat(32));
+        let keep = [
+            "bugsee-cli.exe".to_string(),
+            format!(".other.{ID_A}.__relocated__.exe"),
+            "notes.txt".to_string(),
+        ];
+        for n in mine.iter().chain(keep.iter()) {
+            std::fs::write(tmp.path().join(n), b"x").unwrap();
+        }
+        std::fs::create_dir(tmp.path().join(&dir_named_like_one)).unwrap();
+
+        // The directory entry is not a file: it must not count as removed, and must not abort.
+        assert_eq!(sweep_self_replace_leftovers_in(tmp.path(), "bugsee-cli"), 2);
+
+        let mut left: Vec<String> = std::fs::read_dir(tmp.path())
+            .unwrap()
+            .map(|e| e.unwrap().file_name().to_string_lossy().into_owned())
+            .collect();
+        left.sort();
+        let mut want: Vec<String> = keep.to_vec();
+        want.push(dir_named_like_one);
+        want.sort();
+        assert_eq!(left, want);
+    }
+
+    #[test]
+    fn sweeping_a_missing_directory_is_not_an_error() {
+        let tmp = tempfile::tempdir().unwrap();
+        assert_eq!(
+            sweep_self_replace_leftovers_in(&tmp.path().join("nope"), "bugsee-cli"),
+            0
+        );
+    }
+
     // ── install(): download + verify + extract against a mock server ─
 
     #[tokio::test]

tests/update_e2e.rs

@@ -466,10 +466,14 @@ fn names_in(dir: &Path) -> Vec<String> {
 /// loader ignores) and then checks what a user depends on after an update:
 /// - the replaced `.exe` launches, and reports the version;
 /// - it can update again, over a file that was itself put there by a self-replace;
-/// - nothing is left behind once the helper has finished. `self-replace` moves the old binary and
-///   starts its `.__selfdelete__.exe` helper from `env::temp_dir()` (it only falls back to the
-///   binary's directory when that is on another volume), so the updater is given a temp directory
-///   the test owns and BOTH that and the binary's directory must end up clean.
+/// - the helper cleans up after itself. `self-replace` moves the old binary and starts its
+///   `.__selfdelete__.exe` helper from `env::temp_dir()` (it only falls back to the binary's
+///   directory when that is on another volume), so the updater is given a temp directory the test
+///   owns. The helper makes ONE delete attempt and never retries, so something outside the test
+///   (a scanner) can occasionally beat it; `update` therefore sweeps what an earlier update left.
+///   The test accepts that, but only in the way it is bounded: files left by round N must be gone
+///   after round N+1, and a helper that is broken leaves files EVERY round, so at least one round
+///   must end with nothing left.
 #[cfg(windows)]
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn update_on_windows_leaves_a_runnable_binary_that_updates_again_and_no_litter() {
@@ -481,16 +485,38 @@ async fn update_on_windows_leaves_a_runnable_binary_that_updates_again_and_no_li
     let exe_name = exe.file_name().unwrap().to_string_lossy().into_owned();
     let real = std::fs::read(&exe).unwrap();
     let newer = format!("{}.99.0", current_major());
-
-    for round in 1..=2 {
+    let systemp = tmp.path().join("systemp");
+    std::fs::create_dir(&systemp).unwrap();
+    let mut clean_rounds = 0;
+    let mut left_by_previous: Vec<String> = Vec::new();
+    // Files the sweep has to remove EVERY round whether or not the helper managed to clean up, so
+    // that a sweep that is not called, or looks in the wrong place, cannot pass by luck: one in each
+    // place `self-replace` can put them, the first spelled in another case than the binary's name.
+    // And one that belongs to nobody it knows, which has to survive.
+    let stem = exe_name.trim_end_matches(".exe").to_string();
+    let stranger = format!(".someone-else.{}.__relocated__.exe", "z".repeat(32));
+    std::fs::write(systemp.join(&stranger), b"not ours").unwrap();
+    let only_ours = |dir: &Path| -> Vec<String> {
+        names_in(dir)
+            .into_iter()
+            .filter(|n| *n != stranger)
+            .collect()
+    };
+
+    for round in 1..=3 {
         let mut payload = real.clone();
         payload.extend_from_slice(format!("\0BUGSEE-TEST-OVERLAY-{round}\0").as_bytes());
         let artefacts = tmp.path().join(format!("release-{round}"));
         std::fs::create_dir(&artefacts).unwrap();
         let archive = build_release_archive(&artefacts, triple, &payload);
         let server = serve_release(&newer, triple, &archive).await;
-        let systemp = tmp.path().join(format!("systemp-{round}"));
-        std::fs::create_dir(&systemp).unwrap();
+
+        // The random part self-replace generates: 32 lowercase letters (a different one per round).
+        let id = char::from(b'a' + round as u8).to_string().repeat(32);
+        let planted_temp = format!(".{}.{id}.__relocated__.exe", stem.to_uppercase());
+        let planted_bin = format!(".{stem}.{id}.__selfdelete__.exe");
+        std::fs::write(systemp.join(&planted_temp), b"old").unwrap();
+        std::fs::write(bindir.join(&planted_bin), b"old").unwrap();
 
         let (base, exe_for_run, temp_for_run) = (server.uri(), exe.clone(), systemp.clone());
         tokio::task::spawn_blocking(move || {
@@ -517,24 +543,49 @@ async fn update_on_windows_leaves_a_runnable_binary_that_updates_again_and_no_li
         // that is still here after a generous wait means that attempt failed. The wait is for what
         // is not covered by the pipes: the helper's own `.__selfdelete__.exe` goes away only when
         // the last handle to it closes (`FILE_FLAG_DELETE_ON_CLOSE`), and a scanner may hold one.
-        let (mut left, mut in_temp) = (names_in(&bindir), names_in(&systemp));
+        let (mut left, mut in_temp) = (names_in(&bindir), only_ours(&systemp));
         for _ in 0..80 {
             if left == [exe_name.clone()] && in_temp.is_empty() {
                 break;
             }
             std::thread::sleep(std::time::Duration::from_millis(250));
-            (left, in_temp) = (names_in(&bindir), names_in(&systemp));
+            (left, in_temp) = (names_in(&bindir), only_ours(&systemp));
         }
+        assert!(
+            !left.contains(&planted_bin),
+            "round {round}: the sweep did not remove {planted_bin} from next to the binary"
+        );
         assert_eq!(
             left,
             [exe_name.clone()],
             "round {round}: the old binary was not deleted from next to the new one"
         );
+        // What this update's sweep was meant to remove must be gone, and what is not ours stays.
+        assert!(
+            !in_temp.contains(&planted_temp),
+            "round {round}: the sweep did not remove {planted_temp} from the temp directory"
+        );
         assert!(
-            in_temp.is_empty(),
-            "round {round}: the helper did not delete what it was meant to (a failed delete is never \
-             retried): {in_temp:?}"
+            systemp.join(&stranger).exists(),
+            "round {round}: the sweep removed a file that is not left by self-replace for this binary"
         );
+        for name in &left_by_previous {
+            assert!(
+                !in_temp.contains(name),
+                "round {round}: {name} was left by the previous update and not swept"
+            );
+        }
+        // Whatever is left is the helper's own litter and nothing else.
+        for name in &in_temp {
+            assert!(
+                name.starts_with('.')
+                    && (name.ends_with(".__relocated__.exe")
+                        || name.ends_with(".__selfdelete__.exe")),
+                "round {round}: unexpected file in the temp directory: {name}"
+            );
+        }
+        clean_rounds += usize::from(in_temp.is_empty());
+        left_by_previous = in_temp;
 
         // It is a program, not just bytes: it starts and knows its version.
         let exe_for_run = exe.clone();
@@ -548,4 +599,9 @@ async fn update_on_windows_leaves_a_runnable_binary_that_updates_again_and_no_li
         .await
         .unwrap();
     }
+
+    assert!(
+        clean_rounds >= 1,
+        "the helper left the old binary behind after every one of 3 updates: it is not deleting"
+    );
 }