diff --git a/packages/core/src/services/conpty-host.ts b/packages/core/src/services/conpty-host.ts new file mode 100644 index 00000000000..c06a19e81f9 --- /dev/null +++ b/packages/core/src/services/conpty-host.ts @@ -0,0 +1,211 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import os from 'node:os'; +import { createDebugLogger } from '../utils/debugLogger.js'; + +const debugLogger = createDebugLogger('CONPTY_HOST'); + +/** + * The `WindowsPtyAgent` internals `releaseConPtyHost` needs, at + * `@lydell/node-pty` 1.2.0-beta.10 (the exact pin in `packages/core/package.json`). + * + * The JS field names below were re-checked on 1.2.0-beta.15 and are unchanged. + * The NATIVE teardown semantics were **not** verified there and do differ: + * from 1.2.0-beta.14 upstream erases the pty baton with an unconditional + * `std::erase_if` rather than under `assert`. A bump therefore has to be + * re-checked against `src/win/conpty.cc`, not only against the JS shape. + * + * Every field is optional and the release degrades to a no-op if the shape ever + * changes, so a bump can only bring the leak back — never a kill we did not + * intend. + */ +interface WindowsPtyAgentInternals { + _pty?: number; + _useConptyDll?: boolean; + _ptyNative?: { kill?: (pty: number, useConptyDll: boolean) => void }; + _conoutSocketWorker?: { dispose?: () => void }; +} + +/** + * PTYs whose pseudo-console has already been closed by a `ptyProcess.kill()` + * reported through `noteConPtyHostReleased`. + * + * This guards every path where a `kill()` runs while the shell is still + * alive — the cancel path (`performCancelKill`) and the web-terminal + * ready-case live release. There, `PtyKill` finds the baton and closes the + * pseudo-console. `kill()` runs first and records the note; the later + * `releaseConPtyHost` (the finalizer, or the web-terminal `releaseHost`) then + * skips the redundant native close. Bundled ConPTY still needs the worker + * fallback because node-pty defers its own worker dispose until more output. + * + * It does nothing for the natural-exit path: there the native exit watcher has + * already erased the baton (see `releaseConPtyHost`), so `PtyKill` no-ops and + * there is no close to double. + * + * A WeakSet so a finished PTY is still collectable. + */ +const releasedHosts = new WeakSet(); + +const asPtyObject = (ptyProcess: unknown): object | undefined => + typeof ptyProcess === 'object' && ptyProcess !== null + ? ptyProcess + : undefined; + +/** + * Record that a `ptyProcess.kill()` on the cancel or process-exit path has + * already closed this PTY's pseudo-console, so a later `releaseConPtyHost` + * does not close it a second time. The later release may still dispose the + * bundled backend's conout worker — see `releasedHosts`. + */ +export const noteConPtyHostReleased = (ptyProcess: unknown): void => { + const key = asPtyObject(ptyProcess); + if (key) { + releasedHosts.add(key); + } +}; + +/** + * Dispose only node-pty's conout worker thread for a finished PTY, without + * closing the pseudo-console. + * + * Used by the web-terminal live-release path when the shell has not yet emitted + * its first output byte: node-pty's `WindowsTerminal.kill()` defers its whole + * teardown (the native `ClosePseudoConsole` and this worker dispose) into + * `_deferreds` until `_isReady` flips, so a release at that moment must dispose + * the worker now — the one resource a never-run deferred teardown would strand + * — while leaving the native close to the queued `kill()`. Closing it here too + * would double-close the same HPCON (see `releaseConPtyHost`). On the inbox + * backend the worker dispose is idempotent. On the bundled backend each call + * resets the one-second drain timer, so a later data-driven dispose remains + * safe. No-op off Windows. + * + * Like `releaseConPtyHost`, this never calls `ptyProcess.kill()`; see that + * function for the #6067 recycled-pid argument and the win32-only rationale. + */ +export const disposeConoutWorker = (ptyProcess: unknown): void => { + if (os.platform() !== 'win32') { + return; + } + const agent = (ptyProcess as { _agent?: WindowsPtyAgentInternals } | null) + ?._agent; + if (!agent) { + return; + } + try { + agent._conoutSocketWorker?.dispose?.(); + } catch (e) { + debugLogger.warn( + `disposeConoutWorker: conout worker dispose threw: ${e instanceof Error ? e.message : String(e)}`, + ); + } +}; + +/** + * Releases what node-pty leaves behind when a Windows PTY finishes. + * + * **What this function itself reliably frees is the conout worker thread.** + * Shell PTYs now use node-pty's bundled ConPTY backend, which releases its host + * reference immediately after spawn so the host exits with its last client. + * Web-terminal PTYs still use the Windows inbox backend, whose natural-exit + * host leak is not fixed here. + * + * With the inbox backend, a finished PTY strands both the ConPTY host and the + * `worker_threads` Worker node-pty runs to read the conout pipe. With bundled + * ConPTY, the host lifecycle is handled by `ConptyReleasePseudoConsole`, but + * node-pty's `_$onProcessExit` deliberately skips cleanup and still strands the + * worker. #11303 measured one leaked worker per completed PTY. + * + * - `_conoutSocketWorker.dispose()` is pure JS — a 1 s drain, then + * `worker.terminate()` — and genuinely frees the worker here. + * - `_ptyNative.kill()` here only reaches a live pseudo-console from ONE call + * site: `firePostSettle`'s `'error'` entry point, where it does close it + * (matching the `windowsKillPid` reap beside it). Everywhere else it is a + * silent no-op. In `src/win/conpty.cc` the native exit-watcher thread erases + * the pty baton *before* it delivers the JS `onExit`, and `PtyKill` skips + * `ClosePseudoConsole` when `get_pty_baton` returns null — with no throw, so + * not even the warn below fires; `struct pty_baton` has no destructor, so the + * erase leaks the HPCON rather than closing it. The call sites that run + * strictly after `onExit` land in that no-op group: the shell-tool finalizer + * when no note was recorded (a natural exit, or a cancel that landed before + * the shell's first output byte), `firePostSettle`'s `'exit'` entry, and the + * web-terminal release of an already-exited session (`release()`'s `else` + * arm) — the primary web-terminal path for #11303. Bundled ConPTY has already + * released its host reference after spawn. The remaining sites are + * the ones where a `kill()` already closed the HPCON while the shell was + * alive and recorded the note (the cancel path and the interactive-shell kill + * when `_isReady !== false`, plus the web-terminal live release whose wrapper + * `kill()` really ran), so this function's `releasedHosts` early return skips + * the close. Finally, a web-terminal release whose shell has not emitted its + * first output byte routes to `disposeConoutWorker` instead of this function + * — from the live arm AND from the exited arm alike, because `releaseHost` + * branches only on `_isReady` and never on `session.exited` — so its queued + * `kill()` stays the single closer. The inbox conhost half of #11303 is + * therefore not fixed by this function on the natural-exit path. + * + * The web-terminal PTY (`web-terminal-registry.ts`) and agent-view PTY host do + * not use the bundled backend, so they can still strand an inbox host per + * exited terminal. Do not add a test that treats a stubbed `_ptyNative.kill` + * call as evidence that the inbox host was released. + * + * **Why not just call `ptyProcess.kill()`.** On the inbox backend it forks a + * helper that can fall back after a natural exit to terminating a recycled + * shell pid (#6067). On the bundled backend it defers worker disposal until + * more output arrives, which may never happen after exit. Direct teardown + * avoids both failure modes; taskkill (`windowsKillPid`) covers live children. + * + * win32-only: there is no ConPTY host or conout worker elsewhere, and node-pty's + * `UnixTerminal.kill()` would signal an already-exited, possibly recycled pid. + */ + +export const releaseConPtyHost = (ptyProcess: unknown): void => { + if (os.platform() !== 'win32') { + return; + } + const key = asPtyObject(ptyProcess); + if (!key) { + return; + } + const agent = (ptyProcess as { _agent?: WindowsPtyAgentInternals } | null) + ?._agent; + if (releasedHosts.has(key)) { + if (agent?._useConptyDll) { + disposeConoutWorker(ptyProcess); + } + return; + } + releasedHosts.add(key); + const ptyId = agent?._pty; + const nativeKill = agent?._ptyNative?.kill; + if (!agent) { + debugLogger.warn( + 'releaseConPtyHost: no node-pty agent; nothing released (see #11303)', + ); + return; + } + if (typeof nativeKill === 'function' && typeof ptyId === 'number') { + try { + nativeKill.call(agent._ptyNative, ptyId, agent._useConptyDll ?? false); + } catch (e) { + debugLogger.warn( + `releaseConPtyHost: the native pty kill threw: ${e instanceof Error ? e.message : String(e)}`, + ); + } + } else { + // Degrade to the pre-#11303 behavior rather than to `kill()`: leaking is + // recoverable by restarting the CLI, killing a recycled pid is not. + debugLogger.warn( + 'releaseConPtyHost: native pty shape changed; skipping the pseudo-console close (see #11303)', + ); + } + try { + agent._conoutSocketWorker?.dispose?.(); + } catch (e) { + debugLogger.warn( + `releaseConPtyHost: conout worker dispose threw: ${e instanceof Error ? e.message : String(e)}`, + ); + } +}; diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index ac8d35f3d65..5dd5b4dbb63 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -244,6 +244,8 @@ describe('ShellExecutionService', () => { }; }; let onOutputEventMock: Mock<(event: ShellOutputEvent) => void>; + let mockPtyNativeKill: Mock; + let mockConoutWorkerDispose: Mock; beforeEach(() => { vi.clearAllMocks(); @@ -269,6 +271,19 @@ describe('ShellExecutionService', () => { }; mockPtyProcess.pid = 12345; mockPtyProcess.kill = vi.fn(); + // node-pty's WindowsPtyAgent internals. releaseConPtyHost drives these + // directly instead of ptyProcess.kill(), which would also fork a helper to + // enumerate the console process list and then TerminateProcess a pid that + // ClosePseudoConsole has already freed for reuse. See #11303. + mockPtyNativeKill = vi.fn(); + mockConoutWorkerDispose = vi.fn(); + (mockPtyProcess as unknown as { _agent: Record })._agent = + { + _pty: 777, + _useConptyDll: true, + _ptyNative: { kill: mockPtyNativeKill }, + _conoutSocketWorker: { dispose: mockConoutWorkerDispose }, + }; // node-pty's onData/onExit return IDisposable; the production // background-promote path calls .dispose() on those handles to detach // its listeners cleanly. Mock them to return a disposable stub so the @@ -1244,13 +1259,15 @@ describe('ShellExecutionService', () => { postPromoteExitHandler({ exitCode: 0 }); }); - it('PR-2.5 backwards compat: without postPromote, listeners stay fully detached (no regression on PR-2 contract)', async () => { - // Pin that omitting `postPromote` preserves the PR-2 detach- - // everything contract. The pre-existing post-promote test at - // line ~680 already covers this for the data path; this one - // adds the symmetric guarantee for the exit path — natural - // post-promote exit must NOT invoke any callback the caller - // didn't provide. + it('PR-2.5 backwards compat: without postPromote, no data listener is re-attached and no caller callback fires', async () => { + // Pin the caller-visible half of the PR-2 detach-everything contract: + // omitting `postPromote` re-attaches no data listener and invokes no + // callback the caller didn't provide. The settle listener itself IS + // attached — it is the only path left that can release a promoted + // shell's conout worker (#11303), and `firePostSettle` + // early-returns before any forwarding when there is no onSettle handler. + // Pinned by 'releases the conout worker when a promote passed no + // postPromote handlers' below. const onDataCalls: ShellOutputEvent[] = []; const onSettleCalls: ShellPostPromoteSettleInfo[] = []; const { result } = await simulateExecution( @@ -1272,7 +1289,9 @@ describe('ShellExecutionService', () => { // registration count stays at 1. expect(onDataRegistrations.length).toBe(1); const onExitRegistrations = mockPtyProcess.onExit.mock.calls; - expect(onExitRegistrations.length).toBe(1); + // TWO: the foreground handler (disposed at promote) plus the + // unconditional settle/release handler. + expect(onExitRegistrations.length).toBe(2); // Caller-provided handlers were never invoked. expect(onDataCalls).toHaveLength(0); expect(onSettleCalls).toHaveLength(0); @@ -1794,6 +1813,14 @@ describe('ShellExecutionService', () => { ['/f', '/pid', String(mockPtyProcess.pid)], HIDDEN_WINDOW, ); + // The ConPTY release (#11303) sits in the same spot for the same + // reason: above firePostSettle's `!postPromote?.onSettle` early return. + // This assertion is what goes red if it is ever slid below it — with + // onData but no onSettle, every backgrounded command would leak its + // conout worker. (mockPtyNativeKill only pins that the call is made; the + // real native no-ops after a natural exit — see releaseConPtyHost.) + expect(mockPtyNativeKill).toHaveBeenCalledWith(777, true); + expect(mockConoutWorkerDispose).toHaveBeenCalled(); }); it('win32 promoted shell skips the post-settle reap when the pty already exited', async () => { @@ -1841,6 +1868,277 @@ describe('ShellExecutionService', () => { }); }); + describe('Windows ConPTY release (#11303)', () => { + // Bundled ConPTY releases its host reference after spawn, but node-pty's JS + // still leaves the conout worker running after a natural shell exit. Inbox + // ConPTY users such as web terminals can leave both resources behind. + // + // The release deliberately does NOT go through ptyProcess.kill(): the inbox + // backend can terminate a recycled pid through its helper fallback, while + // the bundled backend waits for more output before disposing the worker. + // + // These cases certify the worker release only. Host lifecycle coverage for + // the shell path belongs to the bundled ConPTY tests below. + + beforeEach(() => { + mockCpSpawn.mockReturnValue(new EventEmitter()); + mockSpawnSync.mockReturnValue({ status: 0 }); + }); + + it('releases the conout worker on a clean win32 completion (the leaking path)', async () => { + mockPlatform.mockReturnValue('win32'); + // The shell exited cleanly: isPtyActive is false, so the taskkill reap is + // (correctly) skipped — and that is exactly the path that leaked. + mockProcessKill.mockImplementation( + (_pid: number, signal?: string | number) => { + if (signal === 0) { + throw new Error('ESRCH'); + } + return true; + }, + ); + + try { + const { result } = await simulateExecution('echo hi', (pty) => { + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + }); + + expect(result.exitCode).toBe(0); + // No taskkill (the shell is already gone)... + expect(mockCpSpawn).not.toHaveBeenCalledWith( + TASKKILL, + expect.anything(), + HIDDEN_WINDOW, + ); + // ...but the stranded conout worker is still released. + expect(mockPtyNativeKill).toHaveBeenCalledWith(777, true); + expect(mockConoutWorkerDispose).toHaveBeenCalled(); + // Never through kill(): see the block comment above. + expect(mockPtyProcess.kill).not.toHaveBeenCalled(); + } finally { + mockProcessKill.mockImplementation(() => true); + } + }); + + it('releases them even when the shell lingers and taskkill fires', async () => { + mockPlatform.mockReturnValue('win32'); + // Default liveness mock: node-pty reported exit but the shell lingers. + const { result } = await simulateExecution('echo hi', (pty) => { + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + }); + + expect(result.exitCode).toBe(0); + expect(mockCpSpawn).toHaveBeenCalledWith( + TASKKILL, + ['/f', '/pid', String(mockPtyProcess.pid)], + HIDDEN_WINDOW, + ); + expect(mockPtyNativeKill).toHaveBeenCalledWith(777, true); + expect(mockConoutWorkerDispose).toHaveBeenCalled(); + }); + + it('does not fail the result when the native pty kill throws', async () => { + mockPlatform.mockReturnValue('win32'); + mockPtyNativeKill.mockImplementation(() => { + throw new Error('pty already gone'); + }); + + const { result } = await simulateExecution('echo hi', (pty) => { + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + }); + + expect(result.exitCode).toBe(0); + expect(result.error).toBeNull(); + // A throwing host close must not skip the worker teardown. + expect(mockConoutWorkerDispose).toHaveBeenCalled(); + }); + + it('still drops the pid from activePtys when the conout worker dispose throws', async () => { + mockPlatform.mockReturnValue('win32'); + // The second guard in releaseConPtyHost — the one around + // _conoutSocketWorker.dispose(). The release runs in finalize()'s + // finally, immediately before activePtys.delete(pid) and after the result + // has already settled, so an escaping throw would leave a finished pid + // registered for the process-exit `taskkill /f /t` — against a pid + // Windows may have recycled by then. + mockConoutWorkerDispose.mockImplementation(() => { + throw new Error('dispose boom'); + }); + + const { result } = await simulateExecution('echo hi', (pty) => { + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + }); + + expect(result.exitCode).toBe(0); + // Direct witness for the guard, and the one that survives a refactor of + // how the process-exit reap spawns taskkill: `resolve(...)` sits in + // finalize()'s try and `disposeForegroundPtyResources()` — which owns + // both the release and activePtys.delete — is in its finally, so the + // finally body has already run by the time the awaited result resumes. + // Remove the try/catch around _conoutSocketWorker.dispose() in + // conpty-host.ts and this comes back true. + expect(ShellExecutionService['activePtys'].has(mockPtyProcess.pid)).toBe( + false, + ); + + ShellExecutionService.cleanup(); + ShellExecutionService['activePtys'].delete(mockPtyProcess.pid); + + // End-to-end consequence of the same invariant: the pid was already + // dropped, so the exit cleanup has nothing to tree-kill. + expect(mockSpawnSync).not.toHaveBeenCalledWith( + TASKKILL, + ['/f', '/t', '/pid', String(mockPtyProcess.pid)], + HIDDEN_WINDOW, + ); + }); + + it('degrades to the pre-fix leak, not to kill(), if node-pty internals change', async () => { + mockPlatform.mockReturnValue('win32'); + delete (mockPtyProcess as unknown as { _agent?: unknown })._agent; + + const { result } = await simulateExecution('echo hi', (pty) => { + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + }); + + expect(result.exitCode).toBe(0); + // Leaking is recoverable by restarting the CLI; killing a recycled pid is + // not, so the fallback must never be ptyProcess.kill(). + expect(mockPtyProcess.kill).not.toHaveBeenCalled(); + }); + + it('still disposes the worker when only the native-kill shape drifts', async () => { + mockPlatform.mockReturnValue('win32'); + // A node-pty bump that renames _pty / _ptyNative must not cost the worker + // dispose — the only teardown that frees anything today. The fused guard + // used to skip both on a native-shape drift; see releaseConPtyHost. + (mockPtyProcess as unknown as { _agent: unknown })._agent = { + _conoutSocketWorker: { dispose: mockConoutWorkerDispose }, + }; + + const { result } = await simulateExecution('echo hi', (pty) => { + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + }); + + expect(result.exitCode).toBe(0); + expect(mockConoutWorkerDispose).toHaveBeenCalled(); + // Never fall back to kill(): leaking is recoverable, killing a recycled + // pid is not. + expect(mockPtyProcess.kill).not.toHaveBeenCalled(); + }); + + it('does not close the pseudo-console twice and still disposes the bundled worker after cancel', async () => { + mockPlatform.mockReturnValue('win32'); + // performCancelKill closes the pseudo-console itself. With bundled + // ConPTY, node-pty waits for more output before disposing the worker, so + // the finalizer must skip the native close but still start the worker's + // drain timeout in case no more data arrives. + const { result } = await simulateExecution('sleep 100', (pty, ac) => { + ac.abort(); + pty.onExit.mock.calls[0][0]({ exitCode: 1, signal: null }); + }); + + expect(result.aborted).toBe(true); + expect(mockPtyProcess.kill).toHaveBeenCalled(); + expect(mockPtyNativeKill).not.toHaveBeenCalled(); + expect(mockConoutWorkerDispose).toHaveBeenCalledOnce(); + }); + + it('never touches the pty on non-win32 (no ConPTY host, no conout worker)', async () => { + // Default platform is 'linux'. + const { result } = await simulateExecution('echo hi', (pty) => { + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + }); + + expect(result.exitCode).toBe(0); + expect(mockPtyNativeKill).not.toHaveBeenCalled(); + expect(mockConoutWorkerDispose).not.toHaveBeenCalled(); + expect(mockPtyProcess.kill).not.toHaveBeenCalled(); + }); + + it('releases the conout worker of a promoted shell when it settles', async () => { + mockPlatform.mockReturnValue('win32'); + + const { result } = await simulateExecution( + 'long-running-command', + (_pty, ac) => { + ac.abort({ + kind: 'background', + shellId: 'bg_11303_settle', + } satisfies ShellAbortReason); + }, + shellExecutionConfig, + { postPromote: { onSettle: () => {} } }, + ); + expect(result.promoted).toBe(true); + // Promote itself must not tear anything down — the caller owns the child. + expect(mockPtyNativeKill).not.toHaveBeenCalled(); + + const onExitRegistrations = mockPtyProcess.onExit.mock.calls; + const postPromoteExitHandler = + onExitRegistrations[onExitRegistrations.length - 1][0]; + postPromoteExitHandler({ exitCode: 0, signal: undefined }); + + // The promote branch already dropped this pid from activePtys, so the + // process-exit cleanup() cannot reach it: settle is the last chance. + expect(mockPtyNativeKill).toHaveBeenCalledWith(777, true); + expect(mockConoutWorkerDispose).toHaveBeenCalled(); + }); + + it('releases the conout worker when a promote passed no postPromote handlers', async () => { + mockPlatform.mockReturnValue('win32'); + + const { result } = await simulateExecution( + 'long-running-command', + (_pty, ac) => { + ac.abort({ + kind: 'background', + shellId: 'bg_11303_no_handlers', + } satisfies ShellAbortReason); + }, + // No options arg → postPromote unset → PR-2 detach contract. + ); + expect(result.promoted).toBe(true); + // Promote itself must not tear anything down — the caller owns the child. + expect(mockPtyNativeKill).not.toHaveBeenCalled(); + + // The settle listener is attached even without postPromote: it is the + // only path that can still reach this PTY, because the promote branch + // dropped the pid from activePtys and disposed exitDisposable. Wrapping + // the attach in `if (postPromote)` again must turn this red. + const onExitRegistrations = mockPtyProcess.onExit.mock.calls; + expect(onExitRegistrations.length).toBe(2); + const postPromoteExitHandler = + onExitRegistrations[onExitRegistrations.length - 1][0]; + postPromoteExitHandler({ exitCode: 0, signal: undefined }); + + expect(mockPtyNativeKill).toHaveBeenCalledWith(777, true); + expect(mockConoutWorkerDispose).toHaveBeenCalled(); + }); + + it('still releases after a cancel that landed before the terminal was ready', async () => { + mockPlatform.mockReturnValue('win32'); + // WindowsTerminal.kill() runs its whole teardown through _deferNoArgs, + // which queues it until `_isReady` — and that flag flips only on the + // conout socket's first data byte. A cancel before that point (Esc during + // pwsh startup, `timeout /t 30 >nul`) queues a teardown that may never + // run, so it must not be recorded as a release: the finalizer still owes + // the conout worker. Noting it unconditionally — the previous code — + // makes both release assertions below fail. + (mockPtyProcess as unknown as { _isReady: boolean })._isReady = false; + + const { result } = await simulateExecution('timeout /t 30', (pty, ac) => { + ac.abort(); + pty.onExit.mock.calls[0][0]({ exitCode: 1, signal: null }); + }); + + expect(result.aborted).toBe(true); + expect(mockPtyProcess.kill).toHaveBeenCalled(); + expect(mockPtyNativeKill).toHaveBeenCalledWith(777, true); + expect(mockConoutWorkerDispose).toHaveBeenCalled(); + }); + }); + describe('Windows bundled ConPTY backend (#11303)', () => { // With the inbox ConPTY backend a natural shell exit orphans the // `conhost.exe --headless` it spawned (microsoft/node-pty#965); #11303 diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index d48554a531f..70b8dff04d5 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -28,6 +28,7 @@ import { normalizePathEnvForWindows } from '../utils/windowsPath.js'; import { sanitizeChildEnv } from '../utils/sanitize-child-env.js'; import { formatMemoryUsage } from '../utils/formatters.js'; import { getShellContextEnvVars } from './shellContextEnv.js'; +import { noteConPtyHostReleased, releaseConPtyHost } from './conpty-host.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { getShellPagerEnv } from '../utils/shell-pager-env.js'; @@ -300,8 +301,10 @@ function appendOutputCaptureLimitNotice( * `'completed'` / `'failed'` on natural child exit. * * Backwards compat: if `postPromote` is unset on the options bag the - * service falls back to the PR-2 detach-everything contract — no - * regressions for callers that don't opt in. + * service preserves the caller-visible half of the PR-2 detach-everything + * contract — no data listener is re-attached and no callback the caller did + * not provide fires. The settle listener that reaps and releases the conout + * worker is still attached internally (see #11303). */ export interface ShellPostPromoteHandlers { /** @@ -343,7 +346,8 @@ export interface ShellExecuteOptions { streamStdout?: boolean; /** * Post-promote callback hooks. See {@link ShellPostPromoteHandlers}. - * Optional; omit to preserve the PR-2 detach-everything contract. + * Optional; omit to preserve the caller-visible PR-2 detach-everything + * contract (the settle listener still attaches internally). */ postPromote?: ShellPostPromoteHandlers; } @@ -616,6 +620,7 @@ const windowsStrategy: ProcessCleanupStrategy = { } catch { // already gone } + noteConPtyHostReleased(pty.ptyProcess); }, killChildProcesses: (pids) => { if (pids.size > 0) { @@ -1901,6 +1906,16 @@ export class ShellExecutionService { ) { windowsKillPid(ptyProcess.pid, cancelKillDispatched); } + // The taskkill above owns the shell; this releases node-pty's conout + // worker thread, which nothing else frees once we delete from + // activePtys below. It is NOT under the isPtyActive guard: the + // healthy path — shell exited cleanly, so no taskkill — is exactly + // the one that leaks it, once per tool call (#11303). + // + // Bundled ConPTY already released its host reference after spawn, + // but node-pty skips worker cleanup on this natural-exit path. + // releaseConPtyHost disposes that worker without signalling the pid. + releaseConPtyHost(ptyProcess); this.activePtys.delete(ptyProcess.pid); }; @@ -2031,7 +2046,9 @@ export class ShellExecutionService { // path), and the eventual natural-exit transitions the // registry entry to `'completed'` / `'failed'` instead of // leaving it stuck on `'running'`. When postPromote is - // undefined the PR-2 detach-everything contract is preserved. + // undefined the caller-visible half of the PR-2 detach-everything + // contract is preserved (no data listener, no caller callback); the + // settle listener still attaches internally to reap and release. exited = true; listenersDetached = true; abortSignal.removeEventListener('abort', abortHandler); @@ -2163,6 +2180,13 @@ export class ShellExecutionService { ) { windowsKillPid(ptyProcess.pid, false); } + // ...and release node-pty's conout worker. The promote branch + // already dropped this pid from activePtys, so the process-exit + // cleanup() cannot reach it either — without this a backgrounded + // command leaks the worker exactly like the foreground path did + // (#11303). Bundled ConPTY handles the host lifecycle separately; + // releaseConPtyHost is still required for the worker. + releaseConPtyHost(ptyProcess); if (!postPromote?.onSettle) return; try { postPromote.onSettle(info); @@ -2192,46 +2216,73 @@ export class ShellExecutionService { ); } } - if (postPromote) { - try { - postPromoteExitDisposable = ptyProcess.onExit( - ({ - exitCode, - signal, - }: { - exitCode: number; - signal?: number; - }) => { - firePostSettle({ - exitCode, - signal: signal === 0 ? null : (signal ?? null), - endTime: Date.now(), - }); - }, - ); - } catch (e) { - debugLogger.warn( - `re-attaching post-promote exit listener threw: ${e instanceof Error ? e.message : String(e)}`, - ); - } - try { - postPromoteErrorListener = (err: NodeJS.ErrnoException) => { - if (isExpectedPtyReadExitError(err)) { - return; - } + // The settle path is attached UNCONDITIONALLY, unlike the onData + // forwarding above. `firePostSettle` is the only thing that reaps a + // promoted shell and releases its conout worker (#11303), and + // the promote branch already dropped this pid from `activePtys`, so + // with no listener a promote that passes no `postPromote` leaks the + // worker for the life of the CLI and nothing left can reach them. + // + // Routing the no-`postPromote` promote through `firePostSettle` also + // gives it the #5873 settle-time reap: `windowsKillPid(pid, false)` + // (`taskkill /f /pid`) runs whenever `isPtyActive(pid)` is still + // true, a taskkill the caller did not explicitly ask for. That is the + // same recycle race the cancel path documents; it is pre-existing in + // kind, and narrower than it looks. Most shipped `execute()` call + // sites omit `postPromote`, but none of them can reach this code: + // `performBackgroundPromote` is only entered from a + // `{ kind: 'background' }` abort (see the abortHandler switch below), + // and that abort's sole producer is the shell tool's Ctrl+B handler + // firing the `promoteAbortController` it created on the foreground + // `execute()` path (tools/shell.ts) — the one call site that also + // passes `postPromote`. So a no-`postPromote` promote is reachable + // only from that user-initiated foreground-to-background handoff, + // where the settle-time reap is the intended ownership transfer + // rather than a surprise taskkill. + // + // Only the *forwarding* to caller handlers stays gated: + // `firePostSettle` early-returns on `!postPromote?.onSettle` after + // the reap and the release, so no caller callback fires and no data + // listener is attached when the caller did not opt in. Attaching the + // 'error' listener unconditionally is load-bearing for a different + // reason than the text above: node-pty routes `on('error')` to the + // conout socket, whose own handler throws once it sees fewer than two + // 'error' listeners (`listeners('error').length < 2`), and the + // foreground handler was removed at promote — so without this + // listener a post-promote socket error escapes as an + // uncaughtException and takes the CLI down. + try { + postPromoteExitDisposable = ptyProcess.onExit( + ({ exitCode, signal }: { exitCode: number; signal?: number }) => { firePostSettle({ - error: err, - exitCode: null, - signal: null, + exitCode, + signal: signal === 0 ? null : (signal ?? null), endTime: Date.now(), }); - }; - ptyProcess.on('error', postPromoteErrorListener); - } catch (e) { - debugLogger.warn( - `re-attaching post-promote error listener threw: ${e instanceof Error ? e.message : String(e)}`, - ); - } + }, + ); + } catch (e) { + debugLogger.warn( + `re-attaching post-promote exit listener threw: ${e instanceof Error ? e.message : String(e)}`, + ); + } + try { + postPromoteErrorListener = (err: NodeJS.ErrnoException) => { + if (isExpectedPtyReadExitError(err)) { + return; + } + firePostSettle({ + error: err, + exitCode: null, + signal: null, + endTime: Date.now(), + }); + }; + ptyProcess.on('error', postPromoteErrorListener); + } catch (e) { + debugLogger.warn( + `re-attaching post-promote error listener threw: ${e instanceof Error ? e.message : String(e)}`, + ); } // Drain in-flight chain work (already-enqueued @@ -2388,10 +2439,41 @@ export class ShellExecutionService { // Then tear down the ConPTY host so onExit fires and the cancel // resolves even if taskkill couldn't kill the tree. Harmless once // the tree is already dead. Mirrors the POSIX branch's kill fallback. + // + // kill() is right *here* — unlike on the healthy path — as the + // fallback for a taskkill that never launched: the shell is then + // genuinely still running, so node-pty's console-process-list + // lookup resolves for real. When the taskkill above did land, the + // shell is already dead by the time kill() runs and the lookup + // takes node-pty's 5 s `[innerPid]` fallback instead + // (windowsPtyAgent._getConsoleProcessList has only a message + // listener plus that timeout), so the #6067 collateral-kill mode is + // still reachable on this path. That is tracked separately and + // deliberately out of scope here — do not read this call as + // evidence the shell is alive. Record the release when it actually + // ran so the finalizer's releaseConPtyHost does not close the same + // pseudo-console twice: while the shell is alive, native PtyKill + // closes the HPCON but leaves the baton in its handle list, so a + // second close is a double-free. See #11303. try { ptyProcess.kill(); + // `WindowsTerminal.kill()` routes its whole teardown through + // `_deferNoArgs`, which QUEUES it until the terminal is ready — + // and `_isReady` flips only inside the conout socket's first + // 'data' callback. A cancel that lands before the shell's first + // output byte (Esc during pwsh startup, `timeout /t 30 >nul`) + // therefore queues a teardown that may never run, so noting a + // release there would permanently suppress the finalizer's + // releaseConPtyHost and leak the conout worker — the one resource + // this path can still release. Reading the optional `_isReady` + // degrades to the previous behavior if the field is ever renamed + // (undefined !== false). + if ((ptyProcess as { _isReady?: boolean })._isReady !== false) { + noteConPtyHostReleased(ptyProcess); + } } catch { - // already gone + // already gone — kill() threw, so nothing was torn down and the + // finalizer's release must still run. } } else { try { diff --git a/packages/core/src/services/web-terminal-registry.test.ts b/packages/core/src/services/web-terminal-registry.test.ts index 11c157996bf..98f880dcbd4 100644 --- a/packages/core/src/services/web-terminal-registry.test.ts +++ b/packages/core/src/services/web-terminal-registry.test.ts @@ -6,14 +6,25 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -const { spawn, getPty, spawnSync } = vi.hoisted(() => ({ +const { spawn, getPty, spawnSync, osPlatform } = vi.hoisted(() => ({ spawn: vi.fn(), getPty: vi.fn(), spawnSync: vi.fn(), + osPlatform: vi.fn(), })); vi.mock('node:child_process', () => ({ spawnSync })); vi.mock('../utils/getPty.js', () => ({ getPty })); +// Only conpty-host reads os.platform(); killPtyTree branches on +// process.platform, so this steers the ConPTY release without touching it. +// Windows CI is skipped on PRs, so the win32 path has to be reachable here. +// Everything else passes through -- Storage (via debugLogger) needs the real +// os.homedir()/os.tmpdir(). +vi.mock('node:os', async (importOriginal) => { + const actual = await importOriginal(); + const patched = { ...actual, platform: osPlatform }; + return { ...patched, default: patched }; +}); import { MAX_CONCURRENT_WEB_TERMINALS, @@ -27,6 +38,8 @@ describe('WebTerminalRegistry', () => { let write: ReturnType; let resize: ReturnType; let kill: ReturnType; + let nativeKill: ReturnType; + let conoutDispose: ReturnType; let disposeData: ReturnType; let disposeExit: ReturnType; @@ -35,14 +48,25 @@ describe('WebTerminalRegistry', () => { write = vi.fn(); resize = vi.fn(); kill = vi.fn(); + nativeKill = vi.fn(); + conoutDispose = vi.fn(); disposeData = vi.fn(); disposeExit = vi.fn(); spawnSync.mockReturnValue({ stdout: '' }); + osPlatform.mockReturnValue(process.platform); spawn.mockReturnValue({ pid: 1, write, resize, kill, + // node-pty's WindowsPtyAgent internals, which releaseConPtyHost drives + // directly instead of going through kill(). See #11303. + _agent: { + _pty: 42, + _useConptyDll: false, + _ptyNative: { kill: nativeKill }, + _conoutSocketWorker: { dispose: conoutDispose }, + }, onData: vi.fn((listener) => { onData = listener; return { dispose: disposeData }; @@ -242,6 +266,146 @@ describe('WebTerminalRegistry', () => { expect(registry.readSnapshot('terminal:release')).toBeUndefined(); }); + it("releases an exited session's conout worker, never by signalling the pid", async () => { + osPlatform.mockReturnValue('win32'); + const registry = new WebTerminalRegistry(); + await registry.create({ + terminalId: 'terminal:release-exited', + workspaceCwd: '/workspace', + }); + onExit({ exitCode: 0 }); + + expect(registry.release('terminal:release-exited')).toBe(true); + // The shell is gone, so nothing may signal its (possibly recycled) pid: + // no taskkill, no process-group kill, and no ptyProcess.kill() either -- + // node-pty's kill() force-terminates the console process list. See #11303. + expect(spawnSync).not.toHaveBeenCalled(); + expect(kill).not.toHaveBeenCalled(); + // node-pty releases neither the ConPTY host nor its conout worker on a + // natural exit, so the release goes at the agent directly. Only the worker + // half actually lands: nativeKill is a stub here, and the real one no-ops + // after a natural exit. See releaseConPtyHost. + expect(nativeKill).toHaveBeenCalledOnce(); + expect(conoutDispose).toHaveBeenCalledOnce(); + expect(disposeData).toHaveBeenCalledOnce(); + expect(disposeExit).toHaveBeenCalledOnce(); + }); + + it('releases a live session whose kill was deferred before its first byte', async () => { + osPlatform.mockReturnValue('win32'); + const registry = new WebTerminalRegistry(); + await registry.create({ + terminalId: 'terminal:release-live-deferred', + workspaceCwd: '/workspace', + }); + // node-pty's WindowsTerminal.kill() queues its teardown while _isReady is + // false — a terminal released before the shell's first output byte. No + // onExit fired, so the session is still live. + (spawn.mock.results[0].value as { _isReady?: boolean })._isReady = false; + + expect(registry.release('terminal:release-live-deferred')).toBe(true); + expect(kill).toHaveBeenCalledOnce(); + // The deferred kill tore nothing down and is still queued in node-pty's + // _deferreds; when it eventually runs it closes the pseudo-console. So + // releaseHost must dispose the worker now (the one resource a never-run + // deferred kill would strand) WITHOUT closing the pseudo-console itself — + // a native close here plus the queued kill's later close would double-free + // the same HPCON. + expect(nativeKill).not.toHaveBeenCalled(); + expect(conoutDispose).toHaveBeenCalledOnce(); + }); + + it('still completes a deferred release when the conout worker dispose throws', async () => { + osPlatform.mockReturnValue('win32'); + // The throw guard inside disposeConoutWorker — the twin of the one in + // releaseConPtyHost. release() calls session.pty.releaseHost?.() bare, + // after the session is already deleted from the map, and dispose()'s loop + // has no per-iteration guard, so a throw escaping the worker dispose would + // abort the teardown of every session still queued behind it. + conoutDispose.mockImplementation(() => { + throw new Error('dispose boom'); + }); + const registry = new WebTerminalRegistry(); + await registry.create({ + terminalId: 'terminal:release-deferred-throws', + workspaceCwd: '/workspace', + }); + (spawn.mock.results[0].value as { _isReady?: boolean })._isReady = false; + + // Remove the try/catch around _conoutSocketWorker.dispose() in + // disposeConoutWorker and this throws out of release() instead of + // returning true. + expect(registry.release('terminal:release-deferred-throws')).toBe(true); + expect(conoutDispose).toHaveBeenCalledOnce(); + // The queued kill() is still the single closer. + expect(nativeKill).not.toHaveBeenCalled(); + expect(kill).toHaveBeenCalledOnce(); + }); + + it('releases an exited session that never became ready through the deferred arm', async () => { + osPlatform.mockReturnValue('win32'); + const registry = new WebTerminalRegistry(); + await registry.create({ + terminalId: 'terminal:release-exited-deferred', + workspaceCwd: '/workspace', + }); + // The shell exits before its first output byte — COMSPEC resolving to a + // binary that quits immediately, or a releaseWorkspace drain racing pwsh + // startup. That is session.exited === true AND _isReady === false, so + // release()'s exited arm routes into releaseHost's deferred branch. + (spawn.mock.results[0].value as { _isReady?: boolean })._isReady = false; + onExit({ exitCode: 0 }); + + expect(registry.release('terminal:release-exited-deferred')).toBe(true); + // Nothing may signal an exited shell's possibly-recycled pid, and the + // native baton is already erased — so no kill and no native close on this + // arm. (Asserting the host WAS released is forbidden; see conpty-host.ts.) + expect(kill).not.toHaveBeenCalled(); + expect(nativeKill).not.toHaveBeenCalled(); + expect(spawnSync).not.toHaveBeenCalled(); + // The conout worker is still freed: the one resource node-pty strands on a + // natural exit, which is the whole point of the else branch in release(). + expect(conoutDispose).toHaveBeenCalledOnce(); + }); + + it('does not double-close a live session whose kill already closed it', async () => { + osPlatform.mockReturnValue('win32'); + // Model node-pty's real WindowsTerminal.kill(): when ready it closes the + // HPCON and disposes the worker. The wrapper notes the close, so the + // releaseHost below must not add a second native kill (double-free). + kill.mockImplementation(() => { + nativeKill(42, false); + conoutDispose(); + }); + const registry = new WebTerminalRegistry(); + await registry.create({ + terminalId: 'terminal:release-live-ready', + workspaceCwd: '/workspace', + }); + + expect(registry.release('terminal:release-live-ready')).toBe(true); + expect(kill).toHaveBeenCalledOnce(); + expect(nativeKill).toHaveBeenCalledOnce(); + expect(conoutDispose).toHaveBeenCalledOnce(); + }); + + it('leaves an exited session alone off Windows', async () => { + osPlatform.mockReturnValue('linux'); + const registry = new WebTerminalRegistry(); + await registry.create({ + terminalId: 'terminal:release-exited-posix', + workspaceCwd: '/workspace', + }); + onExit({ exitCode: 0 }); + + expect(registry.release('terminal:release-exited-posix')).toBe(true); + // No ConPTY host and no conout worker to release, and UnixTerminal.kill() + // would signal an already-exited, possibly recycled pid. + expect(nativeKill).not.toHaveBeenCalled(); + expect(conoutDispose).not.toHaveBeenCalled(); + expect(kill).not.toHaveBeenCalled(); + }); + it('forwards live output and bounds unacknowledged PTY input', async () => { const registry = new WebTerminalRegistry(); await registry.create({ diff --git a/packages/core/src/services/web-terminal-registry.ts b/packages/core/src/services/web-terminal-registry.ts index f819ba3772b..6ab9f7c6d84 100644 --- a/packages/core/src/services/web-terminal-registry.ts +++ b/packages/core/src/services/web-terminal-registry.ts @@ -6,6 +6,11 @@ import { spawnSync } from 'node:child_process'; import { getPty } from '../utils/getPty.js'; +import { + disposeConoutWorker, + noteConPtyHostReleased, + releaseConPtyHost, +} from './conpty-host.js'; /** * Minimal PTY surface used by the web terminal registry. Backed by node-pty. @@ -15,6 +20,16 @@ export interface WebTerminalPty { write(data: string): void; resize(cols: number, rows: number): void; kill(): void; + /** + * Release node-pty's Windows conout worker without signalling the shell pid. + * Used after the shell has exited, where `kill()` would reach a possibly + * recycled pid, and on the live-release path where a deferred `kill()` would + * otherwise strand the worker. When the kill is still deferred (the shell + * has not emitted its first output byte) it disposes only the worker, leaving + * the native ConPTY close to the queued `kill()` — see `disposeConoutWorker` + * and `releaseConPtyHost`. No-op off Windows. See #11303. + */ + releaseHost?(): void; } export interface WebTerminalSnapshot { @@ -278,7 +293,43 @@ export class WebTerminalRegistry { pid: spawned.pid, write: (data) => spawned.write(data), resize: (cols, rows) => spawned.resize(cols, rows), - kill: () => spawned.kill(), + kill: () => { + spawned.kill(); + // Mirror the cancel path (shellExecutionService.performCancelKill): + // node-pty's WindowsTerminal.kill() defers its whole teardown while + // `_isReady` is false, so note the close only when kill() really ran. + // release() then disposes the worker a deferred kill left behind, + // without double-closing a pseudo-console kill() already closed. + if ((spawned as { _isReady?: boolean })._isReady !== false) { + noteConPtyHostReleased(spawned); + } + }, + releaseHost: () => { + // Branches on `_isReady` alone, and release() reaches it from BOTH + // arms — the live one and the already-exited one — with a different + // reason on each. + // + // LIVE: node-pty's WindowsTerminal.kill() defers its whole teardown + // while `_isReady` is false, so killPtyTree has just queued a kill() + // in `_deferreds`. That queued teardown runs the native + // ClosePseudoConsole when it fires, so closing the pseudo-console + // here would double-close the same HPCON. Dispose only the conout + // worker now — the one resource a deferred kill can strand, and an + // idempotent one — and leave the native close to the queued kill(). + // + // EXITED: no kill() ran and nothing is queued, because release() only + // calls killPtyTree on the live arm. The native exit-watcher has + // already erased the baton, so a native close here would no-op rather + // than double-close; the conout worker is still the one resource + // node-pty never releases on a natural exit, and this branch frees + // it. Same outcome releaseConPtyHost would have had on that arm, + // reached for a different reason. + if ((spawned as { _isReady?: boolean })._isReady === false) { + disposeConoutWorker(spawned); + return; + } + releaseConPtyHost(spawned); + }, }; } catch { this.finishCreating(terminalId); @@ -413,6 +464,22 @@ export class WebTerminalRegistry { session.exitListeners.clear(); if (!session.exited) { killPtyTree(session.pty); + // killPtyTree's pty.kill() defers its whole teardown while `_isReady` is + // false, so a terminal released before its shell's first output byte (tab + // closed during slow pwsh startup, or a workspace drain) still has a + // kill() queued in node-pty's `_deferreds`. The wrapper's kill() notes + // the close only when it really ran; releaseHost then disposes the worker + // a deferred kill would strand, and skips the native close so the queued + // kill() stays the single closer — never a second close. + session.pty.releaseHost?.(); + } else { + // The shell already exited, so nothing may signal its (possibly recycled) + // pid — but node-pty does not release its conout worker thread on a + // natural exit, so without this every terminal the user exits leaks one + // for the life of the CLI. Same defect as the shell-tool path in + // shellExecutionService. The conhost.exe half is not freed here (the + // native baton is already gone); see releaseConPtyHost. See #11303. + session.pty.releaseHost?.(); } return true; }