From d26df7acb449d1650af544dcba9b2d6210da0a91 Mon Sep 17 00:00:00 2001 From: 4gray Date: Sun, 2 Aug 2026 18:38:06 +0200 Subject: [PATCH] fix(downloads): fail closed on stale episode state --- CLAUDE.md | 5 ++- docs/architecture/download-manager.md | 12 ++++--- ...eason-download-coordinator.service.spec.ts | 14 ++++++-- .../season-download-coordinator.service.ts | 1 + .../episode-download-identity.spec.ts | 18 ++++++++-- .../downloads/episode-download-identity.ts | 27 +++++++++++++- .../src/lib/download-list-load-state.ts | 35 +++++++++++++++++++ .../src/lib/downloads.service.spec.ts | 25 +++++++------ libs/services/src/lib/downloads.service.ts | 29 +++++++-------- .../season-container.component.spec.ts | 9 +++-- .../season-download-presenter.ts | 13 ++++--- 11 files changed, 145 insertions(+), 43 deletions(-) create mode 100644 libs/services/src/lib/download-list-load-state.ts diff --git a/CLAUDE.md b/CLAUDE.md index d5a8f7edf..60cad2ab5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -943,7 +943,10 @@ engine` (restart required) or for provider URLs, headers, and metadata; the backend still runs one active transfer with a FIFO queue. `DOWNLOADS_START` remains the sole start IPC; its stable `reason: 'already-in-progress'` result is counted as skipped, and no - batch IPC or schema migration is introduced. + batch IPC or schema migration is introduced. Episode and season download + actions require the latest global list request to have succeeded; a failed + refresh leaves loading/empty-state resolution intact but disables starts + until an authoritative snapshot arrives. - 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 54cbf1bb0..a411155c5 100644 --- a/docs/architecture/download-manager.md +++ b/docs/architecture/download-manager.md @@ -55,10 +55,14 @@ variants, contextual buttons, and theme-aware styling. `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. Before each fresh download or - resume the service asks the main process for an authorized folder and calls - the corresponding IPC command. The `onDownloadsUpdate` broadcast triggers a - new global load. + late response cannot replace a newer snapshot. `hasLoadedDownloads` records + that the latest attempt completed, including an error, while + `hasAuthoritativeDownloadList` is true only after the latest request + succeeds. Series download actions require both so a failed refresh cannot + restart rows missing from a stale or empty renderer snapshot. Before each + fresh download or resume the service asks the main process for an authorized + folder and calls the corresponding IPC command. The `onDownloadsUpdate` + broadcast triggers a new global load. - **Series season queueing** `SeasonDownloadCoordinator` owns synchronous, per-identity pending reservations and submits an individual episode or selected-season snapshot diff --git a/libs/portal/shared/data-access/src/lib/downloads/season-download-coordinator.service.spec.ts b/libs/portal/shared/data-access/src/lib/downloads/season-download-coordinator.service.spec.ts index 4ec11dc6e..d1109cb80 100644 --- a/libs/portal/shared/data-access/src/lib/downloads/season-download-coordinator.service.spec.ts +++ b/libs/portal/shared/data-access/src/lib/downloads/season-download-coordinator.service.spec.ts @@ -12,6 +12,7 @@ import { SeasonDownloadCoordinator } from './season-download-coordinator.service interface DownloadsServiceStub { readonly downloads: WritableSignal; + readonly hasAuthoritativeDownloadList: WritableSignal; readonly hasLoadedDownloads: WritableSignal; readonly isAvailable: WritableSignal; readonly startDownload: jest.MockedFunction< @@ -44,6 +45,7 @@ describe('SeasonDownloadCoordinator', () => { beforeEach(() => { downloadsService = { downloads: signal([]), + hasAuthoritativeDownloadList: signal(true), hasLoadedDownloads: signal(true), isAvailable: signal(true), startDownload: jest.fn().mockResolvedValue({ success: true }), @@ -210,10 +212,18 @@ describe('SeasonDownloadCoordinator', () => { } }); - it('blocks eligibility and reservation until downloads have loaded', async () => { - downloadsService.hasLoadedDownloads.set(false); + it('blocks eligibility and reservation without an authoritative download list', async () => { + downloadsService.hasAuthoritativeDownloadList.set(false); const item = candidate(FIRST_IDENTITY); + expect(downloadsService.hasLoadedDownloads()).toBe(true); + expect(coordinator.isEligible(item)).toBe(false); + await expect(coordinator.enqueueOne(item)).resolves.toBe('skipped'); + expect(item.prepare).not.toHaveBeenCalled(); + + downloadsService.hasAuthoritativeDownloadList.set(true); + downloadsService.hasLoadedDownloads.set(false); + expect(coordinator.isEligible(item)).toBe(false); await expect(coordinator.enqueueOne(item)).resolves.toBe('skipped'); expect(item.prepare).not.toHaveBeenCalled(); diff --git a/libs/portal/shared/data-access/src/lib/downloads/season-download-coordinator.service.ts b/libs/portal/shared/data-access/src/lib/downloads/season-download-coordinator.service.ts index d7f539de1..9f90c173f 100644 --- a/libs/portal/shared/data-access/src/lib/downloads/season-download-coordinator.service.ts +++ b/libs/portal/shared/data-access/src/lib/downloads/season-download-coordinator.service.ts @@ -36,6 +36,7 @@ export class SeasonDownloadCoordinator { isEligible(candidate: EpisodeDownloadCandidate): boolean { return ( this.downloadsService.isAvailable() && + this.downloadsService.hasAuthoritativeDownloadList() && this.downloadsService.hasLoadedDownloads() && !this.isPending(candidate.identity) && isEpisodeDownloadEligible(this.findDownload(candidate.identity)) diff --git a/libs/portal/shared/util/src/lib/downloads/episode-download-identity.spec.ts b/libs/portal/shared/util/src/lib/downloads/episode-download-identity.spec.ts index ad2aadc7e..7dd6a767d 100644 --- a/libs/portal/shared/util/src/lib/downloads/episode-download-identity.spec.ts +++ b/libs/portal/shared/util/src/lib/downloads/episode-download-identity.spec.ts @@ -35,15 +35,27 @@ function row( describe('episode download identity', () => { it('prefers a canonical match over an earlier coordinate fallback', () => { const coordinateFallback = row({ xtreamId: 999 }); - const canonical = row({ + const canonical = row(); + + expect( + findEpisodeDownload(identity, [coordinateFallback, canonical]) + ).toBe(canonical); + }); + + it('fails closed when a canonical match has conflicting complete coordinates', () => { + const coordinateFallback = row({ xtreamId: 999 }); + const conflictingCanonical = row({ seriesXtreamId: 50, seasonNumber: 4, episodeNumber: 8, }); expect( - findEpisodeDownload(identity, [coordinateFallback, canonical]) - ).toBe(canonical); + findEpisodeDownload(identity, [ + coordinateFallback, + conflictingCanonical, + ]) + ).toBeUndefined(); }); it('falls back to complete matching episode coordinates', () => { diff --git a/libs/portal/shared/util/src/lib/downloads/episode-download-identity.ts b/libs/portal/shared/util/src/lib/downloads/episode-download-identity.ts index c5559cff9..16b2297c2 100644 --- a/libs/portal/shared/util/src/lib/downloads/episode-download-identity.ts +++ b/libs/portal/shared/util/src/lib/downloads/episode-download-identity.ts @@ -26,6 +26,29 @@ export interface EpisodeDownloadRecord { readonly filePath?: string; } +function hasConflictingCompleteCoordinates( + download: EpisodeDownloadRecord, + identity: EpisodeDownloadIdentity +): boolean { + const { seriesXtreamId, seasonNumber, episodeNumber } = download; + if ( + seriesXtreamId === undefined || + seasonNumber === undefined || + episodeNumber === undefined + ) { + return false; + } + + return ( + !Number.isSafeInteger(seriesXtreamId) || + !Number.isSafeInteger(seasonNumber) || + !Number.isSafeInteger(episodeNumber) || + seriesXtreamId !== identity.seriesXtreamId || + seasonNumber !== identity.seasonNumber || + episodeNumber !== identity.episodeNumber + ); +} + export function findEpisodeDownload( identity: EpisodeDownloadIdentity, downloads: readonly T[] @@ -37,7 +60,9 @@ export function findEpisodeDownload( download.xtreamId === identity.xtreamId ); if (canonicalMatch) { - return canonicalMatch; + return hasConflictingCompleteCoordinates(canonicalMatch, identity) + ? undefined + : canonicalMatch; } return downloads.find( diff --git a/libs/services/src/lib/download-list-load-state.ts b/libs/services/src/lib/download-list-load-state.ts new file mode 100644 index 000000000..2081fc6ab --- /dev/null +++ b/libs/services/src/lib/download-list-load-state.ts @@ -0,0 +1,35 @@ +import { signal } from '@angular/core'; + +export class DownloadListLoadState { + private requestId = 0; + private readonly loading = signal(false); + private readonly loaded = signal(false); + private readonly authoritative = signal(false); + + readonly isLoading = this.loading.asReadonly(); + readonly hasLoaded = this.loaded.asReadonly(); + readonly hasAuthoritativeList = this.authoritative.asReadonly(); + + begin(): number { + this.loading.set(true); + this.authoritative.set(false); + return ++this.requestId; + } + + isLatest(requestId: number): boolean { + return requestId === this.requestId; + } + + markSucceeded(): void { + this.authoritative.set(true); + this.loaded.set(true); + } + + markFailed(): void { + this.loaded.set(true); + } + + finish(): void { + this.loading.set(false); + } +} diff --git a/libs/services/src/lib/downloads.service.spec.ts b/libs/services/src/lib/downloads.service.spec.ts index a2f57da36..ad705f3f7 100644 --- a/libs/services/src/lib/downloads.service.spec.ts +++ b/libs/services/src/lib/downloads.service.spec.ts @@ -12,6 +12,7 @@ import type { ElectronBridgeDownloadStartPayload, ElectronBridgeDownloadStartResult, } from '@iptvnator/shared/interfaces'; +import { DownloadListLoadState } from './download-list-load-state'; import { DownloadItem, DownloadsService } from './downloads.service'; import { RuntimeCapabilitiesService } from './runtime-capabilities.service'; @@ -20,6 +21,7 @@ type TestDownloadsService = { downloadFolder: WritableSignal; isAvailable: () => boolean; isLoadingDownloads: Signal; + hasAuthoritativeDownloadList: Signal; hasLoadedDownloads: Signal; getDownload: DownloadsService['getDownload']; loadDownloads: DownloadsService['loadDownloads']; @@ -30,9 +32,7 @@ type TestDownloadsService = { selectFolder: DownloadsService['selectFolder']; startDownload: DownloadsService['startDownload']; updateMetadata: DownloadsService['updateMetadata']; - _isLoadingDownloads: WritableSignal; - _hasLoadedDownloads: WritableSignal; - loadDownloadsRequestId: number; + downloadListLoadState: DownloadListLoadState; }; type DownloadsElectronStub = { @@ -117,8 +117,7 @@ describe('DownloadsService', () => { function createService(initialDownloads: DownloadItem[] = []) { const downloads = signal(initialDownloads); - const isLoadingDownloads = signal(false); - const hasLoadedDownloads = signal(false); + const downloadListLoadState = new DownloadListLoadState(); const downloadFolder = signal(''); const service = Object.create( DownloadsService.prototype @@ -128,11 +127,11 @@ describe('DownloadsService', () => { downloads, downloadFolder, isAvailable: () => true, - _isLoadingDownloads: isLoadingDownloads, - isLoadingDownloads: isLoadingDownloads.asReadonly(), - _hasLoadedDownloads: hasLoadedDownloads, - hasLoadedDownloads: hasLoadedDownloads.asReadonly(), - loadDownloadsRequestId: 0, + downloadListLoadState, + isLoadingDownloads: downloadListLoadState.isLoading, + hasAuthoritativeDownloadList: + downloadListLoadState.hasAuthoritativeList, + hasLoadedDownloads: downloadListLoadState.hasLoaded, }); return service; @@ -183,6 +182,7 @@ describe('DownloadsService', () => { const request = service.loadDownloads(); expect(service.isLoadingDownloads()).toBe(true); + expect(service.hasAuthoritativeDownloadList()).toBe(false); expect(service.hasLoadedDownloads()).toBe(false); expect(electron.downloadsGetList).toHaveBeenCalledWith(); @@ -191,6 +191,7 @@ describe('DownloadsService', () => { expect(service.downloads()).toEqual([item]); expect(service.isLoadingDownloads()).toBe(false); + expect(service.hasAuthoritativeDownloadList()).toBe(true); expect(service.hasLoadedDownloads()).toBe(true); }); @@ -498,7 +499,7 @@ describe('DownloadsService', () => { ); }); - it('marks downloads as loaded after a failed request while preserving existing data', async () => { + it('marks a failed request complete while making the preserved list non-authoritative', async () => { const existing = createDownload(1); const error = new Error('download query failed'); jest.spyOn(console, 'error').mockImplementation(() => undefined); @@ -508,11 +509,13 @@ describe('DownloadsService', () => { }), }; const service = createService([existing]); + service.downloadListLoadState.markSucceeded(); await service.loadDownloads(); expect(service.downloads()).toEqual([existing]); expect(service.isLoadingDownloads()).toBe(false); + expect(service.hasAuthoritativeDownloadList()).toBe(false); expect(service.hasLoadedDownloads()).toBe(true); expect(console.error).toHaveBeenCalledWith( '[DownloadsService] Error loading downloads:', diff --git a/libs/services/src/lib/downloads.service.ts b/libs/services/src/lib/downloads.service.ts index fa2a0b215..7bfcf4821 100644 --- a/libs/services/src/lib/downloads.service.ts +++ b/libs/services/src/lib/downloads.service.ts @@ -1,6 +1,7 @@ import { computed, inject, Injectable, OnDestroy, signal } from '@angular/core'; import type { DownloadMetadataSnapshot } from '@iptvnator/shared/interfaces'; import type { ElectronBridgeDownloadStartResult } from '@iptvnator/shared/interfaces'; +import { DownloadListLoadState } from './download-list-load-state'; import { updateDownloadMetadata } from './downloads-metadata-update'; import type { DownloadItem, DownloadStartInput } from './downloads.models'; import { formatDownloadBytes } from './downloads.utils'; @@ -16,19 +17,20 @@ export type { export class DownloadsService implements OnDestroy { private readonly runtime = inject(RuntimeCapabilitiesService); private unsubscribe?: () => void; - private loadDownloadsRequestId = 0; - - private readonly _isLoadingDownloads = signal(false); - private readonly _hasLoadedDownloads = signal(false); + private readonly downloadListLoadState = new DownloadListLoadState(); /** Signal for the list of downloads */ readonly downloads = signal([]); /** Whether the download list is currently being loaded */ - readonly isLoadingDownloads = this._isLoadingDownloads.asReadonly(); + readonly isLoadingDownloads = this.downloadListLoadState.isLoading; /** Whether the first download list request has completed */ - readonly hasLoadedDownloads = this._hasLoadedDownloads.asReadonly(); + readonly hasLoadedDownloads = this.downloadListLoadState.hasLoaded; + + /** Whether the latest download list request completed successfully */ + readonly hasAuthoritativeDownloadList = + this.downloadListLoadState.hasAuthoritativeList; /** Whether the download feature is available (Electron only) */ readonly isAvailable = computed(() => this.runtime.supportsDownloads); @@ -91,23 +93,22 @@ export class DownloadsService implements OnDestroy { async loadDownloads(): Promise { if (!this.isAvailable()) return; - const requestId = ++this.loadDownloadsRequestId; - this._isLoadingDownloads.set(true); + const requestId = this.downloadListLoadState.begin(); try { const list = await window.electron.downloadsGetList(); - if (requestId === this.loadDownloadsRequestId) { + if (this.downloadListLoadState.isLatest(requestId)) { this.downloads.set(list); - this._hasLoadedDownloads.set(true); + this.downloadListLoadState.markSucceeded(); } } catch (error) { console.error('[DownloadsService] Error loading downloads:', error); - if (requestId === this.loadDownloadsRequestId) { - this._hasLoadedDownloads.set(true); + if (this.downloadListLoadState.isLatest(requestId)) { + this.downloadListLoadState.markFailed(); } } finally { - if (requestId === this.loadDownloadsRequestId) { - this._isLoadingDownloads.set(false); + if (this.downloadListLoadState.isLatest(requestId)) { + this.downloadListLoadState.finish(); } } } diff --git a/libs/ui/components/src/lib/season-container/season-container.component.spec.ts b/libs/ui/components/src/lib/season-container/season-container.component.spec.ts index 0d7333b74..08da701c9 100644 --- a/libs/ui/components/src/lib/season-container/season-container.component.spec.ts +++ b/libs/ui/components/src/lib/season-container/season-container.component.spec.ts @@ -27,6 +27,7 @@ import { SeasonContainerComponent } from './season-container.component'; interface DownloadsServiceStub { readonly isAvailable: WritableSignal; + readonly hasAuthoritativeDownloadList: WritableSignal; readonly hasLoadedDownloads: WritableSignal; readonly downloads: WritableSignal; readonly startDownload: jest.MockedFunction< @@ -173,6 +174,7 @@ describe('SeasonContainerComponent', () => { adapter: SeasonEpisodeDownloadAdapter | null = downloadAdapter ) => { downloadsServiceStub.isAvailable.set(true); + downloadsServiceStub.hasAuthoritativeDownloadList.set(true); downloadsServiceStub.hasLoadedDownloads.set(true); fixture.componentRef.setInput('downloadAdapter', adapter); }; @@ -189,6 +191,7 @@ describe('SeasonContainerComponent', () => { localStorage.removeItem('iptvnator_episode_view_mode'); downloadsServiceStub = { isAvailable: signal(false), + hasAuthoritativeDownloadList: signal(false), hasLoadedDownloads: signal(false), downloads: signal([]), startDownload: jest.fn().mockResolvedValue({ success: true }), @@ -637,7 +640,7 @@ describe('SeasonContainerComponent', () => { it('disables the season action until authoritative eligible work exists and while work is in flight', async () => { const first = createEpisode(); enableDownloads(); - downloadsServiceStub.hasLoadedDownloads.set(false); + downloadsServiceStub.hasAuthoritativeDownloadList.set(false); setRequiredInputs({ '1': [first] }); fixture.detectChanges(); @@ -646,8 +649,10 @@ describe('SeasonContainerComponent', () => { '[data-test-id="download-season"]' ) as HTMLButtonElement; expect(seasonButton().disabled).toBe(true); + expect(downloadsServiceStub.hasLoadedDownloads()).toBe(true); + expect(episodeAction(first.id).disabled).toBe(true); - downloadsServiceStub.hasLoadedDownloads.set(true); + downloadsServiceStub.hasAuthoritativeDownloadList.set(true); fixture.componentRef.setInput('isLoading', true); fixture.detectChanges(); expect(seasonButton().disabled).toBe(true); diff --git a/libs/ui/components/src/lib/season-container/season-download-presenter.ts b/libs/ui/components/src/lib/season-container/season-download-presenter.ts index b29fb0262..1958b0380 100644 --- a/libs/ui/components/src/lib/season-container/season-download-presenter.ts +++ b/libs/ui/components/src/lib/season-container/season-download-presenter.ts @@ -118,7 +118,9 @@ export class SeasonDownloadPresenter { const adapter = sources.adapter(); const seasonKey = sources.selectedSeason(); const downloadsAvailable = this.downloadsService.isAvailable(); - const downloadsLoaded = this.downloadsService.hasLoadedDownloads(); + const downloadsReady = + this.downloadsService.hasLoadedDownloads() && + this.downloadsService.hasAuthoritativeDownloadList(); this.downloadsService.downloads(); return sources.selectedEpisodes().map((episode) => { @@ -138,7 +140,7 @@ export class SeasonDownloadPresenter { eligible: Boolean( candidate && downloadsAvailable && - downloadsLoaded && + downloadsReady && !pending && isEpisodeDownloadEligible(download) ), @@ -147,7 +149,7 @@ export class SeasonDownloadPresenter { candidate, download, pending, - downloadsLoaded + downloadsReady ), }; }); @@ -168,6 +170,7 @@ export class SeasonDownloadPresenter { sources.isLoading() || this.batchRunning() || !this.downloadsService.hasLoadedDownloads() || + !this.downloadsService.hasAuthoritativeDownloadList() || !sources.selectedSeason() || this.rows().length === 0 || this.eligibleEpisodeCount() === 0 @@ -278,9 +281,9 @@ export class SeasonDownloadPresenter { candidate: EpisodeDownloadCandidate | null, download: DownloadItem | undefined, pending: boolean, - downloadsLoaded: boolean + downloadsReady: boolean ): EpisodeDownloadPresentation { - if (!candidate || !downloadsLoaded) { + if (!candidate || !downloadsReady) { return EPISODE_DOWNLOAD_STATES.blocked; } if (