Fix windows build failure (#8504)

* Fix PTY shutdown crash on Windows by dropping unsupported signals

- node-pty's Windows agent throws when kill() is called with a signal
  argument; route all pty.kill() calls through killPtyProcess(), which
  drops the signal on win32 and forwards it on POSIX

* Expand Windows PTY kill-signal-drop test coverage to all call sites

Replace the single graceful-shutdown test with a suite covering every
place killPtyProcess is invoked (graceful/immediate shutdown, stale-spawn
cleanup, SIGKILL fallback timers, and dispose), so a future regression
that reintroduces a signal argument on any Windows code path is caught.
This commit is contained in:
Jinjing 2026-07-13 00:34:04 -07:00 committed by GitHub
parent c3ab805d12
commit 7e303d26f2
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 100 additions and 6 deletions

View File

@ -1018,6 +1018,88 @@ describe('PtyHandler', () => {
expect(mockKill).toHaveBeenCalledWith('SIGTERM')
})
// Why: node-pty's Windows agent throws "Signals not supported on windows."
// for any signal argument. killPtyProcess drops the signal on win32 — cover
// every call site so a future regression cannot reintroduce signal args.
describe('kills PTY without a signal on Windows', () => {
async function withWindowsPlatform(fn: () => Promise<void>): Promise<void> {
const originalPlatform = process.platform
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
try {
await fn()
} finally {
Object.defineProperty(process, 'platform', {
configurable: true,
value: originalPlatform
})
}
}
function mockKillablePty(): ReturnType<typeof vi.fn> {
const mockKill = vi.fn()
mockPtySpawn.mockReturnValue({
...mockPtyInstance,
kill: mockKill,
onData: vi.fn(),
onExit: vi.fn()
})
return mockKill
}
function expectBareKills(mockKill: ReturnType<typeof vi.fn>, times: number): void {
expect(mockKill).toHaveBeenCalledTimes(times)
expect(mockKill.mock.calls.every((args) => args.length === 0)).toBe(true)
}
it('on graceful shutdown', async () => {
await withWindowsPlatform(async () => {
const mockKill = mockKillablePty()
await dispatcher.callRequest('pty.spawn', {})
await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: false })
expectBareKills(mockKill, 1)
})
})
it('on immediate shutdown', async () => {
await withWindowsPlatform(async () => {
const mockKill = mockKillablePty()
await dispatcher.callRequest('pty.spawn', {})
await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true })
expectBareKills(mockKill, 1)
})
})
it('on stale-spawn cleanup including the SIGKILL fallback', async () => {
await withWindowsPlatform(async () => {
const mockKill = mockKillablePty()
await dispatcher.callRequest('pty.spawn', {}, { isStale: () => true })
expectBareKills(mockKill, 1)
vi.advanceTimersByTime(5000)
expectBareKills(mockKill, 2)
})
})
it('on graceful-shutdown SIGKILL fallback', async () => {
await withWindowsPlatform(async () => {
const mockKill = mockKillablePty()
await dispatcher.callRequest('pty.spawn', {})
await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: false })
expectBareKills(mockKill, 1)
vi.advanceTimersByTime(5000)
expectBareKills(mockKill, 2)
})
})
it('on dispose', async () => {
await withWindowsPlatform(async () => {
const mockKill = mockKillablePty()
await dispatcher.callRequest('pty.spawn', {})
handler.dispose()
expectBareKills(mockKill, 1)
})
})
})
it('flushes pending PTY output before immediate shutdown cleanup', async () => {
let dataCallback: ((data: string) => void) | undefined
const mockKill = vi.fn()

View File

@ -85,6 +85,18 @@ type ManagedStartupCommand = {
timer: ReturnType<typeof setTimeout> | null
}
// Why: node-pty's Windows agent throws "Signals not supported on windows." for
// any signal argument. ConPTY/winpty has no signal semantics — a bare kill()
// force-terminates the child — so drop the signal on Windows and forward it
// (SIGTERM graceful vs SIGKILL force) on POSIX.
function killPtyProcess(pty: IPty, signal: string): void {
if (process.platform === 'win32') {
pty.kill()
return
}
pty.kill(signal)
}
function disposeManagedPty(managed: ManagedPty): void {
if (managed.disposed) {
return
@ -710,11 +722,11 @@ export class PtyHandler {
// response is discarded and no renderer can own this PTY. Shut it down
// immediately so it does not linger as an unreachable remote shell.
this.releaseStartupCommand(managed)
term.kill('SIGTERM')
killPtyProcess(term, 'SIGTERM')
managed.killTimer = setTimeout(() => {
const still = this.ptys.get(id)
if (still && !still.disposed) {
still.pty.kill('SIGKILL')
killPtyProcess(still.pty, 'SIGKILL')
// Why: stale-spawn cleanup has no client who will ever attach. If
// SIGKILL's onExit is missed (kernel edge case, uninterruptible
// sleep), the managed entry + ptmx fd would leak forever. Dispose
@ -839,7 +851,7 @@ export class PtyHandler {
if (immediate) {
this.releaseStartupCommand(managed)
this.flushPtyOutput(id)
managed.pty.kill('SIGKILL')
killPtyProcess(managed.pty, 'SIGKILL')
// Why: SIGKILL has already reaped the child; release the ptmx fd on the
// same tick. Deferring to onExit leaves a window where the fd is live
// with a dead child. Idempotent via the disposed guard — if onExit fires
@ -858,7 +870,7 @@ export class PtyHandler {
this.clearPtyFlowState(id)
} else {
this.releaseStartupCommand(managed)
managed.pty.kill('SIGTERM')
killPtyProcess(managed.pty, 'SIGTERM')
// Why: Some processes ignore SIGTERM (e.g. a hung child, a custom signal
// handler). Without a SIGKILL fallback the PTY process would leak and the
@ -872,7 +884,7 @@ export class PtyHandler {
managed.killTimer = setTimeout(() => {
const still = this.ptys.get(id)
if (still && !still.disposed) {
still.pty.kill('SIGKILL')
killPtyProcess(still.pty, 'SIGKILL')
this.flushPtyOutput(id)
// Why: emit pty.exit BEFORE disposeManagedPty sets disposed=true.
// The natural onExit short-circuits on `managed.disposed`, so
@ -1140,7 +1152,7 @@ export class PtyHandler {
// ptmx fd release via disposeManagedPty is synchronous, so there is
// no graceful-shutdown window to preserve at this point.
try {
managed.pty.kill('SIGKILL')
killPtyProcess(managed.pty, 'SIGKILL')
} catch {
/* child may already be dead */
}