diff --git a/src/main/git/worktree-add.ts b/src/main/git/worktree-add.ts index acce67288ba..2e4198eb665 100644 --- a/src/main/git/worktree-add.ts +++ b/src/main/git/worktree-add.ts @@ -21,7 +21,7 @@ import type { } from './worktree-operation-options' import { gitExecOptions, resolveWorktreeAddTimeoutMs } from './worktree-operation-options' import { bumpWorktreeScanGeneration } from './worktree-scan-cache' -import { assertNoPendingWorktreeRemovalConflict } from '../worktree-background-removal' +import { assertNoPendingWorktreeRemovalConflict } from '../worktree-removal-table' export type WorktreeAddBaseContext = Pick & { effectiveBase: string diff --git a/src/main/git/worktree-removal.ts b/src/main/git/worktree-removal.ts index 39d2056f708..8924fe04a9f 100644 --- a/src/main/git/worktree-removal.ts +++ b/src/main/git/worktree-removal.ts @@ -128,15 +128,21 @@ 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. */ export async function finishUnregisteredWorktreeRemoval( repoPath: string, worktreePath: string, branch: { name: string; head: string } | null, + assertLeftover: () => Promise, options: RemoveWorktreeOptions = {} ): Promise { try { - await runUnderWorktreeDeleteLimit(() => removeCheckoutLeftByGit(worktreePath, options)) + 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) ) diff --git a/src/main/ipc/worktree-metadata-merge.ts b/src/main/ipc/worktree-metadata-merge.ts index 7cd2e296ffd..f9fb08676ff 100644 --- a/src/main/ipc/worktree-metadata-merge.ts +++ b/src/main/ipc/worktree-metadata-merge.ts @@ -50,6 +50,7 @@ export function mergeWorktree( isBare: git.isBare, ...(git.isSparse === true ? { isSparse: true } : {}), isMainWorktree: git.isMainWorktree, + ...(git.removalError ? { removalError: git.removalError } : {}), // Automatic labels follow the live branch; persisted values are only authoritative when pinned. displayName: meta?.displayNameIsPinned === false diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index a505cfe3da2..05396e16946 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -173,7 +173,7 @@ import { import { createRetiredNameLookup } from '../../shared/worktree/retired-name-registry' import { toLocalBaseRefRefreshResult } from '../../shared/worktree/local-base-branch-fast-forward' import { isSshRequestOutcomeUnverifiable } from '../ssh/ssh-channel-multiplexer' -import { findPendingWorktreeRemovalConflict } from '../worktree-background-removal' +import { findPendingWorktreeRemovalConflict } from '../worktree-removal-table' const SSH_WORKTREE_CREATE_FETCH_FRESHNESS_MS = 30_000 const SSH_WORKTREE_CREATE_FETCH_CACHE_MAX = 512 diff --git a/src/main/ipc/worktrees-failed-removal.test.ts b/src/main/ipc/worktrees-failed-removal.test.ts new file mode 100644 index 00000000000..f1cfa13eb80 --- /dev/null +++ b/src/main/ipc/worktrees-failed-removal.test.ts @@ -0,0 +1,252 @@ +// 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; +// runtime-failed-local-worktree-removal.test.ts runs the real thing. +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + killAllProcessesForWorktreeMock, + listWorktreesMock, + removeWorktreeMock +} from './worktrees-test-module-mocks' +import { handlers, setupWorktreeHandlers, store } from './worktrees-test-harness' +import { 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 type * as WorktreeRemovalModule from '../git/worktree-removal' +import type * as WorktreeRemovalTable from '../worktree-removal-table' +import type * as WorktreeRemovalLeftover from '../worktree-removal-leftover' +import { + _resetPendingWorktreeRemovalsForTests, + _settlePendingWorktreeRemovalsForTests, + retryFailedWorktreeRemoval, + startBackgroundWorktreeRemoval +} from '../worktree-background-removal' + +vi.mock('electron', async () => + (await import('./worktrees-test-module-mocks')).electronModuleMock() +) +vi.mock('../git/worktree', async () => + (await import('./worktrees-test-module-mocks')).gitWorktreeModuleMock() +) +vi.mock('../git/runner', async () => + (await import('./worktrees-test-module-mocks')).gitRunnerModuleMock() +) +vi.mock('../git/repo', async () => + (await import('./worktrees-test-module-mocks')).gitRepoModuleMock() +) +vi.mock('../git/git-username', async (importOriginal) => ({ + ...(await importOriginal>()), + resolveLocalGitUsername: (await import('./worktrees-test-module-mocks')) + .resolveLocalGitUsernameMock +})) +vi.mock('../github/client', async () => + (await import('./worktrees-test-module-mocks')).githubClientModuleMock() +) +vi.mock('../source-control/hosted-review', async () => + (await import('./worktrees-test-module-mocks')).hostedReviewModuleMock() +) +vi.mock('../providers/ssh-git-dispatch', async () => + (await import('./worktrees-test-module-mocks')).sshGitDispatchModuleMock() +) +vi.mock('../providers/ssh-filesystem-dispatch', async () => + (await import('./worktrees-test-module-mocks')).sshFilesystemDispatchModuleMock() +) +vi.mock('./worktree-symlinks', async () => + (await import('./worktrees-test-module-mocks')).worktreeSymlinksModuleMock() +) +vi.mock('./ssh', async () => (await import('./worktrees-test-module-mocks')).sshModuleMock()) +vi.mock('../ssh/ssh-target-registry', async () => + (await import('./worktrees-test-module-mocks')).sshTargetRegistryModuleMock() +) +vi.mock('../hooks', async () => (await import('./worktrees-test-module-mocks')).hooksModuleMock()) +vi.mock('../setup-runner-script-text', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).setupRunnerScriptTextModuleMock( + await importOriginal>() + ) +) +vi.mock('../worktree-runner-script', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).worktreeRunnerScriptModuleMock( + await importOriginal>() + ) +) +vi.mock('../effective-hook-config', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).effectiveHookConfigModuleMock( + await importOriginal>() + ) +) +vi.mock('../setup-hook-env-vars', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).setupHookEnvVarsModuleMock( + await importOriginal>() + ) +) +vi.mock('./worktree-logic', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).worktreeLogicModuleMock( + await importOriginal>() + ) +) +vi.mock('../terminal-history-deletion', async () => + (await import('./worktrees-test-module-mocks')).terminalHistoryDeletionModuleMock() +) +vi.mock('../ports/advertised-url-watcher', async () => + (await import('./worktrees-test-module-mocks')).advertisedUrlWatcherModuleMock() +) +vi.mock('../workspace-cleanup-scan-snapshot', async () => + (await import('./worktrees-test-module-mocks')).workspaceCleanupScanSnapshotModuleMock() +) +vi.mock('../workspace-space-analysis-snapshot', async () => + (await import('./worktrees-test-module-mocks')).workspaceSpaceAnalysisSnapshotModuleMock() +) +vi.mock('../workspace-cleanup-removal-snapshot-prune', async () => + (await import('./worktrees-test-module-mocks')).workspaceCleanupRemovalSnapshotPruneModuleMock() +) +vi.mock('../runtime/worktree-teardown', async () => + (await import('./worktrees-test-module-mocks')).worktreeTeardownModuleMock() +) +vi.mock('./pty', async () => (await import('./worktrees-test-module-mocks')).ptyModuleMock()) + +vi.mock('../git/worktree-removal', async (importOriginal) => ({ + ...(await importOriginal()), + finishUnregisteredWorktreeRemoval: vi.fn(async () => ({})) +})) +// The leftover is on disk and is the removed checkout's own (no `.git` left). +vi.mock('../worktree-removal-table', async (importOriginal) => ({ + ...(await importOriginal()), + worktreeCheckoutExists: vi.fn(async () => true) +})) +vi.mock('../worktree-removal-leftover', async (importOriginal) => ({ + ...(await importOriginal()), + isUnregisteredRemovalLeftover: vi.fn(async () => true) +})) + +const featureId = 'repo-1::/workspace/feature-wt' +const GIT_ERROR = "error: failed to delete '/workspace/feature-wt': Operation not permitted" + +function remove(args: Record): Promise { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: worktrees:remove resolves a RemoveWorktreeResult. + return handlers['worktrees:remove'](null, args) as Promise +} + +async function listFeature(): Promise { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: worktrees:list resolves the repo's Worktree rows. + const rows = (await handlers['worktrees:list'](null, { repoId: 'repo-1' })) as Worktree[] + return rows.find((row) => row.id === featureId) +} + +/** Git fails partway and drops the registration, as `git worktree remove --force` does. */ +async function failAfterGitDroppedIt(): Promise { + const [main, feature] = mockKnownFeatureWorktree() + const result = startBackgroundWorktreeRemoval({ + removal: { + worktreeId: featureId, + repoId: 'repo-1', + repoPath: '/workspace/repo', + worktree: feature, + deleteBranch: true, + force: false + }, + run: async () => { + listWorktreesMock.mockResolvedValue([main]) + throw new Error(GIT_ERROR) + }, + publish: () => {} + }) + await expect(result).rejects.toThrow(GIT_ERROR) + await _settlePendingWorktreeRemovalsForTests() +} + +describe('a failed delete Git no longer registers, over desktop IPC', () => { + beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + setupWorktreeHandlers() + }) + + afterEach(() => { + _resetPendingWorktreeRemovalsForTests() + vi.mocked(finishUnregisteredWorktreeRemoval).mockClear() + }) + + it('stays in the listing with the error instead of vanishing', async () => { + await failAfterGitDroppedIt() + + const row = await listFeature() + expect(row).toMatchObject({ path: '/workspace/feature-wt', removalError: GIT_ERROR }) + expect(row?.removing).toBeUndefined() + }) + + it('Delete runs the recorded removal again: teardown, leftover, branch and metadata', async () => { + await failAfterGitDroppedIt() + killAllProcessesForWorktreeMock.mockClear() + + 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', + '/workspace/feature-wt', + { name: 'feature', head: 'feature' }, + expect.any(Function), + {} + ) + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith( + featureId, + expect.objectContaining({ requirePhysicalStop: true }) + ) + expect(store.removeWorktreeMeta).toHaveBeenCalledWith(featureId, 'local') + 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() + listWorktreesMock.mockResolvedValue([main]) + const otherClientsRetry = vi.fn(async () => ({})) + listWorktreesMock.mockImplementationOnce(async () => { + // Another client's Delete takes the failed record during this Delete's `git worktree list`. + void retryFailedWorktreeRemoval(featureId, 'local', () => ({ + run: otherClientsRetry, + publish: () => {} + })) + return [main] + }) + + await expect(remove({ worktreeId: featureId })).resolves.not.toHaveProperty('removing') + await _settlePendingWorktreeRemovalsForTests() + + expect(otherClientsRetry).toHaveBeenCalledTimes(1) + // Neither a second retry nor the delete for leftovers without a record ran. + expect(finishUnregisteredWorktreeRemoval).not.toHaveBeenCalled() + expect(removeWorktreeMock).not.toHaveBeenCalled() + }) + + it('Delete takes the normal delete once Git registers a checkout at the path again', async () => { + await failAfterGitDroppedIt() + // A new checkout at the same path: the recorded choices were for the leftover, not for it. + mockKnownFeatureWorktree() + removeWorktreeMock.mockResolvedValue({}) + + await remove({ worktreeId: featureId, force: false }) + await _settlePendingWorktreeRemovalsForTests() + + expect(finishUnregisteredWorktreeRemoval).not.toHaveBeenCalled() + expect(removeWorktreeMock).toHaveBeenCalledWith( + '/workspace/repo', + '/workspace/feature-wt', + false, + expect.anything() + ) + listWorktreesMock.mockResolvedValue([mockKnownFeatureWorktree()[0]]) + expect(await listFeature()).toBeUndefined() + }) +}) diff --git a/src/main/ipc/worktrees/listing/detected-provider-listing.ts b/src/main/ipc/worktrees/listing/detected-provider-listing.ts index 8bf5d924b2c..e63d024803b 100644 --- a/src/main/ipc/worktrees/listing/detected-provider-listing.ts +++ b/src/main/ipc/worktrees/listing/detected-provider-listing.ts @@ -1,7 +1,8 @@ import { projectPendingWorktreeRemovals, - snapshotPendingWorktreeRemovals -} from '../../../worktree-background-removal' + snapshotPendingWorktreeRemovals, + withUnregisteredRemovalCheckouts +} from '../../../worktree-removal-listing' import { getRepoExecutionHostId, getSshTargetIdForExecutionHost @@ -149,6 +150,9 @@ export async function listDetectedWorktreesForCapturedRepo( return abortedResult() ?? null } const { gitWorktrees, fresh: freshScan, sideEffectToken, metadataPrune, hygieneDue } = scan + const localRows = connectionId + ? gitWorktrees + : await withUnregisteredRemovalCheckouts(repo.id, gitWorktrees) const aborted = abortedResult() if (aborted) { return aborted @@ -193,7 +197,7 @@ export async function listDetectedWorktreesForCapturedRepo( ? buildDetectedGitWorktrees(store, repo, gitWorktrees, allMeta) : // Why always marked: the desktop renderer ships with this main process. projectPendingWorktreeRemovals( - buildDetectedGitWorktrees(store, repo, gitWorktrees, allMeta), + buildDetectedGitWorktrees(store, repo, localRows, allMeta), (worktree) => worktree.id, true, pendingAtScan diff --git a/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts b/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts index c09c1224e88..1473ab31bd8 100644 --- a/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts +++ b/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts @@ -36,8 +36,9 @@ import type { Worktree } from '../../../../shared/worktree/types' import { projectPendingWorktreeRemovals, snapshotPendingWorktreeRemovals, + withUnregisteredRemovalCheckouts, type PendingWorktreeRemovals -} from '../../../worktree-background-removal' +} from '../../../worktree-removal-listing' import { getLocalWorktreeScanGeneration } from '../../../local-worktree-scan-generation' import { getRegisteredWorktreeRootsRevision } from '../../registered-worktree-roots-cache' @@ -167,7 +168,10 @@ export function registerWorktreeCatalogHandlers(context: WorktreeIpcContext): vo } loggedWorktreeListFailures.delete(`${repo.id}:${repo.path}`) const metadata = metadataForRepo(repo) - const worktrees = buildDetectedGitWorktrees(store, repo, gitWorktrees, metadata) + const rows = connectionId + ? gitWorktrees + : await withUnregisteredRemovalCheckouts(repo.id, gitWorktrees) + const worktrees = buildDetectedGitWorktrees(store, repo, rows, metadata) .filter((worktree) => worktree.visible) .map((worktree) => stampAndMergeVisibleDetectedWorktree(store, repo, worktree, metadata)) return connectionId ? worktrees : markLocalWorktreesUnderRemoval(worktrees, pendingAtScan) @@ -261,7 +265,10 @@ export function registerWorktreeCatalogHandlers(context: WorktreeIpcContext): vo } loggedWorktreeListFailures.delete(`${repo.id}:${repo.path}`) const metadata = allMeta ?? readAllWorktreeMetaForRepo(store, repo) - const worktrees = buildDetectedGitWorktrees(store, repo, gitWorktrees, metadata) + const rows = connectionId + ? gitWorktrees + : await withUnregisteredRemovalCheckouts(repo.id, gitWorktrees) + const worktrees = buildDetectedGitWorktrees(store, repo, rows, metadata) .filter((worktree) => worktree.visible) .map((worktree) => stampAndMergeVisibleDetectedWorktree(store, repo, worktree, metadata)) return connectionId ? worktrees : markLocalWorktreesUnderRemoval(worktrees, pendingAtScan) diff --git a/src/main/ipc/worktrees/removal/execute-worktree-removal.ts b/src/main/ipc/worktrees/removal/execute-worktree-removal.ts index fc4053c579a..04e25eac366 100644 --- a/src/main/ipc/worktrees/removal/execute-worktree-removal.ts +++ b/src/main/ipc/worktrees/removal/execute-worktree-removal.ts @@ -36,6 +36,8 @@ import { removeFolderWorkspace } from './remove-folder-workspace' import { removeUnregisteredWorktree } from './remove-unregistered-worktree' import { removeRegisteredRemoteWorktree } from './remove-registered-remote-worktree' import { removeRegisteredLocalWorktree } from './remove-registered-local-worktree' +import { retryFailedLocalWorktreeRemoval } from './retry-failed-local-worktree-removal' +import { retryFailedRemovalUnlessRegistered } from '../../../worktree-removal-table' /** * Refuses a repo row whose two host spellings disagree. @@ -114,6 +116,14 @@ export async function executeWorktreeRemoval( registeredWorktrees, resolveWorktreeRemovalHomeForHost(removalHostId) ) + if ( + !repo.connectionId && + retryFailedRemovalUnlessRegistered(args.worktreeId, worktreePath, registeredWorktrees, () => + retryFailedLocalWorktreeRemoval(context, args, removalHostId) + ) + ) { + return { removing: true } + } if (!registeredWorktree) { return removeUnregisteredWorktree( context, diff --git a/src/main/ipc/worktrees/removal/retry-failed-local-worktree-removal.ts b/src/main/ipc/worktrees/removal/retry-failed-local-worktree-removal.ts new file mode 100644 index 00000000000..30fd0351728 --- /dev/null +++ b/src/main/ipc/worktrees/removal/retry-failed-local-worktree-removal.ts @@ -0,0 +1,62 @@ +import { LOCAL_EXECUTION_HOST_ID, type ExecutionHostId } from '../../../../shared/execution-host' +import type { RemoveWorktreeResult } from '../../../../shared/worktree/create-types' +import { retryFailedWorktreeRemoval } from '../../../worktree-background-removal' +import { interruptedLocalWorktreeRemovalJob } from '../../../runtime/runtime-interrupted-local-worktree-removal' +import { invalidateAuthorizedRootsCacheForRepo } from '../../registered-worktree-roots-scoped-invalidation' +import type { RemoveWorktreeArgs } from '../ipc-context-schemas' +import type { WorktreeIpcContext } from '../worktree-ipc-context' +import { + preserveBranchHeadFallback, + rememberPreservedBranchCleanupTarget +} from './preserved-branch-cleanup' +import { + removeWorktreeMetadataAndTransientState, + stopPtysForDestructiveWorktreeRemoval +} from './worktree-removal-ownership' + +/** + * Delete on the leftover of a local delete that failed after Git dropped the registration: runs the + * recorded removal again with this handler's bookkeeping. Undefined when there is none. + */ +export function retryFailedLocalWorktreeRemoval( + context: WorktreeIpcContext, + args: RemoveWorktreeArgs, + removalHostId: ExecutionHostId +): Promise | undefined { + const { store, runtime, options } = context + return retryFailedWorktreeRemoval(args.worktreeId, removalHostId, (record) => + interruptedLocalWorktreeRemovalJob(record, { + store, + acquireWatcherRemoval: (path) => runtime.acquireFileWatcherRemoval(path), + closeWatchers: (path) => runtime.closeFileWatchersForRemoval(path), + stopPtys: () => + stopPtysForDestructiveWorktreeRemoval(runtime, record.worktreeId, { + allowUnverifiedStop: args.allowUnverifiedPtyStop + }), + preservedBranchCleanup: { + preserveHead: preserveBranchHeadFallback, + remember: (worktreeId, _hostId, result, fallbackHead, pushTarget) => + rememberPreservedBranchCleanupTarget( + worktreeId, + LOCAL_EXECUTION_HOST_ID, + result, + fallbackHead, + pushTarget + ) + }, + purge: ({ worktreeId, repoId }) => { + runtime.clearOptimisticReconcileToken(worktreeId) + removeWorktreeMetadataAndTransientState( + store, + worktreeId, + LOCAL_EXECUTION_HOST_ID, + args.snapshotPruneBatchId + ) + invalidateAuthorizedRootsCacheForRepo(store, repoId) + }, + onRemoved: ({ worktreeId, worktreePath }) => + options?.onWorktreeLifecycle?.({ kind: 'removed', worktreeId, path: worktreePath }), + publish: (repoId) => runtime.publishWorktreeRemovalChange(repoId) + }) + ) +} diff --git a/src/main/repo-maintenance-idle-gate.ts b/src/main/repo-maintenance-idle-gate.ts index dec9acb549f..7bacf6f7cca 100644 --- a/src/main/repo-maintenance-idle-gate.ts +++ b/src/main/repo-maintenance-idle-gate.ts @@ -6,7 +6,7 @@ import { } from './git/local-repo-ref-maintenance' import { hasWorktreeRemovalsInFlight } from './ipc/worktrees/worktree-ipc-context' import { hasPendingWorktreeCreatePreparations } from './worktree-create-preparation' -import { hasPendingWorktreeRemovals } from './worktree-background-removal' +import { hasPendingWorktreeRemovals } from './worktree-removal-table' /** * The app-wide "not now" answer for idle repo maintenance. diff --git a/src/main/runtime/orca-runtime-list-known-resolved-worktrees-for-explicit-target.ts b/src/main/runtime/orca-runtime-list-known-resolved-worktrees-for-explicit-target.ts index 152ef547889..a6b729b465e 100644 --- a/src/main/runtime/orca-runtime-list-known-resolved-worktrees-for-explicit-target.ts +++ b/src/main/runtime/orca-runtime-list-known-resolved-worktrees-for-explicit-target.ts @@ -20,7 +20,12 @@ import type { Repo } from '../../shared/repo-types' import type { ProjectExecutionRuntimeResolution } from '../../shared/project-execution-runtime' import type { RuntimeWorktreeScanResult } from './repo-worktree-resolution-scan' import { getSshGitProviderGeneration } from '../providers/ssh-git-dispatch' -import { getRepoExecutionHostId, getRepoSshConnectionId } from '../../shared/execution-host' +import { + getRepoExecutionHostId, + getRepoSshConnectionId, + LOCAL_EXECUTION_HOST_ID +} from '../../shared/execution-host' +import { withUnregisteredRemovalCheckouts } from '../worktree-removal-listing' import type { RuntimeWorktreeScanCache } from './orca-runtime-core' import { resolveWorktreeScanCacheTtlMs } from './runtime-worktree-scan-cache' @@ -116,7 +121,7 @@ export class OrcaRuntimeWithListKnownResolvedWorktreesForExplicitTarget extends return { store, scanRepo: (repo, projectRuntimeByRepoId) => - this.listRepoWorktreesForResolution(repo, projectRuntimeByRepoId), + this.listRepoWorktreesForListing(repo, projectRuntimeByRepoId), listFolderWorkspaces: (repo, repoOwnerCount) => listRuntimeFolderWorkspaces(store, repo, repoOwnerCount) } @@ -132,6 +137,17 @@ export class OrcaRuntimeWithListKnownResolvedWorktreesForExplicitTarget extends return await resolveScopedWorktreeIdRow(this.repoWorktreeRowDeps(), worktreeId, requiredHostId) } + /** The resolution scan plus the checkouts this host's removals own that Git no longer lists. */ + protected async listRepoWorktreesForListing( + repo: Repo, + projectRuntimeByRepoId?: ReadonlyMap + ): Promise { + const scan = await this.listRepoWorktreesForResolution(repo, projectRuntimeByRepoId) + return scan.ok && getRepoExecutionHostId(repo) === LOCAL_EXECUTION_HOST_ID + ? { ok: true, worktrees: await withUnregisteredRemovalCheckouts(repo.id, scan.worktrees) } + : scan + } + protected async listRepoWorktreesForResolution( repo: Repo, projectRuntimeByRepoId?: ReadonlyMap diff --git a/src/main/runtime/orca-runtime-remove-managed-worktree.ts b/src/main/runtime/orca-runtime-remove-managed-worktree.ts index dc065bd8224..ec9611a1d04 100644 --- a/src/main/runtime/orca-runtime-remove-managed-worktree.ts +++ b/src/main/runtime/orca-runtime-remove-managed-worktree.ts @@ -6,10 +6,7 @@ import { } from '../worktree-removal-repo-owner' import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' import { getRepoExecutionHostId, parseExecutionHostId } from '../../shared/execution-host' -import { - finishAcceptedWorktreeRemoval, - waitForPendingWorktreeRemoval -} from '../worktree-background-removal' +import { finishAcceptedWorktreeRemoval } from '../worktree-background-removal' import { preservedBranchCleanupScopeKey } from '../../shared/preserved-branch-cleanup' import { getRuntimeWorktreeRemovalOptionsKey, @@ -54,7 +51,7 @@ export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateM const cleanupHostId = parseExecutionHostId(hostId)?.id const removalTarget = await this.resolveWorktreeRemovalTarget(worktreeSelector, cleanupHostId) // Why: a retry or a second client asking while Git still deletes joins that removal. - const pending = waitForPendingWorktreeRemoval(removalTarget.id, cleanupHostId) + const pending = this.joinPendingWorktreeRemoval(removalTarget.id, options) if (pending) { return options.waitForBackgroundRemoval ? await pending : { removing: true } } @@ -134,6 +131,9 @@ export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateM registeredWorktrees, removalHome ) + if (this.retryFailedLocalRemoval(route, removalTarget, registeredWorktrees, options)) { + return { removing: true } + } if (!registeredWorktree) { return removeRuntimeUnregisteredWorktree({ repo, diff --git a/src/main/runtime/orca-runtime-resolve-worktree-removal-target.ts b/src/main/runtime/orca-runtime-resolve-worktree-removal-target.ts index 7fef1bdf9db..71b919bbf1b 100644 --- a/src/main/runtime/orca-runtime-resolve-worktree-removal-target.ts +++ b/src/main/runtime/orca-runtime-resolve-worktree-removal-target.ts @@ -1,7 +1,10 @@ // @ts-nocheck -- mechanically split from OrcaRuntimeService; behavior is covered by AST equivalence and characterization tests. import { OrcaRuntimeWithRemoveManagedWorktree } from './orca-runtime-remove-managed-worktree' import type { ExecutionHostId } from '../../shared/execution-host' -import type { RuntimeWorktreeRemovalTarget } from './runtime-worktree-selection' +import type { + RemoveManagedWorktreeOptions, + RuntimeWorktreeRemovalTarget +} from './runtime-worktree-selection' import { resolveRuntimeWorktreeRemovalTarget } from './runtime-worktree-removal-target' import type { RuntimeStore } from './runtime-store-contract' import { splitWorktreeId } from '../../shared/worktree/id' @@ -10,7 +13,10 @@ import { hasWorktreeRemovalRepoOwnerOnOtherHost } from '../worktree-removal-repo import { advertisedUrlWatcher } from '../ports/advertised-url-watcher' import { deleteWorktreeHistoryDir } from '../terminal-history-deletion' import { closeClientHostedBrowserPagesForWorktree } from './worktree-browser-client-page-close' -import type { ForceDeleteWorktreeBranchResult } from '../../shared/worktree/create-types' +import type { + ForceDeleteWorktreeBranchResult, + RemoveWorktreeResult +} from '../../shared/worktree/create-types' import type { RuntimeTerminalRename } from '../../shared/runtime-types' import type { TerminalWorkspaceLaunchScope } from './runtime-legacy-worker-terminal-recovery-types' import type { TerminalCreateOptions } from './runtime-terminal-contracts' @@ -22,10 +28,16 @@ import { resolveBareAgentLaunchCommand } from './runtime-agent-launch-resolution import { buildAgentStartupPlan } from '../../shared/tui-agent-startup' import { resolveAgentStartupPlanInputs } from '../../shared/agent-startup-plan-inputs' import { agentStartedTelemetry } from '../agent-launch/agent-started-telemetry' -import { LOCAL_EXECUTION_HOST_ID } from '../../shared/execution-host' +import { LOCAL_EXECUTION_HOST_ID, parseExecutionHostId } from '../../shared/execution-host' import { invalidateAuthorizedRootsCache } from '../ipc/filesystem-auth' -import { resumeInterruptedWorktreeRemovals } from '../worktree-background-removal' +import { + resumeInterruptedWorktreeRemovals, + retryFailedWorktreeRemoval, + waitForPendingWorktreeRemoval +} from '../worktree-background-removal' import { interruptedLocalWorktreeRemovalJob } from './runtime-interrupted-local-worktree-removal' +import { retryFailedRemovalUnlessRegistered } from '../worktree-removal-table' +import type { GitWorktreeInfo } from '../../shared/worktree/types' export class OrcaRuntimeWithResolveWorktreeRemovalTarget extends OrcaRuntimeWithRemoveManagedWorktree { protected async resolveWorktreeRemovalTarget( @@ -49,20 +61,61 @@ export class OrcaRuntimeWithResolveWorktreeRemovalTarget extends OrcaRuntimeWith return } resumeInterruptedWorktreeRemovals((record) => - interruptedLocalWorktreeRemovalJob(record, { - store, - acquireWatcherRemoval: this.acquireFileWatcherRemoval, - closeWatchers: (path) => this.closeFileWatchersForRemoval(path), - preservedBranchCleanup: this.preservedBranchCleanup, - purge: ({ worktreeId, repoId }) => - this.purgeRemovedWorktree(store, worktreeId, repoId, LOCAL_EXECUTION_HOST_ID), - onRemoved: ({ worktreeId, worktreePath }) => - this.emitWorktreeLifecycle({ kind: 'removed', worktreeId, path: worktreePath }), - publish: (repoId) => this.publishWorktreeRemovalChange(repoId) - }) + interruptedLocalWorktreeRemovalJob(record, this.localRemovalJobHost(store)) ) } + /** The removal a request for this worktree waits on: the one still running. */ + protected joinPendingWorktreeRemoval( + worktreeId: string, + options: RemoveManagedWorktreeOptions + ): Promise | undefined { + return waitForPendingWorktreeRemoval(worktreeId, parseExecutionHostId(options.hostId)?.id) + } + + /** + * 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. + */ + protected retryFailedLocalRemoval( + route: { kind: string }, + target: { id: string; path: string }, + registeredWorktrees: readonly GitWorktreeInfo[], + options: RemoveManagedWorktreeOptions + ): boolean { + const store = this.store + if (route.kind !== 'local' || !store) { + return false + } + const hostId = parseExecutionHostId(options.hostId)?.id + const allowUnverifiedPtyStop = options.allowUnverifiedPtyStop === true + return retryFailedRemovalUnlessRegistered(target.id, target.path, registeredWorktrees, () => + retryFailedWorktreeRemoval(target.id, hostId, (record) => + interruptedLocalWorktreeRemovalJob(record, { + ...this.localRemovalJobHost(store), + stopPtys: () => + this.stopPtysForDestructiveWorktreeRemoval(record.worktreeId, { + allowUnverifiedStop: allowUnverifiedPtyStop + }) + }) + ) + ) + } + + protected localRemovalJobHost(store: RuntimeStore) { + return { + store, + acquireWatcherRemoval: this.acquireFileWatcherRemoval, + closeWatchers: (path) => this.closeFileWatchersForRemoval(path), + preservedBranchCleanup: this.preservedBranchCleanup, + purge: ({ worktreeId, repoId }) => + this.purgeRemovedWorktree(store, worktreeId, repoId, LOCAL_EXECUTION_HOST_ID), + onRemoved: ({ worktreeId, worktreePath }) => + this.emitWorktreeLifecycle({ kind: 'removed', worktreeId, path: worktreePath }), + publish: (repoId) => this.publishWorktreeRemovalChange(repoId) + } + } + // Host state every removal path drops once Git has let go of the checkout. protected purgeRemovedWorktree( store: RuntimeStore, diff --git a/src/main/runtime/orca-runtime-stop-requested-pty-ids.ts b/src/main/runtime/orca-runtime-stop-requested-pty-ids.ts index 4e8087e5a95..8b406b2df6a 100644 --- a/src/main/runtime/orca-runtime-stop-requested-pty-ids.ts +++ b/src/main/runtime/orca-runtime-stop-requested-pty-ids.ts @@ -148,7 +148,7 @@ export class OrcaRuntimeWithStopRequestedPtyIds extends OrcaRuntimeWithRuntimeId listResolved: () => this.listResolvedWorktrees(), resolveRepo: (selector) => this.resolveRepoSelector(selector), selectRepos: (selector) => this.selectReposBySelector(selector), - scanRepo: (repo) => this.listRepoWorktreesForResolution(repo), + scanRepo: (repo) => this.listRepoWorktreesForListing(repo), listKnownHostIds: () => this.listKnownExecutionHostIds() }) 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 new file mode 100644 index 00000000000..a875bc30336 --- /dev/null +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-failed-retry.spec.ts @@ -0,0 +1,206 @@ +// 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. +import { existsSync } from 'node:fs' +import { realpath } from 'node:fs/promises' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + join, + listWorktreesStrict, + mkdir, + mkdtemp, + removeWorktree, + rm, + scanLocalRepoWorktreesForResolutionMock, + tmpdir +} from '../orca-runtime-test-mocks.spec' +import { TEST_REPO_ID, TEST_REPO_PATH } from '../orca-runtime-test-fixtures.spec' +import { createWorktreeRemovalRuntime } from '../orca-runtime-test-scenario-builders.spec' +import { + _resetPendingWorktreeRemovalsForTests, + _settlePendingWorktreeRemovalsForTests, + loadWorktreeRemovalRecords, + retryFailedWorktreeRemoval +} from '../../worktree-background-removal' +import { + readWorktreeRemovalRecords, + writeWorktreeRemovalRecords +} from '../../worktree-removal-records' + +const FAILURE = "error: failed to delete 'node_modules/a/LICENSE': Operation not permitted" + +describe('runtime Delete on a failed delete’s leftover', () => { + let directory = '' + let leftover = '' + let leftoverId = '' + + beforeEach(async () => { + vi.clearAllMocks() + directory = await realpath(await mkdtemp(join(tmpdir(), 'orca-runtime-failed-removal-'))) + leftover = join(directory, 'feature') + leftoverId = `${TEST_REPO_ID}::${leftover}` + await mkdir(join(leftover, 'node_modules'), { recursive: true }) + await writeWorktreeRemovalRecords(directory, () => [ + { + worktreeId: leftoverId, + repoId: TEST_REPO_ID, + repoPath: TEST_REPO_PATH, + worktreePath: leftover, + branch: 'feature', + head: 'abc', + deleteBranch: true, + force: true, + requestedAt: 1, + failure: { message: FAILURE, failedAt: 2 } + } + ]) + await loadWorktreeRemovalRecords(directory) + }) + + afterEach(async () => { + _resetPendingWorktreeRemovalsForTests() + await rm(directory, { recursive: true, force: true }) + }) + + it('lists the leftover with its error, though Git no longer does', async () => { + const runtime = createWorktreeRemovalRuntime() + + const detected = await runtime.listDetectedManagedWorktrees(`id:${TEST_REPO_ID}`) + + expect(detected.worktrees.find((row) => row.id === leftoverId)).toMatchObject({ + path: leftover, + removalError: FAILURE + }) + }) + + it('runs the recorded removal again, answering a client that cannot wait on acceptance', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const runtime = createWorktreeRemovalRuntime() + + await expect( + runtime.removeManagedWorktree(`id:${leftoverId}`, { waitForBackgroundRemoval: false }) + ).resolves.toEqual({ removing: true }) + await _settlePendingWorktreeRemovalsForTests() + + // Git has no registration left for it, so Orca deletes the leftover itself. + 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 () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + // A new checkout at the same path: the recorded choices were for the leftover, not for it. + vi.mocked(listWorktreesStrict).mockResolvedValue([ + { + path: leftover, + head: 'def', + branch: 'refs/heads/other', + isBare: false, + isMainWorktree: false + } + ]) + vi.mocked(removeWorktree).mockResolvedValue({}) + const runtime = createWorktreeRemovalRuntime() + + await runtime.removeManagedWorktree(`id:${leftoverId}`, { waitForBackgroundRemoval: true }) + await _settlePendingWorktreeRemovalsForTests() + + expect(removeWorktree).toHaveBeenCalledWith(TEST_REPO_PATH, leftover, false, expect.anything()) + expect(existsSync(join(leftover, 'node_modules'))).toBe(true) + expect(await readWorktreeRemovalRecords(directory)).toEqual([]) + }) + + it('joins a retry another client started while this Delete listed Git', async () => { + const otherClientsRetry = vi.fn(async () => ({})) + vi.mocked(listWorktreesStrict).mockImplementationOnce(async () => { + void retryFailedWorktreeRemoval(leftoverId, 'local', () => ({ + run: otherClientsRetry, + publish: () => {} + })) + return [] + }) + const runtime = createWorktreeRemovalRuntime() + + await expect( + runtime.removeManagedWorktree(`id:${leftoverId}`, { waitForBackgroundRemoval: true }) + ).resolves.toEqual({}) + + expect(otherClientsRetry).toHaveBeenCalledTimes(1) + // Only the joined retry ran: the leftover is still there because its stub deleted nothing. + 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', () => { + let directory = '' + let leftover = '' + let leftoverId = '' + + beforeEach(async () => { + vi.clearAllMocks() + vi.spyOn(console, 'warn').mockImplementation(() => {}) + directory = await realpath(await mkdtemp(join(tmpdir(), 'orca-runtime-failed-listing-'))) + leftover = join(directory, 'feature') + leftoverId = `${TEST_REPO_ID}::${leftover}` + await mkdir(join(leftover, 'node_modules'), { recursive: true }) + await loadWorktreeRemovalRecords(directory) + }) + + afterEach(async () => { + _resetPendingWorktreeRemovalsForTests() + vi.restoreAllMocks() + await rm(directory, { recursive: true, force: true }) + }) + + it('shows the failed row with its error, not the scan cached before the delete', async () => { + const registered = { + path: leftover, + head: 'abc', + branch: 'refs/heads/feature', + isBare: false, + isMainWorktree: false + } + const gitLists = (worktrees: (typeof registered)[]): void => { + vi.mocked(listWorktreesStrict).mockResolvedValue(worktrees) + scanLocalRepoWorktreesForResolutionMock.mockResolvedValue({ ok: true, worktrees }) + } + gitLists([registered]) + const runtime = createWorktreeRemovalRuntime() + const listLeftover = async () => + (await runtime.listDetectedManagedWorktrees(`id:${TEST_REPO_ID}`)).worktrees.find( + (row) => row.id === leftoverId + ) + // Caches Git's registration for the 30 s scan TTL. + expect(await listLeftover()).not.toHaveProperty('removalError') + vi.mocked(removeWorktree).mockImplementation(async () => { + // Git drops the registration, then fails on a file it cannot delete. + gitLists([]) + throw new Error(FAILURE) + }) + + await expect( + runtime.removeManagedWorktree(`id:${leftoverId}`, { + force: true, + waitForBackgroundRemoval: true + }) + ).rejects.toThrow(/Operation not permitted/) + await _settlePendingWorktreeRemovalsForTests() + + expect(await listLeftover()).toMatchObject({ + path: leftover, + removalError: expect.stringMatching(/Operation not permitted/) + }) + }) +}) diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 070363fdb3f..dca1d189599 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -114,6 +114,7 @@ await import('./orca-runtime-tests/worktree-removal-and-reconciliation-part-02.s await import('./orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec') await import('./orca-runtime-tests/worktree-removal-and-reconciliation-part-04.spec') await import('./orca-runtime-tests/worktree-removal-archive-hook-gate.spec') +await import('./orca-runtime-tests/worktree-removal-failed-retry.spec') await import('./orca-runtime-tests/worktree-removal-execution-host.spec') await import('./orca-runtime-tests/targeting-and-resilience.spec') await import('./orca-runtime-tests/worktree-scan-cache-ttl.spec') diff --git a/src/main/runtime/rpc/methods/worktree-catalog-methods.ts b/src/main/runtime/rpc/methods/worktree-catalog-methods.ts index 082d0400f7c..f4f3ba99cae 100644 --- a/src/main/runtime/rpc/methods/worktree-catalog-methods.ts +++ b/src/main/runtime/rpc/methods/worktree-catalog-methods.ts @@ -5,7 +5,7 @@ import { projectWorktreeListRemovals, projectWorktreePsRemovals } from '../worktree-removal-marker-projection' -import { snapshotPendingWorktreeRemovals } from '../../../worktree-background-removal' +import { snapshotPendingWorktreeRemovals } from '../../../worktree-removal-listing' import { WorktreeDetectedListParams, WorktreeListParams, diff --git a/src/main/runtime/rpc/worktree-removal-marker-projection.test.ts b/src/main/runtime/rpc/worktree-removal-marker-projection.test.ts index f2abee689e7..12e4bf0f7a3 100644 --- a/src/main/runtime/rpc/worktree-removal-marker-projection.test.ts +++ b/src/main/runtime/rpc/worktree-removal-marker-projection.test.ts @@ -11,9 +11,9 @@ import type { } from '../../../shared/runtime-worktree-contracts' import { _resetPendingWorktreeRemovalsForTests, - snapshotPendingWorktreeRemovals, startBackgroundWorktreeRemoval } from '../../worktree-background-removal' +import { snapshotPendingWorktreeRemovals } from '../../worktree-removal-listing' import { projectWorktreeListRemovals, projectWorktreePsRemovals diff --git a/src/main/runtime/rpc/worktree-removal-marker-projection.ts b/src/main/runtime/rpc/worktree-removal-marker-projection.ts index f4f13ca9c41..4e1b18a266c 100644 --- a/src/main/runtime/rpc/worktree-removal-marker-projection.ts +++ b/src/main/runtime/rpc/worktree-removal-marker-projection.ts @@ -7,7 +7,7 @@ import type { import { projectPendingWorktreeRemovals, type PendingWorktreeRemovals -} from '../../worktree-background-removal' +} from '../../worktree-removal-listing' import type { RpcContext } from './core' // Why no in-process default: callers without negotiation (the CLI, host-side readers) print or act diff --git a/src/main/runtime/runtime-failed-local-worktree-removal.test.ts b/src/main/runtime/runtime-failed-local-worktree-removal.test.ts new file mode 100644 index 00000000000..2aad80e1091 --- /dev/null +++ b/src/main/runtime/runtime-failed-local-worktree-removal.test.ts @@ -0,0 +1,382 @@ +// Real-Git coverage for a delete Git fails partway: `git worktree remove --force` drops the +// registration even when it cannot delete a file, so Orca must keep the leftover listed and +// retryable itself. macOS only: `chflags uchg` is the portable way to make a file undeletable for +// 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 { 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 { 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, + _settlePendingWorktreeRemovalsForTests, + loadWorktreeRemovalRecords, + resumeInterruptedWorktreeRemovals, + retryFailedWorktreeRemoval, + startBackgroundWorktreeRemoval, + waitForPendingWorktreeRemoval +} from '../worktree-background-removal' +import { withUnregisteredRemovalCheckouts } from '../worktree-removal-listing' +import { + readWorktreeRemovalRecords, + writeWorktreeRemovalRecords, + type WorktreeRemovalRecord +} from '../worktree-removal-records' +import { interruptedLocalWorktreeRemovalJob } from './runtime-interrupted-local-worktree-removal' + +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) } +}) + +const execFileAsync = promisify(execFile) + +let scratchDir = '' +let recordsDir = '' +let repoPath = '' +let worktreePath = '' +let lockedFile = '' +let worktreeId = '' +let repo: Repo + +async function git(args: string[], cwd = repoPath): Promise { + const { stdout } = await execFileAsync('git', args, { cwd }) + return stdout +} + +async function isRegistered(path: string): Promise { + return (await listWorktreesStrict(repoPath)).some((worktree) => + areWorktreePathsEqual(worktree.path, path) + ) +} + +async function setImmutable(on: boolean, path = lockedFile): Promise { + await execFileAsync('chflags', on ? ['uchg', path] : ['-R', 'nouchg', path]) +} + +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 } : {}) })) +} + +function jobHost(purged: string[], stopPtys = vi.fn(async () => {})) { + 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, + acquireWatcherRemoval: async (path: string) => { + const gate = acquireWatcherRemovalGate(path) + return { finish: async () => gate.release() } + }, + closeWatchers: async () => {}, + stopPtys, + preservedBranchCleanup: { preserveHead: (result) => result ?? {}, remember: vi.fn() }, + purge: ({ worktreeId: id }: WorktreeRemovalRecord) => purged.push(id), + onRemoved: () => {}, + publish: () => {} + } satisfies Parameters[1] +} + +/** 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 { + const record: WorktreeRemovalRecord = { + worktreeId, + repoId: repo.id, + repoPath, + worktreePath, + branch: 'feature', + head: (await git(['rev-parse', 'feature'])).trim(), + deleteBranch: true, + force: true, + requestedAt: 1 + } + await writeWorktreeRemovalRecords(recordsDir, () => [record]) + await loadWorktreeRemovalRecords(recordsDir) + const joined = waitForPendingWorktreeRemoval(worktreeId)! + resumeInterruptedWorktreeRemovals((interrupted) => + interruptedLocalWorktreeRemovalJob(interrupted, jobHost(purged)) + ) + const error = await joined.then( + () => undefined, + (reason: unknown) => reason + ) + await _settlePendingWorktreeRemovalsForTests() + return error +} + +/** The same delete in session: the job runs Git's `worktree remove --force`, as Delete's does. */ +async function failInSession(): Promise { + const error = await startBackgroundWorktreeRemoval({ + removal: { + worktreeId, + repoId: repo.id, + repoPath, + worktree: { path: worktreePath, branch: 'refs/heads/feature', head: 'abc' }, + deleteBranch: true, + force: true + }, + run: () => removeWorktree(repoPath, worktreePath, true), + publish: () => {} + }).then( + () => undefined, + (reason: unknown) => reason + ) + await _settlePendingWorktreeRemovalsForTests() + return error +} + +describe.skipIf(process.platform !== 'darwin')('a worktree delete Git fails partway', () => { + beforeEach(async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + scratchDir = await realpath(await mkdtemp(join(tmpdir(), 'orca-failed-removal-'))) + recordsDir = join(scratchDir, 'profile') + repoPath = join(scratchDir, 'repo') + worktreePath = join(scratchDir, 'workspaces', 'feature') + worktreeId = `repo-1::${worktreePath}` + await mkdir(recordsDir, { recursive: true }) + await mkdir(repoPath, { recursive: true }) + await git(['init', '-q']) + await git(['config', 'user.email', 'removal@example.invalid']) + await git(['config', 'user.name', 'Worktree Removal']) + await writeFile(join(repoPath, 'seed.txt'), 'seed\n') + await git(['add', '-A']) + await git(['commit', '-qm', 'seed']) + await git(['worktree', 'add', '-q', worktreePath, '-b', 'feature']) + lockedFile = join(worktreePath, 'node_modules', 'a', 'LICENSE') + await mkdir(join(worktreePath, 'node_modules', 'a'), { recursive: true }) + await writeFile(lockedFile, 'MIT\n') + await setImmutable(true) + repo = { id: 'repo-1', path: repoPath, displayName: 'repo', badgeColor: '', addedAt: 0 } + await loadWorktreeRemovalRecords(recordsDir) + }) + + afterEach(async () => { + _resetPendingWorktreeRemovalsForTests() + vi.mocked(removeHostTree).mockClear() + vi.restoreAllMocks() + await setImmutable(false, scratchDir) + expect((await execFileAsync('find', [scratchDir, '-flags', '+uchg'])).stdout).toBe('') + await removeTree(scratchDir) + }) + + it('keeps the leftover listed with Git’s error after the startup finish fails', async () => { + 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. + expect(await isRegistered(worktreePath)).toBe(false) + expect(existsSync(lockedFile)).toBe(true) + expect(await git(['branch', '--list', 'feature'])).not.toBe('') + expect(await listedRows()).toEqual([ + { path: worktreePath, removalError: expect.stringMatching(/Operation not permitted/) } + ]) + // The leftover is not fenced: terminals may open in it while it waits for the user. + beginTerminalInstall(worktreePath)() + }) + + it('keeps the leftover listed with Git’s error after an in-session delete fails', async () => { + const error = await failInSession() + + expect(String(error)).toMatch(/Operation not permitted/) + expect(await isRegistered(worktreePath)).toBe(false) + expect(await listedRows()).toEqual([ + { path: worktreePath, removalError: expect.stringMatching(/Operation not permitted/) } + ]) + }) + + it('does not retry it at the next start', async () => { + await failStartupFinish() + _resetPendingWorktreeRemovalsForTests() + await loadWorktreeRemovalRecords(recordsDir) + const jobFor = vi.fn() + + resumeInterruptedWorktreeRemovals(jobFor) + + expect(jobFor).not.toHaveBeenCalled() + expect(waitForPendingWorktreeRemoval(worktreeId)).toBeUndefined() + expect(existsSync(lockedFile)).toBe(true) + expect(await listedRows()).toHaveLength(1) + }) + + it('Delete retries it once the file can be deleted: files, branch, metadata and record go', 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(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(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 () => { + await failInSession() + await setImmutable(false) + await rm(worktreePath, { recursive: true }) + + expect(await listedRows()).toEqual([]) + await vi.waitFor(async () => expect(await readWorktreeRemovalRecords(recordsDir)).toEqual([])) + }) + + it('never deletes a different checkout created at the path since', async () => { + await failInSession() + await setImmutable(false) + await rm(worktreePath, { recursive: true }) + await mkdir(worktreePath) + await git(['init', '-q'], worktreePath) + await writeFile(join(worktreePath, 'unsaved.txt'), 'work\n') + const purged: string[] = [] + + // Delete before any listing noticed: the retry 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 _settlePendingWorktreeRemovalsForTests() + + expect(existsSync(join(worktreePath, 'unsaved.txt'))).toBe(true) + expect(removeHostTree).not.toHaveBeenCalled() + expect(purged).toEqual([]) + expect(await readWorktreeRemovalRecords(recordsDir)).toEqual([]) + expect(await listedRows()).toEqual([]) + }) + + it('never deletes a worktree Git registers at the path since, even on the same branch', async () => { + await failStartupFinish() + await setImmutable(false) + await rm(worktreePath, { recursive: true }) + await git(['worktree', 'add', '-q', worktreePath, 'feature']) + await writeFile(join(worktreePath, 'unsaved.txt'), 'work\n') + const purged: string[] = [] + + const retried = retryFailedWorktreeRemoval(worktreeId, 'local', (record) => + interruptedLocalWorktreeRemovalJob(record, jobHost(purged)) + ) + await expect(retried).rejects.toThrow(/A different checkout is now at/) + await _settlePendingWorktreeRemovalsForTests() + + expect(existsSync(join(worktreePath, 'unsaved.txt'))).toBe(true) + expect(await isRegistered(worktreePath)).toBe(true) + expect(await git(['branch', '--list', 'feature'])).not.toBe('') + expect(purged).toEqual([]) + expect(await readWorktreeRemovalRecords(recordsDir)).toEqual([]) + }) + + it('never deletes a worktree Git registers inside the leftover', async () => { + await failInSession() + await setImmutable(false) + const nested = join(worktreePath, 'sub') + await git(['worktree', 'add', '-q', nested, '-b', 'nested']) + await writeFile(join(nested, 'unsaved.txt'), 'work\n') + const purged: string[] = [] + + const retried = retryFailedWorktreeRemoval(worktreeId, 'local', (record) => + interruptedLocalWorktreeRemovalJob(record, jobHost(purged)) + ) + await expect(retried).rejects.toThrow(/contains another registered worktree/) + 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/) + }) + }) + + it('checks the leftover again inside the delete slot, right before deleting', 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) + + expect(String(await settled)).toMatch(/A different checkout is now at/) + await _settlePendingWorktreeRemovalsForTests() + expect(existsSync(join(worktreePath, 'unsaved.txt'))).toBe(true) + expect(removeHostTree).not.toHaveBeenCalled() + }) + + it('at startup, still deletes the recorded checkout when its missing .git cannot be restored', 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(removeHostTree).toHaveBeenCalledWith(worktreePath) + expect(existsSync(worktreePath)).toBe(false) + expect(await isRegistered(worktreePath)).toBe(false) + expect(await git(['branch', '--list', 'feature'])).toBe('') + expect(purged).toEqual([worktreeId]) + }) +}) diff --git a/src/main/runtime/runtime-interrupted-local-worktree-removal.ts b/src/main/runtime/runtime-interrupted-local-worktree-removal.ts index ca50faf3b8f..9f94feac0dc 100644 --- a/src/main/runtime/runtime-interrupted-local-worktree-removal.ts +++ b/src/main/runtime/runtime-interrupted-local-worktree-removal.ts @@ -18,6 +18,11 @@ 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 +} from '../worktree-removal-leftover' import { CLIENT_REMOVAL_HOME } from '../worktree-removal-home-guard' import type { WorktreeRemovalRecord } from '../worktree-removal-records' import { @@ -31,6 +36,8 @@ 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. */ + stopPtys?: () => Promise /** Drops the worktree's host state (metadata, history, caches), as every removal path does. */ purge: (record: WorktreeRemovalRecord) => void onRemoved: (record: WorktreeRemovalRecord) => void @@ -61,6 +68,7 @@ export function interruptedLocalWorktreeRemovalJob( return host.acquireWatcherRemoval(path) }, closeWatchers: host.closeWatchers, + stopPtys: host.stopPtys, preserveBranchHead: (result, fallbackHead) => host.preservedBranchCleanup.preserveHead(result, fallbackHead), // remember() clears the cleanup target when no branch was preserved. @@ -89,6 +97,7 @@ type InterruptedLocalWorktreeRemovalArgs = Pick< store: Store record: WorktreeRemovalRecord acquireWatcherRemoval: (path: string) => Promise<{ finish: (removed: boolean) => Promise }> + stopPtys?: () => Promise stopSignal: AbortSignal } @@ -138,12 +147,14 @@ async function finishInterruptedLocalWorktreeRemoval( } const gitLink = await readCheckoutGitLink(record.worktreePath) // Why: the finish forces, so a checkout created at this path since the quit must not be taken. - // Git deletes the checkout, `.git` included, before it drops the registration, so a `.git` at an - // unregistered path belongs to a new checkout. - if (deletable ? !isRecordedCheckout(deletable, record) : gitLink === 'present') { - throw new Error( - `A different checkout is now at ${record.worktreePath}; Orca left it in place. Delete it again to remove it.` - ) + // 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)) + ) { + 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. @@ -153,6 +164,14 @@ async function finishInterruptedLocalWorktreeRemoval( gitCanRemove = await restoreMissingWorktreeGitFile(repo.path, deletable.path, localOptions) } const gate = await args.acquireWatcherRemoval(record.worktreePath) + if (args.stopPtys) { + try { + await args.stopPtys() + } catch (error) { + await gate.finish(false) + throw error + } + } if (deletable && gitCanRemove) { return finishRuntimeLocalWorktreeRemoval(finishArgs, deletable, gate, args.stopSignal) } @@ -165,6 +184,10 @@ 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), localOptions ) removed = true diff --git a/src/main/runtime/runtime-local-worktree-create-candidate.ts b/src/main/runtime/runtime-local-worktree-create-candidate.ts index 7af0f8b9910..169e62f5a57 100644 --- a/src/main/runtime/runtime-local-worktree-create-candidate.ts +++ b/src/main/runtime/runtime-local-worktree-create-candidate.ts @@ -32,7 +32,7 @@ import { resolveCreateBranchName } from './runtime-worktree-create-git' import { runtimePathExists } from './runtime-worktree-filesystem' -import { findPendingWorktreeRemovalConflict } from '../worktree-background-removal' +import { findPendingWorktreeRemovalConflict } from '../worktree-removal-table' import type { RuntimeStore } from './runtime-store-contract' import type { HostedReviewExecutionOptions } from '../source-control/hosted-review-git-options' diff --git a/src/main/runtime/runtime-worktree-ps-summaries.test.ts b/src/main/runtime/runtime-worktree-ps-summaries.test.ts index a588c7c2f00..91054cd8046 100644 --- a/src/main/runtime/runtime-worktree-ps-summaries.test.ts +++ b/src/main/runtime/runtime-worktree-ps-summaries.test.ts @@ -34,4 +34,37 @@ describe('buildRuntimeWorktreePsSummaries', () => { expect(summary?.hostId).toBe('ssh:persisted-host') }) + + it('carries the error of a failed delete the host still lists', () => { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the builder reads only these fields for a git row. + const worktree = { + id: 'repo-1::/workspace/app', + repoId: 'repo-1', + path: '/workspace/app', + branch: 'feature', + isArchived: false, + isMainWorktree: false, + parentWorktreeId: null, + childWorktreeIds: [], + lineage: null, + lastActivityAt: 0, + removalError: 'Operation not permitted' + } as unknown as ResolvedWorktree + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the builder reads only these store members. + const store = { + getRepos: () => [], + getWorktreeMeta: () => undefined, + getAllWorktreeMeta: () => ({}), + getFolderWorkspaces: () => [], + getProjectGroups: () => [] + } as unknown as RuntimeStore + + const summary = buildRuntimeWorktreePsSummaries({ + store, + resolvedWorktrees: [worktree], + platformByRepoId: new Map() + }).get(worktree.id) + + expect(summary?.removalError).toBe('Operation not permitted') + }) }) diff --git a/src/main/runtime/runtime-worktree-ps-summaries.ts b/src/main/runtime/runtime-worktree-ps-summaries.ts index 2e52a5c29e1..1d5a161a8a5 100644 --- a/src/main/runtime/runtime-worktree-ps-summaries.ts +++ b/src/main/runtime/runtime-worktree-ps-summaries.ts @@ -42,6 +42,7 @@ export function buildRuntimeWorktreePsSummaries(args: { isArchived: worktree.isArchived, isMainWorktree: worktree.isMainWorktree, hasHostSidebarActivity: false, + ...(worktree.removalError ? { removalError: worktree.removalError } : {}), ...(worktree.instanceId !== undefined ? { worktreeInstanceId: worktree.instanceId } : {}), ...(lineage?.worktreeInstanceId !== undefined ? { lineageWorktreeInstanceId: lineage.worktreeInstanceId } diff --git a/src/main/startup/main-process-ready-runtime.ts b/src/main/startup/main-process-ready-runtime.ts index f3ea135b6a7..41a422c3cfd 100644 --- a/src/main/startup/main-process-ready-runtime.ts +++ b/src/main/startup/main-process-ready-runtime.ts @@ -32,7 +32,7 @@ import { import { initializeMainProcessAutomations } from './main-process-automations' import { initializeMainProcessPlugins } from './main-process-plugins' import { collectWorktreeTrashSweepRoots, sweepStaleWorktreeTrash } from '../worktree-trash' -import { loadWorktreeRemovalRecords } from '../worktree-background-removal' +import { loadWorktreeRemovalRecordsForStore } from './worktree-removal-records-load' import { runAfterFirstWindowShown } from './first-window-deferral' import { logStartupMilestone } from './startup-diagnostics' import { refreshInstalledOpenCodeStatusPlugins } from '../opencode/opencode-status-plugin-startup-refresh' @@ -46,7 +46,7 @@ export async function initializeReadyRuntimeServices(): Promise { throw new Error('Store must be initialized before ready services') } // Why before any listing: a delete a quit or crash interrupted must show as Deleting from first paint. - await loadWorktreeRemovalRecords(store.getProfileStorageDirectory()) + await loadWorktreeRemovalRecordsForStore(store) initializeMainProcessObservers() initializeMainProcessAccountServices() const runtime = initializeMainProcessRuntime() diff --git a/src/main/startup/worktree-removal-records-load.ts b/src/main/startup/worktree-removal-records-load.ts new file mode 100644 index 00000000000..5d988baaf6c --- /dev/null +++ b/src/main/startup/worktree-removal-records-load.ts @@ -0,0 +1,20 @@ +import { getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID } from '../../shared/execution-host' +import type { Repo } from '../../shared/repo-types' +import { loadWorktreeRemovalRecords } from '../worktree-background-removal' + +/** + * Loads this host's removal records at startup. A failed delete is kept only while its repo's LOCAL + * copy is in Orca: only local listings show the row, and an SSH copy under the same id never would. + */ +export function loadWorktreeRemovalRecordsForStore(store: { + getProfileStorageDirectory: () => string + getRepos: () => readonly Pick[] +}): Promise { + return loadWorktreeRemovalRecords(store.getProfileStorageDirectory(), (repoId) => + store + .getRepos() + .some( + (repo) => repo.id === repoId && getRepoExecutionHostId(repo) === LOCAL_EXECUTION_HOST_ID + ) + ) +} diff --git a/src/main/worktree-background-removal-records.test.ts b/src/main/worktree-background-removal-records.test.ts index 6707acfe271..726240fefb1 100644 --- a/src/main/worktree-background-removal-records.test.ts +++ b/src/main/worktree-background-removal-records.test.ts @@ -6,13 +6,15 @@ import { _resetPendingWorktreeRemovalsForTests, _settlePendingWorktreeRemovalsForTests, loadWorktreeRemovalRecords, - projectPendingWorktreeRemovals, resumeInterruptedWorktreeRemovals, - snapshotPendingWorktreeRemovals, startBackgroundWorktreeRemoval, stopBackgroundWorktreeRemovals, waitForPendingWorktreeRemoval } from './worktree-background-removal' +import { + projectPendingWorktreeRemovals, + snapshotPendingWorktreeRemovals +} from './worktree-removal-listing' import type * as WorktreeRemovalRecords from './worktree-removal-records' import { readWorktreeRemovalRecords, diff --git a/src/main/worktree-background-removal.test.ts b/src/main/worktree-background-removal.test.ts index 2ddc1952e8e..1517954f25c 100644 --- a/src/main/worktree-background-removal.test.ts +++ b/src/main/worktree-background-removal.test.ts @@ -3,14 +3,16 @@ import type { ExecutionHostId } from '../shared/execution-host' import { _resetPendingWorktreeRemovalsForTests, _settlePendingWorktreeRemovalsForTests, - assertNoPendingWorktreeRemovalConflict, finishAcceptedWorktreeRemoval, - projectPendingWorktreeRemovals, removesInBackground, - snapshotPendingWorktreeRemovals, startBackgroundWorktreeRemoval, waitForPendingWorktreeRemoval } from './worktree-background-removal' +import { + projectPendingWorktreeRemovals, + snapshotPendingWorktreeRemovals +} from './worktree-removal-listing' +import { assertNoPendingWorktreeRemovalConflict } from './worktree-removal-table' const removal = { worktreeId: 'repo-1::/work/feature', diff --git a/src/main/worktree-background-removal.ts b/src/main/worktree-background-removal.ts index be6bb365d5a..700ecd045ca 100644 --- a/src/main/worktree-background-removal.ts +++ b/src/main/worktree-background-removal.ts @@ -2,14 +2,23 @@ import { LOCAL_EXECUTION_HOST_ID, type ExecutionHostId } from '../shared/executi import type { RemoveWorktreeResult } from '../shared/worktree/create-types' import type { GitWorktreeInfo } from '../shared/worktree/types' import { normalizeLocalBranchRef } from './git/worktree-operation-options' -import { areWorktreePathsEqual } from './git/worktree-path-comparison' import { acquireWatcherRemovalGate, type WatcherRemovalGate } from './ipc/watcher-removal-gate' +import { runWorktreeChangeInvalidators } from './ipc/worktree-change-invalidators' import { parseWslPath } from './wsl' +import { readWorktreeRemovalRecords, type WorktreeRemovalRecord } from './worktree-removal-records' import { - readWorktreeRemovalRecords, - writeWorktreeRemovalRecords, - type WorktreeRemovalRecord -} from './worktree-removal-records' + differentCheckoutAtPathError, + isCheckoutRegistered, + isUnregisteredRemovalLeftover +} from './worktree-removal-leftover' +import { + failedWorktreeRemovals, + finishedWorktreeRemovals, + pendingWorktreeRemovals, + persistWorktreeRemovalRecords, + setWorktreeRemovalRecordsDirectory, + worktreeCheckoutExists +} from './worktree-removal-table' export type BackgroundWorktreeRemovalJob = { /** `stopSignal` aborts on an orderly quit; pass it only to the checkout delete. */ @@ -18,36 +27,40 @@ export type BackgroundWorktreeRemovalJob = { publish: () => void } -/** The removals pending when a listing began to read Git. */ -export type PendingWorktreeRemovals = ReadonlyMap - type RemovalSettlement = { result: Promise resolve: (result: RemoveWorktreeResult) => void reject: (error: unknown) => void } -// The accepted removals, mirrored to disk on every change; listings and joins read only this. -const pendingByWorktreeId = new Map() const jobsByWorktreeId = new Map>() // What every request for a pending removal waits on: the first one and any that join it. const settlementsByWorktreeId = new Map() const stopControllers = new Set() // Loaded removals' terminal/watcher fences, held until the resumed job takes its own gate. const startupFencesByWorktreeId = new Map() -// Why weak: a listing that read Git before a delete finished holds the record until it replies. -const removedRecords = new WeakSet() -const NO_PENDING_REMOVALS: PendingWorktreeRemovals = new Map() -let recordsDirectory: string | null = null /** * Loads removals a quit or crash interrupted, so listings mark them before the first paint and * session restore cannot open a terminal or watcher in a half-deleted checkout before the resume. */ -export async function loadWorktreeRemovalRecords(directory: string): Promise { - recordsDirectory = directory +export async function loadWorktreeRemovalRecords( + directory: string, + hasRepo: (repoId: string) => boolean = () => true +): Promise { + setWorktreeRemovalRecordsDirectory(directory) + let droppedFailure = false for (const record of await readWorktreeRemovalRecords(directory)) { - if (!pendingByWorktreeId.has(record.worktreeId)) { + if (record.failure) { + // Why the repo: only its listing shows the row, so nothing else could end a removed repo's. + if (hasRepo(record.repoId) && (await worktreeCheckoutExists(record.worktreePath))) { + failedWorktreeRemovals.set(record.worktreeId, record) + } else { + droppedFailure = true + } + continue + } + if (!pendingWorktreeRemovals.has(record.worktreeId)) { addPendingRemoval(record) try { startupFencesByWorktreeId.set( @@ -59,6 +72,9 @@ export async function loadWorktreeRemovalRecords(directory: string): Promise {}) const settlement = { result, resolve, reject } - pendingByWorktreeId.set(record.worktreeId, record) + // A new removal of the same workspace supersedes its failed one. + failedWorktreeRemovals.delete(record.worktreeId) + pendingWorktreeRemovals.set(record.worktreeId, record) settlementsByWorktreeId.set(record.worktreeId, settlement) return settlement } -function persistRecords(): Promise { - if (!recordsDirectory) { - return Promise.resolve() - } - return writeWorktreeRemovalRecords(recordsDirectory, () => [ - ...pendingByWorktreeId.values() - ]).catch((error: unknown) => { - // Why: bookkeeping must not gate the delete; a lost write only costs resuming it after a quit. - console.warn('[worktrees] failed to persist worktree removal records', error) - }) -} - /** * The result of the removal this host is running for the worktree, for a request that joins it. * Only this host's local checkouts are removed in the background. @@ -126,41 +132,6 @@ export function removesInBackground( return !options.wslDistro && !parseWslPath(worktreePath) } -export function hasPendingWorktreeRemovals(): boolean { - return pendingByWorktreeId.size > 0 -} - -export function findPendingWorktreeRemovalConflict( - repoPath: string, - target: { worktreePath?: string; branch?: string } -): WorktreeRemovalRecord | undefined { - const branch = target.branch?.replace(/^refs\/heads\//, '') - for (const removal of pendingByWorktreeId.values()) { - if (!areWorktreePathsEqual(removal.repoPath, repoPath)) { - continue - } - if ( - (target.worktreePath && areWorktreePathsEqual(removal.worktreePath, target.worktreePath)) || - (branch && removal.branch === branch) - ) { - return removal - } - } - return undefined -} - -export function assertNoPendingWorktreeRemovalConflict( - repoPath: string, - target: { worktreePath?: string; branch?: string } -): void { - const removal = findPendingWorktreeRemovalConflict(repoPath, target) - if (removal) { - throw new Error( - `Orca is still deleting the workspace at ${removal.worktreePath}. Cleanup is pending; try again shortly.` - ) - } -} - /** * Records an accepted removal and runs its delete detached from the request that asked for it, so * the delete finishes even when that request times out or its client goes away. Resolves with the @@ -183,16 +154,52 @@ export function startBackgroundWorktreeRemoval( requestedAt: Date.now() } const settlement = addPendingRemoval(record) - runBackgroundWorktreeRemoval(record, args, persistRecords()) + runBackgroundWorktreeRemoval(record, args, persistWorktreeRemovalRecords()) publishSafely(args.publish) return settlement.result } +/** + * 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. + */ +export function retryFailedWorktreeRemoval( + worktreeId: string, + hostId: ExecutionHostId | undefined, + jobFor: (record: WorktreeRemovalRecord) => BackgroundWorktreeRemovalJob +): Promise | undefined { + const failed = + (hostId ?? LOCAL_EXECUTION_HOST_ID) === LOCAL_EXECUTION_HOST_ID + ? failedWorktreeRemovals.get(worktreeId) + : undefined + if (!failed) { + return waitForPendingWorktreeRemoval(worktreeId, hostId) + } + const { failure: _failure, ...record } = failed + const settlement = addPendingRemoval(record) + const job = jobFor(record) + const leftoverOnly: BackgroundWorktreeRemovalJob = { + ...job, + run: async (stopSignal) => { + // Why: the recorded choices (force, branch) were for the leftover; a checkout Git registered + // at the path after the caller listed is a new one, which only the normal delete may remove. + if (await isCheckoutRegistered(record)) { + throw differentCheckoutAtPathError(record.worktreePath) + } + return job.run(stopSignal) + } + } + runBackgroundWorktreeRemoval(record, leftoverOnly, persistWorktreeRemovalRecords()) + publishSafely(job.publish) + return settlement.result +} + /** Runs the same delete again for every record a quit or crash left without a running job. */ export function resumeInterruptedWorktreeRemovals( jobFor: (record: WorktreeRemovalRecord) => BackgroundWorktreeRemovalJob ): void { - for (const record of pendingByWorktreeId.values()) { + for (const record of pendingWorktreeRemovals.values()) { if (!jobsByWorktreeId.has(record.worktreeId)) { runBackgroundWorktreeRemoval(record, jobFor(record), Promise.resolve()) } @@ -237,12 +244,13 @@ async function settleBackgroundWorktreeRemoval( ): Promise { await waitForRecordWrite(record, recorded) let settle: (settlement: RemovalSettlement) => void + let failure: WorktreeRemovalRecord['failure'] try { if (stopSignal.aborted) { return } const result = await job.run(stopSignal) - removedRecords.add(record) + finishedWorktreeRemovals.add(record) settle = (settlement) => settlement.resolve(result) } catch (error) { if (stopSignal.aborted) { @@ -251,15 +259,29 @@ async function settleBackgroundWorktreeRemoval( } console.warn(`[worktrees] background removal of ${record.worktreePath} failed`, error) 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() + } + } } finally { // A resumed job that ended before taking its own gate still holds the fence loading gave it. releaseStartupRemovalFence(record.worktreeId) } - // Why clear on failure too: the row returns live and retryable instead of retrying unseen. - const cleared = pendingByWorktreeId.get(record.worktreeId) === record + // Why clear on failure too: the row returns with its error and Delete retries it; nothing + // retries unseen. + const cleared = pendingWorktreeRemovals.get(record.worktreeId) === record if (cleared) { - pendingByWorktreeId.delete(record.worktreeId) + pendingWorktreeRemovals.delete(record.worktreeId) settlementsByWorktreeId.delete(record.worktreeId) + if (failure) { + failedWorktreeRemovals.set(record.worktreeId, { ...record, failure }) + // Git's catalog changed under a failed delete; cached scans still list the checkout. + runWorktreeChangeInvalidators(record.repoId) + } } // Why this run's own settlement: desktop IPC and runtime RPC coalesce separately, so a concurrent // removal can replace the record, and the request waiting on this delete must still get its reply. @@ -270,7 +292,23 @@ async function settleBackgroundWorktreeRemoval( // re-runs a finish that re-derives what is left from Git. publishSafely(job.publish) if (cleared) { - await persistRecords() + await persistWorktreeRemovalRecords() + } +} + +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 } } @@ -306,45 +344,6 @@ function publishSafely(publish: () => void): void { } } -/** Taken before a listing reads Git; pass it to projectPendingWorktreeRemovals with the rows. */ -export function snapshotPendingWorktreeRemovals(): PendingWorktreeRemovals { - return pendingByWorktreeId.size === 0 ? NO_PENDING_REMOVALS : new Map(pendingByWorktreeId) -} - -/** - * Marks rows whose checkout this host is deleting, or leaves them out for a client that cannot - * read the marker: such a client already dropped the row on acceptance and would re-show it. - */ -export function projectPendingWorktreeRemovals< - T extends { hostId?: ExecutionHostId; removing?: true } ->( - rows: T[], - idOf: (row: T) => string, - clientReadsMarker: boolean, - pendingAtScan: PendingWorktreeRemovals -): T[] { - if (pendingByWorktreeId.size === 0 && pendingAtScan.size === 0) { - return rows - } - const projected: T[] = [] - for (const row of rows) { - const id = idOf(row) - const local = row.hostId === undefined || row.hostId === LOCAL_EXECUTION_HOST_ID - if (local && pendingByWorktreeId.has(id)) { - if (clientReadsMarker) { - projected.push({ ...row, removing: true }) - } - continue - } - const scanned = local ? pendingAtScan.get(id) : undefined - // Why: Git was read before this delete finished; unmarked, the gone row reads as a failed delete. - if (!scanned || !removedRecords.has(scanned)) { - projected.push(row) - } - } - return projected -} - export async function _settlePendingWorktreeRemovalsForTests(): Promise { while (jobsByWorktreeId.size > 0) { await Promise.all(jobsByWorktreeId.values()) @@ -352,7 +351,8 @@ export async function _settlePendingWorktreeRemovalsForTests(): Promise { } export function _resetPendingWorktreeRemovalsForTests(): void { - pendingByWorktreeId.clear() + pendingWorktreeRemovals.clear() + failedWorktreeRemovals.clear() jobsByWorktreeId.clear() settlementsByWorktreeId.clear() stopControllers.clear() @@ -360,5 +360,5 @@ export function _resetPendingWorktreeRemovalsForTests(): void { fence.release() } startupFencesByWorktreeId.clear() - recordsDirectory = null + setWorktreeRemovalRecordsDirectory(null) } diff --git a/src/main/worktree-failed-removal.test.ts b/src/main/worktree-failed-removal.test.ts new file mode 100644 index 00000000000..f626e7bd327 --- /dev/null +++ b/src/main/worktree-failed-removal.test.ts @@ -0,0 +1,293 @@ +// 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 +// 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' +import { tmpdir } from 'node:os' +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 { beginTerminalInstall } from './ipc/watcher-removal-gate' +import { registerWorktreeChangeInvalidator } from './ipc/worktree-change-invalidators' +import { + _resetPendingWorktreeRemovalsForTests, + _settlePendingWorktreeRemovalsForTests, + loadWorktreeRemovalRecords, + resumeInterruptedWorktreeRemovals, + retryFailedWorktreeRemoval, + startBackgroundWorktreeRemoval, + waitForPendingWorktreeRemoval +} from './worktree-background-removal' +import { + projectPendingWorktreeRemovals, + snapshotPendingWorktreeRemovals, + withUnregisteredRemovalCheckouts +} from './worktree-removal-listing' +import { readWorktreeRemovalRecords } from './worktree-removal-records' +import { loadWorktreeRemovalRecordsForStore } from './startup/worktree-removal-records-load' + +vi.mock('./git/worktree', () => ({ listWorktreesStrict: vi.fn(async () => []) })) + +const GIT_ERROR = "error: failed to delete 'node_modules/a/LICENSE': Operation not permitted" +let directory = '' +let checkout = '' +let worktreeId = '' +const mainWorktree: GitWorktreeInfo = { + path: '/work/repo', + head: 'abc', + branch: 'refs/heads/main', + isBare: false, + isMainWorktree: true +} + +beforeEach(async () => { + directory = await realpath(await mkdtemp(join(tmpdir(), 'orca-failed-removal-'))) + checkout = join(directory, 'feature') + worktreeId = `repo-1::${checkout}` + // What Git left: part of the checkout, `.git` already deleted. + await mkdir(join(checkout, 'node_modules', 'a'), { recursive: true }) + await writeFile(join(checkout, 'node_modules', 'a', 'LICENSE'), 'MIT\n') + await mkdir(join(directory, 'profile')) + await loadWorktreeRemovalRecords(join(directory, 'profile')) + vi.mocked(listWorktreesStrict).mockResolvedValue([mainWorktree]) + vi.spyOn(console, 'warn').mockImplementation(() => {}) +}) + +afterEach(async () => { + _resetPendingWorktreeRemovalsForTests() + vi.restoreAllMocks() + await rm(directory, { recursive: true, force: true }) +}) + +function startFailingRemoval(): Promise { + return startBackgroundWorktreeRemoval({ + removal: { + worktreeId, + repoId: 'repo-1', + repoPath: '/work/repo', + worktree: { path: checkout, branch: 'refs/heads/feature', head: 'abc' }, + deleteBranch: true, + force: true + }, + run: async () => { + throw new Error(GIT_ERROR) + }, + publish: () => {} + }) +} + +async function failRemoval(): Promise { + await expect(startFailingRemoval()).rejects.toThrow(GIT_ERROR) + await _settlePendingWorktreeRemovalsForTests() +} + +async function listRows(): Promise { + return withUnregisteredRemovalCheckouts('repo-1', [mainWorktree]) +} + +const leftoverRow = (): GitWorktreeInfo => ({ + path: checkout, + head: 'abc', + branch: 'refs/heads/feature', + isBare: false, + isMainWorktree: false, + removalError: GIT_ERROR +}) + +describe('a delete that fails after Git dropped the registration', () => { + it('keeps the leftover listed with the error, recorded on disk, and not pending', async () => { + await failRemoval() + + expect(await listRows()).toEqual([mainWorktree, leftoverRow()]) + const [record] = await readWorktreeRemovalRecords(join(directory, 'profile')) + expect(record).toMatchObject({ worktreeId, failure: { message: GIT_ERROR } }) + expect(waitForPendingWorktreeRemoval(worktreeId)).toBeUndefined() + // Not marked removing and not left out for older clients: it is a row they can delete again. + const rows: { id: string; hostId?: undefined }[] = [{ id: worktreeId }] + expect( + projectPendingWorktreeRemovals( + rows, + (row) => row.id, + false, + snapshotPendingWorktreeRemovals() + ) + ).toEqual(rows) + // Nothing fences the leftover: a failed delete must not block terminals indefinitely. + beginTerminalInstall(checkout)() + }) + + it('invalidates cached listings, which still hold the registration Git dropped', async () => { + const invalidated = vi.fn() + const unregister = registerWorktreeChangeInvalidator(invalidated) + await failRemoval() + unregister() + + expect(invalidated).toHaveBeenCalledWith('repo-1') + }) + + it('clears the record as before when Git still registers the checkout', async () => { + vi.mocked(listWorktreesStrict).mockResolvedValue([ + mainWorktree, + { ...leftoverRow(), removalError: undefined } + ]) + await failRemoval() + + expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([]) + }) + + it('clears the record as before when the checkout is gone', async () => { + await rm(checkout, { recursive: true }) + await failRemoval() + + expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([]) + }) + + it('never retries it on its own, at startup or when interrupted removals resume', async () => { + await failRemoval() + _resetPendingWorktreeRemovalsForTests() + await loadWorktreeRemovalRecords(join(directory, 'profile')) + const jobFor = vi.fn() + + resumeInterruptedWorktreeRemovals(jobFor) + + expect(jobFor).not.toHaveBeenCalled() + expect(waitForPendingWorktreeRemoval(worktreeId)).toBeUndefined() + expect(await listRows()).toEqual([mainWorktree, leftoverRow()]) + beginTerminalInstall(checkout)() + }) + + it('runs the recorded removal again on Delete and clears the record once it succeeds', async () => { + await failRemoval() + const publish = vi.fn() + const run = vi.fn(async () => { + // The retry shows as removing while it runs. + expect(await listRows()).toEqual([ + mainWorktree, + { ...leftoverRow(), removalError: undefined } + ]) + await rm(checkout, { recursive: true }) + return {} + }) + + const retried = retryFailedWorktreeRemoval(worktreeId, 'local', (record) => { + // The user's first choices, without the failure. + expect(record).toMatchObject({ deleteBranch: true, force: true }) + expect(record).not.toHaveProperty('failure') + return { run, publish } + }) + + // A second window's Delete joins the same run. + expect(waitForPendingWorktreeRemoval(worktreeId)).toBe(retried) + await expect(retried).resolves.toEqual({}) + await _settlePendingWorktreeRemovalsForTests() + expect(run).toHaveBeenCalledTimes(1) + expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([]) + expect(await listRows()).toEqual([mainWorktree]) + expect(retryFailedWorktreeRemoval(worktreeId, 'local', vi.fn())).toBeUndefined() + }) + + it('keeps the row with the new error when the retry fails the same way', async () => { + await failRemoval() + const retried = retryFailedWorktreeRemoval(worktreeId, undefined, () => ({ + run: async () => { + throw new Error('still not permitted') + }, + publish: () => {} + })) + + await expect(retried).rejects.toThrow('still not permitted') + await _settlePendingWorktreeRemovalsForTests() + expect(await listRows()).toEqual([ + mainWorktree, + { ...leftoverRow(), removalError: 'still not permitted' } + ]) + }) + + it('does not run the recorded removal once Git registers a checkout at the path again', async () => { + await failRemoval() + vi.mocked(listWorktreesStrict).mockResolvedValue([ + mainWorktree, + { ...leftoverRow(), removalError: undefined } + ]) + const run = vi.fn(async () => ({})) + + const retried = retryFailedWorktreeRemoval(worktreeId, 'local', () => ({ + run, + publish: () => {} + })) + + await expect(retried).rejects.toThrow(/A different checkout is now at/) + await _settlePendingWorktreeRemovalsForTests() + expect(run).not.toHaveBeenCalled() + expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([]) + }) + + it('is not retried for another host', async () => { + await failRemoval() + + expect(retryFailedWorktreeRemoval(worktreeId, 'ssh:box', vi.fn())).toBeUndefined() + }) + + it('ends at the next listing once the checkout is deleted outside Orca', async () => { + await failRemoval() + await rm(checkout, { recursive: true }) + + expect(await listRows()).toEqual([mainWorktree]) + await vi.waitFor(async () => + expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([]) + ) + }) + + it('ends at startup once the checkout is deleted outside Orca', async () => { + await failRemoval() + _resetPendingWorktreeRemovalsForTests() + await rm(checkout, { recursive: true }) + + await loadWorktreeRemovalRecords(join(directory, 'profile')) + + expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([]) + }) + + it('ends at startup once its repo is removed from Orca, leaving the files', async () => { + await failRemoval() + _resetPendingWorktreeRemovalsForTests() + + // Only an SSH copy of the project is left under the same repo id. + await loadWorktreeRemovalRecordsForStore({ + getProfileStorageDirectory: () => join(directory, 'profile'), + getRepos: () => [{ id: 'repo-1', connectionId: 'box', executionHostId: null }] + }) + + expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([]) + expect(retryFailedWorktreeRemoval(worktreeId, 'local', vi.fn())).toBeUndefined() + expect(await readdir(checkout)).toEqual(['node_modules']) + }) + + it('is kept at startup while the repo’s local copy is still in Orca', async () => { + await failRemoval() + _resetPendingWorktreeRemovalsForTests() + + await loadWorktreeRemovalRecordsForStore({ + getProfileStorageDirectory: () => join(directory, 'profile'), + getRepos: () => [ + { id: 'repo-1', connectionId: 'box', executionHostId: null }, + { id: 'repo-1', connectionId: null, executionHostId: null } + ] + }) + + expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toHaveLength(1) + expect(await listRows()).toHaveLength(2) + }) + + it('ends at the next listing once a different checkout takes the path', async () => { + await failRemoval() + await mkdir(join(checkout, '.git')) + + expect(await listRows()).toEqual([mainWorktree]) + expect(retryFailedWorktreeRemoval(worktreeId, 'local', vi.fn())).toBeUndefined() + await vi.waitFor(async () => + expect(await readWorktreeRemovalRecords(join(directory, 'profile'))).toEqual([]) + ) + }) +}) diff --git a/src/main/worktree-removal-leftover.ts b/src/main/worktree-removal-leftover.ts new file mode 100644 index 00000000000..3e17e415ee3 --- /dev/null +++ b/src/main/worktree-removal-leftover.ts @@ -0,0 +1,64 @@ +import { lstat } from 'node:fs/promises' +import { join } from 'node:path' +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 { + 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: + * no `.git` (Git deleted it first), or a `.git` file naming the admin entry Git removed. Any other + * `.git` is a different checkout created at the path since. + */ +export async function isUnregisteredRemovalLeftover( + repoPath: string, + worktreePath: string +): Promise { + try { + await lstat(join(worktreePath, '.git')) + } catch (error) { + return getErrorCode(error) === 'ENOENT' + } + return canSafelyRemoveOrphanedWorktreeDirectory(worktreePath, repoPath, CLIENT_REMOVAL_HOME) +} + +/** The refusal when the path no longer holds the removed checkout's own leftover. */ +export function differentCheckoutAtPathError(worktreePath: string): Error { + return new Error( + `A different checkout is now at ${worktreePath}; Orca left it in place. Delete it again to remove it.` + ) +} + +/** Whether Git registers a checkout at the recorded path now. */ +export async function isCheckoutRegistered(record: { + repoPath: string + worktreePath: string +}): Promise { + return (await listWorktreesStrict(record.repoPath)).some((worktree) => + areWorktreePathsEqual(worktree.path, record.worktreePath) + ) +} + +/** + * 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. + */ +export async function assertUnregisteredRemovalLeftover( + repoPath: string, + worktreePath: string, + options: GitWorktreeExecOptions = {} +): Promise { + const worktrees = await listWorktreesStrict(repoPath, options) + if (worktrees.some((worktree) => areWorktreePathsEqual(worktree.path, worktreePath))) { + throw differentCheckoutAtPathError(worktreePath) + } + assertWorktreeDoesNotContainRegisteredWorktree(worktreePath, worktrees) + if (!(await isUnregisteredRemovalLeftover(repoPath, worktreePath))) { + throw differentCheckoutAtPathError(worktreePath) + } +} diff --git a/src/main/worktree-removal-listing.ts b/src/main/worktree-removal-listing.ts new file mode 100644 index 00000000000..fd4dd7bfce9 --- /dev/null +++ b/src/main/worktree-removal-listing.ts @@ -0,0 +1,100 @@ +import { LOCAL_EXECUTION_HOST_ID, type ExecutionHostId } from '../shared/execution-host' +import type { GitWorktreeInfo } from '../shared/worktree/types' +import { areWorktreePathsEqual } from './git/worktree-path-comparison' +import { isUnregisteredRemovalLeftover } from './worktree-removal-leftover' +import type { WorktreeRemovalRecord } from './worktree-removal-records' +import { + failedWorktreeRemovals, + finishedWorktreeRemovals, + pendingWorktreeRemovals, + persistWorktreeRemovalRecords, + worktreeCheckoutExists +} from './worktree-removal-table' + +/** The removals pending when a listing began to read Git. */ +export type PendingWorktreeRemovals = ReadonlyMap + +const NO_PENDING_REMOVALS: PendingWorktreeRemovals = new Map() + +/** + * Git's rows for a local repo plus one for each removal this host still owns whose checkout Git no + * longer lists but is still on disk: a failed delete (carrying its error) or one still finishing. + * A failed delete ends here once its checkout is gone or a different checkout took the path. + */ +export async function withUnregisteredRemovalCheckouts( + repoId: string, + gitWorktrees: GitWorktreeInfo[] +): Promise { + const unlisted = [...pendingWorktreeRemovals.values(), ...failedWorktreeRemovals.values()].filter( + (record) => + record.repoId === repoId && + !gitWorktrees.some((worktree) => areWorktreePathsEqual(worktree.path, record.worktreePath)) + ) + if (unlisted.length === 0) { + return gitWorktrees + } + const leftovers: GitWorktreeInfo[] = [] + let droppedFailure = false + for (const record of unlisted) { + const failed = failedWorktreeRemovals.get(record.worktreeId) === record + if ( + (await worktreeCheckoutExists(record.worktreePath)) && + (!failed || (await isUnregisteredRemovalLeftover(record.repoPath, record.worktreePath))) + ) { + leftovers.push({ + path: record.worktreePath, + head: record.head, + branch: record.branch ? `refs/heads/${record.branch}` : '', + isBare: false, + isMainWorktree: false, + ...(record.failure ? { removalError: record.failure.message } : {}) + }) + } else if (failed) { + failedWorktreeRemovals.delete(record.worktreeId) + droppedFailure = true + } + } + if (droppedFailure) { + void persistWorktreeRemovalRecords() + } + return leftovers.length === 0 ? gitWorktrees : [...gitWorktrees, ...leftovers] +} + +/** Taken before a listing reads Git; pass it to projectPendingWorktreeRemovals with the rows. */ +export function snapshotPendingWorktreeRemovals(): PendingWorktreeRemovals { + return pendingWorktreeRemovals.size === 0 ? NO_PENDING_REMOVALS : new Map(pendingWorktreeRemovals) +} + +/** + * Marks rows whose checkout this host is deleting, or leaves them out for a client that cannot + * read the marker: such a client already dropped the row on acceptance and would re-show it. + */ +export function projectPendingWorktreeRemovals< + T extends { hostId?: ExecutionHostId; removing?: true } +>( + rows: T[], + idOf: (row: T) => string, + clientReadsMarker: boolean, + pendingAtScan: PendingWorktreeRemovals +): T[] { + if (pendingWorktreeRemovals.size === 0 && pendingAtScan.size === 0) { + return rows + } + const projected: T[] = [] + for (const row of rows) { + const id = idOf(row) + const local = row.hostId === undefined || row.hostId === LOCAL_EXECUTION_HOST_ID + if (local && pendingWorktreeRemovals.has(id)) { + if (clientReadsMarker) { + projected.push({ ...row, removing: true }) + } + continue + } + const scanned = local ? pendingAtScan.get(id) : undefined + // Why: Git was read before this delete finished; unmarked, the gone row reads as a failed delete. + if (!scanned || !finishedWorktreeRemovals.has(scanned)) { + projected.push(row) + } + } + return projected +} diff --git a/src/main/worktree-removal-records.ts b/src/main/worktree-removal-records.ts index 1fd3558d232..b4e7dbdb7fa 100644 --- a/src/main/worktree-removal-records.ts +++ b/src/main/worktree-removal-records.ts @@ -24,6 +24,13 @@ export type WorktreeRemovalRecord = { deleteBranch: boolean force: boolean requestedAt: number + /** The delete failed after Git dropped the registration with the checkout still on disk. */ + failure?: WorktreeRemovalFailure +} + +export type WorktreeRemovalFailure = { + message: string + failedAt: number } type PersistedWorktreeRemovalRecords = { @@ -35,8 +42,19 @@ function isRecord(value: unknown): value is Record { return typeof value === 'object' && value !== null && !Array.isArray(value) } +function parseFailure(value: unknown): WorktreeRemovalFailure | null | undefined { + if (value === undefined) { + return undefined + } + return isRecord(value) && typeof value.message === 'string' && typeof value.failedAt === 'number' + ? { message: value.message, failedAt: value.failedAt } + : null +} + function parseRecord(value: unknown): WorktreeRemovalRecord | null { + const failure = isRecord(value) ? parseFailure(value.failure) : null if ( + failure === null || !isRecord(value) || typeof value.worktreeId !== 'string' || typeof value.repoId !== 'string' || @@ -59,7 +77,8 @@ function parseRecord(value: unknown): WorktreeRemovalRecord | null { head: value.head, deleteBranch: value.deleteBranch, force: value.force, - requestedAt: value.requestedAt + requestedAt: value.requestedAt, + ...(failure ? { failure } : {}) } } diff --git a/src/main/worktree-removal-table.ts b/src/main/worktree-removal-table.ts new file mode 100644 index 00000000000..8c901bc9e37 --- /dev/null +++ b/src/main/worktree-removal-table.ts @@ -0,0 +1,99 @@ +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. +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() +let recordsDirectory: string | null = null + +export function setWorktreeRemovalRecordsDirectory(directory: string | null): void { + recordsDirectory = directory +} + +export function persistWorktreeRemovalRecords(): Promise { + if (!recordsDirectory) { + return Promise.resolve() + } + return writeWorktreeRemovalRecords(recordsDirectory, () => [ + ...pendingWorktreeRemovals.values(), + ...failedWorktreeRemovals.values() + ]).catch((error: unknown) => { + // Why: bookkeeping must not gate the delete; a lost write only costs resuming it after a quit. + console.warn('[worktrees] failed to persist worktree removal records', error) + }) +} + +/** Unreadable counts as present: only a checkout proven gone ends a failed delete. */ +export async function worktreeCheckoutExists(worktreePath: string): Promise { + try { + await lstat(worktreePath) + return true + } catch (error) { + const code = getErrorCode(error) + return code !== 'ENOENT' && code !== 'ENOTDIR' + } +} + +/** + * 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 a new one, so the failed record is dropped and the + * normal delete runs; while Git does not, `retry` runs or joins the recorded removal. True then. + */ +export function retryFailedRemovalUnlessRegistered( + worktreeId: string, + worktreePath: string, + registeredWorktrees: readonly Pick[], + 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 +} + +export function findPendingWorktreeRemovalConflict( + repoPath: string, + target: { worktreePath?: string; branch?: string } +): WorktreeRemovalRecord | undefined { + const branch = target.branch?.replace(/^refs\/heads\//, '') + for (const removal of pendingWorktreeRemovals.values()) { + if (!areWorktreePathsEqual(removal.repoPath, repoPath)) { + continue + } + if ( + (target.worktreePath && areWorktreePathsEqual(removal.worktreePath, target.worktreePath)) || + (branch && removal.branch === branch) + ) { + return removal + } + } + return undefined +} + +export function assertNoPendingWorktreeRemovalConflict( + repoPath: string, + target: { worktreePath?: string; branch?: string } +): void { + const removal = findPendingWorktreeRemovalConflict(repoPath, target) + if (removal) { + throw new Error( + `Orca is still deleting the workspace at ${removal.worktreePath}. Cleanup is pending; try again shortly.` + ) + } +} diff --git a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsx b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsx index 25b103138f2..c69aff187e2 100644 --- a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsx +++ b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsx @@ -159,6 +159,13 @@ function buttonText(props: Record): string { return renderToStaticMarkup(<>{props.children as ReactNode}) } +function clickCancel(): void { + const onClick = mocks.buttonProps.find((props) => props.variant === 'outline')?.onClick + if (typeof onClick === 'function') { + onClick() + } +} + function visibleMarkupText(markup: string): string { return markup.replace(/<[^>]*>/g, '') } @@ -381,6 +388,57 @@ describe('DeleteWorktreeDialog lineage copy', () => { expect(mocks.state.removeWorktree).not.toHaveBeenCalled() }) + it('shows the error the host lists for a failed delete, not a stale local one', async () => { + const failure = 'Operation not permitted' + const workspace = { + ...makeWorktree('Failed workspace', '/workspaces/failed'), + removalError: failure + } + mocks.state.modalData = { worktreeId: workspace.id } + mocks.state.allWorktrees.mockReturnValue([workspace]) + // What a lost retry reply leaves behind while the host lists the retry's own failure. + mocks.state.deleteStateByWorktreeId = { + [workspace.id]: { + isDeleting: false, + error: 'Request timed out', + canForceDelete: false, + forceDeleteReason: null + } + } + + const { default: DeleteWorktreeDialog } = await import('./DeleteWorktreeDialog') + const markup = renderToStaticMarkup() + + expect(markup).toContain(failure) + expect(markup).not.toContain('Request timed out') + }) + + it('shows a failed row’s host error in a batch and clears every stale error on Cancel', async () => { + const failed = { + ...makeWorktree('Failed workspace', '/workspaces/failed'), + removalError: 'Operation not permitted' + } + const dirty = makeWorktree('Dirty workspace', '/workspaces/dirty') + mocks.state.modalData = { worktreeIds: [failed.id, dirty.id] } + mocks.state.allWorktrees.mockReturnValue([failed, dirty]) + mocks.state.deleteStateByWorktreeId = { + [dirty.id]: { + isDeleting: false, + error: 'Worktree has uncommitted changes', + canForceDelete: true, + forceDeleteReason: 'dirty' + } + } + + const { default: DeleteWorktreeDialog } = await import('./DeleteWorktreeDialog') + const markup = renderToStaticMarkup() + clickCancel() + + expect(markup).toContain('Operation not permitted') + expect(mocks.state.clearWorktreeDeleteState).toHaveBeenCalledWith(failed.id, undefined) + expect(mocks.state.clearWorktreeDeleteState).toHaveBeenCalledWith(dirty.id, undefined) + }) + it('notifies the dialog caller after a toast force delete succeeds', async () => { const workspace = makeWorktree('Workspace', '/workspaces/workspace') const onDeleted = vi.fn() diff --git a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx index f6656118f16..8ea17c893bd 100644 --- a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx +++ b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx @@ -9,6 +9,7 @@ import { import { useAppStore } from '@/store' import { useAllWorktrees } from '@/store/selectors' import { runWorktreeDeletesInParallel } from './delete-worktree-flow' +import { getWorktreeDeleteErrorToShow } from './worktree-delete-error-display' import { composeWorktreeHostIdentity, getWorktreeHostIdentity @@ -168,7 +169,7 @@ const DeleteWorktreeDialog = React.memo(function DeleteWorktreeDialog() { ? getDeleteStateForWorktreeHost(worktree, deleteStateByWorktreeId) : undefined const isDeleting = deleteStates.some((state) => state.isDeleting) - const deleteError = !isBatchDelete ? (deleteState?.error ?? null) : null + const deleteError = !isBatchDelete ? getWorktreeDeleteErrorToShow(worktree, deleteState) : null const canForceDelete = !isBatchDelete && (deleteState?.canForceDelete ?? false) const gitStatusByWorktreeIdentity = useDeleteWorktreeStatusHydration({ isOpen, diff --git a/src/renderer/src/components/sidebar/DeleteWorktreeTargetPreview.tsx b/src/renderer/src/components/sidebar/DeleteWorktreeTargetPreview.tsx index fa5a47715ca..16004a9ec7a 100644 --- a/src/renderer/src/components/sidebar/DeleteWorktreeTargetPreview.tsx +++ b/src/renderer/src/components/sidebar/DeleteWorktreeTargetPreview.tsx @@ -6,6 +6,7 @@ import { getWorktreeHostIdentity } from '../../../../shared/worktree/host-qualif import { DeleteWorktreeDirtyChangeHint } from './DeleteWorktreeDirtyChangeHint' import type { AppState } from '@/store/types' import { getDeleteStateForWorktreeHost } from './worktree-delete-state-host-match' +import { getWorktreeDeleteErrorToShow } from './worktree-delete-error-display' import { getExecutionHostLabel, parseExecutionHostId, @@ -61,6 +62,7 @@ export function DeleteWorktreeTargetPreview({
{worktrees.map((item, index) => { const itemDeleteState = getDeleteStateForWorktreeHost(item, deleteStateByWorktreeId) + const itemDeleteError = getWorktreeDeleteErrorToShow(item, itemDeleteState) const labelIds = { name: `${targetIdPrefix}-${index}-name`, path: `${targetIdPrefix}-${index}-path`, @@ -92,9 +94,9 @@ export function DeleteWorktreeTargetPreview({ item.hostId ? getWorktreeHostIdentity(item) : item.id )} /> - {itemDeleteState?.error ? ( + {itemDeleteError ? (
- {itemDeleteState.error} + {itemDeleteError}
) : null}
diff --git a/src/renderer/src/components/sidebar/WorktreeCard.delete-failed.test.tsx b/src/renderer/src/components/sidebar/WorktreeCard.delete-failed.test.tsx new file mode 100644 index 00000000000..4628e568602 --- /dev/null +++ b/src/renderer/src/components/sidebar/WorktreeCard.delete-failed.test.tsx @@ -0,0 +1,160 @@ +import { renderToStaticMarkup } from 'react-dom/server' +import type { ReactNode } from 'react' +import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../../../shared/repo-types' +import type { WorktreeCardProperty } from '../../../../shared/ui-chrome-types' +import type { Worktree } from '../../../../shared/worktree/types' +import type WorktreeCardComponent from './WorktreeCard' + +const fetchHostedReviewForBranch = vi.fn() +const fetchIssue = vi.fn() +const fetchLinearIssue = vi.fn() +const openModal = vi.fn() +const updateWorktreeMeta = vi.fn() + +let WorktreeCard: typeof WorktreeCardComponent +let sshConnectionStates = new Map() +let sshTargetLabels = new Map() +let removedSshTargetLabels = new Map() +let runtimeStatusByEnvironmentId = new Map() +let runtimeEnvironments: { id: string; name: string }[] = [] +let sshStateByEnvironment = new Map() +let worktreesByRepo: Record = {} +let worktreeCardProperties: WorktreeCardProperty[] = ['status'] +let deleteStateByWorktreeId: Record = {} + +vi.mock('@/store', () => ({ + useAppStore: (selector: (state: unknown) => unknown) => + selector({ + deleteStateByWorktreeId, + fetchHostedReviewForBranch, + fetchIssue, + fetchLinearIssue, + gitConflictOperationByWorktree: {}, + hostedReviewCache: {}, + issueCache: {}, + linearIssueCache: {}, + openModal, + projectGroups: [], + remoteBranchConflictByWorktreeId: {}, + runtimeEnvironments, + runtimeStatusByEnvironmentId, + removedSshTargetLabels, + settings: null, + sshConnectionStates, + sshStateByEnvironment, + sshTargetLabels, + sshTargetsHydrated: true, + updateWorktreeMeta, + worktreesByRepo, + worktreeCardProperties + }) +})) + +vi.mock('@/lib/worktree-activation', () => ({ + activateAndRevealWorktree: vi.fn() +})) + +vi.mock('@/components/ui/tooltip', () => ({ + Tooltip: ({ children }: { children: ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: ReactNode }) => <>{children} +})) + +vi.mock('./CacheTimer', () => ({ + default: () => null, + usePromptCacheCountdownStartedAt: () => null +})) + +vi.mock('./WorktreeCardAgents', () => ({ + default: () => null +})) + +vi.mock('./use-worktree-activity-status', () => ({ + useWorktreeActivityStatus: () => 'idle' +})) + +vi.mock('./use-worktree-sleep-state', () => ({ + useIsSleepingWorktree: () => false +})) + +vi.mock('./WorktreeContextMenu', () => ({ + default: ({ children }: { children: ReactNode }) => <>{children}, + CLOSE_ALL_CONTEXT_MENUS_EVENT: 'orca:test-close-context-menus', + WORKTREE_CONTEXT_MENU_SCOPE_ATTR: 'data-orca-context-menu-scope', + WORKTREE_NATIVE_CONTEXT_MENU_ATTR: 'data-worktree-native-context-menu' +})) + +const FAILURE = "error: failed to delete '/repo/worktrees/one': Operation not permitted" + +function makeRepo(): Repo { + return { id: 'repo-1', path: '/repo', displayName: 'Repo', badgeColor: '#999999', addedAt: 1 } +} + +function makeWorktree(overrides: Partial = {}): Worktree { + return { + id: 'worktree-1', + repoId: 'repo-1', + path: '/repo/worktrees/one', + displayName: 'Workspace one', + branch: 'one', + head: 'abc123', + isBare: false, + isMainWorktree: false, + comment: '', + linkedIssue: null, + linkedPR: null, + linkedLinearIssue: null, + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + lastActivityAt: 1, + ...overrides + } +} + +function renderCard(worktree: Worktree): string { + // Static markup escapes the quotes in Git's message. + return renderToStaticMarkup( + + ).replaceAll(''', "'") +} + +describe('WorktreeCard for a delete that failed partway', () => { + beforeAll(async () => { + WorktreeCard = (await import('./WorktreeCard')).default + }, 20_000) + + beforeEach(() => { + vi.clearAllMocks() + deleteStateByWorktreeId = {} + worktreesByRepo = {} + worktreeCardProperties = ['status'] + }) + + it('says the delete failed, with the full error the host lists one hover away', () => { + const markup = renderCard(makeWorktree({ removalError: FAILURE })) + + expect(markup).toContain('data-worktree-card-delete-failed') + expect(markup).toContain('Delete failed') + // The tooltip primitive is rendered inline by this harness. + expect(markup).toContain(FAILURE) + }) + + it('shows nothing extra on a normal row', () => { + const markup = renderCard(makeWorktree()) + + expect(markup).not.toContain('data-worktree-card-delete-failed') + expect(markup).not.toContain('Delete failed') + }) + + it('shows Deleting instead once the retry starts', () => { + deleteStateByWorktreeId = { 'worktree-1': { isDeleting: true, error: null } } + + const markup = renderCard(makeWorktree({ removalError: FAILURE })) + + expect(markup).not.toContain('data-worktree-card-delete-failed') + expect(markup).toContain('Deleting') + }) +}) diff --git a/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts b/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts index d050d1c076a..ef9970e4c20 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts @@ -27,6 +27,7 @@ const mocks = vi.hoisted(() => { displayName: string isMainWorktree: boolean hostId?: ExecutionHostId + removalError?: string } >(), repos: [] as { id: string; displayName: string; connectionId?: string }[], @@ -104,6 +105,7 @@ function setWorktrees( displayName?: string isMainWorktree?: boolean hostId?: ExecutionHostId + removalError?: string }[] ): void { mocks.state.worktreeMap = new Map( @@ -116,7 +118,8 @@ function setWorktrees( path: worktree.path ?? `/workspaces/${worktree.id}`, displayName: worktree.displayName ?? worktree.id, isMainWorktree: worktree.isMainWorktree ?? false, - ...(worktree.hostId ? { hostId: worktree.hostId } : {}) + ...(worktree.hostId ? { hostId: worktree.hostId } : {}), + ...(worktree.removalError ? { removalError: worktree.removalError } : {}) } ]) ) @@ -170,6 +173,29 @@ describe('delete worktree flow', () => { }) }) + it('clears stale delete errors for a mixed batch before its dialog opens', () => { + setWorktrees([{ id: 'wt-failed', removalError: 'Operation not permitted' }, { id: 'wt-dirty' }]) + mocks.state.deleteStateByWorktreeId['wt-failed'] = { + isDeleting: false, + error: 'Request timed out', + canForceDelete: false + } + mocks.state.deleteStateByWorktreeId['wt-dirty'] = { + isDeleting: false, + error: 'Worktree has uncommitted changes', + canForceDelete: true + } + + expect(runWorktreeBatchDelete(['wt-failed', 'wt-dirty'])).toBe(true) + + expect(mocks.state.openModal).toHaveBeenCalledWith( + 'delete-worktree', + expect.objectContaining({ worktreeIds: ['wt-failed', 'wt-dirty'] }) + ) + // The failed row's own error still shows in the dialog: it comes from the row. + expect(mocks.state.deleteStateByWorktreeId).toEqual({}) + }) + it('treats duplicate selected ids as one delete target', () => { setWorktrees([{ id: 'wt-1' }]) diff --git a/src/renderer/src/components/sidebar/worktree-card-secondary-rows.tsx b/src/renderer/src/components/sidebar/worktree-card-secondary-rows.tsx index c71d9d6b0cb..d661726c67f 100644 --- a/src/renderer/src/components/sidebar/worktree-card-secondary-rows.tsx +++ b/src/renderer/src/components/sidebar/worktree-card-secondary-rows.tsx @@ -32,7 +32,8 @@ export function WorktreeCardSecondaryRows({ compactInlineAgentRows, showLineageChildChip, lineageChildAriaLabel, - childWorkspaceShortLabel + childWorkspaceShortLabel, + isDeleting } = card const { hasMetaRow } = presentation @@ -54,6 +55,27 @@ export function WorktreeCardSecondaryRows({ )} + {/* Why from the row: the host lists a failed delete until it is retried, forgotten or gone. + Why a tooltip: the error leads with the path; the Delete dialog shows it inline too. */} + {worktree.removalError && !isDeleting ? ( + + +
+ + + {translate('auto.components.sidebar.WorktreeCard.deleteFailed', 'Delete failed')} + +
+ + + {worktree.removalError} + + + ) : null} + {isActive && worktree.linkedLinearIssue ? ( | null | undefined, + state: { isDeleting: boolean; error: string | null } | undefined +): string | null { + return row?.removalError && !state?.isDeleting ? row.removalError : (state?.error ?? null) +} diff --git a/src/renderer/src/hooks/ipc-events/background-worktree-removal-bridge.ts b/src/renderer/src/hooks/ipc-events/background-worktree-removal-bridge.ts index ca219bdd03e..17b70444ebf 100644 --- a/src/renderer/src/hooks/ipc-events/background-worktree-removal-bridge.ts +++ b/src/renderer/src/hooks/ipc-events/background-worktree-removal-bridge.ts @@ -13,34 +13,79 @@ type AppStoreApi = Pick // Delete states this bridge set from a host marker, keyed like deleteStateByWorktreeId. A state the // local delete flow set is left to that flow. const hostMarkedDeleteStates = new Map() +// Errors this bridge set from a row's `removalError`, cleared once the host stops listing it failed. +const hostFailedDeleteStates = new Map() function deleteStateKey(row: HostMarkedRow): string { return row.hostId ? getWorktreeHostIdentity(row) : row.id } +function showDeleteError(store: AppStoreApi, row: HostMarkedRow, error: string): void { + store.setState((s) => ({ + deleteStateByWorktreeId: { + ...s.deleteStateByWorktreeId, + [deleteStateKey(row)]: { + isDeleting: false, + ...(row.hostId ? { executionHostId: row.hostId } : {}), + error, + canForceDelete: false, + forceDeleteReason: null + } + } + })) +} + +function showHostFailure(store: AppStoreApi, row: Worktree & { removalError: string }): void { + hostFailedDeleteStates.set(deleteStateKey(row), { + id: row.id, + hostId: row.hostId, + error: row.removalError + }) + showDeleteError(store, row, row.removalError) +} + /** * Shows the existing Deleting card while the host lists a row as removing, for views that did not - * ask for the delete. The row leaving means it finished; the row returning unmarked means it did not. + * ask for the delete. The row leaving means it finished; the row returning unmarked means it did + * not, and a row the host lists with `removalError` shows that error until Delete retries it. */ export function reconcileHostWorktreeRemovals(store: AppStoreApi = useAppStore): void { settleHostWorktreeRemovals() + const listed = new Map() + for (const rows of Object.values(store.getState().worktreesByRepo)) { + for (const row of rows) { + listed.set(deleteStateKey(row), row) + } + } + for (const [key, shown] of hostFailedDeleteStates) { + const row = listed.get(key) + if (row?.removalError === shown.error && !row.removing) { + continue + } + hostFailedDeleteStates.delete(key) + const current = store.getState().deleteStateByWorktreeId[key] + if (current && !current.isDeleting && current.error === shown.error) { + store.getState().clearWorktreeDeleteState(shown.id, shown.hostId) + } + } const state = store.getState() const marked: HostMarkedRow[] = [] - const listed = new Map() - for (const rows of Object.values(state.worktreesByRepo)) { - for (const row of rows) { - const key = deleteStateKey(row) - listed.set(key, row) - if (!row.removing || hostMarkedDeleteStates.has(key)) { - continue + for (const [key, row] of listed) { + const current = getDeleteStateForWorktreeHost(row, state.deleteStateByWorktreeId) + if (row.removalError && !row.removing) { + if (!current && !hostMarkedDeleteStates.has(key)) { + showHostFailure(store, { ...row, removalError: row.removalError }) } - const current = getDeleteStateForWorktreeHost(row, state.deleteStateByWorktreeId) - if (current?.isDeleting && current.phase !== 'queued') { - continue - } - hostMarkedDeleteStates.set(key, { id: row.id, hostId: row.hostId }) - marked.push({ id: row.id, hostId: row.hostId }) + continue } + if (!row.removing || hostMarkedDeleteStates.has(key)) { + continue + } + if (current?.isDeleting && current.phase !== 'queued') { + continue + } + hostMarkedDeleteStates.set(key, { id: row.id, hostId: row.hostId }) + marked.push({ id: row.id, hostId: row.hostId }) } if (marked.length > 0) { state.markWorktreesDeleting(marked) @@ -56,20 +101,11 @@ export function reconcileHostWorktreeRemovals(store: AppStoreApi = useAppStore): } if (!listedRow) { store.getState().clearWorktreeDeleteState(row.id, row.hostId) - continue + } else if (listedRow.removalError) { + showHostFailure(store, { ...listedRow, removalError: listedRow.removalError }) + } else { + showDeleteError(store, row, UNFINISHED_WORKTREE_REMOVAL_ERROR) } - store.setState((s) => ({ - deleteStateByWorktreeId: { - ...s.deleteStateByWorktreeId, - [key]: { - isDeleting: false, - ...(row.hostId ? { executionHostId: row.hostId } : {}), - error: UNFINISHED_WORKTREE_REMOVAL_ERROR, - canForceDelete: false, - forceDeleteReason: null - } - } - })) } } @@ -93,4 +129,5 @@ export function registerBackgroundWorktreeRemovalBridge(unsubs: (() => void)[]): export function _resetBackgroundWorktreeRemovalBridgeForTests(): void { hostMarkedDeleteStates.clear() + hostFailedDeleteStates.clear() } diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 52a0b6c1661..3a3d41b2f41 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -5686,6 +5686,7 @@ "74522ee457": "rename failed", "02e19349f4": "Auto-rename failed: view error", "691ccfd622": "Deleting…", + "deleteFailed": "Delete failed", "35ccfe2475": "Project {{value0}}", "1d66d84f0b": "string", "57eaa61b55": "Hide child workspaces", diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index a65f790523a..59d79541e59 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -4708,6 +4708,7 @@ "74522ee457": "falló el cambio de nombre", "02e19349f4": "Error al cambiar el nombre automáticamente: ver error", "691ccfd622": "Eliminando…", + "deleteFailed": "Error al eliminar", "35ccfe2475": "Proyecto {{value0}}", "1d66d84f0b": "cadena", "57eaa61b55": "Ocultar espacios de trabajo secundarios", diff --git a/src/renderer/src/i18n/locales/fr.json b/src/renderer/src/i18n/locales/fr.json index 89b2ccf199d..a77d17aa020 100644 --- a/src/renderer/src/i18n/locales/fr.json +++ b/src/renderer/src/i18n/locales/fr.json @@ -5604,6 +5604,7 @@ "74522ee457": "échec du renommage", "02e19349f4": "Échec du renommage auto : voir l'erreur", "691ccfd622": "Suppression…", + "deleteFailed": "Échec de la suppression", "35ccfe2475": "Projet {{value0}}", "1d66d84f0b": "string", "57eaa61b55": "Masquer les espaces de travail enfants", diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 65867319e87..3bc91234a44 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -5459,6 +5459,7 @@ "74522ee457": "名前の変更に失敗しました", "02e19349f4": "名前の自動変更に失敗しました: 表示エラー", "691ccfd622": "削除中…", + "deleteFailed": "削除に失敗しました", "35ccfe2475": "プロジェクト{{value0}}", "1d66d84f0b": "文字列", "57eaa61b55": "子ワークスペースを非表示にする", diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index f5d93bc0ee0..4aabc391484 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -5459,6 +5459,7 @@ "74522ee457": "이름 바꾸기 실패", "02e19349f4": "자동 이름 바꾸기 실패: 보기 오류", "691ccfd622": "삭제 중…", + "deleteFailed": "삭제 실패", "35ccfe2475": "프로젝트 {{value0}}", "1d66d84f0b": "문자열", "57eaa61b55": "하위 워크스페이스 숨기기", diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index fe1236cfa7c..3f3a4883995 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -5459,6 +5459,7 @@ "74522ee457": "重命名失败", "02e19349f4": "自动重命名失败：查看错误", "691ccfd622": "正在删除...", + "deleteFailed": "删除失败", "35ccfe2475": "项目{{value0}}", "1d66d84f0b": "字符串", "57eaa61b55": "隐藏子工作区", diff --git a/src/renderer/src/store/slices/worktrees-background-removal.test.ts b/src/renderer/src/store/slices/worktrees-background-removal.test.ts index 2b8d1d8394e..6bff5d80837 100644 --- a/src/renderer/src/store/slices/worktrees-background-removal.test.ts +++ b/src/renderer/src/store/slices/worktrees-background-removal.test.ts @@ -24,7 +24,7 @@ const hostKey = getWorktreeHostIdentity({ id: worktreeId, hostId: 'local' }) function seedRow( store: ReturnType, - overrides: { removing?: true } = {} + overrides: { removing?: true; removalError?: string } = {} ): void { seedStore(store, { worktreesByRepo: { @@ -204,6 +204,63 @@ describe('removing a worktree the host deletes in the background', () => { }) }) + it('shows the host error on a row the host lists as a failed delete, until it leaves', () => { + // A window that opened after the delete failed, or a restart after a failed startup finish. + seedRow(store, { removalError: 'Operation not permitted' }) + reconcileHostWorktreeRemovals(store) + expect(deleteState(store)).toMatchObject({ + isDeleting: false, + error: 'Operation not permitted', + canForceDelete: false + }) + + // Forgotten, or the checkout deleted outside Orca: the host stops listing it. + seedStore(store, { worktreesByRepo: { repo1: [] } }) + reconcileHostWorktreeRemovals(store) + expect(deleteState(store)).toBeUndefined() + }) + + it('shows Deleting while the host retries a failed delete, and its new error after', () => { + seedRow(store, { removalError: 'Operation not permitted' }) + reconcileHostWorktreeRemovals(store) + + seedRow(store, { removing: true }) + reconcileHostWorktreeRemovals(store) + expect(deleteState(store)).toMatchObject({ isDeleting: true, phase: 'deleting' }) + + seedRow(store, { removalError: 'Resource busy' }) + reconcileHostWorktreeRemovals(store) + expect(deleteState(store)).toMatchObject({ isDeleting: false, error: 'Resource busy' }) + }) + + it('shows the error the host lists when a delete it marked Deleting fails', () => { + seedRow(store, { removing: true }) + reconcileHostWorktreeRemovals(store) + + seedRow(store, { removalError: 'Operation not permitted' }) + reconcileHostWorktreeRemovals(store) + expect(deleteState(store)).toMatchObject({ + isDeleting: false, + error: 'Operation not permitted' + }) + }) + + it('reports the host error for a lost reply when the host lists the failed delete', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + seedRow(store) + const refresh = vi.fn() + refresh.mockImplementation(async () => { + seedRow(store, { removalError: 'Operation not permitted' }) + return true + }) + store.setState({ fetchWorktrees: refresh }) + mockApi.worktrees.remove.mockRejectedValue(new Error('Request timed out: worktree.rm')) + + await expect( + store.getState().removeWorktree({ id: worktreeId, executionHostId: null }) + ).resolves.toEqual({ ok: false, error: 'Operation not permitted' }) + }) + it('leaves a delete this renderer started to that flow', () => { seedRow(store, { removing: true }) store.getState().markWorktreesDeleting([{ id: worktreeId, hostId: 'local' }]) diff --git a/src/renderer/src/store/slices/worktrees/teardown/host-worktree-removal-state.ts b/src/renderer/src/store/slices/worktrees/teardown/host-worktree-removal-state.ts index e89fa82ecbb..62567af4a53 100644 --- a/src/renderer/src/store/slices/worktrees/teardown/host-worktree-removal-state.ts +++ b/src/renderer/src/store/slices/worktrees/teardown/host-worktree-removal-state.ts @@ -13,7 +13,7 @@ import type { WorktreeSliceGet } from '../listing/worktree-slice-types' export const UNFINISHED_WORKTREE_REMOVAL_ERROR = 'The delete did not finish. Try again.' -type RemovalRow = Pick +type RemovalRow = Pick function rowHostId(row: Pick): ExecutionHostId { return row.hostId ?? LOCAL_EXECUTION_HOST_ID @@ -40,8 +40,9 @@ const pendingJudgements = new Set<() => void>() /** * Settles a delete whose reply was lost from the host's listing, as every other view does: the row - * leaving means the delete finished, and the row listed without `removing` means it did not. When - * the listing cannot be read either, rejects with the lost reply's error. + * leaving means the delete finished, and the row listed without `removing` means it did not (with + * the host's error when it lists one). When the listing cannot be read either, rejects with the + * lost reply's error. */ function waitForHostWorktreeRemoval(args: { hostId: ExecutionHostId | undefined @@ -62,7 +63,7 @@ function waitForHostWorktreeRemoval(args: { } pendingJudgements.delete(judge) if (row) { - reject(new Error(UNFINISHED_WORKTREE_REMOVAL_ERROR)) + reject(new Error(row.removalError ?? UNFINISHED_WORKTREE_REMOVAL_ERROR)) } else { resolve() } diff --git a/src/shared/runtime-worktree-contracts.ts b/src/shared/runtime-worktree-contracts.ts index 68e83c3268c..56cd6c67c48 100644 --- a/src/shared/runtime-worktree-contracts.ts +++ b/src/shared/runtime-worktree-contracts.ts @@ -78,6 +78,8 @@ export type RuntimeWorktreePsSummary = { agents: RuntimeWorktreeAgentRow[] /** See `Worktree.removing`; sent only to clients that advertise background removal. */ removing?: true + /** See `GitWorktreeInfo.removalError`. */ + removalError?: string } export type RuntimeGitLocalBranches = { diff --git a/src/shared/worktree/types.ts b/src/shared/worktree/types.ts index ce2bd025e28..85419632cef 100644 --- a/src/shared/worktree/types.ts +++ b/src/shared/worktree/types.ts @@ -37,6 +37,9 @@ export type GitWorktreeInfo = { /** True for the repo's main working tree (the first entry from `git worktree list`). * Linked worktrees created via `git worktree add` have this set to false. */ isMainWorktree: boolean + /** Not from Git: the error of a local delete that failed after Git dropped this checkout's + * registration. The host lists the leftover so Delete can retry it. */ + removalError?: string } /** Head/branch snapshot read from Git metadata files without spawning Git.