From b27df705bbfccc33da4b510aee3d7dd24f0d2df6 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 13:08:47 -0400 Subject: [PATCH] feat(process): make the signal diagnostic discriminating (Bets 2-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Framing acceptances 2, 3, 4, 5, 7 and 8. Evidence collection only: no tolerance rule, no change to which process is signalled, no disposition change. Three distinct failures previously rendered as one string. **The PTY fallback is now named.** When a PTY's foreground-group lookup yields no group, the target falls back to the leader — and until now that rendered "leader-pid", identical to a pipe child that never had a terminal. `portable-pty::MasterPty::process_group_leader` collapses every failure into `None` before pmacs can see it, so the errno was gone too. pmacs now performs the query itself and reports four distinct outcomes: no master fd, a failed duplicate with its errno, a failed `tcgetpgrp` with its errno, and a non-positive answer. Doing that without `unsafe` is the interesting part. `nix::unistd:: tcgetpgrp` needs `AsFd`; `MasterPty` exposes only `Option`; and every std route between them is `unsafe`, which this crate forbids. `filedescriptor::OwnedHandle::dup` takes any `AsRawFd` through a safe blanket impl and returns an owned handle that IS `AsFd`, so a lifetime-tied view implementing one safe trait is the whole bridge. The borrow is what makes it sound: the view cannot outlive the master, so the descriptor cannot close underneath it. **The report names the signal.** A failed SIGUSR1 and a failed SIGTERM were the same text. Note this is a reporting gap only — every failed `kill` returns before the fatal-signal branch, so failed signals are disposition-identical whatever they are. A separate control pins that the fatal/non-fatal difference is real for calls that SUCCEED, which is what gives the first test its meaning. **`measured_group` is a real observation.** `expected_group` is `-leader_pid`, and on the spawn-group path the target is `-leader_pid` too, so the report printed the same number three times and their agreement was arithmetic rather than evidence. `getpgid` supplies the one field that can disagree. It establishes no identity — it is read inside the same read-then-act window, and no portable mechanism closes that for a group. Bites, each by an actual revert, all observed to fail: - collapsing the PTY fallback back into a bare "leader-pid"; - dropping `signal=` from the report; - making the measured group restate the pid it was handed; - replacing the job-control fixture with a plain `sleep`, as a positive control on the divergence fixture itself. All four exact-string sites were updated individually, never by a blanket rewrite: a wholesale rewrite of expected strings is how a format regression hides. `:2501`'s first-call disposition pin is retained and updated for the new format rather than replaced. `nix`'s `process` feature is now declared explicitly. It already arrived transitively — nix's own `signal` feature depends on it — which is stable but invisible, and a real requirement resting on another feature's internals is one refactor away from vanishing. `filedescriptor` is declared directly for the same reason: pmacs now calls its API. The reap ledger's comment claiming "EPERM cannot happen for our own children" is corrected. Its bounded-growth policy is unchanged, but the justification was wrong: the probe targets a group, and owning the spawned child says nothing about a group unless the child is still a member — which nothing measures. The handoff records this together with the limit of the evidence: the occurrence does NOT establish that the child itself received EPERM. --- Cargo.lock | 1 + Cargo.toml | 18 +- docs/agent-handoff.md | 25 ++ src/process.rs | 523 +++++++++++++++++++++++++++++++++++++++--- 4 files changed, 540 insertions(+), 27 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b80dbae..610ddd0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2574,6 +2574,7 @@ dependencies = [ "codebook-tree-sitter-latex", "crossbeam", "crossterm", + "filedescriptor", "loro", "mlua", "nix 0.29.0", diff --git a/Cargo.toml b/Cargo.toml index 4e21592..e8319f9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -262,12 +262,28 @@ arborium-lean = "2.18" # T M4.4 process supervisor: signal sending without `unsafe`. Keep # the feature surface tight to keep build time low. `poll` feeds the # compile-mode group readers (cancellable poll-based reads, Q#CM3). -nix = { version = "0.29", default-features = false, features = ["signal", "user", "fs", "term", "socket", "poll"] } +# +# `process` is listed explicitly even though it already arrives +# transitively: nix's own `signal` feature depends on it, so `getpgid` +# and `tcgetpgrp` compile today without being asked for. That is +# stable but invisible, and a real requirement that depends on another +# feature's internals is one refactor away from vanishing. The signal +# diagnostic calls both directly. +nix = { version = "0.29", default-features = false, features = ["signal", "user", "fs", "term", "socket", "poll", "process"] } # T M4.4 PTY mode: portable abstraction over openpty / fork+exec # with controlling-tty wiring. The crate uses internal `unsafe` # but exposes a fully safe API; pmacs's own `unsafe_code = "forbid"` # rule still holds. portable-pty = "0.9" +# Safe file-descriptor duplication, already in the tree through +# `portable-pty`. Declared directly because the signal diagnostic calls +# `OwnedHandle::dup` itself: `MasterPty` exposes only a `RawFd`, and +# every std route from a raw fd to something implementing `AsFd` is +# `unsafe`. `dup` takes any `AsRawFd` through a safe blanket impl and +# returns an owned handle that IS `AsFd`, which is what lets pmacs call +# `nix::unistd::tcgetpgrp` — and keep the errno portable-pty discards — +# without a single `unsafe` block of its own. +filedescriptor = "0.8" # T M4.5 LSP wire format: JSON-RPC 2.0 bodies inside Content-Length # framing. Used only on LSP and similar protocols that require JSON # specifically; in-process workers continue to use MessagePack diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index e381de0..f31f2fc 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -223,6 +223,31 @@ commands, read `docs/active-work.md` immediately after this file. remain parked pending the evidence this diagnostic produces, as does `terminate` idempotence for an already-reaped process (a different failure, so a different PR). + - **The diagnostic fired, and what it showed.** macOS CI, PR #191, + [run 30553376486](https://github.com/levineuwirth/pmacs/actions/runs/30553376486/job/90907461258): + `target=-8619 via group, leader_pid=8619, expected_group=-8619, + leader=live` — the `spec.group` pipe path, not the PTY path, with the + leader observed alive by a real `try_wait`. A rerun of the identical + head passed 12/12, so it is intermittent. + - **Owning the child does not license dismissing a group error, and + the occurrence does not prove the child received EPERM.** The failed + target was the *group* `-8619`; `try_wait` observed the *process* + `8619`. Nothing measured `getpgid(8619)`, so the two are not known to + refer to the same thing. What is settled is narrower and still + enough: a group target computed from the spawn-time `pgid == pid` + assumption returned EPERM while the leader was alive, which retires + "EPERM cannot happen for our own children" as a reason to discard an + arbitrary group-directed error. Attributing the errno to the child + would repeat the exact error that killed three tolerance rules — + concluding something about one entity from something about another. + - **A field named like an observation can be a restatement of its + input.** `expected_group` is `-leader_pid`, and on the spawn-group + path the target is `-leader_pid` too, so the report printed the same + number three times and their agreement was arithmetic. Stage B adds a + `measured_group` from `getpgid`, which is the only field able to + disagree — and it still establishes no identity, because it is read + inside the same read-then-act window and no portable mechanism closes + that for a *group* (`pidfd` covers a process; macOS has neither). - **Lean 4 arc (Arc 8) — stages 1, 2, 3a, 3b, 4a, 4b ALL LANDED** (`docs/lean4-mode-framing.md`; #160, #161, #167, #170, #179, #181). pmacs edits Lean 4: `arborium-lean` highlighting, a `lean4` major mode, `⟨⟩ ⦃⦄ ⟮⟯` pairs, and a `lake serve` language diff --git a/src/process.rs b/src/process.rs index 13716e7..7f412ae 100644 --- a/src/process.rs +++ b/src/process.rs @@ -477,6 +477,9 @@ pub struct ProcessSupervisor { /// observation still runs against the real child handle; a stubbed /// observation would bypass the code path under test. forced_kill_errno: Option, + /// Test seam for the PTY foreground-group lookup (see + /// `force_next_pty_lookup_failure`). Always `None` outside tests. + forced_pty_lookup: Option, } /// One armed group in the reap ledger. @@ -726,16 +729,58 @@ enum TargetSource { ForegroundGroup, /// A `group = true` pipe child leading its own process group. SpawnGroup, - /// The child's own pid. + /// The child's own pid, for a pipe child that leads no group. LeaderPid, + /// A **PTY** child whose foreground-group lookup did not yield a + /// group, so the target fell back to the leader pid. + /// + /// Distinct from [`Self::LeaderPid`] on purpose. Before this + /// variant existed both rendered "leader-pid", so a PTY whose + /// terminal query failed was indistinguishable in the report from + /// an ordinary pipe child that never had a terminal — two very + /// different situations reading as one. + PtyForegroundFallback(PtyLookupFailure), +} + +/// Why a PTY's foreground-group lookup produced no group. +/// +/// Each arm is a different fact and none is forged into another: a +/// missing fd is not an errno, and a failure to *duplicate* the master +/// is not a failure to *query* the terminal. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum PtyLookupFailure { + /// The master reported no file descriptor to query. + NoMasterFd, + /// Duplicating the master fd failed, so the terminal was never + /// queried at all. + Duplicate(nix::errno::Errno), + /// `tcgetpgrp` itself failed on a successfully duplicated fd. + Query(nix::errno::Errno), + /// The terminal answered, but with a non-positive group id, which + /// names no group. + NonPositive(i32), +} + +impl PtyLookupFailure { + fn render(self) -> String { + match self { + Self::NoMasterFd => "no-master-fd".to_owned(), + Self::Duplicate(e) => format!("duplicate-master-fd: {e}"), + Self::Query(e) => format!("tcgetpgrp: {e}"), + Self::NonPositive(v) => format!("tcgetpgrp-non-positive: {v}"), + } + } } impl TargetSource { - fn as_str(self) -> &'static str { + fn render(self) -> String { match self { - Self::ForegroundGroup => "tcgetpgrp", - Self::SpawnGroup => "group", - Self::LeaderPid => "leader-pid", + Self::ForegroundGroup => "tcgetpgrp".to_owned(), + Self::SpawnGroup => "group".to_owned(), + Self::LeaderPid => "leader-pid".to_owned(), + Self::PtyForegroundFallback(why) => { + format!("pty-leader-fallback({})", why.render()) + } } } @@ -745,6 +790,85 @@ impl TargetSource { } } +/// A lifetime-tied view of a `MasterPty`'s file descriptor. +/// +/// `MasterPty` exposes only `Option`, and every std route from a +/// raw fd to something implementing `AsFd` — `BorrowedFd::borrow_raw`, +/// `OwnedFd::from_raw_fd`, `File::from_raw_fd` — is `unsafe`, which this +/// crate forbids. `filedescriptor::OwnedHandle::dup` accepts any +/// `AsRawFd` through a safe blanket impl and hands back an owned handle +/// that *is* `AsFd`, so implementing this one safe trait is the whole +/// bridge. +/// +/// The borrow is what makes it sound: the view cannot outlive the master +/// it read the descriptor from, so the fd cannot have been closed +/// underneath it. +struct MasterFdView<'a> { + fd: std::os::fd::RawFd, + _master: &'a (dyn portable_pty::MasterPty + Send), +} + +impl std::os::fd::AsRawFd for MasterFdView<'_> { + fn as_raw_fd(&self) -> std::os::fd::RawFd { + self.fd + } +} + +/// Recover the OS errno from a `filedescriptor` error. +/// +/// Its error type is an enum of thiserror variants, each carrying a +/// `std::io::Error` as a `#[source]` rather than exposing +/// `raw_os_error` itself. Walking the source chain and downcasting keeps +/// every variant working, including ones added later, instead of +/// matching the one arm that exists today. +/// +/// Returns `UnknownErrno` when the chain carries no OS error, rather +/// than inventing a plausible one — a forged errno in a diagnostic is +/// worse than an honest absence. +fn os_errno_of(err: &filedescriptor::Error) -> nix::errno::Errno { + let mut current: Option<&(dyn std::error::Error + 'static)> = Some(err); + while let Some(e) = current { + if let Some(io) = e.downcast_ref::() + && let Some(code) = io.raw_os_error() + { + return nix::errno::Errno::from_raw(code); + } + current = e.source(); + } + nix::errno::Errno::UnknownErrno +} + +/// Read the terminal's foreground process group, keeping the errno. +/// +/// `portable_pty::MasterPty::process_group_leader` collapses every +/// failure into `None`, so pmacs could not tell "this tty has no +/// foreground group" from "the query failed and here is why". This does +/// the query itself and returns the reason on every non-success path. +fn pty_foreground_group( + master: &(dyn portable_pty::MasterPty + Send), +) -> Result { + let Some(fd) = master.as_raw_fd() else { + return Err(PtyLookupFailure::NoMasterFd); + }; + let view = MasterFdView { + fd, + _master: master, + }; + let owned = filedescriptor::OwnedHandle::dup(&view) + .map_err(|e| PtyLookupFailure::Duplicate(os_errno_of(&e)))?; + match nix::unistd::tcgetpgrp(&owned) { + Ok(pgrp) => { + let raw = pgrp.as_raw(); + if raw > 0 { + Ok(raw) + } else { + Err(PtyLookupFailure::NonPositive(raw)) + } + } + Err(e) => Err(PtyLookupFailure::Query(e)), + } +} + /// The entity a signal was actually aimed at, plus the branch that /// chose it. Carried so a failure can report the target as a fact /// separate from the leader's state (Q#PD1). @@ -754,18 +878,36 @@ struct SignalTarget { source: TargetSource, } -fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { +fn signal_target( + proc: &ManagedProcess, + pid: u32, + forced_lookup: Option, +) -> Result { if let Some(runtime) = proc.runtime.as_ref() && let ChildHandle::Pty { _master: master, .. } = &runtime.child - && let Some(pgrp) = master.process_group_leader() - && pgrp > 0 { - return Ok(SignalTarget { - pid: Pid::from_raw(-pgrp), - source: TargetSource::ForegroundGroup, - }); + // A PTY child is always group-directed when the terminal names a + // foreground group. When it does not, the target falls back to + // the leader — and *why* it fell back is carried into the source + // so the report can say it. Previously every one of these paths + // produced a bare `LeaderPid`, identical to a pipe child that + // never had a terminal at all. + let lookup = match forced_lookup { + Some(failure) => Err(failure), + None => pty_foreground_group(master.as_ref()), + }; + return match lookup { + Ok(pgrp) => Ok(SignalTarget { + pid: Pid::from_raw(-pgrp), + source: TargetSource::ForegroundGroup, + }), + Err(why) => Ok(SignalTarget { + pid: Pid::from_raw(i32::try_from(pid).map_err(|e| e.to_string())?), + source: TargetSource::PtyForegroundFallback(why), + }), + }; } // `group = true` pipe children lead a fresh process group // (`process_group(0)` at spawn ⇒ pgid == pid), so fatal signals @@ -824,12 +966,45 @@ fn observe_leader(proc: &mut ManagedProcess) -> LeaderObservation { } } -/// Render a failing `kill` as the five facts of Q#PD1. The disposition -/// is unchanged (Q#PD2) — this only replaces a message that said -/// nothing but the errno. +/// The leader's process group as the kernel reports it, for a target +/// that was *computed* from the spawn-time assumption `pgid == pid`. +/// +/// `expected_group` is that assumption restated — it is `-leader_pid`, +/// and on the `SpawnGroup` path the target is `-leader_pid` too, so the +/// two agreeing is arithmetic rather than evidence. This is the only +/// field in the report that can disagree with the input, which is what +/// makes it worth printing. +/// +/// **It does not establish identity** (framing §1.5). It is read before +/// the `kill` in the same read-then-act window, and a number cannot +/// distinguish the original group from a recycled one. No portable +/// mechanism can: `pidfd` closes pid reuse for a process, not a group, +/// and macOS has none at all. This records an observation; it settles +/// nothing. +fn measured_group_of(leader_pid: u32) -> String { + let Ok(raw) = i32::try_from(leader_pid) else { + return ", measured_group=unobservable(pid out of range)".to_owned(); + }; + match nix::unistd::getpgid(Some(Pid::from_raw(raw))) { + Ok(pgid) => format!(", measured_group=-{}", pgid.as_raw()), + Err(e) => format!(", measured_group=unobservable({e})"), + } +} + +/// Render a failing `kill` as the facts of Q#PD1. The disposition is +/// unchanged (Q#PD2) — this only replaces a message that said nothing +/// but the errno. +/// +/// The signal is named because it could not be recovered otherwise: a +/// failed `SIGUSR1` and a failed `SIGTERM` were previously identical +/// text. Note this is a *reporting* gap only — every failed `kill` +/// returns before the fatal-signal branch, so failed signals are +/// disposition-identical whatever they are. The disposition difference +/// is real only for calls that succeed. fn signal_failure_report( target: SignalTarget, leader_pid: u32, + signal: Signal, errno: nix::errno::Errno, leader: &LeaderObservation, ) -> String { @@ -841,10 +1016,19 @@ fn signal_failure_report( } else { String::new() }; + // Only the spawn-group path computes its target from the assumption, + // so it is the only one where a measurement can contradict anything. + // A PTY target came from the terminal and a leader-directed target is + // not a group at all. + let measured = if matches!(target.source, TargetSource::SpawnGroup) { + measured_group_of(leader_pid) + } else { + String::new() + }; format!( - "kill: {errno} (target={} via {}, leader_pid={leader_pid}{expected}, leader={})", + "kill: {errno} (signal={signal:?}, target={} via {}, leader_pid={leader_pid}{expected}{measured}, leader={})", target.pid.as_raw(), - target.source.as_str(), + target.source.render(), leader.render(), ) } @@ -950,6 +1134,7 @@ impl ProcessSupervisor { reap_ledger: HashMap::new(), group_term_grace: GROUP_TERM_GRACE, forced_kill_errno: None, + forced_pty_lookup: None, } } @@ -963,6 +1148,21 @@ impl ProcessSupervisor { self.forced_kill_errno = Some(errno); } + /// Test seam for the PTY foreground-group lookup, on the same terms + /// as [`Self::force_next_kill_errno`] and for the same reason. + /// + /// The three non-success arms — no master fd, a failed duplicate, a + /// failed `tcgetpgrp` — cannot be produced on demand from a healthy + /// PTY: they need an exhausted descriptor table or a master that has + /// stopped being a terminal. Injecting only the *lookup result* + /// leaves the branch itself, the fallback target choice, the leader + /// observation against the real child, and the report construction + /// all running as production code. Consumed by one call. + #[cfg(test)] + fn force_next_pty_lookup_failure(&mut self, failure: PtyLookupFailure) { + self.forced_pty_lookup = Some(failure); + } + /// Override the SIGTERM-to-SIGKILL grace window. Test helper. pub fn set_grace_period(&mut self, d: Duration) { self.grace_period = d; @@ -1080,7 +1280,8 @@ impl ProcessSupervisor { else { return Err(format!("process {id} is not running")); }; - let target = signal_target(proc, pid)?; + let forced_lookup = self.forced_pty_lookup.take(); + let target = signal_target(proc, pid, forced_lookup)?; // Q#PD4: the seam injects the KILL attempt's result only — // never the observation below — so target selection, the real // `ChildHandle::try_wait` against the real child, and the error @@ -1094,7 +1295,7 @@ impl ProcessSupervisor { // disposition is unchanged — this still returns `Err`, // with no state transition and no ledger arming. let leader = observe_leader(proc); - return Err(signal_failure_report(target, pid, errno, &leader)); + return Err(signal_failure_report(target, pid, signal, errno, &leader)); } if matches!(signal, Signal::SIGTERM | Signal::SIGKILL | Signal::SIGHUP) { proc.state = ProcessState::Exiting { @@ -1243,9 +1444,24 @@ impl ProcessSupervisor { let now = Instant::now(); self.reap_ledger.retain(|pgid, entry| { // ESRCH: no such group — done. Any other probe error is - // also treated as "nothing left we can reach" (EPERM - // cannot happen for our own children) so the ledger - // cannot grow without bound. + // also treated as "nothing left we can reach", so the + // ledger cannot grow without bound. + // + // **That is a bounded-growth policy, not a claim that the + // group is gone.** This comment previously justified it with + // "EPERM cannot happen for our own children". That reasoning + // does not hold: the probe targets a *group*, and owning the + // spawned child says nothing about a group unless the child + // is still a member of it — which nothing here measures. A + // group-directed EPERM against a live leader has since been + // observed in CI (macOS, PR #191, run 30553376486), via an + // explicit signal rather than this probe. + // + // So this arm can silently cancel an escalation, and the + // `SIGKILL` below can fail while the entry is marked killed. + // Both are known and deliberately unchanged here: the + // diagnostic lane that found them does not alter + // disposition. Fixing it is its own lane. if nix::sys::signal::kill(Pid::from_raw(-*pgid), None).is_err() { return false; } @@ -2388,6 +2604,20 @@ mod tests { /// the availability guard and the spawn cannot drift apart. const BASH: &str = "/bin/bash"; + /// A plain PTY child, for tests that care about the PTY *branch* + /// rather than about job control. + 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 { + rows: 24, + cols: 80, + mode: TerminalMode::Canonical, + }; + let id = sup.spawn(spec).expect("spawn"); + (id, spawn_started_pid(sup, id)) + } + /// The tty's current foreground process group, read through the same /// `MasterPty` accessor production uses. `None` for a pipe /// generation, or when the terminal reports no foreground group. @@ -2524,7 +2754,7 @@ mod tests { let err = sup.terminate(id).expect_err("injected EPERM must fail"); let expected = format!( - "kill: {} (target=-{fg} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=live)", + "kill: {} (signal=SIGTERM, target=-{fg} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=live)", nix::errno::Errno::EPERM ); assert_eq!( @@ -2549,6 +2779,247 @@ mod tests { let _ = sup.signal(id, Signal::SIGKILL); } + /// Q#DC2 / acceptance 2 — a PTY whose foreground-group lookup fails + /// is distinguishable from a pipe child that never had a terminal. + /// + /// Before this, both rendered "leader-pid". The PTY fallback was + /// therefore invisible: a terminal query that failed, and a process + /// with no terminal at all, produced the same word. Each arm now + /// names its own stage, and `portable-pty`'s + /// `process_group_leader` — which collapses every failure into + /// `None` before pmacs can see it — is bypassed so the errno + /// survives. + #[test] + fn a_pty_foreground_lookup_failure_names_its_stage() { + let arms = [ + (PtyLookupFailure::NoMasterFd, "no-master-fd".to_owned()), + ( + PtyLookupFailure::Duplicate(nix::errno::Errno::EMFILE), + format!("duplicate-master-fd: {}", nix::errno::Errno::EMFILE), + ), + ( + PtyLookupFailure::Query(nix::errno::Errno::ENOTTY), + format!("tcgetpgrp: {}", nix::errno::Errno::ENOTTY), + ), + ( + PtyLookupFailure::NonPositive(0), + "tcgetpgrp-non-positive: 0".to_owned(), + ), + ]; + + for (failure, rendered) in arms { + let mut sup = ProcessSupervisor::new(); + let (id, pid) = spawn_live_pty(&mut sup, "diag-pty-fallback"); + + sup.force_next_pty_lookup_failure(failure); + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + // The target falls back to the leader — positive, not a + // negated group — and the source says why. + let expected = format!( + "kill: {} (signal=SIGTERM, target={pid} via pty-leader-fallback({rendered}), leader_pid={pid}, leader=live)", + nix::errno::Errno::EPERM + ); + assert_eq!(err, expected, "arm {failure:?} must name its own stage"); + + // And it must NOT read like a pipe child. + assert!( + !err.contains("via leader-pid,"), + "a PTY fallback must not render as a bare pipe leader target: {err}" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + } + + /// The companion half of acceptance 2: a genuine pipe child still + /// renders "leader-pid", so the two really are distinct strings + /// rather than both having moved. + /// + /// Asserted here as well as in the leader-directed test because a + /// rename of one side would otherwise pass every test — the pair is + /// the point, not either string alone. + #[test] + fn a_pipe_child_still_renders_a_bare_leader_target() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-pipe-leader", "/bin/sleep"); + spec.args = vec!["30".into()]; + let id = sup.spawn(spec).expect("spawn"); + let pid = spawn_started_pid(&mut sup, id); + + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + assert!( + err.contains(&format!("target={pid} via leader-pid,")), + "a pipe child with no group renders the bare leader source: {err}" + ); + assert!( + !err.contains("pty-leader-fallback"), + "a pipe child never took the PTY branch: {err}" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#DC3 / acceptance 3(a) — the report names the signal, so two + /// failures that differ only in which signal was sent are no longer + /// the same text. + /// + /// **They differ in text only.** Every failed `kill` returns before + /// the fatal-signal branch, so both leave the state and the ledger + /// exactly as they were. That is asserted here rather than assumed, + /// because revision 2 of the framing claimed the opposite. + #[test] + fn a_failed_signal_names_which_signal_and_changes_nothing() { + let mut reports = Vec::new(); + for signal in [Signal::SIGTERM, Signal::SIGUSR1] { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-signal-name", "/bin/sh"); + spec.args = vec!["-c".into(), "sleep 30".into()]; + spec.group = true; + let id = sup.spawn(spec).expect("spawn"); + let pid = spawn_started_pid(&mut sup, id); + + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup + .signal(id, signal) + .expect_err("injected EPERM must fail"); + + assert!( + err.contains(&format!("signal={signal:?},")), + "the report must name {signal:?}: {err}" + ); + assert!( + matches!( + sup.processes.get(&id).expect("record").state, + ProcessState::Running { .. } + ), + "a failed {signal:?} must not transition the record" + ); + assert!( + sup.reap_ledger.is_empty(), + "a failed {signal:?} must not arm the ledger" + ); + + reports.push(err.replace(&format!("{pid}"), "")); + let _ = nix::sys::signal::kill( + Pid::from_raw(-i32::try_from(pid).unwrap()), + Signal::SIGKILL, + ); + } + + assert_ne!( + reports[0], reports[1], + "SIGTERM and SIGUSR1 failures must no longer be identical text" + ); + } + + /// Q#DC3 / acceptance 3(b) — the disposition control. A *successful* + /// non-fatal signal changes nothing, while a *successful* fatal one + /// transitions the record and arms the ledger. + /// + /// This is the check that gives the previous test its meaning: it + /// shows the fatal/non-fatal distinction is real, and therefore that + /// "failed signals are disposition-identical" is a statement about + /// the failure path rather than about signals generally. + #[test] + fn a_successful_signal_disposition_depends_on_whether_it_is_fatal() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-disposition-live", "/bin/sh"); + // Ignore USR1 so the successful non-fatal signal cannot end the + // child and confuse the state assertion with a real exit. + spec.args = vec!["-c".into(), "trap '' USR1; sleep 30".into()]; + spec.group = true; + let id = sup.spawn(spec).expect("spawn"); + let pid = spawn_started_pid(&mut sup, id); + + sup.signal(id, Signal::SIGUSR1).expect("USR1 delivers"); + assert!( + matches!( + sup.processes.get(&id).expect("record").state, + ProcessState::Running { .. } + ), + "a successful non-fatal signal leaves the record Running" + ); + assert!( + sup.reap_ledger.is_empty(), + "a successful non-fatal signal arms no ledger entry" + ); + + sup.terminate(id).expect("TERM delivers"); + assert!( + matches!( + sup.processes.get(&id).expect("record").state, + ProcessState::Exiting { .. } + ), + "a successful fatal signal transitions the record to Exiting" + ); + assert!( + !sup.reap_ledger.is_empty(), + "a successful fatal signal arms the group reap ledger" + ); + + let _ = + nix::sys::signal::kill(Pid::from_raw(-i32::try_from(pid).unwrap()), Signal::SIGKILL); + } + + /// Q#DC4 / acceptance 4 — the measured group is a real observation, + /// not a restatement of the input. + /// + /// `expected_group` is `-leader_pid` by construction, so on the + /// spawn-group path it can never disagree with the target. The + /// measured field is the only one that can, and this proves it does: + /// a child placed into an *anchor* group reports that group, not its + /// own pid. + /// + /// Without this the field would be exactly the vacuous readout the + /// framing was written to eliminate — an implementation returning + /// `-pid` unconditionally would satisfy every other test. + #[test] + fn the_measured_group_reports_the_real_group_not_the_pid() { + use std::os::unix::process::CommandExt as _; + + // An anchor process leading its own group. + let mut anchor = std::process::Command::new("/bin/sleep"); + anchor.arg("30"); + anchor.process_group(0); + let mut anchor = anchor.spawn().expect("spawn anchor"); + let anchor_pgid = i32::try_from(anchor.id()).expect("pid fits i32"); + + // A second process placed INTO the anchor's group, so its pgid + // is genuinely not its own pid. + let mut joiner = std::process::Command::new("/bin/sleep"); + joiner.arg("30"); + joiner.process_group(anchor_pgid); + let mut joiner = joiner.spawn().expect("spawn joiner"); + let joiner_pid = joiner.id(); + + assert_ne!( + i32::try_from(joiner_pid).unwrap(), + anchor_pgid, + "precondition: the joiner must not be the anchor itself" + ); + + let rendered = measured_group_of(joiner_pid); + assert_eq!( + rendered, + format!(", measured_group=-{anchor_pgid}"), + "the measurement must report the group the kernel actually has" + ); + assert_ne!( + rendered, + format!(", measured_group=-{joiner_pid}"), + "and must NOT restate the pid it was given" + ); + + let _ = joiner.kill(); + let _ = joiner.wait(); + let _ = anchor.kill(); + let _ = anchor.wait(); + } + /// Q#PD1 acceptance 2 — a leader-directed failure records the /// fallback branch and a positive target, and omits the group field /// that would be meaningless for it. Exact message again. @@ -2564,7 +3035,7 @@ mod tests { let err = sup.terminate(id).expect_err("injected ESRCH must fail"); let expected = format!( - "kill: {} (target={pid} via leader-pid, leader_pid={pid}, leader=live)", + "kill: {} (signal=SIGTERM, target={pid} via leader-pid, leader_pid={pid}, leader=live)", nix::errno::Errno::ESRCH ); assert_eq!( @@ -2614,7 +3085,7 @@ mod tests { let err = terminate_until_leader_exited(&mut sup, id, Duration::from_secs(10)); let expected = format!( - "kill: {} (target={pid} via leader-pid, leader_pid={pid}, leader=exited(code 3))", + "kill: {} (signal=SIGTERM, target={pid} via leader-pid, leader_pid={pid}, leader=exited(code 3))", nix::errno::Errno::EPERM ); assert_eq!( @@ -2646,7 +3117,7 @@ mod tests { let err = sup.terminate(id).expect_err("injected EPERM must fail"); let expected = format!( - "kill: {} (target=-{pid} via group, leader_pid={pid}, expected_group=-{pid}, leader=live)", + "kill: {} (signal=SIGTERM, target=-{pid} via group, leader_pid={pid}, expected_group=-{pid}, measured_group=-{pid}, leader=live)", nix::errno::Errno::EPERM ); assert_eq!(err, expected, "a group=true pipe child reports via group");