diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 7e50c874f34..c02e225d1ea 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -9840,6 +9840,37 @@ describe('registerWorktreeHandlers', () => { expect(callOrder).toEqual(['preflight', 'kill', 'git']) }) + // Why (#11960): --force previously stopped at the same PTY gate as a normal + // delete, leaving a workspace with an unprovable PTY unremovable forever. + it('forwards force to the PTY gate so a forced delete has an escape hatch', 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.objectContaining({ requirePhysicalStop: true, allowUnverifiedStop: true }) + ) + }) + + it('keeps the PTY gate strict for a non-forced 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 fe2d6d5fff3..c98b0a46899 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -157,8 +157,9 @@ type DetectedWorktreeRequestArgs = { repoId: string } | ListDetectedWorktreesArg async function stopPtysForDestructiveWorktreeRemoval( runtime: OrcaRuntimeService, worktreeId: string, - connectionId?: string + options: { connectionId?: string; force?: boolean } = {} ): Promise { + const { connectionId, force } = options const provider = connectionId ? getSshPtyProvider(connectionId) : getLocalPtyProvider() if (!provider) { throw new Error(`PTY provider unavailable for worktree deletion: ${worktreeId}`) @@ -168,6 +169,8 @@ async function stopPtysForDestructiveWorktreeRemoval( localProvider: provider, onPtyStopped: clearProviderPtyState, requirePhysicalStop: true, + // Why (#11960): without this, an unprovable stop left the workspace permanently unremovable — force had no way past the gate. + ...(force ? { allowUnverifiedStop: true } : {}), ...(connectionId ? { includeLocalRegistry: false } : {}) }) const total = @@ -2357,11 +2360,10 @@ export function registerWorktreeHandlers( ) let removalCompleted = false try { - await stopPtysForDestructiveWorktreeRemoval( - runtime, - args.worktreeId, - repo.connectionId - ) + await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId, { + connectionId: repo.connectionId, + force: args.force + }) await fsProvider!.deletePath(worktreePath, true) removalCompleted = true } finally { @@ -2378,7 +2380,9 @@ export function registerWorktreeHandlers( const removalGate = await runtime.acquireFileWatcherRemoval(worktreePath) let removalCompleted = false try { - await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId) + await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId, { + force: args.force + }) await removeLocalWorktreePath(worktreePath, localWorktreeGitOptions) removalCompleted = true } finally { @@ -2423,7 +2427,9 @@ export function registerWorktreeHandlers( const removalGate = await runtime.acquireFileWatcherRemoval(worktreePath) let removalCompleted = false try { - await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId) + await stopPtysForDestructiveWorktreeRemoval(runtime, args.worktreeId, { + force: args.force + }) await removeLocalWorktreePath(worktreePath, localWorktreeGitOptions) removalCompleted = true } finally { @@ -2581,11 +2587,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, + force: args.force + }) }) rawRemovalResult = await withWorktreeRemoveStageSpan( 'git_remove', @@ -2688,7 +2693,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, { + force: args.force + }) }) // 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 8c75a00d579..b124a628605 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -3302,8 +3302,9 @@ export class OrcaRuntimeService { private async stopPtysForDestructiveWorktreeRemoval( worktreeId: string, - connectionId?: string + options: { connectionId?: string; force?: boolean } = {} ): Promise { + const { connectionId, force } = options const provider = connectionId ? this.getSshProviderFn?.(connectionId) : this.getLocalProvider() if (!provider) { throw new Error(`PTY provider unavailable for worktree deletion: ${worktreeId}`) @@ -3313,6 +3314,8 @@ export class OrcaRuntimeService { localProvider: provider, onPtyStopped: this.onPtyStopped ?? undefined, requirePhysicalStop: true, + // Why (#11960): without this, an unprovable stop left the workspace permanently unremovable — force had no way past the gate. + ...(force ? { allowUnverifiedStop: true } : {}), ...(connectionId ? { includeLocalRegistry: false } : {}) }) const total = @@ -23412,10 +23415,10 @@ export class OrcaRuntimeService { ) let removalCompleted = false try { - await this.stopPtysForDestructiveWorktreeRemoval( - removalTarget.id, - repo.connectionId - ) + await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id, { + connectionId: repo.connectionId, + force + }) await fsProvider!.deletePath(removalTarget.path, true) removalCompleted = true } finally { @@ -23432,7 +23435,7 @@ export class OrcaRuntimeService { const removalGate = await this.acquireFileWatcherRemoval(removalTarget.path) let removalCompleted = false try { - await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id) + await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id, { force }) await removeLocalWorktreePath(removalTarget.path, localWorktreeGitOptions) removalCompleted = true } finally { @@ -23480,7 +23483,7 @@ export class OrcaRuntimeService { const removalGate = await this.acquireFileWatcherRemoval(removalTarget.path) let removalCompleted = false try { - await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id) + await this.stopPtysForDestructiveWorktreeRemoval(removalTarget.id, { force }) await removeLocalWorktreePath(removalTarget.path, localWorktreeGitOptions) removalCompleted = true } finally { @@ -23599,7 +23602,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, + force + }) rawRemovalResult = await (Object.keys(remoteRemoveOptions).length > 0 ? provider!.removeWorktree(canonicalWorktreePath, force, remoteRemoveOptions) : provider!.removeWorktree(canonicalWorktreePath, force)) @@ -23710,7 +23716,7 @@ 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, { force }) if (linkedPaths.length > 0) { await removeWorktreeLinkedPaths(canonicalWorktreePath, linkedPaths) diff --git a/src/main/runtime/settle-before-deadline.ts b/src/main/runtime/settle-before-deadline.ts new file mode 100644 index 00000000000..214d2d29223 --- /dev/null +++ b/src/main/runtime/settle-before-deadline.ts @@ -0,0 +1,50 @@ +/** + * 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?.() + void 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..7a2c3f3a5fd --- /dev/null +++ b/src/main/runtime/unstopped-pty-verification.ts @@ -0,0 +1,61 @@ +import type { IPtyProvider } from '../providers/types' +import { settleBeforeDeadline } from './settle-before-deadline' + +// Why (#11960): the sweeps normally spend the whole teardown deadline, so +// re-listing on that same budget answered "unverifiable" for PTYs that had +// already exited and wedged the workspace forever. Verification gets its own +// small window instead of inheriting an exhausted one. +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. + */ +export async function verifyUnstoppedPtys( + failedPtyIds: readonly string[], + provider: IPtyProvider, + deadline: number +): Promise { + const verifyDeadline = Math.max(deadline, Date.now() + WORKTREE_TEARDOWN_VERIFY_GRACE_MS) + 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 `Failed to physically stop every PTY for worktree: ${worktreeId} — ${detail}` +} 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..db7f210d569 --- /dev/null +++ b/src/main/runtime/worktree-teardown-unstopped-pty.test.ts @@ -0,0 +1,125 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { listRegisteredPtysMock } = vi.hoisted(() => ({ + listRegisteredPtysMock: vi.fn() +})) + +vi.mock('../memory/pty-registry', () => ({ + listRegisteredPtys: listRegisteredPtysMock +})) + +import { killAllProcessesForWorktree } from './worktree-teardown' +import { WORKTREE_TEARDOWN_VERIFY_GRACE_MS } from './unstopped-pty-verification' +import type { IPtyProvider, PtyProcessInfo } from '../providers/types' + +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 = killAllProcessesForWorktree('w1', { + localProvider, + timeoutMs: 100, + requirePhysicalStop: true + }) + await vi.advanceTimersByTimeAsync(WORKTREE_TEARDOWN_VERIFY_GRACE_MS + 200) + + 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(/w1@@live-1[\s\S]*--force/) + }) + + 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/) + }) + + 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(onPtyStopped).toHaveBeenCalledWith('w1@@live-1') + expect(warn).toHaveBeenCalledWith(expect.stringContaining('w1@@live-1')) + } finally { + warn.mockRestore() + } + }) +}) diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index 64b6a09703f..1558171e68e 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -4,6 +4,12 @@ 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 { settleBeforeDeadline } from './settle-before-deadline' +import { + 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 +27,8 @@ export type WorktreeTeardownDeps = { onPtyStopped?: (ptyId: string) => void timeoutMs?: number requirePhysicalStop?: boolean + /** Escape hatch for `--force`: warn instead of throwing when a stop stays unproven (#11960). */ + allowUnverifiedStop?: boolean includeProviderInventory?: boolean includeLocalRegistry?: boolean } @@ -63,7 +71,9 @@ export function teardownRpcDeadline(sweepDeadline: number): number { * daemon spawns. * * Sweeps are best-effort by default. Destructive removal callers set - * `requirePhysicalStop` so a timeout or unproven stop blocks filesystem work. + * `requirePhysicalStop` so a timeout or unproven stop blocks filesystem work, + * and pass `allowUnverifiedStop` for `--force` so that gate can never wedge a + * workspace permanently (#11960). */ export async function killAllProcessesForWorktree( worktreeId: string, @@ -164,11 +174,18 @@ export async function killAllProcessesForWorktree( [...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}`) + const verdict: UnstoppedPtyVerdict = + failedPtyIds.length === 0 + ? { status: 'exited' } + : await verifyUnstoppedPtys(failedPtyIds, deps.localProvider, deadline) + if (verdict.status !== 'exited') { + const summary = describeUnstoppedPtys(worktreeId, failedPtyIds, verdict) + if (!deps.allowUnverifiedStop) { + throw new Error(`${summary}. Retry with force delete (--force) to remove it anyway.`) + } + // Why: force is the documented escape hatch, so the removal continues — + // but the orphaned PTYs must stay visible instead of vanishing silently. + console.warn(`[worktree-teardown] forcing removal despite unstopped PTYs — ${summary}`) } for (const ptyId of failedPtyIds) { clearStoppedPtyState(ptyId, deps.onPtyStopped) @@ -178,66 +195,6 @@ export async function killAllProcessesForWorktree( 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,