diff --git a/src/main/ipc/pty-ipc-suite-environment.ts b/src/main/ipc/pty-ipc-suite-environment.ts index 5e5a9c819f7..95c7dfe3eea 100644 --- a/src/main/ipc/pty-ipc-suite-environment.ts +++ b/src/main/ipc/pty-ipc-suite-environment.ts @@ -54,7 +54,15 @@ import { __resetShellStartupEnvCache } from '../pty/shell-startup-env' import { _resetWslCachesForTests } from '../wsl' /** The mocked webContents each suite asserts sends against. */ -export type PtyIpcTestWebContents = { on: Mock; send: Mock; removeListener: Mock } +export type PtyIpcTestWebContents = { + on: Mock + send: Mock + removeListener: Mock + // Why real Electron has this: webContents can be destroyed a beat before its BrowserWindow + // during close, so renderer-liveness guards check both. Omitting it here let those guards + // pass vacuously in every suite (STA-2373 / STA-5373). + isDestroyed: Mock +} /** The mocked BrowserWindow handed to registerPtyHandlers. */ export type PtyIpcTestMainWindow = { @@ -83,12 +91,18 @@ export function createPtyIpcSuiteEnvironment(): PtyIpcSuiteEnvironment { webContents: { on: vi.fn(), send: vi.fn(), - removeListener: vi.fn() + removeListener: vi.fn(), + isDestroyed: vi.fn(() => false) } } const mainWindowIpcEvent = { sender: mainWindow.webContents } const foreignWindowIpcEvent = { - sender: { on: vi.fn(), send: vi.fn(), removeListener: vi.fn() } + sender: { + on: vi.fn(), + send: vi.fn(), + removeListener: vi.fn(), + isDestroyed: vi.fn(() => false) + } } const envScope = createPtyIpcProcessEnvScope() @@ -155,6 +169,10 @@ export function createPtyIpcSuiteEnvironment(): PtyIpcSuiteEnvironment { mainWindow.webContents.on.mockReset() mainWindow.webContents.send.mockReset() mainWindow.webContents.removeListener.mockReset() + // Why re-stub, not just reset: a bare mockReset returns undefined, which reads as "alive" + // by accident rather than by intent, and hides a test that forgot to restore liveness. + mainWindow.webContents.isDestroyed.mockReset() + mainWindow.webContents.isDestroyed.mockReturnValue(false) // Why: hidden-delivery gate state is module-level (PTY-keyed), so tests must not leak hidden bits across cases. _resetHiddenRendererPtyDeliveryGateForTest() __resetShellStartupEnvCache() diff --git a/src/main/ipc/pty-renderer-liveness-guard.test.ts b/src/main/ipc/pty-renderer-liveness-guard.test.ts new file mode 100644 index 00000000000..08a925067d8 --- /dev/null +++ b/src/main/ipc/pty-renderer-liveness-guard.test.ts @@ -0,0 +1,167 @@ +import { describe, expect, it, vi } from 'vitest' +import { setupPtyIpcSuite } from './pty-ipc-test-harness' +import { PtyWriteUnavailableError } from '../providers/pty-write-unavailable-error' +import { TERMINAL_INPUT_CHUNK_MAX_BYTES } from '../../shared/terminal-input' +import { registerPtyHandlers } from './pty' + +vi.mock('electron', () => import('./pty-ipc-mock-registry').then((m) => m.electronModuleMock())) +vi.mock('fs', () => import('./pty-ipc-mock-registry').then((m) => m.fsModuleMock())) +vi.mock('node-pty', () => import('./pty-ipc-mock-registry').then((m) => m.nodePtyModuleMock())) +vi.mock('node:child_process', async (importOriginal) => + (await import('./pty-ipc-mock-registry')).childProcessModuleMock(await importOriginal()) +) +vi.mock('../opencode/hook-service', () => + import('./pty-ipc-mock-registry').then((m) => m.openCodeHookServiceModuleMock()) +) +vi.mock('../mimo/hook-service', () => + import('./pty-ipc-mock-registry').then((m) => m.mimoHookServiceModuleMock()) +) +vi.mock('../agent-hooks/server', () => + import('./pty-ipc-mock-registry').then((m) => m.agentHookServerModuleMock()) +) +vi.mock('../pi/titlebar-extension-service', () => + import('./pty-ipc-mock-registry').then((m) => m.piTitlebarExtensionModuleMock()) +) +vi.mock('../pwsh', () => import('./pty-ipc-mock-registry').then((m) => m.pwshModuleMock())) +vi.mock('../wsl', async (importOriginal) => + (await import('./pty-ipc-mock-registry')).wslModuleMock(await importOriginal()) +) +vi.mock('../telemetry/client', () => + import('./pty-ipc-mock-registry').then((m) => m.telemetryClientModuleMock()) +) +vi.mock('../telemetry/classify-error', () => + import('./pty-ipc-mock-registry').then((m) => m.classifyErrorModuleMock()) +) +vi.mock('../cli/linux-terminal-orca-cli-shim', () => + import('./pty-ipc-mock-registry').then((m) => m.linuxCliShimModuleMock()) +) +vi.mock('../memory/pty-registry', () => + import('./pty-ipc-mock-registry').then((m) => m.ptyRegistryModuleMock()) +) +vi.mock('../agent-hooks/migration-unsupported-pty-state', () => + import('./pty-ipc-mock-registry').then((m) => m.migrationUnsupportedPtyModuleMock()) +) +vi.mock('../codex/codex-pane-account-registry', () => + import('./pty-ipc-mock-registry').then((m) => m.codexPaneAccountRegistryModuleMock()) +) +vi.mock('../codex/codex-state-db-backfill-recovery', () => + import('./pty-ipc-mock-registry').then((m) => m.codexBackfillRecoveryModuleMock()) +) + +/** + * A destroyed WebContents under a still-alive BrowserWindow is the STA-2373 state: + * `webContents` can die a beat before its window during close (see createMainWindow.ts + * forceRepaint), and `webContents.send` throws `Object has been destroyed` there. An + * uncaught throw in the main process is fatal — the app exits with every terminal in it, + * clean exit code and no crash report — so both `pty:writeUnavailable` senders must check + * the WebContents, not just the window. #15927 briefly reduced these to a window-only + * check; these tests fail against that regression (STA-5373). + */ +describe('pty:writeUnavailable renderer-liveness guard', () => { + const { + handlers, + mainWindow, + mainWindowIpcEvent, + installDaemonTestProvider, + getPtyWriteListener + } = setupPtyIpcSuite() + + async function spawnDaemonPty(): Promise { + const result = (await handlers.get('pty:spawn')!(null, { cols: 80, rows: 24 })) as { + id: string + } + mainWindow.webContents.send.mockClear() + return result.id + } + + describe('the per-write sender', () => { + it('asks the renderer to remount while the WebContents is alive', async () => { + installDaemonTestProvider({ + write: vi.fn(() => { + throw new PtyWriteUnavailableError('daemon generation lost') + }) + }) + registerPtyHandlers(mainWindow as never) + const id = await spawnDaemonPty() + + getPtyWriteListener()(mainWindowIpcEvent, { id, data: 'x' }) + + expect(mainWindow.webContents.send).toHaveBeenCalledWith('pty:writeUnavailable', { id }) + }) + + // Why a chunked write: the single-chunk path is already fenced by + // isPtyWriteEventFromMainWindow, which rejects the event outright once the WebContents + // is gone. A multi-chunk write awaits a macrotask between chunks, so the sender check + // passes while alive and the renderer dies mid-write — the only way this sender is + // reached with a destroyed WebContents, and what makes its own guard load-bearing. + it('stays silent when the WebContents dies mid-write under a live window', async () => { + let writeCount = 0 + installDaemonTestProvider({ + write: vi.fn(() => { + writeCount += 1 + if (writeCount < 2) { + return + } + mainWindow.webContents.isDestroyed.mockReturnValue(true) + throw new PtyWriteUnavailableError('daemon generation lost') + }) + }) + registerPtyHandlers(mainWindow as never) + const id = await spawnDaemonPty() + + getPtyWriteListener()(mainWindowIpcEvent, { + id, + data: 'x'.repeat(TERMINAL_INPUT_CHUNK_MAX_BYTES * 3) + }) + // The chunk loop yields a macrotask between chunks; wait for the failing one. + await vi.waitFor(() => expect(writeCount).toBeGreaterThan(1)) + + expect(mainWindow.webContents.isDestroyed).toHaveBeenCalled() + expect(mainWindow.webContents.send).not.toHaveBeenCalledWith( + 'pty:writeUnavailable', + expect.anything() + ) + }) + }) + + describe('the daemon-death fan-out', () => { + /** Registers handlers and returns the fan-out callback the provider hands the session. */ + function installProviderCapturingFanout(): (payload: { id: string }) => void { + let fanout: ((payload: { id: string }) => void) | null = null + installDaemonTestProvider({ + onWriteUnavailable: vi.fn((callback: (payload: { id: string }) => void) => { + fanout = callback + return () => {} + }) + }) + registerPtyHandlers(mainWindow as never) + if (!fanout) { + throw new Error('registerPtyHandlers did not subscribe to onWriteUnavailable') + } + return fanout + } + + it('remounts every signalled pane while the WebContents is alive', () => { + const fanout = installProviderCapturingFanout() + + fanout({ id: 'pane-1' }) + + expect(mainWindow.webContents.send).toHaveBeenCalledWith('pty:writeUnavailable', { + id: 'pane-1' + }) + }) + + it('stays silent when the WebContents is destroyed under a live window', () => { + const fanout = installProviderCapturingFanout() + mainWindow.webContents.isDestroyed.mockReturnValue(true) + + fanout({ id: 'pane-1' }) + + expect(mainWindow.webContents.isDestroyed).toHaveBeenCalled() + expect(mainWindow.webContents.send).not.toHaveBeenCalledWith( + 'pty:writeUnavailable', + expect.anything() + ) + }) + }) +}) diff --git a/src/main/ipc/pty-renderer-surface.ts b/src/main/ipc/pty-renderer-surface.ts deleted file mode 100644 index 09fff75e681..00000000000 --- a/src/main/ipc/pty-renderer-surface.ts +++ /dev/null @@ -1,37 +0,0 @@ -import type { BrowserWindow, WebContents } from 'electron' - -/** - * The renderer surface the PTY handlers talk to, which may not exist. - * - * Why: `orca serve` — and a future Node-only backend — run the same PTY handlers - * with no window. That used to be faked: `registerHeadlessPtyRuntime` built a - * `BrowserWindow` whose `isDestroyed()` returned true and whose `webContents.send` - * was a no-op, purely to satisfy the type — the "looks fine, silently lies" shape - * this codebase rejects elsewhere, and what forced an `electron` value import into a - * path that needs none. It now passes `null`. - * - * An absent renderer is semantically identical to a destroyed one — every call site - * already guards on `isDestroyed()` and skips — so model it as `null` and say so. - */ - -/** True when there is no renderer, or it is gone. Callers already treat these the same. */ -export function isRendererGone(window: BrowserWindow | null): boolean { - return window === null || window.isDestroyed() -} - -/** Send to the renderer if one is listening. Absent renderer drops the message, as a destroyed one does. */ -export function sendToRenderer( - window: BrowserWindow | null, - channel: string, - payload?: unknown -): void { - if (isRendererGone(window)) { - return - } - window!.webContents.send(channel, payload) -} - -/** The renderer's WebContents, or null. Used for identity checks and listener registration. */ -export function rendererWebContents(window: BrowserWindow | null): WebContents | null { - return isRendererGone(window) ? null : window!.webContents -}