feat(downloads): blocklist a release that failed, so the retry stops - #918
Open
m4bard wants to merge 3 commits into
Open
feat(downloads): blocklist a release that failed, so the retry stops#918m4bard wants to merge 3 commits into
m4bard wants to merge 3 commits into
Conversation
Listenarrs#838: when a grabbed release fails and FailedDownloadAutoSearch is on, Listenarr searches again, finds the same broken release because nothing records that it failed, grabs it, and repeats. On a title whose indexer has one listing this never terminates. Adds the data model and the search-path exclusion. Deliberately not the management UI from the issue's third bullet: that is a separate surface and this is already the larger half. WHAT IS KEYED ON. Not the download client's id. A qBittorrent info-hash happens to identify the release, but a SABnzbd nzo_id is allocated per submission, so a blocklist keyed on it would never match the same release again and would silently do nothing on Usenet, which is the protocol this issue was reported against. ReleaseIdentity prefers a torrent info-hash, which identifies one release across indexers, and otherwise hashes the release URL. A torrent is matched by hash even when the indexer hands back a different URL than last time, which has a test. Entries are scoped to the audiobook. Blocking one release must not ban the title, and there is a test that fails if it ever does, because that failure would take a book permanently out of circulation and would look like the search being broken rather than the blocklist being wrong. WHY A 409 CANNOT LAND HERE. The entry is written in HandleFailedDownloadAsync, which only sees downloads the client accepted and then failed. A release the client refused at submission never reaches it. That matters because qBittorrent answers 409 when it already holds the release, which happens when one release satisfies several books (Listenarrs#881): blocking on that would ban a release the user is currently downloading. The carve-out is structural rather than a condition someone has to remember, which is why the check sits where it does. Sonarr draws the same line, with DownloadClientRejectedReleaseException reaching ProcessDecisionInternal and returning Skipped with nothing recorded. Filtering happens before selection rather than at display time. When every candidate is excluded the search ends at the existing "no acceptable search results" answer, which is a terminal state the pipeline already has, so this adds no new error case. ON THE TESTS, because two versions of this were worthless. The BlockedReleaseFilter unit tests pass whether or not DownloadService calls it, so they prove the helper and nothing about the wiring. BlocklistSearchWiringTests covers that, and it asserts its own precondition first: an earlier draft passed because the search failed with "Audiobook has no quality profile assigned" and never reached the filter at all. It now fails if the call is removed, which is checked. Full suite: 3126 passed, 0 failed, 130 skipped. Conflicts to expect: this edits the pinned migration list in SqliteMigrationSchemaTests and the model snapshot, so it collides with any other PR adding a migration. Whichever lands second wants a rebase.
…-grab The blocklist never blocked anything on Usenet. It wrote a row on every failure and matched none of them. ReleaseIdentity fell back to a hash of the release URL when there was no torrent info-hash. A Usenet release URL is a per-fetch download link: the indexer mints a new one, with a new token, every time the release is grabbed. So the identity written when a download failed was never the identity computed for the same release at the next search, and the filter had nothing to match. Observed on a live install: one dead Usenet post grabbed several hundred times over roughly twelve hours for a single book, identical title and identical size to the byte every time, a different URL hash every time, and a new blocklist row every time. Those rows were almost the entire table. This class's own summary already warned about exactly this shape, for the client id rather than the URL: an id allocated per submission "would produce an entry that never matches the same release again and a blocklist that silently does nothing on Usenet". The URL fallback had the same property and shipped anyway. Identity is now the info-hash where there is one, then the title and size together, then the URL as a last resort for a release with no usable title. Title and size are what an indexer reports identically for the same post across grabs. Size is in the key so a shared title cannot collapse two genuinely different releases onto one entry, and there is a test for that. Both call sites pass the same inputs, which matters more than it looks: the write side and the read side computing identity differently is how this failed in the first place. One existing test asserted the old behaviour, that two different URLs are two different releases. That is the defect stated as an expectation, so it is replaced rather than adapted. Five tests, three of which fail without the change, including an end-to-end one through the filter with a release whose download link changed since it was blocked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The blocklist was wired into SearchAndDownloadAsync, which covers the manual search endpoint and the retry that follows a failure. It was not wired into AutomaticSearchService, which scores results itself and calls StartDownloadAsync directly. That service is the periodic path, so a release could fail, be blocked, and be grabbed again by the next automatic pass a minute later, forever. Seen on a live install after the identity fix landed: the same release grabbed and failed every sixty to ninety seconds for hours, blocklist rows accumulating the whole time and none of them consulted. The automatic path now excludes blocked releases before it picks a winner, which makes BlockedReleaseFilter public. It was internal to the application assembly and this caller is in infrastructure. Also adds the test that would have caught this. BlocklistSearchWiringTests already existed for exactly this class of mistake, and its own summary says the unit tests pass whether or not the filter is called. It only pointed at one of the two grab paths. The new test asserts the invariant instead of one instance of it: a production file that starts a download has to consult the blocklist somewhere in the same file. Reverting the wiring above fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Implements the data model and search-path exclusion from #838.
I held this as a branch rather than a PR while I asked lisim whether they wanted to build it themselves, since the request was theirs. They have said they would rather not, so here it is.
Not the management view from the issue's third bullet. That is a separate surface and this is already the larger half.
What it does
A
BlockedReleasetable scoped to the audiobook, an entry written when a download fails, and a filter that drops already-blocked candidates before one is selected. When everything is excluded the search ends at the existing "no acceptable search results" answer, which is a terminal state the pipeline already has, so this adds no new error case.That is the repro in the original report: a title whose indexer has one dead listing correctly reports no results on the next search instead of grabbing the same corpse again.
The identity question, which is the part worth review
Not the download client's id. A qBittorrent info-hash happens to identify the release, but a SABnzbd
nzo_idis allocated per submission, so a blocklist keyed on it would never match the same release again and would be silently inert on Usenet, which is the protocol #838 was reported against.ReleaseIdentityprefers a torrent info-hash, which identifies one release across indexers, then falls back to the release title and size together, and only uses the URL when there is no usable title.lisim was right and I was not. They suggested hashing indexer plus title plus size. I originally went with the release URL because
Downloadalready carriesOriginalUrland needed no new plumbing, and that was wrong for the protocol this feature exists for. A Usenet release URL is a per-fetch download link: the indexer mints a new token every grab, so the identity written when a download failed was never the identity computed at the next search. The blocklist wrote a row on every failure and matched none of them.That is the same failure this class's own summary warns about for client ids, and the URL fallback had the property the warning describes. Title and size are what an indexer reports identically for the same post across grabs, which is what makes them usable. Size is in the key so a shared title cannot collapse two different releases onto one entry, and there is a test for that.
Why a 409 cannot reach the trigger
Entries are written in
DownloadMonitorService.OnDownloadFailed, which only ever sees downloads the client accepted and then failed. A release the client refused at submission never gets there.That is deliberate. qBittorrent answers 409 when it already holds the release, which happens when one release satisfies several books (#881). Blocking on that would ban a release the user is currently downloading. Putting the write where it is makes the carve-out structural rather than a condition someone has to remember later.
Sonarr draws the same line:
DownloadClientRejectedReleaseExceptionreachesProcessDecisionInternaland returnsSkipped, and theSkippedarm records nothing at all.Scope, and what it does not fix
This stops a failed release being re-grabbed. It does not stop the #881 loop, where the same release is grabbed for a second book that it also satisfies. That release has not failed, so nothing here excludes it, and it should not: it is a release the user is actively downloading. That case needs the data model question on #881 answered and is deliberately not addressed here.
Conflicts to expect
This edits the pinned migration list in
SqliteMigrationSchemaTestsand the model snapshot, so it collides with any other PR adding a migration, including your #841 against our #763, and with the cover-art branch. Whichever lands second wants a rebase; the resolution is keeping both ids in migration-timestamp order.Two defects found by running this on a real library, not by the suite
Both were caught after the branch was already open, by an operator running it against a real
install. Neither was visible to the tests.
The identity, above. Fixed by keying on title and size.
The filter was wired into one of two grab paths.
SearchAndDownloadAsynccovers the manualsearch endpoint and the retry that follows a failure.
AutomaticSearchServiceis a different path:it scores results itself and calls
StartDownloadAsyncdirectly, and it had no blocklist referenceat all. So a release could fail, be blocked, and be grabbed again by the next automatic pass a
minute later.
The consequence was larger than a book being re-downloaded. On the install where this was seen, for
hours, every grab in the whole application was the same dead release repeating every minute or two.
Nothing else could get a grab in at all. The loop starved acquisition entirely.
Both paths consult the blocklist now, and there is a test asserting the invariant rather than one
instance of it: a production file that starts a download has to consult the blocklist in the same
file. Removing either call site fails it.
Tests, including two that were worthless first
Full suite 3132 passed, 0 failed, 130 skipped.
The
BlockedReleaseFilterunit tests pass whether or notDownloadServicecalls the filter, so they prove the helper and nothing about the wiring.BlocklistSearchWiringTestscovers that, and it asserts its own precondition first: an earlier draft passed with the call removed, because the search was failing earlier on "Audiobook has no quality profile assigned" and never reached the filter at all. It now fails when the call is removed, which I checked.Worked through with Claude Code at my direction. The suite numbers and the control are runs on this machine against this branch. I reviewed this before posting.
One expected conflict
This conflicts with my own #914 in
SqliteMigrationSchemaTests, and only there. Both add a migration, and that test pins an exact ordered list of ids, so any two migration-adding branches collide in it by construction. Whichever of the two lands first, I will rebase the other. Nothing else in the two overlaps.Worked through with Claude Code at my direction. The suite numbers and the controls are runs on this branch. The two defects above were found by running the branch on a real library rather than by reading it, and the behaviour described after each fix was observed there. I reviewed this before posting.