From c2ceb5204f4e675ed6659118f4855fa2f17ebeb3 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Mon, 26 Oct 2026 13:50:11 -0700 Subject: [PATCH] fix(windows): stop recursive deletes from retry-storming when a file is held open (#33722) * fix(windows): stop recursive deletes from retry-storming when a file is held open Node's async recursive rm retries every nested entry separately and orphans sibling walkers when it rejects, so deleting a workspace holding a live browser profile multiplied filesystem work per directory level until the main process hung. Delete Windows host trees with a walker that retries whole passes from one shared budget and leaves nothing running on failure. Also bound and log detached worktree removal, and fail a file transaction whose proper-lockfile lock is compromised instead of throwing from its timer. * fix(windows): retry a locked delete as whole passes instead of a custom walker Replace the hand-rolled tree walker with Node's own recursive rm and no per-entry retries. Async rm gives each nested entry its own retry counter and never stops siblings, which is what multiplied the work; with retries off each pass is one bounded walk, and a transient Windows lock repeats the whole pass from the existing 250/500/1000/2000 ms budget, checking the job's signal between passes. A failed delete now names the file another program holds open instead of Node's EBUSY/EPERM text. Drop the 30-minute background deadline. The lost-lock error no longer embeds the library's message and says the write may or may not have been saved. * test: cover bounded deletion failure and recovery contracts * test: prune unchanged cleanup coverage * fix(worktrees): give truthful failed-delete cleanup advice * test: document fake-clock retry adapter --------- Co-authored-by: Orca Worker Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> --- .../browser/browser-client-upload-staging.ts | 7 +- .../codex/codex-hook-trust-derivation.test.ts | 7 ++ src/main/codex/codex-hook-trust-derivation.ts | 9 +-- src/main/file-transaction-lock.test.ts | 53 +++++++++++++++ src/main/file-transaction-lock.ts | 21 +++++- src/main/git/worktree-removal.ts | 2 +- src/main/host-tree-removal.ts | 64 ++++++++----------- .../remove-registered-local-worktree.ts | 3 +- src/main/local-worktree-filesystem.test.ts | 57 ++++++++--------- src/main/local-worktree-filesystem.ts | 5 +- .../local-worktree-removal-recovery.test.ts | 61 +++++++++++++++--- src/main/local-worktree-removal-recovery.ts | 9 ++- .../orcad-windows-host-state-ops.test.ts | 43 +++++++++++++ .../orcad-windows-host-state-ops.ts | 10 ++- ...ntime-registered-local-worktree-removal.ts | 3 +- .../clipboard-remote-file-staging.test.ts | 37 ++++++----- src/main/window/owned-temp-staging-root.ts | 11 +--- src/main/worktree-failed-removal.test.ts | 37 ++++++++++- src/main/worktree-removal-failure.ts | 6 +- .../worktrees-background-removal.test.ts | 10 ++- src/shared/windows-path-access.ts | 2 +- src/shared/windows-transient-lock-removal.ts | 31 ++++++++- 22 files changed, 364 insertions(+), 124 deletions(-) create mode 100644 src/main/file-transaction-lock.test.ts diff --git a/src/main/browser/browser-client-upload-staging.ts b/src/main/browser/browser-client-upload-staging.ts index 9fd6441f3c8..360de392794 100644 --- a/src/main/browser/browser-client-upload-staging.ts +++ b/src/main/browser/browser-client-upload-staging.ts @@ -1,6 +1,6 @@ import { randomUUID } from 'node:crypto' import { rmSync } from 'node:fs' -import { mkdir, rm, writeFile } from 'node:fs/promises' +import { mkdir, writeFile } from 'node:fs/promises' import path from 'node:path' import { @@ -8,6 +8,7 @@ import { BROWSER_CLIENT_FILE_CHANNEL_TRANSFER_MAX_BYTES } from '../../shared/browser-client-file-channel-protocol' import { normalizeBrowserDownloadFilename } from '../../shared/browser-download-filename' +import { removeTree } from '../../shared/windows-transient-lock-removal' export type BrowserClientStagedUpload = { stagingId: string @@ -34,7 +35,7 @@ type StagingFilesystem = { removeDirectorySync(directory: string): void } -// Windows file handles held by Chromium or antivirus can transiently block removal; retry. +// Chromium or antivirus handles can transiently block removal; rmSync's native retries are top-level only. const stagingRemovalOptions = { recursive: true, force: true, @@ -50,7 +51,7 @@ const nodeStagingFilesystem: StagingFilesystem = { await writeFile(filePath, contents, { mode: 0o600 }) }, removeDirectory: async (directory) => { - await rm(directory, stagingRemovalOptions) + await removeTree(directory) }, removeDirectorySync: (directory) => { rmSync(directory, stagingRemovalOptions) diff --git a/src/main/codex/codex-hook-trust-derivation.test.ts b/src/main/codex/codex-hook-trust-derivation.test.ts index 5e3dfb0a6e5..c0223a1b963 100644 --- a/src/main/codex/codex-hook-trust-derivation.test.ts +++ b/src/main/codex/codex-hook-trust-derivation.test.ts @@ -1,5 +1,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, utimesSync } from 'node:fs' +import { rm } from 'node:fs/promises' +import type * as NodeFsPromises from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import type * as AppServerSession from './codex-app-server-session' @@ -9,6 +11,10 @@ const mocks = vi.hoisted(() => ({ runProcess: vi.fn() })) +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, rm: vi.fn(actual.rm) } +}) vi.mock('./codex-app-server-session', async (importOriginal) => ({ ...(await importOriginal()), runCodexAppServerSession: mocks.runCodexAppServerSession @@ -264,6 +270,7 @@ describe('deriveCodexHookHashes', () => { expect(await probeCodexVersion('/bin/codex')).toBe('codex-cli 0.150.1') expect(existsSync(versionHome)).toBe(false) + expect(rm).toHaveBeenLastCalledWith(versionHome, { recursive: true, force: true }) }) }) diff --git a/src/main/codex/codex-hook-trust-derivation.ts b/src/main/codex/codex-hook-trust-derivation.ts index e2c46173b52..a8355f21049 100644 --- a/src/main/codex/codex-hook-trust-derivation.ts +++ b/src/main/codex/codex-hook-trust-derivation.ts @@ -1,8 +1,9 @@ import { realpathSync, statSync } from 'node:fs' -import { mkdir, mkdtemp, readdir, realpath, rm, stat, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, readdir, realpath, stat, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { runProcess } from '@orca/process-host' +import { removeTree } from '../../shared/windows-transient-lock-removal' import { withCliRuntimeOnPath } from '../codex-cli/command' import { CODEX_SHORT_LIVED_PROBE_APP_SERVER_ARGS } from '../codex-cli/codex-read-only-app-server-args' import { collectListedHooks, type CodexListedHook } from './codex-app-server-client' @@ -149,7 +150,7 @@ async function listScratchHomeHooks( projectHooksPath: join(project, '.codex', 'hooks.json') } } finally { - await rm(root, { recursive: true, force: true, maxRetries: 3 }).catch(() => {}) + await removeTree(root).catch(() => {}) } } @@ -240,7 +241,7 @@ export async function probeCodexVersion(codexCommand: string): Promise {}) + await removeTree(scratchHome).catch(() => {}) } } @@ -251,7 +252,7 @@ export async function sweepStaleCodexScratchHomes(): Promise { const path = join(root, name) const info = await stat(path).catch(() => null) if (info && Date.now() - info.mtimeMs > STALE_SCRATCH_MS) { - await rm(path, { recursive: true, force: true, maxRetries: 3 }).catch(() => {}) + await removeTree(path).catch(() => {}) } } } diff --git a/src/main/file-transaction-lock.test.ts b/src/main/file-transaction-lock.test.ts new file mode 100644 index 00000000000..b5dffd6c26b --- /dev/null +++ b/src/main/file-transaction-lock.test.ts @@ -0,0 +1,53 @@ +import { mkdtemp, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import type * as ProperLockfile from 'proper-lockfile' +import { afterEach, expect, it, vi } from 'vitest' +import { withFileTransactionLock } from './file-transaction-lock' + +const { compromisedSeen } = vi.hoisted(() => ({ compromisedSeen: { notify: () => {} } })) + +// Refresh at proper-lockfile's minimum interval and report compromise to the test. +vi.mock('proper-lockfile', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + lock: (file: string, options: ProperLockfile.LockOptions) => + actual.lock(file, { + ...options, + stale: 2_000, + update: 1_000, + onCompromised: (error) => { + compromisedSeen.notify() + if (!options.onCompromised) { + throw error // proper-lockfile's default + } + options.onCompromised(error) + } + }) + } +}) + +const roots: string[] = [] + +afterEach(async () => { + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +it('fails the transaction instead of throwing from the refresh timer when the lock is lost', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-file-transaction-lock-')) + roots.push(root) + const filePath = join(root, 'agent-session-recovery.json') + + const transaction = withFileTransactionLock(filePath, async () => { + const lost = new Promise((resolve) => { + compromisedSeen.notify = resolve + }) + // Another instance treating the lock as stale removes it; the next mtime refresh then fails. + await rm(`${filePath}.lock`, { recursive: true, force: true }) + await lost + return 'written' + }) + + await expect(transaction).rejects.toThrow(/Lost the lock on .*agent-session-recovery\.json/) +}) diff --git a/src/main/file-transaction-lock.ts b/src/main/file-transaction-lock.ts index 8e7c78d32b2..75178bc03b8 100644 --- a/src/main/file-transaction-lock.ts +++ b/src/main/file-transaction-lock.ts @@ -19,13 +19,28 @@ export async function withFileTransactionLock( const directory = dirname(filePath) await mkdir(directory, { recursive: true, mode: 0o700 }) await chmod(directory, 0o700) + let compromised: Error | undefined const release = await lock(filePath, { realpath: false, - retries: options.retries ?? LOCK_RETRIES + retries: options.retries ?? LOCK_RETRIES, + // The default rethrows from proper-lockfile's refresh timer, an uncaught main-process exception. + onCompromised: (error) => { + compromised = error + } }) try { - return await apply() + const result = await apply() + if (compromised) { + throw new Error( + `Lost the lock on ${filePath} during the transaction; this change may or may not be saved, and another process may have changed the file.`, + { cause: compromised } + ) + } + return result } finally { - await release() + // A compromised lock is already released and may belong to another process now. + if (!compromised) { + await release() + } } } diff --git a/src/main/git/worktree-removal.ts b/src/main/git/worktree-removal.ts index 5e952128ff6..18a383bdd24 100644 --- a/src/main/git/worktree-removal.ts +++ b/src/main/git/worktree-removal.ts @@ -191,5 +191,5 @@ async function removeCheckoutLeftByGit( throw error } console.warn(`[git] worktree remove left files at "${worktreePath}"; deleting them`) - await removeHostTree(worktreePath) + await removeHostTree(worktreePath, { signal: options.checkoutDeleteSignal }) } diff --git a/src/main/host-tree-removal.ts b/src/main/host-tree-removal.ts index 01d30edf8ce..4031254cbef 100644 --- a/src/main/host-tree-removal.ts +++ b/src/main/host-tree-removal.ts @@ -1,13 +1,11 @@ -// Host deletion needs Windows lock retries and unpatched fs for Electron’s .asar handling. +// Host deletion needs bounded Windows lock retries and unpatched fs for Electron’s .asar handling. import { win32 } from 'node:path' -import { setTimeout as delay } from 'node:timers/promises' import { rm } from './asar-transparent-fs' import { isWindowsAbsolutePathLike } from '../shared/cross-platform-path' import { isWslUncPath } from '../shared/wsl-paths' -import { transientLockRemovalOptions } from '../shared/windows-transient-lock-removal' - -const WINDOWS_REMOVE_RETRY_DELAYS_MS = [250, 500, 1_000, 2_000] +import { stripWin32LongPathPrefix } from '../shared/windows-path-access' +import { removeTree } from '../shared/windows-transient-lock-removal' /** Convert a native host filesystem path to the Win32 long-path namespace. */ export function toHostFilesystemPath(targetPath: string): string { @@ -24,37 +22,29 @@ export function toHostRemovalPath(targetPath: string): string { return toHostFilesystemPath(targetPath) } -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) -} - -/** Recursively remove a host directory tree, retrying the transient Windows failures. */ -export async function removeHostTree(targetPath: string): Promise { - const removalPath = toHostRemovalPath(targetPath) - const retryDelays = process.platform === 'win32' ? WINDOWS_REMOVE_RETRY_DELAYS_MS : [] - // Node’s retries absorb transient directory locks before the outer retry loop. - const rmOptions = transientLockRemovalOptions() - let attempt = 0 - - while (true) { - try { - await rm(removalPath, rmOptions) - return - } catch (error) { - if (attempt >= retryDelays.length || !isTransientWindowsRemovalError(error)) { - throw error - } - // Antivirus, indexers, and late handle release can keep just-emptied Windows directories locked. - await delay(retryDelays[attempt]) - attempt += 1 - } +/** Recursively remove a host directory tree, retrying transient Windows locks as whole passes. */ +export async function removeHostTree( + targetPath: string, + options: { signal?: AbortSignal } = {} +): Promise { + try { + await removeTree(toHostRemovalPath(targetPath), { rm, signal: options.signal }) + } catch (error) { + throw describeRemovalFailure(error) } } + +// Failed-delete rows show this message; name the blocking file instead of Node's error text. +function describeRemovalFailure(error: unknown): unknown { + const code = error instanceof Error && 'code' in error ? error.code : undefined + const path = error instanceof Error && 'path' in error ? error.path : undefined + if (path === undefined || (code !== 'EBUSY' && code !== 'EPERM' && code !== 'EACCES')) { + return error + } + const file = stripWin32LongPathPrefix(String(path)) + const message = + code === 'EBUSY' + ? `${file} is open in another program. Close that program and delete again.` + : `${file} could not be deleted; it may be read-only or open in another program. Close any program using it and delete again.` + return Object.assign(new Error(message, { cause: error }), { code, path: file }) +} diff --git a/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts b/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts index 3d75567fd1b..cac2641ee74 100644 --- a/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts +++ b/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts @@ -242,7 +242,8 @@ async function finishLocalWorktreeRemoval({ localWorktreeGitOptions, registeredWorktree: refreshedRegisteredWorktree, deleteBranch, - closeWatcher: (worktreePath) => runtime.closeFileWatchersForRemoval(worktreePath) + closeWatcher: (worktreePath) => runtime.closeFileWatchersForRemoval(worktreePath), + signal: checkoutDeleteSignal }) if (recoveredRemovalResult) { removalResult = recoveredRemovalResult diff --git a/src/main/local-worktree-filesystem.test.ts b/src/main/local-worktree-filesystem.test.ts index e235ad4fdea..1ff40664bdc 100644 --- a/src/main/local-worktree-filesystem.test.ts +++ b/src/main/local-worktree-filesystem.test.ts @@ -92,15 +92,7 @@ describe('local worktree filesystem runtime access', () => { await removeLocalWorktreePath(longPath) expect(toHostRemovalPath(longPath)).toBe(`\\\\?\\${longPath}`) - expect(rmMock).toHaveBeenCalledWith( - `\\\\?\\${longPath}`, - expect.objectContaining({ - recursive: true, - force: true, - maxRetries: expect.any(Number), - retryDelay: expect.any(Number) - }) - ) + expect(rmMock).toHaveBeenCalledWith(`\\\\?\\${longPath}`, { recursive: true, force: true }) }) }) @@ -125,7 +117,7 @@ describe('local worktree filesystem runtime access', () => { }) }) - it('retries transient host removal failures on Windows', async () => { + it('retries a transient Windows lock as a whole pass, never per entry', async () => { vi.useFakeTimers() await withPlatform('win32', async () => { const error = Object.assign(new Error('Directory not empty'), { code: 'ENOTEMPTY' }) @@ -135,27 +127,34 @@ describe('local worktree filesystem runtime access', () => { 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) + // Async rm's maxRetries multiplies per directory level when a file stays open. + expect(rmMock.mock.calls).toEqual([ + [toHostRemovalPath('C:\\repo\\feature'), { recursive: true, force: true }], + [toHostRemovalPath('C:\\repo\\feature'), { recursive: true, force: true }] + ]) + }) + }) + + it('names the file another program holds open once the retries run out', async () => { + vi.useFakeTimers() + await withPlatform('win32', async () => { + const held = toHostRemovalPath('C:\\repo\\feature\\profile\\lockfile') + rmMock.mockRejectedValue( + Object.assign(new Error(`EBUSY: resource busy or locked, unlink '${held}'`), { + code: 'EBUSY', + path: held }) ) + + const removal = removeLocalWorktreePath('C:\\repo\\feature').catch((error: unknown) => error) + await vi.advanceTimersByTimeAsync(3_750) + + expect(await removal).toMatchObject({ + code: 'EBUSY', + message: + 'C:\\repo\\feature\\profile\\lockfile is open in another program. Close that program and delete again.' + }) + expect(rmMock).toHaveBeenCalledTimes(5) }) }) diff --git a/src/main/local-worktree-filesystem.ts b/src/main/local-worktree-filesystem.ts index 4f2544b3b71..99f86ff9f91 100644 --- a/src/main/local-worktree-filesystem.ts +++ b/src/main/local-worktree-filesystem.ts @@ -108,11 +108,12 @@ export function getLocalWorktreePathAccess( export async function removeLocalWorktreePath( targetPath: string, - options: LocalWorktreeFilesystemOptions = {} + options: LocalWorktreeFilesystemOptions = {}, + signal?: AbortSignal ): Promise { const distro = options.wslDistro?.trim() if (!shouldUseWslFilesystem(options) || !distro) { - await removeHostTree(targetPath) + await removeHostTree(targetPath, { signal }) return } diff --git a/src/main/local-worktree-removal-recovery.test.ts b/src/main/local-worktree-removal-recovery.test.ts index 7d5aa442a1e..0ec83a62af8 100644 --- a/src/main/local-worktree-removal-recovery.test.ts +++ b/src/main/local-worktree-removal-recovery.test.ts @@ -1,12 +1,13 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as LocalWorktreeFilesystem from './local-worktree-filesystem' -const { gitExecFileAsyncMock, listWorktreesStrictMock, removeLocalWorktreePathMock } = vi.hoisted( - () => ({ +const { gitExecFileAsyncMock, listWorktreesStrictMock, removeLocalWorktreePathMock, hostRmMock } = + vi.hoisted(() => ({ gitExecFileAsyncMock: vi.fn(), listWorktreesStrictMock: vi.fn(), - removeLocalWorktreePathMock: vi.fn() - }) -) + removeLocalWorktreePathMock: vi.fn(), + hostRmMock: vi.fn() + })) vi.mock('./git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock @@ -16,6 +17,8 @@ vi.mock('./local-worktree-filesystem', () => ({ removeLocalWorktreePath: removeLocalWorktreePathMock })) +vi.mock('./asar-transparent-fs', () => ({ rm: hostRmMock })) + vi.mock('./git/worktree', () => ({ listWorktreesStrict: listWorktreesStrictMock })) @@ -68,7 +71,8 @@ describe('recoverLocalWindowsWorktreeRemoval', () => { expect(removeLocalWorktreePathMock).toHaveBeenCalledWith( 'C:/repo/worktree/delete-e2e-held-cwd', - {} + {}, + undefined ) expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'prune'], { cwd: 'C:/repo' @@ -102,7 +106,8 @@ describe('recoverLocalWindowsWorktreeRemoval', () => { expect(removeLocalWorktreePathMock).toHaveBeenCalledWith( 'C:/repo/worktree/delete-e2e-held-cwd', - {} + {}, + undefined ) expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'prune'], { cwd: 'C:/repo' @@ -116,6 +121,42 @@ describe('recoverLocalWindowsWorktreeRemoval', () => { }) }) + it('rejects native recovery without pruning or another pass when deletion is cancelled', async () => { + const native = await vi.importActual( + './local-worktree-filesystem' + ) + removeLocalWorktreePathMock.mockImplementationOnce(native.removeLocalWorktreePath) + const controller = new AbortController() + const closeWatcher = vi.fn().mockResolvedValue(undefined) + hostRmMock.mockImplementationOnce(async () => { + controller.abort() + throw Object.assign(new Error('file held open'), { code: 'EBUSY' }) + }) + await withPlatform('win32', async () => { + await expect( + recoverLocalWindowsWorktreeRemoval({ + error: new Error('Git deregistered but left the checkout'), + force: true, + canonicalWorktreePath: 'C:/repo/worktree/feature', + repoPath: 'C:/repo', + localWorktreeGitOptions: {}, + registeredWorktree: { branch: 'refs/heads/feature', head: 'abc123' }, + deleteBranch: false, + closeWatcher, + signal: controller.signal + }) + ).rejects.toThrow(/aborted/i) + expect(removeLocalWorktreePathMock).toHaveBeenCalledWith( + 'C:/repo/worktree/feature', + {}, + controller.signal + ) + expect(closeWatcher).toHaveBeenCalledOnce() + expect(hostRmMock).toHaveBeenCalledOnce() + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + }) + it('does not recursively delete while watcher teardown remains unconfirmed', async () => { await withPlatform('win32', async () => { const watcherError = new Error('file watcher process did not exit after termination deadline') @@ -153,7 +194,11 @@ describe('recoverLocalWindowsWorktreeRemoval', () => { closeWatcher: vi.fn().mockResolvedValue(undefined) }) - expect(removeLocalWorktreePathMock).toHaveBeenCalledWith('C:/repo/worktree/feature', {}) + expect(removeLocalWorktreePathMock).toHaveBeenCalledWith( + 'C:/repo/worktree/feature', + {}, + undefined + ) expect(result).toEqual({}) }) }) diff --git a/src/main/local-worktree-removal-recovery.ts b/src/main/local-worktree-removal-recovery.ts index 6483c3eede6..4fc6de09b39 100644 --- a/src/main/local-worktree-removal-recovery.ts +++ b/src/main/local-worktree-removal-recovery.ts @@ -16,11 +16,12 @@ type LocalWindowsRemovalRecoveryArgs = { registeredWorktree: Pick deleteBranch: boolean closeWatcher: (worktreePath: string) => Promise + signal?: AbortSignal } type StaleLocalWorktreeRegistrationArgs = Omit< LocalWindowsRemovalRecoveryArgs, - 'error' | 'force' | 'closeWatcher' + 'error' | 'force' | 'closeWatcher' | 'signal' > function preservedBranchResult( @@ -117,7 +118,11 @@ export async function recoverLocalWindowsWorktreeRemoval( // Release watcher handles before recursively finishing Git’s partial removal; abort if closing fails. await args.closeWatcher(args.canonicalWorktreePath) try { - await removeLocalWorktreePath(args.canonicalWorktreePath, args.localWorktreeGitOptions) + await removeLocalWorktreePath( + args.canonicalWorktreePath, + args.localWorktreeGitOptions, + args.signal + ) } catch (error) { throw new Error(formatWorktreeRemovalError(error, args.canonicalWorktreePath, args.force)) } diff --git a/src/main/managed-server/host-state/orcad-windows-host-state-ops.test.ts b/src/main/managed-server/host-state/orcad-windows-host-state-ops.test.ts index 6ea9a8868a9..6bb5cbead30 100644 --- a/src/main/managed-server/host-state/orcad-windows-host-state-ops.test.ts +++ b/src/main/managed-server/host-state/orcad-windows-host-state-ops.test.ts @@ -92,6 +92,49 @@ describe('Windows snapshot ops', () => { expect(existsSync(join(root, 'daemon', 'daemon.sock.token'))).toBe(true) }) + it.each(['snapshot-clear', 'snapshot-restore'] as const)( + '%s bounds permanent/transient locks and leaves a recoverable snapshot', + async (name) => { + expect(await op('snapshot-capture', root, snapshot)).toBe('CAPTURED') + const preload = join(dir, 'fail-rm.cjs') + const attempts = join(dir, 'rm-attempts.json') + const target = join(root, 'profiles') + for (const failures of [6, 1]) { + writeFileSync( + preload, + `const fs = require('fs'); const rm = fs.promises.rm; const calls = []; +fs.promises.rm = async (p, options) => { + if (p === ${JSON.stringify(target)}) { + calls.push({ options, lockHeld: fs.existsSync(${JSON.stringify(join(dir, 'orcad-state-mutation.lock', 'owner.json'))}) }); + fs.writeFileSync(${JSON.stringify(attempts)}, JSON.stringify(calls)); + if (calls.length <= ${failures}) throw Object.assign(new Error('held open'), { code: 'EPERM' }); + } + return rm(p, options); +};` + ) + const result = await runProcess({ + program: process.execPath, + args: ['--require', preload, script, name, root, snapshot] + }) + expect(result.code, result.stderr).toBe(0) + expect(result.stdout.trim()).toBe(failures === 6 ? 'FAILED' : 'RESTORED') + expect(JSON.parse(readFileSync(attempts, 'utf8'))).toEqual( + Array.from({ length: failures === 6 ? 6 : 2 }, () => ({ + options: { recursive: true, force: true }, + lockHeld: true + })) + ) + expect(existsSync(join(dir, 'orcad-state-mutation.lock'))).toBe(false) + expect(readFileSync(join(root, 'daemon', 'daemon.sock.token'), 'utf8')).toBe( + 'live-daemon-token' + ) + expect(await op('snapshot-probe', snapshot)).toBe('PRESENT') + expect(await op('snapshot-restore', root, snapshot)).toBe('RESTORED') + expect(await op('snapshot-compare', root, snapshot)).toBe('UNCHANGED') + } + } + ) + it('reports the newest member write in epoch seconds, or UNKNOWN', async () => { const at = new Date('2026-09-30T12:00:00.000Z') utimesSync(join(root, 'orca-profile-index.json'), at, at) diff --git a/src/main/managed-server/host-state/orcad-windows-host-state-ops.ts b/src/main/managed-server/host-state/orcad-windows-host-state-ops.ts index 318d8a00b99..805bc7e9b86 100644 --- a/src/main/managed-server/host-state/orcad-windows-host-state-ops.ts +++ b/src/main/managed-server/host-state/orcad-windows-host-state-ops.ts @@ -169,7 +169,15 @@ function renameWithRetry(from, to) { } const removeTree = (target) => fs.rmSync(target, { recursive: true, force: true, maxRetries: 5 }) -const removeTreeAsync = (target) => fs.promises.rm(target, { recursive: true, force: true, maxRetries: 5 }) +// Async rm retries each nested entry separately (multiplying per level); retry whole passes instead. +async function removeTreeAsync(target) { + for (const delay of [0, 100, 200, 300, 400, 500]) { + if (delay) await new Promise((resolve) => setTimeout(resolve, delay)) + try { await fs.promises.rm(target, { recursive: true, force: true }); return } catch (error) { + if (!['EPERM', 'EBUSY', 'ENOTEMPTY'].includes(error.code) || delay === 500) throw error + } + } +} Object.assign(ops, { 'snapshot-capture': withStateMutationLock(async (root, snapshotDir) => { diff --git a/src/main/runtime/runtime-registered-local-worktree-removal.ts b/src/main/runtime/runtime-registered-local-worktree-removal.ts index f6e73cb0f55..930a13cf00f 100644 --- a/src/main/runtime/runtime-registered-local-worktree-removal.ts +++ b/src/main/runtime/runtime-registered-local-worktree-removal.ts @@ -230,7 +230,8 @@ export async function finishRuntimeLocalWorktreeRemoval( localWorktreeGitOptions: localOptions, registeredWorktree: refreshed, deleteBranch: args.deleteBranch, - closeWatcher: args.closeWatchers + closeWatcher: args.closeWatchers, + signal: checkoutDeleteSignal }) if (recovered) { removalResult = recovered diff --git a/src/main/window/clipboard-remote-file-staging.test.ts b/src/main/window/clipboard-remote-file-staging.test.ts index 5ca20e68383..b4086e21aea 100644 --- a/src/main/window/clipboard-remote-file-staging.test.ts +++ b/src/main/window/clipboard-remote-file-staging.test.ts @@ -10,6 +10,11 @@ const { accessMock, lstatMock, mkdirMock, opendirMock, rmMock, writeFileMock } = writeFileMock: vi.fn() })) +vi.mock('node:timers/promises', () => ({ + // ast-grep-ignore: test-no-real-time-wait -- exposes retry backoff to vi.useFakeTimers for deterministic cleanup tests + setTimeout: (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) +})) + vi.mock('node:fs/promises', () => ({ access: accessMock, lstat: lstatMock, @@ -29,7 +34,6 @@ import { } from './clipboard-remote-file-staging' const TTL_MS = 60 * 60 * 1000 -const RETRY_MS = 60 * 1000 const NOW_MS = 1_760_000_000_000 const TEMP_ROOT = resolve('fixture-temp') const UID_SUFFIX = typeof process.getuid === 'function' ? `-${process.getuid()}` : '' @@ -126,9 +130,7 @@ describe('remote clipboard staging ownership', () => { expect(rmMock).toHaveBeenCalledOnce() expect(rmMock).toHaveBeenCalledWith(join(STAGING_ROOT, EXPIRED_TRANSFER), { force: true, - maxRetries: 3, - recursive: true, - retryDelay: 100 + recursive: true }) }) @@ -204,6 +206,7 @@ describe('remote clipboard staging ownership', () => { }) it('continues past Windows-style lock failures and retries on a later sweep', async () => { + vi.useFakeTimers() opendirMock.mockResolvedValue( openedDirectory([directoryEntry(EXPIRED_TRANSFER), directoryEntry(`1759990000000-${UUID_B}`)]) ) @@ -213,22 +216,28 @@ describe('remote clipboard staging ownership', () => { } }) - await expect(cleanupExpiredRemoteClipboardStaging(TEMP_ROOT, NOW_MS)).resolves.toBeUndefined() - - expect(rmMock).toHaveBeenCalledTimes(2) - rmMock.mockResolvedValue(undefined) + const sweep = cleanupExpiredRemoteClipboardStaging(TEMP_ROOT, NOW_MS) + await vi.runAllTimersAsync() + await expect(sweep).resolves.toBeUndefined() + const targets = () => new Set(rmMock.mock.calls.map(([target]) => target)) + expect(targets()).toEqual( + new Set([join(STAGING_ROOT, EXPIRED_TRANSFER), join(STAGING_ROOT, `1759990000000-${UUID_B}`)]) + ) + rmMock.mockClear().mockResolvedValue(undefined) await cleanupExpiredRemoteClipboardStaging(TEMP_ROOT, NOW_MS) - expect(rmMock).toHaveBeenCalledTimes(4) + expect(targets().size).toBe(2) }) it('bounds locked-file timer retries', async () => { vi.useFakeTimers() rmMock.mockRejectedValue(Object.assign(new Error('locked'), { code: 'EPERM' })) - scheduleRemoteClipboardTransferCleanup(TEMP_ROOT, join(STAGING_ROOT, EXPIRED_TRANSFER)) - await vi.advanceTimersByTimeAsync(TTL_MS + 4 * RETRY_MS) + const transferDirectory = join(STAGING_ROOT, EXPIRED_TRANSFER) + scheduleRemoteClipboardTransferCleanup(TEMP_ROOT, transferDirectory) + await vi.runAllTimersAsync() - expect(rmMock).toHaveBeenCalledTimes(4) + expect(lstatMock.mock.calls.filter(([target]) => target === transferDirectory)).toHaveLength(4) + expect(rmMock).toHaveBeenCalled() expect(vi.getTimerCount()).toBe(0) }) @@ -306,9 +315,7 @@ describe('legacy remote clipboard staging compatibility', () => { expect(rmMock).toHaveBeenCalledOnce() expect(rmMock).toHaveBeenCalledWith(join(TEMP_ROOT, LEGACY_EXPIRED), { force: true, - maxRetries: 3, - recursive: true, - retryDelay: 100 + recursive: true }) expect(writeFileMock).toHaveBeenCalledOnce() }) diff --git a/src/main/window/owned-temp-staging-root.ts b/src/main/window/owned-temp-staging-root.ts index 695950fb2c0..a2e25b245bf 100644 --- a/src/main/window/owned-temp-staging-root.ts +++ b/src/main/window/owned-temp-staging-root.ts @@ -1,14 +1,9 @@ import type { Dir, Stats } from 'node:fs' -import { lstat, mkdir, opendir, rm } from 'node:fs/promises' +import { lstat, mkdir, opendir } from 'node:fs/promises' import { dirname, join, resolve } from 'node:path' +import { removeTree } from '../../shared/windows-transient-lock-removal' const SWEEP_CONCURRENCY = 8 -const REMOVE_OPTIONS = { - recursive: true, - force: true, - maxRetries: 3, - retryDelay: 100 -} as const type CleanupResult = 'failed' | 'fresh' | 'ignored' | 'removed' @@ -104,7 +99,7 @@ export async function sweepExpiredOwnedDirectories( } export async function removeOwnedDirectory(directory: string): Promise { - await rm(directory, REMOVE_OPTIONS) + await removeTree(directory) } async function cleanupDirectory( diff --git a/src/main/worktree-failed-removal.test.ts b/src/main/worktree-failed-removal.test.ts index 87a3d996fc8..daceba3248f 100644 --- a/src/main/worktree-failed-removal.test.ts +++ b/src/main/worktree-failed-removal.test.ts @@ -8,6 +8,7 @@ import { listWorktreesStrict } from './git/worktree' import type * as WorktreeRemovalModule from './git/worktree-removal' import { deleteBranchOfUnregisteredWorktree } from './git/worktree-removal' import { beginTerminalInstall } from './ipc/watcher-removal-gate' +import { formatWorktreeRemovalError } from './ipc/worktree-logic' import { registerWorktreeChangeInvalidator } from './ipc/worktree-change-invalidators' import { _resetPendingWorktreeRemovalsForTests, @@ -48,7 +49,7 @@ const mainWorktree: GitWorktreeInfo = { beforeEach(async () => { directory = await realpath(await mkdtemp(join(tmpdir(), 'orca-failed-removal-'))) - checkout = join(directory, 'feature') + checkout = join(directory, 'feature-$&-$$-$`') worktreeId = `repo-1::${checkout}` // What Git left: part of the checkout, `.git` already deleted. await mkdir(join(checkout, 'node_modules', 'a'), { recursive: true }) @@ -67,7 +68,7 @@ afterEach(async () => { }) function startFailingRemoval( - job: Pick = {} + job: Partial> = {} ): Promise { return startBackgroundWorktreeRemoval({ removal: { @@ -125,6 +126,38 @@ describe('a delete that fails after Git dropped the registration', () => { beginTerminalInstall(checkout)() }) + it.each([ + ['is open in another program', 'Close that program'], + [ + 'could not be deleted; it may be read-only or open in another program', + 'Close any program using it' + ] + ])('stores truthful leftover guidance when a file %s', async (reason, advice) => { + const heldFile = join(checkout, 'node_modules', 'a', 'LICENSE') + const initial = formatWorktreeRemovalError( + new Error(`${heldFile} ${reason}. ${advice} and delete again.`), + checkout, + true + ) + const message = `Failed to force delete worktree at ${checkout}. ${heldFile} ${reason}. Close any program using it. You may need to delete the remaining folder at ${checkout} manually to finish removing this workspace.` + await expect( + startFailingRemoval({ + run: async () => { + throw new Error(initial) + } + }) + ).rejects.toThrow(initial) + await _settlePendingWorktreeRemovalsForTests() + + expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toMatchObject([ + { worktreeId, failure: { message } } + ]) + expect(await listRows()).toEqual([mainWorktree, { ...leftoverRow(), removalError: message }]) + _resetPendingWorktreeRemovalsForTests() + await loadWorktreeRemovalRecords(join(directory, 'profile')) + expect(await listRows()).toEqual([mainWorktree, { ...leftoverRow(), removalError: message }]) + }) + it('invalidates cached listings, which still hold the registration Git dropped', async () => { const invalidated = vi.fn() const unregister = registerWorktreeChangeInvalidator(invalidated) diff --git a/src/main/worktree-removal-failure.ts b/src/main/worktree-removal-failure.ts index 1ab285a01eb..39a36cd2c3d 100644 --- a/src/main/worktree-removal-failure.ts +++ b/src/main/worktree-removal-failure.ts @@ -13,7 +13,11 @@ export async function failureLeftByGit( return undefined } const failure = { - message: error instanceof Error ? error.message : String(error), + message: (error instanceof Error ? error.message : String(error)).replace( + / Close (?:that program|any program using it) and delete again\.$/, + () => + ` Close any program using it. You may need to delete the remaining folder at ${record.worktreePath} manually to finish removing this workspace.` + ), failedAt: Date.now() } await finishGitSideOfFailedRemoval(record, cleanupPushTargetRemote) diff --git a/src/renderer/src/store/slices/worktrees-background-removal.test.ts b/src/renderer/src/store/slices/worktrees-background-removal.test.ts index 6bff5d80837..329ee65a6e8 100644 --- a/src/renderer/src/store/slices/worktrees-background-removal.test.ts +++ b/src/renderer/src/store/slices/worktrees-background-removal.test.ts @@ -9,6 +9,7 @@ import { reconcileHostWorktreeRemovals } from '../../hooks/ipc-events/background-worktree-removal-bridge' import { getWorktreeHostIdentity } from '../../../../shared/worktree/host-qualified-identity' +import { getWorktreeDeleteErrorToShow } from '../../components/sidebar/worktree-delete-error-display' vi.mock('sonner', () => ({ toast: { info: vi.fn(), success: vi.fn(), error: vi.fn(), warning: vi.fn() } @@ -206,13 +207,18 @@ describe('removing a worktree the host deletes in the background', () => { it('shows the host error on a row the host lists as a failed delete, until it leaves', () => { // A window that opened after the delete failed, or a restart after a failed startup finish. - seedRow(store, { removalError: 'Operation not permitted' }) + const message = + '/path/wt1/profile/lockfile is open in another program. Close any program using it. You may need to delete the remaining folder at /path/wt1 manually to finish removing this workspace.' + seedRow(store, { removalError: message }) reconcileHostWorktreeRemovals(store) expect(deleteState(store)).toMatchObject({ isDeleting: false, - error: 'Operation not permitted', + error: message, canForceDelete: false }) + expect(getWorktreeDeleteErrorToShow({ removalError: message }, deleteState(store))).toBe( + message + ) // Forgotten, or the checkout deleted outside Orca: the host stops listing it. seedStore(store, { worktreesByRepo: { repo1: [] } }) diff --git a/src/shared/windows-path-access.ts b/src/shared/windows-path-access.ts index 9c9f9f430b5..ffb44aedbe9 100644 --- a/src/shared/windows-path-access.ts +++ b/src/shared/windows-path-access.ts @@ -70,7 +70,7 @@ export function classifyRejectedWindowsPath(windowsPath: string): WindowsPathRej return parsed ? { kind: 'other-distro', distro: parsed.distro } : { kind: 'not-wsl' } } -function stripWin32LongPathPrefix(path: string): string { +export function stripWin32LongPathPrefix(path: string): string { if (/^\\\\\?\\UNC\\/i.test(path)) { return `\\\\${path.slice('\\\\?\\UNC\\'.length)}` } diff --git a/src/shared/windows-transient-lock-removal.ts b/src/shared/windows-transient-lock-removal.ts index c183c22eac9..22cb3c3cf44 100644 --- a/src/shared/windows-transient-lock-removal.ts +++ b/src/shared/windows-transient-lock-removal.ts @@ -2,11 +2,14 @@ import type { RmOptions } from 'node:fs' import { rmSync } from 'node:fs' -import { rm } from 'node:fs/promises' +import { rm as nodeRm } from 'node:fs/promises' +import { setTimeout as delay } from 'node:timers/promises' export const WINDOWS_RM_MAX_RETRIES = 8 export const WINDOWS_RM_RETRY_DELAY_MS = 150 +const WINDOWS_REMOVE_PASS_DELAYS_MS = [250, 500, 1_000, 2_000] +/** For `rmSync` only: its native retry loop is top-level, but async `rm` retries per entry and multiplies. */ export function transientLockRemovalOptions(): RmOptions { const base = { recursive: true, force: true } if (process.platform !== 'win32') { @@ -50,6 +53,28 @@ export function removeTreeSync(targetPath: string): void { } } -export async function removeTree(targetPath: string): Promise { - await rm(targetPath, transientLockRemovalOptions()) +type AsyncRm = (path: string, options: RmOptions) => Promise + +/** Recursive async delete; a transient Windows lock repeats the whole pass from one bounded budget. */ +export async function removeTree( + targetPath: string, + options: { rm?: AsyncRm; signal?: AbortSignal } = {} +): Promise { + const rm = options.rm ?? nodeRm + const passDelays = process.platform === 'win32' ? WINDOWS_REMOVE_PASS_DELAYS_MS : [] + for (let attempt = 0; ; attempt += 1) { + options.signal?.throwIfAborted() + try { + // Never `maxRetries` here: async rm retries each nested entry and never stops siblings, + // so one held-open file multiplies the work at every directory level. + await rm(targetPath, { recursive: true, force: true }) + return + } catch (error) { + if (attempt >= passDelays.length || !isTransientWindowsLockError(error)) { + throw error + } + } + // A pass in flight cannot be cancelled; abort is honored between passes. + await delay(passDelays[attempt], undefined, { signal: options.signal }) + } }