Sign up
View stargazers13Sign up

fix(server): bulk worktree launches no longer fail on Git races#17197

open6
main
GitHub fork: t3/fix-concurrent-worktree-creation
opened15d agobyJjakeleventhal
4 files changed
View on GitHubTwo-way sync enabledPR last imported 4h ago

Concurrent launches into one repository can fail on fetch ref locks, occupied branch names or paths, and shared Git config writes. Retry can hit the same collision again.

Concurrent fetches of the same ref now share their in-flight result, including the fetchWorktreeBaseCommit path used by start-from-origin launches. Branch/path collisions retry with a bounded suffix (temporary branches get a fresh hash), and branch creation, merge-base config writes, and renames share a lock keyed by the Git common directory. Checkout stays parallel. A launch never retries once it has claimed a worktree path, and retrying a failed thread keeps its existing run and message.

Rebased onto main at 5bb54a0f77, resolving conflicts with main's test pruning without restoring unrelated deleted cases.

Validation on macOS:

  • Controlled process fixtures verify six concurrent public fetches or launch-base fetches share one fetch, return the same result, and fetch again after completion. Removing the launch-base fix makes its regression fail with six fetches instead of one.
  • Real Git service integration covers six simultaneous launches both with and without startFromOrigin. Origin advances beyond the local branch; every origin-based launch starts at the fetched commit and keeps the remote comparison base. Occupied branches, uncommitted work, an unrelated directory, and the local base branch are preserved.
  • Both complete touched suites pass: 209 tests across ThreadLaunchService.test.ts and GitVcsDriverCore.test.ts.
  • Server typecheck, targeted lint, formatting, and diff checks pass. Lint reports three existing warnings outside the changes.

Fetch sharing is checked with a controlled process fixture; real Git launch tests use a local remote. No live network race or client UI verification was performed. Coordination is local to one Git driver instance; external Git processes do not use its locks. As with Git's existing worktree add -b, an attempt that fails before claiming a worktree can leave an unused branch. Collision retries skip it without deleting existing work.

Implementation and validation: GPT-6.1 Sol (high) through the Codex harness in T3 Code.

Checks

All checks passed
MmacroscopeappBot15d ago
Synced from GitHub

Approvability

Verdict: Approved at 1ed1257

Macroscope's review found this PR approvable — This is a focused server-side concurrency fix that preserves existing launch behavior while handling Git fetch, branch, path, and config races. The added tests cover concurrent launches, collision recovery, cleanup, renames, and fetch coalescing.

You can add or adjust custom eligibility rules. Learn more.

CcoderabbitaiBot15d ago
Synced from GitHub

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

  • Last reviewed commit: 34c6bf51b20adc8751710311c14fd44b884fe94f
  • Latest commit, not reviewed: 1ed1257f32758b62df2afdb2dc45697131c6ae21

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 54d11896-684e-468f-b5de-52da6842890a
📥 Commits

Reviewing files that changed from the base of the PR and between ea5c623e2013b58b0a6f415308cdeec1f4908a89 and 34c6bf51b20adc8751710311c14fd44b884fe94f.

📒 Files selected for processing: 4
  • apps/server/src/orchestration-v2/ThreadLaunchService.test.ts
  • apps/server/src/orchestration-v2/ThreadLaunchService.ts
  • apps/server/src/vcs/GitVcsDriverCore.test.ts
  • apps/server/src/vcs/GitVcsDriverCore.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.

📝 Walkthrough

GitVcsDriverCore classifies branch-ref collisions, serializes selected repository updates, and shares concurrent matching fetches. ThreadLaunchService retries eligible worktree branch or path collisions. Tests cover concurrent launches, retry limits, failure handling, recovery, and cleanup.

Changes: Git Concurrency and Worktree Provisioning

Layer / File(s)Summary
Classify collisions and serialize Git updates
apps/server/src/vcs/GitVcsDriverCore.ts, apps/server/src/vcs/GitVcsDriverCore.test.ts		The driver classifies existing refs/heads/... collisions as branch_already_exists and serializes worktree base-ref configuration and branch renames by repository. Tests cover ref collision classification and concurrent renames.
Coalesce matching remote fetches
apps/server/src/vcs/GitVcsDriverCore.ts, apps/server/src/vcs/GitVcsDriverCore.test.ts		Concurrent fetches with the same working directory, remote, and optional ref share an in-flight operation. A later fetch starts a new operation after completion.
Retry eligible worktree collisions
apps/server/src/orchestration-v2/ThreadLaunchService.ts, apps/server/src/orchestration-v2/ThreadLaunchService.test.ts		Worktree creation retries branch or path collisions before a path is claimed, up to 100 retries. Tests cover concurrent launches, temporary names, retry limits, recovery, and partial-checkout cleanup.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: bil0000

Merge Risk: ⚪ Minimal · up to 34c6b

The investigated concurrency and retry paths do not show a remaining obstacle to merging after normal checks.

Security architecture risk: 🔵 Low · up to 34c6b

The changes reduce Git races while preserving the repository and base reference used by each launch. No introduced security issue was established. Remaining uncertainty concerns partial Git failures before a worktree is reported as created, rather than a demonstrated expansion of access or privileges.

Retained concerns No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed retry flow operates within the selected project's repository and configured worktree location. It expands the number of candidate branch/path attempts, not the repository or base-reference authority passed to Git. The supplied evidence does not establish deployment-wide or tenant-wide exposure.

Trust Boundaries and Controls

  • observed — The new atomic-ref collision classification is restricted to refs/heads. Only classified branch/path collisions enter the provisioning retry loop; authentication, host-key, and other failures do not satisfy its retry predicate. Git diagnostic classification excludes remote-prefixed output.

Resilience and Maintainability Implications

  • observed — A reported worktree claim prevents collision retries. Failure before workspace recording, or cancellation before run release, attempts removal of the claimed path; thread metadata is cleared only after successful removal. Ordinary failure after recording preserves the workspace for recovery.
  • observed — Ownership reporting occurs after successful git worktree add, and worktree removal contains no branch-ref deletion. Both behaviors existed at the PR base. Inspected evidence does not establish Git's partial-failure rollback before ownership reporting, so orphaned state or its amplification by retries is unresolved, not a verified introduced issue.

Hardening Proposals

  • proposed — Add fault-injection coverage around real worktree creation, including interruption before ownership reporting and failure after ref creation. Establish which directory and ref state survives, and ensure recovery never deletes state owned by another launch.
Pre-merge checks: Passed 1 · Failed 1 · Inconclusive 2

Failed Failed checks: 1 warning · 2 inconclusive

Check nameStatusExplanationResolution
Description checkWarningThe description explains the problem, implementation, limitations, and detailed verification. It does not follow the template headings and does not provide a triaged issue or explicit maintainer appro…Add the required Problem, Change, Scope and approval, and Verification headings. In Scope and approval, link the relevant issue or discussion and include explicit maintainer approval, or explain why this focused fix qualifies for the exempt…
Linked Issues checkInconclusiveThe linked issue assessment could not be completed.Retry the assessment when the required review evidence is available.
Out of Scope Changes checkInconclusiveThe linked issue assessment could not be completed.Retry the assessment when the required review evidence is available.
Passed Passed checks: 1 passed
Check nameStatusExplanation
Title checkPassedThe title clearly describes the primary change: preventing bulk worktree launches from failing because of Git races.
Full details: Description check

Explanation

The description explains the problem, implementation, limitations, and detailed verification. It does not follow the template headings and does not provide a triaged issue or explicit maintainer approval for scope.

Resolution

Add the required Problem, Change, Scope and approval, and Verification headings. In Scope and approval, link the relevant issue or discussion and include explicit maintainer approval, or explain why this focused fix qualifies for the exemption.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

JJulius Marminge4d ago

Taking over this one as maintainer for batch worktree-git-16. I will preserve author credit, merge current main, reduce the tests, rebuild the Git race fix, and run independent adversarial reviews. No merge is planned.

Ssaphid6h ago
Synced from GitHub

Source review at 0924091: the new fetch coalescing is not used by the start-from-origin launch path. ThreadLaunchService calls fetchWorktreeBaseCommit, which still calls private fetchRemote directly (source); only the public fetch service uses fetchRemoteShared. Concurrent launch-base fetches therefore bypass the new shared operation.

Please route that path through the shared fetch and cover concurrent fetchWorktreeBaseCommit/start-from-origin launches, rather than only concurrent public fetchRemote calls. This is a source finding; I have not run the exact-base/head reproduction, so I am not claiming an observed runtime failure or fix validation. The existing report is #17198; no duplicate issue is needed.

Added 6 commits

  • fix(server): bulk worktree launches no longer fail on Git racesb9c904d
  • test(server): deduplicate worktree launch fixture importd2c53e1
  • fix: preserve source behavior against current upstream contracts0d91650
  • test(server): consolidate concurrent worktree launch coverage4916112
  • fix(server): lock worktree branch configuration before checkout9fd989b
  • fix(server): share fetches for start-from-origin launches1ed1257
Jjakeleventhal4h ago
Synced from GitHub

@saphid Addressed your fetch-path feedback in 1ed1257f32758b62df2afdb2dc45697131c6ae21. fetchWorktreeBaseCommit now calls fetchRemoteShared, so start-from-origin launches use the shared fetch.

The fetch regression now covers six concurrent fetchWorktreeBaseCommit calls, checks that they share one fetch and the same resolved base, and confirms a completed fetch is not reused. Temporarily reverting this call makes the test fail with six fetches instead of one. Bulk-launch coverage also exercises six real startFromOrigin: true launches against an advanced local Git remote, verifying every checkout starts at the fetched commit while preserving occupied work and the local branch.

Rebased onto main at 5bb54a0f77, resolved the test conflicts, and pushed. Both complete touched suites pass (209 tests), along with server typecheck, targeted lint, formatting, and diff checks. Fetch-count validation uses a controlled process fixture; launch integration uses a local Git remote.