From 525ffc5ae04a8ff72ac03a96201d7779da493b8e Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Sun, 2 Aug 2026 19:16:58 -0700 Subject: [PATCH] fix(worktree): stop the PTY gate from permanently wedging workspace removal (#12153) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Destructive worktree removal proves every PTY is dead before touching the filesystem. When a stop RPC failed, it re-listed the provider to check whether the PTY had already exited — but on the same deadline the sweeps had just spent, so it timed out without ever asking and read "could not verify" as "still live". The sweep spends that budget every run, making the refusal deterministic; --force never reached the gate, so the workspace was unremovable forever. - Verification gets its own budget instead of an exhausted remainder. - Verdicts split into exited / live / unverifiable; the error names the blocking PTY ids and why. - A reachable escape hatch: allowUnverifiedPtyStop, set only by genuine Force Delete affordances and the CLI's --force — never by the force the ordinary delete confirmation already sets — with an 'unstopped-pty' classifier reason so the desktop actually offers the button. - Force also survives a sweep that cannot complete; the non-force path still fails fast. Fixes #11960 --- src/cli/handlers/worktree.ts | 2 + src/main/ipc/worktrees.test.ts | 50 +++ src/main/ipc/worktrees.ts | 49 ++- src/main/runtime/orca-runtime.ts | 49 ++- .../pty-waiver-source-invariant.test.ts | 61 +++ .../methods/worktree-rm-pty-waiver.test.ts | 47 +++ .../runtime/rpc/methods/worktree-schemas.ts | 4 + src/main/runtime/rpc/methods/worktree.test.ts | 3 +- src/main/runtime/rpc/methods/worktree.ts | 3 +- src/main/runtime/settle-before-deadline.ts | 54 +++ .../runtime/unstopped-pty-verification.ts | 70 ++++ .../worktree-teardown-unstopped-pty.test.ts | 368 ++++++++++++++++++ src/main/runtime/worktree-teardown.ts | 172 ++++---- src/preload/api-types.ts | 4 + .../sidebar/DeleteWorktreeDialog.tsx | 4 +- .../sidebar/delete-worktree-flow.test.ts | 6 +- .../sidebar/delete-worktree-flow.ts | 6 +- .../sidebar/delete-worktree-toast.test.ts | 17 + .../sidebar/delete-worktree-toast.ts | 14 + .../status-bar/WorkspaceSpaceManagerPanel.tsx | 3 +- src/renderer/src/i18n/locales/en.json | 3 +- .../src/store/slices/worktree-helpers.ts | 3 + .../src/store/slices/worktrees.test.ts | 40 +- src/renderer/src/store/slices/worktrees.ts | 15 +- src/renderer/src/web/web-preload-api.ts | 5 +- ...ktree-removal-force-classification.test.ts | 35 ++ src/shared/worktree-removal.ts | 27 +- 27 files changed, 995 insertions(+), 119 deletions(-) create mode 100644 src/main/runtime/pty-waiver-source-invariant.test.ts create mode 100644 src/main/runtime/rpc/methods/worktree-rm-pty-waiver.test.ts create mode 100644 src/main/runtime/settle-before-deadline.ts create mode 100644 src/main/runtime/unstopped-pty-verification.ts create mode 100644 src/main/runtime/worktree-teardown-unstopped-pty.test.ts create mode 100644 src/shared/worktree-removal-force-classification.test.ts 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 }