Fix #5787: recovery reload no longer sweeps local PTYs; guard close for hung-but-alive renderer (#7290)

Co-authored-by: Orca <help@stably.ai>

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<void> = 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 })
     }

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()),

src/main/ipc/pty.ts

@@ -1247,6 +1247,9 @@ export function registerPtyHandlers(
   store?: Store,
   options?: {
     awaitLocalPtyStartup?: () => Promise<void>
+    // 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)

src/main/window/attach-main-window-services.ts

@@ -71,6 +71,8 @@ export function attachMainWindowServices(
   options?: {
     awaitLocalPtyStartup?: () => Promise<void>
     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>
   }
 ): 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)

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<string, (...args: any[]) => 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<string, (...args: any[]) => void> = {}
     const webContents = {

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-

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')
+  })
+})

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'
+}