diff --git a/packages/core/src/services/web-terminal-registry.test.ts b/packages/core/src/services/web-terminal-registry.test.ts index 98f880dcbd4..b503fadd963 100644 --- a/packages/core/src/services/web-terminal-registry.test.ts +++ b/packages/core/src/services/web-terminal-registry.test.ts @@ -406,6 +406,83 @@ describe('WebTerminalRegistry', () => { expect(kill).not.toHaveBeenCalled(); }); + it('frees an exited session at exit time, not at the idle reclaim', async () => { + osPlatform.mockReturnValue('win32'); + const registry = new WebTerminalRegistry(); + await registry.create({ + terminalId: 'terminal:exit-time-release', + workspaceCwd: '/workspace', + }); + + onExit({ exitCode: 0 }); + // One turn of the event loop, on real timers: the 15-minute idle reclaim + // cannot have run, and nothing below calls release(). A live exit closes + // the route's socket with 4000, which the client treats as non-retryable, + // so no tab close follows either — that window is what #11353 is about. + await new Promise((resolve) => setImmediate(resolve)); + + // node-pty strands its conout worker on a natural exit, so that worker is + // the resource an exited web terminal held for up to IDLE_RECLAIM_MS — and + // exited sessions do not count against the admission cap, so accumulation + // inside the window was unbounded. See #11303 / #11353. + expect(conoutDispose).toHaveBeenCalledOnce(); + expect(disposeData).toHaveBeenCalledOnce(); + expect(disposeExit).toHaveBeenCalledOnce(); + // Nothing may signal an exited shell's possibly-recycled pid. + expect(kill).not.toHaveBeenCalled(); + expect(spawnSync).not.toHaveBeenCalled(); + // The session itself survives the release, for scrollback replay. + expect(registry.readSnapshot('terminal:exit-time-release')).toBeDefined(); + }); + + it('still replays buffered scrollback after the exit-time release', async () => { + osPlatform.mockReturnValue('win32'); + const registry = new WebTerminalRegistry(); + await registry.create({ + terminalId: 'terminal:replay-after-exit-release', + workspaceCwd: '/workspace', + }); + + onData('boot\r\n'); + onExit({ exitCode: 3 }); + await new Promise((resolve) => setImmediate(resolve)); + + // The exit-time release frees PTY handles only. The session and its buffer + // stay in the map, so a second tab attaching to this terminal id still gets + // the scrollback plus the exit state — which is what terminal.ts's + // releaseAfterReplay path depends on. + expect(conoutDispose).toHaveBeenCalledOnce(); + expect(registry.readSnapshot('terminal:replay-after-exit-release')).toEqual( + { + output: 'boot\r\n', + exited: true, + exitCode: 3, + workspaceCwd: '/workspace', + }, + ); + }); + + it('does not free an exited session twice when release follows', async () => { + osPlatform.mockReturnValue('win32'); + const registry = new WebTerminalRegistry(); + await registry.create({ + terminalId: 'terminal:exit-release-once', + workspaceCwd: '/workspace', + }); + + onExit({ exitCode: 0 }); + await new Promise((resolve) => setImmediate(resolve)); + // The tab close, a workspace drain, dispose() or the reclaim all still run + // release() on a session whose PTY was already freed at exit time. + expect(registry.release('terminal:exit-release-once')).toBe(true); + + expect(conoutDispose).toHaveBeenCalledOnce(); + expect(disposeData).toHaveBeenCalledOnce(); + expect(disposeExit).toHaveBeenCalledOnce(); + expect(kill).not.toHaveBeenCalled(); + expect(spawnSync).not.toHaveBeenCalled(); + }); + it('forwards live output and bounds unacknowledged PTY input', async () => { const registry = new WebTerminalRegistry(); await registry.create({ diff --git a/packages/core/src/services/web-terminal-registry.ts b/packages/core/src/services/web-terminal-registry.ts index 6ab9f7c6d84..e0f48f10021 100644 --- a/packages/core/src/services/web-terminal-registry.ts +++ b/packages/core/src/services/web-terminal-registry.ts @@ -72,6 +72,13 @@ interface PtySession { reclaimTimer?: ReturnType; dataDisposable?: { dispose(): void }; exitDisposable?: { dispose(): void }; + /** + * Set once the PTY-side resources above have been freed. The exit-time + * release frees them while the session stays in the map for scrollback + * replay, so a later `release()` — tab close, workspace drain, `dispose()`, + * idle reclaim — must not free them a second time. See #11353. + */ + ptyResourcesReleased: boolean; } interface SpawnedWebTerminalPty extends WebTerminalPty { @@ -270,6 +277,23 @@ export class WebTerminalRegistry { session.exited = true; session.exitCode = e.exitCode; for (const listener of [...session.exitListeners]) listener(e); + // Nothing needs the PTY once the shell is gone: write() and resize() + // already short-circuit on `exited`, and readSnapshot() replays the + // JS-side `buffer`, not the console. Waiting for release() instead left + // every exited web terminal holding node-pty's conout worker — and, + // upstream, its conhost.exe — for up to IDLE_RECLAIM_MS, because the + // route keeps the session alive for scrollback and the client treats the + // 4000 close as non-retryable, so only a tab close releases it. Exited + // sessions also do not count against the admission cap, so accumulation + // inside that window was unbounded. See #11303 / #11353. + // + // Deferred one turn rather than run inline: onExit can arrive slightly + // before late PTY data is processed, the same race shellExecutionService + // drains before finalizing. setImmediate runs after the poll-phase + // callbacks already queued this tick, so trailing output still reaches + // `buffer` before the data listener is detached. handleData is fully + // synchronous, so one turn is enough — there is no chain to flush. + setImmediate(() => this.releasePtyResources(session)); }; let dataDisposable: { dispose(): void } | undefined; let exitDisposable: { dispose(): void } | undefined; @@ -347,6 +371,7 @@ export class WebTerminalRegistry { exitListeners: new Set(), dataDisposable, exitDisposable, + ptyResourcesReleased: false, }; sessionRef.current = session; this.sessions.set(terminalId, session); @@ -458,29 +483,20 @@ export class WebTerminalRegistry { listener({ exitCode: 143, signal: 15 }); } } - session.dataDisposable?.dispose(); - session.exitDisposable?.dispose(); session.outputListeners.clear(); session.exitListeners.clear(); if (!session.exited) { + // killPtyTree has to run before releasePtyResources: its pty.kill() + // defers the whole teardown while `_isReady` is false, so a terminal + // released before its shell's first output byte (tab closed during slow + // pwsh startup, or a workspace drain) still has a kill() queued in + // node-pty's `_deferreds`. The wrapper's kill() notes the close only when + // it really ran; releaseHost then disposes the worker a deferred kill + // would strand, and skips the native close so the queued kill() stays the + // single closer — never a second close. killPtyTree(session.pty); - // killPtyTree's pty.kill() defers its whole teardown while `_isReady` is - // false, so a terminal released before its shell's first output byte (tab - // closed during slow pwsh startup, or a workspace drain) still has a - // kill() queued in node-pty's `_deferreds`. The wrapper's kill() notes - // the close only when it really ran; releaseHost then disposes the worker - // a deferred kill would strand, and skips the native close so the queued - // kill() stays the single closer — never a second close. - session.pty.releaseHost?.(); - } else { - // The shell already exited, so nothing may signal its (possibly recycled) - // pid — but node-pty does not release its conout worker thread on a - // natural exit, so without this every terminal the user exits leaks one - // for the life of the CLI. Same defect as the shell-tool path in - // shellExecutionService. The conhost.exe half is not freed here (the - // native baton is already gone); see releaseConPtyHost. See #11303. - session.pty.releaseHost?.(); } + this.releasePtyResources(session); return true; } @@ -502,6 +518,33 @@ export class WebTerminalRegistry { this.cancelledCreations.delete(terminalId); } + /** + * Free a session's PTY-side resources exactly once: detach the data/exit + * listeners, then release the ConPTY host / conout worker that node-pty + * strands on a natural exit. Without the second half every terminal the user + * exits leaks a worker for the life of the CLI — the same defect the + * shell-tool path has. The conhost.exe half is not freed on that path (the + * native baton is already gone); see releaseConPtyHost. See #11303. + * + * Deliberately leaves the session's map entry and its `buffer` alone, and + * never signals the pid: on the exited path the shell is gone and its pid may + * be recycled, which is why #11313 added `releaseHost` instead of reusing + * `kill()`. Keeping the entry is what lets `readSnapshot()` still replay the + * scrollback after an exit-time release. + * + * Called from `handleExit` (deferred one turn, so an exited web terminal + * stops holding the worker for the whole idle-reclaim window — #11353) and + * from `release()` on both of its arms, where the flag keeps a release that + * follows an exit-time release from disposing anything twice. + */ + private releasePtyResources(session: PtySession): void { + if (session.ptyResourcesReleased) return; + session.ptyResourcesReleased = true; + session.dataDisposable?.dispose(); + session.exitDisposable?.dispose(); + session.pty.releaseHost?.(); + } + private clearReclaim(session: PtySession): void { if (session.reclaimTimer) { clearTimeout(session.reclaimTimer);