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.
This commit is contained in:
Neil
2026-09-12 23:03:09 -07:00
parent c1afb65147
commit 60f46ed52c
2 changed files with 65 additions and 6 deletions
@@ -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
@@ -38,10 +38,20 @@ export async function withWorktreeCreationCancellation(
await execute(attempt)
}
} finally {
const cleanup = (deferred: boolean): Promise<boolean> =>
removeCancelledCreation(attempt, deferred).finally(() =>
const cleanup = async (deferred: boolean): Promise<boolean> => {
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<boolean> {
): Promise<CancelledCreationCleanup> {
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 }
}
}