fix: preserve terminal teardown ownership until exit

This commit is contained in:
Brennan Benson
2026-07-13 04:37:41 -07:00
parent a18b7fda26
commit bfd28680a3
15 changed files with 613 additions and 101 deletions
@@ -133,6 +133,40 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => {
expect(typeof result.id).toBe('string')
})
it('fails closed when a preserved legacy session has no observable pane identity', async () => {
const legacy = new DaemonPtyAdapter({ socketPath, tokenPath, protocolVersion: 21 })
const internals = legacy as unknown as {
client: {
ensureConnected: () => Promise<void>
request: () => Promise<unknown>
}
}
vi.spyOn(internals.client, 'ensureConnected').mockResolvedValue(undefined)
const request = vi.spyOn(internals.client, 'request').mockResolvedValue({
sessions: [
{
sessionId: 'legacy-session',
isAlive: true,
cwd: '/repo'
}
]
})
try {
await legacy.listProcesses()
await expect(
legacy.spawn({
sessionId: 'legacy-session',
cols: 80,
rows: 24,
env: { ORCA_PANE_KEY: 'tab-b:leaf-b', ORCA_TAB_ID: 'tab-b' }
})
).rejects.toThrow('Legacy PTY identity unavailable')
expect(request).toHaveBeenCalledOnce()
} finally {
legacy.dispose()
}
})
it('uses worktreeId as session prefix when provided', async () => {
const result = await adapter.spawn({ cols: 80, rows: 24, worktreeId: 'wt-1' })
expect(result.id).toContain('wt-1')
@@ -377,6 +411,21 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => {
})
describe('shutdown', () => {
it('retires identity-bearing shutdowns that cannot be proven on a restarted legacy adapter', async () => {
const legacy = new DaemonPtyAdapter({ socketPath, tokenPath, protocolVersion: 21 })
try {
await expect(
legacy.shutdown('legacy-session', {
immediate: true,
expectedPaneKey: 'tab-a:leaf-a',
expectedTabId: 'tab-a'
})
).rejects.toThrow('Legacy PTY identity unavailable')
} finally {
legacy.dispose()
}
})
it('carries pane identity through the daemon and rejects a stale kill', async () => {
const { id } = await adapter.spawn({
cols: 80,
+16 -5
View File
@@ -202,6 +202,10 @@ export class DaemonPtyAdapter implements IPtyProvider {
expectedPaneKey: requestedIdentity.paneKey ?? undefined,
expectedTabId: requestedIdentity.tabId ?? undefined
})
} else if (requestedIdentity.paneKey || requestedIdentity.tabId) {
// Why: preserved legacy daemons cannot report identity, so attaching a
// new pane by a reused ID could later authorize killing the old pane.
throw new Error(`Legacy PTY identity unavailable for "${sessionId}"`)
}
}
@@ -521,11 +525,11 @@ export class DaemonPtyAdapter implements IPtyProvider {
) {
// Why: preserved pre-v22 daemons ignore kill identity fields. Fail
// closed unless this main process observed the exact attached pane.
assertPtyPaneIdentity(
id,
this.paneIdentityBySessionId.get(id) ?? { paneKey: null, tabId: null },
opts
)
const knownIdentity = this.paneIdentityBySessionId.get(id)
if (!knownIdentity) {
throw new Error(`Legacy PTY identity unavailable for "${id}"`)
}
assertPtyPaneIdentity(id, knownIdentity, opts)
}
// Why: sleep/exact-stop kills the live PTY before the periodic checkpoint may run.
// Force a final snapshot so wake can restore the pane users left.
@@ -796,6 +800,13 @@ export class DaemonPtyAdapter implements IPtyProvider {
const result = await this.client.request<ListSessionsResult>('listSessions', undefined)
for (const session of result.sessions) {
this.rememberSessionIdentity(session)
if (session.isAlive) {
// Why: router startup discovers preserved legacy sessions through this
// method; spawn must know the ID is already live before createOrAttach.
this.activeSessionIds.add(session.sessionId)
} else {
this.activeSessionIds.delete(session.sessionId)
}
}
return result.sessions
.filter((s) => s.isAlive)
+132 -6
View File
@@ -411,12 +411,16 @@ describe('registerPtyHandlers', () => {
}
)
isPwshAvailableMock.mockReturnValue(false)
let defaultExitHandler: ((event: { exitCode: number }) => void) | null = null
spawnMock.mockReturnValue({
onData: vi.fn(() => makeDisposable()),
onExit: vi.fn(() => makeDisposable()),
onExit: vi.fn((handler: (event: { exitCode: number }) => void) => {
defaultExitHandler = handler
return makeDisposable()
}),
write: vi.fn(),
resize: vi.fn(),
kill: vi.fn(),
kill: vi.fn(() => defaultExitHandler?.({ exitCode: -1 })),
process: 'zsh',
pid: 12345
})
@@ -3494,10 +3498,124 @@ describe('registerPtyHandlers', () => {
})
expect(store.removePendingLocalPtyShutdown).toHaveBeenCalledWith('local-pty')
await vi.advanceTimersByTimeAsync(30_000)
await vi.advanceTimersByTimeAsync(30_000)
vi.useRealTimers()
})
it('keeps accepted local kills until provider readback proves physical exit', async () => {
vi.useFakeTimers()
const shutdown = vi.fn().mockResolvedValue(undefined)
const listProcesses = vi
.fn()
.mockResolvedValueOnce([{ id: 'local-pty', cwd: '', title: 'shell' }])
.mockResolvedValue([])
const removePendingLocalPtyShutdown = vi.fn()
setLocalPtyProvider({
requiresShutdownExitProof: true,
shutdown,
listProcesses,
onData: vi.fn(() => () => {}),
onReplay: vi.fn(() => () => {}),
onExit: vi.fn(() => () => {})
} as never)
handlers.clear()
registerPtyHandlers(mainWindow as never, undefined, undefined, undefined, undefined, {
getPendingLocalPtyShutdowns: () => [],
upsertPendingLocalPtyShutdown: vi.fn(),
removePendingLocalPtyShutdown
} as never)
await expect(handlers.get('pty:kill')!(null, { id: 'local-pty' })).rejects.toThrow(
'process exit is not yet confirmed'
)
expect(removePendingLocalPtyShutdown).not.toHaveBeenCalled()
await vi.advanceTimersByTimeAsync(250)
expect(shutdown).toHaveBeenCalledTimes(2)
expect(removePendingLocalPtyShutdown).toHaveBeenCalledWith('local-pty')
vi.useRealTimers()
})
it('does not let an old provider exit readback settle replacement retry ownership', async () => {
vi.useFakeTimers()
type ProcessInfo = { id: string; cwd: string; title: string }
let resolveOldReadback!: (processes: ProcessInfo[]) => void
const oldReadback = new Promise<ProcessInfo[]>((resolve) => {
resolveOldReadback = resolve
})
const oldListProcesses = vi.fn(() => oldReadback)
const newShutdown = vi.fn().mockResolvedValue(undefined)
const provider = (
shutdown: (id: string) => Promise<void>,
listProcesses: () => Promise<ProcessInfo[]>
) =>
({
requiresShutdownExitProof: true,
shutdown,
listProcesses,
onData: vi.fn(() => () => {}),
onReplay: vi.fn(() => () => {}),
onExit: vi.fn(() => () => {})
}) as never
setLocalPtyProvider(provider(vi.fn().mockResolvedValue(undefined), oldListProcesses))
handlers.clear()
registerPtyHandlers(mainWindow as never, undefined, undefined, undefined, undefined, {
getPendingLocalPtyShutdowns: () => [],
upsertPendingLocalPtyShutdown: vi.fn(),
removePendingLocalPtyShutdown: vi.fn()
} as never)
const kill = handlers.get('pty:kill')!(null, { id: 'local-pty' }) as Promise<void>
await vi.waitFor(() => expect(oldListProcesses).toHaveBeenCalledOnce())
setLocalPtyProvider(provider(newShutdown, vi.fn().mockResolvedValue([])))
resolveOldReadback([])
await expect(kill).resolves.toBeUndefined()
expect(newShutdown).toHaveBeenCalledOnce()
await vi.advanceTimersByTimeAsync(30_000)
expect(vi.getTimerCount()).toBe(0)
vi.useRealTimers()
})
it('retires a persisted legacy-daemon shutdown that cannot recover pane identity', async () => {
vi.useFakeTimers()
const removePendingLocalPtyShutdown = vi.fn()
const onPtyExit = vi.fn()
setLocalPtyProvider({
shutdown: vi.fn().mockRejectedValue(new Error('Legacy PTY identity unavailable for "p"')),
onData: vi.fn(() => () => {}),
onReplay: vi.fn(() => () => {}),
onExit: vi.fn(() => () => {})
} as never)
handlers.clear()
registerPtyHandlers(
mainWindow as never,
{ onPtyExit, setPtyController: vi.fn() } as never,
undefined,
undefined,
undefined,
{
getPendingLocalPtyShutdowns: () => [
{
ptyId: 'legacy-pty',
expectedPaneKey: 'tab-a:leaf-a',
expectedTabId: 'tab-a',
requestedAt: 1
}
],
upsertPendingLocalPtyShutdown: vi.fn(),
removePendingLocalPtyShutdown
} as never
)
await vi.advanceTimersByTimeAsync(0)
expect(removePendingLocalPtyShutdown).toHaveBeenCalledWith('legacy-pty')
expect(onPtyExit).not.toHaveBeenCalled()
await vi.advanceTimersByTimeAsync(30_000)
vi.useRealTimers()
})
it('re-drives a direct local kill when its provider is replaced in flight', async () => {
vi.useFakeTimers()
const oldShutdown = makeDeferred()
@@ -11331,10 +11449,14 @@ describe('registerPtyHandlers', () => {
// in local-pty-provider reassigns proc.kill to a no-op to defuse the
// SIGHUP-to-recycled-pid hazard (see docs/fix-pty-fd-leak.md). Reading
// proc.kill.mock after that runs would yield a non-mock and crash.
const killSpy = vi.fn()
let exitCb: ((info: { exitCode: number }) => void) | null = null
const killSpy = vi.fn(() => exitCb?.({ exitCode: -1 }))
const proc = {
onData: vi.fn(() => onDataDisposable),
onExit: vi.fn(() => onExitDisposable),
onExit: vi.fn((cb: (info: { exitCode: number }) => void) => {
exitCb = cb
return onExitDisposable
}),
write: vi.fn(),
resize: vi.fn(),
kill: killSpy,
@@ -11362,10 +11484,14 @@ describe('registerPtyHandlers', () => {
it('keeps PTY listeners until runtime-controller native kill is accepted', async () => {
const onDataDisposable = makeDisposable()
const onExitDisposable = makeDisposable()
const killSpy = vi.fn()
let exitCb: ((info: { exitCode: number }) => void) | null = null
const killSpy = vi.fn(() => exitCb?.({ exitCode: -1 }))
const proc = {
onData: vi.fn(() => onDataDisposable),
onExit: vi.fn(() => onExitDisposable),
onExit: vi.fn((cb: (info: { exitCode: number }) => void) => {
exitCb = cb
return onExitDisposable
}),
write: vi.fn(),
resize: vi.fn(),
kill: killSpy,
+61 -1
View File
@@ -188,6 +188,7 @@ type RetainedPtyShutdown = {
}
const pendingSshShutdownRetries = new Map<string, RetainedPtyShutdown>()
const pendingLocalShutdownRetries = new Map<string, RetainedPtyShutdown>()
const sshRetainedExitUnsubscribers = new Map<string, () => void>()
let retainedShutdownRetryTimer: ReturnType<typeof setTimeout> | null = null
const MAX_RETAINED_SHUTDOWN_BACKOFF_MS = 30_000
const SYNTHETIC_KILL_EXIT_DUPLICATE_WINDOW_MS = 30_000
@@ -627,6 +628,19 @@ function completeRetainedPtyShutdown(
}
}
function retireUnprovableLegacyPtyShutdown(retained: RetainedPtyShutdown): void {
const ownerMap = retained.connectionId ? pendingSshShutdownRetries : pendingLocalShutdownRetries
if (ownerMap.get(retained.id) !== retained) {
return
}
// Why: legacy restart loses pane identity permanently. Drop only the stale
// durable request; without exit proof, provider/UI ownership must survive.
ownerMap.delete(retained.id)
if (!retained.connectionId) {
retained.store?.removePendingLocalPtyShutdown?.(retained.id)
}
}
function attemptRetainedPtyShutdown(retained: RetainedPtyShutdown): Promise<void> {
if (retained.inFlight) {
return retained.inFlight
@@ -642,11 +656,22 @@ function attemptRetainedPtyShutdown(retained: RetainedPtyShutdown): Promise<void
retained.provider = currentProvider
retained.lastError = null
const attempt = shutdownProviderAndDetectExit(currentProvider, retained.id, retained.options)
.then((providerExitObserved) => {
.then(async (providerExitObserved) => {
// Why: provider replacement can finish while an older shutdown is in flight.
if (retained.provider !== currentProvider) {
return
}
if (currentProvider.requiresShutdownExitProof && !providerExitObserved) {
const stopped = await verifyPtyStopped(currentProvider, retained.id, retained.options)
// Why: provider replacement can also occur during the async liveness
// readback; only the provider that still owns this retry may settle it.
if (retained.provider !== currentProvider) {
return
}
if (!stopped) {
throw new Error('PTY shutdown accepted but process exit is not yet confirmed')
}
}
completeRetainedPtyShutdown(retained, providerExitObserved)
})
.catch((error: unknown) => {
@@ -663,6 +688,12 @@ function attemptRetainedPtyShutdown(retained: RetainedPtyShutdown): Promise<void
completeRetainedPtyShutdown(retained, false)
return
}
if (/Legacy PTY identity unavailable/i.test(error instanceof Error ? error.message : '')) {
// Why: after restart a pre-v22 daemon cannot prove pane identity. Retire
// the stale durable owner instead of retrying forever or killing by ID.
retireUnprovableLegacyPtyShutdown(retained)
return
}
retained.lastError = error instanceof Error ? error : new Error(String(error))
retained.attempts += 1
retained.nextRetryAt =
@@ -1344,7 +1375,26 @@ function routesFreshSpawnsToLocalProvider(
/** Register an SSH PTY provider for a connection. */
export function registerSshPtyProvider(connectionId: string, provider: IPtyProvider): void {
sshRetainedExitUnsubscribers.get(connectionId)?.()
sshRetainedExitUnsubscribers.delete(connectionId)
sshProviders.set(connectionId, provider)
if (typeof provider.onExit === 'function') {
sshRetainedExitUnsubscribers.set(
connectionId,
provider.onExit((payload) => {
const retained = pendingSshShutdownRetries.get(payload.id)
if (
retained?.connectionId === connectionId &&
retained.provider === provider &&
sshProviders.get(connectionId) === provider
) {
// Why: exit may arrive after shutdown RPC acceptance. Settle the exact
// provider owner here so its next timer cannot synthesize a duplicate.
completeRetainedPtyShutdown(retained, true)
}
})
)
}
// Durable leases reconcile generation first; a stale boot-qualified owner
// must be dropped before any replacement provider can observe it.
retryPersistedSshShutdowns(connectionId, provider)
@@ -1363,6 +1413,8 @@ export function registerSshPtyProvider(connectionId: string, provider: IPtyProvi
/** Remove an SSH PTY provider when a connection is closed. */
export function unregisterSshPtyProvider(connectionId: string): void {
sshRetainedExitUnsubscribers.get(connectionId)?.()
sshRetainedExitUnsubscribers.delete(connectionId)
sshProviders.delete(connectionId)
for (const retained of pendingSshShutdownRetries.values()) {
if (retained.connectionId === connectionId) {
@@ -2010,6 +2062,10 @@ export function registerPtyHandlers(
},
onSpawned: (id) => runtime?.onPtySpawned(id),
onExit: (id, code) => {
const retained = pendingLocalShutdownRetries.get(id)
if (retained?.provider === localProvider) {
completeRetainedPtyShutdown(retained, true)
}
clearProviderPtyState(id)
ptyOwnership.delete(id)
markClaudePtyExited(id)
@@ -3145,6 +3201,10 @@ export function registerPtyHandlers(
return
}
if (!isLocalProvider) {
const retained = pendingLocalShutdownRetries.get(payload.id)
if (retained?.provider === localProvider) {
completeRetainedPtyShutdown(retained, true)
}
clearProviderPtyState(payload.id)
ptyOwnership.delete(payload.id)
markClaudePtyExited(payload.id)
+21 -4
View File
@@ -978,6 +978,20 @@ describe('LocalPtyProvider', () => {
expect(killSpy).toHaveBeenCalledWith('SIGKILL')
})
it('retains the native PTY until an accepted kill produces exit proof', async () => {
mockProc.kill.mockImplementation(() => {})
const { id } = await provider.spawn({ cols: 80, rows: 24 })
await provider.shutdown(id, { immediate: true })
expect(provider.hasPty(id)).toBe(true)
expect(await provider.listProcesses()).toEqual(
expect.arrayContaining([expect.objectContaining({ id })])
)
exitCb?.({ exitCode: 137 })
expect(provider.hasPty(id)).toBe(false)
})
it('invokes onExit callback via the node-pty exit handler', async () => {
const onExit = vi.fn()
provider.configure({ onExit })
@@ -988,7 +1002,7 @@ describe('LocalPtyProvider', () => {
it('does not destroy after an intentional Windows shutdown kill', async () => {
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
const killSpy = vi.fn()
const killSpy = vi.fn(() => exitCb?.({ exitCode: -1 }))
const destroySpy = vi.fn(() => {
killSpy()
})
@@ -1007,9 +1021,12 @@ describe('LocalPtyProvider', () => {
it('retains Windows ownership when native shutdown throws so retry can succeed', async () => {
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
const kill = vi.fn().mockImplementationOnce(() => {
throw new Error('native close failed')
})
const kill = vi
.fn()
.mockImplementationOnce(() => {
throw new Error('native close failed')
})
.mockImplementationOnce(() => exitCb?.({ exitCode: -1 }))
spawnMock.mockReturnValue({ ...mockProc, kill })
const { id } = await provider.spawn({ cols: 80, rows: 24 })
+13 -8
View File
@@ -328,6 +328,7 @@ export type LocalPtyProviderOptions = {
}
export class LocalPtyProvider implements IPtyProvider {
readonly requiresShutdownExitProof = true
private opts: LocalPtyProviderOptions
constructor(opts: LocalPtyProviderOptions = {}) {
@@ -916,6 +917,7 @@ export class LocalPtyProvider implements IPtyProvider {
// onExit an independent death proof without racing duplicate cleanup.
ptyShutdownInProgress.add(id)
let exitCode = -1
let exitObserved = false
try {
// Why: a renderer or paired/mobile close is destructive. POSIX's
// default SIGHUP can be ignored; Windows node-pty requires no argument.
@@ -931,17 +933,20 @@ export class LocalPtyProvider implements IPtyProvider {
} finally {
ptyShutdownInProgress.delete(id)
exitCode = ptyExitDuringShutdown.get(id) ?? -1
exitObserved = ptyExitDuringShutdown.has(id)
ptyExitDuringShutdown.delete(id)
}
// Timer/writer cleanup happens only after native shutdown was accepted;
// disposing first would suppress the natural exit path on a thrown kill.
// Why: kill returning is request acceptance, not process-death proof. Keep
// the native entry and listeners until onExit (possibly async) confirms it.
runPtyCleanup(id)
disposePtyListeners(id)
destroyPtyProcess(proc, { alreadyKilled: true })
clearPtyState(id)
this.opts.onExit?.(id, exitCode)
for (const cb of exitListeners) {
cb({ id, code: exitCode })
if (exitObserved) {
disposePtyListeners(id)
destroyPtyProcess(proc, { alreadyKilled: true })
clearPtyState(id)
this.opts.onExit?.(id, exitCode)
for (const cb of exitListeners) {
cb({ id, code: exitCode })
}
}
}
+1
View File
@@ -39,6 +39,7 @@ export function isSshPtyIdentityMismatchError(err: unknown): boolean {
* as LocalPtyProvider so the dispatch layer can route transparently.
*/
export class SshPtyProvider implements IPtyProvider {
readonly requiresShutdownExitProof = true
private mux: SshChannelMultiplexer
private connectionId: string
private dataListeners = new Set<DataCallback>()
+3
View File
@@ -161,6 +161,9 @@ export type PtyProcessInfo = {
}
export type IPtyProvider = {
/** A returned kill request is only acceptance; retain ownership until an
* exit event or listProcesses readback proves the backing process is gone. */
readonly requiresShutdownExitProof?: boolean
spawn(opts: PtySpawnOptions): Promise<PtySpawnResult>
attach(id: string): Promise<void>
hasPty?: (id: string) => boolean
+53
View File
@@ -23406,6 +23406,59 @@ describe('OrcaRuntimeService', () => {
await localRemoval
})
it('allows a resolved create when only another host removal starts during resolution', async () => {
const runtime = new OrcaRuntimeService(store)
const resolution = deferred<{
id: string
path: string
hostId: 'ssh:target-1'
connectionId: 'target-1'
repo: ReturnType<typeof store.getRepo>
folderWorkspace: null
}>()
const runtimeWithResolver = runtime as unknown as {
resolveTerminalWorkspaceLaunchScope: () => typeof resolution.promise
}
vi.spyOn(runtimeWithResolver, 'resolveTerminalWorkspaceLaunchScope').mockReturnValue(
resolution.promise
)
const spawn = vi.fn().mockResolvedValue({ id: 'ssh-pty' })
runtime.setPtyController({
spawn,
write: () => true,
kill: () => false,
getForegroundProcess: async () => null,
listProcesses: async () => []
})
const holdLocalRemoval = deferred<void>()
const localRemoval = runtime.runWithWorktreeRemovalAdmission(
TEST_WORKTREE_ID,
() => holdLocalRemoval.promise,
'local'
)
await Promise.resolve()
const create = runtime.createTerminal(`id:${TEST_WORKTREE_ID}`)
await runtime.runWithWorktreeRemovalAdmission(
TEST_WORKTREE_ID,
async () => undefined,
'ssh:target-2'
)
resolution.resolve({
id: TEST_WORKTREE_ID,
path: '/home/dev/orca',
hostId: 'ssh:target-1',
connectionId: 'target-1',
repo: store.getRepo(TEST_REPO_ID),
folderWorkspace: null
})
await expect(create).resolves.toMatchObject({ ptyId: 'ssh-pty' })
expect(spawn).toHaveBeenCalledWith(expect.objectContaining({ connectionId: 'target-1' }))
holdLocalRemoval.resolve()
await localRemoval
})
it('drains a create-first managed worktree before project removal snapshots ownership', async () => {
const removeProject = vi.fn()
const runtime = new OrcaRuntimeService({ ...store, removeProject } as never)
+26 -1
View File
@@ -2263,6 +2263,8 @@ export class OrcaRuntimeService {
// respect to paired/mobile terminal creation, or a late spawn is orphaned.
private terminalAdmissionBlockedWorktreeIds = new Set<string>()
private terminalRemovalGeneration = 0
private terminalRemovalGenerationByScope = new Map<string, number>()
private unresolvedTerminalAdmissionCount = 0
private terminalAdmissionsByWorktreeId = new Map<string, Set<Promise<void>>>()
private projectRemovalBlockedRepoIds = new Set<string>()
private worktreeCreateAdmissionsByRepoId = new Map<string, Set<Promise<void>>>()
@@ -12421,9 +12423,21 @@ export class OrcaRuntimeService {
private blockTerminalAdmissionScope(scopeKey: string): void {
this.terminalRemovalGeneration += 1
// Why: unresolved selectors only need short-lived history while they await
// host discovery; clearing it at zero avoids retaining removed worktree IDs.
if (this.unresolvedTerminalAdmissionCount > 0) {
this.terminalRemovalGenerationByScope.set(scopeKey, this.terminalRemovalGeneration)
}
this.terminalAdmissionBlockedWorktreeIds.add(scopeKey)
}
private terminalRemovalGenerationForScope(worktreeId: string, hostId: ExecutionHostId): number {
return Math.max(
this.terminalRemovalGenerationByScope.get(worktreeId) ?? 0,
this.terminalRemovalGenerationByScope.get(this.admissionScopeKey(worktreeId, hostId)) ?? 0
)
}
private async drainTerminalAdmissions(
worktreeIds: readonly string[],
hostId?: ExecutionHostId | null,
@@ -12445,6 +12459,7 @@ export class OrcaRuntimeService {
let releaseResolutionAdmission: (() => void) | null = null
let removalsActiveAtResolutionStart: ReadonlySet<string> | null = null
let removalGenerationAtResolutionStart: number | null = null
let tracksRemovalGeneration = false
try {
releaseResolutionAdmission = this.beginTerminalAdmission(GLOBAL_TERMINAL_ADMISSION_KEY)
} catch (error) {
@@ -12459,12 +12474,15 @@ export class OrcaRuntimeService {
// cannot reopen the exact stale scope before the atomic transfer.
removalsActiveAtResolutionStart = new Set(this.terminalAdmissionBlockedWorktreeIds)
removalGenerationAtResolutionStart = this.terminalRemovalGeneration
this.unresolvedTerminalAdmissionCount += 1
tracksRemovalGeneration = true
}
try {
const workspace = await this.resolveTerminalWorkspaceLaunchScope(selector, hostId)
if (
removalsActiveAtResolutionStart &&
(removalGenerationAtResolutionStart !== this.terminalRemovalGeneration ||
((removalGenerationAtResolutionStart ?? 0) <
this.terminalRemovalGenerationForScope(workspace.id, workspace.hostId) ||
this.isAdmissionBlocked(removalsActiveAtResolutionStart, workspace.id, workspace.hostId))
) {
throw new Error('terminal_admission_blocked_for_project_removal')
@@ -12475,6 +12493,13 @@ export class OrcaRuntimeService {
} catch (error) {
releaseResolutionAdmission?.()
throw error
} finally {
if (tracksRemovalGeneration) {
this.unresolvedTerminalAdmissionCount -= 1
if (this.unresolvedTerminalAdmissionCount === 0) {
this.terminalRemovalGenerationByScope.clear()
}
}
}
}
+57 -19
View File
@@ -696,10 +696,21 @@ describe('PtyHandler', () => {
it('keeps stale-spawn fallback ownership when the first native kill throws', async () => {
const firstError = new Error('native ConPTY kill failed')
const killSpy = vi.fn().mockImplementationOnce(() => {
throw firstError
})
const term = { ...mockPtyInstance, kill: killSpy, onData: vi.fn(), onExit: vi.fn() }
let onExitCb: ((event: { exitCode: number }) => void) | null = null
const killSpy = vi
.fn()
.mockImplementationOnce(() => {
throw firstError
})
.mockImplementationOnce(() => onExitCb?.({ exitCode: 137 }))
const term = {
...mockPtyInstance,
kill: killSpy,
onData: vi.fn(),
onExit: vi.fn((cb: (event: { exitCode: number }) => void) => {
onExitCb = cb
})
}
mockPtySpawn.mockReturnValue(term)
await expect(dispatcher.callRequest('pty.spawn', {}, { isStale: () => true })).rejects.toThrow(
@@ -713,6 +724,7 @@ describe('PtyHandler', () => {
})
it('contains repeated stale-spawn throws and retains ownership until a proven retry', async () => {
let onExitCb: ((event: { exitCode: number }) => void) | null = null
const killSpy = vi
.fn()
.mockImplementationOnce(() => {
@@ -721,8 +733,16 @@ describe('PtyHandler', () => {
.mockImplementationOnce(() => {
throw new Error('native ConPTY kill failed')
})
.mockImplementationOnce(() => onExitCb?.({ exitCode: 137 }))
const stderr = vi.spyOn(process.stderr, 'write').mockImplementation(() => true)
const term = { ...mockPtyInstance, kill: killSpy, onData: vi.fn(), onExit: vi.fn() }
const term = {
...mockPtyInstance,
kill: killSpy,
onData: vi.fn(),
onExit: vi.fn((cb: (event: { exitCode: number }) => void) => {
onExitCb = cb
})
}
mockPtySpawn.mockReturnValue(term)
try {
@@ -1118,24 +1138,24 @@ describe('PtyHandler', () => {
})
})
it('on stale-spawn cleanup without a duplicate fallback kill', async () => {
it('on stale-spawn cleanup with a signal-free proof retry', async () => {
await withWindowsPlatform(async () => {
const mockKill = mockKillablePty()
await dispatcher.callRequest('pty.spawn', {}, { isStale: () => true })
expectBareKills(mockKill, 1)
vi.advanceTimersByTime(5000)
expectBareKills(mockKill, 1)
expectBareKills(mockKill, 2)
})
})
it('on graceful shutdown without a duplicate fallback kill', async () => {
it('on graceful shutdown and its force-close fallback', async () => {
await withWindowsPlatform(async () => {
const mockKill = mockKillablePty()
await dispatcher.callRequest('pty.spawn', {})
await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: false })
expectBareKills(mockKill, 1)
vi.advanceTimersByTime(5000)
expectBareKills(mockKill, 1)
expectBareKills(mockKill, 2)
})
})
@@ -1193,7 +1213,7 @@ describe('PtyHandler', () => {
expect(mockKill).toHaveBeenCalledWith('SIGTERM')
expect(mockKill).toHaveBeenCalledWith('SIGKILL')
expect(dispatcher.notify).toHaveBeenCalledWith('pty.exit', { id: 'pty-1', code: -1 })
expect(dispatcher.notify).toHaveBeenCalledWith('pty.exit', { id: 'pty-1', code: 137 })
expect(exits).toEqual([{ id: 'pty-1', paneKey: 'tab-fallback:0' }])
expect(handler.activePtyCount).toBe(0)
})
@@ -1241,14 +1261,20 @@ describe('PtyHandler', () => {
it('keeps Windows graceful shutdown and its fallback signal-free', async () => {
const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform')
const mockKill = vi.fn()
let onExitCb: ((event: { exitCode: number }) => void) | null = null
const mockKill = vi
.fn()
.mockImplementationOnce(() => {})
.mockImplementationOnce(() => onExitCb?.({ exitCode: 137 }))
const destroy = vi.fn(() => mockKill())
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
kill: mockKill,
destroy,
onData: vi.fn(),
onExit: vi.fn()
onExit: vi.fn((cb: (event: { exitCode: number }) => void) => {
onExitCb = cb
})
})
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
@@ -1257,7 +1283,7 @@ describe('PtyHandler', () => {
await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: false })
vi.advanceTimersByTime(5000)
expect(mockKill.mock.calls).toEqual([[]])
expect(mockKill.mock.calls).toEqual([[], []])
expect(destroy).not.toHaveBeenCalled()
expect(handler.activePtyCount).toBe(0)
} finally {
@@ -1269,19 +1295,22 @@ describe('PtyHandler', () => {
it('keeps a Windows PTY retryable when immediate native shutdown throws', async () => {
const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform')
let onExitCb: ((event: { exitCode: number }) => void) | null = null
const mockKill = vi
.fn()
.mockImplementationOnce(() => {
throw new Error('invalid ConPTY handle')
})
.mockImplementationOnce(() => {})
.mockImplementationOnce(() => onExitCb?.({ exitCode: 137 }))
const destroy = vi.fn(() => mockKill())
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
kill: mockKill,
destroy,
onData: vi.fn(),
onExit: vi.fn()
onExit: vi.fn((cb: (event: { exitCode: number }) => void) => {
onExitCb = cb
})
})
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
@@ -1305,19 +1334,23 @@ describe('PtyHandler', () => {
it('does not arm Windows graceful fallback until native shutdown succeeds', async () => {
const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform')
let onExitCb: ((event: { exitCode: number }) => void) | null = null
const mockKill = vi
.fn()
.mockImplementationOnce(() => {
throw new Error('invalid ConPTY handle')
})
.mockImplementationOnce(() => {})
.mockImplementationOnce(() => onExitCb?.({ exitCode: 137 }))
const destroy = vi.fn(() => mockKill())
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
kill: mockKill,
destroy,
onData: vi.fn(),
onExit: vi.fn()
onExit: vi.fn((cb: (event: { exitCode: number }) => void) => {
onExitCb = cb
})
})
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
@@ -1331,7 +1364,7 @@ describe('PtyHandler', () => {
await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: false })
vi.advanceTimersByTime(5000)
expect(mockKill.mock.calls).toEqual([[], []])
expect(mockKill.mock.calls).toEqual([[], [], []])
expect(handler.activePtyCount).toBe(0)
} finally {
if (platformDescriptor) {
@@ -2000,6 +2033,8 @@ describe('PtyHandler', () => {
env: { ORCA_PANE_KEY: 'tab-shutdown:0' }
})
await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true })
expect(exits).toEqual([])
expect(handler.activePtyCount).toBe(1)
onExitCb!({ exitCode: 0 })
expect(mockKill).toHaveBeenCalledWith('SIGKILL')
@@ -2008,12 +2043,15 @@ describe('PtyHandler', () => {
})
it('rejects shutdown when a recycled id belongs to another pane generation', async () => {
const mockKill = vi.fn()
let onExitCb: ((event: { exitCode: number }) => void) | null = null
const mockKill = vi.fn(() => onExitCb?.({ exitCode: 137 }))
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
kill: mockKill,
onData: vi.fn(),
onExit: vi.fn()
onExit: vi.fn((cb: (event: { exitCode: number }) => void) => {
onExitCb = cb
})
})
await dispatcher.callRequest('pty.spawn', {
paneKey: 'tab-new:leaf-new',
+38 -40
View File
@@ -29,11 +29,14 @@ function killPtyForShutdown(managed: ManagedPty, signal: 'SIGTERM' | 'SIGKILL'):
// Why: node-pty's Windows backend rejects signal arguments; its no-arg kill
// is the equivalent shutdown operation for ConPTY.
if (process.platform === 'win32') {
if (!managed.windowsShutdownKillIssued) {
managed.windowsShutdownKillIssued = true
try {
managed.pty.kill()
// Why: a native kill throw can leave the process alive. Mark success only
// after return so provider ownership remains retryable on failure.
managed.windowsShutdownKillIssued = true
} catch (error) {
// Why: a native throw is not acceptance; allow the retained owner to
// retry, while the pre-call mark prevents synchronous onExit double-kill.
managed.windowsShutdownKillIssued = false
throw error
}
} else {
managed.pty.kill(signal)
@@ -486,6 +489,25 @@ export class PtyHandler {
}
}
private reapManagedPtyIfProvablyExited(managed: ManagedPty, code = -1): boolean {
if (managed.disposed || this.ptys.get(managed.id) !== managed) {
return true
}
const pid = managed.pty.pid
if (!pid || isProcessAlive(pid)) {
return false
}
// Why: node-pty can omit onExit after accepting a native kill. ESRCH is
// independent OS proof, so synthesize the same one-shot cleanup here.
this.flushPtyOutput(managed.id)
this.dispatcher.notify('pty.exit', { id: managed.id, code })
this.notifyExitListener(managed)
this.ptys.delete(managed.id)
this.clearPtyFlowState(managed.id)
disposeManagedPty(managed)
return true
}
private registerHandlers(): void {
this.dispatcher.onRequest('pty.spawn', (p, context) => this.spawn(p, context))
this.dispatcher.onRequest('pty.attach', (p) => this.attach(p))
@@ -735,6 +757,7 @@ export class PtyHandler {
managed.killTimer = setTimeout(() => {
const still = this.ptys.get(id)
if (still && !still.disposed) {
still.killTimer = undefined
try {
killPtyForShutdown(still, 'SIGKILL')
} catch (error) {
@@ -753,10 +776,12 @@ export class PtyHandler {
}
return
}
this.notifyExitListener(still)
disposeManagedPty(still)
this.ptys.delete(id)
this.clearPtyFlowState(id)
if (!this.reapManagedPtyIfProvablyExited(still)) {
fallbackAttempts += 1
if (fallbackAttempts < STALE_SPAWN_KILL_RETRY_LIMIT) {
scheduleFallback()
}
}
}
}, STALE_SPAWN_KILL_RETRY_MS)
}
@@ -891,22 +916,9 @@ export class PtyHandler {
this.releaseStartupCommand(managed)
this.flushPtyOutput(id)
killPtyForShutdown(managed, 'SIGKILL')
// Why: SIGKILL has already reaped the child; release the ptmx fd on the
// same tick. Deferring to onExit leaves a window where the fd is live
// with a dead child. Idempotent via the disposed guard — if onExit fires
// later and also calls disposeManagedPty, the second call is a no-op.
disposeManagedPty(managed)
// Why: mirror the graceful-shutdown killTimer cleanup. If SIGKILL's
// onExit never fires (kernel edge case: uninterruptible sleep,
// D-state child on a bad NFS mount), the disposed managed entry would
// linger in this.ptys forever. Each stranded entry consumes a slot in
// the 50-PTY cap and is returned by listProcesses/serialize. Deleting
// here makes the map hygiene a hard guarantee, not "hopefully onExit
// runs". If onExit DOES fire later, its own `this.ptys.delete(id)` is
// a no-op.
this.notifyExitListener(managed)
this.ptys.delete(id)
this.clearPtyFlowState(id)
// Why: kill returning is only acceptance. Preserve the PTY and listeners
// unless synchronous onExit or an ESRCH readback proves physical exit.
this.reapManagedPtyIfProvablyExited(managed)
} else {
this.releaseStartupCommand(managed)
killPtyForShutdown(managed, 'SIGTERM')
@@ -923,23 +935,9 @@ export class PtyHandler {
managed.killTimer = setTimeout(() => {
const still = this.ptys.get(id)
if (still && !still.disposed) {
still.killTimer = undefined
killPtyForShutdown(still, 'SIGKILL')
this.flushPtyOutput(id)
// Why: emit pty.exit BEFORE disposeManagedPty sets disposed=true.
// The natural onExit short-circuits on `managed.disposed`, so
// without this notify the renderer never learns the pane is dead
// when the SIGKILL fallback fires for a SIGTERM-ignoring child.
this.dispatcher.notify('pty.exit', { id, code: -1 })
// Why: if SIGKILL's onExit never fires (kernel edge case,
// uninterruptible sleep, child wedged on a bad NFS mount), the
// fd and map entry would leak forever. Dispose synchronously so
// graceful-shutdown's SIGKILL fallback is a hard guarantee, not
// "hopefully onExit will run". The disposed guard inside
// disposeManagedPty makes a later onExit's dispose a no-op.
this.notifyExitListener(still)
disposeManagedPty(still)
this.ptys.delete(id)
this.clearPtyFlowState(id)
this.reapManagedPtyIfProvablyExited(still)
}
}, 5000)
}
@@ -3512,6 +3512,120 @@ describe('removeWorktree state cleanup', () => {
expect(deleteProjectHostSetup).toHaveBeenCalledWith({ setupId: 'setup-runtime-ssh' })
})
it('purges a destroyed runtime SSH project by host when its repo id also exists locally', async () => {
const store = createTestStore()
const wt = makeWorktree({
id: 'same-repo::/path/wt1',
repoId: 'same-repo',
path: '/path/wt1',
hostId: 'local'
})
const removeProject = vi.fn().mockResolvedValue(undefined)
store.setState({
repos: [
{
id: 'same-repo',
path: '/local/repo',
displayName: 'local',
badgeColor: '#000',
addedAt: 0,
executionHostId: 'local'
},
{
id: 'same-repo',
path: '/remote/repo',
displayName: 'remote',
badgeColor: '#111',
addedAt: 1,
connectionId: 'runtime-ssh-orca-1',
executionHostId: 'ssh:runtime-ssh-orca-1'
}
],
worktreesByRepo: { 'same-repo': [wt] },
projectHostSetups: [],
removeProject
} as unknown as Partial<AppState>)
mockApi.ephemeralVm.listRuntimes.mockResolvedValueOnce([
{
id: 'runtime-1',
workspaceId: wt.id,
cleanupStatus: 'not_started',
sshTargetId: 'runtime-ssh-orca-1'
}
])
const result = await store.getState().removeWorktree(wt.id)
expect(result).toEqual({ ok: true })
expect(mockApi.ephemeralVm.cleanup).toHaveBeenCalledWith({ runtimeId: 'runtime-1' })
expect(removeProject).toHaveBeenCalledWith('same-repo', {
hostId: 'ssh:runtime-ssh-orca-1'
})
})
it('tracks setup purges by host when destroyed runtime SSH repos share an id', async () => {
const store = createTestStore()
const wt = makeWorktree({
id: 'same-repo::/path/wt1',
repoId: 'same-repo',
hostId: 'local'
})
const runtimeRepo = (targetId: string) => ({
id: 'same-repo',
path: '/remote/repo',
displayName: targetId,
badgeColor: '#000',
addedAt: 0,
connectionId: targetId,
executionHostId: `ssh:${targetId}` as const
})
const firstRepo = runtimeRepo('runtime-ssh-orca-1')
const secondRepo = runtimeRepo('runtime-ssh-orca-2')
const localRepo = {
id: 'same-repo',
path: '/local/repo',
displayName: 'local',
badgeColor: '#000',
addedAt: 0,
executionHostId: 'local' as const
}
const removeProject = vi.fn().mockResolvedValue(undefined)
const deleteProjectHostSetup = vi.fn().mockResolvedValue({ repo: firstRepo })
store.setState({
repos: [localRepo, firstRepo, secondRepo],
worktreesByRepo: { 'same-repo': [wt] },
projectHostSetups: [
{
id: 'setup-runtime-ssh-1',
hostId: 'ssh:runtime-ssh-orca-1'
} as unknown as AppState['projectHostSetups'][number]
],
removeProject,
deleteProjectHostSetup
} as unknown as Partial<AppState>)
mockApi.ephemeralVm.listRuntimes.mockResolvedValueOnce([
{
id: 'runtime-1',
workspaceId: wt.id,
cleanupStatus: 'not_started',
sshTargetId: 'runtime-ssh-orca-1'
},
{
id: 'runtime-2',
workspaceId: wt.id,
cleanupStatus: 'not_started',
sshTargetId: 'runtime-ssh-orca-2'
}
])
await store.getState().removeWorktree(wt.id)
expect(deleteProjectHostSetup).toHaveBeenCalledWith({ setupId: 'setup-runtime-ssh-1' })
expect(removeProject).toHaveBeenCalledWith('same-repo', {
hostId: 'ssh:runtime-ssh-orca-2'
})
})
it('cleans up editorDrafts for files in the removed worktree', async () => {
const store = createTestStore()
const wt = makeWorktree({ id: 'repo1::/path/wt1', repoId: 'repo1', path: '/path/wt1' })
+15 -10
View File
@@ -25,7 +25,7 @@ import {
getRepoIdFromWorktreeId,
type WorktreeSlice
} from './worktree-helpers'
import { findRepoForHost } from './repo-host-identity'
import { findRepoForHost, getRepoHostIdentity } from './repo-host-identity'
import { ensureHooksConfirmed } from '@/lib/ensure-hooks-confirmed'
import { cleanupEphemeralVmRuntimesForDeleted } from '@/lib/ephemeral-vm-runtime-cleanup'
import { tabHasLivePty } from '@/lib/tab-has-live-pty'
@@ -1276,12 +1276,12 @@ async function purgeOrphanedRuntimeSshProjects(
const orphanedSetupIds = get()
.projectHostSetups.filter((setup) => destroyedHostIds.has(setup.hostId))
.map((setup) => setup.id)
const purgedRepoIds = new Set<string>()
const purgedRepoIdentities = new Set<string>()
for (const setupId of orphanedSetupIds) {
try {
const result = await get().deleteProjectHostSetup({ setupId })
if (result?.repo) {
purgedRepoIds.add(result.repo.id)
purgedRepoIdentities.add(getRepoHostIdentity(result.repo))
}
} catch (error) {
console.error('Failed to purge orphaned per-workspace-env project:', error)
@@ -1289,14 +1289,19 @@ async function purgeOrphanedRuntimeSshProjects(
}
// A repo whose only host was the destroyed runtime can outlive its setup (the setup may be pruned
// by a projection refresh first). Remove it directly so no dead, never-connectable project lingers.
const orphanedRepoIds = get()
.repos.filter(
(repo) => destroyedTargetIds.has(repo.connectionId ?? '') && !purgedRepoIds.has(repo.id)
)
.map((repo) => repo.id)
for (const repoId of orphanedRepoIds) {
const orphanedRepos = get()
.repos.filter((repo) => {
return (
destroyedTargetIds.has(repo.connectionId ?? '') &&
!purgedRepoIdentities.has(getRepoHostIdentity(repo))
)
})
.map((repo) => ({ repoId: repo.id, hostId: getRepoExecutionHostId(repo) }))
for (const { repoId, hostId } of orphanedRepos) {
try {
await get().removeProject(repoId)
// Why: repo IDs are only host-local; an equal local ID must survive the
// runtime-owned SSH target cleanup.
await get().removeProject(repoId, { hostId })
} catch (error) {
console.error('Failed to purge orphaned per-workspace-env repo:', error)
}