From b918c4e05748ba1ab778a06d78832be09641d71a Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Sat, 27 Jun 2026 22:42:45 -0700 Subject: [PATCH] Prevent session-loss cascade when a hung renderer is force-closed (#5787) On Windows, a Claude Code TUI streaming heavy output can hang (not crash) the renderer. The window close handler bypassed the renderer-owned save/running-process confirmation whenever the renderer was gone or crashed. A hung renderer force-killed via the OS "not responding" dialog transitions to gone, so the close bypassed the guard and silently destroyed other sessions in unrelated projects. Two changes: - Add webContents 'unresponsive'/'responsive' handlers. A hung-but-alive renderer now gets a graceful recovery reload after a grace period (PTYs live in main and survive the reload), so the user no longer needs the destructive OS force-close. 'responsive' cancels a pending recovery. - Extract resolveWindowCloseAction (pure, tested): a renderer that is merely unresponsive (not gone, not crashed) still routes through window:close-requested. Only a genuinely gone/crashed renderer bypasses, preserving closability (#5144/#5314). DO NOT MERGE. --- src/main/window/createMainWindow.test.ts | 115 ++++++++++++++++++ src/main/window/createMainWindow.ts | 90 ++++++++++++-- src/main/window/window-close-decision.test.ts | 57 +++++++++ src/main/window/window-close-decision.ts | 32 +++++ 4 files changed, 281 insertions(+), 13 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/window/createMainWindow.test.ts b/src/main/window/createMainWindow.test.ts index 4b4693ed534..79b5a932709 100644 --- a/src/main/window/createMainWindow.test.ts +++ b/src/main/window/createMainWindow.test.ts @@ -2551,6 +2551,121 @@ describe('createMainWindow', () => { consoleError.mockRestore() }) + // Why (#5787): a renderer hung on heavy output is alive but never emits a + // crash 'reason', so it gets no recovery path — the user's only escape was the + // destructive OS force-close. Reload it gracefully so live sessions (PTYs in + // main) survive instead of cascading closed. + it('reloads the app document after a hung renderer stays unresponsive', () => { + vi.useFakeTimers() + + const consoleWarn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const { browserWindowInstance, windowHandlers } = createRendererRecoveryWindowHarness() + + createMainWindow(null) + expect(browserWindowInstance.loadFile).toHaveBeenCalledTimes(1) + + windowHandlers.unresponsive?.() + vi.advanceTimersByTime(8_000) + + expect(browserWindowInstance.loadFile).toHaveBeenCalledTimes(2) + expect(browserWindowInstance.loadURL).not.toHaveBeenCalled() + + consoleWarn.mockRestore() + }) + + it('cancels hung-renderer recovery when the renderer becomes responsive again', () => { + vi.useFakeTimers() + + const consoleWarn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const { browserWindowInstance, windowHandlers } = createRendererRecoveryWindowHarness() + + createMainWindow(null) + + windowHandlers.unresponsive?.() + windowHandlers.responsive?.() + vi.advanceTimersByTime(8_000) + + // Only the initial load — the recovery reload was cancelled. + expect(browserWindowInstance.loadFile).toHaveBeenCalledTimes(1) + + consoleWarn.mockRestore() + }) + + it('does not schedule hung-renderer recovery while quitting', () => { + vi.useFakeTimers() + + const consoleWarn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const { browserWindowInstance, windowHandlers } = createRendererRecoveryWindowHarness() + + createMainWindow(null, { getIsQuitting: () => true }) + + windowHandlers.unresponsive?.() + vi.advanceTimersByTime(8_000) + + expect(browserWindowInstance.loadFile).toHaveBeenCalledTimes(1) + + consoleWarn.mockRestore() + }) + + // Why (#5787): the data-loss cascade. A renderer that went unresponsive but is + // still alive (not gone, not crashed) MUST still route a close through the + // renderer's save/running-process confirmation — bypassing it is what lost the + // other project's sessions when the user force-closed the frozen window. + it('still requests close confirmation for a hung-but-alive renderer', () => { + vi.useFakeTimers() + + const consoleWarn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const windowHandlers: Record void> = {} + const webContents = { + id: 77, + on: vi.fn((event, handler) => { + windowHandlers[event] = handler + }), + setZoomLevel: vi.fn(), + setBackgroundThrottling: vi.fn(), + invalidate: vi.fn(), + setWindowOpenHandler: vi.fn(), + send: vi.fn(), + // Why: a hung renderer is NOT crashed — isCrashed() must report false so + // the close path cannot mistake the hang for an unrecoverable crash. + 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, { getIsQuitting: () => false }) + + // Renderer hangs, then the user closes the frozen window BEFORE the grace + // recovery fires (and before any crash/process-gone event). + windowHandlers.unresponsive?.() + const preventDefault = vi.fn() + windowHandlers.close({ preventDefault } as never) + + // The close must be vetoed and the renderer asked to confirm — NOT bypassed. + expect(preventDefault).toHaveBeenCalledTimes(1) + expect(webContents.send).toHaveBeenCalledWith('window:close-requested', { + isQuitting: false + }) + + consoleWarn.mockRestore() + }) + it('ignores duplicate ready-to-show events after startup maximize has already run', () => { const windowHandlers: Record void> = {} const webContents = { diff --git a/src/main/window/createMainWindow.ts b/src/main/window/createMainWindow.ts index 863392bf155..58e46f27c3e 100644 --- a/src/main/window/createMainWindow.ts +++ b/src/main/window/createMainWindow.ts @@ -42,6 +42,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()) { @@ -101,6 +102,11 @@ const TRAFFIC_LIGHT_RADIUS = 6 const TRAFFIC_LIGHT_X = 16 const MIN_WIDTH = 600 const MIN_HEIGHT = 400 +// Why (#5787): grace period before a hung-but-alive renderer is reloaded. Long +// enough that a transient heavy-output flood can clear (and emit 'responsive') +// without losing renderer state, short enough to rescue the user before they +// reach for the destructive OS "not responding" force-close. +const UNRESPONSIVE_RECOVERY_DELAY_MS = 8_000 function syncTrafficLightPosition(win: BrowserWindow, zoomFactor: number): void { if (process.platform !== 'darwin' || win.isDestroyed()) { @@ -618,6 +624,37 @@ export function createMainWindow( rendererRecoveryTimer = null } } + // Why (#5787): a hung-but-alive renderer needs a longer grace window than a + // crash recovery — a transient output flood often resolves on its own, and + // reloading mid-flood would needlessly discard renderer state. Wait before + // forcing a reload so 'responsive' can cancel it first. + let unresponsiveRecoveryTimer: ReturnType | null = null + const clearUnresponsiveRecoveryTimer = (): void => { + if (unresponsiveRecoveryTimer) { + clearTimeout(unresponsiveRecoveryTimer) + unresponsiveRecoveryTimer = null + } + } + const scheduleUnresponsiveRecovery = (): void => { + if (unresponsiveRecoveryTimer) { + return + } + unresponsiveRecoveryTimer = setTimeout(() => { + unresponsiveRecoveryTimer = null + // Why: re-check at fire time — 'responsive' clears the timer, but a close/ + // quit/process-gone can still race in during the grace window. + if ( + rendererProcessGone || + windowClosing || + opts?.getIsQuitting?.() || + mainWindow.isDestroyed() + ) { + return + } + console.warn('[window] Renderer still unresponsive; reloading app document to recover') + loadMainWindow(mainWindow) + }, UNRESPONSIVE_RECOVERY_DELAY_MS) + } const scheduleRendererRecovery = (details: Electron.RenderProcessGoneDetails): void => { if ( rendererRecoveryTimer || @@ -648,6 +685,7 @@ export function createMainWindow( } mainWindow.webContents.on('render-process-gone', (_event, details) => { rendererProcessGone = true + clearUnresponsiveRecoveryTimer() resetMarkdownEditorFocus() resetTerminalInputFocus() resetFloatingTerminalInputFocus() @@ -665,6 +703,28 @@ export function createMainWindow( } scheduleRendererRecovery(details) }) + // Why (#5787): a renderer hung on a heavy-output flood is still alive — it + // gets no crash 'reason', so scheduleRendererRecovery never fires for it. With + // no graceful path, the user's only escape was the OS "not responding" dialog, + // whose force-kill then bypassed the multi-session save guard and cascaded + // other sessions closed. Detect the hang and reload the app document once + // after a grace period so the user gets a usable window WITHOUT force-closing. + // 'responsive' cancels the pending recovery if the renderer recovers on its own. + mainWindow.webContents.on('unresponsive', () => { + if ( + rendererProcessGone || + windowClosing || + opts?.getIsQuitting?.() || + mainWindow.isDestroyed() + ) { + return + } + console.warn('[window] Renderer unresponsive; scheduling graceful recovery reload') + scheduleUnresponsiveRecovery() + }) + mainWindow.webContents.on('responsive', () => { + clearUnresponsiveRecoveryTimer() + }) mainWindow.webContents.on('destroyed', () => { resetMarkdownEditorFocus() resetTerminalInputFocus() @@ -682,6 +742,9 @@ export function createMainWindow( mainWindow.webContents.on('did-finish-load', () => { rendererProcessGone = false clearRendererRecoveryTimer() + // Why (#5787): a fresh load means the renderer is healthy again; drop any + // pending hung-renderer recovery so it can't reload a now-good document. + clearUnresponsiveRecoveryTimer() }) const doubleTapDetector = new ModifierDoubleTapDetector() @@ -1000,24 +1063,24 @@ export function createMainWindow( return } const isRendererCrashed = mainWindow.webContents.isCrashed?.() ?? false - if (windowCloseConfirmed) { + // Why: a force-killed HUNG renderer must keep the save/running-process guard + // (#5787). Only a genuinely gone/crashed renderer — which cannot answer + // window:close-requested — may bypass it. The unresponsive flag alone never + // bypasses; it routes through 'request-confirmation' below. + const closeAction = resolveWindowCloseAction({ + windowCloseConfirmed, + rendererProcessGone, + isRendererCrashed + }) + if (closeAction === 'allow-confirmed' || closeAction === 'bypass-gone') { 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 // would otherwise make the post-update relaunch come up at minWidth × - // minHeight (issue surfaced in v1.3.26-rc2). - windowClosing = true - if (boundsTimer) { - clearTimeout(boundsTimer) - boundsTimer = null - } - 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. + // minHeight (issue surfaced in v1.3.26-rc2). For a gone/crashed renderer + // this also lets Cmd+Q / OS close complete instead of trapping the user + // in a blank, unquittable window. windowClosing = true if (boundsTimer) { clearTimeout(boundsTimer) @@ -1125,6 +1188,7 @@ export function createMainWindow( floatingTerminalInputFocused = false shortcutRecorderFocused = false clearRendererRecoveryTimer() + clearUnresponsiveRecoveryTimer() ipcMain.removeListener(trafficLightChannel, onSyncTrafficLights) ipcMain.removeListener(minimizeChannel, onMinimize) ipcMain.removeListener(maximizeChannel, onMaximize) 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 00000000000..6ade5a40759 --- /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 00000000000..b4ca32c5289 --- /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' +}