From eb9b36387c21f1c397d9338813982ab19e37434f Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 19:22:59 -0400 Subject: [PATCH 1/5] fix(process): close child stdin before joining reader threads `RuntimeHandles::drop` joined its reader threads in the `Drop` body, which runs before any field drops. The `ChildStdin` sink lives inside `StdinWriter` in the `stdin` FIELD, so it could only be released after the join returned -- and the join was waiting on readers blocked in `read()` on pipes whose write ends the child still held, because the child never received the stdin EOF that would have made it exit. A closed cycle, entirely inside one function. Teardown hung forever. This is the root cause of `m4_5_basedpyright_initializes_and_negotiates_ encoding` hanging indefinitely -- diagnosed with gdb stacks plus /proc fd forensics on a wedged process, reproduced 5/5 deterministically. It also explains why the hang looked intermittent and machine-local: a shim-launched server orphans its real process (basedpyright's console script spawns bundled `node` and exits, leaving it at `PPid 1`), so nothing teardown signals can reach it, while a direct binary like clangd or gopls is a genuine child whose pipes close on reap. `spawn_reader`'s `cancel` flag does not help: it is consulted between reads and around `send_timeout`, never while `read` is blocked. The existing comment's premise -- "dropping the master closes the kernel pipe and unblocks `read`" -- holds for a PTY master but not for pipe mode, where `read` returns only once *every* write end closes. The fix reuses `close_stdin`'s existing, already-idempotent mechanism at the one site missing it. Reordering the struct's fields cannot work: a type's `Drop::drop` body runs before all of its fields regardless of declaration order. Bounded claim: this delivers EOF, so it fixes children that drain stdin to EOF -- which stdio language servers do. A child that ignores EOF, or that stops draining while bytes are queued (the writer's `write_all` is blocking), still wedges the join. Making the `read` itself cancellable via the poll path already used by `spawn_group_reader` is the standing deferral that covers those, and is deliberately not in this change. Test: `teardown_closes_stdin_before_joining_readers`, in `--lib` so it runs in the standard gate. It models the real shape with an orphaned grandchild, and carries two positive controls, because this lane wrote three reproductions that passed against the unfixed tree before one bit. The `<&0` redirect is load-bearing: POSIX XCU 2.9.3 assigns `/dev/null` to an asynchronous list's stdin when job control is off, so a bare `cat &` exits immediately and proves nothing. Teardown runs on a worker thread behind `recv_timeout` so a regression FAILS in 10s rather than hanging -- a hanging test would reproduce the hazard being removed. Bite verified by revert: with the fix `ok` in 2.03s; with the single `stdin.take()` line commented out, FAILED at 10.00s on the timeout, both controls having passed first. Docs: framing doc added; handoff gains the drop-body-before-fields lesson and the reproduction-needs-a-control generalization, and its section 3 caveat is corrected -- the desktop's basedpyright binary was never broken. The `--skip basedpyright` gate entry stays for now; dropping it is a separate proposal owed evidence. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/agent-handoff.md | 51 +- ...process-teardown-stdin-deadlock-framing.md | 491 ++++++++++++++++++ src/process.rs | 155 ++++++ 3 files changed, 693 insertions(+), 4 deletions(-) create mode 100644 docs/process-teardown-stdin-deadlock-framing.md diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 2c5ca22..7b997e1 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -1289,10 +1289,21 @@ git diff --check Machine-specific caveats — re-verify on a machine you haven't used before trusting them: -- **basedpyright**: the DESKTOP's local binary is broken and HANGS the - `m4_5_basedpyright` tests — hence the `--skip` there. The LAPTOP has - a working basedpyright 1.39.9 (verified 2026-07-10: the m4_5 test - passes in 0.18s), so the skip is droppable on the laptop. +- **basedpyright**: the desktop binary was **never broken** — this was a + real code defect, diagnosed and fixed 2026-07-29 (see §5, "A `Drop` + body runs before its fields"). `RuntimeHandles::drop` joined its reader + threads before the `stdin` field dropped, so a shim-launched server + (basedpyright's console script spawns bundled `node` and exits, leaving + the real server at `PPid 1`) never got stdin EOF, never exited, and + kept the output pipe the readers were blocked on. Deterministic on the + desktop, invisible on the laptop and in CI, which is why it read as a + broken local binary for weeks. + The `--skip` above stays for now: it is still correct on any tree + predating the fix, and CI never installs basedpyright at all + (`PMACS_REQUIRE_PYRIGHT` is deliberately unarmed, #194, and stays that + way until the per-test timeout lane lands — arming it without a timeout + would hand CI an unbounded hang). Dropping the skip is a separate + proposal, owed evidence of repeated green runs. - **GPU on the laptop**: AMD Radeon 780M (RADV) — native Vulkan, `PMACS_REQUIRE_GPU=1` works without lavapipe. - **Flaky-under-load tests — rerun isolated before treating a sweep @@ -1459,6 +1470,38 @@ round-trip cannot detect a discriminant shift. asks you to keep. Same family as the skip-reports-`ok` lesson below 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 + 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 + 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 + 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. + 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.** +- **A `Drop` body runs before its fields, whatever the declaration + order.** Cost a multi-week misattribution: `RuntimeHandles::drop` + joined its reader threads in the drop *body*, while the `stdin` sink it + needed to close first sat in a *field* — reachable only after that body + returned. The child never got EOF, never exited, and kept the output + pipe the readers were blocked on, so teardown hung forever. Reordering + the struct's fields cannot fix this shape; the operation has to move + into the body. Generally: **if a `Drop` body waits on anything, check + what the waited-on party needs that only a field drop will release.** + Corollary from the same investigation — `cancel`-flag style wake-outs + only work where the thread actually polls them; a thread blocked in a + raw `read` never sees one, so a flag next to a blocking syscall is + documentation, not a mechanism. - **A test that skips on a missing precondition reports `ok`, and a gate log cannot tell that apart from a pass.** `vterm_stage3_acceptance::a37` — the only acceptance driving a real daemon, a real PTY and a real wgpu render diff --git a/docs/process-teardown-stdin-deadlock-framing.md b/docs/process-teardown-stdin-deadlock-framing.md new file mode 100644 index 0000000..46b509f --- /dev/null +++ b/docs/process-teardown-stdin-deadlock-framing.md @@ -0,0 +1,491 @@ +# Framing — close child stdin before joining readers (process teardown deadlock) + +A pipe-mode child that exits on stdin EOF can deadlock the supervisor's +teardown forever. `RuntimeHandles::drop` joins its reader threads in the +`Drop` body, which runs **before** the `stdin` field drops, so the child +never receives the EOF that would make it close the very pipe write ends +those readers are blocked on. The fix is a two-line reorder that reuses a +mechanism already present in this file. + +This is the diagnosed root cause of +`m4_5_basedpyright_initializes_and_negotiates_encoding` hanging +indefinitely — the hazard that has parked `cargo test --workspace` runs +(once for 2h26m) and forced `-- --skip basedpyright` into every gate +recipe. + +**Scope: `src/process.rs` only. No protocol change. No Lua surface. No +new primitive.** + +--- + +## Revision history + +- **rev 1** — initial framing. Root cause established by live diagnosis + (gdb stacks + `/proc` fd forensics on a wedged process), reproduced + 5/5 deterministically at `e003b81`. +- **rev 2** — review round 1. rev 1's synthetic child was **itself + vacuous** (the third in this lane): POSIX assigns `/dev/null` to a + background job's stdin when job control is off, so `sh -c 'cat & + exit 0'` EOFs instantly and exits against the *unfixed* tree. + Q#TD6 now uses the explicit-redirect form and criterion 2 gains a + positive control. Also: Q#TD3's bound widened to cover a blocked + 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. + +--- + +## 0. Coherence impact (COHERENCE §20) + +This is a defect fix, not coherence work, and it should not claim +otherwise. + +- **Journey steps touched:** none directly. It protects the steps that + depend on a live language server (§2 step 6 onward) from an unbounded + teardown, but it adds no journey surface. +- **Interaction islands added:** none. +- **Config registry:** no new options. +- **Background-work attribution:** unchanged. The supervisor's process + model is untouched; only the order of two teardown operations moves. +- **Protocol:** unchanged. + +The one genuine coherence connection is indirect and worth stating +plainly: the hang parks `cargo test --workspace`, which is the ratchet +every COHERENCE priority is verified against (§19, §25). A gate that can +hang forever degrades every other lane's evidence. That is the argument +for doing this now rather than parking it — not a claim that it advances +a priority. + +--- + +## 1. Ground truth (scouted @ `e003b81`) + +Line numbers are hints; symbols are authoritative. + +### 1.1 The reproduction is deterministic, not intermittent + +`docs/agent-handoff.md` and the test-improvement audit both describe this +hang as intermittent. On a machine where `basedpyright-langserver` +resolves to a uv-installed shim it is **completely reliable**: 5 runs, 5 +hangs, via + +``` +cargo test --test m4_acceptance -- --exact \ + m4_5_basedpyright_initializes_and_negotiates_encoding +``` + +§1.7 explains why it looks intermittent across machines. The practical +consequence: this defect is directly testable, and any fix has a +revert-bite. + +### 1.2 The cycle, in five links + +Observed stack of the wedged test thread (gdb, `sudo` required — +`ptrace_scope=1`): + +``` +tests/m4_acceptance.rs:1374 Rc> dropped +→ ProcessSupervisor::drop src/process.rs:1545 +→ ProcessSupervisor::shutdown src/process.rs:1463 +→ ProcessSupervisor::tick src/process.rs:1188 +→ ProcessSupervisor::poll_one src/process.rs:1268 (drop site: :1337) +→ RuntimeHandles::drop src/process.rs:631 +→ JoinHandle::join ← blocked, indefinitely +``` + +The links: + +1. **`RuntimeHandles::drop` (`:631`)** sets `cancel`, then joins every + handle in `self.readers`. +2. **Rust runs a type's `Drop::drop` body before dropping its fields.** + `stdin: Option` is a *field* (`:505`), so it cannot drop + until the body returns. The body never returns. +3. **`StdinWriter` (`:556`)** holds the `Sender`; `StdinWriter::spawn` + (`:568`) moves the `ChildStdin` sink into its thread, which drops the + sink only once `rx.recv()` errors. Sender alive ⇒ sink alive ⇒ **the + child's stdin write end never closes.** +4. **The child therefore never sees EOF**, stays alive, and keeps the + stdout/stderr **write** ends it inherited. +5. **The readers are blocked in `read()`** at `:1886` inside + `spawn_reader` (`:1874`). `cancel` is consulted only at the loop top + (`:1883`) and around `send_timeout` — **never while `read` is + blocked.** + +Verified on the live process: the test held fd 4 (child stdin, WRONLY) +and fds 5 and 7 (stdout/stderr, RDONLY); the server process held the +matching opposite ends on fds 0, 1, 2. Two reader threads sat in +`anon_pipe_read`, and the stdin-writer thread sat parked in +`Receiver::recv` at `:577` — alive, still owning the sink. + +Confirmation from the other direction: when the wedged test process was +killed, its fd 4 closed, the server immediately saw stdin EOF and +exited. The cycle's load-bearing link is exactly the one the fix cuts. + +### 1.3 The existing comment names the false premise + +`RuntimeHandles::drop` documents its own reasoning: + +> Wake any reader thread blocked in a bounded `send` — dropping the +> master closes the kernel pipe and unblocks `read`, but does nothing +> for a reader stuck on a full channel […] + +The premise is true for a **PTY master** and false for **pipe mode**, +where `read` unblocks only when *every* write end closes. `cancel` was +introduced for the full-channel case and is correct for it; the comment +mistakenly treats the `read` case as already handled. + +### 1.4 `shutdown()`'s SIGKILL phase is unreachable on this path + +`shutdown()` (`:1463`) sends SIGTERM to all ids, then runs a bounded +grace loop (`deadline` at `:1476`) that calls `tick()`, and *then* +escalates to SIGKILL. The stack shows the deadlock occurs **inside that +grace loop's `tick()`**, because `poll_one` drops `RuntimeHandles` the +moment it observes the recorded pid exited. The SIGKILL phase is never +reached. + +So "shutdown force-kills everything first" is not true of this path. +(An earlier working assumption of mine said it did; the stack refutes +it.) Even if reached, SIGKILL targets the *recorded* pid, which per +§1.7 is not the surviving process. + +### 1.5 Only pipe-mode, non-group spawns are affected + +`spawn_pipes` (~`:1712`) chooses per stream: + +| `spec.group` | reader | cancellable mid-`read`? | +| --- | --- | --- | +| `true` | `spawn_group_reader` (`:1941`) — `O_NONBLOCK` + `poll` | **yes** | +| `false` | `spawn_reader` (`:1874`) — blocking `read` | **no** | + +PTY mode (~`:1724`) also uses `spawn_reader`, but there §1.3's premise +holds: dropping the master genuinely ends the read. The `spawn_ansi_parser` +reader also lives in `readers`, and reads a channel rather than an fd, so +it is unaffected. + +Non-group pipe consumers are, per `spawn_reader`'s own doc comment, the +**REPL and LSP** paths. This defect is therefore reachable by every LSP +server and every REPL — not by terminals. + +### 1.6 The fix mechanism already exists in this file + +`close_stdin` (`:1611`) already does precisely what is needed, and +already documents the semantics and the idempotence: + +```rust +// Dropping the writer closes the pipe at the kernel +// level. `take()` is idempotent — second call sees None. +let _ = runtime.stdin.take(); +``` + +The fix is applying an existing, already-reviewed mechanism at the one +site that is missing it. It introduces no new concept. + +### 1.7 Why basedpyright wedges and clangd/gopls do not + +`basedpyright-langserver` is a uv-installed **Python console script**: + +```python +from basedpyright.langserver import main +sys.exit(main()) +``` + +`main()` spawns the bundled `node …/langserver.index.js --stdio` and the +Python process exits, so the real server is an **orphaned grandchild** +(observed `PPid: 1`, reparented to systemd) holding the inherited pipe +fds. The supervisor recorded the shim's pid, which has already exited and +been reaped, so `poll_one` sees a terminated process on its very first +tick and proceeds straight into the deadlock. + +`clangd` and `gopls` are real binaries: genuine children, reaped +normally, write ends closed, blocking `read` returns `Ok(0)` cleanly. The +"intermittency" in the handoff is not timing — it is *which server binary +is installed how*. + +### 1.8 Limits of the evidence + +- The deterministic reproduction is **one machine, one server**. The + causal chain is verified there link by link; its generality to other + shim-launched servers is reasoned, not measured. +- The gdb capture is a single sample of a state that was stable across a + four-minute window and identical across two independent runs. That is + strong for a deadlock and would be weak for a race. +- Nothing here establishes how often the hang has fired in CI. CI never + installs basedpyright (`PMACS_REQUIRE_PYRIGHT` is deliberately never + set, #194), so in CI this test skips and the defect is **dark**. Every + observation is local. + +--- + +## 2. Decisions + +### Q#TD1 — the fix is a reorder inside `Drop`, not a new primitive + +```rust +impl Drop for RuntimeHandles { + fn drop(&mut self) { + self.cancel.store(true, Ordering::Relaxed); + // Close the child's stdin BEFORE joining. A stdio child exits + // on EOF and closes its stdout/stderr write ends, and that — + // not `cancel` — is what unblocks a reader parked in `read` + // (`cancel` is only observed between reads and around `send`). + // The sink lives in the `stdin` field, which cannot drop until + // this body returns, so joining first deadlocks against it. + let _ = self.stdin.take(); + for h in std::mem::take(&mut self.readers) { + let _ = h.join(); + } + } +} +``` + +Rejected alternative: reordering the struct's *fields*. Field order does +not help — the explicit `Drop::drop` body runs before **all** fields +regardless of their declaration order. This is the trap that makes the +bug non-obvious, and it belongs in the comment. + +### Q#TD2 — the reorder is unconditional across modes + +Applying it only to pipe+non-group would require `RuntimeHandles::drop` +to learn which mode it is in, which it currently does not need to know. +Closing stdin before teardown is correct in both modes, so the reorder is +unconditional. + +This is a uniformity change, and uniformity changes in this repo have +made total functions partial before. It is therefore carried as a **bet +with a named falsifier** (§3, Bet 2), not as an assumption: PTY-mode +`stdin` is the pty *writer*, and dropping it while `pair.master` and the +cloned reader still exist must not end the read early. + +### Q#TD3 — the fix assumes the child drains stdin to EOF, and covers nothing outside that + +Stated up front because it bounds the claim: the fix works by making the +child exit. A child that never reads stdin — or reads it and ignores EOF +— keeps its write ends open and still wedges the join. + +There is a third member of that family, and it is not covered by the +wording above because such a child *did* read stdin: **the EOF is only +delivered if the writer thread reaches the end of its queue.** Its body +is a blocking `sink.write_all(&bytes)` (`:578`), so a child that has +stopped draining stdin while queued bytes remain blocks the writer +indefinitely — the sink never drops, EOF never arrives, and the join +re-wedges. This needs only a full stdin pipe buffer at teardown time, not +a misbehaving child. For LSP teardown the queue is near-empty and the +practical risk is nil, but the bound belongs in the claim: **the fix +assumes the child keeps draining stdin until EOF.** A full stdin pipe +with a non-draining child is P1's case as well. + +Covering *that* case requires making the blocking `read` itself +cancellable, i.e. moving non-group readers onto `spawn_group_reader`'s +`O_NONBLOCK` + `poll` mechanism. `spawn_reader`'s doc comment already +names this as a deferral from the compile-mode framing. It stays parked +(§5, P1) rather than riding this PR, because it is a behavioural change +to every REPL and LSP ingest path and deserves its own review. + +The honest claim for this PR is therefore: **it fixes the observed +deadlock for stdio children that honour EOF, which is what LSP servers +are, and narrows — not eliminates — the class.** + +### Q#TD4 — queued stdin writes are not lost, and the writer is not joined + +`crossbeam`'s `Receiver::recv` drains buffered items before reporting +disconnection, so dropping the `Sender` still lets the writer thread +write everything already queued. The writer thread is **not** joined +here, so there remains no guarantee the final flush completes before the +process is signalled. That is pre-existing, unchanged by this PR, and +noted rather than fixed (P3, §5). + +Draining is also the mechanism by which the fix can fail to deliver EOF +at all when the child has stopped reading — see Q#TD3's third case. + +### Q#TD5 — the leaked orphan server is not fixed here + +After the fix, the wedge is gone but a shim-launched server is still an +orphaned grandchild that teardown's recorded pid cannot signal. It exits +here only because it honours stdin EOF — by cooperation, not by +enforcement. A server that ignores EOF leaks. Parked (§5, P2). + +### Q#TD6 — the synthetic reproduction must model EOF-honouring, not sleeping, and needs an explicit stdin redirect + +Two distinct traps here, and this lane has now walked into **three** +vacuous reproductions, so the reasoning is recorded rather than the +conclusion alone. + +**Trap 1 — a sleeping child models the wrong defect.** +`sh -c 'sleep 300 & exit 0'` orphans a grandchild that holds the write +ends but **never reads stdin**, so closing stdin does not free it. That +reproduces a hang this fix does *not* address; it belongs to P1 (§5), not +here. + +**Trap 2 — a background job does not inherit stdin.** POSIX XCU §2.9.3: + +> If job control is disabled, the standard input of an asynchronous +> list, before any explicit redirections, shall be assigned to +> `/dev/null`. + +Job control is off in every non-interactive `sh`, so in +`sh -c 'cat & exit 0'` the background `cat` gets **`/dev/null`**, not the +inherited pipe. It EOFs immediately and exits **against the unfixed +tree** — the test would pass either way and Bet 3's revert-bite would +report VACUOUS. + +Measured on this machine (`/bin/sh` → `bash`), stdin attached to a +held-open fifo, checking the orphan's `/proc//fd/0`: + +| form | grandchild | 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. + +`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. + +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. + +--- + +## 3. Bets (falsifiable) + +1. **The reorder resolves the observed hang.** Falsified if + `m4_5_basedpyright_initializes_and_negotiates_encoding` still fails to + terminate after the change. +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`. +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. +4. **The basedpyright test passes rather than merely terminating.** The + hang is at teardown (`m4_acceptance.rs:1374`), *after* the body's + assertions, so it should now pass outright. Falsified if it terminates + with a failure — which would mean a second, independent defect. + **If falsified, stop the lane and frame that defect separately.** Do + not paper over it: "terminates" was never the goal, and a failing + assertion here is new information, not a loose end. + +--- + +## 4. Acceptance + +1. `RuntimeHandles::drop` takes `stdin` before joining readers, with a + comment naming the drop-body-before-fields trap. +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.) + - 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 + regression would reproduce the exact hazard this PR removes. + - Bound: 10s (default `grace_period` is 2s, `:927`). + - On the failure path the helper thread stays wedged and the `cat` + survives until the harness's fds close at process exit. That is + bounded and acceptable — but the test comment must **say so**, or a + future reviewer correctly flags a leaked thread as a defect. +3. The bite is demonstrated by revert, and the result recorded in the PR + body — pass/fail both ways, per Bet 3. +4. `cargo test --test m4_acceptance` runs **without** + `-- --skip basedpyright` and completes, locally, on the machine where + it currently hangs 5/5. +5. Docs, in **both** places the superseded cause lives — replacing it, + not appending to it: + - `docs/agent-handoff.md` §5 gains the drop-body-before-fields lesson + and the corrected cause, replacing "no timeout on the initialize + handshake". + - `docs/agent-handoff.md` §3's machine caveat currently says the + desktop's **local binary is broken and hangs**. §1.7 shows the + binary was never broken: the shim architecture plus this defect + was. Left alone, §3 keeps steering readers toward a false model — + and toward keeping the skip forever. + +**Deliberately not a criterion:** removing `-- --skip basedpyright` from +`CLAUDE.md`'s standing gate list. It is a separate call that is the +user's to make, and it changes only *local* behaviour — CI skips the test +regardless (§1.8). I will propose it with evidence after the fix has been +green repeatedly, rather than fold a process change into a defect fix. + +When that proposal comes it owes two things beyond the green runs: the +`docs/agent-handoff.md` §3 caveat updated (criterion 5 covers it here, +but the *skip* rationale lives with it), and an explicit note that +`PMACS_REQUIRE_PYRIGHT` stays **unarmed** in CI until the per-test +timeout lane (3a) merges — the ordering #194 established, where presence +of the variable decides execution and arming without a timeout would give +CI the same unbounded hang this PR removes locally. + +--- + +## 5. Parked (each needs its own evidence) + +- **P1 — cancellable non-group `read`.** Move `spawn_reader` onto + `spawn_group_reader`'s `O_NONBLOCK` + `poll` mechanism so `cancel` is + observed within `READER_SEND_POLL_INTERVAL` (`:421`, 50ms) even + mid-`read`. Bounds teardown unconditionally, including for children + that ignore EOF (Q#TD3) — **and** the blocked-writer case, where EOF is + never delivered because `write_all` is stuck on a full pipe. Already + named as a deferral by `spawn_reader`'s own doc comment. Tests: the + `sleep 300` shape from Q#TD6 (child never reads stdin), plus a + fill-the-pipe-then-stop-reading shape for the writer case. +- **P2 — orphaned-grandchild lifecycle (Q#TD5).** Spawn stdio servers in + their own process group and signal the group, reusing the machinery the + group path and `reap_ledger` already have. Fixes a real leak: every + basedpyright-backed session currently leaves a `node` process behind. +- **P3 — join the stdin writer thread** so the final flush is ordered + against child termination (Q#TD4). +- **P4 — re-audit the "intermittent" label** in `docs/agent-handoff.md` + and the audit now that §1.7 explains it. Rides this PR's doc update + only insofar as criterion 5 requires; a broader sweep is separate. + +--- + +## 6. Gates + +Per `CLAUDE.md`, each as its own step with a real exit status checked +(never `cmd | tail` — a pipe returns the tail's status and has masked a +real failure here before): + +- `cargo fmt --check` +- `cargo clippy --workspace --all-targets -- -D warnings` +- `cargo test --lib` +- `cargo test --lib --features crdt` +- `cargo test --test m4_acceptance` — **without** the basedpyright skip +- The PTY/REPL suites named in Bet 2 +- `cargo test --test worker_shutdown_acceptance` +- `PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu` +- `git diff --check` + +Commit before gating, so the results describe the pushed tree. + +--- + +## 7. Branch plan + +One branch, one PR: `process-teardown-stdin-deadlock`, from `main` @ +`e003b81` or later. Worktree `pmacs-hang` (already clean at that SHA). + +Small diff — the reorder, one unit test, one comment, the handoff +update. P1–P4 do not ride it. + +`docs/active-work.md` is integrated **late**, immediately before pushing, +to avoid the ledger-contention treadmill with the other open lanes. diff --git a/src/process.rs b/src/process.rs index 9e02629..d5555ec 100644 --- a/src/process.rs +++ b/src/process.rs @@ -636,6 +636,26 @@ impl Drop for RuntimeHandles { // channel because the consumer fell behind. Cancel flag // unwedges that case before we join. T M6.2. self.cancel.store(true, Ordering::Relaxed); + // Close the child's stdin BEFORE joining. `cancel` covers a + // reader stuck in `send`; it does NOT cover one stuck in + // `read`, which is only consulted between reads. What actually + // unblocks that reader is the child exiting and closing its + // output pipe --- and a stdio child exits on stdin EOF. + // + // The premise in the comment above ("dropping the master + // closes the kernel pipe") holds for a PTY master but NOT for + // pipe mode, where `read` unblocks only once *every* write end + // closes. An escaped descendant holding one (a shim-launched + // language server that orphans its real process) keeps the + // reader blocked indefinitely. + // + // The sink lives in the `stdin` FIELD, and a type's `Drop::drop` + // body runs before *all* of its fields regardless of their + // declaration order --- so reordering the struct cannot fix + // this. Joining first deadlocks against the very EOF that would + // have ended the join. `take()` is idempotent, matching + // `close_stdin`. + let _ = self.stdin.take(); for h in std::mem::take(&mut self.readers) { let _ = h.join(); } @@ -3204,6 +3224,141 @@ mod tests { handle.join().expect("test thread should exit cleanly"); } + /// The stdin sink lives in a *field* of [`RuntimeHandles`], so it + /// cannot drop until `Drop::drop`'s body returns --- and a type's + /// drop body runs before *all* of its fields, whatever their + /// declaration order (so reordering the struct cannot fix this). + /// Joining readers inside that body therefore deadlocks against any + /// child that exits on stdin EOF while still holding the output + /// pipe: no EOF, so no exit, so no pipe close, so a blocking + /// `spawn_reader` never returns. + /// + /// This is the root cause of + /// `m4_5_basedpyright_initializes_and_negotiates_encoding` hanging + /// forever. Modelled with an orphaned grandchild, which is exactly + /// what a shim-launched language server is: the basedpyright + /// 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`. + /// + /// Linux-gated deliberately rather than incidentally: the controls + /// read `/proc`, and the reproduction depends on `sh` async-list + /// semantics. + /// + /// On the failure path this leaks a wedged worker thread, and `cat` + /// survives until the harness's fds close at process exit. Bounded + /// and intentional --- a test that *hung* on regression would + /// reproduce the very hazard it exists to catch. + #[cfg(target_os = "linux")] + #[test] + fn teardown_closes_stdin_before_joining_readers() { + use std::sync::mpsc; + + /// `sh` becomes a zombie when it exits, because this test + /// deliberately never ticks (a tick runs `poll_one`, which is + /// the teardown path under test). `kill(pid, None)` succeeds on + /// a zombie, so liveness has to come from the process state + /// rather than from signal 0. + fn reaped_or_zombie(pid: u32) -> bool { + match std::fs::read_to_string(format!("/proc/{pid}/stat")) { + Err(_) => true, + Ok(s) => s + .rsplit_once(')') + .and_then(|(_, rest)| rest.split_whitespace().next()) + .is_some_and(|state| state == "Z"), + } + } + + 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()]; + // The default, restated because it is the whole point: with + // `StdinMode::Null` there is no sink to drop and no EOF to + // deliver. + spec.stdin = StdinMode::Piped; + let id = sup.spawn(spec).expect("spawn"); + + let sh_pid = sup + .processes + .get(&id) + .and_then(|p| p.runtime.as_ref()) + .map(|rt| rt.pid) + .expect("runtime records the spawned pid"); + + // 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. + let deadline = Instant::now() + Duration::from_secs(5); + while Instant::now() < deadline && !reaped_or_zombie(sh_pid) { + std::thread::sleep(Duration::from_millis(10)); + } + assert!( + reaped_or_zombie(sh_pid), + "control 1 failed: the recorded child (`sh`) should exit \ + promptly, leaving the grandchild orphaned. While `sh` is \ + alive it holds the output pipe itself, so control 2 would \ + pass without the grandchild modelling anything" + ); + + // 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. + let readers = sup + .processes + .get(&id) + .and_then(|p| p.runtime.as_ref()) + .map(|rt| { + ( + rt.readers.len(), + rt.readers.iter().filter(|h| !h.is_finished()).count(), + ) + }) + .expect("runtime still present before teardown"); + assert_eq!( + readers, + (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`" + ); + + // The deadlock, if present, is here: + // shutdown -> tick -> poll_one -> RuntimeHandles::drop -> join. + drop(sup); + let _ = done_tx.send(()); + }); + + done_rx.recv_timeout(Duration::from_secs(10)).expect( + "supervisor drop should complete within 10s --- if hung, \ + `RuntimeHandles::drop` is joining its readers before dropping \ + the `stdin` field, so the child never receives EOF, never \ + exits, and never closes the output pipe the readers are \ + blocked on", + ); + handle.join().expect("test thread should exit cleanly"); + } + // ----------------------------------------------------------------- // Compile-mode group lifecycle (Q#CM3; framing acceptance 34) // ----------------------------------------------------------------- From 36a37f60861b4c14898477037f6be4836422f561 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 19:29:31 -0400 Subject: [PATCH 2/5] docs(active-work): add the lane-4 process-teardown deadlock entry Integrated late, immediately before push, per the ledger-contention rule. Records the measured base, the recovery command, the defect, the reproduce-first diagnosis method, the full gate table with the revert-verified bite, and what is deliberately parked. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/active-work.md | 57 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/docs/active-work.md b/docs/active-work.md index 334ef3a..10b14d5 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -953,6 +953,63 @@ has **no branch and no framing yet**. at 42 insertions against 88 deletions — merging it would *revert* current documentation. The section said "whoever confirms the branch carries nothing unique removes the section"; this is that. + +## Test-improvement arc, lane 4 — process teardown stdin deadlock + +- Portable branch: `githubsucks/process-teardown-stdin-deadlock`, + worktree `../pmacs-hang`. Implements + `docs/process-teardown-stdin-deadlock-framing.md` (rev 2, one review + round). +- **Base, measured rather than quoted:** + + ``` + $ git log --oneline -1 githubsucks/main + e003b81 Merge pull request #190 from levineuwirth/resource-op-delete-guard-impl + ``` + +- Recovery from a clean checkout: + `git fetch githubsucks && git worktree add ../pmacs-hang + -b process-teardown-stdin-deadlock + githubsucks/process-teardown-stdin-deadlock`. +- **The defect:** `RuntimeHandles::drop` joined its reader threads in + the `Drop` **body**, which runs before any field drops. The + `ChildStdin` sink lives in the `stdin` **field**, so it could only be + released after the join returned — and the join waited on readers + blocked in `read()` on pipes whose write ends the child still held, + because the child never got the stdin EOF that would have made it + exit. A closed cycle inside one function; teardown hung forever. +- **This is the root cause of the `m4_5_basedpyright` hang** that has + parked `--workspace` sweeps (once for 2h26m) and forced + `-- --skip basedpyright` into every gate recipe. The handoff's §3 + claim that the desktop's binary was broken is **retired by this PR**: + the binary was fine. `basedpyright-langserver` is a uv console script + that spawns bundled `node` and exits, so the real server is an + orphaned grandchild (`PPid: 1`) holding the pipes; a direct binary + like `clangd` is a genuine child whose pipes close on reap. That is + the whole of the "intermittent" story. +- **Diagnosis method, because reproduce-first was the instruction:** + gdb thread stacks plus `/proc` fd forensics on a live wedged process, + both pipe ends identified in both processes, reproduced 5/5. Three + earlier reproductions were vacuous — see the handoff §5 lesson; the + shipped test carries two positive controls because of it. +- Verification (each gate its own step, real exit status, no + `cmd | tail`): fmt 0; `git diff --check` 0; clippy 0; `--lib` 1864 + passed; `--lib --features crdt` 2049 passed; **`m4_acceptance` + without the skip 150 passed in 2.60s with the basedpyright test + `ok`**; the ten PTY/REPL/worker suites of the framing's Bet 2 all 0 + (98 tests); `PMACS_REQUIRE_GPU=1 -p pmacs-gpu` 202 passed. Bite + verified by revert: `ok` in 2.03s with the fix, FAILED on timeout at + 10.00s without it, both controls passing first. +- **Not fixed here, parked in the framing §5:** cancellable non-group + `read` (covers a child that ignores EOF, and one that stops draining + while `write_all` is blocked); the orphaned-server **leak** — post-fix + the server exits by cooperation, not enforcement. +- `CLAUDE.md`'s `--skip basedpyright` entry is deliberately untouched. + Dropping it is a separate proposal owed evidence of repeated green, + and it must not precede the per-test timeout lane — + `PMACS_REQUIRE_PYRIGHT` stays unarmed in CI until then, or CI inherits + the unbounded hang this PR removes locally. + ## Parked lane: kill-ring browser + persistence - Portable branch: `githubsucks/kill-ring-browser` From 9b1cf3d6c98791ef0be184d27e1a520792ef2868 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 20:58:52 -0400 Subject: [PATCH 3/5] 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: From d5a1dc2660a82c7808a38dda22a615d46436b145 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 21:09:35 -0400 Subject: [PATCH 4/5] docs(active-work): record the CI-falsified reproduction and the acc28 flake Round 2 facts for the lane-4 entry: the dash/bash divergence that falsified the `<&0` form and how the positive control caught it, the eleven-suite Bet 2 result, and the evidence that acc28 on macos/lua54 was a flake -- a rerun of the same job on the identical head, not an assumption. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/active-work.md | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 10b14d5..a195d9f 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -995,11 +995,28 @@ has **no branch and no framing yet**. - Verification (each gate its own step, real exit status, no `cmd | tail`): fmt 0; `git diff --check` 0; clippy 0; `--lib` 1864 passed; `--lib --features crdt` 2049 passed; **`m4_acceptance` - without the skip 150 passed in 2.60s with the basedpyright test - `ok`**; the ten PTY/REPL/worker suites of the framing's Bet 2 all 0 - (98 tests); `PMACS_REQUIRE_GPU=1 -p pmacs-gpu` 202 passed. Bite - verified by revert: `ok` in 2.03s with the fix, FAILED on timeout at - 10.00s without it, both controls passing first. + without the skip 150 passed in 2.66s with the basedpyright test + `ok`**; the **eleven** PTY/REPL/worker/panel suites of the framing's + Bet 2 all 0 (144 tests); `PMACS_REQUIRE_GPU=1 -p pmacs-gpu` 202 + passed. Bite verified by revert: `ok` in 2.03s with the fix, FAILED on + timeout at 10.00s without it, both controls passing first. +- **CI round 1 falsified the reproduction, and the control is what + caught it.** Three Test legs failed on `9b1cf3d`'s predecessor: the + synthetic child used `sh -c 'cat <&0 & exit 0'`, and `<&0` does not + defeat the `/dev/null` rule it was chosen for — the rule applies + *before explicit redirections*, so fd 0 is already `/dev/null` and the + redirect duplicates it onto itself. `bash` skips the default when a + stdin redirect is present; **`dash`, which is Ubuntu's and CI's + `/bin/sh`, does not.** Local probing through `/bin/sh` could not see + it. Now `setsid --fork cat`, with no shell at all. **Lesson recorded in + the handoff §5: never probe shell behaviour through `/bin/sh` — name + the implementation.** +- **`acc28` on macos/lua54 was a flake, established not assumed.** + `bottom_panel_stage1_acceptance::acc28` failed once on that leg; + rerunning the same job on the *identical* head passed, and the suite is + 46/46 locally. It is now in Bet 2's falsifier list — its absence from + rev 1 was a real gap, since it drives real child input through a PTY in + a panel and this PR changes PTY-mode teardown ordering. - **Not fixed here, parked in the framing §5:** cancellable non-group `read` (covers a child that ignores EOF, and one that stops draining while `write_all` is blocked); the orphaned-server **leak** — post-fix From ed544fab41563c1f10af2ef0c27541739b0e5b6c Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 22:54:06 -0400 Subject: [PATCH 5/5] fix(process): arm the setsid dependency, correct the orphan account Review round 2, three findings. setsid is util-linux, not coreutils, and the standard `cargo test --lib` gate must not hard-fail on a tool the README does not declare -- a minimal or BusyBox container would fail without ever testing pmacs. The hard assert becomes skip-unless-armed via PMACS_REQUIRE_SETSID, which is the pattern the silent-skip lane already established, so the test cannot quietly report `ok` having never run where the tool is guaranteed. CI arms it on Linux; README declares it. Both arms verified against a PATH with setsid genuinely removed: unarmed skips with its message, armed FAILS with the diagnostic. The durable causal account was wrong, and this corrects it in the framing, the handoff and the ledger. basedpyright's console script runs bundled node through `subprocess.run` and WAITS (nodejs_wheel/executable.py:50, verified in the installed 1.39.6). It does not exit at spawn. What orphans node is pmacs: `shutdown()` SIGTERMs the recorded pid -- the Python wrapper -- which dies without forwarding the signal, leaving node at PPid 1 holding the pipes. The refutation was already in hand: the initialize handshake succeeds, which a wrapper that exited at spawn could not have done, and the PPid 1 observation was taken after shutdown had killed it. The fix is unaffected -- the deadlock and its bite are unchanged -- but the parked follow-up changes target: not "tolerate servers that self-orphan" but "stop orphaning them", i.e. signal the process group rather than a wrapper pid that swallows the signal. Framing section 5 P2 restated. Also corrects a stale CI-ordering claim: the handoff said pyright must stay unarmed until the timeout lane lands, but #195 is this PR's base and gave every job a timeout-minutes. The one live reason is that CI does not install basedpyright at all. The ci.yml comment asserting the job has no timeout-minutes was stale for the same reason and is rewritten. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- .github/workflows/ci.yml | 23 +++++--- README.md | 7 +++ docs/active-work.md | 33 +++++++++--- docs/agent-handoff.md | 33 ++++++++---- ...process-teardown-stdin-deadlock-framing.md | 54 +++++++++++++++---- src/process.rs | 32 +++++++---- 6 files changed, 138 insertions(+), 44 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b2bb834..c9a9983 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -203,18 +203,27 @@ jobs: # render job. Set only where the install step ran. # # PMACS_REQUIRE_PYRIGHT is deliberately NOT set and basedpyright - # is deliberately NOT installed: that test has no timeout and - # hangs forever (root cause is the non-interruptible reader-thread - # join in `RuntimeHandles::drop`, already a named deferral in - # `src/process.rs`). This job has no `timeout-minutes`, so arming - # it today would trade a vacuous green for a six-hour hang on four - # legs. It gets armed after the hang fix and the CI timeouts land, - # and its own variable exists so that flip is one line. + # is deliberately NOT installed. Both original reasons are now + # gone: the hang's root cause was the stdin-field drop ordering in + # `RuntimeHandles::drop` and is fixed, and this job now carries + # `timeout-minutes`, so a hang could no longer burn six hours. + # The ONE remaining reason is the plain one --- basedpyright is not + # installed here, so arming the variable would fail rather than + # test anything. Installing it (a uv + bundled-node download on + # every leg) is its own decision, not a rider on the hang fix. + # + # PMACS_REQUIRE_SETSID arms the teardown-deadlock unit test. Its + # fixture orphans a grandchild with `setsid --fork`, which is + # util-linux rather than coreutils, so the test skips when the + # 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. - 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' || '' }} - run: cargo test --doc --no-default-features --features ${{ matrix.lua }} # The workspace default member is only the root `pmacs` package, so # the runs above never execute pmacs-protocol's own tests — the diff --git a/README.md b/README.md index 515c259..cd91a87 100644 --- a/README.md +++ b/README.md @@ -220,6 +220,13 @@ translation) are routed through trampolines that exec these tools. shell-locator helper to find `bash` / `zsh` / `fish` for per-shell integration tests. The M7.2 fetcher's timeout test uses `sleep`. +- **`setsid`** (util-linux, Linux only, **optional**). The process + teardown-deadlock test uses `setsid --fork` to orphan a grandchild, + which is the only way to reproduce that deadlock without depending on + shell `&` semantics (they differ between `bash` and `dash`). The test + **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. - **`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 5b95e5a..bb94cc2 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -1069,10 +1069,19 @@ has **no branch and no framing yet**. `-- --skip basedpyright` into every gate recipe. The handoff's §3 claim that the desktop's binary was broken is **retired by this PR**: the binary was fine. `basedpyright-langserver` is a uv console script - that spawns bundled `node` and exits, so the real server is an - orphaned grandchild (`PPid: 1`) holding the pipes; a direct binary - like `clangd` is a genuine child whose pipes close on reap. That is - the whole of the "intermittent" story. + that runs bundled `node` via `subprocess.run` and **waits**; at + teardown `shutdown()` SIGTERMs the recorded pid (the wrapper), which + dies without forwarding, and **that** orphans node to `PPid: 1` + holding the pipes. A direct binary like `clangd` is a genuine child + whose pipes close on reap. That is the whole of the "intermittent" + story. +- **Corrected in review round 2:** rev 1–3 said the wrapper "spawns node + and exits". Wrong — and refutable from evidence already in hand, since + the initialize handshake succeeds, which a wrapper that exited at spawn + could not have done. The `PPid: 1` observation was taken *after* + `shutdown()` had killed the wrapper. **We create the orphan.** The fix + is unaffected; the parked follow-up changes from "tolerate + self-orphaning servers" to "stop orphaning them" (signal the group). - **Diagnosis method, because reproduce-first was the instruction:** gdb thread stacks plus `/proc` fd forensics on a live wedged process, both pipe ends identified in both processes, reproduced 5/5. Three @@ -1108,10 +1117,18 @@ has **no branch and no framing yet**. while `write_all` is blocked); the orphaned-server **leak** — post-fix the server exits by cooperation, not enforcement. - `CLAUDE.md`'s `--skip basedpyright` entry is deliberately untouched. - Dropping it is a separate proposal owed evidence of repeated green, - and it must not precede the per-test timeout lane — - `PMACS_REQUIRE_PYRIGHT` stays unarmed in CI until then, or CI inherits - the unbounded hang this PR removes locally. + Dropping it is a separate proposal owed evidence of repeated green. + The timeout precondition is **already satisfied** — #195 (this PR's + base) gave every job a `timeout-minutes` — so the only remaining reason + `PMACS_REQUIRE_PYRIGHT` stays unarmed is that CI does not install + basedpyright at all; arming it would fail rather than test anything. +- Adds `PMACS_REQUIRE_SETSID`, armed on Linux. The teardown test's + fixture needs `setsid --fork`, which is util-linux rather than + coreutils, so it **skips** when absent (the standard `--lib` gate must + not hard-fail a minimal container on an undeclared tool) and the + variable makes that skip fatal where the tool is guaranteed. Both arms + verified against a PATH with `setsid` genuinely removed: unarmed skips, + armed FAILS. README's test-dependency list declares it. ## Parked lane: kill-ring browser + persistence diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 5a1d4a0..2c39a36 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -1292,18 +1292,29 @@ before trusting them: - **basedpyright**: the desktop binary was **never broken** — this was a real code defect, diagnosed and fixed 2026-07-29 (see §5, "A `Drop` body runs before its fields"). `RuntimeHandles::drop` joined its reader - threads before the `stdin` field dropped, so a shim-launched server - (basedpyright's console script spawns bundled `node` and exits, leaving - the real server at `PPid 1`) never got stdin EOF, never exited, and - kept the output pipe the readers were blocked on. Deterministic on the - desktop, invisible on the laptop and in CI, which is why it read as a - broken local binary for weeks. + threads before the `stdin` field dropped, so the server never got stdin + EOF, never exited, and kept the output pipe the readers were blocked + on. Deterministic on the desktop, invisible on the laptop and in CI, + which is why it read as a broken local binary for weeks. + **How the orphan is actually made — WE make it.** basedpyright's + console script runs bundled `node` through `subprocess.run` and + **waits** (`nodejs_wheel/executable.py:50`, verified in 1.39.6). At + teardown `shutdown()` SIGTERMs the *recorded* pid — the Python wrapper + — which dies without forwarding the signal, orphaning node to `PPid 1` + holding the pipes. An earlier revision of this entry said the wrapper + "spawns node and exits"; that was wrong, and the refutation was already + in hand, since the initialize handshake succeeds, which a + wrapper that exited at spawn could not have done. The consequence is + for the follow-up, not the fix: the orphan-management work is **stop + orphaning them** (signal the group), not tolerate self-orphaning. The `--skip` above stays for now: it is still correct on any tree - predating the fix, and CI never installs basedpyright at all - (`PMACS_REQUIRE_PYRIGHT` is deliberately unarmed, #194, and stays that - way until the per-test timeout lane lands — arming it without a timeout - would hand CI an unbounded hang). Dropping the skip is a separate - proposal, owed evidence of repeated green runs. + predating the fix, and — the one live reason — **CI never installs + basedpyright at all**, so arming `PMACS_REQUIRE_PYRIGHT` would fail + rather than test anything. The two original reasons are both gone: the + hang is fixed, and #195 gave every job a `timeout-minutes`, so a hang + can no longer burn six hours. Installing basedpyright in CI (a uv plus + bundled-node download per leg) and dropping the local skip are two + separate proposals, each owed its own evidence. - **GPU on the laptop**: AMD Radeon 780M (RADV) — native Vulkan, `PMACS_REQUIRE_GPU=1` works without lavapipe. - **Flaky-under-load tests — rerun isolated before treating a sweep diff --git a/docs/process-teardown-stdin-deadlock-framing.md b/docs/process-teardown-stdin-deadlock-framing.md index 246f9b3..d21edb9 100644 --- a/docs/process-teardown-stdin-deadlock-framing.md +++ b/docs/process-teardown-stdin-deadlock-framing.md @@ -32,6 +32,14 @@ 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 4** — review round 2. §1.7's causal account was **wrong**: the + basedpyright wrapper uses `subprocess.run` and *waits*; the orphan is + created by pmacs SIGTERMing the wrapper at shutdown, not by the wrapper + exiting at spawn. Corrected here, in the handoff and in the ledger, and + §5's P2 restated — the follow-up is "stop orphaning them", not "tolerate + self-orphaning". Also: the `setsid` dependency is now skip-unless-armed + rather than a hard assert, since it is util-linux and the standard + `--lib` gate must not fail on an undeclared tool. - **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 @@ -198,12 +206,34 @@ from basedpyright.langserver import main sys.exit(main()) ``` -`main()` spawns the bundled `node …/langserver.index.js --stdio` and the -Python process exits, so the real server is an **orphaned grandchild** -(observed `PPid: 1`, reparented to systemd) holding the inherited pipe -fds. The supervisor recorded the shim's pid, which has already exited and -been reaped, so `poll_one` sees a terminated process on its very first -tick and proceeds straight into the deadlock. +`main()` reaches `run_node.run`, which calls `nodejs_wheel`'s `node(...)` +— and that is **`subprocess.run`** (`nodejs_wheel/executable.py:50`). It +**waits**. Verified in the installed 1.39.6 source, not assumed. + +So the wrapper does *not* exit at spawn time, and **pmacs creates the +orphan itself**: + +1. The wrapper runs `node …/langserver.index.js --stdio` and blocks. Node + is a genuine grandchild; the initialize handshake completes normally. +2. At teardown, `shutdown()` sends **SIGTERM to the recorded pid** — the + Python wrapper — before entering its grace loop. +3. The wrapper dies on the default disposition and **does not forward the + signal**. Node is reparented to `PPid: 1`, still holding the inherited + pipes, idle in `ep_poll`. +4. `poll_one` then observes the recorded pid terminated, drops + `RuntimeHandles`, and enters the deadlock. + +**rev 1–3 of this doc said the wrapper "spawns node and exits".** That was +wrong, and the evidence against it was already in hand: the test's +assertions all pass *before* teardown, so the handshake succeeded — which +is impossible if the wrapper had exited at spawn. The observation that +generated the claim (`PPid: 1`, wrapper gone) was taken **after** +`shutdown()` had already killed it. + +This matters for the parked work, not for the fix. The follow-up is not +"tolerate servers that self-orphan" — it is **stop orphaning them**: +signal the process group rather than a wrapper pid that swallows the +signal. P2 in §5 is restated accordingly. `clangd` and `gopls` are real binaries: genuine children, reaped normally, write ends closed, blocking `read` returns `Ok(0)` cleanly. The @@ -481,10 +511,16 @@ CI the same unbounded hang this PR removes locally. named as a deferral by `spawn_reader`'s own doc comment. Tests: the `sleep 300` shape from Q#TD6 (child never reads stdin), plus a fill-the-pipe-then-stop-reading shape for the writer case. -- **P2 — orphaned-grandchild lifecycle (Q#TD5).** Spawn stdio servers in - their own process group and signal the group, reusing the machinery the - group path and `reap_ledger` already have. Fixes a real leak: every +- **P2 — stop orphaning wrapper-launched servers (Q#TD5).** Restated in + rev 4, because the corrected §1.7 changes the target: the orphan is not + self-inflicted by the server, it is created by **us** SIGTERMing a + wrapper that does not forward the signal. Spawn stdio servers in their + own process group and signal the group, reusing the machinery the group + path and `reap_ledger` already have. Fixes a real leak: every basedpyright-backed session currently leaves a `node` process behind. + Note the ordering consequence — a group-directed SIGTERM would reach + node directly, so this also removes the condition the present fix works + around, rather than merely tolerating it. - **P3 — join the stdin writer thread** so the final flush is ordered against child termination (Q#TD4). - **P4 — re-audit the "intermittent" label** in `docs/agent-handoff.md` diff --git a/src/process.rs b/src/process.rs index bf40fcc..f0b851c 100644 --- a/src/process.rs +++ b/src/process.rs @@ -3284,15 +3284,29 @@ 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" - ); + // setsid(1) is util-linux, not coreutils, and the standard + // `cargo test --lib` gate must not hard-fail on a tool the + // README does not require --- a minimal or BusyBox container + // would fail without ever testing pmacs. So: skip when absent, + // but FAIL when `PMACS_REQUIRE_SETSID` is set, which CI sets on + // Linux. That is the arming pattern from the silent-skip lane, + // and it is what keeps this from becoming a test that reports + // `ok` having never run. Presence decides, so an empty value + // counts as unset (a `${{ cond && '1' || '' }}` expression sets + // the empty string, not nothing). + let armed = std::env::var_os("PMACS_REQUIRE_SETSID").is_some_and(|v| !v.is_empty()); + if !binary_available("setsid") { + assert!( + !armed, + "PMACS_REQUIRE_SETSID is set but setsid(1) is not on PATH: \ + install util-linux, or unset the variable to allow the skip" + ); + eprintln!( + "setsid(1) not on PATH; skipping \ + teardown_closes_stdin_before_joining_readers" + ); + return; + } let (done_tx, done_rx) = mpsc::channel(); let handle = std::thread::spawn(move || {