From 439efe99d62acb71f688e6b0ffb184967950ef67 Mon Sep 17 00:00:00 2001 From: 4gray Date: Sun, 26 Jul 2026 03:58:28 +0200 Subject: [PATCH] perf(m3u): stop cancelled refresh workers --- .changes/m3u-refresh-cancellation.md | 8 + AGENTS.md | 1 + CLAUDE.md | 3 +- .../src/app/api/main.preload.spec.ts | 23 +++ .../src/app/events/playlist.events.spec.ts | 107 +++++++++-- .../src/app/events/playlist.events.ts | 172 +++++++++++------- docs/architecture/m3u-playlist-module.md | 42 ++++- .../src/lib/playlist-refresh.service.spec.ts | 49 +++++ .../src/lib/playlist-refresh.service.ts | 11 +- .../src/lib/electron-api.interface.ts | 5 +- .../src/lib/playlist-refresh.interface.ts | 22 +++ 11 files changed, 357 insertions(+), 86 deletions(-) create mode 100644 .changes/m3u-refresh-cancellation.md create mode 100644 libs/services/src/lib/playlist-refresh.service.spec.ts diff --git a/.changes/m3u-refresh-cancellation.md b/.changes/m3u-refresh-cancellation.md new file mode 100644 index 000000000..6029dad22 --- /dev/null +++ b/.changes/m3u-refresh-cancellation.md @@ -0,0 +1,8 @@ +--- +type: perf +area: m3u +--- + +Cancelling a large M3U refresh now stops its background worker before parsed +channels can be copied or saved, keeping the interface responsive and leaving +the existing playlist unchanged. diff --git a/AGENTS.md b/AGENTS.md index aa1215c03..cdadea77b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -84,6 +84,7 @@ IPTVNATOR_TRACE_STARTUP=1 nx serve electron-backend - `IPTVNATOR_TRACE_WINDOW=1` traces BrowserWindow lifecycle and unresponsive events - `IPTVNATOR_TRACE_PLAYER=1` traces external-player activity and bounded Embedded MPV runtime-probe stderr - `IPTVNATOR_TRACE_RENDERER_CONSOLE=1` mirrors renderer console output into the Electron terminal + - `IPTVNATOR_PERF_WORKER_PROFILING=1` enables development/test-only event-loop metrics in database and playlist-refresh worker responses; the performance benchmark sets it automatically, and production launches must leave it unset - Settings, portal request/response, and trace payloads must use `@iptvnator/shared/logging` or the redacting portal logger before reaching diff --git a/CLAUDE.md b/CLAUDE.md index 9b5b422fd..a8bfaedc1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -141,6 +141,7 @@ Useful narrower flags: - `IPTVNATOR_TRACE_WINDOW=1` traces BrowserWindow navigation/load lifecycle - `IPTVNATOR_TRACE_PLAYER=1` traces external-player activity and bounded Embedded MPV runtime-probe stderr - `IPTVNATOR_TRACE_RENDERER_CONSOLE=1` mirrors renderer console logs into the Electron terminal +- `IPTVNATOR_PERF_WORKER_PROFILING=1` enables development/test-only event-loop metrics in database and playlist-refresh worker responses; the performance benchmark sets it automatically, and production launches must leave it unset Settings, portal request/response, and trace payloads must use `@iptvnator/shared/logging` or the redacting portal logger before reaching @@ -615,7 +616,7 @@ This project uses modern Angular signal-based APIs and patterns. **ALWAYS** use - EPG parsing: `epg-parser.worker.ts`; main-process worker lifecycle is coordinated from `apps/electron-backend/src/app/events/epg-worker.service.ts` - Non-EPG SQLite work: `database.worker.ts` (see `docs/architecture/sqlite-db-worker.md`) -- Playlist refresh: `playlist-refresh.worker.ts` +- Playlist refresh: `playlist-refresh.worker.ts`; explicit cancellation is main-process-owned and terminates the one-shot worker before acknowledging `PLAYLIST_CANCEL_REFRESH` (see `docs/architecture/m3u-playlist-module.md`) ### Key Features diff --git a/apps/electron-backend/src/app/api/main.preload.spec.ts b/apps/electron-backend/src/app/api/main.preload.spec.ts index aaf54a94e..54d2ddc9f 100644 --- a/apps/electron-backend/src/app/api/main.preload.spec.ts +++ b/apps/electron-backend/src/app/api/main.preload.spec.ts @@ -220,6 +220,29 @@ describe('main preload DB IPC contract', () => { ); }); + it('preserves a structured playlist cancellation result across the context bridge', async () => { + const api = getExposedApi(); + const payload = { + operationId: 'playlist-refresh-cancelled', + playlistId: 'playlist-1', + title: 'Large playlist', + url: 'http://127.0.0.1/large.m3u', + }; + const cancelledResult = { + operationId: payload.operationId, + type: 'playlist-refresh-cancelled', + } as const; + mockIpcRenderer.invoke.mockResolvedValueOnce(cancelledResult); + + await expect(api.refreshPlaylist(payload)).resolves.toEqual( + cancelledResult + ); + expect(mockIpcRenderer.invoke).toHaveBeenLastCalledWith( + 'PLAYLIST:REFRESH', + payload + ); + }); + it('keeps the legacy save-content progress bridge scoped to progress events', () => { const api = getExposedApi(); const callback = jest.fn(); diff --git a/apps/electron-backend/src/app/events/playlist.events.spec.ts b/apps/electron-backend/src/app/events/playlist.events.spec.ts index d25d11f9b..f1300f5a6 100644 --- a/apps/electron-backend/src/app/events/playlist.events.spec.ts +++ b/apps/electron-backend/src/app/events/playlist.events.spec.ts @@ -527,7 +527,94 @@ describe('playlist IPC events', () => { expect(worker.terminate).toHaveBeenCalled(); }); - it('routes refresh cancellation to the active worker and converts worker error responses to Error instances', async () => { + it('settles cancellation without waiting for a CPU-bound refresh worker', async () => { + const ipcEvent = createIpcEvent(); + const payload: PlaylistRefreshPayload = { + operationId: 'refresh-busy', + playlistId: 'playlist-busy', + title: 'Busy playlist', + filePath: '/playlists/busy.m3u', + }; + const refreshPromise = getHandler(PLAYLIST_REFRESH)(ipcEvent, payload); + const worker = mockWorkerInstances[0]; + let outcome: + | { error: unknown; status: 'rejected' } + | { status: 'resolved'; value: unknown } + | undefined; + void refreshPromise.then( + (value) => { + outcome = { status: 'resolved', value }; + }, + (error: unknown) => { + outcome = { error, status: 'rejected' }; + } + ); + + worker.emit('message', { type: 'ready' }); + worker.emit('message', { + event: { + operationId: payload.operationId, + phase: 'parsing', + playlistId: payload.playlistId, + status: 'progress', + } satisfies PlaylistRefreshEvent, + type: 'event', + }); + + let finishTermination: ((exitCode: number) => void) | undefined; + worker.terminate.mockImplementationOnce( + () => + new Promise((resolveTermination) => { + finishTermination = resolveTermination; + }) + ); + const cancelPromise = getHandler(PLAYLIST_CANCEL_REFRESH)( + createIpcEvent(), + payload.operationId + ); + await Promise.resolve(); + + expect(worker.removeAllListeners).toHaveBeenCalledTimes(1); + expect(worker.terminate).toHaveBeenCalledTimes(1); + expect(worker.postMessage).toHaveBeenLastCalledWith({ + operationId: payload.operationId, + type: 'cancel', + }); + expect(outcome).toBeUndefined(); + expect(ipcEvent.sender.send).not.toHaveBeenCalledWith( + PLAYLIST_REFRESH_EVENT, + expect.objectContaining({ status: 'cancelled' }) + ); + + finishTermination?.(1); + await expect(cancelPromise).resolves.toEqual({ success: true }); + await Promise.resolve(); + + expect(outcome).toEqual({ + status: 'resolved', + value: { + operationId: payload.operationId, + type: 'playlist-refresh-cancelled', + }, + }); + expect(ipcEvent.sender.send).toHaveBeenLastCalledWith( + PLAYLIST_REFRESH_EVENT, + { + operationId: payload.operationId, + phase: 'parsing', + playlistId: payload.playlistId, + status: 'cancelled', + } + ); + await expect( + getHandler(PLAYLIST_CANCEL_REFRESH)( + createIpcEvent(), + payload.operationId + ) + ).resolves.toEqual({ success: false }); + }); + + it('converts playlist refresh worker error responses to Error instances', async () => { const payload: PlaylistRefreshPayload = { operationId: 'refresh-error', playlistId: 'playlist-error', @@ -540,17 +627,6 @@ describe('playlist IPC events', () => { ); const worker = mockWorkerInstances[0]; - expect( - await getHandler(PLAYLIST_CANCEL_REFRESH)( - createIpcEvent(), - 'refresh-error' - ) - ).toEqual({ success: true }); - expect(worker.postMessage).toHaveBeenCalledWith({ - operationId: 'refresh-error', - type: 'cancel', - }); - const rejectedRefresh = expect(refreshPromise).rejects.toMatchObject({ message: 'Refresh failed', name: 'PlaylistRefreshFailure', @@ -568,13 +644,6 @@ describe('playlist IPC events', () => { }); await rejectedRefresh; - - expect( - await getHandler(PLAYLIST_CANCEL_REFRESH)( - createIpcEvent(), - 'refresh-error' - ) - ).toEqual({ success: false }); }); it('returns save dialog paths and writes files through the filesystem handler', async () => { diff --git a/apps/electron-backend/src/app/events/playlist.events.ts b/apps/electron-backend/src/app/events/playlist.events.ts index d177c7987..ddd273d5a 100644 --- a/apps/electron-backend/src/app/events/playlist.events.ts +++ b/apps/electron-backend/src/app/events/playlist.events.ts @@ -11,9 +11,11 @@ import { AUTO_UPDATE_PLAYLISTS, PLAYLIST_CANCEL_REFRESH, PLAYLIST_REFRESH, + PLAYLIST_REFRESH_CANCELLED_RESULT_TYPE, PLAYLIST_REFRESH_EVENT, ElectronBridgeTrustOptions, Playlist, + PlaylistRefreshCancelledResult, PlaylistRefreshEvent, PlaylistRefreshPayload, summarizeAutoUpdateOutcomes, @@ -40,10 +42,7 @@ export default class PlaylistEvents { const playlistWriteAuthorizer = new PlaylistWriteAuthorizer(); type ActivePlaylistRefresh = { - reject: (reason?: unknown) => void; - resolve: (value: Playlist) => void; - sender: WebContents; - worker: Worker; + cancel: () => Promise; }; const activePlaylistRefreshes = new Map(); @@ -170,76 +169,126 @@ ipcMain.handle( async (event, payload: PlaylistRefreshPayload) => { const worker = resolvePlaylistRefreshWorker(); - return await new Promise((resolve, reject) => { - const cleanup = async (): Promise => { - activePlaylistRefreshes.delete(payload.operationId); - worker.removeAllListeners(); - await worker.terminate().catch(() => undefined); - }; + return await new Promise( + (resolve, reject) => { + let cleanupPromise: Promise | null = null; + let lastPhase: PlaylistRefreshEvent['phase'] = payload.url + ? 'fetching' + : 'reading-file'; + let settled = false; - activePlaylistRefreshes.set(payload.operationId, { - worker, - sender: event.sender, - resolve, - reject, - }); + const cleanup = (): Promise => { + cleanupPromise ??= (async () => { + activePlaylistRefreshes.delete(payload.operationId); + worker.removeAllListeners(); + await worker.terminate().catch(() => undefined); + })(); + return cleanupPromise; + }; - worker.on( - 'message', - async (message: PlaylistRefreshWorkerMessage) => { - if (message.type === 'ready') { - worker.postMessage({ - type: 'request', - payload, - }); + const cancel = async (): Promise => { + if (settled) { return; } + settled = true; - if (message.type === 'event') { - emitPlaylistRefreshEvent(event.sender, message.event); - return; + try { + worker.postMessage({ + type: 'cancel', + operationId: payload.operationId, + }); + } catch { + // Termination below is authoritative even if cooperative + // cancellation cannot be delivered. } await cleanup(); + emitPlaylistRefreshEvent(event.sender, { + operationId: payload.operationId, + playlistId: payload.playlistId, + phase: lastPhase, + status: 'cancelled', + }); + resolve({ + operationId: payload.operationId, + type: PLAYLIST_REFRESH_CANCELLED_RESULT_TYPE, + }); + }; - const response = - message as PlaylistRefreshWorkerResponseMessage; - if (response.success && response.result) { - resolve(response.result); + const activeRefresh: ActivePlaylistRefresh = { cancel }; + activePlaylistRefreshes.set(payload.operationId, activeRefresh); + + worker.on( + 'message', + async (message: PlaylistRefreshWorkerMessage) => { + if (settled) { + return; + } + + if (message.type === 'ready') { + worker.postMessage({ + type: 'request', + payload, + }); + return; + } + + if (message.type === 'event') { + lastPhase = message.event.phase ?? lastPhase; + emitPlaylistRefreshEvent( + event.sender, + message.event + ); + return; + } + + settled = true; + await cleanup(); + + const response = + message as PlaylistRefreshWorkerResponseMessage; + if (response.success && response.result) { + resolve(response.result); + return; + } + + reject( + createPlaylistRefreshError( + response.error ?? { + message: + 'Playlist refresh worker request failed', + } + ) + ); + } + ); + + worker.on('error', async (error) => { + if (settled) { + return; + } + settled = true; + await cleanup(); + reject(error); + }); + + worker.on('exit', async (code) => { + if (settled) { return; } + settled = true; + await cleanup(); reject( - createPlaylistRefreshError( - response.error ?? { - message: - 'Playlist refresh worker request failed', - } + new Error( + code === 0 + ? 'Playlist refresh worker exited unexpectedly' + : `Playlist refresh worker stopped with exit code ${code}` ) ); - } - ); - - worker.on('error', async (error) => { - await cleanup(); - reject(error); - }); - - worker.on('exit', async (code) => { - if (!activePlaylistRefreshes.has(payload.operationId)) { - return; - } - - await cleanup(); - reject( - new Error( - code === 0 - ? 'Playlist refresh worker exited unexpectedly' - : `Playlist refresh worker stopped with exit code ${code}` - ) - ); - }); - }); + }); + } + ); } ); @@ -251,10 +300,7 @@ ipcMain.handle( return { success: false }; } - activeRefresh.worker.postMessage({ - type: 'cancel', - operationId, - }); + await activeRefresh.cancel(); return { success: true }; } diff --git a/docs/architecture/m3u-playlist-module.md b/docs/architecture/m3u-playlist-module.md index a376abbd3..48a4a468b 100644 --- a/docs/architecture/m3u-playlist-module.md +++ b/docs/architecture/m3u-playlist-module.md @@ -62,7 +62,16 @@ Two paths re-download an M3U playlist from its original source: - **Explicit refresh** — `PLAYLIST_REFRESH` runs in `playlist-refresh.worker.ts`, reports progress through `PLAYLIST_REFRESH_EVENT`, and is cancellable via - `PLAYLIST_CANCEL_REFRESH`. + `PLAYLIST_CANCEL_REFRESH`. Cancellation is owned by the main process: it first + sends the cooperative cancel message, then terminates the one-shot worker + without waiting for its event loop. The cancel IPC resolves only after the + worker has stopped, the correlated `cancelled` event has been emitted with the + last known phase, and a structured cancellation result is ready. That result + crosses both Electron IPC and the context bridge unchanged; + `PlaylistRefreshService` converts it into a renderer-local `AbortError`. + Relying on an error created in main or preload would lose its `name` at one of + those serialization boundaries. A cancelled refresh must not update the + renderer store or reach SQLite. - **Startup auto-update** — after `loadPlaylistsSuccess`, `AppComponent` sends `AUTO_UPDATE_PLAYLISTS` for every playlist with `autoRefresh === true`. The main process fulfils it in `playlist-auto-update.ts` on top of `playlist-source.ts`. @@ -87,6 +96,37 @@ dead source must never stall startup (issue #931): so refresh logging goes through `redactSensitiveData()` from `@iptvnator/shared/logging`. +### Refresh Cancellation Performance Regression + +The Electron E2E project includes a deterministic 100,000-channel cancellation +benchmark. It uses only a loopback synthetic M3U server, performs one warm-up, +five measured runs, and one diagnostic run, and writes summaries plus raw +profiles below the gitignored `dist/performance/` directory: + +```bash +perf_output="$PWD/dist/performance/$(date -u +%Y%m%dT%H%M%SZ)-m3u-refresh-cancel" +IPTVNATOR_PERF_OUTPUT_DIR="$perf_output" \ +IPTVNATOR_PERF_VARIANT=after \ +pnpm nx run electron-backend-e2e:benchmark-m3u-refresh-cancellation +``` + +The output path must be an absolute, previously unused descendant of +`dist/performance/`. A formal run fails on a dirty worktree and records the +commit, source-state hash, OS/architecture, Node, Electron, and fixture identity +in its manifest. Commit the harness first and capture `baseline` from that clean +commit; commit the production change separately, rebuild, and capture `after` +with the same harness and machine. Set `IPTVNATOR_PERF_SMOKE=1` for one measured +run during harness development; smoke runs may be dirty and must not support +before/after claims. + +The target reserves and verifies CDP port 9222, freezes renderer long-task, +frame-gap, and heartbeat probes before forced post-GC heap collection, and +enables opt-in worker profiling. Worker event-loop delay is read from a +request-scoped `node:perf_hooks` capture; a worker terminated before it can flush +the capture reports the metric as unavailable rather than zero. Diagnostic CPU +profiles, heap snapshots, and Chromium traces are excluded from the five-run +headline distributions. + ### Reporting The Auto-Update Result Because auto-update isolates failures, it must also report them — otherwise a diff --git a/libs/services/src/lib/playlist-refresh.service.spec.ts b/libs/services/src/lib/playlist-refresh.service.spec.ts new file mode 100644 index 000000000..920db2940 --- /dev/null +++ b/libs/services/src/lib/playlist-refresh.service.spec.ts @@ -0,0 +1,49 @@ +import type { + ElectronBridgeApi, + PlaylistRefreshPayload, +} from '@iptvnator/shared/interfaces'; + +import { PlaylistRefreshService } from './playlist-refresh.service'; + +describe('PlaylistRefreshService', () => { + const originalElectron = window.electron; + const payload: PlaylistRefreshPayload = { + operationId: 'playlist-refresh-cancelled', + playlistId: 'playlist-1', + title: 'Large playlist', + url: 'http://127.0.0.1/large.m3u', + }; + + afterEach(() => { + Object.defineProperty(window, 'electron', { + configurable: true, + value: originalElectron, + writable: true, + }); + }); + + it('creates a renderer-local AbortError from a cancellation result', async () => { + const unsubscribe = jest.fn(); + const electron = { + onPlaylistRefreshEvent: jest.fn(() => unsubscribe), + refreshPlaylist: jest.fn().mockResolvedValue({ + operationId: payload.operationId, + type: 'playlist-refresh-cancelled', + }), + } as unknown as ElectronBridgeApi; + Object.defineProperty(window, 'electron', { + configurable: true, + value: electron, + writable: true, + }); + + await expect( + new PlaylistRefreshService().refreshPlaylist(payload) + ).rejects.toMatchObject({ + message: + 'Playlist refresh "playlist-refresh-cancelled" was cancelled', + name: 'AbortError', + }); + expect(unsubscribe).toHaveBeenCalledTimes(1); + }); +}); diff --git a/libs/services/src/lib/playlist-refresh.service.ts b/libs/services/src/lib/playlist-refresh.service.ts index d4ab49d79..f73f36ab7 100644 --- a/libs/services/src/lib/playlist-refresh.service.ts +++ b/libs/services/src/lib/playlist-refresh.service.ts @@ -1,5 +1,6 @@ import { Injectable } from '@angular/core'; import { + isPlaylistRefreshCancelledResult, Playlist, PlaylistRefreshEvent, PlaylistRefreshPayload, @@ -30,7 +31,15 @@ export class PlaylistRefreshService { }); try { - return await window.electron.refreshPlaylist(payload); + const result = await window.electron.refreshPlaylist(payload); + if (isPlaylistRefreshCancelledResult(result)) { + const error = new Error( + `Playlist refresh "${result.operationId}" was cancelled` + ); + error.name = 'AbortError'; + throw error; + } + return result; } finally { unsubscribe?.(); } diff --git a/libs/shared/interfaces/src/lib/electron-api.interface.ts b/libs/shared/interfaces/src/lib/electron-api.interface.ts index b36355207..08214323f 100644 --- a/libs/shared/interfaces/src/lib/electron-api.interface.ts +++ b/libs/shared/interfaces/src/lib/electron-api.interface.ts @@ -21,6 +21,7 @@ import { XtreamBackupRecentlyViewedItem, } from './playlist-backup.interface'; import { + PlaylistRefreshCancelledResult, PlaylistRefreshEvent, PlaylistRefreshPayload, } from './playlist-refresh.interface'; @@ -671,7 +672,9 @@ export interface ElectronBridgeApi { url: string, method?: 'GET' | 'HEAD' ) => Promise; - refreshPlaylist: (payload: PlaylistRefreshPayload) => Promise; + refreshPlaylist: ( + payload: PlaylistRefreshPayload + ) => Promise; cancelPlaylistRefresh: ( operationId: string ) => Promise; diff --git a/libs/shared/interfaces/src/lib/playlist-refresh.interface.ts b/libs/shared/interfaces/src/lib/playlist-refresh.interface.ts index b02ee028f..6a153f952 100644 --- a/libs/shared/interfaces/src/lib/playlist-refresh.interface.ts +++ b/libs/shared/interfaces/src/lib/playlist-refresh.interface.ts @@ -27,3 +27,25 @@ export interface PlaylistRefreshPayload { url?: string; trustedInsecureTlsHosts?: string[]; } + +export const PLAYLIST_REFRESH_CANCELLED_RESULT_TYPE = + 'playlist-refresh-cancelled' as const; + +export interface PlaylistRefreshCancelledResult { + operationId: string; + type: typeof PLAYLIST_REFRESH_CANCELLED_RESULT_TYPE; +} + +export function isPlaylistRefreshCancelledResult( + value: unknown +): value is PlaylistRefreshCancelledResult { + if (!value || typeof value !== 'object') { + return false; + } + + const candidate = value as Record; + return ( + candidate['type'] === PLAYLIST_REFRESH_CANCELLED_RESULT_TYPE && + typeof candidate['operationId'] === 'string' + ); +}