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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T
This commit is contained in:
parent
36a37f6086
commit
9b1cf3d6c9
|
|
@ -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.**
|
||||
|
|
|
|||
|
|
@ -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/<pid>/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/<pid>/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
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
Loading…
Reference in New Issue