From bded1fb2c0e0620be91643c2b7d6b6cc8b92b4ff Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 23 Jul 2026 18:03:43 -0700 Subject: [PATCH] fix(app): bound the wake/quit paths implicated in the phone-session-ended freeze (#9447) (#9853) * fix(app): bound the wake/quit paths implicated in the phone-session-ended freeze (#9447) - relay-transport: waitForClose now times out (5s) so a half-open post-sleep socket can't wedge runtimeRpc.stop() - will-quit: race teardown against a 20s deadline so app.quit() always runs (Force Quit was the only escape when any teardown member never settled) - terminal-fit-restore: local restoreTerminalFit invoke gets the same 15s bound as the remote path so the held-fit modal buttons can't pin disabled * fix(app): close wake recovery timeout gaps * fix(relay): drop late frames after forced teardown * fix(relay): fence detached socket callbacks * fix(app): close timeout resource gaps * fix(relay): detach retired mobile transports * fix(types): exclude absent stat overloads * fix(runtime): expire wedged terminal restore dedupe * fix(runtime): keep restore retries on one reclaim * chore(skills): refresh bundled skill manifests * fix(window): fence quit acknowledgements by request * fix(relay): bound revoked device socket cleanup --- src/main/index.ts | 15 +- src/main/ipc/runtime.test.ts | 79 +++++ src/main/ipc/runtime.ts | 43 ++- src/main/quit-teardown-deadline.test.ts | 50 ++++ src/main/quit-teardown-deadline.ts | 35 +++ .../runtime/relay/relay-control-origin.ts | 15 +- .../relay/relay-session-broker.test.ts | 8 +- .../runtime/rpc/mobile-socket-wiring.test.ts | 9 +- src/main/runtime/rpc/relay-transport.test.ts | 283 ++++++++++++++++++ src/main/runtime/rpc/relay-transport.ts | 128 ++++++-- src/main/window/createMainWindow.test.ts | 143 ++++++++- src/main/window/createMainWindow.ts | 49 ++- src/main/window/window-close-decision.ts | 6 +- src/preload/index.ts | 10 +- .../terminal-fit-restore.test.ts | 73 +++++ .../terminal-pane/terminal-fit-restore.ts | 51 +++- src/shared/terminal-fit-restore-deadline.ts | 1 + 17 files changed, 938 insertions(+), 60 deletions(-) create mode 100644 src/main/quit-teardown-deadline.test.ts create mode 100644 src/main/quit-teardown-deadline.ts create mode 100644 src/shared/terminal-fit-restore-deadline.ts diff --git a/src/main/index.ts b/src/main/index.ts index d66fb5358..4b8339cdb 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -205,6 +205,7 @@ import { createHeadlessAutomationOutputSnapshotBuffer } from './automations/head import { buildHeadlessAutomationWorktreeCreateArgs } from './automations/headless-workspace-create' import { AgentAwakeService } from './agent-awake-service' import { registerSystemResumeBroadcast } from './system-resume-broadcast' +import { settleTeardownWithinDeadline } from './quit-teardown-deadline' import { recordCoalescedCrashBreadcrumb, recordCrashBreadcrumb @@ -2531,7 +2532,19 @@ app.on('will-quit', (e) => { // Why: telemetry flush folds in before app.quit() (bounded 2s); catch defensively so a flush failure can't cancel the quit chain. // Why: normal quits keep the detached daemon for warm reattach, but a dead dev parent leaves the temp/dev profile ownerless. const daemonTeardown = isDevParentShutdownRequested() ? shutdownDaemon() : disconnectDaemon() - Promise.allSettled([daemonTeardown, rpcStopAndClear, watcherShutdown, emulatorShutdown]) + // Why: a wedged transport (half-open post-sleep socket) can leave one + // member unsettled forever and block app.quit() until Force Quit (#9447). + settleTeardownWithinDeadline([ + { name: 'daemon', promise: daemonTeardown }, + { name: 'runtime-rpc', promise: rpcStopAndClear }, + { name: 'watchers', promise: watcherShutdown }, + { name: 'emulator', promise: emulatorShutdown } + ]) + .then((pendingTeardowns) => { + if (pendingTeardowns.length > 0) { + console.warn('[shutdown] Quit teardown deadline reached', { pendingTeardowns }) + } + }) .then(() => shutdownTelemetry()) .then(() => shutdownObservability()) .catch(() => { diff --git a/src/main/ipc/runtime.test.ts b/src/main/ipc/runtime.test.ts index f951091c9..0a884aedf 100644 --- a/src/main/ipc/runtime.test.ts +++ b/src/main/ipc/runtime.test.ts @@ -17,6 +17,7 @@ vi.mock('electron', () => ({ })) import { registerRuntimeHandlers } from './runtime' +import { TERMINAL_FIT_RESTORE_DEADLINE_MS } from '../../shared/terminal-fit-restore-deadline' describe('registerRuntimeHandlers', () => { beforeEach(() => { @@ -99,4 +100,82 @@ describe('registerRuntimeHandlers', () => { _meta: { runtimeId: 'runtime-1' } }) }) + + it('deduplicates retries while a terminal fit restore is still pending', async () => { + const finishRestoreByPtyId = new Map void>() + const reclaimTerminalForDesktop = vi.fn( + (ptyId: string) => + new Promise((resolve) => { + finishRestoreByPtyId.set(ptyId, resolve) + }) + ) + const runtime = { + syncWindowGraph: vi.fn(), + getStatus: vi.fn(), + reclaimTerminalForDesktop + } + registerRuntimeHandlers(runtime as never) + const restoreRegistration = handleMock.mock.calls.find( + ([channel]) => channel === 'runtime:restoreTerminalFit' + ) + expect(restoreRegistration).toBeTruthy() + const handler = restoreRegistration![1] + + const first = handler({ sender: {} }, { ptyId: 'pty-1' }) + const retry = handler({ sender: {} }, { ptyId: 'pty-1' }) + const otherTerminal = handler({ sender: {} }, { ptyId: 'pty-2' }) + + expect(reclaimTerminalForDesktop).toHaveBeenCalledTimes(2) + expect(reclaimTerminalForDesktop).toHaveBeenNthCalledWith(1, 'pty-1') + expect(reclaimTerminalForDesktop).toHaveBeenNthCalledWith(2, 'pty-2') + finishRestoreByPtyId.get('pty-1')?.(true) + finishRestoreByPtyId.get('pty-2')?.(true) + await expect(otherTerminal).resolves.toEqual({ restored: true }) + await expect(first).resolves.toEqual({ restored: true }) + await expect(retry).resolves.toEqual({ restored: true }) + expect(reclaimTerminalForDesktop).toHaveBeenCalledTimes(2) + + const afterSettlement = handler({ sender: {} }, { ptyId: 'pty-1' }) + expect(reclaimTerminalForDesktop).toHaveBeenCalledTimes(3) + finishRestoreByPtyId.get('pty-1')?.(false) + await expect(afterSettlement).resolves.toEqual({ restored: false }) + }) + + it('bounds retries without accumulating reclaim waiters for one PTY', async () => { + vi.useFakeTimers() + try { + let finishRestore!: (restored: boolean) => void + const reclaimTerminalForDesktop = vi.fn( + () => + new Promise((resolve) => { + finishRestore = resolve + }) + ) + registerRuntimeHandlers({ + syncWindowGraph: vi.fn(), + getStatus: vi.fn(), + reclaimTerminalForDesktop + } as never) + const handler = handleMock.mock.calls.find( + ([channel]) => channel === 'runtime:restoreTerminalFit' + )![1] + + const first = handler({ sender: {} }, { ptyId: 'pty-wedged' }) + await vi.advanceTimersByTimeAsync(TERMINAL_FIT_RESTORE_DEADLINE_MS) + await expect(first).resolves.toEqual({ restored: false }) + + const retry = handler({ sender: {} }, { ptyId: 'pty-wedged' }) + expect(reclaimTerminalForDesktop).toHaveBeenCalledTimes(1) + finishRestore(true) + await expect(retry).resolves.toEqual({ restored: true }) + + const afterSettlement = handler({ sender: {} }, { ptyId: 'pty-wedged' }) + expect(reclaimTerminalForDesktop).toHaveBeenCalledTimes(2) + finishRestore(false) + await expect(afterSettlement).resolves.toEqual({ restored: false }) + expect(vi.getTimerCount()).toBe(0) + } finally { + vi.useRealTimers() + } + }) }) diff --git a/src/main/ipc/runtime.ts b/src/main/ipc/runtime.ts index 3b9a8719a..fbd3e7630 100644 --- a/src/main/ipc/runtime.ts +++ b/src/main/ipc/runtime.ts @@ -8,9 +8,20 @@ import type { RuntimeTerminalDriverState } from '../../shared/runtime-types' import type { RuntimeRpcResponse } from '../../shared/runtime-rpc-envelope' +import { TERMINAL_FIT_RESTORE_DEADLINE_MS } from '../../shared/terminal-fit-restore-deadline' import { RpcDispatcher } from '../runtime/rpc/dispatcher' +function boundTerminalFitRestore(pending: Promise): Promise { + let timer: ReturnType | undefined + const deadline = new Promise((resolve) => { + timer = setTimeout(() => resolve(false), TERMINAL_FIT_RESTORE_DEADLINE_MS) + timer.unref?.() + }) + return Promise.race([pending, deadline]).finally(() => clearTimeout(timer)) +} + export function registerRuntimeHandlers(runtime: OrcaRuntimeService): void { + const pendingTerminalFitRestores = new Map>() ipcMain.removeHandler('runtime:syncWindowGraph') ipcMain.removeHandler('runtime:getStatus') ipcMain.removeHandler('runtime:call') @@ -101,12 +112,34 @@ export function registerRuntimeHandlers(runtime: OrcaRuntimeService): void { // Electron try to structured-clone a Promise — "An object could not // be cloned" error — and the renderer's restoreTerminalFit() rejected // with no useful info. - try { - const reclaimed = await runtime.reclaimTerminalForDesktop(args.ptyId) - return { restored: reclaimed } - } catch { - return { restored: false } + // Why: keep one underlying reclaim per PTY even after callers time out; + // layout serialization means a retry cannot bypass the wedged operation. + let pending = pendingTerminalFitRestores.get(args.ptyId) + if (!pending) { + try { + let tracked!: Promise + const clearTrackedRestore = (): void => { + if (pendingTerminalFitRestores.get(args.ptyId) === tracked) { + pendingTerminalFitRestores.delete(args.ptyId) + } + } + tracked = runtime.reclaimTerminalForDesktop(args.ptyId).then( + (restored) => { + clearTrackedRestore() + return restored + }, + () => { + clearTrackedRestore() + return false + } + ) + pending = tracked + pendingTerminalFitRestores.set(args.ptyId, pending) + } catch { + return { restored: false } + } } + return { restored: await boundTerminalFitRestore(pending) } }) ipcMain.removeHandler('runtime:reclaimBrowserForDesktop') diff --git a/src/main/quit-teardown-deadline.test.ts b/src/main/quit-teardown-deadline.test.ts new file mode 100644 index 000000000..6303aaf86 --- /dev/null +++ b/src/main/quit-teardown-deadline.test.ts @@ -0,0 +1,50 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + settleTeardownWithinDeadline, + WILL_QUIT_TEARDOWN_DEADLINE_MS +} from './quit-teardown-deadline' + +describe('settleTeardownWithinDeadline', () => { + afterEach(() => { + vi.useRealTimers() + }) + + it('resolves as soon as all teardowns settle, including rejections', async () => { + vi.useFakeTimers() + let resolved = false + const pending = settleTeardownWithinDeadline([ + { name: 'daemon', promise: Promise.resolve() }, + { name: 'runtime-rpc', promise: Promise.reject(new Error('daemon disconnect failed')) } + ]).then(() => { + resolved = true + }) + await vi.advanceTimersByTimeAsync(0) + await pending + expect(resolved).toBe(true) + expect(vi.getTimerCount()).toBe(0) + }) + + it('reports the teardowns still pending at the deadline', async () => { + vi.useFakeTimers() + const pending = settleTeardownWithinDeadline([ + { name: 'daemon', promise: Promise.resolve() }, + { name: 'runtime-rpc', promise: new Promise(() => {}) } + ]) + await vi.advanceTimersByTimeAsync(WILL_QUIT_TEARDOWN_DEADLINE_MS - 1) + let resolved = false + void pending.then(() => { + resolved = true + }) + expect(resolved).toBe(false) + await vi.advanceTimersByTimeAsync(1) + await expect(pending).resolves.toEqual(['runtime-rpc']) + expect(vi.getTimerCount()).toBe(0) + }) + + // Why: pin the magnitude so the wedge escape hatch cannot be silently + // shrunk below checkpoint-write time or grown past user patience. + it('keeps the deadline within the checkpoint-safe window', () => { + expect(WILL_QUIT_TEARDOWN_DEADLINE_MS).toBeGreaterThanOrEqual(10_000) + expect(WILL_QUIT_TEARDOWN_DEADLINE_MS).toBeLessThanOrEqual(30_000) + }) +}) diff --git a/src/main/quit-teardown-deadline.ts b/src/main/quit-teardown-deadline.ts new file mode 100644 index 000000000..70328fa2f --- /dev/null +++ b/src/main/quit-teardown-deadline.ts @@ -0,0 +1,35 @@ +// Why: will-quit defers app.quit() until teardown settles. Teardown members +// are individually bounded, but a wedged transport (half-open post-sleep +// socket) can leave one unsettled forever and make Force Quit the only way +// out (#9447). Racing a deadline guarantees quit always completes. + +// Why: generous enough for daemon checkpoint writes on a slow disk; small +// enough that a wedged teardown never needs Force Quit. +export const WILL_QUIT_TEARDOWN_DEADLINE_MS = 20_000 + +export type NamedQuitTeardown = { + name: string + promise: Promise +} + +export async function settleTeardownWithinDeadline( + teardowns: readonly NamedQuitTeardown[], + deadlineMs: number = WILL_QUIT_TEARDOWN_DEADLINE_MS +): Promise { + const pendingNames = new Set(teardowns.map(({ name }) => name)) + const settled = Promise.allSettled( + teardowns.map(({ name, promise }) => + promise.finally(() => { + pendingNames.delete(name) + }) + ) + ).then(() => 'settled' as const) + let timer: ReturnType | undefined + const deadline = new Promise<'deadline'>((resolve) => { + timer = setTimeout(() => resolve('deadline'), deadlineMs) + timer.unref?.() + }) + const outcome = await Promise.race([settled, deadline]) + clearTimeout(timer) + return outcome === 'deadline' ? [...pendingNames] : [] +} diff --git a/src/main/runtime/relay/relay-control-origin.ts b/src/main/runtime/relay/relay-control-origin.ts index a7bfc9809..8035e32c1 100644 --- a/src/main/runtime/relay/relay-control-origin.ts +++ b/src/main/runtime/relay/relay-control-origin.ts @@ -31,7 +31,6 @@ export class RelayControlOrigin { readonly assignment: RelayAssignment readonly transport: CloudRelayTransport private readonly options: RelayControlOriginOptions - private readonly detachTransport: () => void private readonly controls = new Set() private readonly retiredControlTimers = new Map< RelayControlClient, @@ -43,6 +42,7 @@ export class RelayControlOrigin { private leaseExpiresAt = 0 private acceptingConnections = true private closed = false + private readonly detachMobileSocketTransport: () => void constructor(options: RelayControlOriginOptions) { this.options = options @@ -54,8 +54,9 @@ export class RelayControlOrigin { createSocket: options.createDataSocket, onConnectionClosed: (connectionId) => options.onConnectionReleased(connectionId, this) }) - this.detachTransport = options.mobileSocketWiring.attachTransport(this.transport, (ws) => - this.transport.metadataFor(ws) + this.detachMobileSocketTransport = options.mobileSocketWiring.attachTransport( + this.transport, + (ws) => this.transport.metadataFor(ws) ) } @@ -144,7 +145,6 @@ export class RelayControlOrigin { return } this.closed = true - this.detachTransport() for (const timer of this.retiredControlTimers.values()) { clearTimeout(timer) } @@ -154,7 +154,12 @@ export class RelayControlOrigin { } this.controls.clear() this.activeControl = null - await this.transport.stop() + try { + await this.transport.stop() + } finally { + // Why: detaching earlier would skip socket-close cleanup in MobileSocketWiring. + this.detachMobileSocketTransport() + } } closeNow(): void { diff --git a/src/main/runtime/relay/relay-session-broker.test.ts b/src/main/runtime/relay/relay-session-broker.test.ts index 32eb8c896..c25092e67 100644 --- a/src/main/runtime/relay/relay-session-broker.test.ts +++ b/src/main/runtime/relay/relay-session-broker.test.ts @@ -134,6 +134,8 @@ describe('RelaySessionBroker lifecycle ownership', () => { onStatus: (status) => statuses.push(status) }) await vi.waitFor(() => expect(fakes.controls).toHaveLength(1)) + const transportStopped = deferred() + fakes.transports[0]!.stop.mockReturnValue(transportStopped.promise) current = false controlAck.resolve({ type: 'host-hello-ack', @@ -148,7 +150,9 @@ describe('RelaySessionBroker lifecycle ownership', () => { await expect(connecting).rejects.toBeInstanceOf(StaleRelayBrokerError) expect(fakes.controls[0]!.closeNow).toHaveBeenCalledOnce() expect(fakes.transports[0]!.stop).toHaveBeenCalledOnce() - expect(detachTransport).toHaveBeenCalledOnce() + expect(detachTransport).not.toHaveBeenCalled() + transportStopped.resolve(undefined) + await vi.waitFor(() => expect(detachTransport).toHaveBeenCalledOnce()) expect(statuses).toEqual(['connecting']) }) @@ -318,7 +322,7 @@ function brokerOptions( publicKeyB64: Buffer.from(keypair.publicKey).toString('base64') }, appVersion: '1.0.0', - mobileSocketWiring: { attachTransport: vi.fn(() => vi.fn()) } as never, + mobileSocketWiring: { attachTransport: vi.fn(() => () => {}) } as never, isCurrent: () => true, refreshAccessToken: async () => null, onStatus: vi.fn(), diff --git a/src/main/runtime/rpc/mobile-socket-wiring.test.ts b/src/main/runtime/rpc/mobile-socket-wiring.test.ts index de4921915..f9d71c85b 100644 --- a/src/main/runtime/rpc/mobile-socket-wiring.test.ts +++ b/src/main/runtime/rpc/mobile-socket-wiring.test.ts @@ -84,12 +84,19 @@ describe('MobileSocketWiring', () => { onBinary: vi.fn(), onClose: vi.fn() }) - wiring.attachTransport(direct) + const detachDirect = wiring.attachTransport(direct) wiring.attachTransport(relay) expect(wiring.terminateDeviceConnections('valid-token')).toBe(3) expect(direct.terminateClientConnections).toHaveBeenCalledWith('valid-token') expect(relay.terminateClientConnections).toHaveBeenCalledWith('valid-token') + + detachDirect() + direct.terminateClientConnections.mockClear() + relay.terminateClientConnections.mockClear() + expect(wiring.terminateDeviceConnections('valid-token')).toBe(2) + expect(direct.terminateClientConnections).not.toHaveBeenCalled() + expect(relay.terminateClientConnections).toHaveBeenCalledWith('valid-token') }) it('releases detached transports from revocation fanout under origin churn', () => { diff --git a/src/main/runtime/rpc/relay-transport.test.ts b/src/main/runtime/rpc/relay-transport.test.ts index 432503b9a..8b7303ed8 100644 --- a/src/main/runtime/rpc/relay-transport.test.ts +++ b/src/main/runtime/rpc/relay-transport.test.ts @@ -93,6 +93,289 @@ describe('CloudRelayTransport', () => { await vi.waitFor(() => expect(onConnectionClosed).toHaveBeenCalledWith('conn/with spaces')) }) + it('stop() resolves after the close timeout when a socket never emits close', async () => { + vi.useFakeTimers() + try { + const listeners = new Map void)[]>() + const addListener = (event: string, fn: (...args: unknown[]) => void): void => { + const existing = listeners.get(event) ?? [] + existing.push(fn) + listeners.set(event, existing) + } + const removeListener = (event: string, fn: (...args: unknown[]) => void): void => { + listeners.set( + event, + (listeners.get(event) ?? []).filter((listener) => listener !== fn) + ) + } + const emit = (event: string, ...args: unknown[]): void => { + const eventListeners = listeners.get(event) ?? [] + if (event === 'error' && eventListeners.length === 0) { + throw args[0] + } + for (const fn of eventListeners) { + fn(...args) + } + } + // Why: models a half-open post-sleep relay socket — terminate() never + // produces a 'close' event, which previously hung stop() forever. + const fakeSocket = { + readyState: 1, + OPEN: 1, + CLOSED: 3, + on: addListener, + once: addListener, + off: removeListener, + send: vi.fn(), + terminate: () => {} + } + const onConnectionClosed = vi.fn() + const transport = new CloudRelayTransport({ + cellUrl: 'http://127.0.0.1:9', + relayHostId: 'AbCdEf0123_-xyZ9', + generation: 1, + createSocket: () => fakeSocket as unknown as WebSocketClient, + onConnectionClosed + }) + let reply: ((response: string) => void) | null = null + const onMessage = vi.fn( + (_message: string | Uint8Array, respond: (response: string) => void) => { + reply = respond + } + ) + transport.onMessage(onMessage) + const opening = transport.openConnection({ + connId: 'conn-1', + connTicket: 'ticket-1', + kind: 'resume', + relayDeviceId: 'device-1', + attachDeadlineMs: 1_000 + }) + emit('open') + await opening + vi.mocked(fakeSocket.send).mockClear() + emit('message', 'before-stop', false) + expect(onMessage).toHaveBeenCalledOnce() + + let stopped = false + const stopPromise = transport.stop().then(() => { + stopped = true + }) + emit('message', 'during-stop', false) + expect(onMessage).toHaveBeenCalledOnce() + await vi.advanceTimersByTimeAsync(4_999) + expect(stopped).toBe(false) + await vi.advanceTimersByTimeAsync(1) + await stopPromise + expect(stopped).toBe(true) + expect(onConnectionClosed).toHaveBeenCalledWith('conn-1') + expect(() => transport.metadataFor(fakeSocket as unknown as WebSocketClient)).toThrow( + 'unknown_relay_socket' + ) + const onLateMessage = vi.fn() + transport.onMessage((_message, _reply, socket) => { + transport.metadataFor(socket) + onLateMessage() + }) + // Why: timeout cleanup can precede the native socket's eventual close; + // late frames must not reach wiring after their metadata was released. + expect(() => emit('message', 'late-after-stop', false)).not.toThrow() + expect(onLateMessage).not.toHaveBeenCalled() + expect(reply).not.toBeNull() + await transport.start() + reply!('late-reply') + expect(fakeSocket.send).not.toHaveBeenCalled() + expect(() => emit('error', new Error('late socket failure'))).not.toThrow() + emit('close') + expect(listeners.get('error')).toHaveLength(0) + expect(listeners.get('close')).toHaveLength(0) + expect(vi.getTimerCount()).toBe(0) + expect(() => transport.setGeneration(2)).not.toThrow() + } finally { + vi.useRealTimers() + } + }) + + it('observes a synchronous close emitted by terminate without waiting for the deadline', async () => { + vi.useFakeTimers() + try { + const listeners = new Map void)[]>() + const addListener = (event: string, fn: (...args: unknown[]) => void): void => { + listeners.set(event, [...(listeners.get(event) ?? []), fn]) + } + const emit = (event: string): void => { + for (const fn of listeners.get(event) ?? []) { + fn() + } + } + const fakeSocket = { + readyState: 1, + OPEN: 1, + CLOSED: 3, + on: addListener, + once: addListener, + off: vi.fn(), + send: () => {}, + terminate: () => emit('close') + } + const transport = new CloudRelayTransport({ + cellUrl: 'http://127.0.0.1:9', + relayHostId: 'AbCdEf0123_-xyZ9', + generation: 1, + createSocket: () => fakeSocket as unknown as WebSocketClient + }) + const opening = transport.openConnection({ + connId: 'conn-sync-close', + connTicket: 'ticket-1', + kind: 'resume', + relayDeviceId: 'device-1', + attachDeadlineMs: 1_000 + }) + emit('open') + await opening + + await transport.stop() + + expect(vi.getTimerCount()).toBe(0) + } finally { + vi.useRealTimers() + } + }) + + it('releases an expired attach even when terminate never emits close', async () => { + vi.useFakeTimers() + try { + const listeners = new Map void)[]>() + const addListener = (event: string, fn: (...args: unknown[]) => void): void => { + listeners.set(event, [...(listeners.get(event) ?? []), fn]) + } + const removeListener = (event: string, fn: (...args: unknown[]) => void): void => { + listeners.set( + event, + (listeners.get(event) ?? []).filter((listener) => listener !== fn) + ) + } + const emit = (event: string, ...args: unknown[]): void => { + const eventListeners = listeners.get(event) ?? [] + if (event === 'error' && eventListeners.length === 0) { + throw args[0] + } + for (const fn of eventListeners) { + fn(...args) + } + } + const fakeSocket = { + readyState: 0, + OPEN: 1, + CLOSED: 3, + on: addListener, + once: addListener, + off: removeListener, + send: vi.fn(), + terminate: vi.fn() + } + const onConnectionClosed = vi.fn() + const transport = new CloudRelayTransport({ + cellUrl: 'http://127.0.0.1:9', + relayHostId: 'AbCdEf0123_-xyZ9', + generation: 1, + createSocket: () => fakeSocket as unknown as WebSocketClient, + onConnectionClosed + }) + const opening = transport.openConnection({ + connId: 'conn-attach-timeout', + connTicket: 'ticket-1', + kind: 'resume', + relayDeviceId: 'device-1', + attachDeadlineMs: 1_000 + }) + const rejectedOpening = expect(opening).rejects.toThrow('relay_host_data_attach_timeout') + + await vi.advanceTimersByTimeAsync(1_000) + + await rejectedOpening + expect(fakeSocket.terminate).toHaveBeenCalledOnce() + expect(onConnectionClosed).toHaveBeenCalledWith('conn-attach-timeout') + expect(() => transport.metadataFor(fakeSocket as unknown as WebSocketClient)).toThrow( + 'unknown_relay_socket' + ) + expect(() => transport.setGeneration(2)).not.toThrow() + expect(() => emit('message', 'late-after-attach-timeout', false)).not.toThrow() + expect(() => emit('error', new Error('late attach socket failure'))).not.toThrow() + emit('close') + expect(listeners.get('error')).toHaveLength(0) + expect(listeners.get('close')).toHaveLength(0) + } finally { + vi.useRealTimers() + } + }) + + it('bounds device termination cleanup and deduplicates its close waiter', async () => { + vi.useFakeTimers() + try { + const listeners = new Map void)[]>() + const addListener = (event: string, fn: (...args: unknown[]) => void): void => { + listeners.set(event, [...(listeners.get(event) ?? []), fn]) + } + const removeListener = (event: string, fn: (...args: unknown[]) => void): void => { + listeners.set( + event, + (listeners.get(event) ?? []).filter((listener) => listener !== fn) + ) + } + const emit = (event: string, ...args: unknown[]): void => { + for (const fn of listeners.get(event) ?? []) { + fn(...args) + } + } + const fakeSocket = { + readyState: 1, + OPEN: 1, + CLOSED: 3, + on: addListener, + once: addListener, + off: removeListener, + send: vi.fn(), + terminate: vi.fn() + } + const onConnectionClosed = vi.fn() + const transport = new CloudRelayTransport({ + cellUrl: 'http://127.0.0.1:9', + relayHostId: 'AbCdEf0123_-xyZ9', + generation: 1, + createSocket: () => fakeSocket as unknown as WebSocketClient, + onConnectionClosed + }) + transport.onMessage(() => {}) + const opening = transport.openConnection({ + connId: 'conn-device-termination', + connTicket: 'ticket-1', + kind: 'resume', + relayDeviceId: 'device-1', + attachDeadlineMs: 10_000 + }) + emit('open') + await opening + emit('message', 'attached', false) + transport.setClientId(fakeSocket as unknown as WebSocketClient, 'client-1') + + expect(transport.terminateClientConnections('client-1')).toBe(1) + expect(transport.terminateClientConnections('client-1')).toBe(1) + expect(fakeSocket.terminate).toHaveBeenCalledOnce() + expect(vi.getTimerCount()).toBe(1) + await vi.advanceTimersByTimeAsync(5_000) + + expect(onConnectionClosed).toHaveBeenCalledOnce() + expect(onConnectionClosed).toHaveBeenCalledWith('conn-device-termination') + expect(() => transport.metadataFor(fakeSocket as unknown as WebSocketClient)).toThrow( + 'unknown_relay_socket' + ) + expect(vi.getTimerCount()).toBe(0) + } finally { + vi.useRealTimers() + } + }) + it('rejects non-origin cell URLs before opening a socket', () => { expect( () => diff --git a/src/main/runtime/rpc/relay-transport.ts b/src/main/runtime/rpc/relay-transport.ts index 1eb9bd07a..6399218d7 100644 --- a/src/main/runtime/rpc/relay-transport.ts +++ b/src/main/runtime/rpc/relay-transport.ts @@ -1,9 +1,12 @@ -import WebSocket from 'ws' +import WebSocket, { type RawData } from 'ws' import { forEachWithConcurrency } from '../../../shared/map-with-concurrency' import type { RpcTransport } from './transport' import type { MobileSocketTransport, MobileSocketTransportMetadata } from './mobile-socket-wiring' const MAX_RELAY_MESSAGE_BYTES = 1024 * 1024 +// Why: terminate() normally emits 'close' within one tick; 5s covers slow +// teardown without letting a dead socket hold stop() (and app quit) hostage. +export const RELAY_SOCKET_CLOSE_TIMEOUT_MS = 5_000 const RELAY_SOCKET_CLOSE_WAIT_CONCURRENCY = 32 type RelayMessagePayload = string | Uint8Array @@ -48,6 +51,8 @@ export class CloudRelayTransport implements RpcTransport, MobileSocketTransport private readonly socketsByConnectionId = new Map() private readonly metadataBySocket = new Map() private readonly clientIds = new Map() + private readonly detachListenersBySocket = new Map void>() + private readonly closeWaitsBySocket = new Map>() private messageHandler: Parameters[0] | null = null private closeHandler: Parameters[0] | null = null private stopped = false @@ -104,7 +109,7 @@ export class CloudRelayTransport implements RpcTransport, MobileSocketTransport .filter(([, candidate]) => candidate === clientId) .map(([socket]) => socket) for (const socket of sockets) { - socket.terminate() + void this.terminateWithinCloseDeadline(socket) } return sockets.length } @@ -116,11 +121,8 @@ export class CloudRelayTransport implements RpcTransport, MobileSocketTransport async stop(): Promise { this.stopped = true const sockets = [...this.metadataBySocket.keys()] - for (const socket of sockets) { - socket.terminate() - } await forEachWithConcurrency(sockets, RELAY_SOCKET_CLOSE_WAIT_CONCURRENCY, (socket) => - this.waitForClose(socket) + this.terminateWithinCloseDeadline(socket) ) } @@ -149,6 +151,9 @@ export class CloudRelayTransport implements RpcTransport, MobileSocketTransport let finalized = false const deadline = setTimeout(() => { socket.terminate() + // Why: attach expiry makes the socket unusable; release it even if terminate never emits close. + finalize() + this.quarantineDetachedSocket(socket) if (!opened) { reject(new Error('relay_host_data_attach_timeout')) } @@ -159,16 +164,12 @@ export class CloudRelayTransport implements RpcTransport, MobileSocketTransport } finalized = true clearTimeout(deadline) - this.socketsByConnectionId.delete(connection.connId) - this.metadataBySocket.delete(socket) - const clientId = this.clientIds.get(socket) ?? null - this.clientIds.delete(socket) - this.onConnectionClosed?.(connection.connId) - const hasOtherConnections = - clientId !== null && [...this.clientIds.values()].includes(clientId) - this.closeHandler?.(clientId, socket, hasOtherConnections) + this.finalizeConnection(connection.connId, socket) } - socket.on('message', (raw, isBinary) => { + const onMessage = (raw: RawData, isBinary: boolean): void => { + if (this.stopped || finalized) { + return + } if (!attached) { attached = true clearTimeout(deadline) @@ -179,14 +180,14 @@ export class CloudRelayTransport implements RpcTransport, MobileSocketTransport this.messageHandler?.( message, (response) => { - if (socket.readyState === socket.OPEN) { + if (!this.stopped && !finalized && socket.readyState === socket.OPEN) { socket.send(response) } }, socket ) - }) - socket.once('open', () => { + } + const onOpen = (): void => { opened = true const networkSocket = ( socket as unknown as { _socket?: { setNoDelay(value: boolean): void } } @@ -201,21 +202,106 @@ export class CloudRelayTransport implements RpcTransport, MobileSocketTransport }) ) resolve() - }) - socket.once('error', (error) => { + } + const onError = (error: Error): void => { if (!opened) { finalize() reject(error) } + } + this.detachListenersBySocket.set(socket, () => { + finalized = true + socket.off('message', onMessage) + socket.off('open', onOpen) + socket.off('error', onError) + socket.off('close', finalize) }) + socket.on('message', onMessage) + socket.once('open', onOpen) + socket.once('error', onError) socket.once('close', finalize) }) } private waitForClose(socket: WebSocket): Promise { if (socket.readyState === socket.CLOSED) { + const connectionId = this.connectionIdForSocket(socket) + if (connectionId) { + this.finalizeConnection(connectionId, socket) + } return Promise.resolve() } - return new Promise((resolve) => socket.once('close', resolve)) + // Why: a half-open relay socket after system sleep can never emit 'close'; + // an unbounded wait here wedges stop() and blocks app quit (#9447). + return new Promise((resolve) => { + const onClose = (): void => { + clearTimeout(deadline) + resolve() + } + const deadline = setTimeout(() => { + socket.off('close', onClose) + const connectionId = this.connectionIdForSocket(socket) + if (connectionId) { + this.finalizeConnection(connectionId, socket) + } + this.quarantineDetachedSocket(socket) + resolve() + }, RELAY_SOCKET_CLOSE_TIMEOUT_MS) + socket.once('close', onClose) + if (socket.readyState === socket.CLOSED) { + onClose() + } + }) + } + + private terminateWithinCloseDeadline(socket: WebSocket): Promise { + const existing = this.closeWaitsBySocket.get(socket) + if (existing) { + return existing + } + const pending = this.waitForClose(socket) + this.closeWaitsBySocket.set(socket, pending) + void pending.then(() => { + if (this.closeWaitsBySocket.get(socket) === pending) { + this.closeWaitsBySocket.delete(socket) + } + }) + // Why: install the close waiter first because test doubles and native wrappers can close synchronously. + socket.terminate() + return pending + } + + private connectionIdForSocket(socket: WebSocket): string | undefined { + const metadata = this.metadataBySocket.get(socket) + return metadata?.transport === 'relay' ? metadata.basisConnId : undefined + } + + private quarantineDetachedSocket(socket: WebSocket): void { + if (socket.readyState === socket.CLOSED) { + return + } + // Why: forced cleanup can precede ws's terminal error/close event. + const swallowLateError = (): void => {} + const clearQuarantine = (): void => { + socket.off('error', swallowLateError) + socket.off('close', clearQuarantine) + } + socket.on('error', swallowLateError) + socket.once('close', clearQuarantine) + } + + private finalizeConnection(connectionId: string, socket: WebSocket): void { + if (this.socketsByConnectionId.get(connectionId) !== socket) { + return + } + this.socketsByConnectionId.delete(connectionId) + this.metadataBySocket.delete(socket) + this.detachListenersBySocket.get(socket)?.() + this.detachListenersBySocket.delete(socket) + const clientId = this.clientIds.get(socket) ?? null + this.clientIds.delete(socket) + this.onConnectionClosed?.(connectionId) + const hasOtherConnections = clientId !== null && [...this.clientIds.values()].includes(clientId) + this.closeHandler?.(clientId, socket, hasOtherConnections) } } diff --git a/src/main/window/createMainWindow.test.ts b/src/main/window/createMainWindow.test.ts index 5b84fd1f0..78e9089cd 100644 --- a/src/main/window/createMainWindow.test.ts +++ b/src/main/window/createMainWindow.test.ts @@ -60,7 +60,11 @@ vi.mock('../browser/browser-manager', () => ({ } })) -import { createMainWindow, loadMainWindow } from './createMainWindow' +import { + createMainWindow, + loadMainWindow, + WINDOW_QUIT_RENDERER_ACK_TIMEOUT_MS +} from './createMainWindow' import { ipcMain } from 'electron' import { shouldRecoverRendererAfterProcessGone } from '../crash-reporting/process-gone-classification' @@ -1480,7 +1484,10 @@ describe('createMainWindow', () => { const preventDefault = vi.fn() windowHandlers.close({ preventDefault } as never) expect(preventDefault).toHaveBeenCalledTimes(1) - expect(webContents.send).toHaveBeenCalledWith('window:close-requested', { isQuitting: true }) + expect(webContents.send).toHaveBeenCalledWith('window:close-requested', { + isQuitting: true, + requestId: expect.any(Number) + }) windowHandlers['will-prevent-unload']() expect(onQuitAborted).toHaveBeenCalledTimes(1) @@ -1533,9 +1540,10 @@ describe('createMainWindow', () => { windowHandlers.close({ preventDefault } as never) expect(preventDefault).not.toHaveBeenCalled() - expect(webContents.send).not.toHaveBeenCalledWith('window:close-requested', { - isQuitting: true - }) + expect(webContents.send).not.toHaveBeenCalledWith( + 'window:close-requested', + expect.objectContaining({ isQuitting: true }) + ) consoleError.mockRestore() }) @@ -1707,7 +1715,8 @@ describe('createMainWindow', () => { expect(preventDefault).toHaveBeenCalledTimes(1) expect(webContents.send).toHaveBeenCalledWith('window:close-requested', { - isQuitting: true + isQuitting: true, + requestId: expect.any(Number) }) consoleError.mockRestore() @@ -1751,9 +1760,10 @@ describe('createMainWindow', () => { windowHandlers.close({ preventDefault } as never) expect(preventDefault).not.toHaveBeenCalled() - expect(webContents.send).not.toHaveBeenCalledWith('window:close-requested', { - isQuitting: true - }) + expect(webContents.send).not.toHaveBeenCalledWith( + 'window:close-requested', + expect.objectContaining({ isQuitting: true }) + ) }) // Why (#5787): a hung-but-ALIVE renderer (never gone, never crashed) must NOT @@ -1800,10 +1810,114 @@ describe('createMainWindow', () => { expect(preventDefault).toHaveBeenCalledTimes(1) expect(webContents.send).toHaveBeenCalledWith('window:close-requested', { - isQuitting: false + isQuitting: false, + requestId: expect.any(Number) }) }) + it('destroys an already-unresponsive renderer after an app-wide quit deadline', async () => { + vi.useFakeTimers() + const windowHandlers: Record void> = {} + const webContents = { + id: 42, + 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 destroy = vi.fn() + browserWindowMock.mockImplementation(function () { + return { + 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(), + destroy, + loadFile: vi.fn(), + loadURL: vi.fn() + } + }) + createMainWindow(null, { getIsQuitting: () => true }) + + windowHandlers.close({ preventDefault: vi.fn() } as never) + await vi.advanceTimersByTimeAsync(WINDOW_QUIT_RENDERER_ACK_TIMEOUT_MS - 1) + expect(destroy).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(1) + + expect(destroy).toHaveBeenCalledOnce() + }) + + it('keeps the renderer-owned close flow after the quit request is acknowledged', async () => { + vi.useFakeTimers() + const windowHandlers: Record void> = {} + const ipcHandlers: Record void> = {} + vi.mocked(ipcMain.on).mockImplementation((channel, handler) => { + ipcHandlers[channel] = handler as (...args: any[]) => void + return ipcMain + }) + const webContents = { + id: 42, + 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 destroy = vi.fn() + browserWindowMock.mockImplementation(function () { + return { + 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(), + destroy, + loadFile: vi.fn(), + loadURL: vi.fn() + } + }) + createMainWindow(null, { getIsQuitting: () => true }) + + windowHandlers.close({ preventDefault: vi.fn() } as never) + windowHandlers.close({ preventDefault: vi.fn() } as never) + const closeRequests = vi + .mocked(webContents.send) + .mock.calls.filter(([channel]) => channel === 'window:close-requested') + .map(([, request]) => request as { requestId: number }) + expect(closeRequests).toHaveLength(2) + const [staleRequest, currentRequest] = closeRequests + ipcHandlers['window:close-request-received']?.({ sender: { id: 99 } }, currentRequest.requestId) + ipcHandlers['window:close-request-received']?.({ sender: { id: 42 } }, staleRequest.requestId) + await vi.advanceTimersByTimeAsync(WINDOW_QUIT_RENDERER_ACK_TIMEOUT_MS - 1) + expect(destroy).not.toHaveBeenCalled() + ipcHandlers['window:close-request-received']?.({ sender: { id: 42 } }, currentRequest.requestId) + await vi.advanceTimersByTimeAsync(1) + + expect(destroy).not.toHaveBeenCalled() + }) + it('ignores traffic light sync IPC on non-macOS', () => { const windowHandlers: Record void> = {} const webContents = { @@ -3240,7 +3354,8 @@ describe('createMainWindow', () => { expect(instance.hide).not.toHaveBeenCalled() expect(webContents.send).toHaveBeenCalledWith('window:close-requested', { - isQuitting: false + isQuitting: false, + requestId: expect.any(Number) }) }) @@ -3254,7 +3369,8 @@ describe('createMainWindow', () => { expect(instance.hide).not.toHaveBeenCalled() expect(webContents.send).toHaveBeenCalledWith('window:close-requested', { - isQuitting: true + isQuitting: true, + requestId: expect.any(Number) }) }) @@ -3300,7 +3416,8 @@ describe('createMainWindow', () => { expect(instance.hide).not.toHaveBeenCalled() expect(webContents.send).toHaveBeenCalledWith('window:close-requested', { - isQuitting: false + isQuitting: false, + requestId: expect.any(Number) }) }) diff --git a/src/main/window/createMainWindow.ts b/src/main/window/createMainWindow.ts index e35cd24d7..29f046203 100644 --- a/src/main/window/createMainWindow.ts +++ b/src/main/window/createMainWindow.ts @@ -51,6 +51,7 @@ import { installPrivilegedWindowNavigationPolicy } from './privileged-window-nav // Why: show/restore/resume can overlap before the size nudge resets; never capture the temporary width as the next baseline. const activeRepaintJiggles = new WeakSet() +export const WINDOW_QUIT_RENDERER_ACK_TIMEOUT_MS = 10_000 function forceRepaint(window: BrowserWindow): void { // Why: webContents can be destroyed a beat before the BrowserWindow during close, and this runs from timers/focus events in that gap. @@ -849,6 +850,41 @@ export function createMainWindow( // Intercept close so the renderer can confirm killing running-process terminals (replies window:confirm-close to proceed). let windowCloseConfirmed = false const confirmCloseChannel = 'window:confirm-close' + const closeRequestReceivedChannel = 'window:close-request-received' + let closeRequestSequence = 0 + let quitRendererAckRequestId: number | null = null + let quitRendererAckTimer: ReturnType | null = null + const clearQuitRendererAckTimer = (): void => { + quitRendererAckRequestId = null + if (quitRendererAckTimer) { + clearTimeout(quitRendererAckTimer) + quitRendererAckTimer = null + } + } + const armQuitRendererAckTimer = (requestId: number): void => { + quitRendererAckRequestId = requestId + if (quitRendererAckTimer) { + return + } + // Why: will-quit cannot run until the renderer-backed window closes; an + // already-frozen renderer otherwise makes Force Quit the only escape. + quitRendererAckTimer = setTimeout(() => { + quitRendererAckTimer = null + quitRendererAckRequestId = null + if (mainWindow.isDestroyed()) { + return + } + console.warn('[window] Renderer did not acknowledge quit; destroying unresponsive window') + freezeBoundsOnQuit() + mainWindow.destroy() + }, WINDOW_QUIT_RENDERER_ACK_TIMEOUT_MS) + quitRendererAckTimer.unref?.() + } + const onCloseRequestReceived = (event: Electron.IpcMainEvent, requestId: number): void => { + if (event.sender.id === rendererWebContentsId && requestId === quitRendererAckRequestId) { + clearQuitRendererAckTimer() + } + } // Windows minimize-to-tray: hide instead of close when enabled; returns true when it hid so callers skip their close path. const hideToTrayIfEnabled = (): boolean => { @@ -909,19 +945,27 @@ export function createMainWindow( return } e.preventDefault() + const isQuitting = opts?.getIsQuitting?.() ?? false + const requestId = ++closeRequestSequence + if (isQuitting) { + armQuitRendererAckTimer(requestId) + } // Why: renderer owns the close decision; the always-mounted App root subscription lets even pre-workspace states reply (#5144). mainWindow.webContents.send('window:close-requested', { - isQuitting: opts?.getIsQuitting?.() ?? false + isQuitting, + requestId }) }) mainWindow.webContents.on('will-prevent-unload', () => { // Why: a prevented beforeunload cancels the quit; release the bounds-persistence freeze so later resizing still saves. windowClosing = false + clearQuitRendererAckTimer() opts?.onQuitAborted?.() mainWindow.webContents.send('window:unload-prevented') }) const onConfirmClose = (): void => { + clearQuitRendererAckTimer() windowCloseConfirmed = true if (!mainWindow.isDestroyed()) { mainWindow.close() @@ -980,12 +1024,14 @@ export function createMainWindow( ipcMain.handle(isMaximizedChannel, onIsMaximized) ipcMain.on(confirmCloseChannel, onConfirmClose) + ipcMain.on(closeRequestReceivedChannel, onCloseRequestReceived) mainWindow.on('closed', () => { // Why: the dashboard pop-out is a companion of the main window — close it // alongside so it never orphans as a lone window after the app window is // gone (e.g. on macOS where the app stays alive after the window closes). closeDashboardPopout() clearInitialRevealFallbackTimer() + clearQuitRendererAckTimer() // Why: default-deny the Cmd+B carve-out after the window is gone so a stale-true flag can't leak into later state. markdownEditorFocused = false terminalInputFocused = false @@ -1000,6 +1046,7 @@ export function createMainWindow( ipcMain.removeListener(popupMenuChannel, onPopupMenu) ipcMain.removeHandler(isMaximizedChannel) ipcMain.removeListener(confirmCloseChannel, onConfirmClose) + ipcMain.removeListener(closeRequestReceivedChannel, onCloseRequestReceived) ipcMain.removeListener(markdownFocusChannel, onMarkdownEditorFocused) ipcMain.removeListener(terminalInputFocusChannel, onTerminalInputFocused) ipcMain.removeListener(floatingTerminalInputFocusChannel, onFloatingTerminalInputFocused) diff --git a/src/main/window/window-close-decision.ts b/src/main/window/window-close-decision.ts index b4ca32c52..f41e7efa9 100644 --- a/src/main/window/window-close-decision.ts +++ b/src/main/window/window-close-decision.ts @@ -18,8 +18,10 @@ export type WindowCloseState = { * 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). + * to 'request-confirmation' so the save guard runs. App-wide quit separately + * bounds failure to acknowledge that request; ordinary window close does not. + * A genuinely gone renderer still bypasses so the window stays closable + * (#5144/#5314). */ export function resolveWindowCloseAction(state: WindowCloseState): WindowCloseAction { if (state.windowCloseConfirmed) { diff --git a/src/preload/index.ts b/src/preload/index.ts index b6244cc4e..f0a4fef1c 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -3863,8 +3863,14 @@ const api = { /** Fired by main when the user tries to close the window; renderer confirms running * terminals then calls confirmWindowClose(). isQuitting (Cmd+Q / app.quit) skips that dialog. */ onWindowCloseRequested: (callback: (data: { isQuitting: boolean }) => void): (() => void) => { - const listener = (_event: Electron.IpcRendererEvent, data: { isQuitting: boolean }) => - callback(data ?? { isQuitting: false }) + const listener = ( + _event: Electron.IpcRendererEvent, + data: { isQuitting: boolean; requestId?: number } + ): void => { + // Why: main cannot reach will-quit while a frozen renderer owns the window close handshake. + ipcRenderer.send('window:close-request-received', data?.requestId) + callback({ isQuitting: data?.isQuitting ?? false }) + } ipcRenderer.on('window:close-requested', listener) return () => ipcRenderer.removeListener('window:close-requested', listener) }, diff --git a/src/renderer/src/components/terminal-pane/terminal-fit-restore.test.ts b/src/renderer/src/components/terminal-pane/terminal-fit-restore.test.ts index 07f030faa..3c9d94109 100644 --- a/src/renderer/src/components/terminal-pane/terminal-fit-restore.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-fit-restore.test.ts @@ -99,6 +99,62 @@ describe('terminal-fit-restore', () => { await expect(restoreTerminalFitToDesktop('pty-local', undefined)).resolves.toBe(false) }) + it('fails a local restore whose invoke never resolves instead of hanging', async () => { + vi.useFakeTimers() + try { + vi.mocked(getRemoteRuntimeTerminalHandle).mockReturnValue(null) + // Why: models a wedged runtime/daemon after system sleep (#9447) — the + // IPC invoke stays pending forever. + restoreTerminalFit.mockReturnValue(new Promise(() => {})) + + let settled: boolean | null = null + const pending = restoreTerminalFitToDesktop('pty-local', undefined).then((restored) => { + settled = restored + }) + await vi.advanceTimersByTimeAsync(14_999) + expect(settled).toBeNull() + await vi.advanceTimersByTimeAsync(1) + await pending + expect(settled).toBe(false) + } finally { + vi.useRealTimers() + } + }) + + it('gives restores started later only the remainder of the shared bulk deadline', async () => { + vi.useFakeTimers() + try { + vi.mocked(getRemoteRuntimeTerminalHandle).mockReturnValue(null) + restoreTerminalFit.mockImplementation( + (ptyId: string) => + new Promise((resolve) => { + if (ptyId === 'pty-0') { + setTimeout(() => resolve({ restored: false }), 10_000) + } + }) + ) + const ptyIds = Array.from({ length: 100 }, (_, index) => `pty-${index}`) + + const pending = restoreTerminalFitsToDesktop(ptyIds, undefined) + expect(restoreTerminalFit).toHaveBeenCalledTimes(8) + await vi.advanceTimersByTimeAsync(10_000) + expect(restoreTerminalFit).toHaveBeenCalledTimes(9) + await vi.advanceTimersByTimeAsync(4_999) + let settled = false + void pending.then(() => { + settled = true + }) + expect(settled).toBe(false) + await vi.advanceTimersByTimeAsync(1) + + await expect(pending).resolves.toBe(false) + expect(restoreTerminalFit).toHaveBeenCalledTimes(9) + expect(vi.getTimerCount()).toBe(0) + } finally { + vi.useRealTimers() + } + }) + it('treats failed remote RPC restore transport as not restored', async () => { vi.mocked(getRemoteRuntimeTerminalHandle).mockReturnValue('terminal-fail') vi.mocked(getRemoteRuntimePtyEnvironmentId).mockReturnValue('env-fail') @@ -106,4 +162,21 @@ describe('terminal-fit-restore', () => { await expect(restoreTerminalFitToDesktop('remote:pty-fail', undefined)).resolves.toBe(false) }) + + it('bounds a remote restore even when the RPC client does not enforce its timeout', async () => { + vi.useFakeTimers() + try { + vi.mocked(getRemoteRuntimeTerminalHandle).mockReturnValue('terminal-stuck') + vi.mocked(getRemoteRuntimePtyEnvironmentId).mockReturnValue('env-stuck') + vi.mocked(callRuntimeRpc).mockReturnValue(new Promise(() => {})) + + const pending = restoreTerminalFitToDesktop('remote:pty-stuck', undefined) + await vi.advanceTimersByTimeAsync(15_000) + + await expect(pending).resolves.toBe(false) + expect(vi.getTimerCount()).toBe(0) + } finally { + vi.useRealTimers() + } + }) }) diff --git a/src/renderer/src/components/terminal-pane/terminal-fit-restore.ts b/src/renderer/src/components/terminal-pane/terminal-fit-restore.ts index 54738f590..6503d13f5 100644 --- a/src/renderer/src/components/terminal-pane/terminal-fit-restore.ts +++ b/src/renderer/src/components/terminal-pane/terminal-fit-restore.ts @@ -1,5 +1,6 @@ import type { GlobalSettings } from '../../../../shared/types' import { mapWithConcurrency } from '../../../../shared/map-with-concurrency' +import { TERMINAL_FIT_RESTORE_DEADLINE_MS } from '../../../../shared/terminal-fit-restore-deadline' import { callRuntimeRpc } from '@/runtime/runtime-rpc-client' import { getRemoteRuntimePtyEnvironmentId, @@ -20,33 +21,69 @@ const restoreFailedResult = (): { restored: boolean } => { return { restored: false } } -export async function restoreTerminalFitToDesktop( +// Why: a wedged runtime/daemon after system sleep can leave the invoke pending +// forever, which pins the held-fit modal's buttons disabled (#9447). Fail the +// restore instead so the user can retry. +const withRestoreFitTimeout = async ( + pending: Promise<{ restored: boolean }>, + timeoutMs: number +): Promise<{ restored: boolean }> => { + let timer: ReturnType | undefined + const timedOut = new Promise<{ restored: boolean }>((resolve) => { + timer = setTimeout(() => resolve(restoreFailedResult()), timeoutMs) + }) + try { + return await Promise.race([pending, timedOut]) + } finally { + clearTimeout(timer) + } +} + +async function restoreTerminalFitToDesktopWithinDeadline( ptyId: string, - settings: TerminalFitRestoreSettings + settings: TerminalFitRestoreSettings, + deadlineAt: number ): Promise { + const timeoutMs = Math.max(0, deadlineAt - Date.now()) + if (timeoutMs === 0) { + return false + } const remoteHandle = getRemoteRuntimeTerminalHandle(ptyId) const environmentId = getRemoteRuntimePtyEnvironmentId(ptyId) ?? settings?.activeRuntimeEnvironmentId ?? null - const result = + const pending = remoteHandle && environmentId - ? await callRuntimeRpc<{ restored: boolean }>( + ? callRuntimeRpc<{ restored: boolean }>( { kind: 'environment', environmentId }, 'terminal.restoreFit', { terminal: remoteHandle }, - { timeoutMs: 15_000 } + { timeoutMs } ).catch(restoreFailedResult) - : await window.api.runtime.restoreTerminalFit(ptyId).catch(restoreFailedResult) + : window.api.runtime.restoreTerminalFit(ptyId).catch(restoreFailedResult) + const result = await withRestoreFitTimeout(pending, timeoutMs) return result.restored } +export function restoreTerminalFitToDesktop( + ptyId: string, + settings: TerminalFitRestoreSettings +): Promise { + return restoreTerminalFitToDesktopWithinDeadline( + ptyId, + settings, + Date.now() + TERMINAL_FIT_RESTORE_DEADLINE_MS + ) +} + export async function restoreTerminalFitsToDesktop( ptyIds: Iterable, settings: TerminalFitRestoreSettings ): Promise { const uniquePtyIds = [...new Set(ptyIds)] + const deadlineAt = Date.now() + TERMINAL_FIT_RESTORE_DEADLINE_MS const results = await mapWithConcurrency(uniquePtyIds, RESTORE_FIT_CONCURRENCY, (ptyId) => - restoreTerminalFitToDesktop(ptyId, settings) + restoreTerminalFitToDesktopWithinDeadline(ptyId, settings, deadlineAt) ) return results.some(Boolean) } diff --git a/src/shared/terminal-fit-restore-deadline.ts b/src/shared/terminal-fit-restore-deadline.ts new file mode 100644 index 000000000..8cbe78a3b --- /dev/null +++ b/src/shared/terminal-fit-restore-deadline.ts @@ -0,0 +1 @@ +export const TERMINAL_FIT_RESTORE_DEADLINE_MS = 15_000