Skip to content

fix(worktree): serialize worktree admin writes per repo and attribute git to its create - #22342

Closed
brennanb2025 wants to merge 3 commits into
mainfrom
brennanb2025/worktree-admin-lock
Closed

brennanb2025 wants to merge 3 commits into
mainfrom
brennanb2025/worktree-admin-lock

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 16 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1031 $\color{#cf222e}{\Huge{\mathbf{−}}}$​8 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1023
Prod 25 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​863 $\color{#cf222e}{\Huge{\mathbf{−}}}$​302 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​561

ELI5

Scope: this PR adds per-repo serialization of worktree admin writes and trace attribution. It does not reduce create latency. The dominant cost in the reported slow create (waiting on a claimed prepared checkout that is still being built) is untouched; see Remaining gaps.

Git keeps a small bookkeeping folder per repository listing every worktree. Every git worktree add / remove / move / lock / unlock / repair / prune rewrites that folder and walks every entry in it. In a repo with ~800 worktrees, two of those commands running at once on the same repo slow each other down. Orca now lets only one of them run per repository at a time, lets a user's create go before background warm-up, and tags every git command in the trace with the create (or warm-up) that issued it, so a slow create can be explained from the logs.

What Changed

Before. Nothing stopped two admin commands from running at once on one repo. So a user's create could overlap the prepared-checkout pool warming up, a removal, or a prune. In the trace, a pool warm-up's git showed up as parentless root spans, or parented to whichever create happened to fire the re-arm. Runtime/CLI creates opened no worktree.create span at all. So the 32 s git_worktree_add phase could not be tied to specific commands.

After.

  • One lock policy at the git exec layer. gitExecFileAsync (main) and the relay's git() already recognize argv that needs a per-repo lock (fetch/pull → the FETCH_HEAD lane). That recognition now lives in runWithGitExecLocks (src/main/git/command-runner/git-exec-lock-policy.ts) and also recognizes worktree admin mutations. Those queue on a lane keyed by <common-dir>/worktrees. worktree list is a read and stays unlocked. Every existing and future caller is covered without touching call sites: create, cold add, sparse add, pool arm/finalize/discard, stale-preparation cleanup, removal, prune, move, and the relay.
  • Common dir without a subprocess, cached. The .git-marker/commondir walk the FETCH_HEAD lock already used moved to src/shared/git-common-dir-location.ts, so both locks share it. The admin lock caches the result per (cwd, --git-dir), holding at most 256 entries, and only when a real .git was found. It never runs rev-parse.
  • Priority. runWithGitOperationLock went from a promise chain to an explicit waiter queue. Higher priority is granted first, arrival order holds within a priority, and an aborted waiter leaves the queue immediately. The admin lane takes its priority from the existing git admission tier: creates run interactive (2), pool work runs status (1), and stale-preparation reclaim runs background (0). A queued create therefore goes before a queued re-arm. The existing FETCH_HEAD and per-checkout locks keep plain FIFO.
  • Only the admin write holds the lock, not the checkout. A plain git worktree add writes the admin entry and checks out every file and runs the post-checkout hook, all in one process. Holding the lane for that whole process would serialize every checkout in the repo (the reported trace has a 134.9 s cold add). Cold creates on the runtime/CLI path, the IPC path, and the relay now run worktree add --no-checkout under the lock, then run reset --hard --no-recurse-submodules in the new worktree and the post-checkout hook after the lane is released. The shared helper is src/shared/worktree/split-worktree-add.ts. The prepared pool already arms this way.
  • Post-checkout hook behavior is kept. reset --hard does not run hooks. So after it, Orca runs git hook run --ignore-missing post-checkout -- <null oid> <new HEAD> 1. Those are the same three arguments git passes, and the hook runs from the new worktree as before. The one difference is that GIT_DIR is set in the hook's environment, which is also true when a user runs a normal git checkout. A failing hook still fails the create and keeps the worktree, like the plain command. A failed or cancelled checkout removes the worktree and keeps the branch, also like the plain command. That cleanup worktree remove --force runs without the create's cancel signal, with its own 60 s timeout, so cancelling a create mid-checkout does not leave a registered, half-filled worktree behind. git hook run needs Git 2.36+. It is probed once per execution host through GitCapabilityCache (new capability hook-run). On older Git the plain full worktree add runs under the lock instead of skipping the user's hook.
  • One deadline for the whole split add. The admin write, checkout, rev-parse and hook share the single worktree add timeout (default 180 s, or ORCA_WORKTREE_ADD_TIMEOUT_MS). Each step gets only what is left, so the split add is bounded the same as the plain command, not three times over.
  • Tree deletion does not hold the lock. An inline worktree remove (removal fallback, submodule --force retry, discard of a prepared checkout, and the relay's equivalents) deletes the whole working tree. Those calls pass worktreeAdminLock: false and skip the lane. Git's own lock files still guard the admin entry.
  • Bounded wait. A command waits at most 15 s for the lane. After that it logs worktree admin lock still busy … running unlocked and runs without the lane, the same as before this PR. It does not release the current holder. The span attribute git.worktree_admin_lock_held records which case applied. A create can no longer wait behind N queued commands times each one's timeout.
  • Held until the child exits. On abort, the lane now stays held until the killed git process has actually exited, not just until the exec promise settles. In main, the exec layer ties the lease to its existing termination barrier. In the relay, admin commands always run through the termination-aware runner.
  • Bookkeeping never gates the command. If the lock key cannot be derived for any reason other than cancellation, the command logs a warning and runs unlocked. Git's own lock files still own correctness; this lock only shapes load.
  • Trace attribution. Spans can now declare themselves an operation. Descendants inherit it, and each git.exec span gets git.operation and git.operation_span_id, plus git.worktree_admin_lock_wait_ms on admin mutations. worktree.create is an operation, for both the IPC create and now the runtime/CLI create. Pool work runs in its own root worktree.prepare span with reason = prefetch | rearm | discard and triggered_by_span_id, so it can no longer inherit the create that fired it. When a create claims a checkout that is still being built, the wait gets its own worktree.create.await_preparation span naming the preparation's span id.

Why

What actually happened in the reported 37 s create. I read the create path on a prepared-checkout hit end to end and checked it against the raw trace (main.trace.ndjson, trace 4a972071…: 37 365 ms total, git_worktree_add 32 181 ms, hit, not retargeted).

Git commands that run inside git_worktree_add on a hit, in order:

  1. Claim: rev-parse of the base (when selection needs the canonical base); on a retarget, the divergence probes (rev-parse, rev-list, merge-base).
  2. Wait for the claimed entry's ready. If the entry is still arming, this covers that preparation's worktree list --porcelain (stale scan), worktree add --detach --no-checkout, reset --hard, and worktree lock. All of it runs in the preparation's context, not the create's.
  3. Finalize, which resolves the base in parallel:
    • base resolution (rev-parses, and when base refresh/suggestion is on: worktree list --porcelain + status per sibling checkout)
    • rev-parse --verify HEAD in the prepared checkout
    • reset --hard <target> if HEAD drifted
    • worktree move -f -f, checkout --no-track -b
    • config (creation base, push.autoSetupRemote)
    • worktree unlock

In that trace:

  • From +1.7 s to +25.3 s the create ran no git at all. It was in step 2. The claimed entry's worktree add --detach --no-checkout started at +1.2 s (1.3 s). Its reset --hard ran 21 805 ms in a separate root trace. Its worktree lock ran at +24.3 s. The create resumed at +25.3 s.
  • The rest was sequential: probes about 0.4 s; worktree list 906 ms and 1289 ms, each followed by status; a drift reset --hard 458 ms; worktree move 2926 ms; checkout -b 2073 ms; worktree unlock 1312 ms.
  • The 134.9 s worktree add about 169 s earlier has 7 arguments. That is the cold --no-track -b <branch> <path> <base> shape; a pool arm has 6. So it was most likely a runtime/CLI cold create, which had no create span, rather than a pool re-arm.

So this PR does not remove the dominant 23.6 s, and does not claim to make creates faster. That time is a create claiming a "hit" whose checkout is still materializing. This PR keeps admin writes on one repo from overlapping each other, keeps checkouts and hooks out of that critical section, and makes the 23.6 s visible and named in the trace. A still-arming preparation will still hold up the create that claims it. Fixing that changes create behavior and belongs in its own PR; see Remaining gaps.

Alternatives considered.

  • Wrap each call site in a lock helper. Rejected: more than 12 sites across main and the relay, and the next new one would be missed. The exec layer already classifies argv for the FETCH_HEAD lane, so this extends that one policy instead of adding a second mechanism next to it.
  • Resolve the common dir with git rev-parse --git-common-dir per command. Rejected: that adds a subprocess per admin command, and the file walk already exists and handles WSL/UNC spellings.
  • A separate priority scheduler for pool work. Rejected: the admission tier already records who is waiting, so the lock reads it.
  • Hold the lock for the whole plain worktree add. Rejected: it serializes every checkout and hook in the repo behind one lane.
  • Split the add but skip post-checkout. Rejected: it would silently drop users' hooks (for example dependency installs). On Git without git hook run, Orca keeps the plain add instead.
  • No wait bound; rely on each command's timeout. Rejected: a queue of N commands lets a create wait N × the add timeout.

Linked Issue

No linked issue.

Visual Proof

N/A: no UI change. Evidence is the trace numbers above and the benchmark below.

Testing

  • Automated tests added/updated
  • Manual testing in the running app: not done; see gaps

New tests. Each one was confirmed red with its mechanism removed.

Test Covers Ablation that reddens it
src/shared/git-operation-lock.test.ts arrival order; create priority over re-arm over cleanup; abort frees the slot and later waiters still run; pre-aborted callers; release on throw priority ignored → the priority case fails
src/shared/git-worktree-admin-lock.test.ts argv classification (add/remove/move/lock/unlock/repair/prune locked; list/status/fetch not); main checkout and linked worktree share one lane; common dir resolved once; lock-key failure runs unlocked; a command's own failure is not re-run; cancellation propagates rethrowing on key failure → unlocked-run test fails; cache disabled → resolved-once test fails
src/main/git/worktree-admin-lock-real-git.test.ts real git, one temp repo: holds the repo's admin lane, starts a pool re-arm, then a prepared-hit create; asserts both queued, no admin command ran while held, admin spans never overlap, the create's worktree move ran first, both succeed, git spans carry the right operation ids exec layer not locking → fails (queue never forms); priority ignored → fails (worktree.prepare ran first); prepare span not a root operation → fails (re-arm git not attributed)
src/main/runtime/runtime-local-worktree-create.test.ts (new case) runtime/CLI create git runs inside a worktree.create operation span removed → fails
src/main/git/worktree-admin-lock-real-git.test.ts (new cases) real git, non-Windows: a cold addWorktree whose post-checkout hook blocks. While the hook runs the admin lane is free and a worktree prune completes; the hook got null oid, new HEAD, 1, and the files are checked out. Discarding a prepared checkout completes while the lane is held. split add removed (plain add under the lock) → "lane held" fails; discard opt-out removed → fails
src/main/git/worktree-admin-lock-real-git.test.ts (cancel case) real git, non-Windows: a smudge filter holds the checkout open, the create is cancelled, and the worktree must not stay registered run against the previous head of this PR → fails (worktree still listed); removing only the cleanup's detach → fails
src/shared/worktree/split-worktree-add.test.ts split command order and hook arguments; probe once per cache; Git without git hook run keeps the plain add and is remembered; failed checkout removes the worktree detached and without the lane; failed hook keeps it; WSL routing invalidation runs even when the add fails; steps share one deadline; a spent deadline fails with ETIMEDOUT and cleans up shared deadline removed → both deadline cases fail
src/shared/git-operation-lock.test.ts, src/shared/git-worktree-admin-lock.test.ts (new cases) bounded wait expires → runs unlocked without releasing the holder; holdUntil keeps the lane until settled; admin lock warns and runs unlocked past its bound bound ignored → fails
src/main/git/command-runner/git-exec-admission-lifetime.test.ts, src/relay/git-handler-termination.test.ts (new cases) an aborted admin command keeps the lane until its child exits (main and relay) holdUntil line removed → fails; relay not routed to the termination runner → fails
src/main/git/remove-worktree.test.ts, src/relay/git-handler-worktree-ops.test.ts inline tree-deleting removes skip the lane opt-out removed → fails

Commands run.

  • vitest run src/main/git src/relay src/shared/worktree src/shared/git- src/main/runtime/runtime-local-git-worktree: 494 files, 4974 passed, 33 skipped (the skips include the opt-in benchmark and Windows-only cases). Suites that queue git replies in order pin the plain-add path; the split add has its own tests.
  • pnpm tc: passes (all projects), after rebasing onto current main
  • pnpm run check:code-quality:changed: passes, 0 findings across 40 changed files
  • full oxlint on the touched files: clean

Benchmark (opt-in src/main/git/worktree-admin-contention.bench.test.ts). Synthetic repo in /tmp with 600 linked worktrees and 4000 files. Each trial arms a prepared checkout, then runs a pool re-arm and a create that consumes it at the same moment. Per-command timings are from the create's own trace, in ms; "wait" is the admin-lock wait. Before = origin/main, after = this branch, same machine.

create total worktree move worktree unlock other git in the create
before, trial 0 1292 404 552 rev-parse 41/57, checkout 139, config 51/34/38
before, trial 1 1176 468 434 rev-parse 31/32, checkout 181, config 27/29
before, trial 2 1631 566 649 rev-parse 49/66, checkout 243, config 28/69
after, trial 0 1380 317 (wait 0) 542 (wait 248) rev-parse 32/58, checkout 92, config 29/25/58
after, trial 1 1476 417 (wait 0) 490 (wait 321) rev-parse 30/34, checkout 122, config 44/43
after, trial 2 1614 412 (wait 0) 378 (wait 416) rev-parse 100/102, checkout 141, config 96/52

What this shows, plainly:

  • The create's worktree move now gets the lane first, with zero wait, and runs a little faster.
  • Its later worktree unlock waits 250–420 ms behind the re-arm's worktree add. That add was granted while the create was running checkout/config, and a running command is never preempted.
  • Net create time on this idle SSD is unchanged, within noise. The fixture's admin dirs are small and the disk is not under load, so this does not reproduce the I/O amplification behind the reported 32 s. I have no measurement showing the lock shortens creates under that load.

The benchmark predates the split add and the wait bound; it measures prepared-hit creates, which do not use the cold add path. Platforms: macOS only. Windows, Linux, WSL, and SSH are covered by unit and relay-path tests only.

AI Disclosure

Implemented with Claude (Claude Code).

Review

Remaining gaps / adjacent findings (not fixed here).

  1. Claiming a still-arming preparation (the dominant 23.6 s). claimPreparedWorktree takes an entry and awaits its ready even when the checkout has just started. It could treat an unfinished entry as a miss and do a cold add, or race the two. That changes create behavior, so it belongs in its own PR. The new worktree.create.await_preparation span measures it.
  2. Priority inversion. A create that is waiting on a claimed preparation's ready still has that preparation's admin commands queued at status priority. They are not promoted.
  3. FETCH_HEAD lock release vs. child exit. The admin lane now waits for the child to exit on abort. The existing FETCH_HEAD lane still releases when the exec promise settles. Changing that alters the order in which fetch callers enter admission, so it is left for its own change.
  4. Low-priority starvation. Continuous interactive admin work could starve pool warm-up. Creates are sporadic, so there is no aging.
  5. Process-local only. The Orca CLI's own process, a second Orca instance, and git run from a user's terminal are not serialized with the app.
  6. The removal path's worktree remove does not also take the per-checkout lock that pull/rebase use.

Agent skill upstream boundary

  • Not applicable

Notes

  • Cross-platform: key derivation reuses the existing WSL/UNC/drive-letter canonicalization from the FETCH_HEAD lock, which keeps its existing key-derivation tests unchanged and passing.
  • SSH: the relay's git() applies the same lock on the execution host, arrival order only because the relay has no admission tier.
  • Wire compatibility: no RPC or stream changes. CreateWorktreeResult.timing is unchanged. New span attributes stay in local traces only.
  • Performance: one cached file walk per repo, one extra performance.now() per admin command. The split cold add runs three git commands after the admin write instead of none: reset --hard (the checkout the plain command did internally), rev-parse HEAD, and hook run post-checkout. It also runs a one-time git hook run probe per execution host.
  • Docs: the tracked docs/reference/git-compatibility.md capability table gains the hook-run row.

Checklist

  • This PR is focused (one mechanism, the per-repo admin lane, plus the supporting trace attribution)
  • I explained what changed and why
  • N/A for visuals, with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path impact considered
  • pnpm lint, pnpm typecheck, pnpm test, pnpm build: typecheck and targeted tests and lint run locally; full suite and build left to CI

Every git command that rewrites a repository's worktree admin data
(worktree add/remove/move/lock/unlock/repair/prune) now runs under a
process-local queued lock keyed by the repo's git common dir, including
the prepared-checkout pool's arm, re-arm and finalize. A user create
outranks a pool re-arm; if the lock key cannot be derived the command
runs unlocked. git.exec spans now carry the create or pool preparation
span that issued them.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ Minor suggestion only — otherwise a clean, well-scoped change.

Reviewed changes

Read the complete 19-file diff at 5ca80b9 and traced both mechanisms through their callers; ran the new/affected lock suites plus the real-git, runtime-create, and observability tests (77 passing).

  • Per-repo admin lane at the exec layer — runWithGitExecLocks classifies worktree admin argv and queues it on <common-dir>/worktrees; worktree list stays unlocked, and the relay's git() gets the same treatment.
  • Priority waiter queue — runWithGitOperationLock moves from a promise chain to an explicit queue (higher priority first, arrival order within a priority). Moving acquire outside the try also fixes the old behavior where an aborted waiter released a lock it never held.
  • Shared common-dir walk — the .git-marker/commondir walk (no rev-parse) is extracted to git-common-dir-location.ts and consumed by both locks, with a bounded cache only when a real .git was found.
  • Operation-scoped trace attribution — withWorktreeSpan/withWorktreePrepareSpan declare an operation, git.exec gains git.operation/git.operation_span_id, and admin commands record git.worktree_admin_lock_wait_ms.

I confirmed the concurrency invariants of the new queue (no concurrent holders on a key, no stranded lane, no lost wakeup, ordering preserved under abort/reject) and the operation-attribution chain (no stale or cross-operation leakage, including the root: true prepare span). No critical issues.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/shared/git-worktree-admin-lock.ts Resolved
…its wait

Cold creates run `worktree add --no-checkout` under the per-repo admin
lane, then check out and run post-checkout (via `git hook run`) after
release; Git without `git hook run` keeps the plain add. Inline
tree-deleting removes skip the lane. Waits are bounded at 15 s, after
which the command runs unlocked, and an aborted admin command holds the
lane until its child exits in both main and the relay.
@brennanb2025
brennanb2025 force-pushed the brennanb2025/worktree-admin-lock branch from 5ca80b9 to 5fe9128 Compare September 22, 2026 23:24
@brennanb2025 brennanb2025 changed the title fix(worktree): serialize worktree admin mutations per repository fix(worktree): serialize worktree admin writes per repo and attribute git to its create Sep 22, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

The new hook-run probe does not work on Git 2.55+, so every cold create falls back to the plain worktree add under the admin lane — the long lock hold this PR is meant to remove — and the failed probe is never cached. This also leaves the new real-git test red on Git 2.55.0.

Reviewed changes

Reviewed the second commit (5fe9128, 30 files, +679/−85) against the prior review at 5ca80b9; read the full commit diff and ran the lock, split, and real-git suites (44 pass, 1 fail — detailed inline).

  • Split the cold add around the admin lane — addWorktreeWithCheckoutOutsideAdminLock runs worktree add --no-checkout inside the lane, then reset --hard and the post-checkout hook after release, with a plain-add fallback for hosts without git hook run.
  • Bounded the lane wait and tied release to child exit — maxWaitMs (15 s) makes a command log and run unlocked, and a holdUntil lease keeps the lane until an aborted or timed-out child exits, in both main and the relay.
  • Opted inline tree deletes out of the lane — worktreeAdminLock: false on the main removal fallback/force retry, discardPreparedWorktree, and the relay equivalents.
  • Retired the prior review nit — GitWorktreeAdminLockOptions.gitDir is removed.
  • Extended trace attribution — worktree.prepare root spans, worktree.create.await_preparation, and git.operation/git.operation_span_id; runtime/CLI create now opens a worktree.create span.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment on lines +29 to +47
return /is not a git command|unknown (sub)?command|unknown option|usage: git hook/i.test(text)
}

async function supportsHookRun(request: SplitWorktreeAddRequest): Promise<boolean> {
const { capabilities, git, globalArgs, repoPath } = request
if (capabilities.isKnownSupported('hook-run')) {
return true
}
if (!capabilities.shouldTry('hook-run')) {
return false
}
try {
await git([...globalArgs, 'hook', 'run', '--ignore-missing', HOOK_RUN_PROBE_NAME], repoPath)
capabilities.rememberSupported('hook-run')
return true
} catch (error) {
if (isHookRunUnsupportedError(error)) {
capabilities.rememberUnsupported('hook-run')
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

git hook run --ignore-missing orca-capability-probe fails on Git 2.55+ with error: unknown hook event 'orca-capability-probe'; use --allow-unknown-hook-name to allow non-native hook names. So supportsHookRun returns false, every cold create falls back to the plain worktree add under the admin lane (the long hold this PR removes), and because that message matches none of the isHookRunUnsupportedError patterns the failure is not cached, so the probe is re-spawned on every create. The new real-git test checks out and runs post-checkout after the admin lane is released fails on Git 2.55.0 for exactly this reason.

Technical details
# `hook-run` probe fails on Git 2.55+

## Affected sites
- `src/shared/worktree/split-worktree-add.ts:41` — probe runs `git hook run --ignore-missing orca-capability-probe`; Git 2.55.0 exits non-zero with `unknown hook event`. (Git 2.55 added `--allow-unknown-hook-name` and now bails on non-native hook names.)
- `src/shared/worktree/split-worktree-add.ts:24-30` — `isHookRunUnsupportedError` tests for `is not a git command|unknown (sub)?command|unknown option|usage: git hook`; none match `unknown hook event`, so `rememberUnsupported('hook-run')` never runs and `shouldTry` stays true.
- `src/main/git/worktree-admin-lock-real-git.test.ts:181-183` — fails at `expect(_gitOperationLockHeldForTests(key)).toBe(false)` because the fallback plain add is blocked in the hook while holding the lane.

## Required outcome
- On Git 2.55+ the probe must classify `git hook run` as supported so the split path engages.
- Only a genuinely absent `git hook` subcommand (Git < 2.36) should mark the capability unsupported; a 2.55 probe failure must not be re-spawned per create.

## Suggested approach (optional)
- Probe with `--allow-unknown-hook-name`; if that flag is rejected as an unknown option (Git 2.36–2.54), treat hook-run as supported and use the plain form. Treat `unknown hook event` without the flag as supported too. Reserve `is not a git command`/`unknown command` for unsupported.
- Update the `hook-run` row in `docs/reference/git-compatibility.md`: the fallback applies before 2.36 and, until fixed, again on 2.55+.

## Open questions for the human (optional)
- Should the probe avoid a synthetic name entirely (e.g. `--ignore-missing post-checkout`), accepting that it would run a user's hook when one exists?

…dline

The cleanup `worktree remove --force` after a failed or cancelled
checkout now runs without the create's AbortSignal and with its own
timeout, so a cancel mid-checkout no longer leaves a registered,
half-populated worktree. The split add's steps share one deadline
instead of each getting the full add timeout.
@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

📝 Walkthrough

Walkthrough

The change adds canonical Git metadata resolution and process-wide worktree-admin locking. Git command execution now applies lock policies, priorities, bounded waits, and termination-aware leases. Worktree creation can split admin-locked registration from checkout and hook execution when hook-run is supported. Removal paths can bypass the admin lane. New tracing records operation and preparation relationships. Tests, real-Git coverage, benchmarks, and compatibility documentation cover these changes.

Priority: ⬇️ Low

Merge Risk: 🟡 Moderate · up to 29d23

On current Git releases (2.55 and later), the new split worktree creation never activates. Each create re-runs a failing capability probe and then holds the repository's worktree lock through the full checkout and hook, so concurrent creates and pool re-arms still serialize behind long checkouts. A new real-Git test fails for this reason. The PR's main goal is not delivered on up-to-date Git until the probe classification is fixed.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 34.92% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 63 functions across 40 files. (1 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description explains the changes, rationale, testing, limitations, platform impact, and AI usage in detail. However, the required Linked Issue section states "No linked issue" even though the temp… Add the required linked issue reference, such as Fixes #123``, and ensure it points to the issue addressed by this pull request.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the primary changes: per-repository serialization of worktree administration and Git attribution to the creating operation.
Full details: Docstring Coverage

Explanation

Docstring coverage is 34.92% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 63 functions across 40 files. (1 skipped: 1 unsupported.)

Full details: Description check

Explanation

The description explains the changes, rationale, testing, limitations, platform impact, and AI usage in detail. However, the required Linked Issue section states "No linked issue" even though the template requires an issue link.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: b0b197c4-592d-4422-9fd6-2764038cbb3b

📥 Commits

Reviewing files that changed from the base of the PR and between 7cb4380 and 29d238d.

📒 Files selected for processing (41)
  • docs/reference/git-compatibility.md
  • src/main/git/command-runner/git-exec-admission-lifetime.test.ts
  • src/main/git/command-runner/git-exec-file.ts
  • src/main/git/command-runner/git-exec-lock-policy.ts
  • src/main/git/command-runner/git-exec-options.ts
  • src/main/git/remove-worktree.test.ts
  • src/main/git/worktree-add-creation-config.test.ts
  • src/main/git/worktree-add-local-base-refresh.test.ts
  • src/main/git/worktree-add-local-base-suggestion.test.ts
  • src/main/git/worktree-add.ts
  • src/main/git/worktree-admin-contention.bench.test.ts
  • src/main/git/worktree-admin-lock-real-git.test.ts
  • src/main/git/worktree-create-git-executor-real-git.test.ts
  • src/main/git/worktree-create-preparation-real-git.test.ts
  • src/main/git/worktree-create-preparation.ts
  • src/main/git/worktree-removal.ts
  • src/main/git/worktree-test-harness.ts
  • src/main/observability/instrumentation.ts
  • src/main/observability/tracer.ts
  • src/main/runtime/runtime-local-worktree-create.test.ts
  • src/main/runtime/runtime-local-worktree-create.ts
  • src/main/worktree-create-preparation-pool.ts
  • src/main/worktree-create-preparation.ts
  • src/relay/git-handler-operation-context.ts
  • src/relay/git-handler-ops.ts
  • src/relay/git-handler-termination.test.ts
  • src/relay/git-handler-worktree-operations.ts
  • src/relay/git-handler-worktree-ops.test.ts
  • src/relay/git-handler-worktree-ops.ts
  • src/relay/git-handler-worktree-provisioning.test.ts
  • src/relay/git-handler-worktree-remove.ts
  • src/relay/git-handler.ts
  • src/shared/git-capability-cache.ts
  • src/shared/git-common-dir-location.ts
  • src/shared/git-fetch-head-lock.ts
  • src/shared/git-operation-lock.test.ts
  • src/shared/git-operation-lock.ts
  • src/shared/git-worktree-admin-lock.test.ts
  • src/shared/git-worktree-admin-lock.ts
  • src/shared/worktree/split-worktree-add.test.ts
  • src/shared/worktree/split-worktree-add.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.

Comment on lines +55 to +81
function isHookRunUnsupportedError(error: unknown): boolean {
const text = [
error instanceof Error ? error.message : String(error),
typeof error === 'object' && error !== null && 'stderr' in error ? String(error.stderr) : ''
].join('\n')
return /is not a git command|unknown (sub)?command|unknown option|usage: git hook/i.test(text)
}

async function supportsHookRun(request: SplitWorktreeAddRequest, git: StepGit): Promise<boolean> {
const { capabilities, globalArgs, repoPath } = request
if (capabilities.isKnownSupported('hook-run')) {
return true
}
if (!capabilities.shouldTry('hook-run')) {
return false
}
try {
await git([...globalArgs, 'hook', 'run', '--ignore-missing', HOOK_RUN_PROBE_NAME], repoPath)
capabilities.rememberSupported('hook-run')
return true
} catch (error) {
if (isHookRunUnsupportedError(error)) {
capabilities.rememberUnsupported('hook-run')
}
return false
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

The hook-run probe still fails on Git 2.55+. The CI failure confirms it.

The probe still runs git hook run --ignore-missing orca-capability-probe. Git 2.55 rejects hook names it does not know unless the command passes --allow-unknown-hook-name. That error text matches none of the patterns in isHookRunUnsupportedError. As a result, supportsHookRun returns false without caching the result:

  • Every cold create re-runs the probe.
  • Every cold create falls back to the plain worktree add, which holds the admin lane through checkout and post-checkout.

The failing check src/main/git/worktree-admin-lock-real-git.test.ts:183 (expected true to be false) shows this outcome.

Treat unknown hook event as a supported result, or probe with --allow-unknown-hook-name first. Keep the unsupported classification for a missing git hook subcommand only. After the fix, update the hook-run row in docs/reference/git-compatibility.md.

Possible fix
 function isHookRunUnsupportedError(error: unknown): boolean {
   const text = [
     error instanceof Error ? error.message : String(error),
     typeof error === 'object' && error !== null && 'stderr' in error ? String(error.stderr) : ''
   ].join('\n')
-  return /is not a git command|unknown (sub)?command|unknown option|usage: git hook/i.test(text)
+  return /is not a git command|unknown (sub)?command/i.test(text)
+}
+
+// Why: Git 2.55+ rejects non-native hook names, which proves `git hook run` exists.
+function isHookRunUnknownNameError(error: unknown): boolean {
+  const stderr =
+    typeof error === 'object' && error !== null && 'stderr' in error ? String(error.stderr) : ''
+  const message = error instanceof Error ? error.message : String(error)
+  return /unknown hook event/i.test(`${message}\n${stderr}`)
 }
@@
   } catch (error) {
+    if (isHookRunUnknownNameError(error)) {
+      capabilities.rememberSupported('hook-run')
+      return true
+    }
     if (isHookRunUnsupportedError(error)) {
       capabilities.rememberUnsupported('hook-run')
     }
     return false
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function isHookRunUnsupportedError(error: unknown): boolean {
const text = [
error instanceof Error ? error.message : String(error),
typeof error === 'object' && error !== null && 'stderr' in error ? String(error.stderr) : ''
].join('\n')
return /is not a git command|unknown (sub)?command|unknown option|usage: git hook/i.test(text)
}
async function supportsHookRun(request: SplitWorktreeAddRequest, git: StepGit): Promise<boolean> {
const { capabilities, globalArgs, repoPath } = request
if (capabilities.isKnownSupported('hook-run')) {
return true
}
if (!capabilities.shouldTry('hook-run')) {
return false
}
try {
await git([...globalArgs, 'hook', 'run', '--ignore-missing', HOOK_RUN_PROBE_NAME], repoPath)
capabilities.rememberSupported('hook-run')
return true
} catch (error) {
if (isHookRunUnsupportedError(error)) {
capabilities.rememberUnsupported('hook-run')
}
return false
}
}
function isHookRunUnsupportedError(error: unknown): boolean {
const text = [
error instanceof Error ? error.message : String(error),
typeof error === 'object' && error !== null && 'stderr' in error ? String(error.stderr) : ''
].join('\n')
return /is not a git command|unknown (sub)?command/i.test(text)
}
// Why: Git 2.55+ rejects non-native hook names, which proves `git hook run` exists.
function isHookRunUnknownNameError(error: unknown): boolean {
const stderr =
typeof error === 'object' && error !== null && 'stderr' in error ? String(error.stderr) : ''
const message = error instanceof Error ? error.message : String(error)
return /unknown hook event/i.test(`${message}\n${stderr}`)
}
async function supportsHookRun(request: SplitWorktreeAddRequest, git: StepGit): Promise<boolean> {
const { capabilities, globalArgs, repoPath } = request
if (capabilities.isKnownSupported('hook-run')) {
return true
}
if (!capabilities.shouldTry('hook-run')) {
return false
}
try {
await git([...globalArgs, 'hook', 'run', '--ignore-missing', HOOK_RUN_PROBE_NAME], repoPath)
capabilities.rememberSupported('hook-run')
return true
} catch (error) {
if (isHookRunUnknownNameError(error)) {
capabilities.rememberSupported('hook-run')
return true
}
if (isHookRunUnsupportedError(error)) {
capabilities.rememberUnsupported('hook-run')
}
return false
}
}

Source: Linters/SAST tools

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No new issues in the new commits — the delta is a clean follow-up. The previously flagged hook-run probe failure is still open (existing inline thread), so this is not an approval.

Reviewed changes

Re-reviewed the third commit (29d238d, 4 files, +137/−12) against the prior review at 5fe9128; read the incremental range-diff in full and traced the new code through its main-process and relay callers. Ran src/shared/worktree/split-worktree-add.test.ts (8 pass) and src/main/git/worktree-admin-lock-real-git.test.ts (3 pass, 1 fail — the known hook-run probe on Git 2.55).

  • One deadline for the whole split add — withSharedDeadline hands each step only the remaining budget and rejects with ETIMEDOUT once it is spent, so the split add is bounded like the plain command instead of three full timeouts.
  • Detached, self-budgeted cleanup — a failed checkout now runs worktree remove --force through the unwrapped adapter with detached: true (create signal stripped), worktreeAdminLock: false, and its own 60 s timeout, so a cancelled or timed-out create is unregistered.
  • Cancellation coverage — a new real-git test cancels a create mid-smudge-filter and asserts the worktree is not left registered.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

/** Replaces the caller's own per-command timeout. */
timeout?: number
/** Runs without the create's AbortSignal, so cleanup still happens after a cancel. */
detached?: true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

detached is honored only by the main-process adapter (src/main/git/worktree-add.ts); the relay's GitExec type omits it and the relay create path never threads an AbortSignal, so this cleanup-after-cancel guarantee is main-only today. Inert rather than broken right now, but worth a comment here if a request signal is ever threaded into relay creates.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing. It is ~2,400 commits behind and conflicts in 19 files, and its repo-wide lock does not fix the main slow-create cause (a create waiting on an unfinished spare checkout). That cause is addressed in #29115; main has since added narrower concurrency controls and create timing data.

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.

1 participant