fix(remove): force-kill a wedged fsmonitor daemon on worktree removal - #2813
Conversation
`git fsmonitor--daemon stop` is an IPC request; a wedged daemon (the common failure that hangs `git status`, and in turn `wt list`) can't answer it, so the daemon leaked on removal. Add a canonical `stop_fsmonitor_daemon` helper that, after the IPC stop, resolves the daemon PID from its IPC socket and escalates SIGTERM -> bounded wait -> SIGKILL, scoped strictly to the removed worktree's own socket. Routed through all three removal paths; the redundant shell-fragment stop in the detached path is deleted (one canonical path). Unix-only force-kill with a clean #[cfg(not(unix))] no-op. FAQ and troubleshooting docs updated to describe current behavior. Co-Authored-By: Claude <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
Windows CI fails on this branch with error: constant FSMONITOR_LSOF_TIMEOUT is never used — the constant is referenced only inside the #[cfg(unix)] force_kill_fsmonitor_via_socket, so it's dead on the Windows build. FSMONITOR_SIGTERM_GRACE already carries #[cfg(unix)] for the same reason; FSMONITOR_LSOF_TIMEOUT needs it too.
The constant's only use is in the `#[cfg(unix)]` force-kill path, so on Windows it was dead code and `-D warnings` failed `test (windows)`. Mirror `FSMONITOR_SIGTERM_GRACE`, which is already gated for the same reason. `FSMONITOR_STOP_TIMEOUT` stays ungated — it is used by the always-compiled `stop_fsmonitor_daemon`, which keeps `Duration`/`Cmd`/ `Path` live on Windows, so no unused-import fallout. Co-Authored-By: Claude <noreply@anthropic.com>
codecov/patch flagged the best-effort error/no-op arms of stop_fsmonitor_daemon. Add two deterministic, daemon-free tests for the genuinely-testable behavioral contracts: an unresolvable git dir hits the fail-open Err arm (log + return, no panic), and a socket file no process holds drives the real `lsof` lookup to an empty result with nothing signalled and the path left intact. The residual uncovered lines are real-OS-signal paths (SIGTERM ESRCH) and assert-failure-only test format strings, the same class accepted on the companion PR. Co-Authored-By: Claude <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
codecov/patch is still failing after the new tests — 12 lines remain uncovered.
The most addressable gap is the success path of force_kill_fsmonitor_via_socket (lines 175-177): both new tests in this commit exercise no-op paths (unresolvable git dir; socket file with no holder), so the for pid in ... { terminate_pid(pid); } body never runs. A test that spawns a sleeping child whose fd holds the IPC socket path before stop_fsmonitor_daemon/force_kill_fsmonitor_via_socket is invoked would cover the branch — lsof would return the PID and the loop would terminate it.
Two other gaps are hard to reach from a unit test:
- Lines 166-168 (
Cmd::new("lsof").run()returnsErr) — needslsofmissing or timing out. - Lines 193-194 (SIGTERM returns
Err) — needs a PID that's already gone or unsignallable.
Separately: test_terminate_pid_sigterm_honored doesn't actually exercise lines 199-201 (the "exited after SIGTERM" early return), even though its docstring says it does. After SIGTERM the child becomes a zombie, and kill(pid, None) on a zombie returns Ok (the PID stays in the process table until reaped), so the poll loop never sees Err(ESRCH). The loop runs out the full FSMONITOR_SIGTERM_GRACE window and SIGKILLs the zombie as a no-op. The test still passes (child exited with a signal-derived status), but the fast-path return at line 201 isn't reached — which is why it shows uncovered. Reaping the child before invoking terminate_pid flips the branch the test exercises (then kill(pid, SIGTERM) itself hits ESRCH on line 190, so it covers 193-194 instead). Covering line 201 specifically would need a double-fork so the grandchild is auto-reaped by init.
The remaining two uncovered lines (600, 606) are assert! format args inside an existing test — unreachable unless the assertion fails, which is the usual codecov noise.
Dismissing my earlier approval until the patch coverage is addressed, per CLAUDE.md's codecov policy. Happy to push a test for the force-kill success path if you'd like.
codecov/patch is failing — see review for analysis
Resolves a CHANGELOG.md conflict introduced by the v0.52.0 release (#2817) landing on main while this branch was open; the fsmonitor reap entry now sits under `## Unreleased` above the released `## 0.52.0` section. src/output/handlers.rs auto-merged cleanly — the `stop_fsmonitor_daemon` call still runs synchronously on the foreground removal path after the #2806 hook-plan refactor. Co-Authored-By: Claude <noreply@anthropic.com>
Resolves doc + CHANGELOG conflicts after #2813 landed: - CHANGELOG.md: moves the orphan-sweep entry from `## 0.51.0 / Fixed` (where it was incorrectly placed when this branch first diffed against 6fd2b70) into `## Unreleased / Fixed`, alongside #2813's wedged-daemon-on-removal entry. - docs/content/faq.md: keeps both "Other cleanup" bullets — #2813's synchronous-on-removal force-kill and this PR's background orphan-sweep (distinct mechanisms, both worth documenting). - skills/worktrunk/reference/troubleshooting.md: collapses two near-duplicate `pkill` snippets into one and combines #2813's wt-remove-reaps-even-wedged paragraph with this PR's residual-case-on-a-live-worktree paragraph. - skills/worktrunk/reference/faq.md: regenerated by the doc-sync test from docs/content/faq.md. src/main.rs / src/git/mod.rs / src/commands/process.rs auto-merged cleanly; 18 fsmonitor unit tests pass on the merged tree, including this PR's `daemon_from_lsof_*` tests and #2813's `terminate_pid` tests. Co-Authored-By: Claude <noreply@anthropic.com>
## Why Companion to #2813. That PR stops the fsmonitor-daemon leak at its source (`wt remove` force-terminating a wedged daemon). This PR is defense-in-depth for daemons orphaned by paths that bypass `wt remove` entirely — plain `git worktree remove`, a manual `rm -rf`, or a crashed `wt` — which #2813 cannot cover. Background: a wedged `git fsmonitor--daemon` makes `git status` (and `wt list`) hang; orphans had accumulated ~80-deep on one machine. ## What Adds an orphan sweep (`src/git/fsmonitor.rs`: `classify_orphans`, `reap_orphan_fsmonitor_daemons`) hooked into the **existing** `wt remove` internal sweep op — no new always-on/timer mechanism, and deliberately not wired to `wt list` or any read-only command (killing processes as a side effect of a read-only command is out of bounds). It reaps only daemons whose IPC socket resolves to a worktree that no longer exists, SIGTERM → bounded wait → SIGKILL, socket-scoped via `lsof -a -p <pid>` (never broad process-name matching). A daemon serving a *live* worktree is never reaped — proven by unit tests, and the invariant holds even when the live-worktree set is unknowable: `live_git_dirs` returns `Option`, and an unknowable set disables resolved-socket reaping entirely rather than falling back to "reap everything". Shares low-level fsmonitor mechanics with #2813 and both touch `faq.md` / `troubleshooting.md` / `CHANGELOG.md`; whichever of the two lands second needs a small dedup and doc reconciliation. ## Testing `wt hook pre-merge --yes` green (3757/3757). 11 deterministic unit tests cover the pure logic, including `live_worktree_daemon_is_never_reaped` and `unknowable_live_set_spares_resolved_socket_daemons`. `src/git/fsmonitor.rs` is a new file, so it is entirely in patch scope: 94.5% line / 95.4% region. The uncovered lines are real-OS-signal and live daemon-enumeration paths only reachable by spawning real `git fsmonitor--daemon` processes; their pure logic is exhaustively unit-tested. A real-daemon end-to-end test was prototyped and dropped as flaky and net-negative (daemon-spawn races; risk of SIGKILLing unrelated dev-machine daemons). These uncovered OS-only lines are an accepted trade-off — if `codecov/patch` flags them it is a known false positive, not a gap to fill with a fragile test. --------- Co-authored-by: Claude <noreply@anthropic.com>
Why
wt removealready callsgit fsmonitor--daemon stopbefore staging a worktree to trash, but that stop is an IPC request to the daemon itself. When the daemon is wedged — the common failure that makesgit status(and thereforewt list) hang — it can't answer the IPC, so the stop is a silent no-op and the daemon leaks. In practice this accumulated ~80 orphanedgit fsmonitor--daemonprocesses on one machine, one of which wedged and hungwt listoutright.What
Adds a canonical
stop_fsmonitor_daemonhelper: after the IPC stop, it resolves the daemon's PID from its IPC socket and escalates SIGTERM → bounded wait → SIGKILL, scoped strictly to the removed worktree's own socket (never matched by process name, never another worktree's daemon). All three removal paths (library, foreground, detached-background) route through the one helper; the redundantgit ... fsmonitor--daemon stopshell fragment in the detached path is deleted so there is a single canonical stop. Force-kill is Unix-only with a clean#[cfg(not(unix))]no-op (Windows keeps the IPC stop, where the daemon uses a named pipe).This is the source-level fix for the leak on the removal path. Companion PR #2814 adds a defense-in-depth sweep for daemons orphaned by paths that bypass
wt remove(plaingit worktree remove, manualrm, a crashedwt). The two branches share low-level fsmonitor mechanics and both touchfaq.md/troubleshooting.md/CHANGELOG.md, so whichever lands second will need a small dedup and doc reconciliation.Testing
Unit tests cover the deterministic logic: git-dir/socket resolution for a linked worktree, the SIGTERM-honored fast path, and SIGTERM-ignored → SIGKILL escalation (with a readiness handshake to avoid a signal-vs-trap-install race).
wt hook pre-merge --yesis green. Thelsof-failure and git-dir-error arms are single best-effortlog::debug!+return lines that fall through to the existing fail-open behavior.