fix(qbittorrent): one unreadable torrent should not truncate the queue poll - #830
Open
m4bard wants to merge 1 commit into
Open
fix(qbittorrent): one unreadable torrent should not truncate the queue poll#830m4bard wants to merge 1 commit into
m4bard wants to merge 1 commit into
Conversation
…e poll A throw while mapping torrent N escaped the loop walking the response, so torrents N..end were dropped while the poll still reported itself as a healthy live snapshot. The queue simply looked shorter, with nothing to say a row had been lost, and the only warning claimed the client might be unreachable when it had answered fine and answered completely. Guard each torrent individually in both loops, logging the hash and continuing. SabnzbdQueueFetchWorkflow and TransmissionQueueFetchWorkflow already do exactly this, so qBittorrent converges on what its two closest neighbours share rather than introducing a third approach. The hash read now checks ValueKind before GetString(), so a non-string hash cannot throw before the guard is entered. Refs Listenarrs#829
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
One torrent that cannot be mapped currently takes every torrent after it out of the same qBittorrent poll, and the poll still reports itself as a healthy live snapshot. This wraps the per-torrent body of the two loops in a
try/catchso an unreadable torrent costs one torrent rather than the tail of the batch.Full write-up, measurements and the alternative I did not take are in #829.
Changes
Fixed
QbittorrentQueueFetchWorkflow.GetQueueAsyncandQbittorrentItemFetchWorkflow.GetItemsAsyncguard each torrent individually. A throw fromMapQueueItemorMapDownloadClientItemis logged with the torrent hash and the loop continues, instead of unwinding to the outer handler that reports the client as possibly unreachable.hashread in both loops checksValueKindbefore callingGetString(), so a non-string hash cannot throw before the guard is entered.Testing
QbittorrentAdapterTestsgains a theory covering three token forms of a numeric field that the typed accessor rejects: a fractional number, a quoted number, and exponent form. Each asserts the torrents either side of the bad one still arrive and the bad one is absent. All three fail onc92e6089and pass here.The existing qBittorrent tests still pass: 55 total, 0 failed.
Reproduced end to end before and after against
ghcr.io/listenarrs/listenarr:canary, using a stub that serves the qBittorrent WebUI routes the adapter calls, so the trigger is a response shape rather than a particular client state. The stub and the check are public, in the test-data repo linked from the issue.Notes
I chose the per-item guard because
SabnzbdQueueFetchWorkflowandTransmissionQueueFetchWorkflowalready do exactly this, so qBittorrent converges on what its two closest neighbours share rather than adding a third approach.There is a larger change I deliberately left out. #634 made the NZBGet readers tolerant of token form, which keeps the item and may hand downstream code a defaulted field, where Sabnzbd and Transmission drop the item and keep the rest of the poll. Doing the same to
QbittorrentResponseMapperwould keep the partly-downloaded torrent rather than dropping it. I left it out because on a list feeding import decisions a silently defaultedsizeorprogressmay well be worse than a torrent that is openly missing and logged, and because it is the more invasive of the two. That is your call rather than mine, and I have the tolerant readers on another branch if you would rather have them as well, or instead.Two call sites with the same shape are untouched here and I have not reproduced either:
QbittorrentTorrentLookupBuilderreads six fields through the same throwing accessors inline, and the item-fetch path feeds completion and import decisions, so a torrent lost there stops being considered for import at all. The guard in this PR covers the second of those; the lookup builder does not have one.