From ca272a78c08d5a8bfc482dc6ec14995056dc8a58 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 27 Oct 2026 18:36:30 -0700 Subject: [PATCH] fix(daemon): read the PTY process table without blocking on terminal kill and resize (#34603) * fix(daemon): read the PTY process table without blocking on terminal kill and resize The terminal daemon force-killed a session by running ps and reading /proc//stat synchronously, and checked the resize foreground group with execFileSync('ps'). A slow or wedged ps froze every terminal the daemon serves for up to the ps timeout on each call. Both paths now use the async process-table read (runProcess with a timeout that kills the child). SubprocessHandle.forceKill may return a promise; the termination controller awaits it so a refused signal still releases the claim for a retry. A force-kill or SIGWINCH whose root exits while ps runs signals nothing, since the captured groups could already be recycled. ConPTY force-kill stays synchronous so dispose() still sees the node-pty kill it must not repeat. The in-process provider's app-quit sweep keeps the blocking variant. The ps dialect and table readers move to posix-pty-process-table.ts. Refs #34410 Refs #33870 * fix(daemon): address review on async PTY process-table reads Guard only the root-pid fallback on exit so proven groups are still swept after dispose, wait out at most one ps dialect probe, move the sync-launch baseline entry with the table reader, and keep tests cross-platform. Refs #34410 Refs #33870 * fix(daemon): keep the PTY open until an in-flight force-kill sweep finishes A sweep whose root exits mid-read must not signal: the freed TTY may belong to another terminal. Dispose therefore defers destroy() until the sweep settles, so its hangup cannot end the root first. Refs #34410 Refs #33870 * test(daemon): build the destroy-tracking PTY mock without a type assertion Refs #34410 Refs #33870 * fix(pty): make the local graceful-deadline force-kill async and stub ps in tests The in-process provider's graceful-deadline timer now uses the async group sweep; the app-quit sweep and the root-signal path stay synchronous. Tests that drove force-kill through fixture pids now stub the async process-table read instead of querying the host, and a new test pins that waiters do not chain behind a failed dialect probe. Session ids are only read for rows on the root's terminal. Refs #34410 Refs #33870 * fix(pty): hold PTY teardown until an in-flight force-kill sweep settles Orphan cleanup could destroy a local PTY while the graceful-deadline sweep was still reading the process table, neutralizing its fallback kill and hanging up the root first. Both the daemon handle and the local provider now defer teardown through PendingPtyGroupSweep. Concurrent force-kill callers in the daemon now await the shared in-flight request, so a joiner's physical-exit budget starts after the signals are sent and it sees their rejection. Refs #34410 Refs #33870 * fix(pty): let disposal and orphan cleanup join the in-flight force-kill sweep Session disposal now joins the controller's shared force-kill request instead of starting a second sweep. Local orphan cleanup decides its escalation only after a pending graceful-deadline sweep settles, so a refused sweep is escalated before the handle is released rather than leaving the retry with a neutralized kill. Refs #34410 Refs #33870 * fix(daemon): SIGKILL live PTY groups before a shutdown deadline exit The old synchronous sweep sent its signals before the shutdown deadline timer could run; the async sweep can still be reading the process table when it fires. The deadline now SIGKILLs each live session's process group synchronously before exiting (node-pty roots lead their own group), falling back to the root pid when the group kill is refused. POSIX only. Refs #34410 Refs #33870 --- config/main-sync-spawn-baseline.txt | 3 +- .../process-host-direct-import-baseline.txt | 1 - src/main/daemon/daemon-entry.ts | 2 +- src/main/daemon/daemon-main.test.ts | 4 +- src/main/daemon/daemon-main.ts | 5 +- .../daemon/daemon-pty-adapter-test-harness.ts | 4 +- .../daemon-pty-router-history-handoff.test.ts | 4 +- .../daemon-rpc-shutdown-deadline.test.ts | 2 +- src/main/daemon/daemon-server.ts | 2 +- src/main/daemon/daemon-shutdown-deadline.ts | 7 +- src/main/daemon/daemon-spawner.test.ts | 4 +- .../pty-subprocess-handle-lifecycle.test.ts | 70 ++++- .../pty-subprocess-io-failure-cleanup.test.ts | 2 +- .../pty-subprocess/subprocess-handle.ts | 82 ++++-- src/main/daemon/session-subprocess-handle.ts | 3 +- .../daemon/session-termination-controller.ts | 42 ++- src/main/daemon/session.test.ts | 47 +++- src/main/daemon/session.ts | 4 +- .../slow-daemon-session-verification.test.ts | 4 +- src/main/daemon/terminal-host-disposal.ts | 35 +++ ...rminal-host-shutdown-deadline-kill.test.ts | 89 ++++++ src/main/daemon/terminal-host.ts | 25 +- .../local-pty-provider-shutdown.test.ts | 72 ++++- .../providers/local-pty-session-operations.ts | 4 +- src/main/providers/local-pty-termination.ts | 60 +++- src/main/pty/pending-pty-group-sweep.ts | 23 ++ .../pty/posix-pty-foreground-group.test.ts | 107 +++---- src/main/pty/posix-pty-foreground-group.ts | 48 ++-- ...six-pty-process-groups.integration.test.ts | 2 +- src/main/pty/posix-pty-process-groups.test.ts | 183 +++++++----- src/main/pty/posix-pty-process-groups.ts | 262 ++++++------------ ...posix-pty-process-table-event-loop.test.ts | 52 ++++ src/main/pty/posix-pty-process-table.ts | 181 ++++++++++++ 33 files changed, 1031 insertions(+), 404 deletions(-) create mode 100644 src/main/daemon/terminal-host-shutdown-deadline-kill.test.ts create mode 100644 src/main/pty/pending-pty-group-sweep.ts create mode 100644 src/main/pty/posix-pty-process-table-event-loop.test.ts create mode 100644 src/main/pty/posix-pty-process-table.ts diff --git a/config/main-sync-spawn-baseline.txt b/config/main-sync-spawn-baseline.txt index e9a2014a446..8e69898d8a1 100644 --- a/config/main-sync-spawn-baseline.txt +++ b/config/main-sync-spawn-baseline.txt @@ -36,8 +36,7 @@ 2 src/main/persistence/profile-state/profile-state-access-identity.ts 1 src/main/persistence/profile-state/profile-state-recovery-copy.ts 1 src/main/plugins/plugin-private-marketplace-ssh-shim.cjs -1 src/main/pty/posix-pty-foreground-group.ts -1 src/main/pty/posix-pty-process-groups.ts +1 src/main/pty/posix-pty-process-table.ts 1 src/main/pwsh.ts 1 src/main/runtime/runtime-skill-install-authority.ts 2 src/main/startup/cli-launch-redirect.ts diff --git a/config/process-host-direct-import-baseline.txt b/config/process-host-direct-import-baseline.txt index 4e2f7d8c225..957a2e595c6 100644 --- a/config/process-host-direct-import-baseline.txt +++ b/config/process-host-direct-import-baseline.txt @@ -88,7 +88,6 @@ src/main/ports/port-scan-command-execution.ts src/main/providers/macos-login-session-pty-probe.ts src/main/providers/process-cwd.ts src/main/pty-descendant-termination.ts -src/main/pty/posix-pty-foreground-group.ts src/main/pty/windows-environment-path.ts src/main/rate-limits/gemini-cli-oauth-extractor.ts src/main/runtime/windows-default-route-interfaces.ts diff --git a/src/main/daemon/daemon-entry.ts b/src/main/daemon/daemon-entry.ts index 7d53c37b76d..7b40ac4033d 100644 --- a/src/main/daemon/daemon-entry.ts +++ b/src/main/daemon/daemon-entry.ts @@ -160,7 +160,7 @@ async function main(): Promise { let daemon: DaemonHandle | null = null let deathWatch: MacosLoginSessionDeathWatch | null = null let shuttingDown = false - const shutdownDeadline = createDaemonShutdownDeadline(daemonLog, () => process.exit(0)) + const shutdownDeadline = createDaemonShutdownDeadline(daemonLog, () => daemon) const beginShutdown = (): void => { shuttingDown = true shutdownDeadline.start() diff --git a/src/main/daemon/daemon-main.test.ts b/src/main/daemon/daemon-main.test.ts index a52a865c007..afc752ee114 100644 --- a/src/main/daemon/daemon-main.test.ts +++ b/src/main/daemon/daemon-main.test.ts @@ -98,7 +98,9 @@ function createMockSubprocess() { resize: vi.fn(), kill: vi.fn(() => setTimeout(() => onExitCb?.(0), 5)), terminateOwnedTree: () => 'unavailable' as const, - forceKill: vi.fn(() => setTimeout(() => onExitCb?.(137), 5)), + forceKill: vi.fn(() => { + setTimeout(() => onExitCb?.(137), 5) + }), signal: vi.fn(), onData(_cb: (data: string) => void) {}, onExit(cb: (code: number) => void) { diff --git a/src/main/daemon/daemon-main.ts b/src/main/daemon/daemon-main.ts index e04cbd4ce2a..7f2612013bf 100644 --- a/src/main/daemon/daemon-main.ts +++ b/src/main/daemon/daemon-main.ts @@ -37,6 +37,8 @@ export type DaemonHandle = { recordOrderlyExit(): void /** Marks this run as stopped from outside before the teardown, which may not finish. */ recordStopRequested(): void + /** SIGKILLs each live PTY's process group synchronously, for a shutdown that overran its deadline. */ + killLiveSessionGroupsSync(): void } export async function startDaemon(opts: DaemonStartOptions): Promise { @@ -86,6 +88,7 @@ export async function startDaemon(opts: DaemonStartOptions): Promise server.shutdown(), recordOrderlyExit: () => server.automaticWork.recordOrderlyExit(), - recordStopRequested: () => server.automaticWork.recordStopRequested() + recordStopRequested: () => server.automaticWork.recordStopRequested(), + killLiveSessionGroupsSync: () => server.host.killLiveSessionGroupsSync() } } diff --git a/src/main/daemon/daemon-pty-adapter-test-harness.ts b/src/main/daemon/daemon-pty-adapter-test-harness.ts index 1d27df1ba14..55a372516ed 100644 --- a/src/main/daemon/daemon-pty-adapter-test-harness.ts +++ b/src/main/daemon/daemon-pty-adapter-test-harness.ts @@ -46,7 +46,9 @@ export function createMockSubprocess(dataOnSubscribe?: string): SubprocessHandle resume: vi.fn<() => void>(), kill: vi.fn(() => setTimeout(() => onExitCb?.(0), 5)), terminateOwnedTree: () => 'unavailable' as const, - forceKill: vi.fn(() => setTimeout(() => onExitCb?.(137), 5)), + forceKill: vi.fn(() => { + setTimeout(() => onExitCb?.(137), 5) + }), signal: vi.fn(), onData(cb) { onDataCb = cb diff --git a/src/main/daemon/daemon-pty-router-history-handoff.test.ts b/src/main/daemon/daemon-pty-router-history-handoff.test.ts index dca7d53d630..c87a9756563 100644 --- a/src/main/daemon/daemon-pty-router-history-handoff.test.ts +++ b/src/main/daemon/daemon-pty-router-history-handoff.test.ts @@ -26,7 +26,9 @@ function createMockSubprocess(): SubprocessHandle & { resume: vi.fn(), kill: vi.fn(() => setTimeout(() => onExit?.(0), 0)), terminateOwnedTree: () => 'unavailable' as const, - forceKill: vi.fn(() => setTimeout(() => onExit?.(137), 0)), + forceKill: vi.fn(() => { + setTimeout(() => onExit?.(137), 0) + }), signal: vi.fn(), onData(callback) { onData = callback diff --git a/src/main/daemon/daemon-rpc-shutdown-deadline.test.ts b/src/main/daemon/daemon-rpc-shutdown-deadline.test.ts index 3f71813290b..6084ba6562d 100644 --- a/src/main/daemon/daemon-rpc-shutdown-deadline.test.ts +++ b/src/main/daemon/daemon-rpc-shutdown-deadline.test.ts @@ -39,7 +39,7 @@ async function start(): Promise<{ }> { const exit = vi.fn() const log = vi.fn() - const deadline = createDaemonShutdownDeadline({ log, close: () => {} }, exit) + const deadline = createDaemonShutdownDeadline({ log, close: () => {} }, () => daemon, exit) cancelDeadline = deadline.cancel const requested = vi.fn(deadline.start) const socketPath = getDaemonSocketPath(directory) diff --git a/src/main/daemon/daemon-server.ts b/src/main/daemon/daemon-server.ts index 7e5e5a0ff80..67a263d86b0 100644 --- a/src/main/daemon/daemon-server.ts +++ b/src/main/daemon/daemon-server.ts @@ -36,7 +36,7 @@ export class DaemonServer { private readonly log: DaemonFileLog readonly automaticWork: DaemonAutomaticWorkGuard - private readonly host: TerminalHost + readonly host: TerminalHost private readonly transientFactRelay: BackgroundTransientFactRelay private readonly streamDataBatcher: DaemonStreamDataBatcher private readonly attachments: DaemonSessionAttachments diff --git a/src/main/daemon/daemon-shutdown-deadline.ts b/src/main/daemon/daemon-shutdown-deadline.ts index ffaa5900424..ab04f7ca4c7 100644 --- a/src/main/daemon/daemon-shutdown-deadline.ts +++ b/src/main/daemon/daemon-shutdown-deadline.ts @@ -2,10 +2,13 @@ import type { DaemonFileLog } from './daemon-file-log' export const DAEMON_SHUTDOWN_TIMEOUT_MS = 5_000 +type LiveSessionGroups = { killLiveSessionGroupsSync(): void } + /** Force exit if shutdown stalls, including RPC teardown. */ export function createDaemonShutdownDeadline( log: DaemonFileLog, - exit: () => void + liveSessions: () => LiveSessionGroups | null, + exit: () => void = () => process.exit(0) ): { start: () => void; cancel: () => void } { let timer: ReturnType | undefined return { @@ -16,6 +19,8 @@ export function createDaemonShutdownDeadline( timer = setTimeout(() => { log.log('shutdown-error', { message: 'shutdown timeout' }) log.close() + // Async teardown may still be reading the process table; never exit with its PTYs unsignalled. + liveSessions()?.killLiveSessionGroupsSync() exit() }, DAEMON_SHUTDOWN_TIMEOUT_MS) timer.unref() diff --git a/src/main/daemon/daemon-spawner.test.ts b/src/main/daemon/daemon-spawner.test.ts index 403c3a0d451..d862c578605 100644 --- a/src/main/daemon/daemon-spawner.test.ts +++ b/src/main/daemon/daemon-spawner.test.ts @@ -32,7 +32,9 @@ function createMockSubprocess(): SubprocessHandle { resize: vi.fn(), kill: vi.fn(() => setTimeout(() => onExitCb?.(0), 5)), terminateOwnedTree: () => 'unavailable' as const, - forceKill: vi.fn(() => setTimeout(() => onExitCb?.(137), 5)), + forceKill: vi.fn(() => { + setTimeout(() => onExitCb?.(137), 5) + }), signal: vi.fn(), onData(_cb: (data: string) => void) {}, onExit(cb: (code: number) => void) { diff --git a/src/main/daemon/pty-subprocess-handle-lifecycle.test.ts b/src/main/daemon/pty-subprocess-handle-lifecycle.test.ts index 75b2ec56d37..d8cf6848fe2 100644 --- a/src/main/daemon/pty-subprocess-handle-lifecycle.test.ts +++ b/src/main/daemon/pty-subprocess-handle-lifecycle.test.ts @@ -1,14 +1,17 @@ -import { describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import type * as LocalPtyUtils from '../providers/local-pty-utils' +import type * as PosixPtyProcessTable from '../pty/posix-pty-process-table' const { spawnMock, isPwshAvailableMock, validateWorkingDirectoryMock, resolveUnixShellPathMock, - resolveAgentForegroundProcessMock + resolveAgentForegroundProcessMock, + readProcessTableMock } = vi.hoisted(() => ({ spawnMock: vi.fn(), + readProcessTableMock: vi.fn<(rootPid: number) => Promise>(), isPwshAvailableMock: vi.fn(), resolveUnixShellPathMock: vi.fn((shellPath: string) => shellPath), resolveAgentForegroundProcessMock: vi.fn(), @@ -25,6 +28,12 @@ vi.mock('node-pty', () => ({ spawn: spawnMock })) +// Fixture pids must never be looked up in, or signalled through, the real host process table. +vi.mock('../pty/posix-pty-process-table', async (importOriginal) => ({ + ...(await importOriginal()), + readPosixPtyProcessTable: readProcessTableMock +})) + vi.mock('../pwsh', () => ({ isPwshAvailable: isPwshAvailableMock })) @@ -77,6 +86,11 @@ import { import { __setConptyJobNativeForTests } from '../windows/windows-pty-job' describe('createPtySubprocess', () => { + beforeEach(() => { + readProcessTableMock.mockReset() + readProcessTableMock.mockResolvedValue('') + }) + useDaemonPtySubprocessEnv({ spawnMock, isPwshAvailableMock, @@ -184,8 +198,10 @@ describe('createPtySubprocess', () => { try { const handle = await createPtySubprocess({ sessionId: 'test', cols: 80, rows: 24 }) - expect(() => handle.forceKill()).toThrow('SIGKILL rejected') - expect(() => handle.forceKill()).not.toThrow() + // ConPTY force-kill settles synchronously; POSIX awaits the process table. + const forceKill = async (): Promise => handle.forceKill() + await expect(forceKill()).rejects.toThrow('SIGKILL rejected') + await expect(forceKill()).resolves.toBeUndefined() expect(killSpy).toHaveBeenCalledTimes(2) expect(proc.kill).toHaveBeenCalledOnce() } finally { @@ -199,7 +215,7 @@ describe('createPtySubprocess', () => { const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) const handle = await createPtySubprocess({ sessionId: 'test', cols: 80, rows: 24 }) - handle.forceKill() + await handle.forceKill() expect(killSpy).toHaveBeenCalledWith(77, 'SIGKILL') killSpy.mockRestore() @@ -486,6 +502,50 @@ describe('createPtySubprocess', () => { killSpy.mockRestore() }) + it.skipIf(process.platform === 'win32')( + 'forceKill skips the root pid when it exits while the process table is read', + async () => { + const proc = mockPtyProcess(55) + spawnMock.mockReturnValue(proc) + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) + try { + const handle = await createPtySubprocess({ sessionId: 'test', cols: 80, rows: 24 }) + const table = Promise.withResolvers() + readProcessTableMock.mockReturnValueOnce(table.promise) + const pending = handle.forceKill() + proc._simulateExit(0) + table.resolve('') + await pending + expect(killSpy).not.toHaveBeenCalled() + } finally { + killSpy.mockRestore() + } + } + ) + + it.skipIf(process.platform === 'win32')( + 'dispose keeps the PTY master open until an in-flight force-kill has swept', + async () => { + const proc = { ...mockPtyProcess(56), destroy: vi.fn() } + spawnMock.mockReturnValue(proc) + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) + try { + const handle = await createPtySubprocess({ sessionId: 'test', cols: 80, rows: 24 }) + const table = Promise.withResolvers() + readProcessTableMock.mockReturnValueOnce(table.promise) + const pending = handle.forceKill() + handle.dispose() + expect(proc.destroy).not.toHaveBeenCalled() + table.resolve('') + await pending + expect(killSpy).toHaveBeenCalledWith(56, 'SIGKILL') + await vi.waitFor(() => expect(proc.destroy).toHaveBeenCalledOnce()) + } finally { + killSpy.mockRestore() + } + } + ) + it('signal is a no-op once proc.onExit has fired', async () => { const proc = mockPtyProcess(55) spawnMock.mockReturnValue(proc) diff --git a/src/main/daemon/pty-subprocess-io-failure-cleanup.test.ts b/src/main/daemon/pty-subprocess-io-failure-cleanup.test.ts index 76a780e5baf..ec96ec8f50f 100644 --- a/src/main/daemon/pty-subprocess-io-failure-cleanup.test.ts +++ b/src/main/daemon/pty-subprocess-io-failure-cleanup.test.ts @@ -17,7 +17,7 @@ vi.mock('./pty-subprocess/foreground-process-tracker', () => ({ }) })) vi.mock('../pty/posix-pty-process-groups', () => ({ - forceKillPosixPtyProcessGroups: (_pid: number, fallback: () => void) => fallback() + forceKillPosixPtyProcessGroups: async (_pid: number, fallback: () => void) => fallback() })) vi.mock('../pty-descendant-termination', () => ({ killWithDescendantSweep: async (_pid: number, killRoot: () => void) => killRoot() diff --git a/src/main/daemon/pty-subprocess/subprocess-handle.ts b/src/main/daemon/pty-subprocess/subprocess-handle.ts index 395abe113e2..1ce9cbfbc5c 100644 --- a/src/main/daemon/pty-subprocess/subprocess-handle.ts +++ b/src/main/daemon/pty-subprocess/subprocess-handle.ts @@ -3,6 +3,7 @@ import type { RecognizedAgentProcess } from '../../../shared/agent-process-recog import { readPtySlavePath } from '../../../shared/pty-slave-line-discipline-echo' import { forceKillPosixPtyProcessGroups } from '../../pty/posix-pty-process-groups' import { signalPosixPtyForegroundGroup } from '../../pty/posix-pty-foreground-group' +import { PendingPtyGroupSweep } from '../../pty/pending-pty-group-sweep' import { readPtsName } from '../../pty/node-pty-pts-name' import { terminatePtyJob } from '../../windows/windows-pty-job' import { isValidPtySize } from '../daemon-pty-size' @@ -45,6 +46,9 @@ export function createDaemonPtySubprocessHandle(args: { const nativeProc: DisposableNativePty = proc const events = new PtyPreListenerEvents() let dead = false + // Unlike dead, disposal keeps this false so an in-flight force-kill still lands. + let exited = false + const groupSweep = new PendingPtyGroupSweep() // I/O failure is not exit evidence; keep termination and producer flow control available. let ioFailed = false let disposed = false @@ -112,6 +116,7 @@ export function createDaemonPtySubprocessHandle(args: { const acceptExit = ({ exitCode, signal }: { exitCode: number; signal?: number }): void => { // Exit listeners may re-enter cleanup; retire signal authority before notifying them. dead = true + exited = true clearMasterRetiredTimer() foreground.markDead() // Neutralize kill before an async socket-close SIGHUP can target a recycled PID. @@ -130,6 +135,46 @@ export function createDaemonPtySubprocessHandle(args: { proc.onExit(acceptExit) } + const killRoot = (): void => { + process.kill(proc.pid, 'SIGKILL') + } + const fallBackToNodePtyKill = (signalError: unknown): void => { + try { + proc.kill() + nodePtyKillIssued = true + } catch { + nodePtyKillIssued = false + throw signalError + } + } + const forceKillPosix = async (): Promise => { + if (dead) { + return + } + // A reaped root frees its pid and TTY for reuse, so a table read that outlives it signals nothing. + await groupSweep.track( + forceKillPosixPtyProcessGroups(proc.pid, killRoot, { ownsRoot: () => !exited }).catch( + fallBackToNodePtyKill + ) + ) + } + // Stays synchronous: dispose() reads nodePtyKillIssued right after to avoid double-closing ConPTY. + const forceKillConpty = (): void => { + if (dead) { + return + } + // Escalate a ConPTY kill through the job without double-closing node-pty's shell handle. + if (nodePtyKillIssued) { + terminatePtyJob(proc) + return + } + try { + killRoot() + } catch (signalError) { + fallBackToNodePtyKill(signalError) + } + } + const slavePath = readPtySlavePath(proc) return { pid: proc.pid, @@ -215,29 +260,7 @@ export function createDaemonPtySubprocessHandle(args: { } }, terminateOwnedTree: () => terminatePtyJob(proc), - forceKill: () => { - if (dead) { - return - } - // Escalate a ConPTY kill through the job without double-closing node-pty's shell handle. - if (process.platform === 'win32' && nodePtyKillIssued) { - terminatePtyJob(proc) - return - } - try { - forceKillPosixPtyProcessGroups(proc.pid, () => { - process.kill(proc.pid, 'SIGKILL') - }) - } catch (signalError) { - try { - proc.kill() - nodePtyKillIssued = true - } catch { - nodePtyKillIssued = false - throw signalError - } - } - }, + forceKill: () => (process.platform === 'win32' ? forceKillConpty() : forceKillPosix()), signal: (sig) => { if (dead) { return @@ -255,7 +278,9 @@ export function createDaemonPtySubprocessHandle(args: { } // SIGWINCH belongs to the tty foreground group; destructive signals keep the root-pid target. if (sig === 'SIGWINCH') { - signalPosixPtyForegroundGroup(proc.pid, readPtsName(proc), sig, signalRootPid) + void signalPosixPtyForegroundGroup(proc.pid, readPtsName(proc), sig, signalRootPid, { + ownsRoot: () => !exited + }) return } signalRootPid() @@ -271,15 +296,18 @@ export function createDaemonPtySubprocessHandle(args: { clearMasterRetiredTimer() inputClosedListener = null events.clear() + const destroy = (): void => { + try { + nativeProc.destroy?.() + } catch {} + } // POSIX destroy() can asynchronously signal a recycled pid; Windows needs kill() to close ConPTY. if (process.platform !== 'win32') { nativeProc.kill = () => {} } else if (nodePtyKillIssued) { return } - try { - nativeProc.destroy?.() - } catch {} + groupSweep.afterSettled(destroy) } } } diff --git a/src/main/daemon/session-subprocess-handle.ts b/src/main/daemon/session-subprocess-handle.ts index a99d23e7583..de3f65d5ac0 100644 --- a/src/main/daemon/session-subprocess-handle.ts +++ b/src/main/daemon/session-subprocess-handle.ts @@ -37,7 +37,8 @@ export type SubprocessHandle = { /** Resync ConPTY after frontend clears to prevent stale-row prompt repaint. */ clear?(): void kill(): void - forceKill(): void + /** Settles once signals are sent; a rejection means the kernel refused them. */ + forceKill(): void | Promise /** Cover detached descendants via the PTY job; unavailable requires fallback and never proves death. */ terminateOwnedTree(): JobTerminationOutcome signal(sig: string): void diff --git a/src/main/daemon/session-termination-controller.ts b/src/main/daemon/session-termination-controller.ts index 566a1bb97a1..e13778a7587 100644 --- a/src/main/daemon/session-termination-controller.ts +++ b/src/main/daemon/session-termination-controller.ts @@ -19,7 +19,7 @@ export type SessionTerminationControllerDeps = { export class SessionTerminationController { private _isTerminating = false private killTimer: ReturnType | null = null - private forceKillSent = false + private forceKillRequest: Promise | null = null private subprocessDisposed = false private readonly physicalExit = new PhysicalExitTracker() @@ -98,6 +98,11 @@ export class SessionTerminationController { this.deps.subprocess.signal(sig) } + /** Joins any in-flight request so disposal never starts a second process-group sweep. */ + forceKillDuringDispose(): Promise { + return this.requestForceKill() + } + disposeSubprocessHandle(): void { if (this.subprocessDisposed) { return @@ -120,37 +125,44 @@ export class SessionTerminationController { this.killTimer = setTimeout(() => { this.killTimer = null if (!this.deps.isExited()) { - try { - this.requestForceKill() - } catch (error) { + this.requestForceKill().catch((error: unknown) => { console.warn('[Session] failed to force-kill terminating subprocess:', error) // A transient force-kill rejection must not consume the only fallback attempt. if (attemptsRemaining > 1) { this.armForceKillFallback(SESSION_FORCE_KILL_RETRY_MS, attemptsRemaining - 1) } - } + }) } }, delayMs) } - private requestForceKill(): void { - if (this.deps.isExited() || this.forceKillSent) { - return + /** Joiners await the in-flight request so their exit budget starts after signals are sent. */ + private requestForceKill(): Promise { + if (this.deps.isExited()) { + return Promise.resolve() } - this.forceKillSent = true - try { - this.deps.subprocess.forceKill() - } catch (error) { - this.forceKillSent = false - throw error + if (!this.forceKillRequest) { + const request = this.sendForceKill() + this.forceKillRequest = request + // A refused signal releases the claim so a retry can send again. + request.catch(() => { + if (this.forceKillRequest === request) { + this.forceKillRequest = null + } + }) } + return this.forceKillRequest + } + + private async sendForceKill(): Promise { + await this.deps.subprocess.forceKill() } private async requestForceKillWithRetry(): Promise { let lastError: unknown for (let attempt = 0; attempt < SESSION_FORCE_KILL_MAX_ATTEMPTS; attempt++) { try { - this.requestForceKill() + await this.requestForceKill() return } catch (error) { lastError = error diff --git a/src/main/daemon/session.test.ts b/src/main/daemon/session.test.ts index 3368892e1db..ce8f6c63366 100644 --- a/src/main/daemon/session.test.ts +++ b/src/main/daemon/session.test.ts @@ -1,7 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { Session } from './session' import { TerminalSessionTeardown } from './terminal-session-teardown' -import { SESSION_FORCE_KILL_RETRY_MS } from './session-termination-controller' +import { + IMMEDIATE_KILL_PHYSICAL_EXIT_TIMEOUT_MS, + SESSION_FORCE_KILL_RETRY_MS +} from './session-termination-controller' import type { SessionState, ShellReadyState } from './types' import type { TuiAgent } from '../../shared/tui-agent' import { @@ -729,6 +732,48 @@ describe('Session', () => { expect(session.state).toBe('exited') }) + it('starts a joining caller’s exit budget only after the shared force kill is sent', async () => { + const signalsSent = Promise.withResolvers() + let forceKillAttempts = 0 + subprocess.forceKill = () => { + forceKillAttempts++ + return signalsSent.promise + } + createSession() + + const first = session.forceKillAndWaitForExit() + const joined = session.forceKillAndWaitForExit() + let joinedSettled = false + void joined.then( + () => (joinedSettled = true), + () => (joinedSettled = true) + ) + await vi.advanceTimersByTimeAsync(IMMEDIATE_KILL_PHYSICAL_EXIT_TIMEOUT_MS) + expect(joinedSettled).toBe(false) + + signalsSent.resolve() + subprocess.simulateExit(137) + await Promise.all([first, joined]) + expect(forceKillAttempts).toBe(1) + }) + + it('lets dispose join an in-flight force kill instead of starting a second sweep', async () => { + const signalsSent = Promise.withResolvers() + let forceKillAttempts = 0 + subprocess.forceKill = () => { + forceKillAttempts++ + return signalsSent.promise + } + createSession() + + // Dispose ends the session without a physical exit, so this waiter is not the subject here. + void session.forceKillAndWaitForExit().catch(() => {}) + session.dispose() + signalsSent.resolve() + await vi.advanceTimersByTimeAsync(0) + expect(forceKillAttempts).toBe(1) + }) + it('retries a rejected graceful-deadline force kill', async () => { subprocess.kill = () => {} let forceKillAttempts = 0 diff --git a/src/main/daemon/session.ts b/src/main/daemon/session.ts index e93fde52a6d..187df3b54cb 100644 --- a/src/main/daemon/session.ts +++ b/src/main/daemon/session.ts @@ -289,9 +289,7 @@ export class Session { const wasTerminating = this.termination.isTerminating && this._state !== 'exited' const clientsToNotify = wasTerminating ? this.output.snapshotClients() : [] if (wasTerminating) { - try { - this.subprocess.forceKill() - } catch {} + this.termination.forceKillDuringDispose().catch(() => {}) this._exitCode = -1 this.termination.clearTerminating() } diff --git a/src/main/daemon/slow-daemon-session-verification.test.ts b/src/main/daemon/slow-daemon-session-verification.test.ts index cbf2f36b54a..cca94466eba 100644 --- a/src/main/daemon/slow-daemon-session-verification.test.ts +++ b/src/main/daemon/slow-daemon-session-verification.test.ts @@ -25,7 +25,9 @@ function createMockSubprocess(): SubprocessHandle { resize: vi.fn(), kill: vi.fn(() => setTimeout(() => onExitCb?.(0), 5)), terminateOwnedTree: () => 'unavailable' as const, - forceKill: vi.fn(() => setTimeout(() => onExitCb?.(137), 5)), + forceKill: vi.fn(() => { + setTimeout(() => onExitCb?.(137), 5) + }), signal: vi.fn(), onData: vi.fn(), onExit: vi.fn((callback: (code: number) => void) => { diff --git a/src/main/daemon/terminal-host-disposal.ts b/src/main/daemon/terminal-host-disposal.ts index baa432c0f16..0a733db4e34 100644 --- a/src/main/daemon/terminal-host-disposal.ts +++ b/src/main/daemon/terminal-host-disposal.ts @@ -39,3 +39,38 @@ export async function disposeTerminalHostSessions({ ]) killedTombstones.clear() } + +/** Last resort when shutdown overruns its deadline: an async group sweep may not have signalled yet. */ +export function killLiveSessionGroupsSync(sessions: Iterable): void { + if (process.platform === 'win32') { + return + } + for (const session of sessions) { + // Exited roots are reaped, so their pids may already belong to someone else. + if (session.isAlive) { + killPtyRootProcessGroup(session.pid) + } + } +} + +/** node-pty roots lead their own session and group, so -pid reaches the group without a ps lookup. */ +function killPtyRootProcessGroup(rootPid: number): void { + try { + process.kill(-rootPid, 'SIGKILL') + return + } catch (error) { + if (!isGroupKillRefused(error)) { + return + } + } + try { + process.kill(rootPid, 'SIGKILL') + } catch { + // Exit can win the race with this last-resort signal. + } +} + +function isGroupKillRefused(error: unknown): boolean { + const code = error instanceof Error && 'code' in error ? error.code : undefined + return code === 'ESRCH' || code === 'EPERM' +} diff --git a/src/main/daemon/terminal-host-shutdown-deadline-kill.test.ts b/src/main/daemon/terminal-host-shutdown-deadline-kill.test.ts new file mode 100644 index 00000000000..ee64b8bbde9 --- /dev/null +++ b/src/main/daemon/terminal-host-shutdown-deadline-kill.test.ts @@ -0,0 +1,89 @@ +import './mock-descendant-sweep' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { SubprocessHandle } from './session-subprocess-handle' +import { + createDaemonShutdownDeadline, + DAEMON_SHUTDOWN_TIMEOUT_MS +} from './daemon-shutdown-deadline' +import { TerminalHost } from './terminal-host' + +const ROOT_PID = 424_242 + +// The force-kill sweep is still waiting on slow process-table reads when the deadline fires. +function createSubprocessWithSlowSweep(): SubprocessHandle { + return { + pid: ROOT_PID, + getForegroundProcess: () => null, + write: () => {}, + resize: () => {}, + kill: () => {}, + terminateOwnedTree: () => 'unavailable', + forceKill: () => new Promise(() => {}), + signal: () => {}, + onData: () => {}, + onExit: () => {}, + dispose: () => {} + } +} + +describe.skipIf(process.platform === 'win32')('daemon shutdown deadline', () => { + let host: TerminalHost + let exit: ReturnType void>> + let cancelDeadline: () => void + + beforeEach(async () => { + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + host = new TerminalHost({ spawnSubprocess: createSubprocessWithSlowSweep }) + await host.createOrAttach({ + sessionId: 'slow-sweep', + cols: 80, + rows: 24, + streamClient: { onData: () => {}, onExit: () => {} } + }) + exit = vi.fn<() => void>() + const deadline = createDaemonShutdownDeadline( + { log: () => {}, close: () => {} }, + () => host, + exit + ) + cancelDeadline = deadline.cancel + deadline.start() + }) + + afterEach(() => { + cancelDeadline() + vi.useRealTimers() + vi.restoreAllMocks() + }) + + it('SIGKILLs the PTY process group before exiting while the async sweep is still pending', async () => { + const kill = vi.spyOn(process, 'kill').mockImplementation(() => true) + void host.dispose() + + await vi.advanceTimersByTimeAsync(DAEMON_SHUTDOWN_TIMEOUT_MS) + + expect(kill.mock.calls).toEqual([[-ROOT_PID, 'SIGKILL']]) + expect(exit).toHaveBeenCalledOnce() + }) + + it.each(['ESRCH', 'EPERM'])( + 'falls back to the root pid when the group kill fails with %s', + async (code) => { + const kill = vi.spyOn(process, 'kill').mockImplementation((pid) => { + if (Number(pid) < 0) { + throw Object.assign(new Error('group kill refused'), { code }) + } + return true + }) + void host.dispose() + + await vi.advanceTimersByTimeAsync(DAEMON_SHUTDOWN_TIMEOUT_MS) + + expect(kill.mock.calls).toEqual([ + [-ROOT_PID, 'SIGKILL'], + [ROOT_PID, 'SIGKILL'] + ]) + expect(exit).toHaveBeenCalledOnce() + } + ) +}) diff --git a/src/main/daemon/terminal-host.ts b/src/main/daemon/terminal-host.ts index decff455ade..5d572f2ff12 100644 --- a/src/main/daemon/terminal-host.ts +++ b/src/main/daemon/terminal-host.ts @@ -5,7 +5,7 @@ import type { PtyProcessInspectionOptions } from '../providers/pty-process-inspe import type { SessionInfo, TakePendingOutputResult, TerminalSnapshot } from './types' import type { CreateOrAttachResult } from './terminal-host-create-contract' import type { TerminalHostOptions } from './terminal-host-options' -import { disposeTerminalHostSessions } from './terminal-host-disposal' +import { disposeTerminalHostSessions, killLiveSessionGroupsSync } from './terminal-host-disposal' import { getAliveTerminalHostSession } from './terminal-host-session-access' import { writeSessionInput } from './session-input-path' import { TerminalSessionTeardown } from './terminal-session-teardown' @@ -317,12 +317,23 @@ export class TerminalHost { return hasLiveTerminalHostSessions(this.sessions) } + /** Last resort when shutdown overruns its deadline: an async group sweep may not have signalled yet. */ + killLiveSessionGroupsSync(): void { + killLiveSessionGroupsSync(this.sessions.values()) + } + dispose(): Promise { this.creationFenced = true if (this.disposePromise) { return this.disposePromise } - const disposePromise = this.disposeSessions() + const disposePromise = disposeTerminalHostSessions({ + pendingCreations: this.pendingCreations, + sessionTeardown: this.sessionTeardown, + sessions: this.sessions, + onFinalCheckpoint: this.onFinalCheckpoint, + killedTombstones: this.killedTombstones + }) this.disposePromise = disposePromise void disposePromise.catch(() => { // Keep failed native owners retryable on the next shutdown request. @@ -332,14 +343,4 @@ export class TerminalHost { }) return disposePromise } - - private disposeSessions(): Promise { - return disposeTerminalHostSessions({ - pendingCreations: this.pendingCreations, - sessionTeardown: this.sessionTeardown, - sessions: this.sessions, - onFinalCheckpoint: this.onFinalCheckpoint, - killedTombstones: this.killedTombstones - }) - } } diff --git a/src/main/providers/local-pty-provider-shutdown.test.ts b/src/main/providers/local-pty-provider-shutdown.test.ts index 59be8e6d00c..70c037319ec 100644 --- a/src/main/providers/local-pty-provider-shutdown.test.ts +++ b/src/main/providers/local-pty-provider-shutdown.test.ts @@ -1,5 +1,6 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type * as MacosTccLoginShell from './macos-tcc-login-shell' +import type * as PosixPtyProcessTable from '../pty/posix-pty-process-table' const { existsSyncMock, @@ -14,8 +15,10 @@ const { killWithDescendantSweepMock, isWslAvailableAsyncMock, wslUncDirectoryExistsAsyncMock, - createShellPromptReadinessProbeMock + createShellPromptReadinessProbeMock, + readProcessTableMock } = vi.hoisted(() => ({ + readProcessTableMock: vi.fn<(rootPid: number) => Promise>(), existsSyncMock: vi.fn(), statSyncMock: vi.fn(), accessSyncMock: vi.fn(), @@ -53,6 +56,13 @@ vi.mock('node-pty', () => ({ spawn: spawnMock })) +// Fixture pids must never be looked up in the real host process table. +vi.mock('../pty/posix-pty-process-table', async (importOriginal) => ({ + ...(await importOriginal()), + readPosixPtyProcessTable: readProcessTableMock, + readPosixPtyProcessTableSync: () => '' +})) + vi.mock('./macos-tcc-login-shell', async (importOriginal) => ({ ...(await importOriginal()), prepareMacosTccLoginShell: prepareMacosTccLoginShellMock @@ -154,6 +164,8 @@ describe('LocalPtyProvider', () => { } }) spawnMock.mockReturnValue(mockProc) + readProcessTableMock.mockReset() + readProcessTableMock.mockResolvedValue('') provider = new LocalPtyProvider() }) @@ -294,6 +306,64 @@ describe('LocalPtyProvider', () => { } }) + it('keeps the kill handle until a graceful-deadline sweep settles when orphan cleanup joins', async () => { + vi.useFakeTimers() + try { + const killSpy = vi.fn() + const destroySpy = vi.fn() + spawnMock.mockReturnValue({ ...mockProc, kill: killSpy, destroy: destroySpy }) + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + const table = Promise.withResolvers() + readProcessTableMock.mockReturnValueOnce(table.promise) + + const graceful = provider.shutdown(id, { immediate: false }) + await vi.advanceTimersByTimeAsync(LOCAL_PTY_GRACEFUL_FORCE_TIMEOUT_MS) + expect(provider.killOrphanedPtys(1)).toEqual([{ id }]) + expect(destroySpy).not.toHaveBeenCalled() + + table.resolve('') + await vi.waitFor(() => expect(destroySpy).toHaveBeenCalledOnce()) + expect(killSpy.mock.calls).toEqual([['SIGTERM'], ['SIGKILL']]) + exitCb?.({ exitCode: 137 }) + await graceful + } finally { + vi.useRealTimers() + } + }) + + it('escalates through orphan cleanup when the graceful-deadline sweep it joined is refused', async () => { + vi.useFakeTimers() + try { + let forceAttempts = 0 + const destroySpy = vi.fn() + const killSpy = vi.fn((signal: string) => { + if (signal === 'SIGKILL' && forceAttempts++ === 0) { + throw new Error('transient force-kill failure') + } + if (destroySpy.mock.calls.length > 0) { + throw new Error('killed through a released handle') + } + }) + spawnMock.mockReturnValue({ ...mockProc, kill: killSpy, destroy: destroySpy }) + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + const table = Promise.withResolvers() + readProcessTableMock.mockReturnValueOnce(table.promise) + + const graceful = provider.shutdown(id, { immediate: false }) + await vi.advanceTimersByTimeAsync(LOCAL_PTY_GRACEFUL_FORCE_TIMEOUT_MS) + provider.killOrphanedPtys(1) + table.resolve('') + + await vi.waitFor(() => expect(destroySpy).toHaveBeenCalledOnce()) + expect(killSpy.mock.calls).toEqual([['SIGTERM'], ['SIGKILL'], ['SIGKILL']]) + exitCb?.({ exitCode: 137 }) + await graceful + expect(vi.getTimerCount()).toBe(0) + } finally { + vi.useRealTimers() + } + }) + it('retries a rejected graceful-deadline SIGKILL before the physical timeout', async () => { vi.useFakeTimers() try { diff --git a/src/main/providers/local-pty-session-operations.ts b/src/main/providers/local-pty-session-operations.ts index 2c6008c8f5d..b9d23d36330 100644 --- a/src/main/providers/local-pty-session-operations.ts +++ b/src/main/providers/local-pty-session-operations.ts @@ -87,7 +87,9 @@ export async function sendLocalPtySignal(id: string, signal: string): Promise ptyProcesses.get(id) === proc + }) return } signalRootPid() diff --git a/src/main/providers/local-pty-termination.ts b/src/main/providers/local-pty-termination.ts index ddf12a2622b..355a987e05f 100644 --- a/src/main/providers/local-pty-termination.ts +++ b/src/main/providers/local-pty-termination.ts @@ -2,7 +2,11 @@ import type * as pty from 'node-pty' import { PhysicalExitTracker } from '../../shared/physical-exit-tracker' import { killWithDescendantSweep } from '../pty-descendant-termination' import { terminateShutdownDescendants } from '../daemon/terminal-descendant-shutdown' -import { forceKillPosixPtyProcessGroups } from '../pty/posix-pty-process-groups' +import { + forceKillPosixPtyProcessGroups, + forceKillPosixPtyProcessGroupsSync +} from '../pty/posix-pty-process-groups' +import { PendingPtyGroupSweep } from '../pty/pending-pty-group-sweep' import { terminatePtyJob } from '../windows/windows-pty-job' import { clearLocalPtyForceKillTimer, @@ -42,6 +46,17 @@ function waitForPtyPhysicalExit(id: string, physicalExit?: PhysicalExitTracker): ) } +const groupSweeps = new WeakMap() + +function groupSweepFor(proc: pty.IPty): PendingPtyGroupSweep { + let sweep = groupSweeps.get(proc) + if (!sweep) { + sweep = new PendingPtyGroupSweep() + groupSweeps.set(proc, sweep) + } + return sweep +} + function killLocalPtyProcess(proc: pty.IPty, immediate: boolean): void { if (process.platform === 'win32') { proc.kill() @@ -51,7 +66,7 @@ function killLocalPtyProcess(proc: pty.IPty, immediate: boolean): void { proc.kill('SIGTERM') return } - forceKillPosixPtyProcessGroups(proc.pid, () => proc.kill('SIGKILL')) + forceKillPosixPtyProcessGroupsSync(proc.pid, () => proc.kill('SIGKILL')) } function armLocalPtyForceKill( @@ -69,19 +84,24 @@ function armLocalPtyForceKill( return } ptyTerminationMode.set(id, 'force') - try { - killLocalPtyProcess(proc, true) - } catch (error) { - ptyTerminationMode.set(id, 'graceful') - console.error('[pty] failed to force-kill PTY after graceful deadline', { id, error }) - // Why: a transient native rejection must not consume the only SIGKILL owner while shutdown still awaits physical exit. - if (attemptsRemaining > 1) { - armLocalPtyForceKill(id, proc, { - delayMs: LOCAL_PTY_FORCE_KILL_RETRY_MS, - attemptsRemaining: attemptsRemaining - 1 + // Graceful mode is POSIX-only, so this deadline never reaches the ConPTY kill path. + groupSweepFor(proc) + .track( + forceKillPosixPtyProcessGroups(proc.pid, () => proc.kill('SIGKILL'), { + ownsRoot: () => ptyProcesses.get(id) === proc }) - } - } + ) + .catch((error: unknown) => { + ptyTerminationMode.set(id, 'graceful') + console.error('[pty] failed to force-kill PTY after graceful deadline', { id, error }) + // Why: a transient native rejection must not consume the only SIGKILL owner while shutdown still awaits physical exit. + if (attemptsRemaining > 1) { + armLocalPtyForceKill(id, proc, { + delayMs: LOCAL_PTY_FORCE_KILL_RETRY_MS, + attemptsRemaining: attemptsRemaining - 1 + }) + } + }) }, options.delayMs ?? LOCAL_PTY_GRACEFUL_FORCE_TIMEOUT_MS) timer.unref?.() ptyForceKillTimers.set(id, timer) @@ -105,6 +125,16 @@ export function destroyPtyProcess(proc: pty.IPty, options: { alreadyKilled?: boo function requestPtyTermination(id: string, proc: pty.IPty): void { runPtyCleanup(id) disposePtyListeners(id) + // A pending deadline sweep owns escalation until it settles; a rejected one hands it back here. + groupSweepFor(proc).afterSettled(() => finishPtyTermination(id, proc)) +} + +function finishPtyTermination(id: string, proc: pty.IPty): void { + if (ptyProcesses.get(id) !== proc) { + // Exited while the sweep ran: its pid may be reused, so only release the handle. + destroyPtyProcess(proc, { alreadyKilled: true }) + return + } const previousMode = ptyTerminationMode.get(id) // Escalate outstanding graceful shutdown before cleanup neutralizes kill and disables its deadline. if (previousMode !== 'force') { @@ -243,7 +273,7 @@ export function killAllLocalPtys(): void { try { if (ptyAgentSessionIds.has(id) && process.platform !== 'win32') { // Synchronous quit must sweep agent groups before releasing node-pty or OMP workers can survive. - forceKillPosixPtyProcessGroups(proc.pid, () => proc.kill('SIGKILL')) + forceKillPosixPtyProcessGroupsSync(proc.pid, () => proc.kill('SIGKILL')) } else { proc.kill() } diff --git a/src/main/pty/pending-pty-group-sweep.ts b/src/main/pty/pending-pty-group-sweep.ts new file mode 100644 index 00000000000..0342dc7bb23 --- /dev/null +++ b/src/main/pty/pending-pty-group-sweep.ts @@ -0,0 +1,23 @@ +/** Holds PTY teardown until an in-flight group sweep settles: closing the master first hangs up the root, and the sweep then skips its groups. */ +export class PendingPtyGroupSweep { + private pending: Promise | null = null + + track(sweep: Promise): Promise { + this.pending = sweep + const settled = (): void => { + if (this.pending === sweep) { + this.pending = null + } + } + sweep.then(settled, settled) + return sweep + } + + afterSettled(teardown: () => void): void { + if (this.pending) { + this.pending.then(teardown, teardown) + return + } + teardown() + } +} diff --git a/src/main/pty/posix-pty-foreground-group.test.ts b/src/main/pty/posix-pty-foreground-group.test.ts index c8689c6ccea..4c35334dbae 100644 --- a/src/main/pty/posix-pty-foreground-group.test.ts +++ b/src/main/pty/posix-pty-foreground-group.test.ts @@ -1,12 +1,27 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -import { execFileSync } from 'node:child_process' +import type { ProcessResult, ProcessSpec } from '@orca/process-host/process-spec' import { getPosixPtyForegroundGroup, resetPosixPtyForegroundGroupOwnRowCache, signalPosixPtyForegroundGroup } from './posix-pty-foreground-group' -vi.mock('node:child_process', () => ({ execFileSync: vi.fn(() => '') })) +const { runProcessMock } = vi.hoisted(() => ({ + runProcessMock: vi.fn<(spec: ProcessSpec) => Promise>() +})) +const pidArg = (spec: ProcessSpec): string => { + const args = spec.args ?? [] + return args[args.indexOf('-p') + 1] +} +vi.mock('@orca/process-host', () => ({ runProcess: runProcessMock })) + +const psResult = (stdout: string): ProcessResult => ({ + code: 0, + signal: null, + stdout, + stderr: '', + timedOut: false +}) const macosTable = ['84644 84985 ttys318', ' 4242 4242 ttys002'].join('\n') @@ -43,13 +58,13 @@ describe('getPosixPtyForegroundGroup', () => { }) describe('signalPosixPtyForegroundGroup', () => { - const table = (): string => macosTable + const table = async (): Promise => macosTable - it('signals the negated foreground group', () => { + it('signals the negated foreground group', async () => { const kill = vi.spyOn(process, 'kill').mockImplementation(() => true) const fallback = vi.fn() try { - signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', fallback, { + await signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', fallback, { platform: 'darwin', currentPid: 4242, readProcessTable: table @@ -61,10 +76,10 @@ describe('signalPosixPtyForegroundGroup', () => { } }) - it('falls back to the root pid on Windows', () => { + it('falls back to the root pid on Windows', async () => { // Windows rejects negative PIDs and treats SIGTERM/SIGINT as termination. const fallback = vi.fn() - signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', fallback, { + await signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', fallback, { platform: 'win32', currentPid: 4242, readProcessTable: table @@ -72,9 +87,9 @@ describe('signalPosixPtyForegroundGroup', () => { expect(fallback).toHaveBeenCalledOnce() }) - it('falls back when the slave device name is unknown', () => { + it('falls back when the slave device name is unknown', async () => { const fallback = vi.fn() - signalPosixPtyForegroundGroup(84644, undefined, 'SIGWINCH', fallback, { + await signalPosixPtyForegroundGroup(84644, undefined, 'SIGWINCH', fallback, { platform: 'darwin', currentPid: 4242, readProcessTable: table @@ -82,19 +97,19 @@ describe('signalPosixPtyForegroundGroup', () => { expect(fallback).toHaveBeenCalledOnce() }) - it('falls back when the process table cannot be read', () => { + it('falls back when the process table cannot be read', async () => { const fallback = vi.fn() - signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', fallback, { + await signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', fallback, { platform: 'darwin', currentPid: 4242, - readProcessTable: () => { + readProcessTable: async () => { throw new Error('ps timed out') } }) expect(fallback).toHaveBeenCalledOnce() }) - it('treats a vanished foreground group as done rather than falling back', () => { + it('treats a vanished foreground group as done rather than falling back', async () => { // ESRCH means the captured target vanished; root fallback would be stale too. const kill = vi.spyOn(process, 'kill').mockImplementation(() => { const error = new Error('no such process') as NodeJS.ErrnoException @@ -103,7 +118,7 @@ describe('signalPosixPtyForegroundGroup', () => { }) const fallback = vi.fn() try { - signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', fallback, { + await signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', fallback, { platform: 'darwin', currentPid: 4242, readProcessTable: table @@ -114,7 +129,7 @@ describe('signalPosixPtyForegroundGroup', () => { } }) - it('falls back when foreground group signalling fails', () => { + it('falls back when foreground group signalling fails', async () => { const kill = vi.spyOn(process, 'kill').mockImplementation(() => { const error = new Error('operation not permitted') as NodeJS.ErrnoException error.code = 'EPERM' @@ -122,7 +137,7 @@ describe('signalPosixPtyForegroundGroup', () => { }) const fallback = vi.fn() try { - signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', fallback, { + await signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', fallback, { platform: 'darwin', currentPid: 4242, readProcessTable: table @@ -139,29 +154,25 @@ describe('process table lookup', () => { resetPosixPtyForegroundGroupOwnRowCache() }) - it('asks ps for one pid at a time', () => { + it('asks ps for one pid at a time', async () => { // macOS PID lists scan the whole table (~3.6s vs ~3ms), silently timing out into root-only delivery. - const execFileSyncMock = vi.mocked(execFileSync) - execFileSyncMock.mockClear() - execFileSyncMock.mockReturnValue('84644 84985 ttys318' as never) + runProcessMock.mockReset() + runProcessMock.mockResolvedValue(psResult('84644 84985 ttys318')) // Mock group signalling so fixture PGIDs cannot target real host processes. const kill = vi.spyOn(process, 'kill').mockImplementation(() => true) try { - signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', vi.fn(), { + await signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', vi.fn(), { platform: 'darwin', currentPid: 4242 }) - expect(execFileSyncMock).toHaveBeenCalled() + expect(runProcessMock).toHaveBeenCalled() let timeoutBudget = 0 - for (const call of execFileSyncMock.mock.calls) { - const args = call[1] as string[] - const options = call[2] as { timeout: number } - const pidArg = args[args.indexOf('-p') + 1] - expect(pidArg).toMatch(/^\d+$/) - expect(args.filter((arg) => arg === '-p')).toHaveLength(1) - timeoutBudget += options.timeout + for (const [spec] of runProcessMock.mock.calls) { + expect(pidArg(spec)).toMatch(/^\d+$/) + expect(spec.args?.filter((arg) => arg === '-p')).toHaveLength(1) + timeoutBudget += spec.timeoutMs ?? Infinity } expect(timeoutBudget).toBeLessThanOrEqual(250) } finally { @@ -169,9 +180,8 @@ describe('process table lookup', () => { } }) - it('forks ps once per pane after the first SIGWINCH of the process', () => { - // Reuse Orca's own TTY row: repeated ps forks doubled the per-pane synchronous resize stall. - const execFileSyncMock = vi.mocked(execFileSync) + it('forks ps once per pane after the first SIGWINCH of the process', async () => { + // Reuse Orca's own TTY row: repeated ps forks doubled the per-pane resize cost. const kill = vi.spyOn(process, 'kill').mockImplementation(() => true) const panes = [ { rootPid: 900, tty: 'ttys301' }, @@ -181,35 +191,38 @@ describe('process table lookup', () => { ] try { - execFileSyncMock.mockClear() - execFileSyncMock.mockImplementation(((_file: string, args: string[]) => { - const pid = Number(args[args.indexOf('-p') + 1]) + runProcessMock.mockReset() + runProcessMock.mockImplementation(async (spec) => { + const pid = Number(pidArg(spec)) if (pid === 4242) { - return '4242 4242 ttys002' + return psResult('4242 4242 ttys002') } const pane = panes.find((entry) => entry.rootPid === pid) - return pane ? `${pane.rootPid} ${pane.rootPid + 50} ${pane.tty}` : '' - }) as never) + return psResult(pane ? `${pane.rootPid} ${pane.rootPid + 50} ${pane.tty}` : '') + }) for (const pane of panes) { // Hidden restore and reattach each signal once per pane. for (let signalIndex = 0; signalIndex < 2; signalIndex += 1) { - signalPosixPtyForegroundGroup(pane.rootPid, `/dev/${pane.tty}`, 'SIGWINCH', vi.fn(), { - platform: 'darwin', - currentPid: 4242 - }) + await signalPosixPtyForegroundGroup( + pane.rootPid, + `/dev/${pane.tty}`, + 'SIGWINCH', + vi.fn(), + { + platform: 'darwin', + currentPid: 4242 + } + ) } } - const pidArgs = execFileSyncMock.mock.calls.map((call) => - Number((call[1] as string[])[(call[1] as string[]).indexOf('-p') + 1]) - ) + const pidArgs = runProcessMock.mock.calls.map(([spec]) => Number(pidArg(spec))) expect(pidArgs.filter((pid) => pid === 4242)).toHaveLength(1) expect(pidArgs).toHaveLength(9) expect(kill).toHaveBeenCalledTimes(8) } finally { - execFileSyncMock.mockReset() - execFileSyncMock.mockReturnValue('' as never) + runProcessMock.mockReset() kill.mockRestore() } }) diff --git a/src/main/pty/posix-pty-foreground-group.ts b/src/main/pty/posix-pty-foreground-group.ts index 243bc279349..2460b82a6d0 100644 --- a/src/main/pty/posix-pty-foreground-group.ts +++ b/src/main/pty/posix-pty-foreground-group.ts @@ -1,5 +1,4 @@ -import { execFileSync } from 'node:child_process' -import { notifySpawnObserver } from '@orca/process-host/spawn-observer' +import { runProcess } from '@orca/process-host' const PROCESS_TABLE_LOOKUP_TIMEOUT_MS = 250 const PROCESS_TABLE_QUERY_TIMEOUT_MS = PROCESS_TABLE_LOOKUP_TIMEOUT_MS / 2 @@ -8,7 +7,9 @@ const PROCESS_TABLE_MAX_BYTES = 64 * 1024 export type PosixPtyForegroundGroupDeps = { platform?: NodeJS.Platform currentPid?: number - readProcessTable?: () => string + readProcessTable?: () => Promise + /** False once the root has exited; a stale table could then name a recycled group. */ + ownsRoot?: () => boolean } type ProcessRow = { @@ -26,28 +27,26 @@ function hasUsableTty(tty: string): boolean { return tty !== '?' && tty !== '??' && tty !== '-' } -function runPs(pid: number): string { - const args = ['-p', String(pid), '-o', 'pid=,tpgid=,tty='] - const startedAt = performance.now() - try { - return execFileSync('ps', args, { - encoding: 'utf8', - timeout: PROCESS_TABLE_QUERY_TIMEOUT_MS, - maxBuffer: PROCESS_TABLE_MAX_BYTES - }) - } finally { - // Diagnostics only: execFileSync blocks the caller for the child's whole life. - notifySpawnObserver('ps', args, performance.now() - startedAt) +async function runPs(pid: number): Promise { + const result = await runProcess({ + program: 'ps', + args: ['-p', String(pid), '-o', 'pid=,tpgid=,tty='], + timeoutMs: PROCESS_TABLE_QUERY_TIMEOUT_MS, + maxOutputBytes: PROCESS_TABLE_MAX_BYTES + }) + if (result.code !== 0 || result.timedOut || result.outputTruncated) { + throw new Error('PTY foreground group is unavailable') } + return result.stdout } let ownRowCache: { pid: number; row: string } | null = null -/** Cache the own-TTY guard row to avoid a second synchronous ps fork on every SIGWINCH. */ -function readOwnProcessRow(currentPid: number): string { +/** Cache the own-TTY guard row to avoid a second ps fork on every SIGWINCH. */ +async function readOwnProcessRow(currentPid: number): Promise { if (ownRowCache?.pid !== currentPid) { // Failed reads remain retryable and yield no group. - ownRowCache = { pid: currentPid, row: runPs(currentPid) } + ownRowCache = { pid: currentPid, row: await runPs(currentPid) } } return ownRowCache.row } @@ -57,8 +56,8 @@ export function resetPosixPtyForegroundGroupOwnRowCache(): void { } /** macOS PID lists scan the whole table (~3.6s vs ~3ms for one PID), timing out into root-only delivery. */ -function readForegroundGroupTable(rootPid: number, currentPid: number): string { - return `${runPs(rootPid)}\n${readOwnProcessRow(currentPid)}` +async function readForegroundGroupTable(rootPid: number, currentPid: number): Promise { + return `${await runPs(rootPid)}\n${await readOwnProcessRow(currentPid)}` } function parseProcessRows(output: string): ProcessRow[] { @@ -102,13 +101,13 @@ export function getPosixPtyForegroundGroup( /** Fall back to scoped root delivery if group proof is unavailable. * Callers limit this to SIGWINCH; destructive signals require descendant identity checks. */ -export function signalPosixPtyForegroundGroup( +export async function signalPosixPtyForegroundGroup( rootPid: number, ptsName: string | undefined, signal: NodeJS.Signals | (string & {}), fallback: () => void, deps: PosixPtyForegroundGroupDeps = {} -): void { +): Promise { if ((deps.platform ?? process.platform) === 'win32' || !ptsName) { fallback() return @@ -117,7 +116,7 @@ export function signalPosixPtyForegroundGroup( let pgid: number | null try { pgid = getPosixPtyForegroundGroup( - (deps.readProcessTable ?? (() => readForegroundGroupTable(rootPid, currentPid)))(), + await (deps.readProcessTable ?? (() => readForegroundGroupTable(rootPid, currentPid)))(), rootPid, ptsName, currentPid @@ -125,6 +124,9 @@ export function signalPosixPtyForegroundGroup( } catch { pgid = null } + if (deps.ownsRoot && !deps.ownsRoot()) { + return + } if (pgid === null) { fallback() return diff --git a/src/main/pty/posix-pty-process-groups.integration.test.ts b/src/main/pty/posix-pty-process-groups.integration.test.ts index f8b191f9856..5ce3529c01e 100644 --- a/src/main/pty/posix-pty-process-groups.integration.test.ts +++ b/src/main/pty/posix-pty-process-groups.integration.test.ts @@ -73,7 +73,7 @@ itOnPosix( const rootProcessGroup = readProcessGroup(proc.pid) expect(tagged.some((process) => process.pgid !== rootProcessGroup)).toBe(true) - forceKillPosixPtyProcessGroups(proc.pid, () => proc.kill('SIGKILL')) + await forceKillPosixPtyProcessGroups(proc.pid, () => proc.kill('SIGKILL')) await Promise.race([ exited, new Promise((_, reject) => diff --git a/src/main/pty/posix-pty-process-groups.test.ts b/src/main/pty/posix-pty-process-groups.test.ts index 3772200064b..bbd9ab1d61e 100644 --- a/src/main/pty/posix-pty-process-groups.test.ts +++ b/src/main/pty/posix-pty-process-groups.test.ts @@ -1,20 +1,31 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type * as NodeFs from 'node:fs' +import type * as NodeFsPromises from 'node:fs/promises' import type { ProcessResult } from '@orca/process-host/process-spec' -const { readFileSyncMock, recordSelfInitiatedTreeKillMock, runProcessMock, runProcessSyncMock } = - vi.hoisted(() => ({ - readFileSyncMock: vi.fn(), - recordSelfInitiatedTreeKillMock: vi.fn(), - runProcessMock: vi.fn(), - runProcessSyncMock: vi.fn() - })) +const { + readFileMock, + readFileSyncMock, + recordSelfInitiatedTreeKillMock, + runProcessMock, + runProcessSyncMock +} = vi.hoisted(() => ({ + readFileMock: vi.fn(), + readFileSyncMock: vi.fn(), + recordSelfInitiatedTreeKillMock: vi.fn(), + runProcessMock: vi.fn(), + runProcessSyncMock: vi.fn() +})) vi.mock('node:fs', async (importOriginal) => ({ ...(await importOriginal()), readFileSync: readFileSyncMock })) +vi.mock('node:fs/promises', async (importOriginal) => ({ + ...(await importOriginal()), + readFile: readFileMock +})) vi.mock('../crash-reporting/self-initiated-tree-kill-log', () => ({ recordSelfInitiatedTreeKill: recordSelfInitiatedTreeKillMock })) @@ -25,13 +36,16 @@ vi.mock('@orca/process-host', () => ({ import { forceKillPosixPtyProcessGroups, + forceKillPosixPtyProcessGroupsSync, getPosixPtyProcessGroups, isPosixPtyRootStopped, - parseLinuxProcStatSessionId, - readPosixPtyProcessTable, - resetPosixPtyProcessTableDialectForTests, signalPosixPtyProcessGroups } from './posix-pty-process-groups' +import { + parseLinuxProcStatSessionId, + readPosixPtyProcessTable, + resetPosixPtyProcessTableDialectForTests +} from './posix-pty-process-table' // `/proc//stat` fixture: fields after comm are state, ppid, pgrp, session, tty_nr, ... const procStat = (pid: number, session: number, comm = 'bash'): string => @@ -41,6 +55,7 @@ const procSessions = new Map() beforeEach(() => { procSessions.clear() readFileSyncMock.mockReset() + readFileMock.mockReset() // Default fixture: each hundred-block of pids is one terminal session led by its round pid. readFileSyncMock.mockImplementation((path: string) => { const pid = Number(/^\/proc\/(\d+)\/stat$/.exec(path)?.[1]) @@ -49,6 +64,7 @@ beforeEach(() => { } return procStat(pid, procSessions.get(pid) ?? Math.floor(pid / 100) * 100) }) + readFileMock.mockImplementation(async (path: string) => readFileSyncMock(path)) recordSelfInitiatedTreeKillMock.mockReset() runProcessMock.mockReset() runProcessSyncMock.mockReset() @@ -152,6 +168,27 @@ describe('ps selection compatibility', () => { ]) }) + it('releases every waiter after a failed probe instead of chaining another probe per caller', async () => { + // A wedged ps would otherwise stack one timeout per concurrent force-kill. + const probe = Promise.withResolvers() + const queries: ((result: ProcessResult) => void)[] = [] + runProcessMock + .mockImplementationOnce(() => probe.promise) + .mockImplementation(() => new Promise((resolve) => queries.push(resolve))) + const first = readPosixPtyProcessTable(100) + const second = readPosixPtyProcessTable(200) + const third = readPosixPtyProcessTable(300) + + probe.resolve({ code: null, signal: 'SIGTERM', stdout: '', stderr: '', timedOut: true }) + await expect(first).rejects.toThrow('unavailable') + await vi.waitFor(() => expect(runProcessMock).toHaveBeenCalledTimes(3)) + + for (const resolve of queries) { + resolve({ code: 0, signal: null, stdout: '', stderr: '', timedOut: false }) + } + await expect(Promise.all([second, third])).resolves.toEqual(['', '']) + }) + it('uses the cached async dialect for synchronous teardown with fresh membership', async () => { runProcessMock .mockResolvedValueOnce(unsupportedSelection()) @@ -163,7 +200,7 @@ describe('ps selection compatibility', () => { }) const signalProcessGroup = vi.fn() const fallback = vi.fn() - forceKillPosixPtyProcessGroups(100, fallback, { + forceKillPosixPtyProcessGroupsSync(100, fallback, { platform: 'linux', currentPid: 999, signalProcessGroup @@ -179,7 +216,7 @@ describe('ps selection compatibility', () => { .mockReturnValueOnce({ code: 0, stdout: BUSYBOX_TABLE }) const signalProcessGroup = vi.fn() const fallback = vi.fn() - forceKillPosixPtyProcessGroups(100, fallback, { + forceKillPosixPtyProcessGroupsSync(100, fallback, { platform: 'linux', currentPid: 999, signalProcessGroup @@ -224,7 +261,7 @@ describe('ps selection compatibility', () => { runProcessSyncMock.mockReturnValue({ stdout: BUSYBOX_TABLE, ...failure }) const signalProcessGroup = vi.fn() const fallback = vi.fn() - forceKillPosixPtyProcessGroups(100, fallback, { + forceKillPosixPtyProcessGroupsSync(100, fallback, { platform: 'linux', currentPid: 999, signalProcessGroup @@ -334,14 +371,14 @@ describe('POSIX PTY process-group termination', () => { expect(getPosixPtyProcessGroups(TABLE, 999, 998)).toBeNull() }) - it('kills foreground and background groups before the PTY leader', () => { + it('kills foreground and background groups before the PTY leader', async () => { const fallback = vi.fn() const signalProcessGroup = vi.fn() - forceKillPosixPtyProcessGroups(100, fallback, { + await forceKillPosixPtyProcessGroups(100, fallback, { platform: 'darwin', currentPid: 999, - readProcessTable: () => TABLE, + readProcessTable: async () => TABLE, signalProcessGroup }) @@ -352,12 +389,12 @@ describe('POSIX PTY process-group termination', () => { it.each([ ['SIGSTOP', [99, 101, 103]], ['SIGCONT', [101, 103, 99]] - ] as const)('orders shell and job groups safely for %s', (signal, expected) => { + ] as const)('orders shell and job groups safely for %s', async (signal, expected) => { const signalProcessGroup = vi.fn() - signalPosixPtyProcessGroups(100, signal, vi.fn(), { + await signalPosixPtyProcessGroups(100, signal, vi.fn(), { platform: 'linux', currentPid: 999, - readProcessTable: () => TABLE.replace('100 100', '100 99'), + readProcessTable: async () => TABLE.replace('100 100', '100 99'), signalProcessGroup }) expect(signalProcessGroup.mock.calls.map(([pgid]) => pgid)).toEqual(expected) @@ -365,41 +402,38 @@ describe('POSIX PTY process-group termination', () => { it.each(['EPERM', 'ESRCH'])( 'does not stop jobs when stopping the shell fails with %s', - (code) => { + async (code) => { const error = Object.assign(new Error('stop failed'), { code }) const signalProcessGroup = vi.fn(() => { throw error }) - const stop = () => - signalPosixPtyProcessGroups(100, 'SIGSTOP', vi.fn(), { - platform: 'linux', - currentPid: 999, - readProcessTable: () => TABLE, - signalProcessGroup - }) - if (code === 'ESRCH') { - expect(stop).not.toThrow() - } else { - expect(stop).toThrow(error) - } + const stop = signalPosixPtyProcessGroups(100, 'SIGSTOP', vi.fn(), { + platform: 'linux', + currentPid: 999, + readProcessTable: async () => TABLE, + signalProcessGroup + }) + await (code === 'ESRCH' + ? expect(stop).resolves.toBeUndefined() + : expect(stop).rejects.toThrow(error)) expect(signalProcessGroup.mock.calls).toEqual([[100]]) } ) - it('falls back when the process table cannot prove PTY ownership', () => { + it('falls back when the process table cannot prove PTY ownership', async () => { const fallback = vi.fn() - forceKillPosixPtyProcessGroups(100, fallback, { + await forceKillPosixPtyProcessGroups(100, fallback, { platform: 'linux', currentPid: 102, - readProcessTable: () => TABLE, + readProcessTable: async () => TABLE, signalProcessGroup: vi.fn() }) expect(fallback).toHaveBeenCalledOnce() }) - it('ignores groups that exited after the snapshot but preserves real signal errors', () => { + it('ignores groups that exited after the snapshot but preserves real signal errors', async () => { const gone = Object.assign(new Error('gone'), { code: 'ESRCH' }) const denied = Object.assign(new Error('denied'), { code: 'EPERM' }) const signalProcessGroup = vi @@ -411,22 +445,43 @@ describe('POSIX PTY process-group termination', () => { throw denied }) - expect(() => + await expect( forceKillPosixPtyProcessGroups(100, vi.fn(), { platform: 'darwin', currentPid: 999, - readProcessTable: () => TABLE, + readProcessTable: async () => TABLE, signalProcessGroup }) - ).toThrow('denied') + ).rejects.toThrow('denied') expect(signalProcessGroup).toHaveBeenCalledTimes(3) }) - it('uses the existing fallback on Windows without reading ps', () => { + it('signals nothing when the root exits while the table is read', async () => { + // A reaped root frees its pid and TTY, so the table may now describe another terminal. + let rootExited = false const fallback = vi.fn() - const readProcessTable = vi.fn(() => TABLE) + const signalProcessGroup = vi.fn() - forceKillPosixPtyProcessGroups(100, fallback, { + await forceKillPosixPtyProcessGroups(100, fallback, { + platform: 'darwin', + currentPid: 999, + readProcessTable: async () => { + rootExited = true + return TABLE + }, + ownsRoot: () => !rootExited, + signalProcessGroup + }) + + expect(signalProcessGroup).not.toHaveBeenCalled() + expect(fallback).not.toHaveBeenCalled() + }) + + it('uses the existing fallback on Windows without reading ps', async () => { + const fallback = vi.fn() + const readProcessTable = vi.fn(async () => TABLE) + + await forceKillPosixPtyProcessGroups(100, fallback, { platform: 'win32', readProcessTable }) @@ -453,28 +508,28 @@ describe('Linux terminal session identity (#27337)', () => { expect(parseLinuxProcStatSessionId('42 (bash) S 1 42 0 0')).toBeNull() }) - it('force-kills only groups in the shell session, not container processes on the same pts', () => { + it('force-kills only groups in the shell session, not container processes on the same pts', async () => { procSessions.set(500, 500).set(501, 500).set(600, 600) const signalProcessGroup = vi.fn() const fallback = vi.fn() - forceKillPosixPtyProcessGroups(100, fallback, { + await forceKillPosixPtyProcessGroups(100, fallback, { platform: 'linux', currentPid: 999, - readProcessTable: () => COLLIDING_TABLE, + readProcessTable: async () => COLLIDING_TABLE, signalProcessGroup }) expect(signalProcessGroup.mock.calls).toEqual([[101], [100]]) expect(fallback).not.toHaveBeenCalled() - expect(readFileSyncMock).toHaveBeenCalledWith('/proc/500/stat', 'utf8') + expect(readFileMock).toHaveBeenCalledWith('/proc/500/stat', 'utf8') }) - it('applies the same session filter to the stop/continue sweep', () => { + it('applies the same session filter to the stop/continue sweep', async () => { procSessions.set(500, 500).set(501, 500).set(600, 600) const signalProcessGroup = vi.fn() - signalPosixPtyProcessGroups(100, 'SIGSTOP', vi.fn(), { + await signalPosixPtyProcessGroups(100, 'SIGSTOP', vi.fn(), { platform: 'linux', currentPid: 999, - readProcessTable: () => COLLIDING_TABLE, + readProcessTable: async () => COLLIDING_TABLE, signalProcessGroup }) expect(signalProcessGroup.mock.calls).toEqual([[100], [101]]) @@ -489,15 +544,15 @@ describe('Linux terminal session identity (#27337)', () => { }) it.each([ - ['unreadable', () => null], - ['not a session leader', () => 50] - ])('falls back to the root-only kill when the shell session is %s', (_label, sessionOf) => { + ['unreadable', async () => null], + ['not a session leader', async () => 50] + ])('falls back to the root-only kill when the shell session is %s', async (_label, sessionOf) => { const signalProcessGroup = vi.fn() const fallback = vi.fn() - forceKillPosixPtyProcessGroups(100, fallback, { + await forceKillPosixPtyProcessGroups(100, fallback, { platform: 'linux', currentPid: 999, - readProcessTable: () => COLLIDING_TABLE, + readProcessTable: async () => COLLIDING_TABLE, readProcessSessionId: sessionOf, signalProcessGroup }) @@ -505,25 +560,25 @@ describe('Linux terminal session identity (#27337)', () => { expect(signalProcessGroup).not.toHaveBeenCalled() }) - it('keeps macOS selection by terminal alone without reading /proc', () => { + it('keeps macOS selection by terminal alone without reading /proc', async () => { const signalProcessGroup = vi.fn() - forceKillPosixPtyProcessGroups(100, vi.fn(), { + await forceKillPosixPtyProcessGroups(100, vi.fn(), { platform: 'darwin', currentPid: 999, - readProcessTable: () => COLLIDING_TABLE, + readProcessTable: async () => COLLIDING_TABLE, signalProcessGroup }) expect(signalProcessGroup.mock.calls).toEqual([[101], [500], [600], [100]]) - expect(readFileSyncMock).not.toHaveBeenCalled() + expect(readFileMock).not.toHaveBeenCalled() }) }) describe('POSIX PTY group-sweep breadcrumbs', () => { - it('records every group it actually signalled', () => { - forceKillPosixPtyProcessGroups(100, vi.fn(), { + it('records every group it actually signalled', async () => { + await forceKillPosixPtyProcessGroups(100, vi.fn(), { platform: 'darwin', currentPid: 999, - readProcessTable: () => TABLE, + readProcessTable: async () => TABLE, signalProcessGroup: vi.fn() }) @@ -534,11 +589,11 @@ describe('POSIX PTY group-sweep breadcrumbs', () => { ]) }) - it('does not claim a group that was already gone', () => { - forceKillPosixPtyProcessGroups(100, vi.fn(), { + it('does not claim a group that was already gone', async () => { + await forceKillPosixPtyProcessGroups(100, vi.fn(), { platform: 'darwin', currentPid: 999, - readProcessTable: () => TABLE, + readProcessTable: async () => TABLE, signalProcessGroup: (pgid: number) => { if (pgid === 103) { throw Object.assign(new Error('no such process'), { code: 'ESRCH' }) diff --git a/src/main/pty/posix-pty-process-groups.ts b/src/main/pty/posix-pty-process-groups.ts index ec2efb7a2be..91fbe4bdee5 100644 --- a/src/main/pty/posix-pty-process-groups.ts +++ b/src/main/pty/posix-pty-process-groups.ts @@ -1,183 +1,31 @@ -import { readFileSync } from 'node:fs' import { hasControllingTty } from '../../shared/posix-controlling-terminal' import { recordSelfInitiatedTreeKill } from '../crash-reporting/self-initiated-tree-kill-log' -import { waitForPromiseWithSignal } from '../../shared/abort-signal-reason' -import { runProcess, runProcessSync } from '@orca/process-host' -import type { ProcessResult } from '@orca/process-host/process-spec' +import { + parseProcessRows, + readLinuxProcessSessionId, + readLinuxProcessSessionIdAsync, + readPosixPtyProcessTable, + readPosixPtyProcessTableSync +} from './posix-pty-process-table' -const PROCESS_TABLE_TIMEOUT_MS = 1_000 -const PROCESS_TABLE_MAX_BYTES = 1024 * 1024 -const SELECTED_COLUMNS = 'pid=,pgid=,tty=,stat=' -// Explicit widths prevent BusyBox from truncating device numbers into another terminal's identity. -const ALL_PROCESS_ARGS = [ - '-e', - '-o', - 'pid=PROCESS_ID,pgid=PROCESS_GID,tty=TERMINAL_DEVICE_NUMBER,stat=PROCESS_STATE' -] -let psDialect: 'selected' | 'all' | undefined -let dialectProbe: Promise | undefined - -class UnsupportedPsSelectionError extends Error {} - -export function resetPosixPtyProcessTableDialectForTests(): void { - psDialect = undefined - dialectProbe = undefined -} - -type ProcessRow = { - pid: number - pgid: number - tty: string - state?: string -} - -export type PosixPtyProcessGroupTerminationDeps = { +type PosixPtyGroupSignalDeps = { platform?: NodeJS.Platform currentPid?: number - readProcessTable?: () => string signalProcessGroup?: (pgid: number) => void +} + +export type PosixPtyProcessGroupTerminationDeps = PosixPtyGroupSignalDeps & { + readProcessTable?: () => string /** Linux session id of a pid, or null when unreadable. */ readProcessSessionId?: (pid: number) => number | null } -/** Field 6 (`session`) of `/proc//stat`, read past the parenthesised comm. */ -export function parseLinuxProcStatSessionId(stat: string): number | null { - const closingParen = stat.lastIndexOf(')') - if (closingParen === -1) { - return null - } - const session = Number( - stat - .slice(closingParen + 1) - .trim() - .split(/\s+/)[3] - ) - return Number.isSafeInteger(session) && session > 0 ? session : null -} - -function readLinuxProcessSessionId(pid: number): number | null { - try { - return parseLinuxProcStatSessionId(readFileSync(`/proc/${pid}/stat`, 'utf8')) - } catch { - return null - } -} - -function readProcessTableResult(result: ProcessResult): string { - if (result.code !== 0 || result.timedOut || result.outputTruncated) { - throw new Error('PTY process table is unavailable') - } - return result.stdout -} - -function readSelectionResult(result: ProcessResult, option: 'p' | 't'): string { - const rejectedOption = - /^ps: (?:invalid|illegal|unrecognized) option(?: -- |: | )['"]?-?([pt])['"]?\s*$/m.exec( - result.stderr ?? '' - )?.[1] - if ( - result.code !== null && - result.code !== 0 && - !result.signal && - !result.timedOut && - !result.outputTruncated && - rejectedOption === option - ) { - throw new UnsupportedPsSelectionError() - } - const output = readProcessTableResult(result) - if (psDialect === 'all') { - throw new UnsupportedPsSelectionError() - } - return output -} - -function* processTableQueries(rootPid: number): Generator { - if (psDialect !== 'all') { - try { - const root = readSelectionResult(yield ['-p', String(rootPid), '-o', SELECTED_COLUMNS], 'p') - const rootRow = parseProcessRows(root).find((row) => row.pid === rootPid) - if (!rootRow || !hasControllingTty(rootRow.tty)) { - return root - } - const terminal = readSelectionResult(yield ['-t', rootRow.tty, '-o', SELECTED_COLUMNS], 't') - psDialect ??= 'selected' - return `${root}\n${terminal}` - } catch (error) { - if (!(error instanceof UnsupportedPsSelectionError)) { - throw error - } - psDialect = 'all' - } - } - return readProcessTableResult(yield ALL_PROCESS_ARGS) -} - -function processTableSpec(args: string[]) { - return { - program: 'ps', - args, - env: { ...process.env, LC_ALL: 'C' }, - timeoutMs: PROCESS_TABLE_TIMEOUT_MS, - maxOutputBytes: PROCESS_TABLE_MAX_BYTES - } -} - -function readPtyProcessTable(rootPid: number): string { - const queries = processTableQueries(rootPid) - let next = queries.next() - while (!next.done) { - next = queries.next(runProcessSync(processTableSpec(next.value))) - } - return next.value -} - -export async function readPosixPtyProcessTable( - rootPid: number, - signal?: AbortSignal -): Promise { - while (dialectProbe) { - await waitForPromiseWithSignal(dialectProbe, signal) - } - signal?.throwIfAborted() - let releaseProbe: (() => void) | undefined - if (psDialect === undefined) { - dialectProbe = new Promise((resolve) => { - releaseProbe = resolve - }) - } - try { - const queries = processTableQueries(rootPid) - let next = queries.next() - while (!next.done) { - signal?.throwIfAborted() - const result = await runProcess({ ...processTableSpec(next.value), signal }) - signal?.throwIfAborted() - next = queries.next(result) - } - return next.value - } finally { - if (releaseProbe) { - dialectProbe = undefined - releaseProbe() - } - } -} - -function parseProcessRows(output: string): ProcessRow[] { - const rows: ProcessRow[] = [] - for (const line of output.split(/\r?\n/)) { - const match = /^\s*(\d+)\s+(\d+)\s+(\S+)(?:\s+(\S+))?/.exec(line) - if (!match) { - continue - } - const pid = Number(match[1]) - const pgid = Number(match[2]) - if (pid > 0 && pgid > 1) { - rows.push({ pid, pgid, tty: match[3], state: match[4] }) - } - } - return rows +export type PosixPtyProcessGroupSignalDeps = PosixPtyGroupSignalDeps & { + readProcessTable?: () => Promise + /** Linux session id of a pid, or null when unreadable. */ + readProcessSessionId?: (pid: number) => Promise + /** False once the root has exited; its pid and TTY may then belong to another terminal. */ + ownsRoot?: () => boolean } export function isPosixPtyRootStopped(output: string, rootPid: number): boolean { @@ -230,17 +78,58 @@ function isProcessAlreadyGone(error: unknown): boolean { return (error as NodeJS.ErrnoException | undefined)?.code === 'ESRCH' } -/** Force-kill every process group still attached to one POSIX PTY. */ +/** Force-kill every process group still attached to one POSIX PTY without blocking the event loop. */ export function forceKillPosixPtyProcessGroups( + rootPid: number, + fallback: () => void, + deps: PosixPtyProcessGroupSignalDeps = {} +): Promise { + return signalPosixPtyProcessGroups(rootPid, 'SIGKILL', fallback, deps) +} + +/** Blocking variant for synchronous in-process teardown such as app quit. */ +export function forceKillPosixPtyProcessGroupsSync( rootPid: number, fallback: () => void, deps: PosixPtyProcessGroupTerminationDeps = {} ): void { - signalPosixPtyProcessGroups(rootPid, 'SIGKILL', fallback, deps) + signalPosixPtyProcessGroupsSync(rootPid, 'SIGKILL', fallback, deps) } /** Signal every process group proven to belong to one POSIX PTY. */ -export function signalPosixPtyProcessGroups( +export async function signalPosixPtyProcessGroups( + rootPid: number, + signal: NodeJS.Signals, + fallback: () => void, + deps: PosixPtyProcessGroupSignalDeps = {} +): Promise { + const platform = deps.platform ?? process.platform + if (platform === 'win32') { + fallback() + return + } + let groups: number[] | null + try { + const table = await (deps.readProcessTable ?? (() => readPosixPtyProcessTable(rootPid)))() + const readSessionId = + deps.readProcessSessionId ?? + (platform === 'linux' ? readLinuxProcessSessionIdAsync : undefined) + groups = getPosixPtyProcessGroups( + table, + rootPid, + deps.currentPid ?? process.pid, + readSessionId ? await readSessionIds(table, rootPid, readSessionId) : undefined + ) + } catch { + groups = null + } + if (deps.ownsRoot && !deps.ownsRoot()) { + return + } + deliverPosixPtyGroupSignal(groups, signal, fallback, deps) +} + +function signalPosixPtyProcessGroupsSync( rootPid: number, signal: NodeJS.Signals, fallback: () => void, @@ -254,7 +143,7 @@ export function signalPosixPtyProcessGroups( let groups: number[] | null try { groups = getPosixPtyProcessGroups( - (deps.readProcessTable ?? (() => readPtyProcessTable(rootPid)))(), + (deps.readProcessTable ?? (() => readPosixPtyProcessTableSync(rootPid)))(), rootPid, deps.currentPid ?? process.pid, deps.readProcessSessionId ?? (platform === 'linux' ? readLinuxProcessSessionId : undefined) @@ -262,6 +151,31 @@ export function signalPosixPtyProcessGroups( } catch { groups = null } + deliverPosixPtyGroupSignal(groups, signal, fallback, deps) +} + +/** Prefetch session ids for the root's terminal so group selection stays synchronous. */ +async function readSessionIds( + table: string, + rootPid: number, + readSessionId: (pid: number) => Promise +): Promise<(pid: number) => number | null> { + const rows = parseProcessRows(table) + const rootTty = rows.find((row) => row.pid === rootPid)?.tty + // The root's own row carries rootTty, so an absent root reads nothing. + const pids = rows.filter((row) => rootTty !== undefined && row.tty === rootTty) + const sessions = new Map( + await Promise.all(pids.map(async ({ pid }) => [pid, await readSessionId(pid)] as const)) + ) + return (pid) => sessions.get(pid) ?? null +} + +function deliverPosixPtyGroupSignal( + groups: number[] | null, + signal: NodeJS.Signals, + fallback: () => void, + deps: PosixPtyGroupSignalDeps +): void { if (!groups || groups.length === 0) { fallback() return diff --git a/src/main/pty/posix-pty-process-table-event-loop.test.ts b/src/main/pty/posix-pty-process-table-event-loop.test.ts new file mode 100644 index 00000000000..ee6d9646bef --- /dev/null +++ b/src/main/pty/posix-pty-process-table-event-loop.test.ts @@ -0,0 +1,52 @@ +import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterAll, beforeAll, describe, expect, it } from 'vitest' +import { signalPosixPtyForegroundGroup } from './posix-pty-foreground-group' +import { forceKillPosixPtyProcessGroups } from './posix-pty-process-groups' + +const describeOnPosix = process.platform === 'win32' ? describe.skip : describe + +// A wedged ps must time out off the event loop; the daemon froze while kill and resize waited on it (#34410). +describeOnPosix('PTY process-table reads with a ps that never exits', () => { + let stubDir = '' + let originalPath: string | undefined + + beforeAll(() => { + stubDir = mkdtempSync(join(tmpdir(), 'orca-hung-ps-')) + writeFileSync(join(stubDir, 'ps'), '#!/bin/sh\nexec sleep 30\n') + chmodSync(join(stubDir, 'ps'), 0o755) + originalPath = process.env.PATH + process.env.PATH = `${stubDir}:${originalPath ?? ''}` + }) + + afterAll(() => { + process.env.PATH = originalPath + rmSync(stubDir, { recursive: true, force: true }) + }) + + async function recordOrder(run: (fallback: () => void) => unknown): Promise { + const order: string[] = [] + const timer = new Promise((resolve) => + setTimeout(() => { + order.push('timer') + resolve() + }, 0) + ) + await run(() => order.push('fallback')) + await timer + return order + } + + it('lets a concurrent timer run while force-kill waits for the process table', async () => { + const order = await recordOrder((fallback) => forceKillPosixPtyProcessGroups(999_999, fallback)) + expect(order).toEqual(['timer', 'fallback']) + }) + + it('lets a concurrent timer run while resize waits for the foreground group', async () => { + const order = await recordOrder((fallback) => + signalPosixPtyForegroundGroup(999_999, '/dev/ttys999', 'SIGWINCH', fallback) + ) + expect(order).toEqual(['timer', 'fallback']) + }) +}) diff --git a/src/main/pty/posix-pty-process-table.ts b/src/main/pty/posix-pty-process-table.ts new file mode 100644 index 00000000000..db3463c9ea8 --- /dev/null +++ b/src/main/pty/posix-pty-process-table.ts @@ -0,0 +1,181 @@ +import { readFileSync } from 'node:fs' +import { readFile } from 'node:fs/promises' +import { hasControllingTty } from '../../shared/posix-controlling-terminal' +import { waitForPromiseWithSignal } from '../../shared/abort-signal-reason' +import { runProcess, runProcessSync } from '@orca/process-host' +import type { ProcessResult } from '@orca/process-host/process-spec' + +const PROCESS_TABLE_TIMEOUT_MS = 1_000 +const PROCESS_TABLE_MAX_BYTES = 1024 * 1024 +const SELECTED_COLUMNS = 'pid=,pgid=,tty=,stat=' +// Explicit widths prevent BusyBox from truncating device numbers into another terminal's identity. +const ALL_PROCESS_ARGS = [ + '-e', + '-o', + 'pid=PROCESS_ID,pgid=PROCESS_GID,tty=TERMINAL_DEVICE_NUMBER,stat=PROCESS_STATE' +] +let psDialect: 'selected' | 'all' | undefined +let dialectProbe: Promise | undefined + +class UnsupportedPsSelectionError extends Error {} + +export function resetPosixPtyProcessTableDialectForTests(): void { + psDialect = undefined + dialectProbe = undefined +} + +type ProcessRow = { + pid: number + pgid: number + tty: string + state?: string +} + +/** Field 6 (`session`) of `/proc//stat`, read past the parenthesised comm. */ +export function parseLinuxProcStatSessionId(stat: string): number | null { + const closingParen = stat.lastIndexOf(')') + if (closingParen === -1) { + return null + } + const session = Number( + stat + .slice(closingParen + 1) + .trim() + .split(/\s+/)[3] + ) + return Number.isSafeInteger(session) && session > 0 ? session : null +} + +export function readLinuxProcessSessionId(pid: number): number | null { + try { + return parseLinuxProcStatSessionId(readFileSync(`/proc/${pid}/stat`, 'utf8')) + } catch { + return null + } +} + +export async function readLinuxProcessSessionIdAsync(pid: number): Promise { + try { + return parseLinuxProcStatSessionId(await readFile(`/proc/${pid}/stat`, 'utf8')) + } catch { + return null + } +} + +function readProcessTableResult(result: ProcessResult): string { + if (result.code !== 0 || result.timedOut || result.outputTruncated) { + throw new Error('PTY process table is unavailable') + } + return result.stdout +} + +function readSelectionResult(result: ProcessResult, option: 'p' | 't'): string { + const rejectedOption = + /^ps: (?:invalid|illegal|unrecognized) option(?: -- |: | )['"]?-?([pt])['"]?\s*$/m.exec( + result.stderr ?? '' + )?.[1] + if ( + result.code !== null && + result.code !== 0 && + !result.signal && + !result.timedOut && + !result.outputTruncated && + rejectedOption === option + ) { + throw new UnsupportedPsSelectionError() + } + const output = readProcessTableResult(result) + if (psDialect === 'all') { + throw new UnsupportedPsSelectionError() + } + return output +} + +function* processTableQueries(rootPid: number): Generator { + if (psDialect !== 'all') { + try { + const root = readSelectionResult(yield ['-p', String(rootPid), '-o', SELECTED_COLUMNS], 'p') + const rootRow = parseProcessRows(root).find((row) => row.pid === rootPid) + if (!rootRow || !hasControllingTty(rootRow.tty)) { + return root + } + const terminal = readSelectionResult(yield ['-t', rootRow.tty, '-o', SELECTED_COLUMNS], 't') + psDialect ??= 'selected' + return `${root}\n${terminal}` + } catch (error) { + if (!(error instanceof UnsupportedPsSelectionError)) { + throw error + } + psDialect = 'all' + } + } + return readProcessTableResult(yield ALL_PROCESS_ARGS) +} + +function processTableSpec(args: string[]) { + return { + program: 'ps', + args, + env: { ...process.env, LC_ALL: 'C' }, + timeoutMs: PROCESS_TABLE_TIMEOUT_MS, + maxOutputBytes: PROCESS_TABLE_MAX_BYTES + } +} + +export function readPosixPtyProcessTableSync(rootPid: number): string { + const queries = processTableQueries(rootPid) + let next = queries.next() + while (!next.done) { + next = queries.next(runProcessSync(processTableSpec(next.value))) + } + return next.value +} + +export async function readPosixPtyProcessTable( + rootPid: number, + signal?: AbortSignal +): Promise { + // Wait out one probe only: chaining behind each failed probe would stack their timeouts. + if (dialectProbe) { + await waitForPromiseWithSignal(dialectProbe, signal) + } + signal?.throwIfAborted() + let releaseProbe: (() => void) | undefined + if (psDialect === undefined && !dialectProbe) { + dialectProbe = new Promise((resolve) => { + releaseProbe = resolve + }) + } + try { + const queries = processTableQueries(rootPid) + let next = queries.next() + while (!next.done) { + signal?.throwIfAborted() + const result = await runProcess({ ...processTableSpec(next.value), signal }) + signal?.throwIfAborted() + next = queries.next(result) + } + return next.value + } finally { + if (releaseProbe) { + dialectProbe = undefined + releaseProbe() + } + } +} + +export function parseProcessRows(output: string): ProcessRow[] { + const rows: ProcessRow[] = [] + for (const line of output.split(/\r?\n/)) { + const match = /^\s*(\d+)\s+(\d+)\s+(\S+)(?:\s+(\S+))?/.exec(line) + if (!match) { + continue + } + const pid = Number(match[1]) + const pgid = Number(match[2]) + if (pid > 0 && pgid > 1) { + rows.push({ pid, pgid, tty: match[3], state: match[4] }) + } + } + return rows +}