From ce2446957a7af1b3693681f68a086d4bf5ae7b33 Mon Sep 17 00:00:00 2001 From: Sigve Hansen Date: Mon, 14 Sep 2026 14:36:47 +0200 Subject: [PATCH 1/4] feat(downloads): try to prevent silent failures when downloading tracks --- BMM.Core/Extensions/TrackExtensions.cs | 36 +++ .../DownloadFailureClassifier.cs | 70 +++++ .../DownloadQueue/DownloadOutcome.cs | 28 ++ .../DownloadQueue/DownloadQueue.cs | 275 +++++++++++++++--- .../DownloadQueue/IDownloadQueue.cs | 12 + .../DownloadHttpStatusException.cs | 20 ++ .../HttpClientFileDownloader.cs | 2 +- .../Downloading/GlobalTrackProvider.cs | 42 ++- BMM.Core/ViewModels/AlbumViewModel.cs | 6 +- BMM.Core/ViewModels/Base/DownloadViewModel.cs | 83 +++++- .../ViewModels/CuratedPlaylistViewModel.cs | 5 +- .../ViewModels/MyContent/MyTracksViewModel.cs | 5 +- .../Unit/Extensions/TrackExtensionsTests.cs | 196 +++++++++++++ .../Downloading/DownloadQueueTests.cs | 192 +++++++++++- .../FileStorage/GlobalTrackProviderTests.cs | 9 +- .../listitem_trackcollection_header.axml | 2 +- .../layout/listitem_tracklist_header.xml | 2 +- .../Exceptions/IosDownloadException.cs | 9 +- .../Download/IosFileDownloader.cs | 12 +- 19 files changed, 947 insertions(+), 59 deletions(-) create mode 100644 BMM.Core/Implementations/Downloading/DownloadQueue/DownloadFailureClassifier.cs create mode 100644 BMM.Core/Implementations/Downloading/DownloadQueue/DownloadOutcome.cs create mode 100644 BMM.Core/Implementations/Downloading/FileDownloader/DownloadHttpStatusException.cs create mode 100644 BMM.Tests/BMM.Core.Test/Unit/Extensions/TrackExtensionsTests.cs diff --git a/BMM.Core/Extensions/TrackExtensions.cs b/BMM.Core/Extensions/TrackExtensions.cs index 0c6191391..be28f1fc1 100644 --- a/BMM.Core/Extensions/TrackExtensions.cs +++ b/BMM.Core/Extensions/TrackExtensions.cs @@ -45,4 +45,40 @@ public static TimeSpan SumTrackDuration(this IEnumerable tracks) long totalSeconds = tracks.Sum(t => t.Duration / 1000); return TimeSpan.FromSeconds(totalSeconds); } + + /// + /// Total size of the files belonging to these tracks. + /// + /// + /// Both media and files are optional in the API, so a single track without them used to + /// throw a out of the download button's size check, which the user + /// saw as "An unknown error occurred" with nothing downloaded. + /// + public static long SumApproximateDownloadSize(this IEnumerable tracks) + { + if (tracks == null) + return 0; + + return tracks + .Where(track => track?.Media != null) + .SelectMany(track => track.Media) + .Where(medium => medium?.Files != null) + .SelectMany(medium => medium.Files) + .Sum(file => file.Size); + } + + /// + /// The tracks of a collection that the downloader will actually try to fetch. Mirrors the filters in + /// the offline track providers, so that "is everything downloaded" cannot wait for a file that is + /// never going to be requested. + /// + public static IEnumerable WhereDownloadable(this IEnumerable tracks) + { + if (tracks == null) + return Enumerable.Empty(); + + return tracks.Where(track => track != null + && track.Subtype != TrackSubType.Video + && !string.IsNullOrEmpty(track.Url)); + } } \ No newline at end of file diff --git a/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadFailureClassifier.cs b/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadFailureClassifier.cs new file mode 100644 index 000000000..8219abbda --- /dev/null +++ b/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadFailureClassifier.cs @@ -0,0 +1,70 @@ +using System; +using System.IO; +using System.Net; +using System.Net.Http; +using System.Net.Sockets; +using BMM.Api.Framework.Exceptions; +using BMM.Core.Implementations.Downloading.FileDownloader; + +namespace BMM.Core.Implementations.Downloading.DownloadQueue +{ + /// + /// Decides whether a failed download attempt was the network's fault. + /// + /// + /// Analytics showed roughly 62.000 "Connection failure" events against about 113 timeouts, the vast + /// majority of them logged while the device reported no active connection at all. Those failures come + /// back within milliseconds, so a queue that treats them as "this file is done" empties itself in a + /// couple of seconds and then looks exactly like a completed download. + /// + public static class DownloadFailureClassifier + { + public static DownloadOutcome Classify(Exception exception, bool cancellationWasRequested) + { + return exception switch + { + // A cancellation we asked for is not a failure. Anything else that surfaces as + // cancellation is a timeout, which is a network problem. + OperationCanceledException when cancellationWasRequested => DownloadOutcome.Cancelled, + OperationCanceledException => DownloadOutcome.ConnectionFailure, + + InternetProblemsException => DownloadOutcome.ConnectionFailure, + SocketException => DownloadOutcome.ConnectionFailure, + WebException => DownloadOutcome.ConnectionFailure, + + // "Connection failure", "Software caused connection abort", "Socket closed" and + // "Unable to resolve host ..." all arrive as HttpRequestException. + HttpRequestException => DownloadOutcome.ConnectionFailure, + + // The response started arriving and then the connection died mid-body. + IOException => DownloadOutcome.ConnectionFailure, + + DownloadHttpStatusException statusException => ClassifyStatusCode(statusException.StatusCode), + + _ => IsConnectionFailure(exception.InnerException, cancellationWasRequested) + ? DownloadOutcome.ConnectionFailure + : DownloadOutcome.PermanentFailure + }; + } + + private static bool IsConnectionFailure(Exception inner, bool cancellationWasRequested) + { + return inner != null && Classify(inner, cancellationWasRequested) == DownloadOutcome.ConnectionFailure; + } + + private static DownloadOutcome ClassifyStatusCode(HttpStatusCode statusCode) + { + // 401 means the access token needs refreshing, which is worth another attempt later. + // 5xx and 429 are the server asking us to come back. Everything else is about this file. + if (statusCode == HttpStatusCode.Unauthorized + || statusCode == HttpStatusCode.RequestTimeout + || statusCode == HttpStatusCode.TooManyRequests + || (int)statusCode >= 500) + { + return DownloadOutcome.ConnectionFailure; + } + + return DownloadOutcome.PermanentFailure; + } + } +} diff --git a/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadOutcome.cs b/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadOutcome.cs new file mode 100644 index 000000000..ddc13fe1e --- /dev/null +++ b/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadOutcome.cs @@ -0,0 +1,28 @@ +namespace BMM.Core.Implementations.Downloading.DownloadQueue +{ + /// + /// The result of a single download attempt. The distinction that matters is whether the failure + /// says something about this file or about the network: a file that is gone should be + /// skipped so the queue can continue, while a dead connection means every remaining item would + /// fail just as fast, so the queue has to stop instead of burning through it. + /// + public enum DownloadOutcome + { + Success, + + /// + /// This particular file could not be downloaded and retrying now would not help. + /// + PermanentFailure, + + /// + /// The network is unusable. Nothing is wrong with the file itself. + /// + ConnectionFailure, + + /// + /// The download was cancelled deliberately (user interaction, app shutdown). + /// + Cancelled + } +} diff --git a/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadQueue.cs b/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadQueue.cs index 66ffd85ab..5f0a817cf 100644 --- a/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadQueue.cs +++ b/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadQueue.cs @@ -3,8 +3,10 @@ using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; +using BMM.Api.Framework; using BMM.Api.Implementation.Models; using BMM.Core.Implementations.Analytics; +using BMM.Core.Implementations.Connection; using BMM.Core.Implementations.Downloading.FileDownloader; using BMM.Core.Implementations.Exceptions; using MvvmCross.Plugin.Messenger; @@ -13,22 +15,46 @@ namespace BMM.Core.Implementations.Downloading.DownloadQueue { public class DownloadQueue : IDownloadQueue { + /// + /// A connection can be down for a moment without being down. Retrying a couple of times with a + /// growing pause absorbs that without hammering a network that is genuinely gone. + /// + private static readonly TimeSpan[] RetryDelays = + { + TimeSpan.FromSeconds(2), + TimeSpan.FromSeconds(5) + }; + private readonly DownloadableEqualityComparer _downloadableEqualityComparer = new DownloadableEqualityComparer(); private readonly IExceptionHandler _exceptionHandler; private readonly IFileDownloader _fileDownloader; private readonly IMvxMessenger _messenger; private readonly IAnalytics _analytics; + private readonly IConnection _connection; + private readonly INetworkSettings _networkSettings; + private readonly ILogger _logger; private readonly ConcurrentBag _queuedDownloads = new ConcurrentBag(); private IDownloadable _currentDownloadingDownloadable; private int _finishedDownloadCount; private bool _isDownloading; + private bool _cancellationWasRequested; - public DownloadQueue(IFileDownloader fileDownloader, IMvxMessenger messenger, IExceptionHandler exceptionHandler, IAnalytics analytics) + public DownloadQueue( + IFileDownloader fileDownloader, + IMvxMessenger messenger, + IExceptionHandler exceptionHandler, + IAnalytics analytics, + IConnection connection, + INetworkSettings networkSettings, + ILogger logger) { _fileDownloader = fileDownloader; _messenger = messenger; _exceptionHandler = exceptionHandler; _analytics = analytics; + _connection = connection; + _networkSettings = networkSettings; + _logger = logger; } private int CurrentlyDownloadingCount => _currentDownloadingDownloadable == null ? 0 : 1; @@ -37,10 +63,13 @@ public DownloadQueue(IFileDownloader fileDownloader, IMvxMessenger messenger, IE public int RemainingDownloadsCount => _queuedDownloads.Count + CurrentlyDownloadingCount; - private void CancelQueue() - { - DequeueAllExcept(new List()); - } + public bool IsRunning => _isDownloading; + + /// + /// True when the queue gave up with items still in it. Callers must not read an empty queue as + /// "everything was downloaded" without checking this. + /// + public bool StoppedWithPendingDownloads { get; private set; } public void DequeueAllExcept(IEnumerable downloadables) { @@ -89,6 +118,7 @@ public void AppWasKilled() { if (_isDownloading) { + _cancellationWasRequested = true; _fileDownloader.CancelDownload(); } } @@ -99,37 +129,155 @@ public void StartDownloading() return; _isDownloading = true; + _cancellationWasRequested = false; + StoppedWithPendingDownloads = false; - _exceptionHandler.FireAndForgetWithoutUserMessages(async () => + _exceptionHandler.FireAndForgetWithoutUserMessages(RunQueue); + } + + private async Task RunQueue() + { + int succeeded = 0; + int skipped = 0; + var stopReason = QueueStopReason.Completed; + + try { - var downloadSucceeded = true; - try + while (!_queuedDownloads.IsEmpty) { - while (_queuedDownloads.Count > 0) + // Checked per item, not once up front. The connection the sync validated before + // enqueueing may be long gone by the time we get to item 40 of 91. + if (!await IsDownloadingAllowed()) { - _queuedDownloads.TryTake(out var item); - _currentDownloadingDownloadable = item; - _messenger.Publish(new FileDownloadStartedMessage(this, item.Id)); - await SafeDownload(item); - _messenger.Publish(new FileDownloadCompletedMessage(this, item.Id)); - _analytics.LogEvent(Event.TrackHasBeenDownloaded, PrepareAdditionalEventArguments(item)); + stopReason = QueueStopReason.NoConnection; + break; + } + + if (!_queuedDownloads.TryTake(out var item)) + break; + + var (outcome, exception) = await DownloadWithRetries(item); + + switch (outcome) + { + case DownloadOutcome.Success: + succeeded++; + _finishedDownloadCount++; + _messenger.Publish(new FileDownloadCompletedMessage(this, item.Id)); + _analytics.LogEvent(Event.TrackHasBeenDownloaded, PrepareAdditionalEventArguments(item)); + break; + + case DownloadOutcome.PermanentFailure: + // Nothing about this file is going to get better by trying again, so let the + // rest of the queue through instead of stalling on it. The message resets the + // row, which would otherwise keep showing a download in progress. + skipped++; + _finishedDownloadCount++; + _messenger.Publish(new FileDownloadCanceledMessage(this, exception)); + break; + + case DownloadOutcome.ConnectionFailure: + // Every remaining item would fail the same way within milliseconds. Put this + // one back and stop, so the queue survives until the connection returns. + _queuedDownloads.Add(item); + _messenger.Publish(new FileDownloadCanceledMessage(this, exception)); + stopReason = QueueStopReason.NoConnection; + break; + + case DownloadOutcome.Cancelled: + _queuedDownloads.Add(item); + stopReason = QueueStopReason.Cancelled; + break; } - } - catch (TaskCanceledException) - { - CancelQueue(); - downloadSucceeded = false; - // We don't need to show an error message if the task was canceled due to a user interaction + if (stopReason != QueueStopReason.Completed) + break; } - finally + } + finally + { + _currentDownloadingDownloadable = null; + _isDownloading = false; + StoppedWithPendingDownloads = !_queuedDownloads.IsEmpty; + + LogQueueOutcome(stopReason, succeeded, skipped); + + // Only a queue that actually emptied itself counts as a success. Reporting a queue we + // abandoned as succeeded is what made a playlist with no files on disk render as + // downloaded. + bool succeededOverall = stopReason == QueueStopReason.Completed && !StoppedWithPendingDownloads; + + if (!succeededOverall) + _messenger.Publish(new DownloadQueueChangedMessage(this)); + + _finishedDownloadCount = 0; + _messenger.Publish(new QueueFinishedMessage(this, succeededOverall)); + } + } + + /// + /// Retries a download while the failures look like the network rather than the file. + /// + private async Task<(DownloadOutcome Outcome, Exception Exception)> DownloadWithRetries(IDownloadable item) + { + _currentDownloadingDownloadable = item; + _messenger.Publish(new FileDownloadStartedMessage(this, item.Id)); + + try + { + for (int attempt = 0; ; attempt++) { - _isDownloading = false; - _finishedDownloadCount = 0; - _currentDownloadingDownloadable = null; - _messenger.Publish(new QueueFinishedMessage(this, downloadSucceeded)); + var result = await SafeDownload(item, attempt); + + if (result.Outcome != DownloadOutcome.ConnectionFailure || attempt >= RetryDelays.Length) + return result; + + await WaitBeforeRetry(RetryDelays[attempt]); + + if (_cancellationWasRequested) + return (DownloadOutcome.Cancelled, result.Exception); + + if (!await IsDownloadingAllowed()) + return result; } - }); + } + finally + { + _currentDownloadingDownloadable = null; + } + } + + /// + /// Overridable so tests do not have to wait out the real backoff. + /// + protected virtual Task WaitBeforeRetry(TimeSpan delay) => Task.Delay(delay); + + /// + /// Mirrors the check the synchronization does before enqueueing, so that leaving Wi-Fi mid-queue + /// stops the run instead of quietly continuing over a metered connection. + /// + private async Task IsDownloadingAllowed() + { + if (_cancellationWasRequested) + return false; + + if (_connection.GetStatus() != ConnectionStatus.Online) + return false; + + return _connection.IsUsingNetworkWithoutExtraCosts() + || await _networkSettings.GetMobileNetworkDownloadAllowed(); + } + + private static string DescribeException(Exception exception) + { + try + { + return exception.Message; + } + catch (Exception) + { + return exception.GetType().Name; + } } private static Dictionary PrepareAdditionalEventArguments(IDownloadable item) @@ -137,27 +285,80 @@ private static Dictionary PrepareAdditionalEventArguments(IDownl return new Dictionary { { "trackId", item.Id }, - { "tags", string.Join(",", item.Tags.ToArray()) }, + { "tags", item.Tags == null ? string.Empty : string.Join(",", item.Tags) }, { "url", item.Url } }; } - private async Task SafeDownload(IDownloadable item) + private async Task<(DownloadOutcome Outcome, Exception Exception)> SafeDownload(IDownloadable item, int attempt) { try { await _fileDownloader.DownloadFile(item); + return (DownloadOutcome.Success, null); } catch (Exception e) { - var arguments = PrepareAdditionalEventArguments(item); - arguments.Add("exception", e.Message); - _analytics.LogEvent(Event.TrackDownloadingException, arguments); - } - finally - { - _finishedDownloadCount++; + var outcome = DownloadFailureClassifier.Classify(e, _cancellationWasRequested); + + if (outcome == DownloadOutcome.Cancelled) + return (outcome, e); + + // Reporting must never be able to fail the queue. A platform exception whose own Message + // getter threw used to escape this catch block and abandon every remaining download. + try + { + var arguments = PrepareAdditionalEventArguments(item); + arguments.Add("exception", DescribeException(e)); + arguments.Add("outcome", outcome.ToString()); + arguments.Add("attempt", attempt); + _analytics.LogEvent(Event.TrackDownloadingException, arguments); + } + catch (Exception reportingException) + { + _logger.Error(nameof(DownloadQueue), + $"Could not report a failed download of track {item.Id}", + reportingException); + } + + return (outcome, e); } } + + /// + /// One event per queue run rather than one per item: the per-item detail is already in analytics, + /// and a dead connection produces hundreds of identical item failures that would drown out + /// everything else in the error tracker. + /// + private void LogQueueOutcome(QueueStopReason stopReason, int succeeded, int skipped) + { + if (stopReason == QueueStopReason.Completed && !StoppedWithPendingDownloads) + return; + + if (stopReason == QueueStopReason.Cancelled) + return; + + string message = $"Download queue stopped early ({stopReason}). " + + $"Succeeded: {succeeded}, skipped: {skipped}, still queued: {_queuedDownloads.Count}."; + + _logger.Error(nameof(DownloadQueue), message); + + _analytics.LogEvent( + "Download queue stopped early", + new Dictionary + { + { "reason", stopReason.ToString() }, + { "succeeded", succeeded }, + { "skipped", skipped }, + { "stillQueued", _queuedDownloads.Count } + }); + } + + private enum QueueStopReason + { + Completed, + NoConnection, + Cancelled + } } -} \ No newline at end of file +} diff --git a/BMM.Core/Implementations/Downloading/DownloadQueue/IDownloadQueue.cs b/BMM.Core/Implementations/Downloading/DownloadQueue/IDownloadQueue.cs index aedfa4c5d..d8f77a368 100644 --- a/BMM.Core/Implementations/Downloading/DownloadQueue/IDownloadQueue.cs +++ b/BMM.Core/Implementations/Downloading/DownloadQueue/IDownloadQueue.cs @@ -9,6 +9,18 @@ public interface IDownloadQueue int RemainingDownloadsCount { get; } + /// + /// True while the queue is actually working through its items. Items can be queued without the + /// queue running, e.g. after it stopped because the connection died. + /// + bool IsRunning { get; } + + /// + /// True when the last run ended with items still queued, e.g. because the connection died. + /// An empty queue on its own does not mean the downloads succeeded. + /// + bool StoppedWithPendingDownloads { get; } + void DequeueAllExcept(IEnumerable downloadables); void Enqueue(IDownloadable downloadable); diff --git a/BMM.Core/Implementations/Downloading/FileDownloader/DownloadHttpStatusException.cs b/BMM.Core/Implementations/Downloading/FileDownloader/DownloadHttpStatusException.cs new file mode 100644 index 000000000..26072a33c --- /dev/null +++ b/BMM.Core/Implementations/Downloading/FileDownloader/DownloadHttpStatusException.cs @@ -0,0 +1,20 @@ +using System; +using System.Net; + +namespace BMM.Core.Implementations.Downloading.FileDownloader +{ + /// + /// A download that came back with a non-success status code. Carries the code itself so the queue can + /// tell "this file is gone" (404) apart from "come back later" (401, 5xx) instead of parsing a message. + /// + public class DownloadHttpStatusException : Exception + { + public HttpStatusCode StatusCode { get; } + + public DownloadHttpStatusException(HttpStatusCode statusCode) + : base($"The request returned with error HTTP status code {statusCode}") + { + StatusCode = statusCode; + } + } +} diff --git a/BMM.Core/Implementations/Downloading/FileDownloader/HttpClientFileDownloader.cs b/BMM.Core/Implementations/Downloading/FileDownloader/HttpClientFileDownloader.cs index 3046fbe63..ae2223960 100644 --- a/BMM.Core/Implementations/Downloading/FileDownloader/HttpClientFileDownloader.cs +++ b/BMM.Core/Implementations/Downloading/FileDownloader/HttpClientFileDownloader.cs @@ -49,7 +49,7 @@ public async Task DownloadFile(IDownloadable downloadable) var response = await _httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken); if (!response.IsSuccessStatusCode) - throw new Exception($"The request returned with error HTTP status code {response.StatusCode}"); + throw new DownloadHttpStatusException(response.StatusCode); try { diff --git a/BMM.Core/Implementations/Downloading/GlobalTrackProvider.cs b/BMM.Core/Implementations/Downloading/GlobalTrackProvider.cs index 6649dd625..f54108965 100644 --- a/BMM.Core/Implementations/Downloading/GlobalTrackProvider.cs +++ b/BMM.Core/Implementations/Downloading/GlobalTrackProvider.cs @@ -1,6 +1,7 @@ using BMM.Api.Implementation.Models; using BMM.Core.Helpers; using BMM.Core.Implementations.Albums.Interfaces; +using BMM.Core.Implementations.Analytics; using BMM.Core.Implementations.PlaylistPersistence; using BMM.Core.Implementations.Podcasts; using BMM.Core.Implementations.TrackCollections; @@ -13,20 +14,23 @@ public class GlobalTrackProvider : IGlobalTrackProvider private readonly ITrackCollectionOfflineTrackProvider _trackCollectionOfflineTrackProvider; private readonly IPlaylistOfflineTrackProvider _playlistOfflineTrackProvider; private readonly IAlbumOfflineTrackProvider _albumOfflineTrackProvider; + private readonly IAnalytics _analytics; private readonly TrackEqualityComparer _trackEqualityComparer = new(); public GlobalTrackProvider( IPodcastOfflineTrackProvider podcastOfflineTrackProvider, ITrackCollectionOfflineTrackProvider trackCollectionOfflineTrackProvider, IPlaylistOfflineTrackProvider playlistOfflineTrackProvider, - IAlbumOfflineTrackProvider albumOfflineTrackProvider) + IAlbumOfflineTrackProvider albumOfflineTrackProvider, + IAnalytics analytics) { _podcastOfflineTrackProvider = podcastOfflineTrackProvider; _trackCollectionOfflineTrackProvider = trackCollectionOfflineTrackProvider; _playlistOfflineTrackProvider = playlistOfflineTrackProvider; _albumOfflineTrackProvider = albumOfflineTrackProvider; + _analytics = analytics; } - + public async Task GetTracksSupposedToBeDownloaded() { var podcastTracksSupposedToBeDownloaded = await _podcastOfflineTrackProvider.GetTracksSupposedToBeDownloaded(); @@ -34,13 +38,18 @@ public async Task GetTracksSupposedToBeDownloaded() var playlistsSupposedToBeDownloaded = await _playlistOfflineTrackProvider.GetTracksSupposedToBeDownloaded(); var albumsSupposedToBeDownloaded = await _albumOfflineTrackProvider.GetTracksSupposedToBeDownloaded(); - var tracks = podcastTracksSupposedToBeDownloaded.Tracks + var allTracks = podcastTracksSupposedToBeDownloaded.Tracks .Union(collectionTracksSupposedToBeDownloaded.Tracks, _trackEqualityComparer) .Union(playlistsSupposedToBeDownloaded.Tracks, _trackEqualityComparer) .Union(albumsSupposedToBeDownloaded.Tracks, _trackEqualityComparer) + .ToList(); + + var tracks = allTracks .Where(track => !string.IsNullOrEmpty(track.Url)) .ToList(); + LogTracksWithoutUrl(allTracks, tracks.Count); + bool isComplete = podcastTracksSupposedToBeDownloaded.IsComplete && collectionTracksSupposedToBeDownloaded.IsComplete && playlistsSupposedToBeDownloaded.IsComplete @@ -50,5 +59,32 @@ public async Task GetTracksSupposedToBeDownloaded() ? OfflineTracksResult.Complete(tracks) : OfflineTracksResult.Incomplete(tracks); } + + /// + /// A track without a URL cannot be downloaded, so it is dropped here. That used to happen without + /// a trace, which meant a playlist could be marked for offline use, download nothing at all, and + /// leave no record of why. media, files and url are all optional in the API, + /// so this measures how much content is affected and which kind. + /// + private void LogTracksWithoutUrl(IList allTracks, int downloadableCount) + { + int droppedCount = allTracks.Count - downloadableCount; + + if (droppedCount == 0) + return; + + var dropped = allTracks.Where(track => string.IsNullOrEmpty(track.Url)).ToList(); + + _analytics.LogEvent( + "Tracks supposed to be downloaded have no url", + new Dictionary + { + { "droppedCount", droppedCount }, + { "downloadableCount", downloadableCount }, + { "trackIds", string.Join(",", dropped.Take(20).Select(track => track.Id)) }, + { "subtypes", string.Join(",", dropped.Select(track => track.Subtype).Distinct()) }, + { "withoutMedia", dropped.Count(track => track.Media == null) } + }); + } } } \ No newline at end of file diff --git a/BMM.Core/ViewModels/AlbumViewModel.cs b/BMM.Core/ViewModels/AlbumViewModel.cs index d9359ecd1..f71b5beed 100644 --- a/BMM.Core/ViewModels/AlbumViewModel.cs +++ b/BMM.Core/ViewModels/AlbumViewModel.cs @@ -146,10 +146,14 @@ protected override Task CalculateApproximateDownloadSize() { long sum = Documents .OfType() - .Sum(x => x.Track.Media.Sum(t => t.Files.Sum(s => s.Size))); + .Select(x => x.Track) + .SumApproximateDownloadSize(); return Task.FromResult(sum); } + protected override IEnumerable DownloadableTracks + => Documents.OfType().Select(x => x.Track).WhereDownloadable(); + public override async Task Load() { await base.Load(); diff --git a/BMM.Core/ViewModels/Base/DownloadViewModel.cs b/BMM.Core/ViewModels/Base/DownloadViewModel.cs index 24686d2e4..3ddf11d8b 100644 --- a/BMM.Core/ViewModels/Base/DownloadViewModel.cs +++ b/BMM.Core/ViewModels/Base/DownloadViewModel.cs @@ -3,6 +3,7 @@ using System.Threading.Tasks; using Acr.UserDialogs; using BMM.Api.Framework; +using BMM.Api.Implementation.Models; using BMM.Core.Extensions; using BMM.Core.Helpers; using BMM.Core.Implementations.Connection; @@ -33,9 +34,48 @@ protected set SetProperty(ref _isOfflineAvailable, value); RaisePropertyChanged(() => IsDownloaded); RaisePropertyChanged(() => IsDownloading); + RaisePropertyChanged(() => ShowDownloadButton); } } + /// + /// Whether every track we expect offline is actually present on disk. + /// + private bool AreAllTracksDownloaded + { + get => _areAllTracksDownloaded; + set + { + if (_areAllTracksDownloaded == value) + return; + + _areAllTracksDownloaded = value; + RaisePropertyChanged(() => IsDownloaded); + RaisePropertyChanged(() => ShowDownloadButton); + } + } + + /// + /// The tracks whose files are expected on disk once this item is downloaded. Empty by default, so + /// view models that have not opted in keep behaving as before. + /// + protected virtual IEnumerable DownloadableTracks => Enumerable.Empty(); + + /// + /// Re-reads which files are on disk. Cached rather than computed per property read, because the + /// download messages raise these properties dozens of times during a single queue run. + /// + protected void RefreshDownloadedFilesState() + { + var tracks = DownloadableTracks?.ToList(); + + // An item we know nothing about yet must not be reported as incomplete, or opening a + // collection would briefly claim its downloads are missing. + AreAllTracksDownloaded = tracks == null + || tracks.Count == 0 + || tracks.All(track => _storageManager.SelectedStorage.IsDownloaded(track)); + } + private int ToBeDownloadedCount => DownloadQueue.InitialDownloadCount; private int DownloadedFilesCount => DownloadQueue.InitialDownloadCount - DownloadQueue.RemainingDownloadsCount; @@ -48,11 +88,30 @@ protected IEnumerable DownloadingFiles private set => SetProperty(ref _downloadingFiles, value); } - public bool IsDownloading => IsOfflineAvailable && DownloadedFilesCount < ToBeDownloadedCount && ToBeDownloadedCount > 0; + /// + /// Requires the queue to actually be running. Items left behind by a queue that stopped early + /// would otherwise keep the progress indicator on screen forever, with nothing downloading. + /// + public bool IsDownloading => IsOfflineAvailable + && DownloadQueue.IsRunning + && DownloadedFilesCount < ToBeDownloadedCount + && ToBeDownloadedCount > 0; public virtual bool ShowDownloadButtons => true; - public bool IsDownloaded => IsOfflineAvailable && !IsDownloading; + /// + /// "The user asked for this offline" and "the files are here" are different claims. This used to + /// be IsOfflineAvailable && !IsDownloading, which meant an empty download queue was + /// indistinguishable from a finished one: a playlist whose downloads all failed rendered with a + /// checkmark and no files on disk. + /// + public bool IsDownloaded => IsOfflineAvailable && !IsDownloading && AreAllTracksDownloaded; + + /// + /// Offer the download action whenever the files are not all there, so an incomplete download can + /// be retried instead of hiding behind a checkmark. + /// + public bool ShowDownloadButton => ShowDownloadButtons && !IsDownloading && !IsDownloaded; public abstract string Title { get; } @@ -110,6 +169,7 @@ public float DownloadStatus private MvxSubscriptionToken _downloadCancelledMessageToken; private string _durationLabel; private bool _isCompletedPercentageVisible; + private bool _areAllTracksDownloaded = true; public virtual bool ShowSharingInfo => false; public virtual bool ShowImage => true; @@ -181,9 +241,16 @@ protected override void HandleDownloadQueueFinishedMessage(QueueFinishedMessage RaiseDownloadProgressChanged(); } + public override async Task Load() + { + await base.Load(); + RefreshDownloadedFilesState(); + } + public override async Task RefreshInBackgroundAfterCacheUpdate() { await base.RefreshInBackgroundAfterCacheUpdate(); + RefreshDownloadedFilesState(); // Since the max age for a TrackCollection is 0 this will always be executed when opening a TrackCollection if (IsOfflineAvailable) @@ -238,9 +305,11 @@ protected async Task ToggleOffline() await Mvx.IoCProvider.Resolve().WarnAsync(TextSource[Translations.Global_DownloadPlaylistOnceOnWifi]); IsOfflineAvailable = newIsOfflineAvailable; - + await DownloadAction(); + RefreshDownloadedFilesState(); await RaisePropertyChanged(() => IsDownloaded); + await RaisePropertyChanged(() => ShowDownloadButton); } else { @@ -256,7 +325,9 @@ protected async Task ToggleOffline() await DeleteAction(); RefreshAllTracks(); + RefreshDownloadedFilesState(); await RaisePropertyChanged(() => IsDownloaded); + await RaisePropertyChanged(() => ShowDownloadButton); } } @@ -268,8 +339,14 @@ protected Task ResumeDownloading() private void RaiseDownloadProgressChanged() { + // Skipped while downloading: the checkmark is not on screen then, and re-checking every file + // on each of the many progress messages would be wasted work. + if (!IsDownloading) + RefreshDownloadedFilesState(); + RaisePropertyChanged(() => IsDownloading); RaisePropertyChanged(() => IsDownloaded); + RaisePropertyChanged(() => ShowDownloadButton); RaisePropertyChanged(() => DownloadingText); RaisePropertyChanged(() => DownloadStatus); } diff --git a/BMM.Core/ViewModels/CuratedPlaylistViewModel.cs b/BMM.Core/ViewModels/CuratedPlaylistViewModel.cs index 8214e28c5..0f1f417fa 100644 --- a/BMM.Core/ViewModels/CuratedPlaylistViewModel.cs +++ b/BMM.Core/ViewModels/CuratedPlaylistViewModel.cs @@ -117,8 +117,11 @@ protected override async Task DeleteAction() protected override Task CalculateApproximateDownloadSize() { - var sum = Documents.OfType().Sum(x => x.Track.Media.Sum(t => t.Files.Sum(s => s.Size))); + var sum = Documents.OfType().Select(x => x.Track).SumApproximateDownloadSize(); return Task.FromResult(sum); } + + protected override IEnumerable DownloadableTracks + => Documents.OfType().Select(x => x.Track).WhereDownloadable(); } } \ No newline at end of file diff --git a/BMM.Core/ViewModels/MyContent/MyTracksViewModel.cs b/BMM.Core/ViewModels/MyContent/MyTracksViewModel.cs index 32058802a..2094e293c 100644 --- a/BMM.Core/ViewModels/MyContent/MyTracksViewModel.cs +++ b/BMM.Core/ViewModels/MyContent/MyTracksViewModel.cs @@ -148,9 +148,12 @@ protected override async Task DeleteAction() protected override Task CalculateApproximateDownloadSize() { - return Task.FromResult(MyCollection.Tracks.Sum(x => x.Media.Sum(t => t.Files.Sum(s => s.Size)))); + return Task.FromResult(MyCollection?.Tracks.SumApproximateDownloadSize() ?? 0); } + protected override IEnumerable DownloadableTracks + => MyCollection?.Tracks.WhereDownloadable() ?? Enumerable.Empty(); + public override CacheKeys? CacheKey => CacheKeys.TrackCollectionGetById; public override async Task Load() diff --git a/BMM.Tests/BMM.Core.Test/Unit/Extensions/TrackExtensionsTests.cs b/BMM.Tests/BMM.Core.Test/Unit/Extensions/TrackExtensionsTests.cs new file mode 100644 index 000000000..96a24dd6f --- /dev/null +++ b/BMM.Tests/BMM.Core.Test/Unit/Extensions/TrackExtensionsTests.cs @@ -0,0 +1,196 @@ +using System.Collections.Generic; +using System.Linq; +using BMM.Api.Implementation.Models; +using BMM.Core.Extensions; +using BMM.Core.Test.Unit.Implementations.Downloading; +using NUnit.Framework; + +namespace BMM.Core.Test.Unit.Extensions +{ + [TestFixture] + public class TrackExtensionsTests + { + private readonly FakeTrackFactory _fakeTrackFactory = new FakeTrackFactory(); + + private static Track TrackWithMedia(int id, params TrackMedia[] media) + { + return new Track + { + Id = id, + Language = "nb", + Media = media + }; + } + + private static TrackMedia AudioMedia(params TrackMediaFile[] files) + { + return new TrackMedia { Type = TrackMediaType.Audio, Files = files }; + } + + private static TrackMediaFile File(long size, string url = "https://example.org/track.mp3") + { + return new TrackMediaFile { Size = size, Url = url, MimeType = "audio/mpeg" }; + } + + #region SumApproximateDownloadSize + + [Test] + public void SumApproximateDownloadSize_Adds_Up_Every_File() + { + var tracks = new[] + { + TrackWithMedia(1, AudioMedia(File(100), File(200))), + TrackWithMedia(2, AudioMedia(File(300))) + }; + + Assert.AreEqual(600, tracks.SumApproximateDownloadSize()); + } + + [Test] + public void SumApproximateDownloadSize_Of_Nothing_Is_Zero() + { + Assert.AreEqual(0, Enumerable.Empty().SumApproximateDownloadSize()); + } + + [Test] + public void SumApproximateDownloadSize_Handles_A_Null_Collection() + { + IEnumerable tracks = null; + + Assert.AreEqual(0, tracks.SumApproximateDownloadSize()); + } + + /// + /// "media" is optional in the API. A single track without it used to throw a + /// NullReferenceException out of the download button's size check, which the user saw as + /// "An unknown error occurred" with nothing downloaded. + /// + [Test] + public void SumApproximateDownloadSize_Skips_Tracks_Without_Media() + { + var tracks = new[] + { + TrackWithMedia(1, AudioMedia(File(100))), + new Track { Id = 2, Language = "nb", Media = null } + }; + + Assert.AreEqual(100, tracks.SumApproximateDownloadSize()); + } + + [Test] + public void SumApproximateDownloadSize_Skips_Media_Without_Files() + { + var tracks = new[] + { + TrackWithMedia(1, AudioMedia(File(100))), + TrackWithMedia(2, new TrackMedia { Type = TrackMediaType.Audio, Files = null }) + }; + + Assert.AreEqual(100, tracks.SumApproximateDownloadSize()); + } + + [Test] + public void SumApproximateDownloadSize_Skips_Null_Tracks() + { + var tracks = new[] + { + TrackWithMedia(1, AudioMedia(File(100))), + null + }; + + Assert.AreEqual(100, tracks.SumApproximateDownloadSize()); + } + + [Test] + public void SumApproximateDownloadSize_Counts_A_Track_That_Has_No_Downloadable_Url() + { + // The size check is about free space, not about what we will end up fetching, so a file + // without a url still occupies the estimate rather than throwing. + var tracks = new[] { TrackWithMedia(1, AudioMedia(File(100, url: null))) }; + + Assert.AreEqual(100, tracks.SumApproximateDownloadSize()); + } + + #endregion + + #region WhereDownloadable + + [Test] + public void WhereDownloadable_Keeps_Ordinary_Audio_Tracks() + { + var tracks = new[] + { + _fakeTrackFactory.CreateTrackWithId(1), + _fakeTrackFactory.CreateTrackWithId(2) + }; + + CollectionAssert.AreEquivalent( + new[] { 1, 2 }, + tracks.WhereDownloadable().Select(track => track.Id)); + } + + /// + /// Mirrors the filter in TrackCollectionOfflineTrackProvider. A video left in the list would make + /// "is everything downloaded" wait forever for a file that is never requested. + /// + [Test] + public void WhereDownloadable_Excludes_Videos() + { + var video = _fakeTrackFactory.CreateTrackWithId(2); + video.Subtype = TrackSubType.Video; + + var tracks = new[] { _fakeTrackFactory.CreateTrackWithId(1), video }; + + CollectionAssert.AreEquivalent( + new[] { 1 }, + tracks.WhereDownloadable().Select(track => track.Id)); + } + + /// + /// Mirrors the filter in GlobalTrackProvider: a track with no url is never enqueued, so it must + /// not be expected on disk either. + /// + [Test] + public void WhereDownloadable_Excludes_Tracks_Without_A_Url() + { + var tracks = new[] + { + _fakeTrackFactory.CreateTrackWithId(1), + TrackWithMedia(2, AudioMedia(File(100, url: null))), + new Track { Id = 3, Language = "nb", Media = null } + }; + + CollectionAssert.AreEquivalent( + new[] { 1 }, + tracks.WhereDownloadable().Select(track => track.Id)); + } + + [Test] + public void WhereDownloadable_Excludes_Tracks_With_An_Empty_Url() + { + var tracks = new[] { TrackWithMedia(1, AudioMedia(File(100, url: string.Empty))) }; + + CollectionAssert.IsEmpty(tracks.WhereDownloadable()); + } + + [Test] + public void WhereDownloadable_Skips_Null_Tracks() + { + var tracks = new[] { _fakeTrackFactory.CreateTrackWithId(1), null }; + + CollectionAssert.AreEquivalent( + new[] { 1 }, + tracks.WhereDownloadable().Select(track => track.Id)); + } + + [Test] + public void WhereDownloadable_Handles_A_Null_Collection() + { + IEnumerable tracks = null; + + CollectionAssert.IsEmpty(tracks.WhereDownloadable()); + } + + #endregion + } +} diff --git a/BMM.Tests/BMM.Core.Test/Unit/Implementations/Downloading/DownloadQueueTests.cs b/BMM.Tests/BMM.Core.Test/Unit/Implementations/Downloading/DownloadQueueTests.cs index cd3d96aef..d17f9af0f 100644 --- a/BMM.Tests/BMM.Core.Test/Unit/Implementations/Downloading/DownloadQueueTests.cs +++ b/BMM.Tests/BMM.Core.Test/Unit/Implementations/Downloading/DownloadQueueTests.cs @@ -1,7 +1,12 @@ using System; using System.Linq; +using System.Net; +using System.Net.Http; using System.Threading.Tasks; +using BMM.Api.Framework; +using BMM.Api.Implementation.Models; using BMM.Core.Implementations.Analytics; +using BMM.Core.Implementations.Connection; using BMM.Core.Implementations.Downloading.DownloadQueue; using BMM.Core.Implementations.Downloading.FileDownloader; using BMM.Core.Implementations.Exceptions; @@ -19,6 +24,10 @@ public class DownloadQueueTests private Mock _mvxMessenger; private Mock _exceptionHandler; private Mock _analytics; + private Mock _connection; + private Mock _networkSettings; + private Mock _logger; + private Task _queueRun; [SetUp] public void Init() @@ -27,18 +36,43 @@ public void Init() _mvxMessenger = new Mock(); _exceptionHandler = new Mock(); _analytics = new Mock(); + _connection = new Mock(); + _networkSettings = new Mock(); + _logger = new Mock(); + _queueRun = Task.CompletedTask; + + _connection.Setup(x => x.GetStatus()).Returns(ConnectionStatus.Online); + _connection.Setup(x => x.IsUsingNetworkWithoutExtraCosts()).Returns(true); + _networkSettings.Setup(x => x.GetMobileNetworkDownloadAllowed()).ReturnsAsync(true); + + // The queue hands its work to the exception handler, so capture it and run it inline. + _exceptionHandler + .Setup(x => x.FireAndForgetWithoutUserMessages(It.IsAny>())) + .Callback>(action => _queueRun = action()); } - private DownloadQueue CreateDownloadQueue() + private TestableDownloadQueue CreateDownloadQueue() { - return new DownloadQueue( + return new TestableDownloadQueue( _fileDownloader.Object, _mvxMessenger.Object, _exceptionHandler.Object, - _analytics.Object + _analytics.Object, + _connection.Object, + _networkSettings.Object, + _logger.Object ); } + private async Task RunQueueWith(params Track[] tracks) + { + var downloadQueue = CreateDownloadQueue(); + downloadQueue.Enqueue(tracks); + downloadQueue.StartDownloading(); + await _queueRun; + return downloadQueue; + } + [Test] public void Downloadable_Gets_Queued() { @@ -91,5 +125,155 @@ public void DownloadQueue_Should_Start_Downloading() _exceptionHandler.Verify(x => x.FireAndForgetWithoutUserMessages(It.IsAny>()), Times.Once); } + + [Test] + public async Task Successful_Downloads_Empty_The_Queue() + { + var tracks = Enumerable.Range(1, 3).Select(_fakeTrackFactory.CreateTrackWithId).ToArray(); + + var downloadQueue = await RunQueueWith(tracks); + + Assert.AreEqual(0, downloadQueue.RemainingDownloadsCount); + Assert.IsFalse(downloadQueue.StoppedWithPendingDownloads); + _mvxMessenger.Verify(x => x.Publish(It.Is(m => m.Succeeded)), Times.Once); + } + + [Test] + public async Task A_Connection_Failure_Stops_The_Queue_Instead_Of_Draining_It() + { + var tracks = Enumerable.Range(1, 5).Select(_fakeTrackFactory.CreateTrackWithId).ToArray(); + _fileDownloader + .Setup(x => x.DownloadFile(It.IsAny())) + .ThrowsAsync(new HttpRequestException("Connection failure")); + + var downloadQueue = await RunQueueWith(tracks); + + // The whole point: a dead connection must not turn five queued tracks into five instant + // failures and an empty queue that looks like a finished download. + Assert.AreEqual(5, downloadQueue.RemainingDownloadsCount); + Assert.IsTrue(downloadQueue.StoppedWithPendingDownloads); + _fileDownloader.Verify(x => x.DownloadFile(It.IsAny()), Times.Exactly(3)); + } + + [Test] + public async Task A_Stopped_Queue_Does_Not_Report_Success() + { + _fileDownloader + .Setup(x => x.DownloadFile(It.IsAny())) + .ThrowsAsync(new HttpRequestException("Connection failure")); + + await RunQueueWith(_fakeTrackFactory.CreateTrackWithId(1)); + + _mvxMessenger.Verify(x => x.Publish(It.Is(m => m.Succeeded)), Times.Never); + _mvxMessenger.Verify(x => x.Publish(It.Is(m => !m.Succeeded)), Times.Once); + } + + [Test] + public async Task A_Stopped_Queue_Is_Not_Still_Running() + { + _fileDownloader + .Setup(x => x.DownloadFile(It.IsAny())) + .ThrowsAsync(new HttpRequestException("Connection failure")); + + var downloadQueue = await RunQueueWith(_fakeTrackFactory.CreateTrackWithId(1)); + + // Items are still queued, but nothing is downloading them. Reporting otherwise leaves a + // progress indicator on screen for a download that is not happening. + Assert.IsFalse(downloadQueue.IsRunning); + Assert.AreEqual(1, downloadQueue.RemainingDownloadsCount); + } + + [Test] + public async Task A_Failed_Download_Is_Not_Reported_As_Downloaded() + { + _fileDownloader + .Setup(x => x.DownloadFile(It.IsAny())) + .ThrowsAsync(new HttpRequestException("Connection failure")); + + await RunQueueWith(_fakeTrackFactory.CreateTrackWithId(1)); + + _analytics.Verify( + x => x.LogEvent(Event.TrackHasBeenDownloaded, It.IsAny>()), + Times.Never); + } + + [Test] + public async Task A_Missing_File_Is_Skipped_So_The_Rest_Of_The_Queue_Continues() + { + var missing = _fakeTrackFactory.CreateTrackWithId(1); + var tracks = new[] { missing, _fakeTrackFactory.CreateTrackWithId(2), _fakeTrackFactory.CreateTrackWithId(3) }; + + _fileDownloader + .Setup(x => x.DownloadFile(It.Is(d => d.Id == missing.Id))) + .ThrowsAsync(new DownloadHttpStatusException(HttpStatusCode.NotFound)); + + var downloadQueue = await RunQueueWith(tracks); + + Assert.AreEqual(0, downloadQueue.RemainingDownloadsCount); + _fileDownloader.Verify(x => x.DownloadFile(It.IsAny()), Times.Exactly(3)); + } + + [Test] + public async Task Nothing_Is_Downloaded_While_The_Device_Is_Offline() + { + _connection.Setup(x => x.GetStatus()).Returns(ConnectionStatus.Offline); + var tracks = Enumerable.Range(1, 3).Select(_fakeTrackFactory.CreateTrackWithId).ToArray(); + + var downloadQueue = await RunQueueWith(tracks); + + _fileDownloader.Verify(x => x.DownloadFile(It.IsAny()), Times.Never); + Assert.AreEqual(3, downloadQueue.RemainingDownloadsCount); + Assert.IsTrue(downloadQueue.StoppedWithPendingDownloads); + } + + [Test] + public async Task Downloading_Stops_When_The_User_Leaves_A_Free_Network() + { + _connection.Setup(x => x.IsUsingNetworkWithoutExtraCosts()).Returns(false); + _networkSettings.Setup(x => x.GetMobileNetworkDownloadAllowed()).ReturnsAsync(false); + + var downloadQueue = await RunQueueWith(_fakeTrackFactory.CreateTrackWithId(1)); + + _fileDownloader.Verify(x => x.DownloadFile(It.IsAny()), Times.Never); + Assert.AreEqual(1, downloadQueue.RemainingDownloadsCount); + } + + [Test] + public async Task A_Transient_Failure_Is_Retried() + { + int attempts = 0; + _fileDownloader + .Setup(x => x.DownloadFile(It.IsAny())) + .Returns(() => + { + attempts++; + return attempts == 1 + ? Task.FromException(new HttpRequestException("Connection failure")) + : Task.CompletedTask; + }); + + var downloadQueue = await RunQueueWith(_fakeTrackFactory.CreateTrackWithId(1)); + + Assert.AreEqual(2, attempts); + Assert.AreEqual(0, downloadQueue.RemainingDownloadsCount); + Assert.IsFalse(downloadQueue.StoppedWithPendingDownloads); + } + + private class TestableDownloadQueue : DownloadQueue + { + public TestableDownloadQueue( + IFileDownloader fileDownloader, + IMvxMessenger messenger, + IExceptionHandler exceptionHandler, + IAnalytics analytics, + IConnection connection, + INetworkSettings networkSettings, + ILogger logger) + : base(fileDownloader, messenger, exceptionHandler, analytics, connection, networkSettings, logger) + { + } + + protected override Task WaitBeforeRetry(TimeSpan delay) => Task.CompletedTask; + } } -} \ No newline at end of file +} diff --git a/BMM.Tests/BMM.Core.Test/Unit/Implementations/FileStorage/GlobalTrackProviderTests.cs b/BMM.Tests/BMM.Core.Test/Unit/Implementations/FileStorage/GlobalTrackProviderTests.cs index 4f23c23e7..66bb4aab3 100644 --- a/BMM.Tests/BMM.Core.Test/Unit/Implementations/FileStorage/GlobalTrackProviderTests.cs +++ b/BMM.Tests/BMM.Core.Test/Unit/Implementations/FileStorage/GlobalTrackProviderTests.cs @@ -2,6 +2,7 @@ using System.Threading.Tasks; using BMM.Api.Implementation.Models; using BMM.Core.Implementations.Albums.Interfaces; +using BMM.Core.Implementations.Analytics; using BMM.Core.Implementations.Downloading; using BMM.Core.Implementations.PlaylistPersistence; using BMM.Core.Implementations.Podcasts; @@ -19,6 +20,7 @@ public class GlobalTrackProviderTests private Mock _trackCollectionProvider; private Mock _playlistProvider; private Mock _albumProvider; + private Mock _analytics; private readonly FakeTrackFactory _fakeTrackFactory = new FakeTrackFactory(); [SetUp] @@ -28,12 +30,13 @@ public void Setup() _trackCollectionProvider = new Mock(); _playlistProvider = new Mock(); _albumProvider = new Mock(); + _analytics = new Mock(); } [Test] public async Task TrackProvider_Should_Combine_Tracks_From_Providers() { - var globalTrackProvider = new GlobalTrackProvider(_podcastTrackProvider.Object, _trackCollectionProvider.Object, _playlistProvider.Object, _albumProvider.Object); + var globalTrackProvider = new GlobalTrackProvider(_podcastTrackProvider.Object, _trackCollectionProvider.Object, _playlistProvider.Object, _albumProvider.Object, _analytics.Object); var podcastTracks = new List { @@ -82,7 +85,7 @@ public async Task TrackProvider_Should_Combine_Tracks_From_Providers() [Test] public async Task TrackProvider_Should_Filter_Out_Tracks_With_Null_Url() { - var globalTrackProvider = new GlobalTrackProvider(_podcastTrackProvider.Object, _trackCollectionProvider.Object, _playlistProvider.Object, _albumProvider.Object); + var globalTrackProvider = new GlobalTrackProvider(_podcastTrackProvider.Object, _trackCollectionProvider.Object, _playlistProvider.Object, _albumProvider.Object, _analytics.Object); var tracks = new List { @@ -114,7 +117,7 @@ public async Task TrackProvider_Should_Filter_Out_Tracks_With_Null_Url() [Test] public async Task TrackProvider_Should_Report_Incomplete_When_Any_Provider_Is_Incomplete() { - var globalTrackProvider = new GlobalTrackProvider(_podcastTrackProvider.Object, _trackCollectionProvider.Object, _playlistProvider.Object, _albumProvider.Object); + var globalTrackProvider = new GlobalTrackProvider(_podcastTrackProvider.Object, _trackCollectionProvider.Object, _playlistProvider.Object, _albumProvider.Object, _analytics.Object); var podcastTracks = new List { _fakeTrackFactory.CreateTrackWithId(1) }; diff --git a/BMM.UI.Android/Resources/layout/listitem_trackcollection_header.axml b/BMM.UI.Android/Resources/layout/listitem_trackcollection_header.axml index 3102046cc..fdfa1271d 100644 --- a/BMM.UI.Android/Resources/layout/listitem_trackcollection_header.axml +++ b/BMM.UI.Android/Resources/layout/listitem_trackcollection_header.axml @@ -47,7 +47,7 @@ android:layout_width="wrap_content" android:layout_height="wrap_content" android:drawableTint="@color/label_one_color" - app:MvxBind="Click ToggleOfflineCommand; Visibility InvertedVisibility(IsOfflineAvailable)"/> + app:MvxBind="Click ToggleOfflineCommand; Visibility Visibility(ShowDownloadButton)"/> + app:MvxBind="Click ToggleOfflineCommand; Visibility Visibility(ShowDownloadButton)"/> $"Error while downloading file {_downloadRequestUrl}: {_error.Description}"; + /// + /// The error is allowed to be null: a failure detected by the app itself (a corrupt file, a move + /// that did not happen) has no behind it. Dereferencing it unconditionally + /// used to throw a from inside the download queue's own catch + /// block, which aborted the entire queue rather than skipping one file. + /// + public override string Message => + $"Error while downloading file {_downloadRequestUrl}: {_error?.Description ?? "no error details"}"; public IosDownloadException(string downloadRequestUrl, NSError error) { diff --git a/BMM.UI.iOS/Application/Implementations/Download/IosFileDownloader.cs b/BMM.UI.iOS/Application/Implementations/Download/IosFileDownloader.cs index 8377abce5..e1afbe5b2 100644 --- a/BMM.UI.iOS/Application/Implementations/Download/IosFileDownloader.cs +++ b/BMM.UI.iOS/Application/Implementations/Download/IosFileDownloader.cs @@ -129,13 +129,21 @@ private bool IsFileCorrupted(string localPath) // Todo #20112 remove logging after resolving the issue private void RecoverFromCorruptFile(NSUrl localPath, IDownloadable originalDownloadable, out NSError error) { - _fileManager.Remove(localPath, out error); + _fileManager.Remove(localPath, out _); _analytics.LogEvent("Downloaded file is corrupted", new Dictionary { {"trackId", originalDownloadable.Id} }); - error = null; + + // Report the corruption rather than handing back a null error. The caller treats this as a + // failed download either way, and a null error left the reason unreportable. + error = new NSError( + new NSString("BmmDownloadErrorDomain"), + 1, + NSDictionary.FromObjectAndKey( + new NSString($"The downloaded file for track {originalDownloadable.Id} was corrupt and has been removed"), + NSError.LocalizedDescriptionKey)); } } } \ No newline at end of file From e1d6b0d2d250966e1fd3fe904cd2bfa5fb60784d Mon Sep 17 00:00:00 2001 From: Sigve Hansen Date: Mon, 14 Sep 2026 15:02:27 +0200 Subject: [PATCH 2/4] feat(downloads): connection aware download exception on ios --- .../DownloadFailureClassifier.cs | 6 ++ .../IConnectionAwareDownloadException.cs | 12 +++ .../DownloadFailureClassifierTests.cs | 102 ++++++++++++++++++ .../Exceptions/IosDownloadException.cs | 31 +++++- 4 files changed, 149 insertions(+), 2 deletions(-) create mode 100644 BMM.Core/Implementations/Downloading/FileDownloader/IConnectionAwareDownloadException.cs create mode 100644 BMM.Tests/BMM.Core.Test/Unit/Implementations/Downloading/DownloadFailureClassifierTests.cs diff --git a/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadFailureClassifier.cs b/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadFailureClassifier.cs index 8219abbda..3eed8a348 100644 --- a/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadFailureClassifier.cs +++ b/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadFailureClassifier.cs @@ -41,6 +41,12 @@ public static DownloadOutcome Classify(Exception exception, bool cancellationWas DownloadHttpStatusException statusException => ClassifyStatusCode(statusException.StatusCode), + // Platform downloaders do not fail with framework exception types. iOS reports an NSError + // instead, so it has to tell us itself whether the network was at fault. + IConnectionAwareDownloadException platformException => platformException.WasConnectionFailure + ? DownloadOutcome.ConnectionFailure + : DownloadOutcome.PermanentFailure, + _ => IsConnectionFailure(exception.InnerException, cancellationWasRequested) ? DownloadOutcome.ConnectionFailure : DownloadOutcome.PermanentFailure diff --git a/BMM.Core/Implementations/Downloading/FileDownloader/IConnectionAwareDownloadException.cs b/BMM.Core/Implementations/Downloading/FileDownloader/IConnectionAwareDownloadException.cs new file mode 100644 index 000000000..5192cbb7b --- /dev/null +++ b/BMM.Core/Implementations/Downloading/FileDownloader/IConnectionAwareDownloadException.cs @@ -0,0 +1,12 @@ +namespace BMM.Core.Implementations.Downloading.FileDownloader +{ + /// + /// Implemented by platform specific download exceptions that can say whether the failure was the + /// network rather than the file itself. It lets the download queue stop and keep its items instead of + /// skipping them, without BMM.Core having to know about NSError or its Android equivalents. + /// + public interface IConnectionAwareDownloadException + { + bool WasConnectionFailure { get; } + } +} diff --git a/BMM.Tests/BMM.Core.Test/Unit/Implementations/Downloading/DownloadFailureClassifierTests.cs b/BMM.Tests/BMM.Core.Test/Unit/Implementations/Downloading/DownloadFailureClassifierTests.cs new file mode 100644 index 000000000..4ce61e156 --- /dev/null +++ b/BMM.Tests/BMM.Core.Test/Unit/Implementations/Downloading/DownloadFailureClassifierTests.cs @@ -0,0 +1,102 @@ +using System; +using System.IO; +using System.Net; +using System.Net.Http; +using System.Net.Sockets; +using System.Threading.Tasks; +using BMM.Api.Framework.Exceptions; +using BMM.Core.Implementations.Downloading.DownloadQueue; +using BMM.Core.Implementations.Downloading.FileDownloader; +using NUnit.Framework; + +namespace BMM.Core.Test.Unit.Implementations.Downloading +{ + [TestFixture] + public class DownloadFailureClassifierTests + { + private static DownloadOutcome Classify(Exception exception, bool cancellationWasRequested = false) + => DownloadFailureClassifier.Classify(exception, cancellationWasRequested); + + [Test] + public void Http_Request_Failures_Are_Connection_Failures() + { + // "Connection failure", "Software caused connection abort", "Socket closed" and + // "Unable to resolve host ..." all reach us as HttpRequestException. + Assert.AreEqual(DownloadOutcome.ConnectionFailure, Classify(new HttpRequestException("Connection failure"))); + } + + [Test] + public void Socket_And_Web_And_Io_Failures_Are_Connection_Failures() + { + Assert.AreEqual(DownloadOutcome.ConnectionFailure, Classify(new SocketException())); + Assert.AreEqual(DownloadOutcome.ConnectionFailure, Classify(new WebException())); + Assert.AreEqual(DownloadOutcome.ConnectionFailure, Classify(new IOException())); + Assert.AreEqual(DownloadOutcome.ConnectionFailure, Classify(new InternetProblemsException(new Exception()))); + } + + [Test] + public void A_Timeout_Is_A_Connection_Failure() + { + Assert.AreEqual(DownloadOutcome.ConnectionFailure, Classify(new TaskCanceledException())); + } + + [Test] + public void A_Cancellation_We_Asked_For_Is_Not_A_Failure() + { + Assert.AreEqual(DownloadOutcome.Cancelled, Classify(new TaskCanceledException(), cancellationWasRequested: true)); + } + + [Test] + public void A_Connection_Failure_Nested_In_Another_Exception_Is_Still_A_Connection_Failure() + { + var wrapped = new Exception("wrapped", new HttpRequestException("Connection failure")); + + Assert.AreEqual(DownloadOutcome.ConnectionFailure, Classify(wrapped)); + } + + [Test] + public void A_Missing_File_Is_A_Permanent_Failure() + { + Assert.AreEqual(DownloadOutcome.PermanentFailure, Classify(new DownloadHttpStatusException(HttpStatusCode.NotFound))); + Assert.AreEqual(DownloadOutcome.PermanentFailure, Classify(new DownloadHttpStatusException(HttpStatusCode.Forbidden))); + } + + [Test] + public void Statuses_Worth_Retrying_Are_Connection_Failures() + { + // 401 means the access token needs refreshing, 5xx and 429 are the server asking us to + // come back. Skipping the file for those would lose it until the next full synchronization. + Assert.AreEqual(DownloadOutcome.ConnectionFailure, Classify(new DownloadHttpStatusException(HttpStatusCode.Unauthorized))); + Assert.AreEqual(DownloadOutcome.ConnectionFailure, Classify(new DownloadHttpStatusException(HttpStatusCode.TooManyRequests))); + Assert.AreEqual(DownloadOutcome.ConnectionFailure, Classify(new DownloadHttpStatusException(HttpStatusCode.BadGateway))); + } + + /// + /// The iOS downloader fails with an NSError rather than a framework exception type, so it reports + /// the verdict itself. Without this the queue would treat "not connected to the internet" on iOS + /// as a broken file and skip every remaining track. + /// + [Test] + public void A_Platform_Exception_Decides_For_Itself() + { + Assert.AreEqual(DownloadOutcome.ConnectionFailure, Classify(new FakePlatformDownloadException(wasConnectionFailure: true))); + Assert.AreEqual(DownloadOutcome.PermanentFailure, Classify(new FakePlatformDownloadException(wasConnectionFailure: false))); + } + + [Test] + public void An_Unrecognised_Failure_Is_Permanent_So_The_Queue_Continues() + { + Assert.AreEqual(DownloadOutcome.PermanentFailure, Classify(new InvalidOperationException())); + } + + private class FakePlatformDownloadException : Exception, IConnectionAwareDownloadException + { + public FakePlatformDownloadException(bool wasConnectionFailure) + { + WasConnectionFailure = wasConnectionFailure; + } + + public bool WasConnectionFailure { get; } + } + } +} diff --git a/BMM.UI.iOS/Application/Implementations/Download/Exceptions/IosDownloadException.cs b/BMM.UI.iOS/Application/Implementations/Download/Exceptions/IosDownloadException.cs index e8895660e..d668b6a17 100644 --- a/BMM.UI.iOS/Application/Implementations/Download/Exceptions/IosDownloadException.cs +++ b/BMM.UI.iOS/Application/Implementations/Download/Exceptions/IosDownloadException.cs @@ -1,10 +1,32 @@ using System; +using System.Linq; +using BMM.Core.Implementations.Downloading.FileDownloader; using Foundation; namespace BMM.UI.iOS.Implementations.Download.Exceptions { - public class IosDownloadException : Exception + public class IosDownloadException : Exception, IConnectionAwareDownloadException { + private const string UrlErrorDomain = "NSURLErrorDomain"; + + /// + /// NSURLError codes that mean "the network let us down", as opposed to something being wrong with + /// this particular file. The download queue keeps its remaining items for these instead of + /// skipping them, since every other download would fail the same way. + /// + private static readonly int[] ConnectionErrorCodes = + { + -1001, // TimedOut + -1003, // CannotFindHost + -1004, // CannotConnectToHost + -1005, // NetworkConnectionLost + -1009, // NotConnectedToInternet + -1018, // InternationalRoamingOff + -1019, // CallIsActive + -1020, // DataNotAllowed + -997 // Lost connection to the background transfer daemon + }; + private readonly string _downloadRequestUrl; private readonly NSError _error; @@ -17,10 +39,15 @@ public class IosDownloadException : Exception public override string Message => $"Error while downloading file {_downloadRequestUrl}: {_error?.Description ?? "no error details"}"; + public bool WasConnectionFailure => + _error != null + && _error.Domain == UrlErrorDomain + && ConnectionErrorCodes.Contains((int)_error.Code); + public IosDownloadException(string downloadRequestUrl, NSError error) { _downloadRequestUrl = downloadRequestUrl; _error = error; } } -} \ No newline at end of file +} From 6f6981bb5d6b47c31ebcf52f21a586b8a309600f Mon Sep 17 00:00:00 2001 From: Sigve Hansen Date: Tue, 15 Sep 2026 10:12:50 +0200 Subject: [PATCH 3/4] fix(downloads): mark permanently failed downloads as such to allow the rest of the collection to download --- BMM.Core/App.cs | 1 + .../DownloadQueue/DownloadOutcome.cs | 8 +- .../DownloadQueue/DownloadQueue.cs | 101 +++++++++++- .../Downloading/IUnavailableTrackRegistry.cs | 23 +++ .../Downloading/UnavailableTrackRegistry.cs | 54 +++++++ .../Implementations/Storage/AppSettings.cs | 11 ++ BMM.Core/ViewModels/AlbumViewModel.cs | 7 +- BMM.Core/ViewModels/Base/DownloadViewModel.cs | 43 +++++- .../ViewModels/CuratedPlaylistViewModel.cs | 7 +- .../ViewModels/MyContent/MyTracksViewModel.cs | 7 +- .../SharedTrackCollectionViewModel.cs | 9 +- .../ViewModels/TopSongsCollectionViewModel.cs | 9 +- .../ViewModels/TrackCollectionViewModel.cs | 9 +- .../Downloading/DownloadQueueTests.cs | 144 +++++++++++++++++- .../Unit/ViewModels/AlbumViewModelTests.cs | 5 +- 15 files changed, 410 insertions(+), 28 deletions(-) create mode 100644 BMM.Core/Implementations/Downloading/IUnavailableTrackRegistry.cs create mode 100644 BMM.Core/Implementations/Downloading/UnavailableTrackRegistry.cs diff --git a/BMM.Core/App.cs b/BMM.Core/App.cs index 817c2b89e..dcc0f49c0 100644 --- a/BMM.Core/App.cs +++ b/BMM.Core/App.cs @@ -208,6 +208,7 @@ public override void Initialize() Mvx.IoCProvider.LazyConstructAndRegisterSingleton(); Mvx.IoCProvider.RegisterType(); Mvx.IoCProvider.ConstructAndRegisterSingletonIfNotRegistered(); + Mvx.IoCProvider.LazyConstructAndRegisterSingleton(); Mvx.IoCProvider.LazyConstructAndRegisterSingleton(); Mvx.IoCProvider.LazyConstructAndRegisterSingleton(); diff --git a/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadOutcome.cs b/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadOutcome.cs index ddc13fe1e..29b5e4a89 100644 --- a/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadOutcome.cs +++ b/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadOutcome.cs @@ -23,6 +23,12 @@ public enum DownloadOutcome /// /// The download was cancelled deliberately (user interaction, app shutdown). /// - Cancelled + Cancelled, + + /// + /// The device has run out of storage. Like a connection failure the remaining items are kept, but + /// retrying is pointless until the user frees something up, so the queue must not spin on it. + /// + OutOfSpace } } diff --git a/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadQueue.cs b/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadQueue.cs index 5f0a817cf..199f0caa6 100644 --- a/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadQueue.cs +++ b/BMM.Core/Implementations/Downloading/DownloadQueue/DownloadQueue.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.IO; using System.Linq; using System.Threading.Tasks; using BMM.Api.Framework; @@ -9,12 +10,19 @@ using BMM.Core.Implementations.Connection; using BMM.Core.Implementations.Downloading.FileDownloader; using BMM.Core.Implementations.Exceptions; +using BMM.Core.Implementations.FileStorage; using MvvmCross.Plugin.Messenger; namespace BMM.Core.Implementations.Downloading.DownloadQueue { public class DownloadQueue : IDownloadQueue { + /// + /// Headroom below which we call the storage full. It has to exceed the size of a single track so + /// that reclaiming one partial download does not make a full disk look usable again. + /// + private const long MinimumUsableFreeSpaceBytes = 25 * 1024 * 1024; + /// /// A connection can be down for a moment without being down. Retrying a couple of times with a /// growing pause absorbs that without hammering a network that is genuinely gone. @@ -32,6 +40,8 @@ public class DownloadQueue : IDownloadQueue private readonly IAnalytics _analytics; private readonly IConnection _connection; private readonly INetworkSettings _networkSettings; + private readonly IStorageManager _storageManager; + private readonly IUnavailableTrackRegistry _unavailableTracks; private readonly ILogger _logger; private readonly ConcurrentBag _queuedDownloads = new ConcurrentBag(); private IDownloadable _currentDownloadingDownloadable; @@ -46,6 +56,8 @@ public DownloadQueue( IAnalytics analytics, IConnection connection, INetworkSettings networkSettings, + IStorageManager storageManager, + IUnavailableTrackRegistry unavailableTracks, ILogger logger) { _fileDownloader = fileDownloader; @@ -54,6 +66,8 @@ public DownloadQueue( _analytics = analytics; _connection = connection; _networkSettings = networkSettings; + _storageManager = storageManager; + _unavailableTracks = unavailableTracks; _logger = logger; } @@ -163,6 +177,8 @@ private async Task RunQueue() case DownloadOutcome.Success: succeeded++; _finishedDownloadCount++; + // A track that downloads is available again, whatever we concluded before. + _unavailableTracks.MarkAvailable(item.Id); _messenger.Publish(new FileDownloadCompletedMessage(this, item.Id)); _analytics.LogEvent(Event.TrackHasBeenDownloaded, PrepareAdditionalEventArguments(item)); break; @@ -173,6 +189,10 @@ private async Task RunQueue() // row, which would otherwise keep showing a download in progress. skipped++; _finishedDownloadCount++; + // Remembered so the rest of the collection can still count as downloaded. The + // synchronization keeps offering this track, so it un-marks itself if it ever + // comes back. + _unavailableTracks.MarkUnavailable(item.Id); _messenger.Publish(new FileDownloadCanceledMessage(this, exception)); break; @@ -184,6 +204,15 @@ private async Task RunQueue() stopReason = QueueStopReason.NoConnection; break; + case DownloadOutcome.OutOfSpace: + // Keep the item, but stop without retrying: no amount of waiting frees disk. + _queuedDownloads.Add(item); + _messenger.Publish(new FileDownloadCanceledMessage( + this, + new StorageOutOfSpaceException(_storageManager.SelectedStorage))); + stopReason = QueueStopReason.OutOfSpace; + break; + case DownloadOutcome.Cancelled: _queuedDownloads.Add(item); stopReason = QueueStopReason.Cancelled; @@ -194,6 +223,14 @@ private async Task RunQueue() break; } } + catch (Exception exception) + { + // Anything unexpected here abandons the rest of the queue, so it has to be recorded as a + // queue outcome. Letting it escape would leave the run indistinguishable from a normal one + // in the queue's own telemetry. + stopReason = QueueStopReason.Aborted; + _logger.Error(nameof(DownloadQueue), "The download queue stopped with an unexpected error", exception); + } finally { _currentDownloadingDownloadable = null; @@ -268,6 +305,49 @@ private async Task IsDownloadingAllowed() || await _networkSettings.GetMobileNetworkDownloadAllowed(); } + /// + /// Separates "the disk is full" from "the stream died", which .NET reports as the same + /// . Without asking the file system the two are indistinguishable, and a + /// full disk treated as a connection problem would have the queue retrying forever. + /// + private DownloadOutcome RefineWithStorageState(DownloadOutcome outcome, Exception exception) + { + if (outcome != DownloadOutcome.ConnectionFailure || !IsOrWraps(exception)) + return outcome; + + return HasUsableFreeSpace() + ? outcome + : DownloadOutcome.OutOfSpace; + } + + private bool HasUsableFreeSpace() + { + try + { + // The downloader deletes its partial file before the exception reaches us, so a little + // space has just been reclaimed. The threshold has to sit above a single track's size for + // a genuinely full disk to still read as full. + return _storageManager.SelectedStorage.FreeSpace > MinimumUsableFreeSpaceBytes; + } + catch (Exception exception) + { + // Storage that cannot even be queried is not evidence of a full disk. + _logger.Error(nameof(DownloadQueue), "Could not read the free space of the selected storage", exception); + return true; + } + } + + private static bool IsOrWraps(Exception exception) where TException : Exception + { + for (var current = exception; current != null; current = current.InnerException) + { + if (current is TException) + return true; + } + + return false; + } + private static string DescribeException(Exception exception) { try @@ -299,7 +379,7 @@ private static Dictionary PrepareAdditionalEventArguments(IDownl } catch (Exception e) { - var outcome = DownloadFailureClassifier.Classify(e, _cancellationWasRequested); + var outcome = RefineWithStorageState(DownloadFailureClassifier.Classify(e, _cancellationWasRequested), e); if (outcome == DownloadOutcome.Cancelled) return (outcome, e); @@ -332,14 +412,21 @@ private static Dictionary PrepareAdditionalEventArguments(IDownl /// private void LogQueueOutcome(QueueStopReason stopReason, int succeeded, int skipped) { - if (stopReason == QueueStopReason.Completed && !StoppedWithPendingDownloads) + if (stopReason == QueueStopReason.Cancelled) return; - if (stopReason == QueueStopReason.Cancelled) + bool ranToCompletion = stopReason == QueueStopReason.Completed && !StoppedWithPendingDownloads; + + // A run that finished but gave up on some files is not a success. Those tracks will never + // arrive, so the collection can never show as fully downloaded, and without this the only + // record would be the individual failures with nothing tying them together. + if (ranToCompletion && skipped == 0) return; - string message = $"Download queue stopped early ({stopReason}). " + - $"Succeeded: {succeeded}, skipped: {skipped}, still queued: {_queuedDownloads.Count}."; + string message = ranToCompletion + ? $"Download queue finished but abandoned {skipped} file(s). Succeeded: {succeeded}." + : $"Download queue stopped early ({stopReason}). " + + $"Succeeded: {succeeded}, skipped: {skipped}, still queued: {_queuedDownloads.Count}."; _logger.Error(nameof(DownloadQueue), message); @@ -358,7 +445,9 @@ private enum QueueStopReason { Completed, NoConnection, - Cancelled + OutOfSpace, + Cancelled, + Aborted } } } diff --git a/BMM.Core/Implementations/Downloading/IUnavailableTrackRegistry.cs b/BMM.Core/Implementations/Downloading/IUnavailableTrackRegistry.cs new file mode 100644 index 000000000..76c2113ae --- /dev/null +++ b/BMM.Core/Implementations/Downloading/IUnavailableTrackRegistry.cs @@ -0,0 +1,23 @@ +namespace BMM.Core.Implementations.Downloading +{ + /// + /// Remembers tracks the server refuses to hand over, so that a single unobtainable track cannot keep a + /// whole collection reporting itself as not downloaded. + /// + /// + /// This only affects how "is this fully downloaded" is judged. The synchronization keeps offering these + /// tracks to the queue on every run, so a track that becomes available again is picked up by itself and + /// forgotten here the moment it succeeds. + /// + public interface IUnavailableTrackRegistry + { + bool IsUnavailable(int trackId); + + void MarkUnavailable(int trackId); + + /// + /// Called after a successful download. Cheap no-op when the track was never marked. + /// + void MarkAvailable(int trackId); + } +} diff --git a/BMM.Core/Implementations/Downloading/UnavailableTrackRegistry.cs b/BMM.Core/Implementations/Downloading/UnavailableTrackRegistry.cs new file mode 100644 index 000000000..168ef5b62 --- /dev/null +++ b/BMM.Core/Implementations/Downloading/UnavailableTrackRegistry.cs @@ -0,0 +1,54 @@ +using System.Collections.Generic; +using BMM.Core.Implementations.Storage; + +namespace BMM.Core.Implementations.Downloading +{ + public class UnavailableTrackRegistry : IUnavailableTrackRegistry + { + private readonly object _lock = new(); + private HashSet _unavailableTracks; + + private HashSet UnavailableTracks + { + get + { + // Read through once and keep it in memory: this is consulted for every track of a + // collection each time the download state is refreshed. + _unavailableTracks ??= AppSettings.UnavailableTracks; + return _unavailableTracks; + } + } + + public bool IsUnavailable(int trackId) + { + lock (_lock) + { + return UnavailableTracks.Contains(trackId); + } + } + + public void MarkUnavailable(int trackId) + { + lock (_lock) + { + if (!UnavailableTracks.Add(trackId)) + return; + + Save(); + } + } + + public void MarkAvailable(int trackId) + { + lock (_lock) + { + if (!UnavailableTracks.Remove(trackId)) + return; + + Save(); + } + } + + private void Save() => AppSettings.UnavailableTracks = new HashSet(UnavailableTracks); + } +} diff --git a/BMM.Core/Implementations/Storage/AppSettings.cs b/BMM.Core/Implementations/Storage/AppSettings.cs index c02c6e68e..f2da537c7 100644 --- a/BMM.Core/Implementations/Storage/AppSettings.cs +++ b/BMM.Core/Implementations/Storage/AppSettings.cs @@ -169,6 +169,17 @@ public static HashSet LocalTrackCollections get => GetValueOrDefault(nameof(LocalTrackCollections), new HashSet()); set => AddOrUpdateValue(value, nameof(LocalTrackCollections)); } + + /// + /// Tracks the server will not give us, so that a playlist containing one is not stuck reporting + /// itself as unfinished forever. Persisted because the state has to survive a restart, and cleared + /// per track as soon as one does download. + /// + public static HashSet UnavailableTracks + { + get => GetValueOrDefault(nameof(UnavailableTracks), new HashSet()); + set => AddOrUpdateValue(value, nameof(UnavailableTracks)); + } public static TrackPlayedEvent UnfinishedTrackPlayedEvent { diff --git a/BMM.Core/ViewModels/AlbumViewModel.cs b/BMM.Core/ViewModels/AlbumViewModel.cs index f71b5beed..a0fa3cdb0 100644 --- a/BMM.Core/ViewModels/AlbumViewModel.cs +++ b/BMM.Core/ViewModels/AlbumViewModel.cs @@ -10,6 +10,7 @@ using BMM.Core.Implementations.Albums.Interfaces; using BMM.Core.Implementations.Connection; using BMM.Core.Implementations.DocumentFilters; +using BMM.Core.Implementations.Downloading; using BMM.Core.Implementations.Downloading.DownloadQueue; using BMM.Core.Implementations.Factories; using BMM.Core.Implementations.FileStorage; @@ -76,8 +77,10 @@ public AlbumViewModel( INetworkSettings networkSettings, IAlbumManager albumManager, IOfflineAlbumStorage offlineAlbumStorage, - IFirebaseRemoteConfig firebaseRemoteConfig) - : base(storageManager, documentFilter, downloadQueue, connection, networkSettings) + IFirebaseRemoteConfig firebaseRemoteConfig, + IUnavailableTrackRegistry unavailableTracks, + ILogger logger) + : base(storageManager, documentFilter, downloadQueue, connection, networkSettings, unavailableTracks, logger) { _playOrResumePlayAction = playOrResumePlayAction; _documentsPOFactory = documentsPOFactory; diff --git a/BMM.Core/ViewModels/Base/DownloadViewModel.cs b/BMM.Core/ViewModels/Base/DownloadViewModel.cs index 3ddf11d8b..f8bb6cf73 100644 --- a/BMM.Core/ViewModels/Base/DownloadViewModel.cs +++ b/BMM.Core/ViewModels/Base/DownloadViewModel.cs @@ -8,6 +8,7 @@ using BMM.Core.Helpers; using BMM.Core.Implementations.Connection; using BMM.Core.Implementations.DocumentFilters; +using BMM.Core.Implementations.Downloading; using BMM.Core.Implementations.Downloading.DownloadQueue; using BMM.Core.Implementations.Downloading.FileDownloader; using BMM.Core.Implementations.DownloadManager; @@ -67,13 +68,35 @@ private bool AreAllTracksDownloaded /// protected void RefreshDownloadedFilesState() { - var tracks = DownloadableTracks?.ToList(); + try + { + var tracks = DownloadableTracks?.ToList(); + + // An item we know nothing about yet must not be reported as incomplete, or opening a + // collection would briefly claim its downloads are missing. + if (tracks == null || tracks.Count == 0) + { + AreAllTracksDownloaded = true; + return; + } - // An item we know nothing about yet must not be reported as incomplete, or opening a - // collection would briefly claim its downloads are missing. - AreAllTracksDownloaded = tracks == null - || tracks.Count == 0 - || tracks.All(track => _storageManager.SelectedStorage.IsDownloaded(track)); + var storage = _storageManager.SelectedStorage; + + bool everythingObtainableIsHere = tracks.All(track => + storage.IsDownloaded(track) || _unavailableTracks.IsUnavailable(track.Id)); + + // People want to listen to what they downloaded, so one track the server will not give us + // must not hold the whole collection hostage. Requiring at least one real file keeps this + // from turning into a checkmark over nothing when everything failed. + AreAllTracksDownloaded = everythingObtainableIsHere && tracks.Any(storage.IsDownloaded); + } + catch (Exception exception) + { + // Reached from the download queue's own messages. Storage that cannot be read (a removed + // SD card, a storage manager that is not initialised yet) would otherwise throw back into + // the publisher and take the rest of the queue down with it. + _logger.Error(GetType().Name, "Could not determine which files of this item are downloaded", exception); + } } private int ToBeDownloadedCount => DownloadQueue.InitialDownloadCount; @@ -162,6 +185,8 @@ public float DownloadStatus public IMvxAsyncCommand ToggleOfflineCommand { get; private set; } private readonly IStorageManager _storageManager; + private readonly IUnavailableTrackRegistry _unavailableTracks; + private readonly ILogger _logger; protected readonly IDownloadQueue DownloadQueue; protected readonly IConnection Connection; @@ -179,10 +204,14 @@ public DownloadViewModel( IDocumentFilter documentFilter, IDownloadQueue downloadQueue, IConnection connection, - INetworkSettings networkSettings) + INetworkSettings networkSettings, + IUnavailableTrackRegistry unavailableTracks, + ILogger logger) : base(documentFilter) { _storageManager = storageManager; + _unavailableTracks = unavailableTracks; + _logger = logger; DownloadQueue = downloadQueue; Connection = connection; _networkSettings = networkSettings; diff --git a/BMM.Core/ViewModels/CuratedPlaylistViewModel.cs b/BMM.Core/ViewModels/CuratedPlaylistViewModel.cs index 0f1f417fa..1ea6b22bc 100644 --- a/BMM.Core/ViewModels/CuratedPlaylistViewModel.cs +++ b/BMM.Core/ViewModels/CuratedPlaylistViewModel.cs @@ -6,6 +6,7 @@ using BMM.Core.Implementations.Caching; using BMM.Core.Implementations.Connection; using BMM.Core.Implementations.DocumentFilters; +using BMM.Core.Implementations.Downloading; using BMM.Core.Implementations.Downloading.DownloadQueue; using BMM.Core.Implementations.Factories.Tracks; using BMM.Core.Implementations.FileStorage; @@ -65,8 +66,10 @@ public CuratedPlaylistViewModel( IShareLink shareLink, IMvxMainThreadAsyncDispatcher mainThreadAsyncDispatcher, IOfflinePlaylistStorage offlinePlaylistStorage, - IFirebaseRemoteConfig firebaseRemoteConfig) - : base(storageManager, documentFilter, downloadQueue, connection, networkSettings) + IFirebaseRemoteConfig firebaseRemoteConfig, + IUnavailableTrackRegistry unavailableTracks, + ILogger logger) + : base(storageManager, documentFilter, downloadQueue, connection, networkSettings, unavailableTracks, logger) { _playlistManager = playlistManager; _trackPOFactory = trackPOFactory; diff --git a/BMM.Core/ViewModels/MyContent/MyTracksViewModel.cs b/BMM.Core/ViewModels/MyContent/MyTracksViewModel.cs index 2094e293c..8a3a4addf 100644 --- a/BMM.Core/ViewModels/MyContent/MyTracksViewModel.cs +++ b/BMM.Core/ViewModels/MyContent/MyTracksViewModel.cs @@ -9,6 +9,7 @@ using BMM.Core.Implementations.Caching; using BMM.Core.Implementations.Connection; using BMM.Core.Implementations.DocumentFilters; +using BMM.Core.Implementations.Downloading; using BMM.Core.Implementations.Downloading.DownloadQueue; using BMM.Core.Implementations.Factories.Tracks; using BMM.Core.Implementations.FileStorage; @@ -35,8 +36,10 @@ public MyTracksViewModel( IDownloadQueue downloadQueue, IConnection connection, INetworkSettings networkSettings, - ITrackPOFactory trackPOFactory) - : base(storageManager, documentFilter, downloadQueue, connection, networkSettings) + ITrackPOFactory trackPOFactory, + IUnavailableTrackRegistry unavailableTracks, + ILogger logger) + : base(storageManager, documentFilter, downloadQueue, connection, networkSettings, unavailableTracks, logger) { _trackCollectionManager = trackCollectionManager; TrackPOFactory = trackPOFactory; diff --git a/BMM.Core/ViewModels/SharedTrackCollectionViewModel.cs b/BMM.Core/ViewModels/SharedTrackCollectionViewModel.cs index 37f946dd9..6ffcdee8c 100644 --- a/BMM.Core/ViewModels/SharedTrackCollectionViewModel.cs +++ b/BMM.Core/ViewModels/SharedTrackCollectionViewModel.cs @@ -9,6 +9,7 @@ using BMM.Core.GuardedActions.Tracklist.Interfaces; using BMM.Core.Implementations.Connection; using BMM.Core.Implementations.DocumentFilters; +using BMM.Core.Implementations.Downloading; using BMM.Core.Implementations.Downloading.DownloadQueue; using BMM.Core.Implementations.Factories.Tracks; using BMM.Core.Implementations.FileStorage; @@ -34,7 +35,9 @@ public SharedTrackCollectionViewModel( IDownloadQueue downloadQueue, INetworkSettings networkSettings, IAddToMyPlaylistAction addToMyPlaylistAction, - ITrackPOFactory trackPOFactory) + ITrackPOFactory trackPOFactory, + IUnavailableTrackRegistry unavailableTracks, + ILogger logger) : base( storageManager, documentFilter, @@ -42,7 +45,9 @@ public SharedTrackCollectionViewModel( connection, downloadQueue, networkSettings, - trackPOFactory) + trackPOFactory, + unavailableTracks, + logger) { _addToMyPlaylistAction = addToMyPlaylistAction; _addToMyPlaylistAction.AttachDataContext(this); diff --git a/BMM.Core/ViewModels/TopSongsCollectionViewModel.cs b/BMM.Core/ViewModels/TopSongsCollectionViewModel.cs index c3a7bf258..daa73deb4 100644 --- a/BMM.Core/ViewModels/TopSongsCollectionViewModel.cs +++ b/BMM.Core/ViewModels/TopSongsCollectionViewModel.cs @@ -8,6 +8,7 @@ using BMM.Core.GuardedActions.Tracks.Interfaces; using BMM.Core.Implementations.Connection; using BMM.Core.Implementations.DocumentFilters; +using BMM.Core.Implementations.Downloading; using BMM.Core.Implementations.Downloading.DownloadQueue; using BMM.Core.Implementations.Factories.Tracks; using BMM.Core.Implementations.FileStorage; @@ -36,7 +37,9 @@ public TopSongsCollectionViewModel( INetworkSettings networkSettings, ITrackPOFactory trackPOFactory, IPrepareTopSongsViewModelAction prepareTopSongsViewModelAction, - IAddTopSongsPlaylistToFavouritesAction addTopSongsPlaylistToFavouritesAction) + IAddTopSongsPlaylistToFavouritesAction addTopSongsPlaylistToFavouritesAction, + IUnavailableTrackRegistry unavailableTracks, + ILogger logger) : base( storageManager, documentFilter, @@ -44,7 +47,9 @@ public TopSongsCollectionViewModel( connection, downloadQueue, networkSettings, - trackPOFactory) + trackPOFactory, + unavailableTracks, + logger) { _prepareTopSongsViewModelAction = prepareTopSongsViewModelAction; _addTopSongsPlaylistToFavouritesAction = addTopSongsPlaylistToFavouritesAction; diff --git a/BMM.Core/ViewModels/TrackCollectionViewModel.cs b/BMM.Core/ViewModels/TrackCollectionViewModel.cs index 76aff1343..9faca7762 100644 --- a/BMM.Core/ViewModels/TrackCollectionViewModel.cs +++ b/BMM.Core/ViewModels/TrackCollectionViewModel.cs @@ -4,6 +4,7 @@ using BMM.Core.Helpers; using BMM.Core.Implementations.Connection; using BMM.Core.Implementations.DocumentFilters; +using BMM.Core.Implementations.Downloading; using BMM.Core.Implementations.Downloading.DownloadQueue; using BMM.Core.Implementations.Factories.Tracks; using BMM.Core.Implementations.FileStorage; @@ -46,7 +47,9 @@ public TrackCollectionViewModel( IConnection connection, IDownloadQueue downloadQueue, INetworkSettings networkSettings, - ITrackPOFactory trackPOFactory + ITrackPOFactory trackPOFactory, + IUnavailableTrackRegistry unavailableTracks, + ILogger logger ) : base( storageManager, @@ -55,7 +58,9 @@ ITrackPOFactory trackPOFactory downloadQueue, connection, networkSettings, - trackPOFactory) + trackPOFactory, + unavailableTracks, + logger) { DeleteCommand = new ExceptionHandlingCommand(() => DeleteTrackCollection(MyCollection)); diff --git a/BMM.Tests/BMM.Core.Test/Unit/Implementations/Downloading/DownloadQueueTests.cs b/BMM.Tests/BMM.Core.Test/Unit/Implementations/Downloading/DownloadQueueTests.cs index d17f9af0f..2e06b8399 100644 --- a/BMM.Tests/BMM.Core.Test/Unit/Implementations/Downloading/DownloadQueueTests.cs +++ b/BMM.Tests/BMM.Core.Test/Unit/Implementations/Downloading/DownloadQueueTests.cs @@ -1,4 +1,5 @@ using System; +using System.IO; using System.Linq; using System.Net; using System.Net.Http; @@ -7,9 +8,11 @@ using BMM.Api.Implementation.Models; using BMM.Core.Implementations.Analytics; using BMM.Core.Implementations.Connection; +using BMM.Core.Implementations.Downloading; using BMM.Core.Implementations.Downloading.DownloadQueue; using BMM.Core.Implementations.Downloading.FileDownloader; using BMM.Core.Implementations.Exceptions; +using BMM.Core.Implementations.FileStorage; using Moq; using MvvmCross.Plugin.Messenger; using NUnit.Framework; @@ -26,6 +29,9 @@ public class DownloadQueueTests private Mock _analytics; private Mock _connection; private Mock _networkSettings; + private Mock _storageManager; + private Mock _fileStorage; + private Mock _unavailableTracks; private Mock _logger; private Task _queueRun; @@ -38,6 +44,11 @@ public void Init() _analytics = new Mock(); _connection = new Mock(); _networkSettings = new Mock(); + _storageManager = new Mock(); + _fileStorage = new Mock(); + _storageManager.Setup(x => x.SelectedStorage).Returns(_fileStorage.Object); + _fileStorage.Setup(x => x.FreeSpace).Returns(10L * 1024 * 1024 * 1024); + _unavailableTracks = new Mock(); _logger = new Mock(); _queueRun = Task.CompletedTask; @@ -60,6 +71,8 @@ private TestableDownloadQueue CreateDownloadQueue() _analytics.Object, _connection.Object, _networkSettings.Object, + _storageManager.Object, + _unavailableTracks.Object, _logger.Object ); } @@ -238,6 +251,133 @@ public async Task Downloading_Stops_When_The_User_Leaves_A_Free_Network() Assert.AreEqual(1, downloadQueue.RemainingDownloadsCount); } + [Test] + public async Task Abandoned_Files_Are_Reported_Even_When_The_Queue_Empties() + { + // A run that skips permanently broken files still empties the queue, so without this the only + // trace would be the individual failures, and the collection would simply never be able to + // show as downloaded with nothing explaining why. + var missing = _fakeTrackFactory.CreateTrackWithId(1); + _fileDownloader + .Setup(x => x.DownloadFile(It.Is(d => d.Id == missing.Id))) + .ThrowsAsync(new DownloadHttpStatusException(HttpStatusCode.NotFound)); + + var downloadQueue = await RunQueueWith(missing, _fakeTrackFactory.CreateTrackWithId(2)); + + Assert.AreEqual(0, downloadQueue.RemainingDownloadsCount); + _logger.Verify(x => x.Error(nameof(DownloadQueue), It.Is(m => m.Contains("abandoned"))), Times.Once); + _analytics.Verify( + x => x.LogEvent("Download queue stopped early", It.IsAny>()), + Times.Once); + } + + [Test] + public async Task An_Unobtainable_Track_Is_Remembered_So_Its_Collection_Can_Still_Complete() + { + var missing = _fakeTrackFactory.CreateTrackWithId(1); + _fileDownloader + .Setup(x => x.DownloadFile(It.Is(d => d.Id == missing.Id))) + .ThrowsAsync(new DownloadHttpStatusException(HttpStatusCode.NotFound)); + + await RunQueueWith(missing, _fakeTrackFactory.CreateTrackWithId(2)); + + _unavailableTracks.Verify(x => x.MarkUnavailable(missing.Id), Times.Once); + _unavailableTracks.Verify(x => x.MarkAvailable(2), Times.Once); + } + + [Test] + public async Task A_Track_That_Downloads_Is_No_Longer_Considered_Unobtainable() + { + await RunQueueWith(_fakeTrackFactory.CreateTrackWithId(1)); + + _unavailableTracks.Verify(x => x.MarkAvailable(1), Times.Once); + _unavailableTracks.Verify(x => x.MarkUnavailable(It.IsAny()), Times.Never); + } + + [Test] + public async Task A_Connection_Failure_Does_Not_Mark_A_Track_Unobtainable() + { + // The file is probably fine, we just could not reach it. Marking it would let a collection + // report itself complete while tracks are still missing. + _fileDownloader + .Setup(x => x.DownloadFile(It.IsAny())) + .ThrowsAsync(new HttpRequestException("Connection failure")); + + await RunQueueWith(_fakeTrackFactory.CreateTrackWithId(1)); + + _unavailableTracks.Verify(x => x.MarkUnavailable(It.IsAny()), Times.Never); + } + + [Test] + public async Task A_Fully_Successful_Run_Is_Not_Reported_As_A_Problem() + { + await RunQueueWith(_fakeTrackFactory.CreateTrackWithId(1)); + + _logger.Verify(x => x.Error(It.IsAny(), It.IsAny()), Times.Never); + } + + [Test] + public async Task An_Unexpected_Error_Does_Not_Leave_The_Queue_Marked_As_Running() + { + _mvxMessenger + .Setup(x => x.Publish(It.IsAny())) + .Throws(new InvalidOperationException("a subscriber blew up")); + + var downloadQueue = await RunQueueWith(_fakeTrackFactory.CreateTrackWithId(1)); + + Assert.IsFalse(downloadQueue.IsRunning); + _logger.Verify( + x => x.Error(nameof(DownloadQueue), It.IsAny(), It.IsAny(), It.IsAny()), + Times.AtLeastOnce); + } + + [Test] + public async Task A_Full_Disk_Stops_The_Queue_Without_Retrying() + { + // A full disk surfaces as IOException, exactly like a stream that died mid-download. Retrying + // it would spin the queue against a disk that is not going to empty itself. + _fileStorage.Setup(x => x.FreeSpace).Returns(1024); + var tracks = Enumerable.Range(1, 3).Select(_fakeTrackFactory.CreateTrackWithId).ToArray(); + _fileDownloader + .Setup(x => x.DownloadFile(It.IsAny())) + .ThrowsAsync(new IOException("No space left on device")); + + var downloadQueue = await RunQueueWith(tracks); + + _fileDownloader.Verify(x => x.DownloadFile(It.IsAny()), Times.Once); + Assert.AreEqual(3, downloadQueue.RemainingDownloadsCount); + Assert.IsTrue(downloadQueue.StoppedWithPendingDownloads); + } + + [Test] + public async Task A_Full_Disk_Is_Reported_As_Such() + { + _fileStorage.Setup(x => x.FreeSpace).Returns(1024); + _fileDownloader + .Setup(x => x.DownloadFile(It.IsAny())) + .ThrowsAsync(new IOException("No space left on device")); + + await RunQueueWith(_fakeTrackFactory.CreateTrackWithId(1)); + + _mvxMessenger.Verify( + x => x.Publish(It.Is(m => m.Exception is StorageOutOfSpaceException)), + Times.Once); + } + + [Test] + public async Task A_Dropped_Stream_On_A_Healthy_Disk_Is_Still_A_Connection_Failure() + { + // Same exception type as a full disk, but there is plenty of room, so it must keep the + // retry behaviour rather than being written off as out of space. + _fileDownloader + .Setup(x => x.DownloadFile(It.IsAny())) + .ThrowsAsync(new IOException("Connection reset by peer")); + + await RunQueueWith(_fakeTrackFactory.CreateTrackWithId(1)); + + _fileDownloader.Verify(x => x.DownloadFile(It.IsAny()), Times.Exactly(3)); + } + [Test] public async Task A_Transient_Failure_Is_Retried() { @@ -268,8 +408,10 @@ public TestableDownloadQueue( IAnalytics analytics, IConnection connection, INetworkSettings networkSettings, + IStorageManager storageManager, + IUnavailableTrackRegistry unavailableTracks, ILogger logger) - : base(fileDownloader, messenger, exceptionHandler, analytics, connection, networkSettings, logger) + : base(fileDownloader, messenger, exceptionHandler, analytics, connection, networkSettings, storageManager, unavailableTracks, logger) { } diff --git a/BMM.Tests/BMM.Core.Test/Unit/ViewModels/AlbumViewModelTests.cs b/BMM.Tests/BMM.Core.Test/Unit/ViewModels/AlbumViewModelTests.cs index e9e3f90fa..76b655c11 100644 --- a/BMM.Tests/BMM.Core.Test/Unit/ViewModels/AlbumViewModelTests.cs +++ b/BMM.Tests/BMM.Core.Test/Unit/ViewModels/AlbumViewModelTests.cs @@ -6,6 +6,7 @@ using BMM.Core.Implementations.Albums.Interfaces; using BMM.Core.Implementations.Connection; using BMM.Core.Implementations.DocumentFilters; +using BMM.Core.Implementations.Downloading; using BMM.Core.Implementations.Downloading.DownloadQueue; using BMM.Core.Implementations.Factories; using BMM.Core.Implementations.FileStorage; @@ -37,7 +38,9 @@ public async Task LoadItems_ShouldHandleNullValueAndDisplayEmptyDocument() new Mock().Object, new Mock().Object, new Mock().Object, - new Mock().Object); + new Mock().Object, + new Mock().Object, + new Mock().Object); // Act await album.LoadItems(); From 7110222a8f12a6fb3de7ec73baae07036a63f7ae Mon Sep 17 00:00:00 2001 From: Sigve Hansen Date: Tue, 15 Sep 2026 12:28:43 +0200 Subject: [PATCH 4/4] feat(downloads): show download issue in ui --- .../Translation/en/Translations.designer.cs | 2 + BMM.Core/Translation/en/main.json | 4 +- BMM.Core/ViewModels/Base/DownloadViewModel.cs | 93 +++++++- BMM.Tests/BMM.Core.Test/BMM.Core.Test.csproj | 4 + .../ViewModels/Base/DownloadViewModelTests.cs | 225 ++++++++++++++++++ .../listitem_trackcollection_header.axml | 10 + .../layout/listitem_tracklist_header.xml | 4 +- .../ViewController/AlbumViewController.cs | 3 +- .../CuratedPlaylistViewController.cs | 3 +- .../TrackCollectionViewController.cs | 3 +- 10 files changed, 344 insertions(+), 7 deletions(-) create mode 100644 BMM.Tests/BMM.Core.Test/Unit/ViewModels/Base/DownloadViewModelTests.cs diff --git a/BMM.Core/Translation/en/Translations.designer.cs b/BMM.Core/Translation/en/Translations.designer.cs index 2c270f2d9..c8e5f989e 100644 --- a/BMM.Core/Translation/en/Translations.designer.cs +++ b/BMM.Core/Translation/en/Translations.designer.cs @@ -326,6 +326,8 @@ public static class Translations public const string TrackCollectionViewModel_EditPlaylist = nameof(TrackCollectionViewModel_EditPlaylist); public const string TrackCollectionViewModel_SharePlaylist = nameof(TrackCollectionViewModel_SharePlaylist); public const string TrackCollectionViewModel_RemovePlaylist = nameof(TrackCollectionViewModel_RemovePlaylist); + public const string TrackCollectionViewModel_DownloadPausedNoConnection = nameof(TrackCollectionViewModel_DownloadPausedNoConnection); + public const string TrackCollectionViewModel_SomeTracksUnavailable = nameof(TrackCollectionViewModel_SomeTracksUnavailable); public const string EditTrackCollectionViewModel_Title = nameof(EditTrackCollectionViewModel_Title); public const string EditTrackCollectionViewModel_RenameLabel = nameof(EditTrackCollectionViewModel_RenameLabel); public const string EditTrackCollectionViewModel_MenuSave = nameof(EditTrackCollectionViewModel_MenuSave); diff --git a/BMM.Core/Translation/en/main.json b/BMM.Core/Translation/en/main.json index 347f1e186..bb22858f6 100644 --- a/BMM.Core/Translation/en/main.json +++ b/BMM.Core/Translation/en/main.json @@ -398,7 +398,9 @@ "DeletePlaylist": "Delete playlist", "EditPlaylist": "Edit playlist", "SharePlaylist": "Share playlist", - "RemovePlaylist": "Remove playlist" + "RemovePlaylist": "Remove playlist", + "DownloadPausedNoConnection": "Paused – waiting for connection", + "SomeTracksUnavailable": "{0} track(s) unavailable" }, "EditTrackCollectionViewModel": { "Title": "Edit Playlist", diff --git a/BMM.Core/ViewModels/Base/DownloadViewModel.cs b/BMM.Core/ViewModels/Base/DownloadViewModel.cs index f8bb6cf73..699220fc5 100644 --- a/BMM.Core/ViewModels/Base/DownloadViewModel.cs +++ b/BMM.Core/ViewModels/Base/DownloadViewModel.cs @@ -12,6 +12,7 @@ using BMM.Core.Implementations.Downloading.DownloadQueue; using BMM.Core.Implementations.Downloading.FileDownloader; using BMM.Core.Implementations.DownloadManager; +using BMM.Core.Implementations.Exceptions; using BMM.Core.Implementations.FileStorage; using BMM.Core.Implementations.UI; using BMM.Core.Messages; @@ -157,9 +158,44 @@ protected IEnumerable DownloadingFiles public string DurationLabel { get => _durationLabel; - set => SetProperty(ref _durationLabel, value); + set + { + SetProperty(ref _durationLabel, value); + RaisePropertyChanged(() => HeaderSubtitle); + } + } + + /// + /// Why the download did not finish, or null when there is nothing to report. Only ever set for an + /// item the user actually asked for offline, since the queue's messages are global and would + /// otherwise put a stranger's failure on this screen. + /// + public string DownloadProblemText + { + get => _downloadProblemText; + private set + { + if (_downloadProblemText == value) + return; + + _downloadProblemText = value; + RaisePropertyChanged(() => DownloadProblemText); + RaisePropertyChanged(() => HasDownloadProblem); + RaisePropertyChanged(() => HeaderSubtitle); + } } + public bool HasDownloadProblem => !string.IsNullOrEmpty(DownloadProblemText); + + /// + /// The line under the title: the duration, or the reason a download did not finish when there is + /// one. It replaces the duration rather than joining it, because the iOS labels this binds to are + /// single line with tail truncation, so a combined string would simply be cut off. + /// + public string HeaderSubtitle => HasDownloadProblem + ? DownloadProblemText + : DurationLabel; + public bool IsCompletedPercentageVisible { get => _isCompletedPercentageVisible; @@ -195,6 +231,7 @@ public float DownloadStatus private string _durationLabel; private bool _isCompletedPercentageVisible; private bool _areAllTracksDownloaded = true; + private string _downloadProblemText; public virtual bool ShowSharingInfo => false; public virtual bool ShowImage => true; @@ -243,6 +280,9 @@ protected override void DetachEvents() protected override void HandleFileDownloadStartedMessage(FileDownloadStartedMessage message) { base.HandleFileDownloadStartedMessage(message); + + // A new attempt is under way, so whatever went wrong last time is no longer the current story. + DownloadProblemText = null; RaiseDownloadProgressChanged(); } @@ -255,6 +295,12 @@ protected override void HandleFileDownloadCompletedMessage(FileDownloadCompleted protected override void HandleFileDownloadCanceledMessage(FileDownloadCanceledMessage message) { base.HandleFileDownloadCanceledMessage(message); + + // Recorded as it happens, because the reason is only carried on this message. The queue + // reports the run's outcome afterwards, and that must not overwrite a concrete cause. + if (IsOfflineAvailable && message.Exception is StorageOutOfSpaceException) + DownloadProblemText = TextSource[Translations.TrackCollectionViewModel_NotEnoughtSpaceToDownload]; + RaiseDownloadProgressChanged(); } @@ -268,6 +314,49 @@ protected override void HandleDownloadQueueFinishedMessage(QueueFinishedMessage { base.HandleDownloadQueueFinishedMessage(message); RaiseDownloadProgressChanged(); + UpdateDownloadProblem(message.Succeeded); + } + + /// + /// Turns the outcome of a queue run into something the user can act on. Running out of space is + /// already recorded by the time we get here and keeps precedence, because "paused" would say + /// nothing about what to do next. + /// + private void UpdateDownloadProblem(bool queueSucceeded) + { + if (!IsOfflineAvailable) + { + DownloadProblemText = null; + return; + } + + if (HasDownloadProblem && DownloadProblemText == TextSource[Translations.TrackCollectionViewModel_NotEnoughtSpaceToDownload]) + return; + + if (!queueSucceeded && DownloadQueue.StoppedWithPendingDownloads) + { + DownloadProblemText = TextSource[Translations.TrackCollectionViewModel_DownloadPausedNoConnection]; + return; + } + + int unavailableCount = CountUnavailableTracks(); + + DownloadProblemText = unavailableCount > 0 + ? TextSource.GetText(Translations.TrackCollectionViewModel_SomeTracksUnavailable, unavailableCount.ToString()) + : null; + } + + private int CountUnavailableTracks() + { + try + { + return DownloadableTracks?.Count(track => _unavailableTracks.IsUnavailable(track.Id)) ?? 0; + } + catch (Exception exception) + { + _logger.Error(GetType().Name, "Could not count the unavailable tracks of this item", exception); + return 0; + } } public override async Task Load() @@ -353,6 +442,8 @@ protected async Task ToggleOffline() await DeleteAction(); + // Nothing is expected offline any more, so there is no problem left to report. + DownloadProblemText = null; RefreshAllTracks(); RefreshDownloadedFilesState(); await RaisePropertyChanged(() => IsDownloaded); diff --git a/BMM.Tests/BMM.Core.Test/BMM.Core.Test.csproj b/BMM.Tests/BMM.Core.Test/BMM.Core.Test.csproj index 1ec966872..72af3db7c 100644 --- a/BMM.Tests/BMM.Core.Test/BMM.Core.Test.csproj +++ b/BMM.Tests/BMM.Core.Test/BMM.Core.Test.csproj @@ -21,6 +21,10 @@ + + diff --git a/BMM.Tests/BMM.Core.Test/Unit/ViewModels/Base/DownloadViewModelTests.cs b/BMM.Tests/BMM.Core.Test/Unit/ViewModels/Base/DownloadViewModelTests.cs new file mode 100644 index 000000000..0ee8bcafa --- /dev/null +++ b/BMM.Tests/BMM.Core.Test/Unit/ViewModels/Base/DownloadViewModelTests.cs @@ -0,0 +1,225 @@ +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using BMM.Api.Abstraction; +using BMM.Api.Framework; +using BMM.Api.Implementation.Models; +using BMM.Core.Implementations.Connection; +using BMM.Core.Implementations.DocumentFilters; +using BMM.Core.Implementations.Downloading; +using BMM.Core.Implementations.Downloading.DownloadQueue; +using BMM.Core.Implementations.Downloading.FileDownloader; +using BMM.Core.Implementations.Exceptions; +using BMM.Core.Implementations.FileStorage; +using BMM.Core.Models.POs.Base.Interfaces; +using BMM.Core.Test.Unit.Implementations.Downloading; +using BMM.Core.Translation; +using BMM.Core.ViewModels.Base; +using Moq; +using NUnit.Framework; + +namespace BMM.Core.Test.Unit.ViewModels.Base +{ + /// + /// Covers what the header tells the user when a download does not finish. The states are only + /// distinguishable from the queue's messages, so they are driven through the same handlers the + /// messenger calls. + /// + [TestFixture] + public class DownloadViewModelTests : BaseViewModelTests + { + private readonly FakeTrackFactory _fakeTrackFactory = new FakeTrackFactory(); + private Mock _storageManager; + private Mock _fileStorage; + private Mock _downloadQueue; + private Mock _unavailableTracks; + + [SetUp] + public override void SetUp() + { + base.SetUp(); + _storageManager = new Mock(); + _fileStorage = new Mock(); + _storageManager.Setup(x => x.SelectedStorage).Returns(_fileStorage.Object); + _downloadQueue = new Mock(); + _unavailableTracks = new Mock(); + } + + private TestDownloadViewModel CreateViewModel(params Track[] tracks) + { + var viewModel = new TestDownloadViewModel( + _storageManager.Object, + new Mock().Object, + _downloadQueue.Object, + new Mock().Object, + new Mock().Object, + _unavailableTracks.Object, + new Mock().Object, + tracks); + + // Normally supplied by MvvmCross property injection. + viewModel.TextSource = TextResource.Object; + viewModel.SetOfflineAvailable(true); + return viewModel; + } + + [Test] + public void Nothing_Is_Reported_While_All_Is_Well() + { + var track = _fakeTrackFactory.CreateTrackWithId(1); + _fileStorage.Setup(x => x.IsDownloaded(It.IsAny())).Returns(true); + var viewModel = CreateViewModel(track); + + viewModel.SimulateQueueFinished(succeeded: true); + + Assert.IsFalse(viewModel.HasDownloadProblem); + Assert.IsNull(viewModel.DownloadProblemText); + } + + [Test] + public void Running_Out_Of_Space_Is_Reported() + { + var viewModel = CreateViewModel(_fakeTrackFactory.CreateTrackWithId(1)); + + viewModel.SimulateDownloadCanceled(new StorageOutOfSpaceException(_fileStorage.Object)); + + Assert.IsTrue(viewModel.HasDownloadProblem); + Assert.AreEqual(Translations.TrackCollectionViewModel_NotEnoughtSpaceToDownload, viewModel.DownloadProblemText); + } + + [Test] + public void Running_Out_Of_Space_Keeps_Precedence_Over_The_Queue_Outcome() + { + // Both arrive for the same run, and "paused" would tell the user nothing about what to do. + _downloadQueue.Setup(x => x.StoppedWithPendingDownloads).Returns(true); + var viewModel = CreateViewModel(_fakeTrackFactory.CreateTrackWithId(1)); + + viewModel.SimulateDownloadCanceled(new StorageOutOfSpaceException(_fileStorage.Object)); + viewModel.SimulateQueueFinished(succeeded: false); + + Assert.AreEqual(Translations.TrackCollectionViewModel_NotEnoughtSpaceToDownload, viewModel.DownloadProblemText); + } + + [Test] + public void A_Queue_That_Stopped_With_Work_Left_Is_Reported_As_Paused() + { + _downloadQueue.Setup(x => x.StoppedWithPendingDownloads).Returns(true); + var viewModel = CreateViewModel(_fakeTrackFactory.CreateTrackWithId(1)); + + viewModel.SimulateQueueFinished(succeeded: false); + + Assert.AreEqual(Translations.TrackCollectionViewModel_DownloadPausedNoConnection, viewModel.DownloadProblemText); + } + + [Test] + public void Unobtainable_Tracks_Are_Reported_After_A_Finished_Run() + { + var tracks = Enumerable.Range(1, 3).Select(_fakeTrackFactory.CreateTrackWithId).ToArray(); + _unavailableTracks.Setup(x => x.IsUnavailable(2)).Returns(true); + // GetText comes from IMvxLanguageBinder as GetText(string, params object[]). + TextResource + .Setup(x => x.GetText(Translations.TrackCollectionViewModel_SomeTracksUnavailable, It.IsAny())) + .Returns((key, args) => $"{key}:{args[0]}"); + var viewModel = CreateViewModel(tracks); + + viewModel.SimulateQueueFinished(succeeded: true); + + Assert.AreEqual($"{Translations.TrackCollectionViewModel_SomeTracksUnavailable}:1", viewModel.DownloadProblemText); + } + + [Test] + public void A_New_Attempt_Clears_The_Previous_Reason() + { + var viewModel = CreateViewModel(_fakeTrackFactory.CreateTrackWithId(1)); + viewModel.SimulateDownloadCanceled(new StorageOutOfSpaceException(_fileStorage.Object)); + + viewModel.SimulateDownloadStarted(); + + Assert.IsFalse(viewModel.HasDownloadProblem); + } + + [Test] + public void An_Item_The_User_Did_Not_Ask_For_Offline_Reports_Nothing() + { + // The queue's messages are global, so a failure elsewhere must not surface here. + _downloadQueue.Setup(x => x.StoppedWithPendingDownloads).Returns(true); + var viewModel = CreateViewModel(_fakeTrackFactory.CreateTrackWithId(1)); + viewModel.SetOfflineAvailable(false); + + viewModel.SimulateDownloadCanceled(new StorageOutOfSpaceException(_fileStorage.Object)); + viewModel.SimulateQueueFinished(succeeded: false); + + Assert.IsFalse(viewModel.HasDownloadProblem); + } + + [Test] + public void The_Subtitle_Shows_The_Reason_Instead_Of_The_Duration() + { + _downloadQueue.Setup(x => x.StoppedWithPendingDownloads).Returns(true); + var viewModel = CreateViewModel(_fakeTrackFactory.CreateTrackWithId(1)); + viewModel.SetDurationLabel("17 min 9 sek"); + + viewModel.SimulateQueueFinished(succeeded: false); + + // The iOS labels are single line with tail truncation, so the reason replaces the duration + // instead of being appended to it. + Assert.AreEqual(Translations.TrackCollectionViewModel_DownloadPausedNoConnection, viewModel.HeaderSubtitle); + } + + [Test] + public void The_Subtitle_Is_Just_The_Duration_When_There_Is_Nothing_To_Report() + { + var viewModel = CreateViewModel(_fakeTrackFactory.CreateTrackWithId(1)); + viewModel.SetDurationLabel("17 min 9 sek"); + + Assert.AreEqual("17 min 9 sek", viewModel.HeaderSubtitle); + } + + private class TestDownloadViewModel : DownloadViewModel + { + private readonly IList _tracks; + + public TestDownloadViewModel( + IStorageManager storageManager, + IDocumentFilter documentFilter, + IDownloadQueue downloadQueue, + IConnection connection, + INetworkSettings networkSettings, + IUnavailableTrackRegistry unavailableTracks, + ILogger logger, + IList tracks) + : base(storageManager, documentFilter, downloadQueue, connection, networkSettings, unavailableTracks, logger) + { + _tracks = tracks; + } + + public override string Title => "Test"; + + public override string Image => null; + + protected override IEnumerable DownloadableTracks => _tracks; + + public void SetOfflineAvailable(bool value) => IsOfflineAvailable = value; + + public void SetDurationLabel(string value) => DurationLabel = value; + + public void SimulateDownloadStarted() + => HandleFileDownloadStartedMessage(new FileDownloadStartedMessage(this, 1)); + + public void SimulateDownloadCanceled(System.Exception exception) + => HandleFileDownloadCanceledMessage(new FileDownloadCanceledMessage(this, exception)); + + public void SimulateQueueFinished(bool succeeded) + => HandleDownloadQueueFinishedMessage(new QueueFinishedMessage(this, succeeded)); + + protected override Task DownloadAction() => Task.CompletedTask; + + protected override Task DeleteAction() => Task.CompletedTask; + + protected override Task CalculateApproximateDownloadSize() => Task.FromResult(0L); + + public override Task> LoadItems(CachePolicy policy = CachePolicy.UseCacheAndRefreshOutdated) + => Task.FromResult(Enumerable.Empty()); + } + } +} diff --git a/BMM.UI.Android/Resources/layout/listitem_trackcollection_header.axml b/BMM.UI.Android/Resources/layout/listitem_trackcollection_header.axml index fdfa1271d..9ca558a17 100644 --- a/BMM.UI.Android/Resources/layout/listitem_trackcollection_header.axml +++ b/BMM.UI.Android/Resources/layout/listitem_trackcollection_header.axml @@ -92,6 +92,16 @@ android:text="23 tracks" app:MvxBind="Text TrackCountString; Visibility InvertedVisibility(And(IsLoading, EqualTo(Documents.Count, 0)))"/> + + v.BindTitle()).To(vm => vm.PlayButtonText); set.Bind(PlayButton).For(v => v.BindVisible()).To(vm => vm.ShowPlayButton); set.Bind(TrackCountLabel).To(vm => vm.TrackCountString); - set.Bind(DurationLabel).To(vm => vm.DurationLabel); + // Carries the duration plus, when there is one, the reason a download did not finish. + set.Bind(DurationLabel).To(vm => vm.HeaderSubtitle); set.Bind(DurationProgressBar) .For(v => v.Percentage) .To(vm => vm.CompletedPercentage); diff --git a/BMM.UI.iOS/Application/ViewController/CuratedPlaylistViewController.cs b/BMM.UI.iOS/Application/ViewController/CuratedPlaylistViewController.cs index 40f4d85a7..55c36b7f4 100644 --- a/BMM.UI.iOS/Application/ViewController/CuratedPlaylistViewController.cs +++ b/BMM.UI.iOS/Application/ViewController/CuratedPlaylistViewController.cs @@ -63,7 +63,8 @@ public override void ViewDidLoad() set.Bind(DownloadButton).For(v => v.IsDownloaded).To(vm => vm.IsDownloaded); set.Bind(DownloadButton).For(v => v.DownloadProgress).To(vm => vm.DownloadStatus); - set.Bind(DurationLabel).To(vm => vm.DurationLabel); + // Carries the duration plus, when there is one, the reason a download did not finish. + set.Bind(DurationLabel).To(vm => vm.HeaderSubtitle); set.Bind(refreshControl).For(r => r.IsRefreshing).To(vm => vm.IsRefreshing); set.Bind(refreshControl).For(r => r.RefreshCommand).To(vm => vm.ReloadCommand); diff --git a/BMM.UI.iOS/Application/ViewController/TrackCollectionViewController.cs b/BMM.UI.iOS/Application/ViewController/TrackCollectionViewController.cs index 99158fa0d..15b005f53 100644 --- a/BMM.UI.iOS/Application/ViewController/TrackCollectionViewController.cs +++ b/BMM.UI.iOS/Application/ViewController/TrackCollectionViewController.cs @@ -96,7 +96,8 @@ public override void ViewDidLoad() set.Bind(refreshControl).For(r => r.IsRefreshing).To(vm => vm.IsRefreshing); set.Bind(refreshControl).For(r => r.RefreshCommand).To(vm => vm.ReloadCommand); - set.Bind(DurationLabel).To(vm => vm.DurationLabel); + // Carries the duration plus, when there is one, the reason a download did not finish. + set.Bind(DurationLabel).To(vm => vm.HeaderSubtitle); CollectionTable.ReloadData();