diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index ba3ba7556ed1..4c12a2635bc8 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -1080,6 +1080,8 @@ jobs: src/main/windows/windows-process-table-native-addon.win32.test.ts src/main/persistence/profile-state/profile-state-access-windows-native.win32.test.ts src/main/windows-live-tree-kill.win32.test.ts + src/main/tree-removal-walk.test.ts + src/main/host-tree-removal.test.ts src/main/wsl/wsl-runner.test.ts src/main/wsl/wsl-guest-environment.test.ts src/main/wsl/wsl-invocation-boundary.test.ts diff --git a/config/scripts/pr-code-change-scope.mjs b/config/scripts/pr-code-change-scope.mjs index 98dc914052af..22d98b4d2928 100644 --- a/config/scripts/pr-code-change-scope.mjs +++ b/config/scripts/pr-code-change-scope.mjs @@ -288,6 +288,8 @@ const WINDOWS_PACKAGE_TESTS = [ 'src/main/windows/windows-process-table-native-addon.win32.test.ts', 'src/main/persistence/profile-state/profile-state-access-windows-native.win32.test.ts', 'src/main/windows-live-tree-kill.win32.test.ts', + 'src/main/tree-removal-walk.test.ts', + 'src/main/host-tree-removal.test.ts', 'src/main/wsl/wsl-runner.test.ts', 'src/main/wsl/wsl-guest-environment.test.ts', 'src/main/wsl/wsl-invocation-boundary.test.ts', diff --git a/src/main/ai-vault-search/session-search-index-test-fixture.ts b/src/main/ai-vault-search/session-search-index-test-fixture.ts index baa3e2976fe9..488d57a05976 100644 --- a/src/main/ai-vault-search/session-search-index-test-fixture.ts +++ b/src/main/ai-vault-search/session-search-index-test-fixture.ts @@ -1,7 +1,7 @@ import { mkdtemp } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { removeTree } from '../../shared/windows-transient-lock-removal' +import { removeTreeSync } from '../../shared/windows-transient-lock-removal' import type { AiVaultSession } from '../../shared/ai-vault-types' import type { SessionFileCandidate } from '../ai-vault/session-scanner-types' import { TranscriptMessageChannel } from '../ai-vault/session-transcript-channel' @@ -118,7 +118,7 @@ export async function openSessionSearchIndexFile(name: string): Promise { for (const root of roots) { await rm(root, { recursive: true, force: true }).catch(() => {}) } }) -describe('asar-transparent rm', () => { - it('removes a tree recursively where `original-fs` is unresolvable', async () => { +describe('asar-transparent fs', () => { + it('falls back to `node:fs/promises` where `original-fs` is unresolvable', () => { + expect(asarTransparentFs().readdir).toBe(readdir) + }) + + it('removes a tree recursively outside Electron', async () => { expect(process.versions.electron).toBeUndefined() const root = await mkdtemp(join(tmpdir(), 'orca-asar-transparent-')) roots.push(root) diff --git a/src/main/asar-transparent-fs.ts b/src/main/asar-transparent-fs.ts index cddab8434348..4574abc2a234 100644 --- a/src/main/asar-transparent-fs.ts +++ b/src/main/asar-transparent-fs.ts @@ -1,35 +1,37 @@ -// Why: Electron patches `fs` so a `*.asar` file reports `isDirectory() === true`, so Node's -// recursive `rm` descends into the archive, tries to `rmdir` a real file, and fails the parent with -// ENOTEMPTY. Every worktree that has ever run `pnpm install` carries at least one -// (`node_modules/.pnpm/electron@…/…/Electron.app/Contents/Resources/default_app.asar`), so a +// Why: Electron patches `fs` so a `*.asar` file reports `isDirectory() === true` and lists the +// archive's contents, so a tree delete descends into the archive, tries to `rmdir` a real file, and +// fails the parent with ENOTEMPTY. Every worktree that has ever run `pnpm install` carries at least +// one (`node_modules/.pnpm/electron@…/…/Electron.app/Contents/Resources/default_app.asar`), so a // worktree removal aborts there deterministically — the residue is not a concurrent-writer race and // no amount of retrying clears it. `original-fs` is Electron's unpatched `fs`; unlike -// `process.noAsar` it is scoped to this call rather than to the whole process, which matters because -// a multi-GB removal runs for seconds while the main process may still be loading modules out of -// `app.asar`. See `cli/appimage-payload-removal.ts` for the same bug at a call site short enough to -// use the process-global flag. +// `process.noAsar` it is scoped to these calls rather than to the whole process, which matters +// because a multi-GB removal runs for seconds while the main process may still be loading modules +// out of `app.asar`. -import { rm as nodeRm } from 'node:fs/promises' +import * as nodeFsPromises from 'node:fs/promises' import { createRequire } from 'node:module' -type Rm = typeof nodeRm +export type AsarTransparentFs = Pick< + typeof nodeFsPromises, + 'lstat' | 'readdir' | 'rm' | 'rmdir' | 'unlink' +> -let resolvedRm: Rm | undefined +let resolved: AsarTransparentFs | undefined -function resolveRm(): Rm { +function resolveFs(): AsarTransparentFs { try { - // Why require and not an import: `original-fs` only exists inside Electron, so vitest, the - // `orca` CLI and the plain-node entrypoints must resolve `node:fs/promises` instead — and there - // the shim does not exist either, so plain `fs` is already asar-transparent. - const originalFs = createRequire(__filename)('original-fs') as { promises?: { rm?: Rm } } - return typeof originalFs.promises?.rm === 'function' ? originalFs.promises.rm : nodeRm + // Why require and not an import: `original-fs` only exists inside Electron (main and the + // run-as-node daemon), so vitest, the `orca` CLI, plain-node entrypoints and Bun must resolve + // `node:fs/promises` instead — and there the shim does not exist, so plain `fs` already is. + const originalFs: { promises?: AsarTransparentFs } = createRequire(__filename)('original-fs') + return originalFs.promises ?? nodeFsPromises } catch { - return nodeRm + return nodeFsPromises } } -/** `fs.promises.rm` that sees a `*.asar` as the file it is rather than as a directory. */ -export const rm: Rm = (path, options) => { - resolvedRm ??= resolveRm() - return resolvedRm(path, options) +/** `fs.promises` calls that see a `*.asar` as the file it is rather than as a directory. */ +export function asarTransparentFs(): AsarTransparentFs { + resolved ??= resolveFs() + return resolved } diff --git a/src/main/browser/browser-client-download-relay.ts b/src/main/browser/browser-client-download-relay.ts index 2aa294006b2f..1d3fb6263f28 100644 --- a/src/main/browser/browser-client-download-relay.ts +++ b/src/main/browser/browser-client-download-relay.ts @@ -1,6 +1,6 @@ import { randomUUID } from 'node:crypto' import { mkdirSync, rmSync } from 'node:fs' -import { open, rm, stat } from 'node:fs/promises' +import { open, stat } from 'node:fs/promises' import path from 'node:path' import { @@ -11,6 +11,7 @@ import { } from '../../shared/browser-client-file-channel-protocol' import type { BrowserClientHostedPageInventory } from '../../shared/browser-client-host-protocol' import type { BrowserClientFileChannelTransport } from './browser-client-file-channel-transport' +import { removeHostTree } from '../host-tree-removal' export const BROWSER_CLIENT_FILE_CHANNEL_WRITE_METHOD = 'browser.clientHost.fileChannel.write' export const BROWSER_CLIENT_FILE_CHANNEL_ABORT_METHOD = 'browser.clientHost.fileChannel.abort' @@ -73,7 +74,7 @@ const nodeRelayFilesystem: RelayFilesystem = { }, size: async (filePath) => (await stat(filePath)).size, removeDirectory: async (directory) => { - await rm(directory, { recursive: true, force: true }) + await removeHostTree(directory) } } diff --git a/src/main/browser/browser-client-upload-staging.test.ts b/src/main/browser/browser-client-upload-staging.test.ts index 7c47fc82baab..8594c3585988 100644 --- a/src/main/browser/browser-client-upload-staging.test.ts +++ b/src/main/browser/browser-client-upload-staging.test.ts @@ -1,9 +1,9 @@ import { existsSync } from 'node:fs' -import type * as NodeFsPromises from 'node:fs/promises' import { mkdir, mkdtemp, readFile, readdir, realpath, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import path from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as HostTreeRemoval from '../host-tree-removal' import { BROWSER_CLIENT_UPLOAD_STAGING_MAX_BYTES_PER_PAGE, @@ -11,15 +11,15 @@ import { BrowserClientUploadStaging } from './browser-client-upload-staging' -const nodeRemovals = vi.hoisted(() => ({ rm: [] as { target: unknown; options: unknown }[] })) +const hostTreeRemovals = vi.hoisted((): { targets: string[] } => ({ targets: [] })) -vi.mock('node:fs/promises', async (importOriginal) => { - const actual = await importOriginal() +vi.mock('../host-tree-removal', async (importOriginal) => { + const actual = await importOriginal() return { ...actual, - rm: (target: unknown, options: unknown) => { - nodeRemovals.rm.push({ target, options }) - return (actual.rm as (t: unknown, o: unknown) => Promise)(target, options) + removeHostTree: (target: string) => { + hostTreeRemovals.targets.push(target) + return actual.removeHostTree(target) } } }) @@ -310,7 +310,7 @@ describe('BrowserClientUploadStaging', () => { expect(staging.activeStagingCount()).toBe(1) }) - it('retries the staged removal so a briefly held file is not orphaned', async () => { + it('removes through the retrying host tree removal so a briefly held file is not orphaned', async () => { const staging = new BrowserClientUploadStaging(stagingRoot) const staged = await staging.stage({ browserPageId: 'page-1', @@ -318,15 +318,14 @@ describe('BrowserClientUploadStaging', () => { files: [{ remotePath: 'a.txt', contents: Buffer.from('a') }] }) const directory = staging.stagedDirectory(staged.stagingId) - nodeRemovals.rm.length = 0 + if (directory === undefined) { + throw new Error('expected a staged directory') + } + hostTreeRemovals.targets.length = 0 expect(await staging.release(staged.stagingId)).toBe(true) - expect(nodeRemovals.rm).toEqual([ - { - target: directory, - options: { recursive: true, force: true, maxRetries: 3, retryDelay: 50 } - } - ]) + expect(hostTreeRemovals.targets).toEqual([directory]) + expect(existsSync(directory)).toBe(false) }) }) diff --git a/src/main/browser/browser-client-upload-staging.ts b/src/main/browser/browser-client-upload-staging.ts index cc21c2f62891..2c19ec7a8363 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 { removeHostTree } from '../host-tree-removal' export type BrowserClientStagedUpload = { stagingId: string @@ -53,7 +54,7 @@ const nodeStagingFilesystem: StagingFilesystem = { await writeFile(filePath, contents, { mode: 0o600 }) }, removeDirectory: async (directory) => { - await rm(directory, stagingRemovalOptions) + await removeHostTree(directory) }, removeDirectorySync: (directory) => { rmSync(directory, stagingRemovalOptions) diff --git a/src/main/browser/browser-route-partition-storage-dependencies.ts b/src/main/browser/browser-route-partition-storage-dependencies.ts index f672a882360f..723053cf3121 100644 --- a/src/main/browser/browser-route-partition-storage-dependencies.ts +++ b/src/main/browser/browser-route-partition-storage-dependencies.ts @@ -1,5 +1,4 @@ import { session } from 'electron' -import { rm } from 'node:fs/promises' import { releaseBrowserRoutePartitionStorage, type BrowserRoutePartitionStorageDependencies @@ -9,6 +8,7 @@ import { routePartitionDataRoot } from './browser-route-partition-binding-runtime' import { browserSessionRegistry } from './browser-session-registry' +import { removeHostTree } from '../host-tree-removal' export function browserRoutePartitionStorageDependencies( isPartitionLive: (partition: string) => boolean @@ -24,7 +24,7 @@ export function browserRoutePartitionStorageDependencies( browserSessionRegistry.clearRoutePartitionPolicies(partition) }, // Why: force ignores an already-absent directory, keeping the sweep idempotent. - removePartitionDirectory: (directory) => rm(directory, { recursive: true, force: true }) + removePartitionDirectory: (directory) => removeHostTree(directory) } } diff --git a/src/main/cli/appimage-payload-removal.reentrancy.test.ts b/src/main/cli/appimage-payload-removal.reentrancy.test.ts index ebf9c399c9e4..61e293c3a328 100644 --- a/src/main/cli/appimage-payload-removal.reentrancy.test.ts +++ b/src/main/cli/appimage-payload-removal.reentrancy.test.ts @@ -1,9 +1,9 @@ import { afterEach, describe, expect, it, vi } from 'vitest' -// Why a mocked rm: the property under test is the ORDER in which overlapping removals settle, which -// real filesystem timing cannot pin down. Deferreds make the interleaving exact. -const { rmMock } = vi.hoisted(() => ({ rmMock: vi.fn() })) -vi.mock('node:fs/promises', () => ({ rm: rmMock })) +// Why a mocked removal: the property under test is the ORDER in which overlapping removals settle, +// which real filesystem timing cannot pin down. Deferreds make the interleaving exact. +const { removeHostTreeMock } = vi.hoisted(() => ({ removeHostTreeMock: vi.fn() })) +vi.mock('../host-tree-removal', () => ({ removeHostTree: removeHostTreeMock })) const originalNoAsar = process.noAsar @@ -21,45 +21,47 @@ function deferred(): { promise: Promise; resolve: () => void } { } describe('removeExtractedAppImagePayload reentrancy', () => { - // The hazard is the FIRST removal settling while a later one is still running: a naive - // save/restore would hand the shim back and silently leak the rest of the second removal. - it('keeps asar interception disabled while a later removal is still running', async () => { + // The old hazard was a process-wide asar toggle handed back while a later removal still ran; the + // shared removal holds no such flag, so overlapping removals must leave it untouched throughout. + it('leaves asar interception untouched while overlapping removals settle', async () => { process.noAsar = false const first = deferred() const second = deferred() - rmMock.mockReset() - rmMock.mockReturnValueOnce(first.promise).mockReturnValueOnce(second.promise) + removeHostTreeMock.mockReset() + removeHostTreeMock.mockReturnValueOnce(first.promise).mockReturnValueOnce(second.promise) const { removeExtractedAppImagePayload } = await import('./appimage-payload-removal') const firstCall = removeExtractedAppImagePayload('/cache/gen-a') const secondCall = removeExtractedAppImagePayload('/cache/gen-b') - expect(process.noAsar).toBe(true) + expect(removeHostTreeMock.mock.calls).toEqual([['/cache/gen-a'], ['/cache/gen-b']]) + expect(process.noAsar).toBe(false) first.resolve() await firstCall - // gen-b is still being removed: handing the shim back here is exactly the leak. - expect(process.noAsar).toBe(true) + expect(process.noAsar).toBe(false) second.resolve() await secondCall expect(process.noAsar).toBe(false) }) - it('keeps the flag held when the first removal rejects mid-overlap', async () => { + it('rejects only the failed removal when the first rejects mid-overlap', async () => { process.noAsar = false const second = deferred() - rmMock.mockReset() - rmMock.mockRejectedValueOnce(new Error('EACCES')).mockReturnValueOnce(second.promise) + removeHostTreeMock.mockReset() + removeHostTreeMock + .mockRejectedValueOnce(new Error('EACCES')) + .mockReturnValueOnce(second.promise) const { removeExtractedAppImagePayload } = await import('./appimage-payload-removal') const firstCall = removeExtractedAppImagePayload('/cache/gen-a') const secondCall = removeExtractedAppImagePayload('/cache/gen-b') await expect(firstCall).rejects.toThrow('EACCES') - expect(process.noAsar).toBe(true) + expect(process.noAsar).toBe(false) second.resolve() - await secondCall + await expect(secondCall).resolves.toBeUndefined() expect(process.noAsar).toBe(false) }) }) diff --git a/src/main/cli/appimage-payload-removal.test.ts b/src/main/cli/appimage-payload-removal.test.ts index caa3ee33c1c0..ab5235b254de 100644 --- a/src/main/cli/appimage-payload-removal.test.ts +++ b/src/main/cli/appimage-payload-removal.test.ts @@ -27,15 +27,17 @@ describe('removeExtractedAppImagePayload', () => { expect(existsSync(root)).toBe(false) }) - it('disables asar interception for the removal', async () => { + // Why: `process.noAsar` is process-wide; flipping it mid-removal would change asar semantics for + // every concurrent reader in the process, and the unpatched-fs walk makes the toggle unnecessary. + it('removes the payload without toggling process-wide asar interception', async () => { const root = await makePayloadTree() - let observed: boolean | undefined - const originalRealpath = process.noAsar + const assigned: boolean[] = [] + const noAsarBefore = process.noAsar Object.defineProperty(process, 'noAsar', { configurable: true, - get: () => observed ?? originalRealpath, + get: () => noAsarBefore, set: (value: boolean) => { - observed ??= value + assigned.push(value) } }) try { @@ -44,13 +46,14 @@ describe('removeExtractedAppImagePayload', () => { Object.defineProperty(process, 'noAsar', { configurable: true, writable: true, - value: originalRealpath + value: noAsarBefore }) } - expect(observed).toBe(true) + expect(assigned).toEqual([]) + expect(existsSync(root)).toBe(false) }) - it('restores the previous asar setting after a failure', async () => { + it('surfaces a removal failure and leaves the asar setting untouched', async () => { process.noAsar = false await expect( removeExtractedAppImagePayload(join(tmpdir(), 'orca-missing', 'nested', '\0invalid')) diff --git a/src/main/cli/appimage-payload-removal.ts b/src/main/cli/appimage-payload-removal.ts index 86a6a326ceed..1b97cdb6b929 100644 --- a/src/main/cli/appimage-payload-removal.ts +++ b/src/main/cli/appimage-payload-removal.ts @@ -1,35 +1,13 @@ -import { rm } from 'node:fs/promises' - -// Why a counter and not a saved value: `process.noAsar` is process-wide, so two overlapping -// removals would race — the first to settle would restore the shim while the second is still -// running, and the rest of that removal would silently leak again. Removals are sequential today; -// this keeps that a property of the module rather than of its callers. -let activeRemovals = 0 -// Captured once when the outermost removal starts; `process.noAsar` is typed boolean, and an -// unset flag is falsy, so restoring `false` is equivalent to restoring `undefined`. -let asarBeforeOutermostRemoval = false +import { removeHostTree } from '../host-tree-removal' /** * Removes an extracted AppImage payload tree. * - * Why not a plain recursive `rm`: Electron patches `fs` so a `*.asar` file reports - * `isDirectory() === true`. A recursive remove then tries to `rmdir` a real file, fails with - * ENOTEMPTY, and strands the ~105 MB `resources/app.asar` of every superseded generation. - * `process.noAsar` restores real filesystem semantics; the window is ~33 ms for a full 519 MB - * generation, and nothing in the registration path reads asar content inside it. + * Why `removeHostTree` and not a plain recursive `rm`: Electron patches `fs` so a `*.asar` file + * reports `isDirectory() === true`. A recursive remove then tries to `rmdir` a real file, fails + * with ENOTEMPTY, and strands the ~105 MB `resources/app.asar` of every superseded generation. + * `removeHostTree` walks with Electron's unpatched fs, so it needs no process-wide `process.noAsar`. */ export async function removeExtractedAppImagePayload(targetPath: string): Promise { - if (activeRemovals === 0) { - asarBeforeOutermostRemoval = process.noAsar === true - } - activeRemovals += 1 - process.noAsar = true - try { - await rm(targetPath, { recursive: true, force: true }) - } finally { - activeRemovals -= 1 - if (activeRemovals === 0) { - process.noAsar = asarBeforeOutermostRemoval - } - } + await removeHostTree(targetPath) } diff --git a/src/main/computer/desktop-script-provider-client.ts b/src/main/computer/desktop-script-provider-client.ts index a655eeddb079..012c0986ad95 100644 --- a/src/main/computer/desktop-script-provider-client.ts +++ b/src/main/computer/desktop-script-provider-client.ts @@ -1,4 +1,4 @@ -import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { mkdtemp, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import type { @@ -40,6 +40,7 @@ import { DesktopScriptSnapshotStore } from './desktop-script-snapshot-store' import { normalizeBridgeApp, renderSnapshot } from './desktop-script-snapshot-rendering' import { normalizeComputerActionResult } from './computer-action-verification-normalization' import { RuntimeClientError } from './runtime-client-error' +import { removeHostTree } from '../host-tree-removal' export function shouldUseDesktopScriptProvider(): boolean { return desktopScriptPlatform() !== null && resolveDesktopScriptProviderPath() !== null @@ -242,7 +243,7 @@ export class DesktopScriptProviderClient { } return checkedBridgeResponse(response, stderr) } finally { - await rm(operationDirectory, { force: true, recursive: true }) + await removeHostTree(operationDirectory) } } diff --git a/src/main/computer/desktop-script-provider-test-harness.ts b/src/main/computer/desktop-script-provider-test-harness.ts index 43d213c855f5..45c4e5bcb19f 100644 --- a/src/main/computer/desktop-script-provider-test-harness.ts +++ b/src/main/computer/desktop-script-provider-test-harness.ts @@ -4,18 +4,20 @@ import { expect, vi } from 'vitest' import type { DesktopScriptRuntimeHost } from './desktop-script-runtime-host' -const { execFileMock, operationFiles, mkdtempMock, rmMock, writeFileMock } = vi.hoisted(() => { - const files = new Map() - return { - execFileMock: vi.fn(), - operationFiles: files, - mkdtempMock: vi.fn(async (prefix: string) => `${prefix}${files.size}`), - rmMock: vi.fn(async () => undefined), - writeFileMock: vi.fn(async (filePath: string, data: string | Buffer) => { - files.set(filePath, Buffer.isBuffer(data) ? data.toString('utf8') : data) - }) +const { execFileMock, operationFiles, mkdtempMock, removeHostTreeMock, writeFileMock } = vi.hoisted( + () => { + const files = new Map() + return { + execFileMock: vi.fn(), + operationFiles: files, + mkdtempMock: vi.fn(async (prefix: string) => `${prefix}${files.size}`), + removeHostTreeMock: vi.fn(async () => undefined), + writeFileMock: vi.fn(async (filePath: string, data: string | Buffer) => { + files.set(filePath, Buffer.isBuffer(data) ? data.toString('utf8') : data) + }) + } } -}) +) vi.mock('child_process', () => ({ execFile: execFileMock @@ -23,10 +25,11 @@ vi.mock('child_process', () => ({ vi.mock('fs/promises', () => ({ mkdtemp: mkdtempMock, - rm: rmMock, writeFile: writeFileMock })) +vi.mock('../host-tree-removal', () => ({ removeHostTree: removeHostTreeMock })) + /** Builds a client on the one-shot bridge; pass a host to exercise serve mode. */ export async function createDesktopScriptProviderClient( platform: 'linux' | 'windows', @@ -42,7 +45,7 @@ export function resetDesktopScriptProviderTestHarness(): void { execFileMock.mockReset() operationFiles.clear() mkdtempMock.mockClear() - rmMock.mockClear() + removeHostTreeMock.mockClear() writeFileMock.mockClear() } diff --git a/src/main/computer/macos-computer-use-permission-status.test.ts b/src/main/computer/macos-computer-use-permission-status.test.ts index cfa511b11ce0..3fddd7c16a55 100644 --- a/src/main/computer/macos-computer-use-permission-status.test.ts +++ b/src/main/computer/macos-computer-use-permission-status.test.ts @@ -1,9 +1,10 @@ import { execFileSync, spawn, spawnSync } from 'node:child_process' -import { mkdtemp, readFile, rm, stat } from 'node:fs/promises' +import { mkdtemp, readFile, stat } from 'node:fs/promises' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const resolveHelperAppPathMock = vi.hoisted(() => vi.fn()) +const removeHostTreeMock = vi.hoisted(() => vi.fn()) const resolveHelperExecutablePathMock = vi.hoisted(() => vi.fn()) const permissionStatusTempDir = '/tmp/orca-computer-use-permissions-test' const permissionStatusPath = join(permissionStatusTempDir, 'status.json') @@ -31,10 +32,11 @@ vi.mock('child_process', () => ({ vi.mock('fs/promises', () => ({ mkdtemp: vi.fn(), readFile: vi.fn(), - rm: vi.fn(), stat: vi.fn() })) +vi.mock('../host-tree-removal', () => ({ removeHostTree: removeHostTreeMock })) + vi.mock('./macos-native-provider-paths', () => ({ resolveMacOSComputerUseAppPath: resolveHelperAppPathMock, resolveMacOSComputerUseExecutablePath: resolveHelperExecutablePathMock @@ -49,7 +51,7 @@ describe('getComputerUsePermissionStatus', () => { vi.mocked(execFileSync).mockReset() vi.mocked(mkdtemp).mockReset() vi.mocked(readFile).mockReset() - vi.mocked(rm).mockReset() + removeHostTreeMock.mockReset() vi.mocked(stat).mockReset() resolveHelperAppPathMock.mockReset() resolveHelperExecutablePathMock.mockReset() @@ -89,10 +91,7 @@ describe('getComputerUsePermissionStatus', () => { code: 'accessibility_error', message: 'Could not check permissions: failed to launch helper' }) - expect(rm).toHaveBeenCalledWith(permissionStatusTempDir, { - recursive: true, - force: true - }) + expect(removeHostTreeMock).toHaveBeenCalledWith(permissionStatusTempDir) }) it('removes permission status helper listeners after close', async () => { @@ -156,10 +155,7 @@ describe('getComputerUsePermissionStatus', () => { expect(settled).toBe(true) await rejection expect(child.kill).toHaveBeenCalled() - expect(rm).toHaveBeenCalledWith(permissionStatusTempDir, { - recursive: true, - force: true - }) + expect(removeHostTreeMock).toHaveBeenCalledWith(permissionStatusTempDir) }) it('reads permission status through the helper app identity', async () => { @@ -188,10 +184,7 @@ describe('getComputerUsePermissionStatus', () => { ) expect(spawnSync).not.toHaveBeenCalled() expect(readFile).toHaveBeenCalledWith(permissionStatusPath, 'utf8') - expect(rm).toHaveBeenCalledWith(permissionStatusTempDir, { - recursive: true, - force: true - }) + expect(removeHostTreeMock).toHaveBeenCalledWith(permissionStatusTempDir) }) it('returns unavailable permission status when the helper app is missing on macOS', async () => { diff --git a/src/main/computer/macos-computer-use-permission-status.ts b/src/main/computer/macos-computer-use-permission-status.ts index b23c3e9ab96e..2bd3a251df6f 100644 --- a/src/main/computer/macos-computer-use-permission-status.ts +++ b/src/main/computer/macos-computer-use-permission-status.ts @@ -1,5 +1,5 @@ import { spawn } from 'node:child_process' -import { mkdtemp, readFile, rm, stat } from 'node:fs/promises' +import { mkdtemp, readFile, stat } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { setTimeout as delay } from 'node:timers/promises' @@ -13,6 +13,7 @@ import { resolveMacOSComputerUseExecutablePath } from './macos-native-provider-paths' import { RuntimeClientError } from './runtime-client-error' +import { removeHostTree } from '../host-tree-removal' const PERMISSION_STATUS_HELPER_LAUNCH_TIMEOUT_MS = 5_000 @@ -95,7 +96,7 @@ async function readPermissionStatusFromHelperApp( } throw new RuntimeClientError('accessibility_error', 'Timed out checking permissions') } finally { - await rm(tempDir, { recursive: true, force: true }) + await removeHostTree(tempDir) } } diff --git a/src/main/computer/macos-computer-use-permissions.test.ts b/src/main/computer/macos-computer-use-permissions.test.ts index 5ac2da2d2c1d..db296502402a 100644 --- a/src/main/computer/macos-computer-use-permissions.test.ts +++ b/src/main/computer/macos-computer-use-permissions.test.ts @@ -1,5 +1,5 @@ import { execFileSync, spawn, spawnSync } from 'node:child_process' -import { mkdtemp, readFile, rm, stat } from 'node:fs/promises' +import { mkdtemp, readFile, stat } from 'node:fs/promises' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { @@ -8,6 +8,7 @@ import { } from './macos-computer-use-permissions' const resolveHelperAppPathMock = vi.hoisted(() => vi.fn()) +const removeHostTreeMock = vi.hoisted(() => vi.fn()) const resolveHelperExecutablePathMock = vi.hoisted(() => vi.fn()) const permissionStatusTempDir = '/tmp/orca-computer-use-permissions-test' const helperAppPath = '/Applications/Orca Computer Use.app' @@ -70,10 +71,11 @@ vi.mock('child_process', () => ({ vi.mock('fs/promises', () => ({ mkdtemp: vi.fn(), readFile: vi.fn(), - rm: vi.fn(), stat: vi.fn() })) +vi.mock('../host-tree-removal', () => ({ removeHostTree: removeHostTreeMock })) + vi.mock('./macos-native-provider-paths', () => ({ resolveMacOSComputerUseAppPath: resolveHelperAppPathMock, resolveMacOSComputerUseExecutablePath: resolveHelperExecutablePathMock @@ -88,7 +90,7 @@ describe('openComputerUsePermissions', () => { vi.mocked(execFileSync).mockReset() vi.mocked(mkdtemp).mockReset() vi.mocked(readFile).mockReset() - vi.mocked(rm).mockReset() + removeHostTreeMock.mockReset() vi.mocked(stat).mockReset() resolveHelperAppPathMock.mockReset() resolveHelperExecutablePathMock.mockReset() diff --git a/src/main/daemon/terminal-history-session-tombstone-retry.test.ts b/src/main/daemon/terminal-history-session-tombstone-retry.test.ts index 93a5d134ac28..0890de18a923 100644 --- a/src/main/daemon/terminal-history-session-tombstone-retry.test.ts +++ b/src/main/daemon/terminal-history-session-tombstone-retry.test.ts @@ -61,7 +61,10 @@ describe('tombstoned session tree removal retries', () => { expect(removeHostTreeMock).toHaveBeenCalledTimes( SESSION_TREE_REMOVAL_RETRY_DELAYS_MS.length + 1 ) - expect(removeHostTreeMock).toHaveBeenLastCalledWith(expect.stringContaining('.pending-delete')) + expect(removeHostTreeMock).toHaveBeenLastCalledWith( + expect.stringContaining('.pending-delete'), + { lane: 'background' } + ) }) it('does not re-arm a retry after the removal succeeds', async () => { diff --git a/src/main/daemon/terminal-history-session-tombstone.ts b/src/main/daemon/terminal-history-session-tombstone.ts index d557bdac03c7..2ede78b0fc81 100644 --- a/src/main/daemon/terminal-history-session-tombstone.ts +++ b/src/main/daemon/terminal-history-session-tombstone.ts @@ -66,7 +66,7 @@ function scheduleSessionTreeRemoval(dir: string): void { clearTimeout(pendingRetry) sessionTreeRemovalRetryTimers.delete(dir) } - const removal = removeHostTree(dir) + const removal = removeHostTree(dir, { lane: 'background' }) .then(() => { sessionTreeRemovalAttempts.delete(dir) }) diff --git a/src/main/git/repo-clone-path.ts b/src/main/git/repo-clone-path.ts index a8204e333a8d..e834a399f1cf 100644 --- a/src/main/git/repo-clone-path.ts +++ b/src/main/git/repo-clone-path.ts @@ -1,11 +1,12 @@ import { isAbsolute, join, posix, relative, resolve, sep, win32 } from 'node:path' import type { Stats } from 'node:fs' -import { lstat, mkdir, rm } from 'node:fs/promises' +import { lstat, mkdir } from 'node:fs/promises' import { isWindowsAbsolutePathLike, normalizeRuntimePathForComparison, normalizeRuntimePathSeparators } from '../../shared/cross-platform-path' +import { removeHostTree } from '../host-tree-removal' export type ClaimedCloneTarget = { canCleanup: boolean @@ -109,7 +110,7 @@ export async function cleanupClaimedCloneTarget( return } - await rm(clonePath, { recursive: true, force: true }).catch(() => { + await removeHostTree(clonePath).catch(() => { // Best-effort cleanup - do not mask the original clone failure. }) } diff --git a/src/main/github/client/create/create-github-pull-request.ts b/src/main/github/client/create/create-github-pull-request.ts index db201c88e769..e4d144546b54 100644 --- a/src/main/github/client/create/create-github-pull-request.ts +++ b/src/main/github/client/create/create-github-pull-request.ts @@ -8,7 +8,7 @@ import { normalizeHostedReviewBaseRef, normalizeHostedReviewHeadRef } from '../../../../shared/hosted-review-refs' -import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { mkdtemp, writeFile } from 'node:fs/promises' import { join } from 'node:path' import { tmpdir } from 'node:os' import { @@ -25,6 +25,7 @@ import { import { getOriginGitHubApiRepository, githubHostExecOptions } from '../../github-api-repository' import { classifyCreatePRError, parseCreatePRPayload } from './create-pr-error-classification' import { findOpenPRByHeadBase, readPullRequestTemplate } from './pull-request-template' +import { removeHostTree } from '../../../host-tree-removal' export async function createGitHubPullRequest( repoPath: string, input: CreateHostedReviewInput, @@ -162,7 +163,7 @@ export async function createGitHubPullRequest( return classified } } finally { - await rm(tempDir, { recursive: true, force: true }).catch(() => undefined) + await removeHostTree(tempDir).catch(() => undefined) release() } } diff --git a/src/main/host-tree-removal-asar.electron.test.ts b/src/main/host-tree-removal-asar.electron.test.ts index a41a55035ac5..1ba8534385a5 100644 --- a/src/main/host-tree-removal-asar.electron.test.ts +++ b/src/main/host-tree-removal-asar.electron.test.ts @@ -47,9 +47,17 @@ afterAll(() => { type ProbeResult = { failure: string | null; residue: string[] } -function buildDriver(bundlePath: string, target: string, resultPath: string): string { +function buildDriver( + bundlePath: string, + target: string, + resultPath: string, + refuseUnlink: boolean +): string { return [ `const fs = require('node:fs')`, + refuseUnlink + ? `for (const fsp of [require('original-fs').promises, fs.promises]) fsp.unlink = async () => { throw Object.assign(new Error('EBUSY: resource busy or locked'), { code: 'EBUSY' }) }` + : '', `const { removeHostTree } = require(${JSON.stringify(bundlePath)})`, // Why noAsar for the read-back: the shim would report the stranded archive as a directory here // too, so the residue listing has to be taken with real filesystem semantics. @@ -79,7 +87,7 @@ async function bundleHostTreeRemoval(outFile: string): Promise { input: 'src/main/host-tree-removal.ts', // Why mirror `isExternalMainModule` from electron.vite.config.ts exactly — CJS, and // `original-fs` deliberately *not* externalized: the shipped bundle does not list it either, - // so if the archive-aware `rm` ever became a static import (or the bundler learned to fold + // so if the archive-aware fs ever became a static import (or the bundler learned to fold // `createRequire(...)('original-fs')`) production would silently degrade to the shimmed `fs` // while a test that pre-externalized it kept passing. output: { format: 'cjs' }, @@ -103,30 +111,37 @@ function buildStrandedTree(root: string): string { } describe('removeHostTree against a tree holding an asar archive', () => { - it.runIf(FIXTURE_ASAR)( - 'removes the whole tree under the real Electron binary', - async () => { - const root = mkdtempSync(join(tmpdir(), 'orca-host-tree-asar-')) - roots.push(root) - const bundlePath = join(root, 'host-tree-removal.cjs') - await bundleHostTreeRemoval(bundlePath) - const target = buildStrandedTree(root) - const resultPath = join(root, 'result.json') - const driverPath = join(root, 'driver.cjs') - writeFileSync(driverPath, buildDriver(bundlePath, target, resultPath), 'utf8') + // Why refuse unlinks: the walk's first attempt is a plain unlink, which the shim never sees; the + // `rm` fallback it takes for a read-only or locked file on Windows is what must see through it. + for (const { name, refuseUnlink } of [ + { name: 'removes the whole tree under the real Electron binary', refuseUnlink: false }, + { name: 'removes it when every unlink falls back to rm', refuseUnlink: true } + ]) { + it.runIf(FIXTURE_ASAR)( + name, + async () => { + const root = mkdtempSync(join(tmpdir(), 'orca-host-tree-asar-')) + roots.push(root) + const bundlePath = join(root, 'host-tree-removal.cjs') + await bundleHostTreeRemoval(bundlePath) + const target = buildStrandedTree(root) + const resultPath = join(root, 'result.json') + const driverPath = join(root, 'driver.cjs') + writeFileSync(driverPath, buildDriver(bundlePath, target, resultPath, refuseUnlink), 'utf8') - const run = spawnSync(electronBinary, [driverPath], { - encoding: 'utf8', - env: { ...process.env, ELECTRON_RUN_AS_NODE: '1' }, - timeout: 60_000 - }) - expect(run.status, run.stderr?.slice(-2000)).toBe(0) + const run = spawnSync(electronBinary, [driverPath], { + encoding: 'utf8', + env: { ...process.env, ELECTRON_RUN_AS_NODE: '1' }, + timeout: 60_000 + }) + expect(run.status, run.stderr?.slice(-2000)).toBe(0) - const probe = JSON.parse(readFileSync(resultPath, 'utf8')) as ProbeResult - // Without an asar-transparent `rm` this is `ENOTEMPTY` and the residue stops at the archive, - // on every attempt, forever — it is not a race a retry can win. - expect(probe).toEqual({ failure: null, residue: [] }) - }, - 120_000 - ) + const probe = JSON.parse(readFileSync(resultPath, 'utf8')) as ProbeResult + // Without the asar-transparent fs the archive reads as a directory, the removal fails, and + // the residue stops at the archive on every attempt — it is not a race a retry can win. + expect(probe).toEqual({ failure: null, residue: [] }) + }, + 120_000 + ) + } }) diff --git a/src/main/host-tree-removal-boundary.test.ts b/src/main/host-tree-removal-boundary.test.ts new file mode 100644 index 000000000000..4ec77c592e65 --- /dev/null +++ b/src/main/host-tree-removal-boundary.test.ts @@ -0,0 +1,175 @@ +import { readFileSync, readdirSync, statSync } from 'node:fs' +import { join, relative, resolve } from 'node:path' +import { describe, expect, it } from 'vitest' + +/** + * Guard the recursive-delete chokepoint at the tree level rather than per call site. + * + * An async recursive `rm` queues every entry of the tree on libuv's shared 4-thread pool, so every + * other async fs call in the process waits behind the whole tree. A trashed worktree held chat sends + * in the main process for minutes that way. `removeHostTree` caps its share of that pool; this test is what + * stops the next call site from going around it. + */ +const OWNER_FILES = new Set(['src/main/host-tree-removal.ts', 'src/main/tree-removal-walk.ts']) +const SCANNED_ROOTS = ['src/main', 'src/shared'] +const IGNORED_DIRECTORIES = new Set([ + 'node_modules', + 'dist', + 'out', + 'build', + '.git', + '__fixtures__' +]) + +function isTestFile(path: string): boolean { + return ( + /\.(?:test|spec|bench)\.tsx?$/.test(path) || + /(?:test-harness|test-utils|test-setup|test-fixture|test-support|test-rig|fixture|repro)/.test( + path + ) || + path.includes('/__tests__/') || + path.includes('/__mocks__/') + ) +} + +function collectSourceFiles(root: string): string[] { + let found: string[] = [] + for (const entry of readdirSync(root)) { + if (IGNORED_DIRECTORIES.has(entry)) { + continue + } + const full = join(root, entry) + if (statSync(full).isDirectory()) { + found = found.concat(collectSourceFiles(full)) + } else if (full.endsWith('.ts') || full.endsWith('.tsx')) { + found.push(full) + } + } + return found +} + +/** Drop comment-only lines so prose about the old idiom is not an offender. */ +function codeText(contents: string): string { + return contents + .split('\n') + .filter((line) => !/^\s*(?:\/\/|\/\*|\*)/.test(line)) + .join('\n') +} + +function callArguments(text: string, openParen: number): string { + let depth = 0 + for (let index = openParen; index < text.length; index++) { + if (text[index] === '(') { + depth += 1 + } else if (text[index] === ')') { + depth -= 1 + if (depth === 0) { + return text.slice(openParen + 1, index) + } + } + } + return text.slice(openParen + 1) +} + +/** The call's second argument, split at the first top-level comma. */ +function optionsArgument(args: string): string { + let depth = 0 + for (let index = 0; index < args.length; index++) { + const char = args[index] + if ('([{'.includes(char)) { + depth += 1 + } else if (')]}'.includes(char)) { + depth -= 1 + } else if (char === ',' && depth === 0) { + return args.slice(index + 1).trim() + } + } + return '' +} + +function isRecursiveOptions(args: string, source: string): boolean { + const options = optionsArgument(args) + if (/recursive\s*:\s*false\b/.test(options)) { + return false + } + if (/\brecursive\b/.test(options)) { + return true + } + // Options built by a call cannot be read here, so they must prove they are not a recursive delete. + if (/^[\w$.]+\s*\(/.test(options)) { + return true + } + // Options held in a same-file constant: `const REMOVE_OPTIONS = { recursive: true, ... }`. + const identifier = /^([A-Za-z_$][\w$]*)\s*(?:,|$)/.exec(options)?.[1] + if (!identifier) { + return false + } + const declaration = new RegExp(`\\b${identifier}\\b[^=\\n]*=\\s*\\{([^}]*)\\}`).exec(source) + return declaration !== null && /recursive\s*:\s*true/.test(declaration[1]) +} + +/** Async (promise or callback) `rm`/`rmdir` calls that delete recursively. `rmSync` is not matched. */ +function findRecursiveAsyncRemovals(source: string): string[] { + const code = codeText(source) + const offenders: string[] = [] + const callPattern = /(? { + const repoRoot = resolve(__dirname, '..', '..') + const files = SCANNED_ROOTS.flatMap((root) => collectSourceFiles(join(repoRoot, root))) + .map((file) => relative(repoRoot, file).split('\\').join('/')) + .filter((path) => !isTestFile(path) && !OWNER_FILES.has(path)) + + it('scans a plausible number of files', () => { + // A broken root or extension list would make the guard silently vacuous. + expect(files.length).toBeGreaterThan(1000) + }) + + it('recognizes every recursive-delete shape and ignores the rest', () => { + expect( + findRecursiveAsyncRemovals(`await rm(dir, { recursive: true, force: true })`) + ).toHaveLength(1) + expect( + findRecursiveAsyncRemovals( + `await fs.promises.rm(\n dir,\n {\n force: true,\n recursive: true\n }\n)` + ) + ).toHaveLength(1) + expect( + findRecursiveAsyncRemovals( + `const OPTS = { recursive: true, force: true }\nawait rm(dir, OPTS)` + ) + ).toHaveLength(1) + expect( + findRecursiveAsyncRemovals(`await rm(target, { recursive: isDir, force: true })`) + ).toHaveLength(1) + expect(findRecursiveAsyncRemovals(`fs.rmdir(dir, { recursive: true }, done)`)).toHaveLength(1) + expect(findRecursiveAsyncRemovals(`await rm(dir, removalOptions())`)).toHaveLength(1) + expect(findRecursiveAsyncRemovals(`fs.rmdir(dir, done)`)).toEqual([]) + expect(findRecursiveAsyncRemovals(`await rm(join(root, name), { force: true })`)).toEqual([]) + expect(findRecursiveAsyncRemovals(`await rm(file, { force: true })`)).toEqual([]) + expect(findRecursiveAsyncRemovals(`rmSync(dir, { recursive: true, force: true })`)).toEqual([]) + expect(findRecursiveAsyncRemovals(`await removeHostTree(dir)`)).toEqual([]) + expect(findRecursiveAsyncRemovals(`// await rm(dir, { recursive: true })`)).toEqual([]) + }) + + it('routes every async recursive delete through removeHostTree', () => { + const offenders = files.flatMap((path) => + findRecursiveAsyncRemovals(readFileSync(join(repoRoot, path), 'utf8')).map( + (call) => `${path}: ${call}` + ) + ) + expect( + offenders, + 'Async recursive rm floods the shared fs thread pool. Use removeHostTree from src/main/host-tree-removal.' + ).toEqual([]) + }) +}) diff --git a/src/main/host-tree-removal.test.ts b/src/main/host-tree-removal.test.ts new file mode 100644 index 000000000000..1181cb8d05d0 --- /dev/null +++ b/src/main/host-tree-removal.test.ts @@ -0,0 +1,62 @@ +import { existsSync, mkdirSync, mkdtempSync, writeFileSync } from 'node:fs' +import { stat } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { performance } from 'node:perf_hooks' +import { join } from 'node:path' +import { afterAll, describe, expect, it } from 'vitest' +import { removeTreeSync } from '../shared/windows-transient-lock-removal' +import { removeHostTree } from './host-tree-removal' + +const roots: string[] = [] + +afterAll(() => { + for (const root of roots) { + removeTreeSync(root) + } +}) + +function buildTree(root: string, directories: number, filesPerDirectory: number): string { + const target = join(root, 'wt-1700000000000-abcdef01') + for (let d = 0; d < directories; d++) { + const directory = join(target, `pkg-${d}`, 'lib') + mkdirSync(directory, { recursive: true }) + for (let f = 0; f < filesPerDirectory; f++) { + writeFileSync(join(directory, `${f}.js`), 'x') + } + } + return target +} + +describe('removeHostTree', () => { + it('removes the whole tree, and treats an already-missing tree as removed', async () => { + const root = mkdtempSync(join(tmpdir(), 'orca-host-tree-')) + roots.push(root) + const target = buildTree(root, 3, 3) + + await removeHostTree(target) + + expect(existsSync(target)).toBe(false) + expect(existsSync(root)).toBe(true) + await expect(removeHostTree(target)).resolves.toBeUndefined() + }) + + // A pool-backed recursive rm queued every entry ahead of this chain, so the agent-session store + // (and every other async fs caller) waited for the entire tree. + it('leaves the async fs pool free for other callers while a large tree is deleted', async () => { + const root = mkdtempSync(join(tmpdir(), 'orca-host-tree-')) + roots.push(root) + const target = buildTree(root, 40, 50) + const started = performance.now() + const removal = removeHostTree(target).then(() => performance.now() - started) + + for (let i = 0; i < 20; i++) { + await stat(root) + } + const chainMs = performance.now() - started + + // Why a ratio and not "finished first": queued behind the tree, the chain ends within a + // millisecond of the delete, so ordering alone is a coin flip. + expect(chainMs).toBeLessThan((await removal) / 2) + expect(existsSync(target)).toBe(false) + }, 60_000) +}) diff --git a/src/main/host-tree-removal.ts b/src/main/host-tree-removal.ts index f789a9861d06..feb57fb36d6f 100644 --- a/src/main/host-tree-removal.ts +++ b/src/main/host-tree-removal.ts @@ -1,12 +1,13 @@ // Why: every recursive host delete Orca performs (worktrees, terminal history, quarantined recovery -// generations) hits the same two hazards, so one helper exists so no call site forgets either. +// generations) hits the same three hazards, so one helper exists so no call site forgets any. // Windows stickiness — AV/indexers/late handle releases surface transient EBUSY/ENOTEMPTY/EPERM on a -// tree Node just emptied — and Electron's asar shim, which strands any tree holding a `*.asar` -// (see `asar-transparent-fs`). +// tree Node just emptied. Electron's asar shim, which strands any tree holding a `*.asar` (see +// `asar-transparent-fs`). And the async fs thread pool, which a worktree's ~100k entries held for +// minutes, stalling chat sends behind the agent-session store (see `tree-removal-walk`). import { win32 } from 'node:path' import { setTimeout as delay } from 'node:timers/promises' -import { rm } from './asar-transparent-fs' +import { removeTreeWithBoundedFsCalls, type TreeRemovalLane } from './tree-removal-walk' import { isWindowsAbsolutePathLike } from '../shared/cross-platform-path' import { isWslUncPath } from '../shared/wsl-paths' import { transientLockRemovalOptions } from '../shared/windows-transient-lock-removal' @@ -43,7 +44,10 @@ function isTransientWindowsRemovalError(error: unknown): boolean { } /** Recursively remove a host directory tree, retrying the transient Windows failures. */ -export async function removeHostTree(targetPath: string): Promise { +export async function removeHostTree( + targetPath: string, + { lane = 'interactive' }: { lane?: TreeRemovalLane } = {} +): 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 @@ -53,7 +57,7 @@ export async function removeHostTree(targetPath: string): Promise { while (true) { try { - await rm(removalPath, rmOptions) + await removeTreeWithBoundedFsCalls(removalPath, rmOptions, lane) return } catch (error) { if (attempt >= retryDelays.length || !isTransientWindowsRemovalError(error)) { diff --git a/src/main/ipc/filesystem-download-folder.ts b/src/main/ipc/filesystem-download-folder.ts index 60e915956ef2..2b0f5295a55b 100644 --- a/src/main/ipc/filesystem-download-folder.ts +++ b/src/main/ipc/filesystem-download-folder.ts @@ -1,12 +1,13 @@ import { BrowserWindow, dialog, ipcMain } from 'electron' import { randomUUID } from 'node:crypto' -import { rm, stat } from 'node:fs/promises' +import { stat } from 'node:fs/promises' import { dirname, join } from 'node:path' import { getRuntimePathBasename } from '../../shared/cross-platform-path' import { sanitizeLocalDownloadFilename } from '../local-download-filename' import { promoteLocalDownloadedFolder } from '../local-downloaded-folder-promotion' import { requireSshFilesystemProvider } from '../providers/ssh-filesystem-dispatch' import { isENOENT } from './filesystem-path-containment' +import { removeHostTree } from '../host-tree-removal' type DownloadFolderResult = { canceled: true } | { canceled: false; destinationPath: string } @@ -37,7 +38,7 @@ async function assertDownloadFolderDestinationAvailable(destinationPath: string) async function cleanupLocalTransferDirectory(dirPath: string): Promise { try { - await rm(dirPath, { recursive: true, force: true }) + await removeHostTree(dirPath) } catch (error) { // Why: cleanup must not mask the transfer error, but a leaked recursive // download tree needs enough visibility to diagnose and remove it. diff --git a/src/main/ipc/filesystem-download-transfers.test.ts b/src/main/ipc/filesystem-download-transfers.test.ts index 227372c567d3..bed4ef8a2fa6 100644 --- a/src/main/ipc/filesystem-download-transfers.test.ts +++ b/src/main/ipc/filesystem-download-transfers.test.ts @@ -11,6 +11,7 @@ import { openMock, renameMock, rmMock, + removeHostTreeMock, getSshFilesystemProviderMock, promoteLocalDownloadedFolderMock, resetFilesystemIpcMocks @@ -18,6 +19,10 @@ import { vi.mock('electron', async () => (await import('./filesystem-test-harness')).electronMock) vi.mock('fs/promises', async () => (await import('./filesystem-test-harness')).fsPromisesMock) +vi.mock( + '../host-tree-removal', + async () => (await import('./filesystem-test-harness')).hostTreeRemovalMock +) vi.mock( '../wsl-unc-delete', async () => (await import('./filesystem-test-harness')).wslUncDeleteMock @@ -572,7 +577,7 @@ describe('registerFilesystemHandlers', () => { const tempPath = provider.downloadFolder.mock.calls[0][1] expect(promoteLocalDownloadedFolderMock).not.toHaveBeenCalled() - expect(rmMock).toHaveBeenCalledWith(tempPath, { recursive: true, force: true }) + expect(removeHostTreeMock).toHaveBeenCalledWith(tempPath) }) it('logs a recursive temporary-folder cleanup failure without masking the transfer error', async () => { @@ -584,7 +589,7 @@ describe('registerFilesystemHandlers', () => { getSshFilesystemProviderMock.mockReturnValue(provider) showOpenDialogMock.mockResolvedValue({ canceled: false, filePaths: ['/downloads'] }) statMock.mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' })) - rmMock.mockRejectedValueOnce(cleanupError) + removeHostTreeMock.mockRejectedValueOnce(cleanupError) registerFilesystemHandlers(store as never) try { @@ -631,7 +636,7 @@ describe('registerFilesystemHandlers', () => { await expect(result).rejects.toThrow('window closed') const tempPath = provider.downloadFolder.mock.calls[0][1] expect(promoteLocalDownloadedFolderMock).not.toHaveBeenCalled() - expect(rmMock).toHaveBeenCalledWith(tempPath, { recursive: true, force: true }) + expect(removeHostTreeMock).toHaveBeenCalledWith(tempPath) expect(folderDownloadSender.listenerCount('destroyed')).toBe(0) }) }) diff --git a/src/main/ipc/filesystem-import-local-rollback.test.ts b/src/main/ipc/filesystem-import-local-rollback.test.ts index 364c6c88eee6..931d43a8cdc4 100644 --- a/src/main/ipc/filesystem-import-local-rollback.test.ts +++ b/src/main/ipc/filesystem-import-local-rollback.test.ts @@ -3,14 +3,16 @@ import { join, resolve } from 'node:path' import { Readable, Writable } from 'node:stream' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { lstatMock, mkdirMock, openMock, readdirMock, rmMock, unlinkMock } = vi.hoisted(() => ({ - lstatMock: vi.fn(), - mkdirMock: vi.fn(), - openMock: vi.fn(), - readdirMock: vi.fn(), - rmMock: vi.fn(), - unlinkMock: vi.fn() -})) +const { lstatMock, mkdirMock, openMock, readdirMock, removeHostTreeMock, unlinkMock } = vi.hoisted( + () => ({ + lstatMock: vi.fn(), + mkdirMock: vi.fn(), + openMock: vi.fn(), + readdirMock: vi.fn(), + removeHostTreeMock: vi.fn(), + unlinkMock: vi.fn() + }) +) vi.mock('./filesystem-auth', () => ({ authorizeExternalPath: vi.fn() })) vi.mock('node:fs/promises', () => ({ @@ -18,9 +20,9 @@ vi.mock('node:fs/promises', () => ({ mkdir: mkdirMock, open: openMock, readdir: readdirMock, - rm: rmMock, unlink: unlinkMock })) +vi.mock('../host-tree-removal', () => ({ removeHostTree: removeHostTreeMock })) import { importOneSource } from './filesystem-import-local' import { recursiveCopyDir } from './filesystem-import-local-tree-copy' @@ -67,7 +69,7 @@ beforeEach(() => { (entries.get(path) ?? []).map(({ name, kind }) => ({ name, ...statFor(kind) })) ) mkdirMock.mockResolvedValue(undefined) - rmMock.mockResolvedValue(undefined) + removeHostTreeMock.mockResolvedValue(undefined) unlinkMock.mockResolvedValue(undefined) openMock.mockRejectedValue(new Error('unexpected file copy')) }) @@ -84,7 +86,7 @@ describe('local directory import rollback ownership', () => { reason: code }) expect(mkdirMock).toHaveBeenCalledExactlyOnceWith(target, { recursive: false }) - expect(rmMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() expect(openMock).not.toHaveBeenCalled() } ) @@ -106,7 +108,7 @@ describe('local directory import rollback ownership', () => { expect(mkdirMock).toHaveBeenCalledExactlyOnceWith(join(destination, 'incoming copy'), { recursive: false }) - expect(rmMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() } ) @@ -117,19 +119,19 @@ describe('local directory import rollback ownership', () => { status: 'failed', reason: 'source unreadable' }) - expect(rmMock).toHaveBeenCalledExactlyOnceWith(target, { recursive: true, force: true }) + expect(removeHostTreeMock).toHaveBeenCalledExactlyOnceWith(target) }) it('keeps the copy error when best-effort cleanup also fails', async () => { addEntry(source, 'lost.txt', 'file') openMock.mockRejectedValue(new Error('source changed')) - rmMock.mockRejectedValue(new Error('cleanup denied')) + removeHostTreeMock.mockRejectedValue(new Error('cleanup denied')) expect(await importOneSource(source, destination, new Set())).toMatchObject({ status: 'failed', reason: 'source changed' }) - expect(rmMock).toHaveBeenCalledExactlyOnceWith(target, { recursive: true, force: true }) + expect(removeHostTreeMock).toHaveBeenCalledExactlyOnceWith(target) }) it('rolls back the owned root once when a nested mkdir fails', async () => { @@ -144,7 +146,7 @@ describe('local directory import rollback ownership', () => { [target, { recursive: false }], [join(target, 'child'), { recursive: false }] ]) - expect(rmMock).toHaveBeenCalledExactlyOnceWith(target, { recursive: true, force: true }) + expect(removeHostTreeMock).toHaveBeenCalledExactlyOnceWith(target) }) it.each(['symlink', 'unsupported', 'missing'])( @@ -162,7 +164,7 @@ describe('local directory import rollback ownership', () => { status: 'failed' }) expect(mkdirMock).toHaveBeenCalledTimes(2) - expect(rmMock).toHaveBeenCalledExactlyOnceWith(target, { recursive: true, force: true }) + expect(removeHostTreeMock).toHaveBeenCalledExactlyOnceWith(target) expect(openMock).not.toHaveBeenCalled() } ) @@ -181,7 +183,7 @@ describe('local directory import rollback ownership', () => { reason: 'symlink' }) expect(mkdirMock).not.toHaveBeenCalled() - expect(rmMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() } ) @@ -229,7 +231,7 @@ describe('local directory import rollback ownership', () => { expect(Buffer.concat(chunks).toString()).toBe('payload') expect(closeSource).toHaveBeenCalledOnce() expect(closeDestination).toHaveBeenCalledOnce() - expect(rmMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() expect(unlinkMock).not.toHaveBeenCalled() }) @@ -240,7 +242,7 @@ describe('local directory import rollback ownership', () => { status: 'failed' }) expect(mkdirMock).not.toHaveBeenCalled() - expect(rmMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() expect(unlinkMock).not.toHaveBeenCalled() }) @@ -249,6 +251,6 @@ describe('local directory import rollback ownership', () => { readdirMock.mockRejectedValue(failure) await expect(recursiveCopyDir(source, target)).rejects.toBe(failure) - expect(rmMock).toHaveBeenCalledExactlyOnceWith(target, { recursive: true, force: true }) + expect(removeHostTreeMock).toHaveBeenCalledExactlyOnceWith(target) }) }) diff --git a/src/main/ipc/filesystem-import-local-tree-copy.ts b/src/main/ipc/filesystem-import-local-tree-copy.ts index 06d68368ce91..53a01f650cdb 100644 --- a/src/main/ipc/filesystem-import-local-tree-copy.ts +++ b/src/main/ipc/filesystem-import-local-tree-copy.ts @@ -1,7 +1,8 @@ import { constants } from 'node:fs' -import { lstat, mkdir, open, readdir, rm, unlink } from 'node:fs/promises' +import { lstat, mkdir, open, readdir, unlink } from 'node:fs/promises' import { basename, join } from 'node:path' import { pipeline } from 'node:stream/promises' +import { removeHostTree } from '../host-tree-removal' /** * Pre-scan a directory tree for symlinks. Returns true if any symlink @@ -29,7 +30,7 @@ export async function recursiveCopyDir(srcDir: string, destDir: string): Promise try { await copyDirectoryContents(srcDir, destDir) } catch (error) { - await rm(destDir, { recursive: true, force: true }).catch(() => {}) + await removeHostTree(destDir).catch(() => {}) throw error } } diff --git a/src/main/ipc/filesystem-import.test.ts b/src/main/ipc/filesystem-import.test.ts index 7703cd8a8b1b..c330f0dbd955 100644 --- a/src/main/ipc/filesystem-import.test.ts +++ b/src/main/ipc/filesystem-import.test.ts @@ -14,6 +14,7 @@ const { readFileMock, readdirMock, rmMock, + removeHostTreeMock, unlinkMock } = vi.hoisted(() => ({ handleMock: vi.fn(), @@ -25,6 +26,7 @@ const { readFileMock: vi.fn(), readdirMock: vi.fn(), rmMock: vi.fn(), + removeHostTreeMock: vi.fn(), unlinkMock: vi.fn() })) @@ -46,6 +48,8 @@ vi.mock('fs/promises', () => ({ unlink: unlinkMock })) +vi.mock('../host-tree-removal', () => ({ removeHostTree: removeHostTreeMock })) + import { registerFilesystemMutationHandlers } from './filesystem-mutations' const REPO_PATH = path.resolve('/workspace/repo') @@ -164,6 +168,7 @@ describe('fs:importExternalPaths', () => { readFileMock.mockReset() readdirMock.mockReset() rmMock.mockReset() + removeHostTreeMock.mockReset() unlinkMock.mockReset() handleMock.mockImplementation((channel: string, handler: never) => { @@ -178,6 +183,7 @@ describe('fs:importExternalPaths', () => { readFileMock.mockResolvedValue(Buffer.from('file-content')) readdirMock.mockResolvedValue([]) rmMock.mockResolvedValue(undefined) + removeHostTreeMock.mockResolvedValue(undefined) unlinkMock.mockResolvedValue(undefined) registerFilesystemMutationHandlers(store as never) @@ -420,10 +426,7 @@ describe('fs:importExternalPaths', () => { reason: "Symlink not allowed in 'normal.txt'" }) expect(openMock).not.toHaveBeenCalledWith(childPath, expect.anything()) - expect(rmMock).toHaveBeenCalledWith(path.join(destDir, 'mixeddir'), { - recursive: true, - force: true - }) + expect(removeHostTreeMock).toHaveBeenCalledWith(path.join(destDir, 'mixeddir')) }) it('rejects unauthorized destinations', async () => { diff --git a/src/main/ipc/filesystem-test-harness.ts b/src/main/ipc/filesystem-test-harness.ts index a3a06b95e85e..8eab0d9ce84a 100644 --- a/src/main/ipc/filesystem-test-harness.ts +++ b/src/main/ipc/filesystem-test-harness.ts @@ -24,6 +24,7 @@ export const statMock: IpcMock = vi.fn() export const openMock: IpcMock = vi.fn() export const renameMock: IpcMock = vi.fn() export const rmMock: IpcMock = vi.fn() +export const removeHostTreeMock: IpcMock = vi.fn() export const realpathMock: IpcMock = vi.fn() export const lstatMock: IpcMock = vi.fn() export const commitChangesMock: IpcMock = vi.fn() @@ -80,6 +81,8 @@ export const fsPromisesMock = { export const wslUncDeleteMock = { tryDeleteWslUncPath: tryDeleteWslUncPathMock } +export const hostTreeRemovalMock = { removeHostTree: removeHostTreeMock } + export const crashBreadcrumbMock = { recordCrashBreadcrumb: recordCrashBreadcrumbMock } export const folderPromotionMock = { @@ -223,6 +226,7 @@ function collectMocks(moduleMock: Record): IpcMock[] { const ALL_MOCKS = [ electronMock, fsPromisesMock, + hostTreeRemovalMock, wslUncDeleteMock, crashBreadcrumbMock, folderPromotionMock, @@ -272,6 +276,7 @@ export function resetFilesystemIpcMocks(): void { statMock.mockResolvedValue({ size: 10, isDirectory: () => false, mtimeMs: 123 }) renameMock.mockResolvedValue(undefined) rmMock.mockResolvedValue(undefined) + removeHostTreeMock.mockResolvedValue(undefined) openMock.mockResolvedValue({ read: vi.fn(async (buffer: Buffer) => { buffer.fill(0x61) diff --git a/src/main/ipc/pet-bundle-import.ts b/src/main/ipc/pet-bundle-import.ts index a9e897298b1c..582da10ed875 100644 --- a/src/main/ipc/pet-bundle-import.ts +++ b/src/main/ipc/pet-bundle-import.ts @@ -1,5 +1,5 @@ import { BrowserWindow, dialog } from 'electron' -import { mkdir, readFile, rename, rm, stat } from 'node:fs/promises' +import { mkdir, readFile, rename, stat } from 'node:fs/promises' import { randomUUID } from 'node:crypto' import { basename, dirname, join } from 'node:path' import type { CustomPet } from '../../shared/pet-types' @@ -10,6 +10,7 @@ import { resolveBundleSpritesheetSource } from './pet-bundle-spritesheet-source' import { MAX_MANIFEST_BYTES } from './pet-import-size-limits' import { getPetsDir } from './pet-storage-paths' import { copyFileNoFollow, isSymlink } from './pet-symlink-safe-copy' +import { removeHostTree } from '../host-tree-removal' export async function importPetBundle( event: Electron.IpcMainInvokeEvent @@ -76,13 +77,13 @@ export async function importPetBundle( // Why: stage into a sibling .tmp then atomically rename, so a mid-copy failure can't leave a half-imported bundle. const tmpDir = `${destDir}.tmp` try { - await rm(tmpDir, { recursive: true, force: true }).catch(() => {}) + await removeHostTree(tmpDir).catch(() => {}) await mkdir(tmpDir, { recursive: true }) await copyFileNoFollow(sheetSrc, join(tmpDir, sheetFileName)) await copyFileNoFollow(manifestPath, join(tmpDir, 'pet.json')) await rename(tmpDir, destDir) } catch { - await rm(tmpDir, { recursive: true, force: true }).catch(() => {}) + await removeHostTree(tmpDir).catch(() => {}) throw new Error('Could not save the pet bundle.') } diff --git a/src/main/ipc/pet.ts b/src/main/ipc/pet.ts index 3521482ef41d..72cf43ade228 100644 --- a/src/main/ipc/pet.ts +++ b/src/main/ipc/pet.ts @@ -8,6 +8,7 @@ import { importPetBundle } from './pet-bundle-import' import { classifyFile } from './pet-image-formats' import { MAX_BYTES } from './pet-import-size-limits' import { getPetsDir, isSafeId, resolvePetFile } from './pet-storage-paths' +import { removeHostTree } from '../host-tree-removal' // Why: renderer IPC inputs are untrusted — validate shape here; resolvePetFile still gates the actual filesystem path. const PetFileRequestSchema = z.object({ @@ -136,7 +137,7 @@ export function registerPetHandlers(): void { return } try { - await rm(target, { recursive: true, force: true }) + await removeHostTree(target) } catch (error) { console.warn('[pet-overlay] pet:delete (bundle) failed', error) } diff --git a/src/main/ipc/repos-create.test.ts b/src/main/ipc/repos-create.test.ts index cc9081ce2a22..89eacaeda64c 100644 --- a/src/main/ipc/repos-create.test.ts +++ b/src/main/ipc/repos-create.test.ts @@ -20,7 +20,7 @@ const { mkdirMock, accessMock, readdirMock, - rmMock, + removeHostTreeMock, gitExecFileAsyncMock, homedirMock, invalidateAuthorizedRootsCacheMock, @@ -39,7 +39,7 @@ const { mkdirMock: vi.fn(), accessMock: vi.fn(), readdirMock: vi.fn(), - rmMock: vi.fn(), + removeHostTreeMock: vi.fn(), gitExecFileAsyncMock: vi.fn(), homedirMock: vi.fn(), invalidateAuthorizedRootsCacheMock: vi.fn(), @@ -57,10 +57,11 @@ vi.mock('electron', () => ({ vi.mock('fs/promises', () => ({ mkdir: mkdirMock, access: accessMock, - readdir: readdirMock, - rm: rmMock + readdir: readdirMock })) +vi.mock('../host-tree-removal', () => ({ removeHostTree: removeHostTreeMock })) + // `availableParallelism` is read at module load by the git admission scheduler, // which this module graph reaches; a partial `os` mock breaks that import. vi.mock('os', () => ({ @@ -147,7 +148,7 @@ describe('repos:create', () => { accessMock.mockReset().mockRejectedValue(new Error('ENOENT')) readdirMock.mockReset().mockResolvedValue([]) mkdirMock.mockReset().mockResolvedValue(undefined) - rmMock.mockReset().mockResolvedValue(undefined) + removeHostTreeMock.mockReset().mockResolvedValue(undefined) gitExecFileAsyncMock.mockReset().mockResolvedValue({ stdout: '', stderr: '' }) homedirMock.mockReset().mockReturnValue('/Users/alice') @@ -359,7 +360,7 @@ describe('repos:create', () => { const result = await callCreate({ parentPath: '/tmp', name: 'broken', kind: 'git' }) - expect(rmMock).toHaveBeenCalledWith(tmpPath('broken'), { recursive: true, force: true }) + expect(removeHostTreeMock).toHaveBeenCalledWith(tmpPath('broken')) expect(mockStore.addRepo).not.toHaveBeenCalled() expect(result).toMatchObject({ error: expect.stringContaining('Failed to initialize') }) }) @@ -372,7 +373,7 @@ describe('repos:create', () => { const result = await callCreate({ parentPath: '/tmp', name: 'preexisting', kind: 'git' }) - expect(rmMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() expect(mockStore.addRepo).not.toHaveBeenCalled() expect(result).toMatchObject({ error: expect.stringContaining('Failed to initialize') }) }) @@ -383,7 +384,7 @@ describe('repos:create', () => { const result = await callCreate({ parentPath: '/tmp', name: 'initfail', kind: 'git' }) - expect(rmMock).toHaveBeenCalledWith(tmpPath('initfail'), { recursive: true, force: true }) + expect(removeHostTreeMock).toHaveBeenCalledWith(tmpPath('initfail')) expect(mockStore.addRepo).not.toHaveBeenCalled() // Loose match — handler distinguishes init vs commit failures, and we want // to tolerate small wording tweaks as long as it still mentions "initialize". @@ -400,7 +401,7 @@ describe('repos:create', () => { const result = await callCreate({ parentPath: '/tmp', name: 'commitfail', kind: 'git' }) - expect(rmMock).toHaveBeenCalledWith(tmpPath('commitfail'), { recursive: true, force: true }) + expect(removeHostTreeMock).toHaveBeenCalledWith(tmpPath('commitfail')) expect(mockStore.addRepo).not.toHaveBeenCalled() expect(result).toMatchObject({ error: expect.stringContaining('commit') }) }) @@ -418,14 +419,8 @@ describe('repos:create', () => { const result = await callCreate({ parentPath: '/tmp', name: 'pre-existing', kind: 'git' }) - expect(rmMock).toHaveBeenCalledWith(tmpPath('pre-existing', '.git'), { - recursive: true, - force: true - }) - expect(rmMock).not.toHaveBeenCalledWith(tmpPath('pre-existing'), { - recursive: true, - force: true - }) + expect(removeHostTreeMock).toHaveBeenCalledWith(tmpPath('pre-existing', '.git')) + expect(removeHostTreeMock).not.toHaveBeenCalledWith(tmpPath('pre-existing')) expect(mockStore.addRepo).not.toHaveBeenCalled() expect(result).toMatchObject({ error: expect.stringContaining('commit') }) }) @@ -442,7 +437,7 @@ describe('repos:create', () => { const result = await callCreate({ parentPath: '/tmp', name: 'authorless', kind: 'git' }) - expect(rmMock).toHaveBeenCalledWith(tmpPath('authorless'), { recursive: true, force: true }) + expect(removeHostTreeMock).toHaveBeenCalledWith(tmpPath('authorless')) expect(mockStore.addRepo).not.toHaveBeenCalled() expect(result).toMatchObject({ error: expect.stringContaining('Git author identity is not configured') diff --git a/src/main/ipc/repos/repo-creation-handlers.ts b/src/main/ipc/repos/repo-creation-handlers.ts index 57bfcf66c79b..2292039dcd6c 100644 --- a/src/main/ipc/repos/repo-creation-handlers.ts +++ b/src/main/ipc/repos/repo-creation-handlers.ts @@ -1,7 +1,7 @@ import type { BrowserWindow } from 'electron' import { ipcMain } from 'electron' import { randomUUID } from 'node:crypto' -import { access, mkdir, readdir, rm } from 'node:fs/promises' +import { access, mkdir, readdir } from 'node:fs/promises' import { homedir } from 'node:os' import { isAbsolute, join } from 'node:path' import type { Store } from '../../persistence' @@ -20,6 +20,7 @@ import { notifyReposChanged } from './repos-changed-notification' import { addLocalRepoFromPath } from './local-repo-registration' import { addRemoteRepoFromPath } from './remote-repo-registration' import { createRemoteRepo } from './remote-repo-creation' +import { removeHostTree } from '../../host-tree-removal' const GIT_AVAILABILITY_TIMEOUT_MS = 1500 @@ -231,9 +232,9 @@ export function registerRepoCreationHandlers(mainWindow: BrowserWindow, store: S } catch (err) { // Only rm the dir if we made it (pre-existing folders must survive retry); otherwise strip just the .git/ that git init created. if (createdDir) { - await rm(targetPath, { recursive: true, force: true }).catch(() => {}) + await removeHostTree(targetPath).catch(() => {}) } else if (step === 'commit') { - await rm(join(targetPath, '.git'), { recursive: true, force: true }).catch(() => {}) + await removeHostTree(join(targetPath, '.git')).catch(() => {}) } const message = err instanceof Error ? err.message : String(err) if ( diff --git a/src/main/ipc/shallow-watch-delivery-probe-lifetime.test.ts b/src/main/ipc/shallow-watch-delivery-probe-lifetime.test.ts index 6ca703305183..ee40c6475cdd 100644 --- a/src/main/ipc/shallow-watch-delivery-probe-lifetime.test.ts +++ b/src/main/ipc/shallow-watch-delivery-probe-lifetime.test.ts @@ -14,9 +14,9 @@ const h = vi.hoisted(() => ({ vi.mock('node:fs', () => ({ watch: h.watch })) vi.mock('node:fs/promises', () => ({ mkdtemp: h.mkdir, - writeFile: h.write, - rm: h.remove + writeFile: h.write })) +vi.mock('../host-tree-removal', () => ({ removeHostTree: h.remove })) import { detectShallowWatchDelivery, @@ -51,10 +51,7 @@ afterEach(() => { function expectReleased(): void { expect(h.close).toHaveBeenCalledOnce() expect(vi.getTimerCount()).toBe(0) - expect(h.remove).toHaveBeenCalledExactlyOnceWith('/fake-shallow-probe', { - recursive: true, - force: true - }) + expect(h.remove).toHaveBeenCalledExactlyOnceWith('/fake-shallow-probe') } describe('shallow probe resource ownership', () => { diff --git a/src/main/ipc/shallow-watch-delivery-probe.ts b/src/main/ipc/shallow-watch-delivery-probe.ts index 09970e4c0828..2643bf4a354d 100644 --- a/src/main/ipc/shallow-watch-delivery-probe.ts +++ b/src/main/ipc/shallow-watch-delivery-probe.ts @@ -1,7 +1,8 @@ import { watch } from 'node:fs' -import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { mkdtemp, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' +import { removeHostTree } from '../host-tree-removal' // Why: fs.watch reports success and then delivers nothing on hosts where the // platform notification path is broken — observed on a macOS 26.3.1 machine @@ -39,7 +40,7 @@ export async function measureShallowWatchDelivery(timeoutMs = PROBE_TIMEOUT_MS): return false } finally { if (directory) { - await rm(directory, { recursive: true, force: true }).catch(() => {}) + await removeHostTree(directory).catch(() => {}) } } } diff --git a/src/main/ipc/terminal-render-desync-evidence.ts b/src/main/ipc/terminal-render-desync-evidence.ts index dc536faf1f85..33f5f2a746fa 100644 --- a/src/main/ipc/terminal-render-desync-evidence.ts +++ b/src/main/ipc/terminal-render-desync-evidence.ts @@ -1,4 +1,4 @@ -import { mkdir, readdir, rm, stat, writeFile } from 'node:fs/promises' +import { mkdir, readdir, stat, writeFile } from 'node:fs/promises' import path from 'node:path' import { app, ipcMain } from 'electron' import { @@ -7,6 +7,7 @@ import { type WriteTerminalRenderDesyncEvidenceResult } from '../../shared/terminal-render-desync-evidence' import { isTrustedUIRenderer } from './ui' +import { removeHostTree } from '../host-tree-removal' const EVIDENCE_DIRECTORY = 'terminal-render-desync-evidence' const MAX_PNG_DATA_URL_BYTES = 40 * 1024 * 1024 @@ -105,7 +106,7 @@ async function pruneRenderDesyncEvidence(root: string, currentDirectory: string) const removableIndex = captures.findIndex((capture) => capture.directory !== currentDirectory) const index = Math.max(removableIndex, 0) const [capture] = captures.splice(index, 1) - await rm(capture.directory, { recursive: true, force: true }) + await removeHostTree(capture.directory) totalBytes -= capture.bytes if (capture.directory === currentDirectory) { throw new Error('Render-desync evidence exceeds the aggregate storage budget') diff --git a/src/main/local-downloaded-folder-promotion.ts b/src/main/local-downloaded-folder-promotion.ts index f5ba45f7a5c4..b87967826b6b 100644 --- a/src/main/local-downloaded-folder-promotion.ts +++ b/src/main/local-downloaded-folder-promotion.ts @@ -1,8 +1,9 @@ import { fstatSync, lstatSync } from 'node:fs' -import { link, lstat, mkdir, open, readdir, rm, rmdir, unlink } from 'node:fs/promises' +import { link, lstat, mkdir, open, readdir, rmdir, unlink } from 'node:fs/promises' import type { BigIntStats } from 'node:fs' import type { FileHandle } from 'node:fs/promises' import { join } from 'node:path' +import { removeHostTree } from './host-tree-removal' const LOCAL_COPY_CHUNK_BYTES = 1024 * 1024 @@ -264,5 +265,5 @@ export async function promoteLocalDownloadedFolder( } throw error } - await rm(tempPath, { recursive: true, force: true }).catch(() => {}) + await removeHostTree(tempPath).catch(() => {}) } diff --git a/src/main/local-worktree-filesystem.test.ts b/src/main/local-worktree-filesystem.test.ts index d69d82b914fc..1c9c4031f9d3 100644 --- a/src/main/local-worktree-filesystem.test.ts +++ b/src/main/local-worktree-filesystem.test.ts @@ -1,10 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -const { runProcessMock, lstatMock, readFileMock, rmMock } = vi.hoisted(() => ({ +const { runProcessMock, lstatMock, readFileMock, removeTreeMock } = vi.hoisted(() => ({ runProcessMock: vi.fn(), lstatMock: vi.fn(), readFileMock: vi.fn(), - rmMock: vi.fn() + removeTreeMock: vi.fn() })) // Why mock the chokepoint: encoding, timeout and the hidden console are its @@ -15,8 +15,11 @@ vi.mock('../shared/child-process/run-process', () => ({ vi.mock('node:fs/promises', () => ({ lstat: lstatMock, - readFile: readFileMock, - rm: rmMock + readFile: readFileMock +})) + +vi.mock('./tree-removal-walk', () => ({ + removeTreeWithBoundedFsCalls: removeTreeMock })) import { @@ -56,7 +59,7 @@ describe('local worktree filesystem runtime access', () => { runProcessMock.mockReset() lstatMock.mockReset() readFileMock.mockReset() - rmMock.mockReset() + removeTreeMock.mockReset() completeExecFile() }) @@ -76,12 +79,13 @@ describe('local worktree filesystem runtime access', () => { expect(lstatMock).toHaveBeenCalledWith('C:\\repo\\.git') expect(readFileMock).toHaveBeenCalledWith('C:\\repo\\.git', 'utf8') - expect(rmMock).toHaveBeenCalledWith( + expect(removeTreeMock).toHaveBeenCalledWith( toHostRemovalPath('C:\\repo\\feature'), expect.objectContaining({ recursive: true, force: true - }) + }), + 'interactive' ) expect(runProcessMock).not.toHaveBeenCalled() }) @@ -93,14 +97,15 @@ describe('local worktree filesystem runtime access', () => { await removeLocalWorktreePath(longPath) expect(toHostRemovalPath(longPath)).toBe(`\\\\?\\${longPath}`) - expect(rmMock).toHaveBeenCalledWith( + expect(removeTreeMock).toHaveBeenCalledWith( `\\\\?\\${longPath}`, expect.objectContaining({ recursive: true, force: true, maxRetries: expect.any(Number), retryDelay: expect.any(Number) - }) + }), + 'interactive' ) }) }) @@ -130,14 +135,14 @@ describe('local worktree filesystem runtime access', () => { vi.useFakeTimers() await withPlatform('win32', async () => { const error = Object.assign(new Error('Directory not empty'), { code: 'ENOTEMPTY' }) - rmMock.mockRejectedValueOnce(error).mockResolvedValueOnce(undefined) + removeTreeMock.mockRejectedValueOnce(error).mockResolvedValueOnce(undefined) const removal = removeLocalWorktreePath('C:\\repo\\feature') await vi.advanceTimersByTimeAsync(250) await expect(removal).resolves.toBeUndefined() - expect(rmMock).toHaveBeenCalledTimes(2) - expect(rmMock).toHaveBeenNthCalledWith( + expect(removeTreeMock).toHaveBeenCalledTimes(2) + expect(removeTreeMock).toHaveBeenNthCalledWith( 1, toHostRemovalPath('C:\\repo\\feature'), expect.objectContaining({ @@ -145,9 +150,10 @@ describe('local worktree filesystem runtime access', () => { force: true, maxRetries: expect.any(Number), retryDelay: expect.any(Number) - }) + }), + 'interactive' ) - expect(rmMock).toHaveBeenNthCalledWith( + expect(removeTreeMock).toHaveBeenNthCalledWith( 2, toHostRemovalPath('C:\\repo\\feature'), expect.objectContaining({ @@ -155,7 +161,8 @@ describe('local worktree filesystem runtime access', () => { force: true, maxRetries: expect.any(Number), retryDelay: expect.any(Number) - }) + }), + 'interactive' ) }) }) @@ -163,10 +170,10 @@ describe('local worktree filesystem runtime access', () => { it('does not retry host removal failures outside Windows', async () => { await withPlatform('linux', async () => { const error = Object.assign(new Error('Directory not empty'), { code: 'ENOTEMPTY' }) - rmMock.mockRejectedValue(error) + removeTreeMock.mockRejectedValue(error) await expect(removeLocalWorktreePath('/repo/feature')).rejects.toBe(error) - expect(rmMock).toHaveBeenCalledTimes(1) + expect(removeTreeMock).toHaveBeenCalledTimes(1) }) }) @@ -199,7 +206,7 @@ describe('local worktree filesystem runtime access', () => { const removeArgs = runProcessMock.mock.calls[2]?.[0].args as string[] expect(removeArgs.at(-1)).toContain('rm -rf --') expect(removeArgs.at(-1)).toContain(String.raw`rm -rf -- '/mnt/c/Users/me/repo feature'`) - expect(rmMock).not.toHaveBeenCalled() + expect(removeTreeMock).not.toHaveBeenCalled() }) }) diff --git a/src/main/orcad/electron-serve-browser-process.ts b/src/main/orcad/electron-serve-browser-process.ts index ae3508289c99..0cfed8677964 100644 --- a/src/main/orcad/electron-serve-browser-process.ts +++ b/src/main/orcad/electron-serve-browser-process.ts @@ -1,4 +1,4 @@ -import { mkdtemp, rm } from 'node:fs/promises' +import { mkdtemp } from 'node:fs/promises' import { createServer, type AddressInfo } from 'node:net' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -23,6 +23,7 @@ import { electronSidecarRuntimeMethodName, TARGETLESS_BROWSER_METHODS } from './electron-sidecar-method-routing' +import { removeHostTree } from '../host-tree-removal' const START_TIMEOUT_MS = 120_000 const STOP_TIMEOUT_MS = 5_000 @@ -185,7 +186,7 @@ export class ElectronServeBrowserProcess { } } if (sidecarDataPath) { - await rm(sidecarDataPath, { recursive: true, force: true }) + await removeHostTree(sidecarDataPath) } } diff --git a/src/main/orcad/orcad-bun-native-preflight.test.ts b/src/main/orcad/orcad-bun-native-preflight.test.ts index 3e6df8402c1f..6b060dd9c867 100644 --- a/src/main/orcad/orcad-bun-native-preflight.test.ts +++ b/src/main/orcad/orcad-bun-native-preflight.test.ts @@ -26,9 +26,9 @@ vi.mock('../windows/windows-process-table', () => ({ })) vi.mock('node:fs/promises', () => ({ mkdtemp: fixture.temp, - writeFile: fixture.write, - rm: fixture.remove + writeFile: fixture.write })) +vi.mock('../host-tree-removal', () => ({ removeHostTree: fixture.remove })) vi.mock('../ipc/parcel-watcher-process-supervisor', () => ({ WatcherProcessSupervisor: class { subscribe = fixture.subscribe @@ -89,7 +89,7 @@ describe('bundled native readiness', () => { expect(fixture.pty).toHaveBeenCalledOnce() expect(fixture.unsubscribe).toHaveBeenCalledOnce() expect(fixture.dispose).toHaveBeenCalledOnce() - expect(fixture.remove).toHaveBeenCalledWith('/temp/probe', { recursive: true, force: true }) + expect(fixture.remove).toHaveBeenCalledWith('/temp/probe') expect(vi.getTimerCount()).toBe(0) }) diff --git a/src/main/orcad/orcad-bun-native-preflight.ts b/src/main/orcad/orcad-bun-native-preflight.ts index 163f6ce29877..c258d0991152 100644 --- a/src/main/orcad/orcad-bun-native-preflight.ts +++ b/src/main/orcad/orcad-bun-native-preflight.ts @@ -1,4 +1,4 @@ -import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { mkdtemp, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { runPtySpawnHealthProbe } from '../daemon/pty-subprocess/spawn-preflight' @@ -10,6 +10,7 @@ import { isWindowsProcessStartTimeAvailable, readWindowsProcessIdentityTableFresh } from '../windows/windows-process-table' +import { removeHostTree } from '../host-tree-removal' /** The candidate process owns disposable PTY and watcher probes before it touches user state. */ export async function preflightOrcadBunNativeRuntime( @@ -65,7 +66,7 @@ export async function preflightOrcadBunNativeRuntime( await subscription?.unsubscribe() } finally { supervisor.dispose() - await rm(directory, { recursive: true, force: true }) + await removeHostTree(directory) } } } diff --git a/src/main/persistence/profile-state/profile-state-runtime-preflight.ts b/src/main/persistence/profile-state/profile-state-runtime-preflight.ts index 90a8ab1fab13..06a6141eda1e 100644 --- a/src/main/persistence/profile-state/profile-state-runtime-preflight.ts +++ b/src/main/persistence/profile-state/profile-state-runtime-preflight.ts @@ -1,4 +1,4 @@ -import { mkdtemp, rm } from 'node:fs/promises' +import { mkdtemp } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { randomUUID } from 'node:crypto' @@ -9,6 +9,7 @@ import { import { readProfileStateSnapshot } from './profile-state-documents' import { runProfileStateBackupWorker } from './profile-state-backup-worker' import { ProfileStateWriteWorkerClient } from './profile-state-writer-worker-client' +import { removeHostTree } from '../../host-tree-removal' export type ProfileStateRuntimePreflightResult = { sqliteVersion: string @@ -61,7 +62,7 @@ export async function preflightProfileStateRuntime( try { await writer?.close() } finally { - await rm(directory, { recursive: true, force: true }) + await removeHostTree(directory) } } } diff --git a/src/main/plugins/plugin-install-publication.ts b/src/main/plugins/plugin-install-publication.ts index 291afa76fc7f..86a82731ce0f 100644 --- a/src/main/plugins/plugin-install-publication.ts +++ b/src/main/plugins/plugin-install-publication.ts @@ -1,4 +1,4 @@ -import { readdir, rm } from 'node:fs/promises' +import { readdir } from 'node:fs/promises' import { join } from 'node:path' import { PLUGIN_CONTENT_HASH_PATTERN, @@ -16,6 +16,7 @@ import { readPluginInstallProvenance, writePluginInstallProvenance } from './plugin-install-provenance' +import { removeHostTree } from '../host-tree-removal' /** Publishes executable identity and provenance as one recoverable mutation, * then retains only current plus one rollback version. */ @@ -88,7 +89,7 @@ async function pruneHistoricalVersions(pluginDir: string, retained: ReadonlySet< PLUGIN_CONTENT_HASH_PATTERN.test(entry.name) && !retained.has(entry.name) ) - .map((entry) => rm(join(pluginDir, entry.name), { recursive: true, force: true })) + .map((entry) => removeHostTree(join(pluginDir, entry.name))) ) await prunePluginInstallProvenance(pluginDir, retained) } diff --git a/src/main/plugins/plugin-install-staging.ts b/src/main/plugins/plugin-install-staging.ts index 2f0a71fe7a04..1699c54d3ed5 100644 --- a/src/main/plugins/plugin-install-staging.ts +++ b/src/main/plugins/plugin-install-staging.ts @@ -1,5 +1,5 @@ import { existsSync } from 'node:fs' -import { cp, mkdir, rm } from 'node:fs/promises' +import { cp, mkdir } from 'node:fs/promises' import { join, relative, resolve, sep } from 'node:path' import { PLUGIN_MANIFEST_FILENAME, @@ -23,6 +23,7 @@ import { hashPluginTree } from './plugin-content-hash' import { publishPluginInstall } from './plugin-install-publication' import { readPluginManifestText } from './plugin-manifest-file' import { pluginInstallTrustError } from './plugin-install-trust' +import { removeHostTree } from '../host-tree-removal' export type PluginInstallResult = | { @@ -151,13 +152,13 @@ export async function installStagedPluginTree(input: { if (!existingHash.ok || existingHash.hash !== sourceInspection.contentHash) { // Why: bundled resources are release-index verified above, so they can // safely restore a damaged immutable install instead of staying broken. - await rm(versionDir, { recursive: true, force: true }) + await removeHostTree(versionDir) } } if (!existsSync(versionDir)) { const stagedVersionDir = `${versionDir}.staging` try { - await rm(stagedVersionDir, { recursive: true, force: true }) + await removeHostTree(stagedVersionDir) await mkdir(pluginDir, { recursive: true }) // Source trees can change while copying. Hashing the destination closes // that race before the immutable directory becomes current. @@ -197,7 +198,7 @@ export async function installStagedPluginTree(input: { } catch (error) { return { ok: false, error: error instanceof Error ? error.message : String(error) } } finally { - await rm(stagedVersionDir, { recursive: true, force: true }) + await removeHostTree(stagedVersionDir) } } else { // Never repoint at an existing hash directory without proving its bytes; diff --git a/src/main/plugins/plugin-install.ts b/src/main/plugins/plugin-install.ts index c5f7a782d78c..d51a06f2f250 100644 --- a/src/main/plugins/plugin-install.ts +++ b/src/main/plugins/plugin-install.ts @@ -1,4 +1,4 @@ -import { mkdtemp, readdir, realpath, rm } from 'node:fs/promises' +import { mkdtemp, readdir, realpath } from 'node:fs/promises' import { existsSync } from 'node:fs' import { tmpdir } from 'node:os' import { isAbsolute, join, relative, resolve, sep } from 'node:path' @@ -23,6 +23,7 @@ import { checkoutPluginGitSource } from './plugin-git-repository' import { readPluginCurrentPointer } from './plugin-current-pointer' import { readPluginInstallProvenance } from './plugin-install-provenance' import { publishPluginInstall } from './plugin-install-publication' +import { removeHostTree } from '../host-tree-removal' export type { PluginInstallResult } from './plugin-install-staging' @@ -139,7 +140,7 @@ export async function installPluginFromGit(input: { } catch (error) { return { ok: false, error: error instanceof Error ? error.message : String(error) } } finally { - await rm(stagingDir, { recursive: true, force: true }) + await removeHostTree(stagingDir) } }) } @@ -188,7 +189,7 @@ export async function installPluginFromMarketplace(input: { } catch (error) { return { ok: false, error: error instanceof Error ? error.message : String(error) } } finally { - await rm(stagingDir, { recursive: true, force: true }) + await removeHostTree(stagingDir) } }) } @@ -319,5 +320,5 @@ async function removeResolvedPluginDirectory(rootDir: string, pluginKey: string) ) { throw new Error(`refusing to remove plugin path outside ${rootReal}`) } - await rm(resolve(rootDir, pluginKey), { recursive: true, force: true }) + await removeHostTree(resolve(rootDir, pluginKey)) } diff --git a/src/main/plugins/plugin-marketplace-fetch.ts b/src/main/plugins/plugin-marketplace-fetch.ts index d035841eb841..1512ec4252d0 100644 --- a/src/main/plugins/plugin-marketplace-fetch.ts +++ b/src/main/plugins/plugin-marketplace-fetch.ts @@ -1,5 +1,5 @@ import { createReadStream } from 'node:fs' -import { mkdtemp, rm } from 'node:fs/promises' +import { mkdtemp } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { @@ -9,6 +9,7 @@ import { } from '../../shared/plugins/plugin-marketplace' import { checkoutPluginGitSource } from './plugin-git-repository' import type { PluginMarketplaceRegisteredSource } from './plugin-marketplace-store' +import { removeHostTree } from '../host-tree-removal' const MARKETPLACE_INDEX_MAX_BYTES = 16 * 1024 * 1024 @@ -33,7 +34,7 @@ export async function fetchPluginMarketplace( const marketplace = await readPluginMarketplaceIndex(stagingDirectory) return { marketplaceCommit, marketplace } } finally { - await rm(stagingDirectory, { recursive: true, force: true }) + await removeHostTree(stagingDirectory) } } diff --git a/src/main/plugins/plugin-marketplace-installer.ts b/src/main/plugins/plugin-marketplace-installer.ts index 179e687682ce..a14d9977697a 100644 --- a/src/main/plugins/plugin-marketplace-installer.ts +++ b/src/main/plugins/plugin-marketplace-installer.ts @@ -1,4 +1,4 @@ -import { mkdtemp, rm } from 'node:fs/promises' +import { mkdtemp } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import type { PluginManifest } from '../../shared/plugins/plugin-manifest' @@ -16,6 +16,7 @@ import type { PluginMarketplaceService } from './plugin-marketplace-service' import { marketplaceSourceId } from './plugin-marketplace-store' +import { removeHostTree } from '../host-tree-removal' export type PluginMarketplaceInstallPreview = { marketplaceSourceId: string @@ -93,7 +94,7 @@ export class PluginMarketplaceInstaller { ...(listing.blockedByKillList ? { blockedByKillList: listing.blockedByKillList } : {}) } } finally { - await rm(stagingDirectory, { recursive: true, force: true }) + await removeHostTree(stagingDirectory) } } diff --git a/src/main/runtime/runtime-file-commands-create-file-explorer-dir-no-clobber.ts b/src/main/runtime/runtime-file-commands-create-file-explorer-dir-no-clobber.ts index eddafdcdabcd..0f5538f17357 100644 --- a/src/main/runtime/runtime-file-commands-create-file-explorer-dir-no-clobber.ts +++ b/src/main/runtime/runtime-file-commands-create-file-explorer-dir-no-clobber.ts @@ -6,6 +6,7 @@ import { resolveAuthorizedPath } from '../ipc/filesystem-auth' import { constants, copyFile, mkdir, rm } from 'node:fs/promises' import { dirname } from 'node:path' import { renameLocalPathSerializedByDestination } from '../destination-serialized-local-rename' +import { removeHostTree } from '../host-tree-removal' export class RuntimeFileCommandsWithCreateFileExplorerDirNoClobber extends RuntimeFileCommandsWithWriteFileExplorerFile { async createFileExplorerDirNoClobber( @@ -160,7 +161,7 @@ export class RuntimeFileCommandsWithCreateFileExplorerDirNoClobber extends Runti preserveSymlink: true }) // Why: a non-local runtime has no client Trash; this delete is permanent, so the renderer confirms before calling. - await rm(targetPath, { recursive: recursive === true, force: true }) + await (recursive === true ? removeHostTree(targetPath) : rm(targetPath, { force: true })) return { ok: true } } } diff --git a/src/main/runtime/runtime-repository-registration-controller.ts b/src/main/runtime/runtime-repository-registration-controller.ts index cc64e20a9acb..850263bb35e0 100644 --- a/src/main/runtime/runtime-repository-registration-controller.ts +++ b/src/main/runtime/runtime-repository-registration-controller.ts @@ -1,5 +1,5 @@ import { randomUUID } from 'node:crypto' -import { mkdir, readdir, rm, stat } from 'node:fs/promises' +import { mkdir, readdir, stat } from 'node:fs/promises' import { isAbsolute, join } from 'node:path' import { DEFAULT_REPO_BADGE_COLOR } from '../../shared/constants' import { @@ -16,6 +16,7 @@ import { prepareLocalWorktreeRootForRepo } from '../worktree-root-preparation' import type { RuntimeStore } from './runtime-store-contract' import { runtimePathsEqual } from './runtime-worktree-path-identity' import { runtimeRepoMatchesExecutionHost } from './runtime-worktree-selection' +import { removeHostTree } from '../host-tree-removal' type RuntimeRepositoryRegistrationDependencies = { getStore: () => RuntimeStore | null @@ -181,9 +182,9 @@ export class RuntimeRepositoryRegistrationController { return null } catch (error) { if (createdDir) { - await rm(targetPath, { recursive: true, force: true }).catch(() => {}) + await removeHostTree(targetPath).catch(() => {}) } else if (step === 'commit') { - await rm(join(targetPath, '.git'), { recursive: true, force: true }).catch(() => {}) + await removeHostTree(join(targetPath, '.git')).catch(() => {}) } const message = error instanceof Error ? error.message : String(error) if (step === 'commit' && /Please tell me who you are|user\.name|user\.email/i.test(message)) { diff --git a/src/main/skills/skill-bundle-creation.ts b/src/main/skills/skill-bundle-creation.ts index b8a549425a54..c88e00ede2f5 100644 --- a/src/main/skills/skill-bundle-creation.ts +++ b/src/main/skills/skill-bundle-creation.ts @@ -27,6 +27,7 @@ import { } from './skill-package-identity' import { writeSkillTarGzip, type SkillTarWriteEntry } from './skill-package-tar' import { startSkillPhaseOperation } from './skill-operation-observability' +import { removeHostTree } from '../host-tree-removal' export type SkillBundleSource = { id?: string @@ -236,7 +237,7 @@ async function createSkillBundleArchiveUnobserved( await renameSkillPathWithWindowsRetry(temporaryArchive, input.archivePath) return { pluginManifest, manifest, archivePath: input.archivePath, ...archiveIdentity } } finally { - await rm(workDirectory, { recursive: true, force: true }) + await removeHostTree(workDirectory) await rm(temporaryArchive, { force: true }).catch(() => undefined) } } diff --git a/src/main/skills/skill-bundle-extraction.ts b/src/main/skills/skill-bundle-extraction.ts index 44405aef3dc3..a1106b0f7873 100644 --- a/src/main/skills/skill-bundle-extraction.ts +++ b/src/main/skills/skill-bundle-extraction.ts @@ -1,5 +1,5 @@ import { createHash } from 'node:crypto' -import { mkdir, open, readFile, rm } from 'node:fs/promises' +import { mkdir, open, readFile } from 'node:fs/promises' import { dirname, join } from 'node:path' import { AGENT_PLUGIN_MANIFEST_PATH, @@ -23,6 +23,7 @@ import { type TarByteReader } from './skill-package-tar' import { SkillInstallOperationError } from './skill-install-operation-error' +import { removeHostTree } from '../host-tree-removal' export type SkillBundleExtractionResult = { pluginManifest: AgentPluginManifestV1 @@ -256,7 +257,7 @@ export async function extractSkillBundleArchive(input: { archive.abort(failure) await archive.archiveIdentity.catch(() => undefined) if (destinationCreated) { - await rm(input.destinationDirectory, { recursive: true, force: true }) + await removeHostTree(input.destinationDirectory) } throw failure } diff --git a/src/main/skills/skill-install-filesystem.ts b/src/main/skills/skill-install-filesystem.ts index 123d500fe929..4095fa7a807a 100644 --- a/src/main/skills/skill-install-filesystem.ts +++ b/src/main/skills/skill-install-filesystem.ts @@ -1,5 +1,5 @@ import type { Dirent, Stats } from 'node:fs' -import { lstat, readdir, realpath, rm, stat } from 'node:fs/promises' +import { lstat, readdir, realpath, stat } from 'node:fs/promises' import type { SkillPackageManifestV1 } from '../../shared/skill-package-manifest' import { renameSkillPathWithWindowsRetry } from './skill-filesystem-retry' import { runSkillCandidateTasks } from './skill-candidate-concurrency' @@ -8,6 +8,7 @@ import { observeSkillPackage, type ObservedSkillPackage } from './skill-package-identity' +import { removeHostTree } from '../host-tree-removal' export type SkillInstalledFileMode = { path: string; executable: boolean } @@ -63,7 +64,7 @@ export const nativeSkillInstallFilesystem: SkillInstallFilesystem = { : undefined ), rename: renameSkillPathWithWindowsRetry, - remove: (path) => rm(path, { recursive: true, force: true }), + remove: (path) => removeHostTree(path), // Why the bounded pool rather than `Promise.all`: a delete plan enumerates // every discovery root, and unbounded fan-out here is the same burst of // filesystem-metadata work discovery already learned to cap. diff --git a/src/main/skills/skill-package-creation.ts b/src/main/skills/skill-package-creation.ts index 527f447031aa..ee0b38c60bed 100644 --- a/src/main/skills/skill-package-creation.ts +++ b/src/main/skills/skill-package-creation.ts @@ -18,6 +18,7 @@ import { } from './skill-package-identity' import { extractSkillPackageArchive } from './skill-package-extraction' import { writeSkillTarGzip, type SkillTarWriteEntry } from './skill-package-tar' +import { removeHostTree } from '../host-tree-removal' export type CreatedSkillPackage = { manifest: SkillPackageManifestV1 @@ -129,7 +130,7 @@ async function createSkillPackageArchiveUnobserved( await renameSkillPathWithWindowsRetry(temporaryArchive, input.archivePath) return { manifest, archivePath: input.archivePath, ...archiveIdentity } } finally { - await rm(workDirectory, { recursive: true, force: true }) + await removeHostTree(workDirectory) await rm(temporaryArchive, { force: true }).catch(() => undefined) } } diff --git a/src/main/skills/skill-package-download.ts b/src/main/skills/skill-package-download.ts index 595b568eeff7..a28e52263dd7 100644 --- a/src/main/skills/skill-package-download.ts +++ b/src/main/skills/skill-package-download.ts @@ -1,5 +1,5 @@ import { createHash, randomUUID, timingSafeEqual } from 'node:crypto' -import { chmod, mkdir, mkdtemp, open, readdir, rm } from 'node:fs/promises' +import { chmod, mkdir, mkdtemp, open, readdir } from 'node:fs/promises' import { join } from 'node:path' import { SKILL_PACKAGE_CONTENT_TYPE, @@ -11,6 +11,7 @@ import { throwIfSkillDownloadUnavailable } from './skill-package-download-availability' import { startSkillPhaseOperation } from './skill-operation-observability' +import { removeHostTree } from '../host-tree-removal' const REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]) const MAX_REDIRECTS = 3 @@ -146,7 +147,7 @@ async function prepareTemporaryRoot(path: string): Promise { : null const pid = Number(match?.[1]) if (match && Number.isSafeInteger(pid) && !processIsAlive(pid)) { - await rm(join(path, entry.name), { recursive: true, force: true }) + await removeHostTree(join(path, entry.name)) } }) ) @@ -269,10 +270,10 @@ async function downloadSkillPackageGrantUnobserved( archivePath, archiveSha256, compressedBytes, - cleanup: () => rm(temporaryDirectory, { recursive: true, force: true }) + cleanup: () => removeHostTree(temporaryDirectory) } } catch (error) { - await rm(temporaryDirectory, { recursive: true, force: true }) + await removeHostTree(temporaryDirectory) throw error } } finally { diff --git a/src/main/skills/skill-package-extraction.ts b/src/main/skills/skill-package-extraction.ts index e3935b71b11b..786d70f4f744 100644 --- a/src/main/skills/skill-package-extraction.ts +++ b/src/main/skills/skill-package-extraction.ts @@ -1,5 +1,5 @@ import { createHash } from 'node:crypto' -import { mkdir, open, readFile, rm } from 'node:fs/promises' +import { mkdir, open, readFile } from 'node:fs/promises' import { dirname, join } from 'node:path' import { SKILL_PACKAGE_MAX_MANIFEST_BYTES, @@ -21,6 +21,7 @@ import { } from './skill-install-filesystem' import { SKILL_INSTALL_CANCELLED_FAILURE } from '../../shared/skill-install-failure' import { SkillInstallOperationError } from './skill-install-operation-error' +import { removeHostTree } from '../host-tree-removal' export type SkillPackageExtractionResult = { manifest: SkillPackageManifestV1 @@ -227,7 +228,7 @@ export async function extractSkillPackageArchive(input: { archive.abort(failure) await archive.archiveIdentity.catch(() => undefined) if (destinationCreated) { - await rm(input.destinationDirectory, { recursive: true, force: true }) + await removeHostTree(input.destinationDirectory) } throw failure } diff --git a/src/main/skills/skill-share-preparation-service.ts b/src/main/skills/skill-share-preparation-service.ts index 7f7780469be3..86e9a03df130 100644 --- a/src/main/skills/skill-share-preparation-service.ts +++ b/src/main/skills/skill-share-preparation-service.ts @@ -1,5 +1,5 @@ import { randomUUID } from 'node:crypto' -import { mkdir, realpath, rm } from 'node:fs/promises' +import { mkdir, realpath } from 'node:fs/promises' import { join } from 'node:path' import type { SkillSharePreview, @@ -11,6 +11,7 @@ import type { SkillCloudVersion } from '../../shared/skill-cloud-contract' import { createSkillBundleArchive, type CreatedSkillBundle } from './skill-bundle-creation' import type { SkillCloudService } from './skill-cloud-service' import { readSkillInstallReceipt } from './skill-install-provenance' +import { removeHostTree } from '../host-tree-removal' const PREPARATION_TTL_MS = 30 * 60 * 1000 const MAX_PREPARATIONS = 8 @@ -141,7 +142,7 @@ export class SkillSharePreparationService { this.preparations.set(preparationId, value) return preview(preparationId, value) } catch (error) { - await rm(directory, { recursive: true, force: true }) + await removeHostTree(directory) throw error } finally { this.preparingCount -= 1 @@ -216,7 +217,7 @@ export class SkillSharePreparationService { const preparation = this.preparations.get(preparationId) preparation?.controller?.abort() this.preparations.delete(preparationId) - await rm(join(this.root, preparationId), { recursive: true, force: true }) + await removeHostTree(join(this.root, preparationId)) } async dispose(): Promise { @@ -224,7 +225,7 @@ export class SkillSharePreparationService { preparation.controller?.abort() } this.preparations.clear() - await rm(this.root, { recursive: true, force: true }) + await removeHostTree(this.root) } private async prune(): Promise { @@ -240,7 +241,7 @@ export class SkillSharePreparationService { if (this.options.initializeRoot) { await this.options.initializeRoot() } else { - await rm(this.root, { recursive: true, force: true }) + await removeHostTree(this.root) await mkdir(this.root, { recursive: true, mode: 0o700 }) } })() diff --git a/src/main/skills/skill-upload-staging-ownership.ts b/src/main/skills/skill-upload-staging-ownership.ts index cc630b004e02..f04020e4114c 100644 --- a/src/main/skills/skill-upload-staging-ownership.ts +++ b/src/main/skills/skill-upload-staging-ownership.ts @@ -1,6 +1,7 @@ import { randomUUID } from 'node:crypto' -import { lstat, mkdir, opendir, rm } from 'node:fs/promises' +import { lstat, mkdir, opendir } from 'node:fs/promises' import { join } from 'node:path' +import { removeHostTree } from '../host-tree-removal' const OWNER_DIRECTORY_PREFIX = 'owner-' const OWNER_DIRECTORY_PATTERN = @@ -39,7 +40,7 @@ export class SkillUploadStagingOwnership { // Callers race this (an in-flight operation and disposal), and a second rmdir of a // delete-pending directory fails with EPERM on Windows, so join one removal instead. async remove(): Promise { - const removal = (this.removal ??= rm(this.directory, { recursive: true, force: true })) + const removal = (this.removal ??= removeHostTree(this.directory)) try { await removal } catch (error) { @@ -66,7 +67,7 @@ export class SkillUploadStagingOwnership { const candidate = join(this.root, entry.name) const stats = await lstat(candidate).catch(() => null) if (stats?.isDirectory() && !stats.isSymbolicLink()) { - await rm(candidate, { recursive: true, force: true }) + await removeHostTree(candidate) } } } finally { diff --git a/src/main/speech/model-manager.ts b/src/main/speech/model-manager.ts index 8d3cf1bb1def..e6f04e378b0e 100644 --- a/src/main/speech/model-manager.ts +++ b/src/main/speech/model-manager.ts @@ -19,6 +19,7 @@ import { removeModelDownloadFiles, removeModelDownloadStaging } from './speech-model-download-cleanup' +import { removeHostTree } from '../host-tree-removal' type DownloadHandle = { abort: () => void @@ -205,7 +206,7 @@ export class ModelManager extends SpeechModelDownloadTransport { return } - await rm(modelDir, { recursive: true, force: true }) + await removeHostTree(modelDir) await rename(stagingDir, modelDir) this.updateState(modelId, 'ready') } catch (err) { @@ -244,15 +245,15 @@ export class ModelManager extends SpeechModelDownloadTransport { this.cancelDownload(modelId) const modelDir = this.getModelDir(modelId) if (existsSync(modelDir)) { - await rm(modelDir, { recursive: true, force: true }) + await removeHostTree(modelDir) } - await rm(`${modelDir}.partial`, { recursive: true, force: true }) + await removeHostTree(`${modelDir}.partial`) await rm(join(this.modelsDir, `${modelId}.tar.bz2`), { force: true }) // Why: also delete the pre-migration copy, or the next launch re-migrates it and resurrects the model. if (this.migrationSourceDir) { const sourceModelDir = this.getSafeModelDir(modelId, this.migrationSourceDir) if (existsSync(sourceModelDir)) { - await rm(sourceModelDir, { recursive: true, force: true }) + await removeHostTree(sourceModelDir) } } this.modelStates.delete(modelId) diff --git a/src/main/ssh/orcad-artifact-materializer.ts b/src/main/ssh/orcad-artifact-materializer.ts index 2efe902b5042..22104303e4ed 100644 --- a/src/main/ssh/orcad-artifact-materializer.ts +++ b/src/main/ssh/orcad-artifact-materializer.ts @@ -1,6 +1,6 @@ import { createHash, randomUUID } from 'node:crypto' import { createReadStream, existsSync } from 'node:fs' -import { chmod, copyFile, mkdir, readFile, rename, rm, writeFile } from 'node:fs/promises' +import { chmod, copyFile, mkdir, readFile, rename, writeFile } from 'node:fs/promises' import { dirname, join } from 'node:path' import { z } from 'zod' import { getAppEnvironment } from '../../shared/app-environment' @@ -25,6 +25,7 @@ import { verifyFileSha256, type OrcadBunRuntimeMaterializeOptions } from './orcad-bun-runtime-materializer' +import { removeHostTree } from '../host-tree-removal' const TemplateTargetSchema = z .object({ @@ -134,7 +135,7 @@ export async function assembleOrcadArtifact(args: { } return targetDir } finally { - await rm(stagingDir, { recursive: true, force: true }) + await removeHostTree(stagingDir) } } diff --git a/src/main/ssh/orcad-bun-runtime-materializer.ts b/src/main/ssh/orcad-bun-runtime-materializer.ts index e52a87534eb1..7c09fea612df 100644 --- a/src/main/ssh/orcad-bun-runtime-materializer.ts +++ b/src/main/ssh/orcad-bun-runtime-materializer.ts @@ -1,6 +1,6 @@ import { createHash, randomUUID } from 'node:crypto' import { createReadStream, readdirSync } from 'node:fs' -import { chmod, link, mkdir, open, rm } from 'node:fs/promises' +import { chmod, link, mkdir, open } from 'node:fs/promises' import { basename, join } from 'node:path' import { runProcess, type ProcessResult } from '../../shared/child-process/run-process' import { waitForPromiseWithSignal } from '../../shared/abort-signal-reason' @@ -15,6 +15,7 @@ import { orcadBunReleaseUrl, type OrcadBunTarget } from '../../shared/orcad-bun-runtime' +import { removeHostTree } from '../host-tree-removal' const MAX_BUN_ARCHIVE_BYTES = 200 * 1024 * 1024 @@ -81,7 +82,7 @@ export async function materializeCachedOrcadBunRuntime( await verifyFileSha256(runtimePath, asset.executableSha256, `${target} cached Bun executable`) return runtimePath } finally { - await rm(temporaryDir, { recursive: true, force: true }) + await removeHostTree(temporaryDir) } } diff --git a/src/main/ssh/ssh-relay-opencode-runtime.ts b/src/main/ssh/ssh-relay-opencode-runtime.ts index 83657e8b7f29..6031563a1f60 100644 --- a/src/main/ssh/ssh-relay-opencode-runtime.ts +++ b/src/main/ssh/ssh-relay-opencode-runtime.ts @@ -1,5 +1,5 @@ import { randomBytes } from 'node:crypto' -import { copyFile, link, mkdtemp, rm } from 'node:fs/promises' +import { copyFile, link, mkdtemp } from 'node:fs/promises' import { dirname, join } from 'node:path' import { getAppEnvironment } from '../../shared/app-environment' import { waitForPromiseWithSignal } from '../../shared/abort-signal-reason' @@ -30,6 +30,7 @@ import { promoteOpenCodeRuntimeCommand, publishOpenCodeRuntimeReferenceCommand } from './ssh-relay-opencode-runtime-commands' +import { removeHostTree } from '../host-tree-removal' const SETUP_TIMEOUT_MS = 180_000 export type RemoteOpenCodeRuntimeOutcome = @@ -243,7 +244,7 @@ async function install( } executable = promoted.executable } finally { - await rm(localStage, { recursive: true, force: true }).catch(() => {}) + await removeHostTree(localStage).catch(() => {}) } } const referenceName = 'opencode-sqlite-runtime.json' diff --git a/src/main/ssh/system-ssh-file-binary-transfer.ts b/src/main/ssh/system-ssh-file-binary-transfer.ts index 149d10dfbd36..180905382254 100644 --- a/src/main/ssh/system-ssh-file-binary-transfer.ts +++ b/src/main/ssh/system-ssh-file-binary-transfer.ts @@ -1,5 +1,5 @@ import { constants, createWriteStream } from 'node:fs' -import { lstat, mkdtemp, open, rm, writeFile } from 'node:fs/promises' +import { lstat, mkdtemp, open, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import type { Writable } from 'node:stream' @@ -22,6 +22,7 @@ import { writeWindowsRemoteFile, type WindowsWriteSource } from './system-ssh-windows-write-strategy' +import { removeHostTree } from '../host-tree-removal' export { WINDOWS_STDIN_WRITE_CHUNK_BYTES, @@ -233,7 +234,7 @@ async function withTemporaryLocalFile( await writeFile(localPath, contents, { mode: 0o600 }) return await send(localPath) } finally { - await rm(directory, { recursive: true, force: true }).catch(() => {}) + await removeHostTree(directory).catch(() => {}) } } diff --git a/src/main/terminal-history-deletion.ts b/src/main/terminal-history-deletion.ts index 97650a78ac34..4698b30ff802 100644 --- a/src/main/terminal-history-deletion.ts +++ b/src/main/terminal-history-deletion.ts @@ -217,7 +217,8 @@ function scheduleHistoryTreeRemoval(dir: string, wslDistro?: string): void { ) }) : null - const removal = (cleanup ? cleanup.then(() => removeHostTree(dir)) : removeHostTree(dir)) + const removeTree = (): Promise => removeHostTree(dir, { lane: 'background' }) + const removal = (cleanup ? cleanup.then(removeTree) : removeTree()) .then(() => { removalSucceeded = true historyTreeRemovalAttempts.delete(dir) diff --git a/src/main/terminal-history-gc.test.ts b/src/main/terminal-history-gc.test.ts index ece0d9af25f2..91f318b3dcda 100644 --- a/src/main/terminal-history-gc.test.ts +++ b/src/main/terminal-history-gc.test.ts @@ -275,7 +275,10 @@ describe('history GC prune decisions', () => { await runHistoryGc(new Set([LIVE_WORKTREE_ID])) - expect(removeHostTreeMock).toHaveBeenCalledWith(expect.stringContaining('abc123.1700000000000')) + expect(removeHostTreeMock).toHaveBeenCalledWith( + expect.stringContaining('abc123.1700000000000'), + { lane: 'background' } + ) }) it('continues the pass after one orphan tombstone fails', async () => { diff --git a/src/main/terminal-history-tombstone-retry.test.ts b/src/main/terminal-history-tombstone-retry.test.ts index cbfb10cc2b75..d2c4e565f86e 100644 --- a/src/main/terminal-history-tombstone-retry.test.ts +++ b/src/main/terminal-history-tombstone-retry.test.ts @@ -86,7 +86,10 @@ describe('tombstoned history removal retries', () => { expect(removeHostTreeMock).toHaveBeenCalledTimes( HISTORY_TREE_REMOVAL_RETRY_DELAYS_MS.length + 1 ) - expect(removeHostTreeMock).toHaveBeenLastCalledWith(expect.stringContaining('.pending-delete')) + expect(removeHostTreeMock).toHaveBeenLastCalledWith( + expect.stringContaining('.pending-delete'), + { lane: 'background' } + ) }) it('does not re-arm a retry after the removal succeeds', async () => { diff --git a/src/main/terminal-history.test.ts b/src/main/terminal-history.test.ts index c5ce990f32ca..83b1b0e4e4d6 100644 --- a/src/main/terminal-history.test.ts +++ b/src/main/terminal-history.test.ts @@ -17,7 +17,7 @@ const { fstatSyncMock, closeSyncMock, getPathMock, - rmAsyncMock, + removeHostTreeMock, deleteWslFishHistoryFileMock } = vi.hoisted(() => ({ existsSyncMock: vi.fn(), @@ -39,7 +39,7 @@ const { })), closeSyncMock: vi.fn(), getPathMock: vi.fn(), - rmAsyncMock: vi.fn(async () => undefined), + removeHostTreeMock: vi.fn(async () => undefined), deleteWslFishHistoryFileMock: vi.fn(async () => undefined) })) @@ -62,11 +62,12 @@ vi.mock('fs', () => ({ // transitive readFile/mkdir would otherwise resolve to undefined. vi.mock('node:fs/promises', async () => ({ ...(await vi.importActual('node:fs/promises')), - rm: rmAsyncMock, // The tombstone drain lists pending-delete asynchronously; same fixture as the sync reads. readdir: async (path: string) => readdirSyncMock(path) ?? [] })) +vi.mock('./host-tree-removal', () => ({ removeHostTree: removeHostTreeMock })) + const { parseWslPathMock, toLinuxPathMock } = vi.hoisted(() => ({ parseWslPathMock: vi.fn((_path: string) => null as { distro: string; linuxPath: string } | null), toLinuxPathMock: vi.fn((p: string) => p) @@ -112,8 +113,8 @@ describe('terminal-history', () => { renameSyncMock.mockReset() readFileSyncMock.mockReset() readdirSyncMock.mockReset() - rmAsyncMock.mockReset() - rmAsyncMock.mockResolvedValue(undefined) + removeHostTreeMock.mockReset() + removeHostTreeMock.mockResolvedValue(undefined) getPathMock.mockReturnValue('/fake/userData') installFakeAppEnvironment({ getPath: getPathMock }) existsSyncMock.mockReturnValue(true) @@ -552,10 +553,9 @@ describe('terminal-history', () => { deleteWorktreeHistoryDir('repo-1::/path/wt') expect(renameSyncMock).toHaveBeenCalled() expect(rmSyncMock).not.toHaveBeenCalled() - expect(rmAsyncMock).toHaveBeenCalledWith( - expect.stringContaining('.pending-delete'), - expect.objectContaining({ recursive: true, force: true }) - ) + expect(removeHostTreeMock).toHaveBeenCalledWith(expect.stringContaining('.pending-delete'), { + lane: 'background' + }) await flushPendingWorktreeHistoryDeletions() }) @@ -647,14 +647,14 @@ describe('terminal-history', () => { // Why: a path-derived worktree ID can be recreated at the same path, so an async rm aimed at the // live directory could delete a freshly recreated worktree's history. GC reclaims it instead. - expect(rmAsyncMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() expect(rmSyncMock).not.toHaveBeenCalled() await flushPendingWorktreeHistoryDeletions() }) it('does not throw when the async removal itself fails', async () => { existsSyncMock.mockReturnValue(true) - rmAsyncMock.mockRejectedValueOnce(new Error('async fail')) + removeHostTreeMock.mockRejectedValueOnce(new Error('async fail')) expect(() => deleteWorktreeHistoryDir('repo-1::/path/wt')).not.toThrow() await expect(flushPendingWorktreeHistoryDeletions()).resolves.toBeUndefined() }) @@ -677,13 +677,13 @@ describe('terminal-history', () => { } return [] }) - rmAsyncMock.mockImplementation(async () => { + removeHostTreeMock.mockImplementation(async () => { leftoverTombstonePresent = false }) await flushPendingWorktreeHistoryDeletions() - expect(rmAsyncMock).toHaveBeenCalledWith( + expect(removeHostTreeMock).toHaveBeenCalledWith( expect.stringContaining('leftover-tombstone'), - expect.objectContaining({ recursive: true, force: true }) + { lane: 'background' } ) }) }) diff --git a/src/main/tree-removal-walk.test.ts b/src/main/tree-removal-walk.test.ts new file mode 100644 index 000000000000..8aad1fc5c2e8 --- /dev/null +++ b/src/main/tree-removal-walk.test.ts @@ -0,0 +1,248 @@ +import { + Dirent, + chmodSync, + existsSync, + mkdirSync, + mkdtempSync, + readdirSync, + symlinkSync, + writeFileSync +} from 'node:fs' +import * as realFs from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterAll, afterEach, describe, expect, it, vi } from 'vitest' +import { removeTreeSync } from '../shared/windows-transient-lock-removal' + +// Why untyped: the fakes below implement only the call shapes the walk uses. +const walkFs = vi.hoisted(() => { + const state: { current: object | null; inFlight: number; maxInFlight: number } = { + current: null, + inFlight: 0, + maxInFlight: 0 + } + return state +}) + +vi.mock('./asar-transparent-fs', () => ({ + asarTransparentFs: () => walkFs.current +})) + +import { removeTreeWithBoundedFsCalls } from './tree-removal-walk' + +const options = { recursive: true, force: true } +const roots: string[] = [] + +function counted(call: (...args: A) => Promise) { + return async (...args: A): Promise => { + walkFs.inFlight += 1 + walkFs.maxInFlight = Math.max(walkFs.maxInFlight, walkFs.inFlight) + try { + return await call(...args) + } finally { + walkFs.inFlight -= 1 + } + } +} + +type WalkFsOverrides = Record Promise> + +function useCountedRealFs(overrides: WalkFsOverrides = {}): void { + walkFs.inFlight = 0 + walkFs.maxInFlight = 0 + walkFs.current = { + lstat: counted(realFs.lstat), + readdir: counted(realFs.readdir), + rm: counted(realFs.rm), + rmdir: counted(realFs.rmdir), + unlink: counted(realFs.unlink), + ...overrides + } +} + +function makeRoot(): string { + const root = mkdtempSync(join(tmpdir(), 'orca-tree-walk-')) + roots.push(root) + return root +} + +function buildTree(target: string, directories: number, filesPerDirectory: number): string { + for (let d = 0; d < directories; d++) { + const directory = join(target, `pkg-${d}`, 'lib') + mkdirSync(directory, { recursive: true }) + for (let f = 0; f < filesPerDirectory; f++) { + writeFileSync(join(directory, `${f}.js`), 'x') + } + } + return target +} + +// Why: a directory listing may classify a link (a Windows junction) as a directory; only lstat decides. +async function readdirReportingLinksAsDirectories(path: string): Promise { + const entries = await realFs.readdir(path, { withFileTypes: true }) + return entries.map((entry) => + Object.assign(entry, { + isDirectory: () => entry.isSymbolicLink() || Dirent.prototype.isDirectory.call(entry) + }) + ) +} + +afterEach(() => { + walkFs.current = null +}) + +afterAll(() => { + for (const root of roots) { + removeTreeSync(root) + } +}) + +describe('removeTreeWithBoundedFsCalls', () => { + it('never has more than two fs calls in flight across concurrent deletes', async () => { + useCountedRealFs() + const root = makeRoot() + const targets = Array.from({ length: 10 }, (_, i) => buildTree(join(root, `t${i}`), 4, 8)) + + await Promise.all( + targets.map((target, i) => + removeTreeWithBoundedFsCalls(target, options, i % 2 ? 'interactive' : 'background') + ) + ) + + expect(targets.filter((target) => existsSync(target))).toEqual([]) + expect(walkFs.maxInFlight).toBe(2) + }) + + it('gives an interactive delete the next free slot ahead of queued background calls', async () => { + const started: string[] = [] + const pending = new Map void>() + const hold = (label: string): Promise => { + started.push(label) + return new Promise((resolve) => pending.set(label, resolve)) + } + walkFs.current = { + lstat: vi.fn(async (path: string) => { + await hold(`lstat ${path}`) + return { isDirectory: () => false } + }), + readdir: vi.fn(), + unlink: vi.fn((path: string) => hold(`unlink ${path}`)) + } + const flush = (): Promise => new Promise((resolve) => setImmediate(resolve)) + const release = async (label: string): Promise => { + pending.get(label)?.() + pending.delete(label) + await flush() + } + + const deletes = [ + removeTreeWithBoundedFsCalls('/bg-1', options, 'background'), + removeTreeWithBoundedFsCalls('/bg-2', options, 'background'), + removeTreeWithBoundedFsCalls('/bg-3', options, 'background') + ] + await flush() + deletes.push(removeTreeWithBoundedFsCalls('/folder', options, 'interactive')) + await flush() + expect(started).toEqual(['lstat /bg-1', 'lstat /bg-2']) + + await release('lstat /bg-1') + expect(started.at(-1)).toBe('lstat /folder') + + while (pending.size > 0) { + await release([...pending.keys()][0]) + } + await Promise.all(deletes) + }) + + // Why: Windows refuses a plain unlink/rmdir of a read-only or briefly locked entry; `rm` clears the + // read-only flag and retries. + it('falls back to rm for an entry a plain unlink or rmdir refuses', async () => { + const refuse = async (): Promise => { + throw Object.assign(new Error('operation not permitted'), { code: 'EPERM' }) + } + useCountedRealFs({ unlink: refuse, rmdir: refuse }) + const target = buildTree(join(makeRoot(), 'tree'), 2, 3) + + await removeTreeWithBoundedFsCalls(target, options, 'interactive') + + expect(existsSync(target)).toBe(false) + }) + + it('unlinks a directory symlink that points out of the tree without touching its target', async () => { + useCountedRealFs() + const root = makeRoot() + const outside = join(root, 'outside') + mkdirSync(outside) + writeFileSync(join(outside, 'keep.txt'), 'x') + const target = buildTree(join(root, 'tree'), 1, 2) + symlinkSync(outside, join(target, 'linked'), 'junction') + + await removeTreeWithBoundedFsCalls(target, options, 'background') + + expect(existsSync(target)).toBe(false) + expect(existsSync(join(outside, 'keep.txt'))).toBe(true) + }) + + it('does not follow a link that the directory listing reports as a directory', async () => { + const root = makeRoot() + const outside = join(root, 'outside') + mkdirSync(outside) + writeFileSync(join(outside, 'keep.txt'), 'x') + const target = buildTree(join(root, 'tree'), 1, 2) + symlinkSync(outside, join(target, 'linked'), 'junction') + useCountedRealFs({ readdir: readdirReportingLinksAsDirectories }) + + await removeTreeWithBoundedFsCalls(target, options, 'background') + + expect(existsSync(target)).toBe(false) + expect(existsSync(join(outside, 'keep.txt'))).toBe(true) + }) + + it('removes only the link when the target itself is a directory symlink', async () => { + useCountedRealFs() + const root = makeRoot() + const outside = buildTree(join(root, 'outside'), 1, 2) + const link = join(root, 'link') + symlinkSync(outside, link, 'junction') + + await removeTreeWithBoundedFsCalls(link, options, 'interactive') + + expect(existsSync(link)).toBe(false) + expect(existsSync(join(outside, 'pkg-0', 'lib', '0.js'))).toBe(true) + }) + + it('completes on a link loop, even one listed as a directory', async () => { + useCountedRealFs({ readdir: readdirReportingLinksAsDirectories }) + const root = makeRoot() + const target = buildTree(join(root, 'tree'), 1, 1) + symlinkSync(target, join(target, 'pkg-0', 'lib', 'loop'), 'junction') + + await removeTreeWithBoundedFsCalls(target, options, 'background') + + expect(existsSync(target)).toBe(false) + }) + + it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)( + 'rejects with the fs code and the stuck entry after removing everything else', + async () => { + const root = makeRoot() + const target = buildTree(join(root, 'tree'), 6, 4) + const locked = join(target, 'pkg-1', 'lib') + useCountedRealFs() + chmodSync(locked, 0o500) + try { + await expect( + removeTreeWithBoundedFsCalls(target, options, 'interactive') + ).rejects.toMatchObject({ + code: 'EACCES', + path: expect.stringContaining(join('pkg-1', 'lib')) + }) + expect(walkFs.inFlight).toBe(0) + expect(readdirSync(target)).toEqual(['pkg-1']) + expect(readdirSync(locked)).toHaveLength(4) + } finally { + chmodSync(locked, 0o700) + } + } + ) +}) diff --git a/src/main/tree-removal-walk.ts b/src/main/tree-removal-walk.ts new file mode 100644 index 000000000000..af5205c8a81f --- /dev/null +++ b/src/main/tree-removal-walk.ts @@ -0,0 +1,94 @@ +// Why a bounded walk: a recursive `fs.promises.rm` queues every entry of a ~100k-entry worktree on +// libuv's 4-thread pool, so every other async fs call in the process (the agent-session store +// behind chat sends) waited minutes behind it. Every fs call a tree delete makes holds one of two +// process-wide slots, which keeps the rest of the pool free. +// Why not native recursive `rmSync`: on Windows it follows junctions out of the tree and fails on +// read-only files. Here lstat keeps links from being followed, and the `rm` fallback clears the flag. + +import type { Dirent, RmOptions } from 'node:fs' +import { join } from 'node:path' +import { asarTransparentFs } from './asar-transparent-fs' +import { PrioritySemaphore } from '../shared/priority-semaphore' + +/** `background`: fire-and-forget bulk deletes (worktree trash, history tombstones). `interactive`: a caller awaits it. */ +export type TreeRemovalLane = 'interactive' | 'background' + +const TREE_REMOVAL_FS_CALL_SLOTS = 2 +// Why two lanes per directory: enough to keep both slots busy without one promise per entry. +const CHILD_REMOVAL_FAN_OUT = 2 +const LANE_PRIORITY: Record = { interactive: 0, background: 1 } + +const fsCallSlots = new PrioritySemaphore(TREE_REMOVAL_FS_CALL_SLOTS) + +async function withFsCallSlot(lane: TreeRemovalLane, call: () => Promise): Promise { + const release = await fsCallSlots.acquire(LANE_PRIORITY[lane]) + try { + return await call() + } finally { + release() + } +} + +function isVanishedEntryError(error: unknown): boolean { + const code = typeof error === 'object' && error !== null && 'code' in error ? error.code : null + return code === 'ENOENT' || code === 'ENOTDIR' +} + +/** Recursive remove whose fs calls never take more than two pool threads; rejects with the stuck entry's fs error. */ +export async function removeTreeWithBoundedFsCalls( + targetPath: string, + options: RmOptions, + lane: TreeRemovalLane +): Promise { + const fs = asarTransparentFs() + // Why reject only after every call settles: a Windows retry must never overlap the failed attempt. + let firstFailure: { error: unknown } | undefined + + // Why unlink/rmdir first: one pool op per entry. `rm` (lstat, Windows read-only fix, lock + // retries) runs only when that fails. + const removeEntry = (path: string, isDirectory: boolean): Promise => + withFsCallSlot(lane, () => + (isDirectory ? fs.rmdir(path) : fs.unlink(path)).catch(() => fs.rm(path, options)) + ) + + const removeTree = async (path: string): Promise => { + let children: Dirent[] = [] + let isDirectory = false + try { + // Why lstat and never the Dirent alone: a symlink or junction must be unlinked, not followed. + isDirectory = (await withFsCallSlot(lane, () => fs.lstat(path))).isDirectory() + if (isDirectory) { + children = await withFsCallSlot(lane, () => fs.readdir(path, { withFileTypes: true })) + } + } catch (error) { + if (!isVanishedEntryError(error)) { + throw error + } + } + let next = 0 + // Why siblings still go after a failure: as with Node's `rm`, one stuck entry (e.g. a + // root-owned folder) must not strand the rest of the tree, which every later sweep would hit too. + let childFailed = false + const drain = async (): Promise => { + while (next < children.length) { + const child = children[next++] + const childPath = join(path, child.name) + try { + await (child.isDirectory() ? removeTree(childPath) : removeEntry(childPath, false)) + } catch (error) { + firstFailure ??= { error } + childFailed = true + } + } + } + await Promise.all( + Array.from({ length: Math.min(CHILD_REMOVAL_FAN_OUT, children.length) }, drain) + ) + if (childFailed && firstFailure) { + throw firstFailure.error + } + await removeEntry(path, isDirectory) + } + + await removeTree(targetPath) +} diff --git a/src/main/window/clipboard-ipc-handlers.test.ts b/src/main/window/clipboard-ipc-handlers.test.ts index ea3377474215..e57862a3b0f1 100644 --- a/src/main/window/clipboard-ipc-handlers.test.ts +++ b/src/main/window/clipboard-ipc-handlers.test.ts @@ -18,7 +18,7 @@ const { fsLstatMock, fsMkdirMock, fsOpendirMock, - fsRmMock, + removeHostTreeMock, fsWriteFileMock, fsOpenMock, fsStatMock, @@ -54,7 +54,7 @@ const { fsLstatMock: vi.fn(), fsMkdirMock: vi.fn(), fsOpendirMock: vi.fn(), - fsRmMock: vi.fn(), + removeHostTreeMock: vi.fn(), fsWriteFileMock: vi.fn(), fsOpenMock: vi.fn(), fsStatMock: vi.fn(), @@ -79,7 +79,6 @@ vi.mock('node:fs/promises', () => ({ lstat: fsLstatMock, mkdir: fsMkdirMock, opendir: fsOpendirMock, - rm: fsRmMock, open: fsOpenMock, stat: fsStatMock, realpath: vi.fn(), // unused here; only satisfies filesystem-path-containment's named import @@ -89,6 +88,8 @@ vi.mock('node:fs/promises', () => ({ } })) +vi.mock('../host-tree-removal', () => ({ removeHostTree: removeHostTreeMock })) + vi.mock('../ipc/filesystem-auth', () => ({ PATH_ACCESS_DENIED_MESSAGE: 'Access denied: path resolves outside allowed directories. If this blocks a legitimate workflow, please file a GitHub issue.', @@ -218,8 +219,8 @@ describe('registerClipboardHandlers', () => { async *[Symbol.asyncIterator]() {}, close: vi.fn().mockResolvedValue(undefined) })) - fsRmMock.mockReset() - fsRmMock.mockResolvedValue(undefined) + removeHostTreeMock.mockReset() + removeHostTreeMock.mockResolvedValue(undefined) fsWriteFileMock.mockReset() fsOpenMock.mockReset() fsStatMock.mockReset() @@ -360,7 +361,7 @@ describe('registerClipboardHandlers', () => { expect(provider.downloadFile).toHaveBeenCalledWith('/remote/report.pdf', tempPath) expect(fsStatMock).toHaveBeenCalledWith(tempPath) expect(resolveAuthorizedPathMock).not.toHaveBeenCalled() - expect(fsRmMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() }) it('does not materialize remote directories for OS clipboard copy', async () => { @@ -406,12 +407,7 @@ describe('registerClipboardHandlers', () => { ).rejects.toThrow('transfer failed') expect(provider.downloadFile).toHaveBeenCalledWith('/remote/report.pdf', tempPath) - expect(fsRmMock).toHaveBeenCalledWith(tempDir, { - recursive: true, - force: true, - maxRetries: 3, - retryDelay: 100 - }) + expect(removeHostTreeMock).toHaveBeenCalledWith(tempDir) expect(clipboardWriteBufferMock).not.toHaveBeenCalled() }) diff --git a/src/main/window/clipboard-remote-file-staging.test.ts b/src/main/window/clipboard-remote-file-staging.test.ts index 5ca20e683833..ef8aa78b305b 100644 --- a/src/main/window/clipboard-remote-file-staging.test.ts +++ b/src/main/window/clipboard-remote-file-staging.test.ts @@ -1,23 +1,24 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { join, resolve } from 'node:path' -const { accessMock, lstatMock, mkdirMock, opendirMock, rmMock, writeFileMock } = vi.hoisted(() => ({ - accessMock: vi.fn(), - lstatMock: vi.fn(), - mkdirMock: vi.fn(), - opendirMock: vi.fn(), - rmMock: vi.fn(), - writeFileMock: vi.fn() -})) +const { accessMock, lstatMock, mkdirMock, opendirMock, removeHostTreeMock, writeFileMock } = + vi.hoisted(() => ({ + accessMock: vi.fn(), + lstatMock: vi.fn(), + mkdirMock: vi.fn(), + opendirMock: vi.fn(), + removeHostTreeMock: vi.fn(), + writeFileMock: vi.fn() + })) vi.mock('node:fs/promises', () => ({ access: accessMock, lstat: lstatMock, mkdir: mkdirMock, opendir: opendirMock, - rm: rmMock, writeFile: writeFileMock })) +vi.mock('../host-tree-removal', () => ({ removeHostTree: removeHostTreeMock })) import { cleanupExpiredRemoteClipboardStaging, @@ -81,7 +82,7 @@ describe('remote clipboard staging ownership', () => { lstatMock.mockResolvedValue(safeDirectoryStats()) mkdirMock.mockResolvedValue(undefined) opendirMock.mockResolvedValue(openedDirectory([])) - rmMock.mockResolvedValue(undefined) + removeHostTreeMock.mockResolvedValue(undefined) writeFileMock.mockResolvedValue(undefined) }) @@ -123,13 +124,8 @@ describe('remote clipboard staging ownership', () => { await cleanupExpiredRemoteClipboardStaging(TEMP_ROOT, NOW_MS) - expect(rmMock).toHaveBeenCalledOnce() - expect(rmMock).toHaveBeenCalledWith(join(STAGING_ROOT, EXPIRED_TRANSFER), { - force: true, - maxRetries: 3, - recursive: true, - retryDelay: 100 - }) + expect(removeHostTreeMock).toHaveBeenCalledOnce() + expect(removeHostTreeMock).toHaveBeenCalledWith(join(STAGING_ROOT, EXPIRED_TRANSFER)) }) it('rejects a symlinked owned parent before opening it', async () => { @@ -142,7 +138,7 @@ describe('remote clipboard staging ownership', () => { await expect(cleanupExpiredRemoteClipboardStaging(TEMP_ROOT, NOW_MS)).resolves.toBeUndefined() expect(opendirMock).not.toHaveBeenCalled() - expect(rmMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() }) it.skipIf(typeof process.getuid !== 'function')( @@ -176,7 +172,7 @@ describe('remote clipboard staging ownership', () => { await cleanupExpiredRemoteClipboardStaging(TEMP_ROOT, NOW_MS) expect(lstatMock).not.toHaveBeenCalledWith(TEMP_ROOT) - expect(rmMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() }) it('contains explicit removals to direct owned children', async () => { @@ -185,7 +181,7 @@ describe('remote clipboard staging ownership', () => { ).toBe(false) expect(await removeRemoteClipboardTransferDirectory(TEMP_ROOT, STAGING_ROOT)).toBe(false) - expect(rmMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() }) it('treats an already-removed transfer as cleanup success', async () => { @@ -200,14 +196,14 @@ describe('remote clipboard staging ownership', () => { removeRemoteClipboardTransferDirectory(TEMP_ROOT, join(STAGING_ROOT, EXPIRED_TRANSFER)) ).resolves.toBe(true) - expect(rmMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() }) it('continues past Windows-style lock failures and retries on a later sweep', async () => { opendirMock.mockResolvedValue( openedDirectory([directoryEntry(EXPIRED_TRANSFER), directoryEntry(`1759990000000-${UUID_B}`)]) ) - rmMock.mockImplementation(async (targetPath: string) => { + removeHostTreeMock.mockImplementation(async (targetPath: string) => { if (targetPath.endsWith(EXPIRED_TRANSFER)) { throw Object.assign(new Error('locked'), { code: 'EPERM' }) } @@ -215,20 +211,20 @@ describe('remote clipboard staging ownership', () => { await expect(cleanupExpiredRemoteClipboardStaging(TEMP_ROOT, NOW_MS)).resolves.toBeUndefined() - expect(rmMock).toHaveBeenCalledTimes(2) - rmMock.mockResolvedValue(undefined) + expect(removeHostTreeMock).toHaveBeenCalledTimes(2) + removeHostTreeMock.mockResolvedValue(undefined) await cleanupExpiredRemoteClipboardStaging(TEMP_ROOT, NOW_MS) - expect(rmMock).toHaveBeenCalledTimes(4) + expect(removeHostTreeMock).toHaveBeenCalledTimes(4) }) it('bounds locked-file timer retries', async () => { vi.useFakeTimers() - rmMock.mockRejectedValue(Object.assign(new Error('locked'), { code: 'EPERM' })) + removeHostTreeMock.mockRejectedValue(Object.assign(new Error('locked'), { code: 'EPERM' })) scheduleRemoteClipboardTransferCleanup(TEMP_ROOT, join(STAGING_ROOT, EXPIRED_TRANSFER)) await vi.advanceTimersByTimeAsync(TTL_MS + 4 * RETRY_MS) - expect(rmMock).toHaveBeenCalledTimes(4) + expect(removeHostTreeMock).toHaveBeenCalledTimes(4) expect(vi.getTimerCount()).toBe(0) }) @@ -249,7 +245,7 @@ describe('remote clipboard staging ownership', () => { await cleanupExpiredRemoteClipboardStaging(TEMP_ROOT, NOW_MS) - expect(rmMock).toHaveBeenCalledTimes(257) + expect(removeHostTreeMock).toHaveBeenCalledTimes(257) expect(peak).toBe(8) }) }) @@ -261,7 +257,7 @@ describe('legacy remote clipboard staging compatibility', () => { lstatMock.mockResolvedValue(safeDirectoryStats()) mkdirMock.mockResolvedValue(undefined) opendirMock.mockResolvedValue(openedDirectory([])) - rmMock.mockResolvedValue(undefined) + removeHostTreeMock.mockResolvedValue(undefined) writeFileMock.mockResolvedValue(undefined) }) @@ -285,7 +281,7 @@ describe('legacy remote clipboard staging compatibility', () => { expect(visited).toBe(4_096) expect(reachedLateLegacyChild).toBe(false) expect(lstatMock).toHaveBeenCalledOnce() - expect(rmMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() expect(writeFileMock).toHaveBeenCalledWith(MARKER_PATH, '', { flag: 'wx', mode: 0o600 @@ -303,13 +299,8 @@ describe('legacy remote clipboard staging compatibility', () => { await cleanupLegacyRemoteClipboardStaging(TEMP_ROOT, NOW_MS) - expect(rmMock).toHaveBeenCalledOnce() - expect(rmMock).toHaveBeenCalledWith(join(TEMP_ROOT, LEGACY_EXPIRED), { - force: true, - maxRetries: 3, - recursive: true, - retryDelay: 100 - }) + expect(removeHostTreeMock).toHaveBeenCalledOnce() + expect(removeHostTreeMock).toHaveBeenCalledWith(join(TEMP_ROOT, LEGACY_EXPIRED)) expect(writeFileMock).toHaveBeenCalledOnce() }) @@ -320,7 +311,7 @@ describe('legacy remote clipboard staging compatibility', () => { lstatMock.mockImplementation(async (targetPath: string) => safeDirectoryStats(targetPath.endsWith(LEGACY_FRESH) ? NOW_MS - 1_000 : undefined) ) - rmMock.mockRejectedValue(Object.assign(new Error('locked'), { code: 'EBUSY' })) + removeHostTreeMock.mockRejectedValue(Object.assign(new Error('locked'), { code: 'EBUSY' })) await cleanupLegacyRemoteClipboardStaging(TEMP_ROOT, NOW_MS) @@ -341,7 +332,7 @@ describe('legacy remote clipboard staging compatibility', () => { await cleanupLegacyRemoteClipboardStaging(TEMP_ROOT, NOW_MS) - expect(rmMock).not.toHaveBeenCalled() + expect(removeHostTreeMock).not.toHaveBeenCalled() expect(writeFileMock).toHaveBeenCalledOnce() }) diff --git a/src/main/window/clipboard-remote-file-staging.ts b/src/main/window/clipboard-remote-file-staging.ts index 58c1e0f2518c..2485af627eb3 100644 --- a/src/main/window/clipboard-remote-file-staging.ts +++ b/src/main/window/clipboard-remote-file-staging.ts @@ -1,6 +1,7 @@ import type { Dir, Stats } from 'node:fs' -import { access, lstat, mkdir, opendir, rm, writeFile } from 'node:fs/promises' +import { access, lstat, mkdir, opendir, writeFile } from 'node:fs/promises' import { basename, dirname, join, resolve } from 'node:path' +import { removeHostTree } from '../host-tree-removal' const REMOTE_CLIPBOARD_STAGING_ROOT_NAME = 'orca-clipboard-files' const REMOTE_CLIPBOARD_LEGACY_PREFIX = 'orca-clipboard-file-' @@ -17,12 +18,6 @@ const LEGACY_DIRECTORY_PATTERN = new RegExp( `^${REMOTE_CLIPBOARD_LEGACY_PREFIX}\\d{1,16}-${UUID_PATTERN}$`, 'i' ) -const REMOVE_OPTIONS = { - recursive: true, - force: true, - maxRetries: 3, - retryDelay: 100 -} as const type CleanupResult = 'failed' | 'fresh' | 'ignored' | 'removed' @@ -121,7 +116,7 @@ export async function removeRemoteClipboardTransferDirectory( if (!isSafeOwnedDirectory(transferStats)) { return false } - await rm(transferDirectory, REMOVE_OPTIONS) + await removeHostTree(transferDirectory) return true } catch (error) { return isMissingPathError(error) @@ -230,7 +225,7 @@ async function cleanupDirectory(directory: string, nowMs: number): Promise = Promise.resolve() export function scheduleWorktreeTrashDeletion(trashPath: string): void { queuedTrashDeletions = queuedTrashDeletions.then(async () => { try { - await removeHostTree(trashPath) + await removeHostTree(trashPath, { lane: 'background' }) } catch (error) { // Why only a warning: the directory is already invisible to the user, and the startup sweep retries it. console.warn(`[worktrees] Failed to delete trashed worktree at ${trashPath}`, error) @@ -121,7 +121,7 @@ export async function sweepStaleWorktreeTrash( continue } try { - await removeHostTree(join(trashRoot, entry)) + await removeHostTree(join(trashRoot, entry), { lane: 'background' }) removed += 1 } catch (error) { console.warn( diff --git a/src/shared/windows-transient-lock-removal.ts b/src/shared/windows-transient-lock-removal.ts index e77eeecf7f24..791e4287ec20 100644 --- a/src/shared/windows-transient-lock-removal.ts +++ b/src/shared/windows-transient-lock-removal.ts @@ -6,7 +6,6 @@ import type { RmOptions } from 'node:fs' import { rmSync } from 'node:fs' -import { rm } from 'node:fs/promises' export const WINDOWS_RM_MAX_RETRIES = 8 export const WINDOWS_RM_RETRY_DELAY_MS = 150 @@ -58,7 +57,7 @@ export function removeTreeSync(targetPath: string): void { } } -/** Recursively remove a directory, retrying the transient Windows locks. */ +/** Async-signature `removeTreeSync` for test teardown; a pool-backed recursive `rm` is banned here. */ export async function removeTree(targetPath: string): Promise { - await rm(targetPath, transientLockRemovalOptions()) + removeTreeSync(targetPath) }