diff --git a/src/NzbDrone.Core.Test/Download/DownloadClientProviderFixture.cs b/src/NzbDrone.Core.Test/Download/DownloadClientProviderFixture.cs index 6733bcde8..5e42fe0d1 100644 --- a/src/NzbDrone.Core.Test/Download/DownloadClientProviderFixture.cs +++ b/src/NzbDrone.Core.Test/Download/DownloadClientProviderFixture.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using FizzWare.NBuilder; using FluentAssertions; using Moq; @@ -325,5 +326,207 @@ public void should_fail_to_choose_client_when_indexer_reference_does_not_exist() Assert.Throws(() => Subject.GetDownloadClient(DownloadProtocol.Torrent, 1)); } + + [Test] + public void should_return_all_available_clients_for_protocol() + { + WithUsenetClient(); + WithTorrentClient(); + WithTorrentClient(); + WithTorrentClient(); + + var clients = Subject.GetDownloadClients(DownloadProtocol.Torrent); + + clients.Should().HaveCount(3); + clients.Select(c => c.Definition.Id).Should().BeEquivalentTo(new[] { 2, 3, 4 }); + } + + [Test] + public void should_return_empty_when_no_clients_available_for_protocol() + { + WithUsenetClient(); + WithUsenetClient(); + + var clients = Subject.GetDownloadClients(DownloadProtocol.Torrent); + + clients.Should().BeEmpty(); + } + + [Test] + public void should_return_clients_ordered_by_priority_then_by_last_used() + { + WithTorrentClient(priority: 1); + WithTorrentClient(priority: 0); + WithTorrentClient(priority: 0); + WithTorrentClient(priority: 2); + + var clients = Subject.GetDownloadClients(DownloadProtocol.Torrent); + + clients.Should().HaveCount(4); + var clientIds = clients.Select(c => c.Definition.Id).ToArray(); + clientIds[0].Should().Be(2); + clientIds[1].Should().Be(3); + clientIds[2].Should().Be(1); + clientIds[3].Should().Be(4); + } + + [Test] + public void should_rotate_clients_within_same_priority_based_on_last_used() + { + WithTorrentClient(priority: 0); + WithTorrentClient(priority: 0); + WithTorrentClient(priority: 0); + + var clients1 = Subject.GetDownloadClients(DownloadProtocol.Torrent); + clients1.First().Definition.Id.Should().Be(1); + + Subject.ReportSuccessfulDownloadClient(DownloadProtocol.Torrent, 2); + + var clients2 = Subject.GetDownloadClients(DownloadProtocol.Torrent); + var clientIds = clients2.Select(c => c.Definition.Id).ToArray(); + clientIds[0].Should().Be(3); + clientIds[1].Should().Be(1); + clientIds[2].Should().Be(2); + } + + [Test] + public void should_filter_clients_by_tags() + { + var seriesTags = new HashSet { 1, 2 }; + + WithTorrentClient(tags: new HashSet { 1 }); + WithTorrentClient(tags: new HashSet { 3 }); + WithTorrentClient(tags: new HashSet { 2 }); + WithTorrentClient(); + + var clients = Subject.GetDownloadClients(DownloadProtocol.Torrent, tags: seriesTags); + + clients.Should().HaveCount(2); + clients.Select(c => c.Definition.Id).Should().BeEquivalentTo(new[] { 1, 3 }); + } + + [Test] + public void should_return_non_tagged_clients_when_no_matching_tags() + { + var seriesTags = new HashSet { 5 }; + + WithTorrentClient(tags: new HashSet { 1 }); + WithTorrentClient(tags: new HashSet { 2 }); + WithTorrentClient(); + WithTorrentClient(); + + var clients = Subject.GetDownloadClients(DownloadProtocol.Torrent, tags: seriesTags); + + clients.Should().HaveCount(2); + clients.Select(c => c.Definition.Id).Should().BeEquivalentTo(new[] { 3, 4 }); + } + + [Test] + public void should_throw_when_all_clients_have_non_matching_tags() + { + var seriesTags = new HashSet { 5 }; + + WithTorrentClient(tags: new HashSet { 1 }); + WithTorrentClient(tags: new HashSet { 2 }); + + Assert.Throws(() => Subject.GetDownloadClients(DownloadProtocol.Torrent, tags: seriesTags)); + } + + [Test] + public void should_return_indexer_specific_client_when_specified() + { + WithTorrentClient(); + WithTorrentClient(); + WithTorrentClient(); + WithTorrentIndexer(2); + + var clients = Subject.GetDownloadClients(DownloadProtocol.Torrent, indexerId: 1); + + clients.Should().HaveCount(1); + clients.First().Definition.Id.Should().Be(2); + } + + [Test] + public void should_throw_when_indexer_client_does_not_exist() + { + WithTorrentClient(); + WithTorrentClient(); + WithTorrentIndexer(5); + + Assert.Throws(() => Subject.GetDownloadClients(DownloadProtocol.Torrent, indexerId: 1)); + } + + [Test] + public void should_filter_blocked_clients_when_requested() + { + WithTorrentClient(); + WithTorrentClient(); + WithTorrentClient(); + + GivenBlockedClient(2); + + var clients = Subject.GetDownloadClients(DownloadProtocol.Torrent, filterBlockedClients: true); + + clients.Should().HaveCount(2); + clients.Select(c => c.Definition.Id).Should().BeEquivalentTo(new[] { 1, 3 }); + } + + [Test] + public void should_throw_when_all_clients_blocked_and_filter_enabled() + { + WithTorrentClient(); + WithTorrentClient(); + + GivenBlockedClient(1); + GivenBlockedClient(2); + + Assert.Throws(() => Subject.GetDownloadClients(DownloadProtocol.Torrent, filterBlockedClients: true)); + } + + [Test] + public void should_return_blocked_clients_when_filter_disabled() + { + WithTorrentClient(); + WithTorrentClient(); + + GivenBlockedClient(1); + GivenBlockedClient(2); + + var clients = Subject.GetDownloadClients(DownloadProtocol.Torrent, filterBlockedClients: false); + + clients.Should().HaveCount(2); + clients.Select(c => c.Definition.Id).Should().BeEquivalentTo(new[] { 1, 2 }); + } + + [Test] + public void should_throw_when_indexer_client_is_blocked_and_filter_enabled() + { + WithTorrentClient(); + WithTorrentClient(); + WithTorrentIndexer(2); + + GivenBlockedClient(2); + + Assert.Throws(() => Subject.GetDownloadClients(DownloadProtocol.Torrent, indexerId: 1, filterBlockedClients: true)); + } + + [Test] + public void should_combine_tags_and_priority_filtering() + { + var seriesTags = new HashSet { 1 }; + + WithTorrentClient(priority: 1, tags: new HashSet { 1 }); + WithTorrentClient(priority: 0, tags: new HashSet { 1 }); + WithTorrentClient(priority: 0, tags: new HashSet { 2 }); + WithTorrentClient(priority: 2); + + var clients = Subject.GetDownloadClients(DownloadProtocol.Torrent, tags: seriesTags); + + clients.Should().HaveCount(2); + var clientIds = clients.Select(c => c.Definition.Id).ToArray(); + + clientIds[0].Should().Be(2); + clientIds[1].Should().Be(1); + } } } diff --git a/src/NzbDrone.Core.Test/Download/DownloadServiceFixture.cs b/src/NzbDrone.Core.Test/Download/DownloadServiceFixture.cs index 0d42c6da0..264f4a431 100644 --- a/src/NzbDrone.Core.Test/Download/DownloadServiceFixture.cs +++ b/src/NzbDrone.Core.Test/Download/DownloadServiceFixture.cs @@ -29,8 +29,8 @@ public void Setup() _downloadClients = new List(); Mocker.GetMock() - .Setup(v => v.GetDownloadClients(It.IsAny())) - .Returns(_downloadClients); + .Setup(v => v.GetDownloadClients(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny>())) + .Returns>((v, i, f, t) => _downloadClients.Where(d => d.Protocol == v)); Mocker.GetMock() .Setup(v => v.GetDownloadClient(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny>())) @@ -107,7 +107,7 @@ public void Download_report_should_not_publish_on_failed_grab_event() mock.Setup(s => s.Download(It.IsAny(), It.IsAny())) .Throws(new WebException()); - Assert.ThrowsAsync(async () => await Subject.DownloadReport(_parseResult, null)); + Assert.ThrowsAsync(async () => await Subject.DownloadReport(_parseResult, null)); VerifyEventNotPublished(); } @@ -177,7 +177,7 @@ public void Download_report_should_not_trigger_indexer_backoff_on_downloadclient mock.Setup(s => s.Download(It.IsAny(), It.IsAny())) .Throws(new DownloadClientException("Some Error")); - Assert.ThrowsAsync(async () => await Subject.DownloadReport(_parseResult, null)); + Assert.ThrowsAsync(async () => await Subject.DownloadReport(_parseResult, null)); Mocker.GetMock() .Verify(v => v.RecordFailure(It.IsAny(), It.IsAny()), Times.Never()); diff --git a/src/NzbDrone.Core/Download/DownloadClientProvider.cs b/src/NzbDrone.Core/Download/DownloadClientProvider.cs index 5a82abb11..6c74dc9d5 100644 --- a/src/NzbDrone.Core/Download/DownloadClientProvider.cs +++ b/src/NzbDrone.Core/Download/DownloadClientProvider.cs @@ -12,7 +12,9 @@ public interface IProvideDownloadClient { IDownloadClient GetDownloadClient(DownloadProtocol downloadProtocol, int indexerId = 0, bool filterBlockedClients = false, HashSet tags = null); IEnumerable GetDownloadClients(bool filterBlockedClients = false); + IEnumerable GetDownloadClients(DownloadProtocol downloadProtocol, int indexerId = 0, bool filterBlockedClients = false, HashSet tags = null); IDownloadClient Get(int id); + void ReportSuccessfulDownloadClient(DownloadProtocol downloadProtocol, int downloadClientId); } public class DownloadClientProvider : IProvideDownloadClient @@ -38,69 +40,13 @@ public DownloadClientProvider(IDownloadClientStatusService downloadClientStatusS public IDownloadClient GetDownloadClient(DownloadProtocol downloadProtocol, int indexerId = 0, bool filterBlockedClients = false, HashSet tags = null) { - // Tags aren't required, but download clients with tags should not be picked unless there is at least one matching tag. - // Defaulting to an empty HashSet ensures this is always checked. - tags ??= new HashSet(); - - var blockedProviders = new HashSet(_downloadClientStatusService.GetBlockedProviders().Select(v => v.ProviderId)); - var availableProviders = _downloadClientFactory.GetAvailableProviders().Where(v => v.Protocol == downloadProtocol).ToList(); + var availableProviders = GetFilteredDownloadClients(downloadProtocol, indexerId, filterBlockedClients, tags, forSingleClient: true).ToList(); if (!availableProviders.Any()) { return null; } - var matchingTagsClients = availableProviders.Where(i => i.Definition.Tags.Intersect(tags).Any()).ToList(); - - availableProviders = matchingTagsClients.Count > 0 ? - matchingTagsClients : - availableProviders.Where(i => i.Definition.Tags.Empty()).ToList(); - - if (!availableProviders.Any()) - { - throw new DownloadClientUnavailableException("No download client was found without tags or a matching series tag. Please check your settings."); - } - - if (indexerId > 0) - { - var indexer = _indexerFactory.Find(indexerId); - - if (indexer is { DownloadClientId: > 0 }) - { - var client = availableProviders.SingleOrDefault(d => d.Definition.Id == indexer.DownloadClientId); - - if (client == null) - { - throw new DownloadClientUnavailableException($"Indexer specified download client does not exist for {indexer.Name}"); - } - - if (filterBlockedClients && blockedProviders.Contains(client.Definition.Id)) - { - throw new DownloadClientUnavailableException($"Indexer specified download client is not available due to recent failures for {indexer.Name}"); - } - - return client; - } - } - - if (blockedProviders.Any()) - { - var nonBlockedProviders = availableProviders.Where(v => !blockedProviders.Contains(v.Definition.Id)).ToList(); - - if (nonBlockedProviders.Any()) - { - availableProviders = nonBlockedProviders; - } - else if (filterBlockedClients) - { - throw new DownloadClientUnavailableException($"All download clients for {downloadProtocol} are not available"); - } - else - { - _logger.Trace("No non-blocked Download Client available, retrying blocked one."); - } - } - // Use the first priority clients first availableProviders = availableProviders.GroupBy(v => (v.Definition as DownloadClientDefinition).Priority) .OrderBy(v => v.Key) @@ -127,11 +73,54 @@ public IEnumerable GetDownloadClients(bool filterBlockedClients return enabledClients; } + public IEnumerable GetDownloadClients(DownloadProtocol downloadProtocol, int indexerId = 0, bool filterBlockedClients = false, HashSet tags = null) + { + var filteredClients = GetFilteredDownloadClients(downloadProtocol, indexerId, filterBlockedClients, tags, forSingleClient: false).ToList(); + + if (filteredClients.Empty()) + { + return Enumerable.Empty(); + } + + var lastUsedId = _lastUsedDownloadClient.Find(downloadProtocol.ToString()); + + var clientsByPriority = filteredClients + .GroupBy(v => (v.Definition as DownloadClientDefinition).Priority) + .OrderBy(g => g.Key) + .ToList(); + + var orderedClients = new List(); + + foreach (var priorityGroup in clientsByPriority) + { + var clientsInGroup = priorityGroup.OrderBy(v => v.Definition.Id).ToList(); + var lastUsedIndex = clientsInGroup.FindIndex(v => v.Definition.Id == lastUsedId); + + if (lastUsedIndex >= 0) + { + orderedClients.AddRange(clientsInGroup.Skip(lastUsedIndex + 1)); + orderedClients.AddRange(clientsInGroup.Take(lastUsedIndex)); + orderedClients.Add(clientsInGroup[lastUsedIndex]); + } + else + { + orderedClients.AddRange(clientsInGroup); + } + } + + return orderedClients; + } + public IDownloadClient Get(int id) { return _downloadClientFactory.GetAvailableProviders().Single(d => d.Definition.Id == id); } + public void ReportSuccessfulDownloadClient(DownloadProtocol downloadProtocol, int downloadClientId) + { + _lastUsedDownloadClient.Set(downloadProtocol.ToString(), downloadClientId); + } + private IEnumerable FilterBlockedDownloadClients(IEnumerable clients) { var blockedClients = _downloadClientStatusService.GetBlockedProviders().ToDictionary(v => v.ProviderId, v => v); @@ -147,5 +136,73 @@ private IEnumerable FilterBlockedDownloadClients(IEnumerable GetFilteredDownloadClients(DownloadProtocol downloadProtocol, int indexerId, bool filterBlockedClients, HashSet tags, bool forSingleClient) + { + // Tags aren't required, but download clients with tags should not be picked unless there is at least one matching tag. + // Defaulting to an empty HashSet ensures this is always checked. + tags ??= new HashSet(); + + var blockedProviders = new HashSet(_downloadClientStatusService.GetBlockedProviders().Select(v => v.ProviderId)); + var availableProviders = _downloadClientFactory.GetAvailableProviders().Where(v => v.Protocol == downloadProtocol).ToList(); + + if (availableProviders.Empty()) + { + return Enumerable.Empty(); + } + + var matchingTagsClients = availableProviders.Where(i => i.Definition.Tags.Intersect(tags).Any()).ToList(); + + availableProviders = matchingTagsClients.Count > 0 ? + matchingTagsClients : + availableProviders.Where(i => i.Definition.Tags.Empty()).ToList(); + + if (availableProviders.Empty()) + { + throw new DownloadClientUnavailableException("No download client was found without tags or a matching series tag. Please check your settings."); + } + + if (indexerId > 0) + { + var indexer = _indexerFactory.Find(indexerId); + + if (indexer is { DownloadClientId: > 0 }) + { + var client = availableProviders.SingleOrDefault(d => d.Definition.Id == indexer.DownloadClientId); + + if (client == null) + { + throw new DownloadClientUnavailableException($"Indexer specified download client does not exist for {indexer.Name}"); + } + + if (filterBlockedClients && blockedProviders.Contains(client.Definition.Id)) + { + throw new DownloadClientUnavailableException($"Indexer specified download client is not available due to recent failures for {indexer.Name}"); + } + + return [client]; + } + } + + if (blockedProviders.Any()) + { + var nonBlockedProviders = availableProviders.Where(v => !blockedProviders.Contains(v.Definition.Id)).ToList(); + + if (nonBlockedProviders.Any()) + { + availableProviders = nonBlockedProviders; + } + else if (filterBlockedClients) + { + throw new DownloadClientUnavailableException($"All download clients for {downloadProtocol} are not available"); + } + else + { + _logger.Trace("No non-blocked Download Client available{0}.", forSingleClient ? ", retrying blocked one" : $" for {downloadProtocol}, returning all clients"); + } + } + + return availableProviders; + } } } diff --git a/src/NzbDrone.Core/Download/DownloadService.cs b/src/NzbDrone.Core/Download/DownloadService.cs index 9f07145ca..17bed18d1 100644 --- a/src/NzbDrone.Core/Download/DownloadService.cs +++ b/src/NzbDrone.Core/Download/DownloadService.cs @@ -1,4 +1,6 @@ using System; +using System.Collections.Generic; +using System.Linq; using System.Threading.Tasks; using NLog; using NzbDrone.Common.EnsureThat; @@ -56,11 +58,64 @@ public async Task DownloadReport(RemoteEpisode remoteEpisode, int? downloadClien var tags = remoteEpisode.Series?.Tags; - var downloadClient = downloadClientId.HasValue - ? _downloadClientProvider.Get(downloadClientId.Value) - : _downloadClientProvider.GetDownloadClient(remoteEpisode.Release.DownloadProtocol, remoteEpisode.Release.IndexerId, filterBlockedClients, tags); + if (downloadClientId.HasValue) + { + var specificClient = _downloadClientProvider.Get(downloadClientId.Value); + await DownloadReport(remoteEpisode, specificClient); - await DownloadReport(remoteEpisode, downloadClient); + return; + } + + var availableClients = _downloadClientProvider.GetDownloadClients( + remoteEpisode.Release.DownloadProtocol, + remoteEpisode.Release.IndexerId, + filterBlockedClients, + tags).ToList(); + + if (!availableClients.Any()) + { + throw new DownloadClientUnavailableException($"No {remoteEpisode.Release.DownloadProtocol} download client available"); + } + + var triedClients = new HashSet(); + + foreach (var downloadClient in availableClients) + { + if (triedClients.Contains(downloadClient.Definition.Id)) + { + continue; + } + + try + { + _logger.Debug("Attempting download with client: {0}", downloadClient.Definition.Name); + await DownloadReport(remoteEpisode, downloadClient); + + _downloadClientProvider.ReportSuccessfulDownloadClient( + remoteEpisode.Release.DownloadProtocol, + downloadClient.Definition.Id); + + return; + } + catch (DownloadClientException ex) + { + _logger.Trace(ex, "Unable to add report to download client: {0}", downloadClient.Definition.Name); + triedClients.Add(downloadClient.Definition.Id); + } + catch (Exception ex) + { + // Rethrow specific exceptions that should not trigger a fallback + if (ex is ReleaseDownloadException) + { + throw; + } + + _logger.Trace(ex, "Unable to add report to download client: {0}", downloadClient.Definition.Name); + triedClients.Add(downloadClient.Definition.Id); + } + } + + throw new DownloadClientUnavailableException("All '{0}' download clients failed", remoteEpisode.Release.DownloadProtocol); } private async Task DownloadReport(RemoteEpisode remoteEpisode, IDownloadClient downloadClient)