mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 08:03:39 +00:00
test(pty): pin the renderer-liveness guard STA-2373 relies on (STA-5373)
STA-5373 reported that #15927 dropped the `webContents.isDestroyed()` half of the guard #10065 added for STA-2373, and that the app therefore dies when a daemon death fans out to every pane. The guard is present on main: #15172 restored it at both senders when it split the PTY monolith, and a per-file count across v1.4.188..HEAD shows only relocation (pty.ts:4 -> write-input.ts:2 + bind-listeners.ts:2). The reported 4 -> 0 was scoped to `src/main/ipc/pty.ts`, now a 39-line barrel. No production fix is needed. What was real is that nothing pinned the guard — it survived #15927 only because #15172 happened to re-expand it — and the shared PTY test fake made that invisible: its `webContents` had no `isDestroyed` at all, so both guards passed vacuously in every suite. That is also why they are written defensively as `typeof ... === 'function' && ...`. - Give the fake an `isDestroyed` mock, re-stubbed to `false` each beforeEach rather than left `undefined`, so "alive" is stated rather than accidental. - Cover both senders red/green: the daemon-death fan-out (bind-listeners.ts:37) and the per-write reporter (write-input.ts:57). Verified red against a window-only guard and green with the real one. The per-write case needs a chunked write. A single-chunk write is already fenced by `isPtyWriteEventFromMainWindow`, which rejects the event once the WebContents is gone; only the multi-chunk path yields a macrotask mid-write, letting the renderer die after the sender check passes. That is the sole route to this sender with a dead WebContents, and what makes its own guard load-bearing. Also deletes `src/main/ipc/pty-renderer-surface.ts`: zero importers repo-wide, and its `isRendererGone` is exactly the weakened predicate. Its comment claims the headless path "now passes null", but register-headless-runtime.ts:26 still fakes `{ isDestroyed: () => true }` — #15172 rolled that back too. Adopting it would reintroduce STA-5373 for real. Verified: 17 tests across the two pty write suites; full src/main/ipc run 3156 pass (4 pre-existing @parcel/watcher failures in filesystem-watcher-real and worktree-base-directory-poller, unrelated and failing identically on a pristine tree); pnpm typecheck clean; oxlint clean.
This commit is contained in:
@@ -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()
|
||||
|
||||
@@ -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<string> {
|
||||
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()
|
||||
)
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -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
|
||||
}
|
||||
Reference in New Issue
Block a user