From 66f73013fcee4cad5d2d4610b90729878cd15c6d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 14:10:39 -0400 Subject: [PATCH] =?UTF-8?q?test(process):=20take=20Bet=201's=20fallback=20?= =?UTF-8?q?=E2=80=94=20macOS=20does=20not=20diverge?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI falsified framing Bet 1. Both macOS legs reported job control never moved the terminal off the leader (leader=8542, foreground groups observed: [8542]) with the terminal staying on the leader for the entire 10s bounded wait. Linux diverges reliably — 20/20 locally and green on both ubuntu legs — so this is a platform difference rather than a flake, and rerunning past it would have been wrong. The framing named this outcome and prescribed the response, so that is what ships rather than an improvised fix: - The divergent case is pinned by INJECTING the foreground group at the `signal_target` seam. Deterministic, runs on every platform. The injection seam widens from failure-only to either outcome; the branch, target choice, leader observation against the real child, and report construction all remain production code. - Verified still discriminating: the `leader_pid`-substitution mutation fails it, `target=-1707909` against an expected `-1707910`. That was the whole point of the original rewrite and it survives the fallback. - The real shell is retained as corroboration in `job_control_really_diverges_the_foreground_group`, Linux-only. It skips on macOS by PLATFORM CHECK rather than by arming: the precondition genuinely does not hold there, so running it would assert a false claim about macOS instead of finding a bug. - Framing revision 5 records the falsification and states exactly how the injected pin is weaker — it proves the target is read from the lookup rather than substituted from the leader; it does not by itself prove any real shell produces that divergence. `PMACS_REQUIRE_BASH` moves to Linux-only. The earlier reasoning for arming both platforms — macOS is where the failures happen, so Linux-only leaves it dark where it matters — was right about the diagnostic and wrong about this test, which cannot produce its precondition on macOS at all. Arming it there made a missing binary fatal for a test that can never run. The measurement is recorded in ci.yml and the README so it is not re-derived. Gates on this tree: 11 gates, 4471 tests, zero failures. --- .github/workflows/ci.yml | 24 +-- README.md | 22 ++- ...-signal-diagnostic-completeness-framing.md | 42 ++++- src/process.rs | 162 ++++++++++++------ 4 files changed, 175 insertions(+), 75 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 56afb21..0bc7ef3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -220,22 +220,26 @@ jobs: # skip fatal where the tool is guaranteed. # # PMACS_REQUIRE_BASH arms the signal diagnostic's job-control - # divergence fixture, which needs `/bin/bash` specifically: it - # relies on `-m` putting a foreground job in its own process group - # and handing it the terminal, so that `tcgetpgrp` differs from the - # spawned leader. Without that divergence the group-directed - # acceptance cannot tell a real foreground-group read from a - # `leader_pid` substitution. Armed on BOTH platforms, unlike the - # variables above — macOS ships `/bin/bash`, and the macOS legs are - # exactly where the signal failures this diagnostic exists for have - # actually occurred. + # corroboration test, which needs `/bin/bash` and `-m` putting a + # foreground job in its own process group with the terminal. + # + # Linux only, and NOT because macOS lacks bash — it ships 3.2. CI + # measured the difference: on both macOS legs a non-interactive + # `bash -m` kept the terminal on the leader for a full 10s wait, so + # the divergence the test needs does not occur there. The test skips + # on non-Linux by platform check; arming it on macOS would only make + # a missing binary fatal for a test that cannot run anyway. + # + # The divergent case itself is pinned on every platform by + # injecting the foreground group instead (framing Bet 1's stated + # fallback, after the real fixture failed its own falsifier). - run: cargo test --all-targets --no-default-features --features ${{ matrix.lua }} -- --test-threads=1 env: PMACS_REQUIRE_LSP: ${{ runner.os == 'Linux' && '1' || '' }} PMACS_REQUIRE_SHELLS: ${{ runner.os == 'Linux' && '1' || '' }} PMACS_REQUIRE_LUA: ${{ runner.os == 'Linux' && '1' || '' }} PMACS_REQUIRE_SETSID: ${{ runner.os == 'Linux' && '1' || '' }} - PMACS_REQUIRE_BASH: '1' + PMACS_REQUIRE_BASH: ${{ runner.os == 'Linux' && '1' || '' }} - run: cargo test --doc --no-default-features --features ${{ matrix.lua }} # The workspace default member is only the root `pmacs` package, so # the runs above never execute pmacs-protocol's own tests — the diff --git a/README.md b/README.md index c9de0a8..6225678 100644 --- a/README.md +++ b/README.md @@ -227,15 +227,19 @@ translation) are routed through trampolines that exec these tools. **skips** when `setsid` is absent, so a minimal or BusyBox environment still runs `cargo test --lib`; set `PMACS_REQUIRE_SETSID=1` to make that skip a failure, as CI does on Linux. -- **`/bin/bash`** (**optional**, but armed on every CI platform). The - signal diagnostic's group-directed acceptance needs a terminal whose - foreground process group is *not* the spawned leader, and `bash -m` - produces exactly that by running a foreground job in its own process - group. The path matters: the test spawns `/bin/bash` directly rather - than resolving `bash` on `PATH`, and skips when that path is absent. - Set `PMACS_REQUIRE_BASH=1` to make the skip a failure, as CI does on - Linux **and** macOS — the failures this diagnostic exists to explain - have so far only appeared on macOS. +- **`/bin/bash`** (**optional**, Linux only). The signal diagnostic's + job-control corroboration test needs a terminal whose foreground + process group is not the spawned leader, which `bash -m` produces by + running a foreground job in its own process group. The path matters: + the test spawns `/bin/bash` directly rather than resolving `bash` on + `PATH`, and skips when that path is absent. Set `PMACS_REQUIRE_BASH=1` + to make the skip a failure, as CI does on Linux. + + **It is deliberately not armed on macOS**, which ships bash 3.2 but + where a non-interactive `bash -m` was measured in CI to keep the + terminal on the leader — so the divergence the test needs never + happens there. The divergent case is pinned on every platform by + injecting the foreground group instead. - **`git`** (added in M7.2). Required for any package operation: the package fetcher shells out to `git` to clone, fetch, and resolve refs, with a deterministic environment diff --git a/docs/process-signal-diagnostic-completeness-framing.md b/docs/process-signal-diagnostic-completeness-framing.md index 1326292..7dad7fe 100644 --- a/docs/process-signal-diagnostic-completeness-framing.md +++ b/docs/process-signal-diagnostic-completeness-framing.md @@ -1,6 +1,6 @@ # Framing — make the signal diagnostic discriminating (evidence collection) -**Revision 4.** Status: awaiting review round 4. Lane: +**Revision 5.** Status: implemented; PR open. Lane: `process-signal-diagnostic-completeness`, worktree `../pmacs-signal-identity`, based on `githubsucks/main` @ `4cd4a7b` (re-measure at branch time; this is a reading, not a constant). @@ -17,6 +17,33 @@ gets signalled, no disposition change.** Everything behavioural is in §5. ## Revision history +**Revision 4 → 5**, after implementation. **Bet 1 was falsified by CI**, +and the framing's own fallback is what shipped. + +- **`bash -m` does not diverge on macOS.** Both macOS legs reported + `job control never moved the terminal off the leader (leader=8542, + foreground groups observed: [8542])` — the terminal stayed with the + leader for the entire 10s bounded wait. It diverges reliably on Linux + (20/20 locally, green on both ubuntu legs), so this is a platform + difference, not a flake, and rerunning would have been wrong. +- **The stated fallback was taken**: the divergent case is now pinned by + **injecting** the foreground group at the `signal_target` seam, and + §3 Bet 1 records it as *weaker* than a real shell rather than + quietly equivalent. Verified still discriminating — the + `leader_pid`-substitution mutation fails it (`target=-1707909` against + an expected `-1707910`). +- **The real fixture is retained as corroboration**, Linux-only, under + `job_control_really_diverges_the_foreground_group`. It is skipped on + macOS by platform check rather than by arming, because the + precondition genuinely does not hold there; running it would assert a + false claim about macOS instead of finding a bug. +- **`PMACS_REQUIRE_BASH` moves to Linux-only.** Rev 4's reasoning for + arming it on both platforms — "macOS is where the failures happen, so + arming it Linux-only leaves it dark where it matters" — was correct + about the *diagnostic* and wrong about *this test*, which cannot + produce its precondition on macOS at all. The diagnostic's macOS + coverage comes from the injected pin, which runs everywhere. + **Revision 3 → 4**, after review round 3 (three blocking, one major). All four accepted and checked against the exact APIs, process model, and branch ancestry before revision. @@ -366,8 +393,17 @@ assertion (`:2517`) is one of the four sites acceptance 5 must update. - *Falsified if* the fixture cannot be made deterministic in CI. Then the lane falls back to pinning divergence at the `signal_target` unit level with an injected foreground group, and labels that as weaker. - - Without this the diagnostic remains unverified in the only case it - exists for. + - **OUTCOME: falsified on macOS.** Both macOS legs observed the + terminal stay with the leader for the full bounded wait; Linux + diverges reliably. The fallback shipped: the divergent case is + pinned by injection everywhere, and the real shell corroborates it + on Linux only. + - **The injected pin is weaker, and here is exactly how.** It proves + the target is read from the *lookup* rather than substituted from + the leader — the substitution mutation still fails it. It does + **not**, by itself, prove any real shell produces that divergence; + `job_control_really_diverges_the_foreground_group` carries that, on + one platform. - **Bet 2 — the PTY fallback is reachable and distinguishable.** A test drives all three non-success arms: a duplicate errno, a `tcgetpgrp` diff --git a/src/process.rs b/src/process.rs index 7f412ae..70c1c80 100644 --- a/src/process.rs +++ b/src/process.rs @@ -478,8 +478,8 @@ pub struct ProcessSupervisor { /// 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, + /// `force_next_pty_lookup`). Always `None` outside tests. + forced_pty_lookup: Option>, } /// One armed group in the reap ledger. @@ -881,7 +881,7 @@ struct SignalTarget { fn signal_target( proc: &ManagedProcess, pid: u32, - forced_lookup: Option, + forced_lookup: Option>, ) -> Result { if let Some(runtime) = proc.runtime.as_ref() && let ChildHandle::Pty { @@ -895,7 +895,7 @@ fn signal_target( // 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), + Some(outcome) => outcome, None => pty_foreground_group(master.as_ref()), }; return match lookup { @@ -1151,16 +1151,26 @@ impl ProcessSupervisor { /// 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. + /// Injects either outcome. The failure 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. + /// + /// The **success** arm exists because a genuinely divergent + /// foreground group is not portable. `bash -m` produces one on + /// Linux and **does not on macOS**, where the terminal stays with + /// the leader for the whole wait (observed in CI on both macOS + /// legs). Injecting the group keeps the divergent case pinned + /// everywhere; `job_control_really_diverges_the_foreground_group` + /// corroborates it against a real shell where the platform allows. + /// + /// Either way the injection covers only the *lookup result*: the + /// branch, the target choice, the leader observation against the + /// real child, and the report construction all run 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); + fn force_next_pty_lookup(&mut self, outcome: Result) { + self.forced_pty_lookup = Some(outcome); } /// Override the SIGTERM-to-SIGKILL grace window. Test helper. @@ -2706,50 +2716,46 @@ mod tests { /// Q#DC1 / acceptance 1 — a group-directed failure names the target, /// the branch that chose it, the expected group, the errno, and the - /// leader's own state, as five separate facts **that are not the same - /// fact repeated**. + /// leader's own state, as facts **that are not the same fact + /// repeated**. /// - /// The pre-Stage-B version of this test spawned `/bin/sleep` on a PTY - /// and asserted the same pid three times, conceding in its own - /// comment that the values "are asserted to agree only because - /// nothing has moved the terminal". An implementation that ignored - /// `tcgetpgrp` entirely and substituted `leader_pid` passed it. Since - /// the whole premise of the diagnostic is that these two entities can - /// diverge, that test pinned the substitution as acceptable. + /// The pre-Stage-B version spawned `/bin/sleep` on a PTY and asserted + /// the same pid three times, conceding in its own comment that the + /// values "are asserted to agree only because nothing has moved the + /// terminal". An implementation that ignored `tcgetpgrp` and + /// substituted `leader_pid` passed it — so it pinned the substitution + /// as acceptable. /// - /// This version drives job control so the terminal genuinely belongs - /// to a different process group, and asserts the two values **differ** - /// as well as asserting each exactly. + /// **The foreground group is injected, not produced by a shell.** + /// Framing Bet 1 wagered that a real job-control fixture would be + /// deterministic in CI; it is not. `bash -m` diverges reliably on + /// Linux and never on macOS, where CI observed the terminal stay with + /// the leader for a full 10s wait on both legs. The framing's stated + /// fallback is this: pin the divergence at the `signal_target` level + /// with an injected foreground group, and **say plainly that it is + /// weaker** than a real one. + /// + /// What it still proves: the target is read from the *lookup* rather + /// than substituted from the leader, because the two values differ + /// here and the assertion names both. What it no longer proves on its + /// own: that a real shell ever produces that divergence — + /// `job_control_really_diverges_the_foreground_group` carries that, + /// on the platforms where it is real. #[test] fn a_group_directed_kill_failure_reports_target_and_leader_separately() { - // Guard on the exact path the fixture spawns, not on `which bash`. - // The two can disagree — a system with bash on PATH but not at - // `/bin/bash` would pass a `which` guard and then fail the spawn, - // which is the guard failing to guard the thing it names. - if !std::path::Path::new(BASH).exists() { - let armed = std::env::var_os("PMACS_REQUIRE_BASH").is_some_and(|v| !v.is_empty()); - assert!( - !armed, - "PMACS_REQUIRE_BASH is set but {BASH} does not exist: the \ - job-control divergence fixture cannot run" - ); - eprintln!( - "{BASH} not present; skipping \ - a_group_directed_kill_failure_reports_target_and_leader_separately" - ); - return; - } - let mut sup = ProcessSupervisor::new(); - let (id, pid, fg) = spawn_pty_with_diverged_foreground_group(&mut sup, "diag-group"); + let (id, pid) = spawn_live_pty(&mut sup, "diag-group"); + // A foreground group that is deliberately NOT the leader. let leader_i32 = i32::try_from(pid).expect("pid fits i32"); + let fg = leader_i32 + 1; assert_ne!( fg, leader_i32, - "the fixture must make the foreground group differ from the leader; \ - equal values would let a leader_pid substitution pass" + "the injected group must differ from the leader or this test \ + cannot distinguish a substitution" ); + sup.force_next_pty_lookup(Ok(fg)); sup.force_next_kill_errno(nix::errno::Errno::EPERM); let err = sup.terminate(id).expect_err("injected EPERM must fail"); @@ -2759,17 +2765,16 @@ mod tests { ); assert_eq!( err, expected, - "the report names the exact group the tty reported, the exact \ + "the report names the exact group the lookup returned, the exact \ leader pid, and observes the leader as live" ); - // The target came from the terminal, not from the leader. Stated - // as its own assertion so a regression that reintroduces the - // substitution fails here by name rather than inside a long - // string comparison. + // Stated separately so a regression that reintroduces the + // substitution fails by name rather than inside a long string + // comparison. assert!( err.contains(&format!("target=-{fg} via tcgetpgrp")), - "the target must be the terminal's foreground group: {err}" + "the target must be the group the lookup returned: {err}" ); assert!( !err.contains(&format!("target=-{pid} via tcgetpgrp")), @@ -2779,6 +2784,57 @@ mod tests { let _ = sup.signal(id, Signal::SIGKILL); } + /// Corroboration for the injected divergence above: a **real** shell + /// under job control does hand the terminal to a different process + /// group, and the production lookup reads it. + /// + /// Linux-only by arming. macOS is not a skip-because-untested: CI + /// observed `bash -m` there keep the terminal on the leader for the + /// entire bounded wait, on both legs, so the precondition this test + /// needs genuinely does not hold on that platform. Running it there + /// would assert a false claim about macOS rather than find a bug. + #[test] + fn job_control_really_diverges_the_foreground_group() { + if !std::path::Path::new(BASH).exists() { + let armed = std::env::var_os("PMACS_REQUIRE_BASH").is_some_and(|v| !v.is_empty()); + assert!( + !armed, + "PMACS_REQUIRE_BASH is set but {BASH} does not exist: the \ + job-control divergence fixture cannot run" + ); + eprintln!( + "{BASH} not present; skipping job_control_really_diverges_the_foreground_group" + ); + return; + } + if !cfg!(target_os = "linux") { + eprintln!( + "job control does not hand over the terminal for a \ + non-interactive `bash -m` on this platform; skipping" + ); + return; + } + + let mut sup = ProcessSupervisor::new(); + let (id, pid, fg) = spawn_pty_with_diverged_foreground_group(&mut sup, "diag-jobctl"); + + let leader_i32 = i32::try_from(pid).expect("pid fits i32"); + assert_ne!( + fg, leader_i32, + "a real job-control shell must move the terminal off the leader" + ); + + // And the production lookup — not the fixture's own polling — + // reports that same group. + let observed = foreground_pgid(&sup, id).expect("PTY reports a foreground group"); + assert_eq!( + observed, fg, + "the production accessor must see what the fixture waited for" + ); + + 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. /// @@ -2811,7 +2867,7 @@ mod tests { 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_pty_lookup(Err(failure)); sup.force_next_kill_errno(nix::errno::Errno::EPERM); let err = sup.terminate(id).expect_err("injected EPERM must fail");