diff --git a/packages/workspace-server/src/services/shell/shell.test.ts b/packages/workspace-server/src/services/shell/shell.test.ts index c74da28697..b7cf1764a6 100644 --- a/packages/workspace-server/src/services/shell/shell.test.ts +++ b/packages/workspace-server/src/services/shell/shell.test.ts @@ -99,6 +99,51 @@ describe("ShellService.destroy", () => { const { service } = createService(); expect(() => service.destroy("nonexistent")).not.toThrow(); }); + + it("drops the session and still emits exit when pty.destroy() throws", async () => { + const ptyProcess = createMockPtyProcess(); + ptyProcess.destroy.mockImplementation(() => { + throw new Error("spawn UNKNOWN"); + }); + mockPty.spawn.mockReturnValue(ptyProcess); + const { service } = createService(); + const exitHandler = vi.fn(); + service.on(ShellEvent.Exit, exitHandler); + + await service.create("session-1"); + + expect(() => service.destroy("session-1")).not.toThrow(); + expect(service.hasSession("session-1")).toBe(false); + expect(service.getSessionCount()).toBe(0); + expect(exitHandler).toHaveBeenCalledWith({ + sessionId: "session-1", + exitCode: 130, + }); + }); + + it("destroyAll tears down every session when one pty.destroy() throws", async () => { + const failing = createMockPtyProcess(); + failing.destroy.mockImplementation(() => { + throw new Error("spawn UNKNOWN"); + }); + const healthy = createMockPtyProcess(); + mockPty.spawn + .mockReturnValueOnce(failing) + .mockReturnValueOnce(healthy) + .mockReturnValue(createMockPtyProcess()); + const { service } = createService(); + + await service.create("session-1"); + await service.create("session-2"); + await service.create("session-3"); + + expect(() => service.destroyAll()).not.toThrow(); + + // A throw on the first shell must not abort the shutdown sweep, or the + // remaining shells survive after the app quits. + expect(healthy.destroy).toHaveBeenCalled(); + expect(service.getSessionCount()).toBe(0); + }); }); describe("ShellService.createSession workspace env", () => { diff --git a/packages/workspace-server/src/services/shell/shell.ts b/packages/workspace-server/src/services/shell/shell.ts index bf9b4afa4f..a1c0fac7b7 100644 --- a/packages/workspace-server/src/services/shell/shell.ts +++ b/packages/workspace-server/src/services/shell/shell.ts @@ -197,14 +197,7 @@ export class ShellService extends TypedEventEmitter { disposables.push( ptyProcess.onExit(({ exitCode }) => { this.processTracking.unregister(ptyProcess.pid, "exited"); - const session = this.sessions.get(sessionId); - if (session) { - for (const d of session.disposables) { - d.dispose(); - } - session.pty.destroy(); - this.sessions.delete(sessionId); - } + this.teardownSession(sessionId); this.emit(ShellEvent.Exit, { sessionId, exitCode }); resolveExit({ exitCode }); }), @@ -277,14 +270,7 @@ export class ShellService extends TypedEventEmitter { disposables.push( ptyProcess.onExit(({ exitCode }) => { this.processTracking.unregister(ptyProcess.pid, "exited"); - const session = this.sessions.get(sessionId); - if (session) { - for (const d of session.disposables) { - d.dispose(); - } - session.pty.destroy(); - this.sessions.delete(sessionId); - } + this.teardownSession(sessionId); this.emit(ShellEvent.Exit, { sessionId, exitCode }); resolveExit({ exitCode }); }), @@ -341,13 +327,8 @@ export class ShellService extends TypedEventEmitter { destroy(sessionId: string): void { const session = this.sessions.get(sessionId); if (session) { - const pid = session.pty.pid; - this.processTracking.kill(pid); - for (const disposable of session.disposables) { - disposable.dispose(); - } - session.pty.destroy(); - this.sessions.delete(sessionId); + this.processTracking.kill(session.pty.pid); + this.teardownSession(sessionId); this.emit(ShellEvent.Exit, { sessionId, exitCode: DESTROYED_EXIT_CODE, @@ -355,6 +336,32 @@ export class ShellService extends TypedEventEmitter { } } + /** + * Drop a session and release its pty. Teardown is best-effort: `pty.destroy()` + * reaches into platform-specific cleanup that can fail for environmental + * reasons, so the session is removed from the map first and the destroy is + * guarded. Otherwise one failing shell would leak its map entry and abort the + * `destroyAll()` sweep, leaving the remaining shells running after quit. + */ + private teardownSession(sessionId: string): void { + const session = this.sessions.get(sessionId); + if (!session) { + return; + } + this.sessions.delete(sessionId); + for (const disposable of session.disposables) { + disposable.dispose(); + } + try { + session.pty.destroy(); + } catch (error) { + this.log.warn( + `Failed to destroy pty for shell session ${sessionId}`, + error, + ); + } + } + /** * Destroy all active shell sessions. * Used during application shutdown to ensure all child processes are cleaned up. diff --git a/patches/node-pty.patch b/patches/node-pty.patch index f060bb0b53..21d4c66d91 100644 --- a/patches/node-pty.patch +++ b/patches/node-pty.patch @@ -51,3 +51,87 @@ index 1ac5758bedd8cf54f32280dea4e4aeb5afdee30d..d2a5e7911fb823bef3c9d57d2bf35830 }, 'msvs_settings': { # Specify this setting here to override a setting from somewhere +diff --git a/lib/windowsPtyAgent.js b/lib/windowsPtyAgent.js +index a358ffb..c3ff5b3 100644 +--- a/lib/windowsPtyAgent.js ++++ b/lib/windowsPtyAgent.js +@@ -146,6 +146,10 @@ var WindowsPtyAgent = /** @class */ (function () { + // Ignore if process cannot be found (kill ESRCH error) + } + }); ++ }).catch(function () { ++ // Reaping the console process list is best-effort. Nothing awaits ++ // this promise, so a rejection would surface as an unhandled ++ // rejection and crash-report from a routine terminal close. + }); + this._ptyNative.kill(this._pty, this._useConptyDll); + this._conoutSocketWorker.dispose(); +@@ -181,7 +185,21 @@ var WindowsPtyAgent = /** @class */ (function () { + WindowsPtyAgent.prototype._getConsoleProcessList = function () { + var _this = this; + return new Promise(function (resolve) { +- var agent = child_process_1.fork(path.join(__dirname, 'conpty_console_list_agent'), [_this._innerPid.toString()]); ++ var agent; ++ try { ++ agent = child_process_1.fork(path.join(__dirname, 'conpty_console_list_agent'), [_this._innerPid.toString()]); ++ } ++ catch (e) { ++ // fork() throws synchronously when CreateProcess fails outright -- ++ // `spawn UNKNOWN` when antivirus/EDR blocks process creation. Fall back ++ // to the shell PID, matching the timeout path below. ++ resolve([_this._innerPid]); ++ return; ++ } ++ agent.on('error', function () { ++ clearTimeout(timeout); ++ resolve([_this._innerPid]); ++ }); + agent.on('message', function (message) { + clearTimeout(timeout); + resolve(message.consoleProcessList); +diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts +index d705444..38681a1 100644 +--- a/src/windowsPtyAgent.ts ++++ b/src/windowsPtyAgent.ts +@@ -7,7 +7,7 @@ + import * as fs from 'fs'; + import * as os from 'os'; + import * as path from 'path'; +-import { fork } from 'child_process'; ++import { ChildProcess, fork } from 'child_process'; + import { Socket } from 'net'; + import { ArgvOrCommandLine } from './types'; + import { ConoutConnection } from './windowsConoutConnection'; +@@ -152,6 +152,10 @@ export class WindowsPtyAgent { + // Ignore if process cannot be found (kill ESRCH error) + } + }); ++ }).catch(() => { ++ // Reaping the console process list is best-effort. Nothing awaits this ++ // promise, so a rejection would surface as an unhandled rejection and ++ // crash-report from a routine terminal close. + }); + (this._ptyNative as IConptyNative).kill(this._pty, this._useConptyDll); + this._conoutSocketWorker.dispose(); +@@ -184,7 +188,20 @@ export class WindowsPtyAgent { + + private _getConsoleProcessList(): Promise { + return new Promise(resolve => { +- const agent = fork(path.join(__dirname, 'conpty_console_list_agent'), [ this._innerPid.toString() ]); ++ let agent: ChildProcess; ++ try { ++ agent = fork(path.join(__dirname, 'conpty_console_list_agent'), [ this._innerPid.toString() ]); ++ } catch (e) { ++ // fork() throws synchronously when CreateProcess fails outright -- ++ // `spawn UNKNOWN` when antivirus/EDR blocks process creation. Fall back ++ // to the shell PID, matching the timeout path below. ++ resolve([ this._innerPid ]); ++ return; ++ } ++ agent.on('error', () => { ++ clearTimeout(timeout); ++ resolve([ this._innerPid ]); ++ }); + agent.on('message', message => { + clearTimeout(timeout); + resolve(message.consoleProcessList); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 5cdfefdf93..3ef2109dfa 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -96,7 +96,7 @@ overrides: patchedDependencies: node-pty: - hash: 4dfdf785f5ac51a03f5d6032371cebe89036381acd403621f250a896245647c5 + hash: 922d673882c7633b332a34d853be935f0cfe9841b2ba1ef0f005b8e408ac02d0 path: patches/node-pty.patch react-hotkeys-hook: hash: a8cd00b963d4ae6787cc267a2345cc1b32bcba5fa1131328b8017ee0d316ab0e @@ -291,7 +291,7 @@ importers: version: 1.1.12 node-pty: specifier: 1.1.0 - version: 1.1.0(patch_hash=4dfdf785f5ac51a03f5d6032371cebe89036381acd403621f250a896245647c5) + version: 1.1.0(patch_hash=922d673882c7633b332a34d853be935f0cfe9841b2ba1ef0f005b8e408ac02d0) posthog-node: specifier: ^5.35.6 version: 5.37.0(rxjs@7.8.2) @@ -1626,7 +1626,7 @@ importers: version: 7.11.0(reflect-metadata@0.2.2) node-pty: specifier: 1.1.0 - version: 1.1.0(patch_hash=4dfdf785f5ac51a03f5d6032371cebe89036381acd403621f250a896245647c5) + version: 1.1.0(patch_hash=922d673882c7633b332a34d853be935f0cfe9841b2ba1ef0f005b8e408ac02d0) reflect-metadata: specifier: 'catalog:' version: 0.2.2 @@ -28202,7 +28202,7 @@ snapshots: dependencies: process-on-spawn: 1.1.0 - node-pty@1.1.0(patch_hash=4dfdf785f5ac51a03f5d6032371cebe89036381acd403621f250a896245647c5): + node-pty@1.1.0(patch_hash=922d673882c7633b332a34d853be935f0cfe9841b2ba1ef0f005b8e408ac02d0): dependencies: node-addon-api: 7.1.1