From d706c49752af5501c094f1e95c5c445b6869b64b Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 30 Aug 2026 21:15:45 -0700 Subject: [PATCH] fix: close SSH and tab readiness race gaps --- config/patches/node-pty@1.1.0.patch | 18 +++--- pnpm-lock.yaml | 6 +- ...node-pty-windows-input-error.win32.test.ts | 31 ++++++++++ ...ws-shell-preflight-runtime.windows.test.ts | 15 ++++- ...t-session-mirror-frame-ordering-harness.ts | 8 ++- ...n-mirror-hydration-frame-ordering.test.tsx | 60 +++++++++++++++++++ .../src/runtime/web-session-tabs-sync.ts | 34 +++++++++-- 7 files changed, 147 insertions(+), 25 deletions(-) diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index 412de53b19d..77d0431b5f9 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -815,18 +815,16 @@ diff --git a/lib/windowsTerminal.js b/lib/windowsTerminal.js index 3c38f89..e20b3e6 100644 --- a/lib/windowsTerminal.js +++ b/lib/windowsTerminal.js -@@ -50,6 +50,29 @@ var WindowsTerminal = /** @class */ (function (_super) { +@@ -50,6 +50,27 @@ var WindowsTerminal = /** @class */ (function (_super) { // 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._socket = _this._agent.outSocket; + // Attach before readiness so a broken ConPTY output pipe cannot be unhandled. + _this._socket.on('error', function (err) { -+ var code = err.code; -+ var wasClosing = !_this._writable; -+ // Node can report these duplicate stream errors after teardown; active -+ // sockets still surface every other failure to the caller. ++ var code = err && err.code; ++ // PTY output can report EPIPE before `_close()` wins the race. + _this._close(); -+ if (wasClosing && (code === 'EPIPE' || code === 'ERR_STREAM_PUSH_AFTER_EOF' || code === 'ERR_STREAM_DESTROYED')) { ++ if (code === 'EPIPE' || code === 'ERR_STREAM_PUSH_AFTER_EOF' || code === 'ERR_STREAM_DESTROYED') { + return; + } + // EIO, happens when someone closes our child process: the only process @@ -923,7 +921,7 @@ diff --git a/src/windowsTerminal.ts b/src/windowsTerminal.ts index 13f6c6d..eda63c8 100644 --- a/src/windowsTerminal.ts +++ b/src/windowsTerminal.ts -@@ -51,6 +51,32 @@ export class WindowsTerminal extends Terminal { +@@ -51,6 +51,30 @@ export class WindowsTerminal extends Terminal { this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.useConpty, opt.useConptyDll, opt.conptyInheritCursor); this._socket = this._agent.outSocket; - @@ -931,12 +929,10 @@ index 13f6c6d..eda63c8 100644 + // Attach before readiness so a broken ConPTY output pipe cannot be unhandled. + this._socket.on('error', err => { + const code = (err).code; -+ const wasClosing = !this._writable; + -+ // Node can report these duplicate stream errors after teardown; active -+ // sockets still surface every other failure to the caller. ++ // PTY output can report EPIPE before `_close()` wins the race. + this._close(); -+ if (wasClosing && (code === 'EPIPE' || code === 'ERR_STREAM_PUSH_AFTER_EOF' || code === 'ERR_STREAM_DESTROYED')) { ++ if (code === 'EPIPE' || code === 'ERR_STREAM_PUSH_AFTER_EOF' || code === 'ERR_STREAM_DESTROYED') { + return; + } + diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 40407e156c9..2cc21a62fd4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -115,7 +115,7 @@ patchedDependencies: '@xterm/addon-webgl@0.20.0-beta.299': 94687e89a0115e6e6aa102837f986debdc029c091527ee5eb4a4e17ceaf9473e '@xterm/xterm@6.1.0-beta.303': 98756bcedc402bcdb7c6ab7b015d2e59cd18e97b03a2c06a27e95bb3ba429d9d lint-staged@16.4.0: 7333b3837f80a7fbd045964db6d76ba4fc118e49134bdbabb00585b6b7b60673 - node-pty@1.1.0: 1d4405bccb8ad7cb0c45061306a11059f02a23b9f60df8d14ef0563f88f84b0b + node-pty@1.1.0: d4b956b791f0b898bc7708320055fbf913b54d979c26b376f9ea6b193c613df5 importers: @@ -156,7 +156,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=1d4405bccb8ad7cb0c45061306a11059f02a23b9f60df8d14ef0563f88f84b0b) + version: 1.1.0(patch_hash=d4b956b791f0b898bc7708320055fbf913b54d979c26b376f9ea6b193c613df5) posthog-node: specifier: ^5.33.3 version: 5.33.3 @@ -12194,7 +12194,7 @@ snapshots: node-int64@0.4.0: {} - node-pty@1.1.0(patch_hash=1d4405bccb8ad7cb0c45061306a11059f02a23b9f60df8d14ef0563f88f84b0b): + node-pty@1.1.0(patch_hash=d4b956b791f0b898bc7708320055fbf913b54d979c26b376f9ea6b193c613df5): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/daemon/node-pty-windows-input-error.win32.test.ts b/src/main/daemon/node-pty-windows-input-error.win32.test.ts index f77cb757341..dd7a0e79837 100644 --- a/src/main/daemon/node-pty-windows-input-error.win32.test.ts +++ b/src/main/daemon/node-pty-windows-input-error.win32.test.ts @@ -121,4 +121,35 @@ describe.skipIf(process.platform !== 'win32')('node-pty Windows input errors', ( process.off('uncaughtException', uncaughtListener) } }, 20_000) + + it('contains an output EPIPE that races with PTY shutdown', async () => { + const uncaught: unknown[] = [] + const uncaughtListener = (error: unknown): void => { + uncaught.push(error) + } + process.on('uncaughtException', uncaughtListener) + + let terminal: IPty | undefined + try { + terminal = spawn(process.env.ComSpec ?? 'cmd.exe', ['/d', '/q'], { + cwd: process.cwd(), + env: process.env, + useConptyDll: false + }) + const output = (terminal as WindowsPtyInternals)._socket + const exit = waitForExit(terminal) + expect(() => { + output.emit('error', Object.assign(new Error('write EPIPE'), { code: 'EPIPE' })) + terminal?.kill() + }).not.toThrow() + await exit + expect(uncaught).toEqual([]) + } finally { + try { + terminal?.kill() + } catch {} + await new Promise((resolve) => setTimeout(resolve, 1_500)) + process.off('uncaughtException', uncaughtListener) + } + }, 20_000) }) diff --git a/src/main/providers/windows-shell-preflight-runtime.windows.test.ts b/src/main/providers/windows-shell-preflight-runtime.windows.test.ts index bae27db3a59..6c4501a7443 100644 --- a/src/main/providers/windows-shell-preflight-runtime.windows.test.ts +++ b/src/main/providers/windows-shell-preflight-runtime.windows.test.ts @@ -2,6 +2,7 @@ import { copyFileSync, existsSync, linkSync, + mkdirSync, mkdtempSync, readFileSync, rmSync, @@ -155,10 +156,13 @@ describeWindows('Windows Codex shell preflight runtime', () => { const root = makeTempDir() const preflight = writeFailingPreflight(root) - const codexExecutable = join(root, 'codex.exe') + // Keep the fixture ahead of any host-global Codex installation in Git Bash. + const codexExecutable = join(root, '.local', 'bin', 'codex.exe') + mkdirSync(join(root, '.local', 'bin'), { recursive: true }) linkNodeExecutable(codexExecutable) const preflightMarker = join(root, 'git-bash-preflight-ran') const codexMarker = join(root, 'git-bash-codex-ran') + const codexPathMarker = join(root, 'git-bash-codex-path') const previousUserDataPath = process.env.ORCA_USER_DATA_PATH process.env.ORCA_USER_DATA_PATH = join(root, 'user data') @@ -176,7 +180,7 @@ describeWindows('Windows Codex shell preflight runtime', () => { shellArgs: resolved.shellArgs, cwd: root, env: { - ...withPathEntry(process.env, root), + ...withPathEntry(process.env, join(root, '.local', 'bin')), CHERE_INVOKING: '1', HOME: root, ORCA_CODEX_LAUNCH_PREFLIGHT: preflight, @@ -185,7 +189,7 @@ describeWindows('Windows Codex shell preflight runtime', () => { TERM: 'xterm-256color' }, input: - "codex -e \"require('node:fs').writeFileSync(process.env.ORCA_CODEX_MARKER,'ran')\"\nexit\n", + "type -P codex > git-bash-codex-path\ncodex -e \"require('node:fs').writeFileSync(process.env.ORCA_CODEX_MARKER,'ran')\"\nexit\n", // Paired "Windows low spec" QA measured 12.7–15.8s across four runs: Git Bash // cold-starts two large Node executables for AV scanning, so allow 25s without // inflating the faster cmd.exe budget. @@ -200,6 +204,11 @@ describeWindows('Windows Codex shell preflight runtime', () => { } expect(existsSync(preflightMarker)).toBe(true) + const resolvedCodexPath = readFileSync(codexPathMarker, 'utf8') + .trim() + .replaceAll('\\', '/') + .toLowerCase() + expect(resolvedCodexPath).toMatch(/\/\.local\/bin\/codex(?:\.exe)?$/) expect(readFileSync(codexMarker, 'utf8')).toBe('ran') }) }) diff --git a/src/renderer/src/runtime/host-session-mirror-frame-ordering-harness.ts b/src/renderer/src/runtime/host-session-mirror-frame-ordering-harness.ts index db385639a41..98c1e93d34d 100644 --- a/src/renderer/src/runtime/host-session-mirror-frame-ordering-harness.ts +++ b/src/renderer/src/runtime/host-session-mirror-frame-ordering-harness.ts @@ -139,13 +139,17 @@ export function setDocumentVisibility(state: 'visible' | 'hidden'): void { document.dispatchEvent(new Event('visibilitychange')) } -export async function publish(subscription: RuntimeSubscription, result: unknown): Promise { +export async function publish( + subscription: RuntimeSubscription, + result: unknown, + runtimeId = 'runtime-a' +): Promise { await act(async () => { subscription.callbacks.onResponse({ id: 'subscription-event', ok: true as const, result, - _meta: { runtimeId: 'runtime-a' } + _meta: { runtimeId } } as never) await settle() }) diff --git a/src/renderer/src/runtime/host-session-mirror-hydration-frame-ordering.test.tsx b/src/renderer/src/runtime/host-session-mirror-hydration-frame-ordering.test.tsx index 4ece97dc0d4..c61fe14e284 100644 --- a/src/renderer/src/runtime/host-session-mirror-hydration-frame-ordering.test.tsx +++ b/src/renderer/src/runtime/host-session-mirror-hydration-frame-ordering.test.tsx @@ -180,6 +180,66 @@ describe('mirrored-pane resume deferral against real stream frames', () => { expect(state.tabsByWorktree[WT]?.find((tab) => tab.id === MIRROR_TAB_ID)?.ptyId).toBeNull() }) + it('does not let a late bootstrap runtime id retire a newer stream runtime', async () => { + let resolveListAll: (response: unknown) => void = () => {} + runtimeCall.mockImplementation((request: { method: string }) => + request.method === 'session.tabs.listAll' + ? new Promise((resolve) => { + resolveListAll = resolve + }) + : new Promise(() => {}) + ) + renderHook(() => useWebSessionTabsSync()) + await act(settle) + + const backgroundParentTabId = 'host-tab-2' + const backgroundSurfaceId = `${backgroundParentTabId}::${LEAF_ID}` + const firstRuntimeB = makeHostSnapshot(BG_WT, backgroundSurfaceId, backgroundParentTabId) + firstRuntimeB.publicationEpoch = 'runtime-b-epoch' + if (firstRuntimeB.tabs[0]?.type !== 'terminal') { + throw new Error('fixture must contain a terminal surface') + } + firstRuntimeB.tabs[0].terminal = 'runtime-b-terminal-1' + await publish( + findSubscription('session.tabs.subscribeAll'), + { type: 'updated', ...firstRuntimeB }, + 'runtime-b' + ) + + const lateRuntimeA = makeHostSnapshot(WT, HOST_SURFACE_ID, HOST_PARENT_TAB_ID) + lateRuntimeA.publicationEpoch = 'runtime-a-epoch' + if (lateRuntimeA.tabs[0]?.type !== 'terminal') { + throw new Error('fixture must contain a terminal surface') + } + lateRuntimeA.tabs[0].terminal = 'runtime-a-terminal' + await act(async () => { + resolveListAll({ + id: 'listall-after-runtime-restart', + ok: true as const, + result: { snapshots: [lateRuntimeA] }, + _meta: { runtimeId: 'runtime-a' } + }) + await settle() + }) + + const secondRuntimeB = makeHostSnapshot(BG_WT, backgroundSurfaceId, backgroundParentTabId) + secondRuntimeB.publicationEpoch = 'runtime-b-epoch' + secondRuntimeB.snapshotVersion = 2 + if (secondRuntimeB.tabs[0]?.type !== 'terminal') { + throw new Error('fixture must contain a terminal surface') + } + secondRuntimeB.tabs[0].terminal = 'runtime-b-terminal-2' + await publish( + findSubscription('session.tabs.subscribeAll'), + { type: 'updated', ...secondRuntimeB }, + 'runtime-b' + ) + + expect(useAppStore.getState().ptyIdsByTabId[BG_MIRROR_TAB_ID]).toEqual([ + `remote:${ENV}@@runtime-b-terminal-2` + ]) + }) + it('does not relaunch when a stream frame is the first hydration signal', async () => { renderHook(() => useWebSessionTabsSync()) await act(settle) diff --git a/src/renderer/src/runtime/web-session-tabs-sync.ts b/src/renderer/src/runtime/web-session-tabs-sync.ts index 09a9c9669d0..8df42a59935 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync.ts +++ b/src/renderer/src/runtime/web-session-tabs-sync.ts @@ -480,7 +480,23 @@ function isCurrentSessionTabsRuntimeFrame(environmentId: string, runtimeId?: str } /** Returns false for a runtime identity already superseded on this environment. */ -function acceptSessionTabsRuntimeId(environmentId: string, runtimeId: string): boolean { +function acceptSessionTabsRuntimeId( + environmentId: string, + runtimeId: string, + receivedFrame?: number +): boolean { + const history = sessionTabsRuntimeHistoryByEnvironment.get(environmentId) + const latestReceivedFrame = latestReceivedSessionTabsFrameByEnvironment.get(environmentId) ?? 0 + // A late bootstrap response may carry the predecessor process id. Do not + // let that older frame retire the runtime that already published newer data. + if ( + receivedFrame !== undefined && + receivedFrame < latestReceivedFrame && + history !== undefined && + history.current !== runtimeId + ) { + return false + } if (isRetiredSessionTabsRuntimeId(environmentId, runtimeId)) { return false } @@ -528,16 +544,16 @@ function recordReceivedWebSessionTabsSnapshot( const frame = receivedFrame ?? nextReceivedSessionTabsFrame() const key = sessionTabsFreshnessKey(environmentId, snapshot.worktree) const current = latestReceivedSessionTabsSnapshotByWorktree.get(key) - if (runtimeId && !acceptSessionTabsRuntimeId(environmentId, runtimeId)) { - return frame - } - recordReceivedWebSessionTabsEnvironmentFrame(environmentId, frame) // A bootstrap listAll reserves its frame before the request starts. If a // stream frame for this worktree arrived meanwhile, the late list is stale // evidence and must not advance epoch history. if (source === 'bootstrap' && current && frame < current.receivedFrame) { return frame } + if (runtimeId && !acceptSessionTabsRuntimeId(environmentId, runtimeId, frame)) { + return frame + } + recordReceivedWebSessionTabsEnvironmentFrame(environmentId, frame) const publicationEpoch = snapshot.publicationEpoch const history = sessionTabsPublicationEpochHistoryByWorktree.get(key) const isRetired = history?.retired.includes(publicationEpoch) ?? false @@ -4785,7 +4801,13 @@ function loadInitialWebSessionTabs( return } const runtimeId = getSessionTabsRuntimeIdFromResponse(response) - if (runtimeId && !acceptSessionTabsRuntimeId(environmentId, runtimeId)) { + const latestReceivedFrame = + latestReceivedSessionTabsFrameByEnvironment.get(environmentId) ?? 0 + if ( + runtimeId && + latestReceivedFrame <= requestReceivedFrame && + !acceptSessionTabsRuntimeId(environmentId, runtimeId, requestReceivedFrame) + ) { return } recordReceivedWebSessionTabsEnvironmentFrame(environmentId, requestReceivedFrame)