Fix the capture and session timeout hang

[blooop]

Question

Implement the kill strategy decided in "Kill strategy for a timed-out capture", test-first: a failing test whose child forks a grandchild that holds the pipe fd open — capture must return Outcome::TimedOut within a bound instead of hanging. Cover the session reader path as that decision directs.

[blooop]

breadcrumb: entering building — wf-tdd stage subagent launched (manager: wf-mid run working map #299).

[blooop]

breadcrumb: building what #301 decided — fix shape (c) both. Seams under test, all at the Runner trait's production implementation (ProcessRunner), observed only through public behavior (capture/session outcomes and what survives on the machine):

1. a_timed_out_capture_returns_even_when_a_grandchild_holds_the_pipe — child forks a setsid grandchild inheriting the stdout pipe and exec-sleeps past its timeout; capture must return Outcome::TimedOut within a bound. Green move: the pipe-drain joins are abandoned on expiry (output is dropped on a timeout anyway).
2. a_timed_out_capture_kills_the_whole_process_group — a same-group grandchild that would leave a marker after the timeout is dead after expiry. Green move: captures spawn in their own process group and the expiry kill becomes a killpg.
3. a_timed_out_session_returns_even_when_a_grandchild_holds_stderr — same shape through session's one pipe; the stderr reader join gets the same abandon-on-expiry treatment. (session's child stays in this process's group — an interactive child moved out of the foreground group takes SIGTTIN — so the group kill applies to capture only, as the decision anticipated.)

One slice per test, red first.

[blooop]

breadcrumb: build complete, PR #343 pushed. The builder subagent was killed by a spend limit after committing all three slices; the manager ran the verification it had not reached (workspace suite 1,578 pass, clippy -D warnings, fmt) and opened the PR. No code was written outside the builder's commits.

[blooop]

breadcrumb: build → in review. PR #343 green on every check (ci, rust, rust-coverage, e2e, public-api, packaging, prek, gate, codecov). Handing to a fresh wf-review context.

[blooop]

breadcrumb: in review → needs attention. wf-review verdict Request changes on PR #343 (report). Three blocking findings, each reproduced by A/B with only lib.rs swapped: (1) capture children in their own group take SIGTTIN on any terminal read — a permanent hang for clone_bare, push_branch and the launch fetch, which pass no timeout; (2) Ctrl-C no longer reaches a capture child and nothing else kills it, orphaning the launch fetch after the repo flock is released (F3 re-opened); (3) the success-path collect() join is still unbounded, so #301's hang survives on the commoner path. (1) and (2) follow from half (a) of #301, whose premise ("nothing here reads the terminal") measurement refutes — that likely wants #301 reopened rather than a straight rework; (3) is an independent straight fix.

[blooop]

breadcrumb: in review → changes requested, routed back to building. The review of PR #343 reproduced three blocking defects by A/B-measuring one binary against two builds of lib.rs, and two of them refuted the premise behind #301's decided shape rather than the builder's execution of it.

Escalated to the human, who reversed half of #301: the process-group/killpg half is dropped; the bounded-join half is completed on both the timeout and success paths, and that is the whole fix. The full reasoning and the measurements are recorded on #301.

The third defect is independent of that reversal and undisputed: the success path still joins unbounded, so the headline hang survives on the more common path. It lands in the same PR.

This is not a builder error — the two group-related defects follow from the decided shape.

[blooop]

breadcrumb: in review → changes requested, routed back to building. The review of PR #343 reproduced three blocking defects by A/B-measuring one example binary against two builds of lib.rs differing only in this change — and two of them refuted the premise behind #301's decided shape rather than the builder's execution of it.

Escalated to the human, who reversed half of #301: the process-group/killpg half is dropped; the bounded-join half is completed on both the timeout and success paths, and that is the whole fix. Full reasoning and the measurements are on #301.

The third defect is independent of the reversal and undisputed: the drain join was made abandonable on the timeout path only, so the success path still joins unbounded and the headline hang survives on the more common path (child exits 0, setsid'd descendant holds the pipe). Human confirmed it lands in this same PR rather than a separate one.

This is not a builder error — the two group-related defects follow from the shape that was decided for it.

[blooop]

breadcrumb: reworked to the reversed decision, pushed to PR #343 (two commits on the existing branch, no force-push).

The process-group half is out: captures spawn in this process's group again and the timeout kill is single-pid, so wait/kill no longer carry which group a child leads. The bounded-join half is now the whole fix, and it is on both paths — the drained bytes live behind a lock rather than in the reading thread's return value, so what was read can be taken without joining the thread that read it, and the successful case still hands back the exit status and the output. The bound is a fixed grace after the child exits rather than the remaining timeout, because at that point the wait is only for the pipe to close — which is what makes it reach the three captures that pass no timeout at all, the ones no deadline could have rescued.

Three seams, each red before green, and two of them needed a controlling terminal the suite did not have before: a pty harness now drives a capture from inside a session that owns one, so "a captured child may read the terminal" and "a terminal interrupt reaches a captured child" are pinned by measurement rather than by prose. The third is the success-path hang: a child that exits cleanly while a descendant in a session of its own still holds the pipe.

Knowingly given up, and recorded where the kill happens: a timed-out capture may leave what the tool forked running, exactly as the default branch does. The test that pinned the opposite is deleted with the behaviour it pinned.

Per-finding reply posted on the PR. Full suite, clippy and fmt green locally.

[blooop]

breadcrumb: PR #343 green on every check after the rework (ci, rust, rust-coverage, e2e, public-api, packaging, prek, gate, codecov). Three commits on the existing branch, no force-push.

One thing the coverage job taught, worth remembering beyond this ticket: a test helper that has to be a process cannot be an example. The coverage run spawns cargo test --tests, which builds no examples, so a helper shipped that way is present under the plain test run and absent under the instrumented one. Re-executing the test binary itself in a different role cannot go missing either way, and needs no build-order promise from cargo.

Ready for review again.

[blooop]

breadcrumb: changes requested → back in review. Head 6a2579e, all 12 checks green.

All three blocking findings were reproduced by measurement before being fixed, and two of them by building the pty harness that could observe them at all:

• S1/S2 — the process-group half is reverted per the human reversal on Kill strategy for a timed-out capture #301. Pinned by a real pty: terminal.rs::a_captured_child_may_read_the_terminal re-executes the test binary as session leader and types at a capture reading /dev/tty; a_terminal_interrupt_reaches_a_captured_child sends \x03 into the pty and asserts the marker never appears. Both were re-verified red after the harness rewrite by flipping OwnGroup back to Yes, so the tests are known to be sensitive rather than merely green.
• P2 — the success-path fix avoided a trap worth recording: a bounded join returning String::new() would have converted the hang into a silently empty stdout for the callers that parse it. Instead the drained bytes live behind a Mutex so what was read can be taken without joining the reader. The bound is a fixed 500 ms grace rather than the remaining timeout, deliberately — by then the child has exited, so the wait is only for the pipe to close, and a deadline-derived bound would be None for the three captures that pass no timeout at all.

Also cleared: require_setsid() added so the grandchild tests cannot go vacuously green; three copies of the poll-with-deadline scaffolding collapsed to one helper; the OwnGroup docs are true again; the test pinning the now-reverted group behaviour is deleted.

The knowingly accepted regression — a timed-out capture may leave a forked descendant running — is documented in kill's doc comment rather than tested, correctly, since it is the default branch's own behaviour.

One process note from the builder, recorded because it is a trap for the next person: the pty helper began as examples/terminal_capture.rs and failed rust-coverage, because cargo llvm-cov spawns cargo test --tests, which builds no examples. Re-executing the test binary in a role fixes it and cannot go missing under either run.

[blooop]

breadcrumb: re-review at 6a2579e — verdict Approve (report). All three blocking findings resolved. S1/S2 by full reversion: net of the whole PR, only two doc lines mention the process group, and the two new pty tests pin the dangerous direction — verified sensitive by flipping OwnGroup back to Yes, which turns both red with the messages their names promise. P2 by a bounded, byte-preserving drain: the new success-path test is genuinely red against 00e24cc, a 1 MiB payload comes back intact, and the grace is skipped entirely (8 ms) when no descendant holds the pipe — better than the deadline-derived bound I asked for, which would derive None for the three untimed captures. Accepted regression accepted: measured identical to merge-base, and the reintroduction risk is what the pty tests guard. One new non-blocking defect: the hand-rolled read loop treats ErrorKind::Interrupted as EOF where read_to_end retried (proved "abcd" vs "ab"), silently truncating a parsed string — unreachable today since the only handler is glibc signal(SIGINT) with SA_RESTART; one-line follow-up, not a gate. Not merged, nothing pushed.