From 9b1cf3d6c98791ef0be184d27e1a520792ef2868 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 20:58:52 -0400 Subject: [PATCH] fix(process): reproduce the deadlock without a shell CI falsified rev 2 of the framing. The synthetic reproduction used `sh -c 'cat <&0 & exit 0'`, and `<&0` does not defeat the POSIX rule it was chosen to defeat: /dev/null is assigned to an asynchronous list's stdin *before any explicit redirections*, so by the time `<&0` runs, fd 0 already IS /dev/null and the redirect duplicates it onto itself. bash happens to skip the default when a stdin redirect is present; dash -- Ubuntu's /bin/sh, and CI's -- does not. It passed locally and failed on three CI legs. Control 2 caught it and named its own cause. That is the fourth vacuous reproduction in this lane and the first found by a control rather than by a reviewer -- which is the argument for the controls, so the lesson is recorded that way in the handoff. The reproduction now uses `setsid --fork cat`: it forks, the parent exits, and the child inherits stdin/stdout/stderr untouched. No shell, no asynchronous list, no /dev/null rule, no implementation variance. setsid(1) presence is asserted rather than skipped -- a skip would reintroduce the silent-green shape the arming lane removed. The fix under test is unchanged. Bite re-verified by revert on the new form: ok in 2.03s with `stdin.take()`, FAILED at 10.00s on the recv_timeout without it, both controls passing first. Also adds bottom_panel_stage1_acceptance to the framing's Bet 2 falsifier list. It holds PTY-in-panel tests and its absence from rev 1 was a real gap, not a judgement call. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/agent-handoff.md | 29 ++++--- ...process-teardown-stdin-deadlock-framing.md | 86 +++++++++++++------ src/process.rs | 72 ++++++++++------ 3 files changed, 126 insertions(+), 61 deletions(-) diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 7b997e1..5a1d4a0 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -1471,21 +1471,30 @@ round-trip cannot detect a discriminant shift. and the double-invocation traps: **the thing that summarizes a gate must not be able to lose the gate's verdict.** - **A reproduction is a measurement, and needs its own positive control.** - The basedpyright-hang lane wrote **three** reproductions that passed + The basedpyright-hang lane wrote **four** reproductions that passed against the *unfixed* tree, each vacuous for a different reason: the - child exited before the join; the child never read stdin at all; and — - found at framing review — the child's stdin was silently rebound to - `/dev/null`, because POSIX XCU §2.9.3 assigns `/dev/null` to an - asynchronous list's stdin when job control is off, so `sh -c 'cat & - exit 0'` EOFs instantly (the fix is an explicit `<&0` redirect). Every - one looked obviously right when written. Note what a narrower rule + child exited before the join; the child never read stdin at all; the + child's stdin was silently rebound to `/dev/null` (POSIX XCU §2.9.3 + assigns `/dev/null` to an asynchronous list's stdin when job control is + off, so `sh -c 'cat & exit 0'` EOFs instantly); and then **the repair + for that was also wrong** — the rule applies *before explicit + redirections*, so `<&0` duplicates `/dev/null` onto itself. `bash` + skips the default when a stdin redirect is present, `dash` does not, so + `<&0` passed locally and failed in CI. The shipped test uses + `setsid --fork`, removing the shell from the reproduction entirely. + Every one of the four looked obviously right when written, and the + fourth was verified locally before it failed. Note what a narrower rule would have missed: "check the child is still alive" catches only the - first. Only the general form catches all three — **and the ones nobody - has invented yet.** So: assert the precondition your reproduction + first. Only the general form catches all four — **and the ones nobody + has invented yet.** Note also which mechanism caught the fourth: not a + reviewer, but the control itself, failing loudly in CI and naming its + own cause. So: assert the precondition your reproduction depends on, in the test, before exercising the thing under test. In `teardown_closes_stdin_before_joining_readers` that is two controls (the recorded child has exited; both readers are still blocked in - `read`), each with a failure message naming what its absence means. + `read`), each with a failure message naming what its absence means — + and a `/bin/sh` that is `bash` locally and `dash` in CI is exactly the + sort of divergence no amount of local verification reaches. This is the same rule that produced #192's bite positive control and #194's re-read-the-artifact lesson, stated at full generality: **a measurement you have not controlled is a claim, not evidence.** diff --git a/docs/process-teardown-stdin-deadlock-framing.md b/docs/process-teardown-stdin-deadlock-framing.md index 46b509f..246f9b3 100644 --- a/docs/process-teardown-stdin-deadlock-framing.md +++ b/docs/process-teardown-stdin-deadlock-framing.md @@ -32,6 +32,15 @@ new primitive.** stdin writer (a child that read stdin but stopped draining it), criterion 5 extended to `docs/agent-handoff.md` §3, Bet 4 marked as lane-stopping. +- **rev 3** — CI falsified rev 2's repair. `<&0` is defeated on `dash` + (the rule applies *before* explicit redirections, so `<&0` duplicates + `/dev/null` onto itself); it passed locally only because `/bin/sh` here + is `bash`. **Control 2 caught it in CI and named its own cause** — the + fourth vacuity in this lane, and the first one a control found instead + of a reviewer. The reproduction now uses `setsid --fork cat`, removing + the shell entirely. Bet 2's falsifier list also gained + `bottom_panel_stage1_acceptance`, which holds PTY-in-panel tests and + was a genuine gap in rev 1's list. --- @@ -336,20 +345,38 @@ held-open fifo, checking the orphan's `/proc//fd/0`: | `sh -c 'cat & exit 0'` | **gone** | — (EOF'd from `/dev/null`) | | `sh -c 'cat <&0 & exit 0'` | alive | the real pipe | -**The faithful model is therefore `sh -c 'cat <&0 & exit 0'`.** The -explicit redirect is what defeats the `/dev/null` assignment; it is -load-bearing, not incidental, and must not be "simplified" away. +**Trap 3 — `<&0` does not repair it, and the obvious fix is wrong.** rev 2 +proposed `sh -c 'cat <&0 & exit 0'`, verified on this machine. **CI +falsified it.** Re-read the rule: `/dev/null` is assigned *before any +explicit redirections*, so by the time `<&0` runs, fd 0 already **is** +`/dev/null`, and the redirect faithfully duplicates it onto itself. +`bash` happens to skip the default when a stdin redirect is present; +`dash` — Ubuntu's `/bin/sh`, and CI's — does not. Measured: -`sh` exits immediately (so `poll_one` observes termination), `cat` is -orphaned holding the real stdin read end plus both write ends, and it -exits on EOF exactly as a stdio language server does. Unfixed, this -deadlocks; fixed, teardown completes. +| shell | form | grandchild | fd 0 | +| --- | --- | --- | --- | +| bash | `cat & exit 0` | gone | — | +| bash | `cat <&0 & exit 0` | alive | real pipe | +| dash | `cat <&0 & exit 0` | **gone** | — (CI: control 2 failed) | -Which `/bin/sh` applies the rule how varies by machine, so the redirect -alone is not enough of a guarantee — criterion 2 carries a positive -control (§4) so the test cannot silently degrade back into modelling the -wrong thing on someone else's box. This is #192's lesson one level down: -the bite needs a control, and so does the reproduction. +The local probe could not have caught this: `/bin/sh` here is `bash`. + +**The model is therefore `setsid --fork cat`, with no shell at all.** +`setsid --fork` forks, the parent exits, and the child inherits +stdin/stdout/stderr untouched — no asynchronous list, no `/dev/null` +rule, no implementation variance. The recorded pid (`setsid`) terminates +promptly so `poll_one` reaches the teardown path, while `cat` survives +holding the inherited pipes and exits on EOF exactly as a stdio language +server does. Unfixed, this deadlocks; fixed, teardown completes. + +`setsid(1)` is util-linux, which the Linux gate already assumes. +Presence is **asserted, not skipped** — a skip would reintroduce the +silent-green shape lane 2 removed. + +The controls are what make this recoverable rather than a silent +regression: control 2 failed loudly in CI and named its own cause. That +is #192's lesson one level down — the bite needs a control, and so does +the reproduction. --- @@ -361,8 +388,11 @@ the bite needs a control, and so does the reproduction. 2. **The reorder is safe for PTY mode.** Falsified by any regression in `vterm_stage1/2/3_acceptance`, `terminal_config_acceptance`, `terminal_copy_mode_acceptance`, `m6_4/m6_5_repl_acceptance`, - `m6_7_scrollback_acceptance`, `m6_8_multi_repl_acceptance`, or - `worker_shutdown_acceptance`. + `m6_7_scrollback_acceptance`, `m6_8_multi_repl_acceptance`, + `worker_shutdown_acceptance`, or **`bottom_panel_stage1_acceptance`** + — added in rev 3: it holds PTY-in-panel tests (`acc28` drives real + child input and the `C-c` escape) and its absence from rev 1's list + was a real gap, not a judgement call. 3. **The synthetic test bites.** Falsified if the new test passes with `let _ = self.stdin.take();` removed. This must be checked by actual revert, per the standing rule that a new pin needs its own bite. @@ -383,18 +413,22 @@ the bite needs a control, and so does the reproduction. 2. New unit test in `src/process.rs` (so it runs under the standard `cargo test --lib` gate, not only an acceptance suite): `teardown_closes_stdin_before_joining_readers`. - - Spawns `sh -c 'cat <&0 & exit 0'` as a **non-group pipe** process. - The `<&0` is load-bearing (Q#TD6) and gets a comment saying so. - - **Positive control, before teardown starts:** assert the orphaned - grandchild is alive *and* that its `/proc//fd/0` is not - `/dev/null`. Without this the test silently degrades into modelling - the wrong thing wherever `/bin/sh` behaves differently, and reports - green while doing it. - - `#[cfg(target_os = "linux")]`: the control reads `/proc`, and the - reproduction depends on `sh` async-list semantics. Gate it - explicitly and say why, rather than letting it be incidentally - Linux-only. (Same reasoning as the APFS gate — `cfg(unix)` would be - wrong here.) + - Spawns `setsid --fork cat` as a **non-group pipe** process. The + choice of `setsid` over a shell background job is load-bearing + (Q#TD6) and gets a comment saying so. `setsid` presence is + **asserted, not skipped.** + - **Two positive controls, before teardown starts:** (1) the recorded + child has actually exited — while it lives it holds the output pipe + itself, so control 2 would pass for the wrong reason; (2) both + readers are still blocked in `read`, which is only true while + something still holds the write ends. Without these the test + silently degrades into modelling the wrong thing and reports green + while doing it — which is exactly what happened on `dash`, and + control 2 is what caught it. + - `#[cfg(target_os = "linux")]`: the controls read `/proc`, and + `setsid(1)` is util-linux (absent on macOS). Gate it explicitly and + say why, rather than letting it be incidentally Linux-only. (Same + reasoning as the APFS gate — `cfg(unix)` would be wrong here.) - Performs the full reap-and-drop sequence on a helper thread and asserts completion via `recv_timeout`, so a regression **fails** within a bounded window instead of hanging. A test that hangs on diff --git a/src/process.rs b/src/process.rs index d5555ec..bf40fcc 100644 --- a/src/process.rs +++ b/src/process.rs @@ -3240,18 +3240,25 @@ mod tests { /// console script spawns bundled `node` and exits, leaving the real /// server at `PPid 1` holding the inherited pipes. /// - /// `<&0` is LOAD-BEARING, not decoration. POSIX XCU 2.9.3 assigns - /// `/dev/null` to an asynchronous list's stdin when job control is - /// off --- i.e. in every non-interactive `sh` --- so a bare `cat &` - /// reads EOF immediately and exits *against the unfixed tree*, - /// giving a test that passes either way and proves nothing. Measured - /// on `bash`: bare `&` leaves no grandchild, `<&0` leaves one - /// holding the real pipe. Both controls below exist to catch that - /// silently regressing on another `/bin/sh`. + /// `setsid --fork` is used rather than a shell background job, and + /// that choice is LOAD-BEARING. POSIX XCU 2.9.3 assigns `/dev/null` + /// to an asynchronous list's stdin when job control is off --- i.e. + /// in every non-interactive `sh` --- so `sh -c 'cat & exit 0'` reads + /// EOF immediately and exits *against the unfixed tree*, giving a + /// test that passes either way and proves nothing. The obvious + /// repair does not work either: the rule applies **before explicit + /// redirections**, so by the time `<&0` runs, fd 0 already *is* + /// `/dev/null` and the redirect faithfully duplicates it onto + /// itself. `bash` happens to skip the default when a stdin redirect + /// is present; `dash` --- Ubuntu's `/bin/sh`, and CI's --- does not, + /// so `<&0` passed locally and failed in CI. + /// + /// `setsid --fork` sidesteps all of it: it forks, the parent exits, + /// and the child inherits stdin/stdout/stderr untouched by any shell. + /// No async list, no `/dev/null` rule, no implementation variance. /// /// Linux-gated deliberately rather than incidentally: the controls - /// read `/proc`, and the reproduction depends on `sh` async-list - /// semantics. + /// read `/proc`, and `setsid(1)` is util-linux (absent on macOS). /// /// On the failure path this leaks a wedged worker thread, and `cat` /// survives until the harness's fds close at process exit. Bounded @@ -3277,16 +3284,27 @@ mod tests { } } + // Asserted, not skipped: this test is already Linux-gated, and + // setsid(1) is core util-linux. A skip here would reintroduce + // exactly the silent-green shape the arming lane removed. + assert!( + binary_available("setsid"), + "setsid(1) is required to orphan the grandchild without a \ + shell; it is core util-linux and should be present on any \ + Linux runner" + ); + let (done_tx, done_rx) = mpsc::channel(); let handle = std::thread::spawn(move || { let mut sup = ProcessSupervisor::new(); sup.set_grace_period(Duration::from_millis(300)); - let mut spec = ProcessSpec::new("orphan-holds-pipe", "/bin/sh"); - // `cat` reads stdin and exits on EOF, exactly as a stdio - // language server does. `exit 0` makes the *recorded* pid - // terminate promptly, so `poll_one` reaches the teardown - // path while the grandchild still holds the output pipe. - spec.args = vec!["-c".into(), "cat <&0 & exit 0".into()]; + let mut spec = ProcessSpec::new("orphan-holds-pipe", "setsid"); + // `setsid --fork` forks and the parent exits, so the + // *recorded* pid terminates promptly (letting `poll_one` + // reach the teardown path) while `cat` survives holding the + // inherited pipes. `cat` reads stdin and exits on EOF, + // exactly as a stdio language server does. + spec.args = vec!["--fork".into(), "cat".into()]; // The default, restated because it is the whole point: with // `StdinMode::Null` there is no sink to drop and no EOF to // deliver. @@ -3302,7 +3320,9 @@ mod tests { // CONTROL 1: the recorded child must actually exit. Until it // does, *it* holds the output pipe, and control 2 would pass - // for the wrong reason. + // for the wrong reason. (`setsid` without `--fork` may exec + // directly instead of forking, in which case there is no + // grandchild and this is the control that notices.) let deadline = Instant::now() + Duration::from_secs(5); while Instant::now() < deadline && !reaped_or_zombie(sh_pid) { std::thread::sleep(Duration::from_millis(10)); @@ -3317,10 +3337,12 @@ mod tests { // CONTROL 2: both readers must still be blocked in `read`, // which is only true while something still holds the output - // pipe's write ends. If the grandchild never inherited - // stdin (the /dev/null rule above), it has already exited, - // the write ends are closed, the readers have finished --- - // and the deadlock is not being modelled at all. + // pipe's write ends. If the grandchild never inherited the + // real stdin, it has already read EOF and exited, the write + // ends are closed, the readers have finished --- and the + // deadlock is not being modelled at all. This control is + // what caught the shell form failing on dash after it + // passed on bash. let readers = sup .processes .get(&id) @@ -3337,10 +3359,10 @@ mod tests { (2, 2), "control 2 failed: both readers must still be blocked in \ `read`, i.e. an escaped grandchild still holds the output \ - pipe. Finished readers mean `cat` never inherited stdin \ - (POSIX assigns /dev/null to a background job's stdin when \ - job control is off) and the `<&0` redirect has stopped \ - working on this `/bin/sh`" + pipe. Finished readers mean `cat` read EOF and exited \ + already, so it never inherited the real stdin --- check \ + that `setsid --fork` still forks and passes fds 0/1/2 \ + through untouched on this runner" ); // The deadlock, if present, is here: