diff --git a/src/relay/pty-handler.test.ts b/src/relay/pty-handler.test.ts index 332828408..c90e87480 100644 --- a/src/relay/pty-handler.test.ts +++ b/src/relay/pty-handler.test.ts @@ -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): Promise { + 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 { + const mockKill = vi.fn() + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill: mockKill, + onData: vi.fn(), + onExit: vi.fn() + }) + return mockKill + } + + function expectBareKills(mockKill: ReturnType, 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() diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index 965d8cbdc..d7dd99500 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -85,6 +85,18 @@ type ManagedStartupCommand = { timer: ReturnType | 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 */ }