From 140615a11f0f4bf75fd5bd660fa92f82fa127e18 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Fri, 10 Jul 2026 00:59:48 -0400 Subject: [PATCH] Recover Windows worktree deletion after Git deregisters --- src/main/ipc/worktrees.ts | 8 +- ...cal-worktree-removal-recovery-live.test.ts | 129 ++++++++++++++++++ .../local-worktree-removal-recovery.test.ts | 97 ++++++++++++- src/main/local-worktree-removal-recovery.ts | 74 ++++++---- src/main/runtime/orca-runtime.ts | 8 +- 5 files changed, 280 insertions(+), 36 deletions(-) create mode 100644 src/main/local-worktree-removal-recovery-live.test.ts diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index 25e276fe904..469a06a615f 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -132,7 +132,7 @@ import { } from '../local-worktree-filesystem' import { removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval, - recoverLocalWindowsLongPathWorktreeRemoval + recoverLocalWindowsWorktreeRemoval } from '../local-worktree-removal-recovery' const WORKTREE_ARCHIVE_HOOK_TIMEOUT_MS = 120_000 @@ -1766,9 +1766,9 @@ export function registerWorktreeHandlers( refreshedRegisteredWorktree.head ) } catch (error) { - // Why: Git for Windows can fail long-path directory deletion after - // Orca has already validated the target and explicit force delete. - const recoveredRemovalResult = await recoverLocalWindowsLongPathWorktreeRemoval({ + // Why: Git for Windows can deregister a clean worktree before its + // recursive filesystem deletion fails transiently. + const recoveredRemovalResult = await recoverLocalWindowsWorktreeRemoval({ error, force: args.force ?? false, canonicalWorktreePath, diff --git a/src/main/local-worktree-removal-recovery-live.test.ts b/src/main/local-worktree-removal-recovery-live.test.ts new file mode 100644 index 00000000000..7c7978be000 --- /dev/null +++ b/src/main/local-worktree-removal-recovery-live.test.ts @@ -0,0 +1,129 @@ +import { spawn, spawnSync, type ChildProcess } from 'node:child_process' +import { existsSync } from 'node:fs' +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import path from 'node:path' +import { setTimeout as delay } from 'node:timers/promises' +import { afterEach, describe, expect, it } from 'vitest' +import { recoverLocalWindowsWorktreeRemoval } from './local-worktree-removal-recovery' + +const tempRoots: string[] = [] + +function git(cwd: string, args: string[]): string { + const result = spawnSync('git', args, { cwd, encoding: 'utf8' }) + if (result.status !== 0) { + throw new Error(result.stderr || result.stdout || `git ${args.join(' ')} failed`) + } + return result.stdout.trim() +} + +async function waitForPath(targetPath: string): Promise { + const deadline = Date.now() + 5_000 + while (!existsSync(targetPath)) { + if (Date.now() >= deadline) { + throw new Error(`Timed out waiting for ${targetPath}`) + } + await delay(20) + } +} + +async function stopProcess(child: ChildProcess): Promise { + if (child.exitCode !== null || child.signalCode !== null) { + return + } + const exited = new Promise((resolve) => child.once('exit', () => resolve())) + child.kill() + await Promise.race([exited, delay(500)]) + if (child.exitCode === null && child.pid) { + spawnSync('taskkill', ['/pid', String(child.pid), '/t', '/f'], { stdio: 'ignore' }) + await Promise.race([exited, delay(2_000)]) + } + if (child.exitCode === null && child.signalCode === null) { + throw new Error('Failed to stop the filesystem churn process') + } +} + +describe('local Windows worktree removal recovery (live Git)', () => { + afterEach(async () => { + await Promise.all( + tempRoots + .splice(0) + .map((root) => rm(root, { recursive: true, force: true, maxRetries: 8, retryDelay: 100 })) + ) + }) + + it.runIf(process.platform === 'win32')( + 'finishes deletion when Git deregisters before a directory-not-empty failure', + async () => { + const root = await mkdtemp(path.join(tmpdir(), 'orca-worktree-enotempty-')) + tempRoots.push(root) + const repoPath = path.join(root, 'repo') + const worktreePath = path.join(root, 'worktree') + + git(root, ['init', '--quiet', repoPath]) + git(repoPath, ['config', 'user.email', 'test@example.com']) + git(repoPath, ['config', 'user.name', 'Test User']) + await writeFile(path.join(repoPath, '.gitignore'), 'churn/\n') + git(repoPath, ['add', '.gitignore']) + git(repoPath, ['commit', '--quiet', '-m', 'initial']) + git(repoPath, ['worktree', 'add', '--quiet', '-b', 'feature/churn', worktreePath]) + const head = git(worktreePath, ['rev-parse', 'HEAD']) + + // Why: a producer racing Git's recursive delete deterministically exposes + // the Windows ENOTEMPTY state reported by real workspace deletions. + const churn = spawn( + process.execPath, + [ + '-e', + [ + "const fs = require('node:fs')", + "const path = require('node:path')", + 'const root = process.argv[1]', + 'let i = 0', + 'while (fs.existsSync(root)) {', + ' try {', + " const dir = path.join(root, 'churn')", + ' fs.mkdirSync(dir, { recursive: true })', + " fs.writeFileSync(path.join(dir, 'f' + (i++ % 64) + '.txt'), 'x')", + ' } catch {}', + '}' + ].join('\n'), + worktreePath + ], + { stdio: 'ignore' } + ) + + try { + await waitForPath(path.join(worktreePath, 'churn')) + const removal = spawnSync('git', ['worktree', 'remove', worktreePath], { + cwd: repoPath, + encoding: 'utf8' + }) + expect(removal.status).not.toBe(0) + expect(git(repoPath, ['worktree', 'list', '--porcelain'])).not.toContain(worktreePath) + expect(existsSync(worktreePath)).toBe(true) + + await stopProcess(churn) + await expect( + recoverLocalWindowsWorktreeRemoval({ + error: Object.assign(new Error('git worktree remove failed'), { + stderr: removal.stderr + }), + force: false, + canonicalWorktreePath: worktreePath, + repoPath, + localWorktreeGitOptions: {}, + registeredWorktree: { branch: 'refs/heads/feature/churn', head }, + deleteBranch: false, + overrideLock: false, + closeWatcher: async () => {} + }) + ).resolves.toEqual({}) + + expect(existsSync(worktreePath)).toBe(false) + } finally { + await stopProcess(churn) + } + } + ) +}) diff --git a/src/main/local-worktree-removal-recovery.test.ts b/src/main/local-worktree-removal-recovery.test.ts index 5985f21956b..aec47789aba 100644 --- a/src/main/local-worktree-removal-recovery.test.ts +++ b/src/main/local-worktree-removal-recovery.test.ts @@ -26,7 +26,7 @@ vi.mock('./git/worktree', () => ({ })) import { - recoverLocalWindowsLongPathWorktreeRemoval, + recoverLocalWindowsWorktreeRemoval, removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval } from './local-worktree-removal-recovery' @@ -40,7 +40,7 @@ async function withPlatform(platform: NodeJS.Platform, fn: () => Promise): } } -describe('recoverLocalWindowsLongPathWorktreeRemoval', () => { +describe('recoverLocalWindowsWorktreeRemoval', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() listWorktreesStrictMock.mockReset() @@ -62,7 +62,7 @@ describe('recoverLocalWindowsLongPathWorktreeRemoval', () => { stderr: "error: failed to delete 'C:/repo/worktree/delete-e2e-held-cwd': Permission denied" }) - const result = await recoverLocalWindowsLongPathWorktreeRemoval({ + const result = await recoverLocalWindowsWorktreeRemoval({ error, force: true, canonicalWorktreePath: 'C:/repo/worktree/delete-e2e-held-cwd', @@ -90,6 +90,93 @@ describe('recoverLocalWindowsLongPathWorktreeRemoval', () => { }) }) + it('finishes a clean non-force deletion after Git leaves a Windows directory behind', 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': Directory not empty" + }) + + const result = await recoverLocalWindowsWorktreeRemoval({ + error, + force: false, + canonicalWorktreePath: 'C:/repo/worktree/delete-e2e-held-cwd', + repoPath: 'C:/repo', + localWorktreeGitOptions: {}, + registeredWorktree: { branch: 'refs/heads/delete-e2e-held-cwd', head: 'abc123' }, + deleteBranch: true, + overrideLock: false, + 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('recovers localized Windows failures after Git already removed the registration', async () => { + await withPlatform('win32', async () => { + const result = await recoverLocalWindowsWorktreeRemoval({ + error: Object.assign(new Error('git worktree remove failed'), { + stderr: "Fehler: 'C:/repo/worktree/feature' konnte nicht gelöscht werden" + }), + force: false, + canonicalWorktreePath: 'C:/repo/worktree/feature', + repoPath: 'C:/repo', + localWorktreeGitOptions: {}, + registeredWorktree: { branch: 'refs/heads/feature', head: 'abc123' }, + deleteBranch: false, + overrideLock: false, + closeWatcher: vi.fn().mockResolvedValue(undefined) + }) + + expect(removeLocalWorktreePathMock).toHaveBeenCalledWith('C:/repo/worktree/feature', {}) + expect(result).toEqual({}) + }) + }) + + it('does not recover unrelated localized failures while Git still owns the row', async () => { + await withPlatform('win32', async () => { + listWorktreesStrictMock.mockResolvedValue([ + { + path: 'C:/repo/worktree/feature', + head: 'abc123', + branch: 'refs/heads/feature', + isBare: false, + isMainWorktree: false + } + ]) + + await expect( + recoverLocalWindowsWorktreeRemoval({ + error: Object.assign(new Error('git worktree remove failed'), { + stderr: 'Fehler: unerwarteter Git-Zustand' + }), + force: false, + canonicalWorktreePath: 'C:/repo/worktree/feature', + repoPath: 'C:/repo', + localWorktreeGitOptions: {}, + registeredWorktree: { branch: 'refs/heads/feature', head: 'abc123' }, + deleteBranch: false, + overrideLock: false, + closeWatcher: vi.fn().mockResolvedValue(undefined) + }) + ).resolves.toBeUndefined() + expect(removeLocalWorktreePathMock).not.toHaveBeenCalled() + }) + }) + 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'), { @@ -97,7 +184,7 @@ describe('recoverLocalWindowsLongPathWorktreeRemoval', () => { }) await expect( - recoverLocalWindowsLongPathWorktreeRemoval({ + recoverLocalWindowsWorktreeRemoval({ error, force: true, canonicalWorktreePath: 'C:/repo/worktree/delete-e2e-held-cwd', @@ -122,7 +209,7 @@ describe('recoverLocalWindowsLongPathWorktreeRemoval', () => { lockReason: 'active agent' } - await recoverLocalWindowsLongPathWorktreeRemoval({ + await recoverLocalWindowsWorktreeRemoval({ error: Object.assign(new Error('git worktree remove failed'), { stderr: 'error: failed to delete deep/file.txt: Filename too long' }), diff --git a/src/main/local-worktree-removal-recovery.ts b/src/main/local-worktree-removal-recovery.ts index 49e4aa824a7..f4d67ec96e8 100644 --- a/src/main/local-worktree-removal-recovery.ts +++ b/src/main/local-worktree-removal-recovery.ts @@ -9,7 +9,7 @@ import { gitExecFileAsync } from './git/runner' import { listWorktreesStrict, removeWorktree, type GitWorktreeExecOptions } from './git/worktree' import { removeLocalWorktreePath } from './local-worktree-filesystem' -type LocalWindowsLongPathRecoveryArgs = { +type LocalWindowsRemovalRecoveryArgs = { error: unknown force: boolean canonicalWorktreePath: string @@ -22,7 +22,7 @@ type LocalWindowsLongPathRecoveryArgs = { } type StaleLocalWorktreeRegistrationArgs = Omit< - LocalWindowsLongPathRecoveryArgs, + LocalWindowsRemovalRecoveryArgs, 'error' | 'force' | 'closeWatcher' > @@ -41,12 +41,16 @@ function preservedBranchResult( } } -function staleRegistrationRecoveryError(error: unknown, canonicalWorktreePath: string): Error { +function staleRegistrationRecoveryError( + error: unknown, + canonicalWorktreePath: string, + force: boolean +): Error { return new Error( `${formatWorktreeRemovalError( error, canonicalWorktreePath, - true + force )} The worktree directory was removed, but Git still has stale worktree registration. Retry deletion after resolving the Git registration error.` ) } @@ -54,7 +58,8 @@ function staleRegistrationRecoveryError(error: unknown, canonicalWorktreePath: s async function verifyGitWorktreeRegistrationRemoved( repoPath: string, localWorktreeGitOptions: GitWorktreeExecOptions, - canonicalWorktreePath: string + canonicalWorktreePath: string, + force: boolean ): Promise { try { const remainingWorktrees = await listWorktreesStrict(repoPath, localWorktreeGitOptions) @@ -66,12 +71,13 @@ async function verifyGitWorktreeRegistrationRemoved( throw new Error('Git still reports the worktree registration after cleanup.') } } catch (error) { - throw staleRegistrationRecoveryError(error, canonicalWorktreePath) + throw staleRegistrationRecoveryError(error, canonicalWorktreePath, force) } } async function removeRequiredGitWorktreeRegistration( - args: StaleLocalWorktreeRegistrationArgs + args: StaleLocalWorktreeRegistrationArgs, + forceForError = true ): Promise { assertWorktreeUnlockedForRemoval(args.registeredWorktree, args.overrideLock) @@ -104,11 +110,12 @@ async function removeRequiredGitWorktreeRegistration( await verifyGitWorktreeRegistrationRemoved( args.repoPath, args.localWorktreeGitOptions, - args.canonicalWorktreePath + args.canonicalWorktreePath, + forceForError ) } catch (verificationError) { throw removalError - ? staleRegistrationRecoveryError(removalError, args.canonicalWorktreePath) + ? staleRegistrationRecoveryError(removalError, args.canonicalWorktreePath, forceForError) : verificationError } // Why: if Git detached the row before reporting its filesystem error, keep @@ -116,38 +123,59 @@ async function removeRequiredGitWorktreeRegistration( return result ?? preservedBranchResult(args.registeredWorktree, args.deleteBranch) } -export async function recoverLocalWindowsLongPathWorktreeRemoval( - args: LocalWindowsLongPathRecoveryArgs +export async function recoverLocalWindowsWorktreeRemoval( + args: LocalWindowsRemovalRecoveryArgs ): Promise { - if (!args.force || !isRecoverableWindowsFilesystemRemovalError(args.error)) { + if (!(await isRecoverableWindowsFilesystemRemovalFailure(args))) { return undefined } - // Why: watcher shutdown is best-effort, but Git registration must be removed - // before callers clear Orca metadata or the branch remains locked. + // Why: this error means Git accepted removal and started deleting the path; + // finish that partial success with Windows retries, then verify Git metadata. await args.closeWatcher(args.canonicalWorktreePath).catch(() => {}) try { await removeLocalWorktreePath(args.canonicalWorktreePath, args.localWorktreeGitOptions) } catch (error) { - throw new Error(formatWorktreeRemovalError(error, args.canonicalWorktreePath, true)) + throw new Error(formatWorktreeRemovalError(error, args.canonicalWorktreePath, args.force)) } - return removeRequiredGitWorktreeRegistration(args) + return removeRequiredGitWorktreeRegistration(args, args.force) } -function isRecoverableWindowsFilesystemRemovalError(error: unknown): boolean { - if (isWindowsLongPathWorktreeRemovalError(error)) { +async function isRecoverableWindowsFilesystemRemovalFailure( + args: LocalWindowsRemovalRecoveryArgs +): Promise { + if (isWindowsLongPathWorktreeRemovalError(args.error)) { return true } - if (process.platform !== 'win32' || typeof error !== 'object' || error === null) { + if (process.platform !== 'win32' || typeof args.error !== 'object' || args.error === null) { return false } - const errorWithDetails = error as { message?: unknown; stderr?: unknown; stdout?: unknown } + const errorWithDetails = args.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 - ) + if ( + /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 + ) + ) { + return true + } + + try { + const worktrees = await listWorktreesStrict(args.repoPath, args.localWorktreeGitOptions) + // Why: Git can localize ENOTEMPTY prose, but a missing registration proves + // it already accepted removal and only Windows filesystem cleanup remains. + return !worktrees.some((worktree) => + areWorktreePathsEqual(worktree.path, args.canonicalWorktreePath) + ) + } catch { + return false + } } export async function removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval( diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 1bcbcec3077..b6447984744 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -450,7 +450,7 @@ import { } from '../local-worktree-filesystem' import { removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval, - recoverLocalWindowsLongPathWorktreeRemoval + recoverLocalWindowsWorktreeRemoval } from '../local-worktree-removal-recovery' import { connect as connectLinear, @@ -15893,9 +15893,9 @@ export class OrcaRuntimeService { refreshedRegisteredWorktree.head ) } catch (error) { - // Why: Git for Windows can fail long-path directory deletion after - // Orca has already validated the target and explicit force delete. - const recoveredRemovalResult = await recoverLocalWindowsLongPathWorktreeRemoval({ + // Why: Git for Windows can deregister a clean worktree before its + // recursive filesystem deletion fails transiently. + const recoveredRemovalResult = await recoverLocalWindowsWorktreeRemoval({ error, force, canonicalWorktreePath,