mirror of
https://github.com/stablyai/orca.git
synced 2026-10-21 16:03:26 +00:00
fix(terminal): deliver SIGWINCH to the PTY foreground process group
A real window-size change is delivered by the kernel to the terminal's foreground process group, but Orca signalled the pty's root pid. The root is never in that group: the shell setpgid's away for job control, a foreground TUI forms a third group again, and on macOS the root is login(1), which neither handles nor forwards SIGWINCH. So the explicit reattach repaint signal reached a running TUI on no platform, and on macOS reached nothing at all. Resolve the tty's tpgid and signal the negated group instead, pinned to the slave device captured at spawn so a recycled pid cannot aim at a real terminal. Scoped to SIGWINCH: destructive signals keep the narrower root-pid target and the descendant-sweep identity machinery. POSIX-only, so Windows keeps its current behavior where a negative pid is invalid and SIGTERM means terminate. Applied on the relay too, so SSH hosts get it; that is host-local with no wire change.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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<string> {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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 <pid> -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()
|
||||
}
|
||||
})
|
||||
})
|
||||
@@ -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.
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user