1 Commits

Author	SHA1	Message	Date
Maximilian Roos	bb43011c8d	fix(picker): cancel background commands when the picker exits (#3560) ... ## 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>	2026-07-24 16:36:25 -07:00