Repository navigation
Fix the capture and session timeout hang #302
Description
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.
Activity
blooop commentedon Aug 22, 2026
breadcrumb: entering building — wf-tdd stage subagent launched (manager: wf-mid run working map #299).
blooop commentedon Aug 22, 2026
breadcrumb: building what #301 decided — fix shape (c) both. Seams under test, all at the
Runnertrait's production implementation (ProcessRunner), observed only through public behavior (capture/sessionoutcomes and what survives on the machine):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;capturemust returnOutcome::TimedOutwithin a bound. Green move: the pipe-drain joins are abandoned on expiry (output is dropped on a timeout anyway).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 akillpg.a_timed_out_session_returns_even_when_a_grandchild_holds_stderr— same shape throughsession'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 commentedon Aug 22, 2026
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 commentedon Aug 22, 2026
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 commentedon Aug 22, 2026
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.rsswapped: (1) capture children in their own group take SIGTTIN on any terminal read — a permanent hang forclone_bare,push_branchand 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-pathcollect()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 commentedon Aug 22, 2026
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 commentedon Aug 22, 2026
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.rsdiffering 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 commentedon Aug 22, 2026
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/killno 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 commentedon Aug 22, 2026
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 commentedon Aug 22, 2026
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:
terminal.rs::a_captured_child_may_read_the_terminalre-executes the test binary as session leader and types at a capture reading/dev/tty;a_terminal_interrupt_reaches_a_captured_childsends\x03into the pty and asserts the marker never appears. Both were re-verified red after the harness rewrite by flippingOwnGroupback toYes, so the tests are known to be sensitive rather than merely green.String::new()would have converted the hang into a silently empty stdout for the callers that parse it. Instead the drained bytes live behind aMutexso 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 beNonefor 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; theOwnGroupdocs 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.rsand failedrust-coverage, becausecargo llvm-covspawnscargo test --tests, which builds no examples. Re-executing the test binary in a role fixes it and cannot go missing under either run.blooop commentedon Aug 22, 2026
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 flippingOwnGroupback toYes, 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 against00e24cc, 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 deriveNonefor 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 treatsErrorKind::Interruptedas EOF whereread_to_endretried (proved "abcd" vs "ab"), silently truncating a parsed string — unreachable today since the only handler is glibcsignal(SIGINT)withSA_RESTART; one-line follow-up, not a gate. Not merged, nothing pushed.blooop commentedon Aug 22, 2026
breadcrumb: in review → approved, awaiting human merge. All three blocking findings resolved at head 6a2579e; every check green.
The reviewer verified rather than read: it checked the reversion structurally (net of the whole PR, the only lines matching
OwnGroup|killpg|setpgid|fn wait|fn killare two doc comments), and it confirmed the new pty tests' sensitivity itself by flippinglib.rs:435back toOwnGroup::Yesand watching both go red with the messages their names promise.It withdrew the bound it had asked for, which is worth recording as the better outcome: measurement showed the fixed
DRAIN_GRACEbeats the deadline-derived bound the review originally wanted. A 1 MiB payload with a setsid'd holder comes back intact; both pipes held costs 1.01 s; no holder costs 8 ms, so the grace is paid only in the case that previously hung forever. And after the child has exited, the remaining timeout is simply the wrong quantity — it bounds how long the tool may run, not how long its pipe takes to close, so a capture with 100 ms left of 30 s would have truncated output it already held.It also accepted the documented-not-tested regression on a better argument than the one offered: the real risk is someone later reintroducing the killpg to fix the descendant leak and silently re-breaking S1/S2 — and
tests/terminal.rsfails loudly on exactly that. Pinning the cause beats pinning one consequence.One new non-blocking defect graduated to #355: the drain loop's
Ok(0) | Err(_) => breaktreatsErrorKind::Interruptedas EOF whereread_to_endretried, giving silent truncation reported as a successfulRan— the same harm this PR's design notes cite as the reason not to returnString::new(). I checked whether the concurrent #304 work makes it reachable: it does not, because every handler in the workspace goes throughlibc::signal, which on glibc carriesSA_RESTART. So it stays a one-arm fix on its own ticket rather than a reason to reopen this one.This ticket stays open until PR #343 is merged.