From 754c54761a5ab3849958bf376d41bccc3031804a Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Sun, 12 Jul 2026 12:19:15 +0200 Subject: [PATCH] refactor(downloads): consolidate task and metadata flows --- lib/providers/download_provider.dart | 211 +++++++----------- lib/services/download_manager_service.dart | 171 +++++++------- test/providers/download_provider_test.dart | 6 +- .../download_manager_service_test.dart | 55 +++++ 4 files changed, 220 insertions(+), 223 deletions(-) diff --git a/lib/providers/download_provider.dart b/lib/providers/download_provider.dart index fa3ba146..d8db516b 100644 --- a/lib/providers/download_provider.dart +++ b/lib/providers/download_provider.dart @@ -57,6 +57,8 @@ class _RelatedMetadataDownloadContext { final ensuredArtworkKeys = {}; } +typedef _MetadataHydrationResult = ({MediaItem? metadata, bool networkFilled, bool stale}); + /// Provider for managing download state and operations. class DownloadProvider extends ChangeNotifier with DisposableChangeNotifierMixin { final DownloadManagerService _downloadManager; @@ -332,26 +334,7 @@ class DownloadProvider extends ChangeNotifier with DisposableChangeNotifierMixin _artworkPaths[item.globalKey] = DownloadedArtwork(thumbPath: item.thumbPath); - // Look up metadata from the bulk-loaded map (O(1) instead of DB query per item) - // Falls back to individual query for any unpinned entries (e.g., legacy data). - // The fallback dispatches by backend. - final cached = - allMetadata[item.globalKey] ?? - await _downloadManager.lookupMetadata(ServerId(item.serverId), item.ratingKey, preferActiveScope: true); - if (cached != null) { - _metadata[item.globalKey] = cached; - - // For episodes (show/season) and tracks (artist/album), also load - // parent metadata from the same map. - if (cached.isEpisode || cached.kind == MediaKind.track) { - _loadParentMetadataFromMap( - cached, - allMetadata, - clientScopeId: - _downloadManager.activeClientScopeIdForServer(ServerId(item.serverId)) ?? item.clientScopeId, - ); - } - } + await _hydrateDownloadMetadata(item.globalKey, allMetadata, downloadRecord: item); } // Load sync rules from database @@ -391,6 +374,47 @@ class DownloadProvider extends ChangeNotifier with DisposableChangeNotifierMixin _metadataStore.loadParentMetadataFromMap(leaf, allMetadata, clientScopeId: clientScopeId); } + Future<_MetadataHydrationResult> _hydrateDownloadMetadata( + String globalKey, + Map allMetadata, { + DownloadedMediaItem? downloadRecord, + bool fetchOnMiss = false, + bool Function()? isStale, + }) async { + final parsed = parseGlobalKey(globalKey); + if (parsed == null) return (metadata: null, networkFilled: false, stale: false); + + var record = downloadRecord; + if (record == null) { + record = await _downloadManager.getDownloadedMedia(globalKey); + if (isStale?.call() ?? false) return (metadata: null, networkFilled: false, stale: true); + } + + var cached = + allMetadata[globalKey] ?? + await _downloadManager.lookupMetadata(parsed.serverId, parsed.ratingKey, preferActiveScope: true); + if (isStale?.call() ?? false) return (metadata: null, networkFilled: false, stale: true); + + var networkFilled = false; + if (cached == null && fetchOnMiss && _downloads.containsKey(globalKey)) { + cached = await _downloadManager.fetchAndPinMetadata(parsed.serverId, parsed.ratingKey, preferActiveScope: true); + if (isStale?.call() ?? false) return (metadata: null, networkFilled: false, stale: true); + networkFilled = cached != null; + } + + if (cached != null) { + _metadata[globalKey] = cached; + if (cached.isEpisode || cached.kind == MediaKind.track) { + _loadParentMetadataFromMap( + cached, + allMetadata, + clientScopeId: _downloadManager.activeClientScopeIdForServer(parsed.serverId) ?? record?.clientScopeId, + ); + } + } + return (metadata: cached, networkFilled: networkFilled, stale: false); + } + void _onProgressUpdate(DownloadProgress progress) { appLogger.d('Progress update received: ${progress.globalKey} - ${progress.status} - ${progress.progress}%'); @@ -896,48 +920,20 @@ class DownloadProvider extends ChangeNotifier with DisposableChangeNotifierMixin final queued = await _queueSingleDownload(metadata, client, mediaIndex: config.mediaIndex); return queued ? 1 : 0; } else if (metadata.kind == MediaKind.album || metadata.kind == MediaKind.artist) { - final hadMetadata = _metadata.containsKey(globalKey); - _metadata[globalKey] = metadata; - try { - return await _queueMusicContainerDownload(metadata, client); - } catch (_) { - if (!hadMetadata) _metadata.remove(globalKey); - rethrow; - } - } else if (metadata.isShow) { - // Stash metadata pre-queue so the UI can render the queueing state; - // roll back if expansion throws so the orphan doesn't linger. - final hadMetadata = _metadata.containsKey(globalKey); - _metadata[globalKey] = metadata; - try { - return await _queueShowDownload( - metadata, - client, + return _withStashedMetadata(metadata, () => _queueMusicContainerDownload(metadata, client)); + } else if (metadata.isShow || metadata.isSeason) { + return _withStashedMetadata( + metadata, + () => _expandAndQueue( + container: metadata, + client: client, versionConfig: config, filter: filter, maxCount: maxCount, + skipExisting: false, includeSpecials: includeSpecials, - ); - } catch (_) { - if (!hadMetadata) _metadata.remove(globalKey); - rethrow; - } - } else if (metadata.isSeason) { - final hadMetadata = _metadata.containsKey(globalKey); - _metadata[globalKey] = metadata; - try { - return await _queueSeasonDownload( - metadata, - client, - versionConfig: config, - filter: filter, - maxCount: maxCount, - includeSpecials: includeSpecials, - ); - } catch (_) { - if (!hadMetadata) _metadata.remove(globalKey); - rethrow; - } + ), + ); } else { throw Exception('Cannot download ${metadata.kind.id}'); } @@ -947,6 +943,22 @@ class DownloadProvider extends ChangeNotifier with DisposableChangeNotifierMixin } } + Future _withStashedMetadata(MediaItem metadata, Future Function() operation) async { + final globalKey = metadata.globalKey; + final previous = _metadata[globalKey]; + _metadata[globalKey] = metadata; + try { + return await operation(); + } catch (_) { + if (previous == null) { + _metadata.remove(globalKey); + } else { + _metadata[globalKey] = previous; + } + rethrow; + } + } + /// Queue every playable item from a collection/playlist for download. /// /// Movies, episodes, and tracks are queued directly. Shows and seasons are @@ -1175,46 +1187,6 @@ class DownloadProvider extends ChangeNotifier with DisposableChangeNotifierMixin return count; } - /// Queue all episodes from a TV show for download - Future _queueShowDownload( - MediaItem show, - MediaServerClient client, { - DownloadVersionConfig? versionConfig, - DownloadFilter filter = DownloadFilter.all, - int? maxCount, - bool includeSpecials = true, - }) async { - return _expandAndQueue( - container: show, - client: client, - versionConfig: versionConfig, - filter: filter, - maxCount: maxCount, - skipExisting: false, - includeSpecials: includeSpecials, - ); - } - - /// Queue all episodes from a season for download - Future _queueSeasonDownload( - MediaItem season, - MediaServerClient client, { - DownloadVersionConfig? versionConfig, - DownloadFilter filter = DownloadFilter.all, - int? maxCount, - bool includeSpecials = true, - }) async { - return _expandAndQueue( - container: season, - client: client, - versionConfig: versionConfig, - filter: filter, - maxCount: maxCount, - skipExisting: false, - includeSpecials: includeSpecials, - ); - } - /// Queue only the missing (not downloaded) episodes for a show/season. /// Used for resuming partial downloads. Returns the number of episodes queued. Future queueMissingEpisodes( @@ -1476,44 +1448,15 @@ class DownloadProvider extends ChangeNotifier with DisposableChangeNotifierMixin int misses = 0; for (final globalKey in keys) { - final parsed = parseGlobalKey(globalKey); - if (parsed == null) continue; - try { - final downloadRecord = await _downloadManager.getDownloadedMedia(globalKey); - if (isStale()) return; - var cached = - allMetadata[globalKey] ?? - await _downloadManager.lookupMetadata(parsed.serverId, parsed.ratingKey, preferActiveScope: true); - if (isStale()) return; - if (cached != null) { - cacheHits++; - } else if (_downloads.containsKey(globalKey)) { - // Cache miss for an item we know is downloaded — pull from the - // live server. Repairs profiles where the per-backend cache row - // was never written or got cleared, the case that produces - // empty-title sync rules and a missing-downloads list. - cached = await _downloadManager.fetchAndPinMetadata( - parsed.serverId, - parsed.ratingKey, - preferActiveScope: true, - ); - if (isStale()) return; - if (cached != null) networkFills++; - } - - if (cached != null) { - _metadata[globalKey] = cached; - if (cached.isEpisode || cached.kind == MediaKind.track) { - _loadParentMetadataFromMap( - cached, - allMetadata, - clientScopeId: - _downloadManager.activeClientScopeIdForServer(parsed.serverId) ?? downloadRecord?.clientScopeId, - ); - } - } else { + final result = await _hydrateDownloadMetadata(globalKey, allMetadata, fetchOnMiss: true, isStale: isStale); + if (result.stale) return; + if (result.metadata == null) { misses++; + } else if (result.networkFilled) { + networkFills++; + } else { + cacheHits++; } } catch (e) { appLogger.d('Failed to refresh metadata for $globalKey: $e'); diff --git a/lib/services/download_manager_service.dart b/lib/services/download_manager_service.dart index db661f0a..553d6781 100644 --- a/lib/services/download_manager_service.dart +++ b/lib/services/download_manager_service.dart @@ -39,6 +39,22 @@ typedef _NativeTaskForId = Future Function(String taskId); typedef _NativeResumeTask = Future Function(DownloadTask task); typedef _EpisodeStorageDeletion = ({String? seasonDirUri, String? showDirUri}); +typedef NativeTaskPartition = ({List current, List stale}); + +@visibleForTesting +NativeTaskPartition partitionNativeTasks(Iterable tasks, String? currentTaskId) { + final current = []; + final stale = []; + for (final task in tasks) { + if (currentTaskId != null && task.taskId == currentTaskId) { + current.add(task); + } else { + stale.add(task); + } + } + return (current: current, stale: stale); +} + const bool _tvosBuild = bool.fromEnvironment('TVOS_BUILD'); class _DownloadContext { @@ -593,22 +609,27 @@ class DownloadManagerService { } } - Future _reconcileDownloadingNativeTasks(DownloadedMediaItem row, List tasks) async { - final currentTaskId = row.bgTaskId; - final matchingCurrentTasks = currentTaskId == null - ? const [] - : tasks.where((task) => task.taskId == currentTaskId).toList(growable: false); - if (matchingCurrentTasks.length == 1) { - await _cancelNativeTaskIds( - row.globalKey, - tasks.where((task) => task.taskId != currentTaskId).map((task) => task.taskId), - reason: 'duplicate downloading task during recovery', - ); - return; - } + Future _retainUniqueCurrentNativeTask( + DownloadedMediaItem row, + List tasks, { + required String statusLabel, + }) async { + final partition = partitionNativeTasks(tasks, row.bgTaskId); + if (partition.current.length != 1) return partition.current.length; + await _cancelNativeTaskIds( + row.globalKey, + partition.stale.map((task) => task.taskId), + reason: 'duplicate $statusLabel task during recovery', + ); + return 1; + } - if (matchingCurrentTasks.length > 1) { - appLogger.w('Multiple native tasks share current task id $currentTaskId for ${row.globalKey}; re-queueing'); + Future _reconcileDownloadingNativeTasks(DownloadedMediaItem row, List tasks) async { + final currentMatchCount = await _retainUniqueCurrentNativeTask(row, tasks, statusLabel: 'downloading'); + if (currentMatchCount == 1) return; + + if (currentMatchCount > 1) { + appLogger.w('Multiple native tasks share current task id ${row.bgTaskId} for ${row.globalKey}; re-queueing'); await _cancelNativeTaskIds( row.globalKey, tasks.map((task) => task.taskId), @@ -640,22 +661,12 @@ class DownloadManagerService { } Future _reconcilePausedNativeTasks(DownloadedMediaItem row, List tasks) async { - final currentTaskId = row.bgTaskId; - final matchingCurrentTasks = currentTaskId == null - ? const [] - : tasks.where((task) => task.taskId == currentTaskId).toList(growable: false); - if (matchingCurrentTasks.length == 1) { - await _cancelNativeTaskIds( - row.globalKey, - tasks.where((task) => task.taskId != currentTaskId).map((task) => task.taskId), - reason: 'duplicate paused task during recovery', - ); - return; - } + final currentMatchCount = await _retainUniqueCurrentNativeTask(row, tasks, statusLabel: 'paused'); + if (currentMatchCount == 1) return; - if (matchingCurrentTasks.length > 1) { + if (currentMatchCount > 1) { appLogger.w( - 'Multiple paused native tasks share current task id $currentTaskId for ${row.globalKey}; clearing task', + 'Multiple paused native tasks share current task id ${row.bgTaskId} for ${row.globalKey}; clearing task', ); } @@ -1090,6 +1101,13 @@ class DownloadManagerService { await _database.updateDownloadProgress(globalKey, 0, 0, 0); } + Future _requeueDownload(String globalKey, {MediaServerClient? fallbackClient}) async { + await _transitionStatus(globalKey, DownloadStatus.queued); + await _database.addToQueue(mediaGlobalKey: globalKey); + final client = await _getClientForDownloadKey(globalKey) ?? fallbackClient; + if (client != null) unawaited(_processQueue(client)); + } + Future _cancelNativeTask(String globalKey, String taskId, {required String reason}) async { if (!downloadsSupported || taskId.isEmpty) return; try { @@ -1140,14 +1158,16 @@ class DownloadManagerService { String taskId, { required String event, bool cancelStale = false, + DownloadStatus? requiredStatus, }) async { final existing = await _database.getDownloadedMedia(globalKey); final currentTaskId = existing?.bgTaskId; - if (existing != null && currentTaskId == taskId) return existing; + final statusMatches = requiredStatus == null || existing?.status == requiredStatus.index; + if (existing != null && currentTaskId == taskId && statusMatches) return existing; appLogger.d( 'Ignoring stale download $event for $globalKey from task $taskId ' - '(current task: ${currentTaskId ?? 'none'})', + '(current task: ${currentTaskId ?? 'none'}, status: ${existing?.status ?? 'none'})', ); if (cancelStale) { await _cancelNativeTask(globalKey, taskId, reason: 'stale $event'); @@ -1489,17 +1509,10 @@ class DownloadManagerService { update.task.taskId, event: 'status ${update.status}', cancelStale: _isNativeTaskActiveStatus(update.status), + requiredStatus: DownloadStatus.downloading, ); if (existing == null) return; - if (existing.status != DownloadStatus.downloading.index) { - appLogger.d('Ignoring ${update.status} for inactive download $globalKey from task ${update.task.taskId}'); - if (_isNativeTaskActiveStatus(update.status)) { - await _cancelNativeTask(globalKey, update.task.taskId, reason: 'status for inactive download'); - } - return; - } - try { switch (update.status) { case TaskStatus.complete: @@ -1535,24 +1548,20 @@ class DownloadManagerService { Future _onDownloadCanceled(String globalKey, String taskId) async { if (_completingKeys.contains(globalKey)) return; - final existing = await _database.getDownloadedMedia(globalKey); - if (existing == null || - existing.bgTaskId != taskId || - existing.status != DownloadStatus.downloading.index || - existing.status == DownloadStatus.completed.index || - existing.status == DownloadStatus.cancelled.index) { - return; - } + final existing = await _downloadForCurrentTaskSession( + globalKey, + taskId, + event: 'system cancellation', + requiredStatus: DownloadStatus.downloading, + ); + if (existing == null) return; _cancelDownloadTimers(globalKey); _pendingDownloadContext.remove(globalKey); appLogger.w('Download cancelled by system for $globalKey, re-queuing'); await _database.updateBgTaskId(globalKey, null); - await _transitionStatus(globalKey, DownloadStatus.queued); - await _database.addToQueue(mediaGlobalKey: globalKey); - final client = await _getClientForDownloadKey(globalKey); - if (client != null) unawaited(_processQueue(client)); + await _requeueDownload(globalKey); } /// Handle a failed download — auto-retry if retries remain, otherwise permanently fail. @@ -1567,15 +1576,13 @@ class DownloadManagerService { return; } - final existing = await _database.getDownloadedMedia(globalKey); - if (existing == null || - existing.bgTaskId != taskId || - existing.status != DownloadStatus.downloading.index || - existing.status == DownloadStatus.completed.index || - existing.status == DownloadStatus.cancelled.index) { - appLogger.d('Ignoring stale failure for inactive download $globalKey'); - return; - } + final existing = await _downloadForCurrentTaskSession( + globalKey, + taskId, + event: 'failure', + requiredStatus: DownloadStatus.downloading, + ); + if (existing == null) return; _cancelDownloadTimers(globalKey); _pendingDownloadContext.remove(globalKey); final retryCount = existing.retryCount; @@ -1629,15 +1636,13 @@ class DownloadManagerService { return; } - final existing = await _database.getDownloadedMedia(globalKey); - if (existing == null || - existing.bgTaskId != taskId || - existing.status != DownloadStatus.downloading.index || - existing.status == DownloadStatus.completed.index || - existing.status == DownloadStatus.cancelled.index) { - appLogger.d('Ignoring stale permanent failure for inactive download $globalKey'); - return; - } + final existing = await _downloadForCurrentTaskSession( + globalKey, + taskId, + event: 'permanent failure', + requiredStatus: DownloadStatus.downloading, + ); + if (existing == null) return; _cancelDownloadTimers(globalKey); _pendingDownloadContext.remove(globalKey); @@ -1668,9 +1673,7 @@ class DownloadManagerService { appLogger.i('Auto-retrying download for $globalKey'); await _cleanupStaleDownload(globalKey); - await _transitionStatus(globalKey, DownloadStatus.queued); - await _database.addToQueue(mediaGlobalKey: globalKey); - unawaited(_processQueue(client)); + await _requeueDownload(globalKey, fallbackClient: client); } /// Handle a completed video download — store path, download supplementary content, mark done. @@ -1684,18 +1687,16 @@ class DownloadManagerService { _completingKeys.add(globalKey); try { // Fresh DB check — bail if already completed (guards against race with orphan scan) - final existingCheck = await _database.getDownloadedMedia(globalKey); + final existingCheck = await _downloadForCurrentTaskSession( + globalKey, + task.taskId, + event: 'completion', + requiredStatus: DownloadStatus.downloading, + ); if (_cancellingKeys.contains(globalKey) || existingCheck == null) { appLogger.d('Download no longer active for $globalKey, skipping completion'); return; } - if (existingCheck.bgTaskId != task.taskId || existingCheck.status != DownloadStatus.downloading.index) { - appLogger.d( - 'Ignoring stale completion for $globalKey from task ${task.taskId} ' - '(current task: ${existingCheck.bgTaskId ?? 'none'}, status: ${existingCheck.status})', - ); - return; - } // Flush any pending debounced progress write + cancel any scheduled retry. _cancelDownloadTimers(globalKey); @@ -2071,10 +2072,7 @@ class DownloadManagerService { // Native resume failed or not supported (SAF mode) — re-enqueue from scratch await _cleanupStaleDownload(globalKey); - await _transitionStatus(globalKey, DownloadStatus.queued); - await _database.addToQueue(mediaGlobalKey: globalKey); - final resolvedClient = await _getClientForDownloadKey(globalKey) ?? client; - unawaited(_processQueue(resolvedClient)); + await _requeueDownload(globalKey, fallbackClient: client); } Future _tryResumeNativeTask( @@ -2121,10 +2119,7 @@ class DownloadManagerService { _autoRetryTimers.remove(globalKey)?.cancel(); await _cleanupStaleDownload(globalKey); await _database.clearDownloadError(globalKey); - await _transitionStatus(globalKey, DownloadStatus.queued); - await _database.addToQueue(mediaGlobalKey: globalKey); - final resolvedClient = await _getClientForDownloadKey(globalKey) ?? client; - unawaited(_processQueue(resolvedClient)); + await _requeueDownload(globalKey, fallbackClient: client); } /// Cancel a download diff --git a/test/providers/download_provider_test.dart b/test/providers/download_provider_test.dart index a9cfe688..fdecad90 100644 --- a/test/providers/download_provider_test.dart +++ b/test/providers/download_provider_test.dart @@ -1452,7 +1452,11 @@ void main() { await expectLater(p.queueDownload(season, _ThrowingClient()), throwsA(isA())); - expect(p.getMetadata('srv:7'), isNotNull, reason: 'pre-existing metadata must survive rollback'); + expect( + p.getMetadata('srv:7')?.title, + 'Original Title', + reason: 'queue rollback must restore the previous value, not leave the temporary stash', + ); p.dispose(); }); diff --git a/test/services/download_manager_service_test.dart b/test/services/download_manager_service_test.dart index 71d9a556..5d95abc7 100644 --- a/test/services/download_manager_service_test.dart +++ b/test/services/download_manager_service_test.dart @@ -47,6 +47,30 @@ void main() { }); }); + group('partitionNativeTasks', () { + test('separates the current task id from stale and duplicate tasks', () { + final tasks = [ + _downloadTask('current', 'srv:item-1'), + _downloadTask('stale', 'srv:item-1'), + _downloadTask('current', 'srv:item-1'), + ]; + + final partition = partitionNativeTasks(tasks, 'current'); + + expect(partition.current.map((task) => task.taskId), ['current', 'current']); + expect(partition.stale.map((task) => task.taskId), ['stale']); + }); + + test('treats every native task as stale when the row has no task id', () { + final tasks = [_downloadTask('first', 'srv:item-1'), _downloadTask('second', 'srv:item-1')]; + + final partition = partitionNativeTasks(tasks, null); + + expect(partition.current, isEmpty); + expect(partition.stale, tasks); + }); + }); + group('artworkStorageKey', () { test('removes Jellyfin api_key from persisted artwork keys', () { final url = 'https://jf.example/Items/item-1/Images/Primary?tag=abc&api_key=secret-token'; @@ -443,6 +467,37 @@ void main() { expect(row?.bgTaskId, 'current-task'); }); + test('ignores terminal status when the current row is no longer downloading', () async { + final db = AppDatabase.forTesting(NativeDatabase.memory()); + addTearDown(db.close); + const globalKey = 'srv:item-1'; + await db.insertDownload( + serverId: ServerId('srv'), + ratingKey: 'item-1', + globalKey: globalKey, + type: 'movie', + status: DownloadStatus.completed.index, + ); + await db.updateBgTaskId(globalKey, 'current-task'); + + final manager = DownloadManagerService( + database: db, + storageService: DownloadStorageService.instance, + clientResolver: (serverId, {clientScopeId}) => null, + downloadsSupportedOverride: false, + ); + addTearDown(manager.dispose); + + await manager.debugHandleTaskStatus( + TaskStatusUpdate(_downloadTask('current-task', globalKey), TaskStatus.canceled), + ); + + final row = await db.getDownloadedMedia(globalKey); + expect(row?.status, DownloadStatus.completed.index); + expect(row?.bgTaskId, 'current-task'); + expect(await db.getNextQueueItem(), isNull); + }); + test('requeues current system cancel without in-memory context', () async { final db = AppDatabase.forTesting(NativeDatabase.memory()); addTearDown(db.close);