From ed544fab41563c1f10af2ef0c27541739b0e5b6c Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 22:54:06 -0400 Subject: [PATCH] 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 || {