Repository navigation
fix(worktrees): keep creation fast in large repositories - #24346
Conversation
|
| const lockPath = await verifyWorktreePreparationLock(worktreePath, expectedLockReason, options) | ||
| await unlink(lockPath).catch((error: unknown) => { |
There was a problem hiding this comment.
Unlock can remove another lock Verification reads the lock marker, but removal is a separate operation on its path. If another Git process replaces the marker between those operations, this unlink removes the new owner’s lock instead. That leaves the worktree unprotected, including when finalization unlocks a newly attached checkout.
There was a problem hiding this comment.
Acknowledged as an inherent limit. Filesystems offer no atomic compare-and-delete, and Git's own worktree unlock deletes the marker without checking it. This code narrows that gap: it reads the exact reason and checks cancellation immediately before unlink, and preparation paths are Orca-private hidden directories that no ordinary Git workflow locks.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe pull request updates prepared-worktree creation and finalization to use lock-reason ownership checks and coordinate checkout materialization with fetch settlement. It adds direct created-worktree reconciliation and passes created-worktree evidence into terminal startup and split routing. It also adds multi-pack-index probing and maintenance, with configuration handling and separate scheduling for pack-index and ref maintenance. Priority: ⬆️ High Merge Risk: ⚪ Minimal · up to Deferred repository maintenance remains scheduled after index work. The selected changes are mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change strengthens checkout ownership checks and guards background repository writes. A crash-recovery path can nevertheless leave a locked registration behind when its checkout directory is missing. Some terminal-routing and repository-authorization paths remain partially examined. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
There was a problem hiding this comment.
ℹ️ No critical issues — one rough edge inline.
Reviewed changes
- Register during fetch. Prepared
worktree addnow runs while the shared base fetch is in flight, with a cancellablebeforeMaterializationbarrier that waits for settlement, resolves the current OID, then does a singlereset --hard. - Prepared-checkout ownership lock. New
.git/worktrees/<name>/lockedmarker; Git ≥2.33 writes it atomically viaworktree add --lock --reason, older Git registers then writes it exclusively. Every mutate/unlock/discard re-verifies the exact reason and refuses to touch a foreign owner. - Queued tip refresh. An existing warm preparation is refreshed in place (one reset) instead of discarded, published before yielding so a racing create waits for the same work.
- Direct create verification and terminal-start evidence.
resolveCreatedWorktreereads the new checkout directly and lists only as a fallback; startup/setup terminals reuse the in-memory createdWorktreewhen its path, host routing and persistedinstanceIdstill agree. - Idle
multi-pack-indexmaintenance. Bounded, opt-out-aware, bitmap/incremental/foreign-lock-protecting index-only repair scheduled independently from ref packing inRepoRefMaintenance.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This incremental review covers the new commit 646fc7b0cd on top of the previously reviewed ea63633b6e. The only production change is src/main/git/worktree-preparation-add.ts; the rest is the reliability gate and tests.
- Reclaimed unlocked fallback registrations. The older-Git path (
add([])then an exclusive marker write) now catches a failed lock and removes its just-registered worktree with a single--force, gated on the path having been initially absent and no competing owner being detected. This closes the stranded-registration gap raised in the prior review. - Hoisted the initial-absence probe.
isAbsentnow runs once before the capability fallback decision, so the fallback path can decide whether a failed preparation is safe to reclaim rather than leaving an unlocked registration behind. - Regression coverage. A new real-Git suite plus unit cases prove exactly one single-force removal, no
reset, preservation of a competing lock, and preservation of a pre-existing empty target. The real-Git cases exercise both the cancellation and generic probe-failure windows.
The single-force removal is the correct lever here: Git's worktree remove refuses a locked worktree without a second --force, so a competing owner is preserved even when the marker probe itself fails. Cancellation cannot strand the cleanup because the discard path strips the signal and pins its own timeout. The author-acknowledged residual — an older-Git add interrupted before positive registration success — is preserved deliberately, which is the safe choice since ownership is unprovable in that window.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Resolve lock ownership from administrative metadata when… · worktree-preparation-discard.ts:30-42
src/main/git/worktree-preparation-discard.ts:30-42
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winResolve lock ownership from administrative metadata when the checkout path is missing.
When stale cleanup selects a dead preparation owner,
discardPreparedWorktreeresolves the lock withworktreePathas Git'scwd. If the checkout directory was removed externally,git rev-parse --git-path lockedcannot start. The code therefore never reachesgit worktree remove --force.Stale cleanup suppresses this error, and this direct call does not add the target to
pendingDiscards. Later cleanup passes repeat the same failure, so the Git registration can remain indefinitely.Use the registered administrative lock path for ownership verification, then remove the registration from
repoPath. Do not bypass the ownership check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 317a8c02-4c77-46e6-9bbd-e58617a32a3a
📒 Files selected for processing (25)
src/main/git/repo-pack-index-maintenance.test.tssrc/main/git/repo-pack-index-maintenance.tssrc/main/git/worktree-create-admission-tier.test.tssrc/main/git/worktree-create-preparation-real-git.test.tssrc/main/git/worktree-create-preparation-real-wsl.test.tssrc/main/git/worktree-create-preparation.tssrc/main/git/worktree-mutation-route-invalidation.test.tssrc/main/git/worktree-preparation-base-oid.test.tssrc/main/git/worktree-preparation-cancel-latency.bench.test.tssrc/main/git/worktree-preparation-lock-real-git.test.tssrc/main/git/worktree-preparation-lock.test.tssrc/main/git/worktree-preparation-lock.tssrc/main/ipc/worktrees/create/register-worktree-prefetch-handler.tssrc/main/runtime/orca-runtime-get-worktree-terminal-provisioning-host.tssrc/main/worktree-create-base-prefetch.test.tssrc/main/worktree-create-base-prefetch.tssrc/main/worktree-create-preparation-pool.tssrc/main/worktree-create-preparation-stale-cleanup.tssrc/main/worktree-create-preparation.test.tssrc/main/worktree-create-preparation.tssrc/main/worktree-preparation-fetch-barrier.test.tssrc/main/worktree-preparation-refresh-queue.tssrc/main/worktree-preparation-tip-refresh.test.tssrc/shared/repo-maintenance-schedule.tssrc/shared/repo-ref-maintenance.ts
💤 Files with no reviewable changes (1)
- src/main/worktree-preparation-refresh-queue.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/runtime/orca-runtime-get-worktree-terminal-provisioning-host.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This incremental review covers b52b3dd30d since the prior pullfrog review at 646fc7b0cd — a cleanup and optimization pass over the prepared-checkout ownership and idle-maintenance code.
- Single bounded walk for pack-index maintenance. The two
objects/packprobes collapsed into one that both counts packs and detects protected metadata, and idle admission is now checked before walking, so a busy app spends no directory entries. - Lock path reused across finalization.
finalizePreparedWorktreeresolves the ownership lock once and reuses it afterworktree move;unlockWorktreePreparationAtPathre-reads the owner before unlinking. This removes tworev-parsesubprocesses per finalize and is safe because Git'sworktree movepreserves the linked administration directory (confirmed against real Git 2.55). - Required lock reason and verified failure cleanup.
discardPreparedWorktree,unlockPreparedWorktreeandfinalizePreparedWorktreenow requireexpectedLockReason, and failure cleanup double-forces only after re-verifying the marker, so a foreign owner is never clobbered. - Merged maintenance scheduling.
RepoRefMaintenanceCooldownandRepoPackIndexSchedulefolded intoRepoMaintenanceSchedulewith independent ref/index cooldowns preserved. - Cleanup. Dropped the single-use
prepareCheckoutForEntrywrapper andWorktreeCreatePreparationOptionstype, extracted the tip-refresh queue intoworktree-preparation-refresh-queue.ts, and removed the unused reliability-gate manifest entry.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
An idle attempt that found the pack index due but refs still cooling down returned without rescheduling, so loose refs from the arming fetch waited for the next write instead of the ref cooldown.
|
CodeRabbit's outside-diff note on |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This incremental review covers 8087fe631b since the prior pullfrog review at b52b3dd30d — a one-commit refinement to idle ref-maintenance scheduling.
- Re-armed an index-only attempt to its ref phase. When the pack index was due but refs were still cooling down,
RepoRefMaintenance.packIfNeededreturned without rescheduling, so loose refs left by the arming write waited for an unrelated future fetch. It now re-inserts the tracked repo and schedules it atphases.refDueAt(key) - now(), bounded by!disposed && !tracked.has(key)so a concurrentarmis not clobbered. - Regression test. A new case in
repo-pack-index-scheduling.test.tsproves two index passes run while refs stay untouched, then refs are re-probed onceREF_MAINTENANCE_CLEAN_COOLDOWN_MSelapses — with no furtherarm. It fails against the pre-fix code.
The guard against a fresher arm and the positive-delay refDueAt - now() arithmetic (branch only entered when now < refDueAt) rule out the two ways this re-arm could misbehave. This addresses CodeRabbit's "keep ref attempt scheduled after index-only maintenance" finding.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
Resolves the conflicts with #24346 (keep creation fast in large repositories) in the prepared-checkout code. All of #24346's behavior is kept; the create timing and spare recording are re-applied onto its new shape: - finalize keeps main's lock-ownership checks and lock reason, and still reports whether the prepared checkout had to be reset; - the failed-finalize discard is timed only when main actually runs it; - a tip refresh of an existing spare counts as preparation work and extends its build timing (per-entry bookkeeping moved to worktree-create-preparation-activity.ts, the claim registry to worktree-create-preparation-claim-registry.ts, to keep the pool under the line limit; no behavior change); - the create no longer lists worktrees, so the worktree count now comes from one listing of .git/worktrees in the existing post-create repo read.

ELI5
Creating a workspace should not repeatedly inspect every other workspace or fill the same checkout twice. This makes creation cheaper in large repositories and repairs the object lookup slowdown that builds up over time.
What Changed
Why
The captured production requests took 40.3s and 62.0s with 1,730 registered worktrees, 31,191 tracked files and 11,424 packs. Most time went into preparation and finalization; extra full listings also delayed verification and terminal startup.
Keeping Git responsible for registering and moving checkouts preserves its path, collision and recovery checks. Direct verification removes redundant work around those commands. Reusing the idle scheduler avoids a second maintenance service, and an index-only repair avoids the cost and risk of mass pack rewrites.
Linked Issue
N/A — maintainer performance investigation; no issue supplied.
Benchmarks
The large-repository measurements use 1,730 worktrees, 31,191 tracked files and 11,424 packs. The rebuilt samples include object-index repair, Blank Terminal and skipped setup. The before/after boundaries differ, so these samples do not establish a percentage speedup or a latency guarantee. Cold fetch and materialization remain real costs. The visual section records the detailed setup.
The review fix was separately measured using real Git 2.55 on macOS, a one-file scratch repository, and injected cancellation during lock-path discovery after successful registration. There are 10 serial samples per mode and version; the old-Git unsupported option is simulated or cached. Timing starts at the preparation-add function and ends after rejection and any cleanup; fixture setup and inspection are excluded. Concurrent reviewers may affect timing.
Before timings return with the leaked registration. Fixed timings include its removal. The permanent regression cases can be rerun with
ORCA_BACKGROUND_LAUNCH=1 pnpm exec vitest run --config config/vitest.config.ts src/main/git/worktree-preparation-add.test.ts src/main/git/worktree-preparation-fallback-cleanup-real-git.test.ts.The cleanup in
b52b3dd30d6was measured separately on macOS with real Git 2.55. Finalization uses a one-file repository with an unchanged base, 20 pairs with alternating before/after order, and excludes preparation, inspection and removal. Concurrent validation may affect timing; these measurements describe finalization, not end-to-end UI latency.646fc7b0cdfb52b3dd30d6The directory figures are deterministic count tests with four metadata entries per pack. Cancellation, activity changes during the walk and protected metadata still suppress the writer. Run
ORCA_BACKGROUND_LAUNCH=1 pnpm test src/main/git/repo-pack-index-maintenance.test.ts src/main/git/worktree-preparation-lock-real-git.test.tsto verify these bounds.Visual Proof
The latency change is shown by recorded timings, not by a layout change. Production baseline numbers are backend request spans. Final rebuilt macOS daemon UI samples measure submit, after any setup trust decision, to observed PTY prompt output and a rendered terminal. Blank Terminal was selected and setup skipped; after measurements include the one-time object-index repair.
Final controlled samples: 7.09s cold, then 4.68s and 3.08s. The cold sample forces a base advance through a temporary local remote in the same large repository and verifies the fetched HEAD and exactly one materialization. Its fetch takes 3.65s and materialization 3.08s; finalization takes 1.10s. These are observed samples, not a five-second guarantee. The earlier ordinary-base UI samples were 3.63s and 1.48s. All disposable workspaces, the test remote and launched app processes were cleaned up.
Testing
pnpm tc,pnpm build, changed-code quality, reliability manifest andgit diff --checkpass.pnpm lintpasses the base lint and anti-slop stages, then stops at pre-existing mobile import cycles inhost-client-hooks.ts,client-context.tsx,fit-scale.tsandmouse-report-cell.ts; those files are unchanged from main. The changed-code quality gate reports zero new findings.Review
Two initial independent review rounds covered lock ownership, cancellation, failed creation, expiry/eviction, simultaneous fetch/consume, stale workspace instances, split authority, host routing, baseline Git and maintenance contention. Codex used
gpt-6.1-solatultra; Claude used Orca orchestration with the effectiveopuslaunch model. Both reviewers assessed the full updated diff independently; the cleanup below received another independent pass from each.646fc7b0cdfTwo outdated runtime assertions that caused the original CI failure were corrected to verify startup instance evidence and direct WSL verification without an inventory scan. The full runtime suite now passes locally. The initial review workers and coordinator were released/closed after settlement. The cleanup Claude task succeeded; Orca reports its terminal retained after user takeover, so it was left alone.
The elegance pass removes single-use wrappers, unused ownership bypasses and the single-field barrier options type; it also initializes the pool entry with a checked type. Ownership/host/lineage guards and failure cleanup remain because they preserve behavior. Against the same base, net application-source additions fall from 1,034 to 983 lines, and the 227-line manifest addition is gone. Production changes span 34 files; regression tests and mocks account for most of the overall diff.
b52b3dd30d6Final direct-Codex adversarial audit cross-referenced Git source at
~/projects/gitand passed 189 focused safety tests covering all 34 application-source paths. Two explicitly withdrawn bot findings are resolved. The outdated read/unlink race thread remains acknowledged: an exact ownership reread immediately before unlink narrows the interval, and Git's own unlock primitive also uses read-then-unlink. An atomic compare/delete operation is unavailable through this filesystem interface.The audit follow-up #24945 is merged. Superseded queued MIDX/ref writers and delayed probes are fenced by newer activity; admitted writers finish. Ordinary preparation-lock discovery uses one Git query instead of two. The follow-up reuses the ownership reader and draft builder, preserves full SSH review context, and negotiates independently updated session-search catalogs. It adds 315 net application lines across 32 modules, with most overall additions in tests. Two independent direct Codex 6.1-sol ultra reviews verified all 78 final files against cloned Git source; the exact head passed 55 CI checks with nine intentional skips and a rebuilt isolated public CLI smoke. Benchmarks and the two remaining P2 limits/two residual risks are recorded in that PR. The benchmark results above describe this original PR.
Remaining confirmed P2 items:
Validation gaps remain for physically interrupted old-Git registration before successful settlement, external repack/metadata overlap, and physical Windows unlink sharing violations. Unknown registration ownership is deliberately preserved; simulations are not presented as reproduced production corruption. Targeted lock discovery remains a small cost, materially reduced in #24945.
Reliability contract (
agent-session.provider-ownershipand prepared checkout ownership): the same persisted instance and execution host must own startup. Matching marker ownership gates materialization and double-force cleanup. A successfully registered, initially absent unlocked fallback checkout may receive bounded single-force cleanup, which preserves competing locks. The motivating path is a large-repository create racing a fetched base advance or starting setup alongside its terminal. Oracles assert one materialization, one checkout hook, one cleanup, the latest HEAD, zero full listings for verified startup, and fallback for stale/foreign/folder evidence. Normal CI test discovery runs the regression files directly. Dedicated gate routing and CI soak remain accepted gaps. The unused new manifest entry was removed: its hand-entered commands and budgets did not schedule tests or enforce latency, and duplicated the evidence recorded here.Performance budget: existing three-checkout pool and five-minute lifetime remain bounded; maintenance probes at most 65,536 directory names or one second, admits one idle writer, and retains at most 64 directory hints. It respects activity/battery/agent gates, refreshes indexes at most hourly after a new arm, and backs off failures for 30 minutes. A running index writer does not block create's maintenance pause.
Diagnostics use existing creation-phase spans plus index outcome, pack-count floor and write-duration attributes. Residual gaps: live platform/provider runs and CI soak are not established, and external marker/bitmap replacement between a final filesystem check and a Git command remains non-atomic. This evidence establishes the listed tests and samples; long-running CI soak remains unestablished.
Agent skill upstream boundary
Notes
No RPC/preload signature or remote wire field was added. Provider ownership, workspace trust and terminal split authority remain in the launch path. The installed desktop app has not been replaced by this branch.
Checklist