From 24bc0497878122706edc25a1b7494a305ee92f7a Mon Sep 17 00:00:00 2001 From: m4bard <304653687+m4bard@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:35:38 -0500 Subject: [PATCH] NZBGet: warn once per failed history entry, and only for monitored entries in polls NZBGet keeps failed entries in history until somebody deletes them, and the history enrichment workflow warned about every failed entry it saw on every poll. A few old failures nobody had cleaned up therefore produced a warning each, per poll, forever. Monitor polls made it worse. They pass the set of download IDs the monitor cares about, but the history scan ignored that set. It walked the whole configured category, logged and merged everything in it, and NzbgetQueueFilter.FilterByIds discarded the surplus afterwards, so most of the warnings were about downloads Listenarr had never submitted. Two changes. The history scan now attributes an entry to a requested ID before it logs or merges it, whenever a poll supplies IDs. That rule sits next to FilterByIds as IsRequestedByIds. It admits a deliberate superset of what FilterByIds keeps, because an entry with no canonical NZBID can be attributed through the active row its title matches and is then dropped later for having an empty ID; erring wide leaves queue output unchanged. Polls that supply no IDs (the queue UI, the removal workflow) still read the whole category, and GetItemsAsync is unchanged. NzbgetFailedHistoryWarningTracker remembers which failed entries have been warned about and reports only first sightings. State is kept per client and per reading surface, and a read updates it according to how much of the history it actually saw. An unscoped read saw the whole category, so it replaces the stored set, and that is what evicts entries which have left history; memory stays bounded by the size of the history itself with no expiry timer, and the queue poller reads unscoped on a timer so eviction keeps happening. A scoped read saw only the downloads it asked about, so absence from it is not evidence of anything and it may only add. Without that split, a monitor poll would drop every entry outside its scope and the next full poll would report them all as new again. The tracker is registered as a singleton because the workflow that reads it is scoped. The warning template is unchanged for a first sighting and repeats log nothing. The state is in memory, so a restart warns once more for whatever is still in history, and an entry that leaves history and comes back warns again. What a user sees: the queue UI poll still reads the whole configured category and now warns once per failed entry instead of once per entry per poll, and monitor polls no longer warn about entries they do not own. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GDr4ZwWYwhX8fAy26Vq5KD --- .../DownloadClientRegistrationExtensions.cs | 7 +- .../DownloadClients/Nzbget/NzbgetAdapter.cs | 10 +- .../NzbgetFailedHistoryWarningTracker.cs | 103 ++++++ .../Nzbget/NzbgetHistoryEnrichmentWorkflow.cs | 48 ++- .../Nzbget/NzbgetQueueFilter.cs | 16 + .../Nzbget/NzbgetAdapterTests.cs | 300 ++++++++++++++++++ 6 files changed, 472 insertions(+), 12 deletions(-) create mode 100644 listenarr.infrastructure/DownloadClients/Nzbget/NzbgetFailedHistoryWarningTracker.cs diff --git a/listenarr.infrastructure/DependencyInjection/DownloadClients/DownloadClientRegistrationExtensions.cs b/listenarr.infrastructure/DependencyInjection/DownloadClients/DownloadClientRegistrationExtensions.cs index eb931dfe7..3f406a51a 100644 --- a/listenarr.infrastructure/DependencyInjection/DownloadClients/DownloadClientRegistrationExtensions.cs +++ b/listenarr.infrastructure/DependencyInjection/DownloadClients/DownloadClientRegistrationExtensions.cs @@ -226,11 +226,16 @@ private static IServiceCollection AddNzbgetWorkflows(this IServiceCollection ser sp.GetRequiredService(), DownloadClientTypes.Nzbget)); services.AddScoped(); + + // Warn-once state for failed history entries has to outlive the scoped workflow + // that reads it, or every poll would be a first sighting again. + services.AddSingleton(); services.AddScoped(sp => new NzbgetHistoryEnrichmentWorkflow( sp.GetRequiredService(), sp.GetRequiredService>(), - sp.GetRequiredService())); + sp.GetRequiredService(), + sp.GetRequiredService())); services.AddScoped(sp => new NzbgetConnectionTester( sp.GetRequiredService(), diff --git a/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetAdapter.cs b/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetAdapter.cs index c3279d927..d022aaa0e 100644 --- a/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetAdapter.cs +++ b/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetAdapter.cs @@ -59,7 +59,8 @@ internal NzbgetAdapter( IHttpClientFactory httpClientFactory, INzbUrlResolver nzbUrlResolver, ILogger logger, - TimeProvider timeProvider) + TimeProvider timeProvider, + NzbgetFailedHistoryWarningTracker? failedHistoryWarningTracker = null) { ArgumentNullException.ThrowIfNull(httpClientFactory); ArgumentNullException.ThrowIfNull(nzbUrlResolver); @@ -68,10 +69,15 @@ internal NzbgetAdapter( var xmlRpcClient = new NzbgetXmlRpcClient(httpClientFactory, ClientType); var historyReader = new NzbgetHistoryReader(xmlRpcClient); + + // This constructor builds a self-contained adapter for callers that have no + // container. Production resolves the adapter from DI, where the tracker is a + // singleton and so outlives the scoped workflow that reads it. var historyEnrichmentWorkflow = new NzbgetHistoryEnrichmentWorkflow( historyReader, logger, - timeProvider); + timeProvider, + failedHistoryWarningTracker ?? new NzbgetFailedHistoryWarningTracker()); _connectionTester = new NzbgetConnectionTester(xmlRpcClient, logger); _addWorkflow = new NzbgetAddWorkflow(xmlRpcClient, logger); diff --git a/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetFailedHistoryWarningTracker.cs b/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetFailedHistoryWarningTracker.cs new file mode 100644 index 000000000..f309f4c2e --- /dev/null +++ b/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetFailedHistoryWarningTracker.cs @@ -0,0 +1,103 @@ +/* + * 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 . + */ + +namespace Listenarr.Infrastructure.DownloadClients.Nzbget; + +/// +/// Remembers the failed NZBGet history entries already warned about, per download client and +/// reading surface, so an entry NZBGet keeps in history is reported the first time it is seen +/// instead of on every poll. State lives in memory only, so each entry still in history warns +/// once more after a restart. +/// +internal sealed class NzbgetFailedHistoryWarningTracker +{ + private readonly Lock _gate = new(); + private readonly Dictionary<(string Surface, string ClientKey), HashSet> _lastReadKeys = new(); + + /// + /// Records the failed entry keys one history read produced for a client and surface, and + /// returns the ones not already recorded. Keys that are empty are not tracked. Pass + /// isScopedRead when the read covered only part of the client's history, which is what a + /// monitor poll does because it asks NZBGet about specific downloads. + /// + public IReadOnlySet MarkFailed( + string clientKey, + string surface, + IReadOnlyCollection failedKeysThisRead, + bool isScopedRead) + { + var currentKeys = failedKeysThisRead + .Where(key => key.Length > 0) + .ToHashSet(StringComparer.OrdinalIgnoreCase); + + lock (_gate) + { + var previousKeys = _lastReadKeys.GetValueOrDefault((surface, clientKey)); + + // A scoped read saw only the entries it asked about, so absence from it is not + // evidence that anything left NZBGet history. It may only add. Letting it replace + // would drop every entry outside its scope, and the next unscoped read would then + // report those as new all over again. + // + // An unscoped read did see the whole category, so replacing the stored set is what + // evicts entries that have genuinely left history. That keeps memory bounded by + // the size of the history itself with no expiry timer, and it still happens + // regularly because the queue poller reads unscoped on a timer. An entry that + // leaves history and later comes back is a new sighting and warns again, which is + // the intent. + // + // The surface is part of the key because the surfaces read different slices, so + // one surface's read must not evict what another surface saw. + _lastReadKeys[(surface, clientKey)] = isScopedRead && previousKeys != null + ? [.. previousKeys, .. currentKeys] + : currentKeys; + + return currentKeys + .Where(key => previousKeys?.Contains(key) != true) + .ToHashSet(StringComparer.OrdinalIgnoreCase); + } + } + + /// + /// Collects the tracking keys of the failed entries in one history read, dropping the + /// entries that carry neither an ID nor a title. + /// + public static IReadOnlyCollection GetFailedEntryKeys( + IEnumerable historyEntries) + { + return historyEntries + .Where(entry => entry.Outcome == NzbgetHistoryOutcome.Failed) + .Select(entry => GetEntryKey(entry.CanonicalNzbId, entry.Title)) + .Where(key => key.Length > 0) + .ToList(); + } + + /// + /// Identifies a history entry by its canonical NZBID, falling back to its title when + /// NZBGet supplies no ID. An entry with neither returns an empty key and is not tracked. + /// + public static string GetEntryKey(string canonicalNzbId, string title) + { + if (!string.IsNullOrWhiteSpace(canonicalNzbId)) + { + return canonicalNzbId; + } + + return string.IsNullOrWhiteSpace(title) ? string.Empty : title; + } +} diff --git a/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetHistoryEnrichmentWorkflow.cs b/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetHistoryEnrichmentWorkflow.cs index 9a090945b..9c23c7d80 100644 --- a/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetHistoryEnrichmentWorkflow.cs +++ b/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetHistoryEnrichmentWorkflow.cs @@ -24,7 +24,8 @@ namespace Listenarr.Infrastructure.DownloadClients.Nzbget internal sealed class NzbgetHistoryEnrichmentWorkflow( NzbgetHistoryReader historyReader, ILogger logger, - TimeProvider timeProvider) + TimeProvider timeProvider, + NzbgetFailedHistoryWarningTracker failedHistoryWarningTracker) { private const long SlowHistoryThresholdMilliseconds = 2_000; private const string QueueSurface = "GetQueueAsync"; @@ -86,7 +87,8 @@ public async Task EnrichQueueAsync( activeIdentities, history, cancellationToken, - entry => TryMergeOrAppendQueueItem(client, entry, activeIdentities, items)); + entry => TryMergeOrAppendQueueItem(client, entry, activeIdentities, items), + monitoredIdSet); } catch (Exception ex) when (ex is not OperationCanceledException && ex is not OutOfMemoryException && ex is not StackOverflowException) { @@ -152,25 +154,36 @@ private void AppendHistory( IReadOnlyList activeIdentities, IReadOnlyList history, CancellationToken cancellationToken, - Action append) + Action append, + ISet? monitoredIds = null) { var processedHistoryIds = new HashSet(StringComparer.OrdinalIgnoreCase); var matchedTerminalActiveIds = new HashSet(StringComparer.OrdinalIgnoreCase); + var candidates = new List(); foreach (var entry in history) { cancellationToken.ThrowIfCancellationRequested(); - if (!IsHistoryCandidate( + if (IsHistoryCandidate( entry, configuredCategory, activeIdentities, processedHistoryIds, - matchedTerminalActiveIds)) + matchedTerminalActiveIds, + monitoredIds)) { - continue; + candidates.Add(entry); } + } - LogFailedHistoryEntry(client, surface, entry); + var unwarnedFailureKeys = failedHistoryWarningTracker.MarkFailed( + client.Id ?? client.Name ?? client.Type, + surface, + NzbgetFailedHistoryWarningTracker.GetFailedEntryKeys(candidates), + isScopedRead: monitoredIds is { Count: > 0 }); + foreach (var entry in candidates) + { + LogFailedHistoryEntry(client, surface, entry, unwarnedFailureKeys); append(entry); } @@ -182,7 +195,8 @@ private static bool IsHistoryCandidate( string? configuredCategory, IReadOnlyList activeIdentities, ISet processedHistoryIds, - ISet matchedTerminalActiveIds) + ISet matchedTerminalActiveIds, + ISet? monitoredIds) { if (entry.Outcome == NzbgetHistoryOutcome.Ignored || !DownloadClientCategoryFilter.Matches(configuredCategory, entry.Category)) @@ -198,6 +212,15 @@ private static bool IsHistoryCandidate( var activeMatch = FindActiveIdentity(activeIdentities, entry); + // A monitor poll asks about specific downloads and NzbgetQueueFilter.FilterByIds + // discards the rest of the category once enrichment has run. Attributing the + // entry here keeps those rows out of the log and out of the merge. + if (monitoredIds is { Count: > 0 } && + !NzbgetQueueFilter.IsRequestedByIds(entry.CanonicalNzbId, activeMatch, monitoredIds)) + { + return false; + } + // Active listgroups records are progress telemetry. They suppress older // history only while they still look like active work. If NZBGet reports // a terminal-looking active status, history becomes authoritative because @@ -288,13 +311,20 @@ private void TryMergeOrAppendDownloadClientItem( private void LogFailedHistoryEntry( DownloadClientConfiguration client, string surface, - NzbgetHistoryEntry entry) + NzbgetHistoryEntry entry, + IReadOnlySet unwarnedFailureKeys) { if (entry.Outcome != NzbgetHistoryOutcome.Failed) { return; } + var entryKey = NzbgetFailedHistoryWarningTracker.GetEntryKey(entry.CanonicalNzbId, entry.Title); + if (entryKey.Length > 0 && !unwarnedFailureKeys.Contains(entryKey)) + { + return; + } + logger.LogWarning( "NZBGet history reported failure for {NzbId}: Status={Status}, FinalDir={FinalDir}, DestDir={DestDir}, Title={Title}, Category={Category}, ClientId={ClientId}, Surface={Surface}", LogRedaction.SanitizeText(entry.CanonicalNzbId), diff --git a/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetQueueFilter.cs b/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetQueueFilter.cs index d1edf9221..88c651d60 100644 --- a/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetQueueFilter.cs +++ b/listenarr.infrastructure/DownloadClients/Nzbget/NzbgetQueueFilter.cs @@ -20,6 +20,22 @@ namespace Listenarr.Infrastructure.DownloadClients.Nzbget; internal static class NzbgetQueueFilter { + /// + /// Whether a history entry belongs to one of the requested IDs, either directly or + /// through the active queue row it matches. History enrichment applies this before it + /// logs or merges an entry, so it deliberately admits a superset of what + /// keeps: an entry with no canonical NZBID can be attributed + /// here through the active row its title matches, and is then dropped by FilterByIds, + /// which requires a non-empty item ID. Erring wide keeps queue output unchanged. + /// + public static bool IsRequestedByIds( + string canonicalNzbId, + NzbgetHistoryEnrichmentWorkflow.ActiveHistoryIdentity? activeMatch, + ISet ids) + { + return ids.Contains(canonicalNzbId) || activeMatch?.MatchesAny(ids) == true; + } + public static List FilterByIds( List items, List ids, diff --git a/tests/Features/Infrastructure/DownloadClients/Nzbget/NzbgetAdapterTests.cs b/tests/Features/Infrastructure/DownloadClients/Nzbget/NzbgetAdapterTests.cs index eaa06a5f4..7be378ffd 100644 --- a/tests/Features/Infrastructure/DownloadClients/Nzbget/NzbgetAdapterTests.cs +++ b/tests/Features/Infrastructure/DownloadClients/Nzbget/NzbgetAdapterTests.cs @@ -594,6 +594,262 @@ await FetchDownloadsThroughQueuePathAsync( Assert.Equal("/existing/path", download.DownloadPath); } + [Fact] + public async Task GetQueueAsync_RepeatedFailedHistoryEntry_WarnsOnceAndStillReturnsTheItem() + { + // AC: A failed entry NZBGet never purges must not be warned about on every poll. + // Behavior: Same failed history entry on two consecutive polls -> one warning, both results carry the item. + // @category: edge-case + // @lane: integration + // @dependency: NZBGet history enrichment and the failed-history warning tracker + // @complexity: medium + using var apiMock = new NzbgetApiMock(); + var failedEntry = HistoryEntryValue( + nzbId: "601", + title: "Repeated Failure Book", + status: "FAILURE/UNPACK", + fileSizeMb: "100", + downloadedSizeMb: "40"); + QueuePollingResponses(apiMock, [], [failedEntry]); + QueuePollingResponses(apiMock, [], [failedEntry]); + using var http = new HttpClient(apiMock); + var logger = new CapturingLogger(); + var adapter = CreateAdapter(http, logger); + var client = CreateClient(); + + var firstPoll = await adapter.GetQueueAsync(client, CancellationToken.None); + var secondPoll = await adapter.GetQueueAsync(client, CancellationToken.None); + + Assert.Equal("601", Assert.Single(firstPoll).Id); + Assert.Equal("failed", firstPoll[0].Status); + Assert.Equal("601", Assert.Single(secondPoll).Id); + Assert.Equal("failed", secondPoll[0].Status); + var warning = Assert.Single(FailedHistoryWarnings(logger)); + Assert.Equal("601", GetLogValue(warning, "NzbId")); + Assert.Equal("GetQueueAsync", GetLogValue(warning, "Surface")); + } + + [Fact] + public async Task GetQueueAsync_FailedHistoryEntryLeavesAndReturns_WarnsAgain() + { + // AC: Tracking must evict entries that leave NZBGet history so a genuine second failure is reported. + // Behavior: Failed entry present twice, absent, present again -> two warnings for the same NZBID. + // @category: edge-case + // @lane: integration + // @dependency: NZBGet history enrichment and the failed-history warning tracker + // @complexity: medium + using var apiMock = new NzbgetApiMock(); + var failedEntry = HistoryEntryValue( + nzbId: "602", + title: "Returning Failure Book", + status: "FAILURE/HEALTH"); + QueuePollingResponses(apiMock, [], [failedEntry]); + QueuePollingResponses(apiMock, [], [failedEntry]); + QueuePollingResponses(apiMock, [], []); + QueuePollingResponses(apiMock, [], [failedEntry]); + using var http = new HttpClient(apiMock); + var logger = new CapturingLogger(); + var adapter = CreateAdapter(http, logger); + var client = CreateClient(); + + await adapter.GetQueueAsync(client, CancellationToken.None); + await adapter.GetQueueAsync(client, CancellationToken.None); + var withoutEntry = await adapter.GetQueueAsync(client, CancellationToken.None); + await adapter.GetQueueAsync(client, CancellationToken.None); + + Assert.Empty(withoutEntry); + Assert.Equal( + ["602", "602"], + FailedHistoryWarnings(logger).Select(warning => GetLogValue(warning, "NzbId"))); + } + + [Fact] + public void FailedHistoryWarningTracker_ReplacesStoredKeysPerClientAndReportsFirstSightings() + { + // AC: Warn-once needs per-client, per-surface state that replaces rather than accumulates. + // Behavior: Repeated, changed and emptied reads -> only first sightings are returned. + // @category: core-functionality + // @lane: unit + // @dependency: NzbgetFailedHistoryWarningTracker + // @complexity: low + var tracker = new NzbgetFailedHistoryWarningTracker(); + + Assert.Equal( + ["123", "456"], + FullRead(tracker, "client-a", ["123", "456"]).Order(StringComparer.Ordinal)); + Assert.Empty(FullRead(tracker, "client-a", ["123", "456"])); + Assert.Equal(["789"], FullRead(tracker, "client-a", ["123", "789"])); + Assert.Equal(["456"], FullRead(tracker, "client-a", ["123", "456"])); + Assert.Equal(["123"], FullRead(tracker, "client-b", ["123"])); + Assert.Empty(FullRead(tracker, "client-a", [])); + Assert.Equal(["123"], FullRead(tracker, "client-a", ["123"])); + + // A read on the other surface neither inherits nor evicts what this one stored. + Assert.Equal( + ["123"], + tracker.MarkFailed("client-a", "GetItemsAsync", ["123"], isScopedRead: false)); + Assert.Empty(FullRead(tracker, "client-a", ["123"])); + Assert.Empty(tracker.MarkFailed("client-a", "GetItemsAsync", ["123"], isScopedRead: false)); + + // A scoped read adds without evicting, so what it could not see survives it. + Assert.Equal(["456"], FullRead(tracker, "client-c", ["456"])); + Assert.Equal(["123"], ScopedRead(tracker, "client-c", ["123"])); + Assert.Empty(FullRead(tracker, "client-c", ["123", "456"])); + + // An empty key is never tracked, so an entry that has one warns on every read. + Assert.Empty(FullRead(tracker, "client-d", [string.Empty])); + Assert.Empty(FullRead(tracker, "client-d", [string.Empty])); + + Assert.Equal( + ["901", "Titled Failure"], + NzbgetFailedHistoryWarningTracker.GetFailedEntryKeys( + [ + HistoryEntry("901", "Keyed Failure", NzbgetHistoryOutcome.Failed), + HistoryEntry(string.Empty, "Titled Failure", NzbgetHistoryOutcome.Failed), + HistoryEntry(string.Empty, string.Empty, NzbgetHistoryOutcome.Failed), + HistoryEntry("902", "Completed Book", NzbgetHistoryOutcome.Completed) + ])); + + Assert.Equal("900", NzbgetFailedHistoryWarningTracker.GetEntryKey("900", "Keyed Book")); + Assert.Equal( + "Keyed Book", + NzbgetFailedHistoryWarningTracker.GetEntryKey(string.Empty, "Keyed Book")); + Assert.Equal( + string.Empty, + NzbgetFailedHistoryWarningTracker.GetEntryKey(" ", " ")); + } + + [Fact] + public async Task GetQueueAsync_MonitorPoll_LogsAndReturnsOnlyMonitoredFailedHistory() + { + // AC: Monitor polls discard unrequested history afterwards, so it must not be warned about first. + // Behavior: History holds two failed entries, only one requested -> one warning and one result. + // @category: core-functionality + // @lane: integration + // @dependency: NZBGet history enrichment scoping and NzbgetQueueFilter.FilterByIds + // @complexity: medium + using var apiMock = new NzbgetApiMock(); + QueuePollingResponses( + apiMock, + [], + [ + HistoryEntryValue(nzbId: "123", title: "Monitored Failure Book", status: "FAILURE/UNPACK"), + HistoryEntryValue(nzbId: "456", title: "Unmonitored Failure Book", status: "FAILURE/UNPACK") + ]); + using var http = new HttpClient(apiMock); + var logger = new CapturingLogger(); + var adapter = CreateAdapter(http, logger); + + var queue = await adapter.GetQueueAsync(CreateClient(), ["123"], CancellationToken.None); + + Assert.Equal("123", Assert.Single(queue).Id); + var warning = Assert.Single(FailedHistoryWarnings(logger)); + Assert.Equal("123", GetLogValue(warning, "NzbId")); + } + + [Fact] + public async Task GetQueueAsync_NonMonitorPoll_StillReturnsUnrequestedFailedHistory() + { + // AC: The queue UI reads the whole configured category, so an unmonitored failure stays visible. + // Behavior: Same history without requested IDs -> both failed entries returned and warned about. + // @category: core-functionality + // @lane: integration + // @dependency: NZBGet history enrichment scoping + // @complexity: medium + using var apiMock = new NzbgetApiMock(); + QueuePollingResponses( + apiMock, + [], + [ + HistoryEntryValue(nzbId: "123", title: "Monitored Failure Book", status: "FAILURE/UNPACK"), + HistoryEntryValue(nzbId: "456", title: "Unmonitored Failure Book", status: "FAILURE/UNPACK") + ]); + using var http = new HttpClient(apiMock); + var logger = new CapturingLogger(); + var adapter = CreateAdapter(http, logger); + + var queue = await adapter.GetQueueAsync(CreateClient(), CancellationToken.None); + + Assert.Equal(["123", "456"], queue.Select(item => item.Id)); + Assert.Equal( + ["123", "456"], + FailedHistoryWarnings(logger).Select(warning => GetLogValue(warning, "NzbId"))); + } + + [Fact] + public async Task GetQueueAsync_ScopedPollBetweenFullPolls_DoesNotResurrectUnmonitoredFailureWarnings() + { + // AC: A monitor poll sees only what it asked about, so it must not evict what a full poll saw. + // Behavior: Full poll, monitor poll, full poll over the same history -> each entry warns once. + // @category: edge-case + // @lane: integration + // @dependency: NZBGet history enrichment scoping and the failed-history warning tracker + // @complexity: high + using var apiMock = new NzbgetApiMock(); + var failedEntries = new[] + { + HistoryEntryValue(nzbId: "123", title: "Monitored Failure Book", status: "FAILURE/UNPACK"), + HistoryEntryValue(nzbId: "456", title: "Unmonitored Failure Book", status: "FAILURE/UNPACK") + }; + for (var read = 0; read < 3; read++) + { + QueuePollingResponses(apiMock, [], failedEntries); + } + + using var http = new HttpClient(apiMock); + var logger = new CapturingLogger(); + var adapter = CreateAdapter(http, logger); + var client = CreateClient(); + + await adapter.GetQueueAsync(client, CancellationToken.None); + await adapter.GetQueueAsync(client, ["123"], CancellationToken.None); + await adapter.GetQueueAsync(client, CancellationToken.None); + + Assert.Equal( + ["123", "456"], + FailedHistoryWarnings(logger).Select(warning => GetLogValue(warning, "NzbId"))); + } + + [Fact] + public async Task GetQueueAndItemsAsync_AlternatingReads_DoNotRepeatEachOthersFailureWarnings() + { + // AC: The two surfaces read different slices of history and must not evict each other. + // Behavior: Monitor poll, items read, monitor poll, items read -> each surface warns once per entry. + // @category: edge-case + // @lane: integration + // @dependency: NZBGet history enrichment scoping and the failed-history warning tracker + // @complexity: high + using var apiMock = new NzbgetApiMock(); + var failedEntries = new[] + { + HistoryEntryValue(nzbId: "123", title: "Monitored Failure Book", status: "FAILURE/UNPACK"), + HistoryEntryValue(nzbId: "456", title: "Unmonitored Failure Book", status: "FAILURE/UNPACK") + }; + for (var read = 0; read < 4; read++) + { + QueuePollingResponses(apiMock, [], failedEntries); + } + + using var http = new HttpClient(apiMock); + var logger = new CapturingLogger(); + var adapter = CreateAdapter(http, logger); + var client = CreateClient(); + + await adapter.GetQueueAsync(client, ["123"], CancellationToken.None); + await adapter.GetItemsAsync(client, CancellationToken.None); + await adapter.GetQueueAsync(client, ["123"], CancellationToken.None); + await adapter.GetItemsAsync(client, CancellationToken.None); + + Assert.Equal( + [ + "GetQueueAsync/123", + "GetItemsAsync/123", + "GetItemsAsync/456" + ], + FailedHistoryWarnings(logger).Select(warning => + $"{GetLogValue(warning, "Surface")}/{GetLogValue(warning, "NzbId")}")); + } + [Fact] public async Task QueuePath_HistoryMatching_PrioritizesCanonicalIdBeforeSimilarTitle() { @@ -3438,6 +3694,50 @@ private static void QueuePollingResponses( NzbgetApiMock.CreateHistoryResponse(string.Concat(historyEntries))); } + private static IReadOnlySet FullRead( + NzbgetFailedHistoryWarningTracker tracker, + string clientKey, + IReadOnlyCollection failedKeys) + { + return tracker.MarkFailed(clientKey, "GetQueueAsync", failedKeys, isScopedRead: false); + } + + private static IReadOnlySet ScopedRead( + NzbgetFailedHistoryWarningTracker tracker, + string clientKey, + IReadOnlyCollection failedKeys) + { + return tracker.MarkFailed(clientKey, "GetQueueAsync", failedKeys, isScopedRead: true); + } + + private static NzbgetHistoryEntry HistoryEntry( + string canonicalNzbId, + string title, + NzbgetHistoryOutcome outcome) + { + return new NzbgetHistoryEntry + { + CanonicalNzbId = canonicalNzbId, + Title = title, + Category = "audiobooks", + RawStatus = outcome == NzbgetHistoryOutcome.Failed ? "FAILURE/UNPACK" : "SUCCESS/ALL", + Outcome = outcome, + DestDir = string.Empty, + FinalDir = string.Empty, + TotalSizeBytes = 0, + DownloadedSizeBytes = 0, + HistoryTimeUtc = null + }; + } + + private static List FailedHistoryWarnings(CapturingLogger logger) + { + return logger.Entries + .Where(entry => entry.Level == LogLevel.Warning && + entry.Message.StartsWith("NZBGet history reported failure", StringComparison.Ordinal)) + .ToList(); + } + private static string GetLogValue(CapturedLog entry, string key) { return entry.State.TryGetValue(key, out var value)