From 1059224d79e4fa87968d1262d9c811d1d1ee589e Mon Sep 17 00:00:00 2001 From: 4gray Date: Wed, 14 Oct 2026 20:21:52 +0200 Subject: [PATCH] fix(server-storage): Delete All waits for repairs holding a reservation; backups read the server again Delete All raised its write barrier before it held the exclusive authority reset. A Stalker repair already holding its row reservation then waited on that barrier for its conditional write, while the reset waited for the reservation: neither finished, and later playlist operations stalled behind them. The barrier now goes up inside the reset, once repairs in flight have finished; writes queued so far land first and new ones wait for the delete, as before. ready() said yes after any earlier read, so an export could miss what another device added since, and restoring an empty list did nothing when the cache was already empty. Backup export, restore and the restore replay now ask for a fresh read (ready(true)) and fail, the restore staying parked, when it does not succeed. The facade's adoption loop moves to the adopt module to keep the facade under its line limit. Co-Authored-By: Claude Opus 5.5 --- docs/architecture/playlist-backup-restore.md | 7 +- docs/architecture/pwa-self-hosted.md | 5 +- .../pwa-xtream-data-source.spec.ts | 4 +- .../data-sources/pwa-xtream-data-source.ts | 9 +-- ...playlist-backup.service.pwa-xtream.spec.ts | 2 + .../src/lib/playlists.service.spec.ts | 64 +++++++++++++++++++ libs/services/src/lib/playlists.service.ts | 55 ++++++++-------- .../lib/pwa-xtream-backup-state.service.ts | 4 +- .../server-storage/server-user-data-adopt.ts | 39 ++++++++++- .../server-user-data-storage.spec.ts | 16 +++++ .../server-user-data-storage.ts | 42 +++++------- libs/services/src/lib/user-data-storage.ts | 5 +- 12 files changed, 187 insertions(+), 65 deletions(-) diff --git a/docs/architecture/playlist-backup-restore.md b/docs/architecture/playlist-backup-restore.md index 52ac6db09..72d58abbe 100644 --- a/docs/architecture/playlist-backup-restore.md +++ b/docs/architecture/playlist-backup-restore.md @@ -252,9 +252,10 @@ self-hosted server storage is covered too. browser tracks neither. - An unreadable map (storage error, malformed JSON, a playlist entry that is not a list) fails the export instead of being written as an empty list. - So does server mode before its first read of the server's user data: - the export waits for a read in flight (`UserDataStorage.ready`) and fails - when none succeeded. A restore does the same and stays parked, since its + So does server mode when it cannot read the server's user data: the + export reads it again (`UserDataStorage.ready(true)`, as another device + may have changed it since this page's first read) and fails when that + read does not succeed. A restore does the same and stays parked, since its replacement lists would otherwise merge into the server's; it also stays parked until the server took that playlist's lists. - A pending restore snapshot still wins over the stored maps, as on desktop. diff --git a/docs/architecture/pwa-self-hosted.md b/docs/architecture/pwa-self-hosted.md index c3e7fab5b..5936f001a 100644 --- a/docs/architecture/pwa-self-hosted.md +++ b/docs/architecture/pwa-self-hosted.md @@ -264,8 +264,9 @@ settings and guards read them in every build. succeeded. The Xtream data source's favorites, favorite checks, history and position reads wait for it, so a new device does not show empty lists or start playback without its resume position. A backup export or - restore (also its replay when the playlist opens) fails while no read - succeeded: unread maps would export as empty lists, and a restore's lists + restore (also its replay when the playlist opens) reads the server again + first (`ready(true)`: another device may have changed the lists since) and + fails when that read does not succeed: unread maps would export as empty lists, and a restore's lists would merge into the server's instead of replacing them (the restore stays parked). A restore also counts only once the server took that playlist's lists (`UserDataStorage.flushed`); a refused one stays parked. Replayed edits keep recently viewed lists newest first, diff --git a/libs/portal/xtream/data-access/src/lib/data-sources/pwa-xtream-data-source.spec.ts b/libs/portal/xtream/data-access/src/lib/data-sources/pwa-xtream-data-source.spec.ts index 646359f1d..8bf976ea8 100644 --- a/libs/portal/xtream/data-access/src/lib/data-sources/pwa-xtream-data-source.spec.ts +++ b/libs/portal/xtream/data-access/src/lib/data-sources/pwa-xtream-data-source.spec.ts @@ -1021,7 +1021,7 @@ describe('PwaXtreamDataSource', () => { removeItem: (key: string) => { values.delete(key); }, - ready: async () => false, + ready: jest.fn(async () => false), }; TestBed.resetTestingModule(); TestBed.configureTestingModule({ @@ -1043,6 +1043,8 @@ describe('PwaXtreamDataSource', () => { playbackPositions: [], } as never) ).rejects.toThrow(/restore waits/); + // A fresh read: the restore replaces the current lists. + expect(storage.ready).toHaveBeenCalledWith(true); expect(values.get('xtream-favorites')).toBe( JSON.stringify({ p1: ['live:1'] }) ); diff --git a/libs/portal/xtream/data-access/src/lib/data-sources/pwa-xtream-data-source.ts b/libs/portal/xtream/data-access/src/lib/data-sources/pwa-xtream-data-source.ts index 0941c175b..6f9ddad36 100644 --- a/libs/portal/xtream/data-access/src/lib/data-sources/pwa-xtream-data-source.ts +++ b/libs/portal/xtream/data-access/src/lib/data-sources/pwa-xtream-data-source.ts @@ -951,10 +951,11 @@ export class PwaXtreamDataSource implements IXtreamDataSource { * Server mode: favorites, history and positions show the server's data * once its first read settled, which startup does not wait for beyond a * few seconds. A read before it would show a new device empty lists. - * Whether a read succeeded (always true outside server mode). + * Whether a read succeeded (always true outside server mode); `fresh` + * reads again first, for a restore that replaces the current lists. */ - private async userDataRead(): Promise { - return (await this.userDataStorage.ready?.()) ?? true; + private async userDataRead(fresh = false): Promise { + return (await this.userDataStorage.ready?.(fresh)) ?? true; } private getPlaybackPositionsFromStorage(): Record< @@ -1407,7 +1408,7 @@ export class PwaXtreamDataSource implements IXtreamDataSource { void options; // Its lists replace the server's: before the server's are read they // would merge into them instead. The restore stays parked. - if (!(await this.userDataRead())) { + if (!(await this.userDataRead(true))) { throw new Error( "The server's user data could not be read yet; the restore waits." ); diff --git a/libs/services/src/lib/playlist-backup.service.pwa-xtream.spec.ts b/libs/services/src/lib/playlist-backup.service.pwa-xtream.spec.ts index e02f6a0d8..d9c54858b 100644 --- a/libs/services/src/lib/playlist-backup.service.pwa-xtream.spec.ts +++ b/libs/services/src/lib/playlist-backup.service.pwa-xtream.spec.ts @@ -288,6 +288,8 @@ describe('PlaylistBackupService Xtream user state in the PWA', () => { const { manifest } = await service.exportBackup(); + // A fresh read: another device may have changed them since. + expect(ready).toHaveBeenCalledWith(true); expect(ready).toHaveBeenCalledTimes(1); expect(xtreamEntry(manifest).userState.favorites).not.toEqual([]); }); diff --git a/libs/services/src/lib/playlists.service.spec.ts b/libs/services/src/lib/playlists.service.spec.ts index 78c9723e4..95929ee24 100644 --- a/libs/services/src/lib/playlists.service.spec.ts +++ b/libs/services/src/lib/playlists.service.spec.ts @@ -1808,6 +1808,70 @@ describe('PlaylistsService', () => { expect(order).toEqual(['read', 'write', 'delete all', 'read', 'write']); }); + it('lets a repair holding its reservation write before Delete All', async () => { + let releaseRepair: () => void = () => undefined; + const repairHeld = new Promise( + (resolve) => (releaseRepair = resolve) + ); + const request = jest.fn( + async ( + name: string, + options: LockOptions, + callback: (lock: Lock | null) => Promise + ) => { + // The reset waits for the reservation a repair holds. + if (options.mode === 'exclusive') { + await repairHeld; + } + return callback({ + name, + mode: options.mode ?? 'exclusive', + } as Lock); + } + ); + Object.defineProperty(globalThis.navigator, 'locks', { + configurable: true, + value: { request }, + }); + const electron = { + dbGetAppPlaylist: jest.fn(async () => ({ + _id: 'p9', + title: 'P', + favorites: [], + })), + dbGetAppPlaylists: jest.fn(async () => []), + dbGetAppState: jest.fn(async (key: string) => + key === SQLITE_PLAYLIST_MIGRATION_FLAG ? '1' : null + ), + dbSetAppState: jest.fn(), + dbUpsertAppPlaylist: jest.fn(async () => undefined), + dbUpsertAppPlaylists: jest.fn(), + dbDeleteAllPlaylists: jest.fn(async () => undefined), + }; + testWindow.electron = electron; + const service = createService(); + const settle = async () => { + for (let i = 0; i < 50; i += 1) { + await Promise.resolve(); + } + }; + + const removed = firstValueFrom(service.removeAll()); + // The repair's conditional row write, under its reservation. + let written = false; + const repair = firstValueFrom( + service.transformPlaylistFavorites('p9', (f) => [...f, 'a']) + ).then(() => (written = true)); + await settle(); + + expect(written).toBe(true); + expect(electron.dbDeleteAllPlaylists).not.toHaveBeenCalled(); + releaseRepair(); + await repair; + await removed; + expect(electron.dbDeleteAllPlaylists).toHaveBeenCalled(); + }); + it('runs a mutation again on a fresh read when the server refused it', async () => { const versions = [ { _id: 'p9', title: 'P', favorites: ['a'] }, diff --git a/libs/services/src/lib/playlists.service.ts b/libs/services/src/lib/playlists.service.ts index 2f8943982..c987b310e 100644 --- a/libs/services/src/lib/playlists.service.ts +++ b/libs/services/src/lib/playlists.service.ts @@ -1342,34 +1342,39 @@ export class PlaylistsService { } removeAll(): Observable { - // Writes already queued land first; new ones wait for the delete. - return defer(() => { - const pending = [...this.playlistWriteQueues.values()]; - const run = Promise.all(pending).then(() => this.removeAllNow()); - this.deletingAll = run.then( - () => undefined, - () => undefined - ); - return run; - }); + // Inside the exclusive authority reset: a Stalker repair already + // holding its reservation finishes its write first (blocking it here + // would deadlock). Then writes queued so far land, and new ones wait + // for the delete. + return defer(() => + runWithPlaylistAuthorityReset(async () => { + const pending = [...this.playlistWriteQueues.values()]; + let deleted: () => void = () => undefined; + this.deletingAll = new Promise( + (resolve) => (deleted = resolve) + ); + try { + await Promise.all(pending); + await this.deleteAllRows(); + } finally { + deleted(); + } + }) + ); } - private removeAllNow(): Promise { - return runWithPlaylistAuthorityReset(async () => { - if (this.usesDocumentStore) { - await this.ensureElectronPlaylistMigrations(); - const store = this.documentStore; - if (store) { - this.startupMetas = null; - await store.dbDeleteAllPlaylists(); - this.healthEvidence?.connections.next({}); - } - return undefined; + private async deleteAllRows(): Promise { + if (this.usesDocumentStore) { + await this.ensureElectronPlaylistMigrations(); + const store = this.documentStore; + if (store) { + this.startupMetas = null; + await store.dbDeleteAllPlaylists(); + this.healthEvidence?.connections.next({}); } - - await firstValueFrom(this.dbService.clear(DbStores.Playlists)); - return undefined; - }); + return; + } + await firstValueFrom(this.dbService.clear(DbStores.Playlists)); } private normalizePortalRecentIdentity(value: unknown): string { diff --git a/libs/services/src/lib/pwa-xtream-backup-state.service.ts b/libs/services/src/lib/pwa-xtream-backup-state.service.ts index b6baeb322..c72b7810a 100644 --- a/libs/services/src/lib/pwa-xtream-backup-state.service.ts +++ b/libs/services/src/lib/pwa-xtream-backup-state.service.ts @@ -38,7 +38,9 @@ export class PwaXtreamBackupStateService { * restore's replacement lists would merge into the server's instead. */ async ready(): Promise { - if ((await this.storage.ready?.()) === false) { + // Read again: another device may have changed them since this + // page's first read, and a restore must replace the current lists. + if ((await this.storage.ready?.(true)) === false) { throw new Error( "The server's user data could not be read yet; try again once the server answers." ); diff --git a/libs/services/src/lib/server-storage/server-user-data-adopt.ts b/libs/services/src/lib/server-storage/server-user-data-adopt.ts index 756f187ec..b596ca092 100644 --- a/libs/services/src/lib/server-storage/server-user-data-adopt.ts +++ b/libs/services/src/lib/server-storage/server-user-data-adopt.ts @@ -10,7 +10,12 @@ import { replaySteps } from './server-user-data-edit'; import type { UserDataWriteQueue } from './server-user-data-queue'; import type { UserDataStorage } from '../user-data-storage'; import type { DeferredEdit } from './server-user-data-edit'; -import { readDeferredEdits, readUnsentWrites } from './server-unsent-writes'; +import { + othersUnconfirmed, + readDeferredEdits, + readUnsentWrites, +} from './server-unsent-writes'; +import type { UserDataCache } from './server-user-data-cache'; /** * A closed tab's saved edits, ahead of this tab's own (which may have been @@ -128,3 +133,35 @@ export async function settleWrites( } return !unsent(); } + +/** + * Another page load's saved writes and early edits, shown on what this tab + * shows now and queued with its writes (also when the cache shows them). + */ +export function adoptPageEdits( + writes: UserDataWriteQueue, + cache: UserDataCache, + from: { + readonly local: UserDataStorage; + readonly instanceId: string; + readonly tabId: string; + readonly page: string; + } +): void { + const { local, instanceId, tabId, page } = from; + const foreign = othersUnconfirmed(local, instanceId, tabId); + for (const { namespace, playlistId, steps } of [ + ...readDeferredEdits(local, instanceId, page), + ...readUnsentWrites(local, instanceId, page), + ]) { + const value = adoptEdits( + writes, + namespace, + playlistId, + cache.map(namespace)[playlistId], + steps, + foreign.has(`${namespace}|${playlistId}`) + ); + cache.setEntry(namespace, playlistId, value); + } +} diff --git a/libs/services/src/lib/server-storage/server-user-data-storage.spec.ts b/libs/services/src/lib/server-storage/server-user-data-storage.spec.ts index 46b797970..7f8a4b7ce 100644 --- a/libs/services/src/lib/server-storage/server-user-data-storage.spec.ts +++ b/libs/services/src/lib/server-storage/server-user-data-storage.spec.ts @@ -594,6 +594,22 @@ describe('ServerUserDataStorage', () => { expect(api.getJson).toHaveBeenCalledTimes(2); }); + it('reads the server again when a backup asks for fresh data', async () => { + const { api, storage } = setup(); + await storage.hydrate(); + api.getJson.mockClear(); + + // Another device may have changed it since: read again. + await expect(storage.ready(true)).resolves.toBe(true); + expect(api.getJson).toHaveBeenCalledTimes(1); + + api.getJson.mockRejectedValueOnce(new Error('offline')); + await expect(storage.ready(true)).resolves.toBe(false); + // Without `fresh` an earlier read is enough. + await expect(storage.ready()).resolves.toBe(true); + expect(api.getJson).toHaveBeenCalledTimes(2); + }); + it("never writes over another server instance's cache", async () => { const local = memoryStorage(); const tab = (instanceId: string, favorites: unknown) => diff --git a/libs/services/src/lib/server-storage/server-user-data-storage.ts b/libs/services/src/lib/server-storage/server-user-data-storage.ts index 2a2c37a55..e4844dc49 100644 --- a/libs/services/src/lib/server-storage/server-user-data-storage.ts +++ b/libs/services/src/lib/server-storage/server-user-data-storage.ts @@ -14,7 +14,7 @@ import { UNKNOWN_TAG, } from './server-user-data-entries'; import { - adoptEdits, + adoptPageEdits, extendUnsent, restorePage, settleWrites, @@ -36,8 +36,6 @@ import { orphanedPages, pageIsLive, othersUnconfirmed, - readDeferredEdits, - readUnsentWrites, removePage, saveDeferredEdits, saveUnsentWrites, @@ -159,27 +157,12 @@ export class ServerUserDataStorage implements UserDataStorage { /** Its edits go ahead of this tab's own, which may be newer. */ private adoptPage(page: string): void { - const edits = [ - ...readDeferredEdits(this.local, this.instanceId, page), - ...readUnsentWrites(this.local, this.instanceId, page), - ]; - const foreign = othersUnconfirmed( - this.local, - this.instanceId, - this.tabId - ); - for (const { namespace, playlistId, steps } of edits) { - // Queued even when the cache shows it already. - const value = adoptEdits( - this.writes, - namespace, - playlistId, - this.cache.map(namespace)[playlistId], - steps, - foreign.has(`${namespace}|${playlistId}`) - ); - this.cache.setEntry(namespace, playlistId, value); - } + adoptPageEdits(this.writes, this.cache, { + local: this.local, + instanceId: this.instanceId, + tabId: this.tabId, + page, + }); this.adopted.add(page); // The other page's copy goes only once this tab saved its own. this.release(page, this.saveUnsent()); @@ -232,9 +215,16 @@ export class ServerUserDataStorage implements UserDataStorage { /** * Once a first read in flight settled: whether reads show the server's - * data. Starts no read, so an offline page is not held up. + * data. Starts no read, so an offline page is not held up; `fresh` + * (backups) reads the server again and tells whether that succeeded. */ - async ready(): Promise { + async ready(fresh = false): Promise { + if (fresh) { + return this.hydrate().then( + () => true, + () => false + ); + } await (this.hydrated ? undefined : this.hydration); return this.hydrated; } diff --git a/libs/services/src/lib/user-data-storage.ts b/libs/services/src/lib/user-data-storage.ts index d085fa7de..c36f7bfeb 100644 --- a/libs/services/src/lib/user-data-storage.ts +++ b/libs/services/src/lib/user-data-storage.ts @@ -4,7 +4,8 @@ import { InjectionToken } from '@angular/core'; * The slice of the Web Storage API that PWA user-data stores rely on, plus * the stored keys where the store can list them, and where writes reach a * source later (server mode): `ready`, which waits for a first read in - * flight and tells whether one succeeded, and `flushed`, which sends what + * flight and tells whether one succeeded (`fresh`: reads again, for + * backups that must see the source's current data), and `flushed`, which sends what * is queued and tells whether all of it (of one playlist) was taken. */ export type UserDataStorage = Pick< @@ -12,7 +13,7 @@ export type UserDataStorage = Pick< 'getItem' | 'setItem' | 'removeItem' > & { keys?(): string[]; - ready?(): Promise; + ready?(fresh?: boolean): Promise; flushed?(playlistId?: string): Promise; };