fix: replace wait-timeout so a signal-handler panic can't abort wt

[worktrunk-bot]

Problem

Every timed wait in wt went through wait-timeout 0.2.1, which reaps children from a process-global SIGCHLD handler. That handler pokes a self-pipe via notify(), which panics on any write error other than WouldBlock. Because sigchld_handler is extern "C", the panic cannot unwind — it goes straight to panic_cannot_unwind → abort(). Under a sandbox that denies the send (the Codex CLI's workspace-write mode, where the write fails EPERM), wt switch --create died with SIGABRT and no diagnostic the user or wt could do anything with.

Three call sites were exposed: run_with_timeout_impl and Cmd::delayed_stream in src/shell_exec.rs, and the picker's pager in src/commands/picker/pager.rs. (picker/prs.rs uses Condvar::wait_timeout_while from std, which is unrelated.)

Solution

Drop wait-timeout and wait through shared_child instead — SharedChild::spawn plus wait / wait_timeout / kill / id / take_std* at all three sites. It adds exactly two crates to the graph (shared_child, sigchld); signal-hook, os_pipe, libc and windows-sys were already there.

shared_child does reach SIGCHLD on Unix — there is no "wait for this child with a deadline" syscall, so a timed wait is waitid(WNOWAIT) plus a SIGCHLD self-pipe polled against the deadline. The difference from wait-timeout is what its handler does on a failed wake: it goes through signal_hook, which discards write errors on purpose ("we ignore errors, on purpose. We don't have any means to handling them"), so a denied wake costs at worst a wait that runs to its deadline instead of returning early. signal_hook also probes the wake fd and falls back to write() when it isn't a socket, and sigchld hands it a pipe — so send(), the syscall that sandbox denies, isn't even on the path.

Two incidental wins over the in-tree waiter this PR carried first: the timed wait is non-reaping, so the child stays a zombie across the deadline and the stale-pid window at both kill sites is gone; and kill() goes through the Child handle rather than a raw signal by pid. Process-group teardown stays with the existing kill_timed_out_tree, which run_with_timeout_impl still calls first for its group leader.

One pre-existing quirk this deliberately does not change, recorded because an earlier revision of this PR did change it: forward_signal_with_escalation decides whether to escalate by polling killpg(pgid, 0), and a zombie is still a member of its process group, so an unreaped leader keeps that probe true through the 200 ms grace and the group SIGKILL follows the SIGTERM essentially every time — defeating the "git's lockfile handlers run on TERM" rationale the docstring gives for sending TERM first. wait-timeout left the leader unreaped and so does shared_child, so the behavior is identical before and after. (The in-tree ChildWaiter that 74470c2 deleted reaped asynchronously and did let a TERM-respecting child skip the SIGKILL; that is gone along with the waiter.) Worth fixing on its own, by giving the escalation probe something better than group liveness to read.

Two of the three sites — the pager and run_with_timeout_impl — now fold their Err arm into their timeout arm, because a failed wait at either needs the same teardown a timeout does. (Cmd::delayed_stream keeps its standalone Err arm, unchanged from wait-timeout: its Phase 1 timeout is a display threshold rather than a wall-clock bound, so a failed wait there un-bounds nothing. Whether it should fall through to streaming instead of returning is a separate question, left off this head.) The pager's two arms already did the same thing, so merging them was free. run_with_timeout_impl was the load-bearing one: it propagated a failed wait straight out of the thread::scope closure with no kill_timed_out_tree and no child.kill(), and the scope then joins the reader threads on the way out — they sit in read_to_end until the child closes its pipes, so the caller waited out the child's full runtime (~127 s per address for the git ls-remote case) and got back an error that isn't TimedOut. That is precisely what the function's own docstring says the teardown exists to prevent. The shape is pre-existing — wait-timeout's wait_timeout had the same ? — but this PR makes the Err materially more plausible, since shared_child allocates a pipe and registers a SIGCHLD handler on every timed wait where wait-timeout set its self-pipe up once, process-wide. Leaving a path where a denied syscall in the timed-wait machinery silently removes the bound would undercut the point of the PR.

Testing

The EPERM trigger needs a seccomp sandbox that denies the send, which isn't reproducible from CI. test_wait_timeout_crate_stays_out_of_the_dependency_graph pins the root cause instead: wait-timeout must not return to Cargo.lock, however it gets there. (An earlier revision of this PR asserted the SIGCHLD bit was clear in /proc/self/status's SigCgt mask; with shared_child a handler legitimately exists, and its absence was never the property that mattered — a handler that can't abort is.)

Behavior is covered by the existing timeout tests: test_cmd_timeout_kills_slow_command, test_cmd_timeout_bounds_wall_clock_with_surviving_grandchild, and the delayed_stream threshold tests. Two tests are new: test_pipe_through_pager_times_out pins the pager's kill-and-fall-back, and test_cmd_timeout_surfaces_a_spawn_failure pins that an unspawnable command fails as a spawn error rather than being waited out to the deadline. The merged Err-or-timeout arms run every line under the existing timeout tests; the one thing no test drives is the branch that unwraps a real wait error, which needs pipe/sigaction/waitid to fail and shares its lines with the timeout path.

Also run: cargo test --all-features (lib 1512, bins 976, integration 2138 pass — 11 pre-existing config_show snapshot failures in this sandbox, which has nu on PATH; they fail identically with these changes stashed), cargo clippy --all-features --all-targets, cargo fmt --check, RUSTDOCFLAGS=-D warnings cargo doc --no-deps --all-features.

Out of scope: the pager timeout doesn't bound wall-clock

Writing the pager test surfaced a pre-existing hole unrelated to this issue, left alone here. pipe_through_pager kills only the pager itself and then joins its stdout reader, so a pager that forks leaves a grandchild holding the pipe's write end and the join blocks for the grandchild's full runtime — PAGER_TIMEOUT bounds nothing. Measured directly: sh -c "sleep 30" forks rather than execs, the kill lands at 2.0 s, and the reader joins at 30.0 s. run_with_timeout_impl solved the same problem by spawning into its own process group and signalling the group; the pager never got that treatment. The test uses an explicit exec to stay on the case this PR is about.

Upstream context

alexcrichton/wait-timeout#45 flags this notify() panic as an async-signal-safety violation; it's open and classified as theoretical.

process_control is the other credible dependency here and resolves the bug the same way — waitid(WNOWAIT) with signal_hook's Signals iterator for SIGCHLD — but it's a heavier API (a Control builder over the child) for a narrower user base, so shared_child won.

---

Closes #3856 — automated triage

[worktrunk-bot]

Reviewing my own PR, so this is a COMMENT rather than an approval.

fast-checks is red on the doc step (job) — nothing else in that job failed. ChildWaiter is pub, so its two intra-doc links to kill_timed_out_tree resolve to a private item and -D rustdoc::private-intra-doc-links rejects them:

error: public documentation for `ChildWaiter` links to private item `kill_timed_out_tree`
    --> src/shell_exec.rs:1041:7
error: public documentation for `kill` links to private item `kill_timed_out_tree`
    --> src/shell_exec.rs:1084:64

I'm pushing the fix — plain code spans in both places, keeping the prose. No human author will pick this up otherwise.

waiter_gone()'s text names one of the two cases it covers. After the status is delivered the channel is disconnected and empty, so every later wait() / wait_timeout() takes the same arm and reports "the thread waiting on this child terminated unexpectedly" — which is exactly the spent-waiter case test_child_waiter_reports_a_spent_waiter pins, and nothing terminated unexpectedly there. No current caller reaches it (run_with_timeout_impl and the pager discard the second wait(); delayed_stream's Phase 2 wait() still has a pending value), but the message is what a future second-wait() caller would surface through .context("Failed to wait for command"). Pushing a wording fix that covers both causes alongside the doc-link fix.

One thing I left alone, for a maintainer to weigh

kill_timed_out_tree(waiter.pid()) inherits the staleness window that ChildWaiter::kill's docstring reasons about, and its own docstring doesn't mention it. Under wait-timeout the child was still unreaped when wait_timeout returned None, so child.id() was guaranteed to name the same process (a zombie at worst). Now the waiter thread reaps asynchronously, so between the expired deadline and the killpg the child can exit, be reaped, and free the pgid — and this call signals a whole process group by that id, not a single pid. It needs pid wraparound inside a few microseconds, so I don't think it's worth code, but the docstring on kill_timed_out_tree still reads as though the pid is guaranteed live ("its pid is the pgid and the TERM → KILL escalation reaches every member").

Related and benign, but worth recording since it's a real behavior change the PR description doesn't mention: forward_signal_with_escalation decides whether to escalate by polling killpg(pgid, 0). Previously the unreaped zombie leader kept that check true through the 200 ms grace, so the SIGTERM was followed by a group SIGKILL essentially every time — defeating the "git's lockfile handlers run on TERM" rationale the docstring gives for sending TERM first. With the waiter reaping promptly, a child that exits on TERM with no surviving group members now genuinely skips the SIGKILL. That's the intended semantics, so this is a fix, not a regression.

Otherwise the change reads correctly to me. Both run_with_timeout_impl and delayed_stream spawn with Stdio::null() stdin and the pager takes stdin/stdout off the child before handing it over, so moving the Child into a thread that calls wait() — which drops stdin — doesn't close a pipe anyone still writes to. Dropping a ChildWaiter without reaping is also better than the old Child drop: the thread outlives the handle and reaps, where Child::drop leaked a zombie. And the wait-timeout removal is complete — no source, Cargo.toml, or Cargo.lock references remain, and check-unused-dependencies, minimal-versions, and crate-build are all green.

[worktrunk-bot]

Closing out the one item my review left for a maintainer to weigh, since it turned out to be documentation rather than a code decision.

kill_timed_out_tree's docstring now records the stale-pid window it inherits from ChildWaiter::pid(), and that a recycled pid there names a process group rather than a single process — src/shell_exec.rs (7f156df). No code change: the window still needs the child to exit, be reaped, and have its pid recycled inside the few microseconds between the expired deadline and the killpg, which matches the bound ChildWaiter::kill already accepts.

The other half of that note — that reaping promptly changes forward_signal_with_escalation from "SIGKILL essentially every time" to "SIGKILL only when the group really survives TERM" — is a behavior change, so it is now a section in the PR description rather than only a review note.

Correction (edited after 74470c2): that paragraph was accurate for 7f156df, whose in-tree ChildWaiter reaped asynchronously, and stopped being accurate when 74470c2 replaced the waiter with shared_child. shared_child's timed wait is waitid(WNOWAIT) and does not reap, so the group leader is still an unreaped zombie at kill_timed_out_tree, process_group_alive still succeeds through the 200 ms grace, and the escalation to the group SIGKILL happens exactly as often as it did under wait-timeout. The PR makes no behavior change here; the description now records it as a pre-existing quirk left alone.

7f156df went green on every gating check. The head has since advanced to a1dc5d4 (pager timeout coverage), whose own run is still in flight.

Verification

• RUSTDOCFLAGS='-Dwarnings' cargo doc --no-deps --document-private-items — the fast-checks doc gate that was red before db8a850; passes locally on 7f156df, and fast-checks passed on it in CI.
• The escalation claim rests on process_group_alive probing with killpg(pgid, 0): a zombie is still a member of its process group, so an unreaped leader kept that probe true through the 200 ms grace.

[worktrunk-bot]

Reviewing my own PR, so this is a COMMENT rather than an approval.

The shared_child refactor holds up on the points it turns on. wait_deadline is waitid(WEXITED | WNOWAIT) plus a sigchld poll and does not reap on the Ok(None) path, so the new kill_timed_out_tree docstring ("the pid cannot name a different process group by the time the signal lands") is correct as written. wait_timeout, take_stdin/stdout/stderr and SharedChild::spawn all exist in 1.1.0, and timeout is a default feature, so the "1.1" requirement is safe under minimal-versions. sigchld reaches SIGCHLD through signal_hook::low_level::pipe::register over an os_pipe, which is the write-not-send path the description claims. signal-hook 0.3.18 was already in the lock before this PR, so the new crates add no duplicate-version churn.

Two things.

1. The description's "One behavior change worth naming" is wrong, and contradicts the paragraph above it

The description says both of these:

the timed wait is non-reaping, so the child stays a zombie across the deadline

The teardown now reaps promptly, so a child that exits on TERM genuinely skips the SIGKILL.

Only the first is true of what shipped. The second was true of the in-tree ChildWaiter that 74470c2 deleted — its background thread reaped asynchronously. With shared_child, wait_deadline returns Ok(None) without reaping, so at kill_timed_out_tree the group leader is still an unreaped zombie, process_group_alive's killpg(pgid, 0) still succeeds through the 200 ms grace, and forward_signal_with_escalation escalates to the group SIGKILL exactly as often as it did under wait-timeout. Nothing changed here; the section claims a behavior change this PR does not make. I verified the zombie half directly — killpg(pgid, 0) against a group whose only member is an unreaped zombie succeeds, and returns ESRCH only after the reap.

I'm rewriting that section of the description. The same correction applies to my earlier conversation comment, which repeated the claim while it was still accurate for 7f156df.

2. run_with_timeout_impl's Err arm leaves the timeout bounding nothing — pre-existing, but newly reachable

match child.wait_timeout(timeout)? propagates a failed wait straight out of the thread::scope closure, with no kill_timed_out_tree and no child.kill(). thread::scope then joins the two reader threads on the way out, and those sit in read_to_end until the child closes its pipes — so the caller waits the child's full runtime and gets back an error that isn't TimedOut. That is precisely the failure this function's own docstring says the teardown exists to prevent ("otherwise the timeout doesn't bound anything"), and for the git ls-remote case it names, the full runtime is ~127 s per address.

The shape is pre-existing — wait-timeout's wait_timeout had the same ? — but this PR makes the Err materially more plausible. shared_child allocates a pipe and registers a SIGCHLD handler on every timed wait (sigchld::Waiter::new()? inside wait_deadline_noreap), where wait-timeout set its self-pipe up once, process-wide. So fd exhaustion or a sandboxed pipe/sigaction now surfaces as Err where wait-timeout surfaced as an abort. Given the bug this PR exists to fix is "a denied syscall in the timed-wait machinery takes wt down", it's worth not leaving a path where a denied syscall in the same machinery silently removes the bound instead.

a1dc5d4 folded exactly this Err arm into the kill path for the pager; run_with_timeout_impl didn't get the same treatment.

I'm not fixing it here, deliberately: the Err(e) => Err(e) tail can't be driven from a test, so the fix adds an uncovered line and puts codecov/patch back under its 98.12% target on a PR whose point is elsewhere. Better as its own change, where the coverage argument can be made on its own terms.

Smaller notes, not worth changing

• pager.rs's merged arm logs "Pager did not exit within {:?}" for an Err too, where the wait failed rather than the pager overrunning. ?outcome is in the record, so the distinction is recoverable.
• test_wait_timeout_crate_stays_out_of_the_dependency_graph matches the whole lockfile, so a future dev-dependency pulling wait-timeout in transitively (the proptest → rusty-fork path) would fail it without the shipped binary being affected. No such dep today, and "however it gets there" is the stated intent.
• track_if_cancellable(child.id()) now evaluates id() eagerly at all five call sites rather than lazily inside the then, which for SharedChild takes the inner mutex. Immeasurable at one call per spawn.

[worktrunk-bot]

Reviewing my own PR, so this is a COMMENT rather than an approval.

33c187250 closes the run_with_timeout_impl gap my last review left open, and the merged-arm form is the right shape — every line in the arm runs under the existing timeout tests, so the fix cost no patch coverage (codecov/patch is green on this head).

The third exposed site still has the standalone Err arm the description says it lost

The description says "Both timed-wait sites now fold their Err arm into their timeout arm, because a failed wait needs the same teardown a timeout does" — but it also names three exposed call sites up top, and the third, Cmd::delayed_stream's Phase 1 wait (the match child.wait_timeout(remaining) under the // Phase 1: comment), still returns straight out:

Err(e) => {
    let _ = stdout_handle.join();
    let _ = stderr_handle.join();
    trace.fail(&e);
    return Err(e).context("Failed to wait for command");
}

Those arms are outside the diff — wait-timeout's wait_timeout had the same signature, so they carried over unchanged — and the site is genuinely less severe than the one just fixed: Phase 1's timeout is a display threshold, not a wall-clock bound, so nothing gets un-bounded. But the rest of the shape is identical, and the "newly more plausible Err" argument the description makes for run_with_timeout_impl applies here just as well, on the hotter path. The two spawn_delayed_reader joins sit in read_to_end until the child closes its pipes, so the caller waits out the child's full runtime; the SharedChild is then dropped unreaped (shared_child has no Drop impl), leaking a zombie; and a command that ran to completion is reported as Failed to wait for command.

Teardown isn't the fix here — killing a user's git worktree add because our wait pipe failed would be worse than the disease. The fall-through is: treat Err like Ok(None) and drop into streaming. Phase 2's child.wait() goes through waitid(WEXITED | WNOWAIT) with no pipe and no SIGCHLD registration, so it is unaffected by whatever made the timed wait fail; and if the failure is genuine rather than machinery-specific, Phase 2's own Err arm surfaces it anyway, just after the child has actually finished. That turns "wt fails an ordinary command under a sandbox that denies pipe/sigaction" into "wt streams its output immediately", which is the degradation this PR argues for everywhere else.

I'd rather not push it onto this head — it's pre-existing, out of the diff, and this PR is otherwise green. Either way the description needs a line: as written it claims a treatment the third site doesn't have.

What I checked on the new arm

• outcome.err() is None exactly on Ok(None), since Ok(Some(_)) is handled above — the TimedOut synthesis can't shadow a real wait error.
• kill_timed_out_tree's safety argument ("the caller still holds an unreaped SharedChild") carries to the new Err caller, rather than only to the timeout case. In shared_child 1.1.1, every wait_deadline error path fails before a reap: sigchld::Waiter::new()? and try_wait_noreap's waitid both return before Child::wait is reached, and the one post-exit error — inner_guard.child.wait()? — means the waitpid itself failed, so the pid is still reserved.
• let _ = child.wait() in the arm can't hang if the failure was in the wait machinery: SharedChild::wait is waitid(WEXITED | WNOWAIT) plus a blocking reap, with no pipe on the path, and the two kills preceding it close the reader threads' pipes either way.
• Every Cmd::timeout caller is a probe or a read — git ls-remote, the fsmonitor stop/lsof probes, git reap's probes, the shell probe — so newly killing the tree on a spurious wait error can't interrupt a write.

[worktrunk-bot]

Author role, acting on my own review above.

Item 2 is fixed here after all — 33c1872 folds run_with_timeout_impl's Err arm into its timeout arm, the same way a1dc5d4 did for the pager: src/shell_exec.rs#L1017-L1039. My reviewer-side reason for deferring was a predicted red codecov/patch, and that is the one reason CLAUDE.md rules out: "codecov/patch gates the merge, not the design … A predicted red patch is never a reason to shelve or water down an improvement." The prediction was also just wrong — patch is 49/49, 100% on the new head. Writing the merged arm as outcome.err().unwrap_or_else(|| … TimedOut …) puts every line of it on the path the existing timeout tests already drive; the only thing no test reaches is the branch where err() yields a real wait error, and that branch has no line of its own. The separate-PR alternative had a cost I didn't weigh in the review: it would edit the same lines this PR rewrites, so one of the two would need a rebase for nothing.

Item 1 was already applied to the description before the review posted. The stale claim also survived in my earlier conversation comment, which is now edited with the correction rather than left to be read as current.

A further review landed on this push while I was polling, flagging that the description claimed a treatment the third exposed site — Cmd::delayed_stream's Phase 1 wait — doesn't have. That claim was mine and is now corrected in the description: two of the three sites fold, delayed_stream keeps its standalone Err arm. Whether that arm should fall through to streaming instead of returning is a code question I've left open rather than answered on this head.

33c1872 is green on all four required checks (fast-checks, test (linux|macos|windows)) plus full-tests on all three platforms, nix-flake, minimal-versions, and both codecov contexts. One advisory job is red and one is still running — neither is a signal about this change.

The advisory failure, and what's still pending

release-target (aarch64-unknown-linux-musl) failed in test setup, not in a timeout path:

thread 'integration_tests::push::test_push_dirty_target_overlap_renamed_file' panicked at src/testing/mod.rs:1234:9:
git worktree add -b feature /tmp/wt/.tmpUJWkSI/repo.feature failed:
fatal: could not open '.git/worktrees/repo.feature/locked' for writing: No such file or directory

A git worktree add that can't write into the administrative directory it just created is a filesystem-level flake; the same job passed on b8c44e6 with the same test, and run_with_timeout_impl isn't on git worktree add's path — Cmd::timeout is set only for the bounded git ls-remote probe. POST /actions/jobs/97006400410/rerun returns 403 (The workflow run containing this job is already running) while feature-powerset — the one check still pending, ~1 h — holds the run open, so the rerun is available to a maintainer once that clears.

[worktrunk-bot]

nightly went red on 33c1872 in release-target (aarch64-unknown-linux-musl) — a test-setup flake, not the diff. I reran the failed jobs and all five came back green, so the PR is fully green now (including codecov/patch).

Diagnosis

Failed job — integration_tests::push::test_push_dirty_target_overlap_renamed_file, 2641 other tests passing in the same run:

thread 'integration_tests::push::test_push_dirty_target_overlap_renamed_file' panicked at src/testing/mod.rs:1234:9:
git worktree add -b feature /tmp/wt/.tmpUJWkSI/repo.feature failed:
stderr: Preparing worktree (new branch 'feature')
fatal: could not open '.git/worktrees/repo.feature/locked' for writing: No such file or directory

The panic is check_git_status on the repo.add_worktree("feature") line — plain fixture setup, before any wt binary runs in that test. locked is the sentinel git writes into $GIT_COMMON_DIR/worktrees/<id>/ right after creating that directory during worktree add; ENOENT means the directory git had just made wasn't there a moment later. Nothing in this diff touches worktree creation, and the kill path it does touch signals a single pid (Pid::from_raw(self.pid)), never a process group, so it can't reach a sibling test's git. The previous head b8c44e67f ran the same job green (run), and the rerun of this head passed in 4m14s.

First sighting of this shape — no open issue matches it, and one non-reproducing failure on one arm runner isn't enough to build a fix against. Noting it here so a recurrence has a prior to point at; the rerun leaves the check reading pass, so the red attempt is otherwise invisible.