From 5165300981dd3eec979320f917af23924ce187ad Mon Sep 17 00:00:00 2001 From: 4gray Date: Thu, 13 Aug 2026 09:17:22 +0200 Subject: [PATCH] fix(portals): serialize destructive Xtream refreshes per playlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two refresh entry points cannot see each other: the header action tracks one global flag, the sources row tracks its own id set, and neither disables the other's button. Both can be confirmed for the same playlist inside the delete window, and the second run then collects an already-emptied catalog and parks it over the first run's snapshot — `XtreamPendingRestoreService.set()` overwrites unconditionally — so favorites, history, hidden categories and playback positions are lost on re-import. The race predates this PR: on master the two guards are already independent (`isRefreshing()` against the per-row pending sets), so neither entry point could ever have fixed it alone. Extracting the flow is what makes it fixable once. `XtreamRefreshFlowService` now holds the set of playlists with a run in flight and refuses a second one before it resets the guard, releasing it in the same finally that ends the reporter, so a finished run never strands a playlist. Found by Greptile on #1431. Co-Authored-By: Claude Opus 5 --- .../lib/xtream-refresh-flow.service.spec.ts | 63 +++++++++++++++++++ .../ui/src/lib/xtream-refresh-flow.service.ts | 20 +++++- 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/libs/playlist/shared/ui/src/lib/xtream-refresh-flow.service.spec.ts b/libs/playlist/shared/ui/src/lib/xtream-refresh-flow.service.spec.ts index ba436ee96..b3b07a26b 100644 --- a/libs/playlist/shared/ui/src/lib/xtream-refresh-flow.service.spec.ts +++ b/libs/playlist/shared/ui/src/lib/xtream-refresh-flow.service.spec.ts @@ -168,6 +168,69 @@ describe('XtreamRefreshFlowService', () => { expect(dataService.sendIpcEvent).not.toHaveBeenCalled(); }); + it('serializes destructive runs for one playlist across entry points', async () => { + const item = createPlaylistMeta(); + const confirms: Array | undefined> = []; + let deleteStarted!: () => void; + const deleteHasStarted = new Promise((resolve) => { + deleteStarted = resolve; + }); + let releaseDelete!: (state: unknown) => void; + const deleteSettles = new Promise((resolve) => { + releaseDelete = resolve; + }); + + databaseService.deleteXtreamPlaylistContent.mockImplementation(() => { + order.push('delete'); + deleteStarted(); + return deleteSettles; + }); + dialogService.openConfirmDialog.mockImplementation( + ({ onConfirm }: { onConfirm?: () => Promise }) => { + confirms.push(onConfirm?.()); + } + ); + + const header = createReporter(); + const sourcesRow = createReporter(); + + service.confirmAndRefresh(item, header.reporter); + await deleteHasStarted; + + // The row's reporter honestly reports "not busy": it tracks its own id + // set and never saw the header action start. Only the flow can refuse + // this, and it must refuse before the guard reset — a second run would + // park an already-emptied catalog over the first run's snapshot. + service.confirmAndRefresh(item, sourcesRow.reporter); + await Promise.resolve(); + + expect(sourcesRow.calls).toEqual([]); + expect( + databaseService.deleteXtreamPlaylistContent + ).toHaveBeenCalledTimes(1); + expect(dataService.sendIpcEvent).toHaveBeenCalledTimes(1); + + releaseDelete({ + success: true, + favorites: [], + recentlyViewed: [], + hiddenCategories: [], + }); + await Promise.all(confirms); + + expect(header.calls).toEqual(['begin', 'end']); + + // The block lifts with the run: a refresh that already finished must + // not strand the playlist. + service.confirmAndRefresh(item, sourcesRow.reporter); + await confirms[confirms.length - 1]; + + expect(sourcesRow.calls).toEqual(['begin', 'end']); + expect( + databaseService.deleteXtreamPlaylistContent + ).toHaveBeenCalledTimes(2); + }); + it('marks the run busy before the guard reset and any destructive work', async () => { const { reporter, runs } = createReporter({ begin: (run) => { diff --git a/libs/playlist/shared/ui/src/lib/xtream-refresh-flow.service.ts b/libs/playlist/shared/ui/src/lib/xtream-refresh-flow.service.ts index 430a33461..045f799ed 100644 --- a/libs/playlist/shared/ui/src/lib/xtream-refresh-flow.service.ts +++ b/libs/playlist/shared/ui/src/lib/xtream-refresh-flow.service.ts @@ -80,6 +80,19 @@ export class XtreamRefreshFlowService { XtreamPendingRestoreService ); + /** + * Playlists with a destructive run in flight, across every entry point. + * + * A reporter only ever knows its own: the header action tracks one global + * flag, the sources row tracks its own id set, and neither disables the + * other's button. Both can therefore be confirmed for the same playlist + * inside the delete window, and the second run would collect an + * already-emptied catalog and park it over the first run's snapshot — + * losing favorites, history, hidden categories and playback positions on + * re-import. This service is the only place that sees both. + */ + private readonly activeRefreshPlaylistIds = new Set(); + /** * Asks for confirmation, then deletes and re-imports the playlist. Callers * keep their own entry-point guard (a disabled button, a pending row) — @@ -105,7 +118,10 @@ export class XtreamRefreshFlowService { item: PlaylistMeta, reporter: XtreamRefreshProgressReporter ): Promise { - if (reporter.isBusy(item._id)) { + if ( + this.activeRefreshPlaylistIds.has(item._id) || + reporter.isBusy(item._id) + ) { return; } @@ -113,6 +129,7 @@ export class XtreamRefreshFlowService { this.databaseService.createOperationId('xtream-refresh'); const run: XtreamRefreshRun = { playlistId: item._id, operationId }; reporter.begin(run); + this.activeRefreshPlaylistIds.add(item._id); try { // Before anything destructive: this deletes the cached catalog and @@ -186,6 +203,7 @@ export class XtreamRefreshFlowService { ); } } finally { + this.activeRefreshPlaylistIds.delete(item._id); reporter.end(run); } }