From c723ca79aa5a4d7d8986ceeb8de56dde380640db Mon Sep 17 00:00:00 2001 From: Neil Date: Sat, 12 Sep 2026 22:53:56 -0700 Subject: [PATCH] Harden worktree creation cancellation rollback Review follow-ups on the pending-creation cancellation path: - Refuse a replaced-instance rollback inside beginHostQualifiedRemoval, next to the sibling fail-closed refusals and before the row is marked deleting, so the refusal cannot pin a permanent error on a live replacement's delete state. An absent row no longer counts as proof of replacement. - Treat a replaced instance as nothing left to roll back rather than a cleanup failure, so it stops raising a never-dismissing toast. - Fail closed when a deferred rollback cannot identify its workspace: a host too old to stamp instanceId leaves no proof, so leave the workspace in place instead of force-deleting a possible replacement. - Say that a retained runtime is still running and that deleting the workspace releases it. - Warn instead of reporting a clean rollback when a dispatched create failed without proving the host did not create the workspace. - Keep the attempt map private behind register/release/reset so the identity-checked release has one implementation. - Share the creation-flow store stand-in from the fixtures module, keeping both touched files under their max-lines caps. --- .../src/lib/worktree-creation-attempt.ts | 42 ++++++- .../worktree-creation-cancellation.test.ts | 105 +++++++++++++++++- .../src/lib/worktree-creation-cancellation.ts | 62 ++++++++--- .../src/lib/worktree-creation-flow-execute.ts | 8 +- .../src/lib/worktree-creation-flow.test.ts | 85 +++++++------- .../lib/worktree-creation-test-fixtures.ts | 73 ++++++++++++ .../store/slices/worktree-removal-options.ts | 4 + .../worktrees-removal-state-cleanup.test.ts | 18 +++ .../worktrees/create/create-worktree.ts | 3 +- .../host-qualified-worktree-removal.ts | 30 ++++- .../worktrees/teardown/remove-worktree.ts | 5 +- 11 files changed, 356 insertions(+), 79 deletions(-) diff --git a/src/renderer/src/lib/worktree-creation-attempt.ts b/src/renderer/src/lib/worktree-creation-attempt.ts index 2d06fdb9404..4bf10c66cd0 100644 --- a/src/renderer/src/lib/worktree-creation-attempt.ts +++ b/src/renderer/src/lib/worktree-creation-attempt.ts @@ -1,18 +1,52 @@ import type { Worktree } from '../../../shared/worktree/types' +/** Refused before the create reached the host, so nothing can have been created. */ +export class WorktreeCreationCancelledError extends Error {} + export type WorktreeCreationAttempt = { completed: boolean cancelled: boolean cleanupAfterSettlement?: () => Promise cleanupRuntime?: () => Promise worktree?: Worktree + /** The create call failed without proving the host did not create the workspace. */ + createOutcomeUnknown?: boolean isCancelled: () => boolean } -export const activeWorktreeCreationAttempts = new Map() +// Why private: these hold cleanup closures, which a zustand slice cannot serialise, +// so attempt bookkeeping lives here while `pendingWorktreeCreations` stays the store's. +// Every mutation goes through the functions below so the identity-checked release +// (a newer attempt must not be evicted by an older one) has exactly one implementation. +const activeAttempts = new Map() + +export function getActiveWorktreeCreation(creationId: string): WorktreeCreationAttempt | undefined { + return activeAttempts.get(creationId) +} + +export function registerActiveWorktreeCreation( + creationId: string, + attempt: WorktreeCreationAttempt +): void { + activeAttempts.set(creationId, attempt) +} + +/** Drop `attempt` only while it is still the registered one. */ +export function releaseActiveWorktreeCreation( + creationId: string, + attempt: WorktreeCreationAttempt +): void { + if (activeAttempts.get(creationId) === attempt) { + activeAttempts.delete(creationId) + } +} + +export function resetActiveWorktreeCreations(): void { + activeAttempts.clear() +} export function cancelActiveWorktreeCreation(creationId: string): boolean { - const attempt = activeWorktreeCreationAttempts.get(creationId) + const attempt = activeAttempts.get(creationId) if (!attempt || attempt.completed) { return false } @@ -24,9 +58,9 @@ export function cancelActiveWorktreeCreation(creationId: string): boolean { } export function completeActiveWorktreeCreation(creationId: string): void { - const attempt = activeWorktreeCreationAttempts.get(creationId) + const attempt = activeAttempts.get(creationId) if (attempt) { attempt.completed = true } - activeWorktreeCreationAttempts.delete(creationId) + activeAttempts.delete(creationId) } diff --git a/src/renderer/src/lib/worktree-creation-cancellation.test.ts b/src/renderer/src/lib/worktree-creation-cancellation.test.ts index afca8221f93..a28849b6b03 100644 --- a/src/renderer/src/lib/worktree-creation-cancellation.test.ts +++ b/src/renderer/src/lib/worktree-creation-cancellation.test.ts @@ -3,13 +3,14 @@ import { makeWorktree } from '@/store/slices/worktrees-slice-test-fixtures' import { toast } from 'sonner' import { cancelActiveWorktreeCreation } from './worktree-creation-attempt' import { withWorktreeCreationCancellation } from './worktree-creation-cancellation' +import { WORKTREE_INSTANCE_REPLACED_ERROR } from '@/store/slices/worktree-removal-options' const state = vi.hoisted(() => ({ pendingWorktreeCreations: {} as Record, removeWorktree: vi.fn() })) vi.mock('@/store', () => ({ useAppStore: { getState: () => state } })) -vi.mock('sonner', () => ({ toast: { error: vi.fn() } })) +vi.mock('sonner', () => ({ toast: { error: vi.fn(), warning: vi.fn() } })) function deferred() { let resolve!: () => void @@ -18,7 +19,12 @@ function deferred() { }) return { promise, resolve } } -const worktree = makeWorktree({ id: 'repo::/workspace', repoId: 'repo', hostId: 'ssh:owner' }) +const worktree = makeWorktree({ + id: 'repo::/workspace', + repoId: 'repo', + hostId: 'ssh:owner', + instanceId: 'instance-1' +}) beforeEach(() => { vi.clearAllMocks() state.pendingWorktreeCreations = { creation: {}, other: {} } @@ -65,7 +71,11 @@ describe('worktree creation cancellation', () => { expect(state.removeWorktree).toHaveBeenCalledExactlyOnceWith( { id: worktree.id, executionHostId: 'ssh:owner' }, true, - { skipArchiveHooks: true, suppressPreservedBranchToast: true } + { + skipArchiveHooks: true, + suppressPreservedBranchToast: true, + expectedInstanceId: 'instance-1' + } ) expect(cleanup).toHaveBeenCalledOnce() expect(state.pendingWorktreeCreations.other).toBeDefined() @@ -171,6 +181,95 @@ describe('worktree creation cancellation', () => { expect.stringContaining('Host unavailable'), expect.objectContaining({ duration: Infinity }) ) + // The retained runtime is invisible otherwise, so the toast must say how to release it. + expect(toast.error).toHaveBeenCalledWith( + expect.stringContaining('deleting the workspace releases it'), + expect.anything() + ) expect(cancelActiveWorktreeCreation('creation')).toBe(false) }) + + it('omits the runtime hint when the cancelled creation had no runtime', async () => { + state.removeWorktree.mockResolvedValue({ ok: false, error: 'Host unavailable' }) + await withWorktreeCreationCancellation('creation', async (attempt) => { + attempt.worktree = worktree + cancelActiveWorktreeCreation('creation') + }) + expect(toast.error).toHaveBeenCalledWith( + expect.not.stringContaining('runtime is still running'), + expect.anything() + ) + }) + + it('warns instead of claiming a clean rollback when the host never confirmed', async () => { + await withWorktreeCreationCancellation('creation', async (attempt) => { + attempt.createOutcomeUnknown = true + cancelActiveWorktreeCreation('creation') + }) + expect(state.removeWorktree).not.toHaveBeenCalled() + expect(toast.warning).toHaveBeenCalledWith(expect.stringContaining('delete it manually')) + expect(toast.error).not.toHaveBeenCalled() + }) + + it('stays silent when cancellation is known to have preceded the create', async () => { + await withWorktreeCreationCancellation('creation', async () => { + cancelActiveWorktreeCreation('creation') + }) + expect(toast.warning).not.toHaveBeenCalled() + expect(toast.error).not.toHaveBeenCalled() + }) + + it('treats a replaced instance as nothing left to roll back, not a cleanup failure', async () => { + const cleanup = vi.fn() + state.removeWorktree.mockResolvedValue({ + ok: false, + error: WORKTREE_INSTANCE_REPLACED_ERROR + }) + await withWorktreeCreationCancellation('creation', async (attempt) => { + attempt.worktree = worktree + attempt.cleanupRuntime = cleanup + cancelActiveWorktreeCreation('creation') + }) + expect(cleanup).toHaveBeenCalledOnce() + expect(toast.error).not.toHaveBeenCalled() + }) + + it('refuses a deferred rollback the host left unidentifiable rather than force-deleting', async () => { + const unstamped = makeWorktree({ id: 'repo::/workspace', repoId: 'repo', hostId: 'ssh:owner' }) + await withWorktreeCreationCancellation('creation', async (attempt) => { + attempt.worktree = unstamped + }) + expect(cancelActiveWorktreeCreation('creation')).toBe(true) + await vi.waitFor(() => + expect(toast.error).toHaveBeenCalledWith( + expect.stringContaining('Delete it manually'), + expect.objectContaining({ duration: Infinity }) + ) + ) + expect(state.removeWorktree).not.toHaveBeenCalled() + }) + + it('still rolls back an unidentifiable workspace when cancelled in flight', async () => { + const unstamped = makeWorktree({ id: 'repo::/workspace', repoId: 'repo', hostId: 'ssh:owner' }) + await withWorktreeCreationCancellation('creation', async (attempt) => { + attempt.worktree = unstamped + cancelActiveWorktreeCreation('creation') + }) + // In-flight cancellation cannot race a replacement, so it must not fail closed. + expect(state.removeWorktree).toHaveBeenCalledOnce() + expect(toast.error).not.toHaveBeenCalled() + }) + + it('lets retry proceed after the rollback found its workspace already replaced', async () => { + await withWorktreeCreationCancellation('creation', async (attempt) => { + attempt.worktree = worktree + }) + state.removeWorktree.mockResolvedValueOnce({ + ok: false, + error: WORKTREE_INSTANCE_REPLACED_ERROR + }) + const retry = vi.fn() + await withWorktreeCreationCancellation('creation', retry) + expect(retry).toHaveBeenCalledOnce() + }) }) diff --git a/src/renderer/src/lib/worktree-creation-cancellation.ts b/src/renderer/src/lib/worktree-creation-cancellation.ts index 0c922d637f7..e658c75e1d6 100644 --- a/src/renderer/src/lib/worktree-creation-cancellation.ts +++ b/src/renderer/src/lib/worktree-creation-cancellation.ts @@ -2,15 +2,18 @@ import { toast } from 'sonner' import { useAppStore } from '@/store' import { - activeWorktreeCreationAttempts as activeAttempts, + getActiveWorktreeCreation, + registerActiveWorktreeCreation, + releaseActiveWorktreeCreation, type WorktreeCreationAttempt } from './worktree-creation-attempt' +import { WORKTREE_INSTANCE_REPLACED_ERROR } from '@/store/slices/worktree-removal-options' export async function withWorktreeCreationCancellation( creationId: string, execute: (attempt: WorktreeCreationAttempt) => Promise ): Promise { - const previous = activeAttempts.get(creationId) + const previous = getActiveWorktreeCreation(creationId) if (previous && !previous.completed && !previous.cleanupAfterSettlement) { return } @@ -29,33 +32,45 @@ export async function withWorktreeCreationCancellation( isCancelled: () => attempt.cancelled || !useAppStore.getState().pendingWorktreeCreations[creationId] } - activeAttempts.set(creationId, attempt) + registerActiveWorktreeCreation(creationId, attempt) try { if (!attempt.isCancelled()) { await execute(attempt) } } finally { - const cleanup = (): Promise => - removeCancelledCreation(attempt).finally(() => { - if (activeAttempts.get(creationId) === attempt) { - activeAttempts.delete(creationId) - } - }) + const cleanup = (deferred: boolean): Promise => + removeCancelledCreation(attempt, deferred).finally(() => + releaseActiveWorktreeCreation(creationId, attempt) + ) if (!attempt.completed && attempt.isCancelled()) { - await cleanup() + await cleanup(false) } else if (!attempt.completed && attempt.worktree) { // Failed post-create startup still owns a workspace when its error panel is dismissed. - attempt.cleanupAfterSettlement = cleanup - } else if (activeAttempts.get(creationId) === attempt) { - activeAttempts.delete(creationId) + attempt.cleanupAfterSettlement = () => cleanup(true) + } else { + releaseActiveWorktreeCreation(creationId, attempt) } } } -async function removeCancelledCreation(attempt: WorktreeCreationAttempt): Promise { +/** + * `deferred` marks a rollback that outlived its attempt, waiting behind an error + * panel. Only that one can sit long enough for the user to delete and recreate at + * the same path, so only it refuses to force-delete a workspace it cannot prove is + * the one it created — a host too old to stamp `instanceId` leaves no other proof. + */ +async function removeCancelledCreation( + attempt: WorktreeCreationAttempt, + deferred: boolean +): Promise { const { worktree } = attempt try { if (worktree) { + if (deferred && !worktree.instanceId) { + throw new Error( + 'it could not be identified on the host, so it was left in place. Delete it manually if unwanted.' + ) + } const result = await useAppStore .getState() .removeWorktree({ id: worktree.id, executionHostId: worktree.hostId ?? 'local' }, true, { @@ -63,16 +78,31 @@ async function removeCancelledCreation(attempt: WorktreeCreationAttempt): Promis suppressPreservedBranchToast: true, ...(worktree.instanceId ? { expectedInstanceId: worktree.instanceId } : {}) }) - if (!result.ok) { + // A replaced instance means this attempt's workspace is already gone, so + // there is nothing left to roll back — not a cleanup failure. + if (!result.ok && result.error !== WORKTREE_INSTANCE_REPLACED_ERROR) { throw new Error(result.error) } } + // Why: cancelling before the host answered leaves no row to remove, but the + // host may still have finished. Say so rather than reporting a clean rollback. + if (!worktree && attempt.createOutcomeUnknown) { + toast.warning( + 'Cancelled before the host confirmed the workspace. If it appears, delete it manually.' + ) + } await attempt.cleanupRuntime?.() return true } catch (error) { const message = error instanceof Error ? error.message : String(error) console.error('worktree create: cancellation cleanup failed', worktree?.id, error) - toast.error(`Could not remove the cancelled workspace: ${message}`, { + // Why: the runtime is deliberately left running — tearing down a VM whose + // workspace deletion never confirmed could destroy a workspace that survived. + // Deleting the still-visible row is what releases both, so say so. + const runtimeHint = attempt.cleanupRuntime + ? ' Its runtime is still running; deleting the workspace releases it.' + : '' + toast.error(`Could not remove the cancelled workspace: ${message}${runtimeHint}`, { duration: Infinity }) return false diff --git a/src/renderer/src/lib/worktree-creation-flow-execute.ts b/src/renderer/src/lib/worktree-creation-flow-execute.ts index 352752efd7a..d36565e8069 100644 --- a/src/renderer/src/lib/worktree-creation-flow-execute.ts +++ b/src/renderer/src/lib/worktree-creation-flow-execute.ts @@ -1,7 +1,10 @@ import { dispatchWorktreeCreation } from './worktree-creation-dispatch' import { toast } from 'sonner' import { withWorktreeCreationCancellation } from './worktree-creation-cancellation' -import type { WorktreeCreationAttempt } from './worktree-creation-attempt' +import { + WorktreeCreationCancelledError, + type WorktreeCreationAttempt +} from './worktree-creation-attempt' import { useAppStore } from '@/store' import { preflightAgentTrust } from '@/lib/agent-trust-preflight' import { activateAndRevealWorktree, type ActivateAndRevealResult } from '@/lib/worktree-activation' @@ -70,6 +73,9 @@ async function executeWorktreeCreationAttempt( // Why: a missing entry means the user cancelled mid-flight — abandon // silently rather than surfacing an error for work they already dismissed. if (!useAppStore.getState().pendingWorktreeCreations[creationId]) { + // A dispatched call that failed proves nothing about the host: it may have + // finished and lost the response, so cleanup must not claim the path is clear. + attempt.createOutcomeUnknown = !(error instanceof WorktreeCreationCancelledError) return } if (preparedRequest.ephemeralVmRuntimeId) { diff --git a/src/renderer/src/lib/worktree-creation-flow.test.ts b/src/renderer/src/lib/worktree-creation-flow.test.ts index f93cf6e561e..424ea147951 100644 --- a/src/renderer/src/lib/worktree-creation-flow.test.ts +++ b/src/renderer/src/lib/worktree-creation-flow.test.ts @@ -1,52 +1,19 @@ -import { activeWorktreeCreationAttempts } from './worktree-creation-attempt' -import { makeRequest, makePendingCreation } from './worktree-creation-test-fixtures' +import { + resetActiveWorktreeCreations, + WorktreeCreationCancelledError +} from './worktree-creation-attempt' +import { + makeRequest, + makePendingCreation, + makeCreationFlowStore +} from './worktree-creation-test-fixtures' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { PendingWorktreeCreation } from '@/lib/pending-worktree-creation' const { prepareEphemeralVmWorkspaceTargetMock } = vi.hoisted(() => ({ prepareEphemeralVmWorkspaceTargetMock: vi.fn() })) -type TestActiveView = 'terminal' | 'tasks' - -const store = { - settings: { - activeRuntimeEnvironmentId: null as string | null, - experimentalNativeChat: undefined as boolean | undefined, - openAgentTabsInChatByDefault: undefined as boolean | undefined - }, - activeView: 'terminal' as TestActiveView, - activePendingCreationId: 'creation-1' as string | null, - repos: [{ id: 'repo-runtime', connectionId: null }], - pendingWorktreeCreations: {} as Record, - beginPendingWorktreeCreation: vi.fn((entry: PendingWorktreeCreation) => { - store.pendingWorktreeCreations[entry.creationId] = entry - store.activePendingCreationId = entry.creationId - }), - updatePendingWorktreeCreation: vi.fn( - (creationId: string, patch: Partial) => { - const entry = store.pendingWorktreeCreations[creationId] - if (entry) { - store.pendingWorktreeCreations[creationId] = { ...entry, ...patch } - } - } - ), - removePendingWorktreeCreation: vi.fn((creationId: string) => { - delete store.pendingWorktreeCreations[creationId] - }), - updateWorktreeMeta: vi.fn(), - removeWorktree: vi.fn().mockResolvedValue({ ok: true }), - setActivePendingWorktreeCreation: vi.fn(), - setActiveView: vi.fn(), - setSidebarOpen: vi.fn(), - createWorktree: vi.fn(() => new Promise(() => {})), - setupProjectExistingFolder: vi.fn(), - refreshRuntimeEnvironmentStatus: vi.fn(), - seedNativeChatLaunchDraft: vi.fn(), - setTabViewMode: vi.fn(), - tabsByWorktree: {} as Record, - unifiedTabsByWorktree: {} -} +const store = makeCreationFlowStore() vi.mock('@/store', () => ({ useAppStore: { @@ -76,7 +43,8 @@ vi.mock('@/lib/new-workspace', () => ({ vi.mock('sonner', () => ({ toast: { - error: vi.fn() + error: vi.fn(), + warning: vi.fn() } })) @@ -96,7 +64,7 @@ import { } from './worktree-creation-flow' beforeEach(() => { - activeWorktreeCreationAttempts.clear() + resetActiveWorktreeCreations() vi.clearAllMocks() store.settings.activeRuntimeEnvironmentId = null store.settings.experimentalNativeChat = undefined @@ -594,6 +562,33 @@ describe('staged background worktree creation', () => { expect(activateAndRevealWorktree).not.toHaveBeenCalled() }) + // Why: a dispatched create that throws proves nothing (the host may have finished + // and lost the response), while a pre-dispatch refusal proves nothing was created. + it.each([ + { what: 'never reported back', err: () => new Error('socket hang up'), warnings: 1 }, + { + what: 'was refused before dispatch', + err: () => new WorktreeCreationCancelledError('Worktree creation cancelled.'), + warnings: 0 + } + ])('cancelled create whose call $what', async ({ err, warnings }) => { + store.repos = [{ id: 'repo-1', connectionId: null }] + store.createWorktree.mockImplementationOnce(async () => { + delete store.pendingWorktreeCreations['creation-1'] + throw err() + }) + + continueBackgroundWorktreeCreation('creation-1', makeRequest(), { + revealCreationSurface: false + }) + + await vi.waitFor(() => expect(store.createWorktree).toHaveBeenCalled()) + await flushAsyncWorktreeCreation() + expect(toast.warning).toHaveBeenCalledTimes(warnings) + expect(store.removeWorktree).not.toHaveBeenCalled() + expect(toast.error).not.toHaveBeenCalled() + }) + // Why: one-click "Start workspace from issue" commonly backgrounds, so the // user-moved-on path is the common delivery for the repo's issue command; it // must thread through as the 5th positional arg, not be dropped to undefined. diff --git a/src/renderer/src/lib/worktree-creation-test-fixtures.ts b/src/renderer/src/lib/worktree-creation-test-fixtures.ts index 85307598ced..bf88fb98bc8 100644 --- a/src/renderer/src/lib/worktree-creation-test-fixtures.ts +++ b/src/renderer/src/lib/worktree-creation-test-fixtures.ts @@ -1,3 +1,4 @@ +import { vi, type Mock } from 'vitest' import type { PendingWorktreeCreation, WorktreeCreationRequest } from './pending-worktree-creation' export function makeRequest( @@ -28,3 +29,75 @@ export function makePendingCreation(request: WorktreeCreationRequest): PendingWo request } } + +type TestActiveView = 'terminal' | 'tasks' + +export type CreationFlowStore = { + settings: { + activeRuntimeEnvironmentId: string | null + experimentalNativeChat: boolean | undefined + openAgentTabsInChatByDefault: boolean | undefined + } + activeView: TestActiveView + activePendingCreationId: string | null + repos: { id: string; connectionId: string | null }[] + pendingWorktreeCreations: Record + beginPendingWorktreeCreation: Mock + updatePendingWorktreeCreation: Mock + removePendingWorktreeCreation: Mock + updateWorktreeMeta: Mock + removeWorktree: Mock + setActivePendingWorktreeCreation: Mock + setActiveView: Mock + setSidebarOpen: Mock + createWorktree: Mock + setupProjectExistingFolder: Mock + refreshRuntimeEnvironmentStatus: Mock + seedNativeChatLaunchDraft: Mock + setTabViewMode: Mock + tabsByWorktree: Record + unifiedTabsByWorktree: Record +} + +/** The `@/store` stand-in shared by the creation-flow suites. */ +export function makeCreationFlowStore(): CreationFlowStore { + const store: CreationFlowStore = { + settings: { + activeRuntimeEnvironmentId: null, + experimentalNativeChat: undefined, + openAgentTabsInChatByDefault: undefined + }, + activeView: 'terminal', + activePendingCreationId: 'creation-1', + repos: [{ id: 'repo-runtime', connectionId: null }], + pendingWorktreeCreations: {}, + beginPendingWorktreeCreation: vi.fn((entry: PendingWorktreeCreation) => { + store.pendingWorktreeCreations[entry.creationId] = entry + store.activePendingCreationId = entry.creationId + }), + updatePendingWorktreeCreation: vi.fn( + (creationId: string, patch: Partial) => { + const entry = store.pendingWorktreeCreations[creationId] + if (entry) { + store.pendingWorktreeCreations[creationId] = { ...entry, ...patch } + } + } + ), + removePendingWorktreeCreation: vi.fn((creationId: string) => { + delete store.pendingWorktreeCreations[creationId] + }), + updateWorktreeMeta: vi.fn(), + removeWorktree: vi.fn().mockResolvedValue({ ok: true }), + setActivePendingWorktreeCreation: vi.fn(), + setActiveView: vi.fn(), + setSidebarOpen: vi.fn(), + createWorktree: vi.fn(() => new Promise(() => {})), + setupProjectExistingFolder: vi.fn(), + refreshRuntimeEnvironmentStatus: vi.fn(), + seedNativeChatLaunchDraft: vi.fn(), + setTabViewMode: vi.fn(), + tabsByWorktree: {}, + unifiedTabsByWorktree: {} + } + return store +} diff --git a/src/renderer/src/store/slices/worktree-removal-options.ts b/src/renderer/src/store/slices/worktree-removal-options.ts index 765fa528ed7..c95aa9241c2 100644 --- a/src/renderer/src/store/slices/worktree-removal-options.ts +++ b/src/renderer/src/store/slices/worktree-removal-options.ts @@ -1,5 +1,9 @@ import type { ExecutionHostId } from '../../../../shared/execution-host' +/** The rollback target was replaced, so its workspace is already gone. */ +export const WORKTREE_INSTANCE_REPLACED_ERROR = + 'Workspace instance changed before cancellation cleanup.' + export type RemoveWorktreeOptions = { // 'forget-local' drops the workspace from Orca only (no remote Git/FS work) // for workspaces pinned to a removed/disconnected SSH host. Reuses the same diff --git a/src/renderer/src/store/slices/worktrees-removal-state-cleanup.test.ts b/src/renderer/src/store/slices/worktrees-removal-state-cleanup.test.ts index 63bd56af1d2..7326bb9aacb 100644 --- a/src/renderer/src/store/slices/worktrees-removal-state-cleanup.test.ts +++ b/src/renderer/src/store/slices/worktrees-removal-state-cleanup.test.ts @@ -54,6 +54,24 @@ describe('removeWorktree state cleanup', () => { }) expect(mockApi.worktrees.remove).not.toHaveBeenCalled() expect(store.getState().worktreesByRepo.repo1).toEqual([replacement]) + // The refused rollback must leave no trace on the live replacement's delete state. + expect(store.getState().deleteStateByWorktreeId).toEqual({}) + }) + + it('does not read an absent row as a replacement during cancellation cleanup', async () => { + const store = createTestStore() + store.setState({ worktreesByRepo: { repo1: [] } } as Partial) + const result = await store + .getState() + .removeWorktree({ id: 'repo1::/path/gone', executionHostId: 'local' }, true, { + skipArchiveHooks: true, + expectedInstanceId: 'original-instance' + }) + // Absent locally is not proof the checkout is gone, so this must not report + // the benign "replaced" verdict that tells cleanup there is nothing to do. + if (!result.ok) { + expect(result.error).not.toBe('Workspace instance changed before cancellation cleanup.') + } }) beforeEach(() => { diff --git a/src/renderer/src/store/slices/worktrees/create/create-worktree.ts b/src/renderer/src/store/slices/worktrees/create/create-worktree.ts index 46dcb89fb4c..f8a3911b8fe 100644 --- a/src/renderer/src/store/slices/worktrees/create/create-worktree.ts +++ b/src/renderer/src/store/slices/worktrees/create/create-worktree.ts @@ -32,6 +32,7 @@ import { } from './worktree-create-parent-pick' import { repoHostId, withRepoHostOwnership } from '../listing/worktree-host-ownership' +import { WorktreeCreationCancelledError } from '@/lib/worktree-creation-attempt' type RuntimeTarget = ReturnType @@ -193,7 +194,7 @@ export function createCreateWorktree( for (let attempt = 0; attempt < CLIENT_WORKTREE_CREATE_MAX_ATTEMPTS; attempt += 1) { try { if (options?.isCancelled?.()) { - throw new Error('Worktree creation cancelled.') + throw new WorktreeCreationCancelledError('Worktree creation cancelled.') } const outcome = await runCreateAttempt( request, diff --git a/src/renderer/src/store/slices/worktrees/teardown/host-qualified-worktree-removal.ts b/src/renderer/src/store/slices/worktrees/teardown/host-qualified-worktree-removal.ts index 6bd38b04326..a67e9fcfde0 100644 --- a/src/renderer/src/store/slices/worktrees/teardown/host-qualified-worktree-removal.ts +++ b/src/renderer/src/store/slices/worktrees/teardown/host-qualified-worktree-removal.ts @@ -10,6 +10,7 @@ import type { WorktreeSlice } from '../../worktree-helpers' import type { getActiveRuntimeTarget } from '../../../../runtime/runtime-rpc-client' import type { Worktree } from '../../../../../../shared/worktree/types' import type { ExecutionHostId } from '../../../../../../shared/execution-host' +import { WORKTREE_INSTANCE_REPLACED_ERROR } from '../../worktree-removal-options' import { getWorktreeOperationOwnerHostIds, resolveWorktreeOperationRoute, @@ -73,10 +74,18 @@ export function beginHostQualifiedRemoval( worktreeId: string, requiredExecutionHostId: ExecutionHostId | null, forgetLocalOnly: boolean, - ignoreWorkspaceCleanupScanSurvivors = false + ignoreWorkspaceCleanupScanSurvivors = false, + expectedInstanceId?: string ): HostQualifiedRemovalStart { const resolveRemovalRoute = (): WorktreeOperationRoute | null => resolveHostQualifiedRemovalRoute(get, worktreeId, requiredExecutionHostId) + const replacedInstance = refuseReplacedWorktreeInstance( + findWorktreeOnConfirmedHost(get, worktreeId, requiredExecutionHostId), + expectedInstanceId + ) + if (replacedInstance) { + return { ok: false, error: replacedInstance } + } const removalRoute = resolveRemovalRoute() if (!removalRoute && (!forgetLocalOnly || !requiredExecutionHostId)) { // Why: callers mark rows deleting up front for immediate sidebar feedback @@ -159,16 +168,25 @@ export function refuseUnprovableRemoteHostRouting( ) } -/** The row on the confirmed host only — a same-id row elsewhere must not stand in for it. */ -export function assertWorktreeRemovalInstance( +/** + * Refuse a delayed cancellation rollback whose path was reused by another + * instance. Returns a reason instead of throwing so the caller can bail out + * before marking a row deleting — the id now belongs to a live workspace whose + * delete state must not carry this refusal. + */ +export function refuseReplacedWorktreeInstance( worktree: Worktree | undefined, expectedInstanceId?: string -): void { - if (expectedInstanceId && worktree?.instanceId !== expectedInstanceId) { - throw new Error('Workspace instance changed before cancellation cleanup.') +): string | null { + // An absent row is not proof of a replacement — the checkout may simply not be + // in local state yet — so leave that to the ordinary host-routing refusals. + if (!expectedInstanceId || !worktree) { + return null } + return worktree.instanceId === expectedInstanceId ? null : WORKTREE_INSTANCE_REPLACED_ERROR } +/** The row on the confirmed host only — a same-id row elsewhere must not stand in for it. */ export function findWorktreeOnConfirmedHost( get: WorktreeSliceGet, worktreeId: string, diff --git a/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts b/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts index 5f5424ca00f..4585a32a78d 100644 --- a/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts +++ b/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts @@ -15,7 +15,6 @@ import { } from '@/lib/worktree-operation-route' import { beginHostQualifiedRemoval, - assertWorktreeRemovalInstance, completeSameIdHostScopedRemoval, findWorktreeOnConfirmedHost, prepareHostScopedRemovalCompletion, @@ -59,7 +58,8 @@ export function createRemoveWorktree( worktreeId, requiredExecutionHostId, forgetLocalOnly, - options?.ignoreWorkspaceCleanupScanSurvivors === true + options?.ignoreWorkspaceCleanupScanSurvivors === true, + options?.expectedInstanceId ) if (!start.ok) { return { ok: false, error: start.error } @@ -107,7 +107,6 @@ export function createRemoveWorktree( worktreeId, requiredExecutionHostId ) - assertWorktreeRemovalInstance(worktreeBeforeRemoval, options?.expectedInstanceId) const terminalPtyIdsBeforeRemoval = (get().tabsByWorktree[worktreeId] ?? []).flatMap( (tab) => get().ptyIdsByTabId[tab.id] ?? [] )