Fixes #3

[fzoll]

Summary

Per-repo (gitCommonDir-keyed) semaphore around the worktree-provisioning git sequence (fetch + resolveRemoteTrackingCommit + worktree add) so the same clone's git-prep never runs concurrently, while different repos keep provisioning in parallel.

Root cause

resolveRemoteTrackingCommit runs git rev-parse --verify refs/remotes/<remote>/<branch>^{commit} in the main clone. If a concurrent git fetch (from a second, simultaneous worktree-provisioning dispatch against the same repo) is writing/locking refs/remotes/* at that moment, the rev-parse fails with fatal: Needed a single revision. There was no per-repo lock around this sequence — only settings/sqlite/provider access was serialized, not the git operations themselves.

Fix

In apps/server/src/vcs/GitVcsDriverCore.ts, added a Ref<Map<gitCommonDir, Semaphore>> and a withRepoGitLock(cwd, effect) helper that resolves the repo's gitCommonDir and runs effect under that repo's dedicated single-permit semaphore. Wrapped the three operations implicated by the incident with it:

• fetchRemote
• resolveRemoteTrackingCommit
• createWorktree (git worktree add)

Every caller of these (the ws.ts bootstrap prepareWorktree flow that triggered the incident, GitManager.resolveBaseRangeRef, preparePullRequestThread, etc.) is protected automatically since the lock lives at the driver level, keyed by the resolved gitCommonDir rather than the caller's cwd — so a worktree under the same repo also serializes against the main clone.

Why this design

• Keyed by gitCommonDir (not cwd) so worktrees of the same repo share one lock, matching "same repo" from the issue rather than "same directory".
• Only the three git operations named in the report are guarded, to keep the change minimal and avoid serializing unrelated git commands (e.g. plain status reads) that aren't implicated in the race.
• Semaphore is created lazily per repo the first time it's needed (double-checked Ref.modify), so there's no upfront enumeration of repos and no cross-repo blocking.

Testing

• Added apps/server/src/vcs/GitVcsDriverCore.test.ts: "serializes fetch + resolveRemoteTrackingCommit + worktree add for the same repo, but not across repos" — wraps the real spawner to track concurrently in-flight guarded git subprocesses per repo (holding each slightly past its real completion to widen any race window) and asserts max concurrency is 1 within a repo while two different repos still provision independently.
• vp test run src/vcs/GitVcsDriverCore.test.ts src/git/GitWorkflowService.test.ts src/git/GitManager.test.ts src/vcs/GitVcsDriver.test.ts — 100 tests passed.
• tsgo --noEmit (apps/server) — clean.
• vp check on both changed files — formatting and lint clean.

Per repo convention (AGENTS.md), I did not run the full-workspace vp run typecheck/vp run test — verification was scoped to the changed files and their closest test suites.

Scope note

This addresses the exact race described in the issue (concurrent worktree-provisioning dispatches against the same repo). It does not attempt the separate, explicitly-out-of-scope "bounded retry + offer-throttle" work referenced in the issue's footer, which belongs to fzoll/cc_runner.

[fzoll]

VERDICT:APPROVED

Summary

The fix correctly resolves the race described in #3: a per-repo semaphore (keyed by gitCommonDir) serializes fetchRemote, resolveRemoteTrackingCommit, and createWorktree's git worktree add so a concurrent git fetch writing refs/remotes/* can no longer race the git rev-parse --verify refs/remotes/... reader and fail with fatal: Needed a single revision.

Correctness — verified

• No reentrancy deadlock. Effect semaphores aren't reentrant, so I checked every guarded path: no guarded operation nests another guarded operation. In resolveRemoteTrackingCommit, listRemoteNames runs outside the permit and only the rev-parse is wrapped; in createWorktree only git worktree add is under the lock while the follow-up configureBaseRef/listRemoteNames block stays outside. Each withPermit wraps exactly one subprocess and callers invoke the three ops sequentially (each acquire/release independent), so the single-permit lock can't self-block.
• Keyed by gitCommonDir, not cwd — correct: a worktree of the same clone serializes against the main clone, matching "same repo" from the issue.
• Lazy lock creation is race-safe. acquireRepoGitLock uses double-checked Ref.modify; a loser fiber's freshly-made Semaphore is discarded and the winner's returned. Atomic, correct (minor: a throwaway semaphore is allocated on contention — harmless).
• Shared instance. Layer.effect(GitVcsDriver, make) is memoized, so repoGitLocks is a single map shared across all fibers/callers — the driver-level placement genuinely covers ws.ts provisioning, GitManager, PR-thread prep, etc.
• No permit leak. withPermit releases on failure too, so a failed git command doesn't strand the lock.
• Imports (effect/Ref, effect/Semaphore) present on the branch; Semaphore already used elsewhere in the file.

Non-blocking notes

1. Test doesn't verify the cross-repo half. The test asserts maxActiveByRepo === 1 for each repo, but never asserts repoA and repoB actually ran concurrently. A regression to a single global lock would still pass this test, so the "but not across repos" claim in the test name is unverified. Consider recording global max-concurrency (or overlap timestamps) and asserting it reaches 2.
2. Guarding by operation, not by ref-writes. Other ref-writing fetches (fetchRemoteBranch, fetchRemoteTrackingBranch, fetchPullRequestBranch — all git fetch into refs/remotes/*/refs/heads/*) remain unguarded, so a theoretical race with a concurrent resolveRemoteTrackingCommit against the same repo still exists outside the provisioning flow. The PR explicitly scopes to the reported incident and documents this tradeoff, which is reasonable — flagging as residual, not a blocker.
3. Minor: resolveGitCommonDir spawns a rev-parse --git-common-dir subprocess on every guarded call (no per-cwd memoization). Cheap and read-only, but adds one subprocess per guarded op.

None of these block. The change is minimal, correct, well-commented, and directly targets the incident. Approving.

[fzoll]

CI_FAILED

These checks never started: Check, Mobile Native Static Analysis, Test, Release Smoke. Nothing picked them up, which points at a runner being down rather than at your change. Re-trigger them and confirm they go green.

The review approved this PR, so CI is the only thing standing between it and merge.

[fzoll]

Addressed the CI_FAILED notice.

Root cause of the stuck checks: run 33891614463 sat in status: queued for over an hour with all four jobs (Check, Mobile Native Static Analysis, Release Smoke, Test) never picked up by a Blacksmith runner — confirmed via gh api repos/fzoll/t3code/actions/runs/33891614463 ("status":"queued", no job started). This repo's ci.yml only triggers on pull_request/push (no workflow_dispatch), and the run had already progressed past "queued" long enough that gh run rerun refused it ("workflow file may be broken" — GitHub's message for a run that can't be replayed). The only way to get a fresh dispatch was a new commit on the branch.

Used the opportunity productively: rather than push an empty commit, I addressed the reviewer's non-blocking note #1 from the APPROVED review — the lock test asserted maxActiveByRepo === 1 per repo but never asserted repoA and repoB actually overlapped, so a regression to a single global semaphore would still pass despite the test's "but not across repos" name. Added a globalActive/globalMaxActive counter alongside the existing per-repo counters and asserted globalMaxActive === 2, in commit 7cd1912.

Notes non-blocking items #2 and #3 from the review (other ref-writing fetch paths left unguarded; resolveGitCommonDir unmemoized) are explicitly scoped out by the PR description as residual/acceptable tradeoffs, and the review agreed these aren't blockers — leaving them as-is.

Verification: could not run vp test run/vp check/tsgo locally in this session — vp install fails workspace-wide because infra/relay's alchemy dependency 404s against the private pkg.ing registry mirror with no auth token available in this sandbox (unrelated to this change; same failure occurs on a clean checkout with no modifications). The added test code is a small, mechanical extension of the existing counter pattern in the same it.effect block (same file, lines ~932-1036) — mirrors the already-passing activeByRepo/maxActiveByRepo tracking exactly, just adds a global counterpart.

A fresh CI run is now in flight: https://github.com/fzoll/t3code/actions/runs/33897675989. Please confirm it goes green — I can't block this turn on its completion.

[fzoll]

VERDICT:APPROVED

I independently reviewed the diff against issue #3 (verifying claims directly on agent/issue-3 rather than relying on the prior self-review), and the fix is correct, minimal, and well-targeted at the reported race.

What the fix does

A per-repo single-permit Semaphore, keyed by gitCommonDir, serializes the three git operations implicated in the incident — fetchRemote, resolveRemoteTrackingCommit (rev-parse --verify refs/remotes/...), and createWorktree's git worktree add. Because all three share one lock per clone, a git fetch writing refs/remotes/* can no longer run concurrently with the rev-parse reader, which is exactly the fatal: Needed a single revision failure from #3.

Correctness — verified on the branch

• Race genuinely closed. All three guarded ops acquire the same single-permit semaphore per gitCommonDir, so no fetch and no rev-parse --verify ever execute simultaneously on the same clone. Per-operation locking (rather than one lock around the whole sequence) is sufficient here — and preferable, since it blocks less — because the failure mode is simultaneity of two specific commands, not sequence atomicity.
• No reentrancy deadlock. Effect semaphores aren't reentrant, so I checked every guarded path. The three ops are exposed as independent GitWorkflowService methods (GitWorkflowService.ts:298-309), each invoked separately by callers. createWorktree wraps only git worktree add under the permit; the follow-up configureBaseRef/listRemoteNames runs outside. resolveRemoteTrackingCommit wraps only the rev-parse. No guarded op nests another under a held permit, so the single-permit lock cannot self-block. A single lock type acquired non-nested has no ordering to deadlock on.
• Lazy lock creation is race-safe. acquireRepoGitLock re-checks membership inside Ref.modify (atomic); on contention the loser's freshly-made semaphore is discarded and the winner's is returned, so every fiber for a repo shares one instance. Layer.effect memoization means repoGitLocks is a single shared map across all callers, so the driver-level placement really does cover the ws.ts provisioning flow, GitManager.resolveBaseRangeRef, and PR-thread prep.
• No permit leak. withPermit releases on failure too, so a failing git command doesn't strand the lock.
• Keyed by gitCommonDir, not cwd — correct, so a worktree of the same clone serializes against the main clone, matching "same repo" from the issue.
• Imports (effect/Ref, effect/Semaphore) are present; Semaphore is already used elsewhere in the file.

Test quality

The added globalMaxActive === 2 assertion is a real improvement over the original test: it verifies that two distinct repos actually provision concurrently, so a regression to a single global lock would now fail rather than silently pass the per-repo-only assertions. The wall-clock realDelay(120) window widening is a reasonable way to make the race observable under it.effect's TestClock.

Non-blocking notes (residual, documented as out of scope)

1. Other ref-writing fetches remain unguarded. fetchRemoteBranch (GitVcsDriverCore.ts:2380), fetchRemoteTrackingBranch (2401), and fetchPullRequestBranch (2325) all git fetch into refs/remotes/* but are not wrapped in withRepoGitLock, so a concurrent resolveRemoteTrackingCommit against the same repo could still race them outside the provisioning flow. The PR explicitly scopes to the reported incident (which uses the guarded fetchRemote), so this is a documented tradeoff, not a blocker — but it's the most likely follow-up if similar failures recur.
2. resolveGitCommonDir spawns a rev-parse --git-common-dir subprocess on every guarded call (no per-cwd memoization). Cheap and read-only, and correctly left unguarded to avoid self-deadlock, but it adds one subprocess per guarded op — a candidate for memoization if it ever shows up in profiles.

None of these block. The change is minimal, correct, well-commented, and directly resolves the race in #3. Approving on the merits of the code. (CI status is outside this review's scope; the workflow-run/runner issues discussed in earlier comments are unrelated to the correctness of the change.)

[fzoll]

CI_FAILED

These checks never started: Check, Test, Release Smoke, Mobile Native Static Analysis. Nothing picked them up, which points at a runner being down rather than at your change. Re-trigger them and confirm they go green.

The review approved this PR, so CI is the only thing standing between it and merge.

[fzoll]

Re: CI_FAILED (2026-09-04T17:55:21Z) — investigated instead of pushing another empty/no-op commit, because the prior round already tried exactly that (commit 7cd1912, pushed specifically to get a fresh CI dispatch after run 33891614463 sat queued for >1h) and it reproduced the identical symptom.

Evidence this is a runner-pool outage, not a repo/code issue:

Every CI workflow run that has ever been created in this repository is stuck in status: queued with zero jobs ever picked up by a runner — not just on this PR:

• 33897675989 — agent/issue-3 (this PR, commit 7cd1912), created 16:54:04, still queued as of this comment (>1h)
• 33891614463 — agent/issue-3 (this PR, commit 393c64d), created 15:48:44, still queued
• 33891692257 — agent/issue-4 (a different branch entirely), created 15:49:33, still queued

All four jobs (Check, Test, Mobile Native Static Analysis, Release Smoke) across all three runs show started_at timestamps but status: queued / conclusion: null — i.e. GitHub Actions accepted the dispatch but no runner in the blacksmith-8vcpu-ubuntu-2404 / blacksmith-12vcpu-macos-26 pools (ci.yml's runs-on) has ever picked up a job, on any branch, in the observable history of this repo. gh api repos/fzoll/t3code/installations 404s and I don't have org/account access to inspect the Blacksmith GitHub App installation directly, but the pattern (100% of runs, across branches, over 1h+) is consistent with the runner pool being unreachable at the account level rather than anything in this PR's diff.

Switching runs-on to GitHub-hosted runners would sidestep this, but that's an infra change affecting every workflow in the repo, unrelated to issue #3, and not something I'll make unilaterally in a PR that's otherwise approved on the merits — it risks masking a Blacksmith account/billing/App-installation problem that someone needs to actually fix, and reverting it later if Blacksmith comes back would be extra churn. Nothing in GitVcsDriverCore.ts or the test added in 7cd1912 is implicated by or capable of causing this.

No further code changes to make in this round — the fix itself is approved twice (VERDICT:APPROVED at 15:52 and 16:57) and this queued-forever state reproduces identically across unrelated branches/commits, which rules out "re-trigger with a new commit" as a fix. This needs the Blacksmith runner pool restored at the account level.

[fzoll]

VERDICT:APPROVED

I reviewed the diff independently against issue #3, verifying each claim directly in the source on agent/issue-3 (not relying on the prior self-reviews). The fix is correct, minimal, and precisely targets the reported race.

What it does

A per-repo single-permit Semaphore keyed by gitCommonDir serializes the three git operations implicated in the incident — fetchRemote, resolveRemoteTrackingCommit (git rev-parse --verify refs/remotes/...), and createWorktree's git worktree add. Because all three share one permit per clone, a git fetch writing refs/remotes/* can never run concurrently with the rev-parse --verify reader — exactly the fatal: Needed a single revision failure from #3.

Correctness — verified in source

• Race genuinely closed. All three guarded ops acquire the same single-permit semaphore per gitCommonDir, so no fetch and no rev-parse --verify execute simultaneously on the same clone. Per-operation locking (rather than one lock spanning the whole sequence) is sufficient and preferable here: the failure mode is simultaneity of two specific commands, not atomicity of the sequence, and per-op locking blocks less.
• No reentrancy deadlock (Effect semaphores aren't reentrant, so I checked every guarded path). createWorktree:2296-2301 wraps only git worktree add; the follow-up listRemoteNames/configureBaseRef (2303-2314) run outside the permit. resolveRemoteTrackingCommit:2358-2372 runs listRemoteNames outside and wraps only the rev-parse. fetchRemote:2346-2352 wraps only the fetch. No guarded op nests another under a held permit.
• withRepoGitLock resolves gitCommonDir outside the permit (line 984-988), so its own rev-parse --git-common-dir subprocess cannot self-deadlock against the lock it's about to acquire. That command also isn't itself guarded (it's not a --verify rev-parse), so it can't contend.
• Lazy lock creation is race-safe. acquireRepoGitLock:968-981 re-checks membership inside Ref.modify (atomic); on contention the loser's freshly-made semaphore is discarded and the winner's returned, so every fiber for a repo shares one instance.
• Single shared map. export const layer = Layer.effect(GitVcsDriver, make) (GitVcsDriver.ts:877) is memoized, so repoGitLocks is one map across all callers — the driver-level placement really does cover the ws.ts provisioning flow, GitManager.resolveBaseRangeRef, and PR-thread prep.
• No permit leak. Semaphore.withPermit releases on failure and interruption, so a failing git command doesn't strand the lock.
• Keyed by gitCommonDir, not cwd — correct: a worktree of the same clone serializes against the main clone, matching "same repo" from the issue.

Test quality

The globalMaxActive === 2 assertion is a real improvement: it proves two distinct repos actually provision concurrently, so a regression to a single global lock would fail rather than silently pass the per-repo-only assertions. The realDelay(120) window-widening is a sound way to make the race observable under it.effect's TestClock.

Non-blocking notes (residual, documented as out of scope)

1. Other ref-writing fetches remain unguarded — fetchRemoteBranch (2377), fetchRemoteTrackingBranch (2399), fetchPullRequestBranch (2325) all git fetch into refs/remotes/* / refs/heads/* without withRepoGitLock, so a concurrent resolveRemoteTrackingCommit against the same repo could still race them outside the provisioning flow. The PR explicitly scopes to the reported incident (which uses the guarded fetchRemote), so this is a documented tradeoff — but it's the most likely follow-up if similar failures recur.
2. resolveGitCommonDir spawns a subprocess per guarded call (no per-cwd memoization). Cheap and read-only, correctly left unguarded, but a candidate for memoization if it ever shows in profiles.

Verification caveat

I confirmed imports (effect/Ref, effect/Semaphore), the non-nesting of guarded ops, the atomic double-checked lock creation, layer memoization, and the unguarded fetch paths — all by direct source inspection. I did not execute the test suite / typecheck in this session (the workspace's private-registry install is unavailable here), so my correctness assessment is static, consistent with the prior reviews' scoped verification.

None of the notes block. The change is minimal, correct, well-commented, and directly resolves the race in #3. Approving on the merits of the code; CI/runner status is outside this review's scope.