fix(worktrees): harden deferred trash cleanup

This commit is contained in:
Brennan Benson
2026-08-04 01:41:16 -07:00
parent 5775bbc3e6
commit 99690aadb5
4 changed files with 77 additions and 8 deletions
+17 -1
View File
@@ -57,7 +57,8 @@ import {
removeWorktree,
_resetWorktreeScanCacheForTests,
WORKTREE_LIST_TIMEOUT_MS,
WORKTREE_REMOVAL_PREFLIGHT_TIMEOUT_MS
WORKTREE_REMOVAL_PREFLIGHT_TIMEOUT_MS,
WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS
} from './worktree'
// Why: detectSparseCheckout on main also requires core.sparseCheckout=true in git
@@ -330,6 +331,10 @@ branch refs/heads/main
expect(moveWorktreeDirectoryToTrashMock).toHaveBeenCalledWith('/repo-feature')
expect(scheduleWorktreeTrashDeletionMock).toHaveBeenCalledWith('/trash/wt-1-abcdef01')
expect(calls).toContain('git worktree remove --force /repo-feature')
expect(gitExecFileAsyncMock).toHaveBeenCalledWith(
['worktree', 'remove', '--force', '/repo-feature'],
{ cwd: '/repo', timeout: WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS }
)
expect(calls).not.toContain('git worktree remove /repo-feature')
expect(calls).not.toContain('git worktree prune')
expect(calls).toContain('git branch -d -- feature/test')
@@ -362,6 +367,17 @@ branch refs/heads/main
await removeWorktree('/repo', '/repo-feature')
expect(getGitCalls()).toContain('git worktree prune')
expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'prune'], {
cwd: '/repo',
timeout: WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS
})
expect(
gitExecFileAsyncMock.mock.calls.filter(
([args, options]) =>
args.join(' ') === 'worktree list --porcelain -z' &&
options.timeout === WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS
)
).toHaveLength(2)
expect(scheduleWorktreeTrashDeletionMock).toHaveBeenCalledWith('/trash/wt-2-abcdef02')
expect(restoreWorktreeDirectoryFromTrashMock).not.toHaveBeenCalled()
})
+9 -4
View File
@@ -93,6 +93,7 @@ const PRUNABLE_EXISTENCE_PROBE_CONCURRENCY = 8
// Why: bound `git worktree add` so a OneDrive cloud-placeholder stall fails fast (STA-1292); generous enough for a legit large checkout (#7225).
export const WORKTREE_ADD_TIMEOUT_MS = 180_000
export const WORKTREE_REMOVAL_PREFLIGHT_TIMEOUT_MS = 30_000
export const WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS = 30_000
// Why: one wedged shared scan otherwise hangs every later list, including create's post-add re-list.
export const WORKTREE_LIST_TIMEOUT_MS = 30_000
@@ -1222,11 +1223,15 @@ async function clearGitRegistrationForMissingWorktree(
worktreePath: string,
options: RemoveWorktreeOptions
): Promise<void> {
const registrationOptions = {
...options,
timeout: options.timeout ?? WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS
}
try {
// Removing an already-missing directory is accepted back to the Git 2.25 baseline and touches only this entry.
await gitExecFileAsync(
['worktree', 'remove', '--force', worktreePath],
gitExecOptions(repoPath, options)
gitExecOptions(repoPath, registrationOptions)
)
return
} catch (error) {
@@ -1236,10 +1241,10 @@ async function clearGitRegistrationForMissingWorktree(
)
}
await gitExecFileAsync(['worktree', 'prune'], gitExecOptions(repoPath, options))
await gitExecFileAsync(['worktree', 'prune'], gitExecOptions(repoPath, registrationOptions))
// Strict (not the shared scan): an unreadable repo must not read as proof that the row is gone.
const stillRegistered = (await listWorktreesStrict(repoPath, options)).some((worktree) =>
areWorktreePathsEqual(worktree.path, worktreePath)
const stillRegistered = (await listWorktreesStrict(repoPath, registrationOptions)).some(
(worktree) => areWorktreePathsEqual(worktree.path, worktreePath)
)
if (stillRegistered) {
throw new Error(`Git still reports a registration for "${worktreePath}" after pruning it.`)
+40 -1
View File
@@ -1,5 +1,5 @@
import { existsSync } from 'node:fs'
import { mkdir, mkdtemp, readdir, rm, writeFile } from 'node:fs/promises'
import { mkdir, mkdtemp, readdir, rm, symlink, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { afterEach, beforeEach, describe, expect, it } from 'vitest'
@@ -63,6 +63,18 @@ describe('moveWorktreeDirectoryToTrash', () => {
expect(existsSync(getWorktreeTrashRoot(worktreePath))).toBe(false)
})
it.skipIf(process.platform === 'win32')('refuses a symlinked trash root', async () => {
const worktreePath = join(scratchDir, 'repo', 'feature')
const externalRoot = join(scratchDir, 'external')
await createWorktreeDirectory(worktreePath)
await mkdir(externalRoot)
await symlink(externalRoot, getWorktreeTrashRoot(worktreePath))
expect(await moveWorktreeDirectoryToTrash(worktreePath)).toBeUndefined()
expect(existsSync(worktreePath)).toBe(true)
expect(await readdir(externalRoot)).toEqual([])
})
async function seededWorktree(name: string): Promise<string> {
const worktreePath = join(scratchDir, 'repo', name)
await createWorktreeDirectory(worktreePath)
@@ -139,6 +151,18 @@ describe('sweepStaleWorktreeTrash', () => {
expect(await sweepStaleWorktreeTrash([scratchDir])).toEqual({ removed: 0 })
expect(existsSync(join(deepTrashRoot, 'wt-1700000000002-abcdef03'))).toBe(true)
})
it.skipIf(process.platform === 'win32')(
'does not sweep through a symlinked trash root',
async () => {
const externalEntry = join(scratchDir, 'external', 'wt-1700000000003-abcdef04')
await mkdir(externalEntry, { recursive: true })
await symlink(join(scratchDir, 'external'), join(scratchDir, WORKTREE_TRASH_DIR_NAME))
expect(await sweepStaleWorktreeTrash([scratchDir])).toEqual({ removed: 0 })
expect(existsSync(externalEntry)).toBe(true)
}
)
})
describe('collectWorktreeTrashSweepRoots', () => {
@@ -171,4 +195,19 @@ describe('collectWorktreeTrashSweepRoots', () => {
)
).toEqual([])
})
it('skips WSL repos that cannot create host trash', () => {
const originalPlatform = process.platform
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
try {
expect(
collectWorktreeTrashSweepRoots(
[repo({ path: '\\\\wsl.localhost\\Ubuntu\\home\\dev\\orca' })],
settings
)
).toEqual([])
} finally {
Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform })
}
})
})
+11 -2
View File
@@ -3,12 +3,13 @@
// is a metadata operation, and the recursive delete then runs after the IPC has already returned.
import { randomBytes } from 'node:crypto'
import { mkdir, readdir, rename, rmdir } from 'node:fs/promises'
import { lstat, mkdir, readdir, rename, rmdir } from 'node:fs/promises'
import { dirname, join } from 'node:path'
import { removeHostTree } from './host-tree-removal'
import { isFolderRepo } from '../shared/repo-kind'
import { computeWorkspaceRoot, getWorktreePathSettings } from './ipc/worktree-logic'
import type { GlobalSettings, Repo } from '../shared/types'
import { parseWslPath } from './wsl'
export const WORKTREE_TRASH_DIR_NAME = '.orca-worktree-trash'
@@ -39,6 +40,10 @@ export async function moveWorktreeDirectoryToTrash(
const trashPath = join(trashRoot, `wt-${Date.now()}-${randomBytes(4).toString('hex')}`)
try {
await mkdir(trashRoot, { recursive: true })
const trashRootStat = await lstat(trashRoot)
if (!trashRootStat.isDirectory() || trashRootStat.isSymbolicLink()) {
throw new Error(`Refusing non-directory worktree trash root: ${trashRoot}`)
}
await rename(worktreePath, trashPath)
return trashPath
} catch (error) {
@@ -97,6 +102,10 @@ export async function sweepStaleWorktreeTrash(
for (const trashRoot of await collectExistingTrashRoots(workspaceRoots)) {
let entries: string[]
try {
const trashRootStat = await lstat(trashRoot)
if (!trashRootStat.isDirectory() || trashRootStat.isSymbolicLink()) {
continue
}
entries = await readdir(trashRoot)
} catch {
continue
@@ -150,7 +159,7 @@ export function collectWorktreeTrashSweepRoots(
): string[] {
const roots = new Set<string>()
for (const repo of repos) {
if (repo.connectionId || isFolderRepo(repo)) {
if (repo.connectionId || isFolderRepo(repo) || parseWslPath(repo.path)) {
continue
}
try {