From 769d519a61e9cb617a55ecda553a69f61f28376d Mon Sep 17 00:00:00 2001 From: 4gray Date: Sun, 9 Aug 2026 11:23:48 +0200 Subject: [PATCH] fix(settings): harden close guard against stale quit intent and updater installs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-round fixes for the unsaved-settings close guard: - A pending quit intent is never downgraded by a repeated close click while the dialog is open, and choosing Stay now explicitly cancels the intercepted intent in the main process (WINDOW:CANCEL_CLOSE), so a later close cannot replay a stale quit. Renderer navigation or a crash also drops the unanswered intent. (Greptile P1) - Irreversible teardown (MPV/VLC sessions, embedded MPV, DB worker) moves from 'before-quit' to 'will-quit': the guard can cancel a quit mid-flight, and 'before-quit' has already fired by then — the old placement left a cancelled quit with playback and database work destroyed. (Codex P1) - The self-updater's quitAndInstall() stands the guard down for exactly one close (allowNextClose): on macOS it closes windows before 'before-quit', so an armed guard would strand the requested install as a plain window close. (Codex P2) - The EPG source e2e now saves the staged form edit before closing the app; leaving it dirty made the new guard block the close, leaking the Electron process and timing out the Playwright worker teardown on CI. closeElectronApp additionally escalates to SIGKILL when SIGTERM is refused, so a future dirty-form slip cannot wedge the suite again. Co-Authored-By: Claude Fable 5 --- .../src/electron-test-fixtures.ts | 19 ++++++ apps/electron-backend-e2e/src/epg.e2e.ts | 5 ++ .../src/app/api/main.preload.ts | 2 + .../app/services/app-update.service.spec.ts | 31 +++++++++ .../src/app/services/app-update.service.ts | 8 +++ .../window-close-guard.service.spec.ts | 63 ++++++++++++++++++- .../services/window-close-guard.service.ts | 46 +++++++++++++- apps/electron-backend/src/main.ts | 18 ++++-- .../settings-unload-guard.service.spec.ts | 9 +++ .../settings/settings-unload-guard.service.ts | 6 ++ .../test-stubs/settings-test-harness.stub.ts | 1 + .../src/lib/electron-api.interface.ts | 5 ++ .../shared/interfaces/src/lib/ipc-commands.ts | 1 + 13 files changed, 205 insertions(+), 9 deletions(-) diff --git a/apps/electron-backend-e2e/src/electron-test-fixtures.ts b/apps/electron-backend-e2e/src/electron-test-fixtures.ts index 2af39ac3e..87e41df35 100644 --- a/apps/electron-backend-e2e/src/electron-test-fixtures.ts +++ b/apps/electron-backend-e2e/src/electron-test-fixtures.ts @@ -581,6 +581,25 @@ export async function closeElectronApp( closePromise.catch(() => undefined), electronAppKillWaitMs ); + + // SIGTERM asks Electron for a graceful quit, which the app can + // legitimately refuse — the unsaved-settings close guard cancels the + // quit while it waits for an answer. A process that survives here + // would outlive the test, hold its data dir, and time out the worker + // teardown, so escalate to SIGKILL. + if ( + childProcess.exitCode === null && + childProcess.signalCode === null + ) { + console.warn( + 'Electron app survived SIGTERM; escalating to SIGKILL' + ); + childProcess.kill('SIGKILL'); + await waitForPromiseWithTimeout( + closePromise.catch(() => undefined), + electronAppKillWaitMs + ); + } } catch (error) { console.warn('Failed to close Electron app cleanly:', error); } diff --git a/apps/electron-backend-e2e/src/epg.e2e.ts b/apps/electron-backend-e2e/src/epg.e2e.ts index 28c6f9a5c..4e5f55835 100644 --- a/apps/electron-backend-e2e/src/epg.e2e.ts +++ b/apps/electron-backend-e2e/src/epg.e2e.ts @@ -110,6 +110,11 @@ test.describe('Electron EPG', () => { await expect(app.mainWindow.locator('.epg-source-row')).toHaveCount( 0 ); + // Adding and removing the source row leaves the settings form + // dirty; save so the unsaved-changes close guard has nothing to + // ask about when the app closes below — an unanswered dialog + // would block the close and leak the Electron process. + await saveSettings(app.mainWindow); await app.mainWindow .getByRole('button', { name: 'Clear EPG data' }) diff --git a/apps/electron-backend/src/app/api/main.preload.ts b/apps/electron-backend/src/app/api/main.preload.ts index ea2db5c0e..b4602eba4 100644 --- a/apps/electron-backend/src/app/api/main.preload.ts +++ b/apps/electron-backend/src/app/api/main.preload.ts @@ -82,6 +82,7 @@ const WINDOW_GET_STATE = 'WINDOW:GET_STATE'; const WINDOW_STATE_CHANGED = 'WINDOW:STATE_CHANGED'; const WINDOW_SET_CLOSE_GUARD = 'WINDOW:SET_CLOSE_GUARD'; const WINDOW_CONFIRM_CLOSE = 'WINDOW:CONFIRM_CLOSE'; +const WINDOW_CANCEL_CLOSE = 'WINDOW:CANCEL_CLOSE'; const WINDOW_CLOSE_REQUESTED = 'WINDOW:CLOSE_REQUESTED'; const dbSaveContentProgressListeners = new Set< @@ -424,6 +425,7 @@ 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(); ipcRenderer.on(WINDOW_CLOSE_REQUESTED, handler); diff --git a/apps/electron-backend/src/app/services/app-update.service.spec.ts b/apps/electron-backend/src/app/services/app-update.service.spec.ts index 042b7bab4..5d6c7d7be 100644 --- a/apps/electron-backend/src/app/services/app-update.service.spec.ts +++ b/apps/electron-backend/src/app/services/app-update.service.spec.ts @@ -82,6 +82,7 @@ function createService( isPackaged?: boolean; platform?: NodeJS.Platform; env?: NodeJS.ProcessEnv; + prepareQuit?: () => void; } = {} ) { const updater = new FakeUpdater(); @@ -93,6 +94,7 @@ function createService( }, getMainWindow: () => win, platform: overrides.platform ?? 'darwin', + prepareQuit: overrides.prepareQuit, processEnv: overrides.env ?? {}, releaseFetcher: overrides.fetcher, updater, @@ -449,4 +451,33 @@ describe('AppUpdateService', () => { ); expect(updater.quitAndInstall).toHaveBeenCalledTimes(1); }); + + it('stands the close guard down before quitAndInstall', () => { + // quitAndInstall closes the windows before 'before-quit' (macOS), so + // an armed unsaved-settings guard would intercept that close and + // strand the requested install. prepareQuit must run first. + const calls: string[] = []; + const { service, updater } = createService({ + prepareQuit: () => calls.push('prepareQuit'), + }); + updater.quitAndInstall.mockImplementation(() => { + calls.push('quitAndInstall'); + }); + service.handleUpdateAvailable({ version: '0.23.0' }); + service.handleUpdateDownloaded({ version: '0.23.0' }); + + service.installUpdate(); + + expect(calls).toEqual(['prepareQuit', 'quitAndInstall']); + }); + + it('does not stand the close guard down when nothing is installable', () => { + const prepareQuit = jest.fn(); + const { service, updater } = createService({ prepareQuit }); + + service.installUpdate(); + + expect(prepareQuit).not.toHaveBeenCalled(); + expect(updater.quitAndInstall).not.toHaveBeenCalled(); + }); }); diff --git a/apps/electron-backend/src/app/services/app-update.service.ts b/apps/electron-backend/src/app/services/app-update.service.ts index b3c9a05d3..0aeb54c68 100644 --- a/apps/electron-backend/src/app/services/app-update.service.ts +++ b/apps/electron-backend/src/app/services/app-update.service.ts @@ -110,6 +110,13 @@ export interface AppUpdateServiceOptions { platform?: NodeJS.Platform; processEnv?: NodeJS.ProcessEnv; releaseFetcher?: ReleaseFetcher; + /** + * Runs right before `quitAndInstall()`. Lets the unsaved-settings close + * guard stand down for the updater-driven window close, which on macOS + * happens before `before-quit` and would otherwise be intercepted as a + * plain close — abandoning the requested install. + */ + prepareQuit?: () => void; } function isSelfUpdateSupported( @@ -401,6 +408,7 @@ export class AppUpdateService { this.status.status === ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Downloaded ) { + this.options.prepareQuit?.(); this.updater.quitAndInstall(); } diff --git a/apps/electron-backend/src/app/services/window-close-guard.service.spec.ts b/apps/electron-backend/src/app/services/window-close-guard.service.spec.ts index 8565c0199..792dd3d6f 100644 --- a/apps/electron-backend/src/app/services/window-close-guard.service.spec.ts +++ b/apps/electron-backend/src/app/services/window-close-guard.service.spec.ts @@ -18,6 +18,7 @@ jest.mock('electron', () => ({ import { ipcMain } from 'electron'; import { + WINDOW_CANCEL_CLOSE, WINDOW_CLOSE_REQUESTED, WINDOW_CONFIRM_CLOSE, WINDOW_SET_CLOSE_GUARD, @@ -174,12 +175,13 @@ describe('WindowCloseGuard', () => { expect(win.close).not.toHaveBeenCalled(); }); - it('forgets an aborted quit before the next close attempt', () => { + it('forgets a cancelled quit before the next close attempt', () => { const { app, guard, win } = createArmedGuard(); - // Quit intercepted, user stays: the quit is aborted. + // Quit intercepted, user stays: the renderer cancels the request. app.fireBeforeQuit(); win.fireClose(); + guard.cancelClose(); // A later plain window close must not resurrect the quit. win.fireClose(); @@ -189,6 +191,45 @@ describe('WindowCloseGuard', () => { expect(app.quit).not.toHaveBeenCalled(); }); + it('keeps a pending quit when close is clicked again before the user decides', () => { + const { app, guard, win } = createArmedGuard(); + + // Cmd+Q intercepted; the dialog is open. An impatient second click + // on the window's close button must not downgrade the quit — saving + // would then close only the window and leave the app running. + app.fireBeforeQuit(); + win.fireClose(); + win.fireClose(); + + guard.confirmClose(); + + expect(app.quit).toHaveBeenCalledTimes(1); + expect(win.close).not.toHaveBeenCalled(); + }); + + it('escalates a pending close to a quit when the user quits mid-decision', () => { + const { app, guard, win } = createArmedGuard(); + + win.fireClose(); + app.fireBeforeQuit(); + win.fireClose(); + + guard.confirmClose(); + + expect(app.quit).toHaveBeenCalledTimes(1); + expect(win.close).not.toHaveBeenCalled(); + }); + + it('lets exactly one close through after allowNextClose', () => { + const { guard, win } = createArmedGuard(); + + guard.allowNextClose(); + + expect(win.fireClose()).toBe(false); + // The bypass is consumed: the guard is back for the next attempt. + expect(win.fireClose()).toBe(true); + }); + it('confirms as a plain close when nothing was intercepted', () => { const { app, guard, win } = createArmedGuard(); @@ -206,6 +247,22 @@ describe('WindowCloseGuard', () => { expect(win.fireClose()).toBe(false); }); + it('drops an unanswered quit intent when the renderer navigates away', () => { + const { app, guard, win } = createArmedGuard(); + + app.fireBeforeQuit(); + win.fireClose(); + win.fireWebContentsEvent('did-navigate'); + + // A fresh interception cycle must not inherit the stale quit. + guard.setGuardActive(true); + win.fireClose(); + guard.confirmClose(); + + expect(win.close).toHaveBeenCalledTimes(1); + expect(app.quit).not.toHaveBeenCalled(); + }); + it('disarms when the render process is gone', () => { const { win } = createArmedGuard(); @@ -285,6 +342,8 @@ describe('bootstrapWindowCloseGuard', () => { getIpcHandler(WINDOW_SET_CLOSE_GUARD)({}, true); expect(win.fireClose()).toBe(true); + getIpcHandler(WINDOW_CANCEL_CLOSE)({}); + getIpcHandler(WINDOW_CONFIRM_CLOSE)({}); expect(win.close).toHaveBeenCalledTimes(1); }); diff --git a/apps/electron-backend/src/app/services/window-close-guard.service.ts b/apps/electron-backend/src/app/services/window-close-guard.service.ts index ab012e89c..f9ecb311d 100644 --- a/apps/electron-backend/src/app/services/window-close-guard.service.ts +++ b/apps/electron-backend/src/app/services/window-close-guard.service.ts @@ -16,6 +16,7 @@ import { app, ipcMain } from 'electron'; import { + WINDOW_CANCEL_CLOSE, WINDOW_CLOSE_REQUESTED, WINDOW_CONFIRM_CLOSE, WINDOW_SET_CLOSE_GUARD, @@ -86,12 +87,15 @@ export class WindowCloseGuard { // A full navigation (reload included) discards the renderer state the // guard was protecting; a gone renderer can no longer answer the // close request. Either way an armed guard would make the window - // unclosable. + // unclosable, and a pending intent from an unanswered request would + // leak into the next interception cycle. win.webContents.on('did-navigate', () => { this.guardActive = false; + this.pendingIntent = null; }); win.webContents.on('render-process-gone', () => { this.guardActive = false; + this.pendingIntent = null; }); } @@ -99,6 +103,17 @@ export class WindowCloseGuard { this.guardActive = active; } + /** + * Lets exactly one close through even while the guard is armed. Used for + * the self-updater's `quitAndInstall()`: on macOS it closes the windows + * *before* `before-quit` fires, so an intercepted close would downgrade + * the install into a plain window close and abandon the update. The user + * explicitly asked to install, so that close must win. + */ + allowNextClose(): void { + this.bypassClose = true; + } + /** * Renderer verdict: safe to leave (settings saved or discarded). Replays * the intercepted intent with the guard bypassed for exactly one close. @@ -121,6 +136,15 @@ 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. + */ + cancelClose(): void { + this.pendingIntent = null; + } + private handleClose( event: { preventDefault(): void }, win: CloseGuardWindow @@ -131,7 +155,12 @@ export class WindowCloseGuard { const wasQuit = this.quitInProgress; this.quitInProgress = false; - if (!this.guardActive || this.bypassClose) { + if (this.bypassClose) { + this.bypassClose = false; + return; + } + + if (!this.guardActive) { return; } @@ -140,7 +169,15 @@ export class WindowCloseGuard { } event.preventDefault(); - this.pendingIntent = wasQuit ? 'quit' : 'close'; + // A quit is never downgraded while a decision is pending: the + // renderer drops repeated requests while its dialog is open, so a + // second close click must not overwrite an intercepted Cmd+Q — + // saving would then close only the window and leave the app running. + if (wasQuit) { + this.pendingIntent = 'quit'; + } else { + this.pendingIntent = this.pendingIntent ?? 'close'; + } win.webContents.send(WINDOW_CLOSE_REQUESTED); } } @@ -170,6 +207,9 @@ export function bootstrapWindowCloseGuard( ipcMain.handle(WINDOW_CONFIRM_CLOSE, () => { guard.confirmClose(); }); + ipcMain.handle(WINDOW_CANCEL_CLOSE, () => { + guard.cancelClose(); + }); return guard; } diff --git a/apps/electron-backend/src/main.ts b/apps/electron-backend/src/main.ts index 8abd74480..c9299f7ca 100644 --- a/apps/electron-backend/src/main.ts +++ b/apps/electron-backend/src/main.ts @@ -135,20 +135,24 @@ export default class Main { trace('startup', 'bootstrap-events:start'); } + const windowCloseGuard = bootstrapWindowCloseGuard((listener) => + App.onMainWindowCreated(listener) + ); const appUpdateService = new AppUpdateService({ app, appVersion: environment.version, getMainWindow: () => App.mainWindow, updater: () => autoUpdater, + // quitAndInstall() closes the windows before 'before-quit' fires + // (macOS), so without this an armed close guard would intercept + // the install's window close and strand the update. + prepareQuit: () => windowCloseGuard.allowNextClose(), }); AppUpdateEvents.bootstrapAppUpdateEvents(appUpdateService); registerStaticHeaderShims(); ElectronEvents.bootstrapElectronEvents(); WindowEvents.bootstrapWindowEvents(); - bootstrapWindowCloseGuard((listener) => - App.onMainWindowCreated(listener) - ); EmbeddedMpvEvents.bootstrapEmbeddedMpvEvents(); PlaylistEvents.bootstrapPlaylistEvents(); PlaylistOpenEvents.bootstrapPlaylistOpenEvents(); @@ -264,7 +268,13 @@ runEmbeddedMpvRuntimeDiagnosticOrContinue(process.argv, () => { await Main.bootstrapAppEvents(); }); - app.on('before-quit', () => { + // 'will-quit', not 'before-quit': the unsaved-settings close guard can + // cancel a quit mid-flight by preventing the window close, and + // 'before-quit' has already fired by then — tearing sessions and the DB + // worker down there would leave a cancelled quit with the app open but + // playback and database work destroyed. 'will-quit' only fires once + // every window close was allowed through. + app.on('will-quit', () => { shutdownEmbeddedMpv(); shutdownMpvSession(); shutdownVlcSession(); diff --git a/apps/web/src/app/settings/settings-unload-guard.service.spec.ts b/apps/web/src/app/settings/settings-unload-guard.service.spec.ts index ae6f8abfc..3bb364738 100644 --- a/apps/web/src/app/settings/settings-unload-guard.service.spec.ts +++ b/apps/web/src/app/settings/settings-unload-guard.service.spec.ts @@ -23,6 +23,7 @@ describe('SettingsUnloadGuardService', () => { let electronStub: { setWindowCloseGuard: jest.Mock; confirmWindowClose: jest.Mock; + cancelWindowClose: jest.Mock; onWindowCloseRequested: jest.Mock; }; @@ -47,6 +48,7 @@ describe('SettingsUnloadGuardService', () => { electronStub = { setWindowCloseGuard: jest.fn().mockResolvedValue(undefined), confirmWindowClose: jest.fn().mockResolvedValue(undefined), + cancelWindowClose: jest.fn().mockResolvedValue(undefined), onWindowCloseRequested: jest.fn((callback: () => void) => { closeRequestCallback = callback; return unsubscribeCloseRequests; @@ -174,6 +176,10 @@ describe('SettingsUnloadGuardService', () => { await flushAsyncWork(); expect(electronStub.confirmWindowClose).not.toHaveBeenCalled(); + // Staying abandons the request in the main process too, so the + // remembered close-vs-quit intent cannot leak into a later + // attempt. + expect(electronStub.cancelWindowClose).toHaveBeenCalledTimes(1); }); it('ignores repeated close requests while a dialog is open', async () => { @@ -228,6 +234,9 @@ describe('SettingsUnloadGuardService', () => { await flushAsyncWork(); expect(reloadPage).not.toHaveBeenCalled(); + // A reload was never remembered by the main process, so there + // is nothing to cancel there. + expect(electronStub.cancelWindowClose).not.toHaveBeenCalled(); }); }); }); diff --git a/apps/web/src/app/settings/settings-unload-guard.service.ts b/apps/web/src/app/settings/settings-unload-guard.service.ts index e175a7edd..bf3ed9507 100644 --- a/apps/web/src/app/settings/settings-unload-guard.service.ts +++ b/apps/web/src/app/settings/settings-unload-guard.service.ts @@ -119,6 +119,12 @@ export class SettingsUnloadGuardService implements OnDestroy { try { if (!(await host.confirmClose())) { + if (intent === 'close') { + // Staying must clear the intent the main process + // remembered, or a later close attempt would replay a + // stale quit. + void window.electron?.cancelWindowClose?.(); + } return; } diff --git a/apps/web/src/app/settings/test-stubs/settings-test-harness.stub.ts b/apps/web/src/app/settings/test-stubs/settings-test-harness.stub.ts index 8a50210ed..a5b89d662 100644 --- a/apps/web/src/app/settings/test-stubs/settings-test-harness.stub.ts +++ b/apps/web/src/app/settings/test-stubs/settings-test-harness.stub.ts @@ -244,6 +244,7 @@ export function createElectronStub(): typeof window.electron { onWindowCloseRequested: jest.fn(() => jest.fn()), setWindowCloseGuard: jest.fn().mockResolvedValue(undefined), confirmWindowClose: jest.fn().mockResolvedValue(undefined), + cancelWindowClose: jest.fn().mockResolvedValue(undefined), openInMpv: jest.fn(), openInVlc: jest.fn(), platform: 'linux', diff --git a/libs/shared/interfaces/src/lib/electron-api.interface.ts b/libs/shared/interfaces/src/lib/electron-api.interface.ts index e9e93c865..494e3faa4 100644 --- a/libs/shared/interfaces/src/lib/electron-api.interface.ts +++ b/libs/shared/interfaces/src/lib/electron-api.interface.ts @@ -629,6 +629,11 @@ export interface ElectronBridgeApi { * original intent (window close or app quit) with the guard bypassed. */ confirmWindowClose: () => Promise; + /** + * Abandons a close the guard intercepted (the user stays), so the + * remembered close-vs-quit intent cannot leak into a later attempt. + */ + cancelWindowClose: () => Promise; onWindowCloseRequested: (callback: () => void) => () => void; fetchPlaylistByUrl: ( url: string, diff --git a/libs/shared/interfaces/src/lib/ipc-commands.ts b/libs/shared/interfaces/src/lib/ipc-commands.ts index e2272ae44..b1f812746 100644 --- a/libs/shared/interfaces/src/lib/ipc-commands.ts +++ b/libs/shared/interfaces/src/lib/ipc-commands.ts @@ -118,4 +118,5 @@ export const WINDOW_STATE_CHANGED = 'WINDOW:STATE_CHANGED'; // main process and handed to the renderer for a save/discard/stay decision export const WINDOW_SET_CLOSE_GUARD = 'WINDOW:SET_CLOSE_GUARD'; export const WINDOW_CONFIRM_CLOSE = 'WINDOW:CONFIRM_CLOSE'; +export const WINDOW_CANCEL_CLOSE = 'WINDOW:CANCEL_CLOSE'; export const WINDOW_CLOSE_REQUESTED = 'WINDOW:CLOSE_REQUESTED';