From ef1d20ed632e5f598ab7c3fc3ae37c6e4751636e Mon Sep 17 00:00:00 2001 From: 4gray Date: Sun, 2 Aug 2026 23:21:34 +0200 Subject: [PATCH] fix(downloads): coalesce list refreshes --- CLAUDE.md | 14 ++-- docs/architecture/download-manager.md | 20 ++--- .../src/lib/download-list-load-state.ts | 80 ++++++++++++------- .../src/lib/downloads.service.spec.ts | 67 ++++++++++------ libs/services/src/lib/downloads.service.ts | 25 +++--- 5 files changed, 122 insertions(+), 84 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4010fe167..fc312d877 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -943,10 +943,11 @@ engine` (restart required) or responsible for provider URLs, headers, and metadata; the backend still runs one active transfer with a FIFO queue. `DOWNLOADS_START` remains the sole start IPC. A reserved completed-missing match triggers one authoritative - preflight refresh before provider preparation. A request superseded by a - download-update refresh waits for the winning request to settle, so a - restored Stalker file can become a stable skip without a portal request. The - IPC's stable + preflight refresh before provider preparation. Download-list loads are + serialized as one active IPC plus one coalesced trailing refresh; a preflight + assigned to that trailing refresh cannot be starved by later progress + broadcasts. A restored Stalker file can therefore become a stable skip + without a portal request. The IPC's stable `reason: 'already-in-progress'` and `reason: 'already-downloaded'` results are counted as skipped, and no batch IPC is introduced. The latter comes from an asynchronous main-process filesystem recheck before a @@ -970,8 +971,9 @@ engine` (restart required) or successful snapshot remains authoritative while a later background refresh is in flight; a latest refresh failure leaves loading/empty-state resolution intact but disables starts until another - snapshot succeeds. Superseded download-list callers do not resolve on their - discarded responses; they join the next request that settles as latest. + snapshot succeeds. Overlapping download-list callers join one serialized + trailing refresh, so responses commit in request order and frequent progress + events cannot perpetually postpone a waiting series action. - Episode ownership uses normalized `episode.id` as the canonical `xtreamId` for both providers; Stalker playback identifiers only resolve the URL. Exact `(playlistId, contentType, xtreamId)` matches are authoritative, while diff --git a/docs/architecture/download-manager.md b/docs/architecture/download-manager.md index b8ccaf930..96217a21e 100644 --- a/docs/architecture/download-manager.md +++ b/docs/architecture/download-manager.md @@ -54,11 +54,12 @@ variants, contextual buttons, and theme-aware styling. `DownloadsService.downloads` is the renderer's authoritative **global** list. `loadDownloads()` therefore always invokes the Electron list IPC without its legacy optional playlist scope. Route scope, category, and search must never - replace or narrow that signal. Overlapping loads are request-ordered so a - late response cannot replace a newer snapshot. A load promise whose response - is superseded remains pending until that request or a newer one settles as - latest; callers therefore observe either a committed successor snapshot or - its authoritative failure instead of proceeding after a discarded response. + replace or narrow that signal. Loads are serialized: at most one list IPC is + active, and callers arriving during it coalesce behind one trailing refresh. + Each response therefore commits in request order. A caller assigned to the + trailing refresh resolves after that refresh settles even when later + broadcasts queue the following refresh, preventing both stale continuation + and starvation during frequent progress updates. `hasLoadedDownloads` records that the latest attempt completed, including an error, while `hasAuthoritativeDownloadList` becomes true after a successful request, @@ -80,10 +81,11 @@ variants, contextual buttons, and theme-aware styling. provider URL, request header, and metadata preparation; the coordinator owns only provider-neutral orchestration. When a reserved candidate matches a completed-missing row, the coordinator performs one authoritative preflight - refresh before any provider preparation. If a download-update broadcast - supersedes that request, the preflight waits for the winning refresh to - settle. Restored files therefore become stable skips without requiring a - Stalker URL/network request. Both providers + refresh before any provider preparation. If another list request is active, + the preflight joins the single trailing refresh; later download-update + broadcasts cannot delay that assigned refresh. Restored files therefore + become stable skips without requiring a Stalker URL/network request. Both + providers use normalized `episode.id` as the canonical episode `xtreamId`; Stalker `originalCmd` and `originalId` participate only in URL resolution. diff --git a/libs/services/src/lib/download-list-load-state.ts b/libs/services/src/lib/download-list-load-state.ts index 074e63c0a..0a89ec3b1 100644 --- a/libs/services/src/lib/download-list-load-state.ts +++ b/libs/services/src/lib/download-list-load-state.ts @@ -1,9 +1,15 @@ import { signal } from '@angular/core'; +interface QueuedDownloadListLoad { + readonly operation: () => Promise; + readonly promise: Promise; + readonly resolve: () => void; + readonly reject: (reason?: unknown) => void; +} + export class DownloadListLoadState { - private requestId = 0; - private settledRequestId = 0; - private readonly settlementWaiters = new Map void>(); + private activeLoad?: Promise; + private queuedLoad?: QueuedDownloadListLoad; private readonly loading = signal(false); private readonly loaded = signal(false); private readonly authoritative = signal(false); @@ -12,15 +18,6 @@ export class DownloadListLoadState { readonly hasLoaded = this.loaded.asReadonly(); readonly hasAuthoritativeList = this.authoritative.asReadonly(); - begin(): number { - this.loading.set(true); - return ++this.requestId; - } - - isLatest(requestId: number): boolean { - return requestId === this.requestId; - } - markSucceeded(): void { this.authoritative.set(true); this.loaded.set(true); @@ -31,29 +28,54 @@ export class DownloadListLoadState { this.loaded.set(true); } - async finishOrJoinLatest(requestId: number): Promise { - if (!this.isLatest(requestId)) { - await this.waitForSettlementAtOrAfter(requestId); + run(operation: () => Promise): Promise { + if (!this.activeLoad) { + return this.start(operation); + } + + if (!this.queuedLoad) { + this.queuedLoad = this.createQueuedLoad(operation); + } + return this.queuedLoad.promise; + } + + private start(operation: () => Promise): Promise { + this.loading.set(true); + const activeLoad = operation(); + this.activeLoad = activeLoad; + void activeLoad.then( + () => this.finish(activeLoad), + () => this.finish(activeLoad) + ); + return activeLoad; + } + + private finish(activeLoad: Promise): void { + if (this.activeLoad !== activeLoad) { return; } - this.loading.set(false); - this.settledRequestId = requestId; - for (const [minimumRequestId, resolve] of this.settlementWaiters) { - if (minimumRequestId <= requestId) { - this.settlementWaiters.delete(minimumRequestId); - resolve(); - } + this.activeLoad = undefined; + const queuedLoad = this.queuedLoad; + this.queuedLoad = undefined; + if (!queuedLoad) { + this.loading.set(false); + return; } + + const nextLoad = this.start(queuedLoad.operation); + void nextLoad.then(queuedLoad.resolve, queuedLoad.reject); } - private waitForSettlementAtOrAfter(requestId: number): Promise { - if (this.settledRequestId >= requestId) { - return Promise.resolve(); - } - - return new Promise((resolve) => { - this.settlementWaiters.set(requestId, resolve); + private createQueuedLoad( + operation: () => Promise + ): QueuedDownloadListLoad { + let resolve!: () => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((promiseResolve, promiseReject) => { + resolve = promiseResolve; + reject = promiseReject; }); + return { operation, promise, resolve, reject }; } } diff --git a/libs/services/src/lib/downloads.service.spec.ts b/libs/services/src/lib/downloads.service.spec.ts index d85ed3b24..8595c4d2f 100644 --- a/libs/services/src/lib/downloads.service.spec.ts +++ b/libs/services/src/lib/downloads.service.spec.ts @@ -526,8 +526,8 @@ describe('DownloadsService', () => { ); }); - it('keeps only the latest overlapping download list request result', async () => { - const staleItem = createDownload(1, 'playlist-old'); + it('serializes overlapping requests and finishes with the trailing snapshot', async () => { + const initialItem = createDownload(1, 'playlist-initial'); const latestItem = createDownload(2, 'playlist-new'); const first = createDeferred(); const second = createDeferred(); @@ -544,55 +544,70 @@ describe('DownloadsService', () => { const secondRequest = service.loadDownloads(); expect(service.isLoadingDownloads()).toBe(true); + expect(electron.downloadsGetList).toHaveBeenCalledTimes(1); + + first.resolve([initialItem]); + await firstRequest; + + expect(service.downloads()).toEqual([initialItem]); + expect(service.isLoadingDownloads()).toBe(true); + expect(service.hasLoadedDownloads()).toBe(true); + expect(electron.downloadsGetList).toHaveBeenCalledTimes(2); second.resolve([latestItem]); await secondRequest; - expect(service.downloads()).toEqual([latestItem]); - expect(service.isLoadingDownloads()).toBe(false); - expect(service.hasLoadedDownloads()).toBe(true); - - first.resolve([staleItem]); - await firstRequest; - expect(service.downloads()).toEqual([latestItem]); expect(service.isLoadingDownloads()).toBe(false); expect(electron.downloadsGetList).toHaveBeenNthCalledWith(1); expect(electron.downloadsGetList).toHaveBeenNthCalledWith(2); }); - it('keeps a superseded load pending until a newer request commits its snapshot', async () => { - const staleItem = createDownload(1, 'playlist-old'); - const latestItem = createDownload(2, 'playlist-new'); + it('coalesces update storms without starving a queued refresh caller', async () => { + const initialItem = createDownload(1, 'playlist-initial'); + const preflightItem = createDownload(2, 'playlist-preflight'); + const broadcastItem = createDownload(3, 'playlist-broadcast'); const first = createDeferred(); const second = createDeferred(); + const third = createDeferred(); const electron = { downloadsGetList: jest .fn() .mockReturnValueOnce(first.promise) - .mockReturnValueOnce(second.promise), + .mockReturnValueOnce(second.promise) + .mockReturnValueOnce(third.promise), }; testWindow.electron = electron; const service = createService(); - let supersededLoadSettled = false; + let preflightSettled = false; - const firstRequest = service.loadDownloads().finally(() => { - supersededLoadSettled = true; + const firstRequest = service.loadDownloads(); + const preflightRequest = service.loadDownloads().finally(() => { + preflightSettled = true; }); - const secondRequest = service.loadDownloads(); - first.resolve([staleItem]); - await first.promise; - await Promise.resolve(); + expect(electron.downloadsGetList).toHaveBeenCalledTimes(1); - expect(supersededLoadSettled).toBe(false); - expect(service.downloads()).toEqual([]); + first.resolve([initialItem]); + await firstRequest; - second.resolve([latestItem]); - await Promise.all([firstRequest, secondRequest]); + expect(electron.downloadsGetList).toHaveBeenCalledTimes(2); + expect(preflightSettled).toBe(false); - expect(service.downloads()).toEqual([latestItem]); - expect(supersededLoadSettled).toBe(true); + const broadcastRequest = service.loadDownloads(); + expect(electron.downloadsGetList).toHaveBeenCalledTimes(2); + + second.resolve([preflightItem]); + await preflightRequest; + + expect(preflightSettled).toBe(true); + expect(service.downloads()).toEqual([preflightItem]); + expect(electron.downloadsGetList).toHaveBeenCalledTimes(3); + + third.resolve([broadcastItem]); + await broadcastRequest; + + expect(service.downloads()).toEqual([broadcastItem]); }); it('reports paused content and resumes it by content identity', async () => { diff --git a/libs/services/src/lib/downloads.service.ts b/libs/services/src/lib/downloads.service.ts index 6ce6f74e4..904ca9890 100644 --- a/libs/services/src/lib/downloads.service.ts +++ b/libs/services/src/lib/downloads.service.ts @@ -88,28 +88,25 @@ export class DownloadsService implements OnDestroy { } /** - * Load downloads from the backend. A superseded caller waits until its - * request or a newer one settles as the current snapshot. + * Load downloads from the backend. Overlapping callers coalesce behind one + * serialized trailing refresh. */ async loadDownloads(): Promise { if (!this.isAvailable()) return; - const requestId = this.downloadListLoadState.begin(); - - try { - const list = await window.electron.downloadsGetList(); - if (this.downloadListLoadState.isLatest(requestId)) { + return this.downloadListLoadState.run(async () => { + try { + const list = await window.electron.downloadsGetList(); this.downloads.set(list); this.downloadListLoadState.markSucceeded(); - } - } catch (error) { - console.error('[DownloadsService] Error loading downloads:', error); - if (this.downloadListLoadState.isLatest(requestId)) { + } catch (error) { + console.error( + '[DownloadsService] Error loading downloads:', + error + ); this.downloadListLoadState.markFailed(); } - } finally { - await this.downloadListLoadState.finishOrJoinLatest(requestId); - } + }); } /**