diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 5dd5b4dbb63..577a751f81e 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -317,7 +317,7 @@ describe('ShellExecutionService', () => { simulation: ( ptyProcess: typeof mockPtyProcess, ac: AbortController, - ) => void, + ) => void | Promise, config: ShellExecutionConfig = shellExecutionConfig, options: ShellExecuteOptions = {}, ) => { @@ -333,7 +333,7 @@ describe('ShellExecutionService', () => { ); await new Promise((resolve) => process.nextTick(resolve)); - simulation(mockPtyProcess, abortController); + await simulation(mockPtyProcess, abortController); const result = await handle.result; return { result, handle, abortController }; }; @@ -2147,12 +2147,8 @@ describe('ShellExecutionService', () => { let capturedReplyListener: ((data: string) => void) | undefined; - // The forwarder under test subscribes through the terminal's public onData - // event; capture every listener a spawned terminal hands to it. The - // instance field shadows Terminal's prototype getter, so the service's - // subscription lands here instead of xterm's core emitter — the listener - // is invoked manually, and the returned disposable stands in for - // xterm's own so the cleanup path has something to dispose. + // Capture the forwarder for the error-containment and platform-gate tests. + // The device-attributes test below uses xterm's real parser and emitter. class ReplyCapturingTerminal extends pkg.Terminal { override onData: pkg.IEvent = (listener) => { capturedReplyListener = listener; @@ -2233,6 +2229,38 @@ describe('ShellExecutionService', () => { } }); + it('does not run the fallback after the PTY has already spawned', async () => { + mockPlatform.mockReturnValue('win32'); + let pidReads = 0; + Object.defineProperty(mockPtyProcess, 'pid', { + configurable: true, + get: () => { + pidReads++; + if (pidReads === 1) return 12345; + throw new Error('post-spawn handle setup failed'); + }, + }); + + try { + const handle = await ShellExecutionService.execute( + 'echo hi', + '/test/dir', + onOutputEventMock, + new AbortController().signal, + true, + shellExecutionConfig, + ); + const result = await handle.result; + + expect(mockPtySpawn).toHaveBeenCalledOnce(); + expect(mockCpSpawn).not.toHaveBeenCalled(); + expect(result.executionMethod).toBe('none'); + expect(result.error?.message).toBe('post-spawn handle setup failed'); + } finally { + ShellExecutionService['activePtys'].delete(12345); + } + }); + it('leaves the inbox ConPTY backend alone off Windows', async () => { // beforeEach pins the platform to linux. await simulateExecution('echo hi', (pty) => { @@ -2244,19 +2272,17 @@ describe('ShellExecutionService', () => { }); }); - it('writes the emulated terminal query replies back to the PTY on Windows', async () => { + it("writes xterm's device-attributes reply back to the PTY on Windows", async () => { // Bundled ConPTY answers no queries itself, so an unanswered DA probe // stalls the shell for its full ~2s timeout; the forwarder must carry - // the emulated terminal's replies to the PTY. + // the reply generated by xterm's real parser back to the PTY. mockPlatform.mockReturnValue('win32'); - mockLoadXtermHeadless.mockResolvedValueOnce({ - Terminal: ReplyCapturingTerminal, - }); - await simulateExecution('echo hi', (pty) => { - expect(capturedReplyListener).toBeDefined(); - capturedReplyListener!('\x1b[?64;1;22c'); - expect(mockPtyProcess.write).toHaveBeenCalledWith('\x1b[?64;1;22c'); + await simulateExecution('echo hi', async (pty) => { + pty.onData.mock.calls[0][0]('\x1b[c'); + await vi.waitFor(() => { + expect(mockPtyProcess.write).toHaveBeenCalledWith('\x1b[?1;2c'); + }); pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); }); }); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 70b8dff04d5..a2ec874a6c4 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -1483,6 +1483,7 @@ export class ShellExecutionService { // This should not happen, but as a safeguard... throw new Error('PTY implementation not found'); } + const useBundledConpty = os.platform() === 'win32'; // Records whether pty.spawn returned. The catch at the end of this method // needs it to tell a spawn-phase failure — no child exists yet, so handing // the command to the child_process fallback cannot run it twice — from @@ -1528,16 +1529,15 @@ export class ShellExecutionService { // Windows: with the inbox ConPTY backend a natural shell exit orphans // the `conhost.exe --headless` that backend spawned — the native exit // watcher erases the pty baton before JS can reach ClosePseudoConsole - // (microsoft/node-pty#965), so hosts accumulate until the CLI exits - // (#11303: `+7 conhost for 7 tool commands`). This option makes - // node-pty load the conpty.dll shipped with the package instead of the - // one built into Windows — that swap is all @lydell/node-pty documents - // the option as, and its typings mark it EXPERIMENTAL. #11303 measured - // the per-command host growth gone with - // @lydell/node-pty-win32-x64 1.2.0-beta.10 on Windows Server 2025 (30 - // commands: 30 orphaned hosts before, 0 after). Off Windows the option - // is inert: `useConptyDll` appears nowhere in the POSIX prebuilds. - useConptyDll: os.platform() === 'win32', + // (microsoft/node-pty#965), so hosts accumulate until the CLI exits. + // The bundled backend calls ConptyReleasePseudoConsole after spawn, + // releasing the reference that otherwise keeps its host alive after + // the last client exits. Windows verification must count both + // `conhost.exe` and `OpenConsole.exe` (or all attributable children), + // because the bundled backend normally launches `OpenConsole.exe`. Off + // Windows the option is inert: `useConptyDll` appears nowhere in the + // POSIX prebuilds. + useConptyDll: useBundledConpty, }); ptySpawned = true; @@ -1552,26 +1552,25 @@ export class ShellExecutionService { }); headlessTerminal.scrollToTop(); - // Bundled ConPTY (useConptyDll above) answers no terminal queries + // Bundled ConPTY (useBundledConpty above) answers no terminal queries // itself, so a shell that probes the terminal — PowerShell's DA query // at startup — stalls for its full ~2s timeout unless the emulated // terminal's auto-generated reply is written back (measured 3.22s → // 0.23s per command). Scoped to Windows to keep the POSIX path // byte-identical; the hook dies with headlessTerminal.dispose() in // both the foreground and background-promote cleanups. - const queryResponseDisposable = - os.platform() === 'win32' - ? headlessTerminal.onData((data) => { - try { - ptyProcess.write(data); - } catch (e) { - // A reply racing shell exit finds a dead PTY — drop it. - debugLogger.warn( - `writing terminal query reply to PTY threw: ${e instanceof Error ? e.message : String(e)}`, - ); - } - }) - : null; + const queryResponseDisposable = useBundledConpty + ? headlessTerminal.onData((data) => { + try { + ptyProcess.write(data); + } catch (e) { + // A reply racing shell exit finds a dead PTY — drop it. + debugLogger.warn( + `writing terminal query reply to PTY threw: ${e instanceof Error ? e.message : String(e)}`, + ); + } + }) + : null; this.activePtys.set(ptyProcess.pid, { ptyProcess, headlessTerminal }); @@ -2529,7 +2528,7 @@ export class ShellExecutionService { return { pid: ptyProcess.pid, result }; } catch (e) { const error = e as Error; - if (!ptySpawned && os.platform() === 'win32') { + if (!ptySpawned && useBundledConpty) { // The bundled ConPTY backend (useConptyDll above) adds throw sites // node-pty reaches synchronously out of spawn — the conpty.dll it // ships being missing or unloadable among them — and none of those