diff --git a/src/cli/update.rs b/src/cli/update.rs index 252740c..3dce391 100644 --- a/src/cli/update.rs +++ b/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` +/// (`...__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 = 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 = 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] diff --git a/tests/update_e2e.rs b/tests/update_e2e.rs index 1428515..c61e237 100644 --- a/tests/update_e2e.rs +++ b/tests/update_e2e.rs @@ -466,10 +466,14 @@ fn names_in(dir: &Path) -> Vec { /// 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 = 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 { + 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" + ); }