windows: avoid conPTY deadlock when debugger pauses conout worker

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<void>;
+  readonly onError: IEvent<Error>;
+  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<void>();
   public get onReady(): IEvent<void> { return this._onReady.event; }
 
+  private _onError = new EventEmitter2<Error>();
+  public get onError(): IEvent<Error> { 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 {

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<void>();
+  public get onReady(): IEvent<void> { return this._onReady.event; }
+
+  private readonly _onError = new EventEmitter2<Error>();
+  public get onError(): IEvent<Error> { 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<Error>(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<Error>(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);
 

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<Error>();
   public get onError(): IEvent<Error> { 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,15 +140,16 @@ export class WindowsPtyAgent {
     if (!this._pendingPtyInfo) {
       return;
     }
+    this._clearConnectionTimeout();
     const { pty, commandLine, cwd, env } = this._pendingPtyInfo;
     this._pendingPtyInfo = undefined;
 
     try {
       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