From a34fab2e9ebdec345930a072b02ececfc1f9e53c Mon Sep 17 00:00:00 2001 From: Neil Date: Mon, 14 Sep 2026 08:51:03 -0700 Subject: [PATCH] fix(relay): sweep detached tools on immediate terminal close --- src/relay/mock-descendant-sweep.ts | 8 + src/relay/pty-handler-attach-replay.test.ts | 1 + .../pty-handler-dispose-lifecycle.test.ts | 1 + src/relay/pty-handler-grace-timer.test.ts | 1 + .../pty-handler-immediate-descendants.test.ts | 282 ++++++++++++++++++ ...handler-inventory-process-evidence.test.ts | 1 + ...-handler-output-drain-differential.test.ts | 1 + .../pty-handler-output-streaming.test.ts | 1 + .../pty-handler-ownership-attestation.test.ts | 1 + .../pty-handler-resize-stale-pty.test.ts | 1 + .../pty-handler-retired-pane-surface.test.ts | 1 + src/relay/pty-handler-revive.test.ts | 1 + .../pty-handler-shell-resolution.test.ts | 1 + .../pty-handler-shutdown-signals.test.ts | 1 + .../pty-handler-source-publication.test.ts | 1 + src/relay/pty-handler-spawn-admission.test.ts | 1 + src/relay/pty-handler-spawn-cwd.test.ts | 1 + .../pty-handler-spawn-environment.test.ts | 1 + ...y-handler-startup-command-delivery.test.ts | 1 + ...ler-windows-child-process-evidence.test.ts | 1 + src/relay/pty-handler.ts | 52 +++- src/relay/relay-daemon-fatal-reap.test.ts | 1 + tests/tools/omp-relay-close-lifecycle.md | 67 +++++ .../tools/omp-relay-close-lifecycle.test.mjs | 143 +++++++++ 24 files changed, 568 insertions(+), 3 deletions(-) create mode 100644 src/relay/mock-descendant-sweep.ts create mode 100644 src/relay/pty-handler-immediate-descendants.test.ts create mode 100644 tests/tools/omp-relay-close-lifecycle.md create mode 100644 tests/tools/omp-relay-close-lifecycle.test.mjs diff --git a/src/relay/mock-descendant-sweep.ts b/src/relay/mock-descendant-sweep.ts new file mode 100644 index 00000000000..8af046417f9 --- /dev/null +++ b/src/relay/mock-descendant-sweep.ts @@ -0,0 +1,8 @@ +import { vi } from 'vitest' + +// Mock PTYs reuse the runner PID; never enumerate or signal its real descendants. +vi.mock('../main/pty-descendant-termination', () => ({ + killWithDescendantSweep: async (_pid: number, killRoot: () => void): Promise => { + killRoot() + } +})) diff --git a/src/relay/pty-handler-attach-replay.test.ts b/src/relay/pty-handler-attach-replay.test.ts index e289547bf0c..5ec55a6eb2a 100644 --- a/src/relay/pty-handler-attach-replay.test.ts +++ b/src/relay/pty-handler-attach-replay.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' import * as ptyShellUtils from './pty-shell-utils' import { diff --git a/src/relay/pty-handler-dispose-lifecycle.test.ts b/src/relay/pty-handler-dispose-lifecycle.test.ts index 0553f92a4e5..631834ab1b7 100644 --- a/src/relay/pty-handler-dispose-lifecycle.test.ts +++ b/src/relay/pty-handler-dispose-lifecycle.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' const { mockPtySpawn, mockPtyInstance, mockCreateShellPromptReadinessProbe } = vi.hoisted(() => ({ diff --git a/src/relay/pty-handler-grace-timer.test.ts b/src/relay/pty-handler-grace-timer.test.ts index c1797583c14..ed552e71551 100644 --- a/src/relay/pty-handler-grace-timer.test.ts +++ b/src/relay/pty-handler-grace-timer.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' import { DEFAULT_BOUNDED_SSH_RELAY_GRACE_PERIOD_SECONDS } from '../shared/ssh-types' diff --git a/src/relay/pty-handler-immediate-descendants.test.ts b/src/relay/pty-handler-immediate-descendants.test.ts new file mode 100644 index 00000000000..1362052834a --- /dev/null +++ b/src/relay/pty-handler-immediate-descendants.test.ts @@ -0,0 +1,282 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { beginPtyHandlerTest, endPtyHandlerTest } from './pty-handler-test-harness' +import type { MockDispatcher } from './pty-handler-test-harness' +import type { PtyHandler } from './pty-handler' +import type { RelayPtySourcePublication } from './relay-pty-source-publication' + +const { mockPtySpawn, mockPtyInstance, mockCreateShellPromptReadinessProbe, sweep } = vi.hoisted( + () => ({ + mockPtySpawn: vi.fn(), + mockCreateShellPromptReadinessProbe: vi.fn(), + sweep: + vi.fn< + (pid: number, killRoot: () => void, deps?: { ownsRoot?: () => boolean }) => Promise + >(), + mockPtyInstance: { + pid: process.pid, + onData: vi.fn(), + onExit: vi.fn(), + write: vi.fn(), + resize: vi.fn(), + kill: vi.fn(), + clear: vi.fn(), + pause: vi.fn(), + resume: vi.fn() + } + }) +) +vi.mock('node-pty', () => ({ spawn: mockPtySpawn })) +vi.mock('../main/pty-descendant-termination', () => ({ killWithDescendantSweep: sweep })) +vi.mock('../main/pty/posix-pty-process-groups', () => ({ + forceKillPosixPtyProcessGroups: (_pid: number, kill: () => void) => kill() +})) +vi.mock('../main/shell-prompt-readiness-probe', () => ({ + createShellPromptReadinessProbe: mockCreateShellPromptReadinessProbe +})) + +const ensure = { + claim: { + digestVersion: 1, + keyId: 'claim-key', + identityDigest: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', + worktreeScopeDigest: 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb', + agent: 'omp' + }, + surface: { + worktreeId: 'repo::/tmp/worktree', + tabId: '11111111-1111-4111-8111-111111111111', + leafId: '22222222-2222-4222-8222-222222222222', + terminalHandle: 'term_omp' + } +} + +describe('relay immediate descendant cleanup', () => { + let dispatcher: MockDispatcher + let handler: PtyHandler + let originalPlatform: PropertyDescriptor | undefined + let exit: ((event: { exitCode: number }) => void) | undefined + let release: (() => void) | undefined + let kill: ReturnType + + beforeEach(() => { + ;({ dispatcher, handler, originalPlatform } = beginPtyHandlerTest({ + mockPtySpawn, + mockPtyInstance, + mockCreateShellPromptReadinessProbe + })) + exit = undefined + release = undefined + kill = vi.fn() + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill, + onExit: (callback: (event: { exitCode: number }) => void) => { + exit = callback + } + }) + sweep.mockReset() + sweep.mockImplementation( + (_pid, killRoot) => + new Promise((resolve, reject) => { + release = () => { + try { + killRoot() + resolve() + } catch (error) { + reject(error) + } + } + }) + ) + }) + afterEach(async () => { + release?.() + exit?.({ exitCode: 137 }) + await endPtyHandlerTest(handler, originalPlatform) + }) + + async function spawn(params: Record = {}) { + const result = await dispatcher.callRequest('pty.spawn', params) + if ( + !result || + typeof result !== 'object' || + !('id' in result) || + typeof result.id !== 'string' + ) { + throw new Error('missing PTY id') + } + return result.id + } + const close = (id: string) => dispatcher.callRequest('pty.shutdown', { id, immediate: true }) + + it('sweeps a typed agent before force-kill and joins close through physical exit', async () => { + const id = await spawn() + const first = close(id) + const second = close(id) + expect(sweep).toHaveBeenCalledTimes(1) + expect(kill).not.toHaveBeenCalled() + await expect(dispatcher.callRequest('pty.attach', { id })).rejects.toThrow('terminating') + release?.() + await vi.waitFor(() => expect(kill).toHaveBeenCalledWith('SIGKILL')) + expect(handler.activePtyCount).toBe(1) + exit?.({ exitCode: 137 }) + await Promise.all([first, second]) + expect(handler.activePtyCount).toBe(0) + expect(kill).toHaveBeenCalledTimes(1) + }) + + it('does not signal a root that exits while its snapshot is pending', async () => { + const id = await spawn() + const closing = close(id) + const ownsRoot = sweep.mock.calls[0]?.[2]?.ownsRoot + expect(ownsRoot?.()).toBe(true) + exit?.({ exitCode: 0 }) + expect(ownsRoot?.()).toBe(false) + release?.() + await closing + expect(kill).not.toHaveBeenCalled() + }) + + it('retains the agent claim instead of adopting or duplicating a closing owner', async () => { + const id = await spawn({ agentSessionEnsure: ensure }) + const closing = close(id) + await expect(spawn({ agentSessionEnsure: ensure })).rejects.toThrow('terminating') + expect(mockPtySpawn).toHaveBeenCalledTimes(1) + release?.() + await vi.waitFor(() => expect(kill).toHaveBeenCalledTimes(1)) + exit?.({ exitCode: 137 }) + await closing + }) + + it('does not replay a completed create operation while its owner is closing', async () => { + const params = { + agentSessionEnsure: ensure, + agentSessionCreateOperationId: 'ccccccccccccccccccccccccccccccccccccccccccc' + } + const id = await spawn(params) + const closing = close(id) + await expect(spawn(params)).rejects.toThrow('terminating') + expect(mockPtySpawn).toHaveBeenCalledTimes(1) + release?.() + await vi.waitFor(() => expect(kill).toHaveBeenCalledTimes(1)) + exit?.({ exitCode: 137 }) + await closing + }) + + it('allows retry after a failed root signal without releasing the live PTY', async () => { + const id = await spawn() + kill.mockImplementationOnce(() => { + throw new Error('signal refused') + }) + const rejected = expect(close(id)).rejects.toThrow('signal refused') + release?.() + await rejected + expect(handler.activePtyCount).toBe(1) + const retry = close(id) + expect(sweep).toHaveBeenCalledTimes(2) + release?.() + await vi.waitFor(() => expect(kill).toHaveBeenCalledTimes(2)) + exit?.({ exitCode: 137 }) + await retry + }) + + it('keeps the Windows force-kill path and fences attachment until physical exit', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const id = await spawn() + const closing = close(id) + expect(sweep).not.toHaveBeenCalled() + expect(kill).toHaveBeenCalledWith() + await expect(dispatcher.callRequest('pty.attach', { id })).rejects.toThrow('terminating') + exit?.({ exitCode: 137 }) + await closing + }) + + it('keeps graceful shell shutdown off the descendant sweep', async () => { + const id = await spawn() + await dispatcher.callRequest('pty.shutdown', { id, immediate: false }) + expect(sweep).not.toHaveBeenCalled() + expect(kill).toHaveBeenCalledWith('SIGTERM') + }) + it('refuses attach after close completes during source checkpoint wait', async () => { + const id = await spawn() + let finishSource!: (ready: boolean) => void + const sourceWait = new Promise((resolve) => { + finishSource = resolve + }) + const activate = vi.fn(() => false) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: This stub supplies every publication method exercised by the handler. + handler.setSourcePublication({ + accepts: () => false, + exitPublicationSettled: () => true, + sealAndPublishExit: () => false, + publish: () => false, + onCreditAvailable: () => {}, + receivingActivation: () => undefined, + waitForPendingSend: () => sourceWait, + activate, + getDebugSnapshot: () => ({}), + dispose: () => {} + } as unknown as RelayPtySourcePublication) + const attaching = dispatcher.callRequest('pty.attach', { + id, + sourceRecovery: { + status: 'checkpoint', + deliveryToken: 'token', + ptyIncarnation: 'incarnation', + clientGeneration: 1, + ownerGeneration: 1, + acceptedSourceEndSu: 0 + } + }) + const closing = close(id) + release?.() + await vi.waitFor(() => expect(kill).toHaveBeenCalledTimes(1)) + exit?.({ exitCode: 137 }) + await closing + expect(handler.activePtyCount).toBe(0) + finishSource(true) + await expect(attaching).rejects.toThrow() + expect(activate).not.toHaveBeenCalled() + }) + + it('retains claim if close starts before initial claim liveness validation', async () => { + let closing: Promise | undefined + let closeId = '' + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: This stub supplies every publication method exercised by the handler. + handler.setSourcePublication({ + accepts: () => false, + exitPublicationSettled: () => true, + sealAndPublishExit: () => false, + publish: () => false, + onCreditAvailable: () => {}, + receivingActivation: () => undefined, + waitForPendingSend: async () => true, + activate: (id: string) => { + if (!closeId) { + closeId = id + queueMicrotask(() => { + closing = close(id) + void closing.catch(() => {}) + }) + } + return false + }, + getDebugSnapshot: () => ({}), + dispose: () => {} + } as unknown as RelayPtySourcePublication) + await expect(spawn({ agentSessionEnsure: ensure })).rejects.toThrow('terminating') + expect(handler.activePtyCount).toBe(1) + const firstExit = exit + const retried = spawn({ agentSessionEnsure: ensure }) + const outcome = await retried.then( + () => 'created', + () => 'rejected' + ) + const spawnCount = mockPtySpawn.mock.calls.length + release?.() + firstExit?.({ exitCode: 137 }) + await closing + expect(outcome).toBe('rejected') + expect(spawnCount).toBe(1) + }) +}) diff --git a/src/relay/pty-handler-inventory-process-evidence.test.ts b/src/relay/pty-handler-inventory-process-evidence.test.ts index c12e6da62b4..b2e89d56e99 100644 --- a/src/relay/pty-handler-inventory-process-evidence.test.ts +++ b/src/relay/pty-handler-inventory-process-evidence.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' // Regression guard for the SHIPPED inventory path. `pty.listProcesses` resolves // every managed pane's title from one batched host capture; a per-pane tree walk // would restore the O(panes x rows) scan on the relay's single event-loop thread, diff --git a/src/relay/pty-handler-output-drain-differential.test.ts b/src/relay/pty-handler-output-drain-differential.test.ts index cbaee4bcb75..d6c7e92470f 100644 --- a/src/relay/pty-handler-output-drain-differential.test.ts +++ b/src/relay/pty-handler-output-drain-differential.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' const { mockPtySpawn, mockPtyInstance } = vi.hoisted(() => ({ diff --git a/src/relay/pty-handler-output-streaming.test.ts b/src/relay/pty-handler-output-streaming.test.ts index 3be79255fcc..af0a4cbdfaa 100644 --- a/src/relay/pty-handler-output-streaming.test.ts +++ b/src/relay/pty-handler-output-streaming.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' import { PTY_STARTUP_INGRESS_VERSION } from '../shared/pty-startup-ingress' diff --git a/src/relay/pty-handler-ownership-attestation.test.ts b/src/relay/pty-handler-ownership-attestation.test.ts index ee1c144df09..513917ef76c 100644 --- a/src/relay/pty-handler-ownership-attestation.test.ts +++ b/src/relay/pty-handler-ownership-attestation.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' // The host half of #9819: a client may only reap a relay PTY it can prove it created, so the relay // has to say who created each one. The attestation is read from the live consumer grant, never from // a spawn parameter — otherwise it would just echo the caller's claim back at it. diff --git a/src/relay/pty-handler-resize-stale-pty.test.ts b/src/relay/pty-handler-resize-stale-pty.test.ts index 0dcb6b56497..d3478bc6825 100644 --- a/src/relay/pty-handler-resize-stale-pty.test.ts +++ b/src/relay/pty-handler-resize-stale-pty.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const { mockPtySpawn, mockPtyInstance, mockCreateShellPromptReadinessProbe } = vi.hoisted(() => ({ diff --git a/src/relay/pty-handler-retired-pane-surface.test.ts b/src/relay/pty-handler-retired-pane-surface.test.ts index 959c6f06330..c8f50ebf13a 100644 --- a/src/relay/pty-handler-retired-pane-surface.test.ts +++ b/src/relay/pty-handler-retired-pane-surface.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' const { mockPtySpawn, mockPtyInstance, mockCreateShellPromptReadinessProbe } = vi.hoisted(() => ({ diff --git a/src/relay/pty-handler-revive.test.ts b/src/relay/pty-handler-revive.test.ts index e184cd56817..57ecb42e360 100644 --- a/src/relay/pty-handler-revive.test.ts +++ b/src/relay/pty-handler-revive.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' import { existsSync, rmSync } from 'node:fs' import { homedir, tmpdir } from 'node:os' diff --git a/src/relay/pty-handler-shell-resolution.test.ts b/src/relay/pty-handler-shell-resolution.test.ts index 9abd9b90d10..679c2babab7 100644 --- a/src/relay/pty-handler-shell-resolution.test.ts +++ b/src/relay/pty-handler-shell-resolution.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' import * as gitBash from '../main/git-bash' import * as ptyShellUtils from './pty-shell-utils' diff --git a/src/relay/pty-handler-shutdown-signals.test.ts b/src/relay/pty-handler-shutdown-signals.test.ts index 951ce7209e0..5bc3373599c 100644 --- a/src/relay/pty-handler-shutdown-signals.test.ts +++ b/src/relay/pty-handler-shutdown-signals.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' const { mockPtySpawn, mockPtyInstance, mockCreateShellPromptReadinessProbe } = vi.hoisted(() => ({ diff --git a/src/relay/pty-handler-source-publication.test.ts b/src/relay/pty-handler-source-publication.test.ts index 71e8c12b48f..341931cffb5 100644 --- a/src/relay/pty-handler-source-publication.test.ts +++ b/src/relay/pty-handler-source-publication.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { PTY_STARTUP_INGRESS_VERSION } from '../shared/pty-startup-ingress' import { diff --git a/src/relay/pty-handler-spawn-admission.test.ts b/src/relay/pty-handler-spawn-admission.test.ts index 6fef02a9cc0..ea5c6ca3486 100644 --- a/src/relay/pty-handler-spawn-admission.test.ts +++ b/src/relay/pty-handler-spawn-admission.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' import { mkdtempSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' diff --git a/src/relay/pty-handler-spawn-cwd.test.ts b/src/relay/pty-handler-spawn-cwd.test.ts index 2aab95a401e..fd2c64849e4 100644 --- a/src/relay/pty-handler-spawn-cwd.test.ts +++ b/src/relay/pty-handler-spawn-cwd.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' import { mkdtempSync, mkdirSync, rmSync } from 'node:fs' import { homedir, tmpdir } from 'node:os' diff --git a/src/relay/pty-handler-spawn-environment.test.ts b/src/relay/pty-handler-spawn-environment.test.ts index 8879686821e..020b1256cc3 100644 --- a/src/relay/pty-handler-spawn-environment.test.ts +++ b/src/relay/pty-handler-spawn-environment.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs' import { homedir, tmpdir } from 'node:os' diff --git a/src/relay/pty-handler-startup-command-delivery.test.ts b/src/relay/pty-handler-startup-command-delivery.test.ts index 9e7c29202c7..2215eff9d76 100644 --- a/src/relay/pty-handler-startup-command-delivery.test.ts +++ b/src/relay/pty-handler-startup-command-delivery.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' import { mkdtempSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' diff --git a/src/relay/pty-handler-windows-child-process-evidence.test.ts b/src/relay/pty-handler-windows-child-process-evidence.test.ts index 6d723824a04..75f7382df8d 100644 --- a/src/relay/pty-handler-windows-child-process-evidence.test.ts +++ b/src/relay/pty-handler-windows-child-process-evidence.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' // Regression guard for the Windows SSH child-process answer. The relay used to return a hardcoded // `false` here, which every close guard reads as "nothing is running in this pane" -- so a Windows // SSH pane running a build closed with no prompt. The answer now comes from the process table, and diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index cdf436bca2a..5ab847d209f 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -1,5 +1,6 @@ /* oxlint-disable max-lines */ import type { IPty } from 'node-pty' +import { killWithDescendantSweep } from '../main/pty-descendant-termination' import type * as NodePty from 'node-pty' import { existsSync } from 'node:fs' import { basename, join } from 'node:path' @@ -236,6 +237,7 @@ type ManagedPty = { * spawn reply to skip waiting for a marker that will never come (fish, sh, Windows). */ shellReadyArmed?: boolean physicalExit?: PhysicalExitTracker + immediateClose?: Promise forceKillSent?: boolean gracefulKillSent?: boolean startupIngress?: PtyStartupIngress @@ -1685,6 +1687,7 @@ export class PtyHandler { const existing = this.agentSessionCreateOperations.get(operationId) if (existing) { const result = await existing + this.assertPtyNotClosing(this.ptys.get(result.id)) this.sourcePublication?.activate(result.id, result.incarnationId, context) const sourceActivation = context && this.sourcePublication?.receivingActivation?.(result.id, context.clientId) @@ -1799,6 +1802,7 @@ export class PtyHandler { this.agentSessionOwners.release(result.owner.ptyId, result.owner.generation) throw new Error('agent_session_exited_during_start') } + this.assertPtyNotClosing(managed) managed.agentSessionOwners = this.agentSessionOwners.listForPty(managed.id) const adoptedReplay = result.disposition === 'adopted' ? managed.buffered.read() : '' this.sourcePublication?.activate(managed.id, managed.incarnationId, context) @@ -2070,6 +2074,8 @@ export class PtyHandler { throw new Error(`PTY "${id}" not found`) } + this.assertPtyNotClosing(managed) + // Why: verify liveness because shells can exit without node-pty onExit. if (this.reapPtyProvenExited(managed)) { // Why the marker: this is the ONLY not-found answer backed by a liveness check. The unmarked @@ -2108,6 +2114,10 @@ export class PtyHandler { ) { sourceRecovery = Object.freeze({ status: 'checkpointUnavailable' }) } + if (this.ptys.get(id) !== managed || managed.disposed) { + throw new Error(`PTY "${id}" not found`) + } + this.assertPtyNotClosing(managed) const activation = this.sourcePublication?.activate( id, managed.incarnationId, @@ -2282,15 +2292,51 @@ export class PtyHandler { if (immediate) { this.releaseStartupCommand(managed) this.flushPtyOutput(id) - this.requestForceKill(managed) - // Why: preserve timed-out entries so onExit/retry owns native handles. - await this.waitForPhysicalExit(managed, IMMEDIATE_PTY_EXIT_TIMEOUT_MS) + await this.closeImmediately(managed) } else { this.releaseStartupCommand(managed) this.requestGracefulKill(managed, 'force-kill') } } + private assertPtyNotClosing(managed: ManagedPty | undefined): void { + if (managed?.immediateClose) { + throw new Error(`PTY "${managed.id}" is terminating`) + } + } + + private async closeImmediately(managed: ManagedPty): Promise { + if (managed.immediateClose) { + return managed.immediateClose + } + const ownsRoot = (): boolean => this.ptys.get(managed.id) === managed && !managed.disposed + const close = async (): Promise => { + if (process.platform === 'win32') { + this.requestForceKill(managed) + } else { + await killWithDescendantSweep( + managed.pty.pid, + () => { + if (ownsRoot()) { + this.requestForceKill(managed) + } + }, + { ownsRoot, terminateOwnedTree: () => terminatePtyJob(managed.pty) } + ) + } + await this.waitForPhysicalExit(managed, IMMEDIATE_PTY_EXIT_TIMEOUT_MS) + } + const pending = close() + managed.immediateClose = pending + try { + await pending + } finally { + if (managed.immediateClose === pending) { + managed.immediateClose = undefined + } + } + } + /** Re-decide, on the host, whether the caller may destroy this PTY. * * `pty.shutdown` is irreversible and its siblings `pty.spawn`/`pty.attach` already take a diff --git a/src/relay/relay-daemon-fatal-reap.test.ts b/src/relay/relay-daemon-fatal-reap.test.ts index 071185fc311..9e94af4c78c 100644 --- a/src/relay/relay-daemon-fatal-reap.test.ts +++ b/src/relay/relay-daemon-fatal-reap.test.ts @@ -1,3 +1,4 @@ +import './mock-descendant-sweep' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { PtyHandler } from './pty-handler' diff --git a/tests/tools/omp-relay-close-lifecycle.md b/tests/tools/omp-relay-close-lifecycle.md new file mode 100644 index 00000000000..d0ef5052ba3 --- /dev/null +++ b/tests/tools/omp-relay-close-lifecycle.md @@ -0,0 +1,67 @@ +# OMP relay-host immediate-close probe (#9530) + +This opt-in probe uses a real installed OMP binary and native PTYs behind production +`PtyHandler` spawn/data/shutdown handlers. The dispatcher is an in-process test +transport; no SSH connection or rendered client is exercised. OMP source is read-only. + +```sh +ORCA_BACKGROUND_LAUNCH=1 ORCA_OMP_PROBE_BINARY=/absolute/path/to/omp \ + ORCA_OMP_PROBE_SHELL=/bin/bash \ + node node_modules/vitest/vitest.mjs run --config config/vitest.config.ts \ + tests/tools/omp-relay-close-lifecycle.test.mjs +``` + +The test defaults to zsh on macOS and bash on Linux. Windows is skipped. It needs +existing native node-pty dependencies; do not install or rebuild as part of the probe. +HOME, user profile, XDG roots and OMP/PI agent roots are disposable, profiles cleared, +and zsh inheritance fenced to the disposable root. No model request is made. The +probe runs `! /bin/sleep 120`, records exact shell/OMP/tool process rows, requests +immediate close, and observes those PIDs independently of the relay inventory. +Matching PID/start-time/group identities bound leftover cleanup. Reports and capped +terminal transcripts stay in `.bench-fixtures/omp-relay-close-*`. + +## Measured on macOS with OMP 18.1.18 + +At source base `93c370246388`, bash mode leaves sleep PID 43156, PGID 43156, alive +and reparented to PID 1 after root PID 42902 and OMP PID 42949 exit. The zsh control +exits cleanly: OMP uses a headless PTY for zsh/fish user-shell tools, while bash +uses its embedded-shell subprocess path. Thus an external command alone does not +determine the process lifetime; the configured user shell matters. + +With the correction, the same bash probe leaves none of its captured PIDs present. +This is detached-tool leakage, not proof of the original foreground-OMP-survives +report. The local-provider/daemon correction is PR #20642; this probe and correction +cover the separate direct-relay backend. + +## Reliability contract + +- Invariant: `terminal-session.explicit-close-retirement`. Explicit immediate close + captures still-parented detached descendants before root termination, preserves + the exact host owner through physical exit, and cannot attach/adopt that owner + while the close is pending. A concurrent close joins the same operation. +- Failure source/oracle: actual OMP external sleep survives the bash-mode relay + close before the fix; independently queried owned PIDs are absent afterward. + Unit tests also cover pending attachment/adoption/create replay, natural exit + during capture, signal failure/retry, retained claims during initial promotion, + and close completing while attachment awaits a source checkpoint. +- Gate: the existing experimental explicit-close gate's descendant/backend tests, + relay lifecycle suites and this opt-in real-PTY probe. Live SSH transport and + rendered client flows remain explicit validation gaps. +- Budget: one existing bounded process-table capture (one-second timeout, 32-MiB + cap), plus one bounded identity recheck after the two-second grace when there + are descendants. Same-turn captures coalesce; no recurring polling is added. +- Authority: the execution host does all process inspection/signaling. Pending-close + refusal carries no proven-exited marker; it is not evidence of process death. + No new RPC fields/opcodes or required capabilities. Older clients receive an + ordinary failed attach while close is pending, not a successful doomed attachment. +- Scope: every immediate POSIX relay close, including still-parented intentionally + detached jobs. Graceful close, disconnect grace, keep-alive and fatal-exit/dispose + policies are unchanged. Windows retains its immediate force-kill path and now + rejects attachment during the physical-exit wait. Folder workspaces and worktrees + use the same PTY identity, without repository metadata checks. +- Gaps: macOS runtime evidence only; Linux/Windows/WSL runtime, live SSH/mobile and + mixed-version clients are not exercised. Children reparented before capture and + same-second identity ambiguity retain the incumbent cleanup limitations. + +Mock-PTY suites isolate the sweep: their fake PIDs often equal the test runner's PID +and must never reach the real host process table or descendant signals. diff --git a/tests/tools/omp-relay-close-lifecycle.test.mjs b/tests/tools/omp-relay-close-lifecycle.test.mjs new file mode 100644 index 00000000000..4c5cc6d230e --- /dev/null +++ b/tests/tools/omp-relay-close-lifecycle.test.mjs @@ -0,0 +1,143 @@ +import { it, expect } from 'vitest' +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { + createMockDispatcher, + createTestPtyHandler +} from '../../src/relay/pty-handler-test-harness.ts' +import { + captureDescendantSnapshot, + readProcessTable +} from '../../src/main/pty-descendant-termination.ts' +import { runProcess } from '../../src/shared/child-process/run-process.ts' + +const binary = process.env.ORCA_OMP_PROBE_BINARY +const pause = (ms) => new Promise((resolve) => setTimeout(resolve, ms)) +const quote = (value) => `'${value.replaceAll("'", "'\\''")}'` + +it.skipIf(!binary || process.platform === 'win32')( + 'closes actual OMP detached tools through the relay host', + async () => { + const fixtures = join(process.cwd(), '.bench-fixtures') + mkdirSync(fixtures, { recursive: true }) + const output = mkdtempSync(join(fixtures, 'omp-relay-close-')) + const home = mkdtempSync(join(tmpdir(), 'orca-omp-relay-close-home-')) + const agentHome = join(home, 'agent') + mkdirSync(agentHome) + const config = join(home, 'probe.yml') + writeFileSync( + config, + 'startup:\n setupWizard: false\n showSplash: false\n checkUpdate: false\n' + ) + const dispatcher = createMockDispatcher() + let transcript = '' + dispatcher.notify = (method, params) => { + if (method === 'pty.data' && typeof params?.data === 'string') { + transcript = (transcript + params.data).slice(-131072) + } + } + const handler = createTestPtyHandler(dispatcher) + let snapshot + let id + try { + const spawned = await dispatcher.callRequest('pty.spawn', { + cwd: home, + cols: 120, + rows: 35, + env: { + HOME: home, + USERPROFILE: home, + ZDOTDIR: home, + ORCA_ORIG_ZDOTDIR: home, + SHELL: + process.env.ORCA_OMP_PROBE_SHELL ?? + (process.platform === 'darwin' ? '/bin/zsh' : '/bin/bash'), + XDG_CONFIG_HOME: join(home, 'config'), + XDG_DATA_HOME: join(home, 'data'), + XDG_CACHE_HOME: join(home, 'cache'), + XDG_STATE_HOME: join(home, 'state'), + OMP_CODING_AGENT_DIR: agentHome, + PI_CODING_AGENT_DIR: agentHome, + OMP_PROFILE: '', + PI_PROFILE: '', + PI_CONFIG_DIR: '.omp', + PI_CONFIG_FILES: '', + ORCA_BACKGROUND_LAUNCH: '1', + ORCA_PANE_KEY: 'omp-relay-probe:owned-leaf', + ORCA_TAB_ID: 'omp-relay-probe' + }, + envToDelete: ['BASH_ENV', 'ENV', 'ORCA_OMP_STATUS_EXTENSION', 'ORCA_PI_STATUS_EXTENSION'] + }) + id = spawned.id + const [entry] = JSON.parse(await dispatcher.callRequest('pty.serialize', { ids: [id] })) + snapshot = await captureDescendantSnapshot(entry.pid) + expect(snapshot?.root?.pid).toBe(entry.pid) + dispatcher.callNotification('pty.data', { + id, + data: `${quote(binary)} --no-session --config ${quote(config)}\r` + }) + await pause(5000) + dispatcher.callNotification('pty.data', { id, data: '! /bin/sleep 120\r' }) + for (let attempt = 0; attempt < 25; attempt++) { + await pause(200) + snapshot = await captureDescendantSnapshot(entry.pid) + if (snapshot?.descendants.length > 1) { + break + } + } + expect(snapshot?.descendants.length).toBeGreaterThan(1) + const pids = [entry.pid, ...snapshot.descendants.map((row) => row.pid)] + const rows = async () => + ( + await runProcess({ + program: 'ps', + args: ['-p', pids.join(','), '-o', 'pid=,ppid=,pgid=,stat=,comm='], + maxOutputBytes: 16000 + }) + ).stdout.trim() + const before = await rows() + expect(before).toContain('omp') + expect(before).toContain('sleep') + await dispatcher.callRequest('pty.shutdown', { + id, + immediate: true, + expectedIncarnationId: spawned.incarnationId + }) + await pause(6000) + const after = await rows() + writeFileSync( + join(output, 'report.json'), + JSON.stringify({ backend: 'relay-host', before, after, pid: entry.pid, id, home }, null, 2) + ) + writeFileSync(join(output, 'transcript.txt'), transcript) + console.log(output) + expect(after).toBe('') + } finally { + if (snapshot) { + const current = await readProcessTable() + const owned = [ + ...snapshot.descendants, + ...(snapshot.root ? [{ ...snapshot.root, pgid: snapshot.rootPgid }] : []) + ] + for (const row of current.rows) { + if ( + owned.some( + (known) => + known.pid === row.pid && + known.startedAt === row.startedAt && + known.pgid === row.pgid + ) + ) { + try { + process.kill(row.pid, 'SIGKILL') + } catch {} + } + } + } + await handler.dispose({ waitForPhysicalExit: false }) + rmSync(home, { recursive: true, force: true }) + } + }, + 45000 +)