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';