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 eb7bb44f4b3..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 @@ -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() @@ -210,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'] }, @@ -357,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') @@ -395,9 +474,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..183351c4c60 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,36 @@ 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 + // 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 +} + /** * 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,48 +147,57 @@ 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) { 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. - const busyPtyIds = ptyIds.filter((ptyId, index) => { - const result = results[index] - if (result?.status !== 'fulfilled') { - return false - } - if (result.value.hasChildProcesses) { - return true - } - return result.value.unavailable === true && trackedPtyIds.has(ptyId) - }) + // 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) => + 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(() => {