fix(worktree): stop the PTY gate from wedging workspace removal (#11960)

Destructive worktree removal proves every PTY is physically dead before it
touches the filesystem. When a stop RPC failed, it re-listed the provider to
check whether the PTY had actually exited — but on the *same* deadline the
sweeps had just spent. A slow-but-successful sweep therefore reached the
re-list with ~0ms of budget, timed out, and reported "unverifiable", which the
gate treated as "still live". `--force` never reached this gate, so the
workspace became permanently unremovable and every retry failed identically.

- Verification now gets its own 2s window instead of inheriting an exhausted
  deadline, so an already-exited PTY can be proven exited.
- The verdict distinguishes live / unverifiable / exited, and the error names
  the blocking PTY ids plus the escape hatch instead of only the worktree.
- `--force` propagates to the gate (`allowUnverifiedStop`), downgrading an
  unproven stop to a loud warning so removal can always complete.

Extracts `settleBeforeDeadline` and the verification logic into their own
modules to stay under max-lines.
This commit is contained in:
OrcaWin
2026-08-02 17:33:50 -07:00
parent d426e35be3
commit 7a809a0ecd
7 changed files with 326 additions and 89 deletions
+31
View File
@@ -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)
+21 -14
View File
@@ -157,8 +157,9 @@ type DetectedWorktreeRequestArgs = { repoId: string } | ListDetectedWorktreesArg
async function stopPtysForDestructiveWorktreeRemoval(
runtime: OrcaRuntimeService,
worktreeId: string,
connectionId?: string
options: { connectionId?: string; force?: boolean } = {}
): Promise<void> {
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.
+15 -9
View File
@@ -3302,8 +3302,9 @@ export class OrcaRuntimeService {
private async stopPtysForDestructiveWorktreeRemoval(
worktreeId: string,
connectionId?: string
options: { connectionId?: string; force?: boolean } = {}
): Promise<void> {
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)
@@ -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<T>(
run: () => Promise<T>,
fallback: T,
deadline: number,
failClosedError?: Error,
failClosedOnRunError: (error: unknown) => boolean = () => true
): Promise<T> {
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)
)
})
}
@@ -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<UnstoppedPtyVerdict> {
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<UnstoppedPtyVerdict, { status: 'exited' }>
): 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}`
}
@@ -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<PtyProcessInfo[]>): 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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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()
}
})
})
+23 -66
View File
@@ -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<boolean> {
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<T>(
run: () => Promise<T>,
fallback: T,
deadline: number,
failClosedError?: Error,
failClosedOnRunError: (error: unknown) => boolean = () => true
): Promise<T> {
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,