diff --git a/src/main/git/worktree-git-file-restore.ts b/src/main/git/worktree-git-file-restore.ts deleted file mode 100644 index 616aacb2549b..000000000000 --- a/src/main/git/worktree-git-file-restore.ts +++ /dev/null @@ -1,58 +0,0 @@ -import { readdir, readFile, writeFile } from 'node:fs/promises' -import { isAbsolute, join } from 'node:path' -import type { GitWorktreeExecOptions } from './worktree-operation-options' -import { getErrorCode } from './worktree-operation-options' -import { readRepoCommonDirFromGit } from './worktree-list-reader' -import { areWorktreePathsEqual } from './worktree-path-comparison' - -/** - * Rewrites a registered linked worktree's missing `.git` file from Git's own admin entry, the one - * whose `gitdir` points back at it, so `git worktree remove` can validate and delete the checkout. - * - * Why not `git worktree repair` (2.29+): it also re-points every other registered path's `.git`, - * including a checkout another repository now owns at a reused path. - */ -export async function restoreMissingWorktreeGitFile( - repoPath: string, - worktreePath: string, - options: GitWorktreeExecOptions = {} -): Promise { - const commonDir = await readRepoCommonDirFromGit(repoPath, options) - if (!commonDir) { - return false - } - const adminRoot = join(commonDir, 'worktrees') - let entryNames: string[] - try { - entryNames = (await readdir(adminRoot, { withFileTypes: true })) - .filter((entry) => entry.isDirectory()) - .map((entry) => entry.name) - } catch { - return false - } - const gitFilePath = join(worktreePath, '.git') - const owners: string[] = [] - for (const name of entryNames) { - const entryDir = join(adminRoot, name) - const backlink = await readFile(join(entryDir, 'gitdir'), 'utf8').then( - (content) => content.trim(), - () => '' - ) - const resolved = backlink && (isAbsolute(backlink) ? backlink : join(entryDir, backlink)) - if (resolved && areWorktreePathsEqual(resolved, gitFilePath)) { - owners.push(entryDir) - } - } - if (owners.length !== 1) { - return false - } - // Forward slashes are what Git itself writes, and what it compares against on Windows. - const adminDir = process.platform === 'win32' ? owners[0].replaceAll('\\', '/') : owners[0] - try { - await writeFile(gitFilePath, `gitdir: ${adminDir}\n`, { flag: 'wx' }) - } catch (error) { - // Something recreated `.git` meanwhile; Git's own validation judges it. - return getErrorCode(error) === 'EEXIST' - } - return true -} diff --git a/src/main/git/worktree-removal.ts b/src/main/git/worktree-removal.ts index 2f7eb8c38e00..77c117083910 100644 --- a/src/main/git/worktree-removal.ts +++ b/src/main/git/worktree-removal.ts @@ -112,26 +112,35 @@ function deleteBranchOfRemovedWorktree( } /** - * Finishes a removal whose checkout Git no longer registers (it finished deleting, or an earlier - * run did): leftover files, stale admin records, then the branch. Already-gone parts are done. - * `assertLeftover` refuses unless the path still holds the removed checkout's own leftover. + * Finishes a removal whose checkout Git no longer registers and is gone from disk: stale admin + * records, then the branch. Deletes no files: Orca removes a checkout only through Git. + * `assertCheckoutGone` refuses while anything is still at the path. */ export async function finishUnregisteredWorktreeRemoval( repoPath: string, worktreePath: string, branch: { name: string; head: string } | null, - assertLeftover: () => Promise, + assertCheckoutGone: () => Promise, + options: RemoveWorktreeOptions = {} +): Promise { + await assertCheckoutGone() + await gitExecFileAsync(['worktree', 'prune'], gitExecOptions(repoPath, options)).catch( + (error: unknown) => console.warn(`[git] worktree prune failed in ${repoPath}`, error) + ) + return deleteBranchOfUnregisteredWorktree(repoPath, worktreePath, branch, options) +} + +/** + * The branch step of a removal once Git no longer registers the checkout, whatever is left on disk: + * merged only, as `removeWorktree` does. Prunes nothing and deletes no files. + */ +export async function deleteBranchOfUnregisteredWorktree( + repoPath: string, + worktreePath: string, + branch: { name: string; head: string } | null, options: RemoveWorktreeOptions = {} ): Promise { try { - await runUnderWorktreeDeleteLimit(async () => { - // Why in the slot: the wait can outlast two large deletes, and the path may change meanwhile. - await assertLeftover() - await removeCheckoutLeftByGit(worktreePath, options) - }) - await gitExecFileAsync(['worktree', 'prune'], gitExecOptions(repoPath, options)).catch( - (error: unknown) => console.warn(`[git] worktree prune failed in ${repoPath}`, error) - ) if (!branch?.name || !(await localBranchExists(repoPath, branch.name, options))) { return {} } diff --git a/src/main/ipc/worktrees-failed-removal.test.ts b/src/main/ipc/worktrees-failed-removal.test.ts index 087bd6d8950f..8c02b4cb48d9 100644 --- a/src/main/ipc/worktrees-failed-removal.test.ts +++ b/src/main/ipc/worktrees-failed-removal.test.ts @@ -1,17 +1,19 @@ // Desktop IPC for a delete that failed after Git dropped the registration: the leftover stays in -// `worktrees:list` with the error, Delete retries it. Git and the disk are mocked; +// `worktrees:list` with the error; Delete refuses while it is on disk and finishes once it is gone. Git and the disk are mocked; // runtime-failed-local-worktree-removal.test.ts runs the real thing. import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { + gitExecFileAsyncMock, killAllProcessesForWorktreeMock, listWorktreesMock, removeWorktreeMock } from './worktrees-test-module-mocks' import { handlers, setupWorktreeHandlers, store } from './worktrees-test-harness' -import { mockKnownFeatureWorktree } from './worktrees-test-fixtures' +import { makeWorktreeMeta, mockKnownFeatureWorktree } from './worktrees-test-fixtures' import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' import type { Worktree } from '../../shared/worktree/types' import { finishUnregisteredWorktreeRemoval } from '../git/worktree-removal' +import { worktreeCheckoutExists } from '../worktree-removal-table' import type * as WorktreeRemovalModule from '../git/worktree-removal' import type * as WorktreeRemovalTable from '../worktree-removal-table' import type * as WorktreeRemovalLeftover from '../worktree-removal-leftover' @@ -161,6 +163,38 @@ describe('a failed delete Git no longer registers, over desktop IPC', () => { afterEach(() => { _resetPendingWorktreeRemovalsForTests() vi.mocked(finishUnregisteredWorktreeRemoval).mockClear() + vi.mocked(worktreeCheckoutExists).mockResolvedValue(true) + }) + + it('removes the push-target remote Orca added when Delete fails partway', async () => { + const [main] = mockKnownFeatureWorktree() + const pushTarget = { + remoteName: 'pr-contributor-orca', + branchName: 'feature', + remoteUrl: 'https://github.com/contributor/orca.git', + remoteCreated: true + } + store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta({ pushTarget })) + store.getAllWorktreeMeta.mockReturnValue({ [featureId]: makeWorktreeMeta({ pushTarget }) }) + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => + args[0] === 'remote' && args[1] === 'get-url' + ? { stdout: `${pushTarget.remoteUrl}\n`, stderr: '' } + : { stdout: '', stderr: '' } + ) + removeWorktreeMock.mockImplementation(async () => { + listWorktreesMock.mockResolvedValue([main]) + throw new Error(GIT_ERROR) + }) + + await expect(remove({ worktreeId: featureId })).rejects.toThrow(/Operation not permitted/) + await _settlePendingWorktreeRemovalsForTests() + + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['remote', 'remove', pushTarget.remoteName], { + cwd: '/workspace/repo' + }) + expect(await listFeature()).toMatchObject({ + removalError: expect.stringMatching(/Operation not permitted/) + }) }) it('stays in the listing with the error instead of vanishing', async () => { @@ -171,13 +205,31 @@ describe('a failed delete Git no longer registers, over desktop IPC', () => { expect(row?.removing).toBeUndefined() }) - it('Delete runs the recorded removal again: teardown, leftover, branch and metadata', async () => { + it('Delete refuses while the folder is on disk, deleting nothing and keeping the row', async () => { await failAfterGitDroppedIt() killAllProcessesForWorktreeMock.mockClear() + for (const force of [false, true]) { + await expect(remove({ worktreeId: featureId, force })).rejects.toThrow( + '/workspace/feature-wt is no longer a Git worktree, so its folder was left on disk. Delete the folder yourself to finish removing this workspace.' + ) + } + await _settlePendingWorktreeRemovalsForTests() + + expect(finishUnregisteredWorktreeRemoval).not.toHaveBeenCalled() + expect(removeWorktreeMock).not.toHaveBeenCalled() + expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(store.removeWorktreeMeta).not.toHaveBeenCalled() + expect(await listFeature()).toMatchObject({ removalError: GIT_ERROR }) + }) + + it('once the user removed the folder, Delete finishes the rest: teardown, branch and metadata', async () => { + await failAfterGitDroppedIt() + killAllProcessesForWorktreeMock.mockClear() + vi.mocked(worktreeCheckoutExists).mockResolvedValue(false) + await expect(remove({ worktreeId: featureId })).resolves.not.toHaveProperty('removing') - // Git has no registration to delete by; the leftover goes through Orca's own delete. expect(removeWorktreeMock).not.toHaveBeenCalled() expect(finishUnregisteredWorktreeRemoval).toHaveBeenCalledWith( '/workspace/repo', @@ -194,17 +246,6 @@ describe('a failed delete Git no longer registers, over desktop IPC', () => { expect(await listFeature()).toBeUndefined() }) - it('keeps the row with the new error when the retry fails again', async () => { - await failAfterGitDroppedIt() - vi.mocked(finishUnregisteredWorktreeRemoval).mockRejectedValueOnce(new Error('EPERM again')) - - await expect(remove({ worktreeId: featureId })).rejects.toThrow('EPERM again') - await _settlePendingWorktreeRemovalsForTests() - - expect(await listFeature()).toMatchObject({ removalError: 'EPERM again' }) - expect(store.removeWorktreeMeta).not.toHaveBeenCalled() - }) - it('joins a retry another client started while this Delete listed Git', async () => { await failAfterGitDroppedIt() const [main] = mockKnownFeatureWorktree() diff --git a/src/main/ipc/worktrees-orphan-directory-cleanup.test.ts b/src/main/ipc/worktrees-orphan-directory-cleanup.test.ts index 623f88713e52..2fe0982dc85f 100644 --- a/src/main/ipc/worktrees-orphan-directory-cleanup.test.ts +++ b/src/main/ipc/worktrees-orphan-directory-cleanup.test.ts @@ -1,5 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -import { lstat, mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { lstat, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { @@ -285,13 +285,13 @@ describe('registerWorktreeHandlers', () => { } }) - it('prompts then force-removes an Orca-created unregistered leftover directory with no git marker', async () => { + it('refuses an Orca-created unregistered directory with no git marker, even forced, until the user removes it', async () => { const parentDir = await mkdtemp(join(tmpdir(), 'orca-ipc-leftover-')) const repoPath = join(parentDir, 'repo') const leftoverPath = join(parentDir, 'leftover') const worktreeId = `repo-1::${leftoverPath}` await mkdir(leftoverPath, { recursive: true }) - await writeFile(join(leftoverPath, 'leftover.txt'), 'kept until force\n') + await writeFile(join(leftoverPath, 'notes.txt'), 'mine\n') store.getRepo.mockReturnValue({ id: 'repo-1', path: repoPath, @@ -312,30 +312,22 @@ describe('registerWorktreeHandlers', () => { }) try { - await expect(handlers['worktrees:remove'](null, { worktreeId })).rejects.toThrow( - 'Worktree is no longer registered with Git but its directory remains.' - ) - await expect(lstat(leftoverPath)).resolves.toBeTruthy() + for (const force of [false, true]) { + await expect(handlers['worktrees:remove'](null, { worktreeId, force })).rejects.toThrow( + `Refusing to delete unregistered worktree path: ${leftoverPath}` + ) + } + await expect(readFile(join(leftoverPath, 'notes.txt'), 'utf8')).resolves.toBe('mine\n') + expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() expect(removeWorktreeMock).not.toHaveBeenCalled() expect(store.removeWorktreeMeta).not.toHaveBeenCalled() - await expect( - handlers['worktrees:remove'](null, { worktreeId, force: true }) - ).resolves.toEqual({ catalogVersion: anyCatalogVersion }) - - await expect(lstat(leftoverPath)).rejects.toMatchObject({ code: 'ENOENT' }) - expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith( - worktreeId, - expect.objectContaining({ requirePhysicalStop: true }) - ) - expect(runHookMock).not.toHaveBeenCalled() + await rm(leftoverPath, { recursive: true, force: true }) + await expect(handlers['worktrees:remove'](null, { worktreeId })).resolves.toEqual({ + catalogVersion: anyCatalogVersion + }) expect(removeWorktreeMock).not.toHaveBeenCalled() - expect(runtimeStub.clearOptimisticReconcileToken).toHaveBeenCalledWith(worktreeId) expect(store.removeWorktreeMeta).toHaveBeenCalledWith(worktreeId, 'local') - expect(deleteWorktreeHistoryDirMock).toHaveBeenCalledWith(worktreeId) - expect(mainWindow.webContents.send).toHaveBeenCalledWith('worktrees:changed', { - repoId: 'repo-1' - }) } finally { await rm(parentDir, { recursive: true, force: true }) } diff --git a/src/main/ipc/worktrees/removal/execute-worktree-removal.ts b/src/main/ipc/worktrees/removal/execute-worktree-removal.ts index 5f8b0a8f962f..1b5767a633ce 100644 --- a/src/main/ipc/worktrees/removal/execute-worktree-removal.ts +++ b/src/main/ipc/worktrees/removal/execute-worktree-removal.ts @@ -33,7 +33,7 @@ import { removeFolderWorkspace } from './remove-folder-workspace' import { removeUnregisteredWorktree } from './remove-unregistered-worktree' import { removeRegisteredLocalWorktree } from './remove-registered-local-worktree' import { retryFailedLocalWorktreeRemoval } from './retry-failed-local-worktree-removal' -import { retryFailedRemovalUnlessRegistered } from '../../../worktree-removal-table' +import { retryFailedRemovalUnlessRegistered } from '../../../worktree-removal-leftover' export async function executeWorktreeRemoval( context: WorktreeIpcContext, @@ -70,8 +70,11 @@ export async function executeWorktreeRemoval( ]) } if ( - retryFailedRemovalUnlessRegistered(args.worktreeId, worktreePath, registeredWorktrees, () => - retryFailedLocalWorktreeRemoval(context, args, removalHostId) + await retryFailedRemovalUnlessRegistered( + args.worktreeId, + worktreePath, + registeredWorktrees, + () => retryFailedLocalWorktreeRemoval(context, args, removalHostId) ) ) { return { removing: true } 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 52d8f26ee24d..2693b5ca039e 100644 --- a/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts +++ b/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts @@ -132,6 +132,14 @@ export async function removeRegisteredLocalWorktree( } } + const cleanupPushTarget = () => + cleanupUnusedWorktreePushTargetRemote( + repo.path, + args.worktreeId, + removedPushTarget, + context.store, + localWorktreeGitOptions + ) const finish = (checkoutDeleteSignal?: AbortSignal): Promise => finishLocalWorktreeRemoval({ context, @@ -146,7 +154,8 @@ export async function removeRegisteredLocalWorktree( deleteBranch, refreshedRegisteredWorktree, removalGate, - checkoutDeleteSignal + checkoutDeleteSignal, + cleanupPushTarget }) if (!removesInBackground(canonicalWorktreePath, localWorktreeGitOptions)) { const result = await finish() @@ -173,7 +182,8 @@ export async function removeRegisteredLocalWorktree( }) return result }, - publish: () => runtime.publishWorktreeRemovalChange(repoId) + publish: () => runtime.publishWorktreeRemovalChange(repoId), + cleanupPushTargetRemote: cleanupPushTarget }) return { removing: true } } @@ -191,7 +201,8 @@ async function finishLocalWorktreeRemoval({ deleteBranch, refreshedRegisteredWorktree, removalGate, - checkoutDeleteSignal + checkoutDeleteSignal, + cleanupPushTarget }: { context: WorktreeIpcContext args: RemoveWorktreeArgs @@ -206,6 +217,7 @@ async function finishLocalWorktreeRemoval({ refreshedRegisteredWorktree: GitWorktreeInfo removalGate: { finish: (removed: boolean) => Promise } checkoutDeleteSignal?: AbortSignal + cleanupPushTarget: () => Promise }): Promise { const { store, runtime } = context let removalResult: RemoveWorktreeResult | undefined @@ -256,13 +268,7 @@ async function finishLocalWorktreeRemoval({ cwd: repo.path, ...localWorktreeGitOptions }).catch(() => {}) - await cleanupUnusedWorktreePushTargetRemote( - repo.path, - args.worktreeId, - removedPushTarget, - store, - localWorktreeGitOptions - ) + await cleanupPushTarget() runtime.clearOptimisticReconcileToken(args.worktreeId) removeWorktreeMetadataAndTransientState( store, @@ -291,13 +297,7 @@ async function finishLocalWorktreeRemoval({ } finally { await removalGate.finish(removalCompleted) } - await cleanupUnusedWorktreePushTargetRemote( - repo.path, - args.worktreeId, - removedPushTarget, - store, - localWorktreeGitOptions - ) + await cleanupPushTarget() rememberPreservedBranchCleanupTarget( args.worktreeId, removalHostId, diff --git a/src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts b/src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts index 3dafc3ab3afa..1980f77bf1e5 100644 --- a/src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts +++ b/src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts @@ -7,7 +7,6 @@ import type { LocalProjectWorktreeGitOptions } from '../../../project-runtime-gi import { preservedBranchCleanupScopeKey } from '../../../../shared/preserved-branch-cleanup' import { assertWorktreeDoesNotContainRegisteredWorktree, - canCleanupUnregisteredOrcaLeftoverDirectory, canCleanupUnregisteredOrcaWorktreeDirectory, canSafelyRemoveOrphanedWorktreeDirectory, isDangerousWorktreeRemovalPath, @@ -32,7 +31,7 @@ import { removeWorktreeMetadataAndTransientState, stopPtysForDestructiveWorktreeRemoval } from './worktree-removal-ownership' -import { isAlreadyRemovedWorktreePath, isLocalGitRepository } from './worktree-removal-filesystem' +import { isAlreadyRemovedWorktreePath } from './worktree-removal-filesystem' export async function removeUnregisteredWorktree( context: WorktreeIpcContext, @@ -105,59 +104,6 @@ export async function removeUnregisteredWorktree( notifyWorktreesChanged(mainWindow, repoId) return {} } - const access = getLocalWorktreePathAccess(localWorktreeGitOptions) - const runtimeWorktreePath = toLocalWorktreeRuntimePath(worktreePath, localWorktreeGitOptions) - if ( - await canCleanupUnregisteredOrcaLeftoverDirectory({ - meta: removedMeta, - worktreePath, - runtimeWorktreePath, - repo, - runtimeRepoPath: toLocalWorktreeRuntimePath(repo.path, localWorktreeGitOptions), - registeredWorktrees, - statPath: access.statPath, - home: removalHome, - isGitRepository: (path) => isLocalGitRepository(path, localWorktreeGitOptions) - }) - ) { - if (!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) - removalCompleted = true - } finally { - await removalGate.finish(removalCompleted) - } - await cleanupUnusedWorktreePushTargetRemote( - repo.path, - args.worktreeId, - removedPushTarget, - store, - localWorktreeGitOptions - ) - runtime.clearOptimisticReconcileToken(args.worktreeId) - removeWorktreeMetadataAndTransientState( - store, - args.worktreeId, - removalHostId, - args.snapshotPruneBatchId - ) - preservedBranchCleanupByScope.delete( - preservedBranchCleanupScopeKey({ - worktreeId: args.worktreeId, - hostId: removalHostId - }) - ) - invalidateAuthorizedRootsCache() - notifyWorktreesChanged(mainWindow, repoId) - return {} - } if (await isAlreadyRemovedWorktreePath(worktreePath, localWorktreeGitOptions)) { if (!args.force && !removedMeta) { // Why: without persisted metadata, require the renderer recovery path before deleting Orca-only state for an unregistered path. diff --git a/src/main/ipc/worktrees/removal/worktree-removal-filesystem.ts b/src/main/ipc/worktrees/removal/worktree-removal-filesystem.ts index 8acdfec54260..0a039c94c28e 100644 --- a/src/main/ipc/worktrees/removal/worktree-removal-filesystem.ts +++ b/src/main/ipc/worktrees/removal/worktree-removal-filesystem.ts @@ -2,7 +2,6 @@ import { getLocalWorktreePathAccess, toLocalWorktreeRuntimePath } from '../../../local-worktree-filesystem' -import { gitExecFileAsync } from '../../../git/runner' import { isWorktreePathMissing } from '../../../worktree-removal-safety' export async function isAlreadyRemovedWorktreePath( @@ -15,34 +14,3 @@ export async function isAlreadyRemovedWorktreePath( access.statPath ) } - -export async function isLocalGitRepository( - runtimeWorktreePath: string, - localWorktreeGitOptions: { wslDistro?: string } = {} -): Promise { - try { - await gitExecFileAsync(['status', '--short'], { - cwd: runtimeWorktreePath, - ...localWorktreeGitOptions - }) - return true - } catch (error) { - return !gitStatusErrorMeansNotRepository(error) - } -} - -export function gitStatusErrorMeansNotRepository(error: unknown): boolean { - const message = - error instanceof Error - ? error.message - : error && typeof error === 'object' && 'message' in error - ? String((error as { message: unknown }).message) - : typeof error === 'string' - ? error - : '' - const stderr = - error && typeof error === 'object' && 'stderr' in error - ? String((error as { stderr: unknown }).stderr) - : '' - return /not a git repository/i.test(`${message}\n${stderr}`) -} diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts index b0c987185f6e..65b07f7bc6d8 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts @@ -1,4 +1,5 @@ import { describe, expect, it, vi, type MockInstance } from 'vitest' +import { readFile } from 'node:fs/promises' import { _settlePendingWorktreeRemovalsForTests } from '../../worktree-background-removal' import { @@ -322,13 +323,13 @@ describe('OrcaRuntimeService', () => { } }) - it('prompts then force-removes an Orca-created runtime unregistered leftover directory with no git marker', async () => { + it('refuses an Orca-created runtime unregistered directory with no git marker, even forced, until the user removes it', async () => { const parentDir = await mkdtemp(join(tmpdir(), 'orca-runtime-leftover-')) const repoPath = join(parentDir, 'repo') const leftoverPath = join(parentDir, 'leftover') const worktreeId = `${TEST_REPO_ID}::${leftoverPath}` await mkdir(leftoverPath, { recursive: true }) - await writeFile(join(leftoverPath, 'leftover.txt'), 'kept until force\n') + await writeFile(join(leftoverPath, 'notes.txt'), 'mine\n') const { runtimeStore, removeWorktreeMeta } = createStaleRuntimeWorktreeStore(worktreeId, { orcaCreatedAt: Date.now(), orcaCreationSource: 'runtime' @@ -368,16 +369,18 @@ describe('OrcaRuntimeService', () => { try { vi.mocked(listWorktrees).mockResolvedValue([]) - await expect(runtime.removeManagedWorktree(worktreeId)).rejects.toThrow( - 'Worktree is no longer registered with Git but its directory remains.' - ) - await expect(lstat(leftoverPath)).resolves.toBeTruthy() + for (const force of [false, true]) { + await expect(runtime.removeManagedWorktree(worktreeId, { force })).rejects.toThrow( + `Refusing to delete unregistered worktree path: ${leftoverPath}` + ) + } + await expect(readFile(join(leftoverPath, 'notes.txt'), 'utf8')).resolves.toBe('mine\n') expect(removeWorktree).not.toHaveBeenCalled() expect(removeWorktreeMeta).not.toHaveBeenCalled() - await expect(runtime.removeManagedWorktree(worktreeId, { force: true })).resolves.toEqual({}) + await rm(leftoverPath, { recursive: true, force: true }) + await expect(runtime.removeManagedWorktree(worktreeId)).resolves.toEqual({}) - await expect(lstat(leftoverPath)).rejects.toMatchObject({ code: 'ENOENT' }) expect(assertWorktreeCleanForRemoval).not.toHaveBeenCalled() expect(runHook).not.toHaveBeenCalled() expect(removeWorktree).not.toHaveBeenCalled() @@ -387,7 +390,6 @@ describe('OrcaRuntimeService', () => { expect(deleteWorktreeHistoryDirMock).toHaveBeenCalledWith(worktreeId) expect(invalidateAuthorizedRootsCacheMock).toHaveBeenCalled() expect(notifier.worktreesChanged).toHaveBeenCalledWith(TEST_REPO_ID) - expect(gitSpy).toHaveBeenCalledWith(['status', '--short'], { cwd: leftoverPath }) } finally { gitSpy.mockRestore() await rm(parentDir, { recursive: true, force: true, maxRetries: 5, retryDelay: 50 }) 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 f2199ebedd73..e3a7c88f1693 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 @@ -174,7 +174,7 @@ class RemovalRuntime extends OrcaRuntimeService { } retryRemoval(record: WorktreeRemovalRecord) { - return this.retryFailedLocalRemoval( + return this.retryFailedRemoval( { kind: 'local' }, { id: record.worktreeId, path: record.worktreePath }, [], @@ -223,10 +223,10 @@ describe('workspace removal runtime dispatch', () => { expect(() => runtime.finishInterruptedWorktreeRemovals()).toThrow(runtime.helperFailure) }) - it('uses the runtime helper override before retrying a failed removal', () => { + it('uses the runtime helper override before retrying a failed removal', async () => { runtime.stopAtHelper = true failedWorktreeRemovals.set(record.worktreeId, record) - expect(() => runtime.retryRemoval(record)).toThrow(runtime.helperFailure) + await expect(runtime.retryRemoval(record)).rejects.toThrow(runtime.helperFailure) }) it('captures the watcher callback and retains its job-host receiver', async () => { diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-failed-retry.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-failed-retry.spec.ts index 00cd9ad82226..b44a84d748d0 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-failed-retry.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-failed-retry.spec.ts @@ -1,8 +1,8 @@ // A runtime Delete (paired desktop, web, mobile, CLI) on the leftover of a delete that failed after -// Git dropped the registration: the leftover is listed with its error and Delete runs the recorded -// removal again, instead of the leftover vanishing from every listing. +// Git dropped the registration: the leftover is listed with its error instead of vanishing from +// every listing; Delete refuses while the folder is there and finishes the rest once it is gone. import { existsSync } from 'node:fs' -import { realpath } from 'node:fs/promises' +import { readFile, realpath, writeFile } from 'node:fs/promises' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { join, @@ -74,19 +74,45 @@ describe('runtime Delete on a failed delete’s leftover', () => { }) }) - it('runs the recorded removal again, answering a client that cannot wait on acceptance', async () => { - vi.spyOn(console, 'warn').mockImplementation(() => {}) + it('refuses while the folder Git no longer tracks is there, keeping the row', async () => { const runtime = createWorktreeRemovalRuntime() - - await expect( - runtime.removeManagedWorktree(`id:${leftoverId}`, { waitForBackgroundRemoval: false }) - ).resolves.toEqual({ removing: true }) + // The user replaced the leftover with an ordinary folder of their own. + await rm(leftover, { recursive: true, force: true }) + await mkdir(leftover) + await writeFile(join(leftover, 'notes.txt'), 'mine') + + for (const force of [false, true]) { + await expect( + runtime.removeManagedWorktree(`id:${leftoverId}`, { + force, + waitForBackgroundRemoval: false + }) + ).rejects.toThrow( + `${leftover} is no longer a Git worktree, so its folder was left on disk. Delete the folder yourself to finish removing this workspace.` + ) + } await _settlePendingWorktreeRemovalsForTests() - // Git has no registration left for it, so Orca deletes the leftover itself. + expect(await readFile(join(leftover, 'notes.txt'), 'utf8')).toBe('mine') + expect(removeWorktree).not.toHaveBeenCalled() + const detected = await runtime.listDetectedManagedWorktrees(`id:${TEST_REPO_ID}`) + expect(detected.worktrees.find((row) => row.id === leftoverId)).toMatchObject({ + removalError: FAILURE + }) + expect(await readWorktreeRemovalRecords(directory)).toMatchObject([ + { worktreeId: leftoverId, failure: { message: FAILURE } } + ]) + }) + + it('ends the row once the user removed the folder', async () => { + const runtime = createWorktreeRemovalRuntime() + await rm(leftover, { recursive: true, force: true }) + + const detected = await runtime.listDetectedManagedWorktrees(`id:${TEST_REPO_ID}`) + + expect(detected.worktrees.find((row) => row.id === leftoverId)).toBeUndefined() + await vi.waitFor(async () => expect(await readWorktreeRemovalRecords(directory)).toEqual([])) expect(removeWorktree).not.toHaveBeenCalled() - expect(existsSync(leftover)).toBe(false) - expect(await readWorktreeRemovalRecords(directory)).toEqual([]) }) it('takes the normal delete once Git registers a checkout at the path again', async () => { @@ -132,16 +158,6 @@ describe('runtime Delete on a failed delete’s leftover', () => { expect(existsSync(join(leftover, 'node_modules'))).toBe(true) expect(removeWorktree).not.toHaveBeenCalled() }) - - it('replies to a waiting client once the retry finishes', async () => { - vi.spyOn(console, 'warn').mockImplementation(() => {}) - const runtime = createWorktreeRemovalRuntime() - - await expect( - runtime.removeManagedWorktree(`id:${leftoverId}`, { waitForBackgroundRemoval: true }) - ).resolves.toEqual({}) - expect(existsSync(leftover)).toBe(false) - }) }) describe('runtime listing straight after a delete fails partway', () => { 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 2aad80e10915..dcd6d8d22fbd 100644 --- a/src/main/runtime/runtime-failed-local-worktree-removal.test.ts +++ b/src/main/runtime/runtime-failed-local-worktree-removal.test.ts @@ -4,24 +4,19 @@ // the file's owner; worktree-failed-removal.test.ts covers the same rules with Git mocked. import { execFile } from 'node:child_process' import { existsSync } from 'node:fs' -import { mkdir, mkdtemp, realpath, rm, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, readFile, realpath, rename, rm, 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 { Repo } from '../../shared/repo-types' +import type { GitPushTarget } from '../../shared/worktree/types' import { removeTree } from '../../shared/windows-transient-lock-removal' import type { Store } from '../persistence' import type * as HostTreeRemoval from '../host-tree-removal' -import type * as GitFileRestore from '../git/worktree-git-file-restore' -import { restoreMissingWorktreeGitFile } from '../git/worktree-git-file-restore' import { removeHostTree } from '../host-tree-removal' import { listWorktreesStrict, removeWorktree } from '../git/worktree' import { areWorktreePathsEqual } from '../git/worktree-path-comparison' -import { - _worktreeDeleteLimitSnapshotForTests, - runUnderWorktreeDeleteLimit -} from '../git/worktree-delete-limit' import { acquireWatcherRemovalGate, beginTerminalInstall } from '../ipc/watcher-removal-gate' import { _resetPendingWorktreeRemovalsForTests, @@ -33,20 +28,19 @@ import { waitForPendingWorktreeRemoval } from '../worktree-background-removal' import { withUnregisteredRemovalCheckouts } from '../worktree-removal-listing' +import { retryFailedRemovalUnlessRegistered } from '../worktree-removal-leftover' import { readWorktreeRemovalRecords, writeWorktreeRemovalRecords, type WorktreeRemovalRecord } from '../worktree-removal-records' import { interruptedLocalWorktreeRemovalJob } from './runtime-interrupted-local-worktree-removal' +import { removeRuntimeRegisteredLocalWorktree } from './runtime-registered-local-worktree-removal' +import type { RuntimeStore } from './runtime-store-contract' vi.mock('../project-runtime-git-options', () => ({ getLocalProjectWorktreeGitOptions: () => ({}) })) -vi.mock('../git/worktree-git-file-restore', async (importOriginal) => { - const actual = await importOriginal() - return { ...actual, restoreMissingWorktreeGitFile: vi.fn(actual.restoreMissingWorktreeGitFile) } -}) vi.mock('../host-tree-removal', async (importOriginal) => { const actual = await importOriginal() return { ...actual, removeHostTree: vi.fn(actual.removeHostTree) } @@ -83,14 +77,34 @@ async function listedRows(): Promise<{ path: string; removalError?: string }[]> .map(({ path, removalError }) => ({ path, ...(removalError ? { removalError } : {}) })) } -function jobHost(purged: string[], stopPtys = vi.fn(async () => {})) { +/** Reads only repos and worktree metadata, as the finish and the push-target cleanup do. */ +function storeStub(pushTarget?: GitPushTarget) { + const meta = pushTarget ? { pushTarget } : undefined + return { + getRepo: (id: string) => (id === repo.id ? repo : undefined), + getRepos: () => [repo], + getWorktreeMeta: (id: string) => (id === worktreeId ? meta : undefined), + getAllWorktreeMeta: () => (meta ? { [worktreeId]: meta } : {}) + } +} + +/** A remote Orca added for a fork PR's push target, as worktree create does. */ +async function addOrcaPushTargetRemote(): Promise { + const pushTarget = { + remoteName: 'pr-contributor', + branchName: 'feature', + remoteUrl: 'https://github.com/contributor/repo.git', + remoteCreated: true + } + await git(['remote', 'add', pushTarget.remoteName, pushTarget.remoteUrl]) + await git(['config', `remote.${pushTarget.remoteName}.orca-created`, 'true']) + return pushTarget +} + +function jobHost(purged: string[], stopPtys = vi.fn(async () => {}), pushTarget?: GitPushTarget) { return { - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the finish reads only repos and worktree metadata from the store here; git options are mocked and no push target is set. - store: { - getRepo: (id: string) => (id === repo.id ? repo : undefined), - getRepos: () => [repo], - getWorktreeMeta: () => undefined - } as unknown as Store, + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the finish reads only repos and worktree metadata from the store here; git options are mocked. + store: storeStub(pushTarget) as unknown as Store, acquireWatcherRemoval: async (path: string) => { const gate = acquireWatcherRemovalGate(path) return { finish: async () => gate.release() } @@ -104,13 +118,35 @@ function jobHost(purged: string[], stopPtys = vi.fn(async () => {})) { } satisfies Parameters[1] } +/** 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)) + ) + ).catch((reason: unknown) => reason) + if (retried !== true) { + return retried + } + const error = await waitForPendingWorktreeRemoval(worktreeId)?.then( + () => undefined, + (reason: unknown) => reason + ) + await _settlePendingWorktreeRemovalsForTests() + return error +} + +const refusal = (): string => + `${worktreePath} is no longer a Git worktree, so its folder was left on disk. Delete the folder yourself to finish removing this workspace.` + /** A delete a quit interrupted, finished at the next start, where Git fails on the locked file. */ function failStartupFinish(): Promise { return finishAtStartup([]) } /** Resumes a recorded delete of the checkout as the next start does; resolves with its error. */ -async function finishAtStartup(purged: string[]): Promise { +async function finishAtStartup(purged: string[], pushTarget?: GitPushTarget): Promise { const record: WorktreeRemovalRecord = { worktreeId, repoId: repo.id, @@ -126,7 +162,7 @@ async function finishAtStartup(purged: string[]): Promise { await loadWorktreeRemovalRecords(recordsDir) const joined = waitForPendingWorktreeRemoval(worktreeId)! resumeInterruptedWorktreeRemovals((interrupted) => - interruptedLocalWorktreeRemovalJob(interrupted, jobHost(purged)) + interruptedLocalWorktreeRemovalJob(interrupted, jobHost(purged, undefined, pushTarget)) ) const error = await joined.then( () => undefined, @@ -195,10 +231,11 @@ describe.skipIf(process.platform !== 'darwin')('a worktree delete Git fails part const error = await failStartupFinish() expect(String(error)).toMatch(/Operation not permitted/) - // What Git left: no registration, but the checkout, the branch and Orca's record. + // What Git left: no registration, but the checkout and Orca's record. The merged branch went + // with the failed delete, as its finish would have deleted it. expect(await isRegistered(worktreePath)).toBe(false) expect(existsSync(lockedFile)).toBe(true) - expect(await git(['branch', '--list', 'feature'])).not.toBe('') + expect(await git(['branch', '--list', 'feature'])).toBe('') expect(await listedRows()).toEqual([ { path: worktreePath, removalError: expect.stringMatching(/Operation not permitted/) } ]) @@ -230,35 +267,148 @@ describe.skipIf(process.platform !== 'darwin')('a worktree delete Git fails part expect(await listedRows()).toHaveLength(1) }) - it('Delete retries it once the file can be deleted: files, branch, metadata and record go', async () => { + it('Delete refuses while the leftover is there, and finishes metadata and record once the user removed it', async () => { await failInSession() await setImmutable(false) const purged: string[] = [] const stopPtys = vi.fn(async () => {}) - const result = await retryFailedWorktreeRemoval(worktreeId, 'local', (record) => - interruptedLocalWorktreeRemovalJob(record, jobHost(purged, stopPtys)) - ) - await _settlePendingWorktreeRemovalsForTests() + expect(String(await deleteRow(purged, stopPtys))).toBe(`Error: ${refusal()}`) + expect(existsSync(lockedFile)).toBe(true) + expect(stopPtys).not.toHaveBeenCalled() + expect(purged).toEqual([]) + expect(await listedRows()).toEqual([ + { path: worktreePath, removalError: expect.stringMatching(/Operation not permitted/) } + ]) + + await rm(worktreePath, { recursive: true }) + expect(await deleteRow(purged, stopPtys)).toBeUndefined() - expect(result).toEqual({}) expect(stopPtys).toHaveBeenCalledTimes(1) - // Git has no registration left to delete by, so Orca deletes the leftover itself. - expect(removeHostTree).toHaveBeenCalledWith(worktreePath) - expect(existsSync(worktreePath)).toBe(false) + expect(removeHostTree).not.toHaveBeenCalled() expect(await git(['branch', '--list', 'feature'])).toBe('') expect(purged).toEqual([worktreeId]) expect(await readWorktreeRemovalRecords(recordsDir)).toEqual([]) expect(await listedRows()).toEqual([]) }) - it('the record ends once the checkout is deleted outside Orca', async () => { + it('Delete deletes nothing in an ordinary folder the user put in place of the leftover', async () => { await failInSession() await setImmutable(false) await rm(worktreePath, { recursive: true }) + await mkdir(worktreePath) + await writeFile(join(worktreePath, 'notes.txt'), 'mine\n') + const purged: string[] = [] + expect(String(await deleteRow(purged))).toBe(`Error: ${refusal()}`) + + expect(await readFile(join(worktreePath, 'notes.txt'), 'utf8')).toBe('mine\n') + expect(removeHostTree).not.toHaveBeenCalled() + expect(purged).toEqual([]) + expect(await listedRows()).toEqual([ + { path: worktreePath, removalError: expect.stringMatching(/Operation not permitted/) } + ]) + expect(await readWorktreeRemovalRecords(recordsDir)).toMatchObject([ + { worktreeId, failure: { message: expect.stringMatching(/Operation not permitted/) } } + ]) + }) + + it('deletes the merged branch when the delete fails, and only the folder is left to the user', async () => { + await failInSession() + + expect(await git(['branch', '--list', 'feature'])).toBe('') + expect(existsSync(lockedFile)).toBe(true) + expect(removeHostTree).not.toHaveBeenCalled() + expect(await listedRows()).toEqual([ + { path: worktreePath, removalError: expect.stringMatching(/Operation not permitted/) } + ]) + + // The listing ends the record once the folder is gone; nothing of the delete is left owed. + await setImmutable(false) + await rm(worktreePath, { recursive: true }) expect(await listedRows()).toEqual([]) await vi.waitFor(async () => expect(await readWorktreeRemovalRecords(recordsDir)).toEqual([])) + expect(await git(['branch', '--list', 'feature'])).toBe('') + expect(await git(['worktree', 'list', '--porcelain'])).not.toContain(worktreePath) + }) + + it('leaves another worktree registered while its folder is missing, as on an unmounted drive', async () => { + const parent = join(scratchDir, 'volume') + const other = join(parent, 'other') + await git(['worktree', 'add', '-q', other, '-b', 'other']) + await writeFile(join(other, 'wip.txt'), 'wip\n') + await rename(parent, join(scratchDir, 'volume-unmounted')) + + expect(String(await failInSession())).toMatch(/Operation not permitted/) + + expect(await isRegistered(other)).toBe(true) + await rename(join(scratchDir, 'volume-unmounted'), parent) + expect(await git(['status', '--porcelain'], other)).toBe('?? wip.txt\n') + }) + + it('removes the push-target remote Orca added when a runtime delete fails', async () => { + const pushTarget = await addOrcaPushTargetRemote() + const worktree = (await listWorktreesStrict(repoPath)).find((row) => + areWorktreePathsEqual(row.path, worktreePath) + )! + const accepted = await removeRuntimeRegisteredLocalWorktree({ + repo, + target: { id: worktreeId, repoId: repo.id, path: worktreePath, pushTarget }, + registeredWorktree: worktree, + removedPushTarget: pushTarget, + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the delete reads only repos and worktree metadata from the store here. + store: storeStub(pushTarget) as unknown as RuntimeStore, + localOptions: {}, + hasLocalOptions: false, + force: true, + runHooks: false, + allowFailedArchiveHook: false, + allowUnverifiedPtyStop: false, + deleteBranch: true, + acquireWatcherRemoval: async (path) => { + const gate = acquireWatcherRemovalGate(path) + return { finish: async () => gate.release() } + }, + stopPtys: async () => {}, + closeWatchers: async () => {}, + preserveBranchHead: (result) => result ?? {}, + finishRemoval: () => {}, + onRemoved: () => {}, + publish: () => {} + }) + expect(accepted).toMatchObject({ removing: true }) + const error = await waitForPendingWorktreeRemoval(worktreeId)?.catch( + (reason: unknown) => reason + ) + await _settlePendingWorktreeRemovalsForTests() + + expect(String(error)).toMatch(/Operation not permitted/) + expect(await git(['remote'])).not.toContain(pushTarget.remoteName) + expect(existsSync(lockedFile)).toBe(true) + }) + + it('removes the push-target remote Orca added when the startup finish fails', async () => { + const pushTarget = await addOrcaPushTargetRemote() + + expect(String(await finishAtStartup([], pushTarget))).toMatch(/Operation not permitted/) + + expect(await git(['remote'])).not.toContain(pushTarget.remoteName) + expect(existsSync(lockedFile)).toBe(true) + }) + + it('keeps an unmerged branch when the delete fails, as a normal delete does', async () => { + await writeFile(join(worktreePath, 'work.txt'), 'work\n') + await git(['add', 'work.txt'], worktreePath) + await git(['commit', '-qm', 'work'], worktreePath) + const head = (await git(['rev-parse', 'feature'])).trim() + + expect(String(await failInSession())).toMatch(/Operation not permitted/) + + expect((await git(['rev-parse', 'feature'])).trim()).toBe(head) + expect(existsSync(lockedFile)).toBe(true) + expect(await listedRows()).toEqual([ + { path: worktreePath, removalError: expect.stringMatching(/Operation not permitted/) } + ]) }) it('never deletes a different checkout created at the path since', async () => { @@ -270,11 +420,11 @@ describe.skipIf(process.platform !== 'darwin')('a worktree delete Git fails part await writeFile(join(worktreePath, 'unsaved.txt'), 'work\n') const purged: string[] = [] - // Delete before any listing noticed: the retry refuses and lets the record go. + // A retry already past Delete's check: the job refuses and lets the record go. const retried = retryFailedWorktreeRemoval(worktreeId, 'local', (record) => interruptedLocalWorktreeRemovalJob(record, jobHost(purged)) ) - await expect(retried).rejects.toThrow(/A different checkout is now at/) + await expect(retried).rejects.toThrow(refusal()) await _settlePendingWorktreeRemovalsForTests() expect(existsSync(join(worktreePath, 'unsaved.txt'))).toBe(true) @@ -288,7 +438,8 @@ describe.skipIf(process.platform !== 'darwin')('a worktree delete Git fails part await failStartupFinish() await setImmutable(false) await rm(worktreePath, { recursive: true }) - await git(['worktree', 'add', '-q', worktreePath, 'feature']) + // The failed delete already deleted the merged branch; the user makes it again. + await git(['worktree', 'add', '-q', worktreePath, '-b', 'feature']) await writeFile(join(worktreePath, 'unsaved.txt'), 'work\n') const purged: string[] = [] @@ -316,67 +467,61 @@ describe.skipIf(process.platform !== 'darwin')('a worktree delete Git fails part const retried = retryFailedWorktreeRemoval(worktreeId, 'local', (record) => interruptedLocalWorktreeRemovalJob(record, jobHost(purged)) ) - await expect(retried).rejects.toThrow(/contains another registered worktree/) + // Named first: removing the folder, as the usual refusal asks, would delete it too. + const nestedRefusal = `Refusing to delete worktree because it contains another registered worktree: ${nested}` + await expect(retried).rejects.toThrow(nestedRefusal) await _settlePendingWorktreeRemovalsForTests() expect(existsSync(join(nested, 'unsaved.txt'))).toBe(true) expect(await isRegistered(nested)).toBe(true) expect(removeHostTree).not.toHaveBeenCalled() expect(purged).toEqual([]) - // The row keeps the refusal, so the user can move the nested worktree and Delete again. - expect(await listedRows()).toContainEqual({ - path: worktreePath, - removalError: expect.stringMatching(/contains another registered worktree/) - }) + expect(await listedRows()).toContainEqual({ path: worktreePath, removalError: nestedRefusal }) + // Delete's own check names it as well, before any job runs. + expect(String(await deleteRow(purged))).toBe(`Error: ${nestedRefusal}`) + expect(existsSync(join(nested, 'unsaved.txt'))).toBe(true) }) - it('checks the leftover again inside the delete slot, right before deleting', async () => { + it('checks again after the teardown that the folder is still gone', async () => { await failInSession() await setImmutable(false) - // Both delete slots busy, as behind two large deletes. - let releaseSlots = (): void => {} - const held = new Promise((resolve) => { - releaseSlots = resolve - }) - const holders = [ - runUnderWorktreeDeleteLimit(() => held), - runUnderWorktreeDeleteLimit(() => held) - ] - const retried = retryFailedWorktreeRemoval(worktreeId, 'local', (record) => - interruptedLocalWorktreeRemovalJob(record, jobHost([])) - ) - const settled = retried!.then( - () => undefined, - (reason: unknown) => reason - ) - await vi.waitFor(() => expect(_worktreeDeleteLimitSnapshotForTests().waiting).toBe(1)) - // While it waits, the leftover is replaced by a different checkout. await rm(worktreePath, { recursive: true }) - await mkdir(worktreePath) - await git(['init', '-q'], worktreePath) - await writeFile(join(worktreePath, 'unsaved.txt'), 'work\n') - releaseSlots() - await Promise.all(holders) + const purged: string[] = [] + // A folder appears at the path while the retry stops terminals. + const stopPtys = vi.fn(async () => { + await mkdir(worktreePath) + await writeFile(join(worktreePath, 'notes.txt'), 'mine\n') + }) - expect(String(await settled)).toMatch(/A different checkout is now at/) - await _settlePendingWorktreeRemovalsForTests() - expect(existsSync(join(worktreePath, 'unsaved.txt'))).toBe(true) + expect(String(await deleteRow(purged, stopPtys))).toBe(`Error: ${refusal()}`) + + expect(await readFile(join(worktreePath, 'notes.txt'), 'utf8')).toBe('mine\n') expect(removeHostTree).not.toHaveBeenCalled() + expect(purged).toEqual([]) }) - it('at startup, still deletes the recorded checkout when its missing .git cannot be restored', async () => { + it('at startup, unregisters a checkout whose .git is gone, keeping its files, until the user removes it', async () => { await setImmutable(false) - // Git deleted `.git` first and the link cannot be written back, so Git cannot remove it. await rm(join(worktreePath, '.git')) - vi.mocked(restoreMissingWorktreeGitFile).mockResolvedValueOnce(false) const purged: string[] = [] - expect(await finishAtStartup(purged)).toBeUndefined() + expect(String(await finishAtStartup(purged))).toBe(`Error: ${refusal()}`) - expect(removeHostTree).toHaveBeenCalledWith(worktreePath) - expect(existsSync(worktreePath)).toBe(false) + expect(existsSync(join(worktreePath, '.git'))).toBe(false) + expect(existsSync(lockedFile)).toBe(true) + expect(removeHostTree).not.toHaveBeenCalled() expect(await isRegistered(worktreePath)).toBe(false) expect(await git(['branch', '--list', 'feature'])).toBe('') + expect(purged).toEqual([]) + expect(await listedRows()).toEqual([{ path: worktreePath, removalError: refusal() }]) + + expect(String(await deleteRow(purged))).toBe(`Error: ${refusal()}`) + expect(existsSync(lockedFile)).toBe(true) + + await rm(worktreePath, { recursive: true }) + expect(await deleteRow(purged)).toBeUndefined() expect(purged).toEqual([worktreeId]) + expect(await readWorktreeRemovalRecords(recordsDir)).toEqual([]) + expect(await listedRows()).toEqual([]) }) }) 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 8fd770875878..d2b5be461c4c 100644 --- a/src/main/runtime/runtime-interrupted-local-worktree-removal.test.ts +++ b/src/main/runtime/runtime-interrupted-local-worktree-removal.test.ts @@ -2,7 +2,7 @@ // come from Git and disk, whatever point the earlier run reached. import { execFile } from 'node:child_process' import { existsSync } from 'node:fs' -import { mkdir, mkdtemp, realpath, unlink, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, readFile, realpath, unlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { promisify } from 'node:util' @@ -13,8 +13,6 @@ 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 type * as WorktreeGitFileRestore from '../git/worktree-git-file-restore' -import { restoreMissingWorktreeGitFile } from '../git/worktree-git-file-restore' import { listWorktreesStrict } from '../git/worktree' import { areWorktreePathsEqual } from '../git/worktree-path-comparison' import { @@ -29,6 +27,7 @@ import { resumeInterruptedWorktreeRemovals, waitForPendingWorktreeRemoval } from '../worktree-background-removal' +import { withUnregisteredRemovalCheckouts } from '../worktree-removal-listing' import { readWorktreeRemovalRecords, writeWorktreeRemovalRecords, @@ -43,13 +42,6 @@ vi.mock('../host-tree-removal', async (importOriginal) => { const actual = await importOriginal() return { ...actual, removeHostTree: vi.fn(actual.removeHostTree) } }) -vi.mock('../git/worktree-git-file-restore', async (importOriginal) => { - const actual = await importOriginal() - return { - ...actual, - restoreMissingWorktreeGitFile: vi.fn(actual.restoreMissingWorktreeGitFile) - } -}) const execFileAsync = promisify(execFile) @@ -99,10 +91,13 @@ type FinishOutcome = | ({ status: 'removed' } & RemoveWorktreeResult) | { status: 'failed'; error: string } -async function finishAfterRestart(options: { repoGone?: boolean; head?: string } = {}): Promise<{ +async function finishAfterRestart( + options: { repoGone?: boolean; head?: string; force?: boolean } = {} +): Promise<{ outcome: FinishOutcome purged: string[] remember: ReturnType + records: WorktreeRemovalRecord[] }> { const record: WorktreeRemovalRecord = { worktreeId: `repo-1::${worktreePath}`, @@ -112,7 +107,7 @@ async function finishAfterRestart(options: { repoGone?: boolean; head?: string } branch: 'feature', head: options.head ?? (await git(['rev-parse', 'feature'])).trim(), deleteBranch: true, - force: false, + force: options.force ?? false, requestedAt: 1 } await writeWorktreeRemovalRecords(recordsDir, () => [record]) @@ -156,21 +151,29 @@ async function finishAfterRestart(options: { repoGone?: boolean; head?: string } (error: unknown) => ({ status: 'failed' as const, error: String(error) }) ) await _settlePendingWorktreeRemovalsForTests() - expect(await readWorktreeRemovalRecords(recordsDir)).toEqual([]) + const records = await readWorktreeRemovalRecords(recordsDir) expect(waitForPendingWorktreeRemoval(record.worktreeId)).toBeUndefined() // Released on every outcome, including a finish that ended before taking its own gate. beginTerminalInstall(worktreePath)() - return { outcome, purged, remember } + return { outcome, purged, remember, records } +} + +/** 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) + .map(({ path, removalError }) => ({ path, ...(removalError ? { removalError } : {}) })) } describe('finishing an interrupted worktree removal after a restart', () => { - it('finishes a checkout Git was still deleting, branch and metadata included', async () => { + 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 } = await finishAfterRestart() + 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() @@ -180,55 +183,93 @@ describe('finishing an interrupted worktree removal after a restart', () => { expect(purged).toEqual([`repo-1::${worktreePath}`]) }) - it('lets Git finish a checkout whose .git file it had already deleted', async () => { - // Git deletes in directory order; without `.git` it refuses the checkout ("validation failed"). - await unlink(join(worktreePath, '.git')) - await unlink(join(worktreePath, 'seed.txt')) - - const { outcome, purged } = await finishAfterRestart() + it('finishes a clean checkout with the recorded non-forced choice', async () => { + const { outcome, purged, records } = await finishAfterRestart({ force: false }) 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}`]) - // Git's process deleted it, not Orca's. - expect(removeHostTree).not.toHaveBeenCalled() }) - it('lets Git finish a relative-path checkout whose .git file it had already deleted', async (ctx) => { - await git(['worktree', 'remove', worktreePath]) - try { - await git(['worktree', 'add', '-q', '--relative-paths', worktreePath, 'feature']) - } catch { - // Git before 2.48 has no relative-path worktrees. - ctx.skip() + it('keeps the user’s non-forced choice: Git refuses local changes and the row returns as before', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + await writeFile(join(worktreePath, 'notes.txt'), 'mine\n') + + const { outcome, purged, records } = await finishAfterRestart({ force: false }) + + expect(outcome).toMatchObject({ status: 'failed', error: expect.stringMatching(/--force/) }) + 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 }]) + }) + + it.each([true, false])( + 'unregisters a checkout whose .git Git deleted first, keeping its files (force %s)', + async (force) => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + // Git deletes in directory order; without `.git` no `worktree remove` can validate it. + await unlink(join(worktreePath, '.git')) + + const { outcome, purged, records } = await finishAfterRestart({ force }) + + 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}` }) + expect(existsSync(join(worktreePath, '.git'))).toBe(false) + expect(existsSync(join(worktreePath, 'seed.txt'))).toBe(true) + 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(purged).toEqual([]) + expect(records).toMatchObject([{ failure: { message: refusal } }]) + expect(await listedRows()).toEqual([{ path: worktreePath, removalError: refusal }]) } - await unlink(join(worktreePath, '.git')) + ) + + it('keeps the user’s non-forced choice after Git deleted part of the checkout: the row returns as before', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + await writeFile(join(worktreePath, 'kept.txt'), 'kept\n') + await git(['add', 'kept.txt'], worktreePath) + await git(['commit', '-qm', 'kept'], worktreePath) + // Git stopped partway: some tracked files are gone, `.git` is still there. await unlink(join(worktreePath, 'seed.txt')) - const { outcome, purged } = await finishAfterRestart() + const { outcome, purged, records } = await finishAfterRestart({ force: false }) - expect(outcome).toMatchObject({ status: 'removed' }) - expect(existsSync(worktreePath)).toBe(false) - expect(await isRegistered(worktreePath)).toBe(false) - expect(purged).toEqual([`repo-1::${worktreePath}`]) - expect(removeHostTree).not.toHaveBeenCalled() + expect(outcome).toMatchObject({ status: 'failed', error: expect.stringMatching(/--force/) }) + 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 }]) }) - it('deletes the leftover itself only when Git cannot be pointed back at it', async () => { + it('deletes nothing in a folder put at the path after Git unregistered the checkout', async () => { vi.spyOn(console, 'warn').mockImplementation(() => {}) - await unlink(join(worktreePath, '.git')) - vi.mocked(restoreMissingWorktreeGitFile).mockResolvedValueOnce(false) + await git(['worktree', 'remove', worktreePath]) + await mkdir(worktreePath, { recursive: true }) + await writeFile(join(worktreePath, 'notes.txt'), 'mine\n') - const { outcome, purged } = await finishAfterRestart() + const { outcome, purged, records } = await finishAfterRestart({ force: true }) - expect(outcome).toMatchObject({ status: 'removed' }) - expect(removeHostTree).toHaveBeenCalledWith(worktreePath) - expect(existsSync(worktreePath)).toBe(false) - expect(await isRegistered(worktreePath)).toBe(false) + 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}` }) + expect(await readFile(join(worktreePath, 'notes.txt'), 'utf8')).toBe('mine\n') + expect(removeHostTree).not.toHaveBeenCalled() + // Git let go of the checkout, so the recorded branch delete runs (merged only); no files. expect(await git(['branch', '--list', 'feature'])).toBe('') - expect(purged).toEqual([`repo-1::${worktreePath}`]) + expect(purged).toEqual([]) + expect(records).toMatchObject([{ failure: { message: refusal } }]) + expect(await listedRows()).toEqual([{ path: worktreePath, removalError: refusal }]) }) it('leaves a different checkout created at the same path since the quit', async () => { @@ -238,9 +279,10 @@ describe('finishing an interrupted worktree removal after a restart', () => { await git(['worktree', 'add', '-q', worktreePath, '-b', 'other']) await writeFile(join(worktreePath, 'unsaved.txt'), 'work\n') - const { outcome, purged } = await finishAfterRestart({ head }) + const { outcome, purged, records } = await finishAfterRestart({ head, force: true }) expect(outcome).toMatchObject({ status: 'failed' }) + expect(records).toEqual([]) expect(existsSync(join(worktreePath, 'unsaved.txt'))).toBe(true) expect(await isRegistered(worktreePath)).toBe(true) expect(purged).toEqual([]) @@ -253,9 +295,11 @@ describe('finishing an interrupted worktree removal after a restart', () => { await git(['init', '-q'], worktreePath) await writeFile(join(worktreePath, 'unsaved.txt'), 'work\n') - const { outcome, purged } = await finishAfterRestart() + const { outcome, purged, records } = await finishAfterRestart({ force: true }) expect(outcome).toMatchObject({ status: 'failed' }) + // Not the removed checkout's leftover, so no row either, as a listing would decide. + expect(records).toEqual([]) expect(existsSync(join(worktreePath, 'unsaved.txt'))).toBe(true) expect(removeHostTree).not.toHaveBeenCalled() expect(purged).toEqual([]) @@ -270,9 +314,10 @@ describe('finishing an interrupted worktree removal after a restart', () => { await git(['add', '-A'], worktreePath) await git(['commit', '-qm', 'work'], worktreePath) - const { outcome, purged } = await finishAfterRestart({ head }) + const { outcome, purged, records } = await finishAfterRestart({ head, force: true }) expect(outcome).toMatchObject({ status: 'failed' }) + expect(records).toEqual([]) expect(existsSync(join(worktreePath, 'work.txt'))).toBe(true) expect(purged).toEqual([]) }) @@ -282,19 +327,23 @@ describe('finishing an interrupted worktree removal after a restart', () => { await git(['worktree', 'lock', worktreePath]) await unlink(join(worktreePath, '.git')) - const { outcome, purged } = await finishAfterRestart() + const { outcome, purged, records } = await finishAfterRestart({ force: true }) - expect(outcome).toMatchObject({ status: 'failed' }) + expect(outcome).toMatchObject({ status: 'failed', error: expect.stringMatching(/locked/) }) expect(existsSync(join(worktreePath, 'seed.txt'))).toBe(true) + // Not pruned either: the lock keeps Git's registration. + expect(await isRegistered(worktreePath)).toBe(true) expect(purged).toEqual([]) + expect(records).toEqual([]) }) it('deletes the branch when Git finished the checkout but the quit came before the branch', async () => { await git(['worktree', 'remove', worktreePath]) - const { outcome, purged } = await finishAfterRestart() + const { outcome, purged, records } = await finishAfterRestart() expect(outcome).toMatchObject({ status: 'removed' }) + expect(records).toEqual([]) expect(await git(['branch', '--list', 'feature'])).toBe('') expect(purged).toEqual([`repo-1::${worktreePath}`]) }) @@ -306,8 +355,9 @@ describe('finishing an interrupted worktree removal after a restart', () => { const head = (await git(['rev-parse', 'feature'])).trim() await git(['worktree', 'remove', worktreePath]) - const { outcome, remember } = await finishAfterRestart() + const { outcome, remember, records } = await finishAfterRestart() + expect(records).toEqual([]) expect(outcome).toMatchObject({ status: 'removed', preservedBranch: { branchName: 'feature', head } @@ -327,8 +377,9 @@ describe('finishing an interrupted worktree removal after a restart', () => { await git(['worktree', 'remove', worktreePath]) await git(['branch', '-d', 'feature']) - const { outcome, purged } = await finishAfterRestart({ head }) + const { outcome, purged, records } = await finishAfterRestart({ head }) + expect(records).toEqual([]) expect(outcome).toMatchObject({ status: 'removed' }) expect( outcome && 'preservedBranch' in outcome ? outcome.preservedBranch : undefined @@ -341,19 +392,22 @@ describe('finishing an interrupted worktree removal after a restart', () => { // Another Git client locked it while Orca was not running. await git(['worktree', 'lock', worktreePath]) - const { outcome, purged } = await finishAfterRestart() + const { outcome, purged, records } = await finishAfterRestart({ force: true }) expect(outcome).toMatchObject({ status: 'failed' }) expect(existsSync(worktreePath)).toBe(true) expect(await isRegistered(worktreePath)).toBe(true) expect(purged).toEqual([]) + expect(records).toEqual([]) + expect(await listedRows()).toEqual([{ path: worktreePath }]) }) it('drops a record whose repo Orca no longer has', async () => { vi.spyOn(console, 'warn').mockImplementation(() => {}) - const { outcome, purged } = await finishAfterRestart({ repoGone: true }) + const { outcome, purged, records } = await finishAfterRestart({ repoGone: true }) + expect(records).toEqual([]) expect(outcome).toMatchObject({ status: 'removed' }) expect(purged).toEqual([]) expect(existsSync(worktreePath)).toBe(true) diff --git a/src/main/runtime/runtime-interrupted-local-worktree-removal.ts b/src/main/runtime/runtime-interrupted-local-worktree-removal.ts index fb71fc499e1c..ba7d7a6f34b9 100644 --- a/src/main/runtime/runtime-interrupted-local-worktree-removal.ts +++ b/src/main/runtime/runtime-interrupted-local-worktree-removal.ts @@ -1,8 +1,5 @@ -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 { assertWorktreeUnlockedForRemoval } from '../../shared/worktree/removal' import { LOCAL_EXECUTION_HOST_ID } from '../../shared/execution-host' import type { RuntimeStore } from './runtime-store-contract' import { @@ -12,18 +9,23 @@ import { 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 { restoreMissingWorktreeGitFile } from '../git/worktree-git-file-restore' import { areWorktreePathsEqual } from '../git/worktree-path-comparison' import { getLocalProjectWorktreeGitOptions } from '../project-runtime-git-options' import { findRegisteredDeletableWorktree } from '../worktree-removal-safety' import { - assertUnregisteredRemovalLeftover, - differentCheckoutAtPathError, - isUnregisteredRemovalLeftover + assertUnregisteredCheckoutGone, + differentCheckoutAtPathError } from '../worktree-removal-leftover' -import { CLIENT_REMOVAL_HOME } from '../worktree-removal-home-guard' +import { CLIENT_REMOVAL_HOME, getPathOps } from '../worktree-removal-home-guard' +import { removeStaleLocalWorktreeRegistration } from '../local-worktree-removal-recovery' +import { + getLocalWorktreePathAccess, + toLocalWorktreeRuntimePath, + type LocalWorktreeFilesystemOptions +} from '../local-worktree-filesystem' import type { WorktreeRemovalRecord } from '../worktree-removal-records' import { cleanupRemovedWorktreePushTarget, @@ -36,7 +38,7 @@ type InterruptedWorktreeRemovalHost = { acquireWatcherRemoval: (path: string) => Promise<{ finish: (removed: boolean) => Promise }> closeWatchers: (path: string) => Promise preservedBranchCleanup: Pick - /** A retry's teardown: terminals may have opened in the leftover since the failed delete. */ + /** A retry's teardown: terminals opened in the folder before the user removed it. */ stopPtys?: () => Promise /** Drops the worktree's host state (metadata, history, caches), as every removal path does. */ purge: (record: WorktreeRemovalRecord) => void @@ -49,14 +51,16 @@ export function interruptedLocalWorktreeRemovalJob( record: WorktreeRemovalRecord, host: InterruptedWorktreeRemovalHost ): BackgroundWorktreeRemovalJob { + const resolveRemovedPushTarget = () => + resolveWorktreeRemovalMetadata( + host.store, + record.repoId, + record.worktreeId, + LOCAL_EXECUTION_HOST_ID + )?.pushTarget return { run: async (stopSignal) => { - const removedPushTarget = resolveWorktreeRemovalMetadata( - host.store, - record.repoId, - record.worktreeId, - LOCAL_EXECUTION_HOST_ID - )?.pushTarget + const removedPushTarget = resolveRemovedPushTarget() const result = await finishInterruptedLocalWorktreeRemoval({ record, store: host.store, @@ -86,7 +90,19 @@ export function interruptedLocalWorktreeRemovalJob( host.onRemoved(record) return result }, - publish: () => host.publish(record.repoId) + publish: () => host.publish(record.repoId), + cleanupPushTargetRemote: async () => { + const repo = host.store.getRepo(record.repoId) + if (repo) { + await cleanupUnusedWorktreePushTargetRemote( + repo.path, + record.worktreeId, + resolveRemovedPushTarget(), + host.store, + getLocalProjectWorktreeGitOptions(host.store, repo) + ) + } + } } } @@ -102,8 +118,10 @@ type InterruptedLocalWorktreeRemovalArgs = Pick< } /** - * Finishes a removal a quit or crash interrupted. What is left comes from Git and disk, not the - * record, so a delete Git already finished (fully or partly) completes the same way. + * Finishes a removal a quit or crash interrupted, or a failed one whose folder the user removed. + * What is left comes from Git and disk, not the record: a checkout Git registers is deleted by Git + * with the recorded choices, or only unregistered when its `.git` is gone; a folder Git no longer + * registers is refused, never deleted; with the folder gone, the rest of the delete finishes. */ async function finishInterruptedLocalWorktreeRemoval( args: InterruptedLocalWorktreeRemovalArgs @@ -123,8 +141,7 @@ async function finishInterruptedLocalWorktreeRemoval( finishRemoval: args.finishRemoval, repo, localOptions, - // Why force: Git already deleted part of the checkout, which reads as local changes. - force: true, + force: record.force, deleteBranch: record.deleteBranch, target: { id: record.worktreeId } } @@ -132,7 +149,7 @@ async function finishInterruptedLocalWorktreeRemoval( const registered = worktrees.some((worktree) => areWorktreePathsEqual(worktree.path, record.worktreePath) ) - const deletable = registered + let deletable = registered ? findRegisteredDeletableWorktree( repo.path, record.worktreePath, @@ -145,23 +162,24 @@ async function finishInterruptedLocalWorktreeRemoval( `Worktree registration changed during deletion: ${record.worktreePath}. Retry deletion.` ) } - const gitLink = await readCheckoutGitLink(record.worktreePath) - // Why: the finish forces, so a checkout created at this path since the quit must not be taken. - // At an unregistered path, only a `.git` naming the admin entry Git removed is this checkout's - // own leftover (Git drops the registration even when its delete fails partway). - if ( - deletable - ? !isRecordedCheckout(deletable, record) - : !(await isUnregisteredRemovalLeftover(repo.path, record.worktreePath)) - ) { + if (deletable && !isRecordedCheckout(deletable, record)) { throw differentCheckoutAtPathError(record.worktreePath) } - // Why: Git deletes `.git` wherever it falls in directory order (early on NTFS) and refuses to - // remove a checkout left without it; restoring the link from Git's admin entry lets Git finish. - let gitCanRemove = !!deletable - if (deletable && gitLink === 'missing') { - assertWorktreeUnlockedForRemoval(deletable) - gitCanRemove = await restoreMissingWorktreeGitFile(repo.path, deletable.path, localOptions) + if (deletable && (await isCheckoutMissingGitLink(deletable.path, localOptions))) { + // Why: Git can neither remove nor validate a checkout whose `.git` it deleted first (Windows + // order); prune drops only the registration, keeping every file, so the checks below apply. + await removeStaleLocalWorktreeRegistration({ + canonicalWorktreePath: deletable.path, + repoPath: repo.path, + localWorktreeGitOptions: localOptions, + registeredWorktree: deletable, + deleteBranch: false + }) + deletable = undefined + } + // Before the teardown, so a refused folder keeps its terminals and watchers. + if (!deletable) { + await assertUnregisteredCheckoutGone(record.worktreePath, worktrees) } const gate = await args.acquireWatcherRemoval(record.worktreePath) if (args.stopPtys) { @@ -172,11 +190,9 @@ async function finishInterruptedLocalWorktreeRemoval( throw error } } - if (deletable && gitCanRemove) { + if (deletable) { return finishRuntimeLocalWorktreeRemoval(finishArgs, deletable, gate, args.stopSignal) } - // Unregistered, or no admin entry claims the checkout so Git cannot validate it: the leftover is - // deleted in this process as a last resort, then pruned. let result: RemoveWorktreeResult let removed = false try { @@ -184,10 +200,8 @@ async function finishInterruptedLocalWorktreeRemoval( repo.path, record.worktreePath, record.deleteBranch && record.branch ? { name: record.branch, head: record.head } : null, - // Why only unregistered: a registered checkout here was just proven to be the recorded one. - deletable - ? async () => {} - : () => assertUnregisteredRemovalLeftover(repo.path, record.worktreePath, localOptions), + // Again after the teardown's waits: a folder may have appeared at the path meanwhile. + () => assertUnregisteredCheckoutGone(record.worktreePath), localOptions ) removed = true @@ -199,26 +213,30 @@ async function finishInterruptedLocalWorktreeRemoval( return result } -function isRecordedCheckout(worktree: GitWorktreeInfo, record: WorktreeRemovalRecord): boolean { - return ( - normalizeLocalBranchRef(worktree.branch) === record.branch && - (!record.head || worktree.head === record.head) - ) -} - -/** `missing`: the checkout directory is there without its `.git`; unreadable counts as present. */ -async function readCheckoutGitLink( - worktreePath: string -): Promise<'no-checkout' | 'missing' | 'present'> { +/** A checkout folder still on disk whose `.git` link is gone. */ +async function isCheckoutMissingGitLink( + checkoutPath: string, + options: LocalWorktreeFilesystemOptions +): Promise { + const { statPath } = getLocalWorktreePathAccess(options) + const stat = (path: string) => statPath(toLocalWorktreeRuntimePath(path, options)) try { - await lstat(worktreePath) + await stat(checkoutPath) } catch { - return 'no-checkout' + // Git removes a registration whose folder is gone; other errors are Git's to report. + return false } try { - await lstat(join(worktreePath, '.git')) - return 'present' + await stat(getPathOps(checkoutPath).join(checkoutPath, '.git')) + return false } catch (error) { - return getErrorCode(error) === 'ENOENT' ? 'missing' : 'present' + 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 a5e966b1471a..7fc172543fd9 100644 --- a/src/main/runtime/runtime-registered-local-worktree-removal.ts +++ b/src/main/runtime/runtime-registered-local-worktree-removal.ts @@ -182,7 +182,8 @@ async function acceptRuntimeRegisteredLocalWorktreeRemoval(args: { args.onRemoved() return result }, - publish: () => args.publish() + publish: () => args.publish(), + cleanupPushTargetRemote: () => cleanupRemovedWorktreePushTarget(args) }) return { removing: true, ...acceptedFields } } diff --git a/src/main/runtime/runtime-unregistered-worktree-removal.ts b/src/main/runtime/runtime-unregistered-worktree-removal.ts index 36433c45a852..e33c0ebdcb29 100644 --- a/src/main/runtime/runtime-unregistered-worktree-removal.ts +++ b/src/main/runtime/runtime-unregistered-worktree-removal.ts @@ -14,7 +14,6 @@ import { import { cleanupUnusedWorktreePushTargetRemote } from '../ipc/worktree-remote' import { assertWorktreeDoesNotContainRegisteredWorktree, - canCleanupUnregisteredOrcaLeftoverDirectory, canCleanupUnregisteredOrcaWorktreeDirectory, canSafelyRemoveOrphanedWorktreeDirectory, isDangerousWorktreeRemovalPath, @@ -23,10 +22,7 @@ import { } from '../worktree-removal-safety' import type { RuntimeStore } from './runtime-store-contract' import type { RuntimeWorktreeRemovalTarget } from './runtime-worktree-selection' -import { - isLocalRuntimeGitRepository, - isLocalRuntimeWorktreePathMissing -} from './runtime-worktree-filesystem' +import { isLocalRuntimeWorktreePathMissing } from './runtime-worktree-filesystem' type RemovalGate = { finish: (removed: boolean) => Promise } @@ -70,28 +66,6 @@ export async function removeRuntimeUnregisteredWorktree(args: { args.finishRemoval() return {} } - const access = getLocalWorktreePathAccess(args.localOptions) - const runtimeWorktreePath = toLocalWorktreeRuntimePath(target.path, args.localOptions) - if ( - await canCleanupUnregisteredOrcaLeftoverDirectory({ - meta: removedMeta, - worktreePath: target.path, - runtimeWorktreePath, - repo, - runtimeRepoPath: toLocalWorktreeRuntimePath(repo.path, args.localOptions), - registeredWorktrees, - statPath: access.statPath, - home: removalHome, - isGitRepository: (path) => isLocalRuntimeGitRepository(path, args.localOptions) - }) - ) { - if (!args.force) { - throw new Error(ORPHANED_WORKTREE_DIRECTORY_MESSAGE) - } - await deleteUnregisteredDirectory(args) - args.finishRemoval() - return {} - } if (await isLocalRuntimeWorktreePathMissing(target.path, args.localOptions)) { if (!args.force && !removedMeta) { throw new Error(UNREGISTERED_MISSING_WORKTREE_MESSAGE) diff --git a/src/main/runtime/runtime-worktree-filesystem.ts b/src/main/runtime/runtime-worktree-filesystem.ts index 923917d1150b..59955e0dfe80 100644 --- a/src/main/runtime/runtime-worktree-filesystem.ts +++ b/src/main/runtime/runtime-worktree-filesystem.ts @@ -3,7 +3,6 @@ import { stat } from 'node:fs/promises' import { FOLDER_WORKSPACE_INSTANCE_SEPARATOR } from '../../shared/worktree/id' import type { Repo } from '../../shared/repo-types' import type { Worktree } from '../../shared/worktree/types' -import { gitExecFileAsync } from '../git/runner' import { isENOENT } from '../ipc/filesystem-auth' import { getLocalWorktreePathAccess, @@ -19,7 +18,6 @@ import { mergeRuntimeFolderWorkspace } from './runtime-folder-workspace' import type { RuntimeStore } from './runtime-store-contract' -import { gitStatusErrorMeansNotRepository } from './runtime-worktree-selection' /** Callers hold a local removal route; an `ssh:` or `runtime:` host is refused before this. */ export async function isLocalRuntimeWorktreePathMissing( @@ -33,21 +31,6 @@ export async function isLocalRuntimeWorktreePathMissing( ) } -export async function isLocalRuntimeGitRepository( - runtimeWorktreePath: string, - localWorktreeGitOptions: { wslDistro?: string } = {} -): Promise { - try { - await gitExecFileAsync(['status', '--short'], { - cwd: runtimeWorktreePath, - ...localWorktreeGitOptions - }) - return true - } catch (error) { - return !gitStatusErrorMeansNotRepository(error) - } -} - function getRuntimeFolderWorkspaceInstanceIdentity(repo: Repo, worktreeId: string): string { const prefix = `${getRuntimeFolderWorkspaceRootId(repo)}${FOLDER_WORKSPACE_INSTANCE_SEPARATOR}` return worktreeId.startsWith(prefix) ? worktreeId.slice(prefix.length) : randomUUID() diff --git a/src/main/runtime/runtime-worktree-selection.ts b/src/main/runtime/runtime-worktree-selection.ts index 2a00d01ced15..ca214f4d2f62 100644 --- a/src/main/runtime/runtime-worktree-selection.ts +++ b/src/main/runtime/runtime-worktree-selection.ts @@ -10,22 +10,6 @@ export type RuntimeWorktreeRemovalTarget = { pushTarget?: GitPushTarget } -export function gitStatusErrorMeansNotRepository(error: unknown): boolean { - const message = - error instanceof Error - ? error.message - : error && typeof error === 'object' && 'message' in error - ? String((error as { message: unknown }).message) - : typeof error === 'string' - ? error - : '' - const stderr = - error && typeof error === 'object' && 'stderr' in error - ? String((error as { stderr: unknown }).stderr) - : '' - return /not a git repository/i.test(`${message}\n${stderr}`) -} - /** * Options for `removeManagedWorktree`. Named rather than positional on purpose: three of the * four are interchangeable booleans that each waive a different safety check on a destructive diff --git a/src/main/runtime/workspaces/workspace-removal-target.ts b/src/main/runtime/workspaces/workspace-removal-target.ts index 01e88eb4db83..6983f10a0ada 100644 --- a/src/main/runtime/workspaces/workspace-removal-target.ts +++ b/src/main/runtime/workspaces/workspace-removal-target.ts @@ -32,7 +32,7 @@ import { waitForPendingWorktreeRemoval } from '../../worktree-background-removal' import { interruptedLocalWorktreeRemovalJob } from '../runtime-interrupted-local-worktree-removal' -import { retryFailedRemovalUnlessRegistered } from '../../worktree-removal-table' +import { retryFailedRemovalUnlessRegistered } from '../../worktree-removal-leftover' import type { GitWorktreeInfo } from '../../../shared/worktree/types' import { resolveAgentTerminalCreateOptions, @@ -81,15 +81,16 @@ export abstract class OrcaRuntimeWithResolveWorktreeRemovalTarget extends OrcaRu } /** - * Delete on the leftover of a local delete that failed after Git dropped the registration, while - * Git's listing still does not register the path: runs that removal again. True when it did. + * Delete on a local delete that failed after Git dropped the registration, while Git's listing + * still does not register the path: refuses while the folder is there, else finishes that removal. + * True when it did. */ - protected retryFailedLocalRemoval( + protected async retryFailedRemoval( route: { kind: string }, target: { id: string; path: string }, registeredWorktrees: readonly GitWorktreeInfo[], options: RemoveManagedWorktreeOptions - ): boolean { + ): Promise { const store = this.store if (route.kind !== 'local' || !store) { return false diff --git a/src/main/runtime/workspaces/workspace-removal.ts b/src/main/runtime/workspaces/workspace-removal.ts index b5f779511e49..a710800b65e5 100644 --- a/src/main/runtime/workspaces/workspace-removal.ts +++ b/src/main/runtime/workspaces/workspace-removal.ts @@ -61,12 +61,12 @@ export abstract class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWi repoId: string, removalHostId?: ExecutionHostId ): void - protected abstract retryFailedLocalRemoval( + protected abstract retryFailedRemoval( route: { kind: string }, target: { id: string; path: string }, registeredWorktrees: readonly GitWorktreeInfo[], options: RemoveManagedWorktreeOptions - ): boolean + ): Promise async removeManagedWorktree( worktreeSelector: string, @@ -167,7 +167,7 @@ export abstract class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWi registeredWorktrees, removalHome ) - if (this.retryFailedLocalRemoval(route, removalTarget, registeredWorktrees, options)) { + if (await this.retryFailedRemoval(route, removalTarget, registeredWorktrees, options)) { return { removing: true } } if (!registeredWorktree) { diff --git a/src/main/worktree-background-removal-records.test.ts b/src/main/worktree-background-removal-records.test.ts index 726240fefb1f..fc4c0ddaf415 100644 --- a/src/main/worktree-background-removal-records.test.ts +++ b/src/main/worktree-background-removal-records.test.ts @@ -256,4 +256,29 @@ describe('durable worktree removal records', () => { await writeFile(worktreeRemovalRecordsFile(directory), '{not json') expect(await readWorktreeRemovalRecords(directory)).toEqual([]) }) + + it('reads a record that carries a field this version does not know, ignoring the field', async () => { + const record = { + worktreeId: 'repo-1::/work/a', + repoId: 'repo-1', + repoPath: '/work/repo', + worktreePath: '/work/a', + branch: 'feature', + head: 'abc', + deleteBranch: true, + force: false, + requestedAt: 5, + failure: { message: 'Operation not permitted', failedAt: 6 } + } + // Daily builds before this one may have written a checkout identity with the record. + await writeFile( + worktreeRemovalRecordsFile(directory), + JSON.stringify({ + version: 1, + removals: [{ ...record, checkoutIdentity: { dev: 1, ino: 2, birthtimeMs: 3 } }] + }) + ) + + expect(await readWorktreeRemovalRecords(directory)).toEqual([record]) + }) }) diff --git a/src/main/worktree-background-removal.ts b/src/main/worktree-background-removal.ts index 700ecd045cae..487f4a671983 100644 --- a/src/main/worktree-background-removal.ts +++ b/src/main/worktree-background-removal.ts @@ -6,11 +6,8 @@ import { acquireWatcherRemovalGate, type WatcherRemovalGate } from './ipc/watche import { runWorktreeChangeInvalidators } from './ipc/worktree-change-invalidators' import { parseWslPath } from './wsl' import { readWorktreeRemovalRecords, type WorktreeRemovalRecord } from './worktree-removal-records' -import { - differentCheckoutAtPathError, - isCheckoutRegistered, - isUnregisteredRemovalLeftover -} from './worktree-removal-leftover' +import { failureLeftByGit } from './worktree-removal-failure' +import { differentCheckoutAtPathError, isCheckoutRegistered } from './worktree-removal-leftover' import { failedWorktreeRemovals, finishedWorktreeRemovals, @@ -25,6 +22,8 @@ export type BackgroundWorktreeRemovalJob = { run: (stopSignal: AbortSignal) => Promise /** Fires when the row starts showing as removing, and again after it has left the table. */ publish: () => void + /** The push-target remote cleanup a finished delete runs; also run when Git fails partway. */ + cleanupPushTargetRemote?: () => Promise } type RemovalSettlement = { @@ -160,9 +159,9 @@ export function startBackgroundWorktreeRemoval( } /** - * Delete on a failed delete's leftover that Git's current listing still does not register: runs the - * recorded removal again, with the choices the user made the first time, or joins the one another - * request started while this one listed Git. Undefined when neither. + * Delete on a failed delete whose folder is gone and that Git's current listing still does not + * register: finishes the recorded removal with the choices the user made the first time, or joins + * the one another request started while this one listed Git. Undefined when neither. */ export function retryFailedWorktreeRemoval( worktreeId: string, @@ -261,12 +260,7 @@ async function settleBackgroundWorktreeRemoval( settle = (settlement) => settlement.reject(error) // Why: Git drops the registration even when it fails to delete the checkout, and Orca lists // workspaces from Git, so without the record the leftover would vanish with no way to retry. - if (await isCheckoutLeftUnregistered(record)) { - failure = { - message: error instanceof Error ? error.message : String(error), - failedAt: Date.now() - } - } + failure = await failureLeftByGit(record, error, job.cleanupPushTargetRemote) } finally { // A resumed job that ended before taking its own gate still holds the fence loading gave it. releaseStartupRemovalFence(record.worktreeId) @@ -296,22 +290,6 @@ async function settleBackgroundWorktreeRemoval( } } -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) { - // Unknowable: the row stays however Git lists it, as before this record existed. - console.warn(`[worktrees] could not list worktrees of ${record.repoPath}`, error) - return false - } -} - // Why: the record should be on disk before Git deletes, so a quit resumes the delete, but a disk or // file pool stall must not hold the user's delete; past this, a crash only loses the resume. const RECORD_WRITE_WAIT_MS = 2_000 diff --git a/src/main/worktree-failed-removal.test.ts b/src/main/worktree-failed-removal.test.ts index f626e7bd3272..bf88a8b0566a 100644 --- a/src/main/worktree-failed-removal.test.ts +++ b/src/main/worktree-failed-removal.test.ts @@ -1,5 +1,6 @@ // A delete that fails after Git dropped the checkout's registration: the leftover stays listed with -// the error until Delete retries it, the checkout disappears, or its repo leaves Orca. Git is mocked +// the error until the checkout disappears, Git registers a checkout there again, or its repo leaves +// Orca; Delete refuses while the folder is on disk. Git is mocked // here so this runs on every platform; the real-Git version is in // runtime/runtime-failed-local-worktree-removal.test.ts. import { mkdir, mkdtemp, readdir, realpath, rm, writeFile } from 'node:fs/promises' @@ -8,6 +9,8 @@ import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { GitWorktreeInfo } from '../shared/worktree/types' import { listWorktreesStrict } from './git/worktree' +import type * as WorktreeRemovalModule from './git/worktree-removal' +import { deleteBranchOfUnregisteredWorktree } from './git/worktree-removal' import { beginTerminalInstall } from './ipc/watcher-removal-gate' import { registerWorktreeChangeInvalidator } from './ipc/worktree-change-invalidators' import { @@ -17,17 +20,23 @@ import { resumeInterruptedWorktreeRemovals, retryFailedWorktreeRemoval, startBackgroundWorktreeRemoval, - waitForPendingWorktreeRemoval + waitForPendingWorktreeRemoval, + type BackgroundWorktreeRemovalJob } from './worktree-background-removal' import { projectPendingWorktreeRemovals, 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' vi.mock('./git/worktree', () => ({ listWorktreesStrict: vi.fn(async () => []) })) +vi.mock('./git/worktree-removal', async (importOriginal) => ({ + ...(await importOriginal()), + deleteBranchOfUnregisteredWorktree: vi.fn(async () => ({})) +})) const GIT_ERROR = "error: failed to delete 'node_modules/a/LICENSE': Operation not permitted" let directory = '' @@ -56,11 +65,14 @@ beforeEach(async () => { afterEach(async () => { _resetPendingWorktreeRemovalsForTests() + vi.mocked(deleteBranchOfUnregisteredWorktree).mockReset() vi.restoreAllMocks() await rm(directory, { recursive: true, force: true }) }) -function startFailingRemoval(): Promise { +function startFailingRemoval( + job: Pick = {} +): Promise { return startBackgroundWorktreeRemoval({ removal: { worktreeId, @@ -73,7 +85,8 @@ function startFailingRemoval(): Promise { run: async () => { throw new Error(GIT_ERROR) }, - publish: () => {} + publish: () => {}, + ...job }) } @@ -131,9 +144,43 @@ describe('a delete that fails after Git dropped the registration', () => { mainWorktree, { ...leftoverRow(), removalError: undefined } ]) - await failRemoval() + const cleanupPushTargetRemote = vi.fn(async () => {}) + await expect(startFailingRemoval({ cleanupPushTargetRemote })).rejects.toThrow(GIT_ERROR) + await _settlePendingWorktreeRemovalsForTests() expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([]) + // Git still has the checkout, so its branch and remote stay with it. + 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 () => {}) + await expect(startFailingRemoval({ cleanupPushTargetRemote })).rejects.toThrow(GIT_ERROR) + await _settlePendingWorktreeRemovalsForTests() + + expect(deleteBranchOfUnregisteredWorktree).toHaveBeenCalledWith('/work/repo', checkout, { + name: 'feature', + head: 'abc' + }) + expect(cleanupPushTargetRemote).toHaveBeenCalledTimes(1) + expect(await readdir(checkout)).toEqual(['node_modules']) + expect(await listRows()).toEqual([mainWorktree, leftoverRow()]) + }) + + it('keeps the failed row and its error when that bookkeeping fails', async () => { + vi.mocked(deleteBranchOfUnregisteredWorktree).mockRejectedValue(new Error('branch locked')) + const cleanupPushTargetRemote = vi.fn(async () => { + throw new Error('remote busy') + }) + await expect(startFailingRemoval({ cleanupPushTargetRemote })).rejects.toThrow(GIT_ERROR) + await _settlePendingWorktreeRemovalsForTests() + + expect(cleanupPushTargetRemote).toHaveBeenCalledTimes(1) + expect(await listRows()).toEqual([mainWorktree, leftoverRow()]) + expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toMatchObject([ + { worktreeId, failure: { message: GIT_ERROR } } + ]) }) it('clears the record as before when the checkout is gone', async () => { @@ -290,4 +337,47 @@ 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-removal-failure.ts b/src/main/worktree-removal-failure.ts new file mode 100644 index 000000000000..905d1139ca87 --- /dev/null +++ b/src/main/worktree-removal-failure.ts @@ -0,0 +1,68 @@ +import { deleteBranchOfUnregisteredWorktree } from './git/worktree-removal' +import { isCheckoutRegistered, isUnregisteredRemovalLeftover } from './worktree-removal-leftover' +import type { WorktreeRemovalFailure, WorktreeRemovalRecord } from './worktree-removal-records' +import { worktreeCheckoutExists } from './worktree-removal-table' + +/** + * The failure to keep for a delete that failed after Git dropped the registration with the + * checkout still on disk, once the rest of the delete's Git side ran; undefined otherwise. + */ +export async function failureLeftByGit( + record: WorktreeRemovalRecord, + error: unknown, + cleanupPushTargetRemote: (() => Promise) | undefined +): Promise { + if (!(await isCheckoutLeftUnregistered(record))) { + return undefined + } + const failure = { + message: error instanceof Error ? error.message : String(error), + failedAt: Date.now() + } + await finishGitSideOfFailedRemoval(record, cleanupPushTargetRemote) + return failure +} + +/** + * What the delete's finish would have done once Git let go of the checkout, run in the same + * operation: the branch per the recorded choice and the push-target remote. Deletes no files, so + * the failed record owes only the folder, which the user removes. + */ +async function finishGitSideOfFailedRemoval( + record: WorktreeRemovalRecord, + cleanupPushTargetRemote: (() => Promise) | undefined +): Promise { + // Why caught: bookkeeping; the failed row and the request's error stand either way. + try { + // No prune: Git already dropped this entry; a repo-wide prune would unregister any other + // worktree whose folder is missing for now (an unmounted drive). + await deleteBranchOfUnregisteredWorktree( + record.repoPath, + record.worktreePath, + record.deleteBranch && record.branch ? { name: record.branch, head: record.head } : null + ) + } catch (error) { + console.warn(`[worktrees] could not delete the branch of ${record.worktreePath}`, error) + } + try { + await cleanupPushTargetRemote?.() + } catch (error) { + 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) { + // Unknowable: the row stays however Git lists it, as before this record existed. + 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 3e17e415ee30..0582368becf5 100644 --- a/src/main/worktree-removal-leftover.ts +++ b/src/main/worktree-removal-leftover.ts @@ -1,14 +1,20 @@ 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 { areWorktreePathsEqual } from './git/worktree-path-comparison' import { CLIENT_REMOVAL_HOME } from './worktree-removal-home-guard' +import { + failedWorktreeRemovals, + persistWorktreeRemovalRecords, + worktreeCheckoutExists +} from './worktree-removal-table' import { assertWorktreeDoesNotContainRegisteredWorktree, canSafelyRemoveOrphanedWorktreeDirectory } from './worktree-removal-safety' -import type { GitWorktreeExecOptions } from './git/worktree-operation-options' /** * Whether a checkout path Git no longer registers still holds the removed checkout's own leftover: @@ -45,20 +51,51 @@ export async function isCheckoutRegistered(record: { } /** - * Refuses unless the path still holds the removed checkout's own leftover, with no worktree Git - * registers at or inside it. Run right before the delete: the path can change while it waits. + * The refusal for a folder Git no longer registers: Orca deletes a checkout only through Git, so + * the user removes the folder, and the listing drops the workspace once it is gone. */ -export async function assertUnregisteredRemovalLeftover( - repoPath: string, +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.` + ) +} + +/** + * Refuses while anything is at a checkout path Git no longer registers, naming first any worktree + * in `registeredWorktrees` inside it. + */ +export async function assertUnregisteredCheckoutGone( worktreePath: string, - options: GitWorktreeExecOptions = {} + registeredWorktrees: readonly Pick[] = [] ): Promise { - const worktrees = await listWorktreesStrict(repoPath, options) - if (worktrees.some((worktree) => areWorktreePathsEqual(worktree.path, worktreePath))) { - throw differentCheckoutAtPathError(worktreePath) + if (await worktreeCheckoutExists(worktreePath)) { + // Why first: removing the folder, as the refusal asks, would delete that worktree's files too. + assertWorktreeDoesNotContainRegisteredWorktree(worktreePath, registeredWorktrees) + throw unregisteredFolderRefusal(worktreePath) + } +} + +/** + * Delete's choice for a workspace whose earlier delete failed, from Git's listing taken now: a + * checkout Git registers at the path again is the normal delete's, so the failed record is dropped; + * a folder Git does not register is refused, the record kept; with the folder gone, `retry` runs + * or joins the recorded removal to finish the rest. True then. + */ +export async function retryFailedRemovalUnlessRegistered( + worktreeId: string, + worktreePath: string, + registeredWorktrees: readonly Pick[], + retry: () => Promise | undefined +): Promise { + if (registeredWorktrees.some((worktree) => areWorktreePathsEqual(worktree.path, worktreePath))) { + if (failedWorktreeRemovals.delete(worktreeId)) { + void persistWorktreeRemovalRecords() + } + return false } - assertWorktreeDoesNotContainRegisteredWorktree(worktreePath, worktrees) - if (!(await isUnregisteredRemovalLeftover(repoPath, worktreePath))) { - throw differentCheckoutAtPathError(worktreePath) + const failed = failedWorktreeRemovals.get(worktreeId) + if (failed) { + await assertUnregisteredCheckoutGone(failed.worktreePath, registeredWorktrees) } + return retry() !== undefined } diff --git a/src/main/worktree-removal-records.ts b/src/main/worktree-removal-records.ts index b4e7dbdb7fa7..f5df4579a11a 100644 --- a/src/main/worktree-removal-records.ts +++ b/src/main/worktree-removal-records.ts @@ -24,7 +24,10 @@ export type WorktreeRemovalRecord = { deleteBranch: boolean force: boolean requestedAt: number - /** The delete failed after Git dropped the registration with the checkout still on disk. */ + /** + * The delete failed after Git dropped the registration with the checkout still on disk. Its Git + * side (branch, push-target remote) ran when it failed; the record owes only the folder. + */ failure?: WorktreeRemovalFailure } diff --git a/src/main/worktree-removal-safety.test.ts b/src/main/worktree-removal-safety.test.ts index 9aa9775de44d..e8da1d6d6da5 100644 --- a/src/main/worktree-removal-safety.test.ts +++ b/src/main/worktree-removal-safety.test.ts @@ -1,8 +1,7 @@ import { describe, expect, it, vi } from 'vitest' -import { homedir } from 'node:os' + import type { GitWorktreeInfo } from '../shared/worktree/types' import { - canCleanupUnregisteredOrcaLeftoverDirectory, canSafelyRemoveOrphanedWorktreeDirectory, findRegisteredDeletableWorktree, getRegisteredDeletableWorktree, @@ -349,198 +348,6 @@ describe('canSafelyRemoveOrphanedWorktreeDirectory', () => { }) }) -describe('canCleanupUnregisteredOrcaLeftoverDirectory', () => { - const repo = { path: '/repos/main' } - const ownedMeta = { orcaCreatedAt: 1, orcaCreationSource: 'runtime' as const } - const baseArgs = { - meta: ownedMeta, - worktreePath: '/workspaces/orca-owned', - runtimeWorktreePath: '/workspaces/orca-owned', - repo, - runtimeRepoPath: repo.path, - registeredWorktrees: [makeGitWorktree(repo.path, true)], - home: CLIENT_REMOVAL_HOME - } - - it('rejects unregistered existing targets that are files or symlinks', async () => { - const isGitRepository = vi.fn().mockResolvedValue(false) - - await expect( - canCleanupUnregisteredOrcaLeftoverDirectory({ - ...baseArgs, - statPath: makeStatPath(['/workspaces/orca-owned']), - isGitRepository - }) - ).resolves.toBe(false) - await expect( - canCleanupUnregisteredOrcaLeftoverDirectory({ - ...baseArgs, - statPath: async (path) => { - if (path === '/workspaces/orca-owned') { - return { type: 'symlink' } - } - throw missingPath(path) - }, - isGitRepository - }) - ).resolves.toBe(false) - - expect(isGitRepository).not.toHaveBeenCalled() - }) - - it('rejects unregistered leftover directories with a .git marker', async () => { - const isGitRepository = vi.fn().mockResolvedValue(false) - - await expect( - canCleanupUnregisteredOrcaLeftoverDirectory({ - ...baseArgs, - statPath: makeStatPath(['/workspaces/orca-owned/.git'], ['/workspaces/orca-owned']), - isGitRepository - }) - ).resolves.toBe(false) - - expect(isGitRepository).not.toHaveBeenCalled() - }) - - it('rejects no-marker cleanup when only the Orca path shape matches', async () => { - const isGitRepository = vi.fn().mockResolvedValue(false) - - await expect( - canCleanupUnregisteredOrcaLeftoverDirectory({ - ...baseArgs, - meta: undefined, - statPath: makeStatPath([], ['/workspaces/orca-owned']), - isGitRepository - }) - ).resolves.toBe(false) - - expect(isGitRepository).not.toHaveBeenCalled() - }) - - it('checks dangerous paths in the original path space before runtime translation', async () => { - const homePath = homedir() - const runtimeHomePath = homePath - .replace(/^([A-Za-z]):/, (_match, drive: string) => `/mnt/${drive.toLowerCase()}`) - .replace(/\\/g, '/') - const isGitRepository = vi.fn().mockResolvedValue(false) - - await expect( - canCleanupUnregisteredOrcaLeftoverDirectory({ - ...baseArgs, - worktreePath: homePath, - runtimeWorktreePath: runtimeHomePath, - repo: { path: 'C:\\repos\\main' }, - runtimeRepoPath: '/mnt/c/repos/main', - registeredWorktrees: [makeGitWorktree('C:\\repos\\main', true)], - statPath: makeStatPath([], [runtimeHomePath]), - isGitRepository - }) - ).resolves.toBe(false) - - expect(isGitRepository).not.toHaveBeenCalled() - }) - - it('checks dangerous POSIX runtime home paths before no-marker cleanup', async () => { - const isGitRepository = vi.fn().mockResolvedValue(false) - - await expect( - canCleanupUnregisteredOrcaLeftoverDirectory({ - ...baseArgs, - worktreePath: '/home/dev', - runtimeWorktreePath: '/home/dev', - repo: { path: '/repos/main' }, - runtimeRepoPath: '/repos/main', - registeredWorktrees: [makeGitWorktree('/repos/main', true)], - statPath: makeStatPath([], ['/home/dev']), - isGitRepository - }) - ).resolves.toBe(false) - - expect(isGitRepository).not.toHaveBeenCalled() - }) - - it('rejects unregistered leftover directories that still answer git status', async () => { - const isGitRepository = vi.fn().mockResolvedValue(true) - - await expect( - canCleanupUnregisteredOrcaLeftoverDirectory({ - ...baseArgs, - statPath: makeStatPath([], ['/workspaces/orca-owned']), - isGitRepository - }) - ).resolves.toBe(false) - - expect(isGitRepository).toHaveBeenCalledWith('/workspaces/orca-owned') - }) - - it('rejects unregistered leftover directories that contain a registered child worktree', async () => { - await expect( - canCleanupUnregisteredOrcaLeftoverDirectory({ - ...baseArgs, - registeredWorktrees: [ - makeGitWorktree(repo.path, true), - makeGitWorktree('/workspaces/orca-owned/child') - ], - statPath: makeStatPath([], ['/workspaces/orca-owned']), - isGitRepository: vi.fn().mockResolvedValue(false) - }) - ).rejects.toThrow( - 'Refusing to delete worktree because it contains another registered worktree: /workspaces/orca-owned/child' - ) - }) - - it('uses runtime paths for filesystem proof and original paths for nested worktree checks', async () => { - const statPath = vi.fn(async (path: string) => { - if (path === '/mnt/c/workspaces/orca-owned') { - return { type: 'directory' } - } - throw missingPath(path) - }) - const isGitRepository = vi.fn().mockResolvedValue(false) - - await expect( - canCleanupUnregisteredOrcaLeftoverDirectory({ - ...baseArgs, - worktreePath: 'C:\\workspaces\\orca-owned', - runtimeWorktreePath: '/mnt/c/workspaces/orca-owned', - repo: { path: 'C:\\repos\\main' }, - runtimeRepoPath: '/mnt/c/repos/main', - registeredWorktrees: [ - makeGitWorktree('C:\\repos\\main', true), - makeGitWorktree('C:\\workspaces\\orca-owned-sibling') - ], - statPath, - isGitRepository - }) - ).resolves.toBe(true) - - expect(statPath).toHaveBeenCalledWith('/mnt/c/workspaces/orca-owned') - expect(statPath).toHaveBeenCalledWith('/mnt/c/workspaces/orca-owned/.git') - expect(statPath).not.toHaveBeenCalledWith('C:\\workspaces\\orca-owned') - expect(isGitRepository).toHaveBeenCalledWith('/mnt/c/workspaces/orca-owned') - }) - - it('rejects translated-runtime cleanup when original path contains a registered child', async () => { - await expect( - canCleanupUnregisteredOrcaLeftoverDirectory({ - ...baseArgs, - worktreePath: 'C:\\workspaces\\orca-owned', - runtimeWorktreePath: '/mnt/c/workspaces/orca-owned', - repo: { path: 'C:\\repos\\main' }, - runtimeRepoPath: '/mnt/c/repos/main', - registeredWorktrees: [ - makeGitWorktree('C:\\repos\\main', true), - makeGitWorktree('C:\\workspaces\\orca-owned\\child') - ], - statPath: makeStatPath([], ['/mnt/c/workspaces/orca-owned']), - isGitRepository: vi.fn().mockResolvedValue(false) - }) - ).rejects.toThrow( - 'Refusing to delete worktree because it contains another registered worktree: C:\\workspaces\\orca-owned\\child' - ) - }) -}) - describe('isDangerousWorktreeRemovalPath on an execution host', () => { // #18275: these verdicts are about the machine that runs the delete. The // client home is `homedir()` here — a macOS/Linux path that recognises none @@ -722,30 +529,4 @@ describe('canSafelyRemoveOrphanedWorktreeDirectory on an execution host', () => ) ).resolves.toBe(true) }) - - it('refuses the leftover-directory cleanup while the host home is unanswered', async () => { - const leftoverArgs = { - meta: { orcaCreatedAt: 1, orcaCreationSource: 'ssh' as const }, - worktreePath: '/srv/homes/alice', - runtimeWorktreePath: '/srv/homes/alice', - repo: { path: '/opt/src/repo' }, - runtimeRepoPath: '/opt/src/repo', - registeredWorktrees: [], - statPath: makeStatPath([], ['/srv/homes/alice']), - isGitRepository: vi.fn().mockResolvedValue(false) - } - - await expect( - canCleanupUnregisteredOrcaLeftoverDirectory({ - ...leftoverArgs, - home: executionHostRemovalHome(null) - }) - ).resolves.toBe(false) - await expect( - canCleanupUnregisteredOrcaLeftoverDirectory({ - ...leftoverArgs, - home: executionHostRemovalHome('/srv/homes/bob') - }) - ).resolves.toBe(true) - }) }) diff --git a/src/main/worktree-removal-safety.ts b/src/main/worktree-removal-safety.ts index 843d59a74073..97af3a8893af 100644 --- a/src/main/worktree-removal-safety.ts +++ b/src/main/worktree-removal-safety.ts @@ -1,5 +1,4 @@ import { lstat, readFile } from 'node:fs/promises' -import type { Repo } from '../shared/repo-types' 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' @@ -117,7 +116,7 @@ export function findRegisteredDeletableWorktree( export function assertWorktreeDoesNotContainRegisteredWorktree( worktreePath: string, - worktrees: readonly GitWorktreeInfo[] + worktrees: readonly Pick[] ): void { const nestedWorktree = worktrees.find((item) => { if (areWorktreePathsEqual(item.path, worktreePath)) { @@ -180,58 +179,6 @@ export function canCleanupUnregisteredOrcaWorktreeDirectory(args: { return false } -export async function canCleanupUnregisteredOrcaLeftoverDirectory(args: { - meta: UnregisteredOrcaCleanupMeta | null | undefined - worktreePath: string - runtimeWorktreePath: string - repo: Pick - runtimeRepoPath: string - registeredWorktrees: readonly GitWorktreeInfo[] - statPath: StatPath - home: WorktreeRemovalHomeAuthority - isGitRepository: (runtimeWorktreePath: string) => Promise -}): Promise { - // Why: this recovery state has already lost the worktree .git marker, so the - // existing .git-file orphan proof cannot establish ownership. - // Why: without a surviving .git file, path shape alone is too weak to prove - // ownership for recursive deletion; require persisted Orca-created evidence. - if (!hasCurrentOrcaCreationProvenance(args.meta) && !hasLegacyOrcaCreationEvidence(args.meta)) { - return false - } - - // Why: same recursive delete, same rule — no home answer from the executing host, no delete. - if (!isRemovalHomeAuthorityResolved(args.home)) { - return false - } - - if ( - isDangerousWorktreeRemovalPath(args.worktreePath, args.repo.path, args.home) || - isDangerousWorktreeRemovalPath(args.runtimeWorktreePath, args.runtimeRepoPath, args.home) - ) { - return false - } - - assertWorktreeDoesNotContainRegisteredWorktree(args.worktreePath, args.registeredWorktrees) - - const targetEntry = await args.statPath(args.runtimeWorktreePath).catch(() => null) - if (!isDirectoryStat(targetEntry)) { - return false - } - - const pathOps = getPathOps(args.runtimeWorktreePath, args.runtimeRepoPath) - const gitMarkerPath = pathOps.join(args.runtimeWorktreePath, '.git') - try { - await args.statPath(gitMarkerPath) - return false - } catch (error) { - if (!isMissingPathError(error)) { - return false - } - } - - return !(await args.isGitRepository(args.runtimeWorktreePath)) -} - function hasCurrentOrcaCreationProvenance( meta: Pick | null | undefined ): boolean { @@ -287,20 +234,6 @@ function isMissingPathError(error: unknown): boolean { ) } -function isDirectoryStat(stat: unknown): boolean { - const entry = - stat && typeof stat === 'object' - ? (stat as { isDirectory?: () => boolean; isSymbolicLink?: () => boolean; type?: unknown }) - : null - if (!entry) { - return false - } - if (entry.isSymbolicLink?.() === true || entry.type === 'symlink') { - return false - } - return entry.isDirectory?.() === true || entry.type === 'directory' -} - export async function isWorktreePathMissing( worktreePath: string, statPath: (path: string) => Promise = lstat diff --git a/src/main/worktree-removal-table.ts b/src/main/worktree-removal-table.ts index 8c901bc9e37b..857aba660143 100644 --- a/src/main/worktree-removal-table.ts +++ b/src/main/worktree-removal-table.ts @@ -2,14 +2,12 @@ import { lstat } from 'node:fs/promises' import { getErrorCode } from './git/worktree-operation-options' import { areWorktreePathsEqual } from './git/worktree-path-comparison' import { writeWorktreeRemovalRecords, type WorktreeRemovalRecord } from './worktree-removal-records' -import type { RemoveWorktreeResult } from '../shared/worktree/create-types' -import type { GitWorktreeInfo } from '../shared/worktree/types' // The accepted removals, mirrored to disk on every change; listings and joins read only this. export const pendingWorktreeRemovals = new Map() -// Deletes that failed after Git dropped the registration: listed with their error until Delete -// retries them, the checkout disappears or is replaced, or the repo leaves Orca. Never retried -// unasked. +// Deletes that failed after Git dropped the registration: listed with their error until the +// checkout disappears or is replaced, Delete finds Git registering it, or the repo leaves Orca. +// Never retried unasked. export const failedWorktreeRemovals = new Map() // Why weak: a listing that read Git before a delete finished holds the record until it replies. export const finishedWorktreeRemovals = new WeakSet() @@ -43,26 +41,6 @@ export async function worktreeCheckoutExists(worktreePath: string): Promise[], - retry: () => Promise | undefined -): boolean { - if (registeredWorktrees.some((worktree) => areWorktreePathsEqual(worktree.path, worktreePath))) { - if (failedWorktreeRemovals.delete(worktreeId)) { - void persistWorktreeRemovalRecords() - } - return false - } - return retry() !== undefined -} - export function hasPendingWorktreeRemovals(): boolean { return pendingWorktreeRemovals.size > 0 }