From 60f46ed52ccbdf3a14fded69b2af39ca2a5183cf Mon Sep 17 00:00:00 2001 From: Neil Date: Sat, 12 Sep 2026 23:03:09 -0700 Subject: [PATCH] Keep the rollback obligation after a failed deferred cleanup A deferred rollback whose removal call failed still owns a real workspace, and its attempt record was the only reference to it. Releasing the attempt regardless of outcome made the retry guard single-use: the next retry found no previous attempt, created again, hit the name conflict, and left the user with the orphan plus a suffixed second workspace. Retain the attempt across retries and dismissals until a removal actually discharges it. Manual deletion still discharges it, because removing an already-unregistered worktree under force succeeds. An unidentified workspace is exempt. That refusal happens before any host call, so no retry can ever satisfy it, and retaining it would block retry forever behind an obligation nothing can clear. --- .../worktree-creation-cancellation.test.ts | 43 +++++++++++++++++++ .../src/lib/worktree-creation-cancellation.ts | 28 +++++++++--- 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/src/renderer/src/lib/worktree-creation-cancellation.test.ts b/src/renderer/src/lib/worktree-creation-cancellation.test.ts index 8d1b8285afa..39204299eaf 100644 --- a/src/renderer/src/lib/worktree-creation-cancellation.test.ts +++ b/src/renderer/src/lib/worktree-creation-cancellation.test.ts @@ -262,6 +262,49 @@ describe('worktree creation cancellation', () => { expect(toast.error).not.toHaveBeenCalled() }) + it('keeps blocking retry until a failed deferred rollback actually removes its workspace', async () => { + await withWorktreeCreationCancellation('creation', async (attempt) => { + attempt.worktree = worktree + }) + state.removeWorktree.mockResolvedValue({ ok: false, error: 'Host unavailable' }) + + const firstRetry = vi.fn() + await expect(withWorktreeCreationCancellation('creation', firstRetry)).rejects.toThrow( + 'Could not clean up' + ) + expect(firstRetry).not.toHaveBeenCalled() + + // The workspace still exists, so the obligation must survive the first retry + // rather than letting the next one create a second workspace beside it. + const secondRetry = vi.fn() + await expect(withWorktreeCreationCancellation('creation', secondRetry)).rejects.toThrow( + 'Could not clean up' + ) + expect(secondRetry).not.toHaveBeenCalled() + + state.removeWorktree.mockResolvedValue({ ok: true }) + const finalRetry = vi.fn() + await withWorktreeCreationCancellation('creation', finalRetry) + expect(finalRetry).toHaveBeenCalledOnce() + }) + + it('lets retry proceed once an unidentifiable workspace has been reported', async () => { + const unstamped = makeWorktree({ id: 'repo::/workspace', repoId: 'repo', hostId: 'ssh:owner' }) + await withWorktreeCreationCancellation('creation', async (attempt) => { + attempt.worktree = unstamped + }) + const blocked = vi.fn() + await expect(withWorktreeCreationCancellation('creation', blocked)).rejects.toThrow( + 'Could not clean up' + ) + // No removal was attempted, so retrying can never discharge it — do not + // block the user forever behind an obligation nothing can satisfy. + const retry = vi.fn() + await withWorktreeCreationCancellation('creation', retry) + expect(retry).toHaveBeenCalledOnce() + expect(state.removeWorktree).not.toHaveBeenCalled() + }) + it('lets retry proceed after the rollback found its workspace already replaced', async () => { await withWorktreeCreationCancellation('creation', async (attempt) => { attempt.worktree = worktree diff --git a/src/renderer/src/lib/worktree-creation-cancellation.ts b/src/renderer/src/lib/worktree-creation-cancellation.ts index e658c75e1d6..8f6ba04bca0 100644 --- a/src/renderer/src/lib/worktree-creation-cancellation.ts +++ b/src/renderer/src/lib/worktree-creation-cancellation.ts @@ -38,10 +38,20 @@ export async function withWorktreeCreationCancellation( await execute(attempt) } } finally { - const cleanup = (deferred: boolean): Promise => - removeCancelledCreation(attempt, deferred).finally(() => + const cleanup = async (deferred: boolean): Promise => { + const outcome = await removeCancelledCreation(attempt, deferred) + // Why: a deferred rollback whose removal call failed still owns a real + // workspace, and its attempt is the only record of it. Dropping that record + // would let the next retry create a second workspace beside the first, so + // keep the obligation until a removal actually discharges it. An unidentified + // workspace is exempt: no call was made, so retrying can never discharge it. + if (deferred && !outcome.ok && outcome.retryable) { + attempt.cleanupAfterSettlement = () => cleanup(deferred) + } else { releaseActiveWorktreeCreation(creationId, attempt) - ) + } + return outcome.ok + } if (!attempt.completed && attempt.isCancelled()) { await cleanup(false) } else if (!attempt.completed && attempt.worktree) { @@ -53,6 +63,9 @@ export async function withWorktreeCreationCancellation( } } +/** `retryable` is false when no removal was attempted, so retrying cannot help. */ +type CancelledCreationCleanup = { ok: boolean; retryable: boolean } + /** * `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 @@ -62,11 +75,14 @@ export async function withWorktreeCreationCancellation( async function removeCancelledCreation( attempt: WorktreeCreationAttempt, deferred: boolean -): Promise { +): Promise { const { worktree } = attempt + let retryable = true try { if (worktree) { if (deferred && !worktree.instanceId) { + // No removal is attempted, so no later retry can discharge this. + retryable = false throw new Error( 'it could not be identified on the host, so it was left in place. Delete it manually if unwanted.' ) @@ -92,7 +108,7 @@ async function removeCancelledCreation( ) } await attempt.cleanupRuntime?.() - return true + return { ok: true, retryable } } catch (error) { const message = error instanceof Error ? error.message : String(error) console.error('worktree create: cancellation cleanup failed', worktree?.id, error) @@ -105,6 +121,6 @@ async function removeCancelledCreation( toast.error(`Could not remove the cancelled workspace: ${message}${runtimeHint}`, { duration: Infinity }) - return false + return { ok: false, retryable } } }