Repository navigation
test(ci): exercise the Windows self-replace for real, as its own check #100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -419,3 +419,130 @@ async fn update_readonly_dir_fails_safe_and_keeps_binary() { | |
| "a failed self-replace must leave the existing binary untouched" | ||
| ); | ||
| } | ||
|
|
||
| /// Mount the three routes of a release: the per-major pointer, the archive and its SHA-256. | ||
| #[cfg(windows)] | ||
| async fn serve_release(newer: &str, triple: &str, artefact: &[u8]) -> MockServer { | ||
| let server = MockServer::start().await; | ||
| mount_pointer(&server, newer).await; | ||
| Mock::given(method("GET")) | ||
| .and(wm_path(format!( | ||
| "/v{newer}/bugsee-cli-{triple}.{}", | ||
| artifact_ext() | ||
| ))) | ||
| .respond_with(ResponseTemplate::new(200).set_body_bytes(artefact.to_vec())) | ||
| .mount(&server) | ||
| .await; | ||
| Mock::given(method("GET")) | ||
| .and(wm_path(format!( | ||
| "/v{newer}/bugsee-cli-{triple}.{}.sha256", | ||
| artifact_ext() | ||
| ))) | ||
| .respond_with( | ||
| ResponseTemplate::new(200).set_body_string(format!("{} x", sha256_hex(artefact))), | ||
| ) | ||
| .mount(&server) | ||
| .await; | ||
| server | ||
| } | ||
|
|
||
| /// What is in a directory, by file name. | ||
| #[cfg(windows)] | ||
| fn names_in(dir: &Path) -> Vec<String> { | ||
| let mut names: Vec<String> = std::fs::read_dir(dir) | ||
| .unwrap() | ||
| .map(|e| e.unwrap().file_name().to_string_lossy().into_owned()) | ||
| .collect(); | ||
| names.sort(); | ||
| names | ||
| } | ||
|
|
||
| /// The Windows half of `self_replace`, which is not a rename: a running `.exe` cannot be | ||
| /// overwritten, so the crate moves the old binary aside, puts the new one in place and starts a | ||
| /// copy of itself that deletes the leftovers once this process has exited. | ||
| /// | ||
| /// The test above puts a sentinel in place, which proves the bytes land but not that what lands | ||
| /// is a program. This one swaps in a REAL binary (this build plus a few trailing bytes, which a PE | ||
| /// 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. | ||
| #[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() { | ||
| let triple = host_triple(); | ||
| let tmp = tempfile::tempdir().unwrap(); | ||
| let bindir = tmp.path().join("bin"); | ||
| std::fs::create_dir(&bindir).unwrap(); | ||
| let exe = copy_binary(&bindir); | ||
| 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 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(); | ||
|
|
||
| let (base, exe_for_run, temp_for_run) = (server.uri(), exe.clone(), systemp.clone()); | ||
| tokio::task::spawn_blocking(move || { | ||
| assert_cmd::Command::new(&exe_for_run) | ||
| .env("BUGSEE_CLI_UPDATE_BASE_URL", &base) | ||
| .env("TMP", &temp_for_run) | ||
| .env("TEMP", &temp_for_run) | ||
| .arg("update") | ||
| .assert() | ||
| .success(); | ||
| }) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_eq!( | ||
| std::fs::read(&exe).unwrap(), | ||
| payload, | ||
| "round {round}: the binary should be the downloaded release" | ||
| ); | ||
|
|
||
| // The helper that removes the old binary runs after the updater exits: give it time, then | ||
| // require that only the binary is left, and that nothing is left in the temp directory. | ||
| // This comes BEFORE launching the new binary, so nothing else is touching the files while | ||
| // the helper makes its single attempt to delete them. | ||
| let (mut left, mut in_temp) = (names_in(&bindir), names_in(&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)); | ||
| } | ||
| assert_eq!( | ||
| left, | ||
| [exe_name.clone()], | ||
| "round {round}: self-replace left files next to the binary" | ||
| ); | ||
|
cursor[bot] marked this conversation as resolved.
Resolved
|
||
| assert!( | ||
| in_temp.is_empty(), | ||
| "round {round}: self-replace left files in the temp directory: {in_temp:?}" | ||
| ); | ||
|
cursor[bot] marked this conversation as resolved.
Resolved
|
||
|
|
||
| // It is a program, not just bytes: it starts and knows its version. | ||
| let exe_for_run = exe.clone(); | ||
| tokio::task::spawn_blocking(move || { | ||
| assert_cmd::Command::new(&exe_for_run) | ||
| .arg("--version") | ||
| .assert() | ||
| .success() | ||
| .stdout(predicates::str::contains(env!("CARGO_PKG_VERSION"))); | ||
| }) | ||
| .await | ||
| .unwrap(); | ||
| } | ||
|
Comment on lines
+514
to
+547
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 Medium: the "no litter" check looks in a directory the helper never uses, so it cannot fail on CI Problem. In Scenario. Suppose a Impact. One of the three guarantees the test advertises is not checked. The new Fix. Point the child's temp dir at a directory the test owns, then require that it drains as well. let systemp = tmp.path().join(format!("systemp-{round}"));
std::fs::create_dir(&systemp).unwrap();
// …
assert_cmd::Command::new(&exe_for_run)
.env("BUGSEE_CLI_UPDATE_BASE_URL", &base)
.env("TMP", &systemp)
.env("TEMP", &systemp)
.arg("update")
// …
// poll until BOTH hold:
// names_in(&bindir) == [exe_name]
// names_in(&systemp).is_empty()
// The CLI's own download tempdir is also created there, but it is dropped before exit.This also isolates the test from the parallel If you also want to cover the cross-volume fallback, where the helper does live in The doc comment at L469 and the CI job comment ("delete the leftovers from a helper process") should then describe what is actually checked. |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2 Medium:
d0bedf9doesn't fix the ARM64 failure. By the time this poll starts, the helper has already made its one delete attempt.Problem.
.assert()above does not return whenupdateexits. It returns once every holder of the child's stdout/stderr pipes has closed them:self-replacestarts the helper withCommand::new(tmp_exe)…spawn(), which inherits stdio by default. On Windows, std duplicates those handles as inheritable.CreateProcessA("cmd.exe /c exit", bInheritHandles = 1), socmd.exeholds them too.So when
.assert()returns, the helper has already gone throughWaitForSingleObject(parent)→ oneDeleteFileW(relocated)→ExitProcess.self-replacenever retries that delete, in 1.5.0 or in 1.5.1 (#97 only changesHANDLEcasts inwindows.rs). That has two consequences:DeleteFileWfailed, the helper took theExitProcess(1)path without handing off tocmd.exe, and nothing will ever delete either file. That is why the earlier run waited the full 20 s and still saw both.__selfdelete__.exeand.__relocated__.exe. (A hung helper would not have shown up as a 20 s timeout. It would have blocked.assert()forever.)--versionbelow the poll can't change the outcome. The delete attempt is over before either step runs.--versionalso launches the newbin\bugsee-cli.exe, not the relocated file insystemp. The new comment at L516-517 ("nothing else is touching the files while the helper makes its single attempt") describes timing that doesn't happen.--test-threads=1doesn't address it either. The failing log (run 41677155537, jobself-update (windows-11-arm)):16:45:23.26.16:45:51.25. Take off the 20 s poll, and round 1'supdatefinished around16:45:31.So when the helper's delete ran, this test was the only thing running in the harness. What the delete lost to is outside the test process. The most likely cause is real-time AV scanning freshly written or launched PEs, which is slow on the ARM64 runner (round 1's
updatealone took about 8 s). I haven't confirmed that.Impact.
self-update (windows-11-arm)passing once doesn't show the race is gone. Also,test (windows-11-arm)andtest (windows-latest)run this same test with default parallelism throughcargo test --all-targets. Those legs gate every PR, not only chore(deps): bump self-replace from 1.5.0 to 1.5.1 in the cargo-minor-patch group #97.bugsee-cli updatethere would leave two copies of the exe in%TEMP%. The test has found that, and neitherself-replaceversion deals with it.Fix.
--test-threads=1step inci.yml. Make the failure message name the cause, for example "the helper exited without deleting the old binary". On NTFS,DeleteFileWtakes the name away straight after the call returns, so a.__relocated__.exestill present after.assert()means the delete failed. It doesn't mean it's slow.self_replace,updatewould do a best-effort sweep (ignoring errors) ofenv::temp_dir()for.bugsee-cli.*.__relocated__.exeand.bugsee-cli.*.__selfdelete__.exeleft by earlier runs. The test can then make a strict, non-flaky assertion: round 2 removes whatever round 1 left. For the last round, require that anything left insystempis exactly those crate-suffixed names. No download tempdir, no.__temp__.exe.Add-MpPreference -ExclusionPath …) would show whether the leftovers go away. Don't keep that exclusion in CI, though. It would hide exactly the condition users hit.Fix this →