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 17f73fe..bb94cc2 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -953,6 +953,7 @@ 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 3a — CI timeouts and concurrency - Portable branch: `githubsucks/ci-timeouts-concurrency`, worktree @@ -1039,6 +1040,96 @@ has **no branch and no framing yet**. `git fetch githubsucks && git worktree add ../pmacs-ci3 -b ci-timeouts-concurrency githubsucks/ci-timeouts-concurrency`. +## 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 3: one review + round, then a CI round that falsified the reproduction). +- **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 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 + 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.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 + 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. + 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 - Portable branch: `githubsucks/kill-ring-browser` diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 2c5ca22..2c39a36 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -1289,10 +1289,32 @@ 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 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 — 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 @@ -1459,6 +1481,47 @@ 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 **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; 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 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 — + 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.** +- **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..d21edb9 --- /dev/null +++ b/docs/process-teardown-stdin-deadlock-framing.md @@ -0,0 +1,561 @@ +# 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. +- **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 + 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. + +--- + +## 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()` 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 +"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 | + +**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: + +| 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) | + +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. + +--- + +## 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`, + `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. +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 `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 + 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 — 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` + 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..f0b851c 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,177 @@ 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. + /// + /// `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 `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 + /// 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"), + } + } + + // 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 || { + let mut sup = ProcessSupervisor::new(); + sup.set_grace_period(Duration::from_millis(300)); + 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. + 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. (`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)); + } + 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 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) + .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` 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: + // 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) // -----------------------------------------------------------------