fix(daemon): let a create wait out an in-flight session teardown (#18063)

Co-authored-by: Orca Worker <orca-worker@localhost>
This commit is contained in:
OrcaWin
2026-09-01 21:36:12 -07:00
committed by GitHub
co-authored by Orca Worker
parent f801a8447a
commit ededec00ba
8 changed files with 327 additions and 94 deletions
@@ -0,0 +1,20 @@
import { TerminalAttachCanceledError } from './daemon-errors'
/** Never resolves; only rejects, so it can bound a wait without settling it. */
export function rejectOnAbort(
signal: AbortSignal | undefined,
sessionId: string
): Promise<never> {
if (!signal) {
return new Promise<never>(() => {})
}
return new Promise<never>((_resolve, reject) => {
if (signal.aborted) {
reject(new TerminalAttachCanceledError(sessionId))
return
}
signal.addEventListener('abort', () => reject(new TerminalAttachCanceledError(sessionId)), {
once: true
})
})
}
@@ -12,11 +12,14 @@ import type { TerminalHostTombstones } from './terminal-host-tombstones'
import type { TerminalSessionTeardown } from './terminal-session-teardown'
import { resolveDaemonSessionScrollbackRows } from './daemon-session-scrollback-window'
import { TerminalAttachCanceledError } from './daemon-errors'
import { rejectOnAbort } from './terminal-attach-cancellation'
import { SessionNotFoundError } from './types'
import { resolveWslSessionContext } from './wsl-session-context'
type TerminalHostSessionCreateDependencies = {
sessions: Map<string, Session>
/** Re-checks the host's shutdown fence and this request's cancellation after any await. */
assertCreateAllowed: () => void
sessionTeardown: TerminalSessionTeardown
killedTombstones: TerminalHostTombstones
spawnSubprocess: TerminalHostOptions['spawnSubprocess']
@@ -31,12 +34,29 @@ export async function createOrAttachTerminalSession(
deps: TerminalHostSessionCreateDependencies
): Promise<CreateOrAttachResult> {
opts.onSessionResolved?.(opts.sessionId)
const existing = deps.sessions.get(opts.sessionId)
let existing = deps.sessions.get(opts.sessionId)
// Why: descendant capture must finish before attach or recreation, or the
// caller could receive a doomed session while teardown owns its process.
if (deps.sessionTeardown.get(opts.sessionId) || existing?.isTerminating) {
throw new SessionNotFoundError(opts.sessionId)
// An attach must not adopt a doomed session; its caller retires the pane and respawns.
if (opts.attachOnly) {
throw new SessionNotFoundError(opts.sessionId)
}
// A create can wait teardown out instead, and must: a pane respawning onto its own stable id
// reaches this a beat after the attach that retired it, and refusing surfaced the raw
// SessionNotFoundError to the user. Windows makes it the common case, where the plain-shell
// sweep holds the claim across an OS identity probe and taskkill (#18046).
await Promise.race([
deps.sessionTeardown.settle(opts.sessionId),
rejectOnAbort(opts.cancelSignal, opts.sessionId)
])
deps.assertCreateAllowed()
existing = deps.sessions.get(opts.sessionId)
// Unkillable child, or a fresh teardown claimed it while we waited: still nobody's to recreate.
if (existing?.isAlive && existing.isTerminating) {
throw new SessionNotFoundError(opts.sessionId)
}
}
// Why no ownership settle here: attach is synchronous by contract. A viewer
@@ -0,0 +1,151 @@
import { describe, expect, it, vi, type Mock } from 'vitest'
import type { SubprocessHandle } from './session-subprocess-handle'
import { TerminalHost, type TerminalHostOptions } from './terminal-host'
// Why mocked: the win32 plain-shell teardown sweeps for real, and an unmocked run would put a
// live process-table probe -- and, on a recycled pid, a taskkill /T /F -- behind these tests.
const killWithDescendantSweepMock = vi.hoisted(() => vi.fn())
vi.mock('../pty-descendant-termination', () => ({
killWithDescendantSweep: killWithDescendantSweepMock
}))
type SpawnSubprocess = TerminalHostOptions['spawnSubprocess']
type ExitableSubprocess = SubprocessHandle & { exit: (code: number) => void }
/** Shells that report their exit only after `exitDelayMs`, holding the teardown claim open the
* way a real one does while the Windows sweep probes and taskkills its tree. Collected so a test
* can retire an intentionally unkillable child instead of leaking its exit waiter. */
function spawnSubprocessWithSlowExit(exitDelayMs: number): {
spawnSubprocess: Mock<SpawnSubprocess>
handles: ExitableSubprocess[]
} {
const handles: ExitableSubprocess[] = []
const spawnSubprocess = vi.fn<SpawnSubprocess>(() => {
let onExit: ((code: number) => void) | undefined
const handle = {
pid: 4242,
exit: (code: number) => onExit?.(code),
getForegroundProcess: vi.fn(() => null),
write: vi.fn(),
resize: vi.fn(),
kill: vi.fn(() => {
setTimeout(() => onExit?.(0), exitDelayMs).unref?.()
}),
terminateOwnedTree: () => 'unavailable' as const,
forceKill: vi.fn(() => {
setTimeout(() => onExit?.(137), exitDelayMs).unref?.()
}),
signal: vi.fn(),
onData: vi.fn(),
onExit: vi.fn((callback) => {
onExit = callback
}),
dispose: vi.fn()
} as unknown as ExitableSubprocess
handles.push(handle)
return handle
})
return { spawnSubprocess, handles }
}
const streamClient = (): { onData: Mock; onExit: Mock } => ({
onData: vi.fn(),
onExit: vi.fn()
})
describe('TerminalHost recreate during teardown', () => {
it('recreates a session whose id is still being torn down', async () => {
const { spawnSubprocess } = spawnSubprocessWithSlowExit(40)
const host = new TerminalHost({ spawnSubprocess })
const sessionId = 'wt-1@@respawning-pane'
await host.createOrAttach({ sessionId, cols: 80, rows: 24, streamClient: streamClient() })
// The pane closes and immediately respawns onto its own stable id (#18046).
const killed = host.kill(sessionId, { immediate: true })
const recreated = await host.createOrAttach({
sessionId,
cols: 80,
rows: 24,
streamClient: streamClient()
})
expect(recreated.isNew).toBe(true)
expect(spawnSubprocess).toHaveBeenCalledTimes(2)
await killed
await host.dispose()
})
it('still refuses an attach-only respawn onto a session being torn down', async () => {
const { spawnSubprocess } = spawnSubprocessWithSlowExit(40)
const host = new TerminalHost({ spawnSubprocess })
const sessionId = 'wt-1@@attaching-pane'
await host.createOrAttach({ sessionId, cols: 80, rows: 24, streamClient: streamClient() })
const killed = host.kill(sessionId, { immediate: true })
// Why unchanged: adopting a doomed session would hand the pane a shell teardown owns; the
// caller retires the pane binding on this error and spawns fresh.
await expect(
host.createOrAttach({
sessionId,
cols: 80,
rows: 24,
attachOnly: true,
streamClient: streamClient()
})
).rejects.toThrow(`Session not found: ${sessionId}`)
expect(spawnSubprocess).toHaveBeenCalledOnce()
await killed
await host.dispose()
})
it('refuses a create waiting on teardown once the host is shutting down', async () => {
const { spawnSubprocess } = spawnSubprocessWithSlowExit(40)
const host = new TerminalHost({ spawnSubprocess })
const sessionId = 'wt-1@@shutting-down-pane'
await host.createOrAttach({ sessionId, cols: 80, rows: 24, streamClient: streamClient() })
const killed = host.kill(sessionId, { immediate: true })
const create = host.createOrAttach({
sessionId,
cols: 80,
rows: 24,
streamClient: streamClient()
})
// Why: dispose joins pending creations, so a create that waited out teardown must re-read the
// fence rather than publish a session nothing will shut down.
const disposed = host.dispose()
await expect(create).rejects.toThrow('Terminal host is shutting down')
expect(spawnSubprocess).toHaveBeenCalledOnce()
await killed
await disposed
})
it('leaves a canceled create waiting on teardown instead of the full exit budget', async () => {
// Why a child that never exits on its own: the create must leave on its abort signal, not on
// the teardown settling, so the teardown deliberately outlives the assertion.
const { spawnSubprocess, handles } = spawnSubprocessWithSlowExit(30_000)
const host = new TerminalHost({ spawnSubprocess })
const sessionId = 'wt-1@@canceled-pane'
await host.createOrAttach({ sessionId, cols: 80, rows: 24, streamClient: streamClient() })
const killed = host.kill(sessionId, { immediate: true })
const canceled = new AbortController()
const create = host.createOrAttach({
sessionId,
cols: 80,
rows: 24,
cancelSignal: canceled.signal,
isCanceled: () => canceled.signal.aborted,
streamClient: streamClient()
})
canceled.abort()
await expect(create).rejects.toThrow(`Attach canceled for session ${sessionId}`)
expect(spawnSubprocess).toHaveBeenCalledOnce()
handles[0].exit(137)
await killed
await host.dispose()
})
})
+45 -27
View File
@@ -473,14 +473,17 @@ describe('TerminalHost', () => {
expect(lastSubprocess.forceKill).toHaveBeenCalledTimes(1)
expect(lastSubprocess.dispose).not.toHaveBeenCalled()
expect(host.listSessions()).toHaveLength(1)
await expect(
host.createOrAttach({
sessionId: 'session-1',
cols: 80,
rows: 24,
streamClient: { onData: vi.fn(), onExit: vi.fn() }
})
).rejects.toThrow('Session not found')
// An unkillable child never releases the id: the create waits out its own budget and
// then reports absence rather than publishing a session teardown still owns.
const recreate = host.createOrAttach({
sessionId: 'session-1',
cols: 80,
rows: 24,
streamClient: { onData: vi.fn(), onExit: vi.fn() }
})
const refused = expect(recreate).rejects.toThrow('Session not found')
await vi.advanceTimersByTimeAsync(IMMEDIATE_KILL_PHYSICAL_EXIT_TIMEOUT_MS)
await refused
lastSubprocess._onExitCb?.(137)
expect(host.listSessions()).toHaveLength(0)
@@ -525,7 +528,7 @@ describe('TerminalHost', () => {
expect(lastSubprocess.dispose).toHaveBeenCalled()
})
it('rejects reattach while an agent immediate-kill snapshot is pending', async () => {
it('defers a respawn until the agent immediate-kill snapshot completes', async () => {
let finishSweep!: () => void
killWithDescendantSweepMock.mockImplementation(
(_pid: number, finish: () => void) =>
@@ -544,22 +547,35 @@ describe('TerminalHost', () => {
streamClient: { onData: vi.fn(), onExit: vi.fn() }
})
const retiredSubprocess = lastSubprocess
const killing = host.kill('agent-reattach', { immediate: true })
await expect(
host.createOrAttach({
let respawned = false
const respawn = host
.createOrAttach({
sessionId: 'agent-reattach',
cols: 80,
rows: 24,
launchAgent: 'claude',
streamClient: { onData: vi.fn(), onExit: vi.fn() }
})
).rejects.toThrow('Session not found')
expect(lastSubprocess.forceKill).not.toHaveBeenCalled()
.then((result) => {
respawned = true
return result
})
await Promise.resolve()
await Promise.resolve()
// Why it must not resolve yet: capture still owns the process, so publishing here would
// hand the caller a session teardown is about to kill.
expect(respawned).toBe(false)
expect(spawnFn).toHaveBeenCalledTimes(1)
expect(retiredSubprocess.forceKill).not.toHaveBeenCalled()
finishSweep()
lastSubprocess._onExitCb?.(137)
retiredSubprocess._onExitCb?.(137)
await killing
expect(lastSubprocess.forceKill).toHaveBeenCalledOnce()
await expect(respawn).resolves.toMatchObject({ isNew: true })
expect(retiredSubprocess.forceKill).toHaveBeenCalledOnce()
})
it('coalesces duplicate immediate kill while descendant capture is pending', async () => {
@@ -606,29 +622,31 @@ describe('TerminalHost', () => {
const killing = host.kill('agent-natural-exit', { immediate: true })
retiredSubprocess._onExitCb?.(0)
await expect(
host.createOrAttach({
let respawned = false
const respawn = host
.createOrAttach({
sessionId: 'agent-natural-exit',
cols: 80,
rows: 24,
launchAgent: 'claude',
streamClient: { onData: vi.fn(), onExit: vi.fn() }
})
).rejects.toThrow('Session not found')
.then((result) => {
respawned = true
return result
})
await Promise.resolve()
await Promise.resolve()
// The root is already reaped, but the scan still holds the id.
expect(respawned).toBe(false)
expect(spawnFn).toHaveBeenCalledTimes(1)
completeSweep()
await killing
expect(retiredSubprocess.forceKill).not.toHaveBeenCalled()
await expect(
host.createOrAttach({
sessionId: 'agent-natural-exit',
cols: 80,
rows: 24,
launchAgent: 'claude',
streamClient: { onData: vi.fn(), onExit: vi.fn() }
})
).resolves.toEqual(expect.objectContaining({ isNew: true }))
await expect(respawn).resolves.toEqual(expect.objectContaining({ isNew: true }))
expect(spawnFn).toHaveBeenCalledTimes(2)
})
+2 -15
View File
@@ -21,24 +21,10 @@ import { listLiveTerminalHostSessions } from './terminal-host-session-listing'
import { createOrAttachTerminalSession } from './terminal-host-session-create'
import { isShellProcess } from '../../shared/agent-detection'
import { TerminalAttachCanceledError } from './daemon-errors'
import { rejectOnAbort } from './terminal-attach-cancellation'
export type { CreateOrAttachOptions, CreateOrAttachResult } from './terminal-host-create-contract'
/** Never resolves; only rejects, so it can bound a wait without settling it. */
function rejectOnAbort(signal: AbortSignal | undefined, sessionId: string): Promise<never> {
if (!signal) {
return new Promise<never>(() => {})
}
return new Promise<never>((_resolve, reject) => {
if (signal.aborted) {
reject(new TerminalAttachCanceledError(sessionId))
return
}
signal.addEventListener('abort', () => reject(new TerminalAttachCanceledError(sessionId)), {
once: true
})
})
}
export type { TerminalHostOptions } from './terminal-host-options'
const DEFAULT_MAX_TOMBSTONES = 1000
@@ -106,6 +92,7 @@ export class TerminalHost {
}
return await createOrAttachTerminalSession(options, {
sessions: this.sessions,
assertCreateAllowed: () => this.assertCreateOrAttachAllowed(options),
sessionTeardown: this.sessionTeardown,
killedTombstones: this.killedTombstones,
spawnSubprocess: this.spawnSubprocess,
+69 -46
View File
@@ -1,7 +1,7 @@
import { killWithDescendantSweep } from '../pty-descendant-termination'
import type { Session } from './session'
type AgentTeardownOperation = {
type TeardownOperation = {
promise: Promise<void>
immediate: boolean
rootSignalled: boolean
@@ -9,10 +9,10 @@ type AgentTeardownOperation = {
session: Session
}
/** Owns agent teardown by session id until descendant capture and root
* signalling finish, even when the root exits and its Session is reaped. */
/** Owns teardown by session id until descendant capture and root signalling
* finish, even when the root exits and its Session is reaped. */
export class TerminalSessionTeardown {
private operations = new Map<string, AgentTeardownOperation>()
private operations = new Map<string, TeardownOperation>()
constructor(private sessions: ReadonlyMap<string, Session>) {}
@@ -20,6 +20,12 @@ export class TerminalSessionTeardown {
return this.operations.get(sessionId)?.promise
}
/** Resolves once this id's tracked teardown has released the process — a rejected teardown
* released it too. Callers re-read session state afterwards and decide for themselves. */
async settle(sessionId: string): Promise<void> {
await this.operations.get(sessionId)?.promise.catch(() => {})
}
requestImmediate(sessionId: string): Promise<void> | undefined {
const pending = this.operations.get(sessionId)
if (pending) {
@@ -38,11 +44,42 @@ export class TerminalSessionTeardown {
return this.killAgentSession(sessionId, session, immediate)
}
if (immediate) {
return this.forceKillPlainShellSession(sessionId, session)
// Why tracked like the agent path: this claims termination on the Session and then awaits
// an OS probe and taskkill, and a create landing inside that window must be able to wait it
// out rather than be told the id is absent (#18046).
return this.track(sessionId, session, immediate, () =>
this.forceKillPlainShellSession(sessionId, session)
)
}
session.kill()
}
/** Publishes an operation for `sessionId` and retires it once the teardown settles. */
private track(
sessionId: string,
session: Session,
immediate: boolean,
run: (entry: TeardownOperation) => Promise<void>
): Promise<void> {
const entry: TeardownOperation = {
promise: Promise.resolve(),
immediate,
rootSignalled: false,
rootCompletion: Promise.resolve(),
session
}
const operation = run(entry)
entry.promise = operation
this.operations.set(sessionId, entry)
const clearOperation = (): void => {
if (this.operations.get(sessionId) === entry) {
this.operations.delete(sessionId)
}
}
void operation.then(clearOperation, clearOperation)
return operation
}
/**
* Immediate teardown of a non-agent shell. On Windows, closing the ConPTY does not
* reap orphaned children (node-pty `useConptyDll` skips the console-process reap), so a
@@ -92,48 +129,34 @@ export class TerminalSessionTeardown {
session.scheduleForceDisposeFallback()
}
const entry: AgentTeardownOperation = {
promise: Promise.resolve(),
immediate,
rootSignalled: false,
rootCompletion: Promise.resolve(),
session
}
const sweep = Promise.resolve(
killWithDescendantSweep(
session.pid,
() => {
// Why: natural exit reaps the PID while ps is running. Never signal that
// stale numeric PID after the Session no longer represents a live root.
if (!session.isAlive) {
return
return this.track(sessionId, session, immediate, (entry) => {
const sweep = Promise.resolve(
killWithDescendantSweep(
session.pid,
() => {
// Why: natural exit reaps the PID while ps is running. Never signal that
// stale numeric PID after the Session no longer represents a live root.
if (!session.isAlive) {
return
}
entry.rootSignalled = true
if (entry.immediate) {
entry.rootCompletion = session.forceKillAndWaitForExit()
} else {
session.signalTerminationRoot()
}
},
{
// Why: the descendant rows are only authoritative while this exact
// Session still owns the root PID captured by ps.
ownsRoot: () => this.sessions.get(sessionId) === session && session.isAlive,
terminateOwnedTree: () => session.terminateOwnedTree()
}
entry.rootSignalled = true
if (entry.immediate) {
entry.rootCompletion = session.forceKillAndWaitForExit()
} else {
session.signalTerminationRoot()
}
},
{
// Why: the descendant rows are only authoritative while this exact
// Session still owns the root PID captured by ps.
ownsRoot: () => this.sessions.get(sessionId) === session && session.isAlive,
terminateOwnedTree: () => session.terminateOwnedTree()
}
)
)
)
// Why: descendant capture completion only proves signals were requested;
// destructive callers must retain the native owner until OS-confirmed exit.
const operation = sweep.then(() => entry.rootCompletion)
entry.promise = operation
this.operations.set(sessionId, entry)
const clearOperation = (): void => {
if (this.operations.get(sessionId) === entry) {
this.operations.delete(sessionId)
}
}
void operation.then(clearOperation, clearOperation)
return operation
// Why: descendant capture completion only proves signals were requested;
// destructive callers must retain the native owner until OS-confirmed exit.
return sweep.then(() => entry.rootCompletion)
})
}
}
@@ -160,6 +160,18 @@ describe('humanizeTerminalError', () => {
expect(humanized).toContain('Open a new terminal to continue')
})
// A daemon generation old enough to still refuse a pane respawning onto an id it is tearing
// down answers with the raw class name; the user must not be told to file an issue for it.
it('replaces the daemon session-absence string and its id', () => {
const humanized = humanizeTerminalError(
"Error invoking remote method 'pty:spawn': SessionNotFoundError: Session not found: wt-1@@pane-a"
)
expect(humanized).not.toContain('SessionNotFoundError')
expect(humanized).not.toContain('wt-1@@pane-a')
expect(humanized).toContain('Open a new terminal to continue')
expect(isExplainedTerminalError('Session not found: wt-1@@pane-a')).toBe(true)
})
it('replaces the identity-mismatch form of PTY-not-found', () => {
const humanized = humanizeTerminalError('PTY "orca:2f1c@@pty-7" not found (identity mismatch)')
expect(humanized).not.toContain('identity mismatch')
@@ -26,12 +26,14 @@ const TERMINAL_HOST_GONE_PATTERN = new RegExp(TERMINAL_HOST_GONE_SOURCE)
const TERMINAL_HOST_GONE_REPLACE_PATTERN = new RegExp(TERMINAL_HOST_GONE_SOURCE, 'g')
const LEGACY_TERMINAL_HOST_GONE_PATTERN =
/(^|[^a-z])connect (?:ENOENT|ECONNREFUSED) [^\r\n]*orca-terminal-host-v[^\r\n]*/i
// A reattach the host answered "no such session" for: the SSH provider's expiry token, or the relay's
// raw not-found string when nothing mapped it. Both carry an internal PTY id, and neither is proof the
// remote shell died — the copy says only that this pane lost its session. Same lastIndex hazard as above.
// A reattach the host answered "no such session" for: the SSH provider's expiry token, the relay's
// raw not-found string when nothing mapped it, or a daemon generation old enough to still refuse a
// pane respawning onto an id it is tearing down (#18046). None proves the shell died — the copy
// says only that this pane lost its session. Same lastIndex hazard as above.
const UNREATTACHABLE_SESSION_SOURCES = [
'SSH_SESSION_EXPIRED:[ \\t]*\\S*(?:[ \\t]+SSH_PTY_IDENTITY_MISMATCH)?',
'PTY "[^"\\r\\n]*" not found(?: \\(identity mismatch\\))?'
'PTY "[^"\\r\\n]*" not found(?: \\(identity mismatch\\))?',
'(?:SessionNotFoundError: )?Session not found: \\S+'
]
const UNREATTACHABLE_SESSION_PATTERNS = UNREATTACHABLE_SESSION_SOURCES.map(
(source) => new RegExp(source)