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