diff --git a/src/cli/handlers/worktree.ts b/src/cli/handlers/worktree.ts index 597ee437e10..51614029a99 100644 --- a/src/cli/handlers/worktree.ts +++ b/src/cli/handlers/worktree.ts @@ -280,6 +280,8 @@ export const WORKTREE_HANDLERS: Record = { const result = await client.call('worktree.rm', { worktree: await getRequiredWorktreeSelector(flags, 'worktree', cwd, client), force: flags.get('force') === true, + // Why (#11960): --force is explicit here, so it may also waive PTY-stop proof. + allowUnverifiedPtyStop: flags.get('force') === true, runHooks: flags.get('run-hooks') === true }) printHookWarning(result.result, json) diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 811fae77b8d..62be1f63fa1 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -9950,6 +9950,56 @@ describe('registerWorktreeHandlers', () => { expect(callOrder).toEqual(['preflight', 'kill', 'git']) }) + // Why (#11960): the PTY gate previously had no escape hatch at all, so a + // workspace with an unprovable PTY was unremovable forever. + it('forwards an explicit Force Delete to the PTY gate', async () => { + mockKnownFeatureWorktree() + getEffectiveHooksMock.mockReturnValue(null) + + await handlers['worktrees:remove'](null, { + worktreeId: 'repo-1::/workspace/feature-wt', + force: true, + allowUnverifiedPtyStop: true + }) + + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith( + 'repo-1::/workspace/feature-wt', + expect.objectContaining({ requirePhysicalStop: true, allowUnverifiedStop: true }) + ) + }) + + // Why (#11960): the ordinary Delete confirmation already sets force:true to skip + // the dirty-file prompt. Waiving PTY-stop proof off that signal would silently + // disable the gate on the primary delete path. + it('keeps the PTY gate strict for a confirmed delete that only sets force', async () => { + mockKnownFeatureWorktree() + getEffectiveHooksMock.mockReturnValue(null) + + await handlers['worktrees:remove'](null, { + worktreeId: 'repo-1::/workspace/feature-wt', + force: true + }) + + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith( + 'repo-1::/workspace/feature-wt', + expect.not.objectContaining({ allowUnverifiedStop: true }) + ) + }) + + it('keeps the PTY gate strict for a plain delete', async () => { + mockKnownFeatureWorktree() + getEffectiveHooksMock.mockReturnValue(null) + + await handlers['worktrees:remove'](null, { + worktreeId: 'repo-1::/workspace/feature-wt' + }) + + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith( + 'repo-1::/workspace/feature-wt', + expect.not.objectContaining({ allowUnverifiedStop: true }) + ) + }) + it('does not start Git removal when physical PTY teardown cannot be proven', async () => { mockKnownFeatureWorktree() getEffectiveHooksMock.mockReturnValue(null) diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index 103464dc55e..b9f2a57cec3 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -150,6 +150,8 @@ type RemoveWorktreeArgs = { worktreeId: string hostId?: ExecutionHostId force?: boolean + /** Explicit Force Delete only — `force` alone is set by the ordinary confirmation (#11960). */ + allowUnverifiedPtyStop?: boolean skipArchive?: boolean } @@ -158,8 +160,9 @@ type DetectedWorktreeRequestArgs = { repoId: string } | ListDetectedWorktreesArg async function stopPtysForDestructiveWorktreeRemoval( runtime: OrcaRuntimeService, worktreeId: string, - connectionId?: string + options: { connectionId?: string; allowUnverifiedStop?: boolean } = {} ): Promise { + const { connectionId, allowUnverifiedStop } = options const provider = connectionId ? getSshPtyProvider(connectionId) : getLocalPtyProvider() if (!provider) { throw new Error(`PTY provider unavailable for worktree deletion: ${worktreeId}`) @@ -169,6 +172,9 @@ async function stopPtysForDestructiveWorktreeRemoval( localProvider: provider, onPtyStopped: clearProviderPtyState, requirePhysicalStop: true, + // Why (#11960): set only by an explicit Force Delete, never by the ordinary + // confirmation — otherwise the gate would be off on the primary delete path. + ...(allowUnverifiedStop ? { allowUnverifiedStop: true } : {}), ...(connectionId ? { includeLocalRegistry: false } : {}) }) const total = @@ -419,10 +425,17 @@ function gitStatusErrorMeansNotRepository(error: unknown): boolean { return /not a git repository/i.test(`${message}\n${stderr}`) } -function getWorktreeRemovalOptionsKey(args: { force?: boolean; skipArchive?: boolean }): string { +function getWorktreeRemovalOptionsKey(args: { + force?: boolean + allowUnverifiedPtyStop?: boolean + skipArchive?: boolean +}): string { const forceKey = args.force === true ? 'force' : 'normal' const archiveKey = args.skipArchive === true ? 'skip-archive' : 'run-archive' - return `${forceKey}:${archiveKey}` + // Why: a Force Delete retry must not coalesce onto the in-flight attempt that + // just failed the PTY gate — it would inherit that failure instead of retrying. + const ptyKey = args.allowUnverifiedPtyStop === true ? 'allow-unverified-pty' : 'require-pty-stop' + return `${forceKey}:${archiveKey}:${ptyKey}` } function getWorktreeRemovalInFlightKey(worktreeId: string, hostId?: ExecutionHostId): string { @@ -2358,11 +2371,10 @@ export function registerWorktreeHandlers( ) let removalCompleted = false try { - await stopPtysForDestructiveWorktreeRemoval( - runtime, - args.worktreeId, - repo.connectionId - ) + await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId, { + connectionId: repo.connectionId, + allowUnverifiedStop: args.allowUnverifiedPtyStop + }) await fsProvider!.deletePath(worktreePath, true) removalCompleted = true } finally { @@ -2379,7 +2391,9 @@ export function registerWorktreeHandlers( const removalGate = await runtime.acquireFileWatcherRemoval(worktreePath) let removalCompleted = false try { - await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId) + await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId, { + allowUnverifiedStop: args.allowUnverifiedPtyStop + }) await removeLocalWorktreePath(worktreePath, localWorktreeGitOptions) removalCompleted = true } finally { @@ -2424,7 +2438,9 @@ export function registerWorktreeHandlers( const removalGate = await runtime.acquireFileWatcherRemoval(worktreePath) let removalCompleted = false try { - await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId) + await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId, { + allowUnverifiedStop: args.allowUnverifiedPtyStop + }) await removeLocalWorktreePath(worktreePath, localWorktreeGitOptions) removalCompleted = true } finally { @@ -2582,11 +2598,10 @@ export function registerWorktreeHandlers( let removalCompleted = false try { await withWorktreeRemoveStageSpan('pty_sweep', 'remote', async () => { - await stopPtysForDestructiveWorktreeRemoval( - runtime, - args.worktreeId, - remoteConnectionId - ) + await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId, { + connectionId: remoteConnectionId, + allowUnverifiedStop: args.allowUnverifiedPtyStop + }) }) rawRemovalResult = await withWorktreeRemoveStageSpan( 'git_remove', @@ -2689,7 +2704,9 @@ export function registerWorktreeHandlers( // Why: hold the watcher/terminal gate through Git and any recursive fallback so no late spawn recreates a native handle. // Linked-path deletion is destructive too, so PTYs must release every handle before Windows or WSL filesystem cleanup starts. await withWorktreeRemoveStageSpan('pty_sweep', 'local', async () => { - await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId) + await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId, { + allowUnverifiedStop: args.allowUnverifiedPtyStop + }) }) // Why: preflight only ignored these paths, not mutated them; keep watcher installs fenced through Git removal. diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index dfae2861876..2ff118b2e81 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1963,8 +1963,15 @@ type PreservedBranchCleanupTarget = { pushTarget?: GitPushTarget } -function getRuntimeWorktreeRemovalOptionsKey(force: boolean, runHooks: boolean): string { - return `${force ? 'force' : 'normal'}:${runHooks ? 'run-hooks' : 'skip-hooks'}` +function getRuntimeWorktreeRemovalOptionsKey( + force: boolean, + runHooks: boolean, + allowUnverifiedPtyStop: boolean +): string { + // Why: a forced retry must not coalesce onto the in-flight attempt that just + // failed the PTY gate — it would inherit that failure instead of retrying. + const ptyKey = allowUnverifiedPtyStop ? 'allow-unverified-pty' : 'require-pty-stop' + return `${force ? 'force' : 'normal'}:${runHooks ? 'run-hooks' : 'skip-hooks'}:${ptyKey}` } function getRuntimeFolderWorkspaceRootId(repo: Repo): string { @@ -3314,8 +3321,9 @@ export class OrcaRuntimeService { private async stopPtysForDestructiveWorktreeRemoval( worktreeId: string, - connectionId?: string + options: { connectionId?: string; allowUnverifiedStop?: boolean } = {} ): Promise { + const { connectionId, allowUnverifiedStop } = options const provider = connectionId ? this.getSshProviderFn?.(connectionId) : this.getLocalProvider() if (!provider) { throw new Error(`PTY provider unavailable for worktree deletion: ${worktreeId}`) @@ -3325,6 +3333,9 @@ export class OrcaRuntimeService { localProvider: provider, onPtyStopped: this.onPtyStopped ?? undefined, requirePhysicalStop: true, + // Why (#11960): set only by an explicit Force Delete, never by the ordinary + // confirmation — otherwise the gate would be off on the primary delete path. + ...(allowUnverifiedStop ? { allowUnverifiedStop: true } : {}), ...(connectionId ? { includeLocalRegistry: false } : {}) }) const total = @@ -23262,14 +23273,17 @@ export class OrcaRuntimeService { async removeManagedWorktree( worktreeSelector: string, force = false, - runHooks = false + runHooks = false, + // Why (#11960): only an explicit Force Delete waives PTY-stop proof; `force` + // alone is already set by the ordinary delete confirmation. + allowUnverifiedPtyStop = false ): Promise { if (!this.store) { throw new Error('runtime_unavailable') } const store = this.store const removalTarget = await this.resolveWorktreeRemovalTarget(worktreeSelector) - const optionsKey = getRuntimeWorktreeRemovalOptionsKey(force, runHooks) + const optionsKey = getRuntimeWorktreeRemovalOptionsKey(force, runHooks, allowUnverifiedPtyStop) const inFlightRemoval = this.removeManagedWorktreeInFlight.get(removalTarget.id) if (inFlightRemoval) { if (inFlightRemoval.optionsKey === optionsKey) { @@ -23428,10 +23442,10 @@ export class OrcaRuntimeService { ) let removalCompleted = false try { - await this.stopPtysForDestructiveWorktreeRemoval( - removalTarget.id, - repo.connectionId - ) + await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id, { + connectionId: repo.connectionId, + allowUnverifiedStop: allowUnverifiedPtyStop + }) await fsProvider!.deletePath(removalTarget.path, true) removalCompleted = true } finally { @@ -23448,7 +23462,9 @@ export class OrcaRuntimeService { const removalGate = await this.acquireFileWatcherRemoval(removalTarget.path) let removalCompleted = false try { - await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id) + await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id, { + allowUnverifiedStop: allowUnverifiedPtyStop + }) await removeLocalWorktreePath(removalTarget.path, localWorktreeGitOptions) removalCompleted = true } finally { @@ -23496,7 +23512,9 @@ export class OrcaRuntimeService { const removalGate = await this.acquireFileWatcherRemoval(removalTarget.path) let removalCompleted = false try { - await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id) + await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id, { + allowUnverifiedStop: allowUnverifiedPtyStop + }) await removeLocalWorktreePath(removalTarget.path, localWorktreeGitOptions) removalCompleted = true } finally { @@ -23615,7 +23633,10 @@ export class OrcaRuntimeService { let rawRemovalResult: RemoveWorktreeResult | undefined let removalCompleted = false try { - await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id, repo.connectionId) + await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id, { + connectionId: repo.connectionId, + allowUnverifiedStop: allowUnverifiedPtyStop + }) rawRemovalResult = await (Object.keys(remoteRemoveOptions).length > 0 ? provider!.removeWorktree(canonicalWorktreePath, force, remoteRemoveOptions) : provider!.removeWorktree(canonicalWorktreePath, force)) @@ -23726,7 +23747,9 @@ export class OrcaRuntimeService { try { // Why: linked-path deletion is destructive too; PTYs must release every // handle before Windows or WSL filesystem cleanup starts. - await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id) + await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id, { + allowUnverifiedStop: allowUnverifiedPtyStop + }) if (linkedPaths.length > 0) { await removeWorktreeLinkedPaths(canonicalWorktreePath, linkedPaths) diff --git a/src/main/runtime/pty-waiver-source-invariant.test.ts b/src/main/runtime/pty-waiver-source-invariant.test.ts new file mode 100644 index 00000000000..e0b7122d825 --- /dev/null +++ b/src/main/runtime/pty-waiver-source-invariant.test.ts @@ -0,0 +1,61 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' + +// Why (#11960): behavioural tests only reach one of the removal branches in each +// file, so re-deriving the waiver from `force` at any of the others stays green +// while silently disabling the PTY gate on that path. `force` is set by the +// ordinary delete confirmation (to skip the dirty-file prompt) and is NOT user +// intent to delete past live terminals — so pin the wiring itself, at every site. +const FILES = [join(__dirname, '..', 'ipc', 'worktrees.ts'), join(__dirname, 'orca-runtime.ts')] + +// Why: a comment quoting `allowUnverifiedStop:` would otherwise count as a site — +// and this very invariant invites people to write one in the file it guards. +function stripComments(source: string): string { + return source + .replace(/\/\*[\s\S]*?\*\//g, '') + .split('\n') + .filter((line) => !/^\s*(\/\/|\*)/.test(line)) + .join('\n') +} + +describe('the PTY-stop waiver is never derived from `force`', () => { + it.each(FILES)('%s passes only an explicit waiver to the teardown', (file) => { + const source = stripComments(readFileSync(file, 'utf8')) + + // Why: derived, not hardcoded — merging two removal branches is a legitimate + // refactor and must not read as a deleted safety wiring, while dropping the + // waiver from a branch that still exists must still fail loudly. + const teardownCallSites = + [...source.matchAll(/stopPtysForDestructiveWorktreeRemoval\(/g)].length - 1 + expect(teardownCallSites).toBeGreaterThan(0) + + const values = [...source.matchAll(/allowUnverifiedStop:\s*([^,\n}]+)/g)].map((match) => + match[1].trim() + ) + // One per call site, plus the single conditional spread inside the helper. + expect(values).toHaveLength(teardownCallSites + 1) + for (const value of values) { + expect(value).not.toMatch(/\bforce\b/) + // Positive check too: "not literally force" would still admit any other + // in-scope boolean being wired in by mistake. + expect(value).toMatch(/^(?:args\.)?allowUnverifiedPtyStop$|^true$/) + } + // Only the helper's spread may hardcode `true`; a call site doing so would + // waive unconditionally. + expect(values.filter((value) => value === 'true')).toHaveLength(1) + + // Why: checking the value alone is not enough — `...(force || allowUnverifiedStop + // ? { allowUnverifiedStop: true } : {})` re-disables the gate on every confirmed + // delete while the value stays a blameless `true`. Pin the guarding condition too. + // Lazy `[\s\S]*?` rather than `[^?]*` so an optional chain (`args?.force`) inside + // the condition cannot end the match early and slip the whole check. + const conditions = [ + ...source.matchAll(/\.\.\.\(([\s\S]{0,200}?)\?\s*\{\s*allowUnverifiedStop:/g) + ].map((match) => match[1].trim()) + expect(conditions).toHaveLength(1) + for (const condition of conditions) { + expect(condition).not.toMatch(/\bforce\b/i) + } + }) +}) diff --git a/src/main/runtime/rpc/methods/worktree-rm-pty-waiver.test.ts b/src/main/runtime/rpc/methods/worktree-rm-pty-waiver.test.ts new file mode 100644 index 00000000000..3b51a57d054 --- /dev/null +++ b/src/main/runtime/rpc/methods/worktree-rm-pty-waiver.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it, vi } from 'vitest' +import { RpcDispatcher } from '../dispatcher' +import type { RpcRequest } from '../core' +import type { OrcaRuntimeService } from '../../orca-runtime' +import { WORKTREE_METHODS } from './worktree' + +function makeRuntime(): OrcaRuntimeService { + return { + getRuntimeId: () => 'test-runtime', + dedupeWorktreeCreate: (_repo: string, _id: string | undefined, run: () => Promise) => + run(), + removeManagedWorktree: vi.fn().mockResolvedValue({}) + } as unknown as OrcaRuntimeService +} + +// Why (#11960): waiving the proof that every PTY stopped must ride its own field. +// The desktop sets `force` for an ordinary confirmed delete, so keying the waiver +// off `force` would silently disable the gate on the primary delete path. +describe('worktree.rm PTY-stop waiver', () => { + it('forwards an explicit waiver to the runtime', async () => { + const runtime = makeRuntime() + const dispatcher = new RpcDispatcher({ runtime, methods: WORKTREE_METHODS }) + + await dispatcher.dispatch({ + id: 'req-1', + authToken: 'tok', + method: 'worktree.rm', + params: { worktree: 'id:wt-1', force: true, allowUnverifiedPtyStop: true, runHooks: false } + } satisfies RpcRequest) + + expect(runtime.removeManagedWorktree).toHaveBeenCalledWith('id:wt-1', true, false, true) + }) + + it('does not infer a waiver from force alone', async () => { + const runtime = makeRuntime() + const dispatcher = new RpcDispatcher({ runtime, methods: WORKTREE_METHODS }) + + await dispatcher.dispatch({ + id: 'req-1', + authToken: 'tok', + method: 'worktree.rm', + params: { worktree: 'id:wt-1', force: true, runHooks: false } + } satisfies RpcRequest) + + expect(runtime.removeManagedWorktree).toHaveBeenCalledWith('id:wt-1', true, false, false) + }) +}) diff --git a/src/main/runtime/rpc/methods/worktree-schemas.ts b/src/main/runtime/rpc/methods/worktree-schemas.ts index 069a8499d18..57b8a9202a5 100644 --- a/src/main/runtime/rpc/methods/worktree-schemas.ts +++ b/src/main/runtime/rpc/methods/worktree-schemas.ts @@ -276,6 +276,10 @@ export const WorktreeSet = WorktreeSelector.extend({ export const WorktreeRemove = WorktreeSelector.extend({ force: OptionalBoolean, + // Why (#11960): the CLI's --force is an unambiguous force affordance, but the + // desktop sets `force` for an ordinary confirmed delete too, so the PTY-stop + // waiver travels on its own field. + allowUnverifiedPtyStop: OptionalBoolean, runHooks: OptionalBoolean }) diff --git a/src/main/runtime/rpc/methods/worktree.test.ts b/src/main/runtime/rpc/methods/worktree.test.ts index 10271681dc4..3fe4b5da15c 100644 --- a/src/main/runtime/rpc/methods/worktree.test.ts +++ b/src/main/runtime/rpc/methods/worktree.test.ts @@ -92,7 +92,8 @@ describe('worktree RPC methods', () => { }) ) - expect(runtime.removeManagedWorktree).toHaveBeenCalledWith('id:wt-1', true, false) + // Why (#11960): dirty-file force alone must not waive the PTY-stop proof. + expect(runtime.removeManagedWorktree).toHaveBeenCalledWith('id:wt-1', true, false, false) expect(response).toMatchObject({ ok: true, result: { removed: true } }) }) diff --git a/src/main/runtime/rpc/methods/worktree.ts b/src/main/runtime/rpc/methods/worktree.ts index dfa81dde1f5..758e6b83891 100644 --- a/src/main/runtime/rpc/methods/worktree.ts +++ b/src/main/runtime/rpc/methods/worktree.ts @@ -274,7 +274,8 @@ export const WORKTREE_METHODS: RpcMethod[] = [ const result = await runtime.removeManagedWorktree( params.worktree, params.force === true, - params.runHooks === true + params.runHooks === true, + params.allowUnverifiedPtyStop === true ) return { removed: true, ...result } } diff --git a/src/main/runtime/settle-before-deadline.ts b/src/main/runtime/settle-before-deadline.ts new file mode 100644 index 00000000000..4ae98e87f05 --- /dev/null +++ b/src/main/runtime/settle-before-deadline.ts @@ -0,0 +1,54 @@ +/** + * Races `run()` against an absolute deadline (epoch ms). + * + * Without `failClosedError` the call is best-effort: a timeout or a rejection + * resolves to `fallback`. With it, both surface as a rejection so destructive + * callers can block on unproven work — `failClosedOnRunError` narrows which + * rejections count (some are benign sentinels). + */ +export async function settleBeforeDeadline( + run: () => Promise, + fallback: T, + deadline: number, + failClosedError?: Error, + failClosedOnRunError: (error: unknown) => boolean = () => true +): Promise { + const remaining = deadline - Date.now() + if (remaining <= 0) { + if (failClosedError) { + throw failClosedError + } + return fallback + } + return new Promise((resolve, reject) => { + let settled = false + const finish = (value: T): void => { + if (settled) { + return + } + settled = true + clearTimeout(timer) + resolve(value) + } + const fail = (error: unknown): void => { + if (settled) { + return + } + settled = true + clearTimeout(timer) + reject(error) + } + const timer = setTimeout( + () => (failClosedError ? fail(failClosedError) : finish(fallback)), + remaining + ) + timer.unref?.() + // Why: `.then(run)` rather than `run()` so a synchronous throw is routed + // through the same fail-closed filter instead of escaping the executor. + void Promise.resolve() + .then(run) + .then(finish, (error: unknown) => + failClosedError && failClosedOnRunError(error) ? fail(error) : finish(fallback) + ) + }) +} diff --git a/src/main/runtime/unstopped-pty-verification.ts b/src/main/runtime/unstopped-pty-verification.ts new file mode 100644 index 00000000000..b0df601f133 --- /dev/null +++ b/src/main/runtime/unstopped-pty-verification.ts @@ -0,0 +1,70 @@ +import type { IPtyProvider } from '../providers/types' +import { 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. +export const WORKTREE_TEARDOWN_VERIFY_GRACE_MS = 2_000 + +export type UnstoppedPtyVerdict = + | { status: 'exited' } + | { status: 'live'; ptyIds: string[] } + | { status: 'unverifiable'; reason: string } + +/** + * Re-lists the provider's processes to decide what a failed stop RPC actually + * meant. The three verdicts stay distinct on purpose: "we could not ask" is not + * evidence that a PTY survived, and callers word their errors differently. + * + * Why (#11960): this re-list used to run on the sweep's own deadline, which the + * sweeps had normally just spent. An inventory that answers perfectly well in + * 1s then "timed out" against a 0ms budget, so a PTY that had already exited + * read as unverifiable and the workspace could never be removed. Verification + * gets a budget of its own, sized like the sweep's rather than its leftovers. + */ +export async function verifyUnstoppedPtys( + failedPtyIds: readonly string[], + provider: IPtyProvider, + sweepBudgetMs: number +): Promise { + const verifyBudgetMs = Math.max(WORKTREE_TEARDOWN_VERIFY_GRACE_MS, sweepBudgetMs) + const verifyDeadline = Date.now() + verifyBudgetMs + let listError: unknown + const sessions = await settleBeforeDeadline( + async () => { + try { + return await provider.listProcesses({ deadlineMs: verifyDeadline }) + } catch (error) { + listError = error + return null + } + }, + null, + verifyDeadline + ) + if (!sessions) { + return { + status: 'unverifiable', + reason: listError instanceof Error ? listError.message : 'the process list timed out' + } + } + const livePtyIds = new Set(sessions.map((session) => session.id)) + const stillLive = failedPtyIds.filter((ptyId) => livePtyIds.has(ptyId)) + return stillLive.length > 0 ? { status: 'live', ptyIds: stillLive } : { status: 'exited' } +} + +/** Names the blocking PTYs so a wedged removal is diagnosable, not just refused. */ +export function describeUnstoppedPtys( + worktreeId: string, + failedPtyIds: readonly string[], + verdict: Exclude +): string { + const detail = + verdict.status === 'live' + ? `still live: ${verdict.ptyIds.join(', ')}` + : `could not verify these exited: ${failedPtyIds.join(', ')} (${verdict.reason})` + return `${UNSTOPPED_PTY_REMOVAL_PREFIX} ${worktreeId} — ${detail}` +} + +export function describeError(error: unknown): string { + return error instanceof Error ? error.message : String(error) +} diff --git a/src/main/runtime/worktree-teardown-unstopped-pty.test.ts b/src/main/runtime/worktree-teardown-unstopped-pty.test.ts new file mode 100644 index 00000000000..47852b45985 --- /dev/null +++ b/src/main/runtime/worktree-teardown-unstopped-pty.test.ts @@ -0,0 +1,368 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { listRegisteredPtysMock } = vi.hoisted(() => ({ + listRegisteredPtysMock: vi.fn() +})) + +vi.mock('../memory/pty-registry', () => ({ + listRegisteredPtys: listRegisteredPtysMock +})) + +import { killAllProcessesForWorktree, WORKTREE_PROCESS_SWEEP_TIMEOUT_MS } from './worktree-teardown' +import type { IPtyProvider, PtyProcessInfo } from '../providers/types' + +// Why: these tests advance fake timers *before* awaiting the teardown, so a +// rejection mid-advance had no handler yet — Node reported it as an unhandled +// rejection and vitest surfaced it inside whichever test happened to be running. +// Attaching a no-op handler at creation keeps the original semantics while +// making failures land as clean assertion failures in their own test. +function settleTeardown(promise: Promise): Promise { + void promise.catch(() => undefined) + return promise +} + +function createProviderStub(listProcesses: () => Promise): IPtyProvider { + return { + shutdown: vi.fn().mockResolvedValue(undefined), + listProcesses: vi.fn(listProcesses), + onData: vi.fn().mockReturnValue(() => {}), + onReplay: vi.fn().mockReturnValue(() => {}), + onExit: vi.fn().mockReturnValue(() => {}) + } as unknown as IPtyProvider +} + +// A worktree whose PTY teardown cannot be proven must still be removable: the +// gate that blocks Git work is the same one that made #11960 permanent. +describe('destructive teardown when a PTY stop cannot be proven', () => { + beforeEach(() => { + listRegisteredPtysMock.mockReset() + }) + + // Why (#11960): the sweeps routinely burn the whole budget, so re-listing on + // the same exhausted deadline returned "unverifiable" for a PTY that had in + // fact exited — wedging the workspace on every retry. + it('verifies a failed stop against a fresh budget when the sweeps spent the deadline', async () => { + vi.useFakeTimers() + try { + const localProvider = createProviderStub( + () => new Promise((resolve) => setTimeout(() => resolve([]), 90)) + ) + ;(localProvider.shutdown as unknown as ReturnType).mockRejectedValue( + new Error('Session not found: stale-1') + ) + listRegisteredPtysMock.mockReturnValue([ + { ptyId: 'stale-1', worktreeId: 'w1', sessionId: null, paneKey: null, pid: 100 } + ]) + + const teardown = settleTeardown( + killAllProcessesForWorktree('w1', { + localProvider, + timeoutMs: 100, + requirePhysicalStop: true + }) + ) + await vi.runAllTimersAsync() + + await expect(teardown).resolves.toEqual({ + runtimeStopped: 0, + providerStopped: 0, + registryStopped: 0 + }) + } finally { + vi.useRealTimers() + } + }) + + // The reported shape: an automation workspace whose only trace is a stale + // registry row the daemon 404s on, behind an inventory slow enough to consume + // the sweep budget. Verification must still get far enough to prove absence. + it('removes the reported wedged automation workspace without --force', async () => { + const worktreeId = 'repo-1::C:/Users/admin/orca/workspaces/repo/auto-review-run-28' + // Slow enough that a fixed 2s grace could not absorb it, but far enough from + // the budget that the list-completion and timeout timers can't land in the + // same tick — a 100ms margin here raced under parallel load. + const listDelayMs = WORKTREE_PROCESS_SWEEP_TIMEOUT_MS / 2 + vi.useFakeTimers() + try { + const localProvider = createProviderStub( + () => new Promise((resolve) => setTimeout(() => resolve([]), listDelayMs)) + ) + ;(localProvider.shutdown as unknown as ReturnType).mockRejectedValue( + new Error('Session not found: term_abab11ee') + ) + listRegisteredPtysMock.mockReturnValue([ + { ptyId: 'term_abab11ee', worktreeId, sessionId: null, paneKey: null, pid: 4242 } + ]) + + const teardown = settleTeardown( + killAllProcessesForWorktree(worktreeId, { + localProvider, + requirePhysicalStop: true + }) + ) + await vi.runAllTimersAsync() + + await expect(teardown).resolves.toEqual({ + runtimeStopped: 0, + providerStopped: 0, + registryStopped: 0 + }) + } finally { + vi.useRealTimers() + } + }) + + it('names the blocking PTYs and the escape hatch when one is still live', async () => { + const localProvider = createProviderStub(async () => [ + { id: 'w1@@live-1', cwd: '/tmp/w1', title: 'shell' } + ]) + ;(localProvider.shutdown as unknown as ReturnType).mockRejectedValue( + new Error('kill failed') + ) + listRegisteredPtysMock.mockReturnValue([]) + + await expect( + killAllProcessesForWorktree('w1', { localProvider, requirePhysicalStop: true }) + ).rejects.toThrow(/still live: w1@@live-1[\s\S]*--force/) + }) + + // Why: the memory/registry rows this drops are the reason clearStoppedPtyState + // exists; commit 3 moved that loop, so pin it before it can silently vanish. + it('clears PTY state once a failed stop is proven to have exited', async () => { + const localProvider = createProviderStub(async () => []) + ;(localProvider.shutdown as unknown as ReturnType).mockRejectedValue( + new Error('Session not found: stale-1') + ) + listRegisteredPtysMock.mockReturnValue([ + { ptyId: 'stale-1', worktreeId: 'w1', sessionId: null, paneKey: null, pid: 100 } + ]) + const onPtyStopped = vi.fn() + + await expect( + killAllProcessesForWorktree('w1', { + localProvider, + onPtyStopped, + requirePhysicalStop: true + }) + ).resolves.toBeDefined() + expect(onPtyStopped).toHaveBeenCalledWith('stale-1') + }) + + it('names only the PTYs that are actually live, not every failed stop', async () => { + const localProvider = createProviderStub(async () => [ + { id: 'w1@@live-1', cwd: '/tmp/w1', title: 'shell' } + ]) + ;(localProvider.shutdown as unknown as ReturnType).mockRejectedValue( + new Error('kill failed') + ) + listRegisteredPtysMock.mockReturnValue([ + { ptyId: 'w1@@gone-2', worktreeId: 'w1', sessionId: null, paneKey: null, pid: 101 } + ]) + + const error = await killAllProcessesForWorktree('w1', { + localProvider, + requirePhysicalStop: true + }).then( + () => new Error('expected a rejection'), + (rejection: Error) => rejection + ) + expect(error.message).toContain('w1@@live-1') + expect(error.message).not.toContain('w1@@gone-2') + }) + + it('reports unverifiable separately from live when the process list fails', async () => { + const localProvider = createProviderStub(async () => { + throw new Error('daemon socket closed') + }) + listRegisteredPtysMock.mockReturnValue([ + { ptyId: 'stale-1', worktreeId: 'w1', sessionId: null, paneKey: null, pid: 100 } + ]) + ;(localProvider.shutdown as unknown as ReturnType).mockRejectedValue( + new Error('Session not found: stale-1') + ) + + await expect( + killAllProcessesForWorktree('w1', { + localProvider, + includeProviderInventory: false, + requirePhysicalStop: true + }) + ).rejects.toThrow(/could not verify[\s\S]*stale-1[\s\S]*daemon socket closed/) + }) + + // A gate that force cannot cross is the bug, wherever it sits. These two cover + // the sweep-level failures that reject before the unproven-stop gate is reached. + it('lets force through a provider whose inventory rejects outright', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + listRegisteredPtysMock.mockReturnValue([]) + const localProvider = createProviderStub(async () => { + throw new Error('ssh channel closed') + }) + + await expect( + killAllProcessesForWorktree('w1', { + localProvider, + requirePhysicalStop: true, + allowUnverifiedStop: true + }) + ).resolves.toEqual({ runtimeStopped: 0, providerStopped: 0, registryStopped: 0 }) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('ssh channel closed')) + } finally { + warn.mockRestore() + } + }) + + it('lets force through a sweep that never settles before the deadline', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.useFakeTimers() + // Why: hand the stub a resolver instead of a permanently dangling promise, so + // this test leaves no in-flight continuation to interleave into a later one. + let releaseList: (sessions: PtyProcessInfo[]) => void = () => {} + try { + listRegisteredPtysMock.mockReturnValue([]) + const localProvider = createProviderStub( + () => + new Promise((resolve) => { + releaseList = resolve + }) + ) + + const teardown = settleTeardown( + killAllProcessesForWorktree('w1', { + localProvider, + timeoutMs: 100, + requirePhysicalStop: true, + allowUnverifiedStop: true + }) + ) + await vi.runAllTimersAsync() + + await expect(teardown).resolves.toEqual({ + runtimeStopped: 0, + providerStopped: 0, + registryStopped: 0 + }) + } finally { + releaseList([]) + await vi.advanceTimersByTimeAsync(0) + vi.useRealTimers() + warn.mockRestore() + } + }) + + // Why: the caller deletes files the moment this resolves. Returning while another + // sweep is still inside shutdown() would race the delete against a PTY that was + // about to release its handles — EBUSY for a process ~300ms from exiting. + it('waits for in-flight sweeps before force returns', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + const localProvider = createProviderStub(async () => { + throw new Error('ssh channel closed') + }) + let shutdownFinished = false + ;(localProvider.shutdown as unknown as ReturnType).mockImplementation( + async () => { + await new Promise((resolve) => setTimeout(resolve, 20)) + shutdownFinished = true + } + ) + listRegisteredPtysMock.mockReturnValue([ + { ptyId: 'reg-1', worktreeId: 'w1', sessionId: null, paneKey: null, pid: 100 } + ]) + + const result = await killAllProcessesForWorktree('w1', { + localProvider, + requirePhysicalStop: true, + allowUnverifiedStop: true + }) + + expect(shutdownFinished).toBe(true) + // And the surviving sweep's work is reported, not flattened to zero. + expect(result.registryStopped).toBe(1) + } finally { + warn.mockRestore() + } + }) + + // Why: the deadline sentinel only says *something* timed out. Picking the + // rejection by array position let a hung runtime sweep mask the provider error + // that actually explains the failure, in the log the user is left with. + it('warns with the specific sweep failure, not the generic deadline', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.useFakeTimers() + try { + listRegisteredPtysMock.mockReturnValue([]) + const localProvider = createProviderStub(async () => { + throw new Error('ssh channel closed') + }) + const runtime = { + stopTerminalsForWorktree: () => new Promise(() => {}) + } as unknown as Parameters[1]['runtime'] + + const teardown = settleTeardown( + killAllProcessesForWorktree('w1', { + runtime, + localProvider, + timeoutMs: 100, + requirePhysicalStop: true, + allowUnverifiedStop: true + }) + ) + await vi.runAllTimersAsync() + await teardown + + const warning = warn.mock.calls.at(-1)?.[0] as string + expect(warning).toContain('ssh channel closed') + // Both are reported — losing the timeout would trade one blind spot for + // another — but the specific cause must lead, not be buried behind it. + expect(warning).toContain('Timed out waiting') + expect(warning.indexOf('ssh channel closed')).toBeLessThan( + warning.indexOf('Timed out waiting') + ) + } finally { + vi.useRealTimers() + warn.mockRestore() + } + }) + + it('still fails closed on a sweep-level failure without force', async () => { + listRegisteredPtysMock.mockReturnValue([]) + const localProvider = createProviderStub(async () => { + throw new Error('ssh channel closed') + }) + + await expect( + killAllProcessesForWorktree('w1', { localProvider, requirePhysicalStop: true }) + ).rejects.toThrow('ssh channel closed') + }) + + it('lets an explicit force removal proceed past PTYs it could not stop', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + const localProvider = createProviderStub(async () => [ + { id: 'w1@@live-1', cwd: '/tmp/w1', title: 'shell' } + ]) + ;(localProvider.shutdown as unknown as ReturnType).mockRejectedValue( + new Error('kill failed') + ) + listRegisteredPtysMock.mockReturnValue([]) + const onPtyStopped = vi.fn() + + await expect( + killAllProcessesForWorktree('w1', { + localProvider, + onPtyStopped, + requirePhysicalStop: true, + allowUnverifiedStop: true + }) + ).resolves.toMatchObject({ providerStopped: 0 }) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('w1@@live-1')) + // Why: unregistering a PTY we just watched stay alive would hide it from + // the next sweep and from the user — the discoverability half of #11960. + expect(onPtyStopped).not.toHaveBeenCalled() + } finally { + warn.mockRestore() + } + }) +}) diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index 64b6a09703f..45929e21f98 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -4,6 +4,14 @@ 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 { settleBeforeDeadline } from './settle-before-deadline' +import { + describeError, + describeUnstoppedPtys, + verifyUnstoppedPtys, + type UnstoppedPtyVerdict +} from './unstopped-pty-verification' // Why: normal inventories still coalesce into one process scan, while a stale // or pathological inventory cannot fan out unbounded provider/RPC shutdowns. @@ -21,6 +29,8 @@ export type WorktreeTeardownDeps = { onPtyStopped?: (ptyId: string) => void timeoutMs?: number requirePhysicalStop?: boolean + /** Explicit Force Delete only: warn instead of throwing when a stop stays unproven (#11960). */ + allowUnverifiedStop?: boolean includeProviderInventory?: boolean includeLocalRegistry?: boolean } @@ -33,8 +43,10 @@ export type WorktreeTeardownResult = { export const WORKTREE_PROCESS_SWEEP_TIMEOUT_MS = 10_000 -// Why: reserve time after bounded stop RPCs to recheck whether a reported -// failure actually left a live PTY before the outer sweep deadline. +// Why: keep each bounded stop RPC settling before the sweep deadline itself, so +// a wedged provider surfaces as a stop failure rather than as the outer timeout. +// (The recheck this margin once also reserved time for now runs on its own +// budget — see verifyUnstoppedPtys — because sharing this one wedged #11960.) export const WORKTREE_TEARDOWN_RPC_MARGIN_MS = 500 // Absolute deadline (epoch ms) threaded into provider RPCs on the destructive @@ -64,12 +76,17 @@ export function teardownRpcDeadline(sweepDeadline: number): number { * * Sweeps are best-effort by default. Destructive removal callers set * `requirePhysicalStop` so a timeout or unproven stop blocks filesystem work. + * `allowUnverifiedStop` waives that proof so the gate can never wedge a + * workspace permanently (#11960) — it must come only from an explicit Force + * Delete or `--force`, never from the `force` an ordinary confirmed delete + * already sets to skip the dirty-file prompt. */ export async function killAllProcessesForWorktree( worktreeId: string, deps: WorktreeTeardownDeps ): Promise { - const deadline = Date.now() + Math.max(1, deps.timeoutMs ?? WORKTREE_PROCESS_SWEEP_TIMEOUT_MS) + 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 stopAttempts = new Map>() const stopPty = ( @@ -154,90 +171,93 @@ export async function killAllProcessesForWorktree( deadline, deps.requirePhysicalStop ? deadlineError : undefined ) - const [runtimeResult, providerStopped, registryStopped] = await Promise.all([ - runtimeSweep, - providerSweep, - registrySweep - ]) + // Why: a rejection here can outlive this call, and only one of the two paths + // below observes every promise, so mark them all handled up front. + for (const sweep of [runtimeSweep, providerSweep, registrySweep]) { + void sweep.catch(() => undefined) + } + let runtimeResult: { stopped: number } + 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] : [] + ) + 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 + } + } + runtimeResult = (runtimeSettled as PromiseFulfilledResult<{ stopped: number }>).value + providerStopped = (providerSettled as PromiseFulfilledResult).value + registryStopped = (registrySettled as PromiseFulfilledResult).value + } 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 + ]) + } if (deps.requirePhysicalStop) { const stopResults = await Promise.all( [...stopAttempts].map(async ([ptyId, stopped]) => [ptyId, await stopped] as const) ) const failedPtyIds = stopResults.filter(([, stopped]) => !stopped).map(([ptyId]) => ptyId) - const failedPtysExited = - failedPtyIds.length === 0 || - (await verifyFailedPtysExited(failedPtyIds, deps.localProvider, deadline)) - if (!failedPtysExited) { - throw new Error(`Failed to physically stop every PTY for worktree: ${worktreeId}`) - } - for (const ptyId of failedPtyIds) { - clearStoppedPtyState(ptyId, deps.onPtyStopped) + const verdict: UnstoppedPtyVerdict = + failedPtyIds.length === 0 + ? { status: 'exited' } + : await verifyUnstoppedPtys(failedPtyIds, deps.localProvider, sweepBudgetMs) + if (verdict.status === 'exited') { + for (const ptyId of failedPtyIds) { + clearStoppedPtyState(ptyId, deps.onPtyStopped) + } + } else { + const summary = describeUnstoppedPtys(worktreeId, failedPtyIds, verdict) + if (!deps.allowUnverifiedStop) { + throw new Error(`${summary}. ${WORKTREE_TEARDOWN_FORCE_HINT}`) + } + // Why: force is the documented escape hatch, so removal continues — but the + // registry rows stay put. Dropping them would unregister a PTY we just saw + // alive, so a retry could no longer find it and the user could never see it + // (the discoverability half of #11960). + console.warn(`[worktree-teardown] forcing removal despite unstopped PTYs — ${summary}`) } } return { runtimeStopped: runtimeResult.stopped, providerStopped, registryStopped } } -async function verifyFailedPtysExited( - failedPtyIds: readonly string[], - provider: IPtyProvider, - deadline: number -): Promise { - const sessions = await settleBeforeDeadline( - () => provider.listProcesses({ deadlineMs: deadline }), - null, - deadline - ).catch(() => null) - if (!sessions) { - return false - } - const livePtyIds = new Set(sessions.map((session) => session.id)) - return failedPtyIds.every((ptyId) => !livePtyIds.has(ptyId)) -} - -async function settleBeforeDeadline( - run: () => Promise, - fallback: T, - deadline: number, - failClosedError?: Error, - failClosedOnRunError: (error: unknown) => boolean = () => true -): Promise { - const remaining = deadline - Date.now() - if (remaining <= 0) { - if (failClosedError) { - throw failClosedError - } - return fallback - } - return new Promise((resolve, reject) => { - let settled = false - const finish = (value: T): void => { - if (settled) { - return - } - settled = true - clearTimeout(timer) - resolve(value) - } - const fail = (error: unknown): void => { - if (settled) { - return - } - settled = true - clearTimeout(timer) - reject(error) - } - const timer = setTimeout( - () => (failClosedError ? fail(failClosedError) : finish(fallback)), - remaining - ) - timer.unref?.() - void run().then(finish, (error: unknown) => - failClosedError && failClosedOnRunError(error) ? fail(error) : finish(fallback) - ) - }) -} - async function sweepProviderByPrefix( worktreeId: string, provider: IPtyProvider, diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 6e70b03033d..3a09aef319f 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -1409,6 +1409,10 @@ export type PreloadApi = { worktreeId: string hostId?: ExecutionHostId force?: boolean + // Why (#11960): distinct from `force`, which the plain Delete confirmation + // already sets to skip the dirty-file prompt. Only an explicit Force Delete + // may waive the proof that every PTY stopped. + allowUnverifiedPtyStop?: boolean skipArchive?: boolean }) => Promise // Forget a workspace from Orca only (no remote Git/FS work) — for workspaces pinned to a removed/disconnected SSH host. diff --git a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx index b89f3fef3fa..6bbe0581c9a 100644 --- a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx +++ b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx @@ -267,7 +267,9 @@ const DeleteWorktreeDialog = React.memo(function DeleteWorktreeDialog() { // the shared toast wrapper. Close immediately because workspace cards // already show the deleting state while the retry runs. const commitFocus = prepareActiveWorktreeFocusAfterDelete(worktreeId) - const deletePromise = removeWorktree(worktreeId, true) + // Why (#11960): this IS the explicit Force Delete, so it may also waive + // the PTY-stop proof — unlike the confirmed delete in the branch below. + const deletePromise = removeWorktree(worktreeId, true, { allowUnverifiedPtyStop: true }) closeModal() deletePromise .then((result) => { 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 a188f16c83b..941c4d38eaa 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts @@ -209,7 +209,11 @@ describe('runWorktreeBatchDelete', () => { toastOptions?.onForceDelete() await vi.waitFor(() => { - expect(mocks.state.removeWorktree).toHaveBeenNthCalledWith(2, 'wt-1', true) + // Why (#11960): clicking Force Delete on the failure toast is an explicit + // force, so it also waives the PTY-stop proof the first attempt failed. + expect(mocks.state.removeWorktree).toHaveBeenNthCalledWith(2, 'wt-1', true, { + allowUnverifiedPtyStop: true + }) expect(onDeleted).toHaveBeenCalledWith(['wt-1']) }) }) diff --git a/src/renderer/src/components/sidebar/delete-worktree-flow.ts b/src/renderer/src/components/sidebar/delete-worktree-flow.ts index b2dc1dbc1a6..6259789187b 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-flow.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-flow.ts @@ -138,7 +138,11 @@ export function runWorktreeDeleteWithToast( onForceDelete: () => { // Why: recapture at click time — the user may have navigated away while the toast was open, so focus only hands off if still viewed. const commitForceFocus = prepareActiveWorktreeFocusAfterDelete(worktreeId) - const forceRemoval = useAppStore.getState().removeWorktree(worktreeId, true) + // Why (#11960): the user clicked Force Delete on a failure toast, so this + // retry may waive the PTY-stop proof the first attempt could not satisfy. + const forceRemoval = useAppStore + .getState() + .removeWorktree(worktreeId, true, { allowUnverifiedPtyStop: true }) forceRemoval .then((forceResult) => { if (!forceResult.ok) { 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 b66845e94ca..16bf7c4b954 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-toast.test.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-toast.test.ts @@ -10,6 +10,23 @@ describe('getDeleteWorktreeToastCopy', () => { }) }) + // Why (#11960): the PTY gate's error tells the user to force-delete, so the + // 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( + '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: + '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 + }) + }) + it('uses orphaned-directory guidance when Git tracking is already gone', () => { expect( getDeleteWorktreeToastCopy( diff --git a/src/renderer/src/components/sidebar/delete-worktree-toast.ts b/src/renderer/src/components/sidebar/delete-worktree-toast.ts index f9b312a1045..22f7eb7f5ad 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-toast.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-toast.ts @@ -51,6 +51,20 @@ export function getDeleteWorktreeToastCopy( isDestructive: false } } + if (forceDeleteReason === 'unstopped-pty') { + return { + title: translate( + 'auto.components.sidebar.delete.worktree.toast.1d0fa5c0a5', + '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.' + ), + isDestructive: false + } + } if (forceDeleteReason === 'missing-registration') { return { title: translate( diff --git a/src/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsx b/src/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsx index dcfce654488..4353a6fb81a 100644 --- a/src/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsx +++ b/src/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsx @@ -1603,7 +1603,8 @@ export function WorkspaceSpaceManagerPanel(): React.JSX.Element { // Why: Space keeps normal deletes non-force so uncommitted work is not // discarded silently; a failed row gets this explicit recovery path. const commitFocus = prepareActiveWorktreeFocusAfterDelete(worktree.worktreeId) - void removeWorktree(worktree.worktreeId, true) + // Why (#11960): explicit force recovery, so it may also waive PTY-stop proof. + void removeWorktree(worktree.worktreeId, true, { allowUnverifiedPtyStop: true }) .then((result) => { if (!result.ok) { toast.error( diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 38191f93b38..12dbf32685b 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -4764,7 +4764,8 @@ "905fc8efac": "Git already removed this workspace. Use Force Delete to clear it from Orca.", "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." + "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." } } }, diff --git a/src/renderer/src/store/slices/worktree-helpers.ts b/src/renderer/src/store/slices/worktree-helpers.ts index c102439987c..fd548c91e04 100644 --- a/src/renderer/src/store/slices/worktree-helpers.ts +++ b/src/renderer/src/store/slices/worktree-helpers.ts @@ -223,6 +223,9 @@ export type WorktreeSlice = { options?: { mode?: 'remove' | 'forget-local' suppressPreservedBranchToast?: boolean + // Why (#11960): only an explicit Force Delete waives the proof that every + // PTY stopped; `force` alone is set by the ordinary delete confirmation. + allowUnverifiedPtyStop?: boolean } ) => Promise<({ ok: true } & RemoveWorktreeResult) | { ok: false; error: string }> markWorktreesDeleting: (worktreeIds: readonly string[]) => void diff --git a/src/renderer/src/store/slices/worktrees.test.ts b/src/renderer/src/store/slices/worktrees.test.ts index 94782b75fc0..3306c3ec7a2 100644 --- a/src/renderer/src/store/slices/worktrees.test.ts +++ b/src/renderer/src/store/slices/worktrees.test.ts @@ -5483,7 +5483,12 @@ describe('worktree remote runtime mutations', () => { expect(runtimeEnvironmentCall).toHaveBeenCalledWith({ selector: 'env-1', method: 'worktree.rm', - params: { worktree: `id:${wt.id}`, force: undefined, runHooks: true }, + params: { + worktree: `id:${wt.id}`, + force: undefined, + allowUnverifiedPtyStop: false, + runHooks: true + }, timeoutMs: 60_000 }) expect(mockApi.worktrees.remove).not.toHaveBeenCalled() @@ -5520,7 +5525,12 @@ describe('worktree remote runtime mutations', () => { expect(runtimeEnvironmentCall).toHaveBeenCalledWith({ selector: 'owner-hub', method: 'worktree.rm', - params: { worktree: `id:${wt.id}`, force: undefined, runHooks: true }, + params: { + worktree: `id:${wt.id}`, + force: undefined, + allowUnverifiedPtyStop: false, + runHooks: true + }, timeoutMs: 60_000 }) expect(mockApi.worktrees.remove).not.toHaveBeenCalled() @@ -5660,6 +5670,30 @@ describe('worktree remote runtime mutations', () => { expect(store.getState().worktreesByRepo.repo1).toEqual([wt]) }) + // Why (#11960): the store is where `force` and the PTY-stop waiver could most + // easily be collapsed back into one flag. The ordinary delete confirmation + // passes force:true, so that alone must never reach the gate as a waiver. + it('sends force without the PTY-stop waiver unless a caller asks for it', async () => { + const store = createTestStore() + const wt = makeWorktree({ id: 'repo1::/w/one', repoId: 'repo1', path: '/w/one' }) + store.setState({ worktreesByRepo: { repo1: [wt] } } as Partial) + + await store.getState().removeWorktree(wt.id, true) + expect(mockApi.worktrees.remove).toHaveBeenLastCalledWith( + expect.objectContaining({ force: true, allowUnverifiedPtyStop: false }) + ) + + // Re-seed: the first removal dropped the row, and a second call for a missing + // worktree never reaches the API — which would silently re-read the call above. + const retry = makeWorktree({ id: 'repo1::/w/two', repoId: 'repo1', path: '/w/two' }) + store.setState({ worktreesByRepo: { repo1: [retry] } } as Partial) + + await store.getState().removeWorktree(retry.id, true, { allowUnverifiedPtyStop: true }) + expect(mockApi.worktrees.remove).toHaveBeenLastCalledWith( + expect.objectContaining({ force: true, allowUnverifiedPtyStop: true }) + ) + }) + it('removes SSH-owned worktrees through local IPC even when a runtime is focused', async () => { const store = createTestStore() const wt = makeWorktree({ @@ -5689,6 +5723,8 @@ describe('worktree remote runtime mutations', () => { worktreeId: wt.id, hostId: 'ssh:ssh-1', force: undefined, + // Why (#11960): an ordinary remove never waives the PTY-stop proof. + allowUnverifiedPtyStop: false, skipArchive: false }) expect(runtimeEnvironmentCall).not.toHaveBeenCalled() diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 92f6feb6a15..66bf295a715 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -4017,7 +4017,13 @@ export const createWorktreeSlice: StateCreator ? window.api.worktrees.forgetLocal({ worktreeId, hostId }) : target.kind === 'local' ? (removalGenerationGuard?.assertCurrent(), - window.api.worktrees.remove({ worktreeId, hostId, force, skipArchive })) + window.api.worktrees.remove({ + worktreeId, + hostId, + force, + allowUnverifiedPtyStop: options?.allowUnverifiedPtyStop === true, + skipArchive + })) : (removalGenerationGuard?.assertCurrent(), callRuntimeRpc( target, @@ -4025,6 +4031,7 @@ export const createWorktreeSlice: StateCreator { worktree: toRuntimeWorktreeSelector(worktreeId), force, + allowUnverifiedPtyStop: options?.allowUnverifiedPtyStop === true, runHooks: !skipArchive }, { timeoutMs: 60_000 } @@ -4373,7 +4380,11 @@ export const createWorktreeSlice: StateCreator // Why: git refusing a non-force delete for dirty/untracked files is a handled user decision, not an app error. console.warn('Failed to remove worktree:', err) const error = err instanceof Error ? err.message : String(err) - const forceDeleteReason = classifyWorktreeForceDeleteReason(error, force) + const forceDeleteReason = classifyWorktreeForceDeleteReason( + error, + force, + options?.allowUnverifiedPtyStop === true + ) const locked = isLockedWorktreeRemovalError(error) set((s) => ({ deleteStateByWorktreeId: { diff --git a/src/renderer/src/web/web-preload-api.ts b/src/renderer/src/web/web-preload-api.ts index 26221655acf..1c8fe7010f5 100644 --- a/src/renderer/src/web/web-preload-api.ts +++ b/src/renderer/src/web/web-preload-api.ts @@ -1760,11 +1760,14 @@ function createWorktreesApi(): NonNullable['worktrees']> { targetBranch, isCrossRepository }), - remove: async ({ worktreeId, force, skipArchive }) => { + remove: async ({ worktreeId, force, allowUnverifiedPtyStop, skipArchive }) => { invalidateRuntimeWorktreeCaches() return callRuntimeResult('worktree.rm', { worktree: toRuntimeWorktreeSelector(worktreeId), force, + // Why (#11960): the web client renders the same Force Delete affordances, so + // dropping this field here would leave paired clients permanently wedged. + allowUnverifiedPtyStop, runHooks: skipArchive !== true }) }, diff --git a/src/shared/worktree-removal-force-classification.test.ts b/src/shared/worktree-removal-force-classification.test.ts new file mode 100644 index 00000000000..1d995d412f4 --- /dev/null +++ b/src/shared/worktree-removal-force-classification.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from 'vitest' +import { classifyWorktreeForceDeleteReason, WORKTREE_TEARDOWN_FORCE_HINT } from './worktree-removal' + +// 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') + }) + + it('offers force when the stop could not be verified', () => { + expect(classifyWorktreeForceDeleteReason(unverifiableError)).toBe('unstopped-pty') + }) + + // Why (#11960): the ordinary delete confirmation already passes force:true to skip + // the dirty-file prompt. If that suppressed the offer, the most common desktop + // delete would hit the gate with no Force Delete button anywhere — the original + // dead end, restored. + it('still offers force when the failed attempt only set force', () => { + expect(classifyWorktreeForceDeleteReason(liveError, true)).toBe('unstopped-pty') + }) + + it('does not re-offer force once the waiver itself was already used', () => { + expect(classifyWorktreeForceDeleteReason(liveError, true, true)).toBeNull() + expect(classifyWorktreeForceDeleteReason(liveError, false, true)).toBeNull() + }) + + it('leaves unrelated failures unclassified', () => { + expect(classifyWorktreeForceDeleteReason('some other failure')).toBeNull() + }) +}) diff --git a/src/shared/worktree-removal.ts b/src/shared/worktree-removal.ts index b043bdcfa94..17e1b719cae 100644 --- a/src/shared/worktree-removal.ts +++ b/src/shared/worktree-removal.ts @@ -2,7 +2,22 @@ import type { GitWorktreeInfo } from './types' export const LOCKED_WORKTREE_REMOVAL_PREFIX = 'Worktree is locked by Git.' -export type WorktreeForceDeleteReason = 'dirty' | 'orphan-directory' | 'missing-registration' +export const UNSTOPPED_PTY_REMOVAL_PREFIX = 'Failed to physically stop every PTY for worktree:' + +// Why (#11960): the desktop force affordance is driven entirely by the classifier +// below, so this hint and its matcher must stay in the same file — a message that +// tells the user to force-delete while the UI hides the button is the same dead end. +export const WORKTREE_TEARDOWN_FORCE_HINT = 'Retry with force delete (--force) to remove it anyway.' + +export type WorktreeForceDeleteReason = + | 'dirty' + | 'orphan-directory' + | 'missing-registration' + | 'unstopped-pty' + +export function isUnstoppedPtyRemovalError(error: string): boolean { + return error.includes(UNSTOPPED_PTY_REMOVAL_PREFIX) +} export function createLockedWorktreeRemovalError(lockReason?: string): Error { const reason = lockReason?.trim() @@ -46,13 +61,21 @@ const FORMATTED_DIRTY_WORKTREE_REMOVAL_PATTERN = export function classifyWorktreeForceDeleteReason( error: string, - force = false + force = false, + allowUnverifiedPtyStop = false ): WorktreeForceDeleteReason | null { if (isLockedWorktreeRemovalError(error)) { // Why: a Git lock can represent an external safety contract. It must be // unlocked explicitly rather than folded into Orca's dirty-file force path. return null } + // Why (#11960): this must be decided before the `force` guard below. The ordinary + // delete confirmation already passes force:true to skip the dirty-file prompt, but + // it does NOT waive PTY-stop proof — so `force` alone is no evidence that the user + // has already spent this escape hatch. Only the waiver itself is. + if (isUnstoppedPtyRemovalError(error)) { + return allowUnverifiedPtyStop ? null : 'unstopped-pty' + } if (force) { return null }