fix(settings): number close requests and abort installs only on error

Two review-round hardenings for the close guard's edge interleavings:

- Close requests now carry a per-interception id and Stay's cancellation
  cites the one its dialog answered. A cancellation that lost the race
  against a fresh Cmd+Q is ignored as stale, so the newer quit intent
  survives and the follow-up dialog's Save quits instead of silently
  downgrading to a window close.
- The install coordinator's push-based abort now fires only on an Error
  status: a benign push — like the 'checking' from an update check
  clicked while the updater quit winds up — no longer resurrects the
  beforeunload handler mid-install and strands the installation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
4grayandClaude Fable 5 committed 2026-08-09 15:06:13 +02:00
1 parent 27d566a960
commit 524cf578b8
8 files changed
+121 -29

No files matched your search

@@ -425,9 +425,13 @@ const electronApi: ElectronBridgeApi = {
setWindowCloseGuard: (active: boolean) =>
ipcRenderer.invoke(WINDOW_SET_CLOSE_GUARD, active),
confirmWindowClose: () => ipcRenderer.invoke(WINDOW_CONFIRM_CLOSE),
cancelWindowClose: () => ipcRenderer.invoke(WINDOW_CANCEL_CLOSE),
onWindowCloseRequested: (callback: () => void) => {
const handler = () => callback();
cancelWindowClose: (requestId?: number) =>
ipcRenderer.invoke(WINDOW_CANCEL_CLOSE, requestId),
onWindowCloseRequested: (callback: (requestId: number) => void) => {
const handler = (
_event: Electron.IpcRendererEvent,
requestId: number
) => callback(requestId);
ipcRenderer.on(WINDOW_CLOSE_REQUESTED, handler);
return () => ipcRenderer.off(WINDOW_CLOSE_REQUESTED, handler);
},
@@ -145,7 +145,8 @@ describe('WindowCloseGuard', () => {
expect(win.fireClose()).toBe(true);
expect(win.webContents.send).toHaveBeenCalledWith(
WINDOW_CLOSE_REQUESTED
WINDOW_CLOSE_REQUESTED,
1
);
expect(win.close).not.toHaveBeenCalled();
});
@@ -191,6 +192,37 @@ describe('WindowCloseGuard', () => {
expect(app.quit).not.toHaveBeenCalled();
});
it('ignores a cancellation citing an older interception', () => {
// Stay's cancel raced a fresh Cmd+Q: the late cancellation must not
// wipe the newer quit intent, or Save on the follow-up dialog would
// downgrade the quit to a plain window close.
const { app, guard, win } = createArmedGuard();
win.fireClose(); // request 1
app.fireBeforeQuit();
win.fireClose(); // request 2, quit
guard.cancelClose(1);
guard.confirmClose();
expect(app.quit).toHaveBeenCalledTimes(1);
expect(win.close).not.toHaveBeenCalled();
});
it('honors a cancellation citing the current interception', () => {
const { app, guard, win } = createArmedGuard();
app.fireBeforeQuit();
win.fireClose(); // request 1, quit
guard.cancelClose(1);
win.fireClose(); // request 2
guard.confirmClose();
expect(win.close).toHaveBeenCalledTimes(1);
expect(app.quit).not.toHaveBeenCalled();
});
it('keeps a pending quit when close is clicked again before the user decides', () => {
const { app, guard, win } = createArmedGuard();
@@ -37,7 +37,7 @@ export interface CloseGuardWindow {
on(event: string, listener: (...args: unknown[]) => void): unknown;
webContents: {
isDestroyed(): boolean;
send(channel: string): void;
send(channel: string, requestId?: number): void;
on(event: string, listener: (...args: unknown[]) => void): unknown;
};
}
@@ -47,6 +47,8 @@ export class WindowCloseGuard {
private bypassClose = false;
private quitInProgress = false;
private pendingIntent: CloseIntent | null = null;
/** Increments per interception; cancellations must cite the current one. */
private requestSeq = 0;
private window: CloseGuardWindow | null = null;
constructor(private readonly electronApp: CloseGuardApp) {}
@@ -160,8 +162,17 @@ export class WindowCloseGuard {
* Renderer verdict: the user stays. Clears the pending intent so a later
* close attempt starts fresh — without this, choosing Stay on a quit and
* clicking the window's close button minutes later would quit the app.
*
* A cancellation citing an older request than the current interception
* is stale and ignored: a quit intercepted while the cancel was in
* flight must keep its intent, or Save on the follow-up dialog would
* downgrade the user's quit to a plain window close.
*/
cancelClose(): void {
cancelClose(requestId?: number): void {
if (requestId !== undefined && requestId !== this.requestSeq) {
return;
}
this.pendingIntent = null;
}
@@ -198,7 +209,8 @@ export class WindowCloseGuard {
} else {
this.pendingIntent = this.pendingIntent ?? 'close';
}
win.webContents.send(WINDOW_CLOSE_REQUESTED);
this.requestSeq += 1;
win.webContents.send(WINDOW_CLOSE_REQUESTED, this.requestSeq);
}
}
@@ -227,8 +239,10 @@ export function bootstrapWindowCloseGuard(
ipcMain.handle(WINDOW_CONFIRM_CLOSE, () => {
guard.confirmClose();
});
ipcMain.handle(WINDOW_CANCEL_CLOSE, () => {
guard.cancelClose();
ipcMain.handle(WINDOW_CANCEL_CLOSE, (_event, requestId?: number) => {
guard.cancelClose(
typeof requestId === 'number' ? requestId : undefined
);
});
return guard;
@@ -129,6 +129,27 @@ describe('AppUpdateInstallService', () => {
expect(guard.resumeAfterAbortedAppQuit).toHaveBeenCalledTimes(1);
});
it('ignores benign pushes while the install quit is pending', async () => {
// A 'checking' push (update check clicked while the quit winds up)
// proves nothing about the quit; resuming the beforeunload handler
// on it would cancel the updater's window close and strand the
// install.
electronStub.installAppUpdate.mockResolvedValue({
...BASE_STATUS,
status: ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Downloaded,
});
const service = createService();
service.registerUnloadGuard(guard);
await service.installAppUpdate();
pushStatus({
...BASE_STATUS,
status: ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Checking,
});
expect(guard.resumeAfterAbortedAppQuit).not.toHaveBeenCalled();
});
it('survives an error push that beats the install reply', async () => {
// IPC ordering between the invoke reply and status pushes is not
// guaranteed; a stale 'downloaded' reply must not re-suspend the
@@ -34,12 +34,16 @@ export class AppUpdateInstallService {
constructor() {
// App-lifetime subscription on purpose: the failure push can arrive
// after the UI that requested the install is gone.
// after the UI that requested the install is gone. Only an Error
// push aborts — that is how electron-updater reports an install
// failure. Benign pushes (e.g. a 'checking' from an update-check
// clicked while the quit is still winding up) prove nothing about
// the quit and must not resurrect the beforeunload handler mid-
// install.
window.electron?.onAppUpdateStatusChange?.((status) => {
if (
this.installQuitPending &&
status.status !==
ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Downloaded
status.status === ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Error
) {
this.abortInstallQuit();
}
@@ -19,7 +19,7 @@ describe('SettingsUnloadGuardService', () => {
let host: SettingsUnloadGuardHost & { confirmClose: jest.Mock };
const originalElectron = window.electron;
let closeRequestCallback: (() => void) | null;
let closeRequestCallback: ((requestId: number) => void) | null;
let unsubscribeCloseRequests: jest.Mock;
let electronStub: {
setWindowCloseGuard: jest.Mock;
@@ -55,10 +55,12 @@ describe('SettingsUnloadGuardService', () => {
status: 'idle',
supportedSelfUpdate: true,
}),
onWindowCloseRequested: jest.fn((callback: () => void) => {
closeRequestCallback = callback;
return unsubscribeCloseRequests;
}),
onWindowCloseRequested: jest.fn(
(callback: (requestId: number) => void) => {
closeRequestCallback = callback;
return unsubscribeCloseRequests;
}
),
};
});
@@ -210,7 +212,7 @@ describe('SettingsUnloadGuardService', () => {
activateInElectron();
form.markAsDirty();
closeRequestCallback?.();
closeRequestCallback?.(1);
await flushAsyncWork();
expect(host.confirmClose).toHaveBeenCalledTimes(1);
@@ -222,7 +224,7 @@ describe('SettingsUnloadGuardService', () => {
activateInElectron();
form.markAsDirty();
closeRequestCallback?.();
closeRequestCallback?.(1);
await flushAsyncWork();
expect(electronStub.confirmWindowClose).not.toHaveBeenCalled();
@@ -251,7 +253,7 @@ describe('SettingsUnloadGuardService', () => {
expect(host.confirmClose).toHaveBeenCalledTimes(1);
// ...then the user closes the window while it is on screen.
closeRequestCallback?.();
closeRequestCallback?.(1);
resolveConfirm?.(true);
await flushAsyncWork();
@@ -273,7 +275,7 @@ describe('SettingsUnloadGuardService', () => {
dispatchBeforeUnload();
await flushAsyncWork();
closeRequestCallback?.();
closeRequestCallback?.(1);
resolveConfirm?.(false);
await flushAsyncWork();
@@ -300,18 +302,21 @@ describe('SettingsUnloadGuardService', () => {
activateInElectron();
form.markAsDirty();
closeRequestCallback?.();
closeRequestCallback?.(1);
await flushAsyncWork();
expect(host.confirmClose).toHaveBeenCalledTimes(1);
// Second close while the cancel acknowledgment is pending.
closeRequestCallback?.();
closeRequestCallback?.(2);
await flushAsyncWork();
expect(host.confirmClose).toHaveBeenCalledTimes(1);
resolveCancel?.();
await flushAsyncWork();
// The cancellation cited the request its dialog answered, so a
// newer interception's intent survives it in the main process.
expect(electronStub.cancelWindowClose).toHaveBeenCalledWith(1);
expect(host.confirmClose).toHaveBeenCalledTimes(2);
expect(electronStub.confirmWindowClose).toHaveBeenCalledTimes(1);
});
@@ -327,8 +332,8 @@ describe('SettingsUnloadGuardService', () => {
activateInElectron();
form.markAsDirty();
closeRequestCallback?.();
closeRequestCallback?.();
closeRequestCallback?.(1);
closeRequestCallback?.(1);
await flushAsyncWork();
expect(host.confirmClose).toHaveBeenCalledTimes(1);
@@ -56,6 +56,11 @@ export class SettingsUnloadGuardService implements OnDestroy {
private cancelling = false;
/** A close requested during that window, re-asked once the cancel lands. */
private queuedCloseRequest = false;
/**
* Id of the latest intercepted request, cited in cancellations so a
* cancel arriving after a newer interception cannot wipe its intent.
*/
private latestCloseRequestId: number | undefined;
/** Last guard state mirrored to the main process. */
private guardArmed = false;
/** True while an updater-driven app quit must pass unchallenged. */
@@ -71,7 +76,8 @@ export class SettingsUnloadGuardService implements OnDestroy {
window.addEventListener('beforeunload', this.beforeUnloadHandler);
this.unsubscribeCloseRequests =
window.electron?.onWindowCloseRequested?.(() => {
window.electron?.onWindowCloseRequested?.((requestId) => {
this.latestCloseRequestId = requestId;
this.zone.run(() => void this.handleCloseRequest('close'));
}) ?? null;
@@ -208,7 +214,9 @@ export class SettingsUnloadGuardService implements OnDestroy {
this.cancelling = true;
try {
await window.electron?.cancelWindowClose?.();
await window.electron?.cancelWindowClose?.(
this.latestCloseRequestId
);
} finally {
this.cancelling = false;
}
@@ -632,9 +632,13 @@ export interface ElectronBridgeApi {
/**
* Abandons a close the guard intercepted (the user stays), so the
* remembered close-vs-quit intent cannot leak into a later attempt.
* Carries the id of the request being abandoned: a cancellation that
* arrives after a NEWER interception must not wipe that newer intent.
*/
cancelWindowClose: () => Promise<void>;
onWindowCloseRequested: (callback: () => void) => () => void;
cancelWindowClose: (requestId?: number) => Promise<void>;
onWindowCloseRequested: (
callback: (requestId: number) => void
) => () => void;
fetchPlaylistByUrl: (
url: string,
title?: string,