From 20201f3ae822b3f60cd8bdbcdc33b4ba19bf45d3 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 3 Jul 2026 20:09:19 -0700 Subject: [PATCH] Fix #5787: recovery reload no longer sweeps local PTYs; guard close for hung-but-alive renderer (#7290) Co-authored-by: Orca --- src/main/index.ts | 33 ++++ src/main/ipc/pty.test.ts | 174 +++++++++++++++++- src/main/ipc/pty.ts | 14 +- .../window/attach-main-window-services.ts | 5 +- src/main/window/createMainWindow.test.ts | 48 +++++ src/main/window/createMainWindow.ts | 39 ++-- src/main/window/window-close-decision.test.ts | 57 ++++++ src/main/window/window-close-decision.ts | 32 ++++ 8 files changed, 386 insertions(+), 16 deletions(-) create mode 100644 src/main/window/window-close-decision.test.ts create mode 100644 src/main/window/window-close-decision.ts diff --git a/src/main/index.ts b/src/main/index.ts index dc298ddbd..37e1d6d47 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -210,6 +210,9 @@ let watcherShutdownDone = false let automations: AutomationService | null = null let keybindings: KeybindingService | null = null let expectedRendererReload: { webContentsId: number; until: number } | null = null +// Why: the crash/freeze-recovery reload re-fires did-finish-load; flag it so the +// local-PTY orphan sweep spares live sessions across that one reload (#5787). +let recoveryReloadInFlight: { webContentsId: number; until: number } | null = null let firstWindowStartupServicesReady: Promise = Promise.resolve() // Why: GPU child crashes clustered right after launch indicate a broken driver; // track them so Orca can move this build onto software rendering. @@ -458,6 +461,27 @@ function getExpectedTeardownScope(webContentsId?: number): ExpectedTeardownScope : 'none' } +function markRecoveryReloadInFlight(webContentsId: number, durationMs = 10_000): void { + recoveryReloadInFlight = { webContentsId, until: Date.now() + durationMs } +} + +function isRecoveryReloadInFlight(webContentsId: number): boolean { + if (!recoveryReloadInFlight) { + return false + } + if (Date.now() > recoveryReloadInFlight.until) { + recoveryReloadInFlight = null + return false + } + if (recoveryReloadInFlight.webContentsId !== webContentsId) { + return false + } + // Why: consume on read — the recovery reload fires exactly one did-finish-load, + // so clearing here keeps a later genuine reload sweeping orphaned local PTYs. + recoveryReloadInFlight = null + return true +} + function recordAgentStateCrashBreadcrumb(agentType: string, state: string): void { // Why: hook pings can arrive many times per second while an agent works. // Coalescing preserves crash-report room for renderer errors and memory @@ -782,6 +806,12 @@ function openMainWindow(): BrowserWindow { markExpectedRendererReload(webContentsId) } recordCrashBreadcrumb('manual_reload_requested', { ignoreCache }) + }, + // Why: the in-place recovery reload re-fires did-finish-load; flag it so the + // local-PTY orphan sweep is skipped for that one reload (#5787). + onBeforeRecoveryReload: (webContentsId) => { + markRecoveryReloadInFlight(webContentsId) + recordCrashBreadcrumb('renderer_recovery_reload') } }) recordCrashBreadcrumb('main_window_created') @@ -890,6 +920,9 @@ function openMainWindow(): BrowserWindow { } recordCrashBreadcrumb('renderer_reload_requested', { ignoreCache }) }, + // Why: let the PTY layer skip its orphan sweep on the one recovery reload + // that re-fires did-finish-load, so live local sessions survive it (#5787). + isRecoveryReloadInFlight, onBeforeUpdateQuit: () => preserveAgentAuthBeforeRestart({ codexRuntimeHome, claudeRuntimeAuth, store }) } diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 1cde7401c..71f742a71 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -186,9 +186,11 @@ import { setPtyOwnership, setLocalPtyProvider, rebindLocalProviderListeners, - unregisterSshPtyProvider + unregisterSshPtyProvider, + getLocalPtyProvider } from './pty' import { hasLiveClaudePtys, markClaudePtySpawned } from '../claude-accounts/live-pty-gate' +import * as livePtyGate from '../claude-accounts/live-pty-gate' import { encodePowerShellCommand, getPowerShellOsc133Bootstrap @@ -7930,6 +7932,176 @@ describe('registerPtyHandlers', () => { ).toBe(false) }) + // Why (#5787): a crash/freeze-recovery reload re-fires did-finish-load on the + // single window. The orphan sweep must be suppressed for it so live LOCAL PTYs + // stay attached until session restore re-adopts them. + it('does not sweep local PTYs during a recovery reload', async () => { + const killSpy = vi.fn() + const proc = { + onData: vi.fn(() => makeDisposable()), + onExit: vi.fn(() => makeDisposable()), + write: vi.fn(), + resize: vi.fn(), + kill: killSpy, + process: 'zsh', + pid: 12345 + } + const runtime = { + setPtyController: vi.fn(), + onPtySpawned: vi.fn(), + onPtyData: vi.fn(), + onPtyExit: vi.fn(), + preAllocateHandleForPty: vi.fn() + } + spawnMock.mockReturnValue(proc) + const isRecoveryReloadInFlight = vi.fn(() => true) + const markClaudePtyExitedSpy = vi.spyOn(livePtyGate, 'markClaudePtyExited') + + registerPtyHandlers( + mainWindow as never, + runtime as never, + undefined, + undefined, + undefined, + undefined, + { isRecoveryReloadInFlight } + ) + const didFinishLoad = mainWindow.webContents.on.mock.calls.find( + ([eventName]) => eventName === 'did-finish-load' + )?.[1] as (() => void) | undefined + expect(didFinishLoad).toBeTypeOf('function') + + const spawnResult = (await handlers.get('pty:spawn')!(null, { cols: 80, rows: 24 })) as { + id: string + } + + // Without the guard the second load would sweep this PTY as a prior-generation + // orphan. Under recovery-in-flight neither load may touch it. + didFinishLoad?.() + didFinishLoad?.() + + expect(killSpy).not.toHaveBeenCalled() + expect(runtime.onPtyExit).not.toHaveBeenCalled() + expect(markClaudePtyExitedSpy).not.toHaveBeenCalled() + const listed = await getLocalPtyProvider().listProcesses() + expect(listed.some((info) => info.id === spawnResult.id)).toBe(true) + + markClaudePtyExitedSpy.mockRestore() + }) + + // Why: guard against over-suppression — when no recovery reload is in flight the + // sweep MUST still reclaim genuinely orphaned local PTYs. + it('still sweeps orphaned local PTYs when no recovery reload is in flight', async () => { + const killSpy = vi.fn() + const proc = { + onData: vi.fn(() => makeDisposable()), + onExit: vi.fn(() => makeDisposable()), + write: vi.fn(), + resize: vi.fn(), + kill: killSpy, + process: 'zsh', + pid: 12345 + } + const runtime = { + setPtyController: vi.fn(), + onPtySpawned: vi.fn(), + onPtyData: vi.fn(), + onPtyExit: vi.fn(), + preAllocateHandleForPty: vi.fn() + } + spawnMock.mockReturnValue(proc) + const isRecoveryReloadInFlight = vi.fn(() => false) + + registerPtyHandlers( + mainWindow as never, + runtime as never, + undefined, + undefined, + undefined, + undefined, + { isRecoveryReloadInFlight } + ) + const didFinishLoad = mainWindow.webContents.on.mock.calls.find( + ([eventName]) => eventName === 'did-finish-load' + )?.[1] as (() => void) | undefined + + const spawnResult = (await handlers.get('pty:spawn')!(null, { cols: 80, rows: 24 })) as { + id: string + } + + // First load only advances the generation; the second sees this PTY as a + // prior-load orphan. With the flag false the guard must NOT suppress the sweep. + didFinishLoad?.() + didFinishLoad?.() + + expect(killSpy).toHaveBeenCalled() + expect(runtime.onPtyExit).toHaveBeenCalledWith(spawnResult.id, -1) + const listed = await getLocalPtyProvider().listProcesses() + expect(listed.some((info) => info.id === spawnResult.id)).toBe(false) + }) + + // Why (#5787): two PTYs spawned in different load generations must BOTH survive a + // recovery reload — even the older one that a normal sweep would reclaim. + it('keeps local PTYs from different generations alive across recovery reloads', async () => { + const killSpyA = vi.fn() + const killSpyB = vi.fn() + const runtime = { + setPtyController: vi.fn(), + onPtySpawned: vi.fn(), + onPtyData: vi.fn(), + onPtyExit: vi.fn(), + preAllocateHandleForPty: vi.fn() + } + const isRecoveryReloadInFlight = vi.fn(() => true) + + registerPtyHandlers( + mainWindow as never, + runtime as never, + undefined, + undefined, + undefined, + undefined, + { isRecoveryReloadInFlight } + ) + const didFinishLoad = mainWindow.webContents.on.mock.calls.find( + ([eventName]) => eventName === 'did-finish-load' + )?.[1] as (() => void) | undefined + + spawnMock.mockReturnValue({ + onData: vi.fn(() => makeDisposable()), + onExit: vi.fn(() => makeDisposable()), + write: vi.fn(), + resize: vi.fn(), + kill: killSpyA, + process: 'zsh', + pid: 111 + }) + const ptyA = (await handlers.get('pty:spawn')!(null, { cols: 80, rows: 24 })) as { id: string } + + // Advance the generation without sweeping (recovery-in-flight), then spawn a + // second PTY so the two live in different load generations. + didFinishLoad?.() + + spawnMock.mockReturnValue({ + onData: vi.fn(() => makeDisposable()), + onExit: vi.fn(() => makeDisposable()), + write: vi.fn(), + resize: vi.fn(), + kill: killSpyB, + process: 'zsh', + pid: 222 + }) + const ptyB = (await handlers.get('pty:spawn')!(null, { cols: 80, rows: 24 })) as { id: string } + + didFinishLoad?.() + + expect(killSpyA).not.toHaveBeenCalled() + expect(killSpyB).not.toHaveBeenCalled() + const ids = (await getLocalPtyProvider().listProcesses()).map((info) => info.id) + expect(ids).toContain(ptyA.id) + expect(ids).toContain(ptyB.id) + }) + it('clears PTY state even when kill reports the process is already gone', async () => { const proc = { onData: vi.fn(() => makeDisposable()), diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 42806caac..9538f2464 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -1247,6 +1247,9 @@ export function registerPtyHandlers( store?: Store, options?: { awaitLocalPtyStartup?: () => Promise + // Why: returns true (once, consuming the flag) for the crash-recovery reload + // so its did-finish-load skips the orphan sweep and keeps live PTYs (#5787). + isRecoveryReloadInFlight?: (webContentsId: number) => boolean } ): void { registerRendererLifecycleResetHandlers(mainWindow.webContents) @@ -1934,7 +1937,16 @@ export function registerPtyHandlers( if (localProvider instanceof LocalPtyProvider) { const lp = localProvider didFinishLoadHandler = () => { - const killed = lp.killOrphanedPtys(lp.advanceGeneration() - 1) + // Why: always advance so the load generation stays monotonic, but skip the + // sweep (and its per-PTY cleanup) on the crash/freeze-recovery reload — it + // would kill live LOCAL PTYs across the single window before session + // restore re-attaches them (#5787). The getter consumes the flag, so the + // next genuine reload still reclaims genuinely-orphaned PTYs. + const generation = lp.advanceGeneration() + if (options?.isRecoveryReloadInFlight?.(mainWindow.webContents.id)) { + return + } + const killed = lp.killOrphanedPtys(generation - 1) for (const { id } of killed) { clearProviderPtyState(id) ptyOwnership.delete(id) diff --git a/src/main/window/attach-main-window-services.ts b/src/main/window/attach-main-window-services.ts index f496ebb6e..674bd39e3 100644 --- a/src/main/window/attach-main-window-services.ts +++ b/src/main/window/attach-main-window-services.ts @@ -71,6 +71,8 @@ export function attachMainWindowServices( options?: { awaitLocalPtyStartup?: () => Promise onBeforeRendererReload?: (args: { webContentsId: number; ignoreCache: boolean }) => void + // Why: lets the PTY orphan sweep skip the one crash-recovery reload (#5787). + isRecoveryReloadInFlight?: (webContentsId: number) => boolean onBeforeUpdateQuit?: () => void | Promise } ): void { @@ -89,7 +91,8 @@ export function attachMainWindowServices( prepareClaudeAuth, store, { - awaitLocalPtyStartup: options?.awaitLocalPtyStartup + awaitLocalPtyStartup: options?.awaitLocalPtyStartup, + isRecoveryReloadInFlight: options?.isRecoveryReloadInFlight } ) // Why: the Manage Sessions settings panel (docs/daemon-staleness-ux.md §Phase 1) diff --git a/src/main/window/createMainWindow.test.ts b/src/main/window/createMainWindow.test.ts index 212e7c20f..453487a18 100644 --- a/src/main/window/createMainWindow.test.ts +++ b/src/main/window/createMainWindow.test.ts @@ -1673,6 +1673,54 @@ describe('createMainWindow', () => { }) }) + // Why (#5787): a hung-but-ALIVE renderer (never gone, never crashed) must NOT + // silently bypass the close guard — force-killing it that way is what destroyed + // other sessions. It must route through window:close-requested so the + // save/running-process confirmation runs. + it('requests confirmation for a hung-but-alive renderer instead of bypassing', () => { + const windowHandlers: Record void> = {} + const webContents = { + on: vi.fn((event, handler) => { + windowHandlers[event] = handler + }), + setZoomLevel: vi.fn(), + setBackgroundThrottling: vi.fn(), + invalidate: vi.fn(), + setWindowOpenHandler: vi.fn(), + send: vi.fn(), + isCrashed: vi.fn(() => false) + } + const browserWindowInstance = { + webContents, + on: vi.fn((event, handler) => { + windowHandlers[event] = handler + }), + isDestroyed: vi.fn(() => false), + isMaximized: vi.fn(() => true), + isFullScreen: vi.fn(() => false), + getSize: vi.fn(() => [1200, 800]), + setSize: vi.fn(), + maximize: vi.fn(), + show: vi.fn(), + loadFile: vi.fn(), + loadURL: vi.fn() + } + browserWindowMock.mockImplementation(function () { + return browserWindowInstance + }) + + createMainWindow(null) + + // No render-process-gone and isCrashed() === false: the renderer is alive. + const preventDefault = vi.fn() + windowHandlers.close({ preventDefault } as never) + + expect(preventDefault).toHaveBeenCalledTimes(1) + expect(webContents.send).toHaveBeenCalledWith('window:close-requested', { + isQuitting: false + }) + }) + it('ignores traffic light sync IPC on non-macOS', () => { const windowHandlers: Record void> = {} const webContents = { diff --git a/src/main/window/createMainWindow.ts b/src/main/window/createMainWindow.ts index 93f81569e..0d6a89be6 100644 --- a/src/main/window/createMainWindow.ts +++ b/src/main/window/createMainWindow.ts @@ -47,6 +47,7 @@ import { import { getMainE2EConfig } from '../e2e-config' import { buildEditableContextMenuTemplate } from './editable-context-menu' import { clearTrustedUIRendererWebContentsId, setTrustedUIRendererWebContentsId } from '../ipc/ui' +import { resolveWindowCloseAction } from './window-close-decision' function forceRepaint(window: BrowserWindow): void { if (window.isDestroyed()) { @@ -169,6 +170,11 @@ type CreateMainWindowOptions = { title?: string getKeybindings?: () => KeybindingOverrides | undefined onBeforeReload?: (options: { ignoreCache: boolean; webContentsId: number }) => void + /** Why: the in-place renderer-recovery reload re-fires did-finish-load, whose + * local-PTY orphan sweep would kill live sessions across the single window + * before session restore re-attaches them (#5787). This callback lets the host + * mark that one reload so the sweep can be skipped for it. */ + onBeforeRecoveryReload?: (webContentsId: number) => void } export function loadMainWindow(mainWindow: BrowserWindow): void { @@ -711,6 +717,9 @@ export function createMainWindow( // Why: a transient Network Service / renderer loss can leave Chromium // showing a blank shell. Reload the app document once so the user gets // back to a usable window instead of needing a full relaunch. + // Why: mark this one in-place reload so the did-finish-load orphan sweep + // spares live local PTYs until session restore re-attaches them (#5787). + opts?.onBeforeRecoveryReload?.(mainWindow.webContents.id) loadMainWindow(mainWindow) }, 250) } @@ -1084,8 +1093,23 @@ export function createMainWindow( return } const isRendererCrashed = mainWindow.webContents.isCrashed?.() ?? false - if (windowCloseConfirmed) { - windowCloseConfirmed = false + // Why: a hung-but-ALIVE renderer (neither gone nor crashed) must still hit + // the renderer's save/running-process confirmation; only a genuinely gone or + // crashed renderer — which cannot answer window:close-requested — may bypass + // it. Routing this through the pure decision locks that invariant (#5787). + const closeAction = resolveWindowCloseAction({ + windowCloseConfirmed, + rendererProcessGone, + isRendererCrashed + }) + if (closeAction !== 'request-confirmation') { + // allow-confirmed: the renderer already replied and re-entered close(). + // bypass-gone: after a native renderer crash the renderer cannot answer + // window:close-requested, so let Cmd+Q / OS close complete instead of + // trapping the user in a blank, unquittable window. + if (closeAction === 'allow-confirmed') { + windowCloseConfirmed = false + } // Why: past this point Electron/OS may emit resize/move/unmaximize as // the window is destroyed. Freeze bounds persistence so those // teardown events can't clobber the user's saved window size — which @@ -1098,17 +1122,6 @@ export function createMainWindow( } return } - if (rendererProcessGone || isRendererCrashed) { - // Why: after a native renderer crash the renderer cannot answer - // window:close-requested. Let Cmd+Q / OS close complete instead of - // trapping the user in a blank, unquittable window. - windowClosing = true - if (boundsTimer) { - clearTimeout(boundsTimer) - boundsTimer = null - } - return - } e.preventDefault() // Why: the renderer owns the close decision (dirty-file save dialogs, // running-process confirmation). The subscription lives at the always- diff --git a/src/main/window/window-close-decision.test.ts b/src/main/window/window-close-decision.test.ts new file mode 100644 index 000000000..6ade5a407 --- /dev/null +++ b/src/main/window/window-close-decision.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from 'vitest' +import { resolveWindowCloseAction } from './window-close-decision' + +describe('resolveWindowCloseAction', () => { + it('allows a renderer-confirmed close', () => { + expect( + resolveWindowCloseAction({ + windowCloseConfirmed: true, + rendererProcessGone: false, + isRendererCrashed: false + }) + ).toBe('allow-confirmed') + }) + + it('bypasses confirmation only when the renderer is truly gone or crashed', () => { + expect( + resolveWindowCloseAction({ + windowCloseConfirmed: false, + rendererProcessGone: true, + isRendererCrashed: false + }) + ).toBe('bypass-gone') + expect( + resolveWindowCloseAction({ + windowCloseConfirmed: false, + rendererProcessGone: false, + isRendererCrashed: true + }) + ).toBe('bypass-gone') + }) + + // Why (#5787): the data-loss cascade — a HUNG but alive renderer (force-killed + // via the OS "not responding" dialog) must still route through the renderer's + // save/running-process confirmation. It is neither gone nor crashed at the + // moment the user decides to close, so it MUST request confirmation, not bypass. + it('requests confirmation for an alive renderer (no gone/crashed flag)', () => { + expect( + resolveWindowCloseAction({ + windowCloseConfirmed: false, + rendererProcessGone: false, + isRendererCrashed: false + }) + ).toBe('request-confirmation') + }) + + it('prefers the confirmed-close path even if a stale gone/crashed flag is set', () => { + // Why: a renderer that already confirmed close (windowCloseConfirmed) is + // proceeding through the normal teardown; never re-route it. + expect( + resolveWindowCloseAction({ + windowCloseConfirmed: true, + rendererProcessGone: true, + isRendererCrashed: true + }) + ).toBe('allow-confirmed') + }) +}) diff --git a/src/main/window/window-close-decision.ts b/src/main/window/window-close-decision.ts new file mode 100644 index 000000000..b4ca32c52 --- /dev/null +++ b/src/main/window/window-close-decision.ts @@ -0,0 +1,32 @@ +export type WindowCloseAction = 'allow-confirmed' | 'bypass-gone' | 'request-confirmation' + +export type WindowCloseState = { + /** The renderer already replied to window:close-requested and called close(). */ + windowCloseConfirmed: boolean + /** webContents emitted render-process-gone (the process is truly gone). */ + rendererProcessGone: boolean + /** Electron reports the webContents as crashed (isCrashed()). */ + isRendererCrashed: boolean +} + +/** + * Decides how a native 'close' event should be handled. + * + * Why: a force-killed HUNG renderer must not be treated like a true crash. The + * renderer-owned confirmation (dirty-file save, running-process, multi-session + * guard) is only safe to bypass when the renderer is genuinely gone/crashed and + * therefore cannot answer — bypassing it for a merely-unresponsive renderer is + * what silently destroyed other sessions in #5787. An unresponsive-but-alive + * renderer (rendererProcessGone=false, isRendererCrashed=false) still resolves + * to 'request-confirmation' so the save guard runs. A genuinely gone renderer + * still bypasses so the window stays closable (#5144/#5314). + */ +export function resolveWindowCloseAction(state: WindowCloseState): WindowCloseAction { + if (state.windowCloseConfirmed) { + return 'allow-confirmed' + } + if (state.rendererProcessGone || state.isRendererCrashed) { + return 'bypass-gone' + } + return 'request-confirmation' +}