fix(worker): remove PR worktrees before releasing the PR lock - #2564
Conversation
Follow-up and merge-conflict jobs released the PR processing lock before removing their git worktree. Removal takes seconds, and the worktree keeps the PR branch checked out until it finishes, so a queued job that took the lock in that window failed with "branch ... is locked by another worktree" (twice on #2555). Both jobs now remove the worktree first. As a safety net, when `git worktree remove --force` refuses a worktree whose .git file is already gone, only that one registration is dropped (and only when git reports it as prunable) before retrying. A global `git worktree prune` stays avoided, since it can race other tasks. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
/ultrafix |
|
🔄 Ultrafix loop started (goal: 8/10, max cycles: 10) First action:
|
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
|
CI failed: Full Test Suite Shard 3/4 Please investigate and fix this CI failure.
Failure evidence
|
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR addresses the reported race by awaiting worktree cleanup before releasing either job’s PR lock, with targeted recovery for stale Git registrations. Merge-ready within scope, conditional on the four pending test shards passing. ✅ Correct cleanup ordering — Both cleanup paths await worktree removal before releasing the lock, while preserving lock release after a caught cleanup failure. ✅ Targeted stale recovery — The fallback requires Git’s ✅ Relevant regression coverage — The tests cover cleanup ordering and reproduce the missing- This was a static review of the supplied diff and context; no commands were run. The supplied current checks report no failures. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Exercise the integrated recovery pathThe real-Git tests call ScoreThe change is focused, addresses the demonstrated ordering failure, and includes relevant regression tests. No merge-blocking defect was identified; four test shards remain pending. Score: 9/10 🤖 Review Details
🤖 Review by ProPR |
|
✅ Applied the requested follow-up changes in commit 209e8c3 Summary of ChangesFixed the shard failure in Reproduced the failure before the fix; all three related test files pass afterward. The Node.js deprecation warning was unrelated. No visual changes; no previews needed. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…test.ts`: the assertion now accepts both Git error messages, including CI’s “already checked out at.” Fixed the shard failure in `test/staleWorktreeRegistration.test.ts`: the assertion now accepts both Git error messages, including CI’s “already checked out at.” Reproduced the failure before the fix; all three related test files pass afterward. The Node.js deprecation warning was unrelated. No visual changes; no previews needed. PR: #2564 Comment by: @propr-dev[bot] (ID: 5850956378) Model: gpt-6-astra
Full Test Suite ResultsValidation failed during: Redis startup (shard 1), Shard coverage verification.
View shard 1/4 output |
|
✅ Applied the requested follow-up changes in commit 2e0c5b7 Summary of ChangesAdded bounded Redis startup and cleanup retries, preserving container ownership checks and reporting diagnostics before cleanup. Shard coverage failed because shard 1 never ran; coverage enforcement remains unchanged. Validation: 69 tests passed, including new regression cases; shell syntax and diff checks passed. Full CI wasn’t rerun locally because Docker is unavailable. No visual changes. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
… container ownership checks and reporting diagnostics before cleanup. Added bounded Redis startup and cleanup retries, preserving container ownership checks and reporting diagnostics before cleanup. Shard coverage failed because shard 1 never ran; coverage enforcement remains unchanged. Validation: 69 tests passed, including new regression cases; shell syntax and diff checks passed. Full CI wasn’t rerun locally because Docker is unavailable. No visual changes. PR: #2564 Comment by: @github-actions[bot] (ID: 5851292805) Model: gpt-6-astra
2 participants
Problem
Follow-ups on #2555 failed twice with
Cannot create worktree: branch '…' is locked by another worktree. The worker logs show a race:worktree addfails: branch "already used by worktree" (the one being removed);worktree remove --forcethen fails withvalidation failed … .git does not existcleanupJob(src/jobs/prCommentJobUtils.ts) and the merge job'sreleaseMergeJobResourcesboth released the lock before removing the worktree, which keeps the PR branch checked out until removal completes. The 22:43 failure was the same race against the merge job's worktree.Fix
packages/core/src/git/worktreeCreation.ts): when the forced removal of the conflicting worktree fails,removeStaleWorktreeRegistrationdrops only that worktree's registration, and only ifgit worktree list --porcelainreports it asprunable, then retries. A globalgit worktree prunestays avoided, ascleanupWorktreenotes it races other tasks. This also recovers from registrations left behind by a worker crash.Tests
test/staleWorktreeRegistration.test.ts(real git): reproduces the exact production errors (already used by worktree, thenvalidation failedon forced remove) and shows the branch is freed; live worktrees and other stale registrations are left alone.test/prCommentCleanupOrder.test.ts:cleanupJobremoves the worktree before releasing the lock.test/processMergeConflictJob.test.ts: the lock is still held while the merge job's worktree is removed.🤖 Generated with Claude Code