diff --git a/binding.gyp b/binding.gyp index 5f63978b07ab50aaf7523219a2170ec737a6b5db..e23988358eecc292ca769a738d5bfdef8172bb3d 100644 --- a/binding.gyp +++ b/binding.gyp @@ -1,4 +1,7 @@ { + 'variables': { + 'node_pty_test_faults%': 0, + }, 'target_defaults': { 'dependencies': [ "= 18309; } + // Why: a requested Job is effective only when runtime detection actually selects ConPTY. + this._windowsAgentJob = Boolean(this._useConpty && windowsAgentJob); if (this._useConpty) { if (!conptyNative) { conptyNative = utils_1.loadNativeModule('conpty').module; @@ -86,7 +92,7 @@ var WindowsPtyAgent = /** @class */ (function () { }); this._inSocket.setEncoding('utf8'); if (this._useConpty) { - var connect = this._ptyNative.connect(this._pty, commandLine, cwd, env, this._useConptyDll, function (c) { return _this._$onProcessExit(c); }); + var connect = this._ptyNative.connect(this._pty, commandLine, cwd, env, this._useConptyDll, this._windowsAgentJob, function (c) { return _this._$onProcessExit(c); }); this._innerPid = connect.pid; } } @@ -115,6 +121,11 @@ var WindowsPtyAgent = /** @class */ (function () { enumerable: false, configurable: true }); + Object.defineProperty(WindowsPtyAgent.prototype, "windowsAgentJob", { + get: function () { return this._windowsAgentJob; }, + enumerable: false, + configurable: true + }); WindowsPtyAgent.prototype.resize = function (cols, rows) { if (this._useConpty) { if (this._exitCode !== undefined) { @@ -130,6 +141,23 @@ var WindowsPtyAgent = /** @class */ (function () { this._ptyNative.clear(this._pty, this._useConptyDll); } }; + WindowsPtyAgent.prototype.terminateJobTree = function (timeoutMs) { + var _a, _b; + if (!this._windowsAgentJob) { + return undefined; + } + if (this._processExited) { + return Promise.resolve(true); + } + return (_b = (_a = this._ptyNative).terminateJobTree) === null || _b === void 0 ? void 0 : _b.call(_a, this._pty, timeoutMs); + }; + WindowsPtyAgent.prototype.onProcessExit = function (listener) { + if (this._processExited) { + listener(); + return; + } + this._processExitListeners.push(listener); + }; WindowsPtyAgent.prototype.kill = function () { var _this = this; // Tell the agent to kill the pty, this releases handles to the process @@ -221,6 +249,10 @@ var WindowsPtyAgent = /** @class */ (function () { WindowsPtyAgent.prototype._$onProcessExit = function (exitCode) { var _this = this; this._exitCode = exitCode; + this._processExited = true; + var listeners = this._processExitListeners; + this._processExitListeners = []; + listeners.forEach(function (listener) { return listener(); }); if (!this._useConptyDll) { this._flushDataAndCleanUp(); this._outSocket.on('data', function () { return _this._flushDataAndCleanUp(); }); diff --git a/lib/windowsPtyAgent.test.js b/lib/windowsPtyAgent.test.js index 15bbf5ba397ab02a5ec6a11b1c0c6f6ffd137be0..d9a0cf1af2da1cdd56ff6edae0bfa41580b51c60 100644 --- a/lib/windowsPtyAgent.test.js +++ b/lib/windowsPtyAgent.test.js @@ -10,6 +10,26 @@ function check(file, args, expected) { assert.equal(windowsPtyAgent_1.argsToCommandLine(file, args), expected); } if (process.platform === 'win32') { + describe('Windows agent Job control', function () { + it('reports an already-exited owned Job as drained without another native lookup', function () { + var terminateJobTree = function () { return Promise.resolve(false); }; + var agent = Object.create(windowsPtyAgent_1.WindowsPtyAgent.prototype); + agent._windowsAgentJob = true; + agent._processExited = true; + agent._pty = 1; + agent._ptyNative = { terminateJobTree: terminateJobTree }; + assert.strictEqual(agent.windowsAgentJob, true); + return agent.terminateJobTree(1000).then(function (drained) { + assert.strictEqual(drained, true); + }); + }); + it('does not expose teardown when Job ownership was not established', function () { + var agent = Object.create(windowsPtyAgent_1.WindowsPtyAgent.prototype); + agent._windowsAgentJob = false; + assert.strictEqual(agent.windowsAgentJob, false); + assert.strictEqual(agent.terminateJobTree(1000), undefined); + }); + }); describe('argsToCommandLine', function () { describe('Plain strings', function () { it('doesn\'t quote plain string', function () { diff --git a/lib/windowsTerminal.js b/lib/windowsTerminal.js index 3c38f89d14cece8081782a9600ebc582b7c2a62a..52e47c98194daf6f64b22fcaa647077acf989371 100644 --- a/lib/windowsTerminal.js +++ b/lib/windowsTerminal.js @@ -48,8 +48,23 @@ var WindowsTerminal = /** @class */ (function (_super) { // Functions that need to run after `ready` event is emitted. _this._deferreds = []; // Create new termal. - _this._agent = new windowsPtyAgent_1.WindowsPtyAgent(file, args, parsedEnv, cwd, _this._cols, _this._rows, false, opt.useConpty, opt.useConptyDll, opt.conptyInheritCursor); + _this._agent = new windowsPtyAgent_1.WindowsPtyAgent(file, args, parsedEnv, cwd, _this._cols, _this._rows, false, opt.useConpty, opt.useConptyDll, opt.conptyInheritCursor, opt.windowsAgentJob); _this._socket = _this._agent.outSocket; + var socketClosed = false; + var agentProcessExited = false; + var exitEmitted = false; + var emitExitWhenSafe = function () { + if (exitEmitted || !socketClosed || (_this._agent.windowsAgentJob && !agentProcessExited)) { + return; + } + exitEmitted = true; + _this.emit('exit', _this._agent.exitCode); + _this._close(); + }; + _this._agent.onProcessExit(function () { + agentProcessExited = true; + emitExitWhenSafe(); + }); // Not available until `ready` event emitted. _this._pid = _this._agent.innerPid; _this._fd = _this._agent.fd; @@ -95,8 +110,10 @@ var WindowsTerminal = /** @class */ (function (_super) { }); // Cleanup after the socket is closed. _this._socket.on('close', function () { - _this.emit('exit', _this._agent.exitCode); - _this._close(); + // Why: a Job-owned root can close ConPTY output before the native exit + // callback proves every descendant is gone; public exit needs both facts. + socketClosed = true; + emitExitWhenSafe(); }); }); _this._file = file; @@ -144,6 +161,9 @@ var WindowsTerminal = /** @class */ (function (_super) { _this.kill(); }); }; + WindowsTerminal.prototype.terminateJobTree = function (timeoutMs) { + return this._agent.terminateJobTree(timeoutMs); + }; WindowsTerminal.prototype.kill = function (signal) { var _this = this; this._deferNoArgs(function () { diff --git a/lib/windowsTerminal.test.js b/lib/windowsTerminal.test.js index af5f343d17f012f330fe2292083f297768ff3b5e..6b558c789fc77b58c20cd9564d03e93e54164f54 100644 --- a/lib/windowsTerminal.test.js +++ b/lib/windowsTerminal.test.js @@ -88,6 +88,19 @@ function pollForProcessTreeSize(pid, size, intervalMs, timeoutMs) { }); } if (process.platform === 'win32') { + describe('WindowsTerminal Job capability fallback', function () { + it('does not wait for a Job callback when a requested Job falls back to winpty', function (done) { + this.timeout(10000); + var term = new windowsTerminal_1.WindowsTerminal('cmd.exe', '/C exit', { + useConpty: false, + windowsAgentJob: true + }); + term.on('exit', function (code) { + assert.strictEqual(code, 0); + done(); + }); + }); + }); [[false, false], [true, false], [true, true]].forEach(function (_a) { var useConpty = _a[0], useConptyDll = _a[1]; describe("WindowsTerminal (useConpty = " + useConpty + ", useConptyDll = " + useConptyDll + ")", function () { diff --git a/src/conptyConnectFailure.test.ts b/src/conptyConnectFailure.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..70130579acf6c9592266f4834aa4d34f6e95b59f --- /dev/null +++ b/src/conptyConnectFailure.test.ts @@ -0,0 +1,139 @@ +/** + * Copyright (c) 2026, Stably AI (MIT License). + */ + +import assert = require('assert'); +import fs = require('fs'); +import { loadNativeModule } from './utils'; + +interface ITestConptyNative { + startProcess( + file: string, + cols: number, + rows: number, + debug: boolean, + pipeName: string, + inheritCursor: boolean, + useConptyDll: boolean + ): { pty: number; conin: string; conout: string }; + connect( + pty: number, + commandLine: string, + cwd: string, + env: string[], + useConptyDll: boolean, + windowsAgentJob: boolean, + callback: (exitCode: number) => void + ): { pid: number }; + setTestConnectFailure?(failure: string): void; + getTestResourceState?(): ITestResourceState; +} + +interface ITestResourceState { + ptyCount: number; + closedPseudoConsoleCount: number; + closedPipeHandleCount: number; + closedProcessHandleCount: number; + closedThreadHandleCount: number; + closedJobHandleCount: number; + drainedFailedProcessCount: number; +} + +if (process.platform === 'win32') { + const native = loadNativeModule('conpty').module as ITestConptyNative; + const failureCases = [ + ['assign-job', 'Cannot assign process to Job, error code: 5'], + ['resume-thread', 'Cannot resume process, error code: 6'] + ]; + + const hasTestHooks = (): boolean => + Boolean(native.setTestConnectFailure && native.getTestResourceState); + + const runFailureCase = (failure: string, expectedError: string): void => { + assert.ok(native.setTestConnectFailure && native.getTestResourceState); + const pipeName = `node-pty-connect-failure-${process.pid}-${failure}`; + const before = native.getTestResourceState(); + native.setTestConnectFailure(failure); + try { + // Reusing the same first-instance pipe names proves each failed attempt + // closed both server pipes before removing its baton. + for (let attempt = 1; attempt <= 3; attempt++) { + const term = native.startProcess( + process.env.ComSpec || 'cmd.exe', + 80, + 30, + false, + pipeName, + false, + false + ); + const inputFd = fs.openSync(term.conin, 'w'); + const outputFd = fs.openSync(term.conout, 'r'); + try { + assert.throws( + () => native.connect( + term.pty, + 'cmd.exe /C exit', + process.cwd(), + [], + false, + true, + () => undefined + ), + new RegExp(expectedError) + ); + } finally { + fs.closeSync(inputFd); + fs.closeSync(outputFd); + } + + const resourceState: ITestResourceState = native.getTestResourceState(); + assert.strictEqual(resourceState.ptyCount, before.ptyCount); + assert.strictEqual( + resourceState.closedPseudoConsoleCount, + before.closedPseudoConsoleCount + attempt + ); + assert.strictEqual( + resourceState.closedPipeHandleCount, + before.closedPipeHandleCount + attempt * 2 + ); + assert.strictEqual( + resourceState.closedProcessHandleCount, + before.closedProcessHandleCount + attempt + ); + assert.strictEqual( + resourceState.closedThreadHandleCount, + before.closedThreadHandleCount + attempt + ); + assert.strictEqual( + resourceState.closedJobHandleCount, + before.closedJobHandleCount + attempt + ); + assert.strictEqual( + resourceState.drainedFailedProcessCount, + before.drainedFailedProcessCount + attempt + ); + } + } finally { + native.setTestConnectFailure('none'); + } + }; + + if (typeof describe === 'function') { + describe('ConPTY failed-connect cleanup', () => { + failureCases.forEach(([failure, expectedError]) => { + it(`releases every native PTY resource after ${failure}`, function() { + if (!hasTestHooks()) { + this.skip(); + return; + } + runFailureCase(failure, expectedError); + }); + }); + }); + } else { + assert.ok(hasTestHooks(), 'ConPTY test fault injection was required but is unavailable'); + failureCases.forEach(([failure, expectedError]) => runFailureCase(failure, expectedError)); + console.log('ConPTY failed-connect cleanup: 2 passed'); + } +} diff --git a/src/conpty_console_list_agent.ts b/src/conpty_console_list_agent.ts index 181ccabbbe9c4948a9725fb1db907a68e9de01fc..67f31facf85562b67adbfbd04ce28ddd8eeb4a79 100644 --- a/src/conpty_console_list_agent.ts +++ b/src/conpty_console_list_agent.ts @@ -10,6 +10,12 @@ import { loadNativeModule } from './utils'; const getConsoleProcessList = loadNativeModule('conpty_console_list').module.getConsoleProcessList; const shellPid = parseInt(process.argv[2], 10); -const consoleProcessList = getConsoleProcessList(shellPid); +let consoleProcessList: number[]; +try { + consoleProcessList = getConsoleProcessList(shellPid); +} catch { + // Why: AttachConsole can fail after the shell exits; parent already has this fallback. + consoleProcessList = [shellPid]; +} process.send!({ consoleProcessList }); process.exit(0); diff --git a/src/interfaces.ts b/src/interfaces.ts index a269e77bd2acf31c762b0fd8e13e6bf435fd7d07..adc2e8d10190de6bbeea1f2b53320ef92e0b2935 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -121,6 +121,8 @@ export interface IWindowsPtyForkOptions extends IBasePtyForkOptions { useConpty?: boolean; useConptyDll?: boolean; conptyInheritCursor?: boolean; + /** Orca-specific: own this ConPTY and all descendants in a kill-on-close Job. */ + windowsAgentJob?: boolean; } export interface IPtyOpenOptions { diff --git a/src/native.d.ts b/src/native.d.ts index c53e086b4968d6fb0bf601ace3a813809433fedb..ce2d197bbf618f17cdcbf1e6127e831715a8beb3 100644 --- a/src/native.d.ts +++ b/src/native.d.ts @@ -4,10 +4,11 @@ interface IConptyNative { startProcess(file: string, cols: number, rows: number, debug: boolean, pipeName: string, conptyInheritCursor: boolean, useConptyDll: boolean): IConptyProcess; - connect(ptyId: number, commandLine: string, cwd: string, env: string[], useConptyDll: boolean, onExitCallback: (exitCode: number) => void): { pid: number }; + connect(ptyId: number, commandLine: string, cwd: string, env: string[], useConptyDll: boolean, windowsAgentJob: boolean, onExitCallback: (exitCode: number) => void): { pid: number }; resize(ptyId: number, cols: number, rows: number, useConptyDll: boolean): void; clear(ptyId: number, useConptyDll: boolean): void; kill(ptyId: number, useConptyDll: boolean): void; + terminateJobTree?(ptyId: number, timeoutMs: number): Promise | undefined; } interface IWinptyNative { diff --git a/src/unix/pty.cc b/src/unix/pty.cc index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..61f39f0cbb91faa2c515f35d2ca850564e6368d9 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -23,7 +23,9 @@ #include #include #include +#include #include +#include #include #include @@ -237,13 +239,23 @@ pty_getproc(int, char *); #endif #if defined(__APPLE__) || defined(__OpenBSD__) +struct pty_spawn_error { + const char* step; + int errnum; + std::string detail_name; + std::string detail_value; +}; + +static std::string +pty_format_spawn_error(const pty_spawn_error&); + static void pty_posix_spawn(char** argv, char** env, const struct termios *termp, const struct winsize *winp, int* master, pid_t* pid, - int* err); + pty_spawn_error* err); #endif struct DelBuf { @@ -367,10 +379,11 @@ Napi::Value PtyFork(const Napi::CallbackInfo& info) { argv[i + 3] = strdup(arg.c_str()); } - int err = -1; - pty_posix_spawn(argv, env, term, &winp, &master, &pid, &err); - if (err != 0) { - throw Napi::Error::New(napiEnv, "posix_spawnp failed."); + pty_spawn_error spawn_error = { NULL, 0, "", "" }; + pty_posix_spawn(argv, env, term, &winp, &master, &pid, &spawn_error); + if (spawn_error.errnum != 0) { + std::string spawn_message = pty_format_spawn_error(spawn_error); + throw Napi::Error::New(napiEnv, spawn_message); } if (pty_nonblock(master) == -1) { throw Napi::Error::New(napiEnv, "Could not set master fd to nonblocking."); @@ -684,15 +697,73 @@ pty_getproc(int fd, char *tty) { #endif #if defined(__APPLE__) +static const char* +pty_errno_name(int errnum) { + switch (errnum) { + case E2BIG: return "E2BIG"; + case EACCES: return "EACCES"; + case EAGAIN: return "EAGAIN"; + case EMFILE: return "EMFILE"; + case ENFILE: return "ENFILE"; + case ENOENT: return "ENOENT"; + case ENOMEM: return "ENOMEM"; + default: return "errno"; + } +} + +static void +pty_set_spawn_error(pty_spawn_error* err, + const char* step, + int errnum, + const char* detail_name = NULL, + const char* detail_value = NULL) { + err->step = step; + err->errnum = errnum; + err->detail_name = detail_name ? detail_name : ""; + err->detail_value = detail_value ? detail_value : ""; +} + +static std::string +pty_format_spawn_error(const pty_spawn_error& err) { + char errno_buf[64]; + snprintf(errno_buf, sizeof(errno_buf), "%d", err.errnum); + + std::string message = "node-pty: "; + message += err.step ? err.step : "unknown"; + message += " failed: "; + message += pty_errno_name(err.errnum); + message += " (errno "; + message += errno_buf; + message += ", "; + message += strerror(err.errnum); + message += ")"; + + if (!err.detail_name.empty()) { + message += " - "; + message += err.detail_name; + message += "='"; + message += err.detail_value; + message += "'"; + } + + return message; +} + static void pty_posix_spawn(char** argv, char** env, const struct termios *termp, const struct winsize *winp, int* master, pid_t* pid, - int* err) { - int low_fds[3]; + pty_spawn_error* err) { + int low_fds[3] = {-1, -1, -1}; size_t count = 0; + int res = -1; + int slave = -1; + posix_spawn_file_actions_t acts; + bool acts_initialized = false; + posix_spawnattr_t attrs; + bool attrs_initialized = false; for (; count < 3; count++) { low_fds[count] = posix_openpt(O_RDWR); @@ -706,80 +777,118 @@ pty_posix_spawn(char** argv, char** env, POSIX_SPAWN_SETSID; *master = posix_openpt(O_RDWR); if (*master == -1) { - return; + pty_set_spawn_error(err, "posix_openpt", errno); + goto done; + } + + res = grantpt(*master); + if (res == -1) { + pty_set_spawn_error(err, "grantpt", errno); + goto done; } - int res = grantpt(*master) || unlockpt(*master); + res = unlockpt(*master); if (res == -1) { - return; + pty_set_spawn_error(err, "unlockpt", errno); + goto done; } // Use TIOCPTYGNAME instead of ptsname() to avoid threading problems. - int slave; char slave_pty_name[128]; res = ioctl(*master, TIOCPTYGNAME, slave_pty_name); if (res == -1) { - return; + pty_set_spawn_error(err, "ioctl_TIOCPTYGNAME", errno); + goto done; } slave = open(slave_pty_name, O_RDWR | O_NOCTTY); if (slave == -1) { - return; + pty_set_spawn_error(err, "open_slave", errno, "slave", slave_pty_name); + goto done; } if (termp) { res = tcsetattr(slave, TCSANOW, termp); if (res == -1) { - return; + pty_set_spawn_error(err, "tcsetattr", errno, "slave", slave_pty_name); + goto done; }; } if (winp) { res = ioctl(slave, TIOCSWINSZ, winp); if (res == -1) { - return; + pty_set_spawn_error(err, "ioctl_TIOCSWINSZ", errno, "slave", slave_pty_name); + goto done; } } - posix_spawn_file_actions_t acts; - posix_spawn_file_actions_init(&acts); + res = posix_spawn_file_actions_init(&acts); + if (res != 0) { + pty_set_spawn_error(err, "posix_spawn_file_actions_init", res); + goto done; + } + acts_initialized = true; posix_spawn_file_actions_adddup2(&acts, slave, STDIN_FILENO); posix_spawn_file_actions_adddup2(&acts, slave, STDOUT_FILENO); posix_spawn_file_actions_adddup2(&acts, slave, STDERR_FILENO); posix_spawn_file_actions_addclose(&acts, slave); posix_spawn_file_actions_addclose(&acts, *master); - posix_spawnattr_t attrs; - posix_spawnattr_init(&attrs); - *err = posix_spawnattr_setflags(&attrs, flags); - if (*err != 0) { + res = posix_spawnattr_init(&attrs); + if (res != 0) { + pty_set_spawn_error(err, "posix_spawnattr_init", res); + goto done; + } + attrs_initialized = true; + res = posix_spawnattr_setflags(&attrs, flags); + if (res != 0) { + pty_set_spawn_error(err, "posix_spawnattr_setflags", res); goto done; } sigset_t signal_set; /* Reset all signal the child to their default behavior */ sigfillset(&signal_set); - *err = posix_spawnattr_setsigdefault(&attrs, &signal_set); - if (*err != 0) { + res = posix_spawnattr_setsigdefault(&attrs, &signal_set); + if (res != 0) { + pty_set_spawn_error(err, "posix_spawnattr_setsigdefault", res); goto done; } /* Reset the signal mask for all signals */ sigemptyset(&signal_set); - *err = posix_spawnattr_setsigmask(&attrs, &signal_set); - if (*err != 0) { + res = posix_spawnattr_setsigmask(&attrs, &signal_set); + if (res != 0) { + pty_set_spawn_error(err, "posix_spawnattr_setsigmask", res); goto done; } do - *err = posix_spawn(pid, argv[0], &acts, &attrs, argv, env); - while (*err == EINTR); + res = posix_spawn(pid, argv[0], &acts, &attrs, argv, env); + while (res == EINTR); + if (res != 0) { + pty_set_spawn_error(err, "posix_spawn", res, "helper", argv[0]); + } done: - posix_spawn_file_actions_destroy(&acts); - posix_spawnattr_destroy(&attrs); + if (acts_initialized) { + posix_spawn_file_actions_destroy(&acts); + } + if (attrs_initialized) { + posix_spawnattr_destroy(&attrs); + } + if (slave != -1) { + close(slave); + } + if (err->errnum != 0 && *master != -1) { + close(*master); + *master = -1; + } - for (; count > 0; count--) { - close(low_fds[count]); + for (size_t i = 0; i <= count && i < 3; i++) { + if (low_fds[i] != -1) { + close(low_fds[i]); + } } } #endif diff --git a/src/win/conpty.cc b/src/win/conpty.cc index 7b286d3d644c26141df516929703aa6e129df4b2..4ac5716a6678e8ccfdd92ffe1f6529f231875265 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -45,12 +45,37 @@ struct pty_baton { HPCON hpc; HANDLE hShell; - - pty_baton(int _id, HANDLE _hIn, HANDLE _hOut, HPCON _hpc) : id(_id), hIn(_hIn), hOut(_hOut), hpc(_hpc) {}; + HANDLE hJob = nullptr; + // Why: failed connect cleanup must close HPCON without a second DLL load + // that can itself throw and strand the original resources. + PFNCLOSEPSEUDOCONSOLE closePseudoConsole; + PFNRELEASEPSEUDOCONSOLE releasePseudoConsole; + + pty_baton( + int _id, + HANDLE _hIn, + HANDLE _hOut, + HPCON _hpc, + PFNCLOSEPSEUDOCONSOLE _closePseudoConsole, + PFNRELEASEPSEUDOCONSOLE _releasePseudoConsole) : + id(_id), + hIn(_hIn), + hOut(_hOut), + hpc(_hpc), + closePseudoConsole(_closePseudoConsole), + releasePseudoConsole(_releasePseudoConsole) {}; }; static std::vector> ptyHandles; static volatile LONG ptyCounter; +#ifdef NODE_PTY_TEST_FAULTS +static LONG closedPseudoConsoleCount = 0; +static LONG closedPipeHandleCount = 0; +static LONG closedProcessHandleCount = 0; +static LONG closedThreadHandleCount = 0; +static LONG closedJobHandleCount = 0; +static LONG drainedFailedProcessCount = 0; +#endif static pty_baton* get_pty_baton(int id) { auto it = std::find_if(ptyHandles.begin(), ptyHandles.end(), [id](const auto& ptyHandle) { @@ -73,6 +98,117 @@ static bool remove_pty_baton(int id) { return false; } +static void close_pty_resources(pty_baton* handle) { + if (handle->hpc != nullptr) { + handle->closePseudoConsole(handle->hpc); + handle->hpc = nullptr; +#ifdef NODE_PTY_TEST_FAULTS + InterlockedIncrement(&closedPseudoConsoleCount); +#endif + } + if (handle->hIn != INVALID_HANDLE_VALUE) { + CloseHandle(handle->hIn); + handle->hIn = INVALID_HANDLE_VALUE; +#ifdef NODE_PTY_TEST_FAULTS + InterlockedIncrement(&closedPipeHandleCount); +#endif + } + if (handle->hOut != INVALID_HANDLE_VALUE) { + CloseHandle(handle->hOut); + handle->hOut = INVALID_HANDLE_VALUE; +#ifdef NODE_PTY_TEST_FAULTS + InterlockedIncrement(&closedPipeHandleCount); +#endif + } +} + +static void cleanup_failed_connect( + pty_baton* handle, + PROCESS_INFORMATION* process, + PPROC_THREAD_ATTRIBUTE_LIST attributeList) { + if (process->hProcess != nullptr) { + TerminateProcess(process->hProcess, 1); + const DWORD waitResult = WaitForSingleObject(process->hProcess, INFINITE); + CloseHandle(process->hProcess); + process->hProcess = nullptr; +#ifdef NODE_PTY_TEST_FAULTS + InterlockedIncrement(&closedProcessHandleCount); + if (waitResult == WAIT_OBJECT_0) { + InterlockedIncrement(&drainedFailedProcessCount); + } +#else + (void)waitResult; +#endif + } + if (process->hThread != nullptr) { + CloseHandle(process->hThread); + process->hThread = nullptr; +#ifdef NODE_PTY_TEST_FAULTS + InterlockedIncrement(&closedThreadHandleCount); +#endif + } + if (handle->hJob != nullptr) { + CloseHandle(handle->hJob); + handle->hJob = nullptr; +#ifdef NODE_PTY_TEST_FAULTS + InterlockedIncrement(&closedJobHandleCount); +#endif + } + if (attributeList != nullptr) { + DeleteProcThreadAttributeList(attributeList); + } + close_pty_resources(handle); + const int id = handle->id; + assert(remove_pty_baton(id)); +} + +#ifdef NODE_PTY_TEST_FAULTS +// Why: CI must exercise Win32 failure branches that cannot be triggered +// deterministically through the production API. +enum class ConnectFailurePoint { + None, + AssignJob, + ResumeThread +}; + +static ConnectFailurePoint connectFailurePoint = ConnectFailurePoint::None; + +static Napi::Value PtySetTestConnectFailure(const Napi::CallbackInfo& info) { + if (info.Length() != 1 || !info[0].IsString()) { + throw Napi::Error::New(info.Env(), "Usage: pty.setTestConnectFailure(failure)"); + } + const std::string failure = info[0].As().Utf8Value(); + if (failure == "assign-job") { + connectFailurePoint = ConnectFailurePoint::AssignJob; + } else if (failure == "resume-thread") { + connectFailurePoint = ConnectFailurePoint::ResumeThread; + } else { + connectFailurePoint = ConnectFailurePoint::None; + } + return info.Env().Undefined(); +} + +static Napi::Value PtyGetTestResourceState(const Napi::CallbackInfo& info) { + auto result = Napi::Object::New(info.Env()); + result.Set("ptyCount", Napi::Number::New(info.Env(), ptyHandles.size())); + result.Set( + "closedPseudoConsoleCount", + Napi::Number::New(info.Env(), closedPseudoConsoleCount)); + result.Set("closedPipeHandleCount", Napi::Number::New(info.Env(), closedPipeHandleCount)); + result.Set( + "closedProcessHandleCount", + Napi::Number::New(info.Env(), closedProcessHandleCount)); + result.Set( + "closedThreadHandleCount", + Napi::Number::New(info.Env(), closedThreadHandleCount)); + result.Set("closedJobHandleCount", Napi::Number::New(info.Env(), closedJobHandleCount)); + result.Set( + "drainedFailedProcessCount", + Napi::Number::New(info.Env(), drainedFailedProcessCount)); + return result; +} +#endif + struct ExitEvent { int exit_code = 0; }; @@ -91,7 +227,18 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { delete th; }); *th = std::thread([tsfn = std::move(tsfn), baton] { - auto callback = [](Napi::Env env, Napi::Function cb, ExitEvent *exit_event) { + auto callback = [baton](Napi::Env env, Napi::Function cb, ExitEvent *exit_event) { + if (baton->hShell != nullptr) { + GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code)); + CloseHandle(baton->hShell); + baton->hShell = nullptr; + } + HANDLE hJob = baton->hJob; + baton->hJob = nullptr; + assert(remove_pty_baton(baton->id)); + if (hJob != nullptr) { + CloseHandle(hJob); + } cb.Call({Napi::Number::New(env, exit_event->exit_code)}); delete exit_event; }; @@ -99,15 +246,41 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { ExitEvent *exit_event = new ExitEvent; // Wait for process to complete. WaitForSingleObject(baton->hShell, INFINITE); - // Get process exit code. - GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code)); - // Clean up handles - CloseHandle(baton->hShell); - assert(remove_pty_baton(baton->id)); + + if (baton->hJob != nullptr) { + // Why: an agent root can exit before its detached tools. Do not publish + // PTY exit until Windows proves every member has released its handles. + TerminateJobObject(baton->hJob, 1); + while (true) { + JOBOBJECT_BASIC_ACCOUNTING_INFORMATION accounting{}; + if (QueryInformationJobObject( + baton->hJob, + JobObjectBasicAccountingInformation, + &accounting, + sizeof(accounting), + nullptr) && accounting.ActiveProcesses == 0) { + break; + } + // A permanent query/termination failure deliberately keeps the PTY + // owned and un-reaped so destructive callers cannot race deletion. + Sleep(25); + } + } auto status = tsfn.BlockingCall(exit_event, callback); // In main thread switch (status) { case napi_closing: + // The environment no longer accepts JS calls, so no native method can + // race these per-baton handles during finalization. + if (baton->hShell != nullptr) { + CloseHandle(baton->hShell); + baton->hShell = nullptr; + } + if (baton->hJob != nullptr) { + CloseHandle(baton->hJob); + baton->hJob = nullptr; + } + delete exit_event; break; case napi_queue_full: @@ -194,6 +367,8 @@ HRESULT CreateNamedPipesAndPseudoConsole(const Napi::CallbackInfo& info, HANDLE *phInput, HANDLE *phOutput, HPCON* phPC, + PFNCLOSEPSEUDOCONSOLE* pfnClose, + PFNRELEASEPSEUDOCONSOLE* pfnRelease, std::wstring& inName, std::wstring& outName, const std::wstring& pipeName, @@ -214,6 +389,18 @@ HRESULT CreateNamedPipesAndPseudoConsole(const Napi::CallbackInfo& info, return E_INVALIDARG; } + *pfnClose = (PFNCLOSEPSEUDOCONSOLE)GetProcAddress( + (HMODULE)hLibrary, + useConptyDll ? "ConptyClosePseudoConsole" : "ClosePseudoConsole"); + *pfnRelease = useConptyDll + ? (PFNRELEASEPSEUDOCONSOLE)GetProcAddress( + (HMODULE)hLibrary, + "ConptyReleasePseudoConsole") + : nullptr; + if (*pfnClose == nullptr) { + return HRESULT_FROM_WIN32(GetLastError()); + } + bool success = createDataServerPipe(true, L"in", phInput, inName, pipeName); if (!success) { @@ -289,7 +476,21 @@ static Napi::Value PtyStartProcess(const Napi::CallbackInfo& info) { HANDLE hIn, hOut; HPCON hpc; - HRESULT hr = CreateNamedPipesAndPseudoConsole(info, {cols, rows}, inheritCursor ? 1/*PSEUDOCONSOLE_INHERIT_CURSOR*/ : 0, &hIn, &hOut, &hpc, inName, outName, pipeName, useConptyDll); + PFNCLOSEPSEUDOCONSOLE closePseudoConsole; + PFNRELEASEPSEUDOCONSOLE releasePseudoConsole; + HRESULT hr = CreateNamedPipesAndPseudoConsole( + info, + {cols, rows}, + inheritCursor ? 1/*PSEUDOCONSOLE_INHERIT_CURSOR*/ : 0, + &hIn, + &hOut, + &hpc, + &closePseudoConsole, + &releasePseudoConsole, + inName, + outName, + pipeName, + useConptyDll); // Restore default handling of ctrl+c SetConsoleCtrlHandler(NULL, FALSE); @@ -302,7 +503,13 @@ static Napi::Value PtyStartProcess(const Napi::CallbackInfo& info) { const int ptyId = InterlockedIncrement(&ptyCounter); marshal.Set("pty", Napi::Number::New(env, ptyId)); ptyHandles.emplace_back( - std::make_unique(ptyId, hIn, hOut, hpc)); + std::make_unique( + ptyId, + hIn, + hOut, + hpc, + closePseudoConsole, + releasePseudoConsole)); } else { throw Napi::Error::New(env, "Cannot launch conpty"); } @@ -333,14 +540,15 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { std::stringstream errorText; BOOL fSuccess = FALSE; - if (info.Length() != 6 || + if (info.Length() != 7 || !info[0].IsNumber() || !info[1].IsString() || !info[2].IsString() || !info[3].IsArray() || !info[4].IsBoolean() || - !info[5].IsFunction()) { - throw Napi::Error::New(env, "Usage: pty.connect(id, cmdline, cwd, env, useConptyDll, exitCallback)"); + !info[5].IsBoolean() || + !info[6].IsFunction()) { + throw Napi::Error::New(env, "Usage: pty.connect(id, cmdline, cwd, env, useConptyDll, windowsAgentJob, exitCallback)"); } const int id = info[0].As().Int32Value(); @@ -348,7 +556,8 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { const std::wstring cwd(path_util::to_wstring(info[2].As())); const Napi::Array envValues = info[3].As(); const bool useConptyDll = info[4].As().Value(); - Napi::Function exitCallback = info[5].As(); + const bool windowsAgentJob = info[5].As().Value(); + Napi::Function exitCallback = info[6].As(); // Fetch pty handle from ID and start process pty_baton* handle = get_pty_baton(id); @@ -391,11 +600,15 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { SIZE_T size = 0; InitializeProcThreadAttributeList(NULL, 1, 0, &size); - BYTE *attrList = new BYTE[size]; - siEx.lpAttributeList = reinterpret_cast(attrList); + auto attrList = std::make_unique(size); + siEx.lpAttributeList = reinterpret_cast(attrList.get()); + PROCESS_INFORMATION piClient{}; fSuccess = InitializeProcThreadAttributeList(siEx.lpAttributeList, 1, 0, &size); if (!fSuccess) { + const DWORD attributeError = GetLastError(); + cleanup_failed_connect(handle, &piClient, nullptr); + SetLastError(attributeError); throw errorWithCode(info, "InitializeProcThreadAttributeList failed"); } fSuccess = UpdateProcThreadAttribute(siEx.lpAttributeList, @@ -406,36 +619,80 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { NULL, NULL); if (!fSuccess) { + const DWORD attributeError = GetLastError(); + cleanup_failed_connect(handle, &piClient, siEx.lpAttributeList); + SetLastError(attributeError); throw errorWithCode(info, "UpdateProcThreadAttribute failed"); } - PROCESS_INFORMATION piClient{}; fSuccess = !!CreateProcessW( nullptr, mutableCommandline.get(), nullptr, // lpProcessAttributes nullptr, // lpThreadAttributes false, // bInheritHandles VERY IMPORTANT that this is false - EXTENDED_STARTUPINFO_PRESENT | CREATE_UNICODE_ENVIRONMENT, // dwCreationFlags + EXTENDED_STARTUPINFO_PRESENT | CREATE_UNICODE_ENVIRONMENT | + (windowsAgentJob ? CREATE_SUSPENDED : 0), // dwCreationFlags envArg, // lpEnvironment mutableCwd.get(), // lpCurrentDirectory &siEx.StartupInfo, // lpStartupInfo &piClient // lpProcessInformation ); if (!fSuccess) { + const DWORD createError = GetLastError(); + cleanup_failed_connect(handle, &piClient, siEx.lpAttributeList); + SetLastError(createError); throw errorWithCode(info, "Cannot create process"); } - HANDLE hLibrary = LoadConptyDll(info, useConptyDll); - bool fLoadedDll = hLibrary != nullptr; - if (useConptyDll && fLoadedDll) - { - PFNRELEASEPSEUDOCONSOLE const pfnReleasePseudoConsole = (PFNRELEASEPSEUDOCONSOLE)GetProcAddress( - (HMODULE)hLibrary, "ConptyReleasePseudoConsole"); - if (pfnReleasePseudoConsole) - { - pfnReleasePseudoConsole(handle->hpc); + if (windowsAgentJob) { + // Job setup is atomic while the shell is suspended; partial ownership is unsafe. + HANDLE hJob = CreateJobObjectW(nullptr, nullptr); + JOBOBJECT_EXTENDED_LIMIT_INFORMATION limits{}; + limits.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + bool jobReady = hJob != nullptr && + SetInformationJobObject(hJob, JobObjectExtendedLimitInformation, &limits, sizeof(limits)); +#ifdef NODE_PTY_TEST_FAULTS + if (jobReady && connectFailurePoint == ConnectFailurePoint::AssignJob) { + SetLastError(ERROR_ACCESS_DENIED); + jobReady = false; + } +#endif + if (jobReady) { + jobReady = AssignProcessToJobObject(hJob, piClient.hProcess); + } + if (!jobReady) { + const DWORD jobError = GetLastError(); + handle->hJob = hJob; + cleanup_failed_connect(handle, &piClient, siEx.lpAttributeList); + SetLastError(jobError); + throw errorWithCode(info, "Cannot assign process to Job"); } + handle->hJob = hJob; + + DWORD resumeResult; +#ifdef NODE_PTY_TEST_FAULTS + if (connectFailurePoint == ConnectFailurePoint::ResumeThread) { + SetLastError(ERROR_INVALID_HANDLE); + resumeResult = static_cast(-1); + } else { + resumeResult = ResumeThread(piClient.hThread); + } +#else + resumeResult = ResumeThread(piClient.hThread); +#endif + if (resumeResult == static_cast(-1)) { + const DWORD resumeError = GetLastError(); + cleanup_failed_connect(handle, &piClient, siEx.lpAttributeList); + SetLastError(resumeError); + throw errorWithCode(info, "Cannot resume process"); + } + } + + DeleteProcThreadAttributeList(siEx.lpAttributeList); + + if (handle->releasePseudoConsole != nullptr) { + handle->releasePseudoConsole(handle->hpc); } // Update handle @@ -445,7 +702,9 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { CloseHandle(piClient.hThread); // Close the input read and output write handle of the pseudoconsole CloseHandle(handle->hIn); + handle->hIn = INVALID_HANDLE_VALUE; CloseHandle(handle->hOut); + handle->hOut = INVALID_HANDLE_VALUE; SetupExitCallback(env, exitCallback, handle); @@ -544,21 +803,10 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { int id = info[0].As().Int32Value(); const bool useConptyDll = info[1].As().Value(); - const pty_baton* handle = get_pty_baton(id); + pty_baton* handle = get_pty_baton(id); if (handle != nullptr) { - HANDLE hLibrary = LoadConptyDll(info, useConptyDll); - bool fLoadedDll = hLibrary != nullptr; - if (fLoadedDll) - { - PFNCLOSEPSEUDOCONSOLE const pfnClosePseudoConsole = (PFNCLOSEPSEUDOCONSOLE)GetProcAddress( - (HMODULE)hLibrary, - useConptyDll ? "ConptyClosePseudoConsole" : "ClosePseudoConsole"); - if (pfnClosePseudoConsole) - { - pfnClosePseudoConsole(handle->hpc); - } - } + close_pty_resources(handle); if (useConptyDll) { TerminateProcess(handle->hShell, 1); } @@ -567,6 +815,109 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { return env.Undefined(); } +struct JobTerminationResult { + bool drained = false; +}; + +static Napi::Value PtyTerminateJobTree(const Napi::CallbackInfo& info) { + Napi::Env env(info.Env()); + Napi::HandleScope scope(env); + + if (info.Length() != 2 || !info[0].IsNumber() || !info[1].IsNumber()) { + throw Napi::Error::New(env, "Usage: pty.terminateJobTree(id, timeoutMs)"); + } + + const int id = info[0].As().Int32Value(); + const pty_baton* handle = get_pty_baton(id); + if (handle == nullptr || handle->hJob == nullptr) { + return env.Undefined(); + } + + const double timeoutValue = info[1].As().DoubleValue(); + if (timeoutValue < 1 || timeoutValue > 60000) { + throw Napi::RangeError::New(env, "timeoutMs must be between 1 and 60000"); + } + + HANDLE hJob = nullptr; + if (!DuplicateHandle( + GetCurrentProcess(), + handle->hJob, + GetCurrentProcess(), + &hJob, + 0, + FALSE, + DUPLICATE_SAME_ACCESS)) { + return env.Undefined(); + } + if (!TerminateJobObject(hJob, 1)) { + CloseHandle(hJob); + return env.Undefined(); + } + + // Why: termination is asynchronous. Keep a duplicate Job handle until the + // OS reports no members instead of treating the root process exit as proof. + auto deferred = Napi::Promise::Deferred::New(env); + std::thread *th = new std::thread; + auto tsfn = Napi::ThreadSafeFunction::New( + env, + Napi::Function::New(env, [](const Napi::CallbackInfo&) {}), + "PtyTerminateJobTree_resource", + 0, + 1, + [th](Napi::Env) { + th->join(); + delete th; + }); + const DWORD timeoutMs = static_cast(timeoutValue); + *th = std::thread([tsfn = std::move(tsfn), deferred, hJob, timeoutMs] { + auto *result = new JobTerminationResult; + const ULONGLONG startedAt = GetTickCount64(); + while (true) { + JOBOBJECT_BASIC_ACCOUNTING_INFORMATION accounting{}; + if (!QueryInformationJobObject( + hJob, + JobObjectBasicAccountingInformation, + &accounting, + sizeof(accounting), + nullptr)) { + break; + } + if (accounting.ActiveProcesses == 0) { + result->drained = true; + break; + } + if (GetTickCount64() - startedAt >= timeoutMs) { + break; + } + Sleep(25); + } + CloseHandle(hJob); + + const auto status = tsfn.BlockingCall( + result, + [deferred](Napi::Env env, Napi::Function, JobTerminationResult *result) { + deferred.Resolve(Napi::Boolean::New(env, result->drained)); + delete result; + }); + switch (status) { + case napi_closing: + delete result; + break; + case napi_queue_full: + Napi::Error::Fatal("PtyTerminateJobTree", "Queue was full"); + case napi_ok: + if (tsfn.Release() != napi_ok) { + Napi::Error::Fatal("PtyTerminateJobTree", "ThreadSafeFunction.Release() failed"); + } + break; + default: + Napi::Error::Fatal("PtyTerminateJobTree", "ThreadSafeFunction.BlockingCall() failed"); + } + }); + + return deferred.Promise(); +} + /** * Init */ @@ -577,6 +928,11 @@ Napi::Object init(Napi::Env env, Napi::Object exports) { exports.Set("resize", Napi::Function::New(env, PtyResize)); exports.Set("clear", Napi::Function::New(env, PtyClear)); exports.Set("kill", Napi::Function::New(env, PtyKill)); + exports.Set("terminateJobTree", Napi::Function::New(env, PtyTerminateJobTree)); +#ifdef NODE_PTY_TEST_FAULTS + exports.Set("setTestConnectFailure", Napi::Function::New(env, PtySetTestConnectFailure)); + exports.Set("getTestResourceState", Napi::Function::New(env, PtyGetTestResourceState)); +#endif return exports; }; diff --git a/src/windowsPtyAgent.test.ts b/src/windowsPtyAgent.test.ts index dc2104b34fd396722a9e30821ff0b7dea18b4295..23510c3d7c85ecc911b94719832abe5f2b90c1e3 100644 --- a/src/windowsPtyAgent.test.ts +++ b/src/windowsPtyAgent.test.ts @@ -4,13 +4,37 @@ */ import * as assert from 'assert'; -import { argsToCommandLine } from './windowsPtyAgent'; +import { argsToCommandLine, WindowsPtyAgent } from './windowsPtyAgent'; function check(file: string, args: string | string[], expected: string): void { assert.equal(argsToCommandLine(file, args), expected); } if (process.platform === 'win32') { + describe('Windows agent Job control', () => { + it('reports an already-exited owned Job as drained without another native lookup', () => { + const terminateJobTree = () => Promise.resolve(false); + const agent = Object.create(WindowsPtyAgent.prototype) as any; + agent._windowsAgentJob = true; + agent._processExited = true; + agent._pty = 1; + agent._ptyNative = { terminateJobTree }; + + assert.strictEqual(agent.windowsAgentJob, true); + return agent.terminateJobTree(1000).then((drained: boolean) => { + assert.strictEqual(drained, true); + }); + }); + + it('does not expose teardown when Job ownership was not established', () => { + const agent = Object.create(WindowsPtyAgent.prototype) as any; + agent._windowsAgentJob = false; + + assert.strictEqual(agent.windowsAgentJob, false); + assert.strictEqual(agent.terminateJobTree(1000), undefined); + }); + }); + describe('argsToCommandLine', () => { describe('Plain strings', () => { it('doesn\'t quote plain string', () => { diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index d7054449516f0c9a62af351c2caa17331206d530..5cc174373171329f36860103edab95323da89ed7 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -34,6 +34,9 @@ export class WindowsPtyAgent { private _innerPid: number = 0; private _closeTimeout: NodeJS.Timer | undefined; private _exitCode: number | undefined; + private _processExited: boolean = false; + private _windowsAgentJob: boolean = false; + private _processExitListeners: (() => void)[] = []; private _conoutSocketWorker: ConoutConnection; private _fd: any; @@ -45,6 +48,7 @@ export class WindowsPtyAgent { public get fd(): any { return this._fd; } public get innerPid(): number { return this._innerPid; } public get pty(): number { return this._pty; } + public get windowsAgentJob(): boolean { return this._windowsAgentJob; } constructor( file: string, @@ -56,11 +60,14 @@ export class WindowsPtyAgent { debug: boolean, private _useConpty: boolean | undefined, private _useConptyDll: boolean = false, - conptyInheritCursor: boolean = false + conptyInheritCursor: boolean = false, + windowsAgentJob: boolean = false ) { if (this._useConpty === undefined || this._useConpty === true) { this._useConpty = this._getWindowsBuildNumber() >= 18309; } + // Why: a requested Job is effective only when runtime detection actually selects ConPTY. + this._windowsAgentJob = Boolean(this._useConpty && windowsAgentJob); if (this._useConpty) { if (!conptyNative) { conptyNative = loadNativeModule('conpty').module; @@ -116,7 +123,7 @@ export class WindowsPtyAgent { this._inSocket.setEncoding('utf8'); if (this._useConpty) { - const connect = (this._ptyNative as IConptyNative).connect(this._pty, commandLine, cwd, env, this._useConptyDll, c => this._$onProcessExit(c)); + const connect = (this._ptyNative as IConptyNative).connect(this._pty, commandLine, cwd, env, this._useConptyDll, this._windowsAgentJob, c => this._$onProcessExit(c)); this._innerPid = connect.pid; } } @@ -138,6 +145,24 @@ export class WindowsPtyAgent { } } + public terminateJobTree(timeoutMs: number): Promise | undefined { + if (!this._windowsAgentJob) { + return undefined; + } + if (this._processExited) { + return Promise.resolve(true); + } + return (this._ptyNative as IConptyNative).terminateJobTree?.(this._pty, timeoutMs); + } + + public onProcessExit(listener: () => void): void { + if (this._processExited) { + listener(); + return; + } + this._processExitListeners.push(listener); + } + public kill(): void { // Tell the agent to kill the pty, this releases handles to the process if (this._useConpty) { @@ -223,6 +248,10 @@ export class WindowsPtyAgent { */ private _$onProcessExit(exitCode: number): void { this._exitCode = exitCode; + this._processExited = true; + const listeners = this._processExitListeners; + this._processExitListeners = []; + listeners.forEach(listener => listener()); if (!this._useConptyDll) { this._flushDataAndCleanUp(); this._outSocket.on('data', () => this._flushDataAndCleanUp()); diff --git a/src/windowsTerminal.test.ts b/src/windowsTerminal.test.ts index 8f1274edf6dcf6e9257ba9d711099791391ec778..afc5813d26ce0027b826d326fffdbc3c242753aa 100644 --- a/src/windowsTerminal.test.ts +++ b/src/windowsTerminal.test.ts @@ -92,6 +92,20 @@ function pollForProcessTreeSize(pid: number, size: number, intervalMs: number = } if (process.platform === 'win32') { + describe('WindowsTerminal Job capability fallback', () => { + it('does not wait for a Job callback when a requested Job falls back to winpty', function(done) { + this.timeout(10000); + const term = new WindowsTerminal('cmd.exe', '/C exit', { + useConpty: false, + windowsAgentJob: true + }); + term.on('exit', code => { + assert.strictEqual(code, 0); + done(); + }); + }); + }); + [[false, false], [true, false], [true, true]].forEach(([useConpty, useConptyDll]) => { describe(`WindowsTerminal (useConpty = ${useConpty}, useConptyDll = ${useConptyDll})`, () => { describe('kill', () => { diff --git a/src/windowsTerminal.ts b/src/windowsTerminal.ts index 13f6c6dbb3d7965c4699568f4704be3b6a33d947..db6ddd94a35b034c0fb998003e4332c28512809d 100644 --- a/src/windowsTerminal.ts +++ b/src/windowsTerminal.ts @@ -48,8 +48,23 @@ export class WindowsTerminal extends Terminal { this._deferreds = []; // Create new termal. - this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.useConpty, opt.useConptyDll, opt.conptyInheritCursor); + this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.useConpty, opt.useConptyDll, opt.conptyInheritCursor, opt.windowsAgentJob); this._socket = this._agent.outSocket; + let socketClosed = false; + let agentProcessExited = false; + let exitEmitted = false; + const emitExitWhenSafe = (): void => { + if (exitEmitted || !socketClosed || (this._agent.windowsAgentJob && !agentProcessExited)) { + return; + } + exitEmitted = true; + this.emit('exit', this._agent.exitCode); + this._close(); + }; + this._agent.onProcessExit(() => { + agentProcessExited = true; + emitExitWhenSafe(); + }); // Not available until `ready` event emitted. this._pid = this._agent.innerPid; @@ -103,8 +118,10 @@ export class WindowsTerminal extends Terminal { // Cleanup after the socket is closed. this._socket.on('close', () => { - this.emit('exit', this._agent.exitCode); - this._close(); + // Why: a Job-owned root can close ConPTY output before the native exit + // callback proves every descendant is gone; public exit needs both facts. + socketClosed = true; + emitExitWhenSafe(); }); }); @@ -161,6 +178,10 @@ export class WindowsTerminal extends Terminal { }); } + public terminateJobTree(timeoutMs: number): Promise | undefined { + return this._agent.terminateJobTree(timeoutMs); + } + public kill(signal?: string): void { this._deferNoArgs(() => { if (signal) { diff --git a/typings/node-pty.d.ts b/typings/node-pty.d.ts index 6f050ff1d97b8d3477be7ee95d2af516bd9517b7..b26831f60d2aba6cc352fad561cd0c3f7b9bfe02 100644 --- a/typings/node-pty.d.ts +++ b/typings/node-pty.d.ts @@ -108,6 +108,9 @@ declare module 'node-pty' { * @see https://docs.microsoft.com/en-us/windows/console/createpseudoconsole */ conptyInheritCursor?: boolean; + + /** Orca-specific: own this ConPTY and all descendants in a kill-on-close Job. */ + windowsAgentJob?: boolean; } /** @@ -183,6 +186,9 @@ declare module 'node-pty' { */ kill(signal?: string): void; + /** Terminates and waits for the Windows ConPTY Job Object to drain. */ + terminateJobTree?(timeoutMs: number): Promise | undefined; + /** * Pauses the pty for customizable flow control. */