Skip to content

perf(fsmonitor): resolve all daemons in one lsof spawn - #3581

Merged
max-sixty merged 4 commits into
mainfrom
fsmonitor-batch-lsof
Jul 25, 2026
Merged

perf(fsmonitor): resolve all daemons in one lsof spawn#3581
max-sixty merged 4 commits into
mainfrom
fsmonitor-batch-lsof

Conversation

@max-sixty

@max-sixty max-sixty commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Why

wt remove's end-of-command fsmonitor sweep forked one lsof per machine-wide git fsmonitor--daemon — and with core.fsmonitor enabled globally, a machine accumulates one daemon per repo ever touched (100+ is routine). So every wt remove paid ~100 process spawns. The cost is superlinear under load, not linear: a fork storm makes macOS Gatekeeper/XProtect assess each new image under contention, inflating per-spawn cost for everything else on the box, sweeps included. The ~50ms each figure in the old docstring was measured under that load and read as a fixed per-call cost — so the sweep looked merely slow rather than self-amplifying. Two concurrent test suites driving hundreds of wt remove calls pinned all 18 cores (load average 142) with nothing hot in htop.

This was diagnosed and measured before (#2814 added the sweep; #3401 wrote the cost into the docstring and explicitly changed no behavior) but never fixed.

The fix

Resolve every daemon in a single lsof -a -p <pid1,pid2,…> -U -F pn call and split its output on the p<pid> record boundary, handing each PID exactly the output the per-PID call produced — so classification and the whole data-safety contract in the module docs are unchanged. Verified end-to-end against a live 108-daemon machine with a PATH shim counting invocations: 108 spawns → 1.

Two details the batched path must preserve, each covered by a new test:

  • Don't gate on lsof's exit status. It exits non-zero when any requested PID vanished mid-scan, while still printing full records for every survivor. Gating would drop the whole sweep whenever one daemon happened to exit — the common case on a busy machine.
  • A PID with no record is dropped, never synthesized as a socket-less entry. A socket-less entry reads as orphan class 1 and gets SIGTERM/SIGKILL — on a possibly-recycled PID.

Also here

  • Test consolidation. Three integration fixtures built their branch set by spawning git per loop iteration. A new TestRepo::create_branches creates them in one git update-ref --stdin. The over-threshold completion test drops 10.5s → 1.4s (~230 git spawns → 5); switch_picker and statusline branch loops get the same treatment. mock-stub gains an opt-in MOCK_CALL_LOG_DIR call log so a test can assert spawn count, not just response — deliberately outside the repo under test, since a log written inside the tree would dirty it mid-wt merge.

  • Docs. Dropped the "disable core.fsmonitor globally" workaround from the troubleshooting docs (canonical skills/, mirror regenerated). It stays enabled by choice, and the daemons were never the actual fseventsd CPU driver.

  • CI hardening. pre-commit.ci's typos hook maps the token pnon and had auto-rewritten lsof -F pn to the broken lsof -F on (offset+name — no p<pid> records, so the parser reaps nothing). Pinned pn in .typos.toml next to the existing PN-from-PNGs entry that documents the same mapping, and added a test covering the empty-PID-set early return (flagged by codecov/patch).

Testing

New unit tests cover daemons_from_batched_lsof against real captured lsof -F pn output, a vanished-PID gap, and empty output; the rewritten integration test asserts exactly one lsof spawn with a comma-joined PID list. Full integration suite green (1960 passed), clippy and fmt clean.

This was written by Claude Code on behalf of max

max-sixty and others added 2 commits July 24, 2026 16:22
The end-of-command sweep forked one lsof per machine-wide fsmonitor daemon (100+ is routine), so every `wt remove` paid ~100 process spawns. The cost is superlinear under load, not merely linear: a fork storm makes macOS Gatekeeper/XProtect assess each new image under contention, inflating per-spawn cost for every other spawn on the box, sweeps included. The `~50ms each` figure in the old docstring was measured under that load and read as a fixed per-call cost.

Batch the resolution into a single `lsof -a -p <pids> -U -F pn` and split its output on the `p<pid>` record boundary, handing each PID exactly the output the per-PID call produced — so classification and the data-safety contract are unchanged. Verified end-to-end against a live 108-daemon machine: 108 spawns -> 1.

Two details the batched path must preserve, both covered by new tests:
- Do not gate on lsof's exit status. lsof exits non-zero when any requested PID vanished mid-scan while still printing full records for every survivor; gating would drop the whole sweep whenever one daemon happened to exit.
- A PID that produced no record is dropped, never synthesized as a socket-less (class-1 orphan) entry, which would SIGTERM/SIGKILL a possibly-recycled PID.

Also consolidate three integration fixtures that spawned git per loop iteration via a new `TestRepo::create_branches` (one `git update-ref --stdin`): the over-threshold completion test drops 10.5s -> 1.4s (~230 git spawns -> 5), plus the switch_picker and statusline branch loops. The mock-stub gains an opt-in `MOCK_CALL_LOG_DIR` call log so a test can assert spawn count, not just response.

Drop the "disable core.fsmonitor globally" workaround from the troubleshooting docs: it stays enabled by choice, and the daemons were never the actual fseventsd CPU driver.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
worktrunk-bot
worktrunk-bot previously approved these changes Jul 24, 2026
pre-commit.ci's typos hook maps the lowercase token `pn` to "on" and had auto-fixed `lsof -F pn` to `lsof -F on` across the sweep. That silently breaks it: `on` requests offset+name, dropping the `p<pid>` record opener that `daemons_from_batched_lsof` splits on, so the parser would yield zero daemons and reap nothing.

Restore the field spec (and its doc mentions) and add `pn` to `.typos.toml` `extend-words`, next to the existing `PN`-from-`PNGs` entry that documents the same pn->on mapping.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codecov/patch is red on this PR — 97.36% of the diff hit vs a 97.83% target. The two uncovered added lines are both return Vec::new() early-returns in the new enumerate_daemons (src/git/fsmonitor.rs):

  • the empty-PIDs guard — if pids.is_empty(), reached when pgrep exits 0 but yields no parseable PID;
  • the lsof-spawn-failure backstop — the let Ok(out) = … else arm.

Only one of the two needs covering to clear the gate (one more hit takes the 76-line diff to ~98.7%, above target). The empty-PIDs guard is the clean one to test: mock pgrep to emit non-numeric output (exit 0) so the parse::<u32>() filter drops it to an empty set — the same mock shape as test_remove_resolves_all_fsmonitor_daemons_in_one_lsof. The spawn-failure arm is a load-bearing backstop that's awkward to trigger deterministically, so I'd leave it uncovered.

Happy to push that test if you'd like. Otherwise, per the repo's codecov policy these defensive returns need your explicit sign-off to merge over the gate.

Dismissing the earlier approval — it landed ~6 minutes before codecov's verdict was in — until the gap is closed or you approve merging over it. The incremental typos pin (pn = "pn") restoring the intended -F pn field spec looks correct.

@worktrunk-bot
worktrunk-bot dismissed their stale review July 24, 2026 23:38

codecov/patch failing (97.36% vs 97.83% target) — this approval landed before codecov's verdict was in. Dismissing until the empty-PIDs guard in enumerate_daemons is covered or the merge is explicitly signed off.

codecov/patch flagged the `if pids.is_empty()` guard in `enumerate_daemons` as uncovered. Add an integration test that mocks `pgrep` to exit 0 with a non-numeric line, so the PID set parses empty and the sweep returns before spawning `lsof` — asserted by zero `lsof` spawns and the absence of the per-daemon "resolving sockets" trace line.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@max-sixty
max-sixty merged commit a0c1b66 into main Jul 25, 2026
39 checks passed
@max-sixty
max-sixty deleted the fsmonitor-batch-lsof branch July 25, 2026 00:27
@max-sixty max-sixty mentioned this pull request Jul 25, 2026
max-sixty added a commit that referenced this pull request Jul 25, 2026
Cuts v0.69.2, and fixes the Windows code-signing path it depends on.

## The signing fix

v0.69.1 shipped an unsigned `wt.exe` under a green run and a
**Completed** SignPath signing request. `archive: false` (#3566) had
already made the GitHub artifact name
`worktrunk-x86_64-pc-windows-msvc.zip`, and SignPath names its download
after the artifact — so with `output-artifact-directory: target/distrib`
the signed zip landed at `worktrunk-x86_64-pc-windows-msvc.zip.zip`,
beside the untouched unsigned build. The checksum step and the release
upload both kept reading the original. Confirmed by parsing the
published asset's PE certificate table: `size=0`.

The download now goes to a scratch directory and whatever single file
lands there replaces the built zip, so SignPath's naming isn't
load-bearing.

## Verification, because this failure is invisible

Signing is `continue-on-error` by design (self-signed test certificate
pending SignPath's OSS review), so nothing in the logs distinguishes
"signed" from "silently unsigned" — which is how it slipped through
twice, two different ways. `.github/verify-windows-signature.py` reads
the zip's PE certificate tables directly, and runs at two points with
distinct jobs:

- **before the overwrite** — an unsigned release is tolerable while the
certificate is a test one; a corrupt one never is, so the replacement
has to verify before it can clobber a good build.
- **after** — reports what the release actually ships, which is the
question the logs never answered.

## Rehearsed before landing

The signing path only runs on a tag, so each question about it used to
cost a release. This chain was instead run on a Windows runner against
the already-published v0.69.1 zip (identical bytes, no build):

```
saved to …\target\signpath\worktrunk-x86_64-pc-windows-msvc.zip.zip
git-wt.exe: certificate table 8480 bytes
wt.exe:     certificate table 8472 bytes
→ mv → target/distrib/worktrunk-x86_64-pc-windows-msvc.zip
target/distrib/worktrunk-x86_64-pc-windows-msvc.zip: all 2 executables signed
worktrunk-x86_64-pc-windows-msvc.zip: OK   (checksum matches)
```

That also settled the open question behind #3556: SignPath returns the
signed zip itself, not a wrapper, so `skip-decompress: true` is correct
— the extract mode does explode it into loose files.

## Release contents

`wt remove`'s fsmonitor sweep (#3581), the troubleshooting doc trim, and
this fix. The two refactors in range (#3582, #3584) are
behavior-preserving and omitted per convention.

Local gate green (4547 passed). Changelog entries verified against the
diffs by subagent; two claims corrected (the doc entry's rationale, and
an overclaim attributing v0.69.0's unsigned binary to this bug rather
than the separate upload failure #3566 fixed). Data-loss surface
reviewed across the cumulative diff.

> _This was written by Claude Code on behalf of max_
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants