From 0893012e47784068a532d33017c4dd997f560516 Mon Sep 17 00:00:00 2001 From: Charlie Carpinteri Date: Sat, 19 Sep 2026 12:58:50 +1000 Subject: [PATCH 1/3] fix(transmission): identify downloads by info hash, not the session torrent 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 #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 --- internal/downloader/adapter.go | 62 ++++- internal/downloader/adapter_stall_test.go | 18 +- internal/downloader/transmission/client.go | 60 ++++- internal/downloader/transmission/types.go | 21 +- .../importer/scanner_dispatch_matrix_test.go | 4 +- internal/importer/scanner_extra_test.go | 6 +- internal/importer/scanner_filelist_test.go | 2 +- internal/importer/scanner_poll.go | 194 +++++++++++++- .../scanner_transmission_hash_test.go | 246 ++++++++++++++++++ 9 files changed, 564 insertions(+), 49 deletions(-) create mode 100644 internal/importer/scanner_transmission_hash_test.go diff --git a/internal/downloader/adapter.go b/internal/downloader/adapter.go index 97cae4f26..c42c0f403 100644 --- a/internal/downloader/adapter.go +++ b/internal/downloader/adapter.go @@ -12,6 +12,7 @@ import ( "strings" "github.com/vavallee/bindery/internal/downloader/nzbget" + "github.com/vavallee/bindery/internal/downloader/transmission" "github.com/vavallee/bindery/internal/models" "github.com/vavallee/bindery/internal/pathmap" ) @@ -128,14 +129,25 @@ func SendDownload(ctx context.Context, client *models.DownloadClient, sourceURL, if !strings.HasPrefix(transDL, "/") { transDL = "" } - torrentID, err := trans.AddTorrent(ctx, sourceURL, transDL, opts.SeedRatio) + added, err := trans.AddTorrentDetailed(ctx, sourceURL, transDL, opts.SeedRatio) if err != nil { return nil, err } - if torrentID == 0 { + if added.ID == 0 { return nil, fmt.Errorf("downloader accepted request but did not return a trackable torrent ID") } - result.RemoteID = strconv.FormatInt(torrentID, 10) + // Persist the info hash, not the numeric id. Transmission ids are + // session-scoped and are renumbered on every daemon restart, so a + // stored id stops matching the torrent it was grabbed for — the + // download then sits at "downloading" forever, and any later action + // keyed on that id (import, removal) lands on whichever torrent + // inherited the number. Fall back to the id only if the daemon gave + // us no hash at all, which keeps a grab trackable in the same session. + if hash := strings.ToLower(strings.TrimSpace(added.HashString)); hash != "" { + result.RemoteID = hash + } else { + result.RemoteID = strconv.FormatInt(added.ID, 10) + } return result, nil case "qbittorrent": qb := QbittorrentFor(client) @@ -219,6 +231,24 @@ func torrentSavePath(client *models.DownloadClient, opts SendOptions) string { return pathmap.Parse(client.PathRemap).ApplyInverse(localPath) } +// RemoveTransmissionTorrent removes a torrent identified by whatever Bindery +// persisted for it. Anything grabbed since hashes were persisted stores an +// info hash, which Transmission accepts anywhere an id is taken; rows written +// before that store the session-scoped numeric id and are removed by id for +// compatibility. The poller rewrites those rows to the hash as soon as it can +// confirm the torrent, so the numeric branch is only reached for a download +// the poller has not yet reconciled. +func RemoveTransmissionTorrent(ctx context.Context, trans *transmission.Client, ref string, deleteFiles bool) error { + ref = strings.TrimSpace(ref) + if ref == "" { + return nil + } + if torrentID, err := strconv.ParseInt(ref, 10, 64); err == nil { + return trans.RemoveTorrent(ctx, torrentID, deleteFiles) + } + return trans.RemoveTorrentByHash(ctx, ref, deleteFiles) +} + // RemoveDownload removes a download from its client, optionally taking the data // with it. // @@ -234,12 +264,7 @@ func RemoveDownload(ctx context.Context, client *models.DownloadClient, dl *mode if dl.TorrentID == nil || *dl.TorrentID == "" { return nil } - torrentID, err := strconv.ParseInt(*dl.TorrentID, 10, 64) - if err != nil { - return fmt.Errorf("invalid transmission torrent id %q: %w", *dl.TorrentID, err) - } - trans := TransmissionFor(client) - return trans.RemoveTorrent(ctx, torrentID, deleteFiles) + return RemoveTransmissionTorrent(ctx, TransmissionFor(client), *dl.TorrentID, deleteFiles) case "qbittorrent": if dl.TorrentID == nil || *dl.TorrentID == "" { return nil @@ -316,7 +341,13 @@ func GetStalledIDs(ctx context.Context, client *models.DownloadClient) (map[stri for _, t := range torrents { // status 0 = stopped; treat stopped+error as stalled if t.Status == 0 && strings.TrimSpace(t.ErrorString) != "" { - out[strconv.FormatInt(t.ID, 10)] = true + // Keyed by info hash, which is what a download stores. The + // numeric id is deliberately not offered as a fallback: the + // caller removes what it matches here, and a session id that + // has been renumbered would remove the wrong torrent. + if hash := strings.ToLower(strings.TrimSpace(t.HashString)); hash != "" { + out[hash] = true + } } } return out, true, nil @@ -427,11 +458,12 @@ func getTorrentLiveStatuses(ctx context.Context, client *models.DownloadClient) out := make(map[string]LiveStatus, len(torrents)) for _, t := range torrents { id := strconv.FormatInt(t.ID, 10) + hash := strings.ToLower(strings.TrimSpace(t.HashString)) status := strconv.Itoa(t.Status) if errString := strings.TrimSpace(t.ErrorString); errString != "" { status = "error: " + errString } - out[id] = LiveStatus{ + live := LiveStatus{ Percentage: fmt.Sprintf("%.1f", t.PercentDone*100), TimeLeft: etaToTimeLeft(t.ETA), Speed: bytesPerSecondToString(t.DownloadRate), @@ -439,6 +471,14 @@ func getTorrentLiveStatuses(ctx context.Context, client *models.DownloadClient) SizeLeft: t.LeftUntilDone, Status: status, } + // Hash is what a download stores; the numeric id is kept as an + // alias so a row the poller has not reconciled yet still shows + // progress. This overlay is read-only, so a stale id can only + // mislabel a queue row, never act on a torrent. + if hash != "" { + out[hash] = live + } + out[id] = live } return out, nil } diff --git a/internal/downloader/adapter_stall_test.go b/internal/downloader/adapter_stall_test.go index 032e91372..c83e8f405 100644 --- a/internal/downloader/adapter_stall_test.go +++ b/internal/downloader/adapter_stall_test.go @@ -87,7 +87,9 @@ func TestGetStalledIDs_QBittorrent_EmptyList(t *testing.T) { // TestGetStalledIDs_Transmission_StoppedWithError verifies that Transmission // torrents in status 0 (stopped) with a non-empty errorString are reported -// as stalled, while other states are not. +// as stalled, while other states are not. Entries are keyed by info hash, +// which is what a download stores: the caller removes what it matches here, +// and a renumbered session id would remove the wrong torrent. func TestGetStalledIDs_Transmission_StoppedWithError(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/transmission/rpc" { @@ -96,10 +98,10 @@ func TestGetStalledIDs_Transmission_StoppedWithError(t *testing.T) { _ = json.NewEncoder(w).Encode(map[string]any{ "arguments": map[string]any{ "torrents": []map[string]any{ - {"id": 1, "status": 0, "errorString": "tracker error"}, - {"id": 2, "status": 0, "errorString": ""}, - {"id": 3, "status": 2, "errorString": "some error"}, - {"id": 4, "status": 0, "errorString": " "}, + {"id": 1, "hashString": "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", "status": 0, "errorString": "tracker error"}, + {"id": 2, "hashString": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "status": 0, "errorString": ""}, + {"id": 3, "hashString": "cccccccccccccccccccccccccccccccccccccccc", "status": 2, "errorString": "some error"}, + {"id": 4, "hashString": "dddddddddddddddddddddddddddddddddddddddd", "status": 0, "errorString": " "}, }, }, "result": "success", @@ -120,8 +122,10 @@ func TestGetStalledIDs_Transmission_StoppedWithError(t *testing.T) { if len(stalled) != 1 { t.Fatalf("expected 1 stalled entry, got %d: %v", len(stalled), stalled) } - if !stalled["1"] { - t.Error("expected transmission id '1' to be stalled") + // Lower-cased: downloads store the hash lower-cased, and the caller looks + // it up that way. + if !stalled["aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"] { + t.Errorf("expected the stopped-with-error torrent to be stalled, got %v", stalled) } } diff --git a/internal/downloader/transmission/client.go b/internal/downloader/transmission/client.go index 954e6f2f5..e5b9418f4 100644 --- a/internal/downloader/transmission/client.go +++ b/internal/downloader/transmission/client.go @@ -108,6 +108,20 @@ func (c *Client) Test(ctx context.Context) error { // Transmission RPC rejects a negative seedRatioLimit float. A nil pointer // leaves both fields unset so the torrent keeps Transmission's global rule. func (c *Client) AddTorrent(ctx context.Context, magnetOrURL, downloadDir string, seedRatio *float64) (int64, error) { + added, err := c.AddTorrentDetailed(ctx, magnetOrURL, downloadDir, seedRatio) + if err != nil { + return 0, err + } + return added.ID, nil +} + +// AddTorrentDetailed adds a torrent and returns the whole record Transmission +// reported for it, so the caller can persist the info hash rather than the +// numeric id. Transmission ids are session-scoped: the daemon renumbers every +// torrent on restart, so a stored id silently starts pointing at a different +// torrent (or at nothing). Only hashString is stable for the life of the +// torrent. See AddTorrent for the argument semantics. +func (c *Client) AddTorrentDetailed(ctx context.Context, magnetOrURL, downloadDir string, seedRatio *float64) (Torrent, error) { args := map[string]interface{}{} if downloadDir != "" { args["download-dir"] = downloadDir @@ -119,7 +133,7 @@ func (c *Client) AddTorrent(ctx context.Context, magnetOrURL, downloadDir string } else { fetched, err := c.fetchTorrentContent(ctx, magnetOrURL) if err != nil { - return 0, err + return Torrent{}, err } // An indexer http(s) link can 30x-redirect to a magnet: URI (common // with public trackers like The Pirate Bay / Knaben surfaced via @@ -136,31 +150,33 @@ func (c *Client) AddTorrent(ctx context.Context, magnetOrURL, downloadDir string req, err := c.buildRequest(ctx, "torrent-add", args) if err != nil { - return 0, err + return Torrent{}, err } respBody, err := c.doRequest(req) if err != nil { - return 0, err + return Torrent{}, err } var resp TorrentAddResponse if err := json.Unmarshal(respBody, &resp); err != nil { - return 0, fmt.Errorf("decode add torrent response: %w", err) + return Torrent{}, fmt.Errorf("decode add torrent response: %w", err) } if resp.Result != "success" { - return 0, fmt.Errorf("add torrent failed: %s", resp.Result) + return Torrent{}, fmt.Errorf("add torrent failed: %s", resp.Result) } - // Return the ID of the added torrent (prefer newly added, fall back to duplicate) + // Prefer the newly added torrent, fall back to the duplicate: re-adding a + // torrent Transmission already holds is reported under torrent-duplicate + // and is a successful grab as far as Bindery is concerned. if resp.Arguments.TorrentAdded.ID != 0 { - return resp.Arguments.TorrentAdded.ID, nil + return resp.Arguments.TorrentAdded, nil } if resp.Arguments.TorrentDuplicate.ID != 0 { - return resp.Arguments.TorrentDuplicate.ID, nil + return resp.Arguments.TorrentDuplicate, nil } - return 0, fmt.Errorf("no torrent ID returned") + return Torrent{}, fmt.Errorf("no torrent ID returned") } // Transmission seedRatioMode values (RPC spec): 0 = use global limit, @@ -195,7 +211,7 @@ func (c *Client) GetTorrents(ctx context.Context, downloadDir string) ([]Torrent args := map[string]interface{}{ "fields": []string{"id", "hashString", "name", "totalSize", "downloadedEver", "leftUntilDone", "status", "errorString", "rateDownload", "rateUpload", "eta", - "percentDone", "downloadDir", "labels"}, + "percentDone", "downloadDir", "labels", "addedDate"}, } req, err := c.buildRequest(ctx, "torrent-get", args) @@ -294,10 +310,28 @@ func (c *Client) Files(ctx context.Context, torrentID int64) ([]File, error) { return out, nil } -// RemoveTorrent removes a torrent by ID. +// RemoveTorrent removes a torrent by its session-scoped numeric ID. Prefer +// RemoveTorrentByHash when the caller is working from a persisted identifier: +// a stored numeric id goes stale the moment the daemon restarts, and removing +// a stale id deletes whichever torrent has inherited that number. func (c *Client) RemoveTorrent(ctx context.Context, torrentID int64, deleteFiles bool) error { + return c.removeTorrent(ctx, torrentID, fmt.Sprintf("%d", torrentID), deleteFiles) +} + +// RemoveTorrentByHash removes a torrent by its info hash. The Transmission RPC +// spec accepts a SHA1 hash string anywhere an id is taken, and unlike the +// numeric id the hash is stable across daemon restarts. +func (c *Client) RemoveTorrentByHash(ctx context.Context, hash string, deleteFiles bool) error { + hash = strings.ToLower(strings.TrimSpace(hash)) + if hash == "" { + return fmt.Errorf("remove torrent: empty info hash") + } + return c.removeTorrent(ctx, hash, hash, deleteFiles) +} + +func (c *Client) removeTorrent(ctx context.Context, id interface{}, label string, deleteFiles bool) error { args := map[string]interface{}{ - "ids": []int64{torrentID}, + "ids": []interface{}{id}, } if deleteFiles { args["delete-local-data"] = true @@ -325,7 +359,7 @@ func (c *Client) RemoveTorrent(ctx context.Context, torrentID int64, deleteFiles if reason == "" { reason = "Transmission gave no reason" } - return fmt.Errorf("transmission rejected the removal of torrent %d: %s", torrentID, reason) + return fmt.Errorf("transmission rejected the removal of torrent %s: %s", label, reason) } return nil } diff --git a/internal/downloader/transmission/types.go b/internal/downloader/transmission/types.go index fa0b2c6db..8684fb97e 100644 --- a/internal/downloader/transmission/types.go +++ b/internal/downloader/transmission/types.go @@ -11,14 +11,19 @@ type Torrent struct { // Status is the Transmission RPC status enum (stable since 2.40): // 0=stopped 1=queued-to-check 2=checking // 3=queued-to-download 4=downloading 5=queued-to-seed 6=seeding - Status int `json:"status"` - ErrorString string `json:"errorString"` - DownloadRate int64 `json:"rateDownload"` - UploadRate int64 `json:"rateUpload"` - ETA int64 `json:"eta"` - PercentDone float64 `json:"percentDone"` - DownloadDir string `json:"downloadDir"` - Labels []string `json:"labels"` + Status int `json:"status"` + ErrorString string `json:"errorString"` + DownloadRate int64 `json:"rateDownload"` + UploadRate int64 `json:"rateUpload"` + ETA int64 `json:"eta"` + PercentDone float64 `json:"percentDone"` + DownloadDir string `json:"downloadDir"` + // AddedDate is the Unix time Transmission accepted the torrent. It is the + // only field besides hashString that survives a daemon restart unchanged, + // which makes it the reconciliation key for downloads grabbed before the + // info hash was persisted. + AddedDate int64 `json:"addedDate"` + Labels []string `json:"labels"` } // TorrentAddResponse is returned when adding a torrent. diff --git a/internal/importer/scanner_dispatch_matrix_test.go b/internal/importer/scanner_dispatch_matrix_test.go index e65f0d1b3..8a14fdfbe 100644 --- a/internal/importer/scanner_dispatch_matrix_test.go +++ b/internal/importer/scanner_dispatch_matrix_test.go @@ -374,7 +374,7 @@ func TestCheckDownloads_DispatchMatrix_Transmission(t *testing.T) { t.Cleanup(srv.Close) client := f.createClient(t, &models.DownloadClient{Name: "transmission", Type: "transmission"}, srv.URL) - torrentID := "42" // Transmission downloads are matched by numeric torrent id. + torrentID := "feedfacefeedfacefeedfacefeedfacefeedface" // Transmission downloads are matched by info hash. f.createDownload(t, &models.Download{ GUID: "guid-matrix-transmission", Title: "the-book", Status: models.StateDownloading, Protocol: "torrent", TorrentID: &torrentID, DownloadClientID: &client.ID, @@ -686,7 +686,7 @@ func TestCheckDownloads_DispatchMatrix_TwoClients(t *testing.T) { GUID: "guid-multi-sab", Title: "book-a", Status: models.StateDownloading, Protocol: "usenet", SABnzbdNzoID: &nzo, DownloadClientID: &sabClient.ID, }) - torrentID := "42" // transmissionMatrixHandler always returns id=42. + torrentID := "feedfacefeedfacefeedfacefeedfacefeedface" // transmissionMatrixHandler's torrent hash. f.createDownload(t, &models.Download{ GUID: "guid-multi-trans", Title: "the-book", Status: models.StateDownloading, Protocol: "torrent", TorrentID: &torrentID, DownloadClientID: &transClient.ID, diff --git a/internal/importer/scanner_extra_test.go b/internal/importer/scanner_extra_test.go index 175733977..4fb441581 100644 --- a/internal/importer/scanner_extra_test.go +++ b/internal/importer/scanner_extra_test.go @@ -337,6 +337,7 @@ func TestCheckTransmissionDownloads_StoppedWithoutErrorDoesNotFail(t *testing.T) "arguments": map[string]any{ "torrents": []map[string]any{{ "id": 7, + "hashString": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "status": 0, "percentDone": 0.4, "downloadDir": "/downloads", @@ -360,7 +361,7 @@ func TestCheckTransmissionDownloads_StoppedWithoutErrorDoesNotFail(t *testing.T) t.Fatalf("create client: %v", err) } - torrentID := "7" + torrentID := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" dl := &models.Download{ GUID: "guid-paused", DownloadClientID: &client.ID, @@ -398,6 +399,7 @@ func TestCheckTransmissionDownloads_StoppedWithErrorMarksFailed(t *testing.T) { "arguments": map[string]any{ "torrents": []map[string]any{{ "id": 9, + "hashString": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "status": 0, "percentDone": 0.2, "downloadDir": "/downloads", @@ -421,7 +423,7 @@ func TestCheckTransmissionDownloads_StoppedWithErrorMarksFailed(t *testing.T) { t.Fatalf("create client: %v", err) } - torrentID := "9" + torrentID := "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" dl := &models.Download{ GUID: "guid-errored", DownloadClientID: &client.ID, diff --git a/internal/importer/scanner_filelist_test.go b/internal/importer/scanner_filelist_test.go index f55ec67af..406ba4a06 100644 --- a/internal/importer/scanner_filelist_test.go +++ b/internal/importer/scanner_filelist_test.go @@ -121,7 +121,7 @@ func TestImport_SingleFileTorrentSkipsSiblingFiles(t *testing.T) { t.Fatal(err) } - torrentID := "42" + torrentID := "h" // matches the fixture torrent's hashString dl := &models.Download{ GUID: "guid-903", Title: "The Book", diff --git a/internal/importer/scanner_poll.go b/internal/importer/scanner_poll.go index 2caab678a..8fb3ecb91 100644 --- a/internal/importer/scanner_poll.go +++ b/internal/importer/scanner_poll.go @@ -8,6 +8,8 @@ import ( "path/filepath" "strconv" "strings" + "time" + "unicode" "github.com/vavallee/bindery/internal/downloader" "github.com/vavallee/bindery/internal/downloader/deluge" @@ -210,6 +212,18 @@ func (s *Scanner) checkTransmissionDownloads(ctx context.Context, client *models return } + // Torrents are indexed by info hash, which is stable for the life of the + // torrent. The numeric id is not: Transmission renumbers every torrent when + // the daemon restarts, so an id stored at grab time either matches nothing + // (the download strands at "downloading" forever) or matches whichever + // unrelated torrent inherited the number. + torrentsByHash := make(map[string]transmission.Torrent, len(torrents)) + for _, t := range torrents { + if hash := strings.ToLower(strings.TrimSpace(t.HashString)); hash != "" { + torrentsByHash[hash] = t + } + } + // Surface a misconfiguration when the Category filter returns nothing but the // daemon actually holds torrents (#1091). A silent zero-match means every // Bindery grab permanently sits at "downloading" with no indication of why. @@ -226,21 +240,38 @@ func (s *Scanner) checkTransmissionDownloads(ctx context.Context, client *models slog.Warn("download poll: failed to list downloads", "client", client.Name, "error", err) return } - torrentsMap := make(map[string]transmission.Torrent) - for _, t := range torrents { - torrentsMap[fmt.Sprintf("%d", t.ID)] = t - } // Track which downloads' sources we observed this cycle so stale // StateImportFailed downloads (torrent removed) can be terminally blocked // rather than left stuck below the retry limit (issue #706 finding 4). seenSourceIDs := make(map[int64]bool) + // Every hash some download already owns. A torrent belongs to exactly one + // download, so legacy reconciliation may never claim one out from under a + // row that already names it, and two legacy rows may not both land on the + // same torrent. + claimedHashes := make(map[string]bool) + for _, dl := range allDownloads { + if dl.TorrentID == nil { + continue + } + if ref := strings.ToLower(strings.TrimSpace(*dl.TorrentID)); ref != "" { + if _, err := strconv.ParseInt(ref, 10, 64); err != nil { + claimedHashes[ref] = true + } + } + } + + legacyMatches := s.reconcileLegacyTransmissionIDs(ctx, client, allDownloads, torrents, claimedHashes) + for _, dl := range allDownloads { if dl.DownloadClientID == nil || *dl.DownloadClientID != client.ID || dl.TorrentID == nil { continue } - torrent, ok := torrentsMap[*dl.TorrentID] + torrent, ok := torrentsByHash[strings.ToLower(strings.TrimSpace(*dl.TorrentID))] + if !ok { + torrent, ok = legacyMatches[dl.ID] + } if !ok { continue } @@ -966,6 +997,159 @@ func (s *Scanner) tryImportTransmission(ctx context.Context, dl *models.Download s.tryImportInternal(ctx, dl, downloadPath, "transmission", safeRemoteID(dl.TorrentID), "", nil, explicitFiles) } +// legacyTransmissionMatchWindow is how far apart Bindery's grab timestamp and +// Transmission's addedDate may be and still describe the same grab. The two are +// written seconds apart in practice; the window only has to absorb clock skew +// between Bindery and the daemon. +const legacyTransmissionMatchWindow = 5 * time.Minute + +// reconcileLegacyTransmissionIDs recovers downloads grabbed before the info +// hash was persisted — their TorrentID holds Transmission's session-scoped +// numeric id — and backfills the hash so every later poll, import and removal +// keys on a stable value. Same recovery shape as the qBittorrent hash backfill +// (#939). It returns the torrent matched for each recovered download and +// rewrites the passed-in rows so the caller sees the new identifier. +// +// The stored id is deliberately never used to find the torrent. Transmission +// renumbers on restart, so that id either matches nothing or matches an +// unrelated torrent that inherited the number, and with remove_on_import +// enabled acting on a wrong match deletes somebody else's torrent. +// +// Matching runs from the torrent's side, on addedDate against each download's +// grab time — the one other field a restart leaves untouched — and a torrent +// is only claimed when the pairing is unambiguous: +// +// - a torrent no download is within the window of is left alone; +// - a torrent exactly one download matches is assigned to it; +// - a torrent several downloads match (a batch grabbed minutes apart, which +// the window alone cannot separate) is assigned only if the release name +// picks out exactly one of them, and otherwise left for manual resolution. +// +// Terminal downloads are excluded outright: they will not be imported or +// removed again, so rewriting their identifier can only ever be wrong, and +// including them lets a long-finished row outbid the live one for a torrent. +func (s *Scanner) reconcileLegacyTransmissionIDs( + ctx context.Context, + client *models.DownloadClient, + downloads []models.Download, + torrents []transmission.Torrent, + claimedHashes map[string]bool, +) map[int64]transmission.Torrent { + // Index of rows still identified by a numeric id, by position, so a match + // can write the hash back into the caller's slice. + legacy := make([]int, 0, len(downloads)) + for i := range downloads { + dl := &downloads[i] + if dl.DownloadClientID == nil || *dl.DownloadClientID != client.ID || dl.TorrentID == nil { + continue + } + if dl.Status == models.StateImported || dl.Status == models.StateFailed { + continue + } + if _, err := strconv.ParseInt(strings.TrimSpace(*dl.TorrentID), 10, 64); err != nil { + continue // already a hash + } + if legacyGrabTime(dl).IsZero() { + continue + } + legacy = append(legacy, i) + } + if len(legacy) == 0 { + return nil + } + + matches := make(map[int64]transmission.Torrent) + assigned := make(map[int64]bool) // download IDs already matched this pass + for _, t := range torrents { + hash := strings.ToLower(strings.TrimSpace(t.HashString)) + if t.AddedDate == 0 || hash == "" || claimedHashes[hash] { + continue + } + addedAt := time.Unix(t.AddedDate, 0) + + var inWindow []int + for _, i := range legacy { + if assigned[downloads[i].ID] { + continue + } + delta := addedAt.Sub(legacyGrabTime(&downloads[i])) + if delta < 0 { + delta = -delta + } + if delta <= legacyTransmissionMatchWindow { + inWindow = append(inWindow, i) + } + } + if len(inWindow) > 1 { + var named []int + for _, i := range inWindow { + if releaseNamesMatch(t.Name, downloads[i].Title) { + named = append(named, i) + } + } + if len(named) != 1 { + slog.Warn("transmission: several downloads were grabbed at this torrent's added time and none is a clear name match — leaving them for manual resolution rather than guessing", + "torrent", t.Name, "hash", hash, "candidates", len(inWindow)) + continue + } + inWindow = named + } + if len(inWindow) != 1 { + continue + } + + dl := &downloads[inWindow[0]] + slog.Info("transmission: recovered a download stranded by a renumbered torrent id; backfilling its info hash", + "title", dl.Title, "stale_torrent_id", *dl.TorrentID, "current_torrent_id", t.ID, "hash", hash) + if err := s.downloads.SetTorrentID(ctx, dl.ID, hash); err != nil { + slog.Warn("transmission: failed to backfill info hash", "download_id", dl.ID, "error", err) + continue + } + h := hash + dl.TorrentID = &h + claimedHashes[hash] = true + assigned[dl.ID] = true + matches[dl.ID] = t + } + return matches +} + +// legacyGrabTime is when Bindery handed the release to the client: grabbed_at +// when it was stamped, the row's creation time otherwise. +func legacyGrabTime(dl *models.Download) time.Time { + if dl.GrabbedAt != nil { + return *dl.GrabbedAt + } + return dl.AddedAt +} + +// releaseNamesMatch compares a torrent name with a download title tolerantly. +// Trackers routinely hand back URL-ish names where the spaces became '+', '_' +// or '.', so "The+Lantern+Makers+Daughter+EPUB" and "The Lantern Makers +// Daughter EPUB" are the same release and a strict compare would reject the +// match. +func releaseNamesMatch(torrentName, title string) bool { + a := normaliseReleaseName(torrentName) + return a != "" && a == normaliseReleaseName(title) +} + +func normaliseReleaseName(s string) string { + var b strings.Builder + pendingSpace := false + for _, r := range strings.ToLower(strings.TrimSpace(s)) { + if r == '+' || r == '_' || r == '.' || unicode.IsSpace(r) { + pendingSpace = b.Len() > 0 + continue + } + if pendingSpace { + b.WriteRune(' ') + pendingSpace = false + } + b.WriteRune(r) + } + return b.String() +} + // tryImportQbittorrent attempts to import a completed qBittorrent download. See // tryImportTransmission for the semantics of explicitFiles. func (s *Scanner) tryImportQbittorrent(ctx context.Context, dl *models.Download, downloadPath string, explicitFiles []string) { diff --git a/internal/importer/scanner_transmission_hash_test.go b/internal/importer/scanner_transmission_hash_test.go new file mode 100644 index 000000000..6d2feea9d --- /dev/null +++ b/internal/importer/scanner_transmission_hash_test.go @@ -0,0 +1,246 @@ +package importer + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/vavallee/bindery/internal/models" +) + +// transmissionListHandler serves a torrent-get listing holding the given +// torrents, which is all checkTransmissionDownloads asks of a daemon. +func transmissionListHandler(t *testing.T, torrents []map[string]any) http.HandlerFunc { + t.Helper() + return func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/transmission/rpc" { + t.Fatalf("unexpected path: %s", r.URL.Path) + } + _ = json.NewEncoder(w).Encode(map[string]any{ + "result": "success", + "arguments": map[string]any{"torrents": torrents}, + }) + } +} + +// category mirrors a real deployment: Transmission has no categories, so +// Bindery's Category doubles as a downloadDir filter. It also keeps +// failDownloadThatNeverArrived out of these tests — a filtered listing is not +// a complete source list, so absence from it is not treated as definitive. +func transmissionClientFixture(t *testing.T, s *Scanner, ctx context.Context, srv *httptest.Server, category string) *models.DownloadClient { + t.Helper() + host, port := scannerTestHostPort(t, srv.URL) + client := &models.DownloadClient{ + Name: "transmission", + Type: "transmission", + Host: host, + Port: port, + Category: category, + Enabled: true, + } + if err := s.clients.Create(ctx, client); err != nil { + t.Fatalf("create client: %v", err) + } + return client +} + +// TestCheckTransmissionDownloads_RecoversRenumberedTorrentID is the regression +// test for downloads stranded by a Transmission restart. The daemon renumbers +// every torrent when it restarts, so a download grabbed as id 17 comes back as +// id 1 and the poller used to lose it forever — it sat at "downloading" while +// the torrent seeded on, never imported and (with remove_on_import) never +// removed. The row must be recovered via addedDate and rewritten to the hash. +func TestCheckTransmissionDownloads_RecoversRenumberedTorrentID(t *testing.T) { + grabbedAt := time.Now().Add(-48 * time.Hour).UTC() + const hash = "0123456789abcdef0123456789abcdef01234567" + + srv := httptest.NewServer(transmissionListHandler(t, []map[string]any{{ + // Same torrent, renumbered from 17 to 1 by a daemon restart. + "id": 1, + "hashString": hash, + "name": "Stranded Release", + "status": 0, + "percentDone": 0.5, + "downloadDir": "/downloads", + "addedDate": grabbedAt.Unix(), + }})) + defer srv.Close() + + s, _, _, ctx := scannerFixture(t, t.TempDir()) + client := transmissionClientFixture(t, s, ctx, srv, "/downloads") + + staleID := "17" + dl := &models.Download{ + GUID: "guid-renumbered", + DownloadClientID: &client.ID, + Title: "Stranded Release", + NZBURL: "magnet:?xt=urn:btih:0123", + Status: models.DownloadStatusDownloading, + Protocol: "torrent", + TorrentID: &staleID, + } + if err := s.downloads.Create(ctx, dl); err != nil { + t.Fatalf("create download: %v", err) + } + if err := s.downloads.SetGrabbedAt(ctx, dl.ID, grabbedAt); err != nil { + t.Skipf("fixture cannot set grabbed_at: %v", err) + } + + s.checkTransmissionDownloads(ctx, client) + + got, err := s.downloads.GetByGUID(ctx, dl.GUID) + if err != nil { + t.Fatalf("get by guid: %v", err) + } + if got.TorrentID == nil || *got.TorrentID != hash { + t.Fatalf("expected torrent id to be rewritten to the info hash %q, got %v", hash, got.TorrentID) + } +} + +// TestCheckTransmissionDownloads_IgnoresRecycledTorrentID guards the other +// direction: a stale numeric id that now belongs to an unrelated torrent must +// not be matched. Acting on that match would mark the wrong download imported +// and, with remove_on_import enabled, delete a torrent Bindery never grabbed. +func TestCheckTransmissionDownloads_IgnoresRecycledTorrentID(t *testing.T) { + srv := httptest.NewServer(transmissionListHandler(t, []map[string]any{{ + // Holds the id our download remembers, but was added a year later: + // a different torrent that inherited the number. + "id": 17, + "hashString": "ffffffffffffffffffffffffffffffffffffffff", + "name": "Somebody Else's Torrent", + "status": 6, + "percentDone": 1.0, + "downloadDir": "/downloads", + "addedDate": time.Now().Unix(), + }})) + defer srv.Close() + + s, _, _, ctx := scannerFixture(t, t.TempDir()) + client := transmissionClientFixture(t, s, ctx, srv, "/downloads") + + staleID := "17" + dl := &models.Download{ + GUID: "guid-recycled", + DownloadClientID: &client.ID, + Title: "Our Release", + NZBURL: "magnet:?xt=urn:btih:dead", + Status: models.DownloadStatusDownloading, + Protocol: "torrent", + TorrentID: &staleID, + } + if err := s.downloads.Create(ctx, dl); err != nil { + t.Fatalf("create download: %v", err) + } + if err := s.downloads.SetGrabbedAt(ctx, dl.ID, time.Now().Add(-365*24*time.Hour).UTC()); err != nil { + t.Skipf("fixture cannot set grabbed_at: %v", err) + } + + s.checkTransmissionDownloads(ctx, client) + + got, err := s.downloads.GetByGUID(ctx, dl.GUID) + if err != nil { + t.Fatalf("get by guid: %v", err) + } + if got.TorrentID == nil || *got.TorrentID != staleID { + t.Fatalf("expected the stale id %q to be left alone, got %v", staleID, got.TorrentID) + } + if got.Status != models.DownloadStatusDownloading { + t.Fatalf("expected status to remain downloading, got %q", got.Status) + } +} + +func TestReleaseNamesMatch(t *testing.T) { + cases := []struct { + name, torrent, title string + want bool + }{ + {"plus separated", "The+Lantern+Makers+Daughter+by+E.+Vance+EPUB", "The Lantern Makers Daughter by E. Vance EPUB", true}, + {"dot separated", "Some.Release.Name.EPUB", "Some Release Name EPUB", true}, + {"case insensitive", "SOME RELEASE", "some release", true}, + {"different release", "Some Other Book EPUB", "The Lantern Makers Daughter EPUB", false}, + {"empty torrent name", "", "The Lantern Makers Daughter", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := releaseNamesMatch(tc.torrent, tc.title); got != tc.want { + t.Fatalf("releaseNamesMatch(%q, %q) = %v, want %v", tc.torrent, tc.title, got, tc.want) + } + }) + } +} + +// TestCheckTransmissionDownloads_LegacyBatchDoesNotCollapseOntoOneTorrent is a +// regression test for a live incident: a batch of downloads grabbed within +// minutes of each other all fell inside the addedDate window of the single +// torrent still present, so each one "recovered" onto it and had its id +// rewritten to that torrent's hash. A torrent belongs to one download — the +// first claim wins and the rest must be left alone rather than guessed at. +// Terminal downloads are not reconciled at all: they will never be imported or +// removed again, so rewriting their identifier can only ever be wrong. +func TestCheckTransmissionDownloads_LegacyBatchDoesNotCollapseOntoOneTorrent(t *testing.T) { + grabbedAt := time.Now().Add(-72 * time.Hour).UTC() + const hash = "89abcdef0123456789abcdef0123456789abcdef" + + srv := httptest.NewServer(transmissionListHandler(t, []map[string]any{{ + "id": 1, + "hashString": hash, + "name": "The+Lantern+Makers+Daughter+by+E.+Vance+EPUB", + "status": 0, + "percentDone": 0.0, + "downloadDir": "/downloads", + "addedDate": grabbedAt.Unix(), + }})) + defer srv.Close() + + s, _, _, ctx := scannerFixture(t, t.TempDir()) + client := transmissionClientFixture(t, s, ctx, srv, "/downloads") + + // Three downloads grabbed inside the same window as the surviving torrent: + // the one that is really it, a sibling still in flight, and one already + // imported whose torrent is long gone. + mk := func(guid, title, torrentID string, status models.DownloadState, offset time.Duration) *models.Download { + id := torrentID + dl := &models.Download{ + GUID: guid, + DownloadClientID: &client.ID, + Title: title, + NZBURL: "magnet:?xt=urn:btih:" + torrentID, + Status: status, + Protocol: "torrent", + TorrentID: &id, + } + if err := s.downloads.Create(ctx, dl); err != nil { + t.Fatalf("create download %s: %v", guid, err) + } + if err := s.downloads.SetGrabbedAt(ctx, dl.ID, grabbedAt.Add(offset)); err != nil { + t.Fatalf("set grabbed_at: %v", err) + } + return dl + } + real := mk("guid-real", "The Lantern Makers Daughter by E. Vance EPUB", "17", models.DownloadStatusDownloading, 0) + sibling := mk("guid-sibling", "A Second Unrelated Book EPUB", "18", models.DownloadStatusDownloading, 90*time.Second) + done := mk("guid-imported", "A Third Unrelated Book EPUB", "20", models.StateImported, 30*time.Second) + + s.checkTransmissionDownloads(ctx, client) + + assertTorrentID := func(dl *models.Download, want, what string) { + t.Helper() + got, err := s.downloads.GetByGUID(ctx, dl.GUID) + if err != nil { + t.Fatalf("get by guid: %v", err) + } + if got.TorrentID == nil { + t.Fatalf("%s: expected torrent id %q, got nil", what, want) + } + if *got.TorrentID != want { + t.Fatalf("%s: expected torrent id %q, got %q", what, want, *got.TorrentID) + } + } + // The name match resolves the ambiguous window in favour of the real one. + assertTorrentID(real, hash, "the download that really is this torrent") + assertTorrentID(sibling, "18", "a sibling grabbed in the same window") + assertTorrentID(done, "20", "an already-imported download") +} From b785e515063ab93fad627c524a76704b40f1ca05 Mon Sep 17 00:00:00 2001 From: Charlie Carpinteri Date: Sat, 19 Sep 2026 12:59:44 +1000 Subject: [PATCH 2/3] fix(transmission): poll the audiobook category as well as the ebook one 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 --- internal/importer/scanner_poll.go | 51 +++++++++++++++++++------------ 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/internal/importer/scanner_poll.go b/internal/importer/scanner_poll.go index 8fb3ecb91..c87e5c3f9 100644 --- a/internal/importer/scanner_poll.go +++ b/internal/importer/scanner_poll.go @@ -200,27 +200,38 @@ func transmissionCompletion(status int, percentDone float64) (complete, stopped func (s *Scanner) checkTransmissionDownloads(ctx context.Context, client *models.DownloadClient) { trans := downloader.TransmissionFor(client) - // Get torrents — Category is used as a download-directory / label filter so - // Bindery only sees its own torrents on a shared instance. GetTorrents - // normalises the path comparison and also accepts Transmission 3.0+ labels, - // so "books" matches both a downloadDir of "/data/books/" and a label "books". - torrents, err := trans.GetTorrents(ctx, client.Category) - if err != nil { - // Warn, not Debug: see checkSABnzbdDownloads (#1019 failure mode). - slog.Warn("download poll: failed to fetch Transmission torrents — downloads will not be imported", - "client", client.Name, "error", err) - return - } - + // Poll every category this client may have grabbed under. Audiobook grabs + // use CategoryAudiobook when it is set, ebook grabs use Category, and + // polling only Category leaves audiobook torrents invisible: their + // downloads then hang at "downloading" for good. CategoriesToPoll returns + // both. GetTorrents normalises the path comparison and also accepts + // Transmission 3.0+ labels, so "books" matches both a downloadDir of + // "/data/books/" and a label "books". + // // Torrents are indexed by info hash, which is stable for the life of the // torrent. The numeric id is not: Transmission renumbers every torrent when - // the daemon restarts, so an id stored at grab time either matches nothing - // (the download strands at "downloading" forever) or matches whichever - // unrelated torrent inherited the number. - torrentsByHash := make(map[string]transmission.Torrent, len(torrents)) - for _, t := range torrents { - if hash := strings.ToLower(strings.TrimSpace(t.HashString)); hash != "" { - torrentsByHash[hash] = t + // the daemon restarts. + torrentsByHash := make(map[string]transmission.Torrent) + var torrents []transmission.Torrent + seenTorrentIDs := make(map[int64]bool) + for _, cat := range downloader.CategoriesToPoll(client) { + found, err := trans.GetTorrents(ctx, cat) + if err != nil { + // Warn, not Debug: see checkSABnzbdDownloads (#1019 failure mode). + slog.Warn("download poll: failed to fetch Transmission torrents — downloads will not be imported", + "client", client.Name, "category", cat, "error", err) + return + } + // The two categories can overlap, so a torrent seen twice is kept once. + for _, t := range found { + if seenTorrentIDs[t.ID] { + continue + } + seenTorrentIDs[t.ID] = true + torrents = append(torrents, t) + if hash := strings.ToLower(strings.TrimSpace(t.HashString)); hash != "" { + torrentsByHash[hash] = t + } } } @@ -230,7 +241,7 @@ func (s *Scanner) checkTransmissionDownloads(ctx context.Context, client *models if client.Category != "" && len(torrents) == 0 { if all, allErr := trans.GetTorrents(ctx, ""); allErr == nil && len(all) > 0 { slog.Warn("transmission: Category filter matched zero torrents — verify Category matches the torrent download directory path or a torrent label", - "client", client.Name, "category", client.Category, "total_torrents", len(all)) + "client", client.Name, "category", client.Category, "category_audiobook", client.CategoryAudiobook, "total_torrents", len(all)) } } From 9a485e48ed50d984976d40930d4f3f34c2f54dcf Mon Sep 17 00:00:00 2001 From: Charlie Carpinteri Date: Sat, 19 Sep 2026 13:00:19 +1000 Subject: [PATCH 3/3] feat(downloader): remove the torrent after a successful import (#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 --- internal/db/download_clients.go | 39 +++++--- .../090_download_client_remove_on_import.sql | 1 + internal/importer/scanner.go | 5 + .../importer/scanner_client_wrappers_test.go | 7 +- internal/importer/scanner_poll.go | 93 ++++++++++++++----- internal/models/download.go | 1 + web/src/api/downloadclients.ts | 4 + web/src/i18n/locales/en.json | 2 + web/src/pages/SettingsPage.test.tsx | 6 ++ web/src/pages/settings/ClientsTab.test.tsx | 41 ++++++++ web/src/pages/settings/ClientsTab.tsx | 50 +++++++++- 11 files changed, 204 insertions(+), 45 deletions(-) create mode 100644 internal/db/migrations/090_download_client_remove_on_import.sql diff --git a/internal/db/download_clients.go b/internal/db/download_clients.go index f7f2f3f11..de0a2926f 100644 --- a/internal/db/download_clients.go +++ b/internal/db/download_clients.go @@ -17,7 +17,7 @@ type DownloadClientRepo struct { const downloadClientSelectColumns = ` id, name, type, host, port, api_key, use_ssl, url_base, username, password, - category, category_audiobook, path_remap, priority, enabled, created_at, updated_at` + category, category_audiobook, path_remap, priority, enabled, remove_on_import, created_at, updated_at` // torrentClientTypes and usenetClientTypes are the download-client types each // protocol can be served by. They back the protocol-scoped queries below; a new @@ -142,13 +142,15 @@ func (r *DownloadClientRepo) List(ctx context.Context) ([]models.DownloadClient, for rows.Next() { var c models.DownloadClient var enabled, useSSL int + var removeOnImport int if err := rows.Scan(&c.ID, &c.Name, &c.Type, &c.Host, &c.Port, &c.APIKey, &useSSL, &c.URLBase, &c.Username, &c.Password, &c.Category, &c.CategoryAudiobook, &c.PathRemap, &c.Priority, - &enabled, &c.CreatedAt, &c.UpdatedAt); err != nil { + &enabled, &removeOnImport, &c.CreatedAt, &c.UpdatedAt); err != nil { return nil, err } c.Enabled = enabled == 1 c.UseSSL = useSSL == 1 + c.RemoveOnImport = removeOnImport == 1 hydrateClientCredentials(&c) clients = append(clients, c) } @@ -157,13 +159,13 @@ func (r *DownloadClientRepo) List(ctx context.Context) ([]models.DownloadClient, func (r *DownloadClientRepo) GetByID(ctx context.Context, id int64) (*models.DownloadClient, error) { var c models.DownloadClient - var enabled, useSSL int + var enabled, useSSL, removeOnImport int err := r.db.QueryRowContext(ctx, ` SELECT `+downloadClientSelectColumns+` FROM download_clients WHERE id=?`, id). Scan(&c.ID, &c.Name, &c.Type, &c.Host, &c.Port, &c.APIKey, &useSSL, &c.URLBase, &c.Username, &c.Password, &c.Category, &c.CategoryAudiobook, &c.PathRemap, &c.Priority, - &enabled, &c.CreatedAt, &c.UpdatedAt) + &enabled, &removeOnImport, &c.CreatedAt, &c.UpdatedAt) if errors.Is(err, sql.ErrNoRows) { return nil, nil } @@ -172,6 +174,7 @@ func (r *DownloadClientRepo) GetByID(ctx context.Context, id int64) (*models.Dow } c.Enabled = enabled == 1 c.UseSSL = useSSL == 1 + c.RemoveOnImport = removeOnImport == 1 hydrateClientCredentials(&c) return &c, nil } @@ -190,13 +193,15 @@ func (r *DownloadClientRepo) ListEnabled(ctx context.Context) ([]models.Download for rows.Next() { var c models.DownloadClient var enabled, useSSL int + var removeOnImport int if err := rows.Scan(&c.ID, &c.Name, &c.Type, &c.Host, &c.Port, &c.APIKey, &useSSL, &c.URLBase, &c.Username, &c.Password, &c.Category, &c.CategoryAudiobook, &c.PathRemap, &c.Priority, - &enabled, &c.CreatedAt, &c.UpdatedAt); err != nil { + &enabled, &removeOnImport, &c.CreatedAt, &c.UpdatedAt); err != nil { return nil, err } c.Enabled = enabled == 1 c.UseSSL = useSSL == 1 + c.RemoveOnImport = removeOnImport == 1 hydrateClientCredentials(&c) clients = append(clients, c) } @@ -205,13 +210,13 @@ func (r *DownloadClientRepo) ListEnabled(ctx context.Context) ([]models.Download func (r *DownloadClientRepo) GetFirstEnabled(ctx context.Context) (*models.DownloadClient, error) { var c models.DownloadClient - var enabled, useSSL int + var enabled, useSSL, removeOnImport int err := r.db.QueryRowContext(ctx, ` SELECT `+downloadClientSelectColumns+` FROM download_clients WHERE enabled=1 ORDER BY priority LIMIT 1`). Scan(&c.ID, &c.Name, &c.Type, &c.Host, &c.Port, &c.APIKey, &useSSL, &c.URLBase, &c.Username, &c.Password, &c.Category, &c.CategoryAudiobook, &c.PathRemap, &c.Priority, - &enabled, &c.CreatedAt, &c.UpdatedAt) + &enabled, &removeOnImport, &c.CreatedAt, &c.UpdatedAt) if errors.Is(err, sql.ErrNoRows) { return nil, nil } @@ -220,6 +225,7 @@ func (r *DownloadClientRepo) GetFirstEnabled(ctx context.Context) (*models.Downl } c.Enabled = enabled == 1 c.UseSSL = useSSL == 1 + c.RemoveOnImport = removeOnImport == 1 hydrateClientCredentials(&c) return &c, nil } @@ -229,14 +235,14 @@ func (r *DownloadClientRepo) GetFirstEnabled(ctx context.Context) (*models.Downl // Returns (nil, nil) if no matching client is configured. func (r *DownloadClientRepo) GetFirstEnabledByProtocol(ctx context.Context, protocol string) (*models.DownloadClient, error) { var c models.DownloadClient - var enabled, useSSL int + var enabled, useSSL, removeOnImport int placeholders, args := clientTypesForProtocol(protocol) err := r.db.QueryRowContext(ctx, ` SELECT `+downloadClientSelectColumns+` FROM download_clients WHERE enabled=1 AND type IN (`+placeholders+`) ORDER BY priority LIMIT 1`, args...). Scan(&c.ID, &c.Name, &c.Type, &c.Host, &c.Port, &c.APIKey, &useSSL, &c.URLBase, &c.Username, &c.Password, &c.Category, &c.CategoryAudiobook, &c.PathRemap, &c.Priority, - &enabled, &c.CreatedAt, &c.UpdatedAt) + &enabled, &removeOnImport, &c.CreatedAt, &c.UpdatedAt) if err != nil && !errors.Is(err, sql.ErrNoRows) { return nil, err } @@ -245,6 +251,7 @@ func (r *DownloadClientRepo) GetFirstEnabledByProtocol(ctx context.Context, prot } c.Enabled = enabled == 1 c.UseSSL = useSSL == 1 + c.RemoveOnImport = removeOnImport == 1 hydrateClientCredentials(&c) return &c, nil } @@ -266,13 +273,15 @@ func (r *DownloadClientRepo) GetEnabledByProtocol(ctx context.Context, protocol for rows.Next() { var c models.DownloadClient var enabled, useSSL int + var removeOnImport int if err := rows.Scan(&c.ID, &c.Name, &c.Type, &c.Host, &c.Port, &c.APIKey, &useSSL, &c.URLBase, &c.Username, &c.Password, &c.Category, &c.CategoryAudiobook, &c.PathRemap, &c.Priority, - &enabled, &c.CreatedAt, &c.UpdatedAt); err != nil { + &enabled, &removeOnImport, &c.CreatedAt, &c.UpdatedAt); err != nil { return nil, err } c.Enabled = enabled == 1 c.UseSSL = useSSL == 1 + c.RemoveOnImport = removeOnImport == 1 hydrateClientCredentials(&c) clients = append(clients, c) } @@ -283,9 +292,9 @@ func (r *DownloadClientRepo) Create(ctx context.Context, c *models.DownloadClien normalizeClientCredentialStorage(c) now := time.Now().UTC() result, err := r.db.ExecContext(ctx, ` - INSERT INTO download_clients (name, type, host, port, api_key, use_ssl, url_base, username, password, category, category_audiobook, path_remap, priority, enabled, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, - c.Name, c.Type, c.Host, c.Port, c.APIKey, c.UseSSL, c.URLBase, c.Username, c.Password, c.Category, c.CategoryAudiobook, c.PathRemap, c.Priority, c.Enabled, now, now) + INSERT INTO download_clients (name, type, host, port, api_key, use_ssl, url_base, username, password, category, category_audiobook, path_remap, priority, enabled, remove_on_import, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + c.Name, c.Type, c.Host, c.Port, c.APIKey, c.UseSSL, c.URLBase, c.Username, c.Password, c.Category, c.CategoryAudiobook, c.PathRemap, c.Priority, c.Enabled, c.RemoveOnImport, now, now) if err != nil { return fmt.Errorf("create download client: %w", err) } @@ -304,9 +313,9 @@ func (r *DownloadClientRepo) Update(ctx context.Context, c *models.DownloadClien now := time.Now().UTC() _, err := r.db.ExecContext(ctx, ` UPDATE download_clients SET name=?, type=?, host=?, port=?, api_key=?, use_ssl=?, - url_base=?, username=?, password=?, category=?, category_audiobook=?, path_remap=?, priority=?, enabled=?, updated_at=? + url_base=?, username=?, password=?, category=?, category_audiobook=?, path_remap=?, priority=?, enabled=?, remove_on_import=?, updated_at=? WHERE id=?`, - c.Name, c.Type, c.Host, c.Port, c.APIKey, c.UseSSL, c.URLBase, c.Username, c.Password, c.Category, c.CategoryAudiobook, c.PathRemap, c.Priority, c.Enabled, now, c.ID) + c.Name, c.Type, c.Host, c.Port, c.APIKey, c.UseSSL, c.URLBase, c.Username, c.Password, c.Category, c.CategoryAudiobook, c.PathRemap, c.Priority, c.Enabled, c.RemoveOnImport, now, c.ID) return err } diff --git a/internal/db/migrations/090_download_client_remove_on_import.sql b/internal/db/migrations/090_download_client_remove_on_import.sql new file mode 100644 index 000000000..d55d3076a --- /dev/null +++ b/internal/db/migrations/090_download_client_remove_on_import.sql @@ -0,0 +1 @@ +ALTER TABLE download_clients ADD COLUMN remove_on_import INTEGER NOT NULL DEFAULT 0; diff --git a/internal/importer/scanner.go b/internal/importer/scanner.go index 471771023..9dc965a45 100644 --- a/internal/importer/scanner.go +++ b/internal/importer/scanner.go @@ -1422,6 +1422,11 @@ func (s *Scanner) tryImportInternal(ctx context.Context, dl *models.Download, do // Walk: importPending → importing → imported. s.updateDownloadStatus(ctx, dl.ID, models.StateImporting) s.updateDownloadStatus(ctx, dl.ID, models.StateImported) + if cleanupFunc != nil { + if err := cleanupFunc(); err != nil { + slog.Warn("cleanup failed", cleanupWarnAttrs(cleanupClientType, cleanupRemoteID, err)...) + } + } return } // Distinguish "path doesn't exist on this host" from "path exists but has diff --git a/internal/importer/scanner_client_wrappers_test.go b/internal/importer/scanner_client_wrappers_test.go index 0ea3c647d..3f22f6024 100644 --- a/internal/importer/scanner_client_wrappers_test.go +++ b/internal/importer/scanner_client_wrappers_test.go @@ -16,6 +16,7 @@ import ( "path/filepath" "testing" + "github.com/vavallee/bindery/internal/downloader" "github.com/vavallee/bindery/internal/downloader/nzbget" "github.com/vavallee/bindery/internal/downloader/sabnzbd" "github.com/vavallee/bindery/internal/models" @@ -55,8 +56,10 @@ func TestTryImportTransmission_Delegates(t *testing.T) { Title: "Transmission Delegate Test", Status: models.StateCompleted, } - // Transmission wrapper passes nil cleanupFunc; empty dir → fails fast. - s.tryImportTransmission(ctx, dl, t.TempDir(), nil) + client := &models.DownloadClient{Type: "transmission"} + trans := downloader.TransmissionFor(client) + // RemoveOnImport left unset (false) → nil cleanupFunc; empty dir → fails fast. + s.tryImportTransmission(ctx, trans, client, dl, t.TempDir(), nil) } // TestTryImportNZBGet_CleanupCalledOnSuccess verifies that the cleanup closure diff --git a/internal/importer/scanner_poll.go b/internal/importer/scanner_poll.go index c87e5c3f9..592f291e5 100644 --- a/internal/importer/scanner_poll.go +++ b/internal/importer/scanner_poll.go @@ -200,35 +200,35 @@ func transmissionCompletion(status int, percentDone float64) (complete, stopped func (s *Scanner) checkTransmissionDownloads(ctx context.Context, client *models.DownloadClient) { trans := downloader.TransmissionFor(client) - // Poll every category this client may have grabbed under. Audiobook grabs - // use CategoryAudiobook when it is set, ebook grabs use Category, and - // polling only Category leaves audiobook torrents invisible: their - // downloads then hang at "downloading" for good. CategoriesToPoll returns - // both. GetTorrents normalises the path comparison and also accepts - // Transmission 3.0+ labels, so "books" matches both a downloadDir of - // "/data/books/" and a label "books". - // + // Poll every category this client may have grabbed under. Audiobook grabs use + // CategoryAudiobook (when set) while ebook grabs use Category; polling only + // Category leaves audiobook torrents invisible and their downloads hang at + // "downloading" forever. CategoriesToPoll returns both. GetTorrents normalises + // the path comparison and also accepts Transmission 3.0+ labels, so "books" + // matches both a downloadDir of "/data/books/" and a label "books". // Torrents are indexed by info hash, which is stable for the life of the - // torrent. The numeric id is not: Transmission renumbers every torrent when - // the daemon restarts. + // torrent. The session-scoped numeric id is indexed separately and used + // only to reconcile rows grabbed before hashes were persisted: Transmission + // renumbers every torrent when the daemon restarts, so an id stored at grab + // time either matches nothing (the download strands at "downloading" + // forever) or matches whichever unrelated torrent inherited the number. torrentsByHash := make(map[string]transmission.Torrent) - var torrents []transmission.Torrent + var allTorrents []transmission.Torrent seenTorrentIDs := make(map[int64]bool) for _, cat := range downloader.CategoriesToPoll(client) { - found, err := trans.GetTorrents(ctx, cat) + torrents, err := trans.GetTorrents(ctx, cat) if err != nil { // Warn, not Debug: see checkSABnzbdDownloads (#1019 failure mode). slog.Warn("download poll: failed to fetch Transmission torrents — downloads will not be imported", "client", client.Name, "category", cat, "error", err) return } - // The two categories can overlap, so a torrent seen twice is kept once. - for _, t := range found { + for _, t := range torrents { if seenTorrentIDs[t.ID] { continue } seenTorrentIDs[t.ID] = true - torrents = append(torrents, t) + allTorrents = append(allTorrents, t) if hash := strings.ToLower(strings.TrimSpace(t.HashString)); hash != "" { torrentsByHash[hash] = t } @@ -238,7 +238,7 @@ func (s *Scanner) checkTransmissionDownloads(ctx context.Context, client *models // Surface a misconfiguration when the Category filter returns nothing but the // daemon actually holds torrents (#1091). A silent zero-match means every // Bindery grab permanently sits at "downloading" with no indication of why. - if client.Category != "" && len(torrents) == 0 { + if client.Category != "" && len(allTorrents) == 0 { if all, allErr := trans.GetTorrents(ctx, ""); allErr == nil && len(all) > 0 { slog.Warn("transmission: Category filter matched zero torrents — verify Category matches the torrent download directory path or a torrent label", "client", client.Name, "category", client.Category, "category_audiobook", client.CategoryAudiobook, "total_torrents", len(all)) @@ -273,7 +273,7 @@ func (s *Scanner) checkTransmissionDownloads(ctx context.Context, client *models } } - legacyMatches := s.reconcileLegacyTransmissionIDs(ctx, client, allDownloads, torrents, claimedHashes) + legacyMatches := s.reconcileLegacyTransmissionIDs(ctx, client, allDownloads, allTorrents, claimedHashes) for _, dl := range allDownloads { if dl.DownloadClientID == nil || *dl.DownloadClientID != client.ID || dl.TorrentID == nil { @@ -306,7 +306,7 @@ func (s *Scanner) checkTransmissionDownloads(ctx context.Context, client *models bookFiles := s.transmissionFilesFor(ctx, trans, client, torrent) slog.Info("download completed", "title", dl.Title, "path", downloadPath, "files", len(bookFiles)) s.updateDownloadStatus(ctx, dl.ID, models.StateCompleted) - s.tryImportTransmission(ctx, &dl, downloadPath, bookFiles) + s.tryImportTransmission(ctx, trans, client, &dl, downloadPath, bookFiles) } else if isComplete && dl.Status == models.StateImportFailed && dl.ImportRetryCount < importRetryLimit { // Bug #7: retry a previously failed import. downloadPath := s.remapDownloadClientPath(client, torrent.DownloadDir) @@ -320,7 +320,7 @@ func (s *Scanner) checkTransmissionDownloads(ctx context.Context, client *models if err := s.downloads.IncrementImportRetryCount(ctx, dl.ID); err != nil { slog.Warn("failed to increment import retry count", "download_id", dl.ID, "error", err) } - s.tryImportTransmission(ctx, &dl, downloadPath, bookFiles) + s.tryImportTransmission(ctx, trans, client, &dl, downloadPath, bookFiles) } else if isStopped && !isComplete && dl.Status != models.StateFailed { if stopError == "" { // Transmission also reports user-paused torrents as stopped. @@ -622,6 +622,11 @@ func (s *Scanner) checkQbittorrentDownloads(ctx context.Context, client *models. } s.updateDownloadStatus(ctx, dl.ID, models.StateImporting) s.updateDownloadStatus(ctx, dl.ID, models.StateImported) + if cleanup := qbittorrentRemoveOnImportCleanup(ctx, qb, client, &dl); cleanup != nil { + if err := cleanup(); err != nil { + slog.Warn("cleanup failed", cleanupWarnAttrs("qbittorrent", safeRemoteID(dl.TorrentID), err)...) + } + } continue } // Path doesn't exist on disk yet (qBittorrent may sanitise characters @@ -659,7 +664,7 @@ func (s *Scanner) checkQbittorrentDownloads(ctx context.Context, client *models. if dl.Status == models.StateDownloading || dl.Status == models.StateGrabbed { s.updateDownloadStatus(ctx, dl.ID, models.StateCompleted) } - s.tryImportQbittorrent(ctx, &dl, downloadPath, bookFiles) + s.tryImportQbittorrent(ctx, qb, client, &dl, downloadPath, bookFiles) } else if isComplete && dl.Status == models.StateImportFailed && dl.ImportRetryCount < importRetryLimit { // Bug #7: a previous import attempt failed (e.g. transient filesystem // error, path mismatch). The torrent is still seeding so we have the @@ -675,6 +680,11 @@ func (s *Scanner) checkQbittorrentDownloads(ctx context.Context, client *models. s.updateDownloadStatus(ctx, dl.ID, models.StateImportPending) s.updateDownloadStatus(ctx, dl.ID, models.StateImporting) s.updateDownloadStatus(ctx, dl.ID, models.StateImported) + if cleanup := qbittorrentRemoveOnImportCleanup(ctx, qb, client, &dl); cleanup != nil { + if err := cleanup(); err != nil { + slog.Warn("cleanup failed", cleanupWarnAttrs("qbittorrent", safeRemoteID(dl.TorrentID), err)...) + } + } continue } // The files are still not here. Count the miss through the same @@ -697,7 +707,7 @@ func (s *Scanner) checkQbittorrentDownloads(ctx context.Context, client *models. if err := s.downloads.IncrementImportRetryCount(ctx, dl.ID); err != nil { slog.Warn("failed to increment import retry count", "download_id", dl.ID, "error", err) } - s.tryImportQbittorrent(ctx, &dl, downloadPath, bookFiles) + s.tryImportQbittorrent(ctx, qb, client, &dl, downloadPath, bookFiles) } else if isFailed && dl.Status != models.StateFailed { slog.Warn("download failed", "title", dl.Title, "state", torrent.State) s.markDownloadFailed(ctx, &dl, "Torrent failed in qBittorrent") @@ -1004,8 +1014,21 @@ func (s *Scanner) tryImportSABnzbd(ctx context.Context, sab *sabnzbd.Client, dl // of bug where a single-file torrent at a shared download root would cause // every unrelated sibling to be imported. Pass nil to fall back to the // directory walk. -func (s *Scanner) tryImportTransmission(ctx context.Context, dl *models.Download, downloadPath string, explicitFiles []string) { - s.tryImportInternal(ctx, dl, downloadPath, "transmission", safeRemoteID(dl.TorrentID), "", nil, explicitFiles) +func (s *Scanner) tryImportTransmission(ctx context.Context, trans *transmission.Client, client *models.DownloadClient, dl *models.Download, downloadPath string, explicitFiles []string) { + var cleanup func() error + if client.RemoveOnImport && dl.TorrentID != nil && strings.TrimSpace(*dl.TorrentID) != "" { + ref := strings.TrimSpace(*dl.TorrentID) + cleanup = func() error { + slog.Info("removing torrent from Transmission after import", "torrent", ref, "title", dl.Title) + // deleteFiles=false: the payload has been imported (hardlinked or + // copied) but the torrent may still be seeding from those files. + if err := downloader.RemoveTransmissionTorrent(ctx, trans, ref, false); err != nil { + return fmt.Errorf("remove torrent %s: %w", ref, err) + } + return nil + } + } + s.tryImportInternal(ctx, dl, downloadPath, "transmission", safeRemoteID(dl.TorrentID), "", cleanup, explicitFiles) } // legacyTransmissionMatchWindow is how far apart Bindery's grab timestamp and @@ -1163,8 +1186,28 @@ func normaliseReleaseName(s string) string { // tryImportQbittorrent attempts to import a completed qBittorrent download. See // tryImportTransmission for the semantics of explicitFiles. -func (s *Scanner) tryImportQbittorrent(ctx context.Context, dl *models.Download, downloadPath string, explicitFiles []string) { - s.tryImportInternal(ctx, dl, downloadPath, "qbittorrent", safeRemoteID(dl.TorrentID), "", nil, explicitFiles) +func (s *Scanner) tryImportQbittorrent(ctx context.Context, qb *qbittorrent.Client, client *models.DownloadClient, dl *models.Download, downloadPath string, explicitFiles []string) { + cleanup := qbittorrentRemoveOnImportCleanup(ctx, qb, client, dl) + s.tryImportInternal(ctx, dl, downloadPath, "qbittorrent", safeRemoteID(dl.TorrentID), "", cleanup, explicitFiles) +} + +// qbittorrentRemoveOnImportCleanup builds the post-import removal callback +// shared by tryImportQbittorrent and the "book already in library" shortcuts +// in checkQbittorrentDownloads (issue #2046) — those shortcuts close out a +// download without ever calling tryImportInternal, so they need the same +// cleanup construction rather than only the normal import path getting it. +func qbittorrentRemoveOnImportCleanup(ctx context.Context, qb *qbittorrent.Client, client *models.DownloadClient, dl *models.Download) func() error { + if !client.RemoveOnImport || dl.TorrentID == nil { + return nil + } + hash := *dl.TorrentID + return func() error { + slog.Info("removing torrent from qBittorrent after import", "hash", hash, "title", dl.Title) + if err := qb.DeleteTorrent(ctx, hash, false); err != nil { + return fmt.Errorf("remove torrent %s: %w", hash, err) + } + return nil + } } // torrentFile is the minimal shape resolveTorrentFiles consumes; it matches diff --git a/internal/models/download.go b/internal/models/download.go index 7f295a537..101e933db 100644 --- a/internal/models/download.go +++ b/internal/models/download.go @@ -20,6 +20,7 @@ type DownloadClient struct { PathRemap string `json:"pathRemap"` Priority int `json:"priority"` Enabled bool `json:"enabled"` + RemoveOnImport bool `json:"removeOnImport"` CreatedAt time.Time `json:"createdAt"` UpdatedAt time.Time `json:"updatedAt"` Health *DownloadClientHealth `json:"health,omitempty"` diff --git a/web/src/api/downloadclients.ts b/web/src/api/downloadclients.ts index a57aec7cc..a04afa74c 100644 --- a/web/src/api/downloadclients.ts +++ b/web/src/api/downloadclients.ts @@ -22,6 +22,10 @@ export interface DownloadClient { // fall back to `category`. categoryAudiobook?: string pathRemap?: string + // removeOnImport removes the torrent from the client once Bindery has + // imported it (the data is left on disk). Torrent clients only: usenet + // clients always clear their own history entry on import. + removeOnImport?: boolean enabled: boolean health?: DownloadClientHealth } diff --git a/web/src/i18n/locales/en.json b/web/src/i18n/locales/en.json index 776a1ded7..dca4e00d8 100644 --- a/web/src/i18n/locales/en.json +++ b/web/src/i18n/locales/en.json @@ -855,6 +855,8 @@ "pathNotVisible": "Connected, but Bindery can't read the client's completed-downloads folder. Configure a path remap or a shared mount so both point at the same storage, otherwise downloads won't import.", "loopbackHint": "The host is localhost — if Bindery runs in Docker, localhost is the Bindery container itself, not the machine. Use the host's LAN IP or the client's container/service name instead.", "rtorrentCategoryHelp": "Stored as the ruTorrent label (d.custom1) and used to filter which torrents Bindery polls. Leave blank to poll every torrent in the client.", + "removeOnImportLabel": "Remove torrent after import", + "removeOnImportHelp": "When Bindery imports a download from this client, remove the torrent from it. The downloaded files are left on disk. Off by default: on a private tracker this stops the torrent seeding, which can cost you ratio.", "rtorrentUrlBaseHelp": "Full path of rTorrent's XML-RPC endpoint, not a prefix. Usually /RPC2; ruTorrent installs often expose /plugins/rpc/rpc.php instead. Leave blank to use /RPC2. For a plain rTorrent with no web server in front of it, use rTorrent's own SCGI listener: scgi:// for the host and port above, scgi://host:port, or scgi:///path/to/socket. SCGI carries no credentials, so keep it off-network.", "rtorrentScgiIgnoredHint": "{{fields}} will be ignored — rTorrent's SCGI listener has no TLS and no authentication. Put rTorrent behind a web server and use an HTTP path instead if you need either.", "passwordEditLabel": "Password (leave blank to keep current)", diff --git a/web/src/pages/SettingsPage.test.tsx b/web/src/pages/SettingsPage.test.tsx index cc8652d22..cff30686a 100644 --- a/web/src/pages/SettingsPage.test.tsx +++ b/web/src/pages/SettingsPage.test.tsx @@ -1780,6 +1780,9 @@ describe('SettingsPage', () => { enabled: true, useSsl: false, urlBase: '', + // Torrent clients carry the remove-on-import toggle, off by default. + // A usenet client has no torrent to remove, so the form omits it. + ...(type === 'nzbget' ? {} : { removeOnImport: false }), }) }) }) @@ -1819,6 +1822,9 @@ describe('SettingsPage', () => { category: 'ebooks', categoryAudiobook: '', pathRemap: '/media:/books', + // The type switched to a torrent client, so the toggle is now part of + // the payload; the form leaves it off unless the user ticks it. + removeOnImport: false, useSsl: true, urlBase: '/qbittorrent', }) diff --git a/web/src/pages/settings/ClientsTab.test.tsx b/web/src/pages/settings/ClientsTab.test.tsx index 52aa29dfc..167cb150b 100644 --- a/web/src/pages/settings/ClientsTab.test.tsx +++ b/web/src/pages/settings/ClientsTab.test.tsx @@ -256,3 +256,44 @@ describe('download client diagnose', () => { expect(await screen.findByRole('alert')).toHaveTextContent('settings.clients.diagnose.failed') }) }) + +describe('download client remove-on-import toggle', () => { + it('is hidden for a usenet client, which always clears its own history', () => { + renderTab([makeClient({ type: 'sabnzbd' })]) + openEditForm() + + expect(screen.queryByLabelText('settings.clients.removeOnImportLabel')).not.toBeInTheDocument() + }) + + it('is shown for a torrent client and reflects the stored value', () => { + renderTab([makeClient({ type: 'transmission', removeOnImport: true })]) + openEditForm() + + const toggle = screen.getByLabelText('settings.clients.removeOnImportLabel') as HTMLInputElement + expect(toggle.checked).toBe(true) + }) + + it('defaults to off and sends the change on save', async () => { + renderTab([makeClient({ type: 'transmission' })]) + openEditForm() + + const toggle = screen.getByLabelText('settings.clients.removeOnImportLabel') as HTMLInputElement + expect(toggle.checked).toBe(false) + fireEvent.click(toggle) + save() + + await waitFor(() => expect(api.updateDownloadClient).toHaveBeenCalled()) + const [, payload] = vi.mocked(api.updateDownloadClient).mock.calls[0] + expect(payload.removeOnImport).toBe(true) + }) + + it('never sends it enabled for a usenet client', async () => { + renderTab([makeClient({ type: 'sabnzbd', removeOnImport: true })]) + openEditForm() + save() + + await waitFor(() => expect(api.updateDownloadClient).toHaveBeenCalled()) + const [, payload] = vi.mocked(api.updateDownloadClient).mock.calls[0] + expect(payload.removeOnImport).toBe(false) + }) +}) diff --git a/web/src/pages/settings/ClientsTab.tsx b/web/src/pages/settings/ClientsTab.tsx index e3ada7d9d..8b257891f 100644 --- a/web/src/pages/settings/ClientsTab.tsx +++ b/web/src/pages/settings/ClientsTab.tsx @@ -210,6 +210,7 @@ function EditClientForm({ client, onClose, onSaved }: { client: DownloadClient; const [saving, setSaving] = useState(false) const [saveError, setSaveError] = useState(null) const [pathRemap, setPathRemap] = useState(client.pathRemap || '') + const [removeOnImport, setRemoveOnImport] = useState(client.removeOnImport || false) const [testing, setTesting] = useState(false) const [testResult, setTestResult] = useState<{ ok: boolean; msg: string; warn?: string } | null>(null) const labelCls = 'block text-xs text-slate-600 dark:text-zinc-400 mb-1' @@ -256,6 +257,7 @@ function EditClientForm({ client, onClose, onSaved }: { client: DownloadClient; category, categoryAudiobook: categoryAudiobook.trim(), pathRemap: pathRemap.trim(), + removeOnImport: isTorrentClient(type) ? removeOnImport : false, useSsl: useSSL, urlBase: urlBase.trim(), } @@ -394,6 +396,21 @@ function EditClientForm({ client, onClose, onSaved }: { client: DownloadClient; placeholder={type === 'qbittorrent' ? '/downloads:/media/books' : '/media:/books'} help={downloadClientPathRemapHelp(type)} /> + {isTorrentClient(type) && ( +
+
+ setRemoveOnImport(e.target.checked)} + className="rounded border-slate-300 dark:border-zinc-700" + /> + +
+

{t('settings.clients.removeOnImportHelp')}

+
+ )} {saveError &&

{saveError}

} {testResult && (
@@ -416,6 +433,11 @@ function EditClientForm({ client, onClose, onSaved }: { client: DownloadClient; ) } +// Torrent clients only: a usenet client always clears its own history entry +// once Bindery has imported the job, so there is nothing for the toggle to +// control there. Mirrors downloader.IsTorrentClient on the server. +const isTorrentClient = (t: string) => t === 'qbittorrent' || t === 'transmission' || t === 'deluge' || t === 'rtorrent' + function AddClientForm({ onClose, onAdded }: { onClose: () => void; onAdded: (c: DownloadClient) => void }) { const { t } = useTranslation() const [name, setName] = useState('SABnzbd') @@ -431,6 +453,7 @@ function AddClientForm({ onClose, onAdded }: { onClose: () => void; onAdded: (c: const [saving, setSaving] = useState(false) const [saveError, setSaveError] = useState(null) const [pathRemap, setPathRemap] = useState('') + const [removeOnImport, setRemoveOnImport] = useState(false) const [testing, setTesting] = useState(false) const [testResult, setTestResult] = useState<{ ok: boolean; msg: string; warn?: string } | null>(null) const labelCls = 'block text-xs text-slate-600 dark:text-zinc-400 mb-1' @@ -477,9 +500,15 @@ function AddClientForm({ onClose, onAdded }: { onClose: () => void; onAdded: (c: setPort('8080') } - const buildData = () => isPasswordClient(type) - ? { name, host, port: parseInt(port), username: hasUsername(type) ? username : '', password: credential, apiKey: '', category, categoryAudiobook: categoryAudiobook.trim(), pathRemap: pathRemap.trim(), type, enabled: true, useSsl: useSSL, urlBase: urlBase.trim() } - : { name, host, port: parseInt(port), apiKey: credential, username: '', password: '', category, categoryAudiobook: categoryAudiobook.trim(), pathRemap: pathRemap.trim(), type, enabled: true, useSsl: useSSL, urlBase: urlBase.trim() } + // removeOnImport is only sent for a torrent client: a usenet client has no + // torrent to remove, and a new row defaults to off anyway, so sending it + // there would be noise on the wire. + const buildData = () => ({ + ...(isPasswordClient(type) + ? { name, host, port: parseInt(port), username: hasUsername(type) ? username : '', password: credential, apiKey: '', category, categoryAudiobook: categoryAudiobook.trim(), pathRemap: pathRemap.trim(), type, enabled: true, useSsl: useSSL, urlBase: urlBase.trim() } + : { name, host, port: parseInt(port), apiKey: credential, username: '', password: '', category, categoryAudiobook: categoryAudiobook.trim(), pathRemap: pathRemap.trim(), type, enabled: true, useSsl: useSSL, urlBase: urlBase.trim() }), + ...(isTorrentClient(type) ? { removeOnImport } : {}), + }) const submit = async () => { const data = buildData() @@ -594,6 +623,21 @@ function AddClientForm({ onClose, onAdded }: { onClose: () => void; onAdded: (c: placeholder={type === 'qbittorrent' ? '/downloads:/media/books' : '/media:/books'} help={downloadClientPathRemapHelp(type)} /> + {isTorrentClient(type) && ( +
+
+ setRemoveOnImport(e.target.checked)} + className="rounded border-slate-300 dark:border-zinc-700" + /> + +
+

{t('settings.clients.removeOnImportHelp')}

+
+ )} {saveError &&

{saveError}

} {testResult && (