Kill strategy for a timed-out capture

[blooop]

Question

ProcessRunner::capture spawns with OwnGroup::No, SIGKILLs one pid on timeout, and joins the pipe-drain threads after the wait (rust/devlaunch-runner/src/lib.rs:421-434, kill at :818-824). read_to_end returns only at pipe EOF, so a killed child that left a grandchild holding the inherited fd blocks the join forever — Outcome::TimedOut never returns. Reachable in production: clients/git.rs uses capture-with-timeout on five verbs, and git fetch over ssh forks exactly such a grandchild. The suite dodges the shape (exec sleep 30 guarantees a single process).

Decide the fix shape: (a) spawn captures in their own process group and killpg on expiry; (b) bound the drain-thread joins and abandon them on expiry; (c) both. Also decide whether session's reader.join() (lib.rs:561-563) needs the same treatment, and name the failing test that proves the defect (a child that forks a grandchild holding the pipe).

[blooop]

human-decided: Fix shape (c) — both, as asked and as answered.

Captures spawn in their own process group and the expiry kill becomes a killpg, so the common tree (git over ssh) actually dies and output drains fully; the drain-thread joins are additionally time-bounded and abandoned on expiry, because a setsid'd descendant (ssh ControlMaster is the production example) escapes any group kill — the liveness guarantee cannot rest on the group. session's stderr reader.join() (lib.rs:561-563) gets the same bound: it shares the hang shape through its one pipe.

The failing test that proves the defect: a capture whose child forks a setsid grandchild inheriting the stdout pipe and exec-sleeps — today capture never returns; after the fix it returns Outcome::TimedOut within a bound (proposed name: a_timed_out_capture_returns_even_when_a_grandchild_holds_the_pipe). A sibling test pins the cleanup half: a same-group grandchild is dead after expiry.

Evidence gathered first: the kill path is single-pid Child::kill (lib.rs:815-824) and the only killpg in the crate lives in the SIGINT handler (interrupt.rs:168-177), so a group-kill in the timeout path is new code; five git verbs capture with 2–30 s timeouts (clients/git.rs:62-76). Alternatives ruled out: killpg-only re-admits the hang via setsid descendants; bounded-joins-only leaks the tree and loses output on every real timeout. Unblocks #302.

[blooop]

human-decided (reversal): the process-group half is dropped. Bound the drain joins on both paths, and that is the whole fix.

This reverses half of the decision recorded above. The review of PR #343 (ticket #302) A/B-measured one example binary against two builds of devlaunch-runner/src/lib.rs differing only in this change, and refuted the premise the killpg half rested on:

• The SIGTTIN hazard does apply to captures. The decision above asserted "nothing a capture runs reads the terminal". SpawnSpec::stdin defaults to Inherit, and a child outside the foreground process group takes SIGTTIN on any read of the controlling terminal — which is exactly what ssh host-key confirmation, ssh passphrase prompts and git credential prompts do via /dev/tty. try_wait never reports a stopped child, so the wait loop spins to its deadline. Measured under a pty: main returns in 1.8 ms with the prompt served; the process-group build never returns, child in state T. Three captures pass no timeout at all — git clone --bare, git push -u, and the launch-path git fetch — so for those there is no deadline to rescue the hang.
• It also orphans the child on Ctrl-C. Leaving dl's process group leaves the terminal's foreground group, and capture (unlike passthrough) never calls note_foreground_child, so dl's _exit(130) reaches nothing. Measured: main kills the child, the process-group build leaves it running. That launch-path fetch holds the per-repo flock, which _exit releases — leaving an unsignalled git fetch writing the bare cache unlocked. That re-opens the F3 orphan class interrupt.rs exists to close.

What stands instead: the bounded-join half, completed on both paths. The original reasoning survives intact and is the reason this is sufficient — a setsid'd descendant survives any kill aimed at the child, so the join could never have been the liveness guarantee, and bounding it is what makes Outcome::TimedOut actually return. The single-pid SIGKILL still reaps the direct child.

What this gives up, knowingly: a timed-out capture may leave a forked descendant running. That is not a regression — it is precisely what main does today — and it buys back the two defects above. The tree-killing that killpg bought was the lesser concern; hanging forever on an untimed git fetch behind an ssh prompt is the greater one.

Also folded into the same rework (#302), and independent of the above: the drain join was made abandonable on the timeout path only. The success path still joins unbounded, so a child that exits 0 while a setsid'd descendant holds the pipe hangs capture forever — the ControlMaster shape this ticket names, on the path far more common than a timeout. The headline hang was half-fixed.

Note on how this got here: the brief that produced the original decision asserted the SIGTTIN hazard did not apply to captures. That was wrong, and it was the agent's error rather than the human's — recorded here because the decision reads as settled otherwise.