Files
orca/src/relay/pty-handler.test.ts
T
Jinwoo HongandOrca a638215729 fix(pty): release ptmx fd on natural exit + defuse SIGHUP-to-recycled-pid (#1327)
* fix(pty): release ptmx fd on natural exit + defuse SIGHUP-to-recycled-pid

Daemons accumulated ptmx fds over time because node-pty's UnixTerminal
only releases the master fd when destroy() runs. On the natural-exit
path (the common case — user closes a tab, shell runs `exit`) nothing
ever calls destroy(), so the fd leaks until GC. On macOS this
eventually hits kern.tty.ptmx_max=511 and all new terminals fail to
spawn.

Fix: release the fd synchronously on every teardown path (natural
exit, explicit kill, stale SSH spawn, daemon shutdown) and close the
concurrent SIGHUP-to-recycled-pid hazard inside node-pty's
UnixTerminal.destroy().

- src/main/daemon/pty-subprocess.ts: synchronous POSIX proc.kill
  neutralization inside proc.onExit; dead guards on forceKill/signal
  so they never target a reaped-and-possibly-recycled pid
- src/main/daemon/session.ts: new disposeSubprocess() for already-
  exited sessions (fd release only, no SIGKILL) — avoids sending
  SIGKILL to a recycled pid during daemon shutdown
- src/main/daemon/terminal-host.ts: dispose loop routes on isAlive —
  live sessions get forceKillAndDisposeSubprocess (SIGKILL + fd
  release), exited sessions get disposeSubprocess (fd release only)
- src/main/providers/local-pty-provider.ts: same POSIX kill
  neutralization at top of onExit for the legacy local path
- src/relay/pty-handler.ts: same neutralization in wireAndStore;
  disposed flag guards all public entry points; dispose() uses
  SIGKILL (not SIGTERM) before destroy since the relay is exiting;
  killTimer fallback + immediate-shutdown + stale-spawn cleanup all
  call disposeManagedPty + ptys.delete so wedged children (D-state,
  bad NFS) can't leak map entries against the 50-PTY cap

Windows is exempt everywhere — WindowsTerminal.destroy IS a kill()
call internally (closes the ConPTY agent), so neutralizing would
turn destroy into a no-op and leak the agent.

See docs/fix-pty-fd-leak.md for the full design.

Co-authored-by: Orca <help@stably.ai>

* fix(pty): patch node-pty native off-by-one leaking /dev/ptmx per spawn

node-pty 1.1.0's pty_posix_spawn on macOS walks low_fds[0..2] in an
allocation loop that breaks at the first fd >= STDERR_FILENO, then
cleans up via `for (; count > 0; count--) close(low_fds[count])`. In
the typical case (break at count=0) the cleanup body never runs and
low_fds[0] — a /dev/ptmx handle — leaks per spawn. Fixed upstream in
microsoft/node-pty af053f2 (PR #882), not in any 1.1.0 release.

Backport the 3-line cleanup-loop fix as a pnpm patch. E2E validated
against a dev daemon: 200 spawn/kill cycles kept the daemon's ptmx
fd count flat at baseline; prior runs reproduced linear 1-per-spawn
growth. Also documents the native root cause as a status addendum in
docs/fix-pty-fd-leak.md — the JS-side destroy() discipline previously
landed is still load-bearing for the SIGHUP-to-recycled-pid hazard and
for synchronous fd release on daemon shutdown.

Co-authored-by: Orca <help@stably.ai>

* fix(pty): capture stable kill spy ref in pty.test.ts

destroyPtyProcess reassigns proc.kill = () => {} on POSIX to defuse
the SIGHUP-to-recycled-pid hazard (see docs/fix-pty-fd-leak.md). After
that reassignment, proc.kill.mock is undefined and the assertions
crashed in CI. Capture a stable reference to the vi.fn() before it
gets reassigned.

Co-authored-by: Orca <help@stably.ai>

---------

Co-authored-by: Orca <help@stably.ai>
2026-05-02 23:12:14 -07:00

434 lines
13 KiB
TypeScript

/* oxlint-disable max-lines */
import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'
const { mockPtySpawn, mockPtyInstance } = vi.hoisted(() => ({
mockPtySpawn: vi.fn(),
mockPtyInstance: {
pid: 12345,
onData: vi.fn(),
onExit: vi.fn(),
write: vi.fn(),
resize: vi.fn(),
kill: vi.fn(),
clear: vi.fn()
}
}))
vi.mock('node-pty', () => ({
spawn: mockPtySpawn
}))
import { PtyHandler } from './pty-handler'
import type { RelayDispatcher } from './dispatcher'
function createMockDispatcher() {
const requestHandlers = new Map<
string,
(params: Record<string, unknown>, context?: { isStale: () => boolean }) => Promise<unknown>
>()
const notificationHandlers = new Map<string, (params: Record<string, unknown>) => void>()
const notifications: { method: string; params?: Record<string, unknown> }[] = []
const dispatcher = {
onRequest: vi.fn(
(
method: string,
handler: (
params: Record<string, unknown>,
context?: { isStale: () => boolean }
) => Promise<unknown>
) => {
requestHandlers.set(method, handler)
}
),
onNotification: vi.fn((method: string, handler: (params: Record<string, unknown>) => void) => {
notificationHandlers.set(method, handler)
}),
notify: vi.fn((method: string, params?: Record<string, unknown>) => {
notifications.push({ method, params })
}),
// Helpers for tests
_requestHandlers: requestHandlers,
_notificationHandlers: notificationHandlers,
_notifications: notifications,
async callRequest(
method: string,
params: Record<string, unknown> = {},
context?: { isStale: () => boolean }
) {
const handler = requestHandlers.get(method)
if (!handler) {
throw new Error(`No handler for ${method}`)
}
return handler(params, context)
},
callNotification(method: string, params: Record<string, unknown> = {}) {
const handler = notificationHandlers.get(method)
if (!handler) {
throw new Error(`No handler for ${method}`)
}
handler(params)
}
}
return dispatcher
}
describe('PtyHandler', () => {
let dispatcher: ReturnType<typeof createMockDispatcher>
let handler: PtyHandler
beforeEach(() => {
vi.useFakeTimers()
mockPtySpawn.mockReset()
mockPtyInstance.onData.mockReset()
mockPtyInstance.onExit.mockReset()
mockPtyInstance.write.mockReset()
mockPtyInstance.resize.mockReset()
mockPtyInstance.kill.mockReset()
mockPtyInstance.clear.mockReset()
mockPtySpawn.mockReturnValue({ ...mockPtyInstance })
dispatcher = createMockDispatcher()
handler = new PtyHandler(dispatcher as unknown as RelayDispatcher)
})
afterEach(() => {
handler.dispose()
vi.useRealTimers()
})
it('registers all expected handlers', () => {
const methods = Array.from(dispatcher._requestHandlers.keys())
expect(methods).toContain('pty.spawn')
expect(methods).toContain('pty.attach')
expect(methods).toContain('pty.shutdown')
expect(methods).toContain('pty.sendSignal')
expect(methods).toContain('pty.getCwd')
expect(methods).toContain('pty.getInitialCwd')
expect(methods).toContain('pty.clearBuffer')
expect(methods).toContain('pty.hasChildProcesses')
expect(methods).toContain('pty.getForegroundProcess')
expect(methods).toContain('pty.listProcesses')
expect(methods).toContain('pty.getDefaultShell')
const notifMethods = Array.from(dispatcher._notificationHandlers.keys())
expect(notifMethods).toContain('pty.data')
expect(notifMethods).toContain('pty.resize')
expect(notifMethods).toContain('pty.ackData')
})
it('spawns a PTY and returns an id', async () => {
const result = await dispatcher.callRequest('pty.spawn', { cols: 80, rows: 24 })
expect(result).toEqual({ id: 'pty-1' })
expect(mockPtySpawn).toHaveBeenCalled()
expect(handler.activePtyCount).toBe(1)
})
it('terminates spawned PTY when request becomes stale before response', async () => {
const killSpy = vi.fn()
const term = { ...mockPtyInstance, kill: killSpy, onData: vi.fn(), onExit: vi.fn() }
mockPtySpawn.mockReturnValue(term)
await dispatcher.callRequest('pty.spawn', {}, { isStale: () => true })
// Why: assert via the captured spy reference rather than term.kill because
// disposeManagedPty() neutralizes managed.pty.kill (replaces it with a
// no-op) on POSIX to close the UnixTerminal.destroy() → socket-close →
// SIGHUP-to-recycled-pid race. After the 5s timer fires, term.kill is the
// neutralized function, not the original spy. killSpy retains call history.
expect(killSpy).toHaveBeenCalledWith('SIGTERM')
vi.advanceTimersByTime(5000)
expect(killSpy).toHaveBeenCalledWith('SIGKILL')
})
it('increments PTY ids on each spawn', async () => {
const r1 = await dispatcher.callRequest('pty.spawn', {})
const r2 = await dispatcher.callRequest('pty.spawn', {})
expect((r1 as { id: string }).id).toBe('pty-1')
expect((r2 as { id: string }).id).toBe('pty-2')
})
it('accepts SIGWINCH for restored TUI repaint', async () => {
await dispatcher.callRequest('pty.spawn', {})
await dispatcher.callRequest('pty.sendSignal', { id: 'pty-1', signal: 'SIGWINCH' })
const term = mockPtySpawn.mock.results[0].value
expect(term.kill).toHaveBeenCalledWith('SIGWINCH')
})
it('forwards data from PTY to dispatcher notifications', async () => {
let dataCallback: ((data: string) => void) | undefined
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
onData: vi.fn((cb: (data: string) => void) => {
dataCallback = cb
}),
onExit: vi.fn()
})
await dispatcher.callRequest('pty.spawn', {})
expect(dataCallback).toBeDefined()
dataCallback!('hello world')
expect(dispatcher.notify).toHaveBeenCalledWith('pty.data', { id: 'pty-1', data: 'hello world' })
})
it('returns attach replay instead of notifying when replay notification is suppressed', async () => {
let dataCallback: ((data: string) => void) | undefined
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
onData: vi.fn((cb: (data: string) => void) => {
dataCallback = cb
}),
onExit: vi.fn()
})
await dispatcher.callRequest('pty.spawn', {})
dataCallback!('buffered output')
const result = await dispatcher.callRequest('pty.attach', {
id: 'pty-1',
suppressReplayNotification: true
})
expect(result).toEqual({ replay: 'buffered output' })
expect(dispatcher.notify).not.toHaveBeenCalledWith('pty.replay', expect.anything())
})
it('notifies replay on normal attach', async () => {
let dataCallback: ((data: string) => void) | undefined
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
onData: vi.fn((cb: (data: string) => void) => {
dataCallback = cb
}),
onExit: vi.fn()
})
await dispatcher.callRequest('pty.spawn', {})
dataCallback!('buffered output')
dispatcher.notify.mockClear()
const result = await dispatcher.callRequest('pty.attach', { id: 'pty-1' })
expect(result).toEqual({})
expect(dispatcher.notify).toHaveBeenCalledWith('pty.replay', {
id: 'pty-1',
data: 'buffered output'
})
})
it('notifies on PTY exit and removes from map', async () => {
let exitCallback: ((info: { exitCode: number }) => void) | undefined
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
onData: vi.fn(),
onExit: vi.fn((cb: (info: { exitCode: number }) => void) => {
exitCallback = cb
})
})
await dispatcher.callRequest('pty.spawn', {})
expect(handler.activePtyCount).toBe(1)
exitCallback!({ exitCode: 0 })
expect(dispatcher.notify).toHaveBeenCalledWith('pty.exit', { id: 'pty-1', code: 0 })
expect(handler.activePtyCount).toBe(0)
})
it('writes data to PTY via pty.data notification', async () => {
const mockWrite = vi.fn()
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
write: mockWrite,
onData: vi.fn(),
onExit: vi.fn()
})
await dispatcher.callRequest('pty.spawn', {})
dispatcher.callNotification('pty.data', { id: 'pty-1', data: 'ls\n' })
expect(mockWrite).toHaveBeenCalledWith('ls\n')
})
it('resizes PTY via pty.resize notification', async () => {
const mockResize = vi.fn()
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
resize: mockResize,
onData: vi.fn(),
onExit: vi.fn()
})
await dispatcher.callRequest('pty.spawn', {})
dispatcher.callNotification('pty.resize', { id: 'pty-1', cols: 120, rows: 40 })
expect(mockResize).toHaveBeenCalledWith(120, 40)
})
it('kills PTY on shutdown with SIGTERM by default', async () => {
const mockKill = vi.fn()
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
kill: mockKill,
onData: vi.fn(),
onExit: vi.fn()
})
await dispatcher.callRequest('pty.spawn', {})
await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: false })
expect(mockKill).toHaveBeenCalledWith('SIGTERM')
})
it('kills PTY on shutdown with SIGKILL when immediate', async () => {
const mockKill = vi.fn()
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
kill: mockKill,
onData: vi.fn(),
onExit: vi.fn()
})
await dispatcher.callRequest('pty.spawn', {})
await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true })
expect(mockKill).toHaveBeenCalledWith('SIGKILL')
})
it('throws for attach on nonexistent PTY', async () => {
await expect(dispatcher.callRequest('pty.attach', { id: 'pty-999' })).rejects.toThrow(
'PTY "pty-999" not found'
)
})
it('grace timer waits full period even when no PTYs exist', () => {
const onExpire = vi.fn()
handler.startGraceTimer(onExpire)
expect(onExpire).not.toHaveBeenCalled()
vi.advanceTimersByTime(5 * 60 * 1000)
expect(onExpire).toHaveBeenCalledTimes(1)
})
it('grace timer fires after configured delay when PTYs exist', async () => {
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
onData: vi.fn(),
onExit: vi.fn()
})
await dispatcher.callRequest('pty.spawn', {})
const onExpire = vi.fn()
handler.startGraceTimer(onExpire)
expect(onExpire).not.toHaveBeenCalled()
vi.advanceTimersByTime(5 * 60 * 1000)
expect(onExpire).toHaveBeenCalledTimes(1)
})
it('cancelGraceTimer prevents expiration', async () => {
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
onData: vi.fn(),
onExit: vi.fn()
})
await dispatcher.callRequest('pty.spawn', {})
const onExpire = vi.fn()
handler.startGraceTimer(onExpire)
vi.advanceTimersByTime(60_000)
handler.cancelGraceTimer()
vi.advanceTimersByTime(5 * 60 * 1000)
expect(onExpire).not.toHaveBeenCalled()
})
it('attach preserves buffer so repeated attaches return the same data plus new output', async () => {
let dataCallback: ((data: string) => void) | undefined
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
onData: vi.fn((cb: (data: string) => void) => {
dataCallback = cb
}),
onExit: vi.fn()
})
await dispatcher.callRequest('pty.spawn', {})
dataCallback!('initial output')
const r1 = await dispatcher.callRequest('pty.attach', {
id: 'pty-1',
suppressReplayNotification: true
})
expect(r1).toEqual({ replay: 'initial output' })
dataCallback!(' more')
const r2 = await dispatcher.callRequest('pty.attach', {
id: 'pty-1',
suppressReplayNotification: true
})
expect(r2).toEqual({ replay: 'initial output more' })
})
it('second app restart still replays full buffer', async () => {
let dataCallback: ((data: string) => void) | undefined
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
onData: vi.fn((cb: (data: string) => void) => {
dataCallback = cb
}),
onExit: vi.fn()
})
await dispatcher.callRequest('pty.spawn', {})
dataCallback!('$ while true; do date; done\r\n')
dataCallback!('Mon Apr 28\r\n')
await dispatcher.callRequest('pty.attach', {
id: 'pty-1',
suppressReplayNotification: true
})
dataCallback!('Tue Apr 29\r\n')
await dispatcher.callRequest('pty.attach', {
id: 'pty-1',
suppressReplayNotification: true
})
dataCallback!('Wed Apr 30\r\n')
const result = await dispatcher.callRequest('pty.attach', {
id: 'pty-1',
suppressReplayNotification: true
})
expect(result).toEqual({
replay: '$ while true; do date; done\r\nMon Apr 28\r\nTue Apr 29\r\nWed Apr 30\r\n'
})
})
it('dispose kills all PTYs with SIGKILL', async () => {
const mockKill = vi.fn()
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
kill: mockKill,
onData: vi.fn(),
onExit: vi.fn()
})
await dispatcher.callRequest('pty.spawn', {})
await dispatcher.callRequest('pty.spawn', {})
expect(handler.activePtyCount).toBe(2)
handler.dispose()
// Why: dispose uses SIGKILL (not SIGTERM) because the relay process is
// exiting. A SIGTERM-ignoring remote shell (editor with unsaved buffers,
// wedged process, uninterruptible sleep) would survive SIGTERM + immediate
// destroy() as an orphan on the remote host. SIGKILL is not ignorable.
expect(mockKill).toHaveBeenCalledWith('SIGKILL')
expect(handler.activePtyCount).toBe(0)
})
})