Repository navigation
Feature/download client media eligibility - #2633
niddelicious wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
ea77ce7 to
6fae8c9
Compare
vavallee
left a comment
There was a problem hiding this comment.
Thanks for this, it's a nice feature and the routing itself is solid: both send sites filter, polling and removal still go by client id, the migration is safe, and the tests pass. A few things turned up in review that hit exactly the setups this is for, so I'd like to get them sorted before merging.
Needs fixing
- Readarr import creates clients that are eligible for nothing.
internal/migrate/readarr.goaround line 268 builds amodels.DownloadClientwithout the two new flags, so they go in as false and every grab afterwards fails with "no enabled download client is eligible". Setting both to true there (or defaulting inrepo.Create) fixes it. - Approving a pending release sends audiobooks to the ebook client.
internal/api/pending.goaround line 180 uses the book's media type, which isbothfor a dual format book, andFilterEligibleForMediaTypetreats anything that isn'taudiobookas books. The pending release row has its ownMediaType; using that routes it correctly. That bug predates your PR, but the eligibility filter is what turns it into a misroute. SendWithFallbackcan send the same release to two clients. It moves on after any error, but several errors happen after the first client already accepted the download: Transmission returning id 0, qBittorrent with no hash, SABnzbd with no NZO id, or a timeout reading the qBittorrent response. The torrent then lands in both clients and the first copy is never tracked. I'd either drop the fallback (use the top ranked eligible client and report its error) or only fall through on errors that prove nothing was sent, like a connection refused before the request.
Worth doing here or as a follow up
- Auto-grab doesn't look at the other protocol.
scheduler.goaround line 1031 picks the best release across both protocols first, then gives up if that protocol has no eligible client. With SABnzbd set to ebooks only and qBittorrent to audiobooks only, a usenet audiobook that ranks best means the sweep logs "no eligible download client" and ignores the approved torrent, every sweep. Filtering candidates to protocols that have an eligible client before choosing would fix it. The manual grab error also tells you to "enable a download client for audiobooks" when one already exists for the other protocol. - Grabs from the free-text Search page always count as books. Results there have no media type, so an m4b torrent found on /search goes to the ebook client. Book detail and Wanted are fine because they fall back to the book's type.
- Priority now sorts ahead of the category hint in
RankClientsForMediaType. That's reasonable, but it can change which client an existing install uses, so a line in the changelog fragment would help.
Housekeeping
- The red CodeQL check isn't you. Those three alerts are already open on main at the same lines; CodeQL blames this PR because the client URL input moved in
download_clients.go. 086collides with other open PRs (#2607, #2626, #2628). Whichever lands later renumbers, so no action yet.- This will go into v1.37 rather than a patch release.
Happy to help with any of these if you get stuck.
|
Heads up on a migration number collision, not a review. This PR adds a migration numbered 086, and so does #2673 or #2633, whichever this is not. Both are open and both claim it. 086 is a genuine gap on main, which goes 085 then 087, and it has never been filled by a merged commit. Three separate branches have each created their own 086 at some point, which is how the gap became attractive to all of them at once. Main tops out at 089. Current claim map across the open PRs:
Suggest renaming to 093 or above rather than reusing the gap, and picking the number by checking open PRs rather than main. The reason this keeps happening is that each branch computes "next free" against a main that excludes everyone else's in flight work. |
|
Check, I'll look into your comments. I also missed the workflow tests and just went with the app tests, so I'll see if I can get the workflows locally so we don't need to fail the actions EVERY time I think I've got it. |
…allee#2633) - Add independent EnabledForBooks and EnabledForAudiobooks flags to download clients (both default enabled). - Order download clients by Priority first during selection, using category hints as tie-breakers. - Route grabs (manual, auto-grab, pending release approval) to the highest-ranked eligible client for the target media type. - Avoid cross-client retries on ambiguous send failures to prevent duplicate downloads. - Infer media type from release format for free-text indexer searches. - Add migration 096 and changelog fragment. Signed-off-by: niddelicious <github@nidde.nu>
e3b4f1b to
34286f3
Compare
|
Hey @niddelicious, checking in on this one. The branch now conflicts with main, so it needs a merge of main (a plain merge is fine, no rebase needed). On the migration: 096 is free on main and nobody else has claimed it, so you can keep that number. I haven't re-read your 09-29 commit against the review yet. Once the conflict is sorted I will, so it would help if you could say which of the six points from the review it covers (the Readarr import flags, the pending release media type, the double send in the fallback, auto-grab across protocols, the Search page, and the changelog line). If you're short on time, no worries. If I don't hear back by 2026-10-31 I'll pick it up from your branch and keep you credited as the author of the feature. |
…ient-media-eligibility
…ee#2633) Honor media eligibility across manual, pending and scheduled grabs, avoid retrying ambiguous sends, and keep Readarr imports eligible by default. Renumber the migration to avoid active PR claims. Signed-off-by: Nidde Nedelius <github@nidde.nu>
|
Thank you for your patience @vavallee I do believe I've fixed all the issues you mentioned, or rather my buddy Claude did:
I had a look and it seemed like 116 would be the next migration, so I picked that. |
Summary
Added functionality to make download clients eligible/non-eligible for different media type (determined by index search), making any grab filter out any clients not enabled for the grabbed type.
Priority field for download clients is enabled in frontend and after filtering any eligible client is processed in priority order.
Origin for this feature was my personal need to pass books to one Transmission instance and audiobooks to a different Transmission instance.
Checklist
git commit -s— see Sign your workdocs/DEPLOYMENT.mdupdated if env vars, config, or upgrade path changedchangelog.d/(not an edit toCHANGELOG.md) — see changelog.d/README.mdTest plan
make check(or the individualgo test ./cmd/... ./internal/...andcd web && npm run buildsteps)