feat(downloader): remove the torrent after a successful import (#2046) - #2713
Open
ccarpinteri wants to merge 3 commits into
Open
ccarpinteri wants to merge 3 commits into
ccarpinteri wants to merge 3 commits into
Conversation
github-actions
Bot
added
the
bindery-notified
…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! |
ccarpinteri
force-pushed
the
pr/remove-on-import
branch
from
September 19, 2026 03:09
eb4c38c to
9a485e4
Compare
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
1 participant
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.
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