Merge pull request #197 from levineuwirth/process-teardown-stdin-deadlock
fix(process): close child stdin before joining reader threads
This commit is contained in:
GitHub
commit
f5a146ee0f
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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`
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<RefCell<ProcessSupervisor>> 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<StdinWriter>` 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/<pid>/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.
|
||||
191
src/process.rs
191
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)
|
||||
// -----------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Reference in New Issue