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 +}