Skip to content

fix(qbittorrent): one unreadable torrent should not truncate the queue poll - #830

Open
m4bard wants to merge 3 commits into
Listenarrs:canaryfrom
m4bard:fix/bug24-queue-guard
Open

m4bard wants to merge 3 commits into
Listenarrs:canaryfrom
m4bard:fix/bug24-queue-guard

Conversation

@m4bard

@m4bard m4bard commented Aug 19, 2026

Copy link
Copy Markdown

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/catch so 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.GetQueueAsync and QbittorrentItemFetchWorkflow.GetItemsAsync guard each torrent individually. A throw from MapQueueItem or MapDownloadClientItem is logged with the torrent hash and the loop continues, instead of unwinding to the outer handler that reports the client as possibly unreachable.
  • The hash read in both loops checks ValueKind before calling GetString(), so a non-string hash cannot throw before the guard is entered.

Testing

QbittorrentAdapterTests gains 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 on c92e6089 and 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 SabnzbdQueueFetchWorkflow and TransmissionQueueFetchWorkflow already 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 QbittorrentResponseMapper would keep the partly-downloaded torrent rather than dropping it. I left it out because on a list feeding import decisions a silently defaulted size or progress may 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: QbittorrentTorrentLookupBuilder reads 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.

@m4bard
m4bard requested a review from a team August 19, 2026 21:14
@m4bard

m4bard commented Aug 21, 2026

Copy link
Copy Markdown
Author

Putting this on the oldest of the current batch rather than opening a meta-issue, since it is about the batch rather than about any one change.

I have fourteen pull requests open against this repository now, eleven of them from the last two days. That is a lot of review surface to land on one person, and I would rather ask how you want it handled than keep adding to it and hope.

Some options, and I have no preference between them:

  • Pace. I can stop opening new ones until some of these land, and keep filing issues with the evidence instead. Nothing is lost that way. The issues stand on their own and each says what the fix would be, so the work is recorded whether or not a PR exists.
  • Order. If you tell me which two or three are worth looking at first I will leave the rest alone until those are settled.
  • Consolidation. I can fold related ones together if that helps, though I would push back gently on merging unrelated ones: they are separately revertable as they stand, and a combined diff is usually harder to review rather than easier.
  • Close some. If any of them are not changes you want, say so and I will close them. That is a perfectly good answer and it costs me nothing.

If it is useful, my own read on priority is that three are availability failures and the rest are correctness or quality:

Those three are the ones I would look at first if they were mine, and #866 particularly, because it is reachable by anyone upgrading while a hardlink or copy is in flight, so it will keep producing new instances until it is fixed.

Everything else can wait indefinitely as far as I am concerned. There is no deadline on any of it and I would rather these were reviewed at whatever rate is actually sustainable than land quickly.

…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
m4bard and others added 2 commits September 11, 2026 12:04
The per-item guard added on this branch wraps the per-torrent files request as
well as the mapper. A transport failure there is not an unreadable torrent, it
is the client going away between the torrent list and the files calls that
follow it, and every remaining torrent raises the same exception. Swallowing
them logged one warning per torrent and handed the caller a short queue that
still claimed to be a healthy live snapshot, so a monitor poll counted as a
success and neither backed off nor said anything.

Exclude HttpRequestException from the per-item filter so it reaches the outer
handler, which already raises DownloadClientAdapterPollingException on a
monitor poll. An error status from the files request is unaffected: that is
handled by the IsSuccessStatusCode check and never reaches the guard.

SabnzbdQueueFetchWorkflow and TransmissionQueueFetchWorkflow, which this branch
converges on, both guard only the mapper and make no request inside the guarded
block, so this restores the shape the commit message claims.

Also add the item-fetch case. GetItemsAsync had the same guard and no test, so
deleting it left the file green, and that list is what completion and import
decisions are made from.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The skip log is a recurring condition rather than an event. A torrent
whose downloaded field the typed accessor rejects does not heal, so at
the monitor's thirty second default the same line is written on every
poll for as long as that torrent sits in the client, and there is
nothing an operator can act on. TransmissionQueueFetchWorkflow already
logs the identical condition at Debug and says in a comment that it is
non-fatal. Both new calls move to Debug. The torrent's absence from the
queue stays observable either way, which is what the level was buying.

The same loop now checks the token form of the hash before reading it,
which makes leaving that value undefended two lines later, where it is
interpolated into the torrents/files query, an odd place to stop. A
hash carrying an ampersand truncates at the delimiter, so the client is
asked about a torrent it does not have and answers 200 with an empty
file list and no error at all. Uri.EscapeDataString is what the same
method already uses for fields and for hashes.

Tests: the skip is logged at Debug on both the queue and the item path,
and a hash carrying a URL delimiter reaches the files request whole.
Putting either call back to Warning fails the first pair; removing the
escaping fails the third, with the hash truncated at the ampersand.

Nothing in this suite asserted a log level before, so the level cases
need a recording ILoggerProvider. It also has to raise the filter:
AddLogging floors the factory at Information, so a LogDebug call reaches
no provider and an assertion written without that would have passed on
an empty collection rather than failing.

Focused qBittorrent filter: 62 passed, 0 failed. Full backend suite:
3128 passed, 0 failed, 130 skipped.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant