From a101ee76d68b92bef7de6ac72377dd1b6bc06437 Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Tue, 26 May 2026 20:33:33 +0200 Subject: [PATCH] fix(downloads): prevent duplicate native tasks close #1141 --- lib/providers/download_provider.dart | 3 +- lib/services/download_manager_service.dart | 536 ++++++++++++++---- lib/services/sync_rule_executor.dart | 3 +- pubspec.lock | 4 +- pubspec.yaml | 2 +- test/providers/download_provider_test.dart | 16 + .../download_manager_service_test.dart | 174 ++++++ 7 files changed, 626 insertions(+), 112 deletions(-) diff --git a/lib/providers/download_provider.dart b/lib/providers/download_provider.dart index c3c6a446..9121906a 100644 --- a/lib/providers/download_provider.dart +++ b/lib/providers/download_provider.dart @@ -1006,7 +1006,8 @@ class DownloadProvider extends ChangeNotifier with DisposableChangeNotifierMixin final existing = _downloads[globalKey]!; if (existing.status == DownloadStatus.downloading || existing.status == DownloadStatus.completed || - existing.status == DownloadStatus.queued) { + existing.status == DownloadStatus.queued || + existing.status == DownloadStatus.paused) { final claimed = await _claimDownloadForActiveProfile(globalKey); if (claimed) safeNotifyListeners(); return claimed; diff --git a/lib/services/download_manager_service.dart b/lib/services/download_manager_service.dart index 066d4f00..c3ff1bea 100644 --- a/lib/services/download_manager_service.dart +++ b/lib/services/download_manager_service.dart @@ -32,6 +32,8 @@ import '../utils/global_key_utils.dart'; import 'package:sentry_flutter/sentry_flutter.dart'; typedef MediaClientResolver = MediaServerClient? Function(String serverId, {String? clientScopeId}); +typedef _NativeTaskForId = Future Function(String taskId); +typedef _NativeResumeTask = Future Function(DownloadTask task); const bool _tvosBuild = bool.fromEnvironment('TVOS_BUILD'); @@ -449,6 +451,8 @@ class DownloadManagerService { appLogger.i('Rescheduled ${rescheduled.length} killed download task(s)'); } + await _reconcileNativeDownloadTasks(); + // One-time migration: normalize stored file paths that may contain a // doubled base-dir prefix from an earlier bug in the recovery callback. // Re-run on v2 to also fix paths without a leading / that the v1 migration missed. @@ -528,6 +532,156 @@ class DownloadManagerService { } } + Future _reconcileNativeDownloadTasks() async { + if (!downloadsSupported || !_fileDownloaderInitialized) return; + + final List nativeTasks; + try { + nativeTasks = await FileDownloader().allTasks(group: _downloadGroup); + } catch (e) { + appLogger.w('Failed to enumerate native download tasks during recovery', error: e); + return; + } + if (nativeTasks.isEmpty) return; + + final tasksByGlobalKey = >{}; + for (final task in nativeTasks) { + final globalKey = task.metaData; + if (globalKey.isEmpty) continue; + (tasksByGlobalKey[globalKey] ??= []).add(task); + } + if (tasksByGlobalKey.isEmpty) return; + + final rows = await _database.select(_database.downloadedMedia).get(); + final rowsByGlobalKey = {for (final row in rows) row.globalKey: row}; + + for (final entry in tasksByGlobalKey.entries) { + final globalKey = entry.key; + final tasks = entry.value; + final row = rowsByGlobalKey[globalKey]; + if (row == null) { + await _cancelNativeTaskIds( + globalKey, + tasks.map((task) => task.taskId), + reason: 'no download row during recovery', + ); + continue; + } + + switch (DownloadStatus.values[row.status]) { + case DownloadStatus.downloading: + await _reconcileDownloadingNativeTasks(row, tasks); + case DownloadStatus.paused: + await _reconcilePausedNativeTasks(row, tasks); + case DownloadStatus.queued: + await _cancelNativeTaskIds( + globalKey, + tasks.map((task) => task.taskId), + reason: 'queued download during recovery', + ); + await _database.addToQueue(mediaGlobalKey: globalKey); + case DownloadStatus.completed: + case DownloadStatus.failed: + case DownloadStatus.cancelled: + case DownloadStatus.partial: + await _cancelNativeTaskIds( + globalKey, + tasks.map((task) => task.taskId), + reason: 'download status ${DownloadStatus.values[row.status]} during recovery', + ); + } + } + } + + 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; + } + + if (matchingCurrentTasks.length > 1) { + appLogger.w('Multiple native tasks share current task id $currentTaskId for ${row.globalKey}; re-queueing'); + await _cancelNativeTaskIds( + row.globalKey, + tasks.map((task) => task.taskId), + reason: 'duplicate current task id during recovery', + ); + await _database.updateBgTaskId(row.globalKey, null); + await _database.updateDownloadProgress(row.globalKey, 0, 0, 0); + await _transitionStatus(row.globalKey, DownloadStatus.queued); + await _database.addToQueue(mediaGlobalKey: row.globalKey); + return; + } + + if (tasks.length == 1) { + final taskId = tasks.single.taskId; + appLogger.i('Adopting recovered native task $taskId for ${row.globalKey}'); + await _database.updateBgTaskId(row.globalKey, taskId); + return; + } + + await _cancelNativeTaskIds( + row.globalKey, + tasks.map((task) => task.taskId), + reason: 'ambiguous downloading tasks during recovery', + ); + await _database.updateBgTaskId(row.globalKey, null); + await _database.updateDownloadProgress(row.globalKey, 0, 0, 0); + await _transitionStatus(row.globalKey, DownloadStatus.queued); + await _database.addToQueue(mediaGlobalKey: row.globalKey); + } + + 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; + } + + if (matchingCurrentTasks.length > 1) { + appLogger.w( + 'Multiple paused native tasks share current task id $currentTaskId for ${row.globalKey}; clearing task', + ); + } + + await _cancelNativeTaskIds( + row.globalKey, + tasks.map((task) => task.taskId), + reason: 'unexpected paused native tasks during recovery', + ); + await _database.updateBgTaskId(row.globalKey, null); + } + + Future _cancelNativeTaskIds(String globalKey, Iterable taskIds, {required String reason}) async { + if (!downloadsSupported) return; + final ids = taskIds.where((id) => id.isNotEmpty).toSet(); + if (ids.isEmpty) return; + + try { + final cancelled = await FileDownloader().cancelTasksWithIds(ids); + if (cancelled) { + appLogger.d('Cancelled ${ids.length} native task(s) for $globalKey ($reason): ${ids.join(', ')}'); + } + } catch (e) { + appLogger.w('Failed to cancel native tasks for $globalKey ($reason): ${ids.join(', ')}', error: e); + } + } + /// Resume queued downloads that have no active processing. /// Call after a [MediaServerClient] becomes available (e.g. after server connect on launch). void resumeQueuedDownloads(MediaServerClient client) { @@ -841,10 +995,24 @@ class DownloadManagerService { final globalKey = metadata.globalKey; final existing = await _database.getDownloadedMedia(globalKey); - if (existing != null && - (existing.status == DownloadStatus.downloading.index || existing.status == DownloadStatus.completed.index)) { - appLogger.i('Download already exists for $globalKey with status ${existing.status}'); - return; + if (existing != null) { + if (existing.status == DownloadStatus.queued.index) { + await _database.addToQueue( + mediaGlobalKey: globalKey, + priority: priority, + downloadSubtitles: downloadSubtitles, + downloadArtwork: downloadArtwork, + ); + _emitProgress(globalKey, DownloadStatus.queued, 0); + unawaited(_processQueue(client)); + return; + } + if (existing.status == DownloadStatus.downloading.index || + existing.status == DownloadStatus.paused.index || + existing.status == DownloadStatus.completed.index) { + appLogger.i('Download already exists for $globalKey with status ${existing.status}'); + return; + } } await _database.insertDownload( @@ -917,16 +1085,85 @@ class DownloadManagerService { /// Cancel any lingering background task and reset progress before re-enqueuing. Future _cleanupStaleDownload(String globalKey) async { final existingTaskId = await _database.getBgTaskId(globalKey); - if (existingTaskId != null) { - if (downloadsSupported) { - await FileDownloader().cancelTaskWithId(existingTaskId); - } - await _database.updateBgTaskId(globalKey, null); - appLogger.d('Cancelled stale bg task $existingTaskId for $globalKey'); - } + await _database.updateBgTaskId(globalKey, null); + _pendingDownloadContext.remove(globalKey); + await _cancelNativeTasksForGlobalKey( + globalKey, + includeTaskId: existingTaskId, + reason: 'stale task before re-download', + ); await _database.updateDownloadProgress(globalKey, 0, 0, 0); } + Future _cancelNativeTask(String globalKey, String taskId, {required String reason}) async { + if (!downloadsSupported || taskId.isEmpty) return; + try { + final cancelled = await FileDownloader().cancelTaskWithId(taskId); + if (cancelled) { + appLogger.d('Cancelled native task $taskId for $globalKey ($reason)'); + } + } catch (e) { + appLogger.w('Failed to cancel native task $taskId for $globalKey ($reason)', error: e); + } + } + + Future _cancelNativeTasksForGlobalKey( + String globalKey, { + String? includeTaskId, + String? exceptTaskId, + required String reason, + }) async { + if (!downloadsSupported) return; + final taskIds = {}; + if (includeTaskId != null && includeTaskId != exceptTaskId) taskIds.add(includeTaskId); + + if (!_fileDownloaderInitialized && taskIds.isEmpty) return; + + try { + final nativeTasks = await FileDownloader().allTasks(group: _downloadGroup); + for (final task in nativeTasks) { + if (task.metaData == globalKey && task.taskId != exceptTaskId) taskIds.add(task.taskId); + } + } catch (e) { + appLogger.w('Failed to enumerate native tasks for $globalKey ($reason)', error: e); + } + + if (taskIds.isEmpty) return; + + try { + final cancelled = await FileDownloader().cancelTasksWithIds(taskIds); + if (cancelled) { + appLogger.d('Cancelled ${taskIds.length} native task(s) for $globalKey ($reason): ${taskIds.join(', ')}'); + } + } catch (e) { + appLogger.w('Failed to cancel native tasks for $globalKey ($reason): ${taskIds.join(', ')}', error: e); + } + } + + Future _downloadForCurrentTaskSession( + String globalKey, + String taskId, { + required String event, + bool cancelStale = false, + }) async { + final existing = await _database.getDownloadedMedia(globalKey); + final currentTaskId = existing?.bgTaskId; + if (existing != null && currentTaskId == taskId) return existing; + + appLogger.d( + 'Ignoring stale download $event for $globalKey from task $taskId ' + '(current task: ${currentTaskId ?? 'none'})', + ); + if (cancelStale) { + await _cancelNativeTask(globalKey, taskId, reason: 'stale $event'); + } + return null; + } + + bool _isNativeTaskActiveStatus(TaskStatus status) { + return status == TaskStatus.enqueued || status == TaskStatus.running || status == TaskStatus.waitingToRetry; + } + Future _isInactiveForEnqueue(String globalKey) async { if (_cancellingKeys.contains(globalKey)) return true; final existing = await _database.getDownloadedMedia(globalKey); @@ -975,7 +1212,7 @@ class DownloadManagerService { } appLogger.i('Preparing download for $globalKey'); - if (existing.bgTaskId != null) await _cleanupStaleDownload(globalKey); + await _cleanupStaleDownload(globalKey); if (await _isInactiveForEnqueue(globalKey)) { appLogger.d('Skipping enqueue for $globalKey: inactive before transition'); await _database.removeFromQueue(globalKey); @@ -1068,7 +1305,7 @@ class DownloadManagerService { updates: Updates.statusAndProgress, requiresWiFi: requiresWiFi, retries: _nativeRetries, - allowPause: true, + allowPause: false, metaData: globalKey, displayName: displayName, ); @@ -1157,17 +1394,42 @@ class DownloadManagerService { /// Callback: background_downloader progress update void _onTaskProgress(TaskProgressUpdate update) { + if (_disposed) return; + unawaited( + _handleTaskProgress(update).catchError((Object e, StackTrace st) { + appLogger.e('Error handling download progress for ${update.task.metaData}', error: e, stackTrace: st); + }), + ); + } + + @visibleForTesting + Future debugHandleTaskProgress(TaskProgressUpdate update) => _handleTaskProgress(update); + + Future _handleTaskProgress(TaskProgressUpdate update) async { if (_disposed) return; final globalKey = update.task.metaData; if (globalKey.isEmpty || update.progress < 0) return; + final existing = await _downloadForCurrentTaskSession( + globalKey, + update.task.taskId, + event: 'progress', + cancelStale: true, + ); + if (existing == null) return; + if (existing.status != DownloadStatus.downloading.index) { + appLogger.d('Ignoring progress for inactive download $globalKey from task ${update.task.taskId}'); + await _cancelNativeTask(globalKey, update.task.taskId, reason: 'progress for inactive download'); + return; + } + // If this item is being paused, the holding queue promoted it — cancel it if (_pausingKeys.contains(globalKey)) { - if (downloadsSupported) FileDownloader().cancelTaskWithId(update.task.taskId); + await _cancelNativeTask(globalKey, update.task.taskId, reason: 'pause in progress'); return; } if (_cancellingKeys.contains(globalKey)) { - if (downloadsSupported) FileDownloader().cancelTaskWithId(update.task.taskId); + await _cancelNativeTask(globalKey, update.task.taskId, reason: 'cancellation in progress'); return; } @@ -1202,23 +1464,51 @@ class DownloadManagerService { /// Callback: background_downloader status change void _onTaskStatusChanged(TaskStatusUpdate update) { + if (_disposed) return; + unawaited( + _handleTaskStatusChanged(update).catchError((Object e, StackTrace st) { + appLogger.e('Error handling download status for ${update.task.metaData}', error: e, stackTrace: st); + }), + ); + } + + @visibleForTesting + Future debugHandleTaskStatus(TaskStatusUpdate update) => _handleTaskStatusChanged(update); + + Future _handleTaskStatusChanged(TaskStatusUpdate update) async { if (_disposed) return; final globalKey = update.task.metaData; if (globalKey.isEmpty) return; - appLogger.d('Background task status: ${update.status} for $globalKey'); + appLogger.d('Background task status: ${update.status} for $globalKey (task ${update.task.taskId})'); + + final existing = await _downloadForCurrentTaskSession( + globalKey, + update.task.taskId, + event: 'status ${update.status}', + cancelStale: _isNativeTaskActiveStatus(update.status), + ); + 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: - _onDownloadComplete(globalKey, update.task); + await _onDownloadComplete(globalKey, update.task); case TaskStatus.failed: - _onDownloadFailed(globalKey, update.exception?.description ?? 'Download failed'); + await _onDownloadFailed(globalKey, update.task.taskId, update.exception?.description ?? 'Download failed'); case TaskStatus.notFound: - _onDownloadPermanentlyFailed(globalKey, 'File not found (404)'); + await _onDownloadPermanentlyFailed(globalKey, update.task.taskId, 'File not found (404)'); case TaskStatus.canceled: if (_pausingKeys.contains(globalKey) || _cancellingKeys.contains(globalKey)) break; - _onDownloadCanceled(globalKey); + await _onDownloadCanceled(globalKey, update.task.taskId); case TaskStatus.paused: appLogger.d('Download paused by system for $globalKey'); case TaskStatus.waitingToRetry: @@ -1227,10 +1517,10 @@ class DownloadManagerService { case TaskStatus.running: // If this item is being paused, the holding queue promoted it — cancel it if (_pausingKeys.contains(globalKey)) { - if (downloadsSupported) FileDownloader().cancelTaskWithId(update.task.taskId); + await _cancelNativeTask(globalKey, update.task.taskId, reason: 'pause in progress'); } if (_cancellingKeys.contains(globalKey)) { - if (downloadsSupported) FileDownloader().cancelTaskWithId(update.task.taskId); + await _cancelNativeTask(globalKey, update.task.taskId, reason: 'cancellation in progress'); } break; } @@ -1240,19 +1530,21 @@ class DownloadManagerService { } /// Handle a system-initiated cancel — re-queue unless already completed. - Future _onDownloadCanceled(String globalKey) async { - _cancelDownloadTimers(globalKey); - final ctx = _pendingDownloadContext.remove(globalKey); - if (ctx == null) return; + 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; } + _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); @@ -1263,7 +1555,7 @@ class DownloadManagerService { /// Handle a failed download — auto-retry if retries remain, otherwise permanently fail. /// Native retries (Range-based resume) are already exhausted at this point. - Future _onDownloadFailed(String globalKey, String errorMessage) async { + Future _onDownloadFailed(String globalKey, String taskId, String errorMessage) async { if (_cancellingKeys.contains(globalKey)) { appLogger.d('Ignoring failure for $globalKey: cancellation in progress'); return; @@ -1272,16 +1564,18 @@ class DownloadManagerService { appLogger.d('Ignoring failure event for $globalKey: completion in progress'); return; } - _cancelDownloadTimers(globalKey); - _pendingDownloadContext.remove(globalKey); 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; } + _cancelDownloadTimers(globalKey); + _pendingDownloadContext.remove(globalKey); final retryCount = existing.retryCount; // DNS/connection errors fail instantly and exhaust native retries in milliseconds, @@ -1318,12 +1612,12 @@ class DownloadManagerService { appLogger.w('Network error for $globalKey, failing permanently (no auto-retry): $errorMessage'); } final userMessage = isServerError ? t.downloads.serverErrorBitrate : errorMessage; - await _onDownloadPermanentlyFailed(globalKey, userMessage); + await _onDownloadPermanentlyFailed(globalKey, taskId, userMessage); } } /// Handle a non-retryable failure (e.g. 404) — fail immediately without auto-retry. - Future _onDownloadPermanentlyFailed(String globalKey, String errorMessage) async { + Future _onDownloadPermanentlyFailed(String globalKey, String taskId, String errorMessage) async { if (_cancellingKeys.contains(globalKey)) { appLogger.d('Ignoring permanent failure for $globalKey: cancellation in progress'); return; @@ -1332,17 +1626,20 @@ class DownloadManagerService { appLogger.d('Ignoring permanent failure event for $globalKey: completion in progress'); return; } - _cancelDownloadTimers(globalKey); - _pendingDownloadContext.remove(globalKey); 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; } + _cancelDownloadTimers(globalKey); + _pendingDownloadContext.remove(globalKey); + appLogger.e('Download permanently failed for $globalKey: $errorMessage'); await _transitionStatus(globalKey, DownloadStatus.failed, errorMessage: errorMessage); await _database.removeFromQueue(globalKey); @@ -1368,7 +1665,7 @@ class DownloadManagerService { } appLogger.i('Auto-retrying download for $globalKey'); - await _database.updateBgTaskId(globalKey, null); + await _cleanupStaleDownload(globalKey); await _transitionStatus(globalKey, DownloadStatus.queued); await _database.addToQueue(mediaGlobalKey: globalKey); unawaited(_processQueue(client)); @@ -1384,24 +1681,22 @@ class DownloadManagerService { } _completingKeys.add(globalKey); try { - // Flush any pending debounced progress write + cancel any scheduled retry - _cancelDownloadTimers(globalKey); - // Fresh DB check — bail if already completed (guards against race with orphan scan) final existingCheck = await _database.getDownloadedMedia(globalKey); if (_cancellingKeys.contains(globalKey) || existingCheck == null) { appLogger.d('Download no longer active for $globalKey, skipping completion'); return; } - if (existingCheck.status == DownloadStatus.completed.index) { - appLogger.d('Download already completed for $globalKey, skipping'); - return; - } - if (existingCheck.status == DownloadStatus.cancelled.index) { - appLogger.d('Download cancelled for $globalKey, skipping completion'); + 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); final ctx = _pendingDownloadContext.remove(globalKey); // ── Phase 1 (critical): resolve and store the video file path ── @@ -1732,6 +2027,7 @@ class DownloadManagerService { try { _cancelDownloadTimers(globalKey); final bgTaskId = await _database.getBgTaskId(globalKey); + await _cancelNativeTasksForGlobalKey(globalKey, exceptTaskId: bgTaskId, reason: 'duplicate task before pause'); if (bgTaskId != null && downloadsSupported) { final task = await FileDownloader().taskForId(bgTaskId); if (task != null && task is DownloadTask) { @@ -1759,36 +2055,60 @@ class DownloadManagerService { final bgTaskId = await _database.getBgTaskId(globalKey); // Try native resume first (only works for normal-mode DownloadTask that was paused) - if (bgTaskId != null) { - final task = await FileDownloader().taskForId(bgTaskId); - if (task != null && task is DownloadTask) { - final resumed = await FileDownloader().resume(task); - if (resumed) { - appLogger.i('Resumed download via background_downloader for $globalKey'); - await _database.updateDownloadStatus(globalKey, DownloadStatus.downloading.index); - _emitProgress(globalKey, DownloadStatus.downloading, 0); - return; - } - } - } + if (bgTaskId != null && await _tryResumeNativeTask(globalKey, bgTaskId)) return; // Native resume failed or not supported (SAF mode) — re-enqueue from scratch - await _database.updateBgTaskId(globalKey, null); - await _database.updateDownloadProgress(globalKey, 0, 0, 0); + await _cleanupStaleDownload(globalKey); await _transitionStatus(globalKey, DownloadStatus.queued); await _database.addToQueue(mediaGlobalKey: globalKey); final resolvedClient = await _getClientForDownloadKey(globalKey) ?? client; unawaited(_processQueue(resolvedClient)); } + Future _tryResumeNativeTask( + String globalKey, + String bgTaskId, { + _NativeTaskForId? taskForId, + _NativeResumeTask? resumeTask, + }) async { + await _cancelNativeTasksForGlobalKey(globalKey, exceptTaskId: bgTaskId, reason: 'duplicate task before resume'); + + try { + final task = await (taskForId ?? FileDownloader().taskForId)(bgTaskId); + if (task == null || task is! DownloadTask) return false; + + final resumed = await (resumeTask ?? FileDownloader().resume)(task); + if (!resumed) { + appLogger.w('Native resume returned false for $globalKey; re-enqueuing from scratch'); + return false; + } + + await _transitionStatus(globalKey, DownloadStatus.downloading); + appLogger.i('Resumed download via background_downloader for $globalKey'); + return true; + } catch (e) { + appLogger.w('Native resume failed for $globalKey; re-enqueuing from scratch', error: e); + return false; + } + } + + @visibleForTesting + Future debugTryResumeNativeTask( + String globalKey, + String bgTaskId, { + required Future Function(String taskId) taskForId, + required Future Function(DownloadTask task) resumeTask, + }) { + return _tryResumeNativeTask(globalKey, bgTaskId, taskForId: taskForId, resumeTask: resumeTask); + } + /// Retry a failed download Future retryDownload(String globalKey, MediaServerClient client) async { if (_skipDownloadsUnsupported('download retry')) return; _autoRetryTimers.remove(globalKey)?.cancel(); + await _cleanupStaleDownload(globalKey); await _database.clearDownloadError(globalKey); - await _database.updateBgTaskId(globalKey, null); - await _database.updateDownloadProgress(globalKey, 0, 0, 0); await _transitionStatus(globalKey, DownloadStatus.queued); await _database.addToQueue(mediaGlobalKey: globalKey); final resolvedClient = await _getClientForDownloadKey(globalKey) ?? client; @@ -1801,12 +2121,8 @@ class DownloadManagerService { try { _cancelDownloadTimers(globalKey); final bgTaskId = await _database.getBgTaskId(globalKey); - if (bgTaskId != null) { - if (downloadsSupported) { - await FileDownloader().cancelTaskWithId(bgTaskId); - } - await _database.updateBgTaskId(globalKey, null); - } + await _database.updateBgTaskId(globalKey, null); + await _cancelNativeTasksForGlobalKey(globalKey, includeTaskId: bgTaskId, reason: 'user cancellation'); _pendingDownloadContext.remove(globalKey); await _transitionStatus(globalKey, DownloadStatus.cancelled); await _database.removeFromQueue(globalKey); @@ -1816,56 +2132,62 @@ class DownloadManagerService { } Future deleteDownload(String globalKey) async { - _cancelDownloadTimers(globalKey); - final bgTaskId = await _database.getBgTaskId(globalKey); - if (bgTaskId != null) { - if (downloadsSupported) { - await FileDownloader().cancelTaskWithId(bgTaskId); - } + _cancellingKeys.add(globalKey); + try { + _cancelDownloadTimers(globalKey); + final bgTaskId = await _database.getBgTaskId(globalKey); await _database.updateBgTaskId(globalKey, null); - } - _pendingDownloadContext.remove(globalKey); + await _cancelNativeTasksForGlobalKey(globalKey, includeTaskId: bgTaskId, reason: 'delete download'); + _pendingDownloadContext.remove(globalKey); - final parsed = parseGlobalKey(globalKey); - if (parsed == null) { - await _database.deleteDownload(globalKey); - return; - } + final parsed = parseGlobalKey(globalKey); + if (parsed == null) { + await _database.deleteDownload(globalKey); + return; + } - final serverId = parsed.serverId; - final ratingKey = parsed.ratingKey; - final downloadRecord = await _database.getDownloadedMedia(globalKey); - final clientScopeId = downloadRecord?.clientScopeId; - final metadata = await _lookupMetadata(serverId, ratingKey, clientScopeId: clientScopeId); + final serverId = parsed.serverId; + final ratingKey = parsed.ratingKey; + final downloadRecord = await _database.getDownloadedMedia(globalKey); + final clientScopeId = downloadRecord?.clientScopeId; + final metadata = await _lookupMetadata(serverId, ratingKey, clientScopeId: clientScopeId); + + if (metadata == null) { + // Fallback deletion without progress + await _deleteMediaFilesWithMetadata(serverId, ratingKey, clientScopeId: clientScopeId); + await _deleteForItemByServer(serverId, ratingKey, clientScopeId: clientScopeId); + await _database.deleteDownload(globalKey); + return; + } + + final totalItems = await _getTotalItemsToDelete(metadata, serverId, clientScopeId: clientScopeId); + + _emitDeletionProgress( + DeletionProgress( + globalKey: globalKey, + itemTitle: metadata.displayTitle, + currentItem: 0, + totalItems: totalItems, + ), + ); - if (metadata == null) { - // Fallback deletion without progress await _deleteMediaFilesWithMetadata(serverId, ratingKey, clientScopeId: clientScopeId); + await _deleteForItemByServer(serverId, ratingKey, clientScopeId: clientScopeId); + await _database.deleteDownload(globalKey); - return; + + _emitDeletionProgress( + DeletionProgress( + globalKey: globalKey, + itemTitle: metadata.displayTitle, + currentItem: totalItems, + totalItems: totalItems, + ), + ); + } finally { + _cancellingKeys.remove(globalKey); } - - final totalItems = await _getTotalItemsToDelete(metadata, serverId, clientScopeId: clientScopeId); - - _emitDeletionProgress( - DeletionProgress(globalKey: globalKey, itemTitle: metadata.displayTitle, currentItem: 0, totalItems: totalItems), - ); - - await _deleteMediaFilesWithMetadata(serverId, ratingKey, clientScopeId: clientScopeId); - - await _deleteForItemByServer(serverId, ratingKey, clientScopeId: clientScopeId); - - await _database.deleteDownload(globalKey); - - _emitDeletionProgress( - DeletionProgress( - globalKey: globalKey, - itemTitle: metadata.displayTitle, - currentItem: totalItems, - totalItems: totalItems, - ), - ); } void _emitDeletionProgress(DeletionProgress progress) { diff --git a/lib/services/sync_rule_executor.dart b/lib/services/sync_rule_executor.dart index a6434243..016e2349 100644 --- a/lib/services/sync_rule_executor.dart +++ b/lib/services/sync_rule_executor.dart @@ -480,7 +480,8 @@ class SyncRuleExecutor { p != null && (p.status == DownloadStatus.completed || p.status == DownloadStatus.downloading || - p.status == DownloadStatus.queued); + p.status == DownloadStatus.queued || + p.status == DownloadStatus.paused); Future> _readConnectivity() async { try { diff --git a/pubspec.lock b/pubspec.lock index d88bdb76..3278f4f1 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -89,8 +89,8 @@ packages: dependency: "direct main" description: path: "." - ref: "4c965996210e408465e3c8b66e63ab4a659a62f9" - resolved-ref: "4c965996210e408465e3c8b66e63ab4a659a62f9" + ref: b4d36f88bb365faaf308ff26be7ade49bbaec859 + resolved-ref: b4d36f88bb365faaf308ff26be7ade49bbaec859 url: "https://github.com/edde746/background_downloader" source: git version: "9.5.4" diff --git a/pubspec.yaml b/pubspec.yaml index 736e3c2a..fce1905d 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -59,7 +59,7 @@ dependencies: background_downloader: git: url: https://github.com/edde746/background_downloader - ref: 4c965996210e408465e3c8b66e63ab4a659a62f9 + ref: b4d36f88bb365faaf308ff26be7ade49bbaec859 sentry_flutter: git: url: https://github.com/edde746/sentry-dart diff --git a/test/providers/download_provider_test.dart b/test/providers/download_provider_test.dart index fabb7a23..c9b525f2 100644 --- a/test/providers/download_provider_test.dart +++ b/test/providers/download_provider_test.dart @@ -415,6 +415,22 @@ void main() { p.dispose(); }); + test('queueDownload leaves paused downloads paused instead of re-queueing them', () async { + final p = DownloadProvider.forTesting(downloadManager: downloadManager, database: db); + await p.ensureInitialized(); + p.debugSeedState( + downloads: {'srv:1': const DownloadProgress(globalKey: 'srv:1', status: DownloadStatus.paused)}, + metadata: {'srv:1': movie}, + ); + + final count = await p.queueDownload(movie, _ThrowingClient()); + + expect(count, 0); + expect(p.getProgress('srv:1')?.status, DownloadStatus.paused); + + p.dispose(); + }); + test('deleteDownload removes only active-profile ownership when another owner remains', () async { await db.addDownloadOwner(profileId: 'test-profile', globalKey: 'srv:1'); await db.addDownloadOwner(profileId: 'profile-b', globalKey: 'srv:1'); diff --git a/test/services/download_manager_service_test.dart b/test/services/download_manager_service_test.dart index 6912da17..f74c1832 100644 --- a/test/services/download_manager_service_test.dart +++ b/test/services/download_manager_service_test.dart @@ -1,6 +1,7 @@ import 'dart:convert'; import 'dart:io'; +import 'package:background_downloader/background_downloader.dart'; import 'package:drift/drift.dart' show Value; import 'package:drift/native.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -262,6 +263,179 @@ void main() { expect(row?.thumbPath, artworkStorageKey('/ep-thumb')); }); }); + + group('task session validation', () { + test('ignores progress from stale native task ids', () async { + final db = AppDatabase.forTesting(NativeDatabase.memory()); + addTearDown(db.close); + const globalKey = 'srv:item-1'; + await db.insertDownload( + serverId: 'srv', + ratingKey: 'item-1', + globalKey: globalKey, + type: 'movie', + status: DownloadStatus.downloading.index, + ); + await db.updateBgTaskId(globalKey, 'current-task'); + + final manager = DownloadManagerService( + database: db, + storageService: DownloadStorageService.instance, + downloadsSupportedOverride: false, + ); + addTearDown(manager.dispose); + final events = []; + final sub = manager.progressStream.listen(events.add); + addTearDown(sub.cancel); + + await manager.debugHandleTaskProgress(TaskProgressUpdate(_downloadTask('stale-task', globalKey), 0.5, 1000)); + await Future.delayed(Duration.zero); + expect(events, isEmpty); + + await manager.debugHandleTaskProgress(TaskProgressUpdate(_downloadTask('current-task', globalKey), 0.5, 1000)); + await Future.delayed(Duration.zero); + + expect(events, hasLength(1)); + expect(events.single.globalKey, globalKey); + expect(events.single.progress, 50); + }); + + test('ignores terminal status from stale native task ids', () async { + final db = AppDatabase.forTesting(NativeDatabase.memory()); + addTearDown(db.close); + const globalKey = 'srv:item-1'; + await db.insertDownload( + serverId: 'srv', + ratingKey: 'item-1', + globalKey: globalKey, + type: 'movie', + status: DownloadStatus.downloading.index, + ); + await db.updateBgTaskId(globalKey, 'current-task'); + + final manager = DownloadManagerService( + database: db, + storageService: DownloadStorageService.instance, + downloadsSupportedOverride: false, + ); + addTearDown(manager.dispose); + + await manager.debugHandleTaskStatus(TaskStatusUpdate(_downloadTask('stale-task', globalKey), TaskStatus.failed)); + + final row = await db.getDownloadedMedia(globalKey); + expect(row?.status, DownloadStatus.downloading.index); + expect(row?.errorMessage, isNull); + expect(row?.bgTaskId, 'current-task'); + }); + + test('requeues current system cancel without in-memory context', () async { + final db = AppDatabase.forTesting(NativeDatabase.memory()); + addTearDown(db.close); + const globalKey = 'srv:item-1'; + await db.insertDownload( + serverId: 'srv', + ratingKey: 'item-1', + globalKey: globalKey, + type: 'movie', + status: DownloadStatus.downloading.index, + ); + await db.updateBgTaskId(globalKey, 'current-task'); + + final manager = DownloadManagerService( + database: db, + storageService: DownloadStorageService.instance, + 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.queued.index); + expect(row?.bgTaskId, isNull); + expect((await db.getNextQueueItem())?.mediaGlobalKey, globalKey); + }); + }); + + group('resume handling', () { + test('failed native resume leaves paused row paused', () async { + final db = AppDatabase.forTesting(NativeDatabase.memory()); + addTearDown(db.close); + const globalKey = 'srv:item-1'; + await db.insertDownload( + serverId: 'srv', + ratingKey: 'item-1', + globalKey: globalKey, + type: 'movie', + status: DownloadStatus.paused.index, + ); + await db.updateBgTaskId(globalKey, 'current-task'); + + final manager = DownloadManagerService( + database: db, + storageService: DownloadStorageService.instance, + downloadsSupportedOverride: false, + ); + addTearDown(manager.dispose); + + final resumed = await manager.debugTryResumeNativeTask( + globalKey, + 'current-task', + taskForId: (_) async => _downloadTask('current-task', globalKey), + resumeTask: (_) async => false, + ); + + final row = await db.getDownloadedMedia(globalKey); + expect(resumed, isFalse); + expect(row?.status, DownloadStatus.paused.index); + expect(row?.bgTaskId, 'current-task'); + }); + + test('successful native resume transitions to downloading', () async { + final db = AppDatabase.forTesting(NativeDatabase.memory()); + addTearDown(db.close); + const globalKey = 'srv:item-1'; + await db.insertDownload( + serverId: 'srv', + ratingKey: 'item-1', + globalKey: globalKey, + type: 'movie', + status: DownloadStatus.paused.index, + ); + await db.updateBgTaskId(globalKey, 'current-task'); + + final manager = DownloadManagerService( + database: db, + storageService: DownloadStorageService.instance, + downloadsSupportedOverride: false, + ); + addTearDown(manager.dispose); + + final resumed = await manager.debugTryResumeNativeTask( + globalKey, + 'current-task', + taskForId: (_) async => _downloadTask('current-task', globalKey), + resumeTask: (_) async => true, + ); + + final row = await db.getDownloadedMedia(globalKey); + expect(resumed, isTrue); + expect(row?.status, DownloadStatus.downloading.index); + expect(row?.bgTaskId, 'current-task'); + }); + }); +} + +DownloadTask _downloadTask(String taskId, String globalKey) { + return DownloadTask( + taskId: taskId, + url: 'https://example.test/video.mp4', + filename: 'video.mp4', + directory: 'downloads', + metaData: globalKey, + ); } MediaItem _movie({String? thumbPath}) {