Fix Windows worktree force delete cleanup (#6704)
* Fix Windows worktree force delete cleanup * Fix Windows cleanup rm option type import
This commit is contained in:
parent
b86242469c
commit
fdee6dd8cb
|
|
@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
|
|
|
|||
|
|
@ -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<void> {
|
||||
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<void> {
|
||||
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.
|
||||
|
|
|
|||
|
|
@ -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<T>(platform: NodeJS.Platform, fn: () => Promise<T>): Promise<T> {
|
||||
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()
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -62,7 +62,7 @@ async function pruneRequiredGitWorktreeRegistration(
|
|||
export async function recoverLocalWindowsLongPathWorktreeRemoval(
|
||||
args: LocalWindowsLongPathRecoveryArgs
|
||||
): Promise<RemoveWorktreeResult | undefined> {
|
||||
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<RemoveWorktreeResult> {
|
||||
|
|
|
|||
Loading…
Reference in New Issue