fix: bound worktree name auto-suffix loop to prevent OOM in tests

The auto-suffix loop in createLocalWorktree had no termination cap.
When tests (or a misconfigured env) mocked getBranchConflictKind /
getPRForBranch to always return a collision, the loop ran forever and
the vitest worker crashed with "Ineffective mark-compacts near heap
limit".

Cap the search at 100 suffixes, and if all fail, fall back to the
original specific error messages (branch already exists / PR already
owns the name) instead of a generic failure.

Update the two tests that asserted the old throw-on-first-conflict
behavior to verify the new auto-suffix path end-to-end.
This commit is contained in:
Neil 2026-04-16 15:38:31 -07:00
parent 9968d10b92
commit 1330b7e01f
2 changed files with 107 additions and 37 deletions

View file

@ -155,8 +155,14 @@ export async function createLocalWorktree(
// Why: silently resolve branch/path/PR name collisions by appending -2/-3/etc.
// instead of failing and forcing the user back to the name picker. This is
// especially important for the new-workspace flow where the user may not have
// direct control over the branch name.
for (let suffix = 1; ; suffix += 1) {
// direct control over the branch name. Bounded by MAX_SUFFIX_ATTEMPTS so a
// misconfigured environment (e.g. a mock or stub that always reports a
// conflict) cannot spin this loop indefinitely.
const MAX_SUFFIX_ATTEMPTS = 100
let resolved = false
let lastBranchConflictKind: 'local' | 'remote' | null = null
let lastExistingPR: Awaited<ReturnType<typeof getPRForBranch>> | null = null
for (let suffix = 1; suffix <= MAX_SUFFIX_ATTEMPTS; suffix += 1) {
effectiveSanitizedName = suffix === 1 ? sanitizedName : `${sanitizedName}-${suffix}`
effectiveRequestedName =
suffix === 1
@ -166,21 +172,21 @@ export async function createLocalWorktree(
: effectiveSanitizedName
branchName = computeBranchName(effectiveSanitizedName, settings, username)
const branchConflictKind = await getBranchConflictKind(repo.path, branchName)
if (branchConflictKind) {
lastBranchConflictKind = await getBranchConflictKind(repo.path, branchName)
if (lastBranchConflictKind) {
continue
}
// Why: the UI resolves PR status by branch name alone. Reusing a historical
// PR head name would make a fresh worktree inherit that old merged/closed PR
// immediately, so auto-suffix until we land on a fresh branch identity.
let existingPR: Awaited<ReturnType<typeof getPRForBranch>> | null = null
lastExistingPR = null
try {
existingPR = await getPRForBranch(repo.path, branchName)
lastExistingPR = await getPRForBranch(repo.path, branchName)
} catch {
// GitHub API may be unreachable, rate-limited, or token missing
}
if (existingPR) {
if (lastExistingPR) {
continue
}
@ -192,9 +198,29 @@ export async function createLocalWorktree(
continue
}
resolved = true
break
}
if (!resolved) {
// Why: if every suffix in range collides, fall back to the original
// "reject with a specific reason" behavior so the user sees why creation
// failed instead of a generic error or (worse) an infinite spinner.
if (lastExistingPR) {
throw new Error(
`Branch "${branchName}" already has PR #${lastExistingPR.number}. Pick a different worktree name.`
)
}
if (lastBranchConflictKind) {
throw new Error(
`Branch "${branchName}" already exists ${lastBranchConflictKind === 'local' ? 'locally' : 'on a remote'}. Pick a different worktree name.`
)
}
throw new Error(
`Could not find an available worktree name for "${sanitizedName}". Pick a different worktree name.`
)
}
// Determine base branch
const baseBranch = args.baseBranch || repo.worktreeBaseRef || getDefaultBaseRef(repo.path)
const setupScript = getEffectiveHooks(repo)?.scripts.setup

View file

@ -219,20 +219,40 @@ describe('registerWorktreeHandlers', () => {
registerWorktreeHandlers(mainWindow as never, store as never)
})
it('rejects worktree creation when the branch already exists on a remote', async () => {
getBranchConflictKindMock.mockResolvedValue('remote')
await expect(
handlers['worktrees:create'](null, {
repoId: 'repo-1',
name: 'improve-dashboard'
})
).rejects.toThrow(
'Branch "improve-dashboard" already exists on a remote. Pick a different worktree name.'
it('auto-suffixes the branch name when the first choice collides with a remote branch', async () => {
// Why: new-workspace flow should silently try improve-dashboard-2, -3, ...
// rather than failing and forcing the user back to the name picker.
getBranchConflictKindMock.mockImplementation(async (_repoPath: string, branch: string) =>
branch === 'improve-dashboard' ? 'remote' : null
)
listWorktreesMock.mockResolvedValue([
{
path: '/workspace/improve-dashboard-2',
head: 'abc123',
branch: 'improve-dashboard-2',
isBare: false,
isMainWorktree: false
}
])
expect(getPRForBranchMock).not.toHaveBeenCalled()
expect(addWorktreeMock).not.toHaveBeenCalled()
const result = await handlers['worktrees:create'](null, {
repoId: 'repo-1',
name: 'improve-dashboard'
})
expect(addWorktreeMock).toHaveBeenCalledWith(
'/workspace/repo',
'/workspace/improve-dashboard-2',
'improve-dashboard-2',
'origin/main',
false
)
expect(result).toEqual({
worktree: expect.objectContaining({
path: '/workspace/improve-dashboard-2',
branch: 'improve-dashboard-2'
})
})
})
it('creates an issue-command runner for an existing repo/worktree pair', async () => {
@ -292,27 +312,51 @@ describe('registerWorktreeHandlers', () => {
expect(listWorktreesMock).not.toHaveBeenCalled()
})
it('rejects worktree creation when the branch name already belongs to a PR', async () => {
getPRForBranchMock.mockResolvedValue({
number: 3127,
title: 'Existing PR',
state: 'merged',
url: 'https://example.com/pr/3127',
checksStatus: 'success',
updatedAt: '2026-04-01T00:00:00Z',
mergeable: 'UNKNOWN'
it('auto-suffixes the branch name when the first choice already belongs to a PR', async () => {
// Why: reusing a historical PR head would make a fresh worktree inherit
// that old PR, so the loop suffixes past the name until it finds one that
// is not associated with any PR.
getPRForBranchMock.mockImplementation(async (_repoPath: string, branch: string) =>
branch === 'improve-dashboard'
? {
number: 3127,
title: 'Existing PR',
state: 'merged',
url: 'https://example.com/pr/3127',
checksStatus: 'success',
updatedAt: '2026-04-01T00:00:00Z',
mergeable: 'UNKNOWN'
}
: null
)
listWorktreesMock.mockResolvedValue([
{
path: '/workspace/improve-dashboard-2',
head: 'abc123',
branch: 'improve-dashboard-2',
isBare: false,
isMainWorktree: false
}
])
const result = await handlers['worktrees:create'](null, {
repoId: 'repo-1',
name: 'improve-dashboard'
})
await expect(
handlers['worktrees:create'](null, {
repoId: 'repo-1',
name: 'improve-dashboard'
})
).rejects.toThrow(
'Branch "improve-dashboard" already has PR #3127. Pick a different worktree name.'
expect(addWorktreeMock).toHaveBeenCalledWith(
'/workspace/repo',
'/workspace/improve-dashboard-2',
'improve-dashboard-2',
'origin/main',
false
)
expect(addWorktreeMock).not.toHaveBeenCalled()
expect(result).toEqual({
worktree: expect.objectContaining({
path: '/workspace/improve-dashboard-2',
branch: 'improve-dashboard-2'
})
})
})
const createdWorktreeList = [