562 lines
26 KiB
Markdown
562 lines
26 KiB
Markdown
# 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.
|