Skip to content

Commit 6d0815a

Browse files
Dione-bNearxLabs
andcommitted
Clean up after the orphan test whether or not it passes
What: read every recorded pid's liveness first, kill all of them, and only then assert. Factors the `kill` call the check already made into a helper the cleanup shares. Why: the assertion ran per pid and aborted the test on the first survivor, so the processes it was complaining about were exactly the ones it left behind. They are shells spinning on `while :; do :; done` with no exit condition, so each failing run pinned another core on the machine until someone noticed -- and the run that fails is the run where `kill_on_drop(true)` regressed, which is precisely when nothing else is going to clean them up. Reporting a leak by leaking is a poor trade. Ordering matters: liveness is sampled before the kill so the verdict still describes what `doctor` left running, and the assertion moves after it so the cleanup cannot be skipped by an early panic. Verified by removing `kill_on_drop(true)` again: the test still fails with `processes [..] were still running after doctor exited`, and `ps` afterwards shows none of them left. Co-authored-by: Nearx-Labs <nearxlabs@nearx.com.br>
1 parent 798124a commit 6d0815a

1 file changed

Lines changed: 33 additions & 15 deletions

File tree

cmd/crates/soroban-test/tests/it/doctor.rs

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -490,12 +490,24 @@ fn does_not_leave_an_orphaned_process_when_a_probe_times_out() {
490490
.collect();
491491
assert!(!pids.is_empty());
492492

493-
for pid in pids {
494-
assert!(
495-
!process_is_alive(pid),
496-
"process {pid} is still running after doctor exited"
497-
);
493+
// Read the verdict before acting on it, then clean up whatever is left
494+
// regardless of what it says. Failing here means the probes leaked, and
495+
// what they leaked is a shell spinning on a full core with no exit
496+
// condition -- asserting first would strand one per failing run.
497+
let survivors: Vec<u32> = pids
498+
.iter()
499+
.copied()
500+
.filter(|pid| process_is_alive(*pid))
501+
.collect();
502+
503+
for pid in &pids {
504+
kill(*pid, "-9");
498505
}
506+
507+
assert!(
508+
survivors.is_empty(),
509+
"processes {survivors:?} were still running after doctor exited"
510+
);
499511
}
500512

501513
/// Whether `pid` still refers to a live process, checked with `kill -0`
@@ -512,16 +524,7 @@ fn process_is_alive(pid: u32) -> bool {
512524
std::thread::sleep(std::time::Duration::from_millis(50));
513525
}
514526

515-
let status = std::process::Command::new("kill")
516-
.args(["-0", &pid.to_string()])
517-
// A dead pid is the expected outcome here, so let `kill` report it
518-
// through its exit status alone -- its complaint on stderr is
519-
// localized, and would land in the test output as noise.
520-
.stderr(std::process::Stdio::null())
521-
.status()
522-
.unwrap();
523-
524-
if !status.success() {
527+
if !kill(pid, "-0") {
525528
return false;
526529
}
527530
}
@@ -530,3 +533,18 @@ fn process_is_alive(pid: u32) -> bool {
530533
// actually still running.
531534
true
532535
}
536+
537+
/// Send `signal` to `pid`, reporting whether `kill` accepted it -- `-0` sends
538+
/// nothing and so answers whether the process exists at all.
539+
///
540+
/// A pid that is already gone is an expected outcome at both call sites, so
541+
/// `kill` reports it through its exit status alone: its complaint on stderr is
542+
/// localized, and would land in the test output as noise.
543+
fn kill(pid: u32, signal: &str) -> bool {
544+
std::process::Command::new("kill")
545+
.args([signal, &pid.to_string()])
546+
.stderr(std::process::Stdio::null())
547+
.status()
548+
.unwrap()
549+
.success()
550+
}

0 commit comments

Comments
 (0)