Repository navigation
fix(downloader): fail a magnet the client never resolves metadata for (#2709) - #2715
Conversation
…#2709) A magnet nobody serves is accepted by every torrent client and then sits there for ever. None of the native stall signals fire: Transmission leaves errorString empty, qBittorrent parks it in metaDL rather than stalledDL, Deluge calls it Downloading, rTorrent keeps it as a .meta placeholder with no message. failDownloadThatNeverArrived does not catch it either, because the torrent is present, just empty. The reporter's had been at "downloading" for 34 days. GetStalledIDs becomes GetStalledTorrents and returns why, not just which, so the queue row and the log can say what happened. Alongside each client's own signal it now reports a torrent the client is holding with no metadata: no total size, no progress, not complete. That shape is shared by all four torrent clients, so all four are covered. Age is the whole safety argument. A healthy magnet looks identical while it resolves, so the rule is only ever applied to downloads whose grabbed_at is older than the stall timeout (stall.timeout_minutes, default 120), which checkStalledDownloads already enforces before it polls. These go through handleStalledDownload unchanged: removed from the client, failed, blocklisted, re-searched. Blocklisting is right here, unlike in failDownloadThatNeverArrived, because a magnet with no metadata is a property of the release rather than of Bindery's wiring. Reported and diagnosed by ccarpinteri. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee <vavallee@protonmail.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…he batch Review finding: "no metadata" is a property of the network at least as much as of the release, and the blocklist has no expiry and is hand-cleared. A firewall change, a VPN drop, blocked DHT or UDP, a lost port forward: every in-flight magnet reports it at once, and two hours later the previous commit would fail them all, blocklist them all permanently, grab replacements that cannot resolve either, and repeat about a dozen times a day per wanted book. The existing per torrent stall signals never had that shape. Two changes, both needed: - StallKind.Blocklists. Only the client's own per torrent signal blocklists, exactly as before #2709. A no-metadata stall fails the download, removes it from the client and re-searches, but leaves the release grabbable, so a later search recovers it once the network is back. - StallReport.LooksLikeClientOutage. More than half of a client's unfinished torrents with no metadata, with a floor of three, is the client's fault: log once per client per run at Warn and leave that batch alone. GetStalledTorrents now returns a StallReport instead of one map of both kinds. The two halves have different contracts, and one map with only a comment between them was an invitation to a future caller. The unsafe half is a separate field that says in its doc comment which guards it needs. StallNone is rejected by the handler and its Reason is an obvious bug string, so a caller that forgets to set the kind cannot fail a download with a plausible message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 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>
There was a problem hiding this comment.
Clean fix overall. A few observations:
qBittorrent/Deluge: Incomplete can be double-counted
adapter.go: the qBittorrent and Deluge loops in GetStalledTorrents increment report.Incomplete for every torrent returned per category, with no deduplication across categories. When Category="" (empty = all torrents) and CategoryAudiobook is also set, CategoriesToPoll returns both, and the second poll re-visits the same audiobook torrents — Incomplete counts them twice while the maps deduplicate by hash. The Transmission case correctly guards this with seen[t.ID].
The consequence is that LooksLikeClientOutage sees an inflated denominator and becomes harder to fire — a false-negative for outage suppression, not a false-positive, so individual downloads would be failed when they should have been held during a real client outage. The fix would be to mirror the Transmission seen pattern (keyed by hash) for qBittorrent and Deluge, or to call CategoriesToPoll once, collect all hashes, and iterate the merged set.
Not blocking — the case only arises when Category is intentionally left empty alongside a non-empty CategoryAudiobook, and missing the outage guard in that state fails downloads without blocklisting them, so they can be recovered.
Everything else looks solid
- The mutual exclusivity of
ClientReported/NoMetadataviaswitchis correct — a torrent with botherrorStringandtotalSize=0lands inClientReportedonly. - The
StallKindzero-value trap (bug-string reason, no blocklist, rejected by the handler) is a good defensive pattern. - The
stalledReleaseSpecexclusion being context-scoped to onesearchAndGrabFormatcall and not stored is the right scope — ordinary sweeps can pick the release again after the cooldown clears. float64(n) > float64(r.Incomplete) * noMetadataOutageShareusing strict greater-than is consistent with the table test (exactly 50% is not an outage).- Test coverage is thorough: per-client unit tests, the outage threshold table, the zero kind, age gate, healthy torrent, and the re-search runner-up path.
— 🤖 Bindery triage bot (automated). Reply to correct me; a human will see it.
Closes #2709
What
A magnet the client accepts and never resolves the metadata for now counts as stalled once it is older than the stall timeout. Bindery removes the empty entry from the client, fails the queue row with a reason that says why, and re-searches, skipping that one release so the search can pick another. It is deliberately not blocklisted, and a client where most of the unfinished torrents look that way is left alone entirely.
GetStalledIDsbecomesGetStalledTorrentsand returns aStallReportwith the two kinds in separate fields (ClientReported,NoMetadata) plus theIncompletedenominator. The rule, the report and the policy (Reason,Blocklists,DeletesData) live ininternal/downloader/nometadata.go.Why the reporter's torrent fell through both nets
GetStalledIDsqualified a Transmission torrent onStatus == 0 && ErrorString != "", and Transmission does not consider "no peers, no metadata" an error, so the string is empty.failDownloadThatNeverArrivedonly fires when the torrent is absent from the client; this one is present, just empty.Updated for current main (merge of origin/main, no rebase)
Main moved a lot under this branch. Conflicts were resolved keeping both sides, then the logic was re-checked:
hashString, never the session id. That retires the old "known limitation" about renumbered ids entirely: a magnet carries its hash from the moment it is added, so a metadata-less one still has one, andRemoveTransmissionTorrentrefuses a numeric legacy id rather than remove the wrong torrent. Thetorrent-getfield list carries main'saddedDatenext tometadataPercentCompleteandpeersConnected.Category, so an audiobook magnet (or an audiobook torrent with a realerrorString) was invisible to it. It now polls every categoryCategoriesToPollreturns, counting a torrent both filters return once so the outage guard's denominator is not doubled.delete-local-data. It has no files, so asking for a data delete has no upside and could only ever reach something that is not this torrent's (on rTorrent that is Bindery's ownRemoveAll). It is now removed entry only, viaStallKind.DeletesData(), the same way remove on import takes the entry and leaves the disk alone. Client reported stalls still delete their partial data, unchanged. Nothing Bindery imported can reach this path: it only considers rows indownloading.stalledReleaseSpecrejects it for that one search only. Nothing is stored, so once the cooldown has passed an ordinary sweep may pick the release again, which is the point of not blocklisting it.torrent-setafter add and do not interact with this.Client coverage
totalSize0,percentDone0,metadataPercentComplete< 1,errorStringempty; reporter saw status 0metaDLorforcedMetaDL, size 0, progress 0stalledDLDownloading,total_size0, progress 0Error.metaplaceholder,d.size_bytes0, not completed.messagerTorrent sometimes does not list the
.metaplaceholder at all; thenfailDownloadThatNeverArrivedfires instead, which is the right outcome by another route.Peers are fetched and logged but never decided on: a magnet with peers that none of them will serve metadata for is just as dead.
Timeout
There is no separate timeout. The existing stall timeout applies:
stall.timeout_minutes(default 120), andcheck-stalledruns every 5 minutes.checkStalledDownloadsdrops every download whosegrabbed_atis newer thannow - timeoutbefore polling, so nothing is judged before it has had the whole window to resolve. You cannot tell a dead magnet from a slow one; the argument is that two hours without a single peer serving metadata is not a slow magnet, and anyone on a thin swarm can raise the setting.Review findings carried from round 2
LooksLikeClientOutage(): more than half of the client's unfinished torrents with no metadata, with a floor of three, means one Warn line per client per run and the batch is skipped. Client reported stalls are untouched by the guard.StallKindis unusable: bug-shaped reason, no blocklist, no data delete, andhandleStalledDownloadrejects it.Transmission session ids: resolved by fix(transmission): identify downloads by info hash, not the session torrent id #2711, see above.grabbed_atand would be failed on the next tick. Narrow, and now costs one failed row with nothing blocklisted; not guarded.What the user sees
stalled: the download client never resolved this magnet's metadata, so it has no files and no size.stall detectedwithkind=no_metadataandblocklisted=false, an Info line saying the blocklist was skipped, and on a client-wide fault one Warn per client per run.downloadStalled, thendownloadRequeuedwhen the re-search fires.Fail before evidence
This round (merge plus the three fixes)
Captured by neutralising each change in place (stall detection polling
[]string{client.Category}only,DeletesData()returning true, the stalled release spec disabled), then restoring.Keying the no-metadata half by session id (this branch's pre-merge behaviour) against main's hash-keyed downloads:
Earlier rounds
Round 1 (detection) and round 2 (no blocklist, outage guard, zero value) fail before evidence is unchanged from the previous revision of this description: every no-metadata predicate neutralised fails
TestGetStalledTorrents_{Transmission_NoMetadata,QBittorrent_MetaDL,Deluge_NoMetadata,Rtorrent_NoMetadata}and the scheduler end to end test; restoring the round 1 policy failsTestStallReport_LooksLikeClientOutage,TestStallKind_ZeroValueIsUnusable,TestCheckStalledDownloads_NoMetadataClientOutageandTestHandleStalledDownload_RejectsStallNone.NoMetadataYoungerThanTimeout,HealthyTransmissionDownloadUntouchedandNoMetadataBelowOutageFloorpass before as well, by design: they are over-reach guards.Tests
internal/downloader/nometadata_test.go: the reporter's Transmission shape at status 0 and 4, keyed by hash with an upper case wire hash to prove normalisation; healthy and metadata-but-no-progress torrents untouched; a realerrorStringlands inClientReportedonly;torrent-getrequests the fields; Transmission audiobook category polled with a shared torrent counted once and a foreign one ignored; qBittorrentmetaDL/forcedMetaDL; Deluge; rTorrent; the outage threshold table; the zeroStallKind.internal/scheduler/scheduler_nometadata_test.go: end to end Transmission no-metadata stall (failed, removed by hash, no data delete, no blocklist, history event); client outage; below the floor; younger than the timeout; healthy torrent;StallNonerejected; the stall re-search grabbing the runner up instead of nothing.TestCheckStalledDownloads_QBitStalledTorrentstill asserts the blocklist entry for the client reported kind.Verification
go build ./...,go vet ./...,gofmt -lclean,GOOS=windows go build ./...passesgo test ./internal/downloader/... ./internal/importer/... ./internal/db/... ./internal/scheduler/... -count=1passNo new Go module or npm package. No frontend change. No migration.
Security
No new endpoint, no auth or settings surface, no user scoped query. The only destructive action is the existing
removeStalledFromClientpath, now strictly narrower: a no-metadata stall never asks the client to delete data.Thanks ccarpinteri for the report and the diagnosis.
🤖 Generated with Claude Code
https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9