From 56928007f2f99f6b4b17257b0dfddcdf53ae8316 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Fri, 29 May 2026 19:17:34 -0400 Subject: [PATCH] Avoid double-closing Windows ConPTY handles (#3359) --- src/main/providers/local-pty-provider.test.ts | 37 +++++++++++++++++++ src/main/providers/local-pty-provider.ts | 14 ++++--- .../rate-limits/hidden-pty-cleanup.test.ts | 19 +++++++++- src/main/rate-limits/hidden-pty-cleanup.ts | 6 +++ 4 files changed, 68 insertions(+), 8 deletions(-) diff --git a/src/main/providers/local-pty-provider.test.ts b/src/main/providers/local-pty-provider.test.ts index 09b2dc3e2..9871805b4 100644 --- a/src/main/providers/local-pty-provider.test.ts +++ b/src/main/providers/local-pty-provider.test.ts @@ -472,6 +472,25 @@ describe('LocalPtyProvider', () => { expect(onExit).toHaveBeenCalledWith(id, -1) }) + it('does not destroy after an intentional Windows shutdown kill', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const killSpy = vi.fn() + const destroySpy = vi.fn(() => { + killSpy() + }) + spawnMock.mockReturnValue({ + ...mockProc, + kill: killSpy, + destroy: destroySpy + }) + + const { id } = await provider.spawn({ cols: 80, rows: 24 }) + await provider.shutdown(id, { immediate: true }) + + expect(killSpy).toHaveBeenCalledTimes(1) + expect(destroySpy).not.toHaveBeenCalled() + }) + it('is a no-op for unknown PTY ids', async () => { await provider.shutdown('nonexistent', { immediate: true }) expect(mockProc.kill).not.toHaveBeenCalled() @@ -611,5 +630,23 @@ describe('LocalPtyProvider', () => { const list = await provider.listProcesses() expect(list).toHaveLength(0) }) + + it('does not destroy after intentional Windows orphan kills', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const destroySpy = vi.fn() + const killSpy = vi.fn() + spawnMock.mockReturnValue({ + ...mockProc, + kill: killSpy, + destroy: destroySpy + }) + + await provider.spawn({ cols: 80, rows: 24 }) + + provider.killAll() + + expect(killSpy).toHaveBeenCalledTimes(1) + expect(destroySpy).not.toHaveBeenCalled() + }) }) }) diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index 7e6772b97..c14c9b888 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -114,7 +114,7 @@ function clearPtyState(id: string): void { ptyLoadGeneration.delete(id) } -function destroyPtyProcess(proc: pty.IPty): void { +function destroyPtyProcess(proc: pty.IPty, options: { alreadyKilled?: boolean } = {}): void { // Why: node-pty's UnixTerminal.destroy() closes the master socket, which // releases the ptmx fd to the OS — without this call the fd leaks until GC // (see docs/fix-pty-fd-leak.md). destroy() also registers a close listener @@ -122,9 +122,11 @@ function destroyPtyProcess(proc: pty.IPty): void { // the time that listener runs the child may have exited and its pid been // recycled to an unrelated user process — SIGHUP would land on a Chrome tab, // editor, etc. Neutralize proc.kill on this instance before calling - // destroy() to defuse the hazard. Windows exempt: WindowsTerminal.destroy - // IS a kill() call via _deferNoArgs, so neutralizing it would leak the - // ConPTY agent. + // destroy() to defuse the hazard. On Windows, destroy() is itself kill(); + // skip it only after we have already killed the ConPTY. + if (process.platform === 'win32' && options.alreadyKilled) { + return + } if (process.platform !== 'win32') { ;(proc as unknown as { kill: (sig?: string) => void }).kill = () => {} } @@ -142,7 +144,7 @@ function safeKillAndClean(id: string, proc: pty.IPty): void { } catch { /* Process may already be dead */ } - destroyPtyProcess(proc) + destroyPtyProcess(proc, { alreadyKilled: true }) clearPtyState(id) } @@ -557,7 +559,7 @@ export class LocalPtyProvider implements IPtyProvider { } catch { /* Process may already be dead */ } - destroyPtyProcess(proc) + destroyPtyProcess(proc, { alreadyKilled: true }) ptyProcesses.delete(id) ptyShellName.delete(id) ptyLoadGeneration.delete(id) diff --git a/src/main/rate-limits/hidden-pty-cleanup.test.ts b/src/main/rate-limits/hidden-pty-cleanup.test.ts index cc1143279..094931a97 100644 --- a/src/main/rate-limits/hidden-pty-cleanup.test.ts +++ b/src/main/rate-limits/hidden-pty-cleanup.test.ts @@ -69,7 +69,7 @@ describe('cleanupHiddenRateLimitPty', () => { expect(killMock).toHaveBeenCalledWith() }) - it('does not neutralize kill on Windows because destroy closes ConPTY through kill', () => { + it('does not destroy after an intentional Windows kill because destroy kills again', () => { setPlatform('win32') const term = { kill: vi.fn(), @@ -80,7 +80,22 @@ describe('cleanupHiddenRateLimitPty', () => { cleanupHiddenRateLimitPty(term, [], { kill: true }) - expect(term.kill).toHaveBeenCalledTimes(2) + expect(term.kill).toHaveBeenCalledTimes(1) + expect(term.destroy).not.toHaveBeenCalled() + }) + + it('destroys a Windows PTY after natural exit so ConPTY cleanup still runs', () => { + setPlatform('win32') + const term = { + kill: vi.fn(), + destroy: vi.fn(() => { + term.kill() + }) + } + + cleanupHiddenRateLimitPty(term, [], { kill: false }) + + expect(term.kill).toHaveBeenCalledTimes(1) expect(term.destroy).toHaveBeenCalledTimes(1) }) }) diff --git a/src/main/rate-limits/hidden-pty-cleanup.ts b/src/main/rate-limits/hidden-pty-cleanup.ts index 215bccda7..6d8542120 100644 --- a/src/main/rate-limits/hidden-pty-cleanup.ts +++ b/src/main/rate-limits/hidden-pty-cleanup.ts @@ -22,6 +22,12 @@ export function cleanupHiddenRateLimitPty( } catch { /* already exited */ } + + // Why: node-pty WindowsTerminal.destroy() calls kill() again, which can + // close the same ConPTY handle twice after an intentional termination. + if (process.platform === 'win32') { + return + } } // Why: node-pty destroy releases the master PTY fd; on POSIX, neutralize