diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c9a9983..0bc7ef3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -218,12 +218,28 @@ jobs: # binary is absent (a minimal container must not fail `--lib` # without ever testing pmacs) and this variable is what makes the # skip fatal where the tool is guaranteed. + # + # PMACS_REQUIRE_BASH arms the signal diagnostic's job-control + # 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: ${{ 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/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/README.md b/README.md index cd91a87..6225678 100644 --- a/README.md +++ b/README.md @@ -227,6 +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**, 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/active-work.md b/docs/active-work.md index af59d0b..28be8ca 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -118,6 +118,69 @@ declares canonical will pass on a tree the rest of this file does not describe. If it does not, stop and repair the remote/fetch configuration. +## Process-signal diagnostic completeness — IMPLEMENTED, PR OPEN + +- **Portable branch:** `githubsucks/process-signal-diagnostic-completeness`; + worktree `../pmacs-signal-identity`. Governing document: + `docs/process-signal-diagnostic-completeness-framing.md`, **revision 6**; + the contract was approved at revision 4 and has been revised twice + since to match what shipped. +- **State:** **PR #200 open**, four review rounds closed, held for + review. Canonical `githubsucks/main` @ `b8e18f6` (the ledger + absorption #199) is integrated. Four commits: Bet 1 alone, Bets 2–4, + Bet 1's fallback after CI falsified it, then round 4's corrections. +- **Boundary, unchanged:** evidence collection only. No signal + retargeting, tolerance, disposition change, or reap-ledger repair. + **Group identity remains unprovable** (framing §1.5): the measured + group is read in the same read-then-act window, and no portable + mechanism closes that for a *group* — `pidfd` covers a process, and + macOS has neither. The ledger's silent cancellation is parked as its + own lane. +- **What shipped.** The PTY foreground-group lookup now reports four + distinct outcomes instead of one string, through a safe + `filedescriptor::OwnedHandle::dup` bridge that keeps the errno + `portable-pty` discards — with no `unsafe` in pmacs. The report names + the signal. A `measured_group` field from `getpgid` is the only field + in the report able to disagree with its input. The reap-ledger comment + claiming "EPERM cannot happen for our own children" is corrected + **without** claiming the child itself received EPERM. +- **Verification at the merged tree** (not inherited from the pre-merge + head): 11 gates, **4471 tests, zero failures** — fmt, diff-check, + clippy, `--lib`, `--lib --features crdt`, compile-mode, copy-mode in + both feature configurations, bottom-panel Stage 1, M4 with the + basedpyright skip, and required GPU. The job-control divergence + fixture was run 20/20. All local runs used an isolated + `XDG_CONFIG_HOME`. +- **Four 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; and — as a positive control on the fixture itself — + replacing the `bash -m` job-control child with a plain `sleep`. + Separately, the **pre-Stage-B test was restored verbatim under the + substitution mutation and PASSED**, which is the finding that + justified rewriting it rather than adding to it. +- **Bet 1 was falsified by CI and the framing's fallback shipped.** + `bash -m` diverges on Linux and **never on macOS**, where both legs + observed the terminal stay with the leader for a full 10s wait. The + divergent case is now pinned by **injecting** the foreground group + (runs everywhere, weaker); a Linux-only corroboration drives a real + shell and is the **only** test exercising `pty_foreground_group` + end-to-end. `/bin/bash` is a declared optional test dependency armed + by `PMACS_REQUIRE_BASH` on **Linux only** — macOS ships bash but + cannot produce the precondition, so arming it there would make a + missing binary fatal for a test that can never run. +- **Recovery from a clean checkout:** + + ```sh + git fetch githubsucks --prune + git worktree add ../pmacs-signal-identity \ + -b process-signal-diagnostic-completeness \ + githubsucks/process-signal-diagnostic-completeness + cd ../pmacs-signal-identity + git merge-base --is-ancestor b8e18f6 HEAD + git status --short --branch + ``` + ## The CRDT half of the test corpus is dark in CI — NEEDS A LANE - **No branch, no framing yet.** Found while gating #166, then measured diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index dd465b2..b4629f5 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -232,6 +232,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/docs/process-signal-diagnostic-completeness-framing.md b/docs/process-signal-diagnostic-completeness-framing.md new file mode 100644 index 0000000..b945da7 --- /dev/null +++ b/docs/process-signal-diagnostic-completeness-framing.md @@ -0,0 +1,563 @@ +# Framing — make the signal diagnostic discriminating (evidence collection) + +**Revision 6.** Status: implemented; PR open, review round 4 closed. 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). + +**This is Stage B of the lane whose Stage A merged as PR #176** +(`docs/process-signal-tolerance-framing.md`, revision 4). Stage A made a +failing `kill` self-describing and parked every tolerance rule behind +evidence. Evidence arrived (§1.2). It supports none of the parked rules, +no identity claim, and — per review round 2 — no claim that today's +escalation path is safe. + +**Evidence collection only. No tolerance rule, no change to which process +gets signalled, no disposition change.** Everything behavioural is in §5. + +## Revision history + +**Revision 5 → 6**, after review round 4 (three blocking, one major). +All four accepted. + +- **`measured_group` was sampled AFTER the failed `kill`** and after + `observe_leader`, while both the framing and the function's own doc + said before. A concurrent group change would have made the diagnostic + report post-failure state as evidence about the attempted target. It + is now sampled in `signal` before the kill and passed into the report. +- **The Linux corroboration did not exercise the production lookup.** + Its helper read `portable_pty::process_group_leader` — the accessor + this lane stopped using — so `pty_foreground_group` could have fallen + back on every call with every test still green. Demonstrated: forcing + it to always fall back leaves the injected pin **passing** and only + the corroboration failing. The helper now calls the production lookup, + and the corroboration forces *only* the kill so the report is built + from a real terminal read. +- **This document did not update its own acceptance contract** (§4.1). + Revision 5 recorded the falsification in the revision history and Bet + 1 but left the normative criterion demanding the real-shell rewrite — + the exact "implementation quietly diverges from the contract" shape + this project recorded as a lesson on #191/#188. +- **`TargetSource`'s doc had the wrong classification.** Two of four + variants now target the leader pid, not one, and the pid-versus-group + split does not line up with PTY-versus-pipe — which is *why* + `PtyForegroundFallback` needed its own variant. + +**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. + +- **Rev 3's "no safe fd bridge" conclusion was still too absolute** + (§1.6). `filedescriptor::OwnedHandle::dup` accepts an `AsRawFd` through + its safe `AsRawFileDescriptor` blanket implementation and returns an + owned value implementing `AsFd`. A lifetime-tied wrapper around + `MasterPty::as_raw_fd` therefore bridges to + `nix::unistd::tcgetpgrp` with no `unsafe` in pmacs. The crate is + already resolved through `portable-pty`; this lane declares it + directly and restores errno capture. +- **The occurrence did not prove "our own child, alive, EPERM"** (§1.3). + The failed target was a *group* and `try_wait` observed the leader + process. No measurement established that the leader still belonged to + that group. What is invalidated is using ownership of the spawned + child to dismiss an arbitrary group-target error. +- **Bet 1 called a terminal-owning job "background"** (§3). A background + group is, by definition, not the terminal's foreground group. The + fixture now names `/bin/bash`, launches a foreground job in its own + group, and waits for the actual terminal handoff before measuring. +- **The branch-base line described the scout, not the ancestry.** Rev 3's + merge-base with `4cd4a7b` was still `391d38a`. Canonical main is now + integrated, and the lane is recorded in `docs/active-work.md`. + +**Revision 2 → 3**, after review round 2 (three blocking, three major). +All six accepted; all six verified in the code before acceptance. + +- **Rev 3 concluded that the PTY errno proposal had no safe fd bridge.** + It therefore withdrew and reduced rev 2's central new proposal (§1.6). + Revision 4 supersedes that conclusion after checking + `filedescriptor`'s safe duplication API. +- **Rev 2 said `getpgid` was "ungated". It is not** (§1.5a). The claim + came from reading the four lines above the function; the gate is a + block-level `feature!` opened 168 lines earlier. Same error shape as + the truncated-output trap already in the handoff, committed inside a + document about non-discriminating evidence. +- **Bet 4's `setsid` fixture was impossible** (§3, Bet 4). A `spec.group` + child is already a process-group leader, and a group leader's `setsid` + fails with EPERM. +- **"Recoverable" was unsupported** (§1.8). The ledger drops its entry on + *any* probe error — including an EPERM that ownership of the recorded + child cannot rule out for a group target — and discards the `SIGKILL` + result while marking the entry killed. +- **Rev 2 falsified the wrong Stage A sentence** (§1.3). Stage A's + disjointness claim was about the **PTY** path and remains true. +- **Rev 2's signal-disposition argument was wrong** (§1.7). Failed + signals are all disposition-identical. +- **Bet 5 proposed a test that already exists** (§1.9). Cited as ground + truth now, not invented. + +**Revision 1 → 2**, after review round 1 (two blocking, two major); all +accepted. Rev 1 proposed *retargeting* to a measured pgid — a behavioural +change resting on an identity claim a number cannot support; it asserted +`getpgid(child) == pid`, which an implementation ignoring `getpgid` would +satisfy; it scoped the PTY path out; and it never noticed the report +omits the signal. + +**Rev 1 was written to a session scratchpad rather than a branch**, so it +was never on `githubsucks` and review round 1 necessarily landed on +Stage A's merged document. Recorded because "work is portable only after +it is committed and pushed" is a standing rule this lane broke on its +first step. + + +## 0. Coherence impact (COHERENCE §20) + +- **Journey step 8, "Open a terminal"** (§2), teardown half, plus every + compile/grep run through `spec.group`. **No grade change, no + behavioural change.** +- **Serves §9 (worker model), failure attribution.** Stage A made the + failure describe itself; this lane makes the description + *discriminating*, because several distinct failures render identically + today. +- **Interaction islands: none. Config registry: not adopted. + Background-work attribution: unchanged.** +- **No audited claim in COHERENCE.md changes**; under §25 no COHERENCE + edit rides this PR. + + +## 1. Ground truth (verified at `4cd4a7b`) + +### 1.1 Stage A landed and has now fired + +`signal_failure_report` and `LeaderObservation` merged as **PR #176 on +2026-07-26** (`62316a9`). §1.2 is the first failure carrying the new +format rather than a bare errno. Stage A is why this document can exist. + +### 1.2 The new occurrence, verbatim + +PR #191, `Test (macos-latest / lua54)`, +[run 30553376486](https://github.com/levineuwirth/pmacs/actions/runs/30553376486/job/90907461258), +`process::tests::repeated_terminate_does_not_extend_ledger_deadline`. +1873 passed, 1 failed. **A rerun of the identical head passed 12/12**, so +the failure is intermittent, not deterministic: + +``` +re-terminate: "kill: EPERM: Operation not permitted + (target=-8619 via group, leader_pid=8619, expected_group=-8619, leader=live)" +``` + +Established: the target source is `group` — the `spec.group` pipe path +(`signal_target` `:774-780`; `sh_group_spec` `:3402`) — **not** the PTY +path; and `leader=live`, from a real `try_wait` against the real child, +so the leader had neither exited nor been reaped. + +### 1.3 What the occurrence actually invalidates + +- **`src/process.rs:1246-1247` uses an invalid premise.** + `tick_reap_ledger` justifies treating any probe error as "nothing left + we can reach" with the comment "**EPERM cannot happen for our own + children**". But the operation is group-directed: ownership of the + spawned child says nothing unless that child is still a member of the + targeted group. +- **§1.2 does not prove EPERM was "for our own child".** The failed + target was group `-8619`; `leader=live` observed process `8619`. + Nothing measured `getpgid(8619)`, so the occurrence establishes only + that a group target computed from the spawn-time assumption returned + EPERM while the leader process was alive. That is enough to invalidate + the comment as a reason to discard arbitrary group errors, but not to + attribute the errno to the child. +- **Stage A §1.3 is *not* falsified.** It said the ledger is disjoint + from **the PTY path**, because the ledger arms only for + `proc.spec.group` and PTY mode cannot set it. That remains true. §1.2 + is the separate `spec.group` path. Rev 2 conflated "this path" with + "the signal path generally" and claimed a falsification it had not + made. + +Stage A's entity-split analysis concerns the PTY path, where the target +is read from `tcgetpgrp`. It does not apply to §1.2, where the target is +computed as `-leader_pid` with no read at all. + +### 1.4 The landed acceptance cannot discriminate + +`a_group_directed_kill_failure_reports_target_and_leader_separately` +(`:2400`) spawns a PTY child and asserts the exact string + +``` +target=-{pid} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=live +``` + +— the same `pid` three times. **An implementation that ignored +`tcgetpgrp` and substituted `leader_pid` would pass.** The test's own doc +comment concedes it: "here they are asserted to agree only because +nothing has moved the terminal." The premise of the diagnostic is that +these entities can diverge, and nothing exercises a case where they do. + +### 1.5 A numeric pgid cannot establish identity + +The value is read before `kill`, the window remains open, and a *number* +cannot distinguish the original group from a recycled one. + +**No portable mechanism closes this.** `pidfd_open` + `pidfd_send_signal` +close pid reuse for a single *process* on Linux; there is no +process-*group* equivalent, and macOS has no pidfd. The failures are +macOS-only, so nothing available makes group signalling identity-safe. + +**This lane therefore records and does not retarget.** No acceptance +claims the telemetry is sufficient. + +One narrowing fact, stated as narrowing and **not** as identity: POSIX +does not free a child's pid until the parent reaps it, and §1.2 observed +`leader=live` from a `try_wait` that had not reaped. While that pid is +held, no *new* group can be created bearing that pgid value. This makes +recycling an unlikely explanation **for that one occurrence**, and says +nothing about whether the group still held a signallable member — which +is what EPERM actually turns on. + +### 1.5a The nix surface is gated, and available for a non-obvious reason + +Both calls this lane would use live inside block-level gates: + +- `getpgid` (`nix-0.29.0/src/unistd.rs:335`) is inside + `feature! { #![feature = "process"] }` opened at `:167`. +- `tcgetpgrp` (`:368`) is inside + `feature! { #![all(feature = "process", feature = "term")] }` at `:360`. + +pmacs declares `nix` with `features = ["signal", "user", "fs", "term", +"socket", "poll"]` — **`process` is not listed**. It is enabled anyway +because **nix's own `signal` feature depends on `process`**, verified +with `cargo tree -e features -i nix:0.29.0`: + +``` +├── nix feature "process" +│ └── nix feature "signal" +│ └── pmacs v1.0.0 +``` + +Confirmed by compiling both calls against the real dependency graph. + +**This is stable but implicit.** The lane adds `process` to pmacs' own +feature list so the dependency is declared rather than inherited — a +one-line change that makes a real requirement visible. + +*Rev 2 asserted `getpgid` was "ungated", from reading the four lines +above it. The gate was 168 lines up. Recorded because it is the same +defect class this document exists to fix.* + +### 1.6 The PTY fallback is invisible; a safe owned-dup bridge preserves errno + +`signal_target` (`:757-785`): when the PTY branch's +`master.process_group_leader()` returns `None`, control falls through — +`spec.group` is rejected at spawn for PTY mode — and returns +`TargetSource::LeaderPid`, rendered "leader-pid" (`:738`). **A normal +pipe child renders identically.** Two situations, one string. + +`portable-pty` (`portable-pty-0.9.0/src/unix.rs:374`) discards the errno: + +```rust +fn process_group_leader(&self) -> Option { + match unsafe { libc::tcgetpgrp(self.fd.0.as_raw_fd()) } { + pid if pid > 0 => Some(pid), + _ => None, + } +} +``` + +`nix::unistd::tcgetpgrp` requires `F: AsFd`, while +`MasterPty` exposes only `fn as_raw_fd(&self) -> Option` +(`portable-pty-0.9.0/src/lib.rs:114`). Rev 3 inspected only the standard +library's raw-to-owned constructors and concluded every bridge required +`unsafe`. That missed the safe duplication abstraction already in the +dependency graph: + +- `filedescriptor::OwnedHandle::dup(&F)` is safe + (`filedescriptor-0.8.3/src/lib.rs:230`); +- on Unix, `filedescriptor` implements `AsRawFileDescriptor` for every + `T: AsRawFd` (`src/unix.rs:20`); +- `OwnedHandle` implements `AsFd` (`src/unix.rs:64`). + +A small wrapper holds a borrow of `MasterPty` for its lifetime and +implements the safe `AsRawFd` trait by returning the master's reported +fd. `OwnedHandle::dup` consumes that borrowed view immediately and +returns an independently owned duplicate; `tcgetpgrp(&owned)` then +preserves the `Errno`. **No raw-to-owned constructor and no `unsafe` +appears in pmacs.** `filedescriptor 0.8.3` is already in `Cargo.lock` +through `portable-pty`; this lane adds it as a direct dependency because +pmacs now calls its API. + +`OwnedHandle::dup` is itself fallible and preserves its Unix +`std::io::Error` source. That failure must not be collapsed into the +terminal query. The PTY result is therefore four-way and discriminating: + +- a positive pgid selects the foreground group, as today; +- a duplicate failure falls back to the leader and reports the + `duplicate-master-fd` stage plus its OS errno; +- a `tcgetpgrp` error falls back to the leader and reports the errno; +- absence of a master fd is a distinct unavailable source, not forged + into an errno. + +### 1.7 The report omits which signal failed + +`signal_failure_report` (`:830-850`) takes target, leader pid, errno and +leader observation — **not the signal**. `signal` (`:1074`) has it, and +the public Lua surface accepts INT, USR1, USR2 and QUIT besides the fatal +three (`src/lua_bindings/mod.rs:8627-8629`). A failed `SIGUSR1` and a +failed `SIGTERM` are today textually indistinguishable. + +**Rev 2 justified this by claiming their dispositions differ. That was +wrong.** `signal` returns `Err` at `:1092-1098`, *before* the fatal-signal +branch at `:1099`, so **every failed kill is disposition-identical** +regardless of signal. The disposition difference is real only for +**successful** calls. The reporting gap stands on its own: you cannot +tell which signal failed. Acceptance 3 separates the two. + +### 1.8 The disposition consequence, and why "recoverable" was wrong + +`signal` returns `Err` **before** the state transition and **before** +arming the ledger: + +| Which `terminate` hits EPERM | Consequence | +|---|---| +| A **later** one (§1.2's case) | Caller sees `Err`. State is already `Exiting` and a ledger entry **remains scheduled**. | +| The **first** one | State stays `Running`, ledger never armed. **No escalation is ever scheduled** — the child is abandoned. | + +**Rev 2 called the first row "recoverable" and claimed "SIGKILL +escalation still happens". Unsupported.** `tick_reap_ledger` +(`:1249-1254`): + +```rust +if nix::sys::signal::kill(Pid::from_raw(-*pgid), None).is_err() { + return false; // drops on ANY error, incl. EPERM +} +if now >= entry.deadline && !entry.killed { + let _ = nix::sys::signal::kill(..., Some(Signal::SIGKILL)); // result discarded + entry.killed = true; // marked killed regardless +} +``` + +If a ledger probe returns EPERM, it **drops the entry and cancels +escalation silently**; if its `SIGKILL` fails, the result is recorded as +if it succeeded. §1.2 did not observe either ledger call — it observed a +later explicit `SIGTERM` to the same assumed group number — so the +ledger failure is an exposed, still-unmeasured hazard rather than an +observed occurrence. The honest statement remains that escalation is +*scheduled*, not that it happens. + +**This still-silent path is parked, explicitly** (§5) rather than +absorbed: it is a second site with its own disposition questions, and +folding it in would repeat Stage A rev 3's error of implementing Stage B +inside Stage A. + +### 1.9 The first-call variant is already pinned + +`an_injected_failure_changes_no_state_and_arms_no_ledger` (`:2501`) +already spawns a `spec.group` child, injects EPERM on the **first** +`terminate`, and asserts `Running` plus an empty ledger. **Rev 2's Bet 5 +proposed inventing it.** It is ground truth, and its exact-string +assertion (`:2517`) is one of the four sites acceptance 5 must update. + +### 1.10 Limits of the evidence + +- **Not reproduced locally.** Development is Linux; failures are + macOS-only. No claim rests on a local repro of the EPERM. +- **Two occurrences, in different paths** — PR #172 was the PTY path + (`acc28`, luajit), §1.2 the group path (lua54). Not one flaky test. +- **The mechanism is not established**, and this lane does not propose + one. + + +## 2. Questions + +- **Q#DC1** — Can the two entities be made to diverge in a test? *Yes: + under a PTY, `/bin/bash` with job control enabled launches a + **foreground** job in its own process group and hands it the terminal, + so `tcgetpgrp` != leader pid.* +- **Q#DC2** — Should the PTY fallback get its own `TargetSource`? + *Proposed: yes. A failed duplicate or terminal lookup reports its stage + and errno; a missing master fd reports unavailable (§1.6).* +- **Q#DC3** — Should the report name the signal? *Proposed: yes, on the + reporting argument alone (§1.7).* +- **Q#DC4** — Should the measured pgid be reported for `spec.group` + children? *Proposed: yes, as an observation distinct from the assumed + value, with no sufficiency claim (§1.5).* +- **Q#DC5** — Retarget or tolerate anything? **No. Parked.** + + +## 3. Bets + +- **Bet 1 — the divergence is constructible.** A PTY fixture where the + foreground group is not the leader: `/bin/bash --noprofile --norc -m` + launches a foreground child in a fresh process group. The fixture + performs a bounded wait until `tcgetpgrp` itself reports the non-leader + group, asserts that group still has a live member as the positive + control, and only then injects the failing `kill`. The rewritten + acceptance asserts both exact values **and that they differ**. + - *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. + - **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` + errno, and a missing master fd. Each source is distinct from a pipe + child's and from the others. + - *Falsified if* the branch cannot be reached without faking the + lookup — in which case the seam is made injectable exactly as Stage A + made the kill injectable (Q#PD4), stated rather than hidden. + +- **Bet 3 — naming the signal is free.** Thread `signal` into the report. + - *Falsified if* any exact-string test cannot be updated mechanically. + +- **Bet 4 — a `spec.group` child's measured pgid can be made to differ + from its pid.** **Not via `setsid`:** `spec.group` sets + `process_group(0)` before exec, so the recorded child is already a + process-group leader, and a group leader's `setsid` fails with EPERM. + Forking a `setsid` helper does not help either — `getpgid(recorded_pid)` + still observes the wrapper. + The fixture instead has the recorded child **`setpgid` into another + existing group in the same session**, with a readiness handshake before + the measurement and explicit cleanup of the anchor group afterwards. + - *Falsified if* no such fixture is deterministic — in which case the + measurement is unfalsifiable and **does not ship**, per §1.4's lesson. + + +## 4. Acceptance + +1. The divergent case is pinned on **every** platform: a group-directed + failure whose target differs from the leader pid, with both exact + values asserted and asserted to differ. The pre-Stage-B test is + **rewritten**, not supplemented — it pinned a substitution as + acceptable. + + **The divergence is injected at the `signal_target` seam**, per Bet + 1's falsification: a real `bash -m` fixture diverges on Linux and + never on macOS. The injected form is weaker and §3 Bet 1 says how. + +1a. **A Linux-only corroboration** drives a real job-control shell, + forces *only* the kill failure, and asserts the production report + names the real foreground group. This is the sole test that exercises + `pty_foreground_group` end-to-end — every other test supplies the + group itself and therefore cannot detect a lookup that always falls + back. **Residual limitation, stated rather than buried: on macOS the + production lookup has no end-to-end coverage**, because the platform + cannot produce the precondition. +2. The PTY foreground-lookup fallback reports a source distinct from a + pipe child's. Separate tests drive the duplicate-error and + `tcgetpgrp`-error arms and assert the exact stage and errno; a third + drives the unavailable-fd arm. If one cannot be produced reliably + through a real PTY, the lookup result is injected while the branch, + target choice, real child observation, and report construction remain + production code (§3 Bet 2). +3. The report names the signal, split into two independent checks: + (a) a **failure-format** comparison showing `SIGUSR1` and `SIGTERM` + failures differ *in text only*, both leaving state and ledger + unchanged; and (b) a **successful-call disposition control** showing + a successful `SIGUSR1` does not transition state or arm the ledger + while a successful `SIGTERM` does. +4. For `spec.group` children the report carries the measured pgid as a + field distinct from the assumed one, renderable as unobservable, with + a test asserting a case where they **differ** (§3 Bet 4). **It is + sampled before the `kill`**, not during report construction, so it + records pre-kill evidence about the target that was attempted rather + than state left behind by the failure. + + **It does not describe the group at the moment the `kill` executed.** + `getpgid` and `kill` remain separated by the read-then-act window + §1.5 describes, so the sample can be stale by the time the signal is + delivered. Moving it earlier removes a *post-hoc* reading; it does + not make the reading contemporaneous, and no acceptance may claim it + does. +5. All four exact-string sites — `:2408`, `:2435`, `:2485`, `:2517` — + updated **individually**, each listed in the PR body with before and + after. No blanket rewrite: that is how a format regression hides. +6. `:2501`'s existing first-call pin is **retained and cited**, updated + only for the new format. +7. `process` added to pmacs' declared `nix` features, and + `filedescriptor 0.8` declared directly for the safe PTY-fd duplicate + (§1.5a, §1.6). +8. `docs/agent-handoff.md` records that ownership of the recorded child + cannot justify dismissing an error from a group target, with the run + link and §1.2's measurement limit; the comment at `:1246` is corrected + in the same PR without claiming that the child itself received EPERM. +9. **No acceptance claims the telemetry establishes group identity** + (§1.5), and none claims escalation is guaranteed (§1.8). The PR body + repeats both. + + +## 5. Parked + +- **The reap ledger's silent cancellation** (§1.8): if a probe returns + EPERM the entry is dropped, and a failed `SIGKILL` is marked as killed. + The explicit-signal occurrence exposes the premise but did not observe + either ledger call. **Its own lane** — disposition questions, second + site. +- **Retargeting to the measured pgid.** Behavioural; unsupported by §1.5. +- **Any tolerance rule for EPERM or ESRCH.** Unmotivated across Stage A's + three revisions and still unmotivated. +- **§1.8's first-call abandonment.** Pinned at `:2501`, not fixed here. +- **Q#PS6** — `terminate` on an already-reaped process returning `Ok`. +- **`signal_target`'s read-then-kill of `tcgetpgrp`** — Stage A's "most + likely real fix site, still unframed". This lane makes it *observable*, + not fixed. +- **`compile_mode_acceptance` reading the developer's real + `~/.config/pmacs/init.lua`** — separate defect, 11 local failures, + invisible in CI. + + +## 6. Gates + +Standard suite, each its own step with a real exit status and nothing +after the command that could mask it: `cargo fmt --check`; `cargo clippy +--workspace --all-targets -- -D warnings`; `cargo test --lib`; `cargo +test --lib --features crdt`; `compile_mode_acceptance`; +`terminal_copy_mode_acceptance` (both feature configurations); +`bottom_panel_stage1_acceptance`; `cargo test --test m4_acceptance -- +--skip basedpyright`; `PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu`; +`git diff --check`. + +**All local runs use an isolated `XDG_CONFIG_HOME`** — without it +`compile_mode_acceptance` fails 11 tests for unrelated reasons. + +**PTY job-control tests are load-sensitive.** Run repeatedly; the PR body +records the repetition count, not a single green. + + +## 7. Branch plan + +One branch, one PR. Bet 1 first and alone: it decides whether the +diagnostic is worth extending at all. If the divergence fixture cannot be +made deterministic, the lane is re-scoped rather than pushed through. diff --git a/src/process.rs b/src/process.rs index f0b851c..33fdf8f 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`). Always `None` outside tests. + forced_pty_lookup: Option>, } /// One armed group in the reap ledger. @@ -714,10 +717,18 @@ impl ChildHandle { /// Which branch of [`signal_target`] chose the target (Q#PD1). /// /// Recorded on failure because the branches differ in what a failing -/// `kill` can possibly mean: only [`Self::LeaderPid`] aims at the -/// spawned child itself. The other two aim at a *group*, which for a -/// PTY is read from the terminal and can belong to something the -/// supervisor never spawned. +/// `kill` can possibly mean. Two of the four aim at the spawned child +/// itself — [`Self::LeaderPid`] for a pipe child with no group, and +/// [`Self::PtyForegroundFallback`] for a PTY whose terminal named no +/// group. The other two aim at a *group*: +/// [`Self::SpawnGroup`] at one computed from the spawn-time `pgid == +/// pid` assumption, and [`Self::ForegroundGroup`] at one read from the +/// terminal, which can belong to something the supervisor never spawned. +/// +/// The pid-versus-group split is the classification that matters here, +/// and it does **not** line up with the PTY-versus-pipe split — which is +/// exactly why the fallback needed its own variant instead of reusing +/// `LeaderPid`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum TargetSource { /// The tty's current foreground process group, read at signal @@ -726,16 +737,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 +798,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 +886,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(outcome) => outcome, + 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,14 +974,48 @@ 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, + measured: Option<&str>, ) -> String { let expected = if target.source.is_group() { match i32::try_from(leader_pid) { @@ -841,10 +1025,15 @@ fn signal_failure_report( } else { String::new() }; + // Supplied by the caller, which samples it BEFORE the `kill`. Doing + // it here would describe the group as it stands *after* the failure + // and after `observe_leader`, which is post-hoc state presented as + // evidence about the attempted target. + let measured = measured.unwrap_or(""); 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 +1139,7 @@ impl ProcessSupervisor { reap_ledger: HashMap::new(), group_term_grace: GROUP_TERM_GRACE, forced_kill_errno: None, + forced_pty_lookup: None, } } @@ -963,6 +1153,31 @@ 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. + /// + /// 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(&mut self, outcome: Result) { + self.forced_pty_lookup = Some(outcome); + } + /// Override the SIGTERM-to-SIGKILL grace window. Test helper. pub fn set_grace_period(&mut self, d: Duration) { self.grace_period = d; @@ -1080,7 +1295,15 @@ 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)?; + // Sample the real group BEFORE signalling. Only the spawn-group + // path computes its target from the `pgid == pid` assumption, so + // it is the only one a measurement can contradict; a PTY target + // came from the terminal and a leader-directed target is not a + // group at all. + let measured = + matches!(target.source, TargetSource::SpawnGroup).then(|| measured_group_of(pid)); // 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 +1317,14 @@ 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, + measured.as_deref(), + )); } if matches!(signal, Signal::SIGTERM | Signal::SIGKILL | Signal::SIGHUP) { proc.state = ProcessState::Exiting { @@ -1243,9 +1473,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; } @@ -2304,18 +2549,6 @@ mod tests { /// `/bin/sleep` directly rather than through a shell: a shell may /// 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 { - rows: 24, - cols: 80, - mode: TerminalMode::Canonical, - }; - let id = sup.spawn(spec).expect("spawn"); - (id, spawn_started_pid(sup, id)) - } - /// The OS pid straight from the supervisor's own record, WITHOUT /// ticking. /// @@ -2384,39 +2617,492 @@ mod tests { } } - /// Q#PD1 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. + /// The job-control shell the divergence fixture drives. Named once so + /// 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 + /// **production** lookup — not `portable_pty`'s + /// `process_group_leader`, which this crate no longer uses on the + /// signal path. Reading it any other way would let + /// `pty_foreground_group` fall back on every call while every test + /// that depends on it stayed green. `None` for a pipe + /// generation, or when the terminal reports no foreground group. + fn foreground_pgid(sup: &ProcessSupervisor, id: ProcessId) -> Option { + let runtime = sup.processes.get(&id)?.runtime.as_ref()?; + match &runtime.child { + ChildHandle::Pty { + _master: master, .. + } => pty_foreground_group(master.as_ref()).ok(), + ChildHandle::Pipes(_) => None, + } + } + + /// Fixture for Q#DC1: a PTY child whose terminal foreground group is + /// genuinely **not** the spawned leader. /// - /// Asserted as an exact message against the pid the kernel actually - /// assigned, so a hardcoded target could not satisfy it. The leader - /// field is the one that matters: for a PTY the signal goes to the - /// terminal's foreground group, a different entity from the spawned - /// 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. + /// `bash -m` enables job control, so it runs the script's command in + /// a fresh process group and hands that group the terminal. The + /// trailing `; :` matters — with a single simple command `bash -c` + /// execs in place, which would leave the leader owning the terminal + /// and silently restore the very agreement this fixture exists to + /// break. + /// + /// **The wait is load-bearing, not defensive.** The handoff is not + /// instantaneous: a probe of this exact fixture observed the + /// foreground group as the leader first and only then as the job's + /// group. Measuring immediately would pin the non-divergent case and + /// the test would assert the opposite of its purpose. + /// + /// Returns `(id, leader_pid, foreground_pgid)` with the two pids + /// known to differ and the foreground group known to hold a live + /// member. + fn spawn_pty_with_diverged_foreground_group( + sup: &mut ProcessSupervisor, + name: &str, + ) -> (ProcessId, u32, i32) { + let mut spec = ProcessSpec::new(name, BASH); + spec.args = vec![ + "--noprofile".into(), + "--norc".into(), + "-m".into(), + "-c".into(), + "sleep 30; :".into(), + ]; + spec.mode = ProcessMode::Pty { + rows: 24, + cols: 80, + mode: TerminalMode::Canonical, + }; + let id = sup.spawn(spec).expect("spawn"); + let leader = spawn_started_pid(sup, id); + + let leader_i32 = i32::try_from(leader).expect("pid fits i32"); + let deadline = Instant::now() + Duration::from_secs(10); + let mut observed: Vec = Vec::new(); + let mut diverged = None; + while Instant::now() < deadline { + if let Some(fg) = foreground_pgid(sup, id) { + if observed.last() != Some(&fg) { + observed.push(fg); + } + if fg > 0 && fg != leader_i32 { + diverged = Some(fg); + break; + } + } + std::thread::sleep(Duration::from_millis(25)); + } + + let fg = diverged.unwrap_or_else(|| { + panic!( + "job control never moved the terminal off the leader \ + (leader={leader}, foreground groups observed: {observed:?})" + ) + }); + + // Positive control: a divergent number proves nothing if the + // group is already dead. The signal target must be a group that + // could actually receive a signal. + nix::sys::signal::kill(Pid::from_raw(-fg), None).unwrap_or_else(|e| { + panic!("foreground group {fg} has no live member ({e}); divergence is vacuous") + }); + + (id, leader, fg) + } + + /// 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 facts **that are not the same fact + /// repeated**. + /// + /// 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. + /// + /// **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() { let mut sup = ProcessSupervisor::new(); 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 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"); let expected = format!( - "kill: {} (target=-{pid} 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!( err, expected, - "the report names the exact target the tty reported, the exact \ + "the report names the exact group the lookup returned, the exact \ leader pid, and observes the leader as live" ); + // 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 group the lookup returned: {err}" + ); + assert!( + !err.contains(&format!("target=-{pid} via tcgetpgrp")), + "the target must NOT be the leader pid: {err}" + ); + 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" + ); + + // Force ONLY the kill failure. The lookup is left alone, so + // `pty_foreground_group` runs for real against a real terminal + // and the report below is built from what it returned. + // + // This is the assertion that makes the injected pin meaningful: + // without it, `pty_foreground_group` could fall back on every + // call and every other test here would still pass, because they + // all supply the group themselves. + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + let expected = format!( + "kill: {} (signal=SIGTERM, target=-{fg} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=live)", + nix::errno::Errno::EPERM + ); + assert_eq!( + err, expected, + "the production lookup must report the real foreground group" + ); + assert!( + !err.contains("pty-leader-fallback"), + "a healthy terminal must not take the fallback branch: {err}" + ); + + 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(Err(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. @@ -2432,7 +3118,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!( @@ -2482,7 +3168,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!( @@ -2514,7 +3200,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");