mirror of
https://github.com/stablyai/orca.git
synced 2026-10-18 00:03:20 +00:00
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.
This commit is contained in:
@@ -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<string, (...args: any[]) => 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<string, (...args: any[]) => void> = {}
|
||||
const webContents = {
|
||||
|
||||
@@ -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<typeof setTimeout> | 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)
|
||||
|
||||
@@ -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')
|
||||
})
|
||||
})
|
||||
@@ -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'
|
||||
}
|
||||
Reference in New Issue
Block a user