Repository navigation
fix(transmission): identify downloads by info hash, not the session torrent id - #2711
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…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>
0478af4 to
0893012
Compare
vavallee
left a comment
There was a problem hiding this comment.
This is a really good fix, and thanks for writing up the first pass that went wrong. The batch collapsing onto one torrent is exactly the trap I'd have wanted a test for, and you have one.
I checked the premise on main: `checkTransmissionDownloads` keys its lookup on `fmt.Sprintf("%d", t.ID)`, the session id, so a restart strands every row just as you describe. Merged onto current main with #2712 and #2713 stacked on top, all three build, vet, and `importer`, `downloader`, `db`, `scheduler` and `api` pass. I also added a numeric id fallback to the lookup as a mutation, which is the one thing this must never do, and `TestCheckTransmissionDownloads_IgnoresRecycledTorrentID` caught it.
Only blocker is the DCO check: the commits have no `Signed-off-by` at all. From each branch, `git rebase --exec 'git commit --amend --no-edit -s' origin/main` then force push. Since they stack, doing it on #2713's branch and pushing each branch from there is simplest.
…both sides The legacy reconciler only checked ambiguity from the torrent's side. A single legacy download with its own torrent and an unrelated one added in the same five minute window (another tool sharing the daemon, or no category filter) gave each torrent exactly one candidate, so the row was rewritten to whichever torrent the daemon listed first. That imports the wrong payload and hands the wrong hash to every later removal, which is the failure vavallee#2808 is about. A pairing now has to be unambiguous from both sides: the download and the torrent are each other's only candidate in the window, or the release name picks out exactly one partner on each side. Anything else is left alone. The result no longer depends on listing order. Also turn the SetGrabbedAt Skipf calls in the new tests into Fatalf, so a fixture failure cannot pass silently. 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>
…ion id RemoveTransmissionTorrent still removed rows holding a numeric id by that id. Those are exactly the rows the poller could not pair with a torrent, and after a daemon restart the number may name a torrent Bindery never grabbed. Queue removal with deleteFiles would then delete somebody else's data, the destructive half of vavallee#2808. A numeric id is now refused with an error and no RPC is made. Both callers already log and carry on (queue removal still drops the row, stall removal still blocklists and re-searches), so the only cost is that an unreconciled legacy torrent has to be removed in Transmission by hand. Tests: removal by hash sends the lower-cased hash as a JSON string, a session id is refused with or without deleteFiles and never reaches the daemon, and SendDownload returns the info hash for both torrent-added and torrent-duplicate. The old InvalidID test only passed because the client had no host; it is replaced by the refusal test. 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>
Refs vavallee#2808 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>
…very Disabling the skip left every test green: in the batch test the release name already settles the torrent before the imported row matters. The new test has an imported and a live legacy row a minute apart with neither title matching the torrent name, so counting the imported row makes the window ambiguous and strands the live download. Also print the torrent id rather than its pointer in the renumbered id test's failure message. 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>
|
Merged, thanks! I pushed a few commits on top before merging: the recovery pairing for old numeric rows now has to be unambiguous from both sides (an unrelated torrent added in the same window could get picked), and removal refuses a stale numeric id instead of deleting whatever holds that number now. Also added the changelog fragment. For next time, please sign off your commits with |
main already carries the info hash work from vavallee#2711, so this keeps main for that and applies only this PR's poll change, plus a regression test and changelog fragment. Signed-off-by: vavallee <vavallee@protonmail.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>
Problem
torrent-addreturns a numeric id that is scoped to the daemon session. Transmission renumbers every torrent when it restarts. Bindery stores that id as the download'sTorrentIDand the poller looks downloads up by it, so every restart breaks the link.After a restart the stored id matches nothing.
checkTransmissionDownloadsskips the row, the download sits at "downloading" for good, and the torrent keeps seeding. Nothing ever imports it.Ids are also reused. A stored id can come back pointing at a different torrent, and then Bindery acts on the wrong one: the wrong payload is imported, and anything that removes by id removes a torrent the user never grabbed.
I hit this on my own install. Two downloads had been stranded for weeks:
Both torrents were healthy and sitting in the client the whole time. One of them was complete and ready to import.
qBittorrent does not have this problem because it stores the hash. Transmission is the only torrent client keyed on a value that changes.
Change
Store
hashString, which stays the same for the life of the torrent, and look downloads up by it. The RPC accepts a hash anywhere it accepts an id, so removal works off the same value with no extra lookup.Two other places compared the stored value against a map keyed by numeric id, and both are keyed by hash now:
GetStalledIDs, with no numeric fallback. The caller removes what this matches, so a stale id here would remove the wrong torrent.GetLiveStatuses, which keeps the numeric id as an extra key. That overlay only draws a queue row, so the worst a stale id can do is mislabel one.Rows written before this change
Those still hold a numeric id. They are matched by comparing Transmission's
addedDateagainst the download's grab time, which a restart does not change, and only when the pairing is clear:The row is then rewritten to the hash. This is the same recovery shape as the qBittorrent hash backfill in #939.
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 for the same reason: they will not be imported or removed again, so rewriting their identifier can only be wrong.
I got this wrong on the first pass in a way worth mentioning. An earlier version accepted any single torrent inside the time window without the name check, and on a live database seven already imported rows from one afternoon's grabs all matched the one torrent from that window that still existed. The mutual uniqueness rule and the terminal skip are there because of it, and
TestCheckTransmissionDownloads_LegacyBatchDoesNotCollapseOntoOneTorrentcovers exactly that case.Tests
New in
internal/importer/scanner_transmission_hash_test.go:+,_and.where a tracker put them instead of spacesExisting fixtures that asserted id based identity now assert hash based identity.
AddTorrentkeeps its signature and delegates to a newAddTorrentDetailedthat returns the whole torrent, so nothing else that calls it had to change.Full
importer,downloader,db,schedulerandapisuites pass. The two failures I see locally,TestFindCaseInsensitivePathUnderandTestDiagnose_CaseDivergenceWarns, also fail on a clean checkout of main on macOS.Running on my own install since this morning, with both stranded downloads recovered on the first poll.
Note on ordering
This is the first of three. The other two stack on it: polling the audiobook category, and the remove on import feature from #2046. That one needs this fix to be safe, since it removes the torrent it matched.
Maintainer follow ups
Pushed on top of the original commit, which is unchanged:
TestCheckTransmissionDownloads_LegacyRowWithTwoNearbyTorrentsIsLeftAlonefailed on the previous head with the row rewritten to the unrelated hash.RemoveTransmissionTorrentno longer removes by a stored numeric id. Those rows are exactly the ones the poller could not pair, so the number may name someone else's torrent. It now returns an error without calling the daemon; queue removal still drops the row and stall handling still blocklists and re-searches.changelog.dfragment.Closes #2808
🤖 Generated with Claude Code