skills/max-sixty__worktrunk
1
0
Fork 0
mirror of https://github.com/max-sixty/worktrunk.git synced 2026-09-14 20:00:38 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(picker): cancel background commands when the picker exits (#3560)

Browse Source 
## What

`shell_exec` gains `cancel_background_commands()`: it latches a flag
that stops
background `Cmd::run` / `Cmd::pipe_into` before they spawn, and SIGTERMs
the
PIDs of those already running (tracked in a registry each capture-mode
command
joins for its lifetime). The picker calls it once, after skim releases
the
terminal and after the stashed-warning drain.

Preview diffs pick up `--no-optional-locks`, matching what `wt list`'s
`git status` already does: a preview reads a worktree the user may be
working
in, and can now be signalled mid-run.

Cancellation covers both accept and abort, and only ever targets
background
threads — the foreground thread is the one cancelling, and goes on to
run the
switch itself.

## Why

Accepting the picker abandoned its preview work rather than stopping it.
`wt`'s
exit ends the pool's threads but not the `git` children they spawned, so
those
children kept running, orphaned, computing diffs into an in-memory cache
that no
longer existed — churning disk on a repo the user had already left.

Measured on a 5-worktree fixture whose `git diff` sleeps 15s, with a
shell (not
`wt`) as the PTY session leader, since `wt` as session leader SIGHUPs
its own
children on exit and hides the effect entirely:

| | children alive at t+0 / +1 / +3 / +6s after `wt` exits |
| --- | --- |
| without cancellation | 12 / 12 / 12 / 12 |
| with cancellation | 0 / 0 / 0 / 0 |

Single-variable control: same build, same fixture, cancellation gated
off.
Accept latency is unchanged (0.13s).

## Why a latch, not just a sweep

A one-shot sweep over live PIDs isn't enough on its own. A task that has
already
cleared its caller's supersede check and parked on `CMD_SEMAPHORE` holds
no PID
for the sweep to find, and spawns the moment a permit frees — the very
permits
the sweep just freed by signalling everything holding one.

Capping concurrency at 2 to force a queue reproduces it: sweep-only
still leaves
survivors, the latch reaches 0. For the same reason a freshly spawned
child
re-reads the flag as it registers, so a child spawned into the sweep's
window
escapes neither.

This also replaced a first attempt that bumped the preview
orchestrator's spawn
generation. That stops *queued* tasks, but those tasks had already
passed the
generation check before parking on the semaphore, so it measured no
better than
the sweep alone. One mechanism in `shell_exec` covers all three states —
queued,
parked, running — so the orchestrator is untouched.

## Notes

- **`alt-x` removals are exempt.** A removal is dispatched to its own
thread so
  the picker stays live, so pressing Enter straight after can leave a
  `git worktree remove` in flight when the sweep fires. Its *result* is
discardable; its *effects* are not, and a signal between the worktree
remove
  and the branch delete would strand the user half-removed.
`shell_exec::uninterruptible` exempts a thread from both halves — the
sweep
skips its PIDs, and the latch doesn't refuse the calls it makes
afterwards,
since a removal is several git calls rather than one. The picker routes
every
  removal dispatch through one `spawn_removal` helper that applies the
exemption, so it's carried by the spawn path itself rather than
remembered at
  each call site.

- **SIGTERM, not SIGKILL**, so git's lockfile handlers run rather than
stranding
  an `index.lock` in a worktree the user is about to work in.
- **Cancel is ordered after `drain_stashed_warnings`.** A `--prs` forge
call
killed mid-flight fails like any other and stashes that failure as a
warning;
cancelling first would print a spurious "couldn't fetch PRs" to the
user.
- **`Cmd::run`'s plain branch now spawns explicitly** instead of calling
`cmd.output()`, whose stdio defaults it reproduces exactly — `output()`
hands
  back only the finished result, never the running child.
- **Windows** gets the latch but not the signal; a command already
running there
  still finishes. Bounding those would take a job object.
- **Tests.** The cancelled paths (both refusals and the mid-flight
signal) are
covered by `tests/cancel_background.rs`, deliberately its own test
binary:
cancellation latches process-wide state and signals every background PID
in
the process, so beside other tests it takes them out — `TestRepo`'s
helpers
drive git through the same `Cmd` path, and this is a real failure, not a
theoretical one. Alone in its process it has nothing to collide with,
and the
  test is deterministic (a file marker for readiness, not a sleep).

An earlier revision of this description claimed `codecov/patch`
contradicted
its own line data. That was wrong — a mis-read of the API on my part,
and the
check was right. Local `cargo llvm-cov` had been failing to build `skim`
(E0554), so I'd taken the API as a substitute for measuring. The build
failure
was a stale local tool: cargo-llvm-cov ≥ 0.7.0 instruments only
workspace
crates instead of the whole dependency graph, so on CI's pinned 0.8.7
`skim`
never sees the `cfg(coverage)` that trips its nightly feature gate. With
that
upgraded, local coverage reproduces codecov's four missed lines exactly.

Three were in `test_background_command_registers_while_running`, now
removed —
its poll loop's body only ran when the spawned thread hadn't registered
yet.
Coverage is the lesser reason: the test polled a process-global registry
for
non-emptiness, so under the shared-process runner the coverage job uses
it
could be satisfied by another test's PID and pass without its own
command ever
registering. `tests/cancel_background.rs` pins that property correctly,
and
more strongly — the sweep reaches a running command only if it
registered.

Three lines are left uncovered, knowingly. One is the spawn-race
re-check,
  which by construction only fires on a race that can't be scheduled
deterministically — covering it would need a scheduling hook in library
code,
which the repo's "no test code in library code" rule rules out. The
other two
  are pre-existing: the `spawn_removal` refactor re-indents the dispatch
  closures, which codecov counts as new patch lines, pulling in the
morph-failure backstop (the `warn` and `revert_morph` calls). That
backstop
only fires when a file appears in the TOCTOU window between the
removal's
safety check and the rename, so it can't be deterministically tested
either.
If `codecov/patch` reads red, this is why — the structural exemption was
  judged worth more than the metric.

- **PID reuse.** Between the kernel reaping a child and the guard's
`Drop`
removing its PID, a freed PID is briefly still listed. Inherent to
signalling
  by PID — the reap and the deregistration aren't one operation — and
deregistering before the wait isn't a fix but a removal, since the wait
*is*
the command's lifetime. Documented on `BackgroundPid` rather than
mitigated.

> _This was written by Claude Code on behalf of Maximilian Roos_

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Maximilian Roos
2026-07-24 16:36:25 -07:00
committed by GitHub
parent f5d99d4fe5
commit bb43011c8d
7 changed files with 426 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CLAUDE.md
+1 -1
View File
@@ -103,7 +103,7 @@ Why: silent "lookup" paths that walk to the wire (alias dispatch, hook context b
**Network never blocks the first write.** Fast output to the terminal is the priority (Real-time Output Streaming, above): every command paints from local data first, then network-derived detail streams in progressively behind it. A command that can't render its first frame until `gh` or `git fetch` returns is the failure mode, worst on a fresh clone or a slow link. Before adding an accessor that could reach the wire (`gh`, `glab`, `git fetch`, `git ls-remote`, HTTP), confirm it renders progressively and never gates the first paint. A synchronous hot path like a shell prompt is stricter: it must not reach the wire at all, even progressively. `wt list statusline` is not such a path despite running on every prompt, because Claude Code consumes its output asynchronously.
**The picker is the most forgiving home for network work, because its lifetime is bounded by the user, not the job.** It paints immediately, the user browses, and a slow forge call streams into the rows whenever it arrives; if the user picks first, the unfinished request is simply abandoned, so its latency never costs anything. A run-to-completion command is less forgiving: `wt list` renders progressively but still cannot *finish* until every task returns, so a slow `gh` call extends the command the user is waiting on. Prefer the picker for live forge data, and fetch it there progressively.
**The picker is the most forgiving home for network work, because its lifetime is bounded by the user, not the job.** It paints immediately, the user browses, and a slow forge call streams into the rows whenever it arrives; if the user picks first, the picker's exit cancels the unfinished request (`shell_exec::cancel_background_commands`), so its latency never costs anything. A run-to-completion command is less forgiving: `wt list` renders progressively but still cannot *finish* until every task returns, so a slow `gh` call extends the command the user is waiting on. Prefer the picker for live forge data, and fetch it there progressively.
What currently reaches the wire:
Cargo.toml
+7
View File
@@ -144,6 +144,13 @@ name = "integration"
path = "tests/integration.rs"
required-features = ["cli", "syntax-highlighting"]
# Deliberately a second binary rather than another module under `integration`:
# it latches process-wide cancellation state, which would take out any test
# running beside it in the same process. See the file's module docs.
[[test]]
name = "cancel_background"
path = "tests/cancel_background.rs"
[dependencies]
anyhow = "1.0.102"
anstream = "1.0"
src/commands/picker/items.rs
+9 -3
View File
@@ -348,7 +348,11 @@ impl SkimItem for HeaderSkimItem {
/// only ever produces the name-free body.
///
/// `prefix` is the command through the `diff` subcommand (e.g. `["diff"]` or
/// `["-C", path, "diff"]`); `revs` are the positional revisions.
/// `["-C", path, "diff"]`); `revs` are the positional revisions. Both commands
/// use `--no-optional-locks` to avoid index lock contention, as `wt list`'s
/// `git status` does: a preview runs on a background thread against a worktree
/// the user may be working in, and can be signalled mid-run by
/// [`worktrunk::shell_exec::cancel_background_commands`].
///
/// The diff options precede an `--end-of-options` sentinel, which fences the
/// positional `revs` so a ref that looks like a flag (a branch literally named
@@ -364,7 +368,8 @@ fn compute_diff_preview(
let stat_width_arg = format!("--stat-width={width}");
// Check stat output first.
let mut stat_args = prefix.to_vec();
let mut stat_args = vec!["--no-optional-locks"];
stat_args.extend_from_slice(prefix);
stat_args.extend([
"--stat",
"--color=always",
@@ -381,7 +386,8 @@ fn compute_diff_preview(
let mut output = stat;
// Build diff args with color.
let mut diff_args = prefix.to_vec();
let mut diff_args = vec!["--no-optional-locks"];
diff_args.extend_from_slice(prefix);
diff_args.extend(["--color=always", "--end-of-options"]);
diff_args.extend_from_slice(revs);
src/commands/picker/mod.rs
+69 -40
View File
@@ -493,32 +493,30 @@ impl AltXRemover {
let render_tx = Arc::clone(&self.render_tx);
let stashed_warnings = Arc::clone(&self.stashed_warnings);
let header_flash = Arc::clone(&self.header_flash);
let _ = std::thread::Builder::new()
.name(format!("picker-remove-{selected_output}"))
.spawn(move || {
if let Err(e) = Self::do_removal(&repo, &result, &approvals) {
tracing::warn!(selected_output = %selected_output, error = %e, "picker: removal of '{selected_output}' errored: {e:#}");
}
// A removal that keeps its branch never reaches here — that's the
// morph path (`morph_and_remove_in_background`). So a surviving
// target means the removal itself failed: put the row back.
if removal_target_still_present(&repo, &result)
&& let Some((item, pos)) = removed
{
restore_failed_removal(
&items,
&header_flash,
&render_tx,
&stashed_warnings,
DroppedRow {
item,
pos,
label: removal_label,
noun: removal_noun,
},
);
}
});
spawn_removal(format!("picker-remove-{selected_output}"), move || {
if let Err(e) = Self::do_removal(&repo, &result, &approvals) {
tracing::warn!(selected_output = %selected_output, error = %e, "picker: removal of '{selected_output}' errored: {e:#}");
}
// A removal that keeps its branch never reaches here — that's the
// morph path (`morph_and_remove_in_background`). So a surviving
// target means the removal itself failed: put the row back.
if removal_target_still_present(&repo, &result)
&& let Some((item, pos)) = removed
{
restore_failed_removal(
&items,
&header_flash,
&render_tx,
&stashed_warnings,
DroppedRow {
item,
pos,
label: removal_label,
noun: removal_noun,
},
);
}
});
}
/// Flash a one-line message in the header for a beat (see the free
@@ -672,18 +670,16 @@ impl AltXRemover {
branch_token: branch.clone(),
worktree_token: selected_output.clone(),
};
let _ = std::thread::Builder::new()
.name(format!("picker-morph-{branch}"))
.spawn(move || {
if let Err(e) = Self::do_removal(&repo, &result, &approvals) {
tracing::warn!(branch = %branch, error = %e, "picker: removal of '{branch}' worktree errored: {e:#}");
}
// Only the worktree removal can realistically fail here; if it did,
// the worktree dir survives — undo the morph and say so.
if removal_target_still_present(&repo, &result) {
revert_morph(revert, &header_flash, &stashed_warnings, &render_tx);
}
});
spawn_removal(format!("picker-morph-{branch}"), move || {
if let Err(e) = Self::do_removal(&repo, &result, &approvals) {
tracing::warn!(branch = %branch, error = %e, "picker: removal of '{branch}' worktree errored: {e:#}");
}
// Only the worktree removal can realistically fail here; if it did,
// the worktree dir survives — undo the morph and say so.
if removal_target_still_present(&repo, &result) {
revert_morph(revert, &header_flash, &stashed_warnings, &render_tx);
}
});
RemovalEffect::Morphed
}
@@ -2122,8 +2118,9 @@ summary = true
//
// Don't join `collect_handle` after skim exits: drain may still be running
// network tasks, and joining would block exit for up to DRAIN_TIMEOUT
// (120s). Process exit terminates the bg thread; its git subprocesses
// are read-only.
// (120s). Process exit terminates the bg thread; everything it started is
// read-only, and the cancellation below stops the subprocesses it spawned,
// which process exit does not.
let output = run_skim(options, rx, &render_tx);
drop(collect_handle);
// Same rationale as `collect_handle`: don't join — the forge call may still be
@@ -2137,6 +2134,23 @@ summary = true
// the rest fall on the floor with the bg thread.
drain_stashed_warnings(&stashed_warnings);
// The picker is over, so its background work has no consumer left: the
// preview cache dies with the process and skim will never repaint again.
// Uncancelled, a preview diff per worktree runs to completion against a
// repo the user has already left — `wt`'s exit ends the pool's threads,
// not the git children they spawned. Applies equally to accept and abort.
//
// After the drain, not before: a `--prs` forge call killed mid-flight
// fails like any other, and stashes that failure as a warning. Cancelling
// first would drain a spurious "couldn't fetch PRs" onto the user.
//
// Not everything running here is discardable — an `alt-x` removal is
// dispatched to its own thread and can still be mid-`git worktree remove`
// if the user pressed Enter straight after. Removal threads are spawned
// through `spawn_removal`, which marks them `uninterruptible` and lets
// them run to completion; this reaches only speculative work.
worktrunk::shell_exec::cancel_background_commands();
// `run_skim` returns Err only on a genuine TUI init / event-loop failure;
// a user cancel is `Ok` with `is_abort` set. Surface a real failure.
let out = output?;
@@ -2386,6 +2400,21 @@ fn install_remove_keybinding(keymap: &mut skim::binds::KeyMap, remover: AltXRemo
keymap.insert(key, vec![cb]);
}
/// Run a removal's git work on a named background thread, exempt from
/// background-command cancellation: a removal is work the user asked for, and
/// a SIGTERM landing between `git worktree remove` and the branch delete would
/// leave them half-removed. Every removal dispatch goes through here so the
/// exemption is carried by the spawn path itself rather than remembered at
/// each call site.
fn spawn_removal<F>(name: String, work: F)
where
F: FnOnce() + Send + 'static,
{
let _ = std::thread::Builder::new()
.name(name)
.spawn(move || worktrunk::shell_exec::uninterruptible(work));
}
/// Run a row shortcut's OS action on a named background thread, logging any
/// failure — the picker owns the terminal, so an error can't be shown inline.
fn spawn_shortcut<F>(name: &str, action: F)
src/shell_exec.rs
+201 -2
View File
@@ -39,7 +39,15 @@
//! spawn paths; both isolate-by-default for the same `killpg` reason. They
//! never share wt's pgroup because they don't drive the tty (concurrent uses
//! piped stdio, detached escapes the PTY entirely).
//!
//! ## Cancelling background children
//!
//! `wt` exiting ends its own threads but not the children they spawned, which
//! keep running as orphans. [`cancel_background_commands`] lets the foreground
//! thread stop that work — both what is running and what has yet to start —
//! once nobody is left to read its results.
use std::collections::BTreeSet;
use std::ffi::{OsStr, OsString};
use std::fs::Metadata;
use std::io::{BufRead, BufReader, ErrorKind, Read, Write};
@@ -78,6 +86,141 @@ fn is_foreground_thread() -> bool {
FOREGROUND_THREAD.get() == Some(&std::thread::current().id())
}
/// PIDs of capture-mode commands currently running on background threads, so
/// the foreground thread can cancel them once nobody will read their results.
///
/// A background thread dies with the process, but the `git` child it spawned
/// does not — it keeps running, orphaned, against a repo `wt` has already
/// left. The picker is where this bites: accepting a row abandons one preview
/// diff per worktree, each able to churn disk for seconds on a large repo,
/// filling an in-memory cache that no longer exists.
///
/// Only cancellable threads register ([`is_cancellable_thread`]): the
/// foreground thread is the one that cancels, and is never itself inside a
/// tracked command while doing so, so the work the user is actually waiting
/// on is never a target; an [`uninterruptible`] thread finishes what it
/// started.
static BACKGROUND_PIDS: Mutex<BTreeSet<u32>> = Mutex::new(BTreeSet::new());
/// Deregisters a background command's PID however the command finishes.
///
/// The child is reaped inside the `wait` this guard outlives, so for the few
/// instructions between that reap and this `drop` a freed PID is still listed,
/// and a sweep landing in that window would signal whatever the kernel handed
/// the number to next. Signalling by PID can't close this — the reap and the
/// deregistration are not one operation — and deregistering before the wait
/// instead is not a fix but a removal: the wait *is* the command's lifetime,
/// so nothing would ever be cancellable. Accepted rather than mitigated:
/// PID allocation is incremental up to `pid_max`, which makes reuse inside a
/// microsecond window require wrapping the entire PID space first.
struct BackgroundPid(u32);
impl Drop for BackgroundPid {
fn drop(&mut self) {
BACKGROUND_PIDS.lock().unwrap().remove(&self.0);
}
}
std::thread_local! {
/// Whether this thread is running work cancellation must not touch. See
/// [`uninterruptible`].
static UNINTERRUPTIBLE: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
}
/// Run `f` with this thread's commands exempt from cancellation, for work the
/// user has already asked for rather than work done on spec.
///
/// Cancelling is safe for a preview because its only effect is a cache entry
/// nobody will read. It is not safe for a mutation: the picker runs an `alt-x`
/// worktree removal on a background thread so the UI stays live, and a SIGTERM
/// landing between `git worktree remove` and the branch delete would leave the
/// user half-removed. Such a thread finishes what it started; only its result
/// is discardable, not its effects.
pub fn uninterruptible<T>(f: impl FnOnce() -> T) -> T {
struct Restore(bool);
impl Drop for Restore {
fn drop(&mut self) {
UNINTERRUPTIBLE.with(|flag| flag.set(self.0));
}
}
let _restore = Restore(UNINTERRUPTIBLE.with(|flag| flag.replace(true)));
f()
}
/// Whether this thread's commands are subject to cancellation: background (the
/// foreground thread is the one doing the cancelling, and goes on to run the
/// switch itself) and not marked [`uninterruptible`].
fn is_cancellable_thread() -> bool {
!is_foreground_thread() && !UNINTERRUPTIBLE.with(std::cell::Cell::get)
}
/// Register a freshly spawned child as cancellable for as long as the returned
/// guard lives. Returns `None` when this thread's commands aren't subject to
/// cancellation (see [`is_cancellable_thread`]).
fn track_if_cancellable(child: &std::process::Child) -> Option<BackgroundPid> {
is_cancellable_thread().then(|| {
let pid = child.id();
BACKGROUND_PIDS.lock().unwrap().insert(pid);
let guard = BackgroundPid(pid);
// Re-read after publishing the PID, closing the window between this
// command's pre-spawn check and its registration. Either the sweep
// takes the set lock after the insert and signals this PID itself, or
// it ran first — in which case the store it followed is visible here
// and the signal it couldn't deliver is delivered now. A child spawned
// into that window is exactly the orphan the sweep exists to prevent.
if BACKGROUND_CANCELLED.load(Ordering::SeqCst) {
signal_background_pid(pid);
}
guard
})
}
/// Set once the foreground thread has cancelled background work, so commands
/// that haven't spawned yet never do.
///
/// Cancellation has to be a state, not a one-shot sweep over
/// [`BACKGROUND_PIDS`]. A task that already cleared its caller's own
/// supersede check and then parked on [`CMD_SEMAPHORE`] holds no PID for a
/// sweep to find, and would spawn the moment a permit frees — precisely the
/// permits the sweep just freed by signalling everything holding one.
static BACKGROUND_CANCELLED: AtomicBool = AtomicBool::new(false);
/// Whether the calling thread's commands have been cancelled.
fn background_cancelled() -> bool {
is_cancellable_thread() && BACKGROUND_CANCELLED.load(Ordering::SeqCst)
}
fn cancelled_error() -> std::io::Error {
std::io::Error::new(ErrorKind::Interrupted, "background command cancelled")
}
/// Abandon background work: nothing further spawns, and whatever is already
/// running is signalled rather than left to finish as an orphan.
///
/// Callers see either as an ordinary command failure, which every background
/// caller already treats as "no result".
pub fn cancel_background_commands() {
BACKGROUND_CANCELLED.store(true, Ordering::SeqCst);
for &pid in BACKGROUND_PIDS.lock().unwrap().iter() {
signal_background_pid(pid);
}
}
/// SIGTERM rather than SIGKILL: git's lockfile handlers run on the former, so
/// a diff interrupted mid-index-refresh cleans up after itself instead of
/// stranding an `index.lock` in a worktree the user is about to work in.
#[cfg(unix)]
fn signal_background_pid(pid: u32) {
forward_signal_to_pid(pid as i32, signal_hook::consts::SIGTERM);
}
/// Windows has no signal to deliver to an unrelated PID, so a command already
/// running there runs to completion; the latch still stops everything that
/// hasn't spawned, which is the bulk of a large fan-out.
#[cfg(windows)]
fn signal_background_pid(_pid: u32) {}
/// The working directory at `wt` startup. Captured once so relative `GIT_*`
/// path variables inherited from a parent `git` process can be resolved to
/// absolute paths regardless of each subsequent child command's `current_dir`.
@@ -721,6 +864,7 @@ fn run_with_timeout_impl(
.stdout(Stdio::piped())
.stderr(Stdio::piped())
.spawn()?;
let _tracked = track_if_cancellable(&child);
let mut child_stdout = child.stdout.take();
let mut child_stderr = child.stderr.take();
@@ -1313,6 +1457,16 @@ impl Cmd {
let mut trace = CommandTrace::new(self.context.as_deref(), &cmd_str)
.reads_stdin(self.stdin_data.is_some());
// Checked after the permit, not before: a command can be cancelled
// while parked on the semaphore, and that is the common case in a
// large fan-out.
if background_cancelled() {
let e = cancelled_error();
trace.fail(&e);
external_log.record(None);
return Err(e);
}
if let Err(e) = self.check_spawn_preconditions() {
trace.fail(&e);
external_log.record(None);
@@ -1338,6 +1492,7 @@ impl Cmd {
match cmd.spawn() {
Ok(mut child) => {
let _tracked = track_if_cancellable(&child);
// Write stdin data in an inner scope so the handle DROPS
// (closing the pipe) before `wait_with_output` — otherwise a
// child that reads stdin to EOF (e.g. `git … --stdin`) blocks
@@ -1357,8 +1512,21 @@ impl Cmd {
// Timeout handling uses the existing impl
run_with_timeout_impl(&mut cmd, timeout_duration)
} else {
// Simple case: just run and capture output
cmd.output()
// Simple case: run and capture output. Spawned explicitly rather
// than via `cmd.output()` — which matches these stdio defaults —
// because `output()` hands back only the finished result, never
// the running child, and a background command has to be
// registered as cancellable while it runs (see BACKGROUND_PIDS).
cmd.stdin(Stdio::null())
.stdout(Stdio::piped())
.stderr(Stdio::piped());
match cmd.spawn() {
Ok(child) => {
let _tracked = track_if_cancellable(&child);
child.wait_with_output()
}
Err(e) => Err(e),
}
};
record_captured(&mut trace, self.stdin_data.as_deref(), &result);
@@ -1431,6 +1599,19 @@ impl Cmd {
let _guard = (!is_foreground_thread()).then(|| semaphore().acquire());
// Cancelled, possibly while parked on the semaphore above (see
// `background_cancelled`). Nothing has spawned, so neither half runs.
if background_cancelled() {
let e = cancelled_error();
CommandTrace::record_failed(
self.context.as_deref(),
&first_cmd_str,
self.stdin_data.is_some(),
&e,
);
return Err(e);
}
// Validate both commands before spawning either. Nothing has spawned
// yet, so a precondition failure emits a one-shot failed record rather
// than holding a guard across an execution that never happens.
@@ -1476,6 +1657,7 @@ impl Cmd {
return Err(e);
}
};
let _first_tracked = track_if_cancellable(&first_child);
let first_stdout = first_child
.stdout
.take()
@@ -1515,6 +1697,7 @@ impl Cmd {
return Err(e);
}
};
let _second_tracked = track_if_cancellable(&second_child);
// `first`'s stderr must be drained concurrently with `second`'s
// execution; otherwise pathological stderr volume (~64 KiB pipe
@@ -2659,6 +2842,22 @@ mod tests {
assert!(result.is_ok());
}
#[test]
fn test_background_pid_deregisters_on_drop() {
// Not a live PID: this exercises the registry bookkeeping only, and
// nothing in this test signals anything.
let pid = u32::MAX;
{
BACKGROUND_PIDS.lock().unwrap().insert(pid);
let _guard = BackgroundPid(pid);
assert!(BACKGROUND_PIDS.lock().unwrap().contains(&pid));
}
assert!(
!BACKGROUND_PIDS.lock().unwrap().contains(&pid),
"the guard should deregister its PID once the command finishes"
);
}
#[test]
#[cfg(unix)]
fn test_process_group_alive_with_current_process() {
tests/CLAUDE.md
+3 -1
View File
@@ -27,7 +27,9 @@ task coverage
cargo llvm-cov report --show-missing-lines | grep <file> # authoritative miss list; matches codecov line-for-line
```
For each uncovered function, either write a test (integration tests via `assert_cmd_snapshot!` do capture subprocess coverage) or document why it's intentionally untested. If codecov's compare API must be queried directly, `coverage.head` is a `LineType` enum: `0=hit`, `1=miss`, `2=partial`.
For each uncovered function, either write a test (integration tests via `assert_cmd_snapshot!` do capture subprocess coverage) or document why it's intentionally untested. If codecov's compare API must be queried directly, `coverage.head` is a `LineType` enum: `0=hit`, `1=miss`, `2=partial`, and per-file `.totals.head.diff` (`[files, lines, hits, misses, partials, coverage, …]`) is what reproduces the posted patch percentage — the top-level `totals.base.diff` reports different numbers. Prefer measuring: the API is for disputing a posted check, not a substitute for `task coverage`.
**`skim` fails with E0554 (`#![feature]` on stable):** the local `cargo-llvm-cov` predates 0.7.0, which stopped putting the coverage flags in global `RUSTFLAGS` and started instrumenting only workspace crates. Older versions leak `--cfg=coverage` into every dependency, and `skim` gates a nightly feature on it. Install the version the `code-coverage` job pins rather than working around it (`--no-cfg-coverage` also avoids it; `--no-rustc-wrapper` reinstates it).
**Moved and re-indented lines:** codecov counts every line the diff touches as part of the patch, including one the change only relocated — a `git mv`, or a body re-indented because it moved inside a new wrapper. Pre-existing uncovered lines then count against a patch that changed no behavior. Verify against `main` (under the old path, for a rename): if the lines are identical there, the misses predate the change, and the fix is to say so to the user rather than undo the move.
tests/cancel_background.rs
+136
View File
@@ -0,0 +1,136 @@
//! Cancellation of background commands (`worktrunk::shell_exec`).
//!
//! This lives in its own test binary because `cancel_background_commands`
//! latches process-wide state and signals every background PID in the process.
//! Folded into `integration`, it would take out whatever ran beside it —
//! `TestRepo`'s helpers drive git through the same `Cmd` path — and under a
//! shared-process runner that is a real failure, not a theoretical one. One
//! test, alone in its process, has nothing to collide with.
#![cfg(unix)]
use std::io::ErrorKind;
use std::time::{Duration, Instant};
use worktrunk::shell_exec::{Cmd, cancel_background_commands, uninterruptible};
/// Cancelling reaches a command that is already running and refuses one that
/// has not started yet — the two halves of the guarantee, since a sweep over
/// live PIDs can't see a task still queued behind the command semaphore — while
/// leaving an `uninterruptible` thread's commands alone in both directions.
///
/// The first half is also what pins PID registration: the sweep can only reach
/// a running command that registered itself, so if registration regressed this
/// test's `sleep` would run to completion. Asserting on the registry directly
/// instead would be weaker — it is process-global, so under a shared-process
/// runner the assertion can be satisfied by some other test's command.
///
/// Every command runs on a spawned thread: the foreground thread is exempt (it
/// is the one doing the cancelling), and with `FOREGROUND_THREAD` unset under a
/// test harness, every other thread counts as background.
#[test]
fn cancel_stops_speculative_commands_and_spares_uninterruptible_ones() {
let dir = tempfile::tempdir().unwrap();
let started = dir.path().join("started");
let marker = started.display().to_string();
// `exec` so the sleep *is* the process we spawned. Left as a child of the
// shell, it would survive the signal to its parent and keep the captured
// stdout pipe open, blocking the wait for the full 30s.
let running = std::thread::spawn(move || {
Cmd::new("sh")
.arg("-c")
.arg(format!("touch '{marker}'; exec sleep 30"))
.run()
});
// Wait for the child to be genuinely running, so the sweep has a live PID
// to find rather than racing the spawn.
let deadline = Instant::now() + Duration::from_secs(30);
while !started.exists() {
assert!(Instant::now() < deadline, "the command never started");
std::thread::sleep(Duration::from_millis(10));
}
// A mutation the user already asked for — an `alt-x` removal — runs on a
// thread that opts out, and must survive the sweep rather than be killed
// between two git steps.
let protected_marker = dir.path().join("protected");
let protected_path = protected_marker.display().to_string();
let protected = std::thread::spawn(move || {
uninterruptible(|| {
Cmd::new("sh")
.arg("-c")
.arg(format!("touch '{protected_path}'; exec sleep 2"))
.run()
})
});
while !protected_marker.exists() {
assert!(
Instant::now() < deadline,
"the exempt command never started"
);
std::thread::sleep(Duration::from_millis(10));
}
let cancelled_at = Instant::now();
cancel_background_commands();
let output = running
.join()
.unwrap()
.expect("a signalled command still yields its Output");
assert!(
cancelled_at.elapsed() < Duration::from_secs(25),
"the sleep ran to completion instead of being signalled"
);
assert!(
!output.status.success(),
"a signalled command should report failure"
);
// Anything starting afterwards is refused before it spawns a process.
let refused = std::thread::spawn(|| Cmd::new("sh").arg("-c").arg("sleep 30").run())
.join()
.unwrap();
assert_eq!(
refused
.expect_err("a cancelled command must not spawn")
.kind(),
ErrorKind::Interrupted
);
// Same for the two-process pipeline path, which spawns on its own.
let piped = std::thread::spawn(|| {
Cmd::new("sh")
.arg("-c")
.arg("sleep 30")
.pipe_into(Cmd::new("cat"))
})
.join()
.unwrap();
assert_eq!(
piped
.expect_err("a cancelled pipeline must not spawn")
.kind(),
ErrorKind::Interrupted
);
// The exempt command ran to completion despite the sweep.
let protected_output = protected
.join()
.unwrap()
.expect("an uninterruptible command runs");
assert!(
protected_output.status.success(),
"cancellation must not signal a command the user asked for"
);
// And the latch doesn't refuse its *later* commands either — a removal is
// several git calls, not one, and the ones after the sweep must still run.
let after = std::thread::spawn(|| uninterruptible(|| Cmd::new("true").run()))
.join()
.unwrap()
.expect("an uninterruptible command still spawns after cancellation");
assert!(after.status.success());
}