fix(portals): serialize destructive Xtream refreshes per playlist

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 <noreply@anthropic.com>
This commit is contained in:
4grayandClaude Opus 5 committed 2026-08-13 09:17:22 +02:00
1 parent e3fc3c8802
commit 5165300981
2 files changed
+82 -1

No files matched your search

@@ -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<Promise<void> | undefined> = [];
let deleteStarted!: () => void;
const deleteHasStarted = new Promise<void>((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<void> }) => {
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) => {
@@ -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<string>();
/**
* 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<void> {
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);
}
}