neuwirth
/
pmacs
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

test(process): pin exact diagnostic values and drop the timing-dependent sleeps 

Round-1 review found both test weaknesses.

The exited-child tests used a fixed 300 ms sleep as proof the child had
exited, which on a loaded runner can be false and would turn them into
spurious failures. nix's waitid is unavailable on macOS and libc::waitid
would need unsafe, which the crate forbids, so the tests now synchronise
on the observation under test: a bounded loop that drives the production
diagnostic until it reports the leader as exited. Each failing attempt
leaves the record untouched because the failure path returns before any
bookkeeping, so the loop is side-effect free, and it is strictly stronger
than a sleep because it observes the actual state rather than assuming it.

The assertions were substring checks -- target=-, expected_group=-,
leader=exited( -- which a hardcoded target or a wrong exit code would
satisfy. They are now exact message equality built from the pid the
kernel actually assigned and the errno's own Display, and the one-event
test asserts the surviving event carries exit code 7 rather than any
terminal event. The group test also spawns /bin/sleep directly rather
than through a shell, since a shell may place the command in a different
foreground process group than the one being asserted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk
This commit is contained in:
Levi Neuwirth 2026-07-25 22:16:56 -04:00
parent 52731ba121
commit 40f7f81690
1 changed files with 122 additions and 91 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

213
src/process.rs
View File

@ -2278,103 +2278,131 @@ mod tests {
); );
} }
/// Spawn a PTY child that stays alive until terminated, and wait /// Spawn a PTY child that leads its own session and stays alive
/// for its `Started` event so a pid and a foreground group exist. /// until terminated, returning its id and OS pid.
fn spawn_live_pty(sup: &mut ProcessSupervisor, name: &str) -> ProcessId { ///
let mut spec = ProcessSpec::new(name, "/bin/sh"); /// `/bin/sleep` directly rather than through a shell: a shell may
spec.args = vec!["-c".into(), "sleep 30".into()]; /// place the command in a different foreground process group, and
/// these tests assert the exact target the tty reports.
fn spawn_live_pty(sup: &mut ProcessSupervisor, name: &str) -> (ProcessId, u32) {
let mut spec = ProcessSpec::new(name, "/bin/sleep");
spec.args = vec!["30".into()];
spec.mode = ProcessMode::Pty { spec.mode = ProcessMode::Pty {
rows: 24, rows: 24,
cols: 80, cols: 80,
mode: TerminalMode::Canonical, mode: TerminalMode::Canonical,
}; };
let id = sup.spawn(spec).expect("spawn"); let id = sup.spawn(spec).expect("spawn");
let _ = drain_until(sup, id, Duration::from_secs(5), |evs| { (id, spawn_started_pid(sup, id))
}
/// Drain until `Started` and return the OS pid it carries.
fn spawn_started_pid(sup: &mut ProcessSupervisor, id: ProcessId) -> u32 {
let evs = drain_until(sup, id, Duration::from_secs(5), |evs| {
evs.iter() evs.iter()
.any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) .any(|e| matches!(e.kind, ProcessEventKind::Started { .. }))
}); });
id evs.iter()
.find_map(|e| match e.kind {
ProcessEventKind::Started { pid } => Some(pid),
_ => None,
})
.expect("Started carries a pid")
}
/// Drive the production diagnostic until it observes the leader as
/// exited, bounded by `timeout`.
///
/// A fixed sleep is NOT proof of exit — on a loaded runner the child
/// can still be live, which would turn these tests into false
/// failures. This synchronises on the very observation under test.
/// Each failing attempt leaves the record untouched, because the
/// failure path returns before any bookkeeping (Q#PD2), so looping
/// is side-effect free.
fn terminate_until_leader_exited(
sup: &mut ProcessSupervisor,
id: ProcessId,
timeout: Duration,
) -> String {
let deadline = Instant::now() + timeout;
loop {
sup.force_next_kill_errno(nix::errno::Errno::EPERM);
let err = sup.terminate(id).expect_err("injected EPERM must fail");
if err.contains("leader=exited(") {
return err;
}
assert!(
Instant::now() < deadline,
"leader never observed as exited within {timeout:?}: {err}"
);
std::thread::sleep(Duration::from_millis(10));
}
} }
/// Q#PD1 acceptance 1 — a group-directed failure names the target, /// Q#PD1 acceptance 1 — a group-directed failure names the target,
/// the branch that chose it, the expected group, the errno, and the /// the branch that chose it, the expected group, the errno, and the
/// leader's own state, as five separate facts. /// leader's own state, as five separate facts.
/// ///
/// The leader field is the one that matters: for a PTY the signal /// Asserted as an exact message against the pid the kernel actually
/// goes to the terminal's foreground group, which is a different /// assigned, so a hardcoded target could not satisfy it. The leader
/// entity from the spawned child whenever job control has moved /// field is the one that matters: for a PTY the signal goes to the
/// the terminal. Three rejected designs for this code collapsed /// terminal's foreground group, a different entity from the spawned
/// the two; the report keeps them apart. /// child whenever job control has moved the terminal. Three rejected
/// designs for this code collapsed the two; the report keeps them
/// apart, and here they are asserted to agree only because nothing
/// has moved the terminal.
#[test] #[test]
fn a_group_directed_kill_failure_reports_target_and_leader_separately() { fn a_group_directed_kill_failure_reports_target_and_leader_separately() {
let mut sup = ProcessSupervisor::new(); let mut sup = ProcessSupervisor::new();
let id = spawn_live_pty(&mut sup, "diag-group"); let (id, pid) = spawn_live_pty(&mut sup, "diag-group");
sup.force_next_kill_errno(nix::errno::Errno::EPERM); sup.force_next_kill_errno(nix::errno::Errno::EPERM);
let err = sup.terminate(id).expect_err("injected EPERM must fail"); let err = sup.terminate(id).expect_err("injected EPERM must fail");
assert!(err.contains("EPERM"), "errno is reported: {err}"); let expected = format!(
assert!( "kill: {} (target=-{pid} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=live)",
err.contains("via tcgetpgrp"), nix::errno::Errno::EPERM
"the target SOURCE distinguishes a tty-read group from a spawn group: {err}"
); );
assert!( assert_eq!(
err.contains("target=-"), err, expected,
"a group target renders negative: {err}" "the report names the exact target the tty reported, the exact \
); leader pid, and observes the leader as live"
assert!(
err.contains("expected_group=-"),
"the spawn-time group is shown so a divergence is visible: {err}"
);
assert!(
err.contains("leader=live"),
"the leader is observed independently of the group: {err}"
);
// Non-vacuity: the two numbers are actually rendered, not empty.
assert!(
err.contains("leader_pid=") && !err.contains("leader_pid=0,"),
"a real leader pid is reported: {err}"
); );
let _ = sup.signal(id, Signal::SIGKILL);
} }
/// Q#PD1 acceptance 2 — a leader-directed failure records the /// Q#PD1 acceptance 2 — a leader-directed failure records the
/// fallback branch and a positive target, and omits the group /// fallback branch and a positive target, and omits the group field
/// field that would be meaningless for it. /// that would be meaningless for it. Exact message again.
#[test] #[test]
fn a_leader_directed_kill_failure_reports_the_fallback_branch() { fn a_leader_directed_kill_failure_reports_the_fallback_branch() {
let mut sup = ProcessSupervisor::new(); let mut sup = ProcessSupervisor::new();
let mut spec = ProcessSpec::new("diag-leader", "/bin/sh"); let mut spec = ProcessSpec::new("diag-leader", "/bin/sleep");
spec.args = vec!["-c".into(), "sleep 30".into()]; spec.args = vec!["30".into()];
let id = sup.spawn(spec).expect("spawn"); let id = sup.spawn(spec).expect("spawn");
let _ = drain_until(&mut sup, id, Duration::from_secs(5), |evs| { let pid = spawn_started_pid(&mut sup, id);
evs.iter()
.any(|e| matches!(e.kind, ProcessEventKind::Started { .. }))
});
sup.force_next_kill_errno(nix::errno::Errno::ESRCH); sup.force_next_kill_errno(nix::errno::Errno::ESRCH);
let err = sup.terminate(id).expect_err("injected ESRCH must fail"); let err = sup.terminate(id).expect_err("injected ESRCH must fail");
assert!(err.contains("ESRCH"), "errno is reported: {err}"); let expected = format!(
assert!( "kill: {} (target={pid} via leader-pid, leader_pid={pid}, leader=live)",
err.contains("via leader-pid"), nix::errno::Errno::ESRCH
"a non-group pipe child targets its own pid: {err}"
); );
assert!( assert_eq!(
!err.contains("target=-"), err, expected,
"a leader target renders positive: {err}" "a non-group pipe child targets its own pid, and the group \
); field is omitted where it has no meaning"
assert!(
!err.contains("expected_group="),
"the group field is omitted where it has no meaning: {err}"
); );
let _ = sup.signal(id, Signal::SIGKILL); let _ = sup.signal(id, Signal::SIGKILL);
} }
/// Q#PD1 acceptance 3 — every leader state renders distinctly. The /// Q#PD1 acceptance 3 — every leader state renders distinctly. The
/// `Unobservable` and `NoRuntime` arms cannot be produced by a /// `Unobservable` and `NoRuntime` arms cannot be produced by a real
/// real child on demand, so they are pinned directly; `live` and /// child on demand, so they are pinned directly; `live` and `exited`
/// `exited` are pinned through the real path by the tests around /// are pinned through the real path by the tests around this one.
/// this one.
#[test] #[test]
fn every_leader_observation_renders_distinctly() { fn every_leader_observation_renders_distinctly() {
assert_eq!( assert_eq!(
@ -2393,25 +2421,27 @@ mod tests {
assert_eq!(LeaderObservation::NoRuntime.render(), "no-runtime"); assert_eq!(LeaderObservation::NoRuntime.render(), "no-runtime");
} }
/// Q#PD1 acceptance 3, exited arm through the REAL path — the /// Q#PD1 acceptance 3, exited arm through the REAL path — the leader
/// leader has genuinely exited and the report says so. /// has genuinely exited and the report carries its exact code, not
/// merely "some exit".
#[test] #[test]
fn a_failure_after_the_child_exits_reports_the_leader_as_exited() { fn a_failure_after_the_child_exits_reports_the_leader_as_exited() {
let mut sup = ProcessSupervisor::new(); let mut sup = ProcessSupervisor::new();
let mut spec = ProcessSpec::new("diag-exited", "/bin/sh"); let mut spec = ProcessSpec::new("diag-exited", "/bin/sh");
spec.args = vec!["-c".into(), "exit 3".into()]; spec.args = vec!["-c".into(), "exit 3".into()];
let id = sup.spawn(spec).expect("spawn"); let id = sup.spawn(spec).expect("spawn");
// Wait for the child to actually be gone, but do NOT tick past let pid = spawn_started_pid(&mut sup, id);
// the point where the record leaves Running — `signal` needs a
// live record to reach the kill at all.
std::thread::sleep(Duration::from_millis(300));
sup.force_next_kill_errno(nix::errno::Errno::EPERM); let err = terminate_until_leader_exited(&mut sup, id, Duration::from_secs(10));
let err = sup.terminate(id).expect_err("injected EPERM must fail");
assert!( let expected = format!(
err.contains("leader=exited("), "kill: {} (target={pid} via leader-pid, leader_pid={pid}, leader=exited(code 3))",
"an exited leader is observed as exited, not guessed from the errno: {err}" nix::errno::Errno::EPERM
);
assert_eq!(
err, expected,
"the exact exit code is observed from the real child, not \
inferred from the errno"
); );
} }
@ -2427,10 +2457,7 @@ mod tests {
spec.args = vec!["-c".into(), "sleep 30".into()]; spec.args = vec!["-c".into(), "sleep 30".into()];
spec.group = true; spec.group = true;
let id = sup.spawn(spec).expect("spawn"); let id = sup.spawn(spec).expect("spawn");
let _ = drain_until(&mut sup, id, Duration::from_secs(5), |evs| { let pid = spawn_started_pid(&mut sup, id);
evs.iter()
.any(|e| matches!(e.kind, ProcessEventKind::Started { .. }))
});
assert!( assert!(
sup.reap_ledger.is_empty(), sup.reap_ledger.is_empty(),
"precondition: nothing armed before the attempt" "precondition: nothing armed before the attempt"
@ -2438,7 +2465,12 @@ mod tests {
sup.force_next_kill_errno(nix::errno::Errno::EPERM); sup.force_next_kill_errno(nix::errno::Errno::EPERM);
let err = sup.terminate(id).expect_err("injected EPERM must fail"); let err = sup.terminate(id).expect_err("injected EPERM must fail");
assert!(err.contains("via group"), "a group=true pipe child: {err}");
let expected = format!(
"kill: {} (target=-{pid} via group, leader_pid={pid}, expected_group=-{pid}, leader=live)",
nix::errno::Errno::EPERM
);
assert_eq!(err, expected, "a group=true pipe child reports via group");
assert!( assert!(
matches!( matches!(
@ -2458,7 +2490,7 @@ mod tests {
/// Q#PD3/Q#PD4 acceptance 5 — the diagnostic consults the REAL /// Q#PD3/Q#PD4 acceptance 5 — the diagnostic consults the REAL
/// `ChildHandle::try_wait` on the REAL child, which reaps it and /// `ChildHandle::try_wait` on the REAL child, which reaps it and
/// caches the status. `poll_one` must still emit exactly one exit /// caches the status. `poll_one` must still emit exactly one exit
/// event afterwards. /// event, carrying the exact code.
/// ///
/// A stubbed observation would bypass the double-`try_wait` path /// A stubbed observation would bypass the double-`try_wait` path
/// entirely and pin nothing, so the injection replaces the kill /// entirely and pin nothing, so the injection replaces the kill
@ -2474,34 +2506,33 @@ mod tests {
mode: TerminalMode::Canonical, mode: TerminalMode::Canonical,
}; };
let id = sup.spawn(spec).expect("spawn"); let id = sup.spawn(spec).expect("spawn");
std::thread::sleep(Duration::from_millis(300)); let _ = spawn_started_pid(&mut sup, id);
// The forced failure drives `observe_leader`, which try_waits // Drives `observe_leader`, which try_waits the real PTY child
// the real PTY child for the first time. // for the first time and reaps it.
sup.force_next_kill_errno(nix::errno::Errno::EPERM); let err = terminate_until_leader_exited(&mut sup, id, Duration::from_secs(10));
let err = sup.terminate(id).expect_err("injected EPERM must fail");
assert!( assert!(
err.contains("leader=exited("), err.contains("leader=exited(code 7)"),
"the real handle was consulted: {err}" "the real handle was consulted and carries the exact code: {err}"
); );
// Now the supervisor's own try_wait must still see the status. // The supervisor's own try_wait must still see that status.
let evs = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); let evs = drain_until(&mut sup, id, Duration::from_secs(5), has_exited);
let terminal = evs let terminal: Vec<i32> = evs
.iter() .iter()
.filter(|e| { .filter_map(|e| match e.kind {
matches!( ProcessEventKind::Exited { code, .. } => Some(code),
e.kind, ProcessEventKind::Signaled { .. } => Some(-1),
ProcessEventKind::Exited { .. } | ProcessEventKind::Signaled { .. } _ => None,
)
}) })
.count(); .collect();
assert_eq!( assert_eq!(
terminal, 1, terminal,
"exactly one terminal event survives the diagnostic's try_wait" vec![7],
"exactly one terminal event survives the diagnostic's try_wait, \
carrying the child's real exit code"
); );
} }
#[test] #[test]
fn signal_terminates_a_running_child() { fn signal_terminates_a_running_child() {
let mut sup = ProcessSupervisor::new(); let mut sup = ProcessSupervisor::new();