Repository navigation
feat(downloader): remove the torrent after a successful import (#2046) - #2713
Conversation
…orrent id torrent-add returns a numeric id that only lasts as long as the daemon session. Transmission renumbers every torrent when it restarts, but Bindery stored that id as the download's TorrentID and the poller looked downloads up by it. After a restart the id matches nothing, so the poller skips the row and the download sits at "downloading" for good while the torrent carries on seeding. Nothing imports it. Ids are also reused, so a stored id can come back pointing at a different torrent, and then the wrong payload is imported and the wrong torrent is acted on. This stores hashString instead, which stays the same for the life of the torrent, and looks downloads up by it. The RPC takes a hash anywhere it takes an id, so removal works off the same value. Stall detection and the queue's live progress overlay are keyed by hash too, since both compare against the stored value. Stall detection gets no numeric fallback because the caller removes what it matches; the overlay keeps one because it only draws a row. Rows written before this change still hold a numeric id. They are matched by comparing Transmission's addedDate with the download's grab time, which a restart does not change, and only when the pairing is clear: a torrent no download is close to is left alone, a torrent one download is close to is assigned, and a torrent several downloads are close to needs the release name to pick one or it is left for the user. The row is then rewritten to the hash, the same way the qBittorrent hash recovery in vavallee#939 works. The stale id is never used to find the torrent. Leaving a download stuck can be undone, removing the wrong torrent cannot. Terminal downloads are skipped: they will not be imported or removed again, so rewriting their identifier can only be wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
checkTransmissionDownloads only fetched torrents under client.Category. When CategoryAudiobook is set, audiobook grabs go to that category instead, so the poller never saw them and those downloads stayed at "downloading" for good. It now polls every category CategoriesToPoll returns, the same set the other clients already poll, and keeps a torrent once if both categories return it. The zero-match warning names the audiobook category too, so a mismatch there is as visible as one on the ebook category. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lee#2046) Adds a per client "remove on import" toggle, off by default. When it is on, Bindery removes the torrent from the client once it has imported the download. The files are left on disk, so a hardlinked import keeps working and nothing the user already has is deleted. Off by default because private tracker users need it off: removing a torrent stops it seeding and costs them ratio. The callback runs on every route that ends in a finished import, not just the obvious one. tryImportInternal already ran it on three of them; the "no book files at the path but the book is already in the library" return was not one, and the two "book already in library" shortcuts in checkQbittorrentDownloads close a download out without calling tryImportInternal at all. Those two now build the same callback through a shared helper. Usenet clients are untouched. They already clear their own history entry on import, so there is nothing for the toggle to control, and the settings form hides it for them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
eb4c38c to
9a485e4
Compare
|
Thanks for this, it covers exactly the gap I hit. One request: could the toggle offer a "remove torrent and data" option alongside "remove torrent only"? |
|
Heads up on a migration number collision, not a review. This PR adds a migration numbered 090, and so do two others that are open right now: #2761 and #2713 and #2750 all claim 090. Main tops out at 089. Current claim map across the open PRs:
The usual gotcha here is that each of us computes "next free" against a main that does not yet contain anyone else's unpushed work, so this keeps happening. Worth renaming to something above 093 and picking by checking open PRs rather than main. No rush on my side, just flagging it before two of these land in the same window. |
…avallee#2718) Renames 090_requests_auto_approve.sql to 093. Main tops out at 089 and both vavallee#2750 and vavallee#2713 claim 090, so whichever of the three landed second would trip assertUniqueMigrationVersions at boot. Auto-approval removed the pending cap, because that cap counts pending rows and an auto-approved request never sits in pending. The claim now also carries a per-account daily quota, drawn from the same requests.max_pending_per_user limit (25 by default). It is enforced inside the claim statement, the way pendingBelowCap guards Create, so a burst of creates cannot all pass a separate count. Once the account has had that many requests auto-approved since midnight UTC, the next one is left pending for a human, exactly as it is with the setting off. An auto-approved request no longer sends requestCreated, so an admin is not pinged for an item that was added without them. A request that stays pending, whether the add failed or the day's quota is spent, still notifies. The admin user routes move into registerUserAdminRoutes so the route level test drives the same registration the server uses. Moving /auth/users/{id}/auto-approve out of the RequireAdmin group failed nothing before this test; an unknown user id on that route is now a 404 rather than a silent success, and the Users page renders the autoApproveHint key that was already in en.json. Signed-off-by: Tung Lam <lamphamabtung96@gmail.com>
vavallee
left a comment
There was a problem hiding this comment.
Off by default, per client, `deleteFiles` false so a hardlinked import survives, and you found the two qBittorrent "already in library" shortcuts that never reached `tryImportInternal`. That last one is the kind of thing that only turns up by actually walking every success route, so thanks for doing it. Refusing to land before #2711 is the right call too: without stable ids this would remove somebody else's torrent.
090 is fine: main has nothing there, and the other PR claiming it (#2750) has been asked to move to 095.
Two small things before merge: `misspell` fails on "hardlinked" in a comment at `scanner_poll.go:1023` (writing "hard linked" clears it), and the same DCO sign-off as the other two. Passes merged onto main otherwise.
golangci-lint's misspell check fails the lint job on "hardlinked". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee <vavallee@protonmail.com>
|
Pushed a one word comment fix so the lint job passes: misspell was flagging "hardlinked" in scanner_poll.go, now "hard linked". That was the only one the PR added. Thanks ccarpinteri! |
* feat(api): auto-approve requests per requester account (#2718) A requester can only ask, and every ask waits for an admin. With the lean requester UI that turns into a stream of approvals, which is the opposite of what the role is for. Add a per-account setting, off by default so no existing install changes behaviour. It lives on the user row (migration 090) rather than in the settings table because it is a per-account permission like role, and it is set from the Users page or PUT /auth/users/{id}/auto-approve. When it is on, Create claims the new request and runs the same add an admin's Approve runs, with no deciding user recorded. The already-in-the-library check, the stored-payload revalidation and the pending cap all still apply. A book request searches on add; an author request runs the ordinary catalogue sync. If the add fails the request stays pending, so nothing is lost. The switch only affects the next request; anything already queued stays for a human. Signed-off-by: Tung Lam <lamphamabtung96@gmail.com> * fix(api): rename migration 090 to 093 and cap auto-approve per day (#2718) Renames 090_requests_auto_approve.sql to 093. Main tops out at 089 and both #2750 and #2713 claim 090, so whichever of the three landed second would trip assertUniqueMigrationVersions at boot. Auto-approval removed the pending cap, because that cap counts pending rows and an auto-approved request never sits in pending. The claim now also carries a per-account daily quota, drawn from the same requests.max_pending_per_user limit (25 by default). It is enforced inside the claim statement, the way pendingBelowCap guards Create, so a burst of creates cannot all pass a separate count. Once the account has had that many requests auto-approved since midnight UTC, the next one is left pending for a human, exactly as it is with the setting off. An auto-approved request no longer sends requestCreated, so an admin is not pinged for an item that was added without them. A request that stays pending, whether the add failed or the day's quota is spent, still notifies. The admin user routes move into registerUserAdminRoutes so the route level test drives the same registration the server uses. Moving /auth/users/{id}/auto-approve out of the RequireAdmin group failed nothing before this test; an unknown user id on that route is now a 404 rather than a silent success, and the Users page renders the autoApproveHint key that was already in en.json. Signed-off-by: Tung Lam <lamphamabtung96@gmail.com> --------- Signed-off-by: Tung Lam <lamphamabtung96@gmail.com> Co-authored-by: Tung Lam <lamphamabtung96@gmail.com>
main now carries vavallee#2711 and vavallee#2712, which this branch stacked on, so this keeps main for those and applies only the remove on import change. It also limits the settings toggle to qBittorrent and Transmission, the two clients whose import path removes the torrent, and adds the changelog fragment. Signed-off-by: vavallee <vavallee@protonmail.com>
Main moved under this branch: Transmission downloads are keyed by info hash (#2711), the importer polls the Transmission audiobook category (#2712), remove on import exists (#2713) and per indexer seed limits apply after add (#2917). Conflict resolution keeps both sides. GetStalledTorrents now keys both Transmission halves of the StallReport by lower-cased info hash, never the session id, and the torrent-get field list carries main's addedDate alongside metadataPercentComplete and peersConnected. Re-checking the logic against current main turned up three things, fixed here: - Stall detection still polled only the Transmission ebook category, so an audiobook magnet was invisible to it. It now polls every category CategoriesToPoll returns, counting a torrent both filters return once so the outage guard's denominator is not doubled. - A no-metadata torrent was removed with delete-local-data. It has no files, so it is now removed entry only (StallKind.DeletesData), the same as remove on import. Client reported stalls still delete data. - The re-search after a no-metadata stall did nothing when the dead release still ranked first: it was approved again and the #2710 cooldown refused the grab instead of trying the runner up. The re-search now carries the stalled GUID and a decision spec skips it for that one search only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee <vavallee@protonmail.com>
…on (#2046) (#2943) The removal code shipped in #2713 was never executed by a test. These drive it against fake qBittorrent and Transmission daemons and pin the contract: one removal per import, keyed on the info hash, never asking the client to delete the payload, sent only after the row is imported, and skipped when the toggle is off, the import failed, or the row still holds a legacy Transmission number. A refused removal is logged and leaves the import alone. Both qBittorrent "already in library" shortcuts are covered as well as the normal import. The db tests round trip remove_on_import through Create, Update and every getter, rebuild the pre 090 schema to show an existing client upgrades with the toggle off, and check an unreadable value is an error. No behaviour change and no bug found. Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee <vavallee@protonmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Closes #2046.
What it does
Adds a per client "remove on import" toggle, off by default. When it is on, Bindery removes the torrent from the client once it has imported the download.
deleteFilesis false, so the files stay on disk and a hardlinked import keeps working.Off by default for the reason you raised on the issue: private tracker users need it off, because removing a torrent stops it seeding and costs them ratio. The help text under the toggle says so.
Wiring it into every success route
You said the import path has more than one success route and the callback needs to be at each of them, so I went through them all.
tryImportInternalalready rancleanupFuncon three routes. The fourth, the "no book files at the path but the book is already in the library" return, did not, so it does now.The two "book already in library" shortcuts in
checkQbittorrentDownloadswere the real gap. Both close a download out and mark it imported without ever callingtryImportInternal, so no cleanup could have run there at all. They now build the same callback through a shared helper,qbittorrentRemoveOnImportCleanup.Transmission's poller has no equivalent shortcut. Everything goes through
tryImportInternal, so it was already covered.The manual import API path builds a synthetic Download with no
TorrentIDand no client, so there is nothing to remove and it is left alone.Usenet
Untouched.
tryImportNZBGetalready callsRemoveHistoryon import unconditionally, so there is nothing for the toggle to control. The settings form hides it for usenet clients, and the edit form sends it as false if a client is switched from a torrent type to a usenet one, so a stale flag cannot be left on the row.UI
Checkbox in Settings, Download Clients, under the path remap field, for torrent clients only. Screenshot of it on my own install with Transmission selected: the toggle, the label "Remove torrent after import", and the help text explaining the ratio tradeoff.
Tests
ClientsTab.test.tsx: hidden for usenet, reflects the stored value, defaults off and sends the change on save, never sent enabled for a usenet clientSettingsPagepayload assertions updated, since torrent clients now carry the extra keyimporteranddbsuites pass, including the existing cleanup callback testsFull web suite: 92 files, 1058 tests, passing. It needs Node 21 or newer to run, since jsdom's undici calls
webidl.util.markAsUncloneable; on Node 20 the suite cannot start. I ran it on the same Node the Dockerfile uses.Note on ordering
Stacks on #2711 and #2712. #2711 matters for this one: it makes a Transmission download's stored identifier stable. Without it, a stored id can come back pointing at a different torrent after a daemon restart, and this feature would then remove a torrent the user never grabbed. I would rather this did not land first.
Migration is numbered 090 against current main.
🤖 Generated with Claude Code