From fdee6dd8cb09340f3e628b8892686f7fe7b0b60b Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Sun, 28 Jun 2026 23:23:47 -0700 Subject: [PATCH] Fix Windows worktree force delete cleanup (#6704) * Fix Windows worktree force delete cleanup * Fix Windows cleanup rm option type import --- src/main/local-worktree-filesystem.test.ts | 69 ++++++++++++-- src/main/local-worktree-filesystem.ts | 55 ++++++++++- .../local-worktree-removal-recovery.test.ts | 94 +++++++++++++++++++ src/main/local-worktree-removal-recovery.ts | 18 +++- 4 files changed, 226 insertions(+), 10 deletions(-) create mode 100644 src/main/local-worktree-removal-recovery.test.ts diff --git a/src/main/local-worktree-filesystem.test.ts b/src/main/local-worktree-filesystem.test.ts index d49895a6c..b41adb648 100644 --- a/src/main/local-worktree-filesystem.test.ts +++ b/src/main/local-worktree-filesystem.test.ts @@ -49,6 +49,7 @@ describe('local worktree filesystem runtime access', () => { }) afterEach(() => { + vi.useRealTimers() vi.restoreAllMocks() }) @@ -63,10 +64,13 @@ describe('local worktree filesystem runtime access', () => { expect(lstatMock).toHaveBeenCalledWith('C:\\repo\\.git') expect(readFileMock).toHaveBeenCalledWith('C:\\repo\\.git', 'utf8') - expect(rmMock).toHaveBeenCalledWith(toHostRemovalPath('C:\\repo\\feature'), { - recursive: true, - force: true - }) + expect(rmMock).toHaveBeenCalledWith( + toHostRemovalPath('C:\\repo\\feature'), + expect.objectContaining({ + recursive: true, + force: true + }) + ) expect(execFileMock).not.toHaveBeenCalled() }) @@ -77,10 +81,59 @@ describe('local worktree filesystem runtime access', () => { await removeLocalWorktreePath(longPath) expect(toHostRemovalPath(longPath)).toBe(`\\\\?\\${longPath}`) - expect(rmMock).toHaveBeenCalledWith(`\\\\?\\${longPath}`, { - recursive: true, - force: true - }) + expect(rmMock).toHaveBeenCalledWith( + `\\\\?\\${longPath}`, + expect.objectContaining({ + recursive: true, + force: true, + maxRetries: expect.any(Number), + retryDelay: expect.any(Number) + }) + ) + }) + }) + + it('retries transient host removal failures on Windows', async () => { + vi.useFakeTimers() + await withPlatform('win32', async () => { + const error = Object.assign(new Error('Directory not empty'), { code: 'ENOTEMPTY' }) + rmMock.mockRejectedValueOnce(error).mockResolvedValueOnce(undefined) + + const removal = removeLocalWorktreePath('C:\\repo\\feature') + await vi.advanceTimersByTimeAsync(250) + + await expect(removal).resolves.toBeUndefined() + expect(rmMock).toHaveBeenCalledTimes(2) + expect(rmMock).toHaveBeenNthCalledWith( + 1, + toHostRemovalPath('C:\\repo\\feature'), + expect.objectContaining({ + recursive: true, + force: true, + maxRetries: expect.any(Number), + retryDelay: expect.any(Number) + }) + ) + expect(rmMock).toHaveBeenNthCalledWith( + 2, + toHostRemovalPath('C:\\repo\\feature'), + expect.objectContaining({ + recursive: true, + force: true, + maxRetries: expect.any(Number), + retryDelay: expect.any(Number) + }) + ) + }) + }) + + it('does not retry host removal failures outside Windows', async () => { + await withPlatform('linux', async () => { + const error = Object.assign(new Error('Directory not empty'), { code: 'ENOTEMPTY' }) + rmMock.mockRejectedValue(error) + + await expect(removeLocalWorktreePath('/repo/feature')).rejects.toBe(error) + expect(rmMock).toHaveBeenCalledTimes(1) }) }) diff --git a/src/main/local-worktree-filesystem.ts b/src/main/local-worktree-filesystem.ts index 867513728..eb7c888bb 100644 --- a/src/main/local-worktree-filesystem.ts +++ b/src/main/local-worktree-filesystem.ts @@ -1,6 +1,8 @@ import { execFile } from 'node:child_process' +import type { RmOptions } from 'node:fs' import { lstat, readFile, rm } from 'node:fs/promises' import { win32 } from 'node:path' +import { setTimeout as delay } from 'node:timers/promises' import { buildWslLoginShellCommand, escapeWslShCommandForWindows, @@ -24,6 +26,9 @@ type ExecFileTextResult = { } const WSL_FILE_OPERATION_TIMEOUT_MS = 30_000 +const WINDOWS_REMOVE_RETRY_DELAYS_MS = [250, 500, 1_000, 2_000] +const WINDOWS_RM_MAX_RETRIES = 8 +const WINDOWS_RM_RETRY_DELAY_MS = 150 function shouldUseWslFilesystem(options: LocalWorktreeFilesystemOptions): boolean { return process.platform === 'win32' && !!options.wslDistro?.trim() @@ -112,7 +117,7 @@ export async function removeLocalWorktreePath( ): Promise { const distro = options.wslDistro?.trim() if (!shouldUseWslFilesystem(options) || !distro) { - await rm(toHostRemovalPath(targetPath), { recursive: true, force: true }) + await removeHostWorktreePath(targetPath) return } @@ -121,6 +126,54 @@ export async function removeLocalWorktreePath( await runWslLoginShellCommand(distro, `rm -rf -- ${quotePosixShell(toLinuxPath(targetPath))}`) } +async function removeHostWorktreePath(targetPath: string): Promise { + const removalPath = toHostRemovalPath(targetPath) + const retryDelays = process.platform === 'win32' ? WINDOWS_REMOVE_RETRY_DELAYS_MS : [] + const rmOptions = getHostRemovalOptions() + let attempt = 0 + + while (true) { + try { + await rm(removalPath, rmOptions) + return + } catch (error) { + if (attempt >= retryDelays.length || !isTransientWindowsRemovalError(error)) { + throw error + } + // Why: Git/Node recursive deletes on Windows can observe a just-emptied + // directory before antivirus/indexers/handles release it. + await delay(retryDelays[attempt]) + attempt += 1 + } + } +} + +function getHostRemovalOptions(): RmOptions { + const base = { recursive: true, force: true } + if (process.platform !== 'win32') { + return base + } + return { + ...base, + // Why: large Windows dependency trees commonly surface transient + // ENOTEMPTY/EPERM while Node walks and removes nested directories. + maxRetries: WINDOWS_RM_MAX_RETRIES, + retryDelay: WINDOWS_RM_RETRY_DELAY_MS + } +} + +function isTransientWindowsRemovalError(error: unknown): boolean { + if (process.platform !== 'win32' || typeof error !== 'object' || error === null) { + return false + } + const code = 'code' in error && typeof error.code === 'string' ? error.code : undefined + if (code && ['EBUSY', 'ENOTEMPTY', 'EPERM'].includes(code)) { + return true + } + const message = 'message' in error && typeof error.message === 'string' ? error.message : '' + return /directory not empty|resource busy|operation not permitted/i.test(message) +} + export function toHostRemovalPath(targetPath: string): string { // Why: Git for Windows can fail long recursive deletes even after Orca has // proven the worktree target; Node's host deletion should use Win32 long paths. diff --git a/src/main/local-worktree-removal-recovery.test.ts b/src/main/local-worktree-removal-recovery.test.ts new file mode 100644 index 000000000..9f3d75502 --- /dev/null +++ b/src/main/local-worktree-removal-recovery.test.ts @@ -0,0 +1,94 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock, removeLocalWorktreePathMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + removeLocalWorktreePathMock: vi.fn() +})) + +vi.mock('./git/runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock +})) + +vi.mock('./local-worktree-filesystem', () => ({ + removeLocalWorktreePath: removeLocalWorktreePathMock +})) + +import { recoverLocalWindowsLongPathWorktreeRemoval } from './local-worktree-removal-recovery' + +async function withPlatform(platform: NodeJS.Platform, fn: () => Promise): Promise { + const original = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: platform }) + try { + return await fn() + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: original }) + } +} + +describe('recoverLocalWindowsLongPathWorktreeRemoval', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + removeLocalWorktreePathMock.mockReset() + gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + removeLocalWorktreePathMock.mockResolvedValue(undefined) + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + it('recovers Git for Windows partial filesystem deletion failures', async () => { + await withPlatform('win32', async () => { + const error = Object.assign(new Error('git worktree remove failed'), { + stderr: "error: failed to delete 'C:/repo/worktree/delete-e2e-held-cwd': Permission denied" + }) + + const result = await recoverLocalWindowsLongPathWorktreeRemoval({ + error, + force: true, + canonicalWorktreePath: 'C:/repo/worktree/delete-e2e-held-cwd', + repoPath: 'C:/repo', + localWorktreeGitOptions: {}, + registeredWorktree: { branch: 'refs/heads/delete-e2e-held-cwd', head: 'abc123' }, + deleteBranch: true, + closeWatcher: vi.fn().mockResolvedValue(undefined) + }) + + expect(removeLocalWorktreePathMock).toHaveBeenCalledWith( + 'C:/repo/worktree/delete-e2e-held-cwd', + {} + ) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'prune'], { + cwd: 'C:/repo' + }) + expect(result).toEqual({ + preservedBranch: { + branchName: 'delete-e2e-held-cwd', + head: 'abc123' + } + }) + }) + }) + + it('does not recover partial filesystem deletion wording off Windows', async () => { + await withPlatform('linux', async () => { + const error = Object.assign(new Error('git worktree remove failed'), { + stderr: "error: failed to delete 'C:/repo/worktree/delete-e2e-held-cwd': Permission denied" + }) + + await expect( + recoverLocalWindowsLongPathWorktreeRemoval({ + error, + force: true, + canonicalWorktreePath: 'C:/repo/worktree/delete-e2e-held-cwd', + repoPath: 'C:/repo', + localWorktreeGitOptions: {}, + registeredWorktree: { branch: 'refs/heads/delete-e2e-held-cwd', head: 'abc123' }, + deleteBranch: true, + closeWatcher: vi.fn().mockResolvedValue(undefined) + }) + ).resolves.toBeUndefined() + expect(removeLocalWorktreePathMock).not.toHaveBeenCalled() + }) + }) +}) diff --git a/src/main/local-worktree-removal-recovery.ts b/src/main/local-worktree-removal-recovery.ts index 984feb8c3..4e915bfa9 100644 --- a/src/main/local-worktree-removal-recovery.ts +++ b/src/main/local-worktree-removal-recovery.ts @@ -62,7 +62,7 @@ async function pruneRequiredGitWorktreeRegistration( export async function recoverLocalWindowsLongPathWorktreeRemoval( args: LocalWindowsLongPathRecoveryArgs ): Promise { - if (!args.force || !isWindowsLongPathWorktreeRemovalError(args.error)) { + if (!args.force || !isRecoverableWindowsFilesystemRemovalError(args.error)) { return undefined } @@ -82,6 +82,22 @@ export async function recoverLocalWindowsLongPathWorktreeRemoval( return preservedBranchResult(args.registeredWorktree, args.deleteBranch) } +function isRecoverableWindowsFilesystemRemovalError(error: unknown): boolean { + if (isWindowsLongPathWorktreeRemovalError(error)) { + return true + } + if (process.platform !== 'win32' || typeof error !== 'object' || error === null) { + return false + } + const errorWithDetails = error as { message?: unknown; stderr?: unknown; stdout?: unknown } + const details = [errorWithDetails.stderr, errorWithDetails.stdout, errorWithDetails.message] + .filter((value): value is string => typeof value === 'string' && value.trim().length > 0) + .join('\n') + return /failed to delete .*(?:directory not empty|permission denied|access is denied|being used by another process)|(?:directory not empty|permission denied|access is denied|being used by another process).*failed to delete/i.test( + details + ) +} + export async function pruneStaleLocalWorktreeRegistrationAfterFilesystemRemoval( args: StaleLocalWorktreeRegistrationArgs ): Promise {