From f0eb0949d0d9331eba63a7d070a7109a09710a28 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 27 Oct 2026 12:31:10 -0700 Subject: [PATCH] fix(windows): delete again succeeds after a held file is released (#34058) * 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 * fix(windows): preserve checkout ownership until files are removed * fix: preserve worktree ownership checks across metadata aliases * test: make checkout retry clock completion explicit * Keep failed Windows workspace deletions retryable * test: correct removal fixture imports and path formatting * fix: retain failed removal until a retry is accepted * test: identify the mocked backoff clock * test: preserve failed removal records and recreate marker fixtures * test(worktrees): preserve exact checkout bytes in refusal contract * test(worktrees): exercise current native deletion contracts * test(windows): prove long-path deletion through native checkout removal * test(windows): preserve real Git runner exports in removal fixture --------- Co-authored-by: Orca Worker --- src/main/asar-transparent-fs.ts | 32 +- src/main/git/windows-worktree-removal.ts | 153 +++++ src/main/git/worktree-operation-options.ts | 2 +- src/main/git/worktree-removal-preflight.ts | 33 +- .../git/worktree-removal-real-git.test.ts | 125 ++++- src/main/git/worktree-removal.ts | 41 +- src/main/host-checkout-removal.test.ts | 90 +++ src/main/host-tree-removal.ts | 81 ++- .../ipc/worktrees-removal-recovery.test.ts | 226 +++----- .../listing/ssh-worktree-fallback.ts | 4 +- .../removal/execute-worktree-removal.ts | 32 +- .../registered-local-worktree-preflight.ts | 51 ++ .../remove-registered-local-worktree.ts | 48 +- .../removal/remove-unregistered-worktree.ts | 19 +- .../retry-failed-local-worktree-removal.ts | 5 +- src/main/local-orphaned-worktree-cleanup.ts | 3 +- src/main/local-worktree-filesystem.test.ts | 115 +++- src/main/local-worktree-filesystem.ts | 73 +-- ...removal-and-reconciliation-part-02.spec.ts | 102 ++-- .../worktree-removal-execution-host.spec.ts | 1 + ...time-failed-local-worktree-removal.test.ts | 13 +- ...interrupted-local-worktree-removal.test.ts | 521 +++++++++++++++--- ...time-interrupted-local-worktree-removal.ts | 75 ++- ...ntime-registered-local-worktree-removal.ts | 45 +- .../runtime-unregistered-worktree-removal.ts | 19 +- .../stale-worktree-registration-preflight.ts | 32 ++ .../workspaces/workspace-removal-target.ts | 35 +- .../runtime/workspaces/workspace-removal.ts | 49 +- src/main/worktree-background-removal.ts | 6 +- src/main/worktree-failed-removal.test.ts | 83 +-- src/main/worktree-orphan-gitdir-proof.ts | 195 ++++--- src/main/worktree-owner-path-equivalence.ts | 83 +++ src/main/worktree-removal-failure.ts | 40 +- src/main/worktree-removal-leftover.ts | 43 +- src/main/worktree-removal-listing.ts | 26 +- src/main/worktree-removal-safety.test.ts | 258 ++++++--- src/main/worktree-removal-safety.ts | 16 +- src/shared/git-common-directory.ts | 51 +- src/shared/worktree/removal.ts | 6 + 39 files changed, 2069 insertions(+), 763 deletions(-) create mode 100644 src/main/git/windows-worktree-removal.ts create mode 100644 src/main/host-checkout-removal.test.ts create mode 100644 src/main/ipc/worktrees/removal/registered-local-worktree-preflight.ts create mode 100644 src/main/runtime/workspaces/stale-worktree-registration-preflight.ts create mode 100644 src/main/worktree-owner-path-equivalence.ts diff --git a/src/main/asar-transparent-fs.ts b/src/main/asar-transparent-fs.ts index ab6b9fc94f0..694c034a540 100644 --- a/src/main/asar-transparent-fs.ts +++ b/src/main/asar-transparent-fs.ts @@ -1,25 +1,27 @@ // Electron treats .asar files as directories; unpatched fs prevents recursive rm failures. // Avoid process.noAsar: other main-process work may load app.asar during a long removal. -import { rm as nodeRm } from 'node:fs/promises' +import * as nodeFs from 'node:fs/promises' import { createRequire } from 'node:module' -type Rm = typeof nodeRm +let resolvedFs: typeof nodeFs | undefined -let resolvedRm: Rm | undefined - -function resolveRm(): Rm { - try { - // Resolve lazily: original-fs exists only in Electron; plain Node’s fs is already unpatched. - const originalFs = createRequire(__filename)('original-fs') as { promises?: { rm?: Rm } } - return typeof originalFs.promises?.rm === 'function' ? originalFs.promises.rm : nodeRm - } catch { - return nodeRm +function nativeFs(): typeof nodeFs { + if (!resolvedFs) { + try { + // Resolve lazily: original-fs exists only in Electron; plain Node's fs is already unpatched. + const originalFs: { promises?: typeof nodeFs } = createRequire(__filename)('original-fs') + resolvedFs = originalFs.promises ?? nodeFs + } catch { + resolvedFs = nodeFs + } } + return resolvedFs } /** `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) -} +export const rm: typeof nodeFs.rm = (path, options) => nativeFs().rm(path, options) +export const readdir = (path: string): Promise => nativeFs().readdir(path) +export const unlink = (path: string): Promise => nativeFs().unlink(path) +export const rmdir = (path: string): Promise => nativeFs().rmdir(path) +export const lstat = (path: string) => nativeFs().lstat(path) diff --git a/src/main/git/windows-worktree-removal.ts b/src/main/git/windows-worktree-removal.ts new file mode 100644 index 00000000000..6159ec044bc --- /dev/null +++ b/src/main/git/windows-worktree-removal.ts @@ -0,0 +1,153 @@ +import { join, resolve } from 'node:path' +import { readFile } from 'node:fs/promises' +import { lstat, readdir } from '../asar-transparent-fs' +import { removeHostCheckout } from '../host-tree-removal' +import { parseWslPath } from '../wsl' +import { CLIENT_REMOVAL_HOME } from '../worktree-removal-home-guard' +import { + assertWorktreeUnlockedForRemoval, + unregisteredFolderRefusal +} from '../../shared/worktree/removal' +import { + assertWorktreeDoesNotContainRegisteredWorktree, + canSafelyRemoveOrphanedWorktreeDirectory, + findRegisteredDeletableWorktree, + isDangerousWorktreeRemovalPath, + ORPHANED_WORKTREE_DIRECTORY_MESSAGE +} from '../worktree-removal-safety' +import { readTranslatedWorktreeGraph } from './worktree-list-reader' +import { assertWorktreeCleanForRemoval } from './worktree-removal-preflight' +import { getErrorCode, type RemoveWorktreeOptions } from './worktree-operation-options' + +export function usesNativeWindowsRemoval(path: string, options: { wslDistro?: string }): boolean { + return process.platform === 'win32' && !options.wslDistro && !parseWslPath(path) +} + +export async function nativeWindowsMarkerMissing( + path: string, + options: { wslDistro?: string } +): Promise { + if (!usesNativeWindowsRemoval(path, options)) { + return false + } + try { + await lstat(join(path, '.git')) + return false + } catch (error) { + if (getErrorCode(error) === 'ENOENT') { + return true + } + throw error + } +} + +/** Called inside the destructive slot, after teardown; every pass rederives authority from Git/disk. */ +export async function removeWindowsCheckout( + repo: string, + root: string, + force: boolean, + options: RemoveWorktreeOptions +): Promise { + await removeHostCheckout( + root, + (checkClean) => authorizeWindowsCheckout(repo, root, force, options, false, checkClean), + options.checkoutDeleteSignal + ) +} + +/** Share ordinary refusal before either entrypoint publishes a retry or tears down resources. */ +export async function authorizeUnregisteredWindowsRemoval( + registered: boolean, + repo: string, + root: string, + force: boolean, + options: RemoveWorktreeOptions +): Promise { + if (!registered && usesNativeWindowsRemoval(root, options)) { + await authorizeWindowsCheckout( + repo, + root, + force, + { ...options, knownRemovedWorktree: null }, + true + ) + } +} + +/** Read-only eligibility before teardown; destructive admission always repeats it after waits. */ +export async function authorizeWindowsCheckout( + repo: string, + root: string, + force: boolean, + options: RemoveWorktreeOptions & Parameters[2], + beforeTeardown = false, + checkClean = true +): Promise { + const worktrees = await readTranslatedWorktreeGraph(repo, options) + const current = findRegisteredDeletableWorktree(repo, root, worktrees, CLIENT_REMOVAL_HOME) + if (isDangerousWorktreeRemovalPath(root, repo, CLIENT_REMOVAL_HOME)) { + throw new Error(`Refusing to delete protected worktree path: ${root}`) + } + // The unregistered route needs the same fresh nested-worktree refusal. + if (!current) { + assertWorktreeDoesNotContainRegisteredWorktree(root, worktrees) + } + assertWorktreeUnlockedForRemoval(current ?? undefined) + const expected = options.knownRemovedWorktree + if ( + current && + (expected === null || + (expected && + (current.branch !== expected.branch || (expected.head && current.head !== expected.head)))) + ) { + throw new Error( + `A different checkout is now at ${root}; Orca left it in place. Delete it again to remove it.` + ) + } + let entry + try { + entry = await lstat(root) + } catch (error) { + if (getErrorCode(error) === 'ENOENT') { + return false + } + throw error + } + if (!entry.isDirectory() || entry.isSymbolicLink()) { + throw new Error(`Refusing to delete unregistered worktree path: ${root}`) + } + if (await nativeWindowsMarkerMissing(root, options)) { + // Reject only; an empty observation never authorizes recursive deletion after teardown. + if (beforeTeardown && (await readdir(root)).length > 0) { + throw unregisteredFolderRefusal(root) + } + return false + } + if (!(await canSafelyRemoveOrphanedWorktreeDirectory(root, repo, CLIENT_REMOVAL_HOME))) { + throw new Error(`Refusing to delete unregistered worktree path: ${root}`) + } + if (!current && !force) { + throw new Error(ORPHANED_WORKTREE_DIRECTORY_MESSAGE) + } + const marker = join(root, '.git') + const admin = resolve( + root, + (await readFile(marker, 'utf8')) + .split(/\r?\n/, 1)[0] + .trim() + .replace(/^gitdir:\s*/i, '') + ) + try { + await lstat(join(admin, 'locked')) + throw new Error('Cannot remove a locked worktree. Unlock it before deleting.') + } catch (error) { + if (getErrorCode(error) !== 'ENOENT') { + throw error + } + } + if (checkClean && !force) { + await assertWorktreeCleanForRemoval(root, force, options, admin) + return authorizeWindowsCheckout(repo, root, force, options, beforeTeardown, false) + } + return true +} diff --git a/src/main/git/worktree-operation-options.ts b/src/main/git/worktree-operation-options.ts index 591cb578d3c..72387782dac 100644 --- a/src/main/git/worktree-operation-options.ts +++ b/src/main/git/worktree-operation-options.ts @@ -45,7 +45,7 @@ export type AddWorktreeOptions = GitWorktreeExecOptions & { export type RemoveWorktreeOptions = GitWorktreeExecOptions & { deleteBranch?: boolean forceBranchDelete?: boolean - knownRemovedWorktree?: Pick + knownRemovedWorktree?: Pick | null /** Stops only `git worktree remove`; a killed branch cleanup could strand a ref lock. */ checkoutDeleteSignal?: AbortSignal } diff --git a/src/main/git/worktree-removal-preflight.ts b/src/main/git/worktree-removal-preflight.ts index 26a6a5629af..b5ab72129c5 100644 --- a/src/main/git/worktree-removal-preflight.ts +++ b/src/main/git/worktree-removal-preflight.ts @@ -1,5 +1,6 @@ import { stat } from 'node:fs/promises' import { join } from 'node:path' +import { isWorktreePathMissing } from '../worktree-removal-safety' import { SUBMODULE_DIRTY_REMOVAL_DETAIL } from '../../shared/worktree/removal' import { gitExecFileAsync } from './runner' import type { WorktreeRemovalPreflightOptions } from './worktree-operation-options' @@ -8,13 +9,38 @@ import { WORKTREE_REMOVAL_PREFLIGHT_TIMEOUT_MS, gitExecOptions } from './worktre export async function assertWorktreeCleanForRemoval( worktreePath: string, force = false, - options: WorktreeRemovalPreflightOptions = {} + options: WorktreeRemovalPreflightOptions = {}, + adminDirectory?: string ): Promise { if (force) { return } const { ignoredUntrackedPaths = [], ...gitOptions } = options + const bound = adminDirectory + ? { env: { ...process.env, GIT_DIR: join(worktreePath, '.git'), GIT_WORK_TREE: worktreePath } } + : {} + const execution = { + ...gitExecOptions(worktreePath, gitOptions), + ...bound, + timeout: gitOptions.timeout ?? WORKTREE_REMOVAL_PREFLIGHT_TIMEOUT_MS + } + if (adminDirectory) { + const { stdout: index } = await gitExecFileAsync(['ls-files', '--stage', '-z'], execution) + if (!(await isWorktreePathMissing(join(adminDirectory, 'modules')))) { + throw new Error(SUBMODULE_DIRTY_REMOVAL_DETAIL) + } + for (const entry of index.split('\0')) { + if ( + entry.startsWith('160000 ') && + !(await isWorktreePathMissing( + join(worktreePath, entry.slice(entry.indexOf('\t') + 1), '.git') + )) + ) { + throw new Error(SUBMODULE_DIRTY_REMOVAL_DETAIL) + } + } + } const useNullTerminatedStatus = ignoredUntrackedPaths.length > 0 const { stdout } = await gitExecFileAsync( [ @@ -25,10 +51,7 @@ export async function assertWorktreeCleanForRemoval( // User ignore-submodules config must not hide edits from the dirty check. '--ignore-submodules=none' ], - { - ...gitExecOptions(worktreePath, gitOptions), - timeout: gitOptions.timeout ?? WORKTREE_REMOVAL_PREFLIGHT_TIMEOUT_MS - } + execution ) // Use the same entries for the verdict and the error message. const blockingEntries = useNullTerminatedStatus diff --git a/src/main/git/worktree-removal-real-git.test.ts b/src/main/git/worktree-removal-real-git.test.ts index 9d5261ff036..a907c858d2a 100644 --- a/src/main/git/worktree-removal-real-git.test.ts +++ b/src/main/git/worktree-removal-real-git.test.ts @@ -6,9 +6,18 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { performance } from 'node:perf_hooks' import { promisify } from 'node:util' -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import * as nativeFs from '../asar-transparent-fs' +import * as diskFs from 'node:fs/promises' +import * as runner from './runner' +import { + runUnderWorktreeDeleteLimit, + _worktreeDeleteLimitSnapshotForTests +} from './worktree-delete-limit' import { installRealWorkerThreadChildProcesses } from '../worker-thread-child-process/worker-thread-child-process-real.test-fixtures' import { removeTree } from '../../shared/windows-transient-lock-removal' +import { canSafelyRemoveOrphanedWorktreeDirectory } from '../worktree-removal-safety' +import { CLIENT_REMOVAL_HOME } from '../worktree-removal-home-guard' import { classifyWorktreeForceDeleteReason } from '../../shared/worktree/removal' import { formatWorktreeRemovalError } from '../ipc/worktree-logic' import { assertWorktreeCleanForRemoval, listWorktreesStrict, removeWorktree } from './worktree' @@ -62,17 +71,119 @@ beforeEach(async () => { }) afterEach(async () => { + vi.restoreAllMocks() + vi.unstubAllEnvs() await removeTree(scratchDir) }) describe('worktree removal against the real Git binary', () => { - it('deletes the checkout and its registration before returning', async () => { - await removeWorktree(repoPath, worktreePath, false, { deleteBranch: false }) + it.each(['ordinary', 'bare', 'link-before', 'link-after'])( + 'deletes the checkout and only its target administration from a %s parent', + async (layout) => { + if (layout === 'bare') { + await git(['worktree', 'remove', '--force', worktreePath], repoPath) + const bare = join(scratchDir, 'bare.git') + await git(['clone', '--bare', '-q', repoPath, bare], scratchDir) + repoPath = bare + await git(['worktree', 'add', '-q', worktreePath, 'feature'], repoPath) + } + if (layout.startsWith('link-')) { + if (layout === 'link-before') { + await git(['worktree', 'remove', '--force', worktreePath], repoPath) + } + const metadata = join(scratchDir, 'metadata') + await diskFs.rename(join(repoPath, '.git'), metadata) + await diskFs.symlink( + metadata, + join(repoPath, '.git'), + process.platform === 'win32' ? 'junction' : 'dir' + ) + if (layout === 'link-before') { + await git(['worktree', 'add', '-q', worktreePath, 'feature'], repoPath) + } + expect((await diskFs.lstat(join(repoPath, '.git'))).isSymbolicLink()).toBe(true) + } + const sibling = join(scratchDir, 'unrelated') + await git(['worktree', 'add', '-q', sibling, '-b', 'sibling'], repoPath) + await diskFs.rename(sibling, join(scratchDir, 'offline')) + const marker = await readFile(join(worktreePath, '.git'), 'utf8') + const admin = marker.trim().slice(8) + const backlink = await readFile(join(admin, 'gitdir'), 'utf8') + console.info('owning metadata layout', { layout, marker, backlink }) + expect( + await canSafelyRemoveOrphanedWorktreeDirectory(worktreePath, repoPath, CLIENT_REMOVAL_HOME) + ).toBe(true) - expect(existsSync(worktreePath)).toBe(false) - expect(await isRegistered(worktreePath)).toBe(false) - expect(existsSync(join(workspaceRoot, 'repo', WORKTREE_TRASH_DIR_NAME))).toBe(false) - }) + await removeWorktree(repoPath, worktreePath, false, { deleteBranch: false }) + + expect(existsSync(worktreePath)).toBe(false) + expect(await isRegistered(worktreePath)).toBe(false) + expect(existsSync(join(workspaceRoot, 'repo', WORKTREE_TRASH_DIR_NAME))).toBe(false) + expect(existsSync(admin)).toBe(false) + expect(await isRegistered(sibling)).toBe(true) + } + ) + + it.skipIf(process.platform !== 'win32').each(['redirect', 'ls-files', 'status'])( + 'preserves the accepted checkout across bound Git checks: %s', + async (schedule) => { + const dispatch = vi.spyOn(nativeFs, 'rm') + if (schedule === 'redirect') { + const clean = join(scratchDir, 'clean') + await git(['worktree', 'add', '-q', clean, '-b', 'clean'], repoPath) + await git(['config', 'extensions.worktreeConfig', 'true'], repoPath) + await git(['config', '--worktree', 'core.worktree', clean], worktreePath) + await writeFile(join(worktreePath, 'seed.txt'), 'do not delete\n') + vi.stubEnv('GIT_WORK_TREE', clean) + } else { + if (schedule === 'ls-files') { + await writeFile(join(repoPath, '.git', 'info', 'exclude'), 'nested/\n') + } + const execute = runner.gitExecFileAsync + vi.spyOn(runner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + const result = await execute(args, options) + if (args[0] === schedule && options?.env?.GIT_WORK_TREE === worktreePath) { + if (schedule === 'status') { + await git(['worktree', 'lock', worktreePath], repoPath) + } else { + const nested = join(worktreePath, 'nested') + await git(['worktree', 'add', '-q', nested, '-b', 'nested'], repoPath) + } + } + return result + }) + } + await expect(removeWorktree(repoPath, worktreePath, false)).rejects.toThrow( + /uncommitted|locked|contains another/ + ) + expect(dispatch).not.toHaveBeenCalled() + expect(await readFile(join(worktreePath, 'seed.txt'), 'utf8')).toBe( + schedule === 'redirect' ? 'do not delete\n' : 'seed\n' + ) + expect(await isRegistered(worktreePath)).toBe(true) + } + ) + + it.skipIf(process.platform !== 'win32')( + 'freezes accepted absence across the delete-slot wait', + async () => { + await git(['worktree', 'remove', worktreePath], repoPath) + const paused = Promise.withResolvers() + const blockers = [0, 1].map(() => runUnderWorktreeDeleteLimit(() => paused.promise)) + const removal = removeWorktree(repoPath, worktreePath, true, { + knownRemovedWorktree: null + }).catch((error: unknown) => error) + try { + await vi.waitFor(() => expect(_worktreeDeleteLimitSnapshotForTests().waiting).toBe(1)) + await git(['worktree', 'add', '-q', '--detach', worktreePath], repoPath) + } finally { + paused.resolve() + await Promise.all([...blockers, removal]) + } + expect(String(await removal)).toContain('different checkout') + expect(await readFile(join(worktreePath, 'seed.txt'), 'utf8')).toBe('seed\n') + } + ) it('leaves sibling worktrees registered', async () => { const siblingPath = join(workspaceRoot, 'repo', 'sibling') diff --git a/src/main/git/worktree-removal.ts b/src/main/git/worktree-removal.ts index 18a383bdd24..2a6c9de2925 100644 --- a/src/main/git/worktree-removal.ts +++ b/src/main/git/worktree-removal.ts @@ -17,6 +17,9 @@ import { withRepoRefMaintenancePaused } from './local-repo-ref-maintenance' import { bumpWorktreeScanGeneration, listWorktrees } from './worktree-scan-cache' import { invalidateSparseCheckoutState } from './worktree-sparse-checkout-cache' import { runUnderWorktreeDeleteLimit } from './worktree-delete-limit' +import { removeWindowsCheckout, usesNativeWindowsRemoval } from './windows-worktree-removal' +import { listWorktreesStrict } from './worktree-listing' +import { isWorktreePathMissing } from '../worktree-removal-safety' import { runKeyedSerializedOperation } from '../cli/keyed-promise-queue' const branchCleanupQueueByRepo = new Map>() @@ -50,15 +53,16 @@ async function performRemoveWorktree( options: RemoveWorktreeOptions = {} ): Promise { const removedWorktree = - options.knownRemovedWorktree ?? - (await listWorktrees(repoPath, options)).find((worktree) => - areWorktreePathsEqual(worktree.path, worktreePath) - ) + options.knownRemovedWorktree !== undefined + ? options.knownRemovedWorktree + : ((await listWorktrees(repoPath, options)).find((worktree) => + areWorktreePathsEqual(worktree.path, worktreePath) + ) ?? null) const branchName = normalizeLocalBranchRef(removedWorktree?.branch ?? '') const branchHead = removedWorktree?.head ?? '' // Enforce locks even for callers that bypass IPC/runtime preflight. - assertWorktreeUnlockedForRemoval(removedWorktree) + assertWorktreeUnlockedForRemoval(removedWorktree ?? undefined) // Avoid default deadlines during large deletes; preserve Windows long-path support used at creation. const longPathArgs = windowsLongPathGitArgs(repoPath) @@ -74,8 +78,31 @@ async function performRemoveWorktree( } args.push(worktreePath) await runUnderWorktreeDeleteLimit(async () => { - await gitExecFileAsync(args, execOptions) - await removeCheckoutLeftByGit(worktreePath, options) + if (usesNativeWindowsRemoval(worktreePath, options)) { + await removeWindowsCheckout(repoPath, worktreePath, force, { + ...options, + knownRemovedWorktree: removedWorktree + }) + // Release only this administrative entry, and never override a newly acquired Git lock. + if ( + (await listWorktreesStrict(repoPath, options)).some((row) => + areWorktreePathsEqual(row.path, worktreePath) + ) + ) { + if (!(await isWorktreePathMissing(worktreePath))) { + throw new Error( + `Worktree path appeared during deletion: ${worktreePath}. Retry deletion.` + ) + } + await gitExecFileAsync( + [...longPathArgs, 'worktree', 'remove', '--force', '--', worktreePath], + execOptions + ) + } + } else { + await gitExecFileAsync(args, execOptions) + await removeCheckoutLeftByGit(worktreePath, options) + } }) if (!branchName) { diff --git a/src/main/host-checkout-removal.test.ts b/src/main/host-checkout-removal.test.ts new file mode 100644 index 00000000000..2ddef9c54a9 --- /dev/null +++ b/src/main/host-checkout-removal.test.ts @@ -0,0 +1,90 @@ +import { afterEach, beforeEach, expect, it, vi } from 'vitest' +import * as fs from './asar-transparent-fs' +import { removeHostCheckout, removeHostEmptyDirectory } from './host-tree-removal' + +vi.mock('./asar-transparent-fs', () => ({ + readdir: vi.fn(), + rm: vi.fn(), + unlink: vi.fn(), + rmdir: vi.fn(), + lstat: vi.fn() +})) +vi.mock('node:timers/promises', () => ({ + // Route promise backoff through the clock installed by vi.useFakeTimers. + // ast-grep-ignore: test-no-real-time-wait -- Schedules the installed fake clock for production backoff. + setTimeout: (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) +})) +const platform = process.platform +const root = 'C:/checkout-partition' +const busy = Object.assign(new Error('held child'), { code: 'EBUSY' }) +beforeEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + vi.useFakeTimers() +}) +afterEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: platform }) + vi.useRealTimers() + vi.resetAllMocks() +}) + +it('settles public child promises before starting another pass or touching the marker', async () => { + vi.mocked(fs.readdir).mockResolvedValue(['held', 'pending', '.git']) + const pending = Promise.withResolvers() + vi.mocked(fs.rm).mockImplementation(async (target) => { + if (String(target).endsWith('held')) { + throw busy + } + await pending.promise + }) + const controller = new AbortController() + const removal = removeHostCheckout(root, async () => true, controller.signal).catch( + (error: unknown) => error + ) + await vi.advanceTimersByTimeAsync(10_000) + expect(fs.unlink).not.toHaveBeenCalled() + controller.abort() + pending.resolve() + await vi.runAllTimersAsync() + expect(await removal).toMatchObject({ name: 'AbortError' }) + expect(fs.readdir).toHaveBeenCalledOnce() +}) + +it.each([10, 100])( + 'bounds N=%i root entries across child failure and the rmdir-only tail', + async (width) => { + const children = Array.from({ length: width }, (_, index) => `child-${index}`) + vi.mocked(fs.readdir).mockResolvedValue([...children, '.git']) + let attempts = 0 + vi.mocked(fs.rm).mockImplementation(async (target) => { + if (String(target).endsWith('child-0') && ++attempts < 3) { + throw busy + } + }) + let rootAttempts = 0 + vi.mocked(fs.rmdir).mockImplementation(async () => { + throw Object.assign(new Error('new marker sentinel'), { + code: ++rootAttempts === 1 ? 'EBUSY' : 'ENOTEMPTY' + }) + }) + const authorize = vi.fn(async () => true) + const removal = removeHostCheckout(root, authorize).catch((error: unknown) => error) + await vi.runAllTimersAsync() + expect(String(await removal)).toContain('folder was left on disk') + expect(fs.rm).toHaveBeenCalledTimes(3 * width) + expect(fs.rmdir).toHaveBeenCalledTimes(3) + expect(fs.unlink).toHaveBeenCalledOnce() + expect(fs.readdir).toHaveBeenCalledTimes(3) + expect(authorize).toHaveBeenCalledTimes(6) + } +) + +it('does not mistake Windows rmdir ENOENT on an existing file for absence', async () => { + const missing = Object.assign(new Error('not a directory'), { code: 'ENOENT' }) + vi.mocked(fs.rmdir).mockRejectedValue(missing) + await expect(removeHostEmptyDirectory(root)).rejects.toBe(missing) + const denied = Object.assign(new Error('unreadable replacement'), { code: 'EACCES' }) + vi.mocked(fs.lstat).mockRejectedValue(denied) + await expect(removeHostEmptyDirectory(root)).rejects.toBe(denied) + vi.mocked(fs.lstat).mockRejectedValue(missing) + await expect(removeHostEmptyDirectory(root)).resolves.toBeUndefined() +}) diff --git a/src/main/host-tree-removal.ts b/src/main/host-tree-removal.ts index 4031254cbef..54f7776bcd1 100644 --- a/src/main/host-tree-removal.ts +++ b/src/main/host-tree-removal.ts @@ -1,7 +1,9 @@ // Host deletion needs bounded Windows lock retries and unpatched fs for Electron’s .asar handling. -import { win32 } from 'node:path' -import { rm } from './asar-transparent-fs' +import { getErrorCode } from './git/worktree-operation-options' +import { unregisteredFolderRefusal } from '../shared/worktree/removal' +import { join, win32 } from 'node:path' +import { lstat, readdir, rm, rmdir, unlink } from './asar-transparent-fs' import { isWindowsAbsolutePathLike } from '../shared/cross-platform-path' import { isWslUncPath } from '../shared/wsl-paths' import { stripWin32LongPathPrefix } from '../shared/windows-path-access' @@ -34,6 +36,81 @@ export async function removeHostTree( } } +/** Keep checkout proof out of recursive walks; one shared budget covers children and the final rmdir. */ +export async function removeHostCheckout( + targetPath: string, + authorize: (checkClean: boolean) => Promise, + signal?: AbortSignal +): Promise { + const root = toHostRemovalPath(targetPath) + let markerGone = false + try { + await removeTree(root, { + signal, + rm: async () => { + if (markerGone) { + await authorize(false) + } else { + markerGone = !(await authorize(true)) + if (!markerGone) { + const names = (await readdir(root)).filter((name) => name.toLowerCase() !== '.git') + const results = await Promise.allSettled( + names.map((name) => rm(join(root, name), { recursive: true, force: true })) + ) + const failures = results.filter((result) => result.status === 'rejected') + const failure = + failures.find( + ({ reason }) => !['EBUSY', 'EPERM', 'ENOTEMPTY'].includes(reason?.code) + ) ?? failures[0] + if (failure) { + throw failure.reason + } + // Child promises settled; their internal Node sibling walks may still be running. + markerGone = !(await authorize(false)) + if (!markerGone) { + try { + await unlink(join(root, '.git')) + } catch (error) { + if (getErrorCode(error) !== 'ENOENT') { + throw error + } + } + markerGone = true + } + } + } + await removeHostEmptyDirectory(root) + } + }) + } catch (error) { + if (markerGone && getErrorCode(error) === 'ENOTEMPTY') { + throw unregisteredFolderRefusal(targetPath) + } + throw describeRemovalFailure(error) + } +} + +export async function removeHostEmptyDirectory(targetPath: string): Promise { + const root = toHostRemovalPath(targetPath) + try { + await rmdir(root) + } catch (error) { + if (getErrorCode(error) !== 'ENOENT') { + throw error + } + // Windows rmdir can report ENOENT for an existing regular-file replacement. + try { + await lstat(root) + } catch (statError) { + if (getErrorCode(statError) === 'ENOENT') { + return + } + throw statError + } + throw 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 diff --git a/src/main/ipc/worktrees-removal-recovery.test.ts b/src/main/ipc/worktrees-removal-recovery.test.ts index 094e70d2f9a..c04f7c1b7a8 100644 --- a/src/main/ipc/worktrees-removal-recovery.test.ts +++ b/src/main/ipc/worktrees-removal-recovery.test.ts @@ -1,4 +1,13 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import { execFile } from 'node:child_process' +import { promisify } from 'node:util' +import * as diskFs from 'node:fs/promises' +import * as gitRunner from '../git/runner' +import { removeWorktree } from '../git/worktree' +import { removeWorktree as removeNativeWorktree } from '../git/worktree-removal' +import { readTranslatedWorktreeGraph } from '../git/worktree-list-reader' +import { toHostFilesystemPath } from '../host-tree-removal' +import { removeTree } from '../../shared/windows-transient-lock-removal' import * as scopedRootsInvalidation from './registered-worktree-roots-scoped-invalidation' import * as authorizedRootsCache from './registered-worktree-roots-cache' import * as localWorktreeFilesystem from '../local-worktree-filesystem' @@ -45,9 +54,10 @@ vi.mock('electron', async () => vi.mock('../git/worktree', async () => (await import('./worktrees-test-module-mocks')).gitWorktreeModuleMock() ) -vi.mock('../git/runner', async () => - (await import('./worktrees-test-module-mocks')).gitRunnerModuleMock() -) +vi.mock('../git/runner', async (importOriginal) => ({ + ...(await importOriginal()), + ...(await import('./worktrees-test-module-mocks')).gitRunnerModuleMock() +})) vi.mock('../git/repo', async () => (await import('./worktrees-test-module-mocks')).gitRepoModuleMock() ) @@ -116,6 +126,7 @@ vi.mock('./pty/provider/state-cleanup', async () => (await import('./worktrees-test-module-mocks')).ptyStateCleanupModuleMock() ) +const actualGitRunner = await vi.importActual('../git/runner') const anyCatalogVersion = { epoch: expect.any(String), sequence: expect.any(Number) } describe('registerWorktreeHandlers', () => { @@ -416,166 +427,63 @@ describe('registerWorktreeHandlers', () => { } ) - it('recovers forced Windows long-path worktree removal through local deletion and prune', async () => { - setPlatform('win32') - const parentDir = await mkdtemp(join(tmpdir(), 'orca-ipc-long-path-')) - const repoPath = join(parentDir, 'repo') - const worktreePath = join(parentDir, 'feature-wt') - await mkdir(worktreePath, { recursive: true }) - await writeFile(join(worktreePath, 'scratch.txt'), 'delete me') - const registeredWorktrees = mockKnownFeatureWorktree(worktreePath, repoPath) - listWorktreesMock - .mockResolvedValueOnce(registeredWorktrees) - .mockResolvedValueOnce(registeredWorktrees) - .mockResolvedValue([]) - store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta()) - const longPathError = Object.assign(new Error('git worktree remove failed'), { - stderr: 'error: failed to delete deep/file.txt: Filename too long' - }) - removeWorktreeMock.mockRejectedValue(longPathError) - const worktreeId = `repo-1::${worktreePath}` + it.skipIf(ORIGINAL_PLATFORM !== 'win32')( + 'force-deletes a real registered Windows long-path checkout through desktop Delete', + async () => { + const parentDir = await diskFs.realpath(await mkdtemp(join(tmpdir(), 'orca-ipc-long-path-'))) + const repoPath = join(parentDir, 'repo') + const worktreePath = join(parentDir, 'feature-wt') + const worktreeId = `repo-1::${worktreePath}` + const git = async (args: string[]) => + (await promisify(execFile)('git', args, { cwd: repoPath })).stdout + try { + await mkdir(repoPath) + await git(['init', '-q', '-b', 'main']) + await git(['config', 'user.email', 'removal@example.invalid']) + await git(['config', 'user.name', 'Worktree Removal']) + await git(['config', 'maintenance.auto', 'false']) + await git(['config', 'gc.auto', '0']) + await git(['commit', '--allow-empty', '-qm', 'seed']) + await git(['worktree', 'add', '-q', '-b', 'feature', worktreePath]) + const deep = join(worktreePath, ...Array.from({ length: 6 }, () => 'deep-'.repeat(10))) + await mkdir(toHostFilesystemPath(deep), { recursive: true }) + await writeFile(toHostFilesystemPath(join(deep, 'scratch.txt')), 'delete me') + expect(join(deep, 'scratch.txt').length).toBeGreaterThan(260) + expect((await lstat(join(worktreePath, '.git'))).isFile()).toBe(true) + const admin = (await diskFs.readFile(join(worktreePath, '.git'), 'utf8')).trim().slice(8) + vi.mocked(gitRunner.gitExecFileAsync).mockImplementation(actualGitRunner.gitExecFileAsync) + listWorktreesMock.mockImplementation((repo) => readTranslatedWorktreeGraph(repo)) + vi.mocked(removeWorktree).mockImplementation(removeNativeWorktree) + store.getRepo.mockReturnValue({ ...harnessRepo, path: repoPath, worktreeBaseRef: null }) + store.getRepos.mockReturnValue([{ ...harnessRepo, path: repoPath }]) + store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta()) + store.removeWorktreeMeta.mockImplementation(() => + store.getWorktreeMeta.mockReturnValue(undefined) + ) + expect(await readTranslatedWorktreeGraph(repoPath)).toHaveLength(2) + const head = (await git(['rev-parse', 'feature'])).trim() - try { - const result = await handlers['worktrees:remove'](null, { - worktreeId, - force: true - }) + const result = await handlers['worktrees:remove'](null, { worktreeId, force: true }) - expect(result).toEqual({ - preservedBranch: { branchName: 'feature', head: 'feature' }, - catalogVersion: anyCatalogVersion - }) - if (ORIGINAL_PLATFORM === 'win32') { + expect(result).toEqual({ catalogVersion: anyCatalogVersion }) await expect(lstat(worktreePath)).rejects.toMatchObject({ code: 'ENOENT' }) + await expect(lstat(admin)).rejects.toMatchObject({ code: 'ENOENT' }) + expect(await readTranslatedWorktreeGraph(repoPath)).toHaveLength(1) + expect(await git(['branch', '--list', 'feature'])).toBe('') + expect(store.removeWorktreeMeta).toHaveBeenCalledWith( + hostWorktreeTarget({ id: worktreeId, hostId: 'local' }) + ) + expect(store.getWorktreeMeta(worktreeId)).toBeUndefined() + await expect( + handlers['worktrees:forceDeletePreservedBranch'](null, { + worktreeId, + branchName: 'feature', + expectedHead: head + }) + ).rejects.toThrow('No preserved branch cleanup is pending') + } finally { + await removeTree(toHostFilesystemPath(parentDir)) } - expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'prune'], { - cwd: '/workspace/repo' - }) - expect(store.removeWorktreeMeta).toHaveBeenCalledWith( - hostWorktreeTarget({ id: worktreeId, hostId: 'local' }) - ) - expect(mainWindow.webContents.send).toHaveBeenCalledWith('worktrees:changed', { - repoId: 'repo-1' - }) - } finally { - await rm(parentDir, { recursive: true, force: true }) - } - }) - - it('does not create a preserved-branch target when long-path recovery preserves branch by policy', async () => { - setPlatform('win32') - const registeredWorktrees = mockKnownFeatureWorktree() - listWorktreesMock - .mockResolvedValueOnce(registeredWorktrees) - .mockResolvedValueOnce(registeredWorktrees) - .mockResolvedValue([]) - store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta({ preserveBranchOnDelete: true })) - removeWorktreeMock.mockRejectedValue( - Object.assign(new Error('git worktree remove failed'), { - stderr: 'error: failed to delete deep/file.txt: Filename too long' - }) - ) - - const result = await handlers['worktrees:remove'](null, { - worktreeId: 'repo-1::/workspace/feature-wt', - force: true - }) - - expect(result).toEqual({ catalogVersion: anyCatalogVersion }) - await expect( - handlers['worktrees:forceDeletePreservedBranch'](null, { - worktreeId: 'repo-1::/workspace/feature-wt', - branchName: 'feature', - expectedHead: 'feature' - }) - ).rejects.toThrow('No preserved branch cleanup is pending') - }) - - it('does not recover Windows long-path worktree removal without force', async () => { - setPlatform('win32') - mockKnownFeatureWorktree() - const longPathError = Object.assign(new Error('git worktree remove failed'), { - stderr: 'error: failed to delete deep/file.txt: Filename too long' - }) - removeWorktreeMock.mockRejectedValue(longPathError) - - await expect( - handlers['worktrees:remove'](null, { - worktreeId: 'repo-1::/workspace/feature-wt' - }) - ).rejects.toThrow('Failed to delete worktree at /workspace/feature-wt.') - - expect(store.removeWorktreeMeta).not.toHaveBeenCalled() - }) - - it('refuses Windows recovery while Git still reports the row and keeps metadata', async () => { - setPlatform('win32') - mockKnownFeatureWorktree() - store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta()) - const removePathSpy = vi - .spyOn(localWorktreeFilesystem, 'removeLocalWorktreePath') - .mockResolvedValue(undefined) - removeWorktreeMock.mockRejectedValue( - Object.assign(new Error('git worktree remove failed'), { - stderr: 'error: failed to delete deep/file.txt: Filename too long' - }) - ) - - try { - await expect( - handlers['worktrees:remove'](null, { - worktreeId: 'repo-1::/workspace/feature-wt', - force: true - }) - ).rejects.toThrow( - 'Failed to force delete worktree at /workspace/feature-wt. error: failed to delete deep/file.txt: Filename too long' - ) - - expect(removePathSpy).not.toHaveBeenCalled() - expect(gitExecFileAsyncMock).not.toHaveBeenCalledWith( - ['worktree', 'prune'], - expect.anything() - ) - expect(store.removeWorktreeMeta).not.toHaveBeenCalled() - } finally { - removePathSpy.mockRestore() - } - }) - - it.each([false, true])( - 'retries missing registration cleanup (prunable marker: %s)', - async (prunableMarker) => { - setPlatform('win32') - const missingWorktreePath = prunableMarker - ? 'C:\\workspace\\already-removed\\.git' - : 'C:\\workspace\\already-removed' - const worktreeId = `repo-1::${missingWorktreePath}` - const registeredWorktrees = mockKnownFeatureWorktree(missingWorktreePath).map((row) => - prunableMarker && row.path === missingWorktreePath - ? { ...row, branch: 'refs/heads/feature', prunable: true } - : row - ) - listWorktreesMock.mockResolvedValueOnce(registeredWorktrees).mockResolvedValue([]) - store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta()) - - const result = await handlers['worktrees:remove'](null, { - worktreeId, - force: true - }) - - expect(result).toEqual({ - preservedBranch: { branchName: 'feature', head: 'feature' }, - catalogVersion: anyCatalogVersion - }) - expect(runHookMock).not.toHaveBeenCalled() - expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() - expect(removeWorktreeMock).not.toHaveBeenCalled() - expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'prune'], { - cwd: '/workspace/repo' - }) - expect(store.removeWorktreeMeta).toHaveBeenCalledWith( - hostWorktreeTarget({ id: worktreeId, hostId: 'local' }) - ) } ) diff --git a/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts b/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts index 4d1a127d1ef..5708c4f5901 100644 --- a/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts +++ b/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts @@ -141,10 +141,10 @@ export function buildDetectedGitWorktrees( const settings = store.getSettings() const knownOrcaLayouts = buildKnownOrcaWorkspaceLayouts(settings, repo) const isLegacyRepoForVisibility = isLegacyRepoForExternalWorktreeVisibility(repo) - // Prunable registrations lack working directories; omit from display while keeping cleanup discovery intact (#8389). + // Keep failed removals retryable; other prunable registrations stay hidden (#8389). const liveWorktrees = dedupeWorktreesByPath( preserveFolderUpgradeWorktreePath(repo, gitWorktrees).filter( - (gitWorktree) => !gitWorktree.prunable + (gitWorktree) => !gitWorktree.prunable || !!gitWorktree.removalError ) ) const worktreeVisibilitySourceMatcher = createWorktreeVisibilitySourceMatcher( diff --git a/src/main/ipc/worktrees/removal/execute-worktree-removal.ts b/src/main/ipc/worktrees/removal/execute-worktree-removal.ts index 680d86e9bb3..76a9e857edf 100644 --- a/src/main/ipc/worktrees/removal/execute-worktree-removal.ts +++ b/src/main/ipc/worktrees/removal/execute-worktree-removal.ts @@ -9,6 +9,11 @@ import { getLocalProjectWorktreeGitOptions } from '../../../project-runtime-git- import { listWorktreesStrict as listGitWorktreesStrict } from '../../../git/worktree' import { SshHostNotServedError } from '../../../providers/ssh-host-not-served-error' import { resolveWorktreeRemovalMetadata } from '../../../worktree-removal-repo-owner' +import { + authorizeUnregisteredWindowsRemoval, + nativeWindowsMarkerMissing, + usesNativeWindowsRemoval +} from '../../../git/windows-worktree-removal' import { isPrunableGitFileWorktree } from '../../../worktree-prunable-git-file' import { findRegisteredDeletableWorktree } from '../../../worktree-removal-safety' import { assertNestedWorktreeRemovalApproval } from '../../../nested-worktree-removal-plan' @@ -69,11 +74,19 @@ export async function executeWorktreeRemoval( args.expectedCheckout ]) } + await authorizeUnregisteredWindowsRemoval( + !!registeredWorktree, + repo.path, + worktreePath, + args.force ?? false, + localWorktreeGitOptions + ) if ( await retryFailedRemovalUnlessRegistered( args.worktreeId, worktreePath, registeredWorktrees, + localWorktreeGitOptions, () => retryFailedLocalWorktreeRemoval(context, args, removalHostId) ) ) { @@ -105,12 +118,13 @@ export async function executeWorktreeRemoval( // Stale registrations have no checkout to archive; missing cwd must not block metadata cleanup. if ( - (await isPrunableGitFileWorktree(registeredWorktree, localWorktreeGitOptions)) || - (args.force === true && - process.platform === 'win32' && - (isWindowsAbsolutePathLike(canonicalWorktreePath) || !!localWorktreeGitOptions.wslDistro) && - removedMeta && - (await isAlreadyRemovedWorktreePath(canonicalWorktreePath, localWorktreeGitOptions))) + !usesNativeWindowsRemoval(canonicalWorktreePath, localWorktreeGitOptions) && + ((await isPrunableGitFileWorktree(registeredWorktree, localWorktreeGitOptions)) || + (args.force === true && + process.platform === 'win32' && + (isWindowsAbsolutePathLike(canonicalWorktreePath) || !!localWorktreeGitOptions.wslDistro) && + removedMeta && + (await isAlreadyRemovedWorktreePath(canonicalWorktreePath, localWorktreeGitOptions)))) ) { const removalResult = await removeStaleLocalWorktreeRegistration({ canonicalWorktreePath, @@ -151,7 +165,11 @@ export async function executeWorktreeRemoval( // Archive failure must refuse before PTY teardown or checkout deletion (#19334). let archiveHookOverride: ArchiveHookOverride | undefined - if (archiveScript && !args.skipArchive) { + if ( + archiveScript && + !args.skipArchive && + !(await nativeWindowsMarkerMissing(canonicalWorktreePath, localWorktreeGitOptions)) + ) { await withWorktreeRemoveStageSpan('archive_hook', 'local', async () => { const result = await runHook( 'archive', diff --git a/src/main/ipc/worktrees/removal/registered-local-worktree-preflight.ts b/src/main/ipc/worktrees/removal/registered-local-worktree-preflight.ts new file mode 100644 index 00000000000..9e0bef57dfe --- /dev/null +++ b/src/main/ipc/worktrees/removal/registered-local-worktree-preflight.ts @@ -0,0 +1,51 @@ +import type { GitWorktreeInfo } from '../../../../shared/worktree/types' +import type { LocalProjectWorktreeGitOptions } from '../../../project-runtime-git-options' +import { + authorizeWindowsCheckout, + usesNativeWindowsRemoval +} from '../../../git/windows-worktree-removal' +import { assertWorktreeCleanForRemoval } from '../../../git/worktree' +import { formatWorktreeRemovalError, isOrphanCompatiblePreflightError } from '../../worktree-logic' + +export async function assertRegisteredLocalWorktreePreflight( + repoPath: string, + canonicalWorktreePath: string, + worktree: GitWorktreeInfo, + force: boolean, + localOptions: LocalProjectWorktreeGitOptions, + hasLocalOptions: boolean, + ignoredLinkedPaths: readonly string[], + markerMissing: boolean +): Promise { + try { + if (usesNativeWindowsRemoval(canonicalWorktreePath, localOptions)) { + await authorizeWindowsCheckout( + repoPath, + canonicalWorktreePath, + force, + { + ...localOptions, + knownRemovedWorktree: worktree, + ignoredUntrackedPaths: ignoredLinkedPaths + }, + true + ) + } else if (!markerMissing) { + await (hasLocalOptions + ? assertWorktreeCleanForRemoval(canonicalWorktreePath, force, { + ...localOptions, + ...(ignoredLinkedPaths.length > 0 ? { ignoredUntrackedPaths: ignoredLinkedPaths } : {}) + }) + : ignoredLinkedPaths.length > 0 + ? assertWorktreeCleanForRemoval(canonicalWorktreePath, force, { + ignoredUntrackedPaths: ignoredLinkedPaths + }) + : assertWorktreeCleanForRemoval(canonicalWorktreePath, force)) + } + } catch (error) { + if (!isOrphanCompatiblePreflightError(error)) { + throw new Error(formatWorktreeRemovalError(error, canonicalWorktreePath, force)) + } + // Git may discover orphaning after preflight; recursive fallback still requires strict PTY teardown. + } +} diff --git a/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts b/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts index cac2641ee74..7cd7941d0b7 100644 --- a/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts +++ b/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts @@ -1,4 +1,9 @@ +import { assertRegisteredLocalWorktreePreflight } from './registered-local-worktree-preflight' import type { Repo } from '../../../../shared/repo-types' +import { + nativeWindowsMarkerMissing, + usesNativeWindowsRemoval +} from '../../../git/windows-worktree-removal' import { pruneGitWorktrees } from '../../../git/worktree-prune-guard' import type { ExecutionHostId } from '../../../../shared/execution-host' import type { RemoveWorktreeResult } from '../../../../shared/worktree/create-types' @@ -6,7 +11,6 @@ import type { GitPushTarget, GitWorktreeInfo } from '../../../../shared/worktree import { assertWorktreeUnlockedForRemoval } from '../../../../shared/worktree/removal' import type { LocalProjectWorktreeGitOptions } from '../../../project-runtime-git-options' import { - assertWorktreeCleanForRemoval, listWorktreesStrict as listGitWorktreesStrict, removeWorktree } from '../../../git/worktree' @@ -24,11 +28,7 @@ import { } from '../../worktree-symlinks' import { invalidateAuthorizedRootsCacheForRepo } from '../../registered-worktree-roots-scoped-invalidation' import { runWorktreeChangeInvalidators } from '../../worktree-change-invalidators' -import { - formatWorktreeRemovalError, - isOrphanCompatiblePreflightError, - isOrphanedWorktreeError -} from '../../worktree-logic' +import { formatWorktreeRemovalError, isOrphanedWorktreeError } from '../../worktree-logic' import type { RemoveWorktreeArgs } from '../ipc-context-schemas' import type { WorktreeIpcContext } from '../worktree-ipc-context' import { @@ -88,23 +88,20 @@ export async function removeRegisteredLocalWorktree( const ignoredLinkedPaths = args.force ? [] : await findExistingWorktreeSymlinkPaths(canonicalWorktreePath, linkedPaths) - try { - await (hasLocalWorktreeGitOptions - ? assertWorktreeCleanForRemoval(canonicalWorktreePath, args.force ?? false, { - ...localWorktreeGitOptions, - ...(ignoredLinkedPaths.length > 0 ? { ignoredUntrackedPaths: ignoredLinkedPaths } : {}) - }) - : ignoredLinkedPaths.length > 0 - ? assertWorktreeCleanForRemoval(canonicalWorktreePath, args.force ?? false, { - ignoredUntrackedPaths: ignoredLinkedPaths - }) - : assertWorktreeCleanForRemoval(canonicalWorktreePath, args.force ?? false)) - } catch (error) { - if (!isOrphanCompatiblePreflightError(error)) { - throw new Error(formatWorktreeRemovalError(error, canonicalWorktreePath, args.force ?? false)) - } - // Git may discover orphaning after preflight; recursive fallback still requires strict PTY teardown. - } + const markerMissing = await nativeWindowsMarkerMissing( + canonicalWorktreePath, + localWorktreeGitOptions + ) + await assertRegisteredLocalWorktreePreflight( + repo.path, + canonicalWorktreePath, + refreshedRegisteredWorktree, + args.force ?? false, + localWorktreeGitOptions, + hasLocalWorktreeGitOptions, + ignoredLinkedPaths, + markerMissing + ) const removalGate = await withWorktreeRemoveStageSpan('watcher_gate', 'local', async () => runtime.acquireFileWatcherRemoval(canonicalWorktreePath) @@ -118,7 +115,7 @@ export async function removeRegisteredLocalWorktree( }) }) - if (linkedPaths.length > 0) { + if (!markerMissing && linkedPaths.length > 0) { await removeWorktreeLinkedPaths(canonicalWorktreePath, linkedPaths) } accepted = true @@ -233,6 +230,9 @@ async function finishLocalWorktreeRemoval({ refreshedRegisteredWorktree.head ) } catch (error) { + if (usesNativeWindowsRemoval(canonicalWorktreePath, localWorktreeGitOptions)) { + throw error + } // Git for Windows may deregister before recursive filesystem deletion fails. const recoveredRemovalResult = await recoverLocalWindowsWorktreeRemoval({ error, diff --git a/src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts b/src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts index 4bbe90d5b30..bfd211750a1 100644 --- a/src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts +++ b/src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts @@ -1,3 +1,5 @@ +import { removeWorktree } from '../../../git/worktree-removal' +import { usesNativeWindowsRemoval } from '../../../git/windows-worktree-removal' import type { Repo } from '../../../../shared/repo-types' import type { ExecutionHostId } from '../../../../shared/execution-host' import type { RemoveWorktreeResult } from '../../../../shared/worktree/create-types' @@ -60,22 +62,29 @@ export async function removeUnregisteredWorktree( toLocalWorktreeRuntimePath(worktreePath, localWorktreeGitOptions), toLocalWorktreeRuntimePath(repo.path, localWorktreeGitOptions), removalHome, - access.statPath, - access.readPath + access )) } - if (canCleanOrphanedDirectory) { + const native = usesNativeWindowsRemoval(worktreePath, localWorktreeGitOptions) + if (canCleanOrphanedDirectory || (native && removedMeta)) { assertWorktreeDoesNotContainRegisteredWorktree(worktreePath, registeredWorktrees) - if (!args.force) { + if (canCleanOrphanedDirectory && !args.force) { throw new Error(ORPHANED_WORKTREE_DIRECTORY_MESSAGE) } + const removalGate = await runtime.acquireFileWatcherRemoval(worktreePath) let removalCompleted = false try { await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId, { allowUnverifiedStop: args.allowUnverifiedPtyStop }) - await removeLocalWorktreePath(worktreePath, localWorktreeGitOptions) + await (usesNativeWindowsRemoval(worktreePath, localWorktreeGitOptions) + ? removeWorktree(repo.path, worktreePath, args.force ?? false, { + ...localWorktreeGitOptions, + deleteBranch: false, + knownRemovedWorktree: null + }) + : removeLocalWorktreePath(worktreePath, localWorktreeGitOptions)) removalCompleted = true } finally { await removalGate.finish(removalCompleted) diff --git a/src/main/ipc/worktrees/removal/retry-failed-local-worktree-removal.ts b/src/main/ipc/worktrees/removal/retry-failed-local-worktree-removal.ts index 4f16df430ca..0dadb2c6cad 100644 --- a/src/main/ipc/worktrees/removal/retry-failed-local-worktree-removal.ts +++ b/src/main/ipc/worktrees/removal/retry-failed-local-worktree-removal.ts @@ -5,6 +5,7 @@ import { interruptedLocalWorktreeRemovalJob } from '../../../runtime/runtime-int import { invalidateAuthorizedRootsCacheForRepo } from '../../registered-worktree-roots-scoped-invalidation' import type { RemoveWorktreeArgs } from '../ipc-context-schemas' import type { WorktreeIpcContext } from '../worktree-ipc-context' +import type { WorktreeRemovalRecord } from '../../../worktree-removal-records' import { preserveBranchHeadFallback, rememberPreservedBranchCleanupTarget @@ -21,7 +22,7 @@ export function retryFailedLocalWorktreeRemoval( removalHostId: ExecutionHostId ): Promise | undefined { const { store, runtime, options } = context - return retryFailedWorktreeRemoval(args.worktreeId, removalHostId, (record) => + const jobFor = (record: WorktreeRemovalRecord) => interruptedLocalWorktreeRemovalJob(record, { store, acquireWatcherRemoval: (path) => runtime.acquireFileWatcherRemoval(path), @@ -55,5 +56,5 @@ export function retryFailedLocalWorktreeRemoval( options?.onWorktreeLifecycle?.({ kind: 'removed', worktreeId, path: worktreePath }), publish: (repoId) => runtime.publishWorktreeRemovalChange(repoId) }) - ) + return retryFailedWorktreeRemoval(args.worktreeId, removalHostId, jobFor, args.force ?? false) } diff --git a/src/main/local-orphaned-worktree-cleanup.ts b/src/main/local-orphaned-worktree-cleanup.ts index 9ed666e5021..90e3cec0a2a 100644 --- a/src/main/local-orphaned-worktree-cleanup.ts +++ b/src/main/local-orphaned-worktree-cleanup.ts @@ -24,8 +24,7 @@ export async function cleanupLocalOrphanedWorktreeDirectory( runtimePath, toLocalWorktreeRuntimePath(repoPath, options), CLIENT_REMOVAL_HOME, - access.statPath, - access.readPath + access ) ) { await closeWatchers(path) diff --git a/src/main/local-worktree-filesystem.test.ts b/src/main/local-worktree-filesystem.test.ts index 1ff40664bdc..4585d5f1859 100644 --- a/src/main/local-worktree-filesystem.test.ts +++ b/src/main/local-worktree-filesystem.test.ts @@ -1,11 +1,15 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -const { runProcessMock, lstatMock, readFileMock, rmMock } = vi.hoisted(() => ({ - runProcessMock: vi.fn(), - lstatMock: vi.fn(), - readFileMock: vi.fn(), - rmMock: vi.fn() -})) +const { runProcessMock, lstatMock, statMock, realpathMock, readFileMock, rmMock } = vi.hoisted( + () => ({ + runProcessMock: vi.fn(), + lstatMock: vi.fn(), + statMock: vi.fn(), + realpathMock: vi.fn(), + readFileMock: vi.fn(), + rmMock: vi.fn() + }) +) // Process-host tests own encoding, timeouts, and console hiding; assert guest commands here. vi.mock('@orca/process-host', () => ({ @@ -14,6 +18,8 @@ vi.mock('@orca/process-host', () => ({ vi.mock('node:fs/promises', () => ({ lstat: lstatMock, + stat: statMock, + realpath: realpathMock, readFile: readFileMock, rm: rmMock })) @@ -54,6 +60,8 @@ describe('local worktree filesystem runtime access', () => { beforeEach(() => { runProcessMock.mockReset() lstatMock.mockReset() + statMock.mockReset() + realpathMock.mockReset() readFileMock.mockReset() rmMock.mockReset() completeExecFile() @@ -65,15 +73,21 @@ describe('local worktree filesystem runtime access', () => { }) it('uses host filesystem operations when no WSL distro is selected', async () => { - lstatMock.mockResolvedValue({ type: 'file' }) + lstatMock.mockResolvedValue({ type: 'symlink' }) + statMock.mockResolvedValue({ type: 'directory' }) + realpathMock.mockResolvedValue('C:\\metadata') readFileMock.mockResolvedValue('gitdir: ../.git/worktrees/feature') const access = getLocalWorktreePathAccess() - await access.statPath('C:\\repo\\.git') + await expect(access.statPath('C:\\repo\\.git')).resolves.toEqual({ type: 'symlink' }) + await expect(access.layoutStatPath('C:\\repo\\.git')).resolves.toEqual({ type: 'directory' }) + await expect(access.realPath('C:\\repo\\.git')).resolves.toBe('C:\\metadata') await access.readPath('C:\\repo\\.git') await removeLocalWorktreePath('C:\\repo\\feature') expect(lstatMock).toHaveBeenCalledWith('C:\\repo\\.git') + expect(statMock).toHaveBeenCalledWith('C:\\repo\\.git') + expect(realpathMock).toHaveBeenCalledWith('C:\\repo\\.git') expect(readFileMock).toHaveBeenCalledWith('C:\\repo\\.git', 'utf8') expect(rmMock).toHaveBeenCalledWith( toHostRemovalPath('C:\\repo\\feature'), @@ -170,19 +184,41 @@ describe('local worktree filesystem runtime access', () => { it('uses the selected WSL distro for stat, read, and removal on Windows', async () => { await withPlatform('win32', async () => { - completeExecFile('regular file') + completeExecFile('symbolic link') const access = getLocalWorktreePathAccess({ wslDistro: 'Ubuntu' }) - await expect(access.statPath('/home/me/repo/.git')).resolves.toEqual({ type: 'file' }) - - completeExecFile('gitdir: /home/me/repo/.git/worktrees/feature\n') - await expect(access.readPath('/home/me/repo/.git')).resolves.toBe( - 'gitdir: /home/me/repo/.git/worktrees/feature\n' + await expect(access.statPath('/home/me/repo/.git')).resolves.toEqual({ type: 'symlink' }) + completeExecFile('directory') + await expect(access.layoutStatPath('/home/me/repo/.git')).resolves.toEqual({ + type: 'directory' + }) + expect(runProcessMock.mock.calls.slice(0, 2).map(([request]) => request.args.at(-1))).toEqual( + [ + "LC_ALL=C stat -c %F -- '/home/me/repo/.git'", + "LC_ALL=C stat -L -c %F -- '/home/me/repo/.git'" + ] ) + completeExecFile('/home/me/metadata with spaces \n') + await expect(access.realPath('/home/me/repo/.git')).resolves.toBe( + '/home/me/metadata with spaces ' + ) + expect(runProcessMock.mock.calls[2]?.[0].args.at(-1)).toBe( + "LC_ALL=C realpath -e -- '/home/me/repo/.git'" + ) + completeExecFile(' gitdir: /home/me/repo with spaces/.git/worktrees/feature \n') + await expect(access.readPath("/home/me/repo's file")).resolves.toBe( + ' gitdir: /home/me/repo with spaces/.git/worktrees/feature \n' + ) + expect(runProcessMock.mock.calls[3]?.[0]).toMatchObject({ + program: 'wsl.exe', + args: ['-d', 'Ubuntu', '--exec', 'sh', '-c', "LC_ALL=C cat -- '/home/me/repo'\\''s file'"], + timeoutMs: 30_000 + }) + completeExecFile() await removeLocalWorktreePath('C:\\Users\\me\\repo feature', { wslDistro: 'Ubuntu' }) - expect(runProcessMock).toHaveBeenCalledTimes(3) + expect(runProcessMock).toHaveBeenCalledTimes(5) expect(runProcessMock).toHaveBeenNthCalledWith( 1, expect.objectContaining({ @@ -192,9 +228,11 @@ describe('local worktree filesystem runtime access', () => { cwd: expect.any(String) }) ) - 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'`) + const removeArgs: unknown = runProcessMock.mock.calls[4]?.[0].args + if (!Array.isArray(removeArgs) || !removeArgs.every((arg) => typeof arg === 'string')) { + throw new Error('Expected WSL removal command arguments') + } + expect(removeArgs.at(-1)).toBe(String.raw`rm -rf -- '/mnt/c/Users/me/repo feature'`) expect(rmMock).not.toHaveBeenCalled() }) }) @@ -210,6 +248,12 @@ describe('local worktree filesystem runtime access', () => { expect(args).not.toContain('-lc') expect(args).not.toContain('-ilc') expect(args.at(-1)).not.toContain('getent passwd') + await expect( + getLocalWorktreePathAccess({ wslDistro: 'Ubuntu' }).layoutStatPath('/home/me/repo/.git') + ).resolves.toEqual({ type: 'other' }) + await expect( + getLocalWorktreePathAccess({ wslDistro: 'Ubuntu' }).realPath('/home/me/repo/.git') + ).rejects.toThrow('Invalid WSL canonical path') }) }) @@ -224,9 +268,9 @@ describe('local worktree filesystem runtime access', () => { failExecFile(1, stderr) const access = getLocalWorktreePathAccess({ wslDistro: 'Ubuntu' }) - await expect(access.statPath('/mnt/c/repo/missing/.git')).rejects.toMatchObject({ - code: 'ENOENT' - }) + for (const inspect of [access.statPath, access.layoutStatPath, access.realPath]) { + await expect(inspect('/mnt/c/repo/missing/.git')).rejects.toMatchObject({ code: 'ENOENT' }) + } await expect(isWorktreePathMissing('/mnt/c/repo/missing', access.statPath)).resolves.toBe( true ) @@ -243,7 +287,9 @@ describe('local worktree filesystem runtime access', () => { failExecFile(1, stderr) const access = getLocalWorktreePathAccess({ wslDistro: 'Ubuntu' }) - await expect(access.statPath('/mnt/c/repo/locked')).rejects.toThrow('Permission denied') + for (const inspect of [access.statPath, access.layoutStatPath, access.realPath]) { + await expect(inspect('/mnt/c/repo/locked')).rejects.toThrow('Permission denied') + } await expect(isWorktreePathMissing('/mnt/c/repo/locked', access.statPath)).resolves.toBe( false ) @@ -251,6 +297,20 @@ describe('local worktree filesystem runtime access', () => { } ) + it('normalizes only definitive cat absence, preserving path-bearing read failures', async () => { + await withPlatform('win32', async () => { + const access = getLocalWorktreePathAccess({ wslDistro: 'Ubuntu' }) + const path = '/mnt/c/ENOENT/repo/.git/commondir' + // Synthetic cat-shaped diagnostics exercise the same transport as the recorded stat errors. + for (const reason of ['No such file or directory', 'Not a directory']) { + failExecFile(1, `cat: ${path}: ${reason}\n`) + await expect(access.readPath(path)).rejects.toMatchObject({ code: 'ENOENT' }) + } + failExecFile(1, `cat: ${path}: Permission denied\n`) + await expect(access.readPath(path)).rejects.not.toMatchObject({ code: 'ENOENT' }) + }) + }) + it('pins the stat probe to the C locale so its error text stays English', async () => { await withPlatform('win32', async () => { completeExecFile('directory') @@ -267,9 +327,14 @@ describe('local worktree filesystem runtime access', () => { failExecFile(1, "stat: '/mnt/c/repo/missing': \n") const access = getLocalWorktreePathAccess({ wslDistro: 'Ubuntu' }) - await expect(access.statPath('/mnt/c/repo/missing')).rejects.not.toMatchObject({ - code: 'ENOENT' - }) + for (const inspect of [ + access.statPath, + access.layoutStatPath, + access.realPath, + access.readPath + ]) { + await expect(inspect('/mnt/c/repo/missing')).rejects.not.toMatchObject({ code: 'ENOENT' }) + } await expect(isWorktreePathMissing('/mnt/c/repo/missing', access.statPath)).resolves.toBe( false ) diff --git a/src/main/local-worktree-filesystem.ts b/src/main/local-worktree-filesystem.ts index bd7af6eb116..f66c2548648 100644 --- a/src/main/local-worktree-filesystem.ts +++ b/src/main/local-worktree-filesystem.ts @@ -1,12 +1,13 @@ +import { posix } from 'node:path' import { runProcess } from '@orca/process-host' -import { lstat, readFile } from 'node:fs/promises' +import { lstat, stat, readFile, realpath } from 'node:fs/promises' import { buildWslExecArgs } from '../shared/wsl-login-shell-command' import { quotePosixShell } from '../shared/shell-argument-quoting' import { removeHostTree } from './host-tree-removal' import { toLinuxPath } from './wsl' import { resolveWslInteropSpawnCwd } from './wsl-interop-spawn-directory' import { describeWslProcessFailure } from './wsl/wsl-process-failure' -import type { ReadPath, StatPath } from './worktree-orphan-gitdir-proof' +import type { WorktreeProofAccess } from './worktree-orphan-gitdir-proof' export { toHostFilesystemPath, toHostRemovalPath } from './host-tree-removal' @@ -14,11 +15,6 @@ export type LocalWorktreeFilesystemOptions = { wslDistro?: string } -type LocalWorktreePathAccess = { - statPath: StatPath - readPath: ReadPath -} - const WSL_FILE_OPERATION_TIMEOUT_MS = 30_000 function shouldUseWslFilesystem(options: LocalWorktreeFilesystemOptions): boolean { @@ -66,42 +62,55 @@ export function toLocalWorktreeRuntimePath( export function getLocalWorktreePathAccess( options: LocalWorktreeFilesystemOptions = {} -): LocalWorktreePathAccess { +): WorktreeProofAccess { const distro = options.wslDistro?.trim() if (!shouldUseWslFilesystem(options) || !distro) { return { statPath: lstat, - readPath: (path) => readFile(path, 'utf8') + layoutStatPath: stat, + readPath: (path) => readFile(path, 'utf8'), + realPath: realpath } } - return { - statPath: async (path) => { - const target = quotePosixShell(toLinuxPath(path)) - // Shell file tests conflate permission failures with absence; stat preserves the reason. - const stdout = await runWslCommand(distro, `LC_ALL=C stat -c %F -- ${target}`).catch( - (error: unknown) => { - if (isWslMissingPathError(error)) { - throw Object.assign(new Error(`missing ${path}`), { code: 'ENOENT' }) - } - throw error - } - ) - const kind = stdout.trim() - return { - type: - kind === 'symbolic link' - ? 'symlink' - : kind === 'regular file' || kind === 'regular empty file' - ? 'file' - : kind === 'directory' - ? 'directory' - : 'other' + const inspect = (path: string, command: string) => + runWslCommand(distro, command).catch((error: unknown) => { + if (isWslMissingPathError(error)) { + throw Object.assign(new Error(`missing ${path}`), { code: 'ENOENT' }) } + throw error + }) + const statPath = async (path: string, follow: boolean) => { + const target = quotePosixShell(toLinuxPath(path)) + // Shell file tests conflate permission failures with absence; stat preserves the reason. + const stdout = await inspect(path, `LC_ALL=C stat ${follow ? '-L ' : ''}-c %F -- ${target}`) + const kind = stdout.trim() + return { + type: + kind === 'symbolic link' + ? 'symlink' + : kind === 'regular file' || kind === 'regular empty file' + ? 'file' + : kind === 'directory' + ? 'directory' + : 'other' + } + } + return { + statPath: (path) => statPath(path, false), + layoutStatPath: (path) => statPath(path, true), + realPath: async (path) => { + const target = quotePosixShell(toLinuxPath(path)) + const stdout = await inspect(path, `LC_ALL=C realpath -e -- ${target}`) + const resolved = stdout.replace(/\r?\n$/, '') + if (!posix.isAbsolute(resolved) || /[\0\r\n]/.test(resolved)) { + throw new Error('Invalid WSL canonical path') + } + return resolved }, readPath: async (path) => { const target = quotePosixShell(toLinuxPath(path)) - const stdout = await runWslCommand(distro, `cat -- ${target}`) + const stdout = await inspect(path, `LC_ALL=C cat -- ${target}`) return stdout } } diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-02.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-02.spec.ts index 0bb0ea230b4..f4d532d2dfb 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-02.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-02.spec.ts @@ -1,8 +1,9 @@ import { describe, expect, it, vi } from 'vitest' +import { readFile } from 'node:fs/promises' +import { removeWorktree as removeNativeWorktree } from '../../git/worktree-removal' import { MOCK_GIT_WORKTREES, - ORIGINAL_PLATFORM, OrcaRuntimeService, WATCHER_REMOVAL_DRAIN_BUDGET_MS, assertWorktreeCleanForRemoval, @@ -17,7 +18,6 @@ import { listWorktrees, listWorktreesStrict, localWorktreeFilesystem, - lstat, mkdir, removeWorktree, removeWorktreeLinkedPathsMock, @@ -256,9 +256,13 @@ describe('OrcaRuntimeService', () => { } }) - it('recovers forced Windows runtime long-path removal and keeps skipped-hook warnings', async () => { + it('refuses a nonempty markerless Windows folder before teardown and keeps skipped-hook warnings', async () => { setPlatform('win32') - const runtime = createWorktreeRemovalRuntime() + const { runtimeStore, removeWorktreeMeta } = createStaleRuntimeWorktreeStore(TEST_WORKTREE_ID) + const runtime = createWorktreeRemovalRuntime(runtimeStore) + deleteWorktreeHistoryDirMock.mockClear() + const teardown = vi.spyOn(runtime, 'acquireFileWatcherRemoval') + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}) await mkdir(TEST_WORKTREE_PATH, { recursive: true }) await writeFile(join(TEST_WORKTREE_PATH, 'scratch.txt'), 'delete me') const gitSpy = vi.spyOn(gitRunner, 'gitExecFileAsync').mockResolvedValue({ @@ -281,26 +285,30 @@ describe('OrcaRuntimeService', () => { .mockResolvedValue([]) try { - const result = await runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true }) - - expect(result).toEqual({ - preservedBranch: { branchName: 'feature/foo', head: 'abc' }, - warning: `orca.yaml archive hook skipped for ${TEST_WORKTREE_PATH}; pass --run-hooks to run it.` - }) - expect(gitSpy).toHaveBeenCalledWith(['worktree', 'prune'], { - cwd: TEST_REPO_PATH - }) - if (ORIGINAL_PLATFORM === 'win32') { - await expect(lstat(TEST_WORKTREE_PATH)).rejects.toMatchObject({ code: 'ENOENT' }) - } - expect(deleteWorktreeHistoryDirMock).toHaveBeenCalledWith(TEST_WORKTREE_ID) + await expect( + runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true }) + ).rejects.toThrow('is no longer a Git worktree, so its folder was left on disk.') + expect(warning).toHaveBeenCalledWith( + `[hooks] orca.yaml archive hook skipped for ${TEST_WORKTREE_PATH}; pass --run-hooks to run it.` + ) + expect(teardown).not.toHaveBeenCalled() + expect(runHook).not.toHaveBeenCalled() + expect(removeWorktree).not.toHaveBeenCalled() + expect(removeWorktreeMeta).not.toHaveBeenCalled() + expect(deleteWorktreeHistoryDirMock).not.toHaveBeenCalled() + expect(closeLocalWatcherForWorktreePathMock).not.toHaveBeenCalled() + expect(gitSpy).not.toHaveBeenCalledWith(['worktree', 'prune'], expect.anything()) + expect(await readFile(join(TEST_WORKTREE_PATH, 'scratch.txt'), 'utf8')).toBe('delete me') + expect(runtimeStore.getWorktreeMeta(TEST_WORKTREE_ID)).toBeDefined() } finally { + warning.mockRestore() + teardown.mockRestore() gitSpy.mockRestore() await rm(TEST_WORKTREE_PATH, { recursive: true, force: true }) } }) - it('refuses runtime Windows recovery while Git still reports the row and keeps metadata', async () => { + it('passes through native Windows removal failures without recursive recovery or metadata cleanup', async () => { setPlatform('win32') const removeWorktreeMeta = vi.fn() const runtimeStore = { @@ -316,17 +324,20 @@ describe('OrcaRuntimeService', () => { .spyOn(localWorktreeFilesystem, 'removeLocalWorktreePath') .mockResolvedValue(undefined) vi.mocked(getEffectiveHooks).mockReturnValue(null) - vi.mocked(removeWorktree).mockRejectedValue( - Object.assign(new Error('git worktree remove failed'), { - stderr: 'error: failed to delete deep/file.txt: Filename too long' - }) + const path = join(TEST_WORKTREE_PATH, 'deep', 'file.txt') + const cause = Object.assign(new Error('permission denied'), { code: 'EACCES', path }) + const failure = Object.assign( + new Error( + `${path} could not be deleted; it may be read-only or open in another program. Close any program using it and delete again.`, + { cause } + ), + { code: 'EACCES', path } ) + vi.mocked(removeWorktree).mockRejectedValue(failure) try { - await expect( - runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true }) - ).rejects.toThrow( - `Failed to force delete worktree at ${TEST_WORKTREE_PATH}. error: failed to delete deep/file.txt: Filename too long` + await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true })).rejects.toBe( + failure ) expect(removePathSpy).not.toHaveBeenCalled() expect(gitSpy).not.toHaveBeenCalledWith(['worktree', 'prune'], expect.anything()) @@ -337,7 +348,7 @@ describe('OrcaRuntimeService', () => { } }) - it('retries stale runtime Git registration cleanup after prior filesystem recovery', async () => { + it('releases only the missing native checkout registration and preserves its branch and host metadata policy', async () => { setPlatform('win32') const missingWorktreePath = 'C:\\workspace\\already-removed' const worktreeId = `${TEST_REPO_ID}::${missingWorktreePath}` @@ -359,12 +370,23 @@ describe('OrcaRuntimeService', () => { isMainWorktree: false } ] - const gitSpy = vi.spyOn(gitRunner, 'gitExecFileAsync').mockResolvedValue({ - stdout: '', - stderr: '' + const gitSpy = vi.spyOn(gitRunner, 'gitExecFileAsync').mockImplementation(async (args) => { + if (args[0] === 'branch') { + throw new Error('branch is not fully merged') + } + return { + stdout: + args[0] === 'worktree' && args[1] === 'list' + ? registeredWorktrees + .map((row) => `worktree ${row.path}\0HEAD ${row.head}\0branch ${row.branch}\0\0`) + .join('') + : '', + stderr: '' + } }) + vi.mocked(removeWorktree).mockImplementation(removeNativeWorktree) vi.mocked(listWorktrees).mockResolvedValue(registeredWorktrees) - vi.mocked(listWorktreesStrict).mockResolvedValueOnce(registeredWorktrees).mockResolvedValue([]) + vi.mocked(listWorktreesStrict).mockResolvedValue(registeredWorktrees) vi.mocked(getEffectiveHooks).mockReturnValue({ scripts: { archive: 'pnpm worktree:archive' @@ -375,13 +397,21 @@ describe('OrcaRuntimeService', () => { const result = await runtime.removeManagedWorktree(worktreeId, { force: true }) expect(result).toEqual({ - preservedBranch: { branchName: 'feature/foo', head: 'abc' } + preservedBranch: { branchName: 'feature/foo', head: 'abc' }, + warning: `orca.yaml archive hook skipped for ${missingWorktreePath}; pass --run-hooks to run it.` }) expect(runHook).not.toHaveBeenCalled() - expect(removeWorktree).not.toHaveBeenCalled() - expect(gitSpy).toHaveBeenCalledWith(['worktree', 'prune'], { - cwd: TEST_REPO_PATH - }) + expect(removeWorktree).toHaveBeenCalledWith( + TEST_REPO_PATH, + missingWorktreePath, + true, + expect.objectContaining({ knownRemovedWorktree: registeredWorktrees[1] }) + ) + expect(gitSpy).toHaveBeenCalledWith( + ['-c', 'core.longpaths=true', 'worktree', 'remove', '--force', '--', missingWorktreePath], + expect.objectContaining({ cwd: TEST_REPO_PATH, admissionExempt: true }) + ) + expect(gitSpy).not.toHaveBeenCalledWith(['worktree', 'prune'], expect.anything()) // Host-qualified metadata purge preserves same-id rows on other hosts. expect(removeWorktreeMeta).toHaveBeenCalledWith( hostWorktreeTarget({ id: worktreeId, hostId: 'local' }) diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-execution-host.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-execution-host.spec.ts index 3727b586b25..28030090b17 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-execution-host.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-execution-host.spec.ts @@ -179,6 +179,7 @@ class RemovalRuntime extends OrcaRuntimeService { { kind: 'local' }, { id: record.worktreeId, path: record.worktreePath }, [], + {}, {} ) } diff --git a/src/main/runtime/runtime-failed-local-worktree-removal.test.ts b/src/main/runtime/runtime-failed-local-worktree-removal.test.ts index d476f6a1747..c5fe89560ab 100644 --- a/src/main/runtime/runtime-failed-local-worktree-removal.test.ts +++ b/src/main/runtime/runtime-failed-local-worktree-removal.test.ts @@ -120,10 +120,15 @@ function jobHost(purged: string[], stopPtys = vi.fn(async () => {}), pushTarget? /** Delete on the row, as desktop and runtime run it: Git's listing first, then the retry. */ async function deleteRow(purged: string[], stopPtys = vi.fn(async () => {})): Promise { const listed = await listWorktreesStrict(repoPath) - const retried = await retryFailedRemovalUnlessRegistered(worktreeId, worktreePath, listed, () => - retryFailedWorktreeRemoval(worktreeId, 'local', (record) => - interruptedLocalWorktreeRemovalJob(record, jobHost(purged, stopPtys)) - ) + const retried = await retryFailedRemovalUnlessRegistered( + worktreeId, + worktreePath, + listed, + {}, + () => + retryFailedWorktreeRemoval(worktreeId, 'local', (record) => + interruptedLocalWorktreeRemovalJob(record, jobHost(purged, stopPtys)) + ) ).catch((reason: unknown) => reason) if (retried !== true) { return retried diff --git a/src/main/runtime/runtime-interrupted-local-worktree-removal.test.ts b/src/main/runtime/runtime-interrupted-local-worktree-removal.test.ts index d4de3fa740c..2aeea4743e3 100644 --- a/src/main/runtime/runtime-interrupted-local-worktree-removal.test.ts +++ b/src/main/runtime/runtime-interrupted-local-worktree-removal.test.ts @@ -2,17 +2,36 @@ import { installRealWorkerThreadChildProcesses } from '../worker-thread-child-pr // Use real Git/disk evidence to finish interrupted removal regardless of how far the earlier run progressed. import { execFile } from 'node:child_process' import { existsSync } from 'node:fs' -import { mkdir, mkdtemp, readFile, realpath, unlink, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, readFile, realpath, rename, unlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { promisify } from 'node:util' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { GitPushTarget } from '../../shared/worktree/types' import type { Repo } from '../../shared/repo-types' +import { getDefaultSettings } from '../../shared/constants' +import { getProjectHostSetupWorktreeMeta } from '../../shared/project-host-setup-lookup' +import { buildDetectedGitWorktrees } from '../ipc/worktrees/listing/ssh-worktree-fallback' import { removeTree } from '../../shared/windows-transient-lock-removal' import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' import type { Store } from '../persistence' import type * as HostTreeRemoval from '../host-tree-removal' import { removeHostTree } from '../host-tree-removal' +import * as nativeFs from '../asar-transparent-fs' +import * as diskFs from 'node:fs/promises' +import * as nativeRemoval from '../git/windows-worktree-removal' +import * as graphReader from '../git/worktree-list-reader' +import { cleanupLocalOrphanedWorktreeDirectory } from '../local-orphaned-worktree-cleanup' +import * as removalOwnership from '../ipc/worktrees/removal/worktree-removal-ownership' +import { executeWorktreeRemoval } from '../ipc/worktrees/removal/execute-worktree-removal' +import { RuntimeWorktreeRemovalInFlight } from './runtime-worktree-removal-in-flight' +import { OrcaRuntimeWithResolveWorktreeRemovalTarget } from './workspaces/workspace-removal-target' +import type { WorktreeIpcContext } from '../ipc/worktrees/worktree-ipc-context' +import * as backgroundRemoval from '../worktree-background-removal' +import * as worktreeGit from '../git/worktree' +import * as worktreeRemote from '../ipc/worktree-remote' +import { getLocalProjectWorktreeGitOptions } from '../project-runtime-git-options' +import * as removalTable from '../worktree-removal-table' import { listWorktreesStrict } from '../git/worktree' import { areWorktreePathsEqual } from '../git/worktree-path-comparison' import { @@ -35,14 +54,23 @@ import { } from '../worktree-removal-records' import { interruptedLocalWorktreeRemovalJob } from './runtime-interrupted-local-worktree-removal' +vi.mock('node:fs/promises', async (importOriginal) => { + return { ...(await importOriginal()) } +}) + vi.mock('../project-runtime-git-options', () => ({ - getLocalProjectWorktreeGitOptions: () => ({}) + getLocalProjectWorktreeGitOptions: vi.fn(() => ({})) })) vi.mock('../host-tree-removal', async (importOriginal) => { const actual = await importOriginal() return { ...actual, removeHostTree: vi.fn(actual.removeHostTree) } }) +vi.mock('../ipc/worktrees/removal/worktree-removal-ownership', () => ({ + stopPtysForDestructiveWorktreeRemoval: vi.fn(async () => {}), + removeWorktreeMetadataAndTransientState: vi.fn() +})) + const execFileAsync = promisify(execFile) let scratchDir = '' @@ -50,6 +78,7 @@ let recordsDir = '' let repoPath = '' let worktreePath = '' let repo: Repo +let pushTarget: GitPushTarget | undefined async function git(args: string[], cwd = repoPath): Promise { const { stdout } = await execFileAsync('git', args, { cwd }) @@ -64,6 +93,8 @@ async function isRegistered(path: string): Promise { } beforeEach(async () => { + pushTarget = undefined + vi.mocked(getLocalProjectWorktreeGitOptions).mockReturnValue({}) // realpath: macOS hands out /var/... temp paths while Git reports /private/var/.... scratchDir = await realpath(await mkdtemp(join(tmpdir(), 'orca-interrupted-removal-'))) recordsDir = join(scratchDir, 'profile') @@ -83,7 +114,9 @@ beforeEach(async () => { afterEach(async () => { _resetPendingWorktreeRemovalsForTests() + vi.restoreAllMocks() vi.mocked(removeHostTree).mockClear() + vi.mocked(removalOwnership.stopPtysForDestructiveWorktreeRemoval).mockReset() await removeTree(scratchDir) }) @@ -92,7 +125,13 @@ type FinishOutcome = | { status: 'failed'; error: string } async function finishAfterRestart( - options: { repoGone?: boolean; head?: string; force?: boolean } = {} + options: { + repoGone?: boolean + branch?: string + head?: string + force?: boolean + stopPtys?: () => Promise + } = {} ): Promise<{ outcome: FinishOutcome purged: string[] @@ -104,7 +143,7 @@ async function finishAfterRestart( repoId: 'repo-1', repoPath, worktreePath, - branch: 'feature', + branch: options.branch ?? 'feature', head: options.head ?? (await git(['rev-parse', 'feature'])).trim(), deleteBranch: true, force: options.force ?? false, @@ -119,32 +158,15 @@ async function finishAfterRestart( expect(() => beginTerminalInstall(worktreePath)).toThrow(/being removed/) expect(() => beginWatcherInstall(worktreePath)).toThrow(/being removed/) - const storeStub = { - getRepo: (id: string) => (id === repo.id && !options.repoGone ? repo : undefined), - getRepos: () => (options.repoGone ? [] : [repo]), - getWorktreeMeta: () => undefined - } const purged: string[] = [] const remember = vi.fn() + const host = removalHost(options.repoGone, options.stopPtys) + host.purge = ({ worktreeId }) => { + purged.push(worktreeId) + } + host.preservedBranchCleanup.remember = remember resumeInterruptedWorktreeRemovals((interrupted) => - interruptedLocalWorktreeRemovalJob(interrupted, { - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the finish reads only repos and worktree metadata from the store here; git options and push-target cleanup are stubbed or short-circuit without a push target. - store: storeStub as unknown as Store, - // Takes the real gate synchronously, as the runtime's does. - acquireWatcherRemoval: async (path) => { - const gate = acquireWatcherRemovalGate(path) - expect(() => beginTerminalInstall(worktreePath)).toThrow(/being removed/) - return { finish: async () => gate.release() } - }, - closeWatchers: async () => {}, - preservedBranchCleanup: { - preserveHead: (result) => result ?? {}, - remember - }, - purge: ({ worktreeId }) => purged.push(worktreeId), - onRemoved: () => {}, - publish: () => {} - }) + interruptedLocalWorktreeRemovalJob(interrupted, host) ) const outcome: FinishOutcome = await joined!.then( (result) => ({ status: 'removed' as const, ...result }), @@ -160,39 +182,394 @@ async function finishAfterRestart( /** The non-main rows a listing shows, with any failed delete's error. */ async function listedRows(): Promise<{ path: string; removalError?: string }[]> { - return (await withUnregisteredRemovalCheckouts(repo.id, await listWorktreesStrict(repoPath))) - .filter((row) => !row.isMainWorktree) + const rows = await withUnregisteredRemovalCheckouts(repo.id, await listWorktreesStrict(repoPath)) + return buildDetectedGitWorktrees(removalHost().store, repo, rows) + .filter((row) => row.visible && !row.isMainWorktree) .map(({ path, removalError }) => ({ path, ...(removalError ? { removalError } : {}) })) } +function removalHost( + repoGone = false, + stopPtys = async () => {}, + teardown = () => {} +): Parameters[1] & { store: Store } { + const store = { + getRepo: (id: string) => (id === repo.id && !repoGone ? repo : undefined), + getRepos: () => (repoGone ? [] : [repo]), + getWorktreeMeta: () => ({ + ...getProjectHostSetupWorktreeMeta([], repo), + instanceId: 'removal-fixture', + orcaCreatedAt: 1, + orcaCreationSource: 'runtime', + pushTarget + }), + getWorktreeMetaForHost: () => store.getWorktreeMeta(), + getSettings: () => getDefaultSettings(scratchDir), + getProjectHostSetups: () => [], + getAllWorktreeMeta: () => ({}) + } + return { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the removal/listing paths read only the supplied store methods; the fixture owns all process/session services. + store: store as unknown as Store, + acquireWatcherRemoval: async (path) => { + teardown() + const gate = acquireWatcherRemovalGate(path) + return { finish: async () => gate.release() } + }, + closeWatchers: vi.fn(async () => teardown()), + stopPtys, + preservedBranchCleanup: { preserveHead: (result) => result ?? {}, remember: vi.fn() }, + purge: () => expect(existsSync(worktreePath)).toBe(false), + onRemoved: () => {}, + publish: () => {} + } +} + +const deletionOperations = () => + (['rm', 'unlink', 'rmdir'] as const).map((operation) => vi.spyOn(nativeFs, operation)) + +function expectNoTeardownOrDeletion( + ...callbacks: (ReturnType | ReturnType[number])[] +): void { + callbacks.forEach((callback) => expect(callback).not.toHaveBeenCalled()) +} + +type RemovalEntry = 'desktop' | 'runtime' + +async function deleteViaEntry( + entry: RemovalEntry, + force: boolean, + stop = async () => {}, + teardown = vi.fn() +): Promise { + const worktreeId = `repo-1::${worktreePath}` + const host = removalHost(false, stop, teardown) + const { store } = host + const runtime: OrcaRuntimeWithResolveWorktreeRemovalTarget = Object.assign( + Object.create(OrcaRuntimeWithResolveWorktreeRemovalTarget.prototype), + { + store, + requireStore: () => store, + resolveWorktreeRemovalTarget: async () => ({ + id: worktreeId, + repoId: repo.id, + path: worktreePath + }), + removeManagedWorktreeInFlight: new RuntimeWorktreeRemovalInFlight(), + localRemovalJobHost: () => host, + acquireFileWatcherRemoval: host.acquireWatcherRemoval, + stopPtysForDestructiveWorktreeRemoval: stop, + closeFileWatchersForRemoval: host.closeWatchers, + preservedBranchCleanup: { ...host.preservedBranchCleanup, delete: vi.fn() }, + purgeRemovedWorktree: host.purge, + clearOptimisticReconcileToken: () => {}, + publishWorktreeRemovalChange: () => {}, + notifyWorktreesChanged: () => {}, + emitWorktreeLifecycle: () => {} + } + ) + vi.mocked(removalOwnership.stopPtysForDestructiveWorktreeRemoval).mockImplementation(stop) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: native dispatch reads only the fixture-owned context fields below. + const context = { + store, + runtime, + mainWindow: { isDestroyed: () => true } + } as unknown as WorktreeIpcContext + try { + const accepted = + entry === 'desktop' + ? await executeWorktreeRemoval( + context, + { worktreeId, force }, + repo, + repo.id, + worktreePath, + 'local' + ) + : await runtime.removeManagedWorktree(`id:${worktreeId}`, { + force, + waitForBackgroundRemoval: true + }) + return await backgroundRemoval.finishAcceptedWorktreeRemoval(accepted, worktreeId) + } finally { + await _settlePendingWorktreeRemovalsForTests() + } +} + +async function orphanAdmin(): Promise { + const admin = (await readFile(join(worktreePath, '.git'), 'utf8')).trim().replace(/^gitdir: /, '') + await removeTree(admin) + await unlink(join(worktreePath, '.git')) + await writeFile(join(worktreePath, '.git'), ` gitdir: ${admin}\nignored second line\n`) +} + +async function recordFailedRemoval(force: boolean): Promise { + await writeWorktreeRemovalRecords(recordsDir, () => [ + { + worktreeId: `repo-1::${worktreePath}`, + repoId: repo.id, + repoPath, + worktreePath, + branch: 'feature', + head: '', + deleteBranch: true, + force, + requestedAt: 1, + failure: { message: 'held file', failedAt: 2 } + } + ]) + await loadWorktreeRemovalRecords(recordsDir) +} + describe('finishing an interrupted worktree removal after a restart', () => { - it('finishes a forced delete Git was still deleting, branch and metadata included', async () => { - // Git stopped partway: part of the checkout is gone, which reads as local changes. - await unlink(join(worktreePath, 'seed.txt')) - - const { outcome, purged, records } = await finishAfterRestart({ force: true }) - - expect(outcome).toMatchObject({ status: 'removed' }) - expect(records).toEqual([]) - expect( - outcome && 'preservedBranch' in outcome ? outcome.preservedBranch : undefined - ).toBeUndefined() - expect(existsSync(worktreePath)).toBe(false) - expect(await isRegistered(worktreePath)).toBe(false) - expect(await git(['branch', '--list', 'feature'])).toBe('') - expect(purged).toEqual([`repo-1::${worktreePath}`]) + it('refuses an unknown owner through both entries, admission, cleanup and failed listing', async () => { + await orphanAdmin() + const movedOwner = join(scratchDir, 'ENOENT') + await rename(repoPath, movedOwner) + repoPath = movedOwner + repo.path = repoPath + await writeFile(join(repoPath, 'HEAD'), 'ref: refs/heads/main\n') + await writeFile( + join(worktreePath, '.git'), + `gitdir: ${join(repoPath, 'worktrees', 'feature')}\n` + ) + // Successful graph seams isolate proof propagation, not a live permission exploit. + vi.spyOn(worktreeGit, 'listWorktreesStrict').mockResolvedValue([]) + vi.spyOn(graphReader, 'readTranslatedWorktreeGraph').mockResolvedValue([]) + const stat = diskFs.stat + vi.spyOn(diskFs, 'stat').mockImplementation(async (path, options) => { + if (String(path) === join(repoPath, '.git')) { + throw Object.assign(new Error(`EACCES: permission denied, stat '${String(path)}'`), { + code: 'EACCES' + }) + } + return stat(path, options) + }) + const stop = vi.fn(async () => {}) + const teardown = vi.fn() + const operations = deletionOperations() + await expect( + nativeRemoval.removeWindowsCheckout(repoPath, worktreePath, true, {}) + ).rejects.toThrow('Refusing to delete unregistered worktree path') + // Reach both nonnative unregistered consumers after independently checking native admission. + vi.spyOn(nativeRemoval, 'usesNativeWindowsRemoval').mockReturnValue(false) + const metadata = vi.mocked(removalOwnership.removeWorktreeMetadataAndTransientState) + metadata.mockClear() + const pushCleanup = vi.spyOn(worktreeRemote, 'cleanupUnusedWorktreePushTargetRemote') + for (const entry of ['desktop', 'runtime'] as const) { + await expect(deleteViaEntry(entry, true, stop, teardown)).rejects.toThrow( + 'Refusing to delete unregistered worktree path' + ) + } + await expect( + cleanupLocalOrphanedWorktreeDirectory(repoPath, worktreePath, {}, stop) + ).rejects.toThrow('Worktree is no longer registered with Git but its directory remains.') + expectNoTeardownOrDeletion(stop, teardown, ...operations) + expect(removeHostTree).not.toHaveBeenCalled() + expect(metadata).not.toHaveBeenCalled() + expect(pushCleanup).not.toHaveBeenCalled() + expect(await readFile(join(worktreePath, 'seed.txt'), 'utf8')).toBe('seed\n') + await recordFailedRemoval(true) + expect(await readWorktreeRemovalRecords(recordsDir)).toMatchObject([ + { worktreePath, failure: { message: 'held file' } } + ]) + expect(await listedRows()).toEqual([]) + await vi.waitFor(async () => expect(await readWorktreeRemovalRecords(recordsDir)).toEqual([])) + expect(await readFile(join(worktreePath, 'seed.txt'), 'utf8')).toBe('seed\n') }) - it('finishes a clean checkout with the recorded non-forced choice', async () => { - const { outcome, purged, records } = await finishAfterRestart({ force: false }) + it.skipIf(process.platform !== 'win32').each([ + { entry: 'desktop', phase: 'child' }, + { entry: 'runtime', phase: 'child' }, + { entry: 'desktop', phase: 'root' }, + { entry: 'runtime', phase: 'root' } + ] as const)( + '$entry completes explicit retry after releasing held $phase across restart', + async ({ entry, phase }) => { + const remoteUrl = 'https://example.invalid/repo.git' + pushTarget = { + remoteName: 'pr-contributor', + branchName: 'feature', + remoteCreated: true, + remoteUrl + } + await git(['remote', 'add', pushTarget.remoteName, remoteUrl]) + await git(['config', `remote.${pushTarget.remoteName}.orca-created`, 'true']) + const sibling = join(scratchDir, 'temporarily-missing') + await git(['worktree', 'add', '-q', sibling, '-b', 'sibling']) + await rename(sibling, join(scratchDir, 'offline')) + const path = phase === 'child' ? join(worktreePath, 'seed.txt') : worktreePath + const locked = Object.assign(new Error('held path'), { code: 'EBUSY', path }) + const held = vi.spyOn(nativeFs, phase === 'child' ? 'rm' : 'rmdir').mockRejectedValue(locked) + const { outcome, records } = await finishAfterRestart({ force: true }) + const message = `${path} is open in another program. Close that program and delete again.` + expect(outcome.status).toBe('failed') + expect(records).toMatchObject([{ failure: { message } }]) + if (phase === 'root') { + expect((await listWorktreesStrict(repoPath)).filter((row) => row.prunable)).toHaveLength(2) + } + expect(await listedRows()).toEqual([{ path: worktreePath, removalError: message }]) + expect(await git(['remote'])).toContain(pushTarget.remoteName) + expect(await isRegistered(worktreePath)).toBe(true) + expect(await git(['branch', '--list', 'feature'])).not.toBe('') + _resetPendingWorktreeRemovalsForTests() + await loadWorktreeRemovalRecords(recordsDir) + expect(await listedRows()).toEqual([{ path: worktreePath, removalError: message }]) + const resume = vi.fn() + resumeInterruptedWorktreeRemovals(resume) + expect(resume).not.toHaveBeenCalled() + await expect(deleteViaEntry(entry, true)).rejects.toThrow() + expect(await isRegistered(worktreePath)).toBe(true) + held.mockRestore() + await deleteViaEntry(entry, true) + expect(existsSync(worktreePath)).toBe(false) + expect(await isRegistered(worktreePath)).toBe(false) + expect(await git(['branch', '--list', 'feature'])).toBe('') + expect(await git(['remote'])).not.toContain(pushTarget.remoteName) + expect(await isRegistered(sibling)).toBe(true) + expect(await readWorktreeRemovalRecords(recordsDir)).toEqual([]) + }, + 60_000 + ) - expect(outcome).toMatchObject({ status: 'removed' }) - expect(records).toEqual([]) - expect(existsSync(worktreePath)).toBe(false) - expect(await isRegistered(worktreePath)).toBe(false) - expect(await git(['branch', '--list', 'feature'])).toBe('') - expect(purged).toEqual([`repo-1::${worktreePath}`]) - }) + it + .skipIf(process.platform !== 'win32') + .each( + (['desktop', 'runtime'] as const).flatMap((entry) => + ['native', 'wsl'].map((route) => ({ entry, route })) + ) + )( + '$entry refuses $route before publication/teardown and honors current native force', + async ({ entry, route }) => { + const seed = await readFile(join(worktreePath, 'seed.txt')) + await orphanAdmin() + if (route === 'wsl') { + await unlink(join(worktreePath, '.git')) + vi.mocked(getLocalProjectWorktreeGitOptions).mockReturnValue({ wslDistro: 'Ubuntu' }) + // A route-list seam, not a distro run: the request must refuse before invoking a job. + vi.spyOn(worktreeGit, 'listWorktreesStrict').mockResolvedValue([]) + } + const retryJob = vi.spyOn(backgroundRemoval, 'retryFailedWorktreeRemoval') + const pushCleanup = vi.spyOn(worktreeRemote, 'cleanupUnusedWorktreePushTargetRemote') + await recordFailedRemoval(true) + const stop = vi.fn(async () => {}) + const teardown = vi.fn() + const operations = deletionOperations() + for (let attempt = 0; attempt < 2; attempt += 1) { + await expect(deleteViaEntry(entry, false, stop, teardown)).rejects.toThrow( + /no longer (?:registered|a Git worktree)/ + ) + expect(retryJob).not.toHaveBeenCalled() + expect(pushCleanup).not.toHaveBeenCalled() + expectNoTeardownOrDeletion(stop, teardown, ...operations) + expect(await readFile(join(worktreePath, 'seed.txt'))).toEqual(seed) + expect(await readWorktreeRemovalRecords(recordsDir)).toMatchObject([ + { force: true, failure: { message: 'held file' } } + ]) + _resetPendingWorktreeRemovalsForTests() + await loadWorktreeRemovalRecords(recordsDir) + } + if (route === 'wsl') { + return + } + const sibling = join(scratchDir, 'temporarily-missing') + await git(['worktree', 'add', '-q', sibling, '-b', 'sibling']) + await rename(sibling, join(scratchDir, 'offline')) + await recordFailedRemoval(false) + const started = Promise.withResolvers() + const paused = Promise.withResolvers() + const retry = deleteViaEntry(entry, true, async () => { + started.resolve() + await paused.promise + }).catch((error: unknown) => error) + try { + await started.promise + expect([...removalTable.pendingWorktreeRemovals.values()]).toMatchObject([ + { force: true, branch: 'feature', deleteBranch: true } + ]) + expect(await readWorktreeRemovalRecords(recordsDir)).toMatchObject([{ force: true }]) + } finally { + paused.resolve() + } + expect(await retry).not.toBeInstanceOf(Error) + expect(await isRegistered(worktreePath)).toBe(false) + expect(await isRegistered(sibling)).toBe(true) + expect(await git(['branch', '--list', 'feature'])).toBe('') + await git(['worktree', 'add', '-q', worktreePath, '-b', 'feature']) + await removeTree(worktreePath) + expect(await isRegistered(worktreePath)).toBe(true) + await deleteViaEntry(entry, true) + expect(await isRegistered(worktreePath)).toBe(false) + expect(await isRegistered(sibling)).toBe(true) + expect(await git(['branch', '--list', 'feature'])).toBe('') + } + ) + + it.skipIf(process.platform !== 'win32').each( + (['desktop', 'runtime'] as const).flatMap((entry) => + ['attached', 'detached', 'folder', 'nested'].map((schedule) => ({ + entry, + schedule + })) + ) + )( + '$entry preserves $schedule replacements before/after teardown', + async ({ entry, schedule }) => { + await orphanAdmin() + await loadWorktreeRemovalRecords(recordsDir) + const change = async () => { + if (schedule === 'nested') { + await git(['worktree', 'add', '-q', join(worktreePath, 'nested'), '-b', 'nested']) + } else { + await rename(worktreePath, join(scratchDir, 'original')) + await (schedule === 'folder' + ? mkdir(worktreePath) + : git([ + 'worktree', + 'add', + '-q', + ...(schedule === 'detached' ? ['--detach'] : ['-b', 'other']), + worktreePath + ])) + } + await writeFile(join(worktreePath, 'notes.txt'), 'mine\n') + } + const stop = vi.fn(change) + const teardown = vi.fn() + await expect(deleteViaEntry(entry, true, stop, teardown)).rejects.toThrow() + expect(await readFile(join(worktreePath, 'notes.txt'), 'utf8')).toBe('mine\n') + expect(stop).toHaveBeenCalledOnce() + // A second ordinary Delete refuses proofless/nested contents before teardown. + if (['folder', 'nested'].includes(schedule)) { + stop.mockClear() + teardown.mockClear() + const operations = deletionOperations() + await expect(deleteViaEntry(entry, true, stop, teardown)).rejects.toThrow() + expectNoTeardownOrDeletion(stop, teardown, ...operations) + expect(await readFile(join(worktreePath, 'notes.txt'), 'utf8')).toBe('mine\n') + } + } + ) + + it.each(['partial', 'detached'])( + 'finishes the accepted %s pending checkout after restart', + async (kind) => { + await (kind === 'partial' + ? unlink(join(worktreePath, 'seed.txt')) + : git(['checkout', '--detach', '-q'], worktreePath)) + const { outcome, purged, records } = await finishAfterRestart({ + force: kind === 'partial', + branch: kind === 'detached' ? '' : 'feature', + head: (await git(['rev-parse', 'HEAD'], worktreePath)).trim() + }) + expect(outcome).toMatchObject({ status: 'removed' }) + expect(records).toEqual([]) + expect(existsSync(worktreePath)).toBe(false) + expect(await isRegistered(worktreePath)).toBe(false) + expect((await git(['branch', '--list', 'feature'])).trim() !== '').toBe(kind === 'detached') + expect(purged).toEqual([`repo-1::${worktreePath}`]) + } + ) it('keeps the user’s non-forced choice: Git refuses local changes and the row returns as before', async () => { vi.spyOn(console, 'warn').mockImplementation(() => {}) @@ -200,20 +577,23 @@ describe('finishing an interrupted worktree removal after a restart', () => { const { outcome, purged, records } = await finishAfterRestart({ force: false }) - expect(outcome).toMatchObject({ status: 'failed', error: expect.stringMatching(/--force/) }) + expect(outcome).toMatchObject({ status: 'failed' }) expect(await readFile(join(worktreePath, 'notes.txt'), 'utf8')).toBe('mine\n') expect(existsSync(join(worktreePath, 'seed.txt'))).toBe(true) expect(await isRegistered(worktreePath)).toBe(true) expect(await git(['branch', '--list', 'feature'])).not.toBe('') expect(purged).toEqual([]) - expect(records).toEqual([]) - expect(await listedRows()).toEqual([{ path: worktreePath }]) + expect(records).toMatchObject([{ failure: { message: expect.any(String) } }]) + expect(await listedRows()).toEqual([{ path: worktreePath, removalError: expect.any(String) }]) }) it.each([true, false])( - 'unregisters a checkout whose .git Git deleted first, keeping its files (force %s)', + 'preserves a nonempty markerless checkout and platform cleanup policy (force %s)', async (force) => { vi.spyOn(console, 'warn').mockImplementation(() => {}) + const admin = (await readFile(join(worktreePath, '.git'), 'utf8')).trim().slice(8) + const backlink = await readFile(join(admin, 'gitdir'), 'utf8') + const branchHead = await git(['rev-parse', 'feature']) // Git deletes in directory order; without `.git` no `worktree remove` can validate it. await unlink(join(worktreePath, '.git')) @@ -221,12 +601,19 @@ describe('finishing an interrupted worktree removal after a restart', () => { const refusal = `${worktreePath} is no longer a Git worktree, so its folder was left on disk. Delete the folder yourself to finish removing this workspace.` expect(outcome).toEqual({ status: 'failed', error: `Error: ${refusal}` }) + const native = process.platform === 'win32' + if (native && force) { + await expect(deleteViaEntry('runtime', force)).rejects.toThrow(refusal) + await removalTable.persistWorktreeRemovalRecords() + expect(await readWorktreeRemovalRecords(recordsDir)).toEqual(records) + expect(await readFile(join(admin, 'gitdir'), 'utf8')).toBe(backlink) + expect(await git(['rev-parse', 'feature'])).toBe(branchHead) + } expect(existsSync(join(worktreePath, '.git'))).toBe(false) - expect(existsSync(join(worktreePath, 'seed.txt'))).toBe(true) + expect(await readFile(join(worktreePath, 'seed.txt'), 'utf8')).toBe('seed\n') expect(removeHostTree).not.toHaveBeenCalled() - expect(await isRegistered(worktreePath)).toBe(false) - // The merged branch goes with the registration, as the delete asked. - expect(await git(['branch', '--list', 'feature'])).toBe('') + expect(await isRegistered(worktreePath)).toBe(native) + expect((await git(['branch', '--list', 'feature'])).trim() !== '').toBe(native) expect(purged).toEqual([]) expect(records).toMatchObject([{ failure: { message: refusal } }]) expect(await listedRows()).toEqual([{ path: worktreePath, removalError: refusal }]) @@ -243,14 +630,14 @@ describe('finishing an interrupted worktree removal after a restart', () => { const { outcome, purged, records } = await finishAfterRestart({ force: false }) - expect(outcome).toMatchObject({ status: 'failed', error: expect.stringMatching(/--force/) }) + expect(outcome).toMatchObject({ status: 'failed' }) expect(await readFile(join(worktreePath, 'kept.txt'), 'utf8')).toBe('kept\n') expect(existsSync(join(worktreePath, '.git'))).toBe(true) expect(await isRegistered(worktreePath)).toBe(true) expect(await git(['branch', '--list', 'feature'])).not.toBe('') expect(purged).toEqual([]) - expect(records).toEqual([]) - expect(await listedRows()).toEqual([{ path: worktreePath }]) + expect(records).toMatchObject([{ failure: { message: expect.any(String) } }]) + expect(await listedRows()).toEqual([{ path: worktreePath, removalError: expect.any(String) }]) }) it('deletes nothing in a folder put at the path after Git unregistered the checkout', async () => { @@ -334,7 +721,7 @@ describe('finishing an interrupted worktree removal after a restart', () => { // Not pruned either: the lock keeps Git's registration. expect(await isRegistered(worktreePath)).toBe(true) expect(purged).toEqual([]) - expect(records).toEqual([]) + expect(records).toMatchObject([{ failure: { message: expect.any(String) } }]) }) it('deletes the branch when Git finished the checkout but the quit came before the branch', async () => { @@ -398,8 +785,8 @@ describe('finishing an interrupted worktree removal after a restart', () => { expect(existsSync(worktreePath)).toBe(true) expect(await isRegistered(worktreePath)).toBe(true) expect(purged).toEqual([]) - expect(records).toEqual([]) - expect(await listedRows()).toEqual([{ path: worktreePath }]) + expect(records).toMatchObject([{ failure: { message: expect.any(String) } }]) + expect(await listedRows()).toEqual([{ path: worktreePath, removalError: expect.any(String) }]) }) it('drops a record whose repo Orca no longer has', async () => { diff --git a/src/main/runtime/runtime-interrupted-local-worktree-removal.ts b/src/main/runtime/runtime-interrupted-local-worktree-removal.ts index 69d5ebdf16d..6696dab5878 100644 --- a/src/main/runtime/runtime-interrupted-local-worktree-removal.ts +++ b/src/main/runtime/runtime-interrupted-local-worktree-removal.ts @@ -1,5 +1,4 @@ import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' -import type { GitWorktreeInfo } from '../../shared/worktree/types' import { LOCAL_EXECUTION_HOST_ID } from '../../shared/execution-host' import type { RuntimeStore } from './runtime-store-contract' import { @@ -10,13 +9,19 @@ import { resolveWorktreeRemovalMetadata } from '../worktree-removal-repo-owner' import type { RuntimePreservedBranchCleanup } from './runtime-preserved-branch-cleanup' import { listWorktreesStrict } from '../git/worktree' import { cleanupUnusedWorktreePushTargetRemote } from '../ipc/worktree-remote' -import { finishUnregisteredWorktreeRemoval } from '../git/worktree-removal' -import { getErrorCode, normalizeLocalBranchRef } from '../git/worktree-operation-options' +import { authorizeWindowsCheckout, usesNativeWindowsRemoval } from '../git/windows-worktree-removal' +import { + deleteBranchOfUnregisteredWorktree, + finishUnregisteredWorktreeRemoval, + removeWorktree +} from '../git/worktree-removal' +import { getErrorCode } from '../git/worktree-operation-options' import { areWorktreePathsEqual } from '../git/worktree-path-comparison' import { getLocalProjectWorktreeGitOptions } from '../project-runtime-git-options' import { findRegisteredDeletableWorktree } from '../worktree-removal-safety' import { assertUnregisteredCheckoutGone, + isRecordedCheckout, differentCheckoutAtPathError } from '../worktree-removal-leftover' import { CLIENT_REMOVAL_HOME, getPathOps } from '../worktree-removal-home-guard' @@ -117,8 +122,7 @@ type InterruptedLocalWorktreeRemovalArgs = Pick< stopSignal: AbortSignal } -/** Finish from Git/disk evidence: registered checkouts use recorded deletion choices; missing .git permits unregister only. - * Never delete an unregistered remaining folder; once it is gone, finish the recorded cleanup. */ +/** Finish from current Git/disk evidence; native markerless roots permit only empty rmdir. */ async function finishInterruptedLocalWorktreeRemoval( args: InterruptedLocalWorktreeRemovalArgs ): Promise { @@ -161,7 +165,17 @@ async function finishInterruptedLocalWorktreeRemoval( if (deletable && !isRecordedCheckout(deletable, record)) { throw differentCheckoutAtPathError(record.worktreePath) } - if (deletable && (await isCheckoutMissingGitLink(deletable.path, localOptions))) { + const native = usesNativeWindowsRemoval(record.worktreePath, localOptions) + const removalOptions = { + ...localOptions, + deleteBranch: false, + knownRemovedWorktree: { + branch: record.branch ? `refs/heads/${record.branch}` : '', + head: record.head + }, + checkoutDeleteSignal: args.stopSignal + } + if (!native && deletable && (await isCheckoutMissingGitLink(deletable.path, localOptions))) { // If Git deleted .git first, it cannot remove/validate the checkout; prune registration only and keep all files for the checks below. await removeStaleLocalWorktreeRegistration({ canonicalWorktreePath: deletable.path, @@ -173,9 +187,18 @@ async function finishInterruptedLocalWorktreeRemoval( deletable = undefined } // Check before teardown so refused folders retain terminals/watchers. - if (!deletable) { + if (!native && !deletable) { await assertUnregisteredCheckoutGone(record.worktreePath, worktrees) } + if (native) { + await authorizeWindowsCheckout( + repo.path, + record.worktreePath, + record.force, + removalOptions, + true + ) + } const gate = await args.acquireWatcherRemoval(record.worktreePath) if (args.stopPtys) { try { @@ -191,14 +214,29 @@ async function finishInterruptedLocalWorktreeRemoval( let result: RemoveWorktreeResult let removed = false try { - result = await finishUnregisteredWorktreeRemoval( - repo.path, - record.worktreePath, - record.deleteBranch && record.branch ? { name: record.branch, head: record.head } : null, - // Recheck after teardown awaits in case a folder appeared meanwhile. - () => assertUnregisteredCheckoutGone(record.worktreePath), - localOptions - ) + if (native) { + await removeWorktree(repo.path, record.worktreePath, record.force, removalOptions) + } + const branch = + record.deleteBranch && record.branch ? { name: record.branch, head: record.head } : null + if (native) { + await assertUnregisteredCheckoutGone(record.worktreePath) + result = await deleteBranchOfUnregisteredWorktree( + repo.path, + record.worktreePath, + branch, + localOptions + ) + } else { + result = await finishUnregisteredWorktreeRemoval( + repo.path, + record.worktreePath, + branch, + // Recheck after teardown awaits in case a folder appeared meanwhile. + () => assertUnregisteredCheckoutGone(record.worktreePath), + localOptions + ) + } removed = true } finally { await gate.finish(removed) @@ -228,10 +266,3 @@ async function isCheckoutMissingGitLink( return getErrorCode(error) === 'ENOENT' } } - -function isRecordedCheckout(worktree: GitWorktreeInfo, record: WorktreeRemovalRecord): boolean { - return ( - normalizeLocalBranchRef(worktree.branch) === record.branch && - (!record.head || worktree.head === record.head) - ) -} diff --git a/src/main/runtime/runtime-registered-local-worktree-removal.ts b/src/main/runtime/runtime-registered-local-worktree-removal.ts index 930a13cf00f..515068dcdbf 100644 --- a/src/main/runtime/runtime-registered-local-worktree-removal.ts +++ b/src/main/runtime/runtime-registered-local-worktree-removal.ts @@ -1,4 +1,9 @@ import type { GitPushTarget, GitWorktreeInfo } from '../../shared/worktree/types' +import { + authorizeWindowsCheckout, + nativeWindowsMarkerMissing, + usesNativeWindowsRemoval +} from '../git/windows-worktree-removal' import { pruneGitWorktrees } from '../git/worktree-prune-guard' import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' import type { ArchiveHookOverride } from '../../shared/worktree/archive-hook-removal-gate' @@ -84,7 +89,11 @@ async function acceptRuntimeRegisteredLocalWorktreeRemoval(args: { let warning: string | undefined // Archive refusal precedes refresh, preflights, PTY stop and Git deletion; failure must leave them untouched (#19334). let archiveHookOverride: ArchiveHookOverride | undefined - if (hooks?.scripts.archive && args.runHooks) { + if ( + hooks?.scripts.archive && + args.runHooks && + !(await nativeWindowsMarkerMissing(canonicalPath, localOptions)) + ) { const result = await runHook( 'archive', canonicalPath, @@ -126,17 +135,32 @@ async function acceptRuntimeRegisteredLocalWorktreeRemoval(args: { const ignoredLinkedPaths = args.force ? [] : await findExistingWorktreeSymlinkPaths(canonicalPath, linkedPaths) + const markerMissing = await nativeWindowsMarkerMissing(canonicalPath, localOptions) try { - await (args.hasLocalOptions - ? assertWorktreeCleanForRemoval(canonicalPath, args.force, { + if (usesNativeWindowsRemoval(canonicalPath, localOptions)) { + await authorizeWindowsCheckout( + repo.path, + canonicalPath, + args.force, + { ...localOptions, - ...(ignoredLinkedPaths.length > 0 ? { ignoredUntrackedPaths: ignoredLinkedPaths } : {}) - }) - : ignoredLinkedPaths.length > 0 + knownRemovedWorktree: refreshed, + ignoredUntrackedPaths: ignoredLinkedPaths + }, + true + ) + } else if (!markerMissing) { + await (args.hasLocalOptions ? assertWorktreeCleanForRemoval(canonicalPath, args.force, { - ignoredUntrackedPaths: ignoredLinkedPaths + ...localOptions, + ...(ignoredLinkedPaths.length > 0 ? { ignoredUntrackedPaths: ignoredLinkedPaths } : {}) }) - : assertWorktreeCleanForRemoval(canonicalPath, args.force)) + : ignoredLinkedPaths.length > 0 + ? assertWorktreeCleanForRemoval(canonicalPath, args.force, { + ignoredUntrackedPaths: ignoredLinkedPaths + }) + : assertWorktreeCleanForRemoval(canonicalPath, args.force)) + } } catch (error) { if (!isOrphanCompatiblePreflightError(error)) { throw new Error(formatWorktreeRemovalError(error, canonicalPath, args.force)) @@ -147,7 +171,7 @@ async function acceptRuntimeRegisteredLocalWorktreeRemoval(args: { let accepted = false try { await args.stopPtys() - if (linkedPaths.length > 0) { + if (!markerMissing && linkedPaths.length > 0) { await removeWorktreeLinkedPaths(canonicalPath, linkedPaths) } accepted = true @@ -222,6 +246,9 @@ export async function finishRuntimeLocalWorktreeRemoval( refreshed.head ) } catch (error) { + if (usesNativeWindowsRemoval(canonicalPath, localOptions)) { + throw error + } const recovered = await recoverLocalWindowsWorktreeRemoval({ error, force: args.force, diff --git a/src/main/runtime/runtime-unregistered-worktree-removal.ts b/src/main/runtime/runtime-unregistered-worktree-removal.ts index e33c0ebdcb2..27dc621ddb5 100644 --- a/src/main/runtime/runtime-unregistered-worktree-removal.ts +++ b/src/main/runtime/runtime-unregistered-worktree-removal.ts @@ -1,3 +1,5 @@ +import { removeWorktree } from '../git/worktree-removal' +import { usesNativeWindowsRemoval } from '../git/windows-worktree-removal' import type { GitPushTarget, GitWorktreeInfo } from '../../shared/worktree/types' import type { Repo } from '../../shared/repo-types' import type { WorktreeMeta } from '../../shared/worktree/meta-types' @@ -53,15 +55,16 @@ export async function removeRuntimeUnregisteredWorktree(args: { toLocalWorktreeRuntimePath(target.path, args.localOptions), toLocalWorktreeRuntimePath(repo.path, args.localOptions), removalHome, - access.statPath, - access.readPath + access )) } - if (canCleanOrphanedDirectory) { + const native = usesNativeWindowsRemoval(target.path, args.localOptions) + if (canCleanOrphanedDirectory || (native && removedMeta)) { assertWorktreeDoesNotContainRegisteredWorktree(target.path, registeredWorktrees) - if (!args.force) { + if (canCleanOrphanedDirectory && !args.force) { throw new Error(ORPHANED_WORKTREE_DIRECTORY_MESSAGE) } + await deleteUnregisteredDirectory(args) args.finishRemoval() return {} @@ -84,7 +87,13 @@ async function deleteUnregisteredDirectory( let completed = false try { await args.stopPtys(args.target.id, args.allowUnverifiedPtyStop) - await removeLocalWorktreePath(args.target.path, args.localOptions) + await (usesNativeWindowsRemoval(args.target.path, args.localOptions) + ? removeWorktree(args.repo.path, args.target.path, args.force, { + ...args.localOptions, + deleteBranch: false, + knownRemovedWorktree: null + }) + : removeLocalWorktreePath(args.target.path, args.localOptions)) completed = true } finally { await gate.finish(completed) diff --git a/src/main/runtime/workspaces/stale-worktree-registration-preflight.ts b/src/main/runtime/workspaces/stale-worktree-registration-preflight.ts new file mode 100644 index 00000000000..1c60bf6024c --- /dev/null +++ b/src/main/runtime/workspaces/stale-worktree-registration-preflight.ts @@ -0,0 +1,32 @@ +import type { GitWorktreeInfo } from '../../../shared/worktree/types' +import type { WorktreeMeta } from '../../../shared/worktree/meta-types' +import type { LocalProjectWorktreeGitOptions } from '../../project-runtime-git-options' +import { assertWorktreeUnlockedForRemoval } from '../../../shared/worktree/removal' +import { formatWorktreeRemovalError } from '../../ipc/worktree-logic' +import { usesNativeWindowsRemoval } from '../../git/windows-worktree-removal' +import { isPrunableGitFileWorktree } from '../../worktree-prunable-git-file' +import { isWindowsAbsolutePathLike } from '../../../shared/cross-platform-path' +import { isLocalRuntimeWorktreePathMissing } from '../runtime-worktree-filesystem' + +export async function isStaleLocalWorktreeRegistration( + registeredWorktree: GitWorktreeInfo, + force: boolean, + removedMeta: WorktreeMeta | undefined, + localWorktreeGitOptions: LocalProjectWorktreeGitOptions +): Promise { + const canonicalWorktreePath = registeredWorktree.path + try { + assertWorktreeUnlockedForRemoval(registeredWorktree) + } catch (error) { + throw new Error(formatWorktreeRemovalError(error, canonicalWorktreePath, force)) + } + return ( + !usesNativeWindowsRemoval(canonicalWorktreePath, localWorktreeGitOptions) && + ((await isPrunableGitFileWorktree(registeredWorktree, localWorktreeGitOptions)) || + (force === true && + process.platform === 'win32' && + (isWindowsAbsolutePathLike(canonicalWorktreePath) || !!localWorktreeGitOptions.wslDistro) && + removedMeta && + (await isLocalRuntimeWorktreePathMissing(canonicalWorktreePath, localWorktreeGitOptions)))) + ) +} diff --git a/src/main/runtime/workspaces/workspace-removal-target.ts b/src/main/runtime/workspaces/workspace-removal-target.ts index ab356d12dc7..3f5187174ac 100644 --- a/src/main/runtime/workspaces/workspace-removal-target.ts +++ b/src/main/runtime/workspaces/workspace-removal-target.ts @@ -6,6 +6,7 @@ import type { RuntimeWorktreeRemovalTarget } from '../runtime-worktree-selection' import { resolveRuntimeWorktreeRemovalTarget } from '../runtime-worktree-removal-target' +import type { LocalProjectWorktreeGitOptions } from '../../project-runtime-git-options' import type { RuntimeStore } from '../runtime-store-contract' import { splitWorktreeId, worktreeIdsEqual } from '../../../shared/worktree/id' import { hasWorktreeRemovalRepoOwnerOnOtherHost } from '../../worktree-removal-repo-owner' @@ -87,12 +88,13 @@ export abstract class OrcaRuntimeWithResolveWorktreeRemovalTarget extends OrcaRu return waitForPendingWorktreeRemoval(worktreeId, parseExecutionHostId(options.hostId)?.id) } - /** Finish a failed local removal only while Git still omits the path and the directory is gone; refuse while it exists. */ + /** Failed intent schedules a retry; current Git/filesystem proof still controls every deletion. */ protected async retryFailedRemoval( route: { kind: string }, target: { id: string; path: string }, registeredWorktrees: readonly GitWorktreeInfo[], - options: RemoveManagedWorktreeOptions + options: RemoveManagedWorktreeOptions, + localOptions: LocalProjectWorktreeGitOptions ): Promise { const store = this.store if (route.kind !== 'local' || !store) { @@ -100,16 +102,25 @@ export abstract class OrcaRuntimeWithResolveWorktreeRemovalTarget extends OrcaRu } const hostId = parseExecutionHostId(options.hostId)?.id const allowUnverifiedPtyStop = options.allowUnverifiedPtyStop === true - return retryFailedRemovalUnlessRegistered(target.id, target.path, registeredWorktrees, () => - retryFailedWorktreeRemoval(target.id, hostId, (record) => - interruptedLocalWorktreeRemovalJob(record, { - ...this.localRemovalJobHost(store), - stopPtys: () => - this.stopPtysForDestructiveWorktreeRemoval(record.worktreeId, { - allowUnverifiedStop: allowUnverifiedPtyStop - }) - }) - ) + return retryFailedRemovalUnlessRegistered( + target.id, + target.path, + registeredWorktrees, + localOptions, + () => + retryFailedWorktreeRemoval( + target.id, + hostId, + (record) => + interruptedLocalWorktreeRemovalJob(record, { + ...this.localRemovalJobHost(store), + stopPtys: () => + this.stopPtysForDestructiveWorktreeRemoval(record.worktreeId, { + allowUnverifiedStop: allowUnverifiedPtyStop + }) + }), + options.force === true + ) ) } diff --git a/src/main/runtime/workspaces/workspace-removal.ts b/src/main/runtime/workspaces/workspace-removal.ts index 2f8a233bd6a..1bc3e71a297 100644 --- a/src/main/runtime/workspaces/workspace-removal.ts +++ b/src/main/runtime/workspaces/workspace-removal.ts @@ -1,3 +1,5 @@ +import type { LocalProjectWorktreeGitOptions } from '../../project-runtime-git-options' +import { isStaleLocalWorktreeRegistration } from './stale-worktree-registration-preflight' import type { RuntimeStore } from '../runtime-store-contract' import type { GitWorktreeInfo } from '../../../shared/worktree/types' import { OrcaRuntimeWithCreateManagedRemoteWorktree } from './workspace-metadata' @@ -24,13 +26,9 @@ import { resolveWorktreeRemovalRoute } from '../../worktree-removal-execution-host-route' import { listWorktreesOnRemovalRoute } from '../worktree-removal-route-listing' -import { isPrunableGitFileWorktree } from '../../worktree-prunable-git-file' +import { authorizeUnregisteredWindowsRemoval } from '../../git/windows-worktree-removal' import { findRegisteredDeletableWorktree } from '../../worktree-removal-safety' import { removeRuntimeUnregisteredWorktree } from '../runtime-unregistered-worktree-removal' -import { assertWorktreeUnlockedForRemoval } from '../../../shared/worktree/removal' -import { formatWorktreeRemovalError } from '../../ipc/worktree-logic' -import { isWindowsAbsolutePathLike } from '../../../shared/cross-platform-path' -import { isLocalRuntimeWorktreePathMissing } from '../runtime-worktree-filesystem' import { removeStaleLocalWorktreeRegistration } from '../../local-worktree-removal-recovery' import { cleanupUnusedWorktreePushTargetRemote } from '../../ipc/worktree-remote' import { removeRuntimeRegisteredLocalWorktree } from '../runtime-registered-local-worktree-removal' @@ -64,7 +62,8 @@ export abstract class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWi route: { kind: string }, target: { id: string; path: string }, registeredWorktrees: readonly GitWorktreeInfo[], - options: RemoveManagedWorktreeOptions + options: RemoveManagedWorktreeOptions, + localOptions: LocalProjectWorktreeGitOptions ): Promise async removeManagedWorktree( @@ -170,7 +169,22 @@ export abstract class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWi registeredWorktrees, removalHome ) - if (await this.retryFailedRemoval(route, removalTarget, registeredWorktrees, options)) { + await authorizeUnregisteredWindowsRemoval( + !!registeredWorktree, + repo!.path, + removalTarget.path, + force, + localWorktreeGitOptions + ) + if ( + await this.retryFailedRemoval( + route, + removalTarget, + registeredWorktrees, + options, + localWorktreeGitOptions + ) + ) { return { removing: true } } if (!registeredWorktree) { @@ -199,22 +213,13 @@ export abstract class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWi } const canonicalWorktreePath = registeredWorktree.path const deleteBranch = removedMeta?.preserveBranchOnDelete !== true - try { - assertWorktreeUnlockedForRemoval(registeredWorktree) - } catch (error) { - throw new Error(formatWorktreeRemovalError(error, canonicalWorktreePath, force)) - } if ( - (await isPrunableGitFileWorktree(registeredWorktree, localWorktreeGitOptions)) || - (force === true && - process.platform === 'win32' && - (isWindowsAbsolutePathLike(canonicalWorktreePath) || - !!localWorktreeGitOptions.wslDistro) && - removedMeta && - (await isLocalRuntimeWorktreePathMissing( - canonicalWorktreePath, - localWorktreeGitOptions - ))) + await isStaleLocalWorktreeRegistration( + registeredWorktree, + force, + removedMeta, + localWorktreeGitOptions + ) ) { const removalResult = await removeStaleLocalWorktreeRegistration({ canonicalWorktreePath, diff --git a/src/main/worktree-background-removal.ts b/src/main/worktree-background-removal.ts index 45927fed60d..07923d0f77b 100644 --- a/src/main/worktree-background-removal.ts +++ b/src/main/worktree-background-removal.ts @@ -149,7 +149,8 @@ export function startBackgroundWorktreeRemoval( export function retryFailedWorktreeRemoval( worktreeId: string, hostId: ExecutionHostId | undefined, - jobFor: (record: WorktreeRemovalRecord) => BackgroundWorktreeRemovalJob + jobFor: (record: WorktreeRemovalRecord) => BackgroundWorktreeRemovalJob, + force?: boolean ): Promise | undefined { const failed = (hostId ?? LOCAL_EXECUTION_HOST_ID) === LOCAL_EXECUTION_HOST_ID @@ -158,7 +159,8 @@ export function retryFailedWorktreeRemoval( if (!failed) { return waitForPendingWorktreeRemoval(worktreeId, hostId) } - const { failure: _failure, ...record } = failed + const { failure: _failure, ...original } = failed + const record = { ...original, force: force ?? original.force } const settlement = addPendingRemoval(record) const job = jobFor(record) const leftoverOnly: BackgroundWorktreeRemovalJob = { diff --git a/src/main/worktree-failed-removal.test.ts b/src/main/worktree-failed-removal.test.ts index daceba3248f..8c3bccf9242 100644 --- a/src/main/worktree-failed-removal.test.ts +++ b/src/main/worktree-failed-removal.test.ts @@ -25,7 +25,6 @@ import { snapshotPendingWorktreeRemovals, withUnregisteredRemovalCheckouts } from './worktree-removal-listing' -import { retryFailedRemovalUnlessRegistered } from './worktree-removal-leftover' import { readWorktreeRemovalRecords } from './worktree-removal-records' import { loadWorktreeRemovalRecordsForStore } from './startup/worktree-removal-records-load' @@ -167,19 +166,24 @@ describe('a delete that fails after Git dropped the registration', () => { expect(invalidated).toHaveBeenCalledWith('repo-1') }) - it('clears the record as before when Git still registers the checkout', async () => { - vi.mocked(listWorktreesStrict).mockResolvedValue([ - mainWorktree, - { ...leftoverRow(), removalError: undefined } - ]) - const cleanupPushTargetRemote = vi.fn(async () => {}) - await expect(startFailingRemoval({ cleanupPushTargetRemote })).rejects.toThrow(GIT_ERROR) - await _settlePendingWorktreeRemovalsForTests() - - expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([]) - expect(deleteBranchOfUnregisteredWorktree).not.toHaveBeenCalled() - expect(cleanupPushTargetRemote).not.toHaveBeenCalled() - }) + it.each(['branch', 'head'])( + 'retires a stale registered failure by %s without cleanup', + async (kind) => { + const original = { ...leftoverRow(), removalError: undefined } + vi.mocked(listWorktreesStrict).mockResolvedValue([mainWorktree, original]) + const cleanupPushTargetRemote = vi.fn(async () => {}) + await expect(startFailingRemoval({ cleanupPushTargetRemote })).rejects.toThrow(GIT_ERROR) + await _settlePendingWorktreeRemovalsForTests() + expect(await withUnregisteredRemovalCheckouts('repo-1', [original])).toEqual([leftoverRow()]) + const replacement = { ...original, [kind]: kind === 'branch' ? 'refs/heads/other' : 'def' } + expect(await withUnregisteredRemovalCheckouts('repo-1', [replacement])).toEqual([replacement]) + await vi.waitFor(async () => + expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([]) + ) + expect(deleteBranchOfUnregisteredWorktree).not.toHaveBeenCalled() + expect(cleanupPushTargetRemote).not.toHaveBeenCalled() + } + ) it('runs the Git side of the delete when it fails: the recorded branch and the push-target remote', async () => { const cleanupPushTargetRemote = vi.fn(async () => {}) @@ -277,6 +281,7 @@ describe('a delete that fails after Git dropped the registration', () => { it('does not run the recorded removal once Git registers a checkout at the path again', async () => { await failRemoval() + const [record] = await readWorktreeRemovalRecords(join(directory, 'profile')) vi.mocked(listWorktreesStrict).mockResolvedValue([ mainWorktree, { ...leftoverRow(), removalError: undefined } @@ -288,10 +293,13 @@ describe('a delete that fails after Git dropped the registration', () => { publish: () => {} })) - await expect(retried).rejects.toThrow(/A different checkout is now at/) + const message = `A different checkout is now at ${checkout}; Orca left it in place. Delete it again to remove it.` + await expect(retried).rejects.toThrow(message) await _settlePendingWorktreeRemovalsForTests() expect(run).not.toHaveBeenCalled() - expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([]) + expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([ + { ...record, failure: { message, failedAt: expect.any(Number) } } + ]) }) it('is not retried for another host', async () => { @@ -361,47 +369,4 @@ describe('a delete that fails after Git dropped the registration', () => { expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([]) ) }) - - it('Delete refuses while the folder is on disk: nothing runs, the record and row stay', async () => { - await failRemoval() - const retry = vi.fn() - - await expect( - retryFailedRemovalUnlessRegistered(worktreeId, checkout, [mainWorktree], retry) - ).rejects.toThrow( - `${checkout} is no longer a Git worktree, so its folder was left on disk. Delete the folder yourself to finish removing this workspace.` - ) - - expect(retry).not.toHaveBeenCalled() - expect(await readdir(checkout)).toEqual(['node_modules']) - expect(await listRows()).toEqual([mainWorktree, leftoverRow()]) - expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toHaveLength(1) - }) - - it('Delete names a worktree Git registers inside the folder, which removing it would delete', async () => { - await failRemoval() - const nested = { ...mainWorktree, path: join(checkout, 'sub'), isMainWorktree: false } - const retry = vi.fn() - - await expect( - retryFailedRemovalUnlessRegistered(worktreeId, checkout, [mainWorktree, nested], retry) - ).rejects.toThrow( - `Refusing to delete worktree because it contains another registered worktree: ${nested.path}` - ) - - expect(retry).not.toHaveBeenCalled() - expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toHaveLength(1) - }) - - it('Delete finishes the recorded removal once the user removed the folder', async () => { - await failRemoval() - await rm(checkout, { recursive: true }) - const retry = vi.fn(() => Promise.resolve({})) - - await expect( - retryFailedRemovalUnlessRegistered(worktreeId, checkout, [mainWorktree], retry) - ).resolves.toBe(true) - - expect(retry).toHaveBeenCalledTimes(1) - }) }) diff --git a/src/main/worktree-orphan-gitdir-proof.ts b/src/main/worktree-orphan-gitdir-proof.ts index 78471820742..c8a7933fca2 100644 --- a/src/main/worktree-orphan-gitdir-proof.ts +++ b/src/main/worktree-orphan-gitdir-proof.ts @@ -1,31 +1,44 @@ -import { type posix, win32 } from 'node:path' +import type { posix } from 'node:path' +import { isDefinitiveAbsence } from '../shared/definitive-filesystem-absence' +import { resolveGitCommonDirectory } from '../shared/git-common-directory' +import { + areResolvedPathsEqual, + canonicalOwnerPathsEqual, + isDirectWorktreeAdmin +} from './worktree-owner-path-equivalence' type PathOps = typeof posix export type StatPath = (path: string) => Promise export type ReadPath = (path: string) => Promise +// Only owning-layout discovery follows links; candidate and admin-entry proof do not. +export type WorktreeProofAccess = { + statPath: StatPath + layoutStatPath: StatPath + readPath: ReadPath + realPath: (path: string) => Promise +} + export async function gitFileProvesOrphanedWorktreeDirectory(args: { gitFilePath: string worktreePath: string repoPath: string pathOps: PathOps - statPath: StatPath - readPath: ReadPath + access: WorktreeProofAccess }): Promise { try { - const gitEntry = await args.statPath(args.gitFilePath) + const gitEntry = await args.access.statPath(args.gitFilePath) if (!isGitFileStat(gitEntry)) { return false } - const gitFileContents = readFileResultToText(await args.readPath(args.gitFilePath)) + const gitFileContents = readFileResultToText(await args.access.readPath(args.gitFilePath)) return gitFileContents - ? gitFilePointsAtRepoWorktree( + ? await gitFilePointsAtRepoWorktree( gitFileContents, args.worktreePath, args.repoPath, args.pathOps, - args.statPath, - args.readPath + args.access ) : false } catch { @@ -65,65 +78,96 @@ function resolveGitdirPath(gitdirPath: string, basePath: string, pathOps: PathOp : pathOps.resolve(basePath, gitdirPath) } -function areResolvedPathsEqual(leftPath: string, rightPath: string, pathOps: PathOps): boolean { - const left = pathOps.normalize(pathOps.resolve(leftPath)) - const right = pathOps.normalize(pathOps.resolve(rightPath)) - return pathOps === win32 ? left.toLowerCase() === right.toLowerCase() : left === right -} - -async function resolveRepoWorktreesPath( +async function readProvenRepoWorktreesPath( repoPath: string, pathOps: PathOps, - statPath: StatPath, - readPath: ReadPath + access: WorktreeProofAccess ): Promise { - const repoGitPath = pathOps.join(repoPath, '.git') - try { - const repoGitEntry = await statPath(repoGitPath) - if (!isGitFileStat(repoGitEntry)) { - return pathOps.resolve(repoPath, '.git', 'worktrees') + let owningGitDir: string | null = null + const common = await resolveGitCommonDirectory( + repoPath, + {}, + { + statPath: async (file) => { + const entry = await access.layoutStatPath(file) + return { + isFile: () => isGitFileStat(entry), + isDirectory: () => { + if (!entry || typeof entry !== 'object') { + return false + } + return ( + ('isDirectory' in entry && + typeof entry.isDirectory === 'function' && + entry.isDirectory() === true) || + ('type' in entry && entry.type === 'directory') + ) + } + } + }, + readPath: async (file) => { + try { + const text = readFileResultToText(await access.readPath(file)) + if (text === null) { + throw new Error('Cannot read Git metadata') + } + return text + } catch (error) { + if (isDefinitiveAbsence(error)) { + return null + } + throw error + } + }, + join: pathOps.join, + resolvePath: (base, pointer) => resolveGitdirPath(pointer, base, pathOps), + parseMarker: (contents) => { + const marker = parseGitdirPath(contents) + owningGitDir = marker ? resolveGitdirPath(marker, repoPath, pathOps) : null + return marker + }, + isMissing: isDefinitiveAbsence } - const repoGitContents = readFileResultToText(await readPath(repoGitPath)) - const repoGitdirPath = repoGitContents ? parseGitdirPath(repoGitContents) : null - if (!repoGitdirPath) { - return null - } - const resolvedRepoGitdirPath = resolveGitdirPath(repoGitdirPath, repoPath, pathOps) - const parentPath = pathOps.dirname(resolvedRepoGitdirPath) - if ( - pathOps.basename(parentPath) === 'worktrees' && - (await repoGitdirIsLinkedWorktreeAdminEntry( - resolvedRepoGitdirPath, - repoPath, - pathOps, - readPath - )) - ) { - return parentPath - } - return pathOps.join(resolvedRepoGitdirPath, 'worktrees') - } catch { + ) + if (!common) { return null } + const worktrees = pathOps.join(common, 'worktrees') + if ( + owningGitDir && + (await isDirectWorktreeAdmin(owningGitDir, worktrees, pathOps, access)) && + !(await repoGitdirIsLinkedWorktreeAdminEntry(owningGitDir, repoPath, pathOps, access)) + ) { + return null + } + return worktrees } async function repoGitdirIsLinkedWorktreeAdminEntry( resolvedRepoGitdirPath: string, repoPath: string, pathOps: PathOps, - readPath: ReadPath + access: WorktreeProofAccess ): Promise { const adminGitdirPath = pathOps.join(resolvedRepoGitdirPath, 'gitdir') try { - const adminGitdirContents = readFileResultToText(await readPath(adminGitdirPath)) + const adminGitdirContents = readFileResultToText(await access.readPath(adminGitdirPath)) const repoGitPath = adminGitdirContents ? parseFirstLinePath(adminGitdirContents) : null if (!repoGitPath) { return false } const resolvedRepoGitPath = resolveGitdirPath(repoGitPath, resolvedRepoGitdirPath, pathOps) const resolvedExpectedGitPath = pathOps.resolve(repoPath, '.git') - // Only the admin backlink distinguishes a linked checkout from a separate git dir named worktrees. - return areResolvedPathsEqual(resolvedRepoGitPath, resolvedExpectedGitPath, pathOps) + // A linked parent must prove its own backlink before supplying this namespace. + return ( + areResolvedPathsEqual(resolvedRepoGitPath, resolvedExpectedGitPath, pathOps) || + (await canonicalOwnerPathsEqual( + resolvedRepoGitPath, + resolvedExpectedGitPath, + pathOps, + access + )) + ) } catch { return false } @@ -134,23 +178,27 @@ async function gitFilePointsAtRepoWorktree( worktreePath: string, repoPath: string, pathOps: PathOps, - statPath: StatPath, - readPath: ReadPath + access: WorktreeProofAccess ): Promise { const gitdirPath = parseGitdirPath(contents) if (!gitdirPath) { return false } const resolvedGitdirPath = resolveGitdirPath(gitdirPath, worktreePath, pathOps) - const repoWorktreesPath = await resolveRepoWorktreesPath(repoPath, pathOps, statPath, readPath) - if (!repoWorktreesPath || !containsPath(repoWorktreesPath, resolvedGitdirPath, pathOps)) { + const repoWorktreesPath = await readProvenRepoWorktreesPath(repoPath, pathOps, access) + if ( + !repoWorktreesPath || + !(await isDirectWorktreeAdmin(resolvedGitdirPath, repoWorktreesPath, pathOps, access)) + ) { return false } - const relativeGitdirPath = pathOps.relative(repoWorktreesPath, resolvedGitdirPath) - if (relativeGitdirPath === '' || relativeGitdirPath.includes(pathOps.sep)) { - return false - } - return adminGitdirPointsAtCandidate(resolvedGitdirPath, worktreePath, pathOps, statPath, readPath) + return adminGitdirPointsAtCandidate( + resolvedGitdirPath, + worktreePath, + pathOps, + access.statPath, + access.readPath + ) } async function adminGitdirPointsAtCandidate( @@ -176,30 +224,18 @@ async function adminGitdirPointsAtCandidate( // A copied .git file can name another checkout; verify the admin entry's backlink. return areResolvedPathsEqual(resolvedCandidateGitPath, resolvedExpectedGitPath, pathOps) } catch (error) { - if (!isMissingPathError(error)) { + if (!isDefinitiveAbsence(error)) { return false } try { await statPath(resolvedGitdirPath) return false } catch (statError) { - return isMissingPathError(statError) + return isDefinitiveAbsence(statError) } } } -function containsPath(parentPath: string, childPath: string, pathOps: PathOps): boolean { - const relativePath = pathOps.relative(parentPath, childPath) - // ..name is a child name; only .. and ../... escape. - return ( - relativePath === '' || - (!!relativePath && - relativePath !== '..' && - !relativePath.startsWith(`..${pathOps.sep}`) && - !pathOps.isAbsolute(relativePath)) - ) -} - function parseGitdirPath(contents: string): string | null { const firstLine = contents.split(/\r?\n/, 1)[0]?.trim() if (!firstLine) { @@ -214,24 +250,3 @@ function parseFirstLinePath(contents: string): string | null { const firstLine = contents.split(/\r?\n/, 1)[0]?.trim() return firstLine || null } - -function isMissingPathError(error: unknown): boolean { - const code = - error && typeof error === 'object' && 'code' in error - ? String((error as NodeJS.ErrnoException).code) - : undefined - if (code === 'ENOENT' || code === 'ENOTDIR') { - return true - } - const message = - error instanceof Error - ? error.message - : error && typeof error === 'object' && 'message' in error - ? String((error as { message: unknown }).message) - : typeof error === 'string' - ? error - : '' - return /\b(ENOENT|ENOTDIR)\b|no such file or directory|cannot find (?:the )?(?:file|path)|(?:file|path) not found/i.test( - message - ) -} diff --git a/src/main/worktree-owner-path-equivalence.ts b/src/main/worktree-owner-path-equivalence.ts new file mode 100644 index 00000000000..3ee89eee695 --- /dev/null +++ b/src/main/worktree-owner-path-equivalence.ts @@ -0,0 +1,83 @@ +import { type posix, win32 } from 'node:path' +import { isDefinitiveAbsence } from '../shared/definitive-filesystem-absence' +import type { WorktreeProofAccess } from './worktree-orphan-gitdir-proof' + +type PathOps = typeof posix + +export function areResolvedPathsEqual( + leftPath: string, + rightPath: string, + pathOps: PathOps +): boolean { + const left = pathOps.normalize(pathOps.resolve(leftPath)) + const right = pathOps.normalize(pathOps.resolve(rightPath)) + return pathOps === win32 ? left.toLowerCase() === right.toLowerCase() : left === right +} + +async function readOwnerCanonicalPath( + path: string, + pathOps: PathOps, + access: WorktreeProofAccess +): Promise { + try { + const resolved = await access.realPath(path) + if (typeof resolved !== 'string' || !pathOps.isAbsolute(resolved) || resolved.includes('\0')) { + throw new Error('Invalid owning-host canonical path') + } + return resolved + } catch (error) { + if (isDefinitiveAbsence(error)) { + return null + } + throw error + } +} + +export async function canonicalOwnerPathsEqual( + left: string, + right: string, + pathOps: PathOps, + access: WorktreeProofAccess +): Promise { + const [a, b] = await Promise.all([ + readOwnerCanonicalPath(left, pathOps, access), + readOwnerCanonicalPath(right, pathOps, access) + ]) + return a !== null && b !== null && areResolvedPathsEqual(a, b, pathOps) +} + +export async function isDirectWorktreeAdmin( + admin: string, + worktrees: string, + pathOps: PathOps, + access: WorktreeProofAccess +): Promise { + const parent = pathOps.dirname(admin) + if ( + areResolvedPathsEqual(admin, parent, pathOps) || + areResolvedPathsEqual(admin, worktrees, pathOps) + ) { + return false + } + if (areResolvedPathsEqual(parent, worktrees, pathOps)) { + return true + } + const [actual, expected] = await Promise.all([ + readOwnerCanonicalPath(parent, pathOps, access), + readOwnerCanonicalPath(worktrees, pathOps, access) + ]) + if (actual !== null || expected !== null) { + return actual !== null && expected !== null && areResolvedPathsEqual(actual, expected, pathOps) + } + const name = pathOps.basename(parent) + if ((pathOps === win32 ? name.toLowerCase() : name) !== 'worktrees') { + return false + } + // Missing worktrees directories prove namespace only; the admin must still prove absent. + return canonicalOwnerPathsEqual( + pathOps.dirname(parent), + pathOps.dirname(worktrees), + pathOps, + access + ) +} diff --git a/src/main/worktree-removal-failure.ts b/src/main/worktree-removal-failure.ts index 39a36cd2c3d..ce1eeff36af 100644 --- a/src/main/worktree-removal-failure.ts +++ b/src/main/worktree-removal-failure.ts @@ -1,25 +1,42 @@ import { deleteBranchOfUnregisteredWorktree } from './git/worktree-removal' -import { isCheckoutRegistered, isUnregisteredRemovalLeftover } from './worktree-removal-leftover' +import { + isRecordedCheckout, + registeredRemovalCheckout, + isUnregisteredRemovalLeftover +} from './worktree-removal-leftover' import type { WorktreeRemovalFailure, WorktreeRemovalRecord } from './worktree-removal-records' import { worktreeCheckoutExists } from './worktree-removal-table' -/** Keep the error if Git dropped registration but left the checkout on disk. */ +/** Retain failures for the same checkout without cleaning up its live Git ownership. */ export async function failureLeftByGit( record: WorktreeRemovalRecord, error: unknown, cleanupPushTargetRemote: (() => Promise) | undefined ): Promise { - if (!(await isCheckoutLeftUnregistered(record))) { + if (!(await worktreeCheckoutExists(record.worktreePath))) { return undefined } + const message = error instanceof Error ? error.message : String(error) const failure = { - message: (error instanceof Error ? error.message : String(error)).replace( + message: message.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() } + try { + const registered = await registeredRemovalCheckout(record) + if (registered) { + return isRecordedCheckout(registered, record) ? { ...failure, message } : undefined + } + if (!(await isUnregisteredRemovalLeftover(record.repoPath, record.worktreePath))) { + return undefined + } + } catch (error) { + console.warn(`[worktrees] could not inspect failed removal of ${record.worktreePath}`, error) + return failure + } await finishGitSideOfFailedRemoval(record, cleanupPushTargetRemote) return failure } @@ -45,18 +62,3 @@ async function finishGitSideOfFailedRemoval( console.warn(`[worktrees] could not clean up the push target of ${record.worktreePath}`, error) } } - -async function isCheckoutLeftUnregistered(record: WorktreeRemovalRecord): Promise { - if (!(await worktreeCheckoutExists(record.worktreePath))) { - return false - } - try { - return ( - !(await isCheckoutRegistered(record)) && - (await isUnregisteredRemovalLeftover(record.repoPath, record.worktreePath)) - ) - } catch (error) { - console.warn(`[worktrees] could not list worktrees of ${record.repoPath}`, error) - return false - } -} diff --git a/src/main/worktree-removal-leftover.ts b/src/main/worktree-removal-leftover.ts index 0e852e38dea..caad62a314a 100644 --- a/src/main/worktree-removal-leftover.ts +++ b/src/main/worktree-removal-leftover.ts @@ -1,16 +1,16 @@ +import { unregisteredFolderRefusal } from '../shared/worktree/removal' +import { usesNativeWindowsRemoval } from './git/windows-worktree-removal' +import type { LocalProjectWorktreeGitOptions } from './project-runtime-git-options' import { lstat } from 'node:fs/promises' import { join } from 'node:path' import type { RemoveWorktreeResult } from '../shared/worktree/create-types' import type { GitWorktreeInfo } from '../shared/worktree/types' import { listWorktreesStrict } from './git/worktree' -import { getErrorCode } from './git/worktree-operation-options' +import { getErrorCode, normalizeLocalBranchRef } from './git/worktree-operation-options' +import type { WorktreeRemovalRecord } from './worktree-removal-records' import { areWorktreePathsEqual } from './git/worktree-path-comparison' import { CLIENT_REMOVAL_HOME } from './worktree-removal-home-guard' -import { - failedWorktreeRemovals, - persistWorktreeRemovalRecords, - worktreeCheckoutExists -} from './worktree-removal-table' +import { failedWorktreeRemovals, worktreeCheckoutExists } from './worktree-removal-table' import { assertWorktreeDoesNotContainRegisteredWorktree, canSafelyRemoveOrphanedWorktreeDirectory @@ -35,21 +35,34 @@ export function differentCheckoutAtPathError(worktreePath: string): Error { ) } -export async function isCheckoutRegistered(record: { +export async function registeredRemovalCheckout(record: { repoPath: string worktreePath: string -}): Promise { - return (await listWorktreesStrict(record.repoPath)).some((worktree) => +}): Promise { + return (await listWorktreesStrict(record.repoPath)).find((worktree) => areWorktreePathsEqual(worktree.path, record.worktreePath) ) } -export function unregisteredFolderRefusal(worktreePath: string): Error { - return new Error( - `${worktreePath} is no longer a Git worktree, so its folder was left on disk. Delete the folder yourself to finish removing this workspace.` +export async function isCheckoutRegistered(record: { + repoPath: string + worktreePath: string +}): Promise { + return !!(await registeredRemovalCheckout(record)) +} + +export function isRecordedCheckout( + worktree: GitWorktreeInfo, + record: WorktreeRemovalRecord +): boolean { + return ( + normalizeLocalBranchRef(worktree.branch) === record.branch && + (!record.head || worktree.head === record.head) ) } +export { unregisteredFolderRefusal } from '../shared/worktree/removal' + /** Name nested worktrees before asking the user to remove an unregistered folder. */ export async function assertUnregisteredCheckoutGone( worktreePath: string, @@ -66,16 +79,14 @@ export async function retryFailedRemovalUnlessRegistered( worktreeId: string, worktreePath: string, registeredWorktrees: readonly Pick[], + localOptions: LocalProjectWorktreeGitOptions, retry: () => Promise | undefined ): Promise { if (registeredWorktrees.some((worktree) => areWorktreePathsEqual(worktree.path, worktreePath))) { - if (failedWorktreeRemovals.delete(worktreeId)) { - void persistWorktreeRemovalRecords() - } return false } const failed = failedWorktreeRemovals.get(worktreeId) - if (failed) { + if (failed && !usesNativeWindowsRemoval(worktreePath, localOptions)) { await assertUnregisteredCheckoutGone(failed.worktreePath, registeredWorktrees) } return retry() !== undefined diff --git a/src/main/worktree-removal-listing.ts b/src/main/worktree-removal-listing.ts index 7d17bd1d6d0..7a94f4607d9 100644 --- a/src/main/worktree-removal-listing.ts +++ b/src/main/worktree-removal-listing.ts @@ -1,7 +1,7 @@ import { LOCAL_EXECUTION_HOST_ID, type ExecutionHostId } from '../shared/execution-host' import type { GitWorktreeInfo } from '../shared/worktree/types' -import { areWorktreePathsEqual } from './git/worktree-path-comparison' -import { isUnregisteredRemovalLeftover } from './worktree-removal-leftover' +import { areWorktreePathsEqual, canonicalWorktreePath } from './git/worktree-path-comparison' +import { isRecordedCheckout, isUnregisteredRemovalLeftover } from './worktree-removal-leftover' import type { WorktreeRemovalRecord } from './worktree-removal-records' import { failedWorktreeRemovals, @@ -21,16 +21,30 @@ export async function withUnregisteredRemovalCheckouts( repoId: string, gitWorktrees: GitWorktreeInfo[] ): Promise { + let droppedFailure = false + const failures = new Map( + [...failedWorktreeRemovals.values()] + .filter((record) => record.repoId === repoId) + .map((record) => [canonicalWorktreePath(record.worktreePath), record] as const) + ) + gitWorktrees = gitWorktrees.map((row) => { + const record = failures.get(canonicalWorktreePath(row.path)) + if (!record) { + return row + } + if (isRecordedCheckout(row, record)) { + return { ...row, removalError: record.failure?.message } + } + failedWorktreeRemovals.delete(record.worktreeId) + droppedFailure = true + return row + }) const unlisted = [...pendingWorktreeRemovals.values(), ...failedWorktreeRemovals.values()].filter( (record) => record.repoId === repoId && !gitWorktrees.some((worktree) => areWorktreePathsEqual(worktree.path, record.worktreePath)) ) - if (unlisted.length === 0) { - return gitWorktrees - } const leftovers: GitWorktreeInfo[] = [] - let droppedFailure = false for (const record of unlisted) { const failed = failedWorktreeRemovals.get(record.worktreeId) === record if ( diff --git a/src/main/worktree-removal-safety.test.ts b/src/main/worktree-removal-safety.test.ts index ce38f862e74..233dd3a38a0 100644 --- a/src/main/worktree-removal-safety.test.ts +++ b/src/main/worktree-removal-safety.test.ts @@ -1,8 +1,10 @@ import { describe, expect, it, vi } from 'vitest' +import type { ReadPath, StatPath } from './worktree-orphan-gitdir-proof' +import type { WorktreeRemovalHomeAuthority } from './worktree-removal-home-guard' import type { GitWorktreeInfo } from '../shared/worktree/types' import { - canSafelyRemoveOrphanedWorktreeDirectory, + canSafelyRemoveOrphanedWorktreeDirectory as proveWithAccess, findRegisteredDeletableWorktree, getRegisteredDeletableWorktree, isDangerousWorktreeRemovalPath @@ -23,6 +25,11 @@ function missingPath(path: string): Error & { code: string } { return Object.assign(new Error(`missing ${path}`), { code: 'ENOENT' }) } +const deniedPath = (operation: string, path: string) => + Object.assign(new Error(`EACCES: permission denied, ${operation} '${path}'`), { + code: 'EACCES' + }) + function makeStatPath(filePaths: readonly string[], directoryPaths: readonly string[] = []) { const files = new Set(filePaths) const directories = new Set(directoryPaths) @@ -37,6 +44,25 @@ function makeStatPath(filePaths: readonly string[], directoryPaths: readonly str } } +function canSafelyRemoveOrphanedWorktreeDirectory( + candidate: string, + owner: string, + home: WorktreeRemovalHomeAuthority, + statPath: StatPath, + readPath: ReadPath +): Promise { + // Ordinary fixture entries have identical following and nonfollowing kinds. + return proveWithAccess(candidate, owner, home, { + statPath, + layoutStatPath: statPath, + readPath, + realPath: async (path) => { + await statPath(path) + return path + } + }) +} + function makeReadPath(entries: readonly (readonly [string, unknown])[]) { const files = new Map(entries) return async (path: string) => { @@ -114,55 +140,70 @@ describe('getRegisteredDeletableWorktree', () => { }) describe('canSafelyRemoveOrphanedWorktreeDirectory', () => { - it('accepts a linked worktree .git file that points at this repo worktrees entry', async () => { + it('accepts dotdot-prefixed lexical admin membership without canonical operations', async () => { + const realPath = vi.fn(async () => { + throw new Error('unexpected canonical operation') + }) await expect( - canSafelyRemoveOrphanedWorktreeDirectory( - '/workspaces/orphan', - '/repo', - CLIENT_REMOVAL_HOME, - makeStatPath(['/workspaces/orphan/.git'], ['/repo/.git']), - makeReadPath([ - ['/workspaces/orphan/.git', 'gitdir: /repo/.git/worktrees/orphan\n'], - ['/repo/.git/worktrees/orphan/gitdir', '/workspaces/orphan/.git\n'] - ]) - ) - ).resolves.toBe(true) - }) - - it('accepts repo worktree admin entries with dotdot-prefixed directory names', async () => { - await expect( - canSafelyRemoveOrphanedWorktreeDirectory( - '/workspaces/orphan', - '/repo', - CLIENT_REMOVAL_HOME, - makeStatPath(['/workspaces/orphan/.git'], ['/repo/.git']), - makeReadPath([ + proveWithAccess('/workspaces/orphan', '/repo', CLIENT_REMOVAL_HOME, { + statPath: makeStatPath(['/workspaces/orphan/.git'], ['/repo/.git']), + layoutStatPath: makeStatPath([], ['/repo/.git']), + readPath: makeReadPath([ ['/workspaces/orphan/.git', 'gitdir: /repo/.git/worktrees/..orphan\n'], ['/repo/.git/worktrees/..orphan/gitdir', '/workspaces/orphan/.git\n'] - ]) - ) + ]), + realPath + }) ).resolves.toBe(true) + expect(realPath).not.toHaveBeenCalled() }) - it('accepts remote filesystem provider readFile results for linked worktree .git files', async () => { - await expect( - canSafelyRemoveOrphanedWorktreeDirectory( - '/workspaces/orphan', - '/repo', - CLIENT_REMOVAL_HOME, - makeStatPath(['/workspaces/orphan/.git'], ['/repo/.git']), - makeReadPath([ - [ - '/workspaces/orphan/.git', - { isBinary: false, content: 'gitdir: /repo/.git/worktrees/orphan\n' } - ], - [ - '/repo/.git/worktrees/orphan/gitdir', - { isBinary: false, content: '/workspaces/orphan/.git\n' } - ] - ]) - ) - ).resolves.toBe(true) + it('requires definitive absence throughout owner fallback and missing-admin proof', async () => { + const owner = '/repos/ENOENT' + const candidate = '/workspaces/orphan' + const admin = `${owner}/worktrees/orphan` + const stat = makeStatPath([`${candidate}/.git`]) + const statPath = vi.fn(stat) + const layoutStatPath = vi.fn(makeStatPath([])) + const read = makeReadPath([ + [`${candidate}/.git`, { content: `gitdir: ${admin}\n`, isBinary: false }], + [`${owner}/HEAD`, 'ref: refs/heads/main\n'] + ]) + const readPath = vi.fn(read) + const realPath = vi.fn(async () => { + throw new Error('unexpected canonical operation') + }) + const access = { statPath, layoutStatPath, readPath, realPath } + const prove = () => proveWithAccess(candidate, owner, CLIENT_REMOVAL_HOME, access) + await expect(prove()).resolves.toBe(true) + const denied = deniedPath('stat', `${owner}/.git`) + for (const error of [denied, new Error(`cannot inspect '${owner}/.git'`)]) { + layoutStatPath.mockRejectedValueOnce(error) + await expect(prove()).resolves.toBe(false) + } + // Each unknown replaces one independently missing observation in the successful proof. + for (const unknownPath of [`${owner}/commondir`, `${admin}/gitdir`]) { + readPath.mockImplementation(async (path) => { + if (path === unknownPath) { + throw deniedPath('open', path) + } + return read(path) + }) + await expect(prove()).resolves.toBe(false) + } + readPath.mockImplementation(read) + statPath.mockImplementation(async (path) => { + if (path === admin) { + throw deniedPath('stat', path) + } + return stat(path) + }) + await expect(prove()).resolves.toBe(false) + statPath.mockImplementation(stat) + readPath.mockImplementation(async (path) => + path === `${admin}/gitdir` ? { content: `${candidate}/.git\n`, isBinary: false } : read(path) + ) + await expect(prove()).resolves.toBe(true) }) it('preserves forward-slash UNC roots when probing linked worktree .git files', async () => { @@ -245,16 +286,54 @@ describe('canSafelyRemoveOrphanedWorktreeDirectory', () => { }) }) - it('accepts a pruned admin entry when the candidate .git points under this repo worktrees dir', async () => { - await expect( - canSafelyRemoveOrphanedWorktreeDirectory( - '/workspaces/orphan', - '/repo', - CLIENT_REMOVAL_HOME, - makeStatPath(['/workspaces/orphan/.git'], ['/repo/.git']), - makeReadPath([['/workspaces/orphan/.git', 'gitdir: /repo/.git/worktrees/orphan\n']]) - ) - ).resolves.toBe(true) + it('proves a pruned admin through an owner link without following candidate entries', async () => { + const candidate = '/provider-only/workspaces/orphan' + const owner = '/provider-only/repo' + const physical = '/provider-only/metadata' + const ordinaryStat = makeStatPath([`${candidate}/.git`]) + const statPath = vi.fn(async (path: string) => { + if (path === `${owner}/.git`) { + return { type: 'symlink' } + } + return ordinaryStat(path) + }) + const layoutStatPath = vi.fn(async () => ({ type: 'directory' })) + const realPath = vi.fn(async (path: string) => { + if (path === `${owner}/.git` || path === physical) { + return physical + } + throw missingPath(path) + }) + const access = { + statPath, + layoutStatPath, + realPath, + readPath: makeReadPath([ + [ + `${candidate}/.git`, + { content: ` gitdir: ${physical}/worktrees/orphan\nignored`, isBinary: false } + ] + ]) + } + const prove = () => proveWithAccess(candidate, owner, CLIENT_REMOVAL_HOME, access) + await expect(prove()).resolves.toBe(true) + expect(layoutStatPath).toHaveBeenCalledExactlyOnceWith(`${owner}/.git`) + expect(statPath.mock.calls.map(([path]) => path)).toEqual([ + `${candidate}/.git`, + `${physical}/worktrees/orphan` + ]) + expect(realPath.mock.calls.map(([path]) => path)).toEqual([ + `${physical}/worktrees`, + `${owner}/.git/worktrees`, + physical, + `${owner}/.git` + ]) + realPath.mockResolvedValueOnce('') + await expect(prove()).resolves.toBe(false) + layoutStatPath.mockRejectedValueOnce(Object.assign(new Error('denied'), { code: 'EACCES' })) + await expect(prove()).resolves.toBe(false) + layoutStatPath.mockResolvedValueOnce({ type: 'other' }) + await expect(prove()).resolves.toBe(false) }) it('rejects existing admin entries with a missing gitdir backlink', async () => { @@ -273,13 +352,14 @@ describe('canSafelyRemoveOrphanedWorktreeDirectory', () => { const readPath = vi.fn() await expect( - canSafelyRemoveOrphanedWorktreeDirectory( - '/workspaces/orphan', - '/repo', - CLIENT_REMOVAL_HOME, - async () => ({ type: 'symlink' }), + proveWithAccess('/workspaces/orphan', '/repo', CLIENT_REMOVAL_HOME, { + statPath: async () => ({ type: 'symlink' }), + layoutStatPath: async () => ({ type: 'directory' }), + realPath: async () => { + throw new Error('candidate link must refuse first') + }, readPath - ) + }) ).resolves.toBe(false) expect(readPath).not.toHaveBeenCalled() @@ -315,21 +395,49 @@ describe('canSafelyRemoveOrphanedWorktreeDirectory', () => { ).resolves.toBe(false) }) - it('accepts a repo path that is itself a linked worktree', async () => { - await expect( - canSafelyRemoveOrphanedWorktreeDirectory( - '/workspaces/orphan', - '/repos/main-linked', - CLIENT_REMOVAL_HOME, - makeStatPath(['/workspaces/orphan/.git', '/repos/main-linked/.git']), - makeReadPath([ - ['/workspaces/orphan/.git', 'gitdir: /common/.git/worktrees/orphan\n'], - ['/repos/main-linked/.git', 'gitdir: /common/.git/worktrees/main-linked\n'], - ['/common/.git/worktrees/main-linked/gitdir', '/repos/main-linked/.git\n'], - ['/common/.git/worktrees/orphan/gitdir', '/workspaces/orphan/.git\n'] - ]) - ) - ).resolves.toBe(true) + it('checks both owner memberships and the linked-owner alias backlink', async () => { + const candidate = '/provider-only/orphan' + const owner = '/provider-only/linked-alias' + const common = '/provider-only/common-alias' + const physical = '/provider-only/ENOENT' + const ownerMarker = '/provider-only/linked/.git' + const realPath = vi.fn(async (path: string) => { + if (path === `${common}/worktrees` || path === `${physical}/worktrees`) { + return `${physical}/worktrees` + } + if (path === `${owner}/.git` || path === ownerMarker) { + return ownerMarker + } + throw missingPath(path) + }) + const access = { + statPath: makeStatPath([`${candidate}/.git`, `${owner}/.git`]), + layoutStatPath: makeStatPath([`${owner}/.git`]), + realPath, + readPath: makeReadPath([ + [`${candidate}/.git`, `gitdir: ${physical}/worktrees/orphan\n`], + [`${owner}/.git`, `gitdir: ${physical}/worktrees/owner\n`], + [`${physical}/worktrees/owner/commondir`, common], + [`${physical}/worktrees/owner/gitdir`, ownerMarker], + [`${physical}/worktrees/orphan/gitdir`, `${candidate}/.git`] + ]) + } + const prove = () => proveWithAccess(candidate, owner, CLIENT_REMOVAL_HOME, access) + await expect(prove()).resolves.toBe(true) + expect(realPath).toHaveBeenCalledTimes(6) + access.readPath = makeReadPath([ + [`${candidate}/.git`, `gitdir: ${common}/worktrees/orphan\n`], + [`${owner}/.git`, `gitdir: ${physical}/worktrees/owner\n`], + [`${physical}/worktrees/owner/commondir`, common], + [`${common}/worktrees/orphan/gitdir`, `${candidate}/.git`] + ]) + for (const error of [ + deniedPath('realpath', `${physical}/worktrees`), + new Error(`owner unavailable, realpath '${physical}/worktrees'`) + ]) { + realPath.mockRejectedValueOnce(error) + await expect(prove()).resolves.toBe(false) + } }) it('rejects POSIX home directories even when host homedir has a different path shape', async () => { diff --git a/src/main/worktree-removal-safety.ts b/src/main/worktree-removal-safety.ts index 33e43cac40b..9729783660e 100644 --- a/src/main/worktree-removal-safety.ts +++ b/src/main/worktree-removal-safety.ts @@ -1,4 +1,4 @@ -import { lstat, readFile } from 'node:fs/promises' +import { lstat, stat, readFile, realpath } from 'node:fs/promises' import type { WorktreeMeta } from '../shared/worktree/meta-types' import type { GitWorktreeInfo } from '../shared/worktree/types' import { NESTED_WORKTREE_REMOVAL_PREFIX } from '../shared/worktree/nested-removal' @@ -12,8 +12,7 @@ import { } from './worktree-removal-home-guard' import { gitFileProvesOrphanedWorktreeDirectory, - type ReadPath, - type StatPath + type WorktreeProofAccess } from './worktree-orphan-gitdir-proof' const ORCA_CREATION_SOURCES = new Set>([ @@ -130,8 +129,12 @@ export async function canSafelyRemoveOrphanedWorktreeDirectory( worktreePath: string, repoPath: string, home: WorktreeRemovalHomeAuthority, - statPath: StatPath = lstat, - readPath: ReadPath = (path) => readFile(path, 'utf8') + access: WorktreeProofAccess = { + statPath: lstat, + layoutStatPath: stat, + readPath: (path) => readFile(path, 'utf8'), + realPath: realpath + } ): Promise { // A .git file can also belong to a dotfiles home; unknown host home must never authorize deletion. if (!isRemovalHomeAuthorityResolved(home)) { @@ -149,8 +152,7 @@ export async function canSafelyRemoveOrphanedWorktreeDirectory( worktreePath, repoPath, pathOps, - statPath, - readPath + access }) } diff --git a/src/shared/git-common-directory.ts b/src/shared/git-common-directory.ts index c36b7244d3d..6750368ba18 100644 --- a/src/shared/git-common-directory.ts +++ b/src/shared/git-common-directory.ts @@ -6,6 +6,16 @@ import { parseGitdirMarkerPayload } from './gitdir-marker-payload' export type GitAdminReadOptions = GitMetadataPathOptions & { signal?: AbortSignal } +type GitLayoutStat = { isDirectory(): boolean; isFile(): boolean } +export type GitCommonDirectoryReader = { + statPath(path: string): Promise + readPath(path: string): Promise + join(...paths: string[]): string + resolvePath(base: string, pointer: string): string | null + parseMarker(content: string): string | null + isMissing(error: unknown): boolean +} + export function isMissingGitAdminEntry(error: unknown): boolean { return ( error instanceof Error && @@ -32,33 +42,40 @@ export async function readGitAdminFile( /** Read the owning host's Git layout without starting a subprocess. */ export async function resolveGitCommonDirectory( repoPath: string, - options: GitAdminReadOptions = {} + options: GitAdminReadOptions = {}, + reader: GitCommonDirectoryReader = { + statPath: (file) => waitForPromiseWithSignal(stat(file), options.signal), + readPath: (file) => readGitAdminFile(file, options.signal), + join: path.join, + resolvePath: (base, pointer) => resolveGitMetadataPath(base, pointer, options), + parseMarker: parseGitdirMarkerPayload, + isMissing: isMissingGitAdminEntry + } ): Promise { - const dotGit = path.join(repoPath, '.git') + const dotGit = reader.join(repoPath, '.git') + let metadata: GitLayoutStat | undefined let gitDir: string | null = null try { - const metadata = await waitForPromiseWithSignal(stat(dotGit), options.signal) - if (metadata.isDirectory()) { - gitDir = dotGit - } else if (metadata.isFile()) { - const marker = parseGitdirMarkerPayload( - (await readGitAdminFile(dotGit, options.signal)) ?? '' - ) - if (marker) { - gitDir = resolveGitMetadataPath(repoPath, marker, options) - } - } + metadata = await reader.statPath(dotGit) } catch (error) { - if (!isMissingGitAdminEntry(error)) { + if (!reader.isMissing(error)) { throw error } - if ((await readGitAdminFile(path.join(repoPath, 'HEAD'), options.signal)) !== null) { + if ((await reader.readPath(reader.join(repoPath, 'HEAD'))) !== null) { gitDir = repoPath } } + if (metadata?.isDirectory()) { + gitDir = dotGit + } else if (metadata?.isFile()) { + const marker = reader.parseMarker((await reader.readPath(dotGit)) ?? '') + if (marker) { + gitDir = reader.resolvePath(repoPath, marker) + } + } if (!gitDir) { return null } - const commonDir = (await readGitAdminFile(path.join(gitDir, 'commondir'), options.signal))?.trim() - return commonDir ? resolveGitMetadataPath(gitDir, commonDir, options) : gitDir + const commonDir = (await reader.readPath(reader.join(gitDir, 'commondir')))?.trim() + return commonDir ? reader.resolvePath(gitDir, commonDir) : gitDir } diff --git a/src/shared/worktree/removal.ts b/src/shared/worktree/removal.ts index f78f48b4b8e..3088e532ddb 100644 --- a/src/shared/worktree/removal.ts +++ b/src/shared/worktree/removal.ts @@ -1,6 +1,12 @@ import type { ExecutionHostId } from '../execution-host' import type { GitWorktreeInfo, Worktree } from './types' +export function unregisteredFolderRefusal(worktreePath: string): Error { + return new Error( + `${worktreePath} is no longer a Git worktree, so its folder was left on disk. Delete the folder yourself to finish removing this workspace.` + ) +} + export const LOCKED_WORKTREE_REMOVAL_PREFIX = 'Worktree is locked by Git.' export const UNSTOPPED_PTY_REMOVAL_PREFIX = 'Failed to physically stop every PTY for worktree:'