From d69897a3a02fb4d3bfa20db277afcc6a2d99fef7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 02:34:54 -0700 Subject: [PATCH] fix(pty,remote): close the pty master fd leak and two remote-terminal defects #8362: node-pty leaves the forkpty()/posix_openpt() master without FD_CLOEXEC, so on Linux every later child of the process -- both later pty children and plain child_process spawns -- inherits it and keeps the /dev/pts device alive. Measured on Linux with stock node-pty 1.1.0: master fd flags 0404002 (cloexec=false), and 17 -> /dev/pts/ptmx present in both a later pty child's /proc/self/fd and a later child_process child's. Extend the existing node-pty patch with pty_cloexec() on both PtyFork spawn paths; after the patch the flags read 02404002 (cloexec=true) and neither child sees the master. This covers the app and terminal daemon only -- the SSH relay installs node-pty from npm on the remote host, so it stays exposed (see the report). #8414: the inspection queue's fire-and-forget `.finally()` chain re-raised a rejecting inspection as a renderer-global unhandledrejection, which an unreachable runtime produced on every cadence tick. #9194: `tab_not_found` is the host's definitive "no such tab", but the close path cleared the close intent for it exactly like a dropped connection, so a host that keeps republishing the dead surface re-materialized the pane the user just closed. Keep that intent and drop its TTL. Also route the banner's "Remote terminal was closed." line through translate() so it stops mixing English into a localized banner. --- config/patches/node-pty@1.1.0.patch | 61 +++++++++++- pnpm-lock.yaml | 6 +- src/main/daemon/node-pty-fd-leak.test.ts | 95 ++++++++++++++++++- .../terminal-pane/TerminalErrorToast.tsx | 12 +++ ...ection-queue-rejection-containment.test.ts | 72 ++++++++++++++ .../agent-process-inspection-queue.ts | 18 ++-- ...l-error-remote-closed-localization.test.ts | 24 +++++ src/renderer/src/i18n/locales/en.json | 3 +- src/renderer/src/i18n/locales/es.json | 3 +- src/renderer/src/i18n/locales/ja.json | 3 +- src/renderer/src/i18n/locales/ko.json | 3 +- src/renderer/src/i18n/locales/zh.json | 3 +- ...runtime-session-tab-activate-close.test.ts | 31 ++++++ .../web-runtime-session-tab-lifecycle.ts | 18 +++- .../src/runtime/web-session-close-intent.ts | 26 ++++- 15 files changed, 351 insertions(+), 27 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/agent-process-inspection-queue-rejection-containment.test.ts create mode 100644 src/renderer/src/components/terminal-pane/terminal-error-remote-closed-localization.test.ts diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index 9ee2ebd39b4..348ce6ef7ce 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -176,7 +176,7 @@ index 181ccabbbe9c4948a9725fb1db907a68e9de01fc..67f31facf85562b67adbfbd04ce28ddd process.send!({ consoleProcessList }); process.exit(0); diff --git a/src/unix/pty.cc b/src/unix/pty.cc -index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..383df0c9c48355547c65e6c9bbba593d15c4dd44 100644 +index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..2ae787c5bd4f3eba470584dc658a01a52c690e0a 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -23,7 +23,9 @@ @@ -215,7 +215,17 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..383df0c9c48355547c65e6c9bbba593d /* Some platforms name VWERASE and VDISCARD differently */ #if !defined(VWERASE) && defined(VWERSE) #define VWERASE VWERSE -@@ -237,13 +258,23 @@ pty_getproc(int, char *); +@@ -228,6 +249,9 @@ Napi::Value PtyGetProc(const Napi::CallbackInfo& info); + static int + pty_nonblock(int); + ++static int ++pty_cloexec(int); ++ + #if defined(__APPLE__) + static char * + pty_getproc(int); +@@ -237,13 +261,23 @@ pty_getproc(int, char *); #endif #if defined(__APPLE__) || defined(__OpenBSD__) @@ -240,7 +250,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..383df0c9c48355547c65e6c9bbba593d #endif struct DelBuf { -@@ -367,10 +398,11 @@ Napi::Value PtyFork(const Napi::CallbackInfo& info) { +@@ -367,14 +401,18 @@ Napi::Value PtyFork(const Napi::CallbackInfo& info) { argv[i + 3] = strdup(arg.c_str()); } @@ -256,7 +266,48 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..383df0c9c48355547c65e6c9bbba593d } if (pty_nonblock(master) == -1) { throw Napi::Error::New(napiEnv, "Could not set master fd to nonblocking."); -@@ -684,15 +716,73 @@ pty_getproc(int fd, char *tty) { + } ++ if (pty_cloexec(master) == -1) { ++ throw Napi::Error::New(napiEnv, "Could not set master fd to close-on-exec."); ++ } + #else + int argc = argv_.Length(); + int argl = argc + 2; +@@ -445,6 +483,9 @@ Napi::Value PtyFork(const Napi::CallbackInfo& info) { + if (pty_nonblock(master) == -1) { + throw Napi::Error::New(napiEnv, "Could not set master fd to nonblocking."); + } ++ if (pty_cloexec(master) == -1) { ++ throw Napi::Error::New(napiEnv, "Could not set master fd to close-on-exec."); ++ } + } + #endif + +@@ -586,6 +627,23 @@ pty_nonblock(int fd) { + return fcntl(fd, F_SETFL, flags | O_NONBLOCK); + } + ++/** ++ * Orca: close-on-exec FD ++ * ++ * forkpty()/posix_openpt() have no atomic O_CLOEXEC, so a master left without ++ * FD_CLOEXEC is inherited by every later child of this process -- including ++ * later pty children -- which keeps its /dev/pts device and buffers alive long ++ * after its own session ends (#8362). ++ */ ++ ++static int ++pty_cloexec(int fd) { ++ int flags = fcntl(fd, F_GETFD); ++ if (flags == -1) return -1; ++ if (flags & FD_CLOEXEC) return 0; ++ return fcntl(fd, F_SETFD, flags | FD_CLOEXEC); ++} ++ + /** + * pty_getproc + * Taken from tmux. +@@ -684,15 +742,73 @@ pty_getproc(int fd, char *tty) { #endif #if defined(__APPLE__) @@ -332,7 +383,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..383df0c9c48355547c65e6c9bbba593d for (; count < 3; count++) { low_fds[count] = posix_openpt(O_RDWR); -@@ -706,80 +796,118 @@ pty_posix_spawn(char** argv, char** env, +@@ -706,80 +822,118 @@ pty_posix_spawn(char** argv, char** env, POSIX_SPAWN_SETSID; *master = posix_openpt(O_RDWR); if (*master == -1) { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 5b5b4c054a0..1abec6c0590 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: 572a46f539dd9da26e259702da974e1e693329e299625c97eb7c28e4e642500e + node-pty@1.1.0: 40b6b6b814c89a8a29c995495ea86cff2cf6766f42124b5702aedf4ce0565c0e importers: @@ -156,7 +156,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=572a46f539dd9da26e259702da974e1e693329e299625c97eb7c28e4e642500e) + version: 1.1.0(patch_hash=40b6b6b814c89a8a29c995495ea86cff2cf6766f42124b5702aedf4ce0565c0e) 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=572a46f539dd9da26e259702da974e1e693329e299625c97eb7c28e4e642500e): + node-pty@1.1.0(patch_hash=40b6b6b814c89a8a29c995495ea86cff2cf6766f42124b5702aedf4ce0565c0e): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/daemon/node-pty-fd-leak.test.ts b/src/main/daemon/node-pty-fd-leak.test.ts index 91958b49975..687952abb53 100644 --- a/src/main/daemon/node-pty-fd-leak.test.ts +++ b/src/main/daemon/node-pty-fd-leak.test.ts @@ -1,5 +1,6 @@ -import { execFileSync } from 'node:child_process' -import { existsSync, renameSync } from 'node:fs' +import { execFileSync, spawn } from 'node:child_process' +import { once } from 'node:events' +import { existsSync, readdirSync, readFileSync, readlinkSync, renameSync } from 'node:fs' import { setTimeout as delay } from 'node:timers/promises' import * as pty from 'node-pty' import { describe, expect, it } from 'vitest' @@ -90,3 +91,93 @@ describeOnDarwin('node-pty macOS spawn fd handling', () => { expect(after - before).toBe(0) }, 15000) }) + +// Linux is the only platform where node-pty takes the forkpty() path, which has no atomic +// O_CLOEXEC. /proc is what makes the inheritance observable, so the assertions live here. +const describeOnLinux = process.platform === 'linux' ? describe : describe.skip + +const O_CLOEXEC = 0o2000000 + +function ptyMasterFd(term: pty.IPty): number { + return (term as unknown as { fd: number }).fd +} + +function isCloseOnExec(fd: number): boolean { + const flags = /flags:\s*(\d+)/.exec(readFileSync(`/proc/self/fdinfo/${fd}`, 'utf8')) + expect(flags).toBeTruthy() + return (Number.parseInt(flags![1]!, 8) & O_CLOEXEC) !== 0 +} + +function openFdTargets(pid: number): string[] { + return readdirSync(`/proc/${pid}/fd`).map((entry) => { + try { + return readlinkSync(`/proc/${pid}/fd/${entry}`) + } catch { + return '' + } + }) +} + +describeOnLinux('node-pty Linux forkpty fd handling', () => { + it('marks pty masters close-on-exec so later children cannot inherit them', async () => { + const terms: pty.IPty[] = [] + let child: ReturnType | null = null + try { + for (let i = 0; i < 3; i++) { + terms.push( + pty.spawn('/bin/sh', ['-c', 'sleep 30'], { + name: 'xterm-256color', + cols: 80, + rows: 24, + cwd: process.cwd(), + env: { ...process.env, ORCA_FD_LEAK_TEST_INDEX: String(i) } + }) + ) + } + + // The masters this process owns must not survive an exec in any child it forks later. + expect(terms.map((term) => isCloseOnExec(ptyMasterFd(term)))).toEqual([true, true, true]) + + child = spawn('/bin/sh', ['-c', 'sleep 5'], { stdio: 'ignore' }) + await once(child, 'spawn') + const inherited = openFdTargets(child.pid!).filter((target) => target.includes('ptmx')) + expect(inherited).toEqual([]) + } finally { + child?.kill() + for (const term of terms) { + term.kill() + } + } + }, 15000) + + it('does not hand an earlier pty master to a later pty child', async () => { + const first = pty.spawn('/bin/sh', ['-c', 'sleep 30'], { + name: 'xterm-256color', + cols: 80, + rows: 24, + cwd: process.cwd(), + env: { ...process.env } + }) + try { + const second = pty.spawn('/bin/sh', ['-c', 'ls -l /proc/self/fd; exit 0'], { + name: 'xterm-256color', + cols: 200, + rows: 24, + cwd: process.cwd(), + env: { ...process.env } + }) + let output = '' + second.onData((data) => { + output += data + }) + await new Promise((resolve) => { + second.onExit(() => resolve()) + }) + await delay(100) + + expect(output).not.toMatch(/ptmx/) + } finally { + first.kill() + } + }, 15000) +}) diff --git a/src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx b/src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx index ee876ae719e..663a9a664c2 100644 --- a/src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx +++ b/src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx @@ -18,6 +18,10 @@ const STALE_DAEMON_CWD_MARKERS = [ ] // Thrown by ipc/pty.ts when a persisted pane owner can't be proven alive or dead (STA-3536). const PANE_OWNER_UNVERIFIED_MARKER = 'terminal_pane_owner_unverified' +// remote-runtime-pty-transport.ts surfaces this English literal as a wire-level marker, so it is +// translated here rather than at the source -- otherwise the banner mixes English with the +// localized chrome around it (#9194). +const REMOTE_TERMINAL_CLOSED_MARKER = 'Remote terminal was closed.' // Why one source: the test and replace forms must match the same token, and a lone /g regex carries // lastIndex state across .test() calls. Capture the leading boundary so replacement can restore it. const TERMINAL_HOST_GONE_SOURCE = '(^|[^a-z0-9_])terminal_host_gone(?=$|[^a-z0-9_])' @@ -102,6 +106,14 @@ export function humanizeTerminalError(error: string): string { ) ) } + if (humanized.includes(REMOTE_TERMINAL_CLOSED_MARKER)) { + humanized = humanized.replaceAll(REMOTE_TERMINAL_CLOSED_MARKER, () => + translate( + 'auto.components.terminal.pane.TerminalErrorToast.remoteTerminalClosed', + 'Remote terminal was closed.' + ) + ) + } humanized = humanizeUnreattachableSession(humanized) if (!isExplainedTerminalError(humanized)) { return humanized diff --git a/src/renderer/src/components/terminal-pane/agent-process-inspection-queue-rejection-containment.test.ts b/src/renderer/src/components/terminal-pane/agent-process-inspection-queue-rejection-containment.test.ts new file mode 100644 index 00000000000..639c457016f --- /dev/null +++ b/src/renderer/src/components/terminal-pane/agent-process-inspection-queue-rejection-containment.test.ts @@ -0,0 +1,72 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { + enqueueAgentProcessInspection, + resetAgentProcessInspectionQueueForTests +} from './agent-process-inspection-queue' + +// Node emits 'unhandledRejection' a turn after the microtask queue drains. +async function settleRejections(): Promise { + for (let index = 0; index < 4; index += 1) { + await new Promise((resolve) => setTimeout(resolve, 0)) + } +} + +async function collectUnhandledRejections(run: () => Promise): Promise { + const unhandled: unknown[] = [] + const onUnhandledRejection = (reason: unknown): void => { + unhandled.push(reason) + } + process.on('unhandledRejection', onUnhandledRejection) + try { + await run() + } finally { + process.off('unhandledRejection', onUnhandledRejection) + } + return unhandled +} + +describe('agent process inspection queue rejection containment', () => { + afterEach(() => { + resetAgentProcessInspectionQueueForTests() + }) + + it('contains an unreachable-runtime inspection failure instead of raising unhandledrejection', async () => { + const unhandled = await collectUnhandledRejections(async () => { + enqueueAgentProcessInspection({ + priority: 'cadence', + canRun: () => true, + run: () => + Promise.reject( + new Error( + "Error invoking remote method 'runtimeEnvironments:call': RemoteRuntimeClientError: Could not connect to the remote Orca runtime." + ) + ) + }) + await settleRejections() + }) + + expect(unhandled).toEqual([]) + }) + + it('keeps draining the queue after a rejecting inspection', async () => { + const ran: string[] = [] + const unhandled = await collectUnhandledRejections(async () => { + enqueueAgentProcessInspection({ + priority: 'cadence', + canRun: () => true, + run: () => Promise.reject(new Error('unreachable')) + }) + enqueueAgentProcessInspection({ + priority: 'cadence', + canRun: () => true, + run: async () => { + ran.push('second') + } + }) + await settleRejections() + }) + + expect(unhandled).toEqual([]) + expect(ran).toEqual(['second']) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/agent-process-inspection-queue.ts b/src/renderer/src/components/terminal-pane/agent-process-inspection-queue.ts index f7512ad8c60..6dc1dc4ccf2 100644 --- a/src/renderer/src/components/terminal-pane/agent-process-inspection-queue.ts +++ b/src/renderer/src/components/terminal-pane/agent-process-inspection-queue.ts @@ -63,12 +63,18 @@ function pumpInspectionQueue(): void { activeInspections += 1 inspectionStarts.push(now) - void next.run().finally(() => { - activeInspections = Math.max(0, activeInspections - 1) - if (inspectionQueue.length > 0) { - scheduleInspectionPump() - } - }) + // Why the catch before finally: an unreachable runtime rejects the inspection on a cadence, and a + // bare `.finally()` chain re-raises it as a renderer-global unhandledrejection. Coordinators own + // their own failure/backoff state, so the queue only has to keep its accounting running. + void next + .run() + .catch(() => {}) + .finally(() => { + activeInspections = Math.max(0, activeInspections - 1) + if (inspectionQueue.length > 0) { + scheduleInspectionPump() + } + }) if (inspectionQueue.length > 0) { scheduleInspectionPump() diff --git a/src/renderer/src/components/terminal-pane/terminal-error-remote-closed-localization.test.ts b/src/renderer/src/components/terminal-pane/terminal-error-remote-closed-localization.test.ts new file mode 100644 index 00000000000..12ff0dbda82 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-error-remote-closed-localization.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it, vi } from 'vitest' + +// Why a locale stand-in: the banner's own chrome is already translated, so the only way to see the +// mixed-language regression (#9194) is to render the message through a non-English catalog. +vi.mock('@/i18n/i18n', () => ({ + translate: (key: string, fallback: string) => + key === 'auto.components.terminal.pane.TerminalErrorToast.remoteTerminalClosed' + ? '远程终端已关闭。' + : fallback +})) + +import { humanizeTerminalError } from './TerminalErrorToast' + +describe('remote-closed terminal banner localization', () => { + it('translates the remote-closed line instead of pinning it to English', () => { + expect(humanizeTerminalError('Remote terminal was closed.')).toBe('远程终端已关闭。') + }) + + it('translates the line when it is accumulated with other errors', () => { + expect(humanizeTerminalError('Paste failed.\nRemote terminal was closed.')).toBe( + 'Paste failed.\n远程终端已关闭。' + ) + }) +}) diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 081493dc10c..59b629cd791 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -3072,7 +3072,8 @@ "cc6d997c65": "Restart the terminal daemon from here to clear stale daemon state.", "7ee11bc0db": "Orca couldn't confirm whether this terminal's previous session is still running, so it left the session untouched. Reopen this pane to retry.", "e16012e31e": "The terminal daemon that owned this session exited, so the session and its scrollback could not be recovered. Open a new terminal to continue.", - "sessionUnavailable": "Orca couldn't reattach to this pane's terminal session on the host. Open a new terminal to continue." + "sessionUnavailable": "Orca couldn't reattach to this pane's terminal session on the host. Open a new terminal to continue.", + "remoteTerminalClosed": "Remote terminal was closed." }, "TerminalProcessExitOverlay": { "capacityTitle": "Git Bash console limit reached", diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 91e9dc49dcc..775cfda6e38 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -2741,7 +2741,8 @@ "e4aa243f8c": "Reiniciar servicio", "a7e2fd2699": "abre un issue", "5c8ce20be6": "Si esto persiste, por favor", - "cc6d997c65": "Reinicia el servicio del terminal desde aquí para borrar el estado obsoleto." + "cc6d997c65": "Reinicia el servicio del terminal desde aquí para borrar el estado obsoleto.", + "remoteTerminalClosed": "La terminal remota se cerró." }, "TerminalProcessExitOverlay": { "capacityTitle": "Se alcanzó el límite de consolas de Git Bash", diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 493dfce6bda..c7c8449270a 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -2741,7 +2741,8 @@ "e4aa243f8c": "デーモンを再起動します", "a7e2fd2699": "Issue を登録", "5c8ce20be6": "この状態が続く場合は、", - "cc6d997c65": "ここからターミナルデーモンを再起動して、古いデーモン状態をクリアします。" + "cc6d997c65": "ここからターミナルデーモンを再起動して、古いデーモン状態をクリアします。", + "remoteTerminalClosed": "リモートターミナルが閉じられました。" }, "TerminalProcessExitOverlay": { "capacityTitle": "Git Bash のコンソール上限に達しました", diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 583b6ba2da6..0dfe91c821d 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -2746,7 +2746,8 @@ "e4aa243f8c": "데몬 재시작", "a7e2fd2699": "이슈 등록", "5c8ce20be6": "문제가 계속되면", - "cc6d997c65": "오래된 데몬 상태를 지우려면 여기에서 terminal 데몬을 다시 시작하세요." + "cc6d997c65": "오래된 데몬 상태를 지우려면 여기에서 terminal 데몬을 다시 시작하세요.", + "remoteTerminalClosed": "원격 터미널이 종료되었습니다." }, "TerminalProcessExitOverlay": { "capacityTitle": "Git Bash 콘솔 한도에 도달했습니다", diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index f4dfa74fd72..31a66a14125 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -2756,7 +2756,8 @@ "e4aa243f8c": "重新启动守护进程", "a7e2fd2699": "提交议题", "5c8ce20be6": "如果这种情况持续存在，请", - "cc6d997c65": "从此处重新启动终端守护进程以清除失效的守护进程状态。" + "cc6d997c65": "从此处重新启动终端守护进程以清除失效的守护进程状态。", + "remoteTerminalClosed": "远程终端已关闭。" }, "TerminalProcessExitOverlay": { "capacityTitle": "已达到 Git Bash 控制台上限", diff --git a/src/renderer/src/runtime/web-runtime-session-tab-activate-close.test.ts b/src/renderer/src/runtime/web-runtime-session-tab-activate-close.test.ts index 6ed42c3c841..ca9e0cdd276 100644 --- a/src/renderer/src/runtime/web-runtime-session-tab-activate-close.test.ts +++ b/src/renderer/src/runtime/web-runtime-session-tab-activate-close.test.ts @@ -9,6 +9,7 @@ import { recordWebSessionCloseIntent, resetWebSessionCloseIntentForTests } from './web-session-close-intent' +import { toHostSessionTabId } from './web-terminal-surface-id' import { ENVIRONMENT_ID, WORKTREE_ID, makeSnapshot } from './web-runtime-session-test-harness' const mocks = vi.hoisted(() => ({ @@ -305,6 +306,36 @@ describe('web runtime session tab actions', () => { ).resolves.toBe(outcome) }) + // #9194: a host can answer tab_not_found and still keep republishing the surface. The close + // intent is what hides the mirror, so letting it age out handed the user back a phantom pane + // whose handle is already gone -- and closing it again just restarted the same 10s loop. + it.each([ + ['tab_not_found', true], + ['runtime_rpc_timeout', false] + ])('keeps a %s close suppressed past the close-intent TTL: %s', async (code, stillPending) => { + const runtimeCall = vi + .fn() + .mockResolvedValueOnce({ id: 'close', ok: false, error: { code, message: code } }) + .mockResolvedValueOnce({ id: 'list', ok: true, result: makeSnapshot() }) + vi.stubGlobal('window', { api: { runtimeEnvironments: { call: runtimeCall } } }) + + await closeWebRuntimeSessionTab({ + worktreeId: WORKTREE_ID, + tabId: 'local-browser-unified', + reason: 'user' + }) + + const hostTabId = toHostSessionTabId('local-browser-unified') + expect( + isWebSessionCloseIntentPending( + { environmentId: ENVIRONMENT_ID }, + WORKTREE_ID, + hostTabId, + Date.now() + 60_000 + ) + ).toBe(stillPending) + }) + it('fails closed when reconnect routes a lifecycle close to an older host', async () => { const runtimeCall = vi .fn() diff --git a/src/renderer/src/runtime/web-runtime-session-tab-lifecycle.ts b/src/renderer/src/runtime/web-runtime-session-tab-lifecycle.ts index a7381f10d6a..67dfba846af 100644 --- a/src/renderer/src/runtime/web-runtime-session-tab-lifecycle.ts +++ b/src/renderer/src/runtime/web-runtime-session-tab-lifecycle.ts @@ -6,7 +6,11 @@ import type { import { useAppStore } from '../store' import { hasRuntimeRpcErrorCode, unwrapRuntimeRpcResult } from './runtime-rpc-client' import { toRuntimeWorktreeSelector } from './runtime-worktree-selector' -import { clearWebSessionCloseIntent, recordWebSessionCloseIntent } from './web-session-close-intent' +import { + clearWebSessionCloseIntent, + makeWebSessionCloseIntentDurable, + recordWebSessionCloseIntent +} from './web-session-close-intent' import { clearWebSessionFocusIntentIfMatches, recordWebSessionFocusIntent @@ -157,8 +161,16 @@ async function callWebRuntimeSessionTabMethod( if (activationHostTabId) { clearWebSessionFocusIntentIfMatches(intentOwner, args.worktreeId, activationHostTabId) } + // Why the split: only 'tab_not_found' is absence proof (see the outcome doc above). Restoring the + // mirror on it hands the user back a pane the host cannot close and whose handle is already gone + // (#9194), so keep the suppression and drop its TTL instead. Every other failure is a "not now". + const hostHasNoSuchTab = hasRuntimeRpcErrorCode(error, 'tab_not_found') for (const hostTabId of closeIntentTabIds) { - clearWebSessionCloseIntent(intentOwner, args.worktreeId, hostTabId) + if (hostHasNoSuchTab) { + makeWebSessionCloseIntentDurable(intentOwner, args.worktreeId, hostTabId) + } else { + clearWebSessionCloseIntent(intentOwner, args.worktreeId, hostTabId) + } } if (isLifecycleClose) { const { acceptReplayedWebSessionTabsSnapshot } = await import('./web-session-tabs-sync') @@ -171,6 +183,6 @@ async function callWebRuntimeSessionTabMethod( `[web-runtime-session] failed to ${isClose ? 'close' : 'activate'} tab:`, error instanceof Error ? error.message : String(error) ) - return hasRuntimeRpcErrorCode(error, 'tab_not_found') ? 'unknown-tab' : 'failed' + return hostHasNoSuchTab ? 'unknown-tab' : 'failed' } } diff --git a/src/renderer/src/runtime/web-session-close-intent.ts b/src/renderer/src/runtime/web-session-close-intent.ts index 012dfd81d26..140d3aa66ef 100644 --- a/src/renderer/src/runtime/web-session-close-intent.ts +++ b/src/renderer/src/runtime/web-session-close-intent.ts @@ -4,7 +4,7 @@ import { webSessionIntentOwnerKey, type WebSessionIntentOwner } from './web-sess const CLOSE_INTENT_TTL_MS = 10_000 -type CloseIntent = { recordedAt: number } +type CloseIntent = { recordedAt: number; durable: boolean } const pendingCloseByOwnerAndWorktree = new Map>() @@ -28,7 +28,27 @@ export function recordWebSessionCloseIntent( byTab = new Map() pendingCloseByOwnerAndWorktree.set(partitionKey, byTab) } - byTab.set(trimmed, { recordedAt: now }) + byTab.set(trimmed, { recordedAt: now, durable: byTab.get(trimmed)?.durable === true }) +} + +/** + * Why no TTL: `tab_not_found` is the host's definitive answer that it does not have this tab, yet a + * host can keep republishing the surface in its snapshot (#9194). Letting that intent age out + * re-materializes a pane whose handle is already gone, and the pane the user just closed comes back + * showing "Remote terminal was closed." with no way to dismiss it. The intent still clears the + * moment the surface leaves a snapshot, so a host that recovers the tab is never suppressed forever. + */ +export function makeWebSessionCloseIntentDurable( + owner: WebSessionIntentOwner, + worktreeId: string, + hostTabId: string +): void { + const intent = pendingCloseByOwnerAndWorktree + .get(closeIntentPartitionKey(owner, worktreeId)) + ?.get(hostTabId) + if (intent) { + intent.durable = true + } } export function isWebSessionCloseIntentPending( @@ -43,7 +63,7 @@ export function isWebSessionCloseIntentPending( if (!intent) { return false } - if (now - intent.recordedAt > CLOSE_INTENT_TTL_MS) { + if (!intent.durable && now - intent.recordedAt > CLOSE_INTENT_TTL_MS) { byTab!.delete(hostTabId) if (byTab!.size === 0) { pendingCloseByOwnerAndWorktree.delete(partitionKey)