Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,26 @@ jobs:
# A binary killed by a signal must not look like a success (#87).
run: node --test npm/test-launcher.js

# `bugsee-cli update` replaces the running executable with the `self-replace` crate, whose Windows
# implementation (move the old .exe aside, drop the new one in, delete the leftovers from a helper
# process) is entirely different from the Unix rename. A dependency bump that touches it (the crate
# itself, or the `windows-sys` it is built on) must fail HERE, by name, not somewhere in the
# 500-test suite. Both Windows architectures, because the release ships both.
self-update-windows:
name: self-update (${{ matrix.os }})
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os: [windows-latest, windows-11-arm]
steps:
- uses: actions/checkout@v7
- uses: dtolnay/rust-toolchain@stable
- uses: Swatinem/rust-cache@v2
# One test at a time: each self-replace starts a helper process that makes a single attempt
# to delete the leftovers, and the test watches for exactly that.
- run: cargo test --test update_e2e -- --nocapture --test-threads=1

test:
name: test (${{ matrix.os }})
runs-on: ${{ matrix.os }}
Expand Down
127 changes: 127 additions & 0 deletions tests/update_e2e.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Comment on lines +514 to +525

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Medium: d0bedf9 doesn'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 when update exits. It returns once every holder of the child's stdout/stderr pipes has closed them:

  • self-replace starts the helper with Command::new(tmp_exe)…spawn(), which inherits stdio by default. On Windows, std duplicates those handles as inheritable.
  • The helper then calls CreateProcessA("cmd.exe /c exit", bInheritHandles = 1), so cmd.exe holds them too.

So when .assert() returns, the helper has already gone through WaitForSingleObject(parent) → one DeleteFileW(relocated) → ExitProcess. self-replace never retries that delete, in 1.5.0 or in 1.5.1 (#97 only changes HANDLE casts in windows.rs). That has two consequences:

  • The 20 s poll can't turn a failed delete into a pass. If DeleteFileW failed, the helper took the ExitProcess(1) path without handing off to cmd.exe, and nothing will ever delete either file. That is why the earlier run waited the full 20 s and still saw both .__selfdelete__.exe and .__relocated__.exe. (A hung helper would not have shown up as a 20 s timeout. It would have blocked .assert() forever.)
  • Moving --version below the poll can't change the outcome. The delete attempt is over before either step runs. --version also launches the new bin\bugsee-cli.exe, not the relocated file in systemp. 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=1 doesn't address it either. The failing log (run 41677155537, job self-update (windows-11-arm)):

  • The other five tests had all finished by 16:45:23.26.
  • The panic came at 16:45:51.25. Take off the 20 s poll, and round 1's update finished around 16: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 update alone took about 8 s). I haven't confirmed that.

Impact.

  • Both Windows ARM gates stay intermittently red. This push's self-update (windows-11-arm) passing once doesn't show the race is gone. Also, test (windows-11-arm) and test (windows-latest) run this same test with default parallelism through cargo 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.
  • It may be a real user-facing leak. If AV can beat the helper's delete on a runner, it can on a user's machine. Each bugsee-cli update there would leave two copies of the exe in %TEMP%. The test has found that, and neither self-replace version deals with it.

Fix.

  1. Correct the comments here and on the --test-threads=1 step in ci.yml. Make the failure message name the cause, for example "the helper exited without deleting the old binary". On NTFS, DeleteFileW takes the name away straight after the call returns, so a .__relocated__.exe still present after .assert() means the delete failed. It doesn't mean it's slow.
  2. Make cleanup deterministic in the product rather than hoping the race goes your way. Before calling self_replace, update would do a best-effort sweep (ignoring errors) of env::temp_dir() for .bugsee-cli.*.__relocated__.exe and .bugsee-cli.*.__selfdelete__.exe left 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 in systemp is exactly those crate-suffixed names. No download tempdir, no .__temp__.exe.
  3. If you'd rather confirm the cause first: a one-off run of the dedicated job with a Defender exclusion on the test's temp dir (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 →

assert_eq!(
left,
[exe_name.clone()],
"round {round}: self-replace left files next to the binary"
);
Comment thread
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:?}"
);
Comment thread
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 self-replace 1.5.0 (src/windows.rs), self_replace first renames the running exe to <bindir>/.bugsee-cli.<rand>.__relocated__.exe. Then schedule_self_deletion_on_shutdown(&old_exe, None) tries fs::rename(exe, env::temp_dir()/.bugsee-cli.<rand>.__relocated__.exe). When that rename succeeds, both the relocated old binary and the .__selfdelete__.exe helper copy are created in %TEMP%, not in bindir. The helper and its leftovers only stay in bindir when that rename fails, which in practice means %TEMP% is on a different volume.

Scenario. tmp comes from tempfile::tempdir(), which is std::env::temp_dir(), so bindir and %TEMP% are always on the same volume. The rename always succeeds. By the time update exits, bindir already contains only bugsee-cli.exe, before the helper has done anything. The poll exits on its first iteration.

Suppose a self-replace or windows-sys bump breaks the helper. For example, WaitForSingleObject gets a bad handle and DeleteFileW never runs, or the DELETE_ON_CLOSE / cmd.exe /c exit trick stops deleting the .__selfdelete__.exe copy. This test still passes. Every update then leaves roughly an 8 MB .exe (×2) in the user's %TEMP%. That is the failure mode the PR description says this test covers ("anything about the helper's leftovers").

Impact. One of the three guarantees the test advertises is not checked. The new self-update job therefore gives less protection against #97 than it claims. The other two checks are real: the bytes land and launch, and a second update works.

Fix. Point the child's temp dir at a directory the test owns, then require that it drains as well. std::env::temp_dir() on Windows reads TMP first, then TEMP:

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 update_self_replaces_to_a_newer_same_major_release, which writes .bugsee-cli.* helpers into the shared runner %TEMP% under the same name prefix.

If you also want to cover the cross-volume fallback, where the helper does live in bindir, you would need a second volume, which isn't practical on hosted runners. It is enough to note in the doc comment that the bindir assertion only matters on that path.

The doc comment at L469 and the CI job comment ("delete the leftovers from a helper process") should then describe what is actually checked.

}