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 || {