From 0153d9846291c16fc01cd14044ec84d12610ee42 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:32:28 -0700 Subject: [PATCH 1/4] fix(terminal): ask before a tab close kills a pty the app cannot vouch for A rejected liveness probe observed nothing, but the tab-close guard read it as "idle" and closed, calling window.api.pty.kill. The window-close guard asks on the same signal, and this guard's own timeout arm asks on the same class of non-answer twenty lines up. Narrow the rejection arm the way the `unavailable` arm is already narrowed: a rejection blocks only for an id `ptyIdsByTabId` still tracks. The SSH-reconnect case that motivated failing open is a layout-only id, so it keeps closing. Also de-vacuates the `unavailable` guard test, which set `hasChildProcesses: true` and so exercised the live-child rule instead. --- .../running-terminal-close-guard.test.ts | 39 ++++++++++++++++--- .../terminal/running-terminal-close-guard.ts | 19 +++++---- 2 files changed, 43 insertions(+), 15 deletions(-) diff --git a/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts b/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts index eb7bb44f4b3..39d84689aab 100644 --- a/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts +++ b/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts @@ -181,13 +181,40 @@ describe('guardRunningTerminalClose', () => { expect(onClose).not.toHaveBeenCalled() }) - it('fails open and closes when the probe rejects (wedged relay / legacy provider)', async () => { + // Why not fail open: a rejection observed nothing, and this close kills the pty — the + // same destruction the window-close path stops and asks about on a rejected probe. + it('asks when the probe rejects for a tracked pty (wedged relay / legacy provider)', async () => { inspectRuntimeTerminalProcessMock.mockRejectedValue(new Error('rpc_timeout')) const onClose = vi.fn() guard(onClose) await settleProbe() + expect(onClose).not.toHaveBeenCalled() + expect(visibleRequest()).toMatchObject({ terminalTabId: 'tab-1' }) + }) + + // Why per-id and not per-tab: the rejection arm is narrowed by the same liveness map the + // `unavailable` arm uses, so a stale layout leaf cannot drag a closable tab into a prompt. + it('closes when only a layout-only pty rejects and the tracked pane is idle', async () => { + setState({ + ptyIdsByTabId: { 'tab-1': ['pty-a'] }, + terminalLayoutsByTabId: { + 'tab-1': { ptyIdsByLeafId: { [LEAF_A]: 'pty-a', [LEAF_B]: 'pty-stale' } } + } + }) + inspectRuntimeTerminalProcessMock.mockImplementation(async (_settings, ptyId: string) => { + if (ptyId === 'pty-stale') { + throw new Error('no registered provider owns this PTY id') + } + return { foregroundProcess: 'zsh', hasChildProcesses: false } + }) + const onClose = vi.fn() + + guard(onClose) + await settleProbe() + + expect(inspectRuntimeTerminalProcessMock).toHaveBeenCalledTimes(2) expect(onClose).toHaveBeenCalledTimes(1) expect(visibleRequest()).toBeNull() }) @@ -196,9 +223,11 @@ describe('guardRunningTerminalClose', () => { // Why not fail open: the host answered "I could not route to this pane", which is the // same non-answer this guard's own timeout already prompts on. It applies only to an id // the liveness map still vouches for; see running-terminal-close-absence-evidence.test.ts. + // `hasChildProcesses` stays false so this exercises the `unavailable` rule, not the + // live-child rule above it. inspectRuntimeTerminalProcessMock.mockResolvedValue({ foregroundProcess: null, - hasChildProcesses: true, + hasChildProcesses: false, unavailable: true }) const onClose = vi.fn() @@ -395,9 +424,9 @@ describe('guardRunningTerminalClose', () => { }) // Why: an SSH drop zeroes ptyIdsByTabId while the layout still names the pane. The stale - // binding is probed, that probe fails on the dead link, and the close falls open — so a - // reconnecting tab stays closable instead of being blocked behind a prompt for a pty - // nobody can reach. Documented so the behavior is a decision, not an accident. + // binding is probed and that probe fails on the dead link, but the id is layout-only, so + // the rejection arm's narrowing lets the close through — a reconnecting tab stays closable + // instead of being blocked behind a prompt for a pty nobody can reach. it('closes a reconnecting ssh tab whose pty ids were already zeroed', async () => { setState({ ptyIdsByTabId: { 'tab-1': [] } }) inspectRuntimeTerminalProcessMock.mockRejectedValue(new Error('ssh_disconnected')) diff --git a/src/renderer/src/components/terminal/running-terminal-close-guard.ts b/src/renderer/src/components/terminal/running-terminal-close-guard.ts index c5f9b81c9eb..8aa7764db21 100644 --- a/src/renderer/src/components/terminal/running-terminal-close-guard.ts +++ b/src/renderer/src/components/terminal/running-terminal-close-guard.ts @@ -129,19 +129,18 @@ export function guardRunningTerminalClose(params: { if (decided) { return } - // Why: fail open on a *rejection* (wedged relay, legacy provider), matching the Cmd+W - // pane path — a close button that silently does nothing is worse than closing a busy - // tab. `unavailable` now means exactly "could not ask", which this guard's own timeout - // already prompts on, so an answered non-answer asks too — but only for an id the - // liveness map still vouches for, the same id set the window-close guard reads. A - // layout-only id is usually a leftover leaf whose pane is long gone: it answers - // `unavailable` forever, and prompting on it would put a dialog in front of every - // cleanly-exited tab. It can still block the close by answering *positively*, which is - // the mounting-pane window the union exists for. + // Why: a non-answer asks — a rejection (wedged relay, legacy provider) and + // `unavailable` ("could not ask") are the same evidence as this guard's own timeout, + // and this close kills the pty, so it owes the same prompt the window-close path + // already gives. Both narrow to an id the liveness map still vouches for, the id set + // the window-close guard reads. A layout-only id is usually a leftover leaf whose pane + // is long gone — it answers `unavailable` or throws forever, and prompting on it would + // put a dialog in front of every cleanly-exited tab and every reconnecting ssh tab. It + // can still block by answering *positively*, the mounting-pane window the union exists for. const busyPtyIds = ptyIds.filter((ptyId, index) => { const result = results[index] if (result?.status !== 'fulfilled') { - return false + return trackedPtyIds.has(ptyId) } if (result.value.hasChildProcesses) { return true From 14f87d9531315241d70e7ef0dc8164f4e42f3f3d Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 20:24:07 -0700 Subject: [PATCH 2/4] fix(terminal): preserve close-probe verdicts --- ...ng-terminal-close-absence-evidence.test.ts | 8 +-- .../running-terminal-close-guard.test.ts | 50 ++++++++++++++ .../terminal/running-terminal-close-guard.ts | 66 ++++++++++++++----- 3 files changed, 103 insertions(+), 21 deletions(-) diff --git a/src/renderer/src/components/terminal/running-terminal-close-absence-evidence.test.ts b/src/renderer/src/components/terminal/running-terminal-close-absence-evidence.test.ts index 05ad945322a..cc78ef31485 100644 --- a/src/renderer/src/components/terminal/running-terminal-close-absence-evidence.test.ts +++ b/src/renderer/src/components/terminal/running-terminal-close-absence-evidence.test.ts @@ -132,9 +132,7 @@ describe('terminal-tab close on PTY absence evidence', () => { expect(onClose).not.toHaveBeenCalled() }) - it('leaves a degraded in-contact probe on its existing close-silently behavior', async () => { - // Loss of contact with the child-process probe on a pane the host still routes to: - // no `unavailable`, so this guard reaches no new verdict and behaves as it always has. + it('asks when an in-contact child-process probe is unverifiable', async () => { inspectRuntimeTerminalProcessMock.mockResolvedValue( buildPtyProcessInspectionWireResult( { verdict: 'unverifiable', reason: 'process table scan degraded' }, @@ -144,7 +142,7 @@ describe('terminal-tab close on PTY absence evidence', () => { const onClose = await closeTab() - expect(visibleRequest()).toBeNull() - expect(onClose).toHaveBeenCalledTimes(1) + expect(visibleRequest()).not.toBeNull() + expect(onClose).not.toHaveBeenCalled() }) }) diff --git a/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts b/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts index 39d84689aab..dbdb90299d9 100644 --- a/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts +++ b/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts @@ -239,6 +239,24 @@ describe('guardRunningTerminalClose', () => { expect(visibleRequest()).not.toBeNull() }) + it('asks when a tracked pty child-process inspection is unverifiable', async () => { + inspectRuntimeTerminalProcessMock.mockResolvedValue({ + foregroundProcess: null, + hasChildProcesses: false, + processEvidence: { + foreground: { verdict: 'unverifiable', reason: 'ps timed out' }, + children: { verdict: 'unverifiable', reason: 'ps timed out' } + } + }) + const onClose = vi.fn() + + guard(onClose) + await settleProbe() + + expect(onClose).not.toHaveBeenCalled() + expect(visibleRequest()).toMatchObject({ terminalTabId: 'tab-1' }) + }) + it('prompts once for a split tab where only the second pane is busy', async () => { setState({ ptyIdsByTabId: { 'tab-1': ['pty-a', 'pty-b'] }, @@ -386,6 +404,38 @@ describe('guardRunningTerminalClose', () => { expect(visibleRequest()?.copyKind).toBe('agent') }) + it('closes after a tracked pty exits when only a stale layout probe times out', async () => { + setState({ + ptyIdsByTabId: { 'tab-1': ['pty-a'] }, + terminalLayoutsByTabId: { + 'tab-1': { ptyIdsByLeafId: { [LEAF_A]: 'pty-a', [LEAF_B]: 'pty-stale' } } + } + }) + inspectRuntimeTerminalProcessMock.mockImplementation(async (_settings, ptyId: string) => { + if (ptyId === 'pty-stale') { + return new Promise(() => {}) + } + return { + foregroundProcess: 'zsh', + hasChildProcesses: false, + processEvidence: { + foreground: { verdict: 'observed' as const, processName: 'zsh' }, + children: { verdict: 'exited' as const } + } + } + }) + vi.useFakeTimers() + const onClose = vi.fn() + + guard(onClose) + await settleProbe() + vi.advanceTimersByTime(RUNNING_CLOSE_PROBE_TIMEOUT_MS) + vi.useRealTimers() + + expect(onClose).toHaveBeenCalledTimes(1) + expect(visibleRequest()).toBeNull() + }) + it('closes rather than wedging when the timed-out prompt throws', async () => { const requestSpy = vi .spyOn(useRunningTerminalCloseConfirmStore.getState(), 'requestRunningTerminalCloseConfirm') diff --git a/src/renderer/src/components/terminal/running-terminal-close-guard.ts b/src/renderer/src/components/terminal/running-terminal-close-guard.ts index 8aa7764db21..8f35fcbb0c7 100644 --- a/src/renderer/src/components/terminal/running-terminal-close-guard.ts +++ b/src/renderer/src/components/terminal/running-terminal-close-guard.ts @@ -1,8 +1,12 @@ import { useAppStore } from '@/store' -import { inspectRuntimeTerminalProcess } from '@/runtime/runtime-terminal-inspection' +import { + inspectRuntimeTerminalProcess, + type RuntimeTerminalProcessInspection +} from '@/runtime/runtime-terminal-inspection' import { useRunningTerminalCloseConfirmStore } from '@/store/running-terminal-close-confirm' import type { TerminalTabCloseReason } from '@/store/slices/terminal-tab-retirement' import type { AppState } from '@/store/types' +import { readPtyProcessInspectionEvidence } from '../../../../shared/pty-process-inspection-evidence' import { resolveBusyPtyCloseCopyKind } from './terminal-close-copy-kind' export type RunningTerminalCloseGuardOptions = { @@ -64,6 +68,26 @@ function collectTabPtyIds( return { ptyIds: [...ptyIds], trackedPtyIds } } +type SettledCloseProbe = + | { status: 'fulfilled'; value: RuntimeTerminalProcessInspection } + | { status: 'rejected' } + +function shouldConfirmForProbe( + ptyId: string, + trackedPtyIds: ReadonlySet, + probe: SettledCloseProbe | undefined +): boolean { + const tracked = trackedPtyIds.has(ptyId) + if (probe === undefined || probe.status === 'rejected') { + return tracked + } + if (probe.value.unavailable === true) { + return tracked + } + const children = readPtyProcessInspectionEvidence(probe.value).children + return children.verdict === 'live' || (children.verdict === 'unverifiable' && tracked) +} + /** * Routes an interactive terminal-tab close through the running-process confirmation. * Closes immediately when nothing is running, so idle tabs keep today's behavior. @@ -113,17 +137,34 @@ export function guardRunningTerminalClose(params: { decided = true } + const settledProbes = new Map() const probeTimeout = setTimeout(() => { try { - // Why: a probe that has not answered yet is unknown, not idle. Ask, treating every pty - // as a candidate, so a degraded relay costs a click instead of a killed remote command. - confirmClose(ptyIds) + const busyPtyIds = ptyIds.filter((ptyId) => + shouldConfirmForProbe(ptyId, trackedPtyIds, settledProbes.get(ptyId)) + ) + if (busyPtyIds.length === 0) { + closeNow() + return + } + confirmClose(busyPtyIds) } catch { closeNow() } }, RUNNING_CLOSE_PROBE_TIMEOUT_MS) - void Promise.allSettled(ptyIds.map((ptyId) => inspectRuntimeTerminalProcess(settings, ptyId))) + const probes = ptyIds.map(async (ptyId): Promise => { + let probe: SettledCloseProbe + try { + probe = { status: 'fulfilled', value: await inspectRuntimeTerminalProcess(settings, ptyId) } + } catch { + probe = { status: 'rejected' } + } + settledProbes.set(ptyId, probe) + return probe + }) + + void Promise.all(probes) .then((results) => { clearTimeout(probeTimeout) if (decided) { @@ -137,23 +178,16 @@ export function guardRunningTerminalClose(params: { // is long gone — it answers `unavailable` or throws forever, and prompting on it would // put a dialog in front of every cleanly-exited tab and every reconnecting ssh tab. It // can still block by answering *positively*, the mounting-pane window the union exists for. - const busyPtyIds = ptyIds.filter((ptyId, index) => { - const result = results[index] - if (result?.status !== 'fulfilled') { - return trackedPtyIds.has(ptyId) - } - if (result.value.hasChildProcesses) { - return true - } - return result.value.unavailable === true && trackedPtyIds.has(ptyId) - }) + const busyPtyIds = ptyIds.filter((ptyId, index) => + shouldConfirmForProbe(ptyId, trackedPtyIds, results[index]) + ) if (busyPtyIds.length === 0) { closeNow() return } confirmClose(busyPtyIds) }) - // Why: allSettled never rejects, so this only fires when the decision above throws (a + // Why: each probe catches its own rejection, so this only fires when the decision above throws (a // copy-kind lookup, a store subscriber). Without it the tab would silently never close // and the user would get no feedback at all; the pane path it replaced had this catch. .catch(() => { From 211104cc99b3a3ddb94568ec83e6016339d9e092 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 21:14:54 -0700 Subject: [PATCH 3/4] fix(terminal): keep a legacy positive child-process report voting on tab close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `hasChildProcesses` is `children.verdict === 'live'` collapsed, so only its `false` pole is lossy. Reading only the verdict therefore drops a positive observation from a peer this client cannot vouch for — a malformed or foreign `processEvidence` normalizes to `unverifiable`, which needs a tracked id to prompt, while the legacy `true` beside it is a real report that children exist. Restores that vote inside shouldConfirmForProbe, so the polarity rule holds in the one helper both the probe path and the timeout path read. --- .../components/terminal/running-terminal-close-guard.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/renderer/src/components/terminal/running-terminal-close-guard.ts b/src/renderer/src/components/terminal/running-terminal-close-guard.ts index 8f35fcbb0c7..a0f7c722aad 100644 --- a/src/renderer/src/components/terminal/running-terminal-close-guard.ts +++ b/src/renderer/src/components/terminal/running-terminal-close-guard.ts @@ -85,7 +85,14 @@ function shouldConfirmForProbe( return tracked } const children = readPtyProcessInspectionEvidence(probe.value).children - return children.verdict === 'live' || (children.verdict === 'unverifiable' && tracked) + // Why `hasChildProcesses` still votes: it is `children.verdict === 'live'` collapsed, so only + // its `false` pole is lossy. A `true` beside evidence this client cannot vouch for — a + // malformed or foreign `processEvidence`, which reads as `unverifiable` — is still a positive + // observation, and the #16900/#16908 polarity rule keeps a positive observation's vote. + if (children.verdict === 'live' || probe.value.hasChildProcesses) { + return true + } + return children.verdict === 'unverifiable' && tracked } /** From 1a61c269d84688f56ffe0bf562a9b87108d15cb1 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 21:54:46 -0700 Subject: [PATCH 4/4] fix(terminal): let the children verdict decide the tab close on its own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The legacy `hasChildProcesses` vote added one commit ago is removed, so this guard and the window-close guard now read exactly one signal each and read the same one (#17077 removed the same vote there). Measured, not argued. A 186-input cross-product through the real guard shows the vote changes the outcome on 24 inputs in two shapes: an `exited` verdict beside a legacy `true`, and an `unverifiable` verdict beside a legacy `true` on a layout-only id. A producer sweep over every builder that can reach this guard finds nothing that emits the first — `buildPtyProcessInspectionWireResult`, `classifyLocalPtyChildProcesses`, `composeLegacyPtyProcessInspection` and `buildDaemonInspectProcessResult` all set the boolean from the verdict or from a name that decides both. The second shape is real, from one producer: a daemon pane whose `SubprocessHandle` has no evidence channel publishes the boolean off node-pty's raw title while the children verdict stays `unverifiable`. That producer is why the vote goes rather than stays. The same degraded read publishes `true` for a `codex` title and `false` for a `zsh` one, so the vote asks or closes on the foreground name while the host is saying it observed neither life nor exit — the legacy collapse being spent as evidence, which is what this stack exists to stop. The host states it outright, in the assertion that pins the shape: terminal-host-inspection-degraded-handle.test.ts, "does not upgrade a named agent to observed either". With the vote gone the arm above answers instead: a non-answer asks for an id the liveness map vouches for and closes for a leftover layout leaf, uniformly for all three non-answer kinds. --- .../terminal/running-terminal-close-guard.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/renderer/src/components/terminal/running-terminal-close-guard.ts b/src/renderer/src/components/terminal/running-terminal-close-guard.ts index a0f7c722aad..183351c4c60 100644 --- a/src/renderer/src/components/terminal/running-terminal-close-guard.ts +++ b/src/renderer/src/components/terminal/running-terminal-close-guard.ts @@ -85,11 +85,14 @@ function shouldConfirmForProbe( return tracked } const children = readPtyProcessInspectionEvidence(probe.value).children - // Why `hasChildProcesses` still votes: it is `children.verdict === 'live'` collapsed, so only - // its `false` pole is lossy. A `true` beside evidence this client cannot vouch for — a - // malformed or foreign `processEvidence`, which reads as `unverifiable` — is still a positive - // observation, and the #16900/#16908 polarity rule keeps a positive observation's vote. - if (children.verdict === 'live' || probe.value.hasChildProcesses) { + // Why the verdict alone decides, with no vote from `hasChildProcesses`: the boolean is + // `children.verdict === 'live'` collapsed, so it says nothing new on the positive pole and + // nothing trustworthy on the others. The one producer that publishes `true` beside a + // non-`live` verdict is a daemon pane whose handle has no evidence channel, and that host + // states outright that such a read proves neither life nor exit — so voting on it would ask + // for a non-shell title and close silently for a shell one, off the very same degraded read. + // The window-close guard reads this same single signal (#17077). + if (children.verdict === 'live') { return true } return children.verdict === 'unverifiable' && tracked