Fix NZBGet import completion#634
Conversation
NzbgetAdapter.FetchDownloadsAsync read the listgroups fields FileSizeMB and RemainingSizeMB via JsonElement.GetString(), but NZBGet returns them as JSON Number, so GetString() threw InvalidOperationException by design. The inner try/catch suppressed the exception but aborted progress mapping for the group and emitted "Error updating NZBGet queue progress for group", so completed downloads were never imported. NZBGet has always documented these fields as int, so the fix reads them straight into a double via a new JsonExtensions.GetDoubleOrDefault helper. The downstream double.TryParse round-trip goes away. NzbgetApiMock gets a /jsonrpc route returning a canned listgroups response with numeric size fields plus public constants (ACTIVE_DOWNLOAD_NZBID / FILE_SIZE_MB / REMAINING_SIZE_MB). The test builds a Download via DownloadBuilder with the mock's NZBID, runs FetchDownloadsAsync, and asserts the resulting Status / Progress / Metadata["ClientState"|"AmountLeft"] are correctly mutated by AdapterUtils.MapDownloadProgress. Addresses review feedback on #628 (inline comment removed, NzbgetApiMock used directly, builders for client and Download, state assertion instead of log-warning absence, helper in Listenarr.Application.Extensions.JsonExtensions with double return type). Signed-off-by: pascalriemer <155780777+pascalriemer@users.noreply.github.com>
- Drop JsonExtensions.GetDoubleOrDefault (added in 5c289e9). The existing GetPropertyOrDefault<T> deserialises a JSON Number into double via JsonSerializer.Deserialize<double>(GetRawText()) without going through a string, so a new method adds no value. NzbgetAdapter now calls group.GetPropertyOrDefault("FileSizeMB", 0d) directly. - Refactor NzbgetAdapterTests onto BaseTests + IDownloadClientGateway, matching SabnzbdAdapterTests. _provider resolves the gateway, _downloadClientConfigurationRepository persists the test config, and the NzbgetApiMock is wired automatically through the named "nzbget" HttpClient registered by ServiceCollectionBuilder. The three existing TestConnection*/GetQueue tests keep their hand-rolled handlers and are not touched. - Assert Download.Status and Download.Progress (direct properties) in the new test, instead of Metadata["ClientState"]/["AmountLeft"]. DownloadedSize would also be a direct property but is 0 here because Download.TotalSize defaults to 0 (and an existing off-by-100 between the adapter's 0..1 progress formula and MapDownloadProgress's 0..100 input would otherwise show through — pre-existing, separate concern). - Drop the comment in NzbgetApiMock.GetJsonrpc that referenced the adapter's polling behaviour. Signed-off-by: pascalriemer <155780777+pascalriemer@users.noreply.github.com>
therobbiedavis
added
the
patch
There was a problem hiding this comment.
Pull request overview
This PR fixes NZBGet completion/import reconciliation by improving JSON-RPC listgroups parsing, normalizing progress reporting to the 0–100 scale used by AdapterUtils.MapDownloadProgress, and adding a history-based reconciliation pass so downloads that have already left the active queue can still be marked Completed and queued for import with a resolved source path.
Changes:
- Update NZBGet JSON-RPC queue polling to read numeric size fields as numbers and report progress as a 0–100 percentage.
- Reconcile tracked downloads against NZBGet history (by
NZBID) to detect completions that have leftlistgroups, preferringFinalDirwithDestDirfallback for the import path. - Expand test/mocks to cover numeric progress mapping, history reconciliation behavior, and best-effort handling when history is temporarily unavailable.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
listenarr.infrastructure/Adapters/NzbgetAdapter.cs |
Adds history reconciliation helpers, resolves import paths via FinalDir/DestDir, normalizes listgroups progress to 0–100, and makes history lookup best-effort. |
tests/Mocks/Api/NzbgetApiMock.cs |
Extends the NZBGet mock to support JSON-RPC listgroups, distinct history ID vs NZBID, selectable active queue presence, and simulated history failures. |
tests/Features/Infrastructure/Adapters/NzbgetAdapterTests.cs |
Adds regression coverage for numeric progress mapping, skipping history lookup when not needed, best-effort history failures, and completion→job enqueue with resolved path. |
CHANGELOG.md |
Adds a changelog entry describing the NZBGet import completion fix. |
| var nzbName = group.TryGetProperty("NZBName", out var nameProp) ? nameProp.GetString() ?? "" : ""; | ||
| var status = group.TryGetProperty("Status", out var statusProp) ? statusProp.GetString() ?? "" : ""; | ||
| var fileSizeMB = group.TryGetProperty("FileSizeMB", out var sizeProp) ? sizeProp.GetString() ?? "" : ""; | ||
| var remainingSizeMB = group.TryGetProperty("RemainingSizeMB", out var remainingSizeProp) ? remainingSizeProp.GetString() ?? "" : ""; | ||
| var fileSizeMB = group.GetPropertyOrDefault("FileSizeMB", 0d); | ||
| var remainingSizeMB = group.GetPropertyOrDefault("RemainingSizeMB", 0d); |
| // Issue #618 — NZBGet's JSON-RPC `listgroups` returns FileSizeMB / RemainingSizeMB | ||
| // as JSON Number; verify FetchDownloadsAsync maps progress onto the matching | ||
| // Download via AdapterUtils.MapDownloadProgress, instead of throwing | ||
| // InvalidOperationException at the Number-as-String access (the original bug). |
Delete the repository's CHANGELOG.md file (file removed in this changeset).
| - Use provide/inject for deep component communication | ||
| - Use async components for code-splitting | ||
|
|
||
| --- |
There was a problem hiding this comment.
fyi: If we are more rigorous regarding the commit messages, we could parse them to generate the changelog automatically. In Jira, I also did some proof of concept to manage changelog based on ticket/issue metadata that may be done too with Github issue/PR
| }; | ||
| } | ||
|
|
||
| var response = """ |
There was a problem hiding this comment.
Maybe we can simplify this a bit with the $$""" notation (i was not aware of it's existence before the previous PR)
| return MockUtils.GetCannedResponse(response, "text/xml"); | ||
| } | ||
|
|
||
| private async Task<HttpResponseMessage> GetJsonrpc(HttpRequestMessage request, CancellationToken ct) |
There was a problem hiding this comment.
Another suggestion but if NZBGet allows to do the same thing with XML and JSON RPC, we probably want to stick to one unless we have good reasons to keep both
| public class NzbgetAdapterTests | ||
| public class NzbgetAdapterTests : BaseTests | ||
| { | ||
| private DownloadClientConfiguration? _client; |
There was a problem hiding this comment.
i believe you can also use the null! notation here if you want
| return null; | ||
| } | ||
|
|
||
| private sealed record NzbgetHistoryItem(string NzbId, string Name, string Status, string ContentPath); |
There was a problem hiding this comment.
I would put this at the start or at the end of the class declaration (depending on which standard you want to follow) but not in the middle of it
|
|
||
| private sealed record NzbgetHistoryItem(string NzbId, string Name, string Status, string ContentPath); | ||
|
|
||
| private static IEnumerable<IReadOnlyDictionary<string, string>> EnumerateXmlRpcStructMembers(XElement? arrayData) |
There was a problem hiding this comment.
This should be in a XML utils file probably (it will probably be handy for Sabnzbd too)
| status.StartsWith("FAILED", StringComparison.OrdinalIgnoreCase); | ||
| } | ||
|
|
||
| private static double GetJsonDoubleOrDefault(JsonElement element, string propertyName, double defaultValue = 0d) |
There was a problem hiding this comment.
Why not reuse GetPropertyOrDefault in JsonExtensions ?
| try | ||
| { | ||
| var nzbId = group.TryGetProperty("NZBID", out var nzbIdProp) ? nzbIdProp.GetInt32() : 0; | ||
| if (nzbId > 0) |
There was a problem hiding this comment.
Are you sure 0 is an invalid NZBID ? (i dont see anything about that here: https://nzbget.com/documentation/api/listgroups/)
| { | ||
| await ApplyHistoryUpdatesAsync(client, downloads, activeNzbIds); | ||
| } | ||
| catch (Exception ex) when (ex is not OperationCanceledException && ex is not OutOfMemoryException && ex is not StackOverflowException) |
There was a problem hiding this comment.
What do you think of catch (Exception exception) when (exception is not (OperationCanceledException or OutOfMemoryException or StackOverflowException)) instead ?
| } | ||
| } | ||
|
|
||
| private static string ResolveHistoryContentPath(IReadOnlyDictionary<string, string> members) |
There was a problem hiding this comment.
Its not obvious what this method does with only the prototype, maybe we want to document what is the members parameter or what it does
Add an UPSTREAM_PRS list (by PR number); each is fetched fresh from refs/pull/<N>/head every run and merged like a patch branch. Extract the merge+rerere logic into a shared merge_branch() helper. Seeded with Listenarrs#634 (NZBGet import fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Adjacent gap: Not asking to expand this PR's scope — flagging for a possible follow-up, since it lives in the exact reconciliation path this PR adds. (Independently verified the SUCCESS path here works: deployed an equivalent fix and watched 6 stranded grabs go Queued→Moved with real files imported — so this isn't a critique, just an adjacent gap found in the same path on a production instance.) NZBGet's built-in DupeCheck removes a redundant grab from the active queue and records it in history as This is not rare in practice. Any title grabbed more than once (re-search, manual + auto, etc.) trips DupeCheck: on one instance 147 of 501 history items were Suggested handling (the nuance is block vs fail):
Two implementation notes from doing this elsewhere:
Happy to open a separate PR for this if useful, or it can fold into this one — your call on scope. |
Add an UPSTREAM_PRS list (by PR number); each is fetched fresh from refs/pull/<N>/head every run and merged like a patch branch. Extract the merge+rerere logic into a shared merge_branch() helper. Seeded with Listenarrs#634 (NZBGet import fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Fixes #619 and follows up #628.
This PR keeps the original NZBGet
listgroupsJSON-number fix from #628, then completes the larger import path: NZBGet downloads that have already left the active queue can now be reconciled from history, marked complete, and queued for import with a usable source path.Changed
NzbgetAdapter.FetchDownloadsAsyncnow readsFileSizeMBandRemainingSizeMBas JSON numbers or numeric strings.AdapterUtils.MapDownloadProgress.ClientDownloadId/ NZBGetNZBIDinstead of the history rowID.FinalDirand fall back toDestDirwhen resolving the import source path.listgroupsNZB IDs are excluded from the history pass, so normal active progress polling does not also scan history..github/CLAUDE.mdno longer instructs AI agents to add changelog entries for meaningful changes.Fixed
listgroupsJSON type issue where numeric size fields could throw during polling, while preserving support for numeric strings from proxies or mocks.0and skip progress updates.0.5%instead of50%.listgroupsbefore Listenarr could mark themCompletedand enqueue import processing.IDinstead of the actual NZB download ID.audiobooks, in the Quality column.Added
NZBID,Status,NZBName,FinalDir, andDestDir.listgroupsprogress mapping, including the expected50Mprogress value.IDvsNZBID, active queue toggling, completed content paths, history request counts, simulated history failures, and string-valued active queue sizes.Removed
IDis the same identifier stored on tracked downloads.FileSizeMB/RemainingSizeMBinFetchDownloadsAsync.0.2.72changelog entry for this PR..github/CLAUDE.md.Testing
dotnet test tests/Listenarr.Tests.csproj --filter FullyQualifiedName~NzbgetAdapterTests- 8 passeddotnet test tests/Listenarr.Tests.csproj --filter FullyQualifiedName~DownloadClientAdapterTests- 7 passeddotnet test tests/Listenarr.Tests.csproj --filter "FullyQualifiedName~UsenetAdapterFilteringTests|FullyQualifiedName~NzbgetAdapterTests"- 10 passeddotnet test tests/Listenarr.Tests.csproj --no-build- 677 passedNote: backend restore/build still reports the pre-existing
NU1902advisory forSharpCompress 0.47.4.