fix(terminal): keep descendant teardown fresh and identity-safe

This commit is contained in:
Brennan Benson
2026-07-14 23:10:04 -07:00
parent d50cbeb7fe
commit f7fcac15ae
10 changed files with 122 additions and 60 deletions
+19 -8
View File
@@ -711,13 +711,13 @@
"ssh",
"runtime"
],
"coverageNotes": "Live macOS Electron tests prove exact local PTY disappearance after parked-tab close and detached-pgid descendant death after agent close. Deterministic tests cover daemon and SSH routing, local/daemon pending-snapshot ownership across natural exit, graceful-to-immediate kill upgrades, duplicate-kill completion sharing, locale-stable bounded/fresh/coalesced process-table reads, concurrent bulk teardown, source-scan timestamp identity, same-second PID ambiguity, ordinary runtime close ownership, unified-only hydration, split ownership, pane detach transfer, restart alias hydration, and late-hook suppression; live Linux, Windows, WSL, SSH, and remote-runtime process evidence remains pending.",
"coverageNotes": "Live macOS Electron tests prove exact local PTY disappearance after parked-tab close and detached-pgid descendant death after agent close. Deterministic tests cover daemon and SSH routing, local/daemon pending-snapshot ownership across natural exit, stale-root descendant-signal suppression, graceful-to-immediate kill upgrades, duplicate-kill completion sharing, locale-stable bounded/fresh/coalesced process-table reads, deadline-safe successor scans, concurrent bulk teardown, source-scan timestamp identity, same-second PID ambiguity, ordinary runtime close ownership, unified-only hydration, split ownership, pane detach transfer, restart alias hydration, and late-hook suppression; live Linux, Windows, WSL, SSH, and remote-runtime process evidence remains pending.",
"motivatingLinks": [
"https://github.com/stablyai/orca/pull/8628",
"https://github.com/stablyai/orca/pull/8706"
],
"invariant": "Close permanently removes the owned provider session, agent descendants, and resume authority even when no TerminalPane is mounted; a terminating id remains reserved through natural exit, duplicate callers await the same completion, and immediate teardown upgrades any graceful request without signalling a recycled PID; process-table work is locale-stable, bounded, fresh for each post-start request, and same-turn coalesced, including bulk worktree cleanup; detach and park preserve ownership; aliases prevent a detached agent's immutable physical pane key from being retired with its former tab.",
"oracle": "Capture the exact PTY before parking, prove it remains listed while the view is absent, close through the product state boundary, and poll the provider inventory until that exact ID disappears; an agent-marked PTY's detached-pgid child is alive before close and absent afterward; unit tests keep a naturally exited id reserved without re-killing its PID, upgrade pending and post-snapshot graceful kills to immediate, force ps into the C locale, begin bulk shutdowns together, share duplicate teardown completion, coalesce 20 same-turn process-table requests, force a successor scan for later arrivals, retain the source scan's timestamp, bound both read phases, avoid ambiguous SIGKILL, and assert canonical owner dedupe, exact pane tombstones, chained detach transfer, and restart alias restoration.",
"invariant": "Close permanently removes the owned provider session, agent descendants, and resume authority even when no TerminalPane is mounted; a terminating id remains reserved through natural exit, duplicate callers await the same completion, and immediate teardown upgrades any graceful request without signalling a recycled PID or a descendant tree after root ownership is lost; process-table work is locale-stable, bounded, fresh for each post-start request, same-turn coalesced, and begins within the requesting caller's deadline, including bulk worktree cleanup; detach and park preserve ownership; aliases prevent a detached agent's immutable physical pane key from being retired with its former tab.",
"oracle": "Capture the exact PTY before parking, prove it remains listed while the view is absent, close through the product state boundary, and poll the provider inventory until that exact ID disappears; an agent-marked PTY's detached-pgid child is alive before close and absent afterward; unit tests keep a naturally exited id reserved without re-killing its PID or signalling its captured tree, upgrade pending and post-snapshot graceful kills to immediate, force ps into the C locale, begin bulk shutdowns together, share duplicate teardown completion, coalesce 20 same-turn process-table requests, start one shared successor without waiting for the prior scan, retain the source scan's timestamp, bound both read phases, avoid ambiguous SIGKILL, and assert canonical owner dedupe, exact pane tombstones, chained detach transfer, and restart alias restoration.",
"commands": [
"pnpm dlx node@24 ./node_modules/vitest/vitest.mjs run --config config/vitest.config.ts src/main/agent-hooks/server-pane-authority.test.ts src/main/ipc/agent-hooks.test.ts src/main/ipc/agent-pane-authority-ownership.test.ts src/main/ipc/pty-management.test.ts src/main/persistence.test.ts src/renderer/src/store/slices/agent-pane-authority.test.ts src/renderer/src/store/slices/terminal-pane-detach-agent-identity.test.ts src/renderer/src/store/slices/terminal-tab-retirement.test.ts src/renderer/src/store/slices/terminal-tab-retirement-store.test.ts src/renderer/src/components/shared/kill-all-terminal-surfaces.test.ts",
"pnpm exec vitest run --config config/vitest.config.ts src/main/pty-descendant-termination.test.ts src/main/daemon/session.test.ts src/main/daemon/terminal-host.test.ts src/main/providers/local-pty-provider.test.ts src/main/runtime/worktree-teardown.test.ts",
@@ -767,10 +767,11 @@
{
"file": "src/main/pty-descendant-termination.test.ts",
"assertions": [
"20 same-turn process-table requests execute one fresh scan while later arrivals receive a successor scan",
"20 same-turn process-table requests execute one fresh scan while later arrivals start one shared successor inside their own deadline",
"snapshot and escalation readers stop at their deadline",
"production ps reads force locale-independent C timestamps",
"the source scan timestamp survives request resolution and capture-second identities are never escalated with SIGKILL"
"the source scan timestamp survives request resolution and capture-second identities are never escalated with SIGKILL",
"descendant signals are suppressed after the caller loses root ownership"
]
},
{
@@ -804,11 +805,20 @@
"result": "passed",
"durationSeconds": 40.3,
"summary": "A fresh E2E build launched an isolated Electron profile, parked a live terminal, closed it through closeTab, and observed its exact PTY disappear."
},
{
"date": "2026-07-14",
"runner": "local",
"platform": "macos",
"command": "pnpm run test:e2e -- tests/e2e/agent-descendant-process-kill.spec.ts --workers=1",
"result": "passed",
"durationSeconds": 63.5,
"summary": "A fresh-build run proved a detached-pgid child was alive before agent PTY kill and absent afterward on the deadline-safe, root-ownership-gated implementation."
}
],
"runtimeBudget": {
"p95Seconds": 60,
"scope": "fresh E2E build plus one isolated local Electron parked-close test"
"scope": "fresh E2E build plus isolated local Electron parked-close and descendant-kill tests"
},
"flakeHistory": {
"status": "unknown",
@@ -820,7 +830,7 @@
},
"performanceBudget": {
"required": true,
"evidence": "The close path is user-triggered and bounded by canonical live-owner indexing. Production ps has a 1s kill timeout; 20 same-turn requests execute one fresh process-table read, while a request arriving after a scan starts queues one successor rather than consuming stale signal targets. Completed tables are never reused. Bulk worktree shutdown begins owned sessions concurrently so the production coordinator can coalesce the initial scan instead of serializing one subprocess per session. Escalation uses the same bounded coordinator, and kill-all store scale remains covered by terminal-session.kill-all-surface-cleanup."
"evidence": "The close path is user-triggered and bounded by canonical live-owner indexing. Production ps has a 1s kill timeout; 20 same-turn requests execute one fresh process-table read, while requests arriving after a scan starts immediately share one successor so their deadline is not consumed waiting and no unusable scan starts after timeout. Completed tables are never reused. Bulk worktree shutdown begins owned sessions concurrently so the production coordinator can coalesce the initial scan instead of serializing one subprocess per session. Escalation uses the same bounded coordinator, and kill-all store scale remains covered by terminal-session.kill-all-surface-cleanup."
},
"promotionCriteria": [
"Accumulate 100 clean runs or 14 days on required CI platforms.",
@@ -832,7 +842,8 @@
"Disconnected SSH relay death still requires reconnect-aware provider ownership.",
"Daemon owner leases and durable retry inventory remain follow-up hardening.",
"Windows ConPTY, SSH-hosted PTYs, app-quit killAll, and daemon dispose retain foreground-tree-only teardown.",
"A process born in the capture second is SIGTERMed but not SIGKILLed because ps cannot prove its recycled-PID identity."
"A process born in the capture second is SIGTERMed but not SIGKILLed because ps cannot prove its recycled-PID identity.",
"A descendant orphaned before or during root ownership loss requires the separate crash-orphan sweep and is not recovered from a stale kill-time snapshot."
],
"demotionRule": "Keep experimental or demote to protection none if exact PTY disappearance flakes, a sibling/detached pane is retired, or late hooks can recreate closed authority."
},
+7 -5
View File
@@ -81,11 +81,13 @@ could invalidate intentional long-lived `worktree-sleep` checkpoints.
reach — via `pty-descendant-termination.ts` (bounded fresh snapshot with
same-turn coalescing, SIGTERM, grace window, then identity-safe SIGKILL).
Completed process tables are never reused as signal targets, and identity
checks use C-locale timestamps from the source scan. A session is marked as
terminating before capture and keeps teardown ownership through natural exit,
so reattach, duplicate kill, and graceful-to-immediate upgrade cannot race the
snapshot. Windows and SSH-hosted PTYs keep the previous foreground-tree
contract for now.
checks use C-locale timestamps from the source scan. Later requests start a
fresh same-turn-coalesced successor inside their own deadline instead of
waiting behind older scans. A session is marked as terminating before capture
and keeps request ownership through natural exit, so reattach, duplicate kill,
and graceful-to-immediate upgrade cannot race the snapshot; descendant signals
still require the exact root session/handle to be live. Windows and SSH-hosted
PTYs keep the previous foreground-tree contract for now.
- Changing agent-provider resume commands or permission flags.
- Making a UI close wait for a remote process to exit before the tab disappears.
- Replacing worktree sleep with tab close. Sleep remains resumable by design.
+5 -1
View File
@@ -425,7 +425,11 @@ describe('Session', () => {
it('agent kill routes through the descendant sweep with the subprocess as root', () => {
createSession({ launchAgent: 'claude' })
session.kill()
expect(killWithDescendantSweepMock).toHaveBeenCalledWith(subprocess.pid, expect.any(Function))
expect(killWithDescendantSweepMock).toHaveBeenCalledWith(
subprocess.pid,
expect.any(Function),
expect.objectContaining({ ownsRoot: expect.any(Function) })
)
// The root kill is deferred to the sweep's snapshot-first sequencing.
expect(subprocess.killed).toBe(false)
const killRoot = killWithDescendantSweepMock.mock.calls[0][1] as () => void
+11 -3
View File
@@ -283,9 +283,17 @@ export class Session {
// shell's SIGHUP never reaches. The bounded snapshot briefly defers the
// signal; the kill timer below starts now, so force-dispose timing is
// unaffected.
void killWithDescendantSweep(this.subprocess.pid, () => {
this.signalTerminationRoot()
})
void killWithDescendantSweep(
this.subprocess.pid,
() => {
this.signalTerminationRoot()
},
{
// Why: if the root exits during ps, its numeric PID can be recycled.
// Never apply that stale snapshot to a different process tree.
ownsRoot: () => this.isAlive
}
)
}
this.scheduleForceDisposeFallback()
}
+5 -1
View File
@@ -427,7 +427,11 @@ describe('TerminalHost', () => {
// Why order matters: force-killing first would let orphans reparent to
// pid 1 and escape the sweep's ppid walk entirely.
expect(killWithDescendantSweepMock).toHaveBeenCalledWith(99999, expect.any(Function))
expect(killWithDescendantSweepMock).toHaveBeenCalledWith(
99999,
expect.any(Function),
expect.objectContaining({ ownsRoot: expect.any(Function) })
)
expect(lastSubprocess.forceKill).not.toHaveBeenCalled()
expect(host.isKilled('agent-1')).toBe(true)
+19 -11
View File
@@ -69,18 +69,26 @@ export class TerminalSessionTeardown {
immediate
}
const operation = 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
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
}
if (entry.immediate) {
this.finishImmediate(sessionId, session)
} 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
}
if (entry.immediate) {
this.finishImmediate(sessionId, session)
} else {
session.signalTerminationRoot()
}
})
)
)
entry.promise = operation
this.operations.set(sessionId, entry)
@@ -1059,6 +1059,31 @@ describe('LocalPtyProvider', () => {
await Promise.all([first, second])
expect(captureDescendantSnapshotMock).toHaveBeenCalledOnce()
})
it('does not signal a captured tree after the tracked root exits naturally', async () => {
let resolveSnapshot!: (value: {
rootPgid: number
descendants: []
capturedAtMs: number
}) => void
captureDescendantSnapshotMock.mockReturnValue(
new Promise((resolve) => {
resolveSnapshot = resolve
})
)
const { id } = await provider.spawn({
cols: 80,
rows: 24,
launchAgent: 'claude'
})
const shutdown = provider.shutdown(id, { immediate: true })
exitCb?.({ exitCode: 0 })
resolveSnapshot({ rootPgid: mockProc.pid, descendants: [], capturedAtMs: Date.now() })
await shutdown
expect(terminateDescendantSnapshotMock).not.toHaveBeenCalled()
})
})
describe('hasChildProcesses', () => {
+2 -9
View File
@@ -936,16 +936,14 @@ export class LocalPtyProvider implements IPtyProvider {
const descendants = ptyAgentSessionIds.has(id)
? await captureDescendantSnapshot(proc.pid)
: null
let descendantsSignalled = false
// Why the handle re-check: the snapshot is this method's only await, and a
// natural exit (or same-id respawn) may have raced it. That path already
// ran clearPtyState and notified exit — only the descendant sweep remains.
// natural exit may have raced it. Signalling after ownership is lost could
// apply the old numeric PID's snapshot to a recycled, unrelated process.
if (ptyProcesses.get(id) === proc) {
if (descendants) {
// Signal captured children before killing the root so parent links do
// not disappear during the sweep.
terminateDescendantSnapshot(descendants)
descendantsSignalled = true
}
// Why: disposePtyListeners removes the onExit callback, so the natural
// exit cleanup path from node-pty won't fire. Cleanup and notification
@@ -966,11 +964,6 @@ export class LocalPtyProvider implements IPtyProvider {
cb({ id, code: -1 })
}
}
if (descendants && !descendantsSignalled) {
// Natural exit may have won while the snapshot was in flight. The
// captured descendants still belong to that retired process identity.
terminateDescendantSnapshot(descendants)
}
}
async sendSignal(id: string, signal: string): Promise<void> {
+22 -1
View File
@@ -288,9 +288,12 @@ describe('createProcessTableSnapshotReader', () => {
const laterA = readTable()
const laterB = readTable()
await Promise.resolve()
// A successor must begin inside its callers' deadline. Waiting for the
// prior scan can make both callers time out before their own scan starts.
expect(readFresh).toHaveBeenCalledTimes(2)
firstGate.resolve(tableCapture([row(10, 1, 10)], CAPTURED_AT_MS))
await first
await vi.waitFor(() => expect(readFresh).toHaveBeenCalledTimes(2))
const newer = tableCapture([row(20, 1, 20)], CAPTURED_AT_MS + 1_000)
secondGate.resolve(newer)
@@ -349,4 +352,22 @@ describe('killWithDescendantSweep', () => {
expect(killRoot).toHaveBeenCalledOnce()
expect(sendSignal).not.toHaveBeenCalled()
})
it('does not signal a captured tree after the caller loses root ownership', async () => {
const sendSignal = vi.fn()
const killRoot = vi.fn()
const readTable = vi.fn().mockResolvedValue(tableCapture([row(10, 1, 10), row(20, 10, 20)]))
const ownsRoot = vi.fn(() => false)
await killWithDescendantSweep(10, killRoot, {
readTable,
sendSignal,
platform: 'darwin',
ownsRoot
})
expect(ownsRoot).toHaveBeenCalledOnce()
expect(sendSignal).not.toHaveBeenCalled()
expect(killRoot).toHaveBeenCalledOnce()
})
})
+7 -21
View File
@@ -85,7 +85,6 @@ function readFreshProcessTable(
export function createProcessTableSnapshotReader(
readFresh: ProcessTableReader
): ProcessTableReader {
let inFlight: Promise<ProcessTableCapture> | null = null
let queued: { promise: Promise<ProcessTableCapture>; started: boolean } | null = null
return (timeoutMs) => {
@@ -93,29 +92,16 @@ export function createProcessTableSnapshotReader(
return queued.promise
}
const prior = queued?.promise ?? inFlight
const entry: { promise: Promise<ProcessTableCapture>; started: boolean } = {
promise: Promise.resolve(undefined as never),
started: false
}
entry.promise = Promise.resolve().then(async () => {
if (prior) {
try {
await prior
} catch {
// The post-request scan below owns this caller's result.
}
}
entry.promise = Promise.resolve().then(() => {
// Why: a later caller's deadline starts when it requests a fresh table.
// Waiting behind an older scan can consume that entire budget, then run
// this subprocess after nobody can use its result.
entry.started = true
const request = readFresh(timeoutMs)
inFlight = request
try {
return await request
} finally {
if (inFlight === request) {
inFlight = null
}
}
return readFresh(timeoutMs)
})
queued = entry
const clearQueued = (): void => {
@@ -229,13 +215,13 @@ export async function captureDescendantSnapshot(
export async function killWithDescendantSweep(
rootPid: number,
killRoot: () => void,
deps: SnapshotDeps & TerminateDeps = {}
deps: SnapshotDeps & TerminateDeps & { ownsRoot?: () => boolean } = {}
): Promise<void> {
const snapshot = await captureDescendantSnapshot(rootPid, deps)
try {
// Signal the captured descendants while their parent links still exist;
// killing the root first creates a reparent/PID-reuse window.
if (snapshot) {
if (snapshot && (deps.ownsRoot?.() ?? true)) {
terminateDescendantSnapshot(snapshot, deps)
}
} finally {