Repository navigation
Kill strategy for a timed-out capture #301
Description
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).
Activity
blooop commentedon Aug 22, 2026
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 asetsid'd descendant (ssh ControlMaster is the production example) escapes any group kill — the liveness guarantee cannot rest on the group.session's stderrreader.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
capturenever returns; after the fix it returnsOutcome::TimedOutwithin 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 onlykillpgin 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 commentedon Aug 22, 2026
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.rsdiffering only in this change, and refuted the premise thekillpghalf rested on:SpawnSpec::stdindefaults toInherit, 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_waitnever reports a stopped child, so the wait loop spins to its deadline. Measured under a pty:mainreturns in 1.8 ms with the prompt served; the process-group build never returns, child in stateT. Three captures pass no timeout at all —git clone --bare,git push -u, and the launch-pathgit fetch— so for those there is no deadline to rescue the hang.capture(unlikepassthrough) never callsnote_foreground_child, so dl's_exit(130)reaches nothing. Measured:mainkills the child, the process-group build leaves it running. That launch-path fetch holds the per-repo flock, which_exitreleases — leaving an unsignalledgit fetchwriting the bare cache unlocked. That re-opens the F3 orphan classinterrupt.rsexists 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 makesOutcome::TimedOutactually 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
maindoes today — and it buys back the two defects above. The tree-killing thatkillpgbought was the lesser concern; hanging forever on an untimedgit fetchbehind 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 hangscaptureforever — 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.
revert: a captured child stays in this process's group
fix: a capture returns when a grandchild holds the pipe past a clean …