From de8d962cbc4cd79607815ab43a2b40c5ae2fb7d1 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 03:48:38 -0700 Subject: [PATCH] perf(worktrees): converge the trash sweep instead of re-walking doomed trees `transientLockRemovalOptions()` only asked for `maxRetries` on Windows, and `removeHostTree`'s retry ladder was gated on `process.platform === 'win32'`. A concurrent writer is not Windows-specific: Spotlight/`mds`, a scanner, or a live process writing under the tree surface the same EBUSY/ENOTEMPTY/EPERM on macOS and Linux. So on POSIX the startup sweep got exactly one attempt per entry, failed, and re-issued the same guaranteed-to-fail walk on every launch. - Extend the retry policy to every platform. Windows keeps its error set, its message fallback, and its delays; the message fallback stays Windows-only because POSIX always sets a code. - Persist a per-entry failure ledger in the trash root so a repeatedly failing entry is retried on a 15m/1h/6h ladder rather than on every launch. Nothing is abandoned: the ladder clamps, records are pruned when the entry goes, and a torn ledger fails open to a full sweep. - Defer the sweep behind first paint, so its recursive readdir/rm no longer competes with window creation and worktree-catalog hydration. --- src/main/host-tree-removal.test.ts | 96 ++++++++++ src/main/host-tree-removal.ts | 39 ++-- .../host/deferred-secret-protection-report.ts | 20 +- src/main/local-worktree-filesystem.test.ts | 20 +- src/main/startup/first-window-deferral.ts | 29 +++ .../startup/main-process-ready-runtime.ts | 20 +- src/main/worktree-trash-sweep-backoff.test.ts | 171 ++++++++++++++++++ src/main/worktree-trash-sweep-backoff.ts | 87 +++++++++ src/main/worktree-trash.test.ts | 9 +- src/main/worktree-trash.ts | 53 +++++- .../windows-transient-lock-removal.test.ts | 34 +++- src/shared/windows-transient-lock-removal.ts | 41 +++-- 12 files changed, 540 insertions(+), 79 deletions(-) create mode 100644 src/main/host-tree-removal.test.ts create mode 100644 src/main/startup/first-window-deferral.ts create mode 100644 src/main/worktree-trash-sweep-backoff.test.ts create mode 100644 src/main/worktree-trash-sweep-backoff.ts diff --git a/src/main/host-tree-removal.test.ts b/src/main/host-tree-removal.test.ts new file mode 100644 index 00000000000..ad14b34dfa2 --- /dev/null +++ b/src/main/host-tree-removal.test.ts @@ -0,0 +1,96 @@ +import type * as NodeFsPromises from 'node:fs/promises' +import { afterEach, describe, expect, it, vi } from 'vitest' + +const { rmMock, delayMock } = vi.hoisted(() => ({ + rmMock: vi.fn<(path: string, options?: unknown) => Promise>(), + delayMock: vi.fn(async (_ms?: number) => undefined) +})) + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, rm: rmMock } +}) +// Keep the retry ladder's real delays out of the test clock; only the attempt count is asserted. +vi.mock('node:timers/promises', () => ({ setTimeout: delayMock })) + +const { removeHostTree } = await import('./host-tree-removal') +const { WINDOWS_RM_MAX_RETRIES } = await import('../shared/windows-transient-lock-removal') + +/** POSIX-shaped so `toHostRemovalPath` is the identity on every platform under test. */ +const TRASH_ROOT = '/workspaces/.orca-worktree-trash' +const TRASH_ENTRY = `${TRASH_ROOT}/wt-1700000000000-abcdef01` + +function transientError(code: string): NodeJS.ErrnoException { + return Object.assign(new Error(`${code}: transient`), { code }) +} + +function withPlatform(platform: NodeJS.Platform): void { + Object.defineProperty(process, 'platform', { configurable: true, value: platform }) +} + +const originalPlatform = process.platform + +afterEach(() => { + withPlatform(originalPlatform) + rmMock.mockReset() + delayMock.mockClear() +}) + +describe('removeHostTree', () => { + // Why every platform: a concurrent writer (Spotlight/`mds`, a scanner, a live process under the + // tree) is not a Windows-only hazard, and a one-shot removal on POSIX never converged. + for (const platform of ['darwin', 'linux', 'win32'] as const) { + it(`retries a transient ENOTEMPTY until it succeeds on ${platform}`, async () => { + withPlatform(platform) + rmMock.mockRejectedValueOnce(transientError('ENOTEMPTY')) + rmMock.mockRejectedValueOnce(transientError('ENOTEMPTY')) + rmMock.mockResolvedValueOnce(undefined) + + await expect(removeHostTree(TRASH_ENTRY)).resolves.toBeUndefined() + + expect(rmMock).toHaveBeenCalledTimes(3) + // The retry may only change how many times the SAME path is attempted. + expect(new Set(rmMock.mock.calls.map((call) => call[0]))).toEqual(new Set([TRASH_ENTRY])) + expect(rmMock.mock.calls[0]?.[1]).toEqual({ + recursive: true, + force: true, + maxRetries: WINDOWS_RM_MAX_RETRIES, + retryDelay: expect.any(Number) + }) + }) + + it(`gives up on a non-transient failure without retrying on ${platform}`, async () => { + withPlatform(platform) + rmMock.mockRejectedValue(transientError('EIO')) + + await expect(removeHostTree(TRASH_ENTRY)).rejects.toThrow('EIO') + expect(rmMock).toHaveBeenCalledTimes(1) + }) + + it(`stops after a bounded number of attempts on ${platform}`, async () => { + withPlatform(platform) + rmMock.mockRejectedValue(transientError('EBUSY')) + + await expect(removeHostTree(TRASH_ENTRY)).rejects.toThrow('EBUSY') + expect(rmMock).toHaveBeenCalledTimes(5) + expect(new Set(rmMock.mock.calls.map((call) => call[0]))).toEqual(new Set([TRASH_ENTRY])) + }) + } + + it('does not retry prose-only failures off Windows, where every error carries a code', async () => { + withPlatform('linux') + rmMock.mockRejectedValue(new Error('directory not empty')) + + await expect(removeHostTree(TRASH_ENTRY)).rejects.toThrow('directory not empty') + expect(rmMock).toHaveBeenCalledTimes(1) + }) + + it('still retries a codeless Windows failure by its message', async () => { + withPlatform('win32') + rmMock.mockRejectedValueOnce(new Error('directory not empty')) + rmMock.mockResolvedValueOnce(undefined) + + await expect(removeHostTree(TRASH_ENTRY)).resolves.toBeUndefined() + expect(rmMock).toHaveBeenCalledTimes(2) + }) +}) diff --git a/src/main/host-tree-removal.ts b/src/main/host-tree-removal.ts index a5d5d447956..ab8c314b23e 100644 --- a/src/main/host-tree-removal.ts +++ b/src/main/host-tree-removal.ts @@ -1,15 +1,19 @@ // Why: every recursive host delete Orca performs (worktrees, terminal history, quarantined recovery -// generations) hits the same Windows stickiness — AV/indexers/late handle releases surface transient -// EBUSY/ENOTEMPTY/EPERM on a tree Node just emptied. One helper so no call site forgets the retries. +// generations) races whatever else is touching the tree — AV/indexers/late handle releases on +// Windows, Spotlight/`mds` or a live process writing under it on macOS and Linux — and all of them +// surface transient EBUSY/ENOTEMPTY/EPERM. One helper so no call site forgets the retries. import { rm } from 'node:fs/promises' import { win32 } from 'node:path' import { setTimeout as delay } from 'node:timers/promises' import { isWindowsAbsolutePathLike } from '../shared/cross-platform-path' import { isWslUncPath } from '../shared/wsl-paths' -import { transientLockRemovalOptions } from '../shared/windows-transient-lock-removal' +import { + isTransientRemovalError, + transientLockRemovalOptions +} from '../shared/windows-transient-lock-removal' -const WINDOWS_REMOVE_RETRY_DELAYS_MS = [250, 500, 1_000, 2_000] +const REMOVE_RETRY_DELAYS_MS = [250, 500, 1_000, 2_000] /** Convert a native host filesystem path to the Win32 long-path namespace. */ export function toHostFilesystemPath(targetPath: string): string { @@ -28,24 +32,11 @@ 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. */ +/** Recursively remove a host directory tree, retrying the transient concurrent-writer failures. */ export async function removeHostTree(targetPath: string): Promise { const removalPath = toHostRemovalPath(targetPath) - const retryDelays = process.platform === 'win32' ? WINDOWS_REMOVE_RETRY_DELAYS_MS : [] - // Why: large Windows trees commonly surface transient ENOTEMPTY/EPERM while Node walks and - // removes nested directories; Node's own retries absorb that before the loop below has to. + // Why: large trees commonly surface transient ENOTEMPTY/EPERM while Node walks and removes nested + // directories; Node's own retries absorb that before the loop below has to. const rmOptions = transientLockRemovalOptions() let attempt = 0 @@ -54,12 +45,12 @@ export async function removeHostTree(targetPath: string): Promise { await rm(removalPath, rmOptions) return } catch (error) { - if (attempt >= retryDelays.length || !isTransientWindowsRemovalError(error)) { + if (attempt >= REMOVE_RETRY_DELAYS_MS.length || !isTransientRemovalError(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]) + // Why a whole second pass and not just Node's inner retries: a writer that outlives them + // leaves directories Node already descended into, so only a fresh walk can finish the tree. + await delay(REMOVE_RETRY_DELAYS_MS[attempt]) attempt += 1 } } diff --git a/src/main/host/deferred-secret-protection-report.ts b/src/main/host/deferred-secret-protection-report.ts index 8f5be1fb047..7b4ea14367c 100644 --- a/src/main/host/deferred-secret-protection-report.ts +++ b/src/main/host/deferred-secret-protection-report.ts @@ -1,4 +1,4 @@ -import { app, type BrowserWindow } from 'electron' +import { runAfterFirstWindowShown } from '../startup/first-window-deferral' import { reportSecretProtectionGap } from './secret-protection-report' /** @@ -72,21 +72,5 @@ export function scheduleSecretProtectionGapReport({ } } - let ran = false - const run = (): void => { - if (ran) { - return - } - ran = true - clearTimeout(fallback) - // Why setImmediate: keep the blocking keyring probe off the event handler that - // reveals the window, so the reveal paints first. - setImmediate(report) - } - - const fallback = setTimeout(run, REPORT_FALLBACK_MS) - fallback.unref?.() - app.once('browser-window-created', (_event: Electron.Event, window: BrowserWindow) => { - window.once('ready-to-show', run) - }) + runAfterFirstWindowShown(report, REPORT_FALLBACK_MS) } diff --git a/src/main/local-worktree-filesystem.test.ts b/src/main/local-worktree-filesystem.test.ts index c9e97f72e90..3c8acca6331 100644 --- a/src/main/local-worktree-filesystem.test.ts +++ b/src/main/local-worktree-filesystem.test.ts @@ -159,9 +159,27 @@ describe('local worktree filesystem runtime access', () => { }) }) - it('does not retry host removal failures outside Windows', async () => { + // Why not Windows-only: Spotlight/`mds`, a scanner, or a live process writing under the tree race + // a POSIX removal the same way, and a one-shot rm left the worktree on disk for good. + it('retries transient host removal failures outside Windows too', async () => { + vi.useFakeTimers() await withPlatform('linux', async () => { const error = Object.assign(new Error('Directory not empty'), { code: 'ENOTEMPTY' }) + rmMock.mockRejectedValueOnce(error).mockResolvedValueOnce(undefined) + + const removal = removeLocalWorktreePath('/repo/feature') + await vi.advanceTimersByTimeAsync(250) + + await expect(removal).resolves.toBeUndefined() + expect(rmMock).toHaveBeenCalledTimes(2) + // The retry may only change how many times the SAME path is attempted. + expect(new Set(rmMock.mock.calls.map((call) => call[0]))).toEqual(new Set(['/repo/feature'])) + }) + }) + + it('does not retry a non-transient host removal failure outside Windows', async () => { + await withPlatform('linux', async () => { + const error = Object.assign(new Error('I/O error'), { code: 'EIO' }) rmMock.mockRejectedValue(error) await expect(removeLocalWorktreePath('/repo/feature')).rejects.toBe(error) diff --git a/src/main/startup/first-window-deferral.ts b/src/main/startup/first-window-deferral.ts new file mode 100644 index 00000000000..5a4d5e751b6 --- /dev/null +++ b/src/main/startup/first-window-deferral.ts @@ -0,0 +1,29 @@ +import { app, type BrowserWindow } from 'electron' + +/** + * Run `task` once the first window can paint, or after `fallbackMs` if it never does. + * + * For startup work nothing on the critical path consumes: a probe or a disk sweep started before the + * window exists competes with window creation for the same main thread and libuv threadpool, and the + * user sees that as the app being slow to open. + * + * Why a fallback as well as the window event: `ready-to-show` can fail to fire at all when the + * GPU/driver cannot present (see main-window-state-lifecycle), and headless serve has no window. + */ +export function runAfterFirstWindowShown(task: () => void, fallbackMs: number): void { + let ran = false + const run = (): void => { + if (ran) { + return + } + ran = true + clearTimeout(fallback) + // Why setImmediate: keep the work off the event handler that reveals the window, so it paints first. + setImmediate(task) + } + const fallback = setTimeout(run, fallbackMs) + fallback.unref?.() + app.once('browser-window-created', (_event: Electron.Event, window: BrowserWindow) => { + window.once('ready-to-show', run) + }) +} diff --git a/src/main/startup/main-process-ready-runtime.ts b/src/main/startup/main-process-ready-runtime.ts index f89f63186a0..75784a4c196 100644 --- a/src/main/startup/main-process-ready-runtime.ts +++ b/src/main/startup/main-process-ready-runtime.ts @@ -32,8 +32,12 @@ import { import { initializeMainProcessAutomations } from './main-process-automations' import { initializeMainProcessPlugins } from './main-process-plugins' import { collectWorktreeTrashSweepRoots, sweepStaleWorktreeTrash } from '../worktree-trash' +import { runAfterFirstWindowShown } from './first-window-deferral' import { logStartupMilestone } from './startup-diagnostics' +// Headless serve never opens a window, so the sweep still has to run off a timer there. +const WORKTREE_TRASH_SWEEP_FALLBACK_MS = 15_000 + export async function initializeReadyRuntimeServices(): Promise { const store = state.store if (!store) { @@ -74,12 +78,16 @@ export async function initializeReadyRuntimeServices(): Promise { state.emulatorBridge = new EmulatorBridge() runtime.setEmulatorBridge(state.emulatorBridge) // Why: worktree deletion renames the checkout aside and deletes it in the background, so a quit or - // crash mid-delete can leave the moved directory on disk. - void sweepStaleWorktreeTrash( - collectWorktreeTrashSweepRoots(store.getRepos(), store.getSettings()) - ).catch((error) => { - console.warn('[worktrees] Failed to sweep leftover worktree directories:', error) - }) + // crash mid-delete can leave the moved directory on disk. Why deferred: the sweep's recursive + // readdir/rm runs on the same libuv threadpool the window's first paint and worktree-catalog + // hydration are reading disk on, and nothing on the startup path consumes its result. + runAfterFirstWindowShown(() => { + void sweepStaleWorktreeTrash( + collectWorktreeTrashSweepRoots(store.getRepos(), store.getSettings()) + ).catch((error) => { + console.warn('[worktrees] Failed to sweep leftover worktree directories:', error) + }) + }, WORKTREE_TRASH_SWEEP_FALLBACK_MS) nativeTheme.themeSource = store.getSettings().theme ?? 'system' // Why (#16441): the real-home grant runs a codex app-server session. It stays // ordered before managed-hook reconciliation — an incapable host must re-arm diff --git a/src/main/worktree-trash-sweep-backoff.test.ts b/src/main/worktree-trash-sweep-backoff.test.ts new file mode 100644 index 00000000000..a3c55c58558 --- /dev/null +++ b/src/main/worktree-trash-sweep-backoff.test.ts @@ -0,0 +1,171 @@ +import { existsSync } from 'node:fs' +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { dirname, join, sep } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { removeHostTreeMock } = vi.hoisted(() => ({ + removeHostTreeMock: vi.fn<(path: string) => Promise>() +})) +vi.mock('./host-tree-removal', () => ({ removeHostTree: removeHostTreeMock })) + +const { sweepStaleWorktreeTrash, WORKTREE_TRASH_DIR_NAME } = await import('./worktree-trash') +const { TRASH_SWEEP_MAX_RETRY_DELAY_MS, TRASH_SWEEP_RETRY_DELAYS_MS, trashSweepBackoffPath } = + await import('./worktree-trash-sweep-backoff') + +const ENTRY = 'wt-1700000000000-abcdef01' +const OTHER_ENTRY = 'wt-1700000000001-abcdef02' + +let scratchDir = '' +let trashRoot = '' + +function transientFailure(): NodeJS.ErrnoException { + return Object.assign(new Error('ENOTEMPTY: directory not empty'), { code: 'ENOTEMPTY' }) +} + +async function readBackoff(): Promise> { + return JSON.parse(await readFile(trashSweepBackoffPath(trashRoot), 'utf8')) +} + +beforeEach(async () => { + scratchDir = await mkdtemp(join(tmpdir(), 'orca-trash-backoff-')) + trashRoot = join(scratchDir, WORKTREE_TRASH_DIR_NAME) + await mkdir(join(trashRoot, ENTRY, 'node_modules'), { recursive: true }) + removeHostTreeMock.mockReset() + removeHostTreeMock.mockRejectedValue(transientFailure()) +}) + +afterEach(async () => { + await rm(scratchDir, { recursive: true, force: true }) +}) + +describe('sweepStaleWorktreeTrash backoff', () => { + it('does not re-walk a failing entry on the next launch', async () => { + await sweepStaleWorktreeTrash([scratchDir]) + await sweepStaleWorktreeTrash([scratchDir]) + + // Without the persisted record, every launch re-walks the same doomed tree forever. + expect(removeHostTreeMock).toHaveBeenCalledTimes(1) + expect(existsSync(join(trashRoot, ENTRY))).toBe(true) + }) + + it('reports the deferral rather than counting it as swept', async () => { + expect(await sweepStaleWorktreeTrash([scratchDir])).toEqual({ removed: 0, deferred: 0 }) + expect(await sweepStaleWorktreeTrash([scratchDir])).toEqual({ removed: 0, deferred: 1 }) + }) + + it('persists the record before the sweep returns, so a quit mid-sweep keeps it', async () => { + await sweepStaleWorktreeTrash([scratchDir]) + + const backoff = await readBackoff() + expect(backoff[ENTRY].failures).toBe(1) + expect(backoff[ENTRY].nextAttemptAt).toBeGreaterThan(Date.now()) + }) + + it('retries the entry once the backoff window has elapsed, and clears the record on success', async () => { + await sweepStaleWorktreeTrash([scratchDir]) + await writeFile( + trashSweepBackoffPath(trashRoot), + JSON.stringify({ [ENTRY]: { failures: 1, nextAttemptAt: Date.now() - 1 } }), + 'utf8' + ) + removeHostTreeMock.mockImplementation(async (path) => { + await rm(path, { recursive: true, force: true }) + }) + + expect(await sweepStaleWorktreeTrash([scratchDir])).toEqual({ removed: 1, deferred: 0 }) + expect(removeHostTreeMock).toHaveBeenCalledTimes(2) + expect(existsSync(join(trashRoot, ENTRY))).toBe(false) + // An empty ledger is deleted rather than left as a permanent file in the trash root. + expect(existsSync(trashSweepBackoffPath(trashRoot))).toBe(false) + }) + + it('escalates the delay with each consecutive failure', async () => { + for (const [index, delayMs] of TRASH_SWEEP_RETRY_DELAYS_MS.entries()) { + await writeFile( + trashSweepBackoffPath(trashRoot), + JSON.stringify({ [ENTRY]: { failures: index, nextAttemptAt: Date.now() - 1 } }), + 'utf8' + ) + const before = Date.now() + await sweepStaleWorktreeTrash([scratchDir]) + + const backoff = await readBackoff() + expect(backoff[ENTRY].failures).toBe(index + 1) + expect(backoff[ENTRY].nextAttemptAt).toBeGreaterThanOrEqual(before + delayMs) + } + }) + + it('clamps at the last ladder step rather than deferring an entry forever', async () => { + await writeFile( + trashSweepBackoffPath(trashRoot), + JSON.stringify({ [ENTRY]: { failures: 99, nextAttemptAt: Date.now() - 1 } }), + 'utf8' + ) + const before = Date.now() + await sweepStaleWorktreeTrash([scratchDir]) + const after = Date.now() + + const { nextAttemptAt } = (await readBackoff())[ENTRY] + expect(nextAttemptAt).toBeGreaterThanOrEqual(before + TRASH_SWEEP_MAX_RETRY_DELAY_MS) + expect(nextAttemptAt).toBeLessThanOrEqual(after + TRASH_SWEEP_MAX_RETRY_DELAY_MS) + }) + + it('retries an entry parked by a clock that has since moved backwards', async () => { + await writeFile( + trashSweepBackoffPath(trashRoot), + JSON.stringify({ + [ENTRY]: { failures: 1, nextAttemptAt: Date.now() + TRASH_SWEEP_MAX_RETRY_DELAY_MS * 10 } + }), + 'utf8' + ) + + expect(await sweepStaleWorktreeTrash([scratchDir])).toEqual({ removed: 0, deferred: 0 }) + expect(removeHostTreeMock).toHaveBeenCalledTimes(1) + }) + + it('drops a torn ledger and sweeps everything, exactly as before it existed', async () => { + await writeFile( + trashSweepBackoffPath(trashRoot), + '{"wt-1700000000000-abcdef01": {"fail', + 'utf8' + ) + + expect(await sweepStaleWorktreeTrash([scratchDir])).toEqual({ removed: 0, deferred: 0 }) + expect(removeHostTreeMock).toHaveBeenCalledTimes(1) + expect((await readBackoff())[ENTRY].failures).toBe(1) + }) + + it('prunes records for entries that are no longer on disk', async () => { + await writeFile( + trashSweepBackoffPath(trashRoot), + JSON.stringify({ [OTHER_ENTRY]: { failures: 4, nextAttemptAt: Date.now() + 60_000 } }), + 'utf8' + ) + + await sweepStaleWorktreeTrash([scratchDir]) + + expect(Object.keys(await readBackoff())).toEqual([ENTRY]) + }) + + it('never offers the removal a path outside a trash root', async () => { + const liveWorktree = join(scratchDir, 'feature') + await mkdir(join(liveWorktree, 'src'), { recursive: true }) + await mkdir(join(trashRoot, OTHER_ENTRY), { recursive: true }) + await mkdir(join(trashRoot, 'unrelated-directory'), { recursive: true }) + await writeFile(join(trashRoot, 'wt-notes.txt'), 'keep me\n') + + await sweepStaleWorktreeTrash([scratchDir]) + + const attempted = removeHostTreeMock.mock.calls.map((call) => call[0]) + expect(new Set(attempted)).toEqual( + new Set([join(trashRoot, ENTRY), join(trashRoot, OTHER_ENTRY)]) + ) + for (const path of attempted) { + expect(dirname(path).endsWith(`${sep}${WORKTREE_TRASH_DIR_NAME}`)).toBe(true) + } + expect(existsSync(liveWorktree)).toBe(true) + expect(existsSync(join(trashRoot, 'unrelated-directory'))).toBe(true) + expect(existsSync(join(trashRoot, 'wt-notes.txt'))).toBe(true) + }) +}) diff --git a/src/main/worktree-trash-sweep-backoff.ts b/src/main/worktree-trash-sweep-backoff.ts new file mode 100644 index 00000000000..5551c016059 --- /dev/null +++ b/src/main/worktree-trash-sweep-backoff.ts @@ -0,0 +1,87 @@ +// Why: a trashed checkout that a live process keeps writing under fails removal on every launch, and +// the sweep had no memory of that — measured on one machine at 267 leftover entries / 3804 inodes, +// a 265 ms warm walk (worse cold) re-issued at every cold start against trees it is guaranteed to +// fail. This records the failure so the retry is scheduled instead of immediate. It only ever delays +// a retry; nothing here abandons an entry, and the disk is still reclaimed once the writer goes. + +import { readFile, rm, writeFile } from 'node:fs/promises' +import { join } from 'node:path' + +/** Deliberately not a `wt-…` name, so `isWorktreeTrashEntryName` never treats this as sweepable. */ +export const TRASH_SWEEP_BACKOFF_FILE_NAME = '.orca-sweep-backoff.json' + +// Consecutive-failure ladder, clamped at the last step. Launches a day apart still retry every +// entry; only a burst of relaunches stops re-walking the trees that just failed. +export const TRASH_SWEEP_RETRY_DELAYS_MS = [15 * 60_000, 60 * 60_000, 6 * 60 * 60_000] +export const TRASH_SWEEP_MAX_RETRY_DELAY_MS = Math.max(...TRASH_SWEEP_RETRY_DELAYS_MS) + +export type TrashSweepFailure = { readonly failures: number; readonly nextAttemptAt: number } +export type TrashSweepFailures = Map + +export function trashSweepBackoffPath(trashRoot: string): string { + return join(trashRoot, TRASH_SWEEP_BACKOFF_FILE_NAME) +} + +function parseFailures(parsed: unknown): TrashSweepFailures { + const failures: TrashSweepFailures = new Map() + if (typeof parsed !== 'object' || parsed === null) { + return failures + } + for (const [entry, record] of Object.entries(parsed as Record)) { + if (typeof record !== 'object' || record === null) { + continue + } + const { failures: count, nextAttemptAt } = record as Partial + if (Number.isFinite(count) && Number.isFinite(nextAttemptAt) && (count as number) > 0) { + failures.set(entry, { failures: count as number, nextAttemptAt: nextAttemptAt as number }) + } + } + return failures +} + +export async function readTrashSweepFailures(trashRoot: string): Promise { + const path = trashSweepBackoffPath(trashRoot) + try { + return parseFailures(JSON.parse(await readFile(path, 'utf8'))) + } catch (error) { + // Fail open: a torn or hand-edited file must not pin the sweep, so drop it and sweep everything. + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + await rm(path, { force: true }).catch(() => {}) + } + return new Map() + } +} + +export async function writeTrashSweepFailures( + trashRoot: string, + failures: TrashSweepFailures +): Promise { + const path = trashSweepBackoffPath(trashRoot) + try { + if (failures.size === 0) { + await rm(path, { force: true }) + return + } + await writeFile(path, JSON.stringify(Object.fromEntries(failures)), 'utf8') + } catch (error) { + // Best effort: losing the record costs one extra walk on the next launch, nothing more. + console.warn(`[worktrees] Failed to record trash sweep backoff at ${path}`, error) + } +} + +export function isTrashSweepEntryDue(record: TrashSweepFailure | undefined, now: number): boolean { + if (!record) { + return true + } + // The second clause: a clock that has since moved backwards must not park an entry indefinitely. + return now >= record.nextAttemptAt || record.nextAttemptAt - now > TRASH_SWEEP_MAX_RETRY_DELAY_MS +} + +export function nextTrashSweepFailure( + previous: TrashSweepFailure | undefined, + now: number +): TrashSweepFailure { + const failures = (previous?.failures ?? 0) + 1 + const step = Math.min(failures, TRASH_SWEEP_RETRY_DELAYS_MS.length) - 1 + return { failures, nextAttemptAt: now + TRASH_SWEEP_RETRY_DELAYS_MS[step] } +} diff --git a/src/main/worktree-trash.test.ts b/src/main/worktree-trash.test.ts index 5bec9f53475..9fd6d7cc87d 100644 --- a/src/main/worktree-trash.test.ts +++ b/src/main/worktree-trash.test.ts @@ -141,14 +141,17 @@ describe('sweepStaleWorktreeTrash', () => { }) it('ignores workspace roots that do not exist', async () => { - expect(await sweepStaleWorktreeTrash([join(scratchDir, 'missing')])).toEqual({ removed: 0 }) + expect(await sweepStaleWorktreeTrash([join(scratchDir, 'missing')])).toEqual({ + removed: 0, + deferred: 0 + }) }) it('never descends past the trash roots beside worktrees', async () => { const deepTrashRoot = join(scratchDir, 'repo', 'feature', WORKTREE_TRASH_DIR_NAME) await mkdir(join(deepTrashRoot, 'wt-1700000000002-abcdef03'), { recursive: true }) - expect(await sweepStaleWorktreeTrash([scratchDir])).toEqual({ removed: 0 }) + expect(await sweepStaleWorktreeTrash([scratchDir])).toEqual({ removed: 0, deferred: 0 }) expect(existsSync(join(deepTrashRoot, 'wt-1700000000002-abcdef03'))).toBe(true) }) @@ -159,7 +162,7 @@ describe('sweepStaleWorktreeTrash', () => { await mkdir(externalEntry, { recursive: true }) await symlink(join(scratchDir, 'external'), join(scratchDir, WORKTREE_TRASH_DIR_NAME)) - expect(await sweepStaleWorktreeTrash([scratchDir])).toEqual({ removed: 0 }) + expect(await sweepStaleWorktreeTrash([scratchDir])).toEqual({ removed: 0, deferred: 0 }) expect(existsSync(externalEntry)).toBe(true) } ) diff --git a/src/main/worktree-trash.ts b/src/main/worktree-trash.ts index cec17bf87cd..b503739dca3 100644 --- a/src/main/worktree-trash.ts +++ b/src/main/worktree-trash.ts @@ -6,6 +6,13 @@ import { randomBytes } from 'node:crypto' import { lstat, mkdir, readdir, rename, rmdir } from 'node:fs/promises' import { dirname, join } from 'node:path' import { removeHostTree } from './host-tree-removal' +import { + isTrashSweepEntryDue, + nextTrashSweepFailure, + readTrashSweepFailures, + writeTrashSweepFailures, + type TrashSweepFailures +} from './worktree-trash-sweep-backoff' import { isFolderRepo } from '../shared/repo-kind' import { computeWorkspaceRoot, getWorktreePathSettings } from './ipc/worktree-logic' import type { GlobalSettings } from '../shared/global-settings-types' @@ -94,12 +101,14 @@ export function whenWorktreeTrashDeletionsSettled(): Promise { /** * Delete trash entries left behind by a previous run (a crash or a kill during background deletion). - * Only entries matching the generated name pattern inside a trash root are removed. + * Only entries matching the generated name pattern inside a trash root are removed. An entry whose + * removal keeps failing is deferred onto a backoff rather than re-walked on every single launch. */ export async function sweepStaleWorktreeTrash( workspaceRoots: readonly string[] -): Promise<{ removed: number }> { +): Promise<{ removed: number; deferred: number }> { let removed = 0 + let deferred = 0 for (const trashRoot of await collectExistingTrashRoots(workspaceRoots)) { let entries: string[] try { @@ -111,25 +120,63 @@ export async function sweepStaleWorktreeTrash( } catch { continue } + const failures = await readTrashSweepFailures(trashRoot) + // Records for entries that are gone (removed here, or restored) must not accumulate forever. + let changed = pruneFailuresForMissingEntries(failures, entries) for (const entry of entries) { if (!isWorktreeTrashEntryName(entry)) { continue } + const failure = failures.get(entry) + if (!isTrashSweepEntryDue(failure, Date.now())) { + deferred += 1 + continue + } try { await removeHostTree(join(trashRoot, entry)) removed += 1 + changed = failures.delete(entry) || changed } catch (error) { + failures.set(entry, nextTrashSweepFailure(failure, Date.now())) + // Flush now: a sweep over many failing entries is long, and a quit mid-way must not throw + // away the backoff that was just earned. + await writeTrashSweepFailures(trashRoot, failures) + changed = false console.warn( `[worktrees] Failed to sweep leftover worktree at ${trashRoot}/${entry}`, error ) } } + if (changed) { + await writeTrashSweepFailures(trashRoot, failures) + } } if (removed > 0) { console.log(`[worktrees] Swept ${removed} leftover worktree director(ies) from a previous run`) } - return { removed } + if (deferred > 0) { + console.log(`[worktrees] Deferred ${deferred} leftover worktree director(ies) still in use`) + } + return { removed, deferred } +} + +function pruneFailuresForMissingEntries( + failures: TrashSweepFailures, + entries: readonly string[] +): boolean { + if (failures.size === 0) { + return false + } + const present = new Set(entries) + let pruned = false + for (const entry of failures.keys()) { + if (!present.has(entry)) { + failures.delete(entry) + pruned = true + } + } + return pruned } /** Trash roots live beside worktrees, so they sit at the workspace root (flat) or one level in (nested). */ diff --git a/src/shared/windows-transient-lock-removal.test.ts b/src/shared/windows-transient-lock-removal.test.ts index 32b527ebfe6..3eff6950743 100644 --- a/src/shared/windows-transient-lock-removal.test.ts +++ b/src/shared/windows-transient-lock-removal.test.ts @@ -47,8 +47,10 @@ describe('transient lock removal options', () => { rmSyncMock.mockReset() }) - it('retries on Windows, where a late handle release is the whole problem', () => { - withPlatform('win32') + // Why every platform: Windows is the acute case, but Spotlight/`mds`, a scanner or a live process + // writing under the tree race a POSIX removal the same way, and a one-shot rm never converged. + it.each(['win32', 'darwin', 'linux'] as const)('retries on %s', (platform) => { + withPlatform(platform) expect(transientLockRemovalOptions()).toEqual({ recursive: true, @@ -62,12 +64,28 @@ describe('transient lock removal options', () => { expect(WINDOWS_RM_MAX_RETRIES).toBe(8) }) - it('asks for no retries where removal is not raced by the OS', () => { - for (const platform of ['darwin', 'linux'] as const) { - withPlatform(platform) - expect(transientLockRemovalOptions()).toEqual({ recursive: true, force: true }) - Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) - } + it('retries a transient ENOTEMPTY off Windows too', () => { + withPlatform('darwin') + rmSyncMock.mockImplementationOnce(() => { + throw Object.assign(new Error('ENOTEMPTY: directory not empty'), { code: 'ENOTEMPTY' }) + }) + rmSyncMock.mockImplementationOnce(() => undefined) + + expect(() => removeTreeSync('/tmp/orca-host-job')).not.toThrow() + expect(rmSyncMock).toHaveBeenCalledTimes(2) + expect(new Set(rmSyncMock.mock.calls.map((call) => call[0]))).toEqual( + new Set(['/tmp/orca-host-job']) + ) + }) + + it('does not read a POSIX failure out of its prose, only its code', () => { + withPlatform('linux') + rmSyncMock.mockImplementation(() => { + throw new Error('operation not permitted') + }) + + expect(() => removeTreeSync('/tmp/orca-host-job')).toThrow('operation not permitted') + expect(rmSyncMock).toHaveBeenCalledTimes(1) }) it('retries a transient EPERM instead of treating force: true as enough', () => { diff --git a/src/shared/windows-transient-lock-removal.ts b/src/shared/windows-transient-lock-removal.ts index e77eeecf7f2..33f5f5bbf6d 100644 --- a/src/shared/windows-transient-lock-removal.ts +++ b/src/shared/windows-transient-lock-removal.ts @@ -1,8 +1,9 @@ -// Why: Windows releases handles late. Antivirus, the search indexer, a just-exited child and a -// freshly dlopen'd DLL all keep a tree Node has just emptied locked for a few milliseconds, which -// surfaces as EBUSY/ENOTEMPTY/EPERM. Node's own `maxRetries` absorbs exactly that, and the repo -// already settled on 8 attempts — but only product code was using it, so test teardown kept -// failing tests whose assertions had already passed. +// Why: a recursive removal races whatever else is touching the tree. Windows is the acute case — +// antivirus, the search indexer, a just-exited child and a freshly dlopen'd DLL all keep a tree Node +// has just emptied locked for a few milliseconds — but it is not the only one: on macOS and Linux +// Spotlight/`mds`, a scanner, or a live process writing under the tree surface the same +// EBUSY/ENOTEMPTY/EPERM while Node walks it. Node's own `maxRetries` absorbs exactly that, and the +// repo settled on 8 attempts. The constant names keep the prefix the repo already ratchets on. import type { RmOptions } from 'node:fs' import { rmSync } from 'node:fs' @@ -11,23 +12,32 @@ import { rm } from 'node:fs/promises' export const WINDOWS_RM_MAX_RETRIES = 8 export const WINDOWS_RM_RETRY_DELAY_MS = 150 -/** `rm`/`rmSync` options for a recursive removal that must survive a late handle release. */ +/** `rm`/`rmSync` options for a recursive removal that must survive a concurrent writer. */ export function transientLockRemovalOptions(): RmOptions { - const base = { recursive: true, force: true } - if (process.platform !== 'win32') { - return base + // Why every platform: `maxRetries` only ever re-issues the syscall that failed, against the path + // the caller already chose. It cannot reach a path the one-shot removal would not have touched. + return { + recursive: true, + force: true, + maxRetries: WINDOWS_RM_MAX_RETRIES, + retryDelay: WINDOWS_RM_RETRY_DELAY_MS } - return { ...base, maxRetries: WINDOWS_RM_MAX_RETRIES, retryDelay: WINDOWS_RM_RETRY_DELAY_MS } } -function isTransientWindowsLockError(error: unknown): boolean { - if (process.platform !== 'win32' || typeof error !== 'object' || error === null) { +/** Whether a removal failure is a concurrent writer worth re-attempting rather than a real fault. */ +export function isTransientRemovalError(error: unknown): boolean { + if (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 } + // Why Windows-only: there the failure can arrive with no `code` at all. POSIX always sets one, so + // matching prose there would only retry unrelated errors that happen to read like a lock. + if (process.platform !== 'win32') { + return false + } const message = 'message' in error && typeof error.message === 'string' ? error.message : '' return /directory not empty|resource busy|operation not permitted/i.test(message) } @@ -36,10 +46,9 @@ function sleepSync(ms: number): void { Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms) } -/** Recursively remove a directory, retrying the transient Windows locks. */ +/** Recursively remove a directory, retrying a transient concurrent-writer failure. */ export function removeTreeSync(targetPath: string): void { const options = transientLockRemovalOptions() - const extraAttempts = process.platform === 'win32' ? WINDOWS_RM_MAX_RETRIES : 0 let attempt = 0 for (;;) { try { @@ -49,7 +58,7 @@ export function removeTreeSync(targetPath: string): void { // Why the outer loop: Node's `maxRetries` only runs inside a real `rmSync`. A mock, or a // handle that outlives those inner attempts, still surfaces EPERM. `force: true` only // suppresses ENOENT. - if (attempt >= extraAttempts || !isTransientWindowsLockError(error)) { + if (attempt >= WINDOWS_RM_MAX_RETRIES || !isTransientRemovalError(error)) { throw error } sleepSync(WINDOWS_RM_RETRY_DELAY_MS) @@ -58,7 +67,7 @@ export function removeTreeSync(targetPath: string): void { } } -/** Recursively remove a directory, retrying the transient Windows locks. */ +/** Recursively remove a directory, retrying a transient concurrent-writer failure. */ export async function removeTree(targetPath: string): Promise { await rm(targetPath, transientLockRemovalOptions()) }