Skip to content

fix: exempt the foreground thread from the command semaphore - #3544

Merged
max-sixty merged 3 commits into
mainfrom
switch-return-async
Jul 22, 2026
Merged

fix: exempt the foreground thread from the command semaphore#3544
max-sixty merged 3 commits into
mainfrom
switch-return-async

Conversation

@max-sixty

Copy link
Copy Markdown
Owner

Accepting a row in the wt switch picker could stall for ~10s on a large repo — and indefinitely under sustained background traffic — before the switch began. Every Cmd::run acquires the global 32-permit command semaphore, so the accept path's first command queued behind the picker's per-row preview diffs, which on a 38-worktree rust-lang/rust fixture each held a permit for ~14s (trace: Enter at t=4.3s, first switch command spawned at t=14.3s, all of it inside semaphore().acquire()). Since permits go to an arbitrary waiter (Condvar::notify_one), the foreground can also lose every race while background tasks keep replenishing the queue.

The foreground thread — the one that enters main() — now bypasses the semaphore in run and pipe_into. It executes commands one at a time, so it can't contribute to the fan-out the semaphore exists to bound, and it's what the user is waiting on. stream/delayed_stream already followed this rule; background pool tasks keep the full 32-permit cap. The cap was already approximate (rayon work-stealing can run a capped closure on the calling thread), so no permits are reserved.

Verified with a PTY driver against a repo whose git shim sleeps 15s on diff, with WORKTRUNK_MAX_CONCURRENT_COMMANDS=2: accept-to-exit was >100s before (killed) and 0.08s after; a control run of the unfixed binary with an unsaturated cap also exits in 0.08s, isolating the semaphore as the cause. A unit test covers the exemption (initializing thread exempt, spawned threads capped).

This was written by Claude Code on behalf of max

max-sixty and others added 2 commits July 22, 2026 02:40
Background fan-out (per-row collect and preview tasks) shares one
32-permit semaphore with every `Cmd::run`. The foreground thread runs
commands sequentially, yet it queued for a permit like any pool task, so
a saturated pool stalled it: accepting a row in the `wt switch` picker
while per-worktree preview diffs held every permit blocked the switch
until background work drained — ~10s on a 38-worktree rust-lang/rust
fixture, and unboundedly under sustained traffic (permits are handed to
an arbitrary waiter, so the foreground can lose every race).

The thread that enters `main()` now bypasses the semaphore in `run` and
`pipe_into` — it can't contribute to fan-out, and it is what the user is
waiting on. `stream`/`delayed_stream` already followed this rule.

Reproduced with a PTY driver, a `git` shim sleeping 15s on `diff`, and
WORKTRUNK_MAX_CONCURRENT_COMMANDS=2: accept-to-exit was >100s before
and 0.08s after (control: 0.08s pre-fix with an unsaturated cap).

Co-Authored-By: Claude Fable 5 <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.

Looks correct and cleanly scoped — the foreground exemption is what the user waits on, it can't fan out past one command from a single thread, and the rayon work-stealing overage is acknowledged. init_startup capture ordering (first line of main, before the rayon pool spawns) is right, the rename is complete, and the Option<SemaphoreGuard> RAII holds/drops correctly in both branches. The unit test pins both directions of the exemption.

One small doc drift, just outside the diff: pipe_into's doc still says "The pipeline consumes one semaphore permit even though it runs two processes concurrently" — now that the foreground thread bypasses the guard, that's only true on background threads (foreground consumes zero). Happy to push a one-line tweak to that comment if you'd like; not blocking.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@max-sixty
max-sixty merged commit a3de0b0 into main Jul 22, 2026
51 checks passed
@max-sixty
max-sixty deleted the switch-return-async branch July 22, 2026 20:46
max-sixty added a commit that referenced this pull request Jul 24, 2026
Highlights: clickable CI and dev-server-URL statusline segments in Claude Code (#3550), `wt merge --no-rebase` accepting merge-shaped fast-forward histories (#3509), a faster and lower-CPU `wt switch` picker (#3544, #3534), and SignPath code signing for the Windows release binary (#3553). Full details in CHANGELOG.md.

> _This was written by Claude Code on behalf of max_
max-sixty added a commit that referenced this pull request Jul 24, 2026
…3557)

## Summary

Follow-up to #3544's `wt switch` review, from investigating why the
accept path re-forks `git config --list -z` and rebuilds `Repository`
more than necessary.

- Skip the destination-rooted `Repository::at()` in
`spawn_switch_background_hooks` when the approved hook plan is empty
(the common no-project-hooks case) — `HookAnnouncer::flush()` is already
a no-op there, so this changes no behavior, only cost.
- Fix the picker's `is_recovered` accept-path arm, which reused the
startup-time `Repository` unconditionally. An in-picker alt-x/alt-r
during a recovered session can mutate the worktree/branch inventory, and
that arm never rebuilt to see it — the non-recovered arm already gets
this via a fresh `Repository::current()`. Rebuild via `Repository::at`
(mirrors the picker's own `rebuild_repo` idiom) instead of
`Repository::current()`, which fails after a deleted-CWD recovery.
- Replace a misleading comment ("reuse the recovered repo") with the
actual freshness rationale.

A fourth change — sharing the bulk config cache across same-process
`Repository` instances by `git_common_dir` — was implemented and then
reverted: the pre-merge gate's
`test_primary_remote_honours_checkout_default_remote` caught a real
staleness bug (a config mutation between two `Repository::at()` calls,
e.g. from a switch hook, would go unobserved by a cache hit with no
invalidation path). Measured idle-repo cost of the remaining duplicated
config forks is small (~5-10ms each), and the risk of a bespoke
process-wide cache with no write-invalidation wasn't worth it.

## Test plan

- [x] `cargo run -- hook pre-merge --yes` (full test + lint suite): 4483
tests passed
- [x] `cargo clippy --all-targets --features shell-integration-tests --
-D warnings`: clean
- [x] Targeted tests: `git::repository::`, `picker::`,
`worktree::switch::`, `hook_plan::`

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

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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