From 118f428167f2f009be005104ad0d2128b7b60ff7 Mon Sep 17 00:00:00 2001 From: Pyseph <45090858+Pyseph@users.noreply.github.com> Date: Wed, 4 Nov 2026 12:54:16 +0100 Subject: [PATCH] perf(server): report Windows terminal exit about 0.9 s sooner node-pty waited FLUSH_DATA_INTERVAL = 1000 ms after the shell exited before closing the ConPTY output socket and emitting exit, so the terminal showed Process exited a second late. Wait 100 ms instead, and run the cleanup one setImmediate turn after the timer fires so output already buffered in the pipe is read first after a main-thread stall (lowering the constant alone loses cmd's final line when a stall exceeds the interval). --- .../nodePtyFlushDataAndCleanUp.test.ts | 153 ++++++++++++++++++ patches/node-pty@1.2.0-beta.15.patch | 33 +++- pnpm-lock.yaml | 6 +- 3 files changed, 188 insertions(+), 4 deletions(-) create mode 100644 apps/server/src/terminal/nodePtyFlushDataAndCleanUp.test.ts diff --git a/apps/server/src/terminal/nodePtyFlushDataAndCleanUp.test.ts b/apps/server/src/terminal/nodePtyFlushDataAndCleanUp.test.ts new file mode 100644 index 0000000000000..f438e37e58734 --- /dev/null +++ b/apps/server/src/terminal/nodePtyFlushDataAndCleanUp.test.ts @@ -0,0 +1,153 @@ +import * as NodeModule from "node:module"; + +import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test"; + +const requireCjs = NodeModule.createRequire(import.meta.url); +const { + WindowsPtyAgent, +}: { + WindowsPtyAgent: { + prototype: { + _flushDataAndCleanUp(this: FakeAgent): void; + _cleanUpProcess(this: FakeAgent): void; + }; + }; +} = requireCjs("node-pty/lib/windowsPtyAgent.js"); + +interface FakeAgent { + _useConptyDll: boolean; + _closeTimeout?: unknown; + _inSocket: { readable: boolean }; + _outSocket: { readable: boolean; destroy: () => void }; + _cleanUpProcess: () => void; +} + +// The sockets are stand-ins; the timer logic and _cleanUpProcess are node-pty's own (as patched by T3). +const makeAgent = (): FakeAgent => { + const agent: FakeAgent = { + _useConptyDll: false, + _inSocket: { readable: true }, + _outSocket: { readable: true, destroy: vi.fn() }, + _cleanUpProcess: () => WindowsPtyAgent.prototype._cleanUpProcess.call(agent), + }; + return agent; +}; + +const flush = (agent: FakeAgent) => WindowsPtyAgent.prototype._flushDataAndCleanUp.call(agent); + +// A minimal event loop that mirrors Node's phases: expired timers run first, then immediates (the check phase). +// node-pty calls the global timer functions at call time, so replacing them lets the test place a "poll phase" +// (delivery of already-buffered pipe data) exactly between the two. +interface Timer { + readonly id: number; + readonly dueAt: number; + readonly run: () => void; +} +let now = 0; +let nextId = 0; +let timers: Timer[] = []; +let immediates: Array<() => void> = []; + +const runExpiredTimers = () => { + for (const timer of timers.filter((t) => t.dueAt <= now).toSorted((a, b) => a.dueAt - b.dueAt)) { + timers = timers.filter((t) => t.id !== timer.id); + timer.run(); + } +}; +const runImmediates = () => { + const batch = immediates; + immediates = []; + for (const run of batch) run(); +}; +/** One loop iteration after `ms` of wall time; `poll` runs between the timers and check phases. */ +const loopIteration = (ms: number, poll?: () => void) => { + now += ms; + runExpiredTimers(); + poll?.(); + runImmediates(); +}; + +// ConPTY never closes the output pipe after the shell exits, so node-pty ends the session on a timer. +// T3 shortens it (patches/node-pty@1.2.0-beta.15.patch) so the terminal reports "exited" about 0.9 s sooner. +describe("node-pty Windows exit flush", () => { + beforeEach(() => { + now = 0; + nextId = 0; + timers = []; + immediates = []; + vi.stubGlobal("setTimeout", (run: () => void, ms: number) => { + const id = ++nextId; + timers.push({ id, dueAt: now + ms, run }); + return id; + }); + vi.stubGlobal("clearTimeout", (id: number) => { + timers = timers.filter((t) => t.id !== id); + }); + vi.stubGlobal("setImmediate", (run: () => void) => { + immediates.push(run); + return ++nextId; + }); + }); + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("closes the output socket 100 ms after the exit, not a second later", () => { + const agent = makeAgent(); + flush(agent); + + loopIteration(99); + expect(agent._outSocket.destroy).not.toHaveBeenCalled(); + + loopIteration(1); + expect(agent._outSocket.destroy).toHaveBeenCalledTimes(1); + expect(agent._outSocket.readable).toBe(false); + expect(agent._inSocket.readable).toBe(false); + }); + + it("restarts the window when more output arrives", () => { + const agent = makeAgent(); + flush(agent); + loopIteration(60); + flush(agent); + + loopIteration(99); + expect(agent._outSocket.destroy).not.toHaveBeenCalled(); + + loopIteration(1); + expect(agent._outSocket.destroy).toHaveBeenCalledTimes(1); + }); + + // Timers run before the poll phase. After a main-thread stall longer than the window, output that is already + // buffered in the pipe is read only after the timer has expired, so the cleanup has to wait one loop turn for it. + it("keeps the socket open when buffered output is read right after a stalled timer expires", () => { + const agent = makeAgent(); + flush(agent); + + loopIteration(150, () => flush(agent)); + expect(agent._outSocket.destroy).not.toHaveBeenCalled(); + + loopIteration(99); + expect(agent._outSocket.destroy).not.toHaveBeenCalled(); + + loopIteration(1); + expect(agent._outSocket.destroy).toHaveBeenCalledTimes(1); + }); + + it("closes after a stall when no output was buffered", () => { + const agent = makeAgent(); + flush(agent); + + loopIteration(150); + expect(agent._outSocket.destroy).toHaveBeenCalledTimes(1); + }); + + it("does nothing when node-pty hosts conpty itself", () => { + const agent = makeAgent(); + agent._useConptyDll = true; + flush(agent); + + loopIteration(5000); + expect(agent._outSocket.destroy).not.toHaveBeenCalled(); + }); +}); diff --git a/patches/node-pty@1.2.0-beta.15.patch b/patches/node-pty@1.2.0-beta.15.patch index 125447607990d..0736c118d2ca2 100644 --- a/patches/node-pty@1.2.0-beta.15.patch +++ b/patches/node-pty@1.2.0-beta.15.patch @@ -17,7 +17,18 @@ diff --git a/lib/windowsConoutConnection.js b/lib/windowsConoutConnection.js diff --git a/lib/windowsPtyAgent.js b/lib/windowsPtyAgent.js --- a/lib/windowsPtyAgent.js +++ b/lib/windowsPtyAgent.js -@@ -218,13 +218,31 @@ +@@ -20,7 +20,9 @@ var conptyNative; + * shutting down the socket. The timer will be reset if a new data event comes in after the timer + * has started. + */ +-var FLUSH_DATA_INTERVAL = 1000; ++// ConPTY never closes the output pipe on its own after the shell exits, so wait this long for the last ++// screen update. Trailing output follows the exit within about 20 ms; 1000 ms made every exit report 1 s late. ++var FLUSH_DATA_INTERVAL = 100; + var CONNECTION_TIMEOUT = 5000; + var defaultWindowsPtyAgentOptions = { + connectionTimeout: CONNECTION_TIMEOUT, +@@ -218,13 +220,31 @@ return Promise.resolve([]); } return new Promise(function (resolve) { @@ -53,3 +64,23 @@ diff --git a/lib/windowsPtyAgent.js b/lib/windowsPtyAgent.js - resolve([_this._innerPid]); + finish([_this._innerPid]); }, 5000); +@@ -259,7 +279,18 @@ var WindowsPtyAgent = /** @class */ (function () { + if (this._closeTimeout) { + clearTimeout(this._closeTimeout); + } +- this._closeTimeout = setTimeout(function () { return _this._cleanUpProcess(); }, FLUSH_DATA_INTERVAL); ++ // Timers run before the poll phase. After a main-thread stall longer than the interval, output that is already ++ // buffered in the pipe would be dropped by the cleanup, so give the poll phase one turn to read it first; every ++ // chunk it reads re-arms this timer. ++ this._flushDataSeq = (this._flushDataSeq || 0) + 1; ++ this._closeTimeout = setTimeout(function () { ++ var seq = _this._flushDataSeq; ++ setImmediate(function () { ++ if (_this._flushDataSeq === seq) { ++ _this._cleanUpProcess(); ++ } ++ }); ++ }, FLUSH_DATA_INTERVAL); + }; + WindowsPtyAgent.prototype._cleanUpProcess = function () { + if (this._useConptyDll) { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 6ddbc362245c9..a1cb78b9cfb06 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -391,7 +391,7 @@ patchedDependencies: micromark-extension-gfm-table@2.1.1: 9ad23a530e45d9f83cba1da8654fe16d5af8b59d5c73d5c7a7c91fd25b9be1d8 micromark-extension-llm-math@3.1.0: 4eeb9d9e27ebc75e98795e21c0719005483dad7d233055698257f7d10e91603b micromark@4.0.2: cb79edde7eca83bbcaaafb6ceb7b044b81b987314cde4910823b15257ecada67 - node-pty@1.2.0-beta.15: f1e680afec85f650699ea756f91f469844ec4c976f4f168492a45c6b3f67d651 + node-pty@1.2.0-beta.15: ff8117509663fe1d5a07ef2648c324e80a9349b7d90401a00a591702615b8b30 react-native-gesture-handler@3.3.0: abe20e1d1c23549297cbbc818e14fe479b41455930d9663135d6b40fe359b626 react-native-image-viewing@0.2.2: 419f35ee91eac88ced9438058281db5eb09d9c799d8974c1172926068c9448cf react-native-keyboard-controller@1.22.8: 6eb6c847ac693b636ec97c83575acd44b77e92ad0d2c776de0cb4e78e8fb9d4f @@ -927,7 +927,7 @@ importers: version: 6.2.2 node-pty: specifier: 1.2.0-beta.15 - version: 1.2.0-beta.15(patch_hash=f1e680afec85f650699ea756f91f469844ec4c976f4f168492a45c6b3f67d651) + version: 1.2.0-beta.15(patch_hash=ff8117509663fe1d5a07ef2648c324e80a9349b7d90401a00a591702615b8b30) playwright-core: specifier: 1.60.0 version: 1.60.0 @@ -20571,7 +20571,7 @@ snapshots: node-mock-http@1.0.5: {} - node-pty@1.2.0-beta.15(patch_hash=f1e680afec85f650699ea756f91f469844ec4c976f4f168492a45c6b3f67d651): + node-pty@1.2.0-beta.15(patch_hash=ff8117509663fe1d5a07ef2648c324e80a9349b7d90401a00a591702615b8b30): dependencies: node-addon-api: 7.1.1