diff --git a/src/main/runtime/forced-sweep-settlement.ts b/src/main/runtime/forced-sweep-settlement.ts new file mode 100644 index 00000000000..e53a0e66ce0 --- /dev/null +++ b/src/main/runtime/forced-sweep-settlement.ts @@ -0,0 +1,122 @@ +import { describeError } from './unstopped-pty-verification' + +// Why: an abandoned sweep is one whose provider ignored the deadline it was already given, +// so this is a second chance, not a second budget — long enough for a shutdown() that is +// mid-kill to finish releasing handles, short enough that Force Delete still feels forced. +export const ABANDONED_SWEEP_GRACE_MS = 2_000 + +export type WorktreeSweepTracker = { + /** Wraps a sweep body so the promise stays observable after its deadline abandons it. */ + track: (run: () => Promise) => () => Promise + /** Resolves when every tracked sweep settled, or the grace expired; true if one is still running. */ + awaitAbandonedSweeps: (graceMs?: number) => Promise +} + +/** + * Keeps the real sweep promises reachable after `settleBeforeDeadline` stops waiting on them. + * + * Why: that helper rejects on its timer without cancelling `run()`, so by construction the + * deadline error can only fire while work is still in flight — and forced removal deletes + * the worktree directory the moment teardown returns. + */ +export function createWorktreeSweepTracker(): WorktreeSweepTracker { + const running = new Set>() + return { + track: + (run: () => Promise) => + () => { + const sweep = run() + running.add(sweep) + // Why: the tracked copy is observed here too, so a rejection this call abandons + // never surfaces as an unhandled rejection in whatever ran next. + void sweep.catch(() => undefined).finally(() => running.delete(sweep)) + return sweep + }, + awaitAbandonedSweeps: async (graceMs = ABANDONED_SWEEP_GRACE_MS) => { + const pending = [...running] + if (pending.length === 0) { + return false + } + let expiry: ReturnType | undefined + const expired = new Promise((resolve) => { + expiry = setTimeout(() => resolve(true), graceMs) + expiry.unref?.() + }) + const stillRunning = await Promise.race([ + Promise.allSettled(pending).then(() => false), + expired + ]) + clearTimeout(expiry) + return stillRunning + } + } +} + +export type WorktreeSweepPromises = { + runtime: Promise<{ stopped: number }> + provider: Promise + registry: Promise +} + +export type ForcedSweepSettlement = { + stopped: { runtimeStopped: number; providerStopped: number; registryStopped: number } + /** A sweep failed outright, so no per-PTY verdict below it can be trusted. */ + incomplete: boolean +} + +/** + * Reconciles the three sweeps for a removal carrying the Force Delete waiver (#11960). + * + * Force goes on to delete files, so every sweep must finish releasing handles first — + * `Promise.all` would abandon the siblings of the first rejection while they were still + * inside shutdown(). Waiting on the tracked promises rather than only on their deadline + * wrappers is what makes that true for the timeout case as well. + */ +export async function settleSweepsForForcedRemoval( + worktreeId: string, + sweeps: WorktreeSweepPromises, + tracker: WorktreeSweepTracker, + deadlineError: Error +): Promise { + const settled = await Promise.allSettled([sweeps.runtime, sweeps.provider, sweeps.registry]) + const [runtimeSettled, providerSettled, registrySettled] = settled + // Report what the surviving sweeps actually stopped rather than a flat zero. + const stopped = { + runtimeStopped: runtimeSettled.status === 'fulfilled' ? runtimeSettled.value.stopped : 0, + providerStopped: providerSettled.status === 'fulfilled' ? providerSettled.value : 0, + registryStopped: registrySettled.status === 'fulfilled' ? registrySettled.value : 0 + } + const reasons = settled.flatMap((result) => + result.status === 'rejected' ? [result.reason as unknown] : [] + ) + const stillRunning = await tracker.awaitAbandonedSweeps() + if (reasons.length === 0 && !stillRunning) { + return { stopped, incomplete: false } + } + // Why (#11960): a sweep that cannot even complete — unresponsive daemon, dropped SSH + // channel — fails before the unproven-stop gate could offer its escape hatch, so an + // explicit Force Delete has to survive it. Report every reason, specific ones first: the + // shared deadline sentinel only says that *something* timed out, so leading with it would + // bury the provider error that actually explains the failure. This warning is the only + // trace of a removal that deleted files without proving a single stop. + const detail = [ + ...reasons.filter((candidate) => candidate !== deadlineError), + ...reasons.filter((candidate) => candidate === deadlineError) + ].map(describeError) + if (stillRunning) { + // Force must never wedge, so the wait is bounded — but "we stopped waiting" is not + // "handles released", and on Windows/WSL that is the difference between a clean + // removal and a half-deleted directory. Say so instead of implying the sweep finished. + detail.push( + `a sweep was still running after the ${ABANDONED_SWEEP_GRACE_MS}ms grace, so its PTY handles may outlive the delete` + ) + } + console.warn( + `[worktree-teardown] forcing removal after an incomplete PTY sweep for ${worktreeId} — ${detail.join('; ')}` + ) + // Reporting `incomplete` does skip the per-PTY verdict, which could still have named live + // PTYs when only one sweep failed — accepted for now because this path is behind an + // explicit Force Delete, deletes either way, and clears no registry rows, so the cost is + // diagnosability rather than safety. + return { stopped, incomplete: reasons.length > 0 } +} diff --git a/src/main/runtime/unstopped-pty-verification.ts b/src/main/runtime/unstopped-pty-verification.ts index b0df601f133..068334eb98d 100644 --- a/src/main/runtime/unstopped-pty-verification.ts +++ b/src/main/runtime/unstopped-pty-verification.ts @@ -1,5 +1,9 @@ import type { IPtyProvider } from '../providers/types' -import { UNSTOPPED_PTY_REMOVAL_PREFIX } from '../../shared/worktree-removal' +import { + UNSTOPPED_PTY_DETAIL_SEPARATOR, + UNSTOPPED_PTY_LIVE_DETAIL_PREFIX, + UNSTOPPED_PTY_REMOVAL_PREFIX +} from '../../shared/worktree-removal' import { settleBeforeDeadline } from './settle-before-deadline' // Floor for the verification window when the sweep ran on a very short budget. @@ -60,9 +64,21 @@ export function describeUnstoppedPtys( ): string { const detail = verdict.status === 'live' - ? `still live: ${verdict.ptyIds.join(', ')}` + ? `${UNSTOPPED_PTY_LIVE_DETAIL_PREFIX} ${verdict.ptyIds.join(', ')}` : `could not verify these exited: ${failedPtyIds.join(', ')} (${verdict.reason})` - return `${UNSTOPPED_PTY_REMOVAL_PREFIX} ${worktreeId} — ${detail}` + return `${UNSTOPPED_PTY_REMOVAL_PREFIX} ${worktreeId}${UNSTOPPED_PTY_DETAIL_SEPARATOR}${detail}` +} + +/** + * Words a sweep that never produced a per-PTY verdict — a wedged daemon, a dropped SSH + * channel — as the unstopped-PTY failure it is. + * + * Why (#11960): this rejection carried only the provider's own wording, which the force + * classifier cannot recognise, so the wedge the escape hatch exists for was the one case + * that never got offered it. The provider's message stays in the text; only the shape changes. + */ +export function describeFailedPtySweep(worktreeId: string, error: unknown): string { + return `${UNSTOPPED_PTY_REMOVAL_PREFIX} ${worktreeId}${UNSTOPPED_PTY_DETAIL_SEPARATOR}the terminal sweep failed: ${describeError(error)}` } export function describeError(error: unknown): string { diff --git a/src/main/runtime/worktree-teardown-unstopped-pty.test.ts b/src/main/runtime/worktree-teardown-unstopped-pty.test.ts index 47852b45985..ca09e01b2d8 100644 --- a/src/main/runtime/worktree-teardown-unstopped-pty.test.ts +++ b/src/main/runtime/worktree-teardown-unstopped-pty.test.ts @@ -9,6 +9,11 @@ vi.mock('../memory/pty-registry', () => ({ })) import { killAllProcessesForWorktree, WORKTREE_PROCESS_SWEEP_TIMEOUT_MS } from './worktree-teardown' +import { ABANDONED_SWEEP_GRACE_MS } from './forced-sweep-settlement' +import { + classifyWorktreeForceDeleteReason, + isProvenLivePtyRemovalError +} from '../../shared/worktree-removal' import type { IPtyProvider, PtyProcessInfo } from '../providers/types' // Why: these tests advance fake timers *before* awaiting the teardown, so a @@ -243,6 +248,13 @@ describe('destructive teardown when a PTY stop cannot be proven', () => { providerStopped: 0, registryStopped: 0 }) + // Why: the deadline gave up on this sweep without cancelling it, so the grace + // expired with the provider still inside listProcesses. Force deletes the + // directory anyway, and this warning is the only record that a PTY handle may + // have outlived it — a silent "incomplete sweep" reads as if it had finished. + expect(warn).toHaveBeenCalledWith( + expect.stringContaining(`still running after the ${ABANDONED_SWEEP_GRACE_MS}ms grace`) + ) } finally { releaseList([]) await vi.advanceTimersByTimeAsync(0) @@ -337,6 +349,79 @@ describe('destructive teardown when a PTY stop cannot be proven', () => { ).rejects.toThrow('ssh channel closed') }) + // Why (#11960): the wedge the escape hatch exists for — an unresponsive daemon, a dropped + // SSH channel — rejects the sweep in the provider's own words, which the force classifier + // cannot recognise. Failing closed with no Force Delete button is the dead end itself. + it('offers force delete for a sweep-level failure without losing the provider wording', async () => { + listRegisteredPtysMock.mockReturnValue([]) + const localProvider = createProviderStub(async () => { + throw new Error('SSH channel closed while listing processes') + }) + + const error = await killAllProcessesForWorktree('repo-1::/w', { + localProvider, + requirePhysicalStop: true + }).then( + () => new Error('expected a rejection'), + (rejection: Error) => rejection + ) + expect(error.message).toContain('SSH channel closed while listing processes') + expect(classifyWorktreeForceDeleteReason(error.message)).toBe('unstopped-pty') + // Nothing was verified here, so it must not read as the proven-live verdict. + expect(isProvenLivePtyRemovalError(error.message)).toBe(false) + }) + + // Why: the outer deadline rejects without cancelling the sweep it gave up on, so force + // could return — and the caller start deleting the directory — while shutdown() was still + // killing the PTY holding it open. On Windows that rmdir fails and half-deletes the + // workspace, which is exactly what the ordering at the removal call site exists to avoid. + it('waits for a shutdown the deadline abandoned before force returns', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + let releaseShutdown: () => void = () => {} + try { + listRegisteredPtysMock.mockReturnValue([]) + const localProvider = createProviderStub(async () => [ + { id: 'w1@@live-1', cwd: '/tmp/w1', title: 'shell' } + ]) + let shutdownFinished = false + ;(localProvider.shutdown as unknown as ReturnType).mockImplementation( + async () => + await new Promise((resolve) => { + releaseShutdown = () => { + shutdownFinished = true + resolve() + } + }) + ) + + const teardown = settleTeardown( + killAllProcessesForWorktree('w1', { + localProvider, + timeoutMs: 30, + requirePhysicalStop: true, + allowUnverifiedStop: true + }) + ) + let returned = false + void teardown.then(() => { + returned = true + }) + await new Promise((resolve) => setTimeout(resolve, 150)) + + expect(shutdownFinished).toBe(false) + expect(returned).toBe(false) + releaseShutdown() + await expect(teardown).resolves.toEqual({ + runtimeStopped: 0, + providerStopped: 0, + registryStopped: 0 + }) + } finally { + releaseShutdown() + warn.mockRestore() + } + }) + it('lets an explicit force removal proceed past PTYs it could not stop', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) try { diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index 45929e21f98..993a263d348 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -4,10 +4,16 @@ import { listRegisteredPtys } from '../memory/pty-registry' import { isPathInsideOrEqual } from '../../shared/cross-platform-path' import { splitWorktreeId, splitWorktreeIdForFilesystem } from '../../shared/worktree-id' import { mapWithConcurrency } from '../../shared/map-with-concurrency' -import { WORKTREE_TEARDOWN_FORCE_HINT } from '../../shared/worktree-removal' +import { + isUnstoppedPtyRemovalError, + WORKTREE_TEARDOWN_FORCE_HINT, + WORKTREE_TEARDOWN_TIMEOUT_PREFIX +} from '../../shared/worktree-removal' import { settleBeforeDeadline } from './settle-before-deadline' +import { createWorktreeSweepTracker, settleSweepsForForcedRemoval } from './forced-sweep-settlement' import { describeError, + describeFailedPtySweep, describeUnstoppedPtys, verifyUnstoppedPtys, type UnstoppedPtyVerdict @@ -87,7 +93,10 @@ export async function killAllProcessesForWorktree( ): Promise { const sweepBudgetMs = Math.max(1, deps.timeoutMs ?? WORKTREE_PROCESS_SWEEP_TIMEOUT_MS) const deadline = Date.now() + sweepBudgetMs - const deadlineError = new Error(`Timed out waiting for physical PTY teardown: ${worktreeId}`) + const deadlineError = new Error( + `${WORKTREE_TEARDOWN_TIMEOUT_PREFIX} ${worktreeId}. ${WORKTREE_TEARDOWN_FORCE_HINT}` + ) + const sweeps = createWorktreeSweepTracker() const stopAttempts = new Map>() const stopPty = ( ptyId: string, @@ -116,7 +125,7 @@ export async function killAllProcessesForWorktree( // destructive removal closed. const runtimeSweep = deps.runtime ? settleBeforeDeadline( - () => + sweeps.track(() => deps.runtime!.stopTerminalsForWorktree(worktreeId, { deadline, stopPty, @@ -127,7 +136,8 @@ export async function killAllProcessesForWorktree( ...(deps.resolvedRuntimeEnvironmentId ? { resolvedRuntimeEnvironmentId: deps.resolvedRuntimeEnvironmentId } : {}) - }), + }) + ), { stopped: 0 }, deadline, deps.requirePhysicalStop ? deadlineError : undefined, @@ -142,7 +152,7 @@ export async function killAllProcessesForWorktree( deps.includeProviderInventory === false ? Promise.resolve(0) : settleBeforeDeadline( - () => + sweeps.track(() => sweepProviderByPrefix( worktreeId, deps.localProvider, @@ -150,7 +160,8 @@ export async function killAllProcessesForWorktree( stopPty, deps.onPtyStopped, deps.requirePhysicalStop - ), + ) + ), 0, deadline, deps.requirePhysicalStop ? deadlineError : undefined @@ -159,14 +170,15 @@ export async function killAllProcessesForWorktree( deps.includeLocalRegistry === false ? Promise.resolve(0) : settleBeforeDeadline( - () => + sweeps.track(() => sweepRegistryForWorktree( worktreeId, deps.localProvider, deadline, stopPty, deps.onPtyStopped - ), + ) + ), 0, deadline, deps.requirePhysicalStop ? deadlineError : undefined @@ -180,54 +192,39 @@ export async function killAllProcessesForWorktree( let providerStopped: number let registryStopped: number if (deps.allowUnverifiedStop) { - // Why: force goes on to delete files, so every sweep must finish releasing - // handles first — Promise.all would abandon the siblings of the first - // rejection while they were still inside shutdown(), racing the delete. - const settled = await Promise.allSettled([runtimeSweep, providerSweep, registrySweep]) - const [runtimeSettled, providerSettled, registrySettled] = settled - const reasons = settled.flatMap((result) => - result.status === 'rejected' ? [result.reason as unknown] : [] + const forced = await settleSweepsForForcedRemoval( + worktreeId, + { runtime: runtimeSweep, provider: providerSweep, registry: registrySweep }, + sweeps, + deadlineError ) - if (reasons.length > 0) { - // Why (#11960): a sweep that cannot even complete — unresponsive daemon, - // dropped SSH channel — fails before the unproven-stop gate below could - // offer its escape hatch, so an explicit Force Delete has to survive it. - // Report every reason, specific ones first: the shared deadline sentinel - // only says that *something* timed out, so leading with it would bury the - // provider error that actually explains the failure. This warning is the - // only trace of a removal that deleted files without proving a single stop. - const detail = [ - ...reasons.filter((candidate) => candidate !== deadlineError), - ...reasons.filter((candidate) => candidate === deadlineError) - ] - .map(describeError) - .join('; ') - console.warn( - `[worktree-teardown] forcing removal after an incomplete PTY sweep for ${worktreeId} — ${detail}` - ) - // Returning here does skip the verdict below, which could still have named - // live PTYs when only one sweep failed — accepted for now because this path - // is behind an explicit Force Delete, deletes either way, and clears no - // registry rows, so the cost is diagnosability rather than safety. - // Report what the surviving sweeps actually stopped rather than a flat zero. - return { - runtimeStopped: runtimeSettled.status === 'fulfilled' ? runtimeSettled.value.stopped : 0, - providerStopped: providerSettled.status === 'fulfilled' ? providerSettled.value : 0, - registryStopped: registrySettled.status === 'fulfilled' ? registrySettled.value : 0 - } + if (forced.incomplete) { + return forced.stopped } - runtimeResult = (runtimeSettled as PromiseFulfilledResult<{ stopped: number }>).value - providerStopped = (providerSettled as PromiseFulfilledResult).value - registryStopped = (registrySettled as PromiseFulfilledResult).value + runtimeResult = { stopped: forced.stopped.runtimeStopped } + providerStopped = forced.stopped.providerStopped + registryStopped = forced.stopped.registryStopped } else { // Why: without the waiver a rejection aborts the removal and nothing is // deleted, so failing fast is safe — and keeps a dead host reporting // immediately instead of after the full sweep budget. - ;[runtimeResult, providerStopped, registryStopped] = await Promise.all([ - runtimeSweep, - providerSweep, - registrySweep - ]) + try { + ;[runtimeResult, providerStopped, registryStopped] = await Promise.all([ + runtimeSweep, + providerSweep, + registrySweep + ]) + } catch (error) { + // Why (#11960): this rejection is the provider's own wording, which the force + // classifier cannot recognise — so the wedge Force Delete exists for was the one + // failure that never offered it. Re-word it, keeping the original as the cause. + throw deps.requirePhysicalStop && !isUnstoppedPtyRemovalError(describeError(error)) + ? new Error( + `${describeFailedPtySweep(worktreeId, error)}. ${WORKTREE_TEARDOWN_FORCE_HINT}`, + { cause: error } + ) + : error + } } if (deps.requirePhysicalStop) { const stopResults = await Promise.all( diff --git a/src/renderer/src/components/sidebar/delete-worktree-toast.test.ts b/src/renderer/src/components/sidebar/delete-worktree-toast.test.ts index 16bf7c4b954..acaa7d8f16f 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-toast.test.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-toast.test.ts @@ -1,5 +1,13 @@ import { describe, expect, it } from 'vitest' import { getDeleteWorktreeToastCopy } from './delete-worktree-toast' +import { classifyWorktreeForceDeleteReason } from '../../../../shared/worktree-removal' + +// Why: production never hands this function a literal reason — the store derives it from +// classifyWorktreeForceDeleteReason (store/slices/worktrees.ts). Passing one in would let a +// message the classifier rejects still render the force copy, testing a UI that never runs. +function toastCopyForRemovalError(worktreeName: string, error: string): unknown { + return getDeleteWorktreeToastCopy(worktreeName, classifyWorktreeForceDeleteReason(error), error) +} describe('getDeleteWorktreeToastCopy', () => { it('uses direct guidance when force delete is available', () => { @@ -14,11 +22,58 @@ describe('getDeleteWorktreeToastCopy', () => { // toast has to actually offer it — a reason of null hides the button entirely. it('uses terminal-teardown guidance when a PTY stop could not be proven', () => { expect( - getDeleteWorktreeToastCopy( + toastCopyForRemovalError( + 'feature/foo', + 'Failed to physically stop every PTY for worktree: repo-1::/w — could not verify these exited: term_a (the process list timed out)' + ) + ).toEqual({ + title: 'Failed to delete workspace feature/foo', + description: + 'Orca could not confirm every terminal in this workspace has exited, so it stopped before deleting any files. Use Force Delete to remove it anyway.', + isDestructive: false + }) + }) + + // Why: Force Delete proceeds on a proven-live PTY too, so the copy must not describe + // that as an unconfirmed exit — the user is killing a terminal Orca watched running. + it('names the running terminals when verification proved they are still live', () => { + expect( + toastCopyForRemovalError( 'feature/foo', - 'unstopped-pty', 'Failed to physically stop every PTY for worktree: repo-1::/w — still live: term_a' ) + ).toEqual({ + title: 'Failed to delete workspace feature/foo', + description: + 'This workspace still has running terminals, so Orca stopped before deleting any files. Force Delete will kill them and discard any uncommitted work they hold.', + isDestructive: false + }) + }) + + // Why: a sweep that never answered wedges removal the same way, and the waiver clears + // both — so it must reach the same force affordance instead of a dead end. + it('offers force delete when the teardown sweep itself timed out', () => { + expect( + toastCopyForRemovalError( + 'feature/foo', + 'Timed out waiting for physical PTY teardown: repo-1::/w. Retry with force delete (--force) to remove it anyway.' + ) + ).toEqual({ + title: 'Failed to delete workspace feature/foo', + description: + 'Orca could not confirm every terminal in this workspace has exited, so it stopped before deleting any files. Use Force Delete to remove it anyway.', + isDestructive: false + }) + }) + + // Why: the same wedge worded by the provider instead of the deadline — a dropped SSH + // channel — must reach the same offer, or the escape hatch misses the case it was for. + it('offers force delete when the sweep failed rather than timed out', () => { + expect( + toastCopyForRemovalError( + 'feature/foo', + 'Failed to physically stop every PTY for worktree: repo-1::/w — the terminal sweep failed: SSH channel closed while listing processes. Retry with force delete (--force) to remove it anyway.' + ) ).toEqual({ title: 'Failed to delete workspace feature/foo', description: diff --git a/src/renderer/src/components/sidebar/delete-worktree-toast.ts b/src/renderer/src/components/sidebar/delete-worktree-toast.ts index 22f7eb7f5ad..fac1c6ee06a 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-toast.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-toast.ts @@ -1,6 +1,7 @@ import { translate } from '@/i18n/i18n' import { isLockedWorktreeRemovalError, + isProvenLivePtyRemovalError, type WorktreeForceDeleteReason } from '../../../../shared/worktree-removal' export type DeleteWorktreeToastCopy = { @@ -58,10 +59,18 @@ export function getDeleteWorktreeToastCopy( 'Failed to delete workspace {{value0}}', { value0: worktreeName } ), - description: translate( - 'auto.components.sidebar.delete.worktree.toast.unstoppedPty', - 'Orca could not confirm every terminal in this workspace has exited, so it stopped before deleting any files. Use Force Delete to remove it anyway.' - ), + // Why: Force Delete proceeds either way, so the copy must say which case this is. + // Telling a user "could not confirm" about terminals Orca watched stay alive asks + // them to waive a doubt that does not exist, and any running agent's work dies with it. + description: isProvenLivePtyRemovalError(error) + ? translate( + 'auto.components.sidebar.delete.worktree.toast.unstoppedPtyLive', + 'This workspace still has running terminals, so Orca stopped before deleting any files. Force Delete will kill them and discard any uncommitted work they hold.' + ) + : translate( + 'auto.components.sidebar.delete.worktree.toast.unstoppedPty', + 'Orca could not confirm every terminal in this workspace has exited, so it stopped before deleting any files. Use Force Delete to remove it anyway.' + ), isDestructive: false } } diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index ad63afa7aff..e9b8965451b 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -4769,7 +4769,8 @@ "0899ebdb28": "Git already forgot this workspace, but its directory is still on disk. Use Force Delete to remove the orphaned directory.", "locked": "This workspace is locked by Git. Run git worktree unlock from its repository, then retry deletion.", "lockedReason": "This workspace is locked by Git. Git reported: {{value0}}. Run git worktree unlock from its repository, then retry deletion.", - "unstoppedPty": "Orca could not confirm every terminal in this workspace has exited, so it stopped before deleting any files. Use Force Delete to remove it anyway." + "unstoppedPty": "Orca could not confirm every terminal in this workspace has exited, so it stopped before deleting any files. Use Force Delete to remove it anyway.", + "unstoppedPtyLive": "This workspace still has running terminals, so Orca stopped before deleting any files. Force Delete will kill them and discard any uncommitted work they hold." } } }, diff --git a/src/shared/worktree-removal-force-classification.test.ts b/src/shared/worktree-removal-force-classification.test.ts index 1d995d412f4..b4d95a5727d 100644 --- a/src/shared/worktree-removal-force-classification.test.ts +++ b/src/shared/worktree-removal-force-classification.test.ts @@ -1,13 +1,19 @@ import { describe, expect, it } from 'vitest' -import { classifyWorktreeForceDeleteReason, WORKTREE_TEARDOWN_FORCE_HINT } from './worktree-removal' +import { + classifyWorktreeForceDeleteReason, + isProvenLivePtyRemovalError, + UNSTOPPED_PTY_REMOVAL_PREFIX, + WORKTREE_TEARDOWN_FORCE_HINT, + WORKTREE_TEARDOWN_TIMEOUT_PREFIX +} from './worktree-removal' + +const liveError = `${UNSTOPPED_PTY_REMOVAL_PREFIX} repo-1::/w — still live: term_a. ${WORKTREE_TEARDOWN_FORCE_HINT}` +const unverifiableError = `${UNSTOPPED_PTY_REMOVAL_PREFIX} repo-1::/w — could not verify these exited: term_a (daemon socket closed). ${WORKTREE_TEARDOWN_FORCE_HINT}` // Why (#11960): the desktop Force Delete button renders only when this classifier // returns a reason. The PTY-teardown error tells the user to force-delete, so an // unclassified message leaves them reading advice they cannot act on. describe('classifyWorktreeForceDeleteReason for unstopped PTYs', () => { - const liveError = `Failed to physically stop every PTY for worktree: repo-1::/w — still live: term_a. ${WORKTREE_TEARDOWN_FORCE_HINT}` - const unverifiableError = `Failed to physically stop every PTY for worktree: repo-1::/w — could not verify these exited: term_a (daemon socket closed). ${WORKTREE_TEARDOWN_FORCE_HINT}` - it('offers force for a PTY that is still live', () => { expect(classifyWorktreeForceDeleteReason(liveError)).toBe('unstopped-pty') }) @@ -29,7 +35,35 @@ describe('classifyWorktreeForceDeleteReason for unstopped PTYs', () => { expect(classifyWorktreeForceDeleteReason(liveError, false, true)).toBeNull() }) + // Why: a wedged provider rejects the sweep before any per-PTY verdict exists, so the + // failure arrives worded as a teardown timeout. The waiver clears it exactly like an + // unproven stop, so leaving it unclassified hid the button for the wedge #11960 targeted. + it('offers force when the teardown sweep itself timed out', () => { + expect( + classifyWorktreeForceDeleteReason( + `${WORKTREE_TEARDOWN_TIMEOUT_PREFIX} repo-1::/w. ${WORKTREE_TEARDOWN_FORCE_HINT}` + ) + ).toBe('unstopped-pty') + }) + it('leaves unrelated failures unclassified', () => { expect(classifyWorktreeForceDeleteReason('some other failure')).toBeNull() }) }) + +// The live verdict escalates the delete toast to "Force Delete will kill them", so it must +// come from Orca's own detail — not from the worktree id, which is a user-chosen path. +describe('isProvenLivePtyRemovalError', () => { + it('reads the verdict Orca actually recorded', () => { + expect(isProvenLivePtyRemovalError(liveError)).toBe(true) + expect(isProvenLivePtyRemovalError(unverifiableError)).toBe(false) + }) + + it('does not let a worktree path spell out a live verdict', () => { + expect( + isProvenLivePtyRemovalError( + `${UNSTOPPED_PTY_REMOVAL_PREFIX} repo-1::/Users/dev/still live: notes — could not verify these exited: term_a (daemon socket closed)` + ) + ).toBe(false) + }) +}) diff --git a/src/shared/worktree-removal.ts b/src/shared/worktree-removal.ts index 17e1b719cae..8788171ebe0 100644 --- a/src/shared/worktree-removal.ts +++ b/src/shared/worktree-removal.ts @@ -15,8 +15,34 @@ export type WorktreeForceDeleteReason = | 'missing-registration' | 'unstopped-pty' +// Why: everything before this separator is the worktree id — a user-chosen filesystem path. +// Only the detail after it is Orca's own wording, so verdict matchers anchor on the boundary +// rather than scanning the whole message and letting a path spell out a verdict. +export const UNSTOPPED_PTY_DETAIL_SEPARATOR = ' — ' + +// Why: verification distinguishes a PTY it watched stay alive from one it could not reach, +// and the delete toast must not flatten the two — a user waiving "we could not confirm" is +// making a different decision than one killing a terminal Orca just saw running. The marker +// and its matcher stay together for the same reason the force hint does. +export const UNSTOPPED_PTY_LIVE_DETAIL_PREFIX = 'still live:' + +// Why (#11960): a sweep that never answers wedges removal exactly like a stop that could not +// be proven, and the waiver clears both — but this error carries different words, so without +// its own matcher the force affordance stayed hidden for the very case it was added for. +export const WORKTREE_TEARDOWN_TIMEOUT_PREFIX = 'Timed out waiting for physical PTY teardown:' + export function isUnstoppedPtyRemovalError(error: string): boolean { - return error.includes(UNSTOPPED_PTY_REMOVAL_PREFIX) + return ( + error.includes(UNSTOPPED_PTY_REMOVAL_PREFIX) || error.includes(WORKTREE_TEARDOWN_TIMEOUT_PREFIX) + ) +} + +/** True only when verification positively observed the PTYs still running. */ +export function isProvenLivePtyRemovalError(error: string): boolean { + return ( + isUnstoppedPtyRemovalError(error) && + error.includes(`${UNSTOPPED_PTY_DETAIL_SEPARATOR}${UNSTOPPED_PTY_LIVE_DETAIL_PREFIX}`) + ) } export function createLockedWorktreeRemovalError(lockReason?: string): Error {