From 4a3cf4298dc3e7c64a5913eb2d55787849320eb9 Mon Sep 17 00:00:00 2001 From: m4bard <304653687+m4bard@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:18:46 -0500 Subject: [PATCH 1/2] fix(scoring): resolve indexers once per batch, not once per result in parallel ScoreSearchResults fans out with Task.WhenAll, and each task built a SearchResultScorer over the same scoped IIndexerRepository and queried it for the result's indexer. IIndexerRepository is scoped and the ListenArrDbContext behind it is scoped, so a batch of N results issued N concurrent queries against one context. EF rejects a second operation started on a context while another is in flight. The symptom is not a failed request. The scorer catches the exception and logs at Debug, leaving indexerRetention at 0 and skipping the Usenet detection that sets isNzb. So age and retention checks silently score against the wrong assumptions for whichever results lost the race, and the batch comes back plausibly ordered and quietly wrong. Resolve each distinct indexer once, sequentially, before fanning out, and pass the results into the scorer. Three callers reach this: the quality profile controller, download submission, and the six-hour automatic search sweep. This is also fewer queries rather than merely safer ones. A batch commonly carries many results across a handful of indexers, so it goes from one query per result to one per distinct indexer. IndexerSearchWorkflow already fetches its indexers before its own fan-out, so this makes the two agree. The single-result ScoreSearchResult keeps its old behaviour and still queries, since there is nothing to batch there. Asserting on the EF exception would mean racing it, so the test counts the overlap directly: a stub repository records the highest number of calls in flight at once. It reports 12 without the change and 1 with it, and also pins the query count at one per distinct indexer rather than one per result. --- .../Quality/QualityProfileService.cs | 57 ++++++++- .../Search/Scoring/SearchResultScorer.cs | 23 +++- .../QualityProfileScoringConcurrencyTests.cs | 111 ++++++++++++++++++ 3 files changed, 186 insertions(+), 5 deletions(-) create mode 100644 tests/Features/Application/Audiobooks/Quality/QualityProfileScoringConcurrencyTests.cs diff --git a/listenarr.application/Audiobooks/Quality/QualityProfileService.cs b/listenarr.application/Audiobooks/Quality/QualityProfileService.cs index 3097a5060..eaab906c0 100644 --- a/listenarr.application/Audiobooks/Quality/QualityProfileService.cs +++ b/listenarr.application/Audiobooks/Quality/QualityProfileService.cs @@ -190,9 +190,15 @@ private async Task UnsetAllDefaultsAsync() } } - public async Task ScoreSearchResult(SearchResult searchResult, QualityProfile profile) + public Task ScoreSearchResult(SearchResult searchResult, QualityProfile profile) => + ScoreSearchResult(searchResult, profile, resolvedIndexers: null); + + private async Task ScoreSearchResult( + SearchResult searchResult, + QualityProfile profile, + IReadOnlyDictionary? resolvedIndexers) { - var scorer = new SearchResultScorer(_indexerRepository, _logger); + var scorer = new SearchResultScorer(_indexerRepository, _logger, resolvedIndexers); var score = await scorer.Score(searchResult, profile); // Also calculate the Prowlarr-style composite (Smart) score so the UI @@ -287,7 +293,15 @@ private int GetQualityScore(string? quality) public async Task> ScoreSearchResults(List searchResults, QualityProfile profile) { - var scores = await Task.WhenAll(searchResults.Select(result => ScoreSearchResult(result, profile))); + // Resolve every indexer this batch refers to before fanning out, not inside it. + // Scoring runs in parallel and the scorer reads indexer retention per result, so a + // per-result lookup meant N concurrent queries against one scoped DbContext. EF + // rejects the overlap, the scorer catches it and logs at Debug, and the result keeps + // a retention of 0 with Usenet detection skipped. The scores come out quietly wrong + // rather than the request failing. + var resolvedIndexers = await ResolveIndexersAsync(searchResults); + var scores = await Task.WhenAll( + searchResults.Select(result => ScoreSearchResult(result, profile, resolvedIndexers))); // Ensure rejected results are ordered last regardless of numeric TotalScore return scores @@ -296,6 +310,43 @@ public async Task> ScoreSearchResults(List sear .ToList(); } + private async Task> ResolveIndexersAsync( + List searchResults) + { + var resolved = new Dictionary(); + if (_indexerRepository == null) + { + return resolved; + } + + // Sequential and de-duplicated: a batch usually refers to a handful of indexers even + // when it carries hundreds of results, so this is fewer queries than before as well as + // non-overlapping ones. + foreach (var indexerId in searchResults + .Where(result => result.IndexerId.HasValue) + .Select(result => result.IndexerId!.Value) + .Distinct()) + { + try + { + var indexer = await _indexerRepository.GetByIdAsync(indexerId); + if (indexer != null) + { + resolved[indexerId] = indexer; + } + } + catch (Exception ex) when (ex is not OperationCanceledException && ex is not OutOfMemoryException && ex is not StackOverflowException) + { + _logger.LogWarning( + ex, + "Failed to resolve indexer {IndexerId} while scoring a search batch; retention and Usenet detection will be skipped for its results", + indexerId); + } + } + + return resolved; + } + /// /// Checks if a quality string contains VBR preset indicators (v0, v1, v2). /// diff --git a/listenarr.application/Search/Scoring/SearchResultScorer.cs b/listenarr.application/Search/Scoring/SearchResultScorer.cs index bfb446143..7dc4a95bd 100644 --- a/listenarr.application/Search/Scoring/SearchResultScorer.cs +++ b/listenarr.application/Search/Scoring/SearchResultScorer.cs @@ -34,10 +34,24 @@ public class SearchResultScorer public int QualityNotAllowedPenalty { get; set; } = -20; public int ForbiddenWordRejectionFlag { get; set; } = -1; // sentinel for rejection + private readonly IReadOnlyDictionary? _resolvedIndexers; + public SearchResultScorer(IIndexerRepository? indexerRepository, ILogger logger) + : this(indexerRepository, logger, resolvedIndexers: null) + { + } + + // resolvedIndexers lets a caller scoring a whole batch resolve each indexer once up front + // and pass the results in. The repository is scoped, and so is the DbContext behind it, so + // results scored in parallel must not each run their own lookup. + public SearchResultScorer( + IIndexerRepository? indexerRepository, + ILogger logger, + IReadOnlyDictionary? resolvedIndexers) { _indexerRepository = indexerRepository; _logger = logger; + _resolvedIndexers = resolvedIndexers; } public async Task Score(SearchResult searchResult, QualityProfile profile) @@ -118,11 +132,16 @@ public async Task Score(SearchResult searchResult, QualityProfile // Age checks and indexer retention double ageDays = 0; int indexerRetention = 0; - if (searchResult.IndexerId.HasValue && _indexerRepository != null) + if (searchResult.IndexerId.HasValue + && (_resolvedIndexers != null || _indexerRepository != null)) { try { - var idx = await _indexerRepository.GetByIdAsync(searchResult.IndexerId.Value); + var idx = _resolvedIndexers != null + ? (_resolvedIndexers.TryGetValue(searchResult.IndexerId.Value, out var preresolved) + ? preresolved + : null) + : await _indexerRepository!.GetByIdAsync(searchResult.IndexerId.Value); if (idx != null) { indexerRetention = idx.Retention; diff --git a/tests/Features/Application/Audiobooks/Quality/QualityProfileScoringConcurrencyTests.cs b/tests/Features/Application/Audiobooks/Quality/QualityProfileScoringConcurrencyTests.cs new file mode 100644 index 000000000..087e54e46 --- /dev/null +++ b/tests/Features/Application/Audiobooks/Quality/QualityProfileScoringConcurrencyTests.cs @@ -0,0 +1,111 @@ +/* + * Listenarr - Audiobook Management System + * Copyright (C) 2024-2026 Listenarr Contributors + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published + * by the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +using Listenarr.Tests.Common; +using Microsoft.Extensions.Logging.Abstractions; + +namespace Listenarr.Tests.Features.Application.Audiobooks.Quality +{ + [Trait("Name", "QualityProfileScoringConcurrencyTests")] + [Trait("Category", "Unit")] + public class QualityProfileScoringConcurrencyTests : BaseTests + { + // IIndexerRepository is registered scoped and the ListenArrDbContext behind it is scoped + // too, so every repository in a scope shares one context. EF rejects a second operation + // started on a context while another is in flight. Asserting on the real exception would + // mean racing it, so this counts overlap directly: the stub records the highest number of + // calls it ever had in flight at once. Anything above one is the condition EF refuses. + private sealed class OverlapRecordingIndexerRepository : IIndexerRepository + { + private int _inFlight; + public int MaxConcurrent { get; private set; } + public int CallCount { get; private set; } + + public async Task GetByIdAsync(int id, CancellationToken ct = default) + { + var now = Interlocked.Increment(ref _inFlight); + lock (this) + { + CallCount++; + if (now > MaxConcurrent) MaxConcurrent = now; + } + + // A real query is not instantaneous. Without this the tasks can complete one at a + // time by luck and the overlap the test exists to catch would go unobserved. + await Task.Delay(20); + + Interlocked.Decrement(ref _inFlight); + return new Indexer { Id = id, Name = $"indexer-{id}", Type = "Usenet", Retention = 1500 }; + } + + public Task> GetAllAsync(CancellationToken ct = default) => + Task.FromResult(new List()); + public Task> GetEnabledAsync(bool isAutomaticSearch, CancellationToken ct = default) => + Task.FromResult(new List()); + public Task GetByNameAsync(string name, CancellationToken ct = default) => + Task.FromResult(null); + public Task AddAsync(Indexer indexer, CancellationToken ct = default) => + Task.FromResult(indexer); + public Task UpdateAsync(Indexer indexer, CancellationToken ct = default) => Task.CompletedTask; + public Task DeleteAsync(int id, CancellationToken ct = default) => Task.CompletedTask; + } + + [Fact] + public async Task ScoreSearchResults_DoesNotQueryTheIndexerRepositoryConcurrently() + { + var indexerRepository = new OverlapRecordingIndexerRepository(); + var service = new QualityProfileService( + Mock.Of(), + NullLogger.Instance, + indexerRepository); + + // Twelve results across three indexers: enough to overlap, and enough to show that the + // batch does not need one query per result. + var searchResults = Enumerable.Range(0, 12) + .Select(i => new SearchResult + { + Id = $"result-{i}", + Title = $"A Book {i}", + IndexerId = (i % 3) + 1, + Format = "mp3", + Language = "English", + PublishedDate = DateTime.UtcNow.AddDays(-1).ToString("o") + }) + .ToList(); + + var profile = new QualityProfile + { + MinimumSize = 0, + MaximumSize = 0, + PreferredFormats = ["mp3"], + PreferredWords = [], + MustNotContain = [], + MustContain = [], + PreferredLanguages = ["English"], + MinimumSeeders = 0, + MaximumAge = 3650 + }; + + var scores = await service.ScoreSearchResults(searchResults, profile); + + Assert.Equal(searchResults.Count, scores.Count); + Assert.Equal(1, indexerRepository.MaxConcurrent); + // Three distinct indexers, so three lookups rather than one per result. + Assert.Equal(3, indexerRepository.CallCount); + } + } +} From bf32ec8cf3a447f39f57f975fdc728476309a288 Mon Sep 17 00:00:00 2001 From: m4bard <304653687+m4bard@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:43:46 -0500 Subject: [PATCH 2/2] test(scoring): assert the pre-resolved indexer actually reaches the scorer The concurrency test asserts the batch makes three non-overlapping lookups for twelve results. Both of those are properties of the lookup, not of the use. Pass an empty dictionary to the scorer instead of the resolved one and the test still sees three non-overlapping calls and still passes, while every result silently loses its indexer retention and its Usenet detection. That is the same quietly wrong scoring the change exists to prevent, so the regression the test would have to catch is the one it cannot see. The new case gives the stub a ten day retention and a result published a hundred days ago, with the profile's MaximumAge set well beyond that so nothing else in the scorer can produce the rejection. An ignored or empty dictionary leaves retention at zero and the result accepted. No production code changed. Co-Authored-By: Claude Fable 5.1 --- .../QualityProfileScoringConcurrencyTests.cs | 53 ++++++++++++++++++- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/tests/Features/Application/Audiobooks/Quality/QualityProfileScoringConcurrencyTests.cs b/tests/Features/Application/Audiobooks/Quality/QualityProfileScoringConcurrencyTests.cs index 087e54e46..e91d25264 100644 --- a/tests/Features/Application/Audiobooks/Quality/QualityProfileScoringConcurrencyTests.cs +++ b/tests/Features/Application/Audiobooks/Quality/QualityProfileScoringConcurrencyTests.cs @@ -29,7 +29,7 @@ public class QualityProfileScoringConcurrencyTests : BaseTests // started on a context while another is in flight. Asserting on the real exception would // mean racing it, so this counts overlap directly: the stub records the highest number of // calls it ever had in flight at once. Anything above one is the condition EF refuses. - private sealed class OverlapRecordingIndexerRepository : IIndexerRepository + private sealed class OverlapRecordingIndexerRepository(int retention = 1500) : IIndexerRepository { private int _inFlight; public int MaxConcurrent { get; private set; } @@ -49,7 +49,7 @@ private sealed class OverlapRecordingIndexerRepository : IIndexerRepository await Task.Delay(20); Interlocked.Decrement(ref _inFlight); - return new Indexer { Id = id, Name = $"indexer-{id}", Type = "Usenet", Retention = 1500 }; + return new Indexer { Id = id, Name = $"indexer-{id}", Type = "Usenet", Retention = retention }; } public Task> GetAllAsync(CancellationToken ct = default) => @@ -107,5 +107,54 @@ public async Task ScoreSearchResults_DoesNotQueryTheIndexerRepositoryConcurrentl // Three distinct indexers, so three lookups rather than one per result. Assert.Equal(3, indexerRepository.CallCount); } + + // The overlap assertion above passes whether or not the resolved indexers reach the + // scorer: a batch that looks each indexer up once and then throws the answers away makes + // exactly the same three non-overlapping calls. This asks the other half of the question. + // Retention is 10 days and the result is 100 days old, so the indexer's own retention has + // to be the thing that rejects it. A profile MaximumAge well beyond 100 days means nothing + // else in the scorer can produce that rejection, and an empty or ignored dictionary leaves + // retention at 0 and the result accepted. + [Fact] + public async Task ScoreSearchResults_AppliesTheResolvedIndexerRetention() + { + var indexerRepository = new OverlapRecordingIndexerRepository(retention: 10); + var service = new QualityProfileService( + Mock.Of(), + NullLogger.Instance, + indexerRepository); + + var searchResults = new List + { + new() + { + Id = "aged-result", + Title = "A Book", + IndexerId = 7, + Format = "mp3", + Language = "English", + PublishedDate = DateTime.UtcNow.AddDays(-100).ToString("o") + } + }; + + var profile = new QualityProfile + { + MinimumSize = 0, + MaximumSize = 0, + PreferredFormats = ["mp3"], + PreferredWords = [], + MustNotContain = [], + MustContain = [], + PreferredLanguages = ["English"], + MinimumSeeders = 0, + MaximumAge = 3650 + }; + + var scores = await service.ScoreSearchResults(searchResults, profile); + + var score = Assert.Single(scores); + Assert.True(score.IsRejected); + Assert.Contains(score.RejectionReasons, reason => reason.Contains("indexer retention 10 days")); + } } }