From b07b6b560701045a61fdf55430258cdf68c849fd Mon Sep 17 00:00:00 2001 From: vavallee Date: Mon, 5 Oct 2026 00:19:24 -0300 Subject: [PATCH 1/3] fix(downloader): apply the global remap to sent save paths and lowercase Deluge labels (#2665) The save path sent to qBittorrent (no category) and rTorrent only ran Bindery's download folder back through the client's own PathRemap, so with just BINDERY_DOWNLOAD_PATH_REMAP set the client was told to save into Bindery's container folder. torrentSavePath now goes through ClientSidePath, the inverse of RemapClientPath with the same precedence the importer, health check and rTorrent removal use: client remap first, the global remap when that leaves the path unchanged. The queue grab and the scheduler pass the global remap in, and the diagnose action's warning about the old behaviour is gone because the case no longer exists. Deluge's Label plugin stores labels lowercased but set_torrent and get_options compare the id exactly, so a category typed as "Books" was rejected as Unknown Label and the error discarded. Bindery now sends deluge.LabelID (lowercase) when labelling and when reading label options, logs a WARN when Deluge still refuses a label, and the diagnose category check lists Deluge's labels and compares them in that form. Deluge downloads are matched by hash, never by label, so nothing else changes. Signed-off-by: vavallee Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 --- .../2665-save-path-remap-deluge-label.md | 3 + docs/API.md | 5 +- docs/DEPLOYMENT.md | 2 + internal/api/download_client_diagnose.go | 40 +++--- .../api/download_client_diagnose_grab_test.go | 120 ++++++++++++++---- internal/api/queue.go | 12 +- internal/api/queue_grab_remap_test.go | 76 +++++++++++ internal/downloader/adapter.go | 14 +- internal/downloader/adapter_test.go | 73 +++++++++++ internal/downloader/clientpath.go | 34 +++-- internal/downloader/deluge/client.go | 16 ++- internal/downloader/deluge/client_test.go | 42 ++++++ internal/downloader/deluge/config.go | 37 ++++-- internal/downloader/deluge/config_test.go | 42 ++++++ internal/downloader/deluge/label_test.go | 84 ++++++++++++ internal/downloader/pathcheck.go | 15 +++ internal/downloader/rtorrent_test.go | 20 +++ internal/scheduler/scheduler.go | 6 +- .../scheduler/scheduler_grab_remap_test.go | 74 +++++++++++ 19 files changed, 637 insertions(+), 78 deletions(-) create mode 100644 changelog.d/2665-save-path-remap-deluge-label.md create mode 100644 internal/api/queue_grab_remap_test.go create mode 100644 internal/downloader/deluge/config_test.go create mode 100644 internal/downloader/deluge/label_test.go create mode 100644 internal/scheduler/scheduler_grab_remap_test.go diff --git a/changelog.d/2665-save-path-remap-deluge-label.md b/changelog.d/2665-save-path-remap-deluge-label.md new file mode 100644 index 000000000..ecb72eead --- /dev/null +++ b/changelog.d/2665-save-path-remap-deluge-label.md @@ -0,0 +1,3 @@ +### Fixed +- **The global path remap now applies to the save path Bindery sends** (#2665). With only `BINDERY_DOWNLOAD_PATH_REMAP` set, rTorrent and qBittorrent (when the client has no category) were told to save into Bindery's own folder, such as `/downloads`, which may not exist on the client's side. The save path now goes through the same remaps Bindery reads paths back with: the client's own path remap first, then the global one. +- **Deluge categories with capital letters now label the torrent** (#2665). Deluge stores labels in lowercase and rejects any other spelling, so a category typed as `Books` left grabs unlabelled and any move path on the label was skipped. Bindery now sends the label lowercased, logs a warning when Deluge still refuses a label (for example when the Label plugin is off or the label does not exist), and Diagnose checks your Deluge categories against the labels Deluge has. diff --git a/docs/API.md b/docs/API.md index dee8d301b..5c7eb2bd1 100644 --- a/docs/API.md +++ b/docs/API.md @@ -601,9 +601,10 @@ only, never a path, and it runs only when called. * `code` is stable: `config`, `connect`, `category`, `client_path`, `remap`, `local_path`, `hardlinks`, `indexer_reach`. `message` and `fix` are English sentences. * `status` is `pass`, `warn`, `fail`, `skipped` or `unknown`. Every check after a `fail` is `skipped` without running, except `indexer_reach`, which is always `unknown` because Bindery cannot test the client's own route to indexers. -* The folder checked is where a grab actually lands, worked out the way the grab itself is sent: the save path Bindery sends (rTorrent always, qBittorrent without a category, Transmission with an absolute category), the category save path (qBittorrent, with an empty one meaning the default save path plus the category name), the category folder (SABnzbd), the category DestDir or DestDir plus the category name when `AppendCategoryDir` is on (NZBGet), a Deluge label's move completed path, or the client default. `source` names which. The category is used exactly as a grab sends it: a category with spaces around it, or a Deluge category with capital letters (the Label plugin only accepts lowercase labels, so such grabs are not labelled), gives `client_path: warn`. +* The folder checked is where a grab actually lands, worked out the way the grab itself is sent: the save path Bindery sends (rTorrent always, qBittorrent without a category, Transmission with an absolute category), the category save path (qBittorrent, with an empty one meaning the default save path plus the category name), the category folder (SABnzbd), the category DestDir or DestDir plus the category name when `AppendCategoryDir` is on (NZBGet), a Deluge label's move completed path, or the client default. `source` names which. The category is used exactly as a grab sends it: a category with spaces around it gives `client_path: warn`. A Deluge category is sent lowercased, because the Label plugin stores labels in lowercase and only accepts that form when labelling a torrent, so `Books` uses the `books` label's move completed path. +* For Deluge, `category` compares the configured categories, lowercased, with the labels the Label plugin lists, and says when it lowercases one. When the plugin is off it cannot list labels and `category` is `unknown`. * Ebook and audiobook grabs are checked separately, because each resolves its own category and download folder. When both land in the same folder there is one `paths` row and the `client_path`, `remap` and `local_path` checks have no `mediaType`; otherwise each carries `mediaType` `ebook` or `audiobook`. -* `remapRule` is `client` (this client's path remap changed the path), `global` (`BINDERY_DOWNLOAD_PATH_REMAP` did) or `none`. A Windows drive path fails for a missing remap only when Bindery itself is not running on Windows. When no remap applies and the client's folder is not under any folder Bindery uses, `remap` is `warn` and `local_path` carries the failure and the fix. When Bindery sends the save path itself (rTorrent, qBittorrent without a category) and only the global remap is set, `remap` is `warn`: sending does not apply the global remap, so the client is given Bindery's own folder path and the round trip proves nothing. +* `remapRule` is `client` (this client's path remap changed the path), `global` (`BINDERY_DOWNLOAD_PATH_REMAP` did) or `none`. A Windows drive path fails for a missing remap only when Bindery itself is not running on Windows. When no remap applies and the client's folder is not under any folder Bindery uses, `remap` is `warn` and `local_path` carries the failure and the fix. When Bindery sends the save path itself (rTorrent, qBittorrent without a category) it runs its download folder through the same remaps in reverse, the client's own first and `BINDERY_DOWNLOAD_PATH_REMAP` as the fallback, so `clientPath` is the folder the client is actually given. * `hardlinks` has one row per download folder and library root pair, `{mediaType, downloadPath, root, result, linkable, reason}`. Every pair gets a real link probe, because two bind mounts of one filesystem share a device ID yet refuse links across them. `result` is `yes`, `no`, `unknown` (Bindery could not write a test file in the download folder) or `missing` (the library folder does not exist and was not probed). * `primaryFix` is the fix of the first failure, or of the first warning when nothing failed. diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 578ea0081..bd3640ffb 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -370,6 +370,8 @@ When Bindery and your download client run in **separate containers**, they typic Set a download-client path remap in **Settings → Download clients** or set the global `BINDERY_DOWNLOAD_PATH_REMAP` fallback to a comma-separated list of `from:to` pairs. Bindery applies a longest-prefix match to every path the download client reports, replacing the matched prefix before it tries to access the file. A per-client remap takes precedence when it matches the reported path; the global env var still applies as a fallback. +The same remaps also run in reverse, with the same precedence, when Bindery tells a client where to save a grab. rTorrent always gets a download directory, and qBittorrent gets a save path when the client has no category set. That directory is Bindery's download folder (`BINDERY_DOWNLOAD_DIR`, or `BINDERY_AUDIOBOOK_DOWNLOAD_DIR` for audiobooks) translated into the client's path: by the client's own remap when it covers that folder, otherwise by `BINDERY_DOWNLOAD_PATH_REMAP`. With `BINDERY_DOWNLOAD_PATH_REMAP=/data:/downloads` and no client remap, rTorrent is told to save into `/data`. Bindery 1.40.0 and earlier applied only the client's own remap here, so with just the global remap set the client was sent Bindery's `/downloads`. + Per-client remaps are stored on each download client, so separate qBittorrent / SABnzbd / NZBGet instances can map different mount points. Existing download clients keep an empty remap after upgrade, which preserves the previous global-only behavior until you add a client-specific value. For a per-client remap, open **Settings → Download clients**, edit the client, and set **Download client path remap**. To see what a remap actually resolves to, press **Diagnose** on the client: it reports where that client says ebook and audiobook grabs land, which remap applies, and whether Bindery can read the result. The left side is the path the client reports; the right side is the path Bindery can read. For qBittorrent this normally means mapping the qBittorrent category save path or torrent content path to Bindery's download mount. Example: if qBittorrent reports `/downloads/books/My.Book` and Bindery sees that same folder as `/media/books/My.Book`, set `/downloads:/media/books`. diff --git a/internal/api/download_client_diagnose.go b/internal/api/download_client_diagnose.go index edc2bf008..28d5e81b5 100644 --- a/internal/api/download_client_diagnose.go +++ b/internal/api/download_client_diagnose.go @@ -10,12 +10,14 @@ import ( "os" "path/filepath" "runtime" + "slices" "sort" "strings" "time" "github.com/vavallee/bindery/internal/config" "github.com/vavallee/bindery/internal/downloader" + "github.com/vavallee/bindery/internal/downloader/deluge" "github.com/vavallee/bindery/internal/httpsec" "github.com/vavallee/bindery/internal/models" "github.com/vavallee/bindery/internal/pathmap" @@ -104,10 +106,9 @@ type diagnoseResponse struct { // diagTarget is one folder grabs land in: ebook, audiobook, or both when // they resolve to the same client folder. type diagTarget struct { - row diagPathRow - sentByBindery bool // the client path is the save path Bindery sends - probePath string // symlink-resolved local path, set once readable - stopped bool // an earlier per folder check had nothing to pass on + row diagPathRow + probePath string // symlink-resolved local path, set once readable + stopped bool // an earlier per folder check had nothing to pass on } // diagState is what the checks share. Each check reads what earlier checks @@ -429,7 +430,20 @@ func checkDiagCategory(ctx context.Context, st *diagState) []diagCheckResult { Fix: fmt.Sprintf("Create the category %s in %s, or change this client's category in Bindery to one that exists.", quoteJoin(report.Missing), st.clientName), }) } - return one(diagCheckResult{Status: diagPass, Message: fmt.Sprintf("%s has the category %s.", st.clientName, quoteJoin(report.Wanted))}) + res := diagCheckResult{Status: diagPass, Message: fmt.Sprintf("%s has the category %s.", st.clientName, quoteJoin(report.Wanted))} + if st.client.Type == "deluge" { + // Grabs lowercase the label (#2665), so a capital is not a problem, + // but say so, because the label in Deluge will not look like the + // category typed here. + var noted []string + for _, c := range []string{st.client.Category, st.client.CategoryAudiobook} { + if id := deluge.LabelID(c); id != c && !slices.Contains(noted, c) { + noted = append(noted, c) + res.Message += fmt.Sprintf(" Deluge keeps labels in lowercase, so Bindery sends the category %q as %q.", c, id) + } + } + } + return one(res) } // checkDiagClientPath works out where ebook and audiobook grabs land, using @@ -440,7 +454,7 @@ func checkDiagClientPath(ctx context.Context, st *diagState) []diagCheckResult { return downloader.ResolveCategory(st.client, mediaType) + "\x00" + downloader.TargetDownloadDir(mediaType, st.downloadDir, st.audiobookDownloadDir) } - ebook, ebookErr := downloader.GrabSavePath(ctx, st.client, models.MediaTypeEbook, st.downloadDir, st.audiobookDownloadDir) + ebook, ebookErr := downloader.GrabSavePath(ctx, st.client, models.MediaTypeEbook, st.downloadDir, st.audiobookDownloadDir, st.globalRemap) type resolved struct { mediaType string info downloader.ClientPathInfo @@ -450,7 +464,7 @@ func checkDiagClientPath(ctx context.Context, st *diagState) []diagCheckResult { if grabKey(models.MediaTypeEbook) == grabKey(models.MediaTypeAudiobook) { found = []resolved{{"", ebook, ebookErr}} } else { - audio, audioErr := downloader.GrabSavePath(ctx, st.client, models.MediaTypeAudiobook, st.downloadDir, st.audiobookDownloadDir) + audio, audioErr := downloader.GrabSavePath(ctx, st.client, models.MediaTypeAudiobook, st.downloadDir, st.audiobookDownloadDir, st.globalRemap) if ebookErr == nil && audioErr == nil && ebook.Path == audio.Path && ebook.Source == audio.Source && ebook.Note == audio.Note { found = []resolved{{"", ebook, nil}} } else { @@ -460,7 +474,7 @@ func checkDiagClientPath(ctx context.Context, st *diagState) []diagCheckResult { out := make([]diagCheckResult, 0, len(found)) for _, f := range found { - t := &diagTarget{row: diagPathRow{MediaType: f.mediaType, ClientPath: f.info.Path, Source: f.info.Source}, sentByBindery: f.info.SentByBindery} + t := &diagTarget{row: diagPathRow{MediaType: f.mediaType, ClientPath: f.info.Path, Source: f.info.Source}} st.targets = append(st.targets, t) res := describeClientPath(st, f.mediaType, f.info, f.err) res.MediaType = f.mediaType @@ -570,16 +584,6 @@ func remapTarget(st *diagState, t *diagTarget, global *pathmap.Remapper) diagChe Fix: "Make both sides of the path remap absolute paths.", } } - // torrentSavePath inverts only the client's own remap when sending, so - // with only the global remap set the client is sent Bindery's own folder, - // and remapping it back proves nothing about the client's side. - if t.sentByBindery && strings.TrimSpace(st.client.PathRemap) == "" && !global.Empty() { - return diagCheckResult{ - Status: diagWarn, - Message: fmt.Sprintf("Bindery sends its own folder %q to %s, because BINDERY_DOWNLOAD_PATH_REMAP is not applied when sending. This check can only confirm Bindery reads its own folder, not that %s writes there.", raw, st.clientName, st.clientName), - Fix: fmt.Sprintf("Set a path remap on this client, so the folder Bindery sends is translated into the path %s uses for the same storage.", st.clientName), - } - } switch rule { case downloader.RemapRuleClient: return diagCheckResult{Status: diagPass, Message: fmt.Sprintf("This client's path remap turns %q into %q.", raw, local)} diff --git a/internal/api/download_client_diagnose_grab_test.go b/internal/api/download_client_diagnose_grab_test.go index 345741df2..994a4e712 100644 --- a/internal/api/download_client_diagnose_grab_test.go +++ b/internal/api/download_client_diagnose_grab_test.go @@ -285,11 +285,21 @@ func TestDiagnose_SlowFilesystemAnswersUnknown(t *testing.T) { } func delugeLabelServer(t *testing.T, downloads string, labelOptions string) (string, int) { + t.Helper() + return delugeLabelServerWithLabels(t, downloads, labelOptions, "") +} + +// delugeLabelServerWithLabels is delugeLabelServer whose label.get_labels +// answers labels (a JSON list); "" leaves the method unanswered, as with the +// plugin off. Like the real plugin, label.get_options knows labels only by +// their lowercase id. +func delugeLabelServerWithLabels(t *testing.T, _ string, labelOptions, labels string) (string, int) { t.Helper() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var req struct { - Method string `json:"method"` - ID int64 `json:"id"` + Method string `json:"method"` + Params []json.RawMessage `json:"params"` + ID int64 `json:"id"` } _ = json.NewDecoder(r.Body).Decode(&req) switch req.Method { @@ -297,11 +307,25 @@ func delugeLabelServer(t *testing.T, downloads string, labelOptions string) (str fmt.Fprintf(w, `{"result":true,"error":null,"id":%d}`, req.ID) case "core.get_config_values": fmt.Fprintf(w, `{"result":{"download_location":"/incomplete","move_completed":false,"move_completed_path":""},"error":null,"id":%d}`, req.ID) + case "label.get_labels": + if labels == "" { + fmt.Fprintf(w, `{"result":null,"error":{"code":2,"message":"Unknown method"},"id":%d}`, req.ID) + return + } + fmt.Fprintf(w, `{"result":%s,"error":null,"id":%d}`, labels, req.ID) case "label.get_options": if labelOptions == "" { fmt.Fprintf(w, `{"result":null,"error":{"code":2,"message":"Unknown method"},"id":%d}`, req.ID) return } + var label string + if len(req.Params) > 0 { + _ = json.Unmarshal(req.Params[0], &label) + } + if label != strings.ToLower(label) { + fmt.Fprintf(w, `{"result":null,"error":{"code":2,"message":"KeyError: '%s'"},"id":%d}`, label, req.ID) + return + } fmt.Fprintf(w, `{"result":%s,"error":null,"id":%d}`, labelOptions, req.ID) default: fmt.Fprintf(w, `{"result":null,"error":null,"id":%d}`, req.ID) @@ -327,20 +351,16 @@ func TestDiagnose_DelugeLabelMovePath(t *testing.T) { } }) - // The Label plugin rejects a label with capitals and the grab ignores - // that error, so the torrent lands in the global folder. The doctor must - // say so rather than report the lowercase label's folder. - t.Run("capital letters are not labelled", func(t *testing.T) { + // Grabs send a category with capitals as the lowercase label Deluge + // stores (#2665), so the label's move path applies to it as well. + t.Run("capital letters reach the lowercase label", func(t *testing.T) { host, port := delugeLabelServer(t, downloads, string(applied)) resp, _ := runDiagnose(t, diagnoseSetup{downloadDir: downloads}, &models.DownloadClient{ Name: "Deluge", Type: "deluge", Host: host, Port: port, Password: "deluge", Category: "Books", Enabled: true, }) - path := wantDiagStatus(t, resp, diagCodeClientPath, diagWarn) - if resp.Paths[0].ClientPath != "/incomplete" { - t.Errorf("clientPath = %q, want the global folder the unlabelled grab lands in", resp.Paths[0].ClientPath) - } - if !strings.Contains(path.Message, "only accepts lowercase labels") || path.Fix != "Use a lowercase category for this client in Bindery." { - t.Errorf("check = %+v", path) + path := wantDiagStatus(t, resp, diagCodeClientPath, diagPass) + if resp.Paths[0].ClientPath != downloads || !strings.Contains(path.Message, `label "books"`) { + t.Errorf("paths = %+v, message = %q", resp.Paths, path.Message) } }) @@ -368,29 +388,83 @@ func TestDiagnose_DelugeLabelMovePath(t *testing.T) { }) } -// TestDiagnose_GlobalRemapOnlyWarnsForSentSavePath: torrentSavePath inverts -// only the client's own remap, so with just BINDERY_DOWNLOAD_PATH_REMAP the -// client is sent Bindery's own folder and the round trip back proves nothing. -func TestDiagnose_GlobalRemapOnlyWarnsForSentSavePath(t *testing.T) { +// TestDiagnose_DelugeLabelCategory: with the Label plugin on, the category +// check compares the configured category with Deluge's labels the way grabs +// send it, lowercased (#2665), and says when it lowercases. +func TestDiagnose_DelugeLabelCategory(t *testing.T) { + defer httpsec.AllowLoopbackForTests()() + downloads := t.TempDir() + deluge := func(host string, port int, category string) *models.DownloadClient { + return &models.DownloadClient{Name: "Deluge", Type: "deluge", Host: host, Port: port, Password: "deluge", Category: category, Enabled: true} + } + + t.Run("a capitalised category matches its lowercase label", func(t *testing.T) { + host, port := delugeLabelServerWithLabels(t, downloads, "", `["books","tv"]`) + resp, _ := runDiagnose(t, diagnoseSetup{downloadDir: downloads}, deluge(host, port, "Books")) + cat := wantDiagStatus(t, resp, diagCodeCategory, diagPass) + if !strings.Contains(cat.Message, `has the category "books"`) || !strings.Contains(cat.Message, `sends the category "Books" as "books"`) { + t.Errorf("message = %q", cat.Message) + } + }) + + t.Run("a lowercase category says nothing about case", func(t *testing.T) { + host, port := delugeLabelServerWithLabels(t, downloads, "", `["books"]`) + resp, _ := runDiagnose(t, diagnoseSetup{downloadDir: downloads}, deluge(host, port, "books")) + cat := wantDiagStatus(t, resp, diagCodeCategory, diagPass) + if strings.Contains(cat.Message, "lowercase") { + t.Errorf("message = %q", cat.Message) + } + }) + + t.Run("a missing label fails", func(t *testing.T) { + host, port := delugeLabelServerWithLabels(t, downloads, "", `["tv"]`) + resp, _ := runDiagnose(t, diagnoseSetup{downloadDir: downloads}, deluge(host, port, "Books")) + cat := wantDiagStatus(t, resp, diagCodeCategory, diagFail) + if !strings.Contains(cat.Message, `no category "books"`) || !strings.Contains(cat.Message, `"tv"`) { + t.Errorf("message = %q", cat.Message) + } + }) + + t.Run("plugin off stays unknown", func(t *testing.T) { + host, port := delugeLabelServerWithLabels(t, downloads, "", "") + resp, _ := runDiagnose(t, diagnoseSetup{downloadDir: downloads}, deluge(host, port, "Books")) + cat := wantDiagStatus(t, resp, diagCodeCategory, diagUnknown) + if !strings.Contains(cat.Message, "Label plugin") { + t.Errorf("message = %q", cat.Message) + } + }) +} + +// TestDiagnose_GlobalRemapAppliesToSentSavePath: the save path a grab sends +// falls back to BINDERY_DOWNLOAD_PATH_REMAP when the client has no remap of +// its own (#2665), so the doctor reports the client's folder and the global +// remap reading it back, not Bindery's own folder. +func TestDiagnose_GlobalRemapAppliesToSentSavePath(t *testing.T) { defer httpsec.AllowLoopbackForTests()() downloads := t.TempDir() t.Run("qbittorrent without a category", func(t *testing.T) { host, port := qbitGrabServer(t, map[string]string{}, "/default") resp, _ := runDiagnose(t, diagnoseSetup{downloadDir: downloads, remap: "/qbit/downloads:" + downloads}, qbitClient(host, port, "", "")) - remap := wantDiagStatus(t, resp, diagCodeRemap, diagWarn) - if !strings.Contains(remap.Message, "BINDERY_DOWNLOAD_PATH_REMAP is not applied") { + remap := wantDiagStatus(t, resp, diagCodeRemap, diagPass) + if !strings.Contains(remap.Message, "BINDERY_DOWNLOAD_PATH_REMAP") { t.Errorf("message = %q", remap.Message) } - if resp.Paths[0].ClientPath != downloads { - t.Errorf("clientPath = %q, want Bindery's own folder, as the grab sends it", resp.Paths[0].ClientPath) + if resp.Paths[0].ClientPath != "/qbit/downloads" { + t.Errorf("clientPath = %q, want the client's folder from the global remap, as the grab sends it", resp.Paths[0].ClientPath) } }) - t.Run("a client remap removes the warning", func(t *testing.T) { + t.Run("a client remap wins over the global one", func(t *testing.T) { host, port := qbitGrabServer(t, map[string]string{}, "/default") - resp, _ := runDiagnose(t, diagnoseSetup{downloadDir: downloads, remap: "/qbit/downloads:" + downloads}, qbitClient(host, port, "", "/qbit/downloads:"+downloads)) - wantDiagStatus(t, resp, diagCodeRemap, diagPass) + resp, _ := runDiagnose(t, diagnoseSetup{downloadDir: downloads, remap: "/global/downloads:" + downloads}, qbitClient(host, port, "", "/qbit/downloads:"+downloads)) + remap := wantDiagStatus(t, resp, diagCodeRemap, diagPass) + if !strings.Contains(remap.Message, "This client's path remap") { + t.Errorf("message = %q", remap.Message) + } + if resp.Paths[0].ClientPath != "/qbit/downloads" { + t.Errorf("clientPath = %q, want the client remap's folder", resp.Paths[0].ClientPath) + } }) t.Run("a category save path is not sent by Bindery", func(t *testing.T) { diff --git a/internal/api/queue.go b/internal/api/queue.go index 70ac7ab21..ce092655d 100644 --- a/internal/api/queue.go +++ b/internal/api/queue.go @@ -317,10 +317,13 @@ func (h *QueueHandler) WithStoragePaths(downloadDir, audiobookDownloadDir string // WithDownloadPathRemap attaches the global BINDERY_DOWNLOAD_PATH_REMAP. // -// Only "remove with data" on an rTorrent client reads it — rTorrent has no -// delete-with-data command, so Bindery deletes the payload itself and has to -// translate rTorrent's path the same way the importer does. Leaving it unset -// simply falls back to the client's own PathRemap. +// Two things read it. A manual grab runs the save path it sends to +// qBittorrent or rTorrent through it when the client's own PathRemap does not +// cover the download folder (#2665). "Remove with data" on an rTorrent client +// uses it too: rTorrent has no delete-with-data command, so Bindery deletes +// the payload itself and has to translate rTorrent's path the same way the +// importer does. Leaving it unset simply falls back to the client's own +// PathRemap. func (h *QueueHandler) WithDownloadPathRemap(remap string) *QueueHandler { h.downloadPathRemap = remap return h @@ -1428,6 +1431,7 @@ func (h *QueueHandler) grab(ctx context.Context, req grabRequest) (*models.Downl MediaType: req.MediaType, DownloadDir: h.downloadDir, AudiobookDownloadDir: h.audiobookDownloadDir, + GlobalRemap: h.downloadPathRemap, }.WithSeedLimits(h.resolveSeedLimits(ctx, indexerID))) if err != nil { slog.Error("failed to send download", "client_type", client.Type, "error", err, "title", req.Title) diff --git a/internal/api/queue_grab_remap_test.go b/internal/api/queue_grab_remap_test.go new file mode 100644 index 000000000..1377b8d8c --- /dev/null +++ b/internal/api/queue_grab_remap_test.go @@ -0,0 +1,76 @@ +package api + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "sync" + "testing" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/httpsec" + "github.com/vavallee/bindery/internal/models" +) + +// TestQueueGrab_SavePathUsesGlobalRemap: a manual grab through qBittorrent +// with no category sends a save path, and with only BINDERY_DOWNLOAD_PATH_REMAP +// set that path has to be the client's, not Bindery's /downloads (#2665). +func TestQueueGrab_SavePathUsesGlobalRemap(t *testing.T) { + defer httpsec.AllowLoopbackForTests()() + + var mu sync.Mutex + var savePaths []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v2/auth/login": + _, _ = w.Write([]byte("Ok.")) + case "/api/v2/torrents/add": + _ = r.ParseForm() + mu.Lock() + savePaths = append(savePaths, r.FormValue("savepath")) + mu.Unlock() + _, _ = w.Write([]byte("Ok.")) + default: + _, _ = w.Write([]byte("[]")) + } + })) + t.Cleanup(srv.Close) + + h, database, _, clients, books, ctx := queueFixture(t) + h.WithStoragePaths("/downloads", "").WithDownloadPathRemap("/data:/downloads") + host, port := testServerHostPort(t, srv.URL) + if err := clients.Create(ctx, &models.DownloadClient{ + Name: "qbit", Type: "qbittorrent", Host: host, Port: port, Username: "u", Password: "p", Enabled: true, + }); err != nil { + t.Fatalf("create client: %v", err) + } + author := &models.Author{ForeignID: "remap-author", Name: "Remap Author", SortName: "Author, Remap", Monitored: true} + if err := db.NewAuthorRepo(database).Create(ctx, author); err != nil { + t.Fatalf("create author: %v", err) + } + book := &models.Book{ForeignID: "remap-book", AuthorID: author.ID, Title: "Remap Book", SortTitle: "remap book", MediaType: models.MediaTypeEbook} + if err := books.Create(ctx, book); err != nil { + t.Fatalf("create book: %v", err) + } + + payload, _ := json.Marshal(map[string]any{ + "guid": "remap-guid", + "title": "Remap Author - Remap Book (epub)", + "nzbUrl": "magnet:?xt=urn:btih:aabbccddeeff00112233445566778899aabbccdd&dn=Remap+Book", + "size": 100, + "bookId": book.ID, + "protocol": "torrent", + "mediaType": models.MediaTypeEbook, + }) + w := httptest.NewRecorder() + h.Grab(w, httptest.NewRequest(http.MethodPost, "/api/v1/queue/grab", bytes.NewReader(payload))) + if w.Code != http.StatusAccepted { + t.Fatalf("grab: status %d: %s", w.Code, w.Body.String()) + } + mu.Lock() + defer mu.Unlock() + if len(savePaths) != 1 || savePaths[0] != "/data" { + t.Errorf("save paths sent = %q, want one add with /data", savePaths) + } +} diff --git a/internal/downloader/adapter.go b/internal/downloader/adapter.go index 45825634b..fb24b2354 100644 --- a/internal/downloader/adapter.go +++ b/internal/downloader/adapter.go @@ -63,6 +63,10 @@ type SendOptions struct { // the full table. Usenet clients ignore them. SeedTimeMinutes *int InactiveSeedTimeMinutes *int + // GlobalRemap is BINDERY_DOWNLOAD_PATH_REMAP. torrentSavePath falls back + // to it when the client's own PathRemap does not cover the download + // folder, the same precedence the importer reads paths back with (#2665). + GlobalRemap string } // SeedLimits is the set of per-indexer seeding overrides resolved for a grab. @@ -286,7 +290,7 @@ func SendDownload(ctx context.Context, client *models.DownloadClient, sourceURL, rt := RtorrentFor(client) // Category is the ruTorrent label (d.custom1); the download directory // comes from Bindery's configured download dir run back through the - // client's PathRemap, same as qBittorrent's savePath. + // path remaps, same as qBittorrent's savePath. hash, err := rt.AddTorrent(ctx, sourceURL, ResolveCategory(client, opts.MediaType), torrentSavePath(client, opts), opts.SeedRatio) if err != nil { return nil, err @@ -321,14 +325,16 @@ func SendDownload(ctx context.Context, client *models.DownloadClient, sourceURL, } // torrentSavePath renders Bindery's target download directory in the download -// client's own filesystem namespace by running it back through the client's -// PathRemap. Shared by qBittorrent (savePath) and rTorrent (d.directory.set). +// client's own filesystem namespace via ClientSidePath: the client's PathRemap +// first, then the global remap in opts.GlobalRemap, the order the importer +// reads the path back in. Shared by qBittorrent (savePath) and rTorrent +// (d.directory.set). func torrentSavePath(client *models.DownloadClient, opts SendOptions) string { localPath := TargetDownloadDir(opts.MediaType, opts.DownloadDir, opts.AudiobookDownloadDir) if strings.TrimSpace(localPath) == "" { return "" } - return pathmap.Parse(client.PathRemap).ApplyInverse(localPath) + return ClientSidePath(client, localPath, pathmap.Parse(opts.GlobalRemap)) } // errTransmissionSessionID is returned when a removal is asked to act on a diff --git a/internal/downloader/adapter_test.go b/internal/downloader/adapter_test.go index 8f72ea965..97b715db7 100644 --- a/internal/downloader/adapter_test.go +++ b/internal/downloader/adapter_test.go @@ -13,6 +13,7 @@ import ( "github.com/vavallee/bindery/internal/httpsec" "github.com/vavallee/bindery/internal/models" + "github.com/vavallee/bindery/internal/pathmap" ) func TestProtocolForClient(t *testing.T) { @@ -1048,3 +1049,75 @@ func TestSendDownload_Transmission_PersistsInfoHash(t *testing.T) { }) } } + +// TestTorrentSavePath_RemapPrecedence: the save path sent to the client is +// Bindery's download folder run back through the same precedence the importer +// reads it with (RemapClientPath): the client's own PathRemap first, and +// BINDERY_DOWNLOAD_PATH_REMAP when that leaves the folder unchanged (#2665). +func TestTorrentSavePath_RemapPrecedence(t *testing.T) { + cases := []struct { + name string + clientRemap string + globalRemap string + downloadDir string + want string + }{ + {name: "neither set sends Bindery's folder", downloadDir: "/downloads", want: "/downloads"}, + {name: "client remap only", clientRemap: "/seedbox:/downloads", downloadDir: "/downloads/books", want: "/seedbox/books"}, + {name: "global remap only", globalRemap: "/data:/downloads", downloadDir: "/downloads", want: "/data"}, + {name: "global remap only, under the prefix", globalRemap: "/data:/downloads", downloadDir: "/downloads/books", want: "/data/books"}, + {name: "both set, client remap wins", clientRemap: "/seedbox:/downloads", globalRemap: "/data:/downloads", downloadDir: "/downloads/books", want: "/seedbox/books"}, + {name: "client remap misses, global applies", clientRemap: "/other:/elsewhere", globalRemap: "/data:/downloads", downloadDir: "/downloads/books", want: "/data/books"}, + {name: "global remap to a Windows client", globalRemap: `D:\Torrents:/downloads`, downloadDir: "/downloads/books", want: `D:\Torrents\books`}, + {name: "global remap to a network share", globalRemap: `\\nas\torrents:/downloads`, downloadDir: "/downloads/books", want: `\\nas\torrents\books`}, + {name: "no download folder sends nothing", globalRemap: "/data:/downloads", want: ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + client := &models.DownloadClient{Type: "rtorrent", PathRemap: tc.clientRemap} + got := torrentSavePath(client, SendOptions{DownloadDir: tc.downloadDir, GlobalRemap: tc.globalRemap}) + if got != tc.want { + t.Errorf("torrentSavePath = %q, want %q", got, tc.want) + } + if got == "" { + return + } + // Whatever is sent must read back as Bindery's own folder, which + // is the point of sharing the importer's precedence. + if back, _ := RemapClientPath(client, got, pathmap.Parse(tc.globalRemap)); back != tc.downloadDir { + t.Errorf("RemapClientPath(%q) = %q, want the download folder %q back", got, back, tc.downloadDir) + } + }) + } +} + +// TestSendDownload_QbittorrentGlobalRemapOnly: with no category qBittorrent +// takes the save path Bindery sends, and with only the global remap set that +// path must be the client's own, not Bindery's container folder (#2665). +func TestSendDownload_QbittorrentGlobalRemapOnly(t *testing.T) { + var gotSavePath string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v2/auth/login": + _, _ = w.Write([]byte("Ok.")) + case "/api/v2/torrents/add": + _ = r.ParseForm() + gotSavePath = r.FormValue("savepath") + _, _ = w.Write([]byte("Ok.")) + } + })) + defer srv.Close() + host, port := serverHostPort(t, srv.URL) + client := &models.DownloadClient{Type: "qbittorrent", Host: host, Port: port, Username: "u", Password: "p"} + _, err := SendDownload(context.Background(), client, "magnet:?xt=urn:btih:ABCDEF123&dn=Book", "", SendOptions{ + MediaType: models.MediaTypeEbook, + DownloadDir: "/downloads", + GlobalRemap: "/data:/downloads", + }) + if err != nil { + t.Fatalf("SendDownload: %v", err) + } + if gotSavePath != "/data" { + t.Errorf("savepath = %q, want /data from the global remap", gotSavePath) + } +} diff --git a/internal/downloader/clientpath.go b/internal/downloader/clientpath.go index 2c6b3a13a..ff459df83 100644 --- a/internal/downloader/clientpath.go +++ b/internal/downloader/clientpath.go @@ -8,6 +8,7 @@ import ( "sort" "strings" + "github.com/vavallee/bindery/internal/downloader/deluge" "github.com/vavallee/bindery/internal/models" "github.com/vavallee/bindery/internal/pathmap" ) @@ -23,9 +24,6 @@ type ClientPathInfo struct { // Category is the category or label the path was resolved for, exactly // as the grab sends it. Category string - // SentByBindery is true when Path is the save path Bindery itself sends - // with the grab (torrentSavePath), rather than a folder the client chose. - SentByBindery bool // Note and NoteFix are set when a grab will not behave the way the // configuration suggests, or when part of the answer could not be checked. Note string @@ -106,12 +104,13 @@ func CompletedPath(ctx context.Context, client *models.DownloadClient) (ClientPa // actually finishes in, worked out from the same inputs SendDownload uses: // ResolveCategory for the category or label, and torrentSavePath for the save // path Bindery sends. downloadDir and audiobookDownloadDir are Bindery's own -// BINDERY_DOWNLOAD_DIR and BINDERY_AUDIOBOOK_DOWNLOAD_DIR. +// BINDERY_DOWNLOAD_DIR and BINDERY_AUDIOBOOK_DOWNLOAD_DIR, and globalRemap is +// BINDERY_DOWNLOAD_PATH_REMAP, which the sent save path falls back to. // // An error means the client would not answer (for SABnzbd, typically an NZB // only API key). A zero Path with no error means the client answered but // exposes no usable folder, for example a category it does not have. -func GrabSavePath(ctx context.Context, client *models.DownloadClient, mediaType, downloadDir, audiobookDownloadDir string) (ClientPathInfo, error) { +func GrabSavePath(ctx context.Context, client *models.DownloadClient, mediaType, downloadDir, audiobookDownloadDir, globalRemap string) (ClientPathInfo, error) { if client == nil { return ClientPathInfo{}, nil } @@ -119,7 +118,7 @@ func GrabSavePath(ctx context.Context, client *models.DownloadClient, mediaType, // save path exactly as they are, so the doctor has to use them the same // way to reach the same folder. category := ResolveCategory(client, mediaType) - sent := torrentSavePath(client, SendOptions{MediaType: mediaType, DownloadDir: downloadDir, AudiobookDownloadDir: audiobookDownloadDir}) + sent := torrentSavePath(client, SendOptions{MediaType: mediaType, DownloadDir: downloadDir, AudiobookDownloadDir: audiobookDownloadDir, GlobalRemap: globalRemap}) info := ClientPathInfo{Category: category} switch client.Type { case "qbittorrent": @@ -128,7 +127,7 @@ func GrabSavePath(ctx context.Context, client *models.DownloadClient, mediaType, // rTorrent receives d.directory.set whenever Bindery has a download // folder, so directory.default only matters without one. if sent != "" { - info.Path, info.Source, info.SentByBindery = sent, "the save path Bindery sends", true + info.Path, info.Source = sent, "the save path Bindery sends" return info, nil } dir, err := RtorrentFor(client).DefaultDirectory(ctx) @@ -185,7 +184,7 @@ func qbittorrentGrabPath(ctx context.Context, client *models.DownloadClient, inf qb := QbittorrentFor(client) if info.Category == "" { if sent != "" { - info.Path, info.Source, info.SentByBindery = sent, "the save path Bindery sends", true + info.Path, info.Source = sent, "the save path Bindery sends" return info, nil } def, err := qb.GetDefaultSavePath(ctx) @@ -243,7 +242,8 @@ func TestConnection(ctx context.Context, client *models.DownloadClient) error { // CategoryReport is the result of CheckCategories. type CategoryReport struct { // Checked is false for client types without a category list Bindery can - // read (Transmission, Deluge, rTorrent). + // read (Transmission, rTorrent), and for Deluge when its Label plugin + // will not list labels. Checked bool // Wanted is the distinct non-empty categories configured in Bindery. Wanted []string @@ -256,9 +256,14 @@ type CategoryReport struct { // CheckCategories compares the categories configured in Bindery with the ones // the client defines. Matching is exact, as it is at grab time; a nested // qBittorrent category such as "books/ebooks" is its own key and matches. +// Deluge labels are compared as deluge.LabelID, the lowercase form grabs send +// them in, and Wanted holds that form. func CheckCategories(ctx context.Context, client *models.DownloadClient) (CategoryReport, error) { report := CategoryReport{} for _, c := range []string{client.Category, client.CategoryAudiobook} { + if client.Type == "deluge" { + c = deluge.LabelID(c) + } // Compared exactly as grabs send it; a category with spaces around it // does not match the client's, and the grab would not either. if strings.TrimSpace(c) != "" && !slices.Contains(report.Wanted, c) { @@ -281,7 +286,16 @@ func CheckCategories(ctx context.Context, client *models.DownloadClient) (Catego return report, err } existing = cats - case "transmission", "deluge", "rtorrent": + case "deluge": + labels, err := DelugeFor(client).Labels(ctx) + if err != nil { + // Almost always the Label plugin being off. The report stays + // unchecked, and the diagnose action's advice for an unchecked + // Deluge list is to turn the plugin on. + return report, nil + } + existing = labels + case "transmission", "rtorrent": return report, nil default: cats, err := SabnzbdFor(client).GetCategories(ctx) diff --git a/internal/downloader/deluge/client.go b/internal/downloader/deluge/client.go index 8a320439f..387d447d7 100644 --- a/internal/downloader/deluge/client.go +++ b/internal/downloader/deluge/client.go @@ -276,8 +276,14 @@ func (c *Client) AddTorrent(ctx context.Context, magnetOrURL, label string, seed } if label != "" { - // Label plugin is optional; ignore errors if it is not loaded. - _ = c.setLabel(ctx, hash, label) + // The torrent is already added, so a label that does not apply must + // not fail the grab, but it does change where a label move path puts + // the files, so it is logged rather than discarded (#2665). The usual + // causes are the Label plugin being off or the label not existing. + if err := c.setLabel(ctx, hash, label); err != nil { + slog.Warn("deluge: torrent added but its label was not applied; check the Label plugin is on and the label exists in Deluge", + "hash", hash, "label", LabelID(label), "error", err) + } } // Apply the per-indexer seed-ratio override. Skipped for nil (no override) @@ -525,11 +531,11 @@ func readLimited(r io.Reader, maxBytes int64) ([]byte, error) { return data, nil } -// setLabel applies a label to a torrent via the Deluge label plugin. -// Errors are intentionally swallowed by the caller — the plugin is optional. +// setLabel applies a label to a torrent via the Deluge label plugin, sending +// it as LabelID so a category typed with capitals still matches. func (c *Client) setLabel(ctx context.Context, hash, label string) error { var result any - return c.call(ctx, true, "label.set_torrent", []any{hash, label}, &result) + return c.call(ctx, true, "label.set_torrent", []any{hash, LabelID(label)}, &result) } // GetTorrents returns status for all torrents, keyed by lower-cased hash. diff --git a/internal/downloader/deluge/client_test.go b/internal/downloader/deluge/client_test.go index 11e131402..4aba6970b 100644 --- a/internal/downloader/deluge/client_test.go +++ b/internal/downloader/deluge/client_test.go @@ -8,6 +8,7 @@ import ( "net" "net/http" "net/http/httptest" + "sort" "strings" "syscall" "testing" @@ -82,6 +83,15 @@ type delugeServer struct { // realFileHash makes core.add_torrent_file report the payload's real v1 // infohash instead of the fixed placeholder. realFileHash bool + + // labels, when non-nil, makes the stub behave like Deluge's Label plugin + // (deluge_label/core.py). The plugin's add() stores every label + // lowercased, so tests seed lowercase ids here; label.get_labels lists + // them, and label.set_torrent accepts only an id it holds exactly, + // answering "Unknown Label" otherwise. nil keeps the old accept-anything + // stub. torrentLabels records what was applied. + labels map[string]bool + torrentLabels map[string]string } // duplicate answers an add of hash the way the configured Deluge does when @@ -214,8 +224,40 @@ func (s *delugeServer) handler() http.HandlerFunc { write(hash) case "label.set_torrent": + if s.labels == nil { + write(nil) + return + } + var hash, label string + json.Unmarshal(req.Params[0], &hash) + json.Unmarshal(req.Params[1], &label) + // set_torrent does not lowercase: the id must match exactly. + if label != "" && !s.labels[label] { + writeErr("Unknown Label") + return + } + if _, ok := s.torrents[hash]; !ok { + writeErr("Unknown Torrent") + return + } + if s.torrentLabels == nil { + s.torrentLabels = make(map[string]string) + } + s.torrentLabels[hash] = label write(nil) + case "label.get_labels": + if s.labels == nil { + writeErr("Unknown method") + return + } + out := make([]string, 0, len(s.labels)) + for l := range s.labels { + out = append(out, l) + } + sort.Strings(out) + write(out) + case "core.set_torrent_stop_ratio": if s.stopRatioErr { writeErr("set stop ratio error") diff --git a/internal/downloader/deluge/config.go b/internal/downloader/deluge/config.go index db9949cd3..7bf7473be 100644 --- a/internal/downloader/deluge/config.go +++ b/internal/downloader/deluge/config.go @@ -18,18 +18,37 @@ type Location struct { NoteFix string } +// LabelID is the form of a category Deluge's Label plugin knows it by. The +// plugin lowercases a label when it is created but its set_torrent and +// get_options take the id exactly as given, so a category typed as "Books" +// only reaches the "books" label when Bindery lowercases it first (#2665). +func LabelID(label string) string { + return strings.ToLower(label) +} + +// Labels lists the labels Deluge's Label plugin holds, as label.get_labels +// returns them (always lowercase). An error usually means the plugin is off, +// in which case the method does not exist. The plugin answers a list, empty +// or not, so a null answer is an error too rather than "no labels". +func (c *Client) Labels(ctx context.Context) ([]string, error) { + var labels *[]string + if err := c.call(ctx, true, "label.get_labels", []any{}, &labels); err != nil { + return nil, fmt.Errorf("list deluge labels: %w", err) + } + if labels == nil { + return nil, fmt.Errorf("list deluge labels: no label list in the reply") + } + return *labels, nil +} + // DownloadLocation returns where deluged leaves a torrent that Bindery adds -// with label, the label exactly as AddTorrent sends it to label.set_torrent. +// with label, looked up as LabelID, the form AddTorrent sends to +// label.set_torrent. // // Order of precedence matches Deluge: a label whose options apply a move // completed path wins, then the global move_completed_path when "move // completed" is on, then download_location. Only the needed keys are // requested. -// -// The Label plugin only knows lowercase labels and its set_torrent does not -// lowercase what it is given, so a label with capitals is rejected. AddTorrent -// ignores that error, which leaves the torrent unlabelled in the global -// folder; this reports exactly that. func (c *Client) DownloadLocation(ctx context.Context, label string) (Location, error) { var cfg struct { DownloadLocation string `json:"download_location"` @@ -48,11 +67,7 @@ func (c *Client) DownloadLocation(ctx context.Context, label string) (Location, if label == "" { return loc, nil } - if label != strings.ToLower(label) { - loc.Note = fmt.Sprintf("Deluge only accepts lowercase labels, so grabs with the category %q are not labelled and land in the default folder.", label) - loc.NoteFix = "Use a lowercase category for this client in Bindery." - return loc, nil - } + label = LabelID(label) var opts struct { ApplyMoveCompleted bool `json:"apply_move_completed"` MoveCompleted bool `json:"move_completed"` diff --git a/internal/downloader/deluge/config_test.go b/internal/downloader/deluge/config_test.go new file mode 100644 index 000000000..eff81fa9f --- /dev/null +++ b/internal/downloader/deluge/config_test.go @@ -0,0 +1,42 @@ +package deluge_test + +import ( + "context" + "strings" + "testing" + + "github.com/vavallee/bindery/internal/downloader/deluge" +) + +// TestLabels lists the labels the plugin holds. +func TestLabels(t *testing.T) { + srv, ds := newTestServer(t, "pw") + ds.labels = map[string]bool{"books": true, "audiobooks": true} + c := clientFromServer(srv, "pw") + + got, err := c.Labels(context.Background()) + if err != nil { + t.Fatalf("Labels: %v", err) + } + if strings.Join(got, ",") != "audiobooks,books" { + t.Errorf("Labels = %v", got) + } +} + +// TestLabels_PluginOff: with the plugin off the method does not exist, which +// is an error rather than an empty list. +func TestLabels_PluginOff(t *testing.T) { + srv, _ := newTestServer(t, "pw") + c := clientFromServer(srv, "pw") + if _, err := c.Labels(context.Background()); err == nil { + t.Fatal("expected an error with the Label plugin off") + } +} + +func TestLabelID(t *testing.T) { + for in, want := range map[string]string{"Books": "books", "books": "books", "AudioBooks_2": "audiobooks_2", "": ""} { + if got := deluge.LabelID(in); got != want { + t.Errorf("LabelID(%q) = %q, want %q", in, got, want) + } + } +} diff --git a/internal/downloader/deluge/label_test.go b/internal/downloader/deluge/label_test.go new file mode 100644 index 000000000..db867d005 --- /dev/null +++ b/internal/downloader/deluge/label_test.go @@ -0,0 +1,84 @@ +package deluge_test + +import ( + "bytes" + "context" + "log/slog" + "strings" + "sync" + "testing" +) + +const labelTestMagnet = "magnet:?xt=urn:btih:aabbccddeeff00112233445566778899aabbccdd&dn=Test+Book" +const labelTestHash = "aabbccddeeff00112233445566778899aabbccdd" + +// lockedBuffer is a bytes.Buffer safe for the slog handler and the test to +// share. +type lockedBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *lockedBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *lockedBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + +func captureWarnings(t *testing.T) *lockedBuffer { + t.Helper() + buf := &lockedBuffer{} + orig := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelWarn}))) + t.Cleanup(func() { slog.SetDefault(orig) }) + return buf +} + +// TestAddTorrent_CapitalCategoryIsLabelled: the Label plugin stores every +// label lowercased and set_torrent only accepts the stored id, so a category +// typed as "Books" has to be sent as "books" or the torrent is left +// unlabelled (#2665). +func TestAddTorrent_CapitalCategoryIsLabelled(t *testing.T) { + srv, ds := newTestServer(t, "pw") + // Created in the Deluge UI as "Books", which the plugin keeps as "books". + ds.labels = map[string]bool{"books": true} + c := clientFromServer(srv, "pw") + + hash, err := c.AddTorrent(context.Background(), labelTestMagnet, "Books", nil) + if err != nil { + t.Fatalf("AddTorrent: %v", err) + } + if got := ds.torrentLabels[hash]; got != "books" { + t.Errorf("torrent label = %q, want %q", got, "books") + } +} + +// TestAddTorrent_UnknownLabelWarns: a label Deluge does not have still adds +// the torrent, but the failure is logged at WARN instead of discarded. +func TestAddTorrent_UnknownLabelWarns(t *testing.T) { + logs := captureWarnings(t) + srv, ds := newTestServer(t, "pw") + ds.labels = map[string]bool{"tv": true} + c := clientFromServer(srv, "pw") + + hash, err := c.AddTorrent(context.Background(), labelTestMagnet, "books", nil) + if err != nil { + t.Fatalf("AddTorrent must not fail over a label: %v", err) + } + if hash != labelTestHash { + t.Errorf("hash = %q", hash) + } + if _, labelled := ds.torrentLabels[hash]; labelled { + t.Errorf("torrent was labelled %q, want none", ds.torrentLabels[hash]) + } + out := logs.String() + if !strings.Contains(out, "level=WARN") || !strings.Contains(out, "Unknown Label") || !strings.Contains(out, "label=books") { + t.Errorf("expected a WARN naming the label and Deluge's error, got:\n%s", out) + } +} diff --git a/internal/downloader/pathcheck.go b/internal/downloader/pathcheck.go index caba8b302..c8b3e2f39 100644 --- a/internal/downloader/pathcheck.go +++ b/internal/downloader/pathcheck.go @@ -138,6 +138,21 @@ func RemapClientPath(client *models.DownloadClient, rawPath string, global *path return rawPath, RemapRuleNone } +// ClientSidePath is RemapClientPath run the other way: it renders a path on +// Bindery's filesystem in the download client's own namespace, for a path +// Bindery sends to the client. The precedence is the same, so whatever is +// sent reads back to where it started: the client's own PathRemap first, and +// only if that leaves the path unchanged the global BINDERY_DOWNLOAD_PATH_REMAP +// (#2665). +func ClientSidePath(client *models.DownloadClient, localPath string, global *pathmap.Remapper) string { + if client != nil && strings.TrimSpace(client.PathRemap) != "" { + if clientPath := pathmap.Parse(client.PathRemap).ApplyInverse(localPath); clientPath != localPath { + return clientPath + } + } + return global.ApplyInverse(localPath) +} + // remapClientPath is RemapClientPath for callers holding the global remap as // its raw setting string and needing only the path. func remapClientPath(client *models.DownloadClient, rawPath, globalRemap string) string { diff --git a/internal/downloader/rtorrent_test.go b/internal/downloader/rtorrent_test.go index 553169ffa..f0ea205ab 100644 --- a/internal/downloader/rtorrent_test.go +++ b/internal/downloader/rtorrent_test.go @@ -512,3 +512,23 @@ func TestRtorrentFor_IsCached(t *testing.T) { t.Fatalf("constructor count after rotation: got %d, want 2", got) } } + +// TestSendDownload_RtorrentGlobalRemapOnly: rTorrent always receives +// d.directory.set, so with only BINDERY_DOWNLOAD_PATH_REMAP configured the +// directory must still be in rTorrent's namespace (#2665). +func TestSendDownload_RtorrentGlobalRemapOnly(t *testing.T) { + stub := newRtorrentStub(t, "0", "") + client := stub.client(t, 105) + + magnet := "magnet:?xt=urn:btih:" + rtorrentTestHash + if _, err := SendDownload(context.Background(), client, magnet, "The Book", SendOptions{ + DownloadDir: "/downloads", + GlobalRemap: "/data:/downloads", + }); err != nil { + t.Fatalf("SendDownload: %v", err) + } + body := stub.allBodies() + if !strings.Contains(body, "d.directory.set="/data"") { + t.Errorf("download directory not run back through the global remap: %s", body) + } +} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 461ae024c..4c5d281b4 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -217,6 +217,8 @@ type Scheduler struct { audiobookDownloadDir string // downloadHealth and downloadPathRemap back the periodic client-health // probe (#2029). nil store means the job is not registered. + // downloadPathRemap is also the global remap automatic grabs send their + // save path through (#2665) and rTorrent removals resolve data with. downloadHealth *downloader.HealthStore downloadPathRemap string } @@ -319,7 +321,8 @@ func (s *Scheduler) WithEditions(editions *db.EditionRepo) { // WithDownloadClientHealth attaches the health store and the global path remap // so client health can be re-probed on a schedule (#2029). Without it the -// periodic job is not registered and health stays what it was at boot. +// periodic job is not registered and health stays what it was at boot. The +// remap is also what automatic grabs and rTorrent removals fall back to. func (s *Scheduler) WithDownloadClientHealth(store *downloader.HealthStore, globalRemap string) { s.downloadHealth = store s.downloadPathRemap = globalRemap @@ -1244,6 +1247,7 @@ func (s *Scheduler) searchAndGrabFormat(ctx context.Context, book models.Book, m MediaType: mediaType, DownloadDir: s.downloadDir, AudiobookDownloadDir: s.audiobookDownloadDir, + GlobalRemap: s.downloadPathRemap, }.WithSeedLimits(s.resolveSeedLimits(ctx, best.IndexerID))) if err != nil { slog.Error("SearchAndGrabBook: failed to send to downloader", "client", client.Type, "title", best.Title, "error", err) diff --git a/internal/scheduler/scheduler_grab_remap_test.go b/internal/scheduler/scheduler_grab_remap_test.go new file mode 100644 index 000000000..b7fd06887 --- /dev/null +++ b/internal/scheduler/scheduler_grab_remap_test.go @@ -0,0 +1,74 @@ +package scheduler + +import ( + "context" + "net" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "sync" + "testing" + + "github.com/vavallee/bindery/internal/httpsec" + "github.com/vavallee/bindery/internal/indexer/newznab" + "github.com/vavallee/bindery/internal/models" +) + +// TestSearchAndGrabFormat_SavePathUsesGlobalRemap: an automatic grab through +// qBittorrent with no category sends a save path, and with only +// BINDERY_DOWNLOAD_PATH_REMAP set that path has to be the client's, not +// Bindery's /downloads (#2665). +func TestSearchAndGrabFormat_SavePathUsesGlobalRemap(t *testing.T) { + defer httpsec.AllowLoopbackForTests()() + ctx := context.Background() + + var mu sync.Mutex + var savePaths []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v2/auth/login": + _, _ = w.Write([]byte("Ok.")) + case "/api/v2/torrents/add": + _ = r.ParseForm() + mu.Lock() + savePaths = append(savePaths, r.FormValue("savepath")) + mu.Unlock() + _, _ = w.Write([]byte("Ok.")) + default: + _, _ = w.Write([]byte("[]")) + } + })) + t.Cleanup(srv.Close) + + s, _, _, book := freeleechFixture(t, false, []newznab.SearchResult{{ + GUID: "g-remap", Title: "Ratio Book.epub", + NZBURL: "magnet:?xt=urn:btih:aabbccddeeff00112233445566778899aabbccdd&dn=Ratio+Book", + Protocol: "torrent", DownloadVolumeFactor: ratio(0), + }}) + u, err := url.Parse(srv.URL) + if err != nil { + t.Fatal(err) + } + host, portStr, _ := net.SplitHostPort(u.Host) + port, _ := strconv.Atoi(portStr) + clients, err := s.clients.List(ctx) + if err != nil || len(clients) != 1 { + t.Fatalf("clients = %v, err = %v", clients, err) + } + c := clients[0] + c.Host, c.Port, c.Username, c.Password = host, port, "u", "p" + if err := s.clients.Update(ctx, &c); err != nil { + t.Fatalf("update client: %v", err) + } + s.WithStoragePaths("/downloads", "") + s.WithDownloadClientHealth(nil, "/data:/downloads") + + s.searchAndGrabFormat(ctx, book, models.MediaTypeEbook, nil) + + mu.Lock() + defer mu.Unlock() + if len(savePaths) != 1 || savePaths[0] != "/data" { + t.Errorf("save paths sent = %q, want one add with /data", savePaths) + } +} From 0423dc824e1c9381643cb90e186cbb1451477c62 Mon Sep 17 00:00:00 2001 From: vavallee Date: Mon, 5 Oct 2026 00:42:13 -0300 Subject: [PATCH 2/3] fix(downloader): let an identity client remap opt out of the global remap on send (#2665) On the way out the global remap always matches, because its right side is Bindery's own folder, so a BINDERY_DOWNLOAD_PATH_REMAP written for one client (SABnzbd sees /downloads as /data) also rewrote what an rTorrent or category-less qBittorrent mounting /downloads directly was sent. The natural opt-out, an identity client remap "/downloads:/downloads", did not work: ClientSidePath fell back to the global remap whenever the client remap left the path unchanged, which cannot tell "matched and mapped to itself" from "no rule matched". pathmap gains ApplyMatched and ApplyInverseMatched, which also report whether a rule matched; Apply and ApplyInverse delegate to them unchanged. ClientSidePath branches on the match. RemapClientPath (the read side) is left as it was: there the global remap's left side is a client path and does not match a client mounting Bindery's folder directly. Diagnose lost its "global remap not applied when sending" warning with the first commit. It now warns instead when the global remap produced the folder Bindery sends and the client's own default save folder (qBittorrent default save path, rTorrent directory.default) is neither that folder nor inside or above it, and the fix names the identity remap. A folder from the client's own remap is not second guessed. The qBittorrent category health check also resolves the category save path with the global remap as fallback, the importer's precedence. Docs and the changelog fragment describe the identity opt-out and carry an upgrade note for users with the global remap and rTorrent or a category-less qBittorrent. Signed-off-by: vavallee Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 --- .../2665-save-path-remap-deluge-label.md | 3 +- docs/API.md | 2 +- docs/DEPLOYMENT.md | 2 + .../api/download_client_diagnose_grab_test.go | 31 +++++++- internal/downloader/adapter_test.go | 6 ++ internal/downloader/clientpath.go | 75 +++++++++++++++++-- internal/downloader/clientpath_test.go | 22 ++++++ internal/downloader/health.go | 15 ++-- internal/downloader/health_test.go | 30 ++++++++ internal/downloader/pathcheck.go | 12 ++- internal/downloader/rtorrent_test.go | 35 +++++++++ internal/pathmap/remap.go | 32 ++++++-- internal/pathmap/remap_test.go | 39 ++++++++++ 13 files changed, 275 insertions(+), 29 deletions(-) diff --git a/changelog.d/2665-save-path-remap-deluge-label.md b/changelog.d/2665-save-path-remap-deluge-label.md index ecb72eead..8d7603a5b 100644 --- a/changelog.d/2665-save-path-remap-deluge-label.md +++ b/changelog.d/2665-save-path-remap-deluge-label.md @@ -1,3 +1,4 @@ ### Fixed -- **The global path remap now applies to the save path Bindery sends** (#2665). With only `BINDERY_DOWNLOAD_PATH_REMAP` set, rTorrent and qBittorrent (when the client has no category) were told to save into Bindery's own folder, such as `/downloads`, which may not exist on the client's side. The save path now goes through the same remaps Bindery reads paths back with: the client's own path remap first, then the global one. +- **The global path remap now applies to the save path Bindery sends** (#2665). With only `BINDERY_DOWNLOAD_PATH_REMAP` set, rTorrent and qBittorrent (when the client has no category) were told to save into Bindery's own folder, such as `/downloads`, which may not exist on the client's side. The save path now goes through the same remaps Bindery reads paths back with: the client's own path remap first, then the global one. The qBittorrent category health check now honours the global remap too. +- **Upgrade note for `BINDERY_DOWNLOAD_PATH_REMAP` users with rTorrent, or qBittorrent without a category** (#2665). The folder Bindery sends those clients changes to the global remap's client side. Run **Diagnose** on each such client after upgrading. If the client mounts Bindery's download folder at the same path, because the global remap is meant for a different client, give it an identity path remap such as `/downloads:/downloads` so it keeps being sent Bindery's own folder. - **Deluge categories with capital letters now label the torrent** (#2665). Deluge stores labels in lowercase and rejects any other spelling, so a category typed as `Books` left grabs unlabelled and any move path on the label was skipped. Bindery now sends the label lowercased, logs a warning when Deluge still refuses a label (for example when the Label plugin is off or the label does not exist), and Diagnose checks your Deluge categories against the labels Deluge has. diff --git a/docs/API.md b/docs/API.md index 5c7eb2bd1..bf0a3a3de 100644 --- a/docs/API.md +++ b/docs/API.md @@ -604,7 +604,7 @@ only, never a path, and it runs only when called. * The folder checked is where a grab actually lands, worked out the way the grab itself is sent: the save path Bindery sends (rTorrent always, qBittorrent without a category, Transmission with an absolute category), the category save path (qBittorrent, with an empty one meaning the default save path plus the category name), the category folder (SABnzbd), the category DestDir or DestDir plus the category name when `AppendCategoryDir` is on (NZBGet), a Deluge label's move completed path, or the client default. `source` names which. The category is used exactly as a grab sends it: a category with spaces around it gives `client_path: warn`. A Deluge category is sent lowercased, because the Label plugin stores labels in lowercase and only accepts that form when labelling a torrent, so `Books` uses the `books` label's move completed path. * For Deluge, `category` compares the configured categories, lowercased, with the labels the Label plugin lists, and says when it lowercases one. When the plugin is off it cannot list labels and `category` is `unknown`. * Ebook and audiobook grabs are checked separately, because each resolves its own category and download folder. When both land in the same folder there is one `paths` row and the `client_path`, `remap` and `local_path` checks have no `mediaType`; otherwise each carries `mediaType` `ebook` or `audiobook`. -* `remapRule` is `client` (this client's path remap changed the path), `global` (`BINDERY_DOWNLOAD_PATH_REMAP` did) or `none`. A Windows drive path fails for a missing remap only when Bindery itself is not running on Windows. When no remap applies and the client's folder is not under any folder Bindery uses, `remap` is `warn` and `local_path` carries the failure and the fix. When Bindery sends the save path itself (rTorrent, qBittorrent without a category) it runs its download folder through the same remaps in reverse, the client's own first and `BINDERY_DOWNLOAD_PATH_REMAP` as the fallback, so `clientPath` is the folder the client is actually given. +* `remapRule` is `client` (this client's path remap changed the path), `global` (`BINDERY_DOWNLOAD_PATH_REMAP` did) or `none`. A Windows drive path fails for a missing remap only when Bindery itself is not running on Windows. When no remap applies and the client's folder is not under any folder Bindery uses, `remap` is `warn` and `local_path` carries the failure and the fix. When Bindery sends the save path itself (rTorrent, qBittorrent without a category) it runs its download folder through the same remaps in reverse, the client's own first and `BINDERY_DOWNLOAD_PATH_REMAP` as the fallback, so `clientPath` is the folder the client is actually given. A client remap rule that covers the folder wins even when it maps the folder to itself (`/downloads:/downloads`), which is how a client opts out of a global remap meant for others. When the global remap produced the sent folder and the client's own default save folder is neither that folder nor inside or above it, `client_path` is `warn` and the fix names that identity remap. * `hardlinks` has one row per download folder and library root pair, `{mediaType, downloadPath, root, result, linkable, reason}`. Every pair gets a real link probe, because two bind mounts of one filesystem share a device ID yet refuse links across them. `result` is `yes`, `no`, `unknown` (Bindery could not write a test file in the download folder) or `missing` (the library folder does not exist and was not probed). * `primaryFix` is the fix of the first failure, or of the first warning when nothing failed. diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index edf248978..9b349be1e 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -379,6 +379,8 @@ Set a download-client path remap in **Settings → Download clients** or set the The same remaps also run in reverse, with the same precedence, when Bindery tells a client where to save a grab. rTorrent always gets a download directory, and qBittorrent gets a save path when the client has no category set. That directory is Bindery's download folder (`BINDERY_DOWNLOAD_DIR`, or `BINDERY_AUDIOBOOK_DOWNLOAD_DIR` for audiobooks) translated into the client's path: by the client's own remap when it covers that folder, otherwise by `BINDERY_DOWNLOAD_PATH_REMAP`. With `BINDERY_DOWNLOAD_PATH_REMAP=/data:/downloads` and no client remap, rTorrent is told to save into `/data`. Bindery 1.40.0 and earlier applied only the client's own remap here, so with just the global remap set the client was sent Bindery's `/downloads`. +On the way out the global remap matches every client, because its right side is Bindery's own folder. If the global remap is written for one client (say SABnzbd sees Bindery's `/downloads` as `/data`) and an rTorrent or category-less qBittorrent mounts Bindery's folder at the same path, give that client an identity path remap, `/downloads:/downloads`. A client remap that covers the folder always wins, even when it maps it to itself, so that client keeps being sent `/downloads`. **Diagnose** on the client warns when the global remap produced the folder it sends and the client's own default save folder is unrelated to it. + Per-client remaps are stored on each download client, so separate qBittorrent / SABnzbd / NZBGet instances can map different mount points. Existing download clients keep an empty remap after upgrade, which preserves the previous global-only behavior until you add a client-specific value. For a per-client remap, open **Settings → Download clients**, edit the client, and set **Download client path remap**. To see what a remap actually resolves to, press **Diagnose** on the client: it reports where that client says ebook and audiobook grabs land, which remap applies, and whether Bindery can read the result. The left side is the path the client reports; the right side is the path Bindery can read. For qBittorrent this normally means mapping the qBittorrent category save path or torrent content path to Bindery's download mount. Example: if qBittorrent reports `/downloads/books/My.Book` and Bindery sees that same folder as `/media/books/My.Book`, set `/downloads:/media/books`. diff --git a/internal/api/download_client_diagnose_grab_test.go b/internal/api/download_client_diagnose_grab_test.go index 994a4e712..8c035a1e5 100644 --- a/internal/api/download_client_diagnose_grab_test.go +++ b/internal/api/download_client_diagnose_grab_test.go @@ -444,8 +444,11 @@ func TestDiagnose_GlobalRemapAppliesToSentSavePath(t *testing.T) { downloads := t.TempDir() t.Run("qbittorrent without a category", func(t *testing.T) { - host, port := qbitGrabServer(t, map[string]string{}, "/default") + // qBittorrent's default sits above the folder it is sent, so the + // global remap is plausibly meant for it and nothing is flagged. + host, port := qbitGrabServer(t, map[string]string{}, "/qbit") resp, _ := runDiagnose(t, diagnoseSetup{downloadDir: downloads, remap: "/qbit/downloads:" + downloads}, qbitClient(host, port, "", "")) + wantDiagStatus(t, resp, diagCodeClientPath, diagPass) remap := wantDiagStatus(t, resp, diagCodeRemap, diagPass) if !strings.Contains(remap.Message, "BINDERY_DOWNLOAD_PATH_REMAP") { t.Errorf("message = %q", remap.Message) @@ -472,6 +475,32 @@ func TestDiagnose_GlobalRemapAppliesToSentSavePath(t *testing.T) { resp, _ := runDiagnose(t, diagnoseSetup{downloadDir: downloads, remap: "/qbit/downloads:" + downloads}, qbitClient(host, port, "books", "")) wantDiagStatus(t, resp, diagCodeRemap, diagPass) }) + + // The mixed setup: the global remap is written for another client (say + // SABnzbd sees Bindery's folder as /data), but this qBittorrent mounts + // Bindery's folder at the same path. The global remap still rewrites the + // folder it is sent, and the doctor says so and names the opt out. + t.Run("a global remap meant for another client is flagged", func(t *testing.T) { + host, port := qbitGrabServer(t, map[string]string{}, downloads) + resp, _ := runDiagnose(t, diagnoseSetup{downloadDir: downloads, remap: "/data:" + downloads}, qbitClient(host, port, "", "")) + path := wantDiagStatus(t, resp, diagCodeClientPath, diagWarn) + if resp.Paths[0].ClientPath != "/data" { + t.Errorf("clientPath = %q, want /data, the folder the grab is sent", resp.Paths[0].ClientPath) + } + if !strings.Contains(path.Message, "BINDERY_DOWNLOAD_PATH_REMAP") || !strings.Contains(path.Fix, downloads+":"+downloads) { + t.Errorf("check = %+v", path) + } + }) + + t.Run("an identity client remap opts out of the global one", func(t *testing.T) { + host, port := qbitGrabServer(t, map[string]string{}, downloads) + resp, _ := runDiagnose(t, diagnoseSetup{downloadDir: downloads, remap: "/data:" + downloads}, qbitClient(host, port, "", downloads+":"+downloads)) + wantDiagStatus(t, resp, diagCodeClientPath, diagPass) + if resp.Paths[0].ClientPath != downloads { + t.Errorf("clientPath = %q, want Bindery's own folder %q", resp.Paths[0].ClientPath, downloads) + } + wantDiagStatus(t, resp, diagCodeLocalPath, diagPass) + }) } // TestDiagnose_FilesystemSlotsAreCapped: repeated Diagnose calls against a diff --git a/internal/downloader/adapter_test.go b/internal/downloader/adapter_test.go index 97b715db7..dd66ab71a 100644 --- a/internal/downloader/adapter_test.go +++ b/internal/downloader/adapter_test.go @@ -1071,6 +1071,12 @@ func TestTorrentSavePath_RemapPrecedence(t *testing.T) { {name: "global remap to a Windows client", globalRemap: `D:\Torrents:/downloads`, downloadDir: "/downloads/books", want: `D:\Torrents\books`}, {name: "global remap to a network share", globalRemap: `\\nas\torrents:/downloads`, downloadDir: "/downloads/books", want: `\\nas\torrents\books`}, {name: "no download folder sends nothing", globalRemap: "/data:/downloads", want: ""}, + // A client that mounts Bindery's folder at the same path opts out of + // a global remap written for other clients with an identity rule. + // The rule matches without changing the path, which must still stop + // the global fallback. + {name: "identity client remap opts out of the global remap", clientRemap: "/downloads:/downloads", globalRemap: "/data:/downloads", downloadDir: "/downloads/books", want: "/downloads/books"}, + {name: "identity client remap on the folder itself", clientRemap: "/downloads:/downloads", globalRemap: "/data:/downloads", downloadDir: "/downloads", want: "/downloads"}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/downloader/clientpath.go b/internal/downloader/clientpath.go index ff459df83..dfcd3045f 100644 --- a/internal/downloader/clientpath.go +++ b/internal/downloader/clientpath.go @@ -2,6 +2,7 @@ package downloader import ( "context" + "fmt" "os" "path/filepath" "slices" @@ -118,7 +119,15 @@ func GrabSavePath(ctx context.Context, client *models.DownloadClient, mediaType, // save path exactly as they are, so the doctor has to use them the same // way to reach the same folder. category := ResolveCategory(client, mediaType) - sent := torrentSavePath(client, SendOptions{MediaType: mediaType, DownloadDir: downloadDir, AudiobookDownloadDir: audiobookDownloadDir, GlobalRemap: globalRemap}) + opts := SendOptions{MediaType: mediaType, DownloadDir: downloadDir, AudiobookDownloadDir: audiobookDownloadDir, GlobalRemap: globalRemap} + sent := sentSavePath{ + path: torrentSavePath(client, opts), + local: TargetDownloadDir(mediaType, downloadDir, audiobookDownloadDir), + } + if sent.path != "" { + opts.GlobalRemap = "" + sent.viaGlobal = sent.path != torrentSavePath(client, opts) + } info := ClientPathInfo{Category: category} switch client.Type { case "qbittorrent": @@ -126,11 +135,14 @@ func GrabSavePath(ctx context.Context, client *models.DownloadClient, mediaType, case "rtorrent": // rTorrent receives d.directory.set whenever Bindery has a download // folder, so directory.default only matters without one. - if sent != "" { - info.Path, info.Source = sent, "the save path Bindery sends" + dir, err := RtorrentFor(client).DefaultDirectory(ctx) + if sent.path != "" { + info.Path, info.Source = sent.path, "the save path Bindery sends" + if err == nil { + sent.noteAgainstDefault(&info, "rTorrent", dir) + } return info, nil } - dir, err := RtorrentFor(client).DefaultDirectory(ctx) if err != nil { return info, err } @@ -177,17 +189,64 @@ func GrabSavePath(ctx context.Context, client *models.DownloadClient, mediaType, } } +// sentSavePath is the save path a grab sends (torrentSavePath), with what the +// diagnose action needs to judge it. +type sentSavePath struct { + path string // as sent to the client; "" when Bindery sends none + local string // Bindery's own folder it was made from + viaGlobal bool // BINDERY_DOWNLOAD_PATH_REMAP produced it +} + +// noteAgainstDefault sets a note on info when the global remap produced the +// folder Bindery sends and that folder is unrelated to the client's own +// default save folder: not that folder, not inside it and not above it. The +// global remap always matches a sent folder, because its Bindery side is +// Bindery's own folder, so a global remap written for one client also +// rewrites what another client is sent. This is how that shows up when the +// other client mounts Bindery's folder at the same path (#2665). A folder from +// the client's own remap is the operator's explicit choice and is not second +// guessed, and a default the client did not report is not checked. +func (s sentSavePath) noteAgainstDefault(info *ClientPathInfo, clientName, def string) { + def = strings.TrimSpace(def) + if !s.viaGlobal || def == "" || clientPathsOverlap(s.path, def) { + return + } + info.Note = fmt.Sprintf("BINDERY_DOWNLOAD_PATH_REMAP turns Bindery's folder %q into %q for %s, but %s's own default save folder is %q, which is unrelated to it.", s.local, s.path, clientName, clientName, def) + info.NoteFix = fmt.Sprintf("If %s sees Bindery's download folder at the same path, set the path remap %q on this client, so it keeps that path instead of using the global remap. Otherwise check that %s can write to %q.", clientName, s.local+":"+s.local, clientName, s.path) +} + +// clientPathsOverlap reports whether two paths in a download client's +// namespace are the same folder or one lies inside the other. Windows paths +// compare without regard to case or separator style. +func clientPathsOverlap(a, b string) bool { + norm := func(p string) string { + c := pathmap.CleanClientPath(p) + if pathmap.IsWindowsPath(c) || pathmap.IsUNCPath(c) { + c = strings.ToLower(strings.ReplaceAll(c, `\`, "/")) + } + return strings.TrimRight(c, "/") + } + a, b = norm(a), norm(b) + under := func(child, parent string) bool { + return child == parent || strings.HasPrefix(child, parent+"/") + } + return under(a, b) || under(b, a) +} + // qbittorrentGrabPath follows addTorrentFields: with a category Bindery turns // on automatic torrent management and sends no save path, so the category // decides. Without one it sends the save path, or leaves the client default. -func qbittorrentGrabPath(ctx context.Context, client *models.DownloadClient, info ClientPathInfo, sent string) (ClientPathInfo, error) { +func qbittorrentGrabPath(ctx context.Context, client *models.DownloadClient, info ClientPathInfo, sent sentSavePath) (ClientPathInfo, error) { qb := QbittorrentFor(client) if info.Category == "" { - if sent != "" { - info.Path, info.Source = sent, "the save path Bindery sends" + def, err := qb.GetDefaultSavePath(ctx) + if sent.path != "" { + info.Path, info.Source = sent.path, "the save path Bindery sends" + if err == nil { + sent.noteAgainstDefault(&info, "qBittorrent", def) + } return info, nil } - def, err := qb.GetDefaultSavePath(ctx) if err != nil { return info, err } diff --git a/internal/downloader/clientpath_test.go b/internal/downloader/clientpath_test.go index a282ab15b..2862e3e14 100644 --- a/internal/downloader/clientpath_test.go +++ b/internal/downloader/clientpath_test.go @@ -73,3 +73,25 @@ func TestFindCaseInsensitivePathUnder(t *testing.T) { t.Errorf("followed an exact-named symlink out of base to %q", r) } } + +func TestClientPathsOverlap(t *testing.T) { + tests := []struct { + a, b string + want bool + }{ + {"/downloads", "/downloads", true}, + {"/downloads/books", "/downloads", true}, + {"/downloads", "/downloads/incomplete/", true}, + {"/data", "/downloads", false}, + {"/down", "/downloads", false}, + {"/", "/downloads", true}, + {`D:\Torrents\books`, `d:/torrents`, true}, + {`\\nas\share\books`, `\\NAS\share`, true}, + {`D:\Torrents`, `E:\Torrents`, false}, + } + for _, tc := range tests { + if got := clientPathsOverlap(tc.a, tc.b); got != tc.want { + t.Errorf("clientPathsOverlap(%q, %q) = %v, want %v", tc.a, tc.b, got, tc.want) + } + } +} diff --git a/internal/downloader/health.go b/internal/downloader/health.go index 5c5ce0da1..91a7195ec 100644 --- a/internal/downloader/health.go +++ b/internal/downloader/health.go @@ -195,7 +195,7 @@ func CheckDownloadClientHealth(ctx context.Context, client *models.DownloadClien return models.DownloadClientHealth{Status: HealthError, Message: err.Error()} } if client.Type == "qbittorrent" { - return checkQbittorrentCategoryPath(ctx, client, downloadDir, audiobookDownloadDir) + return checkQbittorrentCategoryPath(ctx, client, downloadDir, audiobookDownloadDir, globalRemap) } vis := CheckCompletedPathVisibility(ctx, client, downloadDir, audiobookDownloadDir, globalRemap) @@ -242,7 +242,7 @@ func TargetDownloadDir(mediaType, downloadDir, audiobookDownloadDir string) stri return strings.TrimSpace(downloadDir) } -func checkQbittorrentCategoryPath(ctx context.Context, client *models.DownloadClient, downloadDir, audiobookDownloadDir string) models.DownloadClientHealth { +func checkQbittorrentCategoryPath(ctx context.Context, client *models.DownloadClient, downloadDir, audiobookDownloadDir, globalRemap string) models.DownloadClientHealth { category := strings.TrimSpace(client.Category) if category == "" { return healthError("qBittorrent category is empty; configure a category with a save path") @@ -260,14 +260,14 @@ func checkQbittorrentCategoryPath(ctx context.Context, client *models.DownloadCl // Validate the ebook category first; if it fails, return immediately. // When CategoryAudiobook is set, validate it against audiobookDownloadDir // as well — both must be healthy for the client to be healthy (#700). - if h := validateQbittorrentCategorySavePath(ctx, qb, client, category, expected, categories); h.Status != HealthOK { + if h := validateQbittorrentCategorySavePath(ctx, qb, client, category, expected, globalRemap, categories); h.Status != HealthOK { return h } audioCategory := strings.TrimSpace(client.CategoryAudiobook) if audioCategory != "" && audioCategory != category { expectedAudio := cleanConfiguredDir(ExpectedDownloadDirForClient(client, models.MediaTypeAudiobook, downloadDir, audiobookDownloadDir)) - if h := validateQbittorrentCategorySavePath(ctx, qb, client, audioCategory, expectedAudio, categories); h.Status != HealthOK { + if h := validateQbittorrentCategorySavePath(ctx, qb, client, audioCategory, expectedAudio, globalRemap, categories); h.Status != HealthOK { return h } } @@ -302,7 +302,7 @@ func cleanConfiguredDir(dir string) string { // validateQbittorrentCategorySavePath checks that a single qBittorrent category // exists and that its (path-remapped) save path falls at or under expected. -func validateQbittorrentCategorySavePath(ctx context.Context, qb *qbittorrent.Client, client *models.DownloadClient, category, expected string, categories map[string]qbittorrent.Category) models.DownloadClientHealth { +func validateQbittorrentCategorySavePath(ctx context.Context, qb *qbittorrent.Client, client *models.DownloadClient, category, expected, globalRemap string, categories map[string]qbittorrent.Category) models.DownloadClientHealth { qbCategory, ok := categories[category] if !ok { if expected == "" { @@ -324,8 +324,9 @@ func validateQbittorrentCategorySavePath(ctx context.Context, qb *qbittorrent.Cl } // The remap runs on the save path exactly as qBittorrent reports it; only - // its result is a path on this host. - localPath := filepath.Clean(pathmap.Parse(client.PathRemap).Apply(savePath)) + // its result is a path on this host. Same precedence as the importer: + // the client's remap, then the global one. + localPath := filepath.Clean(remapClientPath(client, savePath, globalRemap)) // With no download directory configured there is nothing to be under, and // the stat below is the whole check (#2902). if expected != "" && !pathIsAtOrUnder(localPath, expected) { diff --git a/internal/downloader/health_test.go b/internal/downloader/health_test.go index 8a70331ad..0c806e6a1 100644 --- a/internal/downloader/health_test.go +++ b/internal/downloader/health_test.go @@ -400,3 +400,33 @@ func TestCheckDownloadClientHealth_MalformedHost(t *testing.T) { }) } } + +// TestQbittorrentCategoryPath_GlobalRemapOnly: the category health check +// resolves qBittorrent's save path with the importer's precedence, so a +// client covered only by BINDERY_DOWNLOAD_PATH_REMAP is healthy, as its +// imports are (#2665). +func TestQbittorrentCategoryPath_GlobalRemapOnly(t *testing.T) { + tmp := t.TempDir() + local := filepath.Join(tmp, "books") + if err := os.MkdirAll(local, 0o755); err != nil { + t.Fatal(err) + } + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v2/auth/login": + _, _ = w.Write([]byte("Ok.")) + case "/api/v2/torrents/categories": + _, _ = w.Write([]byte(`{"books":{"name":"books","savePath":"/qbit/books"}}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer srv.Close() + host, port := serverHostPort(t, srv.URL) + client := &models.DownloadClient{Type: "qbittorrent", Host: host, Port: port, Username: "u", Password: "p", Category: "books"} + + got := CheckDownloadClientHealth(context.Background(), client, tmp, "", "/qbit:"+tmp) + if got.Status != HealthOK { + t.Fatalf("status = %q, want %q; message=%s", got.Status, HealthOK, got.Message) + } +} diff --git a/internal/downloader/pathcheck.go b/internal/downloader/pathcheck.go index c8b3e2f39..e5109611f 100644 --- a/internal/downloader/pathcheck.go +++ b/internal/downloader/pathcheck.go @@ -142,11 +142,17 @@ func RemapClientPath(client *models.DownloadClient, rawPath string, global *path // Bindery's filesystem in the download client's own namespace, for a path // Bindery sends to the client. The precedence is the same, so whatever is // sent reads back to where it started: the client's own PathRemap first, and -// only if that leaves the path unchanged the global BINDERY_DOWNLOAD_PATH_REMAP -// (#2665). +// only if none of its rules covers the path the global +// BINDERY_DOWNLOAD_PATH_REMAP (#2665). +// +// "Covers" means a rule matched, not that the path changed. A client that +// mounts Bindery's download folder at the same path, next to a global remap +// written for other clients, opts out of the global remap with an identity +// rule such as "/downloads:/downloads"; that rule matches and maps the folder +// to itself, so the global remap is not consulted. func ClientSidePath(client *models.DownloadClient, localPath string, global *pathmap.Remapper) string { if client != nil && strings.TrimSpace(client.PathRemap) != "" { - if clientPath := pathmap.Parse(client.PathRemap).ApplyInverse(localPath); clientPath != localPath { + if clientPath, matched := pathmap.Parse(client.PathRemap).ApplyInverseMatched(localPath); matched { return clientPath } } diff --git a/internal/downloader/rtorrent_test.go b/internal/downloader/rtorrent_test.go index f0ea205ab..a33626d28 100644 --- a/internal/downloader/rtorrent_test.go +++ b/internal/downloader/rtorrent_test.go @@ -532,3 +532,38 @@ func TestSendDownload_RtorrentGlobalRemapOnly(t *testing.T) { t.Errorf("download directory not run back through the global remap: %s", body) } } + +// TestSendDownload_RtorrentMixedSetup is a global remap written for one client +// (SABnzbd sees Bindery's /downloads as /data) next to an rTorrent that mounts +// Bindery's /downloads directly. The global remap's Bindery side always +// matches a sent folder, so without an opt out every client gets /data; an +// identity remap on rTorrent keeps it on /downloads, while a second rTorrent +// with no remap of its own still follows the global one (#2665). +func TestSendDownload_RtorrentMixedSetup(t *testing.T) { + const global = "/data:/downloads" + cases := []struct { + name string + clientRemap string + want string + }{ + {name: "identity remap keeps Bindery's folder", clientRemap: "/downloads:/downloads", want: "/downloads"}, + {name: "no client remap follows the global one", want: "/data"}, + } + for i, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + stub := newRtorrentStub(t, "0", "") + client := stub.client(t, int64(106+i)) + client.PathRemap = tc.clientRemap + magnet := "magnet:?xt=urn:btih:" + rtorrentTestHash + if _, err := SendDownload(context.Background(), client, magnet, "The Book", SendOptions{ + DownloadDir: "/downloads", + GlobalRemap: global, + }); err != nil { + t.Fatalf("SendDownload: %v", err) + } + if body := stub.allBodies(); !strings.Contains(body, "d.directory.set=""+tc.want+""") { + t.Errorf("want d.directory.set=%q in: %s", tc.want, body) + } + }) + } +} diff --git a/internal/pathmap/remap.go b/internal/pathmap/remap.go index 39da7f5f6..37d32afa7 100644 --- a/internal/pathmap/remap.go +++ b/internal/pathmap/remap.go @@ -115,8 +115,17 @@ func Validate(spec string) error { // platform: a Windows source mapped to a POSIX destination yields forward // slashes. If no rule matches, p is returned unchanged. func (r *Remapper) Apply(p string) string { + out, _ := r.ApplyMatched(p) + return out +} + +// ApplyMatched is Apply that also reports whether a rule matched. A rule that +// maps a prefix onto itself (`/downloads:/downloads`) matches while leaving +// the path unchanged, which comparing Apply's result with p cannot tell apart +// from no rule matching at all. +func (r *Remapper) ApplyMatched(p string) (string, bool) { if r == nil || len(r.rules) == 0 || p == "" { - return p + return p, false } for _, rule := range r.rules { rest, ok := matchPrefix(p, rule.fromKey, rule.fromWin) @@ -124,19 +133,26 @@ func (r *Remapper) Apply(p string) string { continue } if rest == "" { - return rule.to + return rule.to, true } - return joinRemainder(rule.to, rule.toWin, rule.toSep, rest, rule.fromWin) + return joinRemainder(rule.to, rule.toWin, rule.toSep, rest, rule.fromWin), true } - return p + return p, false } // ApplyInverse rewrites p in the opposite direction, from a Bindery-visible // path back to the external service mount point. It round-trips Apply, // including the separator style the Windows side was configured with. func (r *Remapper) ApplyInverse(p string) string { + out, _ := r.ApplyInverseMatched(p) + return out +} + +// ApplyInverseMatched is ApplyInverse that also reports whether a rule +// matched, for the same reason as ApplyMatched. +func (r *Remapper) ApplyInverseMatched(p string) (string, bool) { if r == nil || len(r.rules) == 0 || p == "" { - return p + return p, false } var best *remapRule var bestRest string @@ -151,12 +167,12 @@ func (r *Remapper) ApplyInverse(p string) string { } } if best == nil { - return p + return p, false } if bestRest == "" { - return best.from + return best.from, true } - return joinRemainder(best.from, best.fromWin, best.fromSep, bestRest, best.toWin) + return joinRemainder(best.from, best.fromWin, best.fromSep, bestRest, best.toWin), true } // Empty reports whether the remapper has no rules. diff --git a/internal/pathmap/remap_test.go b/internal/pathmap/remap_test.go index d878e2384..9c2ae02c2 100644 --- a/internal/pathmap/remap_test.go +++ b/internal/pathmap/remap_test.go @@ -319,3 +319,42 @@ func TestValidateShares(t *testing.T) { t.Fatalf("Validate share with no destination = %v, want a share example", err) } } + +// TestApplyMatchedReportsIdentityRules: an identity rule matches without +// changing the path, which only the matched flag can show (#2665). +func TestApplyMatchedReportsIdentityRules(t *testing.T) { + cases := []struct { + spec, path, wantFwd string + fwdMatched bool + wantInv string + invMatched bool + }{ + {"/downloads:/downloads", "/downloads/book", "/downloads/book", true, "/downloads/book", true}, + {"/downloads:/downloads", "/other/book", "/other/book", false, "/other/book", false}, + {"/data:/downloads", "/downloads/book", "/downloads/book", false, "/data/book", true}, + {"/data:/downloads", "/data/book", "/downloads/book", true, "/data/book", false}, + {`S:\Books:/books`, `s:/books/a`, "/books/a", true, `s:/books/a`, false}, + {"", "/downloads/book", "/downloads/book", false, "/downloads/book", false}, + } + for _, tc := range cases { + r := Parse(tc.spec) + got, ok := r.ApplyMatched(tc.path) + if got != tc.wantFwd || ok != tc.fwdMatched { + t.Errorf("Parse(%q).ApplyMatched(%q) = %q, %v; want %q, %v", tc.spec, tc.path, got, ok, tc.wantFwd, tc.fwdMatched) + } + if plain := r.Apply(tc.path); plain != got { + t.Errorf("Apply(%q) = %q, disagrees with ApplyMatched %q", tc.path, plain, got) + } + got, ok = r.ApplyInverseMatched(tc.path) + if got != tc.wantInv || ok != tc.invMatched { + t.Errorf("Parse(%q).ApplyInverseMatched(%q) = %q, %v; want %q, %v", tc.spec, tc.path, got, ok, tc.wantInv, tc.invMatched) + } + if plain := r.ApplyInverse(tc.path); plain != got { + t.Errorf("ApplyInverse(%q) = %q, disagrees with ApplyInverseMatched %q", tc.path, plain, got) + } + } + var nilRemapper *Remapper + if got, ok := nilRemapper.ApplyInverseMatched("/x"); got != "/x" || ok { + t.Errorf("nil remapper = %q, %v", got, ok) + } +} From 4e8b901bd99246dbb9147f445b164a5cce7d7d54 Mon Sep 17 00:00:00 2001 From: vavallee Date: Mon, 5 Oct 2026 00:53:00 -0300 Subject: [PATCH 3/3] fix(downloader): flag only the mixed remap setup in Diagnose (#2665) The sent folder warning fired whenever the client's default save folder was unrelated to the folder Bindery sends, which covers plenty of correct setups: a dedicated Bindery folder next to the client default, rTorrent's split incomplete and complete folders, and relative or tilde defaults (stock rTorrent ships with "./"). Its suggested identity remap would have broken those. It now warns only on the signature of a global remap meant for another client: the global remap produced the sent folder, but the client's default is an absolute path on Bindery's own folder rather than on the sent one. Non absolute defaults are not checked. The note says to ignore it if the client really keeps that storage at the remapped path, the one false positive left. Signed-off-by: vavallee Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 --- docs/API.md | 2 +- docs/DEPLOYMENT.md | 2 +- internal/downloader/clientpath.go | 32 ++++++++++++++--------- internal/downloader/clientpath_test.go | 36 ++++++++++++++++++++++++++ 4 files changed, 58 insertions(+), 14 deletions(-) diff --git a/docs/API.md b/docs/API.md index bf0a3a3de..2a8c0b4db 100644 --- a/docs/API.md +++ b/docs/API.md @@ -604,7 +604,7 @@ only, never a path, and it runs only when called. * The folder checked is where a grab actually lands, worked out the way the grab itself is sent: the save path Bindery sends (rTorrent always, qBittorrent without a category, Transmission with an absolute category), the category save path (qBittorrent, with an empty one meaning the default save path plus the category name), the category folder (SABnzbd), the category DestDir or DestDir plus the category name when `AppendCategoryDir` is on (NZBGet), a Deluge label's move completed path, or the client default. `source` names which. The category is used exactly as a grab sends it: a category with spaces around it gives `client_path: warn`. A Deluge category is sent lowercased, because the Label plugin stores labels in lowercase and only accepts that form when labelling a torrent, so `Books` uses the `books` label's move completed path. * For Deluge, `category` compares the configured categories, lowercased, with the labels the Label plugin lists, and says when it lowercases one. When the plugin is off it cannot list labels and `category` is `unknown`. * Ebook and audiobook grabs are checked separately, because each resolves its own category and download folder. When both land in the same folder there is one `paths` row and the `client_path`, `remap` and `local_path` checks have no `mediaType`; otherwise each carries `mediaType` `ebook` or `audiobook`. -* `remapRule` is `client` (this client's path remap changed the path), `global` (`BINDERY_DOWNLOAD_PATH_REMAP` did) or `none`. A Windows drive path fails for a missing remap only when Bindery itself is not running on Windows. When no remap applies and the client's folder is not under any folder Bindery uses, `remap` is `warn` and `local_path` carries the failure and the fix. When Bindery sends the save path itself (rTorrent, qBittorrent without a category) it runs its download folder through the same remaps in reverse, the client's own first and `BINDERY_DOWNLOAD_PATH_REMAP` as the fallback, so `clientPath` is the folder the client is actually given. A client remap rule that covers the folder wins even when it maps the folder to itself (`/downloads:/downloads`), which is how a client opts out of a global remap meant for others. When the global remap produced the sent folder and the client's own default save folder is neither that folder nor inside or above it, `client_path` is `warn` and the fix names that identity remap. +* `remapRule` is `client` (this client's path remap changed the path), `global` (`BINDERY_DOWNLOAD_PATH_REMAP` did) or `none`. A Windows drive path fails for a missing remap only when Bindery itself is not running on Windows. When no remap applies and the client's folder is not under any folder Bindery uses, `remap` is `warn` and `local_path` carries the failure and the fix. When Bindery sends the save path itself (rTorrent, qBittorrent without a category) it runs its download folder through the same remaps in reverse, the client's own first and `BINDERY_DOWNLOAD_PATH_REMAP` as the fallback, so `clientPath` is the folder the client is actually given. A client remap rule that covers the folder wins even when it maps the folder to itself (`/downloads:/downloads`), which is how a client opts out of a global remap meant for others. When the global remap produced the sent folder but the client's own default save folder is an absolute path on Bindery's own folder (that folder, inside it or above it) rather than on the sent one, `client_path` is `warn` and the fix names that identity remap. A relative default such as rTorrent's stock `./` is not checked. * `hardlinks` has one row per download folder and library root pair, `{mediaType, downloadPath, root, result, linkable, reason}`. Every pair gets a real link probe, because two bind mounts of one filesystem share a device ID yet refuse links across them. `result` is `yes`, `no`, `unknown` (Bindery could not write a test file in the download folder) or `missing` (the library folder does not exist and was not probed). * `primaryFix` is the fix of the first failure, or of the first warning when nothing failed. diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 9b349be1e..0105b5685 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -379,7 +379,7 @@ Set a download-client path remap in **Settings → Download clients** or set the The same remaps also run in reverse, with the same precedence, when Bindery tells a client where to save a grab. rTorrent always gets a download directory, and qBittorrent gets a save path when the client has no category set. That directory is Bindery's download folder (`BINDERY_DOWNLOAD_DIR`, or `BINDERY_AUDIOBOOK_DOWNLOAD_DIR` for audiobooks) translated into the client's path: by the client's own remap when it covers that folder, otherwise by `BINDERY_DOWNLOAD_PATH_REMAP`. With `BINDERY_DOWNLOAD_PATH_REMAP=/data:/downloads` and no client remap, rTorrent is told to save into `/data`. Bindery 1.40.0 and earlier applied only the client's own remap here, so with just the global remap set the client was sent Bindery's `/downloads`. -On the way out the global remap matches every client, because its right side is Bindery's own folder. If the global remap is written for one client (say SABnzbd sees Bindery's `/downloads` as `/data`) and an rTorrent or category-less qBittorrent mounts Bindery's folder at the same path, give that client an identity path remap, `/downloads:/downloads`. A client remap that covers the folder always wins, even when it maps it to itself, so that client keeps being sent `/downloads`. **Diagnose** on the client warns when the global remap produced the folder it sends and the client's own default save folder is unrelated to it. +On the way out the global remap matches every client, because its right side is Bindery's own folder. If the global remap is written for one client (say SABnzbd sees Bindery's `/downloads` as `/data`) and an rTorrent or category-less qBittorrent mounts Bindery's folder at the same path, give that client an identity path remap, `/downloads:/downloads`. A client remap that covers the folder always wins, even when it maps it to itself, so that client keeps being sent `/downloads`. **Diagnose** on the client warns when the global remap produced the folder it sends while the client's own default save folder sits on Bindery's path, which is what that setup looks like. If the client really keeps that storage at the remapped path, ignore the warning. Per-client remaps are stored on each download client, so separate qBittorrent / SABnzbd / NZBGet instances can map different mount points. Existing download clients keep an empty remap after upgrade, which preserves the previous global-only behavior until you add a client-specific value. diff --git a/internal/downloader/clientpath.go b/internal/downloader/clientpath.go index dfcd3045f..d074cf3d8 100644 --- a/internal/downloader/clientpath.go +++ b/internal/downloader/clientpath.go @@ -197,22 +197,30 @@ type sentSavePath struct { viaGlobal bool // BINDERY_DOWNLOAD_PATH_REMAP produced it } -// noteAgainstDefault sets a note on info when the global remap produced the -// folder Bindery sends and that folder is unrelated to the client's own -// default save folder: not that folder, not inside it and not above it. The -// global remap always matches a sent folder, because its Bindery side is -// Bindery's own folder, so a global remap written for one client also -// rewrites what another client is sent. This is how that shows up when the -// other client mounts Bindery's folder at the same path (#2665). A folder from -// the client's own remap is the operator's explicit choice and is not second -// guessed, and a default the client did not report is not checked. +// noteAgainstDefault sets a note on info for the signature of a global remap +// meant for a different client (#2665): the global remap produced the folder +// Bindery sends, yet the client's own default save folder sits on Bindery's +// own folder (that folder, inside it or above it) rather than on the folder +// it is sent. The global remap always matches a sent folder, because its +// Bindery side is Bindery's own folder, so a remap written for SABnzbd also +// rewrites what an rTorrent mounting Bindery's path directly is sent. +// +// Anything else is left alone, because a default unrelated to both folders is +// normal: a dedicated Bindery folder next to the client's default, or +// separate incomplete and complete folders. A default that is not an +// absolute path (rTorrent ships with "./", and "~/download" is common) says +// nothing about mounts and is not checked, nor is a folder from the client's +// own remap, which is the operator's explicit choice. func (s sentSavePath) noteAgainstDefault(info *ClientPathInfo, clientName, def string) { def = strings.TrimSpace(def) - if !s.viaGlobal || def == "" || clientPathsOverlap(s.path, def) { + if !s.viaGlobal || !pathmap.IsAbsClientPath(def) || s.local == "" { + return + } + if !clientPathsOverlap(def, s.local) || clientPathsOverlap(def, s.path) { return } - info.Note = fmt.Sprintf("BINDERY_DOWNLOAD_PATH_REMAP turns Bindery's folder %q into %q for %s, but %s's own default save folder is %q, which is unrelated to it.", s.local, s.path, clientName, clientName, def) - info.NoteFix = fmt.Sprintf("If %s sees Bindery's download folder at the same path, set the path remap %q on this client, so it keeps that path instead of using the global remap. Otherwise check that %s can write to %q.", clientName, s.local+":"+s.local, clientName, s.path) + info.Note = fmt.Sprintf("BINDERY_DOWNLOAD_PATH_REMAP turns Bindery's folder %q into %q for %s, but %s's own default save folder %q is on Bindery's path, so the global remap may be meant for a different client. If %s really keeps this storage at %q, ignore this.", s.local, s.path, clientName, clientName, def, clientName, s.path) + info.NoteFix = fmt.Sprintf("If %s sees Bindery's download folder at the same path, set the path remap %q on this client, so it keeps that path instead of using the global remap.", clientName, s.local+":"+s.local) } // clientPathsOverlap reports whether two paths in a download client's diff --git a/internal/downloader/clientpath_test.go b/internal/downloader/clientpath_test.go index 2862e3e14..f636c28bd 100644 --- a/internal/downloader/clientpath_test.go +++ b/internal/downloader/clientpath_test.go @@ -3,6 +3,7 @@ package downloader import ( "os" "path/filepath" + "strings" "testing" "github.com/vavallee/bindery/internal/fsutil" @@ -95,3 +96,38 @@ func TestClientPathsOverlap(t *testing.T) { } } } + +// TestNoteAgainstDefault: the warning fires only for the mixed setup, a +// client whose default sits on Bindery's own folder while the global remap +// sends it somewhere else, and stays quiet on correct setups (#2665). +func TestNoteAgainstDefault(t *testing.T) { + tests := []struct { + name string + sent sentSavePath + def string + warn bool + }{ + {"mixed setup: default is Bindery's folder", sentSavePath{path: "/data", local: "/downloads", viaGlobal: true}, "/downloads", true}, + {"mixed setup: default inside Bindery's folder", sentSavePath{path: "/data/books", local: "/downloads/books", viaGlobal: true}, "/downloads/books/complete", true}, + {"mixed setup: default above Bindery's folder", sentSavePath{path: "/data/books", local: "/downloads/books", viaGlobal: true}, "/downloads", true}, + {"default on the sent folder", sentSavePath{path: "/data/books", local: "/downloads/books", viaGlobal: true}, "/data", false}, + {"dedicated folder next to the client default", sentSavePath{path: "/downloads/books", local: "/media/books", viaGlobal: true}, "/downloads/complete", false}, + {"rTorrent split incomplete and complete", sentSavePath{path: "/seedbox/complete/books", local: "/media/books", viaGlobal: true}, "/seedbox/incomplete", false}, + {"stock rTorrent relative default", sentSavePath{path: "/data", local: "/downloads", viaGlobal: true}, "./", false}, + {"tilde default", sentSavePath{path: "/data", local: "/downloads", viaGlobal: true}, "~/download", false}, + {"no default reported", sentSavePath{path: "/data", local: "/downloads", viaGlobal: true}, "", false}, + {"folder from the client's own remap", sentSavePath{path: "/data", local: "/downloads", viaGlobal: false}, "/downloads", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var info ClientPathInfo + tc.sent.noteAgainstDefault(&info, "rTorrent", tc.def) + if got := info.Note != ""; got != tc.warn { + t.Fatalf("warned = %v, want %v (note %q)", got, tc.warn, info.Note) + } + if tc.warn && (!strings.Contains(info.NoteFix, tc.sent.local+":"+tc.sent.local) || !strings.Contains(info.Note, "ignore this")) { + t.Errorf("note = %q, fix = %q", info.Note, info.NoteFix) + } + }) + } +}