From 8515b10cc4928ce45172473a72612ac3cb7a18e0 Mon Sep 17 00:00:00 2001 From: 4gray Date: Mon, 28 Sep 2026 08:21:09 +0200 Subject: [PATCH] fix(services): share only the startup inventory read A pending read was shared with any concurrent caller, but not every playlist write goes through PlaylistsService: the settings reset deletes all playlists through DatabaseService, so a caller could join a read taken before that deletion (Codex review). Share only the first read, which the startup pair (playlist effect and XMLTV reconciliation) needs while the startup screen still hides every writing action; sharing ends when it settles or a PlaylistsService write starts. Co-Authored-By: Claude Opus 5.5 --- docs/architecture/performance-journeys.md | 15 ++++---- .../src/lib/playlist-migration.spec.ts | 1 - .../playlists.service.inventory-read.spec.ts | 34 ++++++++++++++++--- .../src/lib/playlists.service.spec.ts | 1 - libs/services/src/lib/playlists.service.ts | 31 +++++++++-------- 5 files changed, 54 insertions(+), 28 deletions(-) diff --git a/docs/architecture/performance-journeys.md b/docs/architecture/performance-journeys.md index a40b3ab9e..191441e17 100644 --- a/docs/architecture/performance-journeys.md +++ b/docs/architecture/performance-journeys.md @@ -184,13 +184,14 @@ harness, which is what the ratchet needs. The main process start `renderer.ipcCallsToFirstCard` counts what the renderer asks of the main process before the first card. -- `PlaylistsService.getAllPlaylists()` shares one pending SQLite read between - concurrent callers: at startup the playlist effect and the XMLTV source - reconciliation both read the inventory, and the second caller joins the - first read and receives a `structuredClone` of its result. A settled read - is never reused, and every SQLite write detaches the pending read, so a - caller that follows a write reads again. `dbGetAppPlaylistMetas` before the - first card: 2 → 1. +- `PlaylistsService.getAllPlaylists()` shares its first SQLite read: at + startup the playlist effect and the XMLTV source reconciliation both read + the inventory, and the second caller joins the first read and receives a + `structuredClone` of its result. Sharing ends when that read settles or a + `PlaylistsService` write starts. It is limited to startup on purpose: + other services write playlists too (the settings reset deletes them + through `DatabaseService`), and while the startup screen is up no such + action can run. `dbGetAppPlaylistMetas` before the first card: 2 → 1. - `reconcileEpgSources` stays before the first card on purpose: its completion bumps `EpgSourceSettingsService.revision()`, the fence that keeps XMLTV lookups from returning data of a removed source. diff --git a/libs/services/src/lib/playlist-migration.spec.ts b/libs/services/src/lib/playlist-migration.spec.ts index ed66a5ac2..a68e974ee 100644 --- a/libs/services/src/lib/playlist-migration.spec.ts +++ b/libs/services/src/lib/playlist-migration.spec.ts @@ -38,7 +38,6 @@ describe('Electron legacy playlist migration', () => { dbService, runtime: { supportsSqlite: true }, electronMigrationPromise: null, - pendingMetas: null, }); return { playlists, dbService, electron, service }; } diff --git a/libs/services/src/lib/playlists.service.inventory-read.spec.ts b/libs/services/src/lib/playlists.service.inventory-read.spec.ts index 800171f83..d0be5779d 100644 --- a/libs/services/src/lib/playlists.service.inventory-read.spec.ts +++ b/libs/services/src/lib/playlists.service.inventory-read.spec.ts @@ -18,6 +18,13 @@ describe('PlaylistsService inventory reads', () => { return { promise, resolve }; } + async function until(ready: () => boolean, what: string) { + for (let turn = 0; !ready(); turn += 1) { + if (turn > 100) throw new Error(`${what} never started`); + await Promise.resolve(); + } + } + function setup() { const reads: ReturnType>[] = []; const electron = { @@ -43,12 +50,11 @@ describe('PlaylistsService inventory reads', () => { dbService: { getAll: jest.fn(() => of([])) }, runtime: { supportsSqlite: true }, electronMigrationPromise: null, - pendingMetas: null, playlistWriteQueues: new Map(), }); const settle = async (index: number, playlists: Playlist[]) => { // The read starts after the memoized migration's awaits. - while (!reads[index]) await Promise.resolve(); + await until(() => !!reads[index], `metadata read ${index}`); reads[index].resolve(playlists); }; return { electron, service, settle }; @@ -85,13 +91,33 @@ describe('PlaylistsService inventory reads', () => { expect(electron.dbGetAppPlaylistMetas).toHaveBeenCalledTimes(2); }); + it('stops sharing once the startup read settled', async () => { + const { electron, service, settle } = setup(); + + const startup = firstValueFrom(service.getAllPlaylists()); + await settle(0, [source('startup')]); + await startup; + // Writes that bypass this service (e.g. the settings reset through + // DatabaseService) are possible from here on, so every caller reads. + const first = firstValueFrom(service.getAllPlaylists()); + const second = firstValueFrom(service.getAllPlaylists()); + await settle(1, [source('one')]); + await settle(2, [source('two')]); + + await expect(first).resolves.toEqual([source('one')]); + await expect(second).resolves.toEqual([source('two')]); + expect(electron.dbGetAppPlaylistMetas).toHaveBeenCalledTimes(3); + }); + it('starts a fresh read for callers that arrive after a write', async () => { const { electron, service, settle } = setup(); const added = source('added'); const staleRead = firstValueFrom(service.getAllPlaylists()); - while (electron.dbGetAppPlaylistMetas.mock.calls.length === 0) - await Promise.resolve(); + await until( + () => electron.dbGetAppPlaylistMetas.mock.calls.length > 0, + 'metadata read 0' + ); await firstValueFrom(service.addPlaylist(added)); const freshRead = firstValueFrom(service.getAllPlaylists()); await settle(0, []); diff --git a/libs/services/src/lib/playlists.service.spec.ts b/libs/services/src/lib/playlists.service.spec.ts index 6b5332a2a..71a4961f6 100644 --- a/libs/services/src/lib/playlists.service.spec.ts +++ b/libs/services/src/lib/playlists.service.spec.ts @@ -60,7 +60,6 @@ describe('PlaylistsService', () => { }, electronMigrationPromise: null, indexedDbMigrationPromise: null, - pendingMetas: null, playlistWriteQueues: new Map(), playlistDeleteCleanups: [], }); diff --git a/libs/services/src/lib/playlists.service.ts b/libs/services/src/lib/playlists.service.ts index 143d7a798..9719348bf 100644 --- a/libs/services/src/lib/playlists.service.ts +++ b/libs/services/src/lib/playlists.service.ts @@ -117,9 +117,11 @@ export class PlaylistsService { inject(PLAYLIST_DELETE_CLEANUP, { optional: true }) ?? []; private electronMigrationPromise: Promise | null = null; // Startup reads the inventory twice at once (the playlist effect and the - // XMLTV source reconciliation); both share one worker round trip. Only a - // pending read is shared, and every SQLite write detaches it. - private pendingMetas: Promise | null = null; + // XMLTV source reconciliation); both share one worker round trip. Only the + // first read is shared, while the startup screen still hides every action + // that writes playlists (other services write them too, e.g. the settings + // reset); it ends when that read settles or a write here starts (null). + private startupMetas?: Promise | null; private indexedDbMigrationPromise: Promise | null = null; private readonly playlistWriteQueues = new Map>(); @@ -401,7 +403,7 @@ export class PlaylistsService { return playlist; } - this.pendingMetas = null; + this.startupMetas = null; if (operationId === undefined) { await electron.dbUpsertAppPlaylist(playlist); } else { @@ -419,7 +421,7 @@ export class PlaylistsService { return playlists; } - this.pendingMetas = null; + this.startupMetas = null; await electron.dbUpsertAppPlaylists(playlists); playlists.forEach((playlist) => this.healthEvidence?.connections.next({ id: playlist._id, playlist })); return playlists; @@ -498,9 +500,9 @@ export class PlaylistsService { getAllPlaylists() { if (this.isElectronStorageAvailable) { - const pending = this.pendingMetas; + const shared = this.startupMetas; // A joining caller gets its own copy of the shared result. - if (pending) return from(pending.then((p) => structuredClone(p))); + if (shared) return from(shared.then((p) => structuredClone(p))); const read = firstValueFrom( this.runOnSqlite(async () => { const electron = this.electronApi; @@ -513,12 +515,11 @@ export class PlaylistsService { ); }) ); - this.pendingMetas = read; - const settle = () => { - if (this.pendingMetas === read) - this.pendingMetas = null; - }; - read.then(settle, settle); + if (shared === undefined) { + this.startupMetas = read; + const end = () => (this.startupMetas = null); + read.then(end, end); + } return from(read); } @@ -573,7 +574,7 @@ export class PlaylistsService { await this.ensureElectronPlaylistMigrations(); const electron = this.electronApi; if (electron) { - this.pendingMetas = null; + this.startupMetas = null; if (options) { const deleted = await this.databaseService.deletePlaylist( @@ -1345,7 +1346,7 @@ export class PlaylistsService { await this.ensureElectronPlaylistMigrations(); const electron = this.electronApi; if (electron) { - this.pendingMetas = null; + this.startupMetas = null; await electron.dbDeleteAllPlaylists(); this.healthEvidence?.connections.next({}); }