diff --git a/src/main/daemon/pty-subprocess.ts b/src/main/daemon/pty-subprocess.ts index 14fb0b5aa7b..6fe39f57fba 100644 --- a/src/main/daemon/pty-subprocess.ts +++ b/src/main/daemon/pty-subprocess.ts @@ -17,6 +17,8 @@ import { validateWorkingDirectory } from '../providers/local-pty-utils' import { wrapShellSpawnForMacosTccAttribution } from '../providers/macos-tcc-login-shell' +import { signalPosixPtyForegroundGroup } from '../pty/posix-pty-foreground-group' +import { readPtsName } from '../pty/node-pty-pts-name' import { resolveWindowsShellLaunchArgs } from '../providers/windows-shell-args' import { resolveEffectiveWindowsPowerShell, @@ -1239,11 +1241,22 @@ export function createPtySubprocess(opts: PtySubprocessOptions): SubprocessHandl if (dead) { return } - try { - process.kill(proc.pid, sig) - } catch { - // Process may already be dead + const signalRootPid = (): void => { + try { + process.kill(proc.pid, sig) + } catch { + // Process may already be dead + } } + // Why only SIGWINCH: the kernel delivers a real resize to the tty's foreground + // group, and proc.pid is never in it (the shell setpgid's away, and on macOS + // proc.pid is login(1), which drops SIGWINCH). Destructive signals keep the + // narrower root-pid target. + if (sig === 'SIGWINCH') { + signalPosixPtyForegroundGroup(proc.pid, readPtsName(proc), sig, signalRootPid) + return + } + signalRootPid() }, onData: (cb) => { onDataCb = cb diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index 34d2485d776..61146a373b3 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -67,6 +67,8 @@ import { PhysicalExitTracker } from '../../shared/physical-exit-tracker' import { mergeGitConfigEnvProtocol } from '../../shared/git-credential-prompt-env' import { PtyStartupIngress, type PtyIngressEmission } from '../../shared/pty-startup-ingress' import { resolvePtyOwnerBackend } from '../../shared/pty-owner-backend' +import { signalPosixPtyForegroundGroup } from '../pty/posix-pty-foreground-group' +import { readPtsName } from '../pty/node-pty-pts-name' import { createPtySlaveEchoProbe, readPtySlavePath @@ -1207,11 +1209,20 @@ export class LocalPtyProvider implements IPtyProvider { if (!proc) { return } - try { - process.kill(proc.pid, signal) - } catch { - /* Process may already be dead */ + const signalRootPid = (): void => { + try { + process.kill(proc.pid, signal) + } catch { + /* Process may already be dead */ + } } + // Why only SIGWINCH: see posix-pty-foreground-group — a real resize reaches the + // tty's foreground group, which proc.pid is never a member of. + if (signal === 'SIGWINCH') { + signalPosixPtyForegroundGroup(proc.pid, readPtsName(proc), signal, signalRootPid) + return + } + signalRootPid() } async getCwd(id: string): Promise { diff --git a/src/main/pty/node-pty-pts-name.ts b/src/main/pty/node-pty-pts-name.ts new file mode 100644 index 00000000000..c3c4d441ceb --- /dev/null +++ b/src/main/pty/node-pty-pts-name.ts @@ -0,0 +1,10 @@ +/** + * node-pty's unix terminal exposes the slave device path (`/dev/ttys003`, + * `/dev/pts/3`) but does not declare it on IPty, and the Windows terminal has no + * equivalent. Read it defensively so callers get `undefined` rather than a throw + * on a platform or version that lacks it. + */ +export function readPtsName(proc: unknown): string | undefined { + const value = (proc as { ptsName?: unknown } | null | undefined)?.ptsName + return typeof value === 'string' && value.length > 0 ? value : undefined +} diff --git a/src/main/pty/posix-pty-foreground-group.test.ts b/src/main/pty/posix-pty-foreground-group.test.ts new file mode 100644 index 00000000000..78b5ab51637 --- /dev/null +++ b/src/main/pty/posix-pty-foreground-group.test.ts @@ -0,0 +1,150 @@ +import { describe, expect, it, vi } from 'vitest' +import { execFileSync } from 'node:child_process' +import { + getPosixPtyForegroundGroup, + signalPosixPtyForegroundGroup +} from './posix-pty-foreground-group' + +vi.mock('node:child_process', () => ({ execFileSync: vi.fn(() => '') })) + +// The concatenated output of two `ps -p -o pid=,tpgid=,tty=` calls. +const macosTable = ['84644 84985 ttys318', ' 4242 4242 ttys002'].join('\n') + +describe('getPosixPtyForegroundGroup', () => { + it('returns the tty foreground group, not the root pid group', () => { + // Why: on macOS the root is login(1) and the shell setpgid's into its own group, + // so the root pid is never a member of the group a real resize signals. + expect(getPosixPtyForegroundGroup(macosTable, 84644, '/dev/ttys318', 4242)).toBe(84985) + }) + + it('accepts a Linux pts device name', () => { + expect(getPosixPtyForegroundGroup('900 931 pts/3', 900, '/dev/pts/3', 4242)).toBe(931) + }) + + it('refuses when the pid now answers for a different tty', () => { + // Why: `ps -p` answers for whoever owns the pid now — a recycled pid must not + // aim a group signal at a real terminal. + expect(getPosixPtyForegroundGroup(macosTable, 84644, '/dev/ttys999', 4242)).toBeNull() + }) + + it('refuses when this process shares the PTY', () => { + // Why: a dev daemon can inherit its launch TTY; group-signalling would hit Orca. + const shared = ['84644 84985 ttys318', '4242 84985 ttys318'].join('\n') + expect(getPosixPtyForegroundGroup(shared, 84644, '/dev/ttys318', 4242)).toBeNull() + }) + + it('refuses a detached tty or an unowned foreground group', () => { + expect(getPosixPtyForegroundGroup('84644 84985 ??', 84644, '/dev/ttys318', 4242)).toBeNull() + expect(getPosixPtyForegroundGroup('84644 -1 ttys318', 84644, '/dev/ttys318', 4242)).toBeNull() + expect(getPosixPtyForegroundGroup('84644 1 ttys318', 84644, '/dev/ttys318', 4242)).toBeNull() + }) + + it('refuses when the root pid is absent from the table', () => { + expect(getPosixPtyForegroundGroup('4242 4242 ttys002', 84644, '/dev/ttys318', 4242)).toBeNull() + }) +}) + +describe('signalPosixPtyForegroundGroup', () => { + const table = (): string => macosTable + + it('signals the negated foreground group', () => { + const kill = vi.spyOn(process, 'kill').mockImplementation(() => true) + const fallback = vi.fn() + try { + signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', fallback, { + platform: 'darwin', + currentPid: 4242, + readProcessTable: table + }) + expect(kill).toHaveBeenCalledWith(-84985, 'SIGWINCH') + expect(fallback).not.toHaveBeenCalled() + } finally { + kill.mockRestore() + } + }) + + it('falls back to the root pid on Windows', () => { + // Why: a negative pid is invalid there, and SIGTERM/SIGINT mean terminate. + const fallback = vi.fn() + signalPosixPtyForegroundGroup(84644, undefined, 'SIGWINCH', fallback, { + platform: 'win32', + currentPid: 4242, + readProcessTable: table + }) + expect(fallback).toHaveBeenCalledOnce() + }) + + it('falls back when the slave device name is unknown', () => { + const fallback = vi.fn() + signalPosixPtyForegroundGroup(84644, undefined, 'SIGWINCH', fallback, { + platform: 'darwin', + currentPid: 4242, + readProcessTable: table + }) + expect(fallback).toHaveBeenCalledOnce() + }) + + it('falls back when the process table cannot be read', () => { + const fallback = vi.fn() + signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', fallback, { + platform: 'darwin', + currentPid: 4242, + readProcessTable: () => { + throw new Error('ps timed out') + } + }) + expect(fallback).toHaveBeenCalledOnce() + }) + + it('treats a vanished foreground group as done rather than falling back', () => { + // Why: the fallback target is just as stale, and re-signalling a recycled pid is worse. + const kill = vi.spyOn(process, 'kill').mockImplementation(() => { + const error = new Error('no such process') as NodeJS.ErrnoException + error.code = 'ESRCH' + throw error + }) + const fallback = vi.fn() + try { + signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', fallback, { + platform: 'darwin', + currentPid: 4242, + readProcessTable: table + }) + expect(fallback).not.toHaveBeenCalled() + } finally { + kill.mockRestore() + } + }) +}) + +describe('process table lookup', () => { + it('asks ps for one pid at a time', () => { + // Why pinned: macOS ps only takes its by-pid fast path for a SINGLE pid. Any list + // form walks the whole process table (~3.6s on a busy machine vs ~3ms), which + // exceeds this module's timeout and silently reinstates root-pid delivery — the + // exact bug it exists to fix. Regressing this looks like a working feature. + const execFileSyncMock = vi.mocked(execFileSync) + execFileSyncMock.mockClear() + execFileSyncMock.mockReturnValue('84644 84985 ttys318' as never) + // Why stubbed: the resolver succeeds against the mocked table, so a live + // process.kill would aim a real group signal at whatever owns pgid 84985 here. + const kill = vi.spyOn(process, 'kill').mockImplementation(() => true) + + try { + signalPosixPtyForegroundGroup(84644, '/dev/ttys318', 'SIGWINCH', vi.fn(), { + platform: 'darwin', + currentPid: 4242 + }) + + expect(execFileSyncMock).toHaveBeenCalled() + for (const call of execFileSyncMock.mock.calls) { + const args = call[1] as string[] + const pidArg = args[args.indexOf('-p') + 1] + expect(pidArg).toMatch(/^\d+$/) + expect(args.filter((arg) => arg === '-p')).toHaveLength(1) + } + } finally { + kill.mockRestore() + } + }) +}) diff --git a/src/main/pty/posix-pty-foreground-group.ts b/src/main/pty/posix-pty-foreground-group.ts new file mode 100644 index 00000000000..5fff740903c --- /dev/null +++ b/src/main/pty/posix-pty-foreground-group.ts @@ -0,0 +1,135 @@ +import { execFileSync } from 'node:child_process' + +const PROCESS_TABLE_TIMEOUT_MS = 250 +const PROCESS_TABLE_MAX_BYTES = 64 * 1024 + +export type PosixPtyForegroundGroupDeps = { + platform?: NodeJS.Platform + currentPid?: number + readProcessTable?: () => string +} + +type ProcessRow = { + pid: number + tpgid: number + tty: string +} + +/** `ps` prints `ttys003` / `pts/3`; node-pty reports `/dev/ttys003` / `/dev/pts/3`. */ +function normalizeTty(value: string): string { + return value.replace(/^\/dev\//, '') +} + +function hasUsableTty(tty: string): boolean { + return tty !== '?' && tty !== '??' && tty !== '-' +} + +function runPs(pid: number): string { + return execFileSync('ps', ['-p', String(pid), '-o', 'pid=,tpgid=,tty='], { + encoding: 'utf8', + timeout: PROCESS_TABLE_TIMEOUT_MS, + maxBuffer: PROCESS_TABLE_MAX_BYTES + }) +} + +/** + * Why two calls instead of `-p a,b`: macOS `ps` only takes the KERN_PROC_PID fast + * path for a single pid. ANY pid list — even a duplicate of one pid — walks the + * whole process table, measured at ~3.6s on a busy machine versus ~3ms here. That + * blew the timeout below, so the group lookup silently fell back to the very + * root-pid delivery this module exists to replace. + */ +function readForegroundGroupTable(rootPid: number, currentPid: number): string { + return `${runPs(rootPid)}\n${runPs(currentPid)}` +} + +function parseProcessRows(output: string): ProcessRow[] { + const rows: ProcessRow[] = [] + for (const line of output.split(/\r?\n/)) { + const match = /^\s*(\d+)\s+(-?\d+)\s+(\S+)/.exec(line) + if (!match) { + continue + } + const pid = Number(match[1]) + if (pid > 0) { + rows.push({ pid, tpgid: Number(match[2]), tty: match[3] }) + } + } + return rows +} + +/** + * Resolves the foreground process group of one PTY, which is who the kernel + * signals on a real window-size change. + * + * Why not the root pid: the shell calls `setpgid` for job control, so it leaves + * the root's group immediately, and on macOS the root is `login(1)` — which + * neither handles nor forwards SIGWINCH. A foreground TUI is a third group + * again, so only the tty's `tpgid` reproduces a kernel resize. + */ +export function getPosixPtyForegroundGroup( + output: string, + rootPid: number, + ptsName: string, + currentPid = process.pid +): number | null { + const rows = parseProcessRows(output) + const root = rows.find((row) => row.pid === rootPid) + if (!root || !hasUsableTty(root.tty)) { + return null + } + // Why: `ps -p` answers for whatever owns the pid now. Without pinning the tty we + // captured at spawn, a recycled pid could aim a group signal at a real terminal. + if (normalizeTty(root.tty) !== normalizeTty(ptsName)) { + return null + } + // Why: a development daemon can inherit its launch TTY. Never group-signal when + // this process shares the PTY; fall back to the already-scoped root signal. + if (rows.some((row) => row.pid === currentPid && row.tty === root.tty)) { + return null + } + // tpgid is -1 when no foreground group owns the tty, and pid 1 is never one. + return root.tpgid > 1 ? root.tpgid : null +} + +/** + * Sends `signal` to the PTY's foreground process group, falling back to the + * supplied root-pid delivery when the group cannot be established safely. + * + * Scoped to SIGWINCH by callers on purpose: a destructive signal must keep the + * narrower root-pid target plus the descendant-sweep identity machinery. + */ +export function signalPosixPtyForegroundGroup( + rootPid: number, + ptsName: string | undefined, + signal: NodeJS.Signals | string, + fallback: () => void, + deps: PosixPtyForegroundGroupDeps = {} +): void { + if ((deps.platform ?? process.platform) === 'win32' || !ptsName) { + fallback() + return + } + const currentPid = deps.currentPid ?? process.pid + let pgid: number | null + try { + pgid = getPosixPtyForegroundGroup( + (deps.readProcessTable ?? (() => readForegroundGroupTable(rootPid, currentPid)))(), + rootPid, + ptsName, + currentPid + ) + } catch { + pgid = null + } + if (pgid === null) { + fallback() + return + } + try { + process.kill(-pgid, signal as NodeJS.Signals) + } catch { + // Why: the group can exit between `ps` and `kill`. The fallback would be just + // as stale, so a vanished foreground group is success, not a reason to retry. + } +} diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index 6e8116158b6..4d4aa3ae06f 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -56,6 +56,8 @@ import { ClaimedAgentPtyOwnerRegistry } from '../shared/claimed-agent-pty-owner' import type { RelayPtySourceOutput } from './relay-pty-source-output' +import { signalPosixPtyForegroundGroup } from '../main/pty/posix-pty-foreground-group' +import { readPtsName } from '../main/pty/node-pty-pts-name' import type { RelayPtySourcePublication } from './relay-pty-source-publication' import type { PtySourceRecoveryRequest, @@ -1743,6 +1745,16 @@ export class PtyHandler { if (!managed || managed.disposed) { throw new Error(`PTY "${id}" not found`) } + // Why only SIGWINCH: a real resize reaches the tty's foreground process group, + // and node-pty's kill targets the root pid, which the shell setpgid's away from. + // Host-local behavior only — no wire change, so an older client simply gets a + // SIGWINCH that now lands. Destructive signals keep node-pty's own path. + if (signal === 'SIGWINCH') { + signalPosixPtyForegroundGroup(managed.pty.pid, readPtsName(managed.pty), signal, () => { + managed.pty.kill(signal) + }) + return + } managed.pty.kill(signal) }