fix(process): close child stdin before joining reader threads
`RuntimeHandles::drop` joined its reader threads in the `Drop` body, which runs before any field drops. The `ChildStdin` sink lives inside `StdinWriter` in the `stdin` FIELD, so it could only be released after the join returned -- and the join was waiting on readers blocked in `read()` on pipes whose write ends the child still held, because the child never received the stdin EOF that would have made it exit. A closed cycle, entirely inside one function. Teardown hung forever. This is the root cause of `m4_5_basedpyright_initializes_and_negotiates_ encoding` hanging indefinitely -- diagnosed with gdb stacks plus /proc fd forensics on a wedged process, reproduced 5/5 deterministically. It also explains why the hang looked intermittent and machine-local: a shim-launched server orphans its real process (basedpyright's console script spawns bundled `node` and exits, leaving it at `PPid 1`), so nothing teardown signals can reach it, while a direct binary like clangd or gopls is a genuine child whose pipes close on reap. `spawn_reader`'s `cancel` flag does not help: it is consulted between reads and around `send_timeout`, never while `read` is blocked. The existing comment's premise -- "dropping the master closes the kernel pipe and unblocks `read`" -- holds for a PTY master but not for pipe mode, where `read` returns only once *every* write end closes. The fix reuses `close_stdin`'s existing, already-idempotent mechanism at the one site missing it. Reordering the struct's fields cannot work: a type's `Drop::drop` body runs before all of its fields regardless of declaration order. Bounded claim: this delivers EOF, so it fixes children that drain stdin to EOF -- which stdio language servers do. A child that ignores EOF, or that stops draining while bytes are queued (the writer's `write_all` is blocking), still wedges the join. Making the `read` itself cancellable via the poll path already used by `spawn_group_reader` is the standing deferral that covers those, and is deliberately not in this change. Test: `teardown_closes_stdin_before_joining_readers`, in `--lib` so it runs in the standard gate. It models the real shape with an orphaned grandchild, and carries two positive controls, because this lane wrote three reproductions that passed against the unfixed tree before one bit. The `<&0` redirect is load-bearing: POSIX XCU 2.9.3 assigns `/dev/null` to an asynchronous list's stdin when job control is off, so a bare `cat &` exits immediately and proves nothing. Teardown runs on a worker thread behind `recv_timeout` so a regression FAILS in 10s rather than hanging -- a hanging test would reproduce the hazard being removed. Bite verified by revert: with the fix `ok` in 2.03s; with the single `stdin.take()` line commented out, FAILED at 10.00s on the timeout, both controls having passed first. Docs: framing doc added; handoff gains the drop-body-before-fields lesson and the reproduction-needs-a-control generalization, and its section 3 caveat is corrected -- the desktop's basedpyright binary was never broken. The `--skip basedpyright` gate entry stays for now; dropping it is a separate proposal owed evidence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T
This commit is contained in:
parent
e003b81cdd
commit
eb9b36387c
|
|
@ -1289,10 +1289,21 @@ git diff --check
|
|||
Machine-specific caveats — re-verify on a machine you haven't used
|
||||
before trusting them:
|
||||
|
||||
- **basedpyright**: the DESKTOP's local binary is broken and HANGS the
|
||||
`m4_5_basedpyright` tests — hence the `--skip` there. The LAPTOP has
|
||||
a working basedpyright 1.39.9 (verified 2026-07-10: the m4_5 test
|
||||
passes in 0.18s), so the skip is droppable on the laptop.
|
||||
- **basedpyright**: the desktop binary was **never broken** — this was a
|
||||
real code defect, diagnosed and fixed 2026-07-29 (see §5, "A `Drop`
|
||||
body runs before its fields"). `RuntimeHandles::drop` joined its reader
|
||||
threads before the `stdin` field dropped, so a shim-launched server
|
||||
(basedpyright's console script spawns bundled `node` and exits, leaving
|
||||
the real server at `PPid 1`) never got stdin EOF, never exited, and
|
||||
kept the output pipe the readers were blocked on. Deterministic on the
|
||||
desktop, invisible on the laptop and in CI, which is why it read as a
|
||||
broken local binary for weeks.
|
||||
The `--skip` above stays for now: it is still correct on any tree
|
||||
predating the fix, and CI never installs basedpyright at all
|
||||
(`PMACS_REQUIRE_PYRIGHT` is deliberately unarmed, #194, and stays that
|
||||
way until the per-test timeout lane lands — arming it without a timeout
|
||||
would hand CI an unbounded hang). Dropping the skip is a separate
|
||||
proposal, owed evidence of repeated green runs.
|
||||
- **GPU on the laptop**: AMD Radeon 780M (RADV) — native Vulkan,
|
||||
`PMACS_REQUIRE_GPU=1` works without lavapipe.
|
||||
- **Flaky-under-load tests — rerun isolated before treating a sweep
|
||||
|
|
@ -1459,6 +1470,38 @@ round-trip cannot detect a discriminant shift.
|
|||
asks you to keep. Same family as the skip-reports-`ok` lesson below
|
||||
and the double-invocation traps: **the thing that summarizes a gate
|
||||
must not be able to lose the gate's verdict.**
|
||||
- **A reproduction is a measurement, and needs its own positive control.**
|
||||
The basedpyright-hang lane wrote **three** reproductions that passed
|
||||
against the *unfixed* tree, each vacuous for a different reason: the
|
||||
child exited before the join; the child never read stdin at all; and —
|
||||
found at framing review — the child's stdin was silently rebound to
|
||||
`/dev/null`, because POSIX XCU §2.9.3 assigns `/dev/null` to an
|
||||
asynchronous list's stdin when job control is off, so `sh -c 'cat &
|
||||
exit 0'` EOFs instantly (the fix is an explicit `<&0` redirect). Every
|
||||
one looked obviously right when written. Note what a narrower rule
|
||||
would have missed: "check the child is still alive" catches only the
|
||||
first. Only the general form catches all three — **and the ones nobody
|
||||
has invented yet.** So: assert the precondition your reproduction
|
||||
depends on, in the test, before exercising the thing under test. In
|
||||
`teardown_closes_stdin_before_joining_readers` that is two controls
|
||||
(the recorded child has exited; both readers are still blocked in
|
||||
`read`), each with a failure message naming what its absence means.
|
||||
This is the same rule that produced #192's bite positive control and
|
||||
#194's re-read-the-artifact lesson, stated at full generality: **a
|
||||
measurement you have not controlled is a claim, not evidence.**
|
||||
- **A `Drop` body runs before its fields, whatever the declaration
|
||||
order.** Cost a multi-week misattribution: `RuntimeHandles::drop`
|
||||
joined its reader threads in the drop *body*, while the `stdin` sink it
|
||||
needed to close first sat in a *field* — reachable only after that body
|
||||
returned. The child never got EOF, never exited, and kept the output
|
||||
pipe the readers were blocked on, so teardown hung forever. Reordering
|
||||
the struct's fields cannot fix this shape; the operation has to move
|
||||
into the body. Generally: **if a `Drop` body waits on anything, check
|
||||
what the waited-on party needs that only a field drop will release.**
|
||||
Corollary from the same investigation — `cancel`-flag style wake-outs
|
||||
only work where the thread actually polls them; a thread blocked in a
|
||||
raw `read` never sees one, so a flag next to a blocking syscall is
|
||||
documentation, not a mechanism.
|
||||
- **A test that skips on a missing precondition reports `ok`, and a gate log
|
||||
cannot tell that apart from a pass.** `vterm_stage3_acceptance::a37` — the
|
||||
only acceptance driving a real daemon, a real PTY and a real wgpu render
|
||||
|
|
|
|||
|
|
@ -0,0 +1,491 @@
|
|||
# Framing — close child stdin before joining readers (process teardown deadlock)
|
||||
|
||||
A pipe-mode child that exits on stdin EOF can deadlock the supervisor's
|
||||
teardown forever. `RuntimeHandles::drop` joins its reader threads in the
|
||||
`Drop` body, which runs **before** the `stdin` field drops, so the child
|
||||
never receives the EOF that would make it close the very pipe write ends
|
||||
those readers are blocked on. The fix is a two-line reorder that reuses a
|
||||
mechanism already present in this file.
|
||||
|
||||
This is the diagnosed root cause of
|
||||
`m4_5_basedpyright_initializes_and_negotiates_encoding` hanging
|
||||
indefinitely — the hazard that has parked `cargo test --workspace` runs
|
||||
(once for 2h26m) and forced `-- --skip basedpyright` into every gate
|
||||
recipe.
|
||||
|
||||
**Scope: `src/process.rs` only. No protocol change. No Lua surface. No
|
||||
new primitive.**
|
||||
|
||||
---
|
||||
|
||||
## Revision history
|
||||
|
||||
- **rev 1** — initial framing. Root cause established by live diagnosis
|
||||
(gdb stacks + `/proc` fd forensics on a wedged process), reproduced
|
||||
5/5 deterministically at `e003b81`.
|
||||
- **rev 2** — review round 1. rev 1's synthetic child was **itself
|
||||
vacuous** (the third in this lane): POSIX assigns `/dev/null` to a
|
||||
background job's stdin when job control is off, so `sh -c 'cat &
|
||||
exit 0'` EOFs instantly and exits against the *unfixed* tree.
|
||||
Q#TD6 now uses the explicit-redirect form and criterion 2 gains a
|
||||
positive control. Also: Q#TD3's bound widened to cover a blocked
|
||||
stdin writer (a child that read stdin but stopped draining it),
|
||||
criterion 5 extended to `docs/agent-handoff.md` §3, Bet 4 marked as
|
||||
lane-stopping.
|
||||
|
||||
---
|
||||
|
||||
## 0. Coherence impact (COHERENCE §20)
|
||||
|
||||
This is a defect fix, not coherence work, and it should not claim
|
||||
otherwise.
|
||||
|
||||
- **Journey steps touched:** none directly. It protects the steps that
|
||||
depend on a live language server (§2 step 6 onward) from an unbounded
|
||||
teardown, but it adds no journey surface.
|
||||
- **Interaction islands added:** none.
|
||||
- **Config registry:** no new options.
|
||||
- **Background-work attribution:** unchanged. The supervisor's process
|
||||
model is untouched; only the order of two teardown operations moves.
|
||||
- **Protocol:** unchanged.
|
||||
|
||||
The one genuine coherence connection is indirect and worth stating
|
||||
plainly: the hang parks `cargo test --workspace`, which is the ratchet
|
||||
every COHERENCE priority is verified against (§19, §25). A gate that can
|
||||
hang forever degrades every other lane's evidence. That is the argument
|
||||
for doing this now rather than parking it — not a claim that it advances
|
||||
a priority.
|
||||
|
||||
---
|
||||
|
||||
## 1. Ground truth (scouted @ `e003b81`)
|
||||
|
||||
Line numbers are hints; symbols are authoritative.
|
||||
|
||||
### 1.1 The reproduction is deterministic, not intermittent
|
||||
|
||||
`docs/agent-handoff.md` and the test-improvement audit both describe this
|
||||
hang as intermittent. On a machine where `basedpyright-langserver`
|
||||
resolves to a uv-installed shim it is **completely reliable**: 5 runs, 5
|
||||
hangs, via
|
||||
|
||||
```
|
||||
cargo test --test m4_acceptance -- --exact \
|
||||
m4_5_basedpyright_initializes_and_negotiates_encoding
|
||||
```
|
||||
|
||||
§1.7 explains why it looks intermittent across machines. The practical
|
||||
consequence: this defect is directly testable, and any fix has a
|
||||
revert-bite.
|
||||
|
||||
### 1.2 The cycle, in five links
|
||||
|
||||
Observed stack of the wedged test thread (gdb, `sudo` required —
|
||||
`ptrace_scope=1`):
|
||||
|
||||
```
|
||||
tests/m4_acceptance.rs:1374 Rc<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()` spawns the bundled `node …/langserver.index.js --stdio` and the
|
||||
Python process exits, so the real server is an **orphaned grandchild**
|
||||
(observed `PPid: 1`, reparented to systemd) holding the inherited pipe
|
||||
fds. The supervisor recorded the shim's pid, which has already exited and
|
||||
been reaped, so `poll_one` sees a terminated process on its very first
|
||||
tick and proceeds straight into the deadlock.
|
||||
|
||||
`clangd` and `gopls` are real binaries: genuine children, reaped
|
||||
normally, write ends closed, blocking `read` returns `Ok(0)` cleanly. The
|
||||
"intermittency" in the handoff is not timing — it is *which server binary
|
||||
is installed how*.
|
||||
|
||||
### 1.8 Limits of the evidence
|
||||
|
||||
- The deterministic reproduction is **one machine, one server**. The
|
||||
causal chain is verified there link by link; its generality to other
|
||||
shim-launched servers is reasoned, not measured.
|
||||
- The gdb capture is a single sample of a state that was stable across a
|
||||
four-minute window and identical across two independent runs. That is
|
||||
strong for a deadlock and would be weak for a race.
|
||||
- Nothing here establishes how often the hang has fired in CI. CI never
|
||||
installs basedpyright (`PMACS_REQUIRE_PYRIGHT` is deliberately never
|
||||
set, #194), so in CI this test skips and the defect is **dark**. Every
|
||||
observation is local.
|
||||
|
||||
---
|
||||
|
||||
## 2. Decisions
|
||||
|
||||
### Q#TD1 — the fix is a reorder inside `Drop`, not a new primitive
|
||||
|
||||
```rust
|
||||
impl Drop for RuntimeHandles {
|
||||
fn drop(&mut self) {
|
||||
self.cancel.store(true, Ordering::Relaxed);
|
||||
// Close the child's stdin BEFORE joining. A stdio child exits
|
||||
// on EOF and closes its stdout/stderr write ends, and that —
|
||||
// not `cancel` — is what unblocks a reader parked in `read`
|
||||
// (`cancel` is only observed between reads and around `send`).
|
||||
// The sink lives in the `stdin` field, which cannot drop until
|
||||
// this body returns, so joining first deadlocks against it.
|
||||
let _ = self.stdin.take();
|
||||
for h in std::mem::take(&mut self.readers) {
|
||||
let _ = h.join();
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Rejected alternative: reordering the struct's *fields*. Field order does
|
||||
not help — the explicit `Drop::drop` body runs before **all** fields
|
||||
regardless of their declaration order. This is the trap that makes the
|
||||
bug non-obvious, and it belongs in the comment.
|
||||
|
||||
### Q#TD2 — the reorder is unconditional across modes
|
||||
|
||||
Applying it only to pipe+non-group would require `RuntimeHandles::drop`
|
||||
to learn which mode it is in, which it currently does not need to know.
|
||||
Closing stdin before teardown is correct in both modes, so the reorder is
|
||||
unconditional.
|
||||
|
||||
This is a uniformity change, and uniformity changes in this repo have
|
||||
made total functions partial before. It is therefore carried as a **bet
|
||||
with a named falsifier** (§3, Bet 2), not as an assumption: PTY-mode
|
||||
`stdin` is the pty *writer*, and dropping it while `pair.master` and the
|
||||
cloned reader still exist must not end the read early.
|
||||
|
||||
### Q#TD3 — the fix assumes the child drains stdin to EOF, and covers nothing outside that
|
||||
|
||||
Stated up front because it bounds the claim: the fix works by making the
|
||||
child exit. A child that never reads stdin — or reads it and ignores EOF
|
||||
— keeps its write ends open and still wedges the join.
|
||||
|
||||
There is a third member of that family, and it is not covered by the
|
||||
wording above because such a child *did* read stdin: **the EOF is only
|
||||
delivered if the writer thread reaches the end of its queue.** Its body
|
||||
is a blocking `sink.write_all(&bytes)` (`:578`), so a child that has
|
||||
stopped draining stdin while queued bytes remain blocks the writer
|
||||
indefinitely — the sink never drops, EOF never arrives, and the join
|
||||
re-wedges. This needs only a full stdin pipe buffer at teardown time, not
|
||||
a misbehaving child. For LSP teardown the queue is near-empty and the
|
||||
practical risk is nil, but the bound belongs in the claim: **the fix
|
||||
assumes the child keeps draining stdin until EOF.** A full stdin pipe
|
||||
with a non-draining child is P1's case as well.
|
||||
|
||||
Covering *that* case requires making the blocking `read` itself
|
||||
cancellable, i.e. moving non-group readers onto `spawn_group_reader`'s
|
||||
`O_NONBLOCK` + `poll` mechanism. `spawn_reader`'s doc comment already
|
||||
names this as a deferral from the compile-mode framing. It stays parked
|
||||
(§5, P1) rather than riding this PR, because it is a behavioural change
|
||||
to every REPL and LSP ingest path and deserves its own review.
|
||||
|
||||
The honest claim for this PR is therefore: **it fixes the observed
|
||||
deadlock for stdio children that honour EOF, which is what LSP servers
|
||||
are, and narrows — not eliminates — the class.**
|
||||
|
||||
### Q#TD4 — queued stdin writes are not lost, and the writer is not joined
|
||||
|
||||
`crossbeam`'s `Receiver::recv` drains buffered items before reporting
|
||||
disconnection, so dropping the `Sender` still lets the writer thread
|
||||
write everything already queued. The writer thread is **not** joined
|
||||
here, so there remains no guarantee the final flush completes before the
|
||||
process is signalled. That is pre-existing, unchanged by this PR, and
|
||||
noted rather than fixed (P3, §5).
|
||||
|
||||
Draining is also the mechanism by which the fix can fail to deliver EOF
|
||||
at all when the child has stopped reading — see Q#TD3's third case.
|
||||
|
||||
### Q#TD5 — the leaked orphan server is not fixed here
|
||||
|
||||
After the fix, the wedge is gone but a shim-launched server is still an
|
||||
orphaned grandchild that teardown's recorded pid cannot signal. It exits
|
||||
here only because it honours stdin EOF — by cooperation, not by
|
||||
enforcement. A server that ignores EOF leaks. Parked (§5, P2).
|
||||
|
||||
### Q#TD6 — the synthetic reproduction must model EOF-honouring, not sleeping, and needs an explicit stdin redirect
|
||||
|
||||
Two distinct traps here, and this lane has now walked into **three**
|
||||
vacuous reproductions, so the reasoning is recorded rather than the
|
||||
conclusion alone.
|
||||
|
||||
**Trap 1 — a sleeping child models the wrong defect.**
|
||||
`sh -c 'sleep 300 & exit 0'` orphans a grandchild that holds the write
|
||||
ends but **never reads stdin**, so closing stdin does not free it. That
|
||||
reproduces a hang this fix does *not* address; it belongs to P1 (§5), not
|
||||
here.
|
||||
|
||||
**Trap 2 — a background job does not inherit stdin.** POSIX XCU §2.9.3:
|
||||
|
||||
> If job control is disabled, the standard input of an asynchronous
|
||||
> list, before any explicit redirections, shall be assigned to
|
||||
> `/dev/null`.
|
||||
|
||||
Job control is off in every non-interactive `sh`, so in
|
||||
`sh -c 'cat & exit 0'` the background `cat` gets **`/dev/null`**, not the
|
||||
inherited pipe. It EOFs immediately and exits **against the unfixed
|
||||
tree** — the test would pass either way and Bet 3's revert-bite would
|
||||
report VACUOUS.
|
||||
|
||||
Measured on this machine (`/bin/sh` → `bash`), stdin attached to a
|
||||
held-open fifo, checking the orphan's `/proc/<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 |
|
||||
|
||||
**The faithful model is therefore `sh -c 'cat <&0 & exit 0'`.** The
|
||||
explicit redirect is what defeats the `/dev/null` assignment; it is
|
||||
load-bearing, not incidental, and must not be "simplified" away.
|
||||
|
||||
`sh` exits immediately (so `poll_one` observes termination), `cat` is
|
||||
orphaned holding the real stdin read end plus both write ends, and it
|
||||
exits on EOF exactly as a stdio language server does. Unfixed, this
|
||||
deadlocks; fixed, teardown completes.
|
||||
|
||||
Which `/bin/sh` applies the rule how varies by machine, so the redirect
|
||||
alone is not enough of a guarantee — criterion 2 carries a positive
|
||||
control (§4) so the test cannot silently degrade back into modelling the
|
||||
wrong thing on someone else's box. This is #192's lesson one level down:
|
||||
the bite needs a control, and so does the reproduction.
|
||||
|
||||
---
|
||||
|
||||
## 3. Bets (falsifiable)
|
||||
|
||||
1. **The reorder resolves the observed hang.** Falsified if
|
||||
`m4_5_basedpyright_initializes_and_negotiates_encoding` still fails to
|
||||
terminate after the change.
|
||||
2. **The reorder is safe for PTY mode.** Falsified by any regression in
|
||||
`vterm_stage1/2/3_acceptance`, `terminal_config_acceptance`,
|
||||
`terminal_copy_mode_acceptance`, `m6_4/m6_5_repl_acceptance`,
|
||||
`m6_7_scrollback_acceptance`, `m6_8_multi_repl_acceptance`, or
|
||||
`worker_shutdown_acceptance`.
|
||||
3. **The synthetic test bites.** Falsified if the new test passes with
|
||||
`let _ = self.stdin.take();` removed. This must be checked by actual
|
||||
revert, per the standing rule that a new pin needs its own bite.
|
||||
4. **The basedpyright test passes rather than merely terminating.** The
|
||||
hang is at teardown (`m4_acceptance.rs:1374`), *after* the body's
|
||||
assertions, so it should now pass outright. Falsified if it terminates
|
||||
with a failure — which would mean a second, independent defect.
|
||||
**If falsified, stop the lane and frame that defect separately.** Do
|
||||
not paper over it: "terminates" was never the goal, and a failing
|
||||
assertion here is new information, not a loose end.
|
||||
|
||||
---
|
||||
|
||||
## 4. Acceptance
|
||||
|
||||
1. `RuntimeHandles::drop` takes `stdin` before joining readers, with a
|
||||
comment naming the drop-body-before-fields trap.
|
||||
2. New unit test in `src/process.rs` (so it runs under the standard
|
||||
`cargo test --lib` gate, not only an acceptance suite):
|
||||
`teardown_closes_stdin_before_joining_readers`.
|
||||
- Spawns `sh -c 'cat <&0 & exit 0'` as a **non-group pipe** process.
|
||||
The `<&0` is load-bearing (Q#TD6) and gets a comment saying so.
|
||||
- **Positive control, before teardown starts:** assert the orphaned
|
||||
grandchild is alive *and* that its `/proc/<pid>/fd/0` is not
|
||||
`/dev/null`. Without this the test silently degrades into modelling
|
||||
the wrong thing wherever `/bin/sh` behaves differently, and reports
|
||||
green while doing it.
|
||||
- `#[cfg(target_os = "linux")]`: the control reads `/proc`, and the
|
||||
reproduction depends on `sh` async-list semantics. Gate it
|
||||
explicitly and say why, rather than letting it be incidentally
|
||||
Linux-only. (Same reasoning as the APFS gate — `cfg(unix)` would be
|
||||
wrong here.)
|
||||
- Performs the full reap-and-drop sequence on a helper thread and
|
||||
asserts completion via `recv_timeout`, so a regression **fails**
|
||||
within a bounded window instead of hanging. A test that hangs on
|
||||
regression would reproduce the exact hazard this PR removes.
|
||||
- Bound: 10s (default `grace_period` is 2s, `:927`).
|
||||
- On the failure path the helper thread stays wedged and the `cat`
|
||||
survives until the harness's fds close at process exit. That is
|
||||
bounded and acceptable — but the test comment must **say so**, or a
|
||||
future reviewer correctly flags a leaked thread as a defect.
|
||||
3. The bite is demonstrated by revert, and the result recorded in the PR
|
||||
body — pass/fail both ways, per Bet 3.
|
||||
4. `cargo test --test m4_acceptance` runs **without**
|
||||
`-- --skip basedpyright` and completes, locally, on the machine where
|
||||
it currently hangs 5/5.
|
||||
5. Docs, in **both** places the superseded cause lives — replacing it,
|
||||
not appending to it:
|
||||
- `docs/agent-handoff.md` §5 gains the drop-body-before-fields lesson
|
||||
and the corrected cause, replacing "no timeout on the initialize
|
||||
handshake".
|
||||
- `docs/agent-handoff.md` §3's machine caveat currently says the
|
||||
desktop's **local binary is broken and hangs**. §1.7 shows the
|
||||
binary was never broken: the shim architecture plus this defect
|
||||
was. Left alone, §3 keeps steering readers toward a false model —
|
||||
and toward keeping the skip forever.
|
||||
|
||||
**Deliberately not a criterion:** removing `-- --skip basedpyright` from
|
||||
`CLAUDE.md`'s standing gate list. It is a separate call that is the
|
||||
user's to make, and it changes only *local* behaviour — CI skips the test
|
||||
regardless (§1.8). I will propose it with evidence after the fix has been
|
||||
green repeatedly, rather than fold a process change into a defect fix.
|
||||
|
||||
When that proposal comes it owes two things beyond the green runs: the
|
||||
`docs/agent-handoff.md` §3 caveat updated (criterion 5 covers it here,
|
||||
but the *skip* rationale lives with it), and an explicit note that
|
||||
`PMACS_REQUIRE_PYRIGHT` stays **unarmed** in CI until the per-test
|
||||
timeout lane (3a) merges — the ordering #194 established, where presence
|
||||
of the variable decides execution and arming without a timeout would give
|
||||
CI the same unbounded hang this PR removes locally.
|
||||
|
||||
---
|
||||
|
||||
## 5. Parked (each needs its own evidence)
|
||||
|
||||
- **P1 — cancellable non-group `read`.** Move `spawn_reader` onto
|
||||
`spawn_group_reader`'s `O_NONBLOCK` + `poll` mechanism so `cancel` is
|
||||
observed within `READER_SEND_POLL_INTERVAL` (`:421`, 50ms) even
|
||||
mid-`read`. Bounds teardown unconditionally, including for children
|
||||
that ignore EOF (Q#TD3) — **and** the blocked-writer case, where EOF is
|
||||
never delivered because `write_all` is stuck on a full pipe. Already
|
||||
named as a deferral by `spawn_reader`'s own doc comment. Tests: the
|
||||
`sleep 300` shape from Q#TD6 (child never reads stdin), plus a
|
||||
fill-the-pipe-then-stop-reading shape for the writer case.
|
||||
- **P2 — orphaned-grandchild lifecycle (Q#TD5).** Spawn stdio servers in
|
||||
their own process group and signal the group, reusing the machinery the
|
||||
group path and `reap_ledger` already have. Fixes a real leak: every
|
||||
basedpyright-backed session currently leaves a `node` process behind.
|
||||
- **P3 — join the stdin writer thread** so the final flush is ordered
|
||||
against child termination (Q#TD4).
|
||||
- **P4 — re-audit the "intermittent" label** in `docs/agent-handoff.md`
|
||||
and the audit now that §1.7 explains it. Rides this PR's doc update
|
||||
only insofar as criterion 5 requires; a broader sweep is separate.
|
||||
|
||||
---
|
||||
|
||||
## 6. Gates
|
||||
|
||||
Per `CLAUDE.md`, each as its own step with a real exit status checked
|
||||
(never `cmd | tail` — a pipe returns the tail's status and has masked a
|
||||
real failure here before):
|
||||
|
||||
- `cargo fmt --check`
|
||||
- `cargo clippy --workspace --all-targets -- -D warnings`
|
||||
- `cargo test --lib`
|
||||
- `cargo test --lib --features crdt`
|
||||
- `cargo test --test m4_acceptance` — **without** the basedpyright skip
|
||||
- The PTY/REPL suites named in Bet 2
|
||||
- `cargo test --test worker_shutdown_acceptance`
|
||||
- `PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu`
|
||||
- `git diff --check`
|
||||
|
||||
Commit before gating, so the results describe the pushed tree.
|
||||
|
||||
---
|
||||
|
||||
## 7. Branch plan
|
||||
|
||||
One branch, one PR: `process-teardown-stdin-deadlock`, from `main` @
|
||||
`e003b81` or later. Worktree `pmacs-hang` (already clean at that SHA).
|
||||
|
||||
Small diff — the reorder, one unit test, one comment, the handoff
|
||||
update. P1–P4 do not ride it.
|
||||
|
||||
`docs/active-work.md` is integrated **late**, immediately before pushing,
|
||||
to avoid the ledger-contention treadmill with the other open lanes.
|
||||
155
src/process.rs
155
src/process.rs
|
|
@ -636,6 +636,26 @@ impl Drop for RuntimeHandles {
|
|||
// channel because the consumer fell behind. Cancel flag
|
||||
// unwedges that case before we join. T M6.2.
|
||||
self.cancel.store(true, Ordering::Relaxed);
|
||||
// Close the child's stdin BEFORE joining. `cancel` covers a
|
||||
// reader stuck in `send`; it does NOT cover one stuck in
|
||||
// `read`, which is only consulted between reads. What actually
|
||||
// unblocks that reader is the child exiting and closing its
|
||||
// output pipe --- and a stdio child exits on stdin EOF.
|
||||
//
|
||||
// The premise in the comment above ("dropping the master
|
||||
// closes the kernel pipe") holds for a PTY master but NOT for
|
||||
// pipe mode, where `read` unblocks only once *every* write end
|
||||
// closes. An escaped descendant holding one (a shim-launched
|
||||
// language server that orphans its real process) keeps the
|
||||
// reader blocked indefinitely.
|
||||
//
|
||||
// The sink lives in the `stdin` FIELD, and a type's `Drop::drop`
|
||||
// body runs before *all* of its fields regardless of their
|
||||
// declaration order --- so reordering the struct cannot fix
|
||||
// this. Joining first deadlocks against the very EOF that would
|
||||
// have ended the join. `take()` is idempotent, matching
|
||||
// `close_stdin`.
|
||||
let _ = self.stdin.take();
|
||||
for h in std::mem::take(&mut self.readers) {
|
||||
let _ = h.join();
|
||||
}
|
||||
|
|
@ -3204,6 +3224,141 @@ mod tests {
|
|||
handle.join().expect("test thread should exit cleanly");
|
||||
}
|
||||
|
||||
/// The stdin sink lives in a *field* of [`RuntimeHandles`], so it
|
||||
/// cannot drop until `Drop::drop`'s body returns --- and a type's
|
||||
/// drop body runs before *all* of its fields, whatever their
|
||||
/// declaration order (so reordering the struct cannot fix this).
|
||||
/// Joining readers inside that body therefore deadlocks against any
|
||||
/// child that exits on stdin EOF while still holding the output
|
||||
/// pipe: no EOF, so no exit, so no pipe close, so a blocking
|
||||
/// `spawn_reader` never returns.
|
||||
///
|
||||
/// This is the root cause of
|
||||
/// `m4_5_basedpyright_initializes_and_negotiates_encoding` hanging
|
||||
/// forever. Modelled with an orphaned grandchild, which is exactly
|
||||
/// what a shim-launched language server is: the basedpyright
|
||||
/// console script spawns bundled `node` and exits, leaving the real
|
||||
/// server at `PPid 1` holding the inherited pipes.
|
||||
///
|
||||
/// `<&0` is LOAD-BEARING, not decoration. POSIX XCU 2.9.3 assigns
|
||||
/// `/dev/null` to an asynchronous list's stdin when job control is
|
||||
/// off --- i.e. in every non-interactive `sh` --- so a bare `cat &`
|
||||
/// reads EOF immediately and exits *against the unfixed tree*,
|
||||
/// giving a test that passes either way and proves nothing. Measured
|
||||
/// on `bash`: bare `&` leaves no grandchild, `<&0` leaves one
|
||||
/// holding the real pipe. Both controls below exist to catch that
|
||||
/// silently regressing on another `/bin/sh`.
|
||||
///
|
||||
/// Linux-gated deliberately rather than incidentally: the controls
|
||||
/// read `/proc`, and the reproduction depends on `sh` async-list
|
||||
/// semantics.
|
||||
///
|
||||
/// On the failure path this leaks a wedged worker thread, and `cat`
|
||||
/// survives until the harness's fds close at process exit. Bounded
|
||||
/// and intentional --- a test that *hung* on regression would
|
||||
/// reproduce the very hazard it exists to catch.
|
||||
#[cfg(target_os = "linux")]
|
||||
#[test]
|
||||
fn teardown_closes_stdin_before_joining_readers() {
|
||||
use std::sync::mpsc;
|
||||
|
||||
/// `sh` becomes a zombie when it exits, because this test
|
||||
/// deliberately never ticks (a tick runs `poll_one`, which is
|
||||
/// the teardown path under test). `kill(pid, None)` succeeds on
|
||||
/// a zombie, so liveness has to come from the process state
|
||||
/// rather than from signal 0.
|
||||
fn reaped_or_zombie(pid: u32) -> bool {
|
||||
match std::fs::read_to_string(format!("/proc/{pid}/stat")) {
|
||||
Err(_) => true,
|
||||
Ok(s) => s
|
||||
.rsplit_once(')')
|
||||
.and_then(|(_, rest)| rest.split_whitespace().next())
|
||||
.is_some_and(|state| state == "Z"),
|
||||
}
|
||||
}
|
||||
|
||||
let (done_tx, done_rx) = mpsc::channel();
|
||||
let handle = std::thread::spawn(move || {
|
||||
let mut sup = ProcessSupervisor::new();
|
||||
sup.set_grace_period(Duration::from_millis(300));
|
||||
let mut spec = ProcessSpec::new("orphan-holds-pipe", "/bin/sh");
|
||||
// `cat` reads stdin and exits on EOF, exactly as a stdio
|
||||
// language server does. `exit 0` makes the *recorded* pid
|
||||
// terminate promptly, so `poll_one` reaches the teardown
|
||||
// path while the grandchild still holds the output pipe.
|
||||
spec.args = vec!["-c".into(), "cat <&0 & exit 0".into()];
|
||||
// The default, restated because it is the whole point: with
|
||||
// `StdinMode::Null` there is no sink to drop and no EOF to
|
||||
// deliver.
|
||||
spec.stdin = StdinMode::Piped;
|
||||
let id = sup.spawn(spec).expect("spawn");
|
||||
|
||||
let sh_pid = sup
|
||||
.processes
|
||||
.get(&id)
|
||||
.and_then(|p| p.runtime.as_ref())
|
||||
.map(|rt| rt.pid)
|
||||
.expect("runtime records the spawned pid");
|
||||
|
||||
// CONTROL 1: the recorded child must actually exit. Until it
|
||||
// does, *it* holds the output pipe, and control 2 would pass
|
||||
// for the wrong reason.
|
||||
let deadline = Instant::now() + Duration::from_secs(5);
|
||||
while Instant::now() < deadline && !reaped_or_zombie(sh_pid) {
|
||||
std::thread::sleep(Duration::from_millis(10));
|
||||
}
|
||||
assert!(
|
||||
reaped_or_zombie(sh_pid),
|
||||
"control 1 failed: the recorded child (`sh`) should exit \
|
||||
promptly, leaving the grandchild orphaned. While `sh` is \
|
||||
alive it holds the output pipe itself, so control 2 would \
|
||||
pass without the grandchild modelling anything"
|
||||
);
|
||||
|
||||
// CONTROL 2: both readers must still be blocked in `read`,
|
||||
// which is only true while something still holds the output
|
||||
// pipe's write ends. If the grandchild never inherited
|
||||
// stdin (the /dev/null rule above), it has already exited,
|
||||
// the write ends are closed, the readers have finished ---
|
||||
// and the deadlock is not being modelled at all.
|
||||
let readers = sup
|
||||
.processes
|
||||
.get(&id)
|
||||
.and_then(|p| p.runtime.as_ref())
|
||||
.map(|rt| {
|
||||
(
|
||||
rt.readers.len(),
|
||||
rt.readers.iter().filter(|h| !h.is_finished()).count(),
|
||||
)
|
||||
})
|
||||
.expect("runtime still present before teardown");
|
||||
assert_eq!(
|
||||
readers,
|
||||
(2, 2),
|
||||
"control 2 failed: both readers must still be blocked in \
|
||||
`read`, i.e. an escaped grandchild still holds the output \
|
||||
pipe. Finished readers mean `cat` never inherited stdin \
|
||||
(POSIX assigns /dev/null to a background job's stdin when \
|
||||
job control is off) and the `<&0` redirect has stopped \
|
||||
working on this `/bin/sh`"
|
||||
);
|
||||
|
||||
// The deadlock, if present, is here:
|
||||
// shutdown -> tick -> poll_one -> RuntimeHandles::drop -> join.
|
||||
drop(sup);
|
||||
let _ = done_tx.send(());
|
||||
});
|
||||
|
||||
done_rx.recv_timeout(Duration::from_secs(10)).expect(
|
||||
"supervisor drop should complete within 10s --- if hung, \
|
||||
`RuntimeHandles::drop` is joining its readers before dropping \
|
||||
the `stdin` field, so the child never receives EOF, never \
|
||||
exits, and never closes the output pipe the readers are \
|
||||
blocked on",
|
||||
);
|
||||
handle.join().expect("test thread should exit cleanly");
|
||||
}
|
||||
|
||||
// -----------------------------------------------------------------
|
||||
// Compile-mode group lifecycle (Q#CM3; framing acceptance 34)
|
||||
// -----------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Reference in New Issue