From 18fdefdcfd6feeb22355530c528e09ceb2c4ae92 Mon Sep 17 00:00:00 2001 From: Deepak Mohan Date: Sat, 1 Aug 2026 03:11:17 +0900 Subject: [PATCH] windows: avoid conPTY deadlock when debugger pauses conout worker The conPTY output connection is established on a worker thread before the main Node.js thread calls the native connect implementation. Calling conptyNative.connect() before that worker reports readiness is unsafe because the native implementation synchronously calls ConnectNamedPipe() for the input and output pipes. ETW stacks from the frozen Code - OSS agent host showed: agentHostTerminalManager._spawnPty -> node-pty.spawn -> WindowsTerminal -> WindowsPtyAgent -> ConoutConnection -> Worker The conout worker was stopped in the Node inspector startup message loop while processing Debugger.enable. Five seconds later, the agent-host event loop thread entered the WindowsPtyAgent timeout fallback and proceeded through conptyNative.connect() into NtFsControlFile/ConnectNamedPipe. Since the paused worker had not connected the output side, ConnectNamedPipe waited synchronously and blocked the event loop. Consequently, CDP's Runtime.enable request could not complete and the debugger attachment appeared frozen. The worker-ready handshake was originally introduced to prevent this deadlock, but a later timeout fallback called connect() anyway to avoid leaving the PTY in a zombie state. That fallback violated the handshake invariant and restored the blocking path under debugger induced worker delays. Make worker readiness a hard prerequisite for calling connect(): - Fail and clean up the pending PTY when the readiness watchdog expires instead of attempting the native connection. - Propagate worker startup errors and premature exits to WindowsPtyAgent. - Kill the pending native PTY, dispose the worker, destroy its sockets, and report the failure through onError. - Clear the watchdog after readiness, connection failure, or explicit kill. - Ignore readiness and error events arriving after timeout or termination. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/windowsConoutConnection.ts | 23 ++++++- src/windowsPtyAgent.test.ts | 121 ++++++++++++++++++++++++++++++++- src/windowsPtyAgent.ts | 66 ++++++++++++++---- 3 files changed, 194 insertions(+), 16 deletions(-) diff --git a/src/windowsConoutConnection.ts b/src/windowsConoutConnection.ts index fa2d62de1..aedf8ec97 100644 --- a/src/windowsConoutConnection.ts +++ b/src/windowsConoutConnection.ts @@ -28,14 +28,24 @@ const FLUSH_DATA_INTERVAL = 1000; * - https://github.com/microsoft/terminal/issues/1810 * - https://docs.microsoft.com/en-us/windows/console/closepseudoconsole */ -export class ConoutConnection implements IDisposable { +export interface IConoutConnection extends IDisposable { + readonly onReady: IEvent; + readonly onError: IEvent; + connectSocket(socket: Socket): void; +} + +export class ConoutConnection implements IConoutConnection { private _worker: Worker; private _drainTimeout: NodeJS.Timeout | undefined; private _isDisposed: boolean = false; + private _isReady: boolean = false; private _onReady = new EventEmitter2(); public get onReady(): IEvent { return this._onReady.event; } + private _onError = new EventEmitter2(); + public get onError(): IEvent { return this._onError.event; } + constructor( private _conoutPipeName: string, private _useConptyDll: boolean @@ -48,12 +58,23 @@ export class ConoutConnection implements IDisposable { this._worker.on('message', (message: ConoutWorkerMessage) => { switch (message) { case ConoutWorkerMessage.READY: + this._isReady = true; this._onReady.fire(); return; default: console.warn('Unexpected ConoutWorkerMessage', message); } }); + this._worker.on('error', error => { + if (!this._isDisposed && !this._isReady) { + this._onError.fire(error); + } + }); + this._worker.on('exit', code => { + if (!this._isDisposed && !this._isReady) { + this._onError.fire(new Error(`Conout worker exited before connecting (code ${code})`)); + } + }); } dispose(): void { diff --git a/src/windowsPtyAgent.test.ts b/src/windowsPtyAgent.test.ts index ccc34acd2..9d6979500 100644 --- a/src/windowsPtyAgent.test.ts +++ b/src/windowsPtyAgent.test.ts @@ -4,12 +4,50 @@ */ import * as assert from 'assert'; -import { argsToCommandLine, WindowsPtyAgent } from './windowsPtyAgent'; +import { Socket } from 'net'; +import { EventEmitter2, IEvent } from './eventEmitter2'; +import { argsToCommandLine, IWindowsPtyAgentOptions, WindowsPtyAgent } from './windowsPtyAgent'; +import { IConoutConnection } from './windowsConoutConnection'; function check(file: string, args: string | string[], expected: string): void { assert.equal(argsToCommandLine(file, args), expected); } +class TestConoutConnection implements IConoutConnection { + private readonly _onReady = new EventEmitter2(); + public get onReady(): IEvent { return this._onReady.event; } + + private readonly _onError = new EventEmitter2(); + public get onError(): IEvent { return this._onError.event; } + + public connectSocketCallCount = 0; + public isDisposed = false; + + public connectSocket(socket: Socket): void { + void socket; + this.connectSocketCallCount++; + } + + public dispose(): void { + this.isDisposed = true; + } + + public fireReady(): void { + this._onReady.fire(); + } + + public fireError(error: Error): void { + this._onError.fire(error); + } +} + +function createTestAgentOptions(connection: IConoutConnection, connectionTimeout: number): IWindowsPtyAgentOptions { + return { + connectionTimeout, + conoutConnectionFactory: () => connection + }; +} + if (process.platform === 'win32') { describe('argsToCommandLine', () => { describe('Plain strings', () => { @@ -94,6 +132,87 @@ if (process.platform === 'win32') { describe('WindowsPtyAgent', () => { describe('connection timing (issue #763)', () => { + it('should fail without connecting when the worker times out', async function () { + this.timeout(10000); + const connection = new TestConoutConnection(); + const term = new WindowsPtyAgent( + 'cmd.exe', + '/c echo test', + Object.keys(process.env).map(k => `${k}=${process.env[k]}`), + process.cwd(), + 80, + 30, + false, + false, + false, + createTestAgentOptions(connection, 10) + ); + + let eventLoopResponsive = false; + setImmediate(() => eventLoopResponsive = true); + const error = await new Promise(resolve => term.onError(resolve)); + + assert.strictEqual(error.message, 'Timed out waiting for ConPTY output worker'); + assert.strictEqual(eventLoopResponsive, true, 'event loop should remain responsive'); + assert.strictEqual(connection.connectSocketCallCount, 0); + assert.strictEqual(connection.isDisposed, true); + assert.strictEqual(term.innerPid, 0); + + connection.fireReady(); + assert.strictEqual(connection.connectSocketCallCount, 0, 'late readiness must be ignored'); + }); + + it('should fail when the worker errors before becoming ready', async () => { + const connection = new TestConoutConnection(); + const term = new WindowsPtyAgent( + 'cmd.exe', + '/c echo test', + Object.keys(process.env).map(k => `${k}=${process.env[k]}`), + process.cwd(), + 80, + 30, + false, + false, + false, + createTestAgentOptions(connection, 1000) + ); + + const expectedError = new Error('worker failed'); + const errorPromise = new Promise(resolve => term.onError(resolve)); + connection.fireError(expectedError); + const error = await errorPromise; + + assert.strictEqual(error, expectedError); + assert.strictEqual(connection.connectSocketCallCount, 0); + assert.strictEqual(connection.isDisposed, true); + assert.strictEqual(term.innerPid, 0); + }); + + it('should ignore worker events after kill before readiness', () => { + const connection = new TestConoutConnection(); + const term = new WindowsPtyAgent( + 'cmd.exe', + '/c echo test', + Object.keys(process.env).map(k => `${k}=${process.env[k]}`), + process.cwd(), + 80, + 30, + false, + false, + false, + createTestAgentOptions(connection, 1000) + ); + let errorCount = 0; + term.onError(() => errorCount++); + + term.kill(); + connection.fireReady(); + connection.fireError(new Error('late error')); + + assert.strictEqual(connection.connectSocketCallCount, 0); + assert.strictEqual(errorCount, 0); + }); + it('should defer conptyNative.connect() until worker is ready', function (done) { this.timeout(10000); diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index 06450864e..73f83d6ec 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -9,7 +9,7 @@ import * as path from 'path'; import { fork } from 'child_process'; import { Socket } from 'net'; import { ArgvOrCommandLine } from './types'; -import { ConoutConnection } from './windowsConoutConnection'; +import { ConoutConnection, IConoutConnection } from './windowsConoutConnection'; import { EventEmitter2, IEvent } from './eventEmitter2'; import { loadNativeModule } from './utils'; @@ -21,6 +21,17 @@ let conptyNative: IConptyNative; * has started. */ const FLUSH_DATA_INTERVAL = 1000; +const CONNECTION_TIMEOUT = 5000; + +export interface IWindowsPtyAgentOptions { + readonly connectionTimeout: number; + readonly conoutConnectionFactory: (conoutPipeName: string, useConptyDll: boolean) => IConoutConnection; +} + +const defaultWindowsPtyAgentOptions: IWindowsPtyAgentOptions = { + connectionTimeout: CONNECTION_TIMEOUT, + conoutConnectionFactory: (conoutPipeName, useConptyDll) => new ConoutConnection(conoutPipeName, useConptyDll) +}; /** * This agent sits between the WindowsTerminal class and provides an interface for conpty. @@ -30,8 +41,9 @@ export class WindowsPtyAgent { private _outSocket: Socket; private _innerPid: number = 0; private _closeTimeout: NodeJS.Timer | undefined; + private _connectionTimeout: NodeJS.Timeout | undefined; private _exitCode: number | undefined; - private _conoutSocketWorker: ConoutConnection; + private _conoutSocketWorker: IConoutConnection; private _onError = new EventEmitter2(); public get onError(): IEvent { return this._onError.event; } @@ -57,7 +69,8 @@ export class WindowsPtyAgent { rows: number, debug: boolean, private _useConptyDll: boolean = false, - conptyInheritCursor: boolean = false + conptyInheritCursor: boolean = false, + options: IWindowsPtyAgentOptions = defaultWindowsPtyAgentOptions ) { if (!conptyNative) { conptyNative = loadNativeModule('conpty').module; @@ -87,27 +100,29 @@ export class WindowsPtyAgent { // We must wait for the worker to connect before calling conptyNative.connect() // to avoid blocking the Node.js event loop in ConnectNamedPipe. // See https://github.com/microsoft/node-pty/issues/763 - this._conoutSocketWorker = new ConoutConnection(term.conout, this._useConptyDll); + this._conoutSocketWorker = options.conoutConnectionFactory(term.conout, this._useConptyDll); // Store pending connection info - we'll complete the connection when worker is ready this._pendingPtyInfo = { pty: this._pty, commandLine, cwd, env }; - // Timeout to ensure connection completes even if worker fails to signal ready - const connectionTimeout = setTimeout(() => { - if (this._pendingPtyInfo) { - // Worker never signaled ready - complete connection anyway to avoid zombie state - this._completePtyConnection(); - } - }, 5000); + // Never call connect() before the worker is ready, as ConnectNamedPipe + // would block the Node.js event loop while waiting for the output client. + this._connectionTimeout = setTimeout(() => { + this._failPtyConnection(new Error('Timed out waiting for ConPTY output worker')); + }, options.connectionTimeout); this._conoutSocketWorker.onReady(() => { - clearTimeout(connectionTimeout); + if (!this._pendingPtyInfo) { + return; + } + this._clearConnectionTimeout(); this._conoutSocketWorker.connectSocket(this._outSocket); // Now that the worker has connected to the output pipe, we can safely call // conptyNative.connect() which calls ConnectNamedPipe - it won't block because // the client (worker) is already connected this._completePtyConnection(); }); + this._conoutSocketWorker.onError(error => this._failPtyConnection(error)); this._outSocket.on('connect', () => { this._outSocket.emit('ready_datapipe'); }); @@ -125,6 +140,7 @@ export class WindowsPtyAgent { if (!this._pendingPtyInfo) { return; } + this._clearConnectionTimeout(); const { pty, commandLine, cwd, env } = this._pendingPtyInfo; this._pendingPtyInfo = undefined; @@ -132,8 +148,8 @@ export class WindowsPtyAgent { const connect = conptyNative.connect(pty, commandLine, cwd, env, this._useConptyDll, c => this._$onProcessExit(c)); this._innerPid = connect.pid; } catch (err) { - // connect() runs from the conout worker's onReady callback (or its - // timeout fallback), so a throw here would otherwise surface as an + // connect() runs from the conout worker's onReady callback, so a throw + // here would otherwise surface as an // uncaughtException with no way for the consumer to observe it. const code = /error code: (\d+)/.exec((err as Error).message)?.[1]; this._exitCode = code ? parseInt(code, 10) : -1; @@ -145,6 +161,27 @@ export class WindowsPtyAgent { } } + private _failPtyConnection(error: Error): void { + if (!this._pendingPtyInfo) { + return; + } + this._clearConnectionTimeout(); + this._pendingPtyInfo = undefined; + this._exitCode = -1; + try { this._ptyNative.kill(this._pty, this._useConptyDll); } catch { /* already gone */ } + this._conoutSocketWorker.dispose(); + this._inSocket.destroy(); + this._outSocket.destroy(); + this._onError.fire(error); + } + + private _clearConnectionTimeout(): void { + if (this._connectionTimeout) { + clearTimeout(this._connectionTimeout); + this._connectionTimeout = undefined; + } + } + public resize(cols: number, rows: number): void { if (this._exitCode !== undefined) { throw new Error('Cannot resize a pty that has already exited'); @@ -158,6 +195,7 @@ export class WindowsPtyAgent { public kill(): void { // Prevent deferred connection from completing after kill + this._clearConnectionTimeout(); this._pendingPtyInfo = undefined; // Tell the agent to kill the pty, this releases handles to the process