fix(worktrees): never delete a folder git no longer tracks

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<void>,
+  assertCheckoutGone: () => Promise<void>,
+  options: RemoveWorktreeOptions = {}
+): Promise<RemoveWorktreeResult> {
+  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<RemoveWorktreeResult> {
   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 {}
     }

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()

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 })
     }

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 }

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<RemoveWorktreeResult> =>
     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<void> }
   checkoutDeleteSignal?: AbortSignal
+  cleanupPushTarget: () => Promise<void>
 }): Promise<RemoveWorktreeResult> {
   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,