From 99690aadb5b54ee7caf194da1986d9aa3e7bfc37 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 4 Aug 2026 01:41:16 -0700 Subject: [PATCH] fix(worktrees): harden deferred trash cleanup --- src/main/git/remove-worktree.test.ts | 18 +++++++++++- src/main/git/worktree.ts | 13 ++++++--- src/main/worktree-trash.test.ts | 41 +++++++++++++++++++++++++++- src/main/worktree-trash.ts | 13 +++++++-- 4 files changed, 77 insertions(+), 8 deletions(-) diff --git a/src/main/git/remove-worktree.test.ts b/src/main/git/remove-worktree.test.ts index b85a2828930..fd8b476ff87 100644 --- a/src/main/git/remove-worktree.test.ts +++ b/src/main/git/remove-worktree.test.ts @@ -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() }) diff --git a/src/main/git/worktree.ts b/src/main/git/worktree.ts index 4b24412b7f7..e532962309a 100644 --- a/src/main/git/worktree.ts +++ b/src/main/git/worktree.ts @@ -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 { + 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.`) diff --git a/src/main/worktree-trash.test.ts b/src/main/worktree-trash.test.ts index d5f9784f075..302bb56d0c5 100644 --- a/src/main/worktree-trash.test.ts +++ b/src/main/worktree-trash.test.ts @@ -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 { 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 }) + } + }) }) diff --git a/src/main/worktree-trash.ts b/src/main/worktree-trash.ts index d1e7b50747f..34cc29aef16 100644 --- a/src/main/worktree-trash.ts +++ b/src/main/worktree-trash.ts @@ -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() for (const repo of repos) { - if (repo.connectionId || isFolderRepo(repo)) { + if (repo.connectionId || isFolderRepo(repo) || parseWslPath(repo.path)) { continue } try {