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>
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 #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: it is removed from the client, the queue row is failed with a reason that says why, and a fresh search runs. It is deliberately not blocklisted, and a client where most of the unfinished torrents look that way is left alone entirely. Both of those are review findings, written up under "Round 2" below.
GetStalledIDsbecomesGetStalledTorrentsand returns aStallReportrather than a map of ids. The rule, the report and the policy live ininternal/downloader/nometadata.go.Why the reporter's torrent fell through both nets
Confirmed on main.
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. Its doc comment did claim to cover "a magnet whose metadata never resolves", which only holds when the client dropped the add.Round 2: the review findings
1. BLOCKING, accepted in full. A no-metadata stall no longer blocklists, and a client-wide fault is caught as one
The finding is right and I had it wrong. I originally argued that a dead magnet is "a property of the release, not of Bindery's wiring", and blocklisted on it. That reasoning only holds for one torrent in isolation. "No metadata" is a property of the network at least as much as of the release: blocked DHT or UDP, a VPN drop, a lost port forward and a firewall change all produce exactly the same shape, for every in-flight magnet at once.
internal/db/blocklist.gohas no expiry andIsBlockedis global, so the entries are permanent and hand-cleared. Two hours into a network fault, the previous commit would have failed and permanently blocklisted every grabbed release, then grabbed replacements that could not resolve either.Both halves of the requested fix are in:
(a) Fail without blocklisting.
StallKind.Blocklists()returns true only forStallClientReported, which is the client asserting that a specific torrent has gone wrong. That path is byte for byte what it was before this PR, blocklist included. AStallNoMetadatastall removes the torrent, fails the row and re-searches, leaving the release grabbable so a later search recovers it once the network is back.handleStalledDownloadlogs the skipped blocklist at Info so it is not silent.On the retry rate: without a blocklist the re-search can pick the same release, and each fresh grab gets a new
grabbed_at, so a book can round-trip about every 2 hours rather than a dozen times a day. Nothing permanent accumulates now, and #2714's six hour cooldown on re-grabbing a dead row bounds it further. I kept the re-search rather than dropping it, because it is what the reporter asked for and because on a working network it genuinely does find a different release.(b) Batch guard.
StallReport.LooksLikeClientOutage(): more than half of the client's unfinished torrents reporting no metadata, with a floor of three, means blame the client.checkStalledDownloadsthen logs one Warn line per client per run naming the count and the denominator, and skips that batch. The client's own per torrent signals are untouched by the guard: those are still individually meaningful during an outage.Justifying the two numbers:
2. PLAUSIBLE misuse, accepted. The unsafe half is now a separate, named field
GetStalledTorrentsreturnsStallReport{ClientReported, NoMetadata, Incomplete, UsesTorrentID}instead of onemap[string]StallKind. I chose separating the kinds over passing grab times in, for two reasons: the age gate is not the only guard theNoMetadatahalf needs (the breadth check is the other, and it needs the denominator the report already carries), and the downloader has no business knowing when Bindery grabbed anything. A caller now has to reach for the field calledNoMetadata, whose doc comment opens with the two guards it owes, rather than iterate one map and get both.ClientReportedstays safe to act on the moment it appears.3. LOW, accepted. The zero value is unusable
StallNone.Reason()returnsBUG: the stall handler was called with no stall reason, please report this,StallNone.Blocklists()is false, andhandleStalledDownloadrejectsStallNonewith aslog.Errorand returns before touching the download. Covered byTestStallKind_ZeroValueIsUnusableandTestHandleStalledDownload_RejectsStallNone.4. LOW, confirmed, documented as a known limitation
I cannot key Transmission on the hash cheaply.
SendDownloadstoresstrconv.FormatInt(torrentID, 10)inDownload.TorrentIDfor Transmission, andRemoveDownloadparses it straight back withParseInt; the live-status poller and the importer key on the same value, and every existing row in every install holds a numeric id. Switching tohashString(which the add response does return) means a migration plus a dual-key read path across four call sites, which is its own PR and its own risk.So, stated plainly: Transmission remote ids are session-local integers and are reassigned after a daemon restart; qBittorrent, Deluge and rTorrent are all keyed on the info hash and are not affected. The review is right that this PR widens the window: previously a mismatched id had to be stopped with an error string to matter, now it only has to be a zero-size magnet, which is more common. What a mismatch costs is one wrong row failed and one wrong torrent removed from the client, both bounded by the client's configured download dir or label filter, and with no blocklist entry now. Worth fixing properly, separately.
5. LOW, mentioned rather than guarded
A download that was legitimately slow for weeks, removed and re-added by hand today, keeps its old
grabbed_atand the same hash, so the very next tick sees "grabbed weeks ago, no metadata" and fails it. Real, and narrow: it needs a hand re-add of a magnet that is still resolving at the moment the job runs.I chose not to guard it. The clean guard is the client's own added date (
addedDate,added_on,time_added,d.timestamp.started), which is four more fields across four clients and two of them not currently fetched, to cover a case whose entire cost is now one failed queue row that will be searched for again, with nothing blocklisted. A guard on Transmission alone would be worse than none, because it would read as covered.The four points from the original brief
Age, and what the minimum safe age is
stallTimeoutDefaultininternal/scheduler/scheduler.gois 120 minutes, overridden bystall.timeout_minuteswhen it parses to a positive integer.check-stalledruns every 5 minutes.The age gate already existed:
checkStalledDownloadsdrops every download whosegrabbed_atis newer thannow - timeoutbefore it groups by client and polls. Minimum safe age is therefore the stall timeout itself, 2 hours by default. The argument cannot be "we can tell a dead magnet from a slow one", because you cannot; it is "we waited long enough that it does not matter". Two hours of a client failing to find one peer willing to serve metadata is not a slow magnet.Every torrent client has the blind spot, and all four are covered
totalSize0,percentDone0,metadataPercentComplete0,errorStringempty; reporter saw status 0errorStringmetaDL(orforcedMetaDL), size 0, progress 0stalledDLonlyDownloading,total_size0, because Deluge folds libtorrent'sdownloading_metadatainto plain DownloadingErrorstate<hash>.metaplaceholder,d.size_bytes0,d.messageemptyd.messagerTorrent caveat: it sometimes does not publish the
.metaplaceholder in the download list at all (Client.Addalready warns about that window). When it is invisible this rule cannot see it andfailDownloadThatNeverArrivedfires instead, which is the right outcome by a different route.Transmission gained two requested RPC fields,
metadataPercentCompleteandpeersConnected(RPC 14, Transmission 2.80, 2013; an older daemon omits them and they decode as 0, which thetotalSizetest has already established).totalSizeis load-bearing. Status is deliberately not part of the test, because the reporter's torrent was status 0 without anyone having stopped it.I did not require zero connected peers, although the issue suggests it: a magnet with peers that none of them will serve metadata for is just as dead, and requiring 0 would let that case sit for ever. The field is fetched and logged, never decided on.
What
handleStalledDownloaddoesConfirmed on main, in order:
removeStalledFromClient(delete with data),SetError, adownloadStalledhistory event, a blocklist entry, then, if auto grab is on, adownloadRequeuedevent and a re-search. That is what the reporter asked for, except the blocklist, which now applies to the client-reported kind only, for the reasons in finding 1. Removal with data stays correct for both kinds: a torrent that never resolved metadata has no data, so the removal is just the empty entry going away.What the user sees
stalled: the download client never resolved this magnet's metadata, so it has no files and no size. 98 characters, underERROR_SUMMARY_LEN(200), sosummarizeErrorshows it whole with no expander. Distinct from thestalled: no peers / no download progressthe client-reported kind still writes.stall detectedwithkind=no_metadata, the reason, andblocklisted=false, plus an Info line saying the blocklist was skipped and why.stall check: most of this client's unfinished torrents have no metadata, which is a download client or network fault rather than bad releases — leaving them alone, with the count and the denominator. A silent skip here would look exactly like a working stall detector, which is the No import from Deluge #1019 failure mode.downloadStalled, thendownloadRequeuedwhen the re-search fires.No new frontend strings, so no
en.jsonchange: these are server generated and stored on the row.Fail before evidence
Round 2 findings
Captured by restoring the previous commit's policy in place (
Blocklists()returns true for every kind,LooksLikeClientOutage()returns false, theStallNonerejection removed) and leaving everything else, then restoring.go test ./internal/downloader/ -run 'LooksLikeClientOutage|ZeroValueIsUnusable'go test ./internal/scheduler/ -run 'NoMetadata|RejectsStallNone'— the outage case is the reviewer's scenario reproduced, and it shows exactly the damage described:Round 1, the detection rule
Captured against main by neutralising the four predicates to
return falseand reverting the two added Transmission RPC fields.go test ./internal/downloader/ -run 'NoMetadata|MetaDL'(The assertions moved between rounds: round 1's version of that test asserted the blocklist entry, round 2's asserts its absence. The line numbers above are each round's file.)
Stated plainly:
TestCheckStalledDownloads_NoMetadataYoungerThanTimeout,TestCheckStalledDownloads_HealthyTransmissionDownloadUntouchedandTestCheckStalledDownloads_NoMetadataBelowOutageFloorpass before their respective changes as well, and they should. They are guards against over-reach, not proofs of new behaviour: they go red if a later edit drops the age gate, widens the rule onto healthy torrents, or lets the batch guard swallow ordinary stalls.TestGetStalledTorrents_Transmission_StoppedWithErrorneeded its fixture fixed rather than its assertions: its four fake torrents carried nototalSizeat all, so under the new rule every one of them also matched "no metadata". They now carry a size, which is what a torrent with metadata looks like.Tests
internal/downloader/nometadata_test.go:NoMetadataerrorStringlands inClientReportedand not inNoMetadatatorrent-getactually asks fortotalSize,percentDoneandmetadataPercentCompletemetaDLandforcedMetaDL, DelugeDownloadingwithtotal_size0, rTorrent's zerod.size_bytesplaceholderStallKind: bug-shaped reason, no blocklist, and the two real kinds' blocklist policyinternal/scheduler/scheduler_nometadata_test.go:handleStalledDownloadwith the zero kind does nothingThe pre-existing
TestCheckStalledDownloads_QBitStalledTorrentstill asserts the blocklist entry and still passes, which is the regression guard for "client-reported stalls behave exactly as before".Verification
go build ./...,go vet ./...,gofmt -lcleango test ./internal/downloader/... ./internal/scheduler/... ./internal/importer/... ./internal/api/...passgo test -race ./internal/downloader/ ./internal/scheduler/passGOOS=windows go build ./...andGOOS=darwin go build ./...passNo new Go module or npm package. No frontend change.
Security
No new endpoint, no auth or settings surface, no user scoped query, nothing reading
X-Forwarded-*. The one new outbound behaviour is two extra fields on an existing Transmissiontorrent-get. The only destructive action, removing the torrent with data, is the pre-existingremoveStalledFromClientpath with its guards unchanged, and the batch guard now makes it strictly harder to reach during a fault than it was before this PR.A note for the maintainer
ccarpinteri offered to put up a PR for this and diagnosed it down to the line in
GetStalledIDsbefore filing. I did not wait, so please credit them as you see fit; the changelog fragment thanks them for the report and the diagnosis. Note that their suggested outcome (blocklist and re-search) is the part I ended up not taking, for the reason in finding 1.🤖 Generated with Claude Code
https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9