From ede01d703b46f4444c648bf272072ff0d21afb5a Mon Sep 17 00:00:00 2001 From: vavallee Date: Wed, 14 Oct 2026 21:08:39 -0300 Subject: [PATCH 1/8] feat(db): add a nullable download client pin to indexers (#3080) Storage only: migration 109 and the column plumbing in the indexer repo. Routing and validation follow in later commits. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- internal/db/indexers.go | 18 +++++++++--------- .../migrations/109_indexer_download_client.sql | 17 +++++++++++++++++ internal/models/indexer.go | 7 +++++++ 3 files changed, 33 insertions(+), 9 deletions(-) create mode 100644 internal/db/migrations/109_indexer_download_client.sql diff --git a/internal/db/indexers.go b/internal/db/indexers.go index bee0836d4..dedf9a505 100644 --- a/internal/db/indexers.go +++ b/internal/db/indexers.go @@ -23,7 +23,7 @@ func (r *IndexerRepo) List(ctx context.Context) ([]models.Indexer, error) { SELECT id, name, type, url, api_key, categories, include_parent_categories, priority, enabled, supports_search, prowlarr_instance_id, prowlarr_indexer_id, seed_ratio, seed_ratio_source, freeleech_only, seed_time_minutes, seed_time_source, inactive_seed_time_minutes, - daily_query_limit, + daily_query_limit, download_client_id, last_error, last_error_code, last_failure_at, last_success_at, created_at, updated_at FROM indexers ORDER BY priority`) if err != nil { @@ -47,7 +47,7 @@ func (r *IndexerRepo) GetByID(ctx context.Context, id int64) (*models.Indexer, e SELECT id, name, type, url, api_key, categories, include_parent_categories, priority, enabled, supports_search, prowlarr_instance_id, prowlarr_indexer_id, seed_ratio, seed_ratio_source, freeleech_only, seed_time_minutes, seed_time_source, inactive_seed_time_minutes, - daily_query_limit, + daily_query_limit, download_client_id, last_error, last_error_code, last_failure_at, last_success_at, created_at, updated_at FROM indexers WHERE id=?`, id) if err != nil { @@ -70,7 +70,7 @@ func (r *IndexerRepo) ListByProwlarrInstance(ctx context.Context, instanceID int SELECT id, name, type, url, api_key, categories, include_parent_categories, priority, enabled, supports_search, prowlarr_instance_id, prowlarr_indexer_id, seed_ratio, seed_ratio_source, freeleech_only, seed_time_minutes, seed_time_source, inactive_seed_time_minutes, - daily_query_limit, + daily_query_limit, download_client_id, last_error, last_error_code, last_failure_at, last_success_at, created_at, updated_at FROM indexers WHERE prowlarr_instance_id=?`, instanceID) if err != nil { @@ -98,13 +98,13 @@ func (r *IndexerRepo) Create(ctx context.Context, idx *models.Indexer) error { INSERT INTO indexers (name, type, url, api_key, categories, include_parent_categories, priority, enabled, supports_search, prowlarr_instance_id, prowlarr_indexer_id, seed_ratio, seed_ratio_source, freeleech_only, seed_time_minutes, seed_time_source, inactive_seed_time_minutes, - daily_query_limit, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + daily_query_limit, download_client_id, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, idx.Name, idx.Type, idx.URL, idx.APIKey, string(catsJSON), idx.IncludeParentCategories, idx.Priority, idx.Enabled, idx.SupportsSearch, idx.ProwlarrInstanceID, idx.ProwlarrIndexerID, idx.SeedRatio, idx.SeedRatioSource, idx.FreeleechOnly, idx.SeedTimeMinutes, idx.SeedTimeSource, idx.InactiveSeedTimeMinutes, - idx.DailyQueryLimit, now, now) + idx.DailyQueryLimit, idx.DownloadClientID, now, now) if err != nil { return fmt.Errorf("create indexer: %w", err) } @@ -128,12 +128,12 @@ func (r *IndexerRepo) Update(ctx context.Context, idx *models.Indexer) error { UPDATE indexers SET name=?, type=?, url=?, api_key=?, categories=?, include_parent_categories=?, priority=?, enabled=?, supports_search=?, seed_ratio=?, seed_ratio_source=?, freeleech_only=?, seed_time_minutes=?, seed_time_source=?, inactive_seed_time_minutes=?, - daily_query_limit=?, updated_at=? + daily_query_limit=?, download_client_id=?, updated_at=? WHERE id=?`, idx.Name, idx.Type, idx.URL, idx.APIKey, string(catsJSON), idx.IncludeParentCategories, idx.Priority, idx.Enabled, idx.SupportsSearch, idx.SeedRatio, idx.SeedRatioSource, idx.FreeleechOnly, idx.SeedTimeMinutes, idx.SeedTimeSource, idx.InactiveSeedTimeMinutes, - idx.DailyQueryLimit, now, idx.ID) + idx.DailyQueryLimit, idx.DownloadClientID, now, idx.ID) return err } @@ -176,7 +176,7 @@ func scanIndexer(s indexerScanner) (models.Indexer, error) { &catsJSON, &includeParentCategories, &idx.Priority, &enabled, &supportsSearch, &idx.ProwlarrInstanceID, &idx.ProwlarrIndexerID, &idx.SeedRatio, &idx.SeedRatioSource, &freeleechOnly, &idx.SeedTimeMinutes, &idx.SeedTimeSource, &idx.InactiveSeedTimeMinutes, - &idx.DailyQueryLimit, + &idx.DailyQueryLimit, &idx.DownloadClientID, &idx.LastError, &idx.LastErrorCode, &idx.LastFailureAt, &idx.LastSuccessAt, &idx.CreatedAt, &idx.UpdatedAt, ); err != nil { diff --git a/internal/db/migrations/109_indexer_download_client.sql b/internal/db/migrations/109_indexer_download_client.sql new file mode 100644 index 000000000..9d76db8fa --- /dev/null +++ b/internal/db/migrations/109_indexer_download_client.sql @@ -0,0 +1,17 @@ +-- +migrate Up +-- Per-indexer download client pin (#3080). +-- +-- download_client_id names the one client every grab from this indexer must go +-- to, so a private tracker's releases never reach the public client. NULL, which +-- is what every existing row gets on upgrade, keeps today's routing unchanged. +-- +-- ON DELETE RESTRICT is the backstop for the API's own refusal (409 naming the +-- indexers): the database must never be allowed to quietly turn a pinned +-- indexer back into an unpinned one, because that is exactly the fallback this +-- column exists to prevent. +-- NOTE: the migration runner splits on semicolons, so keep them out of comments. +ALTER TABLE indexers ADD COLUMN download_client_id INTEGER REFERENCES download_clients(id) ON DELETE RESTRICT; + +-- +migrate Down +-- SQLite before 3.35 cannot DROP COLUMN. download_client_id is additive and NULL +-- already reads as "no pin" to every reader, so leaving it in place is a no-op. diff --git a/internal/models/indexer.go b/internal/models/indexer.go index d07f39dd4..21108568a 100644 --- a/internal/models/indexer.go +++ b/internal/models/indexer.go @@ -86,6 +86,13 @@ type Indexer struct { // of the cap is spent. It is never persisted and anything a client sends in // it is ignored. nil on an indexer with no cap set. DailyQueriesUsed *int `json:"dailyQueriesUsed,omitempty"` + // DownloadClientID pins this indexer to one download client (#3080). nil, + // the default, keeps today's routing. When set, every grab from this + // indexer goes to that client and to no other: a disabled, missing or + // wrong protocol client fails the grab instead of falling back. It is local + // configuration like Priority and the seed limits, so a Prowlarr sync keeps + // it. + DownloadClientID *int64 `json:"downloadClientId"` // Search health, written by the searcher rather than by the user (#1935). // All four are nil on an indexer that has never been searched. // From 9672bbd81733b060e3b5c58b0a8bd52f130c6752 Mon Sep 17 00:00:00 2001 From: vavallee Date: Wed, 14 Oct 2026 21:27:13 -0300 Subject: [PATCH 2/8] feat(indexer): send an indexer's grabs to a pinned download client (#3080) An indexer can be pinned to one download client. Every grab from it (scheduled, interactive, pending, retry) goes to that client, ahead of priority and media type routing. A disabled, missing or wrong protocol client fails the grab with a message naming the indexer and client, and never falls back. Deleting a client an indexer is pinned to is refused with 409 listing the indexers. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- cmd/bindery/main.go | 1 + internal/api/download_clients.go | 8 + internal/api/indexer_pin_api_test.go | 238 +++++++++ internal/api/indexer_pin_grab_test.go | 456 ++++++++++++++++++ internal/api/indexers.go | 69 ++- internal/api/queue.go | 114 ++++- internal/db/download_client_pin.go | 130 +++++ internal/db/download_clients.go | 17 +- internal/db/indexer_download_client_test.go | 88 ++++ internal/models/indexer.go | 9 + internal/prowlarr/syncer_pin_test.go | 76 +++ internal/scheduler/scheduler.go | 62 ++- .../scheduler/scheduler_indexer_pin_test.go | 216 +++++++++ 13 files changed, 1454 insertions(+), 30 deletions(-) create mode 100644 internal/api/indexer_pin_api_test.go create mode 100644 internal/api/indexer_pin_grab_test.go create mode 100644 internal/db/download_client_pin.go create mode 100644 internal/db/indexer_download_client_test.go create mode 100644 internal/prowlarr/syncer_pin_test.go create mode 100644 internal/scheduler/scheduler_indexer_pin_test.go diff --git a/cmd/bindery/main.go b/cmd/bindery/main.go index 3fd7a20aa..c5195c5f4 100644 --- a/cmd/bindery/main.go +++ b/cmd/bindery/main.go @@ -666,6 +666,7 @@ func main() { WithAliases(authorAliasRepo). WithQualityProfiles(qualityProfileRepo). WithEditions(editionRepo). + WithDownloadClients(dlClientRepo). WithSearchResults(searchResults). WithSearchDebugLog(searchDebugLog) if clients, err := dlClientRepo.List(ctxBoot); err == nil { diff --git a/internal/api/download_clients.go b/internal/api/download_clients.go index 54e717ee9..56792fee0 100644 --- a/internal/api/download_clients.go +++ b/internal/api/download_clients.go @@ -403,6 +403,14 @@ func (h *DownloadClientHandler) Delete(w http.ResponseWriter, r *http.Request) { return } if err := h.clients.Delete(r.Context(), id); err != nil { + // An indexer pinned to this client (#3080): refuse, and name them, + // because deleting it would quietly send their grabs to the default + // client, which is what the pin exists to prevent. + var pinned *db.ClientPinnedError + if errors.As(err, &pinned) { + writeJSON(w, http.StatusConflict, map[string]any{"error": pinned.Error(), "indexers": pinned.Indexers}) + return + } writeServerError(w, r, err) return } diff --git a/internal/api/indexer_pin_api_test.go b/internal/api/indexer_pin_api_test.go new file mode 100644 index 000000000..7c53ccbea --- /dev/null +++ b/internal/api/indexer_pin_api_test.go @@ -0,0 +1,238 @@ +package api + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "testing" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/models" +) + +// pinAPIFixture is an indexer handler and a download client handler over one +// database, with a usenet and a torrent client to pin against. +type pinAPIFixture struct { + ih *IndexerHandler + ch *DownloadClientHandler + clients *db.DownloadClientRepo + usenet *models.DownloadClient + torrent *models.DownloadClient + off *models.DownloadClient +} + +func newPinAPIFixture(t *testing.T) *pinAPIFixture { + t.Helper() + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + clients := db.NewDownloadClientRepo(database) + indexers := db.NewIndexerRepo(database) + ih := NewIndexerHandler(indexers, db.NewBookRepo(database), db.NewAuthorRepo(database), + db.NewMetadataProfileRepo(database), nil, db.NewSettingsRepo(database), db.NewBlocklistRepo(database)). + WithDownloadClients(clients) + ch := NewDownloadClientHandler(clients) + ctx := t.Context() + f := &pinAPIFixture{ih: ih, ch: ch, clients: clients, + usenet: &models.DownloadClient{Name: "sab", Type: "sabnzbd", Host: "192.168.1.70", Port: 8080, Enabled: true}, + torrent: &models.DownloadClient{Name: "qbit-private", Type: "qbittorrent", Host: "192.168.1.71", Port: 8081, Enabled: true}, + off: &models.DownloadClient{Name: "qbit-off", Type: "qbittorrent", Host: "192.168.1.72", Port: 8082, Enabled: false}, + } + for _, c := range []*models.DownloadClient{f.usenet, f.torrent, f.off} { + if err := clients.Create(ctx, c); err != nil { + t.Fatal(err) + } + } + return f +} + +func (f *pinAPIFixture) create(body string) *httptest.ResponseRecorder { + rec := httptest.NewRecorder() + f.ih.Create(rec, httptest.NewRequest(http.MethodPost, "/indexer", bytes.NewBufferString(body))) + return rec +} + +func (f *pinAPIFixture) update(id int64, body string) *httptest.ResponseRecorder { + rec := httptest.NewRecorder() + idStr := strconv.FormatInt(id, 10) + f.ih.Update(rec, withURLParam(httptest.NewRequest(http.MethodPut, "/indexer/"+idStr, bytes.NewBufferString(body)), "id", idStr)) + return rec +} + +func decodeIndexer(t *testing.T, rec *httptest.ResponseRecorder) models.Indexer { + t.Helper() + var out models.Indexer + if err := json.NewDecoder(rec.Body).Decode(&out); err != nil { + t.Fatalf("decode: %v", err) + } + return out +} + +func TestIndexerPin_CreateAndUpdateValidateAndPersist(t *testing.T) { + f := newPinAPIFixture(t) + torrentID := strconv.FormatInt(f.torrent.ID, 10) + + // A torznab indexer pinned to a torrent client saves and echoes the pin. + rec := f.create(`{"name":"Priv","url":"http://192.168.1.50:9117/api","apiKey":"k","type":"torznab","downloadClientId":` + torrentID + `}`) + if rec.Code != http.StatusCreated { + t.Fatalf("create: %d %s", rec.Code, rec.Body.String()) + } + created := decodeIndexer(t, rec) + if created.DownloadClientID == nil || *created.DownloadClientID != f.torrent.ID { + t.Fatalf("created pin = %v, want %d", created.DownloadClientID, f.torrent.ID) + } + + // An update that omits the key keeps the pin. + rec = f.update(created.ID, `{"name":"Priv renamed"}`) + if rec.Code != http.StatusOK { + t.Fatalf("rename: %d %s", rec.Code, rec.Body.String()) + } + if got := decodeIndexer(t, rec); got.DownloadClientID == nil || *got.DownloadClientID != f.torrent.ID { + t.Errorf("an update that omitted the pin changed it: %v", got.DownloadClientID) + } + + // An explicit null clears it. + rec = f.update(created.ID, `{"downloadClientId":null}`) + if rec.Code != http.StatusOK { + t.Fatalf("clear: %d %s", rec.Code, rec.Body.String()) + } + if got := decodeIndexer(t, rec); got.DownloadClientID != nil { + t.Errorf("null did not clear the pin: %v", *got.DownloadClientID) + } + + // And it can be set again on update. + rec = f.update(created.ID, `{"downloadClientId":`+torrentID+`}`) + if rec.Code != http.StatusOK { + t.Fatalf("set: %d %s", rec.Code, rec.Body.String()) + } + if got := decodeIndexer(t, rec); got.DownloadClientID == nil || *got.DownloadClientID != f.torrent.ID { + t.Errorf("set pin = %v", got.DownloadClientID) + } +} + +func TestIndexerPin_RejectedOnSave(t *testing.T) { + f := newPinAPIFixture(t) + usenetID := strconv.FormatInt(f.usenet.ID, 10) + offID := strconv.FormatInt(f.off.ID, 10) + + cases := []struct { + name string + body string + want []string + }{ + {"protocol mismatch, torznab to a usenet client", `{"name":"A","url":"http://192.168.1.50:9117/api","type":"torznab","downloadClientId":` + usenetID + `}`, []string{"usenet", "torrent", "sab"}}, + {"unknown client", `{"name":"B","url":"http://192.168.1.51:9117/api","type":"newznab","downloadClientId":9999}`, []string{"does not exist"}}, + {"zero is not a client", `{"name":"C","url":"http://192.168.1.52:9117/api","type":"newznab","downloadClientId":0}`, []string{"does not exist"}}, + {"new pin to a disabled client", `{"name":"D","url":"http://192.168.1.53:9117/api","type":"torznab","downloadClientId":` + offID + `}`, []string{"disabled", "qbit-off"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rec := f.create(tc.body) + if rec.Code != http.StatusBadRequest { + t.Fatalf("create: status %d, want 400: %s", rec.Code, rec.Body.String()) + } + for _, w := range tc.want { + if !strings.Contains(rec.Body.String(), w) { + t.Errorf("message %q does not mention %q", rec.Body.String(), w) + } + } + }) + } + + // The same checks run on update, and a rejected update changes nothing. + rec := f.create(`{"name":"Plain","url":"http://192.168.1.60:9117/api","type":"torznab"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("create plain: %d %s", rec.Code, rec.Body.String()) + } + plain := decodeIndexer(t, rec) + if rec := f.update(plain.ID, `{"downloadClientId":`+usenetID+`}`); rec.Code != http.StatusBadRequest { + t.Fatalf("update with a usenet client on a torznab indexer: %d %s", rec.Code, rec.Body.String()) + } + got, _ := f.ih.indexers.GetByID(t.Context(), plain.ID) + if got.DownloadClientID != nil { + t.Errorf("a rejected update still stored a pin: %d", *got.DownloadClientID) + } +} + +// TestIndexerPin_UnrelatedEditKeepsPinToDisabledClient: disabling a client is +// allowed, so an edit to some other field of the indexer must still save. +func TestIndexerPin_UnrelatedEditKeepsPinToDisabledClient(t *testing.T) { + f := newPinAPIFixture(t) + torrentID := strconv.FormatInt(f.torrent.ID, 10) + created := decodeIndexer(t, f.create(`{"name":"Priv","url":"http://192.168.1.50:9117/api","type":"torznab","downloadClientId":`+torrentID+`}`)) + f.torrent.Enabled = false + if err := f.clients.Update(t.Context(), f.torrent); err != nil { + t.Fatal(err) + } + rec := f.update(created.ID, `{"name":"Priv renamed","downloadClientId":`+torrentID+`}`) + if rec.Code != http.StatusOK { + t.Fatalf("edit: %d %s", rec.Code, rec.Body.String()) + } + if got := decodeIndexer(t, rec); got.DownloadClientID == nil || *got.DownloadClientID != f.torrent.ID { + t.Errorf("pin lost: %v", got.DownloadClientID) + } +} + +func (f *pinAPIFixture) deleteClient(id int64) *httptest.ResponseRecorder { + rec := httptest.NewRecorder() + idStr := strconv.FormatInt(id, 10) + f.ch.Delete(rec, withURLParam(httptest.NewRequest(http.MethodDelete, "/downloadclient/"+idStr, nil), "id", idStr)) + return rec +} + +// TestDownloadClientDelete_RefusedWhileAnIndexerIsPinned: 409 naming every +// pinned indexer, and the client survives. Unpinning lets the delete through. +func TestDownloadClientDelete_RefusedWhileAnIndexerIsPinned(t *testing.T) { + f := newPinAPIFixture(t) + torrentID := strconv.FormatInt(f.torrent.ID, 10) + a := decodeIndexer(t, f.create(`{"name":"TrackerA","url":"http://192.168.1.50:9117/api","type":"torznab","downloadClientId":`+torrentID+`}`)) + b := decodeIndexer(t, f.create(`{"name":"TrackerB","url":"http://192.168.1.51:9117/api","type":"torznab","downloadClientId":`+torrentID+`}`)) + decodeIndexer(t, f.create(`{"name":"Unpinned","url":"http://192.168.1.52:9117/api","type":"torznab"}`)) + + rec := f.deleteClient(f.torrent.ID) + if rec.Code != http.StatusConflict { + t.Fatalf("delete: status %d, want 409: %s", rec.Code, rec.Body.String()) + } + for _, want := range []string{"TrackerA", "TrackerB"} { + if !strings.Contains(rec.Body.String(), want) { + t.Errorf("409 message %q does not list %q", rec.Body.String(), want) + } + } + if strings.Contains(rec.Body.String(), "Unpinned") { + t.Errorf("409 message lists an indexer that is not pinned: %s", rec.Body.String()) + } + if c, _ := f.clients.GetByID(t.Context(), f.torrent.ID); c == nil { + t.Fatal("the client was deleted despite the refusal") + } + + // Unpin both, then the delete goes through. + for _, id := range []int64{a.ID, b.ID} { + if rec := f.update(id, `{"downloadClientId":null}`); rec.Code != http.StatusOK { + t.Fatalf("unpin %d: %d %s", id, rec.Code, rec.Body.String()) + } + } + if rec := f.deleteClient(f.torrent.ID); rec.Code != http.StatusNoContent { + t.Fatalf("delete after unpinning: %d %s", rec.Code, rec.Body.String()) + } + // A client no indexer uses was never in the way. + if rec := f.deleteClient(f.usenet.ID); rec.Code != http.StatusNoContent { + t.Fatalf("delete of an unused client: %d %s", rec.Code, rec.Body.String()) + } +} + +// TestDownloadClientDisable_StaysAllowedWhilePinned: disabling is not refused. +func TestDownloadClientDisable_StaysAllowedWhilePinned(t *testing.T) { + f := newPinAPIFixture(t) + torrentID := strconv.FormatInt(f.torrent.ID, 10) + decodeIndexer(t, f.create(`{"name":"TrackerA","url":"http://192.168.1.50:9117/api","type":"torznab","downloadClientId":`+torrentID+`}`)) + rec := httptest.NewRecorder() + f.ch.Update(rec, withURLParam(httptest.NewRequest(http.MethodPut, "/downloadclient/"+torrentID, bytes.NewBufferString(`{"enabled":false}`)), "id", torrentID)) + if rec.Code != http.StatusOK { + t.Fatalf("disable: %d %s", rec.Code, rec.Body.String()) + } +} diff --git a/internal/api/indexer_pin_grab_test.go b/internal/api/indexer_pin_grab_test.go new file mode 100644 index 000000000..31b781320 --- /dev/null +++ b/internal/api/indexer_pin_grab_test.go @@ -0,0 +1,456 @@ +package api + +import ( + "bytes" + "context" + "database/sql" + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "sync" + "testing" + + "github.com/vavallee/bindery/internal/auth" + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/httpsec" + "github.com/vavallee/bindery/internal/models" +) + +// pinSAB is a fake SABnzbd that also serves any *.nzb path, and counts the +// addfile calls it receives. Two of them stand in for a public and a private +// client, so a test can say which one a grab reached. +type pinSAB struct { + mu sync.Mutex + adds int + srv *httptest.Server +} + +func newPinSAB(t *testing.T) *pinSAB { + t.Helper() + p := &pinSAB{} + p.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, ".nzb") { + w.Header().Set("Content-Type", "application/x-nzb") + _, _ = w.Write([]byte(``)) + return + } + if r.URL.Query().Get("mode") == "addfile" { + p.mu.Lock() + p.adds++ + p.mu.Unlock() + } + _ = json.NewEncoder(w).Encode(map[string]any{"status": true, "nzo_ids": []string{"nzo-1"}}) + })) + t.Cleanup(p.srv.Close) + return p +} + +func (p *pinSAB) count() int { + p.mu.Lock() + defer p.mu.Unlock() + return p.adds +} + +// pinFixture is a queue handler with a public client (priority 1, so the +// default pick) and a private client (priority 50), plus a usenet indexer. +type pinFixture struct { + h *QueueHandler + database *sql.DB + ctx context.Context + clients *db.DownloadClientRepo + indexers *db.IndexerRepo + downl *db.DownloadRepo + book *models.Book + pub *pinSAB + priv *pinSAB + pubC *models.DownloadClient + privC *models.DownloadClient + idx *models.Indexer + relURL string +} + +func newPinFixture(t *testing.T) *pinFixture { + t.Helper() + t.Cleanup(httpsec.AllowLoopbackForTests()) + h, database, downloads, clients, books, ctx := queueFixture(t) + indexers := db.NewIndexerRepo(database) + h.WithIndexers(indexers) + + pub, priv := newPinSAB(t), newPinSAB(t) + // The indexer's own host serves the release, so the host match fallback + // that signs a download URL can find it. + rel := newPinSAB(t) + + pubHost, pubPort := testServerHostPort(t, pub.srv.URL) + pubC := &models.DownloadClient{Name: "sab-public", Type: "sabnzbd", Host: pubHost, Port: pubPort, Enabled: true, Priority: 1} + if err := clients.Create(ctx, pubC); err != nil { + t.Fatalf("create public client: %v", err) + } + privHost, privPort := testServerHostPort(t, priv.srv.URL) + privC := &models.DownloadClient{Name: "sab-private", Type: "sabnzbd", Host: privHost, Port: privPort, Enabled: true, Priority: 50} + if err := clients.Create(ctx, privC); err != nil { + t.Fatalf("create private client: %v", err) + } + + idx := &models.Indexer{Name: "TrackerX", Type: "newznab", URL: rel.srv.URL + "/api", APIKey: "secret", Enabled: true} + if err := indexers.Create(ctx, idx); err != nil { + t.Fatalf("create indexer: %v", err) + } + + authors := db.NewAuthorRepo(database) + author := &models.Author{ForeignID: "pin-author", Name: "Pin Author", SortName: "Author, Pin", Monitored: true} + if err := authors.Create(ctx, author); err != nil { + t.Fatalf("create author: %v", err) + } + book := &models.Book{ForeignID: "pin-book", AuthorID: author.ID, Title: "Pinned Book", SortTitle: "pinned book", MediaType: models.MediaTypeEbook} + if err := books.Create(ctx, book); err != nil { + t.Fatalf("create book: %v", err) + } + return &pinFixture{ + h: h, database: database, ctx: ctx, clients: clients, indexers: indexers, downl: downloads, book: book, + pub: pub, priv: priv, pubC: pubC, privC: privC, idx: idx, relURL: rel.srv.URL + "/rel.nzb", + } +} + +// asAdmin gives a request the role the auth middleware stamps on an admin (and +// on every request when auth is disabled), which is what lets an error name the +// pinned client. +func asAdmin(r *http.Request) *http.Request { + return r.WithContext(auth.WithUserRole(r.Context(), "admin")) +} + +// pin sets the indexer's download client in storage. +func (f *pinFixture) pin(t *testing.T, clientID *int64) { + t.Helper() + f.idx.DownloadClientID = clientID + if err := f.indexers.Update(f.ctx, f.idx); err != nil { + t.Fatalf("pin indexer: %v", err) + } +} + +func (f *pinFixture) grabBody(guid string, indexerID *int64) []byte { + body := map[string]any{ + "guid": guid, "title": "Pin Author - Pinned Book epub", "nzbUrl": f.relURL, + "size": 100, "bookId": f.book.ID, "protocol": "usenet", + } + if indexerID != nil { + body["indexerId"] = *indexerID + } + b, _ := json.Marshal(body) + return b +} + +func (f *pinFixture) doGrab(guid string, indexerID *int64) *httptest.ResponseRecorder { + // Calls grab and writeGrabError the way Grab does, minus the search + // result registry check that a role bearing caller is held to, which is + // not what these tests are about. + var req grabRequest + _ = json.Unmarshal(f.grabBody(guid, indexerID), &req) + r := asAdmin(httptest.NewRequest(http.MethodPost, "/api/v1/queue/grab", nil)) + w := httptest.NewRecorder() + if _, err := f.h.grab(r.Context(), req); err != nil { + writeGrabError(w, r, err) + return w + } + w.WriteHeader(http.StatusAccepted) + return w +} + +func (f *pinFixture) downloadFor(t *testing.T, guid string) *models.Download { + t.Helper() + d, err := f.downl.GetByGUID(f.ctx, guid) + if err != nil || d == nil { + t.Fatalf("download %s: %v", guid, err) + } + return d +} + +// TestGrab_UnpinnedIndexerRoutesAsBefore is the regression guard: an indexer +// with no pin selects its client exactly as it did before #3080, which is the +// highest priority enabled client of the protocol. +func TestGrab_UnpinnedIndexerRoutesAsBefore(t *testing.T) { + f := newPinFixture(t) + w := f.doGrab("g-unpinned", &f.idx.ID) + if w.Code != http.StatusCreated && w.Code != http.StatusAccepted { + t.Fatalf("grab: status %d: %s", w.Code, w.Body.String()) + } + if f.pub.count() != 1 || f.priv.count() != 0 { + t.Fatalf("unpinned grab: public=%d private=%d, want 1 and 0", f.pub.count(), f.priv.count()) + } + if got := f.downloadFor(t, "g-unpinned").DownloadClientID; got == nil || *got != f.pubC.ID { + t.Errorf("DownloadClientID = %v, want the public client %d", got, f.pubC.ID) + } +} + +// TestGrab_PinnedIndexerGoesToItsClient: the pin wins over priority, and the +// download row records the pinned client. +func TestGrab_PinnedIndexerGoesToItsClient(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + w := f.doGrab("g-pinned", &f.idx.ID) + if w.Code != http.StatusCreated && w.Code != http.StatusAccepted { + t.Fatalf("grab: status %d: %s", w.Code, w.Body.String()) + } + if f.pub.count() != 0 || f.priv.count() != 1 { + t.Fatalf("pinned grab: public=%d private=%d, want 0 and 1", f.pub.count(), f.priv.count()) + } + if got := f.downloadFor(t, "g-pinned").DownloadClientID; got == nil || *got != f.privC.ID { + t.Errorf("DownloadClientID = %v, want the pinned client %d", got, f.privC.ID) + } +} + +// TestGrab_PinWinsOverMediaTypeRouting: an audiobook release would normally +// prefer a client with an audiobook category. The pin overrides that. +func TestGrab_PinWinsOverMediaTypeRouting(t *testing.T) { + f := newPinFixture(t) + f.pubC.CategoryAudiobook = "audio" + if err := f.clients.Update(f.ctx, f.pubC); err != nil { + t.Fatal(err) + } + f.pin(t, &f.privC.ID) + body, _ := json.Marshal(map[string]any{ + "guid": "g-audio", "title": "Pin Author - Pinned Book m4b", "nzbUrl": f.relURL, + "size": 100, "bookId": f.book.ID, "protocol": "usenet", "indexerId": f.idx.ID, "mediaType": "audiobook", + }) + w := httptest.NewRecorder() + f.h.Grab(w, httptest.NewRequest(http.MethodPost, "/api/v1/queue/grab", bytes.NewReader(body))) + if w.Code != http.StatusCreated && w.Code != http.StatusAccepted { + t.Fatalf("grab: status %d: %s", w.Code, w.Body.String()) + } + if f.pub.count() != 0 || f.priv.count() != 1 { + t.Fatalf("public=%d private=%d, want 0 and 1", f.pub.count(), f.priv.count()) + } +} + +// TestGrab_PinFoundByHostMatchStillApplies: a caller that omits the indexer id +// is attributed to an indexer by the download URL's host, and that indexer's +// pin must apply even though the id was not on the request. +func TestGrab_PinFoundByHostMatchStillApplies(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + w := f.doGrab("g-hostmatch", nil) + if w.Code != http.StatusCreated && w.Code != http.StatusAccepted { + t.Fatalf("grab: status %d: %s", w.Code, w.Body.String()) + } + if f.pub.count() != 0 || f.priv.count() != 1 { + t.Fatalf("public=%d private=%d, want 0 and 1", f.pub.count(), f.priv.count()) + } +} + +// TestGrab_PinnedClientDisabledFailsWithoutFallback: the whole point of the +// feature. The other client is enabled and would take the release, and must not. +func TestGrab_PinnedClientDisabledFailsWithoutFallback(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + f.privC.Enabled = false + if err := f.clients.Update(f.ctx, f.privC); err != nil { + t.Fatal(err) + } + w := f.doGrab("g-disabled", &f.idx.ID) + if w.Code != http.StatusBadRequest { + t.Fatalf("status %d, want 400: %s", w.Code, w.Body.String()) + } + msg := w.Body.String() + for _, want := range []string{"TrackerX", "sab-private", "disabled"} { + if !strings.Contains(msg, want) { + t.Errorf("message %q does not name %q", msg, want) + } + } + if f.pub.count() != 0 || f.priv.count() != 0 { + t.Fatalf("a grab with its pinned client disabled reached a client: public=%d private=%d", f.pub.count(), f.priv.count()) + } + if strings.Contains(msg, "no_download_client") { + t.Errorf("a pin failure must not carry the no_download_client setup nudge code: %s", msg) + } +} + +// TestGrab_PinnedClientWrongProtocolFails: a torrent client pinned to a usenet +// indexer, changed behind the pin's back, fails the grab and sends nothing. +func TestGrab_PinnedClientWrongProtocolFails(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + f.privC.Type = "qbittorrent" + if err := f.clients.Update(f.ctx, f.privC); err != nil { + t.Fatal(err) + } + w := f.doGrab("g-proto", &f.idx.ID) + if w.Code != http.StatusBadRequest { + t.Fatalf("status %d, want 400: %s", w.Code, w.Body.String()) + } + for _, want := range []string{"TrackerX", "sab-private", "torrent", "usenet"} { + if !strings.Contains(w.Body.String(), want) { + t.Errorf("message %q does not mention %q", w.Body.String(), want) + } + } + if f.pub.count() != 0 || f.priv.count() != 0 { + t.Fatalf("reached a client: public=%d private=%d", f.pub.count(), f.priv.count()) + } +} + +// TestGrab_PinnedClientMissingFails: a pin that points at a client row that is +// gone (foreign keys off, as a restored backup or manual edit could leave it). +func TestGrab_PinnedClientMissingFails(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + conn, err := f.database.Conn(f.ctx) + if err != nil { + t.Fatal(err) + } + // The in memory database has one connection, so it must be handed back + // before the grab runs or the grab waits on it forever. + if _, err := conn.ExecContext(f.ctx, "PRAGMA foreign_keys=OFF"); err != nil { + t.Fatal(err) + } + if _, err := conn.ExecContext(f.ctx, "DELETE FROM download_clients WHERE id=?", f.privC.ID); err != nil { + t.Fatal(err) + } + if _, err := conn.ExecContext(f.ctx, "PRAGMA foreign_keys=ON"); err != nil { + t.Fatal(err) + } + _ = conn.Close() + w := f.doGrab("g-missing", &f.idx.ID) + if w.Code != http.StatusBadRequest { + t.Fatalf("status %d, want 400: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "TrackerX") || !strings.Contains(w.Body.String(), "no longer exists") { + t.Errorf("message %q should name the indexer and say the client no longer exists", w.Body.String()) + } + if f.pub.count() != 0 { + t.Fatalf("fell back to the public client") + } +} + +// TestGrab_PinnedClientUnreachableNamesIndexerAndClient: an enabled pinned +// client that does not answer fails the grab, names both, and does not try the +// other client. +func TestGrab_PinnedClientUnreachableNamesIndexerAndClient(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + f.priv.srv.Close() + w := f.doGrab("g-down", &f.idx.ID) + if w.Code != http.StatusBadGateway { + t.Fatalf("status %d, want 502: %s", w.Code, w.Body.String()) + } + for _, want := range []string{"TrackerX", "sab-private", "no other client"} { + if !strings.Contains(w.Body.String(), want) { + t.Errorf("message %q does not mention %q", w.Body.String(), want) + } + } + if f.pub.count() != 0 { + t.Fatalf("fell back to the public client") + } +} + +// TestGrab_PinFailureDoesNotNameClientToNonAdmin: indexers and clients are +// admin managed, so a non-admin's error names the indexer (it is in their +// search results) but not the client. +func TestGrab_PinFailureDoesNotNameClientToNonAdmin(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + f.privC.Enabled = false + if err := f.clients.Update(f.ctx, f.privC); err != nil { + t.Fatal(err) + } + ctx := auth.WithUserRole(auth.WithUserID(f.ctx, 7), "user") + _, err := f.h.grab(ctx, grabRequest{GUID: "g-nonadmin", Title: "Pin Author - Pinned Book epub", NZBURL: f.relURL, Size: 1, Protocol: "usenet", IndexerID: &f.idx.ID}) + if err == nil { + t.Fatal("expected the grab to fail") + } + w := httptest.NewRecorder() + writeGrabError(w, httptest.NewRequest(http.MethodPost, "/x", nil).WithContext(ctx), err) + if w.Code != http.StatusBadRequest { + t.Fatalf("status %d, want 400: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "TrackerX") { + t.Errorf("message %q should name the indexer", w.Body.String()) + } + if strings.Contains(w.Body.String(), "sab-private") { + t.Errorf("message %q leaks the client name to a non-admin", w.Body.String()) + } +} + +// TestRetryAndRegrab_UsePin: a failed row for a pinned indexer is re-sent to +// the pinned client, both by the single retry route and by the bulk retry. +func TestRetryAndRegrab_UsePin(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + dl := &models.Download{ + GUID: "g-retry", Title: "Pin Author - Pinned Book epub", NZBURL: f.relURL, Size: 100, + Status: models.StateFailed, Protocol: "usenet", Quality: "epub", IndexerID: &f.idx.ID, + BookID: &f.book.ID, ErrorMessage: "boom", + } + if err := f.downl.Create(f.ctx, dl); err != nil { + t.Fatalf("create failed download: %v", err) + } + w := httptest.NewRecorder() + f.h.RetryDownload(w, asAdmin(retryDownloadRequest(t, dl.ID))) + if w.Code != http.StatusAccepted { + t.Fatalf("retry: status %d: %s", w.Code, w.Body.String()) + } + if f.pub.count() != 0 || f.priv.count() != 1 { + t.Fatalf("retry: public=%d private=%d, want 0 and 1", f.pub.count(), f.priv.count()) + } + if got := f.downloadFor(t, "g-retry").DownloadClientID; got == nil || *got != f.privC.ID { + t.Errorf("DownloadClientID = %v, want %d", got, f.privC.ID) + } +} + +// TestRetry_PinnedClientDisabledFails: the same retry with the pinned client +// disabled fails and sends nothing. +func TestRetry_PinnedClientDisabledFails(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + f.privC.Enabled = false + if err := f.clients.Update(f.ctx, f.privC); err != nil { + t.Fatal(err) + } + dl := &models.Download{ + GUID: "g-retry2", Title: "Pin Author - Pinned Book epub", NZBURL: f.relURL, Size: 100, + Status: models.StateFailed, Protocol: "usenet", Quality: "epub", IndexerID: &f.idx.ID, BookID: &f.book.ID, + } + if err := f.downl.Create(f.ctx, dl); err != nil { + t.Fatal(err) + } + w := httptest.NewRecorder() + f.h.RetryDownload(w, asAdmin(retryDownloadRequest(t, dl.ID))) + if w.Code != http.StatusBadRequest { + t.Fatalf("retry: status %d, want 400: %s", w.Code, w.Body.String()) + } + if f.pub.count() != 0 || f.priv.count() != 0 { + t.Fatalf("reached a client: public=%d private=%d", f.pub.count(), f.priv.count()) + } +} + +// TestPendingGrab_UsesPin: a release held by a delay profile, force grabbed +// from the Pending tab, goes to the pinned client. +func TestPendingGrab_UsesPin(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + pending := db.NewPendingReleaseRepo(f.database) + releaseJSON, _ := json.Marshal(map[string]any{ + "guid": "g-pending", "title": "Pin Author - Pinned Book epub", "nzbUrl": f.relURL, + "size": 100, "protocol": "usenet", "indexerId": f.idx.ID, + }) + if err := pending.Upsert(f.ctx, &models.PendingRelease{ + BookID: f.book.ID, MediaType: models.MediaTypeEbook, Title: "Pin Author - Pinned Book epub", + IndexerID: &f.idx.ID, GUID: "g-pending", Protocol: "usenet", Reason: "delay", ReleaseJSON: string(releaseJSON), + }); err != nil { + t.Fatal(err) + } + all, err := pending.List(f.ctx) + if err != nil || len(all) != 1 { + t.Fatalf("pending list: %v (%d rows)", err, len(all)) + } + id := strconv.FormatInt(all[0].ID, 10) + ph := NewPendingHandler(pending, f.h, f.h.downloads, f.h.books) + w := httptest.NewRecorder() + ph.Grab(w, withURLParam(httptest.NewRequest(http.MethodPost, "/api/v1/pending/"+id+"/grab", nil), "id", id)) + if w.Code != http.StatusCreated { + t.Fatalf("pending grab: status %d: %s", w.Code, w.Body.String()) + } + if f.pub.count() != 0 || f.priv.count() != 1 { + t.Fatalf("public=%d private=%d, want 0 and 1", f.pub.count(), f.priv.count()) + } +} diff --git a/internal/api/indexers.go b/internal/api/indexers.go index 243a9bdd8..9a4451ee0 100644 --- a/internal/api/indexers.go +++ b/internal/api/indexers.go @@ -33,7 +33,10 @@ type indexerSearcher interface { } type IndexerHandler struct { - indexers *db.IndexerRepo + indexers *db.IndexerRepo + // clients validates a download client pin (#3080) on save. Optional: when + // nil, any pin is refused rather than stored unchecked. + clients *db.DownloadClientRepo books *db.BookRepo authors *db.AuthorRepo aliases *db.AuthorAliasRepo @@ -74,6 +77,13 @@ func (h *IndexerHandler) WithSearchDebugLog(l *indexer.DebugLog) *IndexerHandler return h } +// WithDownloadClients attaches the download client repo, which a per-indexer +// download client pin (#3080) is validated against on save. +func (h *IndexerHandler) WithDownloadClients(clients *db.DownloadClientRepo) *IndexerHandler { + h.clients = clients + return h +} + // WithAliases attaches the author alias repo used to populate AuthorAliases in MatchCriteria. func (h *IndexerHandler) WithAliases(aliases *db.AuthorAliasRepo) *IndexerHandler { h.aliases = aliases @@ -189,6 +199,55 @@ func validateDailyQueryLimit(idx models.Indexer) error { return nil } +// validateDownloadClientPin checks an indexer's download client pin (#3080) on +// save. The client must exist and serve the indexer's protocol. It must also be +// enabled, but only when the pin is new or changed: disabling a client is +// allowed, and an unrelated edit of an indexer pinned to a disabled client has +// to keep saving. A disabled pinned client fails its grabs instead. +func (h *IndexerHandler) validateDownloadClientPin(ctx context.Context, idx models.Indexer, stored *models.Indexer) error { + if idx.DownloadClientID == nil { + return nil + } + var c *models.DownloadClient + if h.clients != nil { + var err error + if c, err = h.clients.GetByID(ctx, *idx.DownloadClientID); err != nil { + return err + } + } + if c == nil { + return errPinInvalid(fmt.Sprintf("downloadClientId %d: that download client does not exist", *idx.DownloadClientID)) + } + if got := db.ClientProtocol(c.Type); got != idx.Protocol() { + return errPinInvalid(fmt.Sprintf("download client %q is a %s client but this indexer is %s, so it cannot be pinned to it; pick a %s client", + c.Name, got, idx.Protocol(), idx.Protocol())) + } + changed := stored == nil || stored.DownloadClientID == nil || *stored.DownloadClientID != *idx.DownloadClientID + if changed && !c.Enabled { + return errPinInvalid(fmt.Sprintf("download client %q is disabled; enable it first or pick another", c.Name)) + } + return nil +} + +// pinInvalidError is a 400 about the download client pin, as opposed to a +// storage failure. +type pinInvalidError string + +func (e pinInvalidError) Error() string { return string(e) } + +func errPinInvalid(msg string) error { return pinInvalidError(msg) } + +// writePinError answers a failed validateDownloadClientPin: a 400 for a bad +// pin, a generic 500 for a storage fault. +func writePinError(w http.ResponseWriter, r *http.Request, err error) { + var bad pinInvalidError + if errors.As(err, &bad) { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": bad.Error()}) + return + } + writeServerError(w, r, err) +} + // maxSeedTimeMinutes bounds the per-indexer seed time limits (#2206) at ten // years. Nothing real needs more, and it keeps the value well inside the int32 // that qBittorrent parses it into. @@ -311,6 +370,10 @@ func (h *IndexerHandler) Create(w http.ResponseWriter, r *http.Request) { if idx.Type == "" { idx.Type = "newznab" } + if err := h.validateDownloadClientPin(r.Context(), idx, nil); err != nil { + writePinError(w, r, err) + return + } if len(idx.Categories) == 0 { // Books (7000 parent, 7020 ebook) + Audio/Audiobook (3030). // The searcher filters per-media-type at query time. @@ -411,6 +474,10 @@ func (h *IndexerHandler) Update(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusBadRequest, map[string]string{"error": err.Error()}) return } + if err := h.validateDownloadClientPin(r.Context(), idx, existing); err != nil { + writePinError(w, r, err) + return + } idx.ID = id // A user editing the indexer takes ownership of the seed-ratio override, so // the Prowlarr syncer (#1065) will not overwrite it on the next sync. This diff --git a/internal/api/queue.go b/internal/api/queue.go index 75f5dbd4d..7d6a9b3d0 100644 --- a/internal/api/queue.go +++ b/internal/api/queue.go @@ -72,7 +72,15 @@ const sendFailedMessage = "failed to send the release to the download client" // sendError carries the download client's own failure for the logs and the // download row. Error() keeps the full text; only errSendFailed is matched by // errors.Is, so callers branch on the kind without relaying the cause. -type sendError struct{ cause error } +type sendError struct { + cause error + // pin is set when the client was the indexer's pinned one (#3080), so the + // failure can say no other client was tried. + pin *pinTarget +} + +// pinTarget names the indexer and the client of a pinned grab. +type pinTarget struct{ indexer, client string } func (e *sendError) Error() string { return errSendFailed.Error() + ": " + e.cause.Error() } func (e *sendError) Unwrap() []error { return []error{errSendFailed, e.cause} } @@ -81,8 +89,34 @@ func (e *sendError) Unwrap() []error { return []error{errSendFailed, e.cause} } // a network failure (refused, timed out, unknown container name). The hint is // fixed text chosen by the class of failure, so it names no host and no URL, // and it is what tells a user the client is down, not that the release is bad. -func sendFailedText(err error) string { - return sendFailedMessage + nethint.ForErr(err) +func sendFailedText(ctx context.Context, err error) string { + text := sendFailedMessage + nethint.ForErr(err) + var se *sendError + if errors.As(err, &se) && se.pin != nil { + if isAdminCtx(ctx) { + text += fmt.Sprintf(". Indexer %q is pinned to download client %q, so no other client was tried", se.pin.indexer, se.pin.client) + } else { + text += fmt.Sprintf(". Indexer %q is pinned to one download client, so no other client was tried", se.pin.indexer) + } + } + return text +} + +// isAdminCtx reports whether the request is an admin's. Auth disabled and API +// key callers are stamped admin by the auth middleware. +func isAdminCtx(ctx context.Context) bool { + return auth.UserRoleFromContext(ctx) == "admin" +} + +// codeIndexerClientUnavailable is the code on the 400 for a grab whose +// indexer's pinned download client cannot take it (#3080). It is not +// codeNoDownloadClient on purpose: that code drives a "go add a client" setup +// nudge, which would be wrong when clients exist and the pin is the problem. +const codeIndexerClientUnavailable = "indexer_client_unavailable" + +// pinnedMessage renders a pin failure for the caller's role. +func pinnedMessage(ctx context.Context, perr *db.PinnedClientError) string { + return perr.Message(isAdminCtx(ctx)) } // writeGrabError answers an error from grab or resendRelease. A missing client @@ -90,12 +124,16 @@ func sendFailedText(err error) string { // a refused or unreachable client is a generic 502, since the text comes from // the client; anything else is a storage fault and a generic 500. func writeGrabError(w http.ResponseWriter, r *http.Request, err error) { + var perr *db.PinnedClientError switch { + case errors.As(err, &perr): + slog.Warn("grab: the indexer's pinned download client cannot take the release", "method", r.Method, "path", r.URL.Path, "error", perr.Error()) + writeJSON(w, http.StatusBadRequest, map[string]string{"error": pinnedMessage(r.Context(), perr), "code": codeIndexerClientUnavailable}) case errors.Is(err, errNoDownloadClient): writeJSON(w, http.StatusBadRequest, map[string]string{"error": err.Error(), "code": codeNoDownloadClient}) case errors.Is(err, errSendFailed): slog.Warn("grab: download client did not take the release", "method", r.Method, "path", r.URL.Path, "error", err) - writeJSON(w, http.StatusBadGateway, map[string]string{"error": sendFailedText(err)}) + writeJSON(w, http.StatusBadGateway, map[string]string{"error": sendFailedText(r.Context(), err)}) default: writeServerError(w, r, err) } @@ -110,14 +148,17 @@ const codeNoDownloadClient = "no_download_client" // handlers already treat as safe (a conflict, a missing book, no client) pass // through, a send failure is generic, and a storage fault is logged and // replaced. -func queueItemMessage(err error) string { +func queueItemMessage(ctx context.Context, err error) string { + var perr *db.PinnedClientError switch { + case errors.As(err, &perr): + return pinnedMessage(ctx, perr) case errors.Is(err, errNoDownloadClient), errors.Is(err, errAlreadyGrabbed), errors.Is(err, errRetryNotResendable), errors.Is(err, errGrabBookNotFound): return err.Error() case errors.Is(err, errSendFailed): slog.Warn("bulk queue item: download client did not take the release", "error", err) - return sendFailedText(err) + return sendFailedText(ctx, err) default: slog.Error("bulk queue item failed", "error", err) return "internal server error" @@ -1280,7 +1321,7 @@ func (h *QueueHandler) BulkRetry(w http.ResponseWriter, r *http.Request) { accepted, found, err := h.downloads.ResetImportRetry(ctx, id) switch { case err != nil: - setResult(id, queueRetryResult{Error: queueItemMessage(err)}) + setResult(id, queueRetryResult{Error: queueItemMessage(ctx, err)}) case !found: setResult(id, queueRetryResult{Error: "download not found"}) case !accepted: @@ -1292,7 +1333,7 @@ func (h *QueueHandler) BulkRetry(w http.ResponseWriter, r *http.Request) { return } if _, err := h.resendRelease(ctx, target); err != nil { - setResult(id, queueRetryResult{Error: queueItemMessage(err)}) + setResult(id, queueRetryResult{Error: queueItemMessage(ctx, err)}) return } setResult(id, queueRetryResult{OK: true, Action: retryActionResend}) @@ -1320,6 +1361,31 @@ func (h *QueueHandler) selectClient(ctx context.Context, protocol, mediaType str return db.PickClientForMediaType(candidates, mediaType), nil } +// clientForGrab picks the client for a grab. An indexer pinned to a download +// client (#3080) gets exactly that client, whatever the protocol routing, media +// type or priority would have chosen, or a *db.PinnedClientError when it cannot +// be used: there is no fallback, because falling back is how a private +// tracker's traffic reaches the public client. The returned pinTarget is +// non-nil only for a pinned grab. An unpinned indexer, or no indexer, takes +// selectClient exactly as before. +func (h *QueueHandler) clientForGrab(ctx context.Context, indexerID *int64, protocol, mediaType string) (*models.DownloadClient, *pinTarget, error) { + if h.indexers != nil && indexerID != nil && *indexerID != 0 { + idx, err := h.indexers.GetByID(ctx, *indexerID) + if err != nil { + return nil, nil, err + } + if idx != nil && idx.DownloadClientID != nil { + c, err := h.clients.ResolvePinnedClient(ctx, idx, protocol) + if err != nil { + return nil, nil, err + } + return c, &pinTarget{indexer: idx.Name, client: c.Name}, nil + } + } + c, err := h.selectClient(ctx, protocol, mediaType) + return c, nil, err +} + // noProtocolClientError builds an actionable "no enabled download client" error. // If clients of the opposite protocol are enabled, it surfaces the mismatch // (e.g. the release is usenet but only torrent clients are enabled) and names @@ -1473,18 +1539,6 @@ func (h *QueueHandler) grab(ctx context.Context, req grabRequest) (*models.Downl // ebook category (#2933). See grabMediaType. req.MediaType = grabMediaType(req.MediaType, req.Title) - client, err := h.selectClient(ctx, req.Protocol, req.MediaType) - if err != nil { - // selectClient already produced an actionable, protocol-aware message - // (see noProtocolClientError); propagate it rather than flattening it - // to the generic "no enabled download client configured". - return nil, err - } - if client == nil { - return nil, h.noProtocolClientError(ctx, req.Protocol) - } - - protocol := downloader.ProtocolForClient(client.Type) // Tenancy (#1457): stamp from the request identity. API key and trusted // local requests carry the first admin's id (auth.withOperatorUserID), so // they own what they grab like any signed-in user. A request reaches here @@ -1512,6 +1566,22 @@ func (h *QueueHandler) grab(ctx context.Context, req grabRequest) (*models.Downl if hostMatched != nil { indexerID = hostMatched } + + // The client is chosen only now, after the host match above has settled + // which indexer the release came from: an indexer pinned to a download + // client (#3080) must be found even when the caller sent no indexer id. + client, pin, err := h.clientForGrab(ctx, indexerID, req.Protocol, req.MediaType) + if err != nil { + // Both selectClient (see noProtocolClientError) and a failed pin + // produce an actionable message; propagate it rather than flattening + // it to the generic "no enabled download client configured". + return nil, err + } + if client == nil { + return nil, h.noProtocolClientError(ctx, req.Protocol) + } + protocol := downloader.ProtocolForClient(client.Type) + dl := &models.Download{ GUID: req.GUID, BookID: bookID, @@ -1571,7 +1641,7 @@ func (h *QueueHandler) grab(ctx context.Context, req grabRequest) (*models.Downl if h.notif != nil { h.notif.Send(ctx, notifier.EventDownloadFailed, map[string]any{"title": req.Title, "message": err.Error()}) } - return nil, &sendError{cause: err} + return nil, &sendError{cause: err, pin: pin} } if remoteID := sendRes.RemoteID; remoteID != "" { @@ -1869,7 +1939,7 @@ func (h *QueueHandler) BulkDelete(w http.ResponseWriter, r *http.Request) { return } if err := h.removeQueueItem(ctx, &target, opts); err != nil { - setResult(id, bulkItemResult{Error: queueItemMessage(err)}) + setResult(id, bulkItemResult{Error: queueItemMessage(ctx, err)}) return } setResult(id, bulkItemResult{OK: true}) diff --git a/internal/db/download_client_pin.go b/internal/db/download_client_pin.go new file mode 100644 index 000000000..573402870 --- /dev/null +++ b/internal/db/download_client_pin.go @@ -0,0 +1,130 @@ +package db + +import ( + "context" + "fmt" + "slices" + "strings" + + "github.com/vavallee/bindery/internal/models" +) + +// ClientPinnedError is what DownloadClientRepo.Delete returns while one or more +// indexers are pinned to the client (#3080). Indexers holds their names. +type ClientPinnedError struct { + ClientID int64 + Indexers []string +} + +func (e *ClientPinnedError) Error() string { + return fmt.Sprintf("this download client is pinned by %d indexer(s): %s. Choose another download client for them, or clear it, before deleting this one", + len(e.Indexers), strings.Join(e.Indexers, ", ")) +} + +// pinnedError returns a *ClientPinnedError naming every indexer pinned to the +// client, or nil when none is. +func (r *DownloadClientRepo) pinnedError(ctx context.Context, clientID int64) error { + rows, err := r.db.QueryContext(ctx, `SELECT name FROM indexers WHERE download_client_id=? ORDER BY name`, clientID) + if err != nil { + return err + } + defer rows.Close() + var names []string + for rows.Next() { + var n string + if err := rows.Scan(&n); err != nil { + return err + } + names = append(names, n) + } + if err := rows.Err(); err != nil { + return err + } + if len(names) == 0 { + return nil + } + return &ClientPinnedError{ClientID: clientID, Indexers: names} +} + +// Why a pinned client cannot take a grab. +const ( + PinClientMissing = "missing" + PinClientDisabled = "disabled" + PinClientWrongProtocol = "wrong protocol" +) + +// PinnedClientError says a grab from a pinned indexer cannot go to the client +// it is pinned to. The grab fails and is never sent anywhere else (#3080). +type PinnedClientError struct { + IndexerName string + ClientID int64 + ClientName string // empty when the client no longer exists + Reason string + ClientProtocol string + WantProtocol string +} + +const pinNoFallback = "Nothing was sent and no other client was tried. Change the indexer's download client in Settings, Indexers." + +func (e *PinnedClientError) Error() string { return e.Message(true) } + +// Message renders the error. Indexers and clients are admin managed, so a +// caller that is not an admin gets the indexer (already in their search +// results) but not the client's name. +func (e *PinnedClientError) Message(includeClient bool) string { + target := "a download client" + if includeClient && e.ClientName != "" { + target = fmt.Sprintf("download client %q", e.ClientName) + } + head := fmt.Sprintf("indexer %q is pinned to %s", e.IndexerName, target) + switch e.Reason { + case PinClientMissing: + if includeClient { + return fmt.Sprintf("%s (id %d), which no longer exists. %s", head, e.ClientID, pinNoFallback) + } + return head + " that no longer exists. " + pinNoFallback + case PinClientDisabled: + return head + ", which is disabled. " + pinNoFallback + default: + return fmt.Sprintf("%s, which is a %s client, but the indexer is %s. %s", head, e.ClientProtocol, e.WantProtocol, pinNoFallback) + } +} + +// ClientProtocol reports "torrent" or "usenet" for a download client type. +func ClientProtocol(clientType string) string { + if slices.Contains(torrentClientTypes, clientType) { + return "torrent" + } + return "usenet" +} + +// ResolvePinnedClient returns the client idx is pinned to, (nil, nil) when it +// is not pinned, or a *PinnedClientError when the pinned client is missing, +// disabled or of the wrong protocol. It never substitutes another client. +// releaseProtocol, when set, is the protocol of the release being sent and +// must match too, so a usenet release cannot reach a torrent client. +func (r *DownloadClientRepo) ResolvePinnedClient(ctx context.Context, idx *models.Indexer, releaseProtocol string) (*models.DownloadClient, error) { + if idx == nil || idx.DownloadClientID == nil { + return nil, nil + } + perr := &PinnedClientError{IndexerName: idx.Name, ClientID: *idx.DownloadClientID, WantProtocol: idx.Protocol()} + c, err := r.GetByID(ctx, *idx.DownloadClientID) + if err != nil { + return nil, err + } + if c == nil { + perr.Reason = PinClientMissing + return nil, perr + } + perr.ClientName = c.Name + perr.ClientProtocol = ClientProtocol(c.Type) + if !c.Enabled { + perr.Reason = PinClientDisabled + return nil, perr + } + if perr.ClientProtocol != perr.WantProtocol || (releaseProtocol != "" && perr.ClientProtocol != releaseProtocol) { + perr.Reason = PinClientWrongProtocol + return nil, perr + } + return c, nil +} diff --git a/internal/db/download_clients.go b/internal/db/download_clients.go index de0a2926f..ed1562847 100644 --- a/internal/db/download_clients.go +++ b/internal/db/download_clients.go @@ -319,9 +319,22 @@ func (r *DownloadClientRepo) Update(ctx context.Context, c *models.DownloadClien return err } +// Delete removes a client. It is refused with a *ClientPinnedError while any +// indexer is pinned to the client (#3080): quietly unpinning those indexers +// would send their grabs to the default client, which is the fallback the pin +// exists to prevent. The foreign key on indexers.download_client_id is the +// backstop for a pin written between the check and the delete. func (r *DownloadClientRepo) Delete(ctx context.Context, id int64) error { - _, err := r.db.ExecContext(ctx, "DELETE FROM download_clients WHERE id=?", id) - return err + if err := r.pinnedError(ctx, id); err != nil { + return err + } + if _, err := r.db.ExecContext(ctx, "DELETE FROM download_clients WHERE id=?", id); err != nil { + if pinned := r.pinnedError(ctx, id); pinned != nil { + return pinned + } + return err + } + return nil } // PickClientForMediaType selects the best client from a list for the given diff --git a/internal/db/indexer_download_client_test.go b/internal/db/indexer_download_client_test.go new file mode 100644 index 000000000..2066763ab --- /dev/null +++ b/internal/db/indexer_download_client_test.go @@ -0,0 +1,88 @@ +package db + +import ( + "context" + "errors" + "testing" + + "github.com/vavallee/bindery/internal/models" +) + +// TestMigrate109_ExistingIndexersStayUnpinned: the column is added to a table +// that already has rows, and every one of them reads back with no pin. +func TestMigrate109_ExistingIndexersStayUnpinned(t *testing.T) { + database, err := OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + ctx := context.Background() + + // Roll the column back out, insert a row the way a pre 109 install had it, + // then add the column again exactly as the migration does. + if _, err := database.ExecContext(ctx, `ALTER TABLE indexers DROP COLUMN download_client_id`); err != nil { + t.Fatalf("drop column: %v", err) + } + if _, err := database.ExecContext(ctx, `INSERT INTO indexers (name, type, url, api_key, categories, priority, enabled, supports_search, created_at, updated_at) + VALUES ('old', 'newznab', 'http://x', 'k', '[7020]', 25, 1, 1, datetime('now'), datetime('now'))`); err != nil { + t.Fatalf("insert old row: %v", err) + } + if _, err := database.ExecContext(ctx, `ALTER TABLE indexers ADD COLUMN download_client_id INTEGER REFERENCES download_clients(id) ON DELETE RESTRICT`); err != nil { + t.Fatalf("re-add column: %v", err) + } + + got, err := NewIndexerRepo(database).List(ctx) + if err != nil || len(got) != 1 { + t.Fatalf("list: %v (%d rows)", err, len(got)) + } + if got[0].DownloadClientID != nil { + t.Errorf("an existing indexer came back pinned to %d", *got[0].DownloadClientID) + } +} + +// TestIndexerRepo_PinRoundTripAndForeignKey: the pin persists through +// Create, Update, GetByID and List, and the database itself refuses to delete +// a client an indexer is pinned to. +func TestIndexerRepo_PinRoundTripAndForeignKey(t *testing.T) { + database, err := OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + ctx := context.Background() + clients := NewDownloadClientRepo(database) + indexers := NewIndexerRepo(database) + + c := &models.DownloadClient{Name: "qbit", Type: "qbittorrent", Host: "h", Port: 1, Enabled: true} + if err := clients.Create(ctx, c); err != nil { + t.Fatal(err) + } + idx := &models.Indexer{Name: "t", Type: "torznab", URL: "http://t", Enabled: true, Categories: []int{7020}, DownloadClientID: &c.ID} + if err := indexers.Create(ctx, idx); err != nil { + t.Fatal(err) + } + got, err := indexers.GetByID(ctx, idx.ID) + if err != nil || got == nil || got.DownloadClientID == nil || *got.DownloadClientID != c.ID { + t.Fatalf("GetByID pin = %+v, %v", got, err) + } + + err = clients.Delete(ctx, c.ID) + var pinned *ClientPinnedError + if !errors.As(err, &pinned) { + t.Fatalf("Delete of a pinned client = %v, want a *ClientPinnedError", err) + } + if len(pinned.Indexers) != 1 || pinned.Indexers[0] != "t" { + t.Errorf("pinned indexers = %v, want [t]", pinned.Indexers) + } + if again, _ := clients.GetByID(ctx, c.ID); again == nil { + t.Fatal("the client was deleted") + } + + got.DownloadClientID = nil + if err := indexers.Update(ctx, got); err != nil { + t.Fatal(err) + } + if err := clients.Delete(ctx, c.ID); err != nil { + t.Fatalf("Delete after unpinning: %v", err) + } +} diff --git a/internal/models/indexer.go b/internal/models/indexer.go index 21108568a..f77cc378a 100644 --- a/internal/models/indexer.go +++ b/internal/models/indexer.go @@ -115,6 +115,15 @@ type Indexer struct { UpdatedAt time.Time `json:"updatedAt"` } +// Protocol is the release protocol this indexer serves: torznab is torrent and +// everything else is usenet, the same mapping the searcher stamps on results. +func (i Indexer) Protocol() string { + if i.Type == "torznab" { + return "torrent" + } + return "usenet" +} + // NeedsAttention reports whether the last search against this indexer failed // with something only a human can fix: Newznab's 1xx range (100 bad // credentials, 101 account suspended, 102 VPN forbidden). A 5xx rate limit and diff --git a/internal/prowlarr/syncer_pin_test.go b/internal/prowlarr/syncer_pin_test.go new file mode 100644 index 000000000..3043f0e10 --- /dev/null +++ b/internal/prowlarr/syncer_pin_test.go @@ -0,0 +1,76 @@ +package prowlarr + +import ( + "context" + "testing" + "time" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/models" +) + +// TestSyncer_PreservesDownloadClientPin runs a sync against the real indexer +// repo. The pin is local configuration, like priority, and Prowlarr knows +// nothing about it, so an update driven by a Prowlarr side rename and a new +// seed ratio must write the row back with the pin intact (#3080). +func TestSyncer_PreservesDownloadClientPin(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + ctx := context.Background() + + instances := db.NewProwlarrRepo(database) + inst := &models.ProwlarrInstance{Name: "p", URL: "http://prowlarr.invalid", APIKey: "k", Enabled: true} + if err := instances.Create(ctx, inst); err != nil { + t.Fatal(err) + } + clients := db.NewDownloadClientRepo(database) + priv := &models.DownloadClient{Name: "qbit-private", Type: "qbittorrent", Host: "qb.local", Port: 8081, Enabled: true} + if err := clients.Create(ctx, priv); err != nil { + t.Fatal(err) + } + indexers := db.NewIndexerRepo(database) + + first := prowlarrStub(t, `[{"id":7,"name":"Tracker","enable":true,"protocol":"torrent","supportsSearch":true, + "categories":[{"id":7020}],"fields":[{"name":"torrentBaseSettings.seedRatio","value":1.0}]}]`) + defer first.Close() + if _, err := NewSyncer(New(first.URL, "k"), indexers, instanceTouch{}).Sync(ctx, inst.ID); err != nil { + t.Fatalf("first sync: %v", err) + } + rows, err := indexers.ListByProwlarrInstance(ctx, inst.ID) + if err != nil || len(rows) != 1 { + t.Fatalf("after first sync: %v (%d rows)", err, len(rows)) + } + row := rows[0] + row.DownloadClientID = &priv.ID + if err := indexers.Update(ctx, &row); err != nil { + t.Fatalf("pin: %v", err) + } + + second := prowlarrStub(t, `[{"id":7,"name":"Tracker Renamed","enable":true,"protocol":"torrent","supportsSearch":true, + "categories":[{"id":7020}],"fields":[{"name":"torrentBaseSettings.seedRatio","value":2.5}]}]`) + defer second.Close() + res, err := NewSyncer(New(second.URL, "k"), indexers, instanceTouch{}).Sync(ctx, inst.ID) + if err != nil { + t.Fatalf("second sync: %v", err) + } + if res.Updated != 1 { + t.Fatalf("the second sync should have updated the row, got %s", res) + } + rows, err = indexers.ListByProwlarrInstance(ctx, inst.ID) + if err != nil || len(rows) != 1 { + t.Fatalf("after second sync: %v (%d rows)", err, len(rows)) + } + if rows[0].Name != "Tracker Renamed" { + t.Fatalf("the sync did not apply the rename, so this test proves nothing: %q", rows[0].Name) + } + if rows[0].DownloadClientID == nil || *rows[0].DownloadClientID != priv.ID { + t.Errorf("the sync dropped the download client pin: %v", rows[0].DownloadClientID) + } +} + +type instanceTouch struct{} + +func (instanceTouch) SetLastSyncAt(context.Context, int64, time.Time) error { return nil } diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index b922e068a..aabb94d4b 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -6,6 +6,7 @@ package scheduler import ( "context" "encoding/json" + "errors" "fmt" "log/slog" "strconv" @@ -999,6 +1000,39 @@ func (s *Scheduler) resolveAllowedLanguages(ctx context.Context, author *models. return models.ParseAllowedLanguages(p.AllowedLanguages) } +// pinnedClient returns the download client the release's indexer is pinned to +// (#3080), or (nil, nil) when the indexer has no pin. A pin that cannot be +// honoured is a *db.PinnedClientError, never a different client. +func (s *Scheduler) pinnedClient(ctx context.Context, res *newznab.SearchResult) (*models.DownloadClient, error) { + if s.indexers == nil || res.IndexerID == 0 { + return nil, nil + } + idx, err := s.indexers.GetByID(ctx, res.IndexerID) + if err != nil { + return nil, err + } + if idx == nil || idx.DownloadClientID == nil { + return nil, nil + } + return s.clients.ResolvePinnedClient(ctx, idx, res.Protocol) +} + +// recordPinnedFailure writes the refused grab to History, so it is not only a +// log line. History is readable by every user, so it names the indexer and not +// the client; the log line carries both. +func (s *Scheduler) recordPinnedFailure(ctx context.Context, bookID int64, res *newznab.SearchResult, perr *db.PinnedClientError) { + if s.history == nil { + return + } + data, _ := json.Marshal(map[string]any{"guid": res.GUID, "message": perr.Message(false)}) + _ = s.history.Create(ctx, &models.HistoryEvent{ + BookID: &bookID, + EventType: models.HistoryEventDownloadFailed, + SourceTitle: res.Title, + Data: string(data), + }) +} + // resolveSeedLimits looks up the per-indexer seed ratio (#883) and seed time // (#2206) overrides for the indexer a release was grabbed from. Each is nil // (no override) when the indexer repo is unset, the id is zero, the lookup @@ -1262,13 +1296,31 @@ func (s *Scheduler) searchAndGrabFormat(ctx context.Context, book models.Book, m } approved = 1 - candidates, err := s.clients.GetEnabledByProtocol(ctx, best.Protocol) - if err != nil { - slog.Error("SearchAndGrabBook: failed to list download clients", "protocol", best.Protocol, "error", err) + // An indexer pinned to a download client (#3080) sends every grab to that + // client or fails. There is deliberately no fallback to the default pick. + client, pinErr := s.pinnedClient(ctx, best) + if pinErr != nil { + var perr *db.PinnedClientError + if errors.As(pinErr, &perr) { + slog.Warn("SearchAndGrabBook: the indexer's pinned download client cannot take the grab", + "book", book.Title, "release", best.Title, "error", perr.Error()) + s.recordPinnedFailure(ctx, book.ID, best, perr) + outcome = "pinned download client unavailable" + return + } + slog.Error("SearchAndGrabBook: failed to resolve the indexer's pinned download client", "indexer", best.IndexerName, "error", pinErr) outcome = "download client lookup failed" return } - client := db.PickClientForMediaType(candidates, mediaType) + if client == nil { + candidates, err := s.clients.GetEnabledByProtocol(ctx, best.Protocol) + if err != nil { + slog.Error("SearchAndGrabBook: failed to list download clients", "protocol", best.Protocol, "error", err) + outcome = "download client lookup failed" + return + } + client = db.PickClientForMediaType(candidates, mediaType) + } // No cross-protocol fallback: a usenet release must not be pushed to a // torrent client (qBittorrent would accept the .nzb URL, fail to parse it // as a torrent, and report "hash could not be determined"), and vice versa. @@ -1361,7 +1413,7 @@ func (s *Scheduler) searchAndGrabFormat(ctx context.Context, book models.Book, m 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) + slog.Error("SearchAndGrabBook: failed to send to downloader", "client", client.Type, "client_name", client.Name, "indexer", best.IndexerName, "title", best.Title, "error", err) if setErr := s.downloads.SetError(ctx, dl.ID, err.Error()); setErr != nil { slog.Warn("failed to persist download error", "download_id", dl.ID, "error", setErr) } diff --git a/internal/scheduler/scheduler_indexer_pin_test.go b/internal/scheduler/scheduler_indexer_pin_test.go new file mode 100644 index 000000000..cd4f6ebc9 --- /dev/null +++ b/internal/scheduler/scheduler_indexer_pin_test.go @@ -0,0 +1,216 @@ +package scheduler + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/httpsec" + "github.com/vavallee/bindery/internal/indexer/newznab" + "github.com/vavallee/bindery/internal/models" +) + +// pinSAB is a fake SABnzbd that also serves any *.nzb path and counts addfile +// calls, so a test can say which of two clients a grab reached. +type pinSAB struct { + mu sync.Mutex + adds int + srv *httptest.Server +} + +func newPinSAB(t *testing.T) *pinSAB { + t.Helper() + p := &pinSAB{} + p.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, ".nzb") { + w.Header().Set("Content-Type", "application/x-nzb") + _, _ = w.Write([]byte(``)) + return + } + if r.URL.Query().Get("mode") == "addfile" { + p.mu.Lock() + p.adds++ + p.mu.Unlock() + } + _ = json.NewEncoder(w).Encode(map[string]any{"status": true, "nzo_ids": []string{"nzo-1"}}) + })) + t.Cleanup(p.srv.Close) + return p +} + +func (p *pinSAB) count() int { + p.mu.Lock() + defer p.mu.Unlock() + return p.adds +} + +type schedPinFixture struct { + s *Scheduler + ctx context.Context + clients *db.DownloadClientRepo + indexers *db.IndexerRepo + downloads *db.DownloadRepo + history *db.HistoryRepo + book models.Book + pub, priv *pinSAB + pubC *models.DownloadClient + privC *models.DownloadClient + idx *models.Indexer +} + +func newSchedPinFixture(t *testing.T) *schedPinFixture { + t.Helper() + t.Cleanup(httpsec.AllowLoopbackForTests()) + pub, priv, rel := newPinSAB(t), newPinSAB(t), newPinSAB(t) + + database, err := db.OpenMemory() + if err != nil { + t.Fatalf("OpenMemory: %v", err) + } + t.Cleanup(func() { database.Close() }) + ctx := context.Background() + + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + clients := db.NewDownloadClientRepo(database) + downloads := db.NewDownloadRepo(database) + indexers := db.NewIndexerRepo(database) + history := db.NewHistoryRepo(database) + + a := &models.Author{ForeignID: "OL-PIN-A", Name: "Pin Author", SortName: "Author, Pin", MetadataProvider: "ol", Monitored: true} + if err := authors.Create(ctx, a); err != nil { + t.Fatalf("author create: %v", err) + } + book := models.Book{ + ForeignID: "OL-PIN-B", AuthorID: a.ID, Title: "Pinned Book", SortTitle: "Pinned Book", + Status: models.BookStatusWanted, Genres: []string{}, MetadataProvider: "ol", Monitored: true, + MediaType: models.MediaTypeEbook, + } + if err := books.Create(ctx, &book); err != nil { + t.Fatalf("book create: %v", err) + } + + pubHost, pubPort := stallServerHostPort(t, pub.srv.URL) + pubC := &models.DownloadClient{Name: "sab-public", Type: "sabnzbd", Host: pubHost, Port: pubPort, Enabled: true, Priority: 1} + if err := clients.Create(ctx, pubC); err != nil { + t.Fatalf("client create: %v", err) + } + privHost, privPort := stallServerHostPort(t, priv.srv.URL) + privC := &models.DownloadClient{Name: "sab-private", Type: "sabnzbd", Host: privHost, Port: privPort, Enabled: true, Priority: 50} + if err := clients.Create(ctx, privC); err != nil { + t.Fatalf("client create: %v", err) + } + idx := &models.Indexer{Name: "TrackerX", Type: "newznab", URL: rel.srv.URL, Enabled: true} + if err := indexers.Create(ctx, idx); err != nil { + t.Fatalf("indexer create: %v", err) + } + + searcher := &perFormatSearcher{byFormat: map[string][]newznab.SearchResult{ + models.MediaTypeEbook: {{ + GUID: "g-pin", Title: "Pin Author Pinned Book epub", NZBURL: rel.srv.URL + "/pin.nzb", + Protocol: "usenet", IndexerID: idx.ID, IndexerName: "TrackerX", Size: 2 << 20, + }}, + }} + s := &Scheduler{ + searcher: searcher, + indexers: indexers, + authors: authors, + books: books, + settings: db.NewSettingsRepo(database), + blocklist: db.NewBlocklistRepo(database), + downloads: downloads, + clients: clients, + history: history, + profiles: db.NewMetadataProfileRepo(database), + } + return &schedPinFixture{ + s: s, ctx: ctx, clients: clients, indexers: indexers, downloads: downloads, history: history, + book: book, pub: pub, priv: priv, pubC: pubC, privC: privC, idx: idx, + } +} + +func (f *schedPinFixture) pin(t *testing.T, clientID int64) { + t.Helper() + f.idx.DownloadClientID = &clientID + if err := f.indexers.Update(f.ctx, f.idx); err != nil { + t.Fatalf("pin: %v", err) + } +} + +// TestAutoGrab_UnpinnedIndexerRoutesAsBefore is the regression guard. +func TestAutoGrab_UnpinnedIndexerRoutesAsBefore(t *testing.T) { + f := newSchedPinFixture(t) + f.s.SearchAndGrabBook(f.ctx, f.book) + if f.pub.count() != 1 || f.priv.count() != 0 { + t.Fatalf("unpinned: public=%d private=%d, want 1 and 0", f.pub.count(), f.priv.count()) + } +} + +// TestAutoGrab_PinnedIndexerGoesToItsClient: the pin beats priority, and the +// download row records the pinned client. +func TestAutoGrab_PinnedIndexerGoesToItsClient(t *testing.T) { + f := newSchedPinFixture(t) + f.pin(t, f.privC.ID) + f.s.SearchAndGrabBook(f.ctx, f.book) + if f.pub.count() != 0 || f.priv.count() != 1 { + t.Fatalf("pinned: public=%d private=%d, want 0 and 1", f.pub.count(), f.priv.count()) + } + dl, err := f.downloads.GetByGUID(f.ctx, "g-pin") + if err != nil || dl == nil { + t.Fatalf("download row: %v", err) + } + if dl.DownloadClientID == nil || *dl.DownloadClientID != f.privC.ID { + t.Errorf("DownloadClientID = %v, want %d", dl.DownloadClientID, f.privC.ID) + } +} + +// TestAutoGrab_PinnedClientDisabledFailsWithoutFallback: nothing reaches the +// other client, and History says why, naming indexer and client. +func TestAutoGrab_PinnedClientDisabledFailsWithoutFallback(t *testing.T) { + f := newSchedPinFixture(t) + f.pin(t, f.privC.ID) + f.privC.Enabled = false + if err := f.clients.Update(f.ctx, f.privC); err != nil { + t.Fatal(err) + } + f.s.SearchAndGrabBook(f.ctx, f.book) + if f.pub.count() != 0 || f.priv.count() != 0 { + t.Fatalf("a grab with its pinned client disabled reached a client: public=%d private=%d", f.pub.count(), f.priv.count()) + } + if dl, _ := f.downloads.GetByGUID(f.ctx, "g-pin"); dl != nil { + t.Errorf("a download row was created for a grab that never happened: %+v", dl) + } + events, err := f.history.ListByBook(f.ctx, f.book.ID) + if err != nil { + t.Fatal(err) + } + found := false + for _, e := range events { + if e.EventType == models.HistoryEventDownloadFailed && strings.Contains(e.Data, "TrackerX") && strings.Contains(e.Data, "disabled") { + found = true + } + } + if !found { + t.Errorf("no downloadFailed history event naming the indexer and the disabled client: %+v", events) + } +} + +// TestAutoGrab_PinnedClientWrongProtocolFails: a client whose type was changed +// to the other protocol after the pin is refused at grab time. +func TestAutoGrab_PinnedClientWrongProtocolFails(t *testing.T) { + f := newSchedPinFixture(t) + f.pin(t, f.privC.ID) + f.privC.Type = "qbittorrent" + if err := f.clients.Update(f.ctx, f.privC); err != nil { + t.Fatal(err) + } + f.s.SearchAndGrabBook(f.ctx, f.book) + if f.pub.count() != 0 || f.priv.count() != 0 { + t.Fatalf("reached a client: public=%d private=%d", f.pub.count(), f.priv.count()) + } +} From 002f67f9ba4d7dc28df9290e1635f62d8ce33436 Mon Sep 17 00:00:00 2001 From: vavallee Date: Wed, 14 Oct 2026 21:39:09 -0300 Subject: [PATCH 3/8] feat(web): pick a download client per indexer, with docs and changelog (#3080) Download client dropdown on the indexer forms listing only enabled clients of the indexer's protocol, a row warning while the pinned client is disabled, and the user guide, API, troubleshooting and changelog entries. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- .../3080-per-indexer-download-client.md | 2 + docs/API.md | 24 +++ docs/Troubleshooting-Wiki.md | 17 ++ docs/User-Guide-Wiki.md | 26 +++ web/src/api/indexers.ts | 5 + web/src/i18n/locales/en.json | 8 +- web/src/pages/SettingsPage.test.tsx | 6 +- .../pages/settings/IndexersTab.pin.test.tsx | 164 ++++++++++++++++++ web/src/pages/settings/IndexersTab.tsx | 109 +++++++++++- 9 files changed, 350 insertions(+), 11 deletions(-) create mode 100644 changelog.d/3080-per-indexer-download-client.md create mode 100644 web/src/pages/settings/IndexersTab.pin.test.tsx diff --git a/changelog.d/3080-per-indexer-download-client.md b/changelog.d/3080-per-indexer-download-client.md new file mode 100644 index 000000000..57cc4fd57 --- /dev/null +++ b/changelog.d/3080-per-indexer-download-client.md @@ -0,0 +1,2 @@ +### Added +- **Per-indexer download client** (#3080): each indexer now has an optional **Download client** setting in Settings → Indexers. Leave it on Default and nothing changes. Pick a client and every grab from that indexer goes to it: scheduled searches, the search page, held releases in Pending and Retry download. It overrides the client's priority and category routing, so a private tracker's torrents can live on their own qBittorrent. If that client is disabled, missing, the wrong protocol or not answering, the grab fails with a message naming the indexer and the client and is never sent to another client. A download client that an indexer is pinned to cannot be deleted (the request is refused with the indexer names), and the indexer row shows a warning while its client is disabled. Prowlarr syncs keep the setting. diff --git a/docs/API.md b/docs/API.md index fbd901a2b..e2bbeab68 100644 --- a/docs/API.md +++ b/docs/API.md @@ -538,6 +538,30 @@ value is rejected with 400. `GET /indexer` and `GET /indexer/{id}` also return `dailyQueriesUsed` on capped indexers, which is a display figure summed from the stored hourly buckets and lags the live tally by up to one flush interval. +#### Per-indexer download client pin + +`downloadClientId` on an indexer pins every grab from it to one download client +(#3080). `null` and omitted on create mean no pin, which keeps the usual routing. +Omitted on `PUT` keeps the stored pin, and `null` clears it. The client must exist +and its protocol must match the indexer's (`torznab` is torrent, `newznab` is +usenet), otherwise `POST` and `PUT` answer `400` with the reason. A client that is +disabled is refused for a new or changed pin, but an unrelated edit of an +indexer already pinned to a client that was disabled later still saves. + +A pinned grab ignores the client's priority and media type routing. If the pinned +client is disabled, missing or of the wrong protocol, the grab fails and no other +client is tried: `POST /queue/grab`, `POST /queue/{id}/retry`, the bulk retry and +`POST /pending/{id}/grab` answer `400` with `code: "indexer_client_unavailable"` +and a message naming the indexer (and the client, for admins). A client that is +enabled but does not answer is the usual `502`, with the message extended to say +the indexer is pinned and no other client was tried. The field is only returned by +the admin only indexer endpoints. + +`DELETE /downloadclient/{id}` answers `409` while any indexer is pinned to the +client, with `error` and an `indexers` array of their names. Disabling the client +with `PUT` is not refused. A Prowlarr sync, and any other update from outside, +keeps the pin. + #### Per-indexer seed limits Three optional overrides are applied to a torrent grabbed from the indexer: diff --git a/docs/Troubleshooting-Wiki.md b/docs/Troubleshooting-Wiki.md index 0ff33c8ac..12495f69a 100644 --- a/docs/Troubleshooting-Wiki.md +++ b/docs/Troubleshooting-Wiki.md @@ -249,6 +249,23 @@ A Queue row in `failed` never produced a file: the grab did not reach the downlo To retry in bulk, tick the rows and use **Retry selected**, or use **Retry all failed** above the list, which covers both stages: an import failure has its import re-armed, a failed download has its release re-sent. Both ask for confirmation when a re-send is in the batch, because that hands work back to your download client. +### A grab fails with "indexer ... is pinned to download client ..." + +The indexer has a **Download client** set in Settings → Indexers, and that client +cannot take the grab. The message says which of three things is wrong: the client +is **disabled** (enable it in Settings → Download Clients), it **no longer +exists** (choose another client on the indexer), or it is the **wrong protocol** +(a torrent client on a usenet indexer, usually because the client's type was +changed after the pin). If the client is enabled but the message says the +release could not be sent, the client is not answering: check that it is running +and reachable from Bindery. + +Bindery will not send the release to a different client. That is deliberate: the +pin exists so a private tracker's torrents never reach your public client. To +send the indexer's grabs through the normal routing again, set its **Download +client** back to *Default*. Failed scheduled grabs appear in History, and the +Queue row of a failed grab offers **Retry download** once the cause is fixed. + ### The Queue looks empty, or shorter than it should be Look for a banner above the list saying *Could not reach ``. This list may be incomplete*. Bindery gives each download client a short deadline when it renders the Queue, and a client that misses it is dropped from that render, so its rows are missing until the next poll succeeds. The rows and the downloads themselves are fine. Check the client is up and reachable at the host and port in **Settings → Download clients** (its **Test** button is the quickest answer), and remember a client that is up but very slow to answer will do this intermittently. diff --git a/docs/User-Guide-Wiki.md b/docs/User-Guide-Wiki.md index 0658ff259..d04a05bcc 100644 --- a/docs/User-Guide-Wiki.md +++ b/docs/User-Guide-Wiki.md @@ -424,6 +424,32 @@ book set to both formats is still held for its sibling and released with it. Aud the indexer, and why the category (default `books`) must already exist in the client. Bindery fetches the .torrent/NZB itself and hands it over. +**Pinning an indexer to a download client.** Settings → Indexers has a +**Download client** dropdown on every indexer. Leave it on *Default* and +nothing changes: Bindery picks the client by protocol, priority and media type +as before. Pick a client and every grab from that indexer goes to that client, +whether it came from the scheduled search, the search page, the Pending tab, or +Retry download on the Queue. This is how you keep a private tracker's torrents +on a separate qBittorrent (its own VPN, port or seeding policy) from your public +one. The list shows only enabled clients of the indexer's protocol, and saving a +pin to a client of the other protocol is refused. + +* **A pin wins over everything else.** It overrides the client's priority and + its media type or category routing: an audiobook from a pinned indexer goes to + the pinned client even if another client has an audiobook category. +* **There is no fallback.** If the pinned client is disabled, missing or of the + wrong protocol, or does not answer, the grab fails with a message naming the + indexer and the client, and nothing is sent to any other client. In the + scheduler the failure shows in History and the log. The indexer's row in + Settings shows an amber warning while its pinned client is disabled. Enable + the client or pick another one and the next grab goes through. +* **A pinned client cannot be deleted.** Deleting a download client that an + indexer is pinned to is refused (HTTP 409) with the indexer names. Choose + another client for those indexers, or set them back to *Default*, first. + Disabling the client is allowed. +* **A Prowlarr sync keeps the pin.** It is local configuration like priority and + the seed limits. + **Seeding limits.** A torrent indexer can carry its own seeding rules in Settings → Indexers, so a private tracker's minimum is met without changing the client's global rules for everything else. **Seed ratio** stops seeding at that diff --git a/web/src/api/indexers.ts b/web/src/api/indexers.ts index aca3c7440..392a519eb 100644 --- a/web/src/api/indexers.ts +++ b/web/src/api/indexers.ts @@ -42,6 +42,11 @@ export interface Indexer { // requests rather than books: one book on one indexer costs between one and // eight, depending on how far the query cascade falls through. dailyQueryLimit?: number | null + // Download client this indexer is pinned to (#3080). null/omitted = no pin + // (automatic routing). When set, every grab from this indexer goes to that + // client and fails rather than falling back if it cannot take it. The + // client must be enabled and of the indexer's protocol when pinning. + downloadClientId?: number | null // How much of dailyQueryLimit is spent, written by the backend rather than // the user, and absent on an indexer with no cap. It lags the live tally by // up to one flush interval, so it is a display figure and not the number the diff --git a/web/src/i18n/locales/en.json b/web/src/i18n/locales/en.json index 691cafabf..73dcaf59b 100644 --- a/web/src/i18n/locales/en.json +++ b/web/src/i18n/locales/en.json @@ -1184,8 +1184,14 @@ "freeleechOnlyHint": "Automatic grabs from this indexer are limited to freeleech releases; anything else is held in the Pending queue for manual approval. Interactive search still shows everything. For private trackers where ratio matters — leave off for public trackers and Usenet.", "dailyQueryLimit": "Daily query limit", "dailyQueryLimitPlaceholder": "e.g. 1000", - "dailyQueryLimitHint": "Stop searching this indexer once it has been sent this many requests in the last 24 hours. Leave blank for no limit. Requests are counted in hourly blocks, so capacity comes back an hour at a time rather than all at once, and a block is only released after it is a full hour outside the window. The unit is requests, not books: one book costs between one and eight depending on how far the search has to fall back. Use it on trackers with a daily API allowance, so one scheduled sweep over a large library cannot spend the whole thing. The Test button is exempt and never counts." + "dailyQueryLimitHint": "Stop searching this indexer once it has been sent this many requests in the last 24 hours. Leave blank for no limit. Requests are counted in hourly blocks, so capacity comes back an hour at a time rather than all at once, and a block is only released after it is a full hour outside the window. The unit is requests, not books: one book costs between one and eight depending on how far the search has to fall back. Use it on trackers with a daily API allowance, so one scheduled sweep over a large library cannot spend the whole thing. The Test button is exempt and never counts.", + "downloadClient": "Download client", + "downloadClientDefault": "Default (automatic routing)", + "downloadClientDisabledOption": "{{name}} (disabled)", + "downloadClientHint": "Send every grab from this indexer to one download client, whatever its priority or category routing says. Leave on Default to let Bindery choose. If the chosen client is disabled, missing or unreachable the grab fails with an error instead of going to another client, so private tracker traffic never reaches your public client. Only enabled clients of this indexer's protocol are listed." }, + "pinnedClientDisabled": "Pinned to download client {{name}}, which is disabled. Grabs from this indexer will fail, and will not go to another client, until you enable it or choose a different one.", + "pinnedClientMissing": "Pinned to a download client that no longer exists. Grabs from this indexer will fail until you choose a different one.", "quotaUsage": "{{used}} of {{limit}} queries in the last 24 hours", "quotaReached": "Daily query limit of {{limit}} reached. This indexer is being skipped until the oldest hour of queries falls out of the window.", "healthAuthFail": "This indexer rejected the last search and will keep rejecting until someone fixes it: {{error}}", diff --git a/web/src/pages/SettingsPage.test.tsx b/web/src/pages/SettingsPage.test.tsx index cbd5b73a4..9b2352ab0 100644 --- a/web/src/pages/SettingsPage.test.tsx +++ b/web/src/pages/SettingsPage.test.tsx @@ -1451,7 +1451,7 @@ describe('SettingsPage', () => { fireEvent.click(screen.getByRole('button', { name: 'settings.indexers.addButton' })) fireEvent.change(screen.getByPlaceholderText('settings.indexers.form.namePlaceholderExample'), { target: { value: 'SceneNZBs' } }) - fireEvent.change(screen.getByRole('combobox'), { target: { value: 'torznab' } }) + fireEvent.change(screen.getByDisplayValue('settings.indexers.form.typeNewznab'), { target: { value: 'torznab' } }) fireEvent.change(screen.getByPlaceholderText('settings.indexers.form.urlPlaceholderExample'), { target: { value: 'http://prowlarr:9696/1/api' } }) fireEvent.change(screen.getByPlaceholderText('settings.indexers.form.apiKey'), { target: { value: 'scene-key' } }) fireEvent.change(screen.getByDisplayValue('7020'), { target: { value: '7020, 7120, bad, 3030' } }) @@ -1472,6 +1472,7 @@ describe('SettingsPage', () => { inactiveSeedTimeMinutes: null, freeleechOnly: false, dailyQueryLimit: null, + downloadClientId: null, }) }) expect(await screen.findByText('SceneNZBs')).toBeInTheDocument() @@ -1485,7 +1486,7 @@ describe('SettingsPage', () => { fireEvent.click(screen.getByRole('button', { name: 'common.edit' })) fireEvent.change(screen.getByPlaceholderText('settings.indexers.form.namePlaceholder'), { target: { value: 'DrunkenSlug' } }) - fireEvent.change(screen.getByRole('combobox'), { target: { value: 'torznab' } }) + fireEvent.change(screen.getByDisplayValue('settings.indexers.form.typeNewznabUsenet'), { target: { value: 'torznab' } }) fireEvent.change(screen.getByPlaceholderText('settings.indexers.form.urlPlaceholder'), { target: { value: 'https://slug.example.com/api' } }) fireEvent.change(screen.getByPlaceholderText('••••••••'), { target: { value: 'slug-key' } }) fireEvent.change(screen.getByDisplayValue('7020, 3030'), { target: { value: '7020, bad, 3030' } }) @@ -1505,6 +1506,7 @@ describe('SettingsPage', () => { inactiveSeedTimeMinutes: null, freeleechOnly: false, dailyQueryLimit: null, + downloadClientId: null, }) }) expect(await screen.findByText('DrunkenSlug')).toBeInTheDocument() diff --git a/web/src/pages/settings/IndexersTab.pin.test.tsx b/web/src/pages/settings/IndexersTab.pin.test.tsx new file mode 100644 index 000000000..bcd758cc0 --- /dev/null +++ b/web/src/pages/settings/IndexersTab.pin.test.tsx @@ -0,0 +1,164 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { render, screen, waitFor, fireEvent, within } from '@testing-library/react' +import { useState } from 'react' +import type { DownloadClient, Indexer, ProwlarrInstance } from '../../api/client' + +// i18n: echo the key plus interpolated options so assertions do not depend on +// English copy. +vi.mock('react-i18next', () => ({ + useTranslation: () => ({ + t: (key: string, options?: Record) => { + if (!options) return key + let out = key + for (const [k, v] of Object.entries(options)) out += ` ${k}=${String(v)}` + return out + }, + }), +})) + +vi.mock('../../api/client', () => ({ + api: { + addIndexer: vi.fn(), + updateIndexer: vi.fn(), + deleteIndexer: vi.fn(), + testIndexer: vi.fn(), + testIndexerConfig: vi.fn(), + listDownloadClients: vi.fn(), + }, +})) + +import { api } from '../../api/client' +import IndexersTab from './IndexersTab' + +const m = vi.mocked(api) + +function client(overrides: Partial): DownloadClient { + return { + id: 1, name: 'c', type: 'qbittorrent', host: 'h', port: 1, apiKey: '', username: '', password: '', + apiKeyConfigured: false, passwordConfigured: false, useSsl: false, urlBase: '', category: '', + enabled: true, ...overrides, + } +} + +const clients: DownloadClient[] = [ + client({ id: 1, name: 'qbit-public', type: 'qbittorrent' }), + client({ id: 2, name: 'qbit-private', type: 'qbittorrent' }), + client({ id: 3, name: 'qbit-off', type: 'qbittorrent', enabled: false }), + client({ id: 4, name: 'sab', type: 'sabnzbd' }), +] + +function makeIndexer(overrides: Partial = {}): Indexer { + return { + id: 1, name: 'TrackerX', type: 'torznab', url: 'https://t.example/api', apiKey: '', apiKeyConfigured: true, + categories: [7020], priority: 0, enabled: true, ...overrides, + } as Indexer +} + +function Harness({ indexers: initial }: { indexers: Indexer[] }) { + const [indexers, setIndexers] = useState(initial) + const [prowlarr, setProwlarr] = useState([]) + return +} + +const label = 'settings.indexers.form.downloadClient' + +beforeEach(() => { + vi.clearAllMocks() + m.listDownloadClients.mockResolvedValue(clients) +}) + +async function openEdit() { + fireEvent.click(screen.getByText('common.edit')) + return await screen.findByLabelText(label) +} + +describe('IndexersTab download client pin', () => { + it('lists only enabled clients of the indexer protocol', async () => { + render() + const select = await openEdit() + const names = within(select).getAllByRole('option').map(o => o.textContent) + expect(names).toEqual(['settings.indexers.form.downloadClientDefault', 'qbit-public', 'qbit-private']) + }) + + it('lists usenet clients for a usenet indexer', async () => { + render() + const select = await openEdit() + const names = within(select).getAllByRole('option').map(o => o.textContent) + expect(names).toEqual(['settings.indexers.form.downloadClientDefault', 'sab']) + }) + + it('sends the chosen client id on save', async () => { + m.updateIndexer.mockImplementation(async (_id, p) => ({ ...makeIndexer(), ...p }) as Indexer) + render() + const select = await openEdit() + fireEvent.change(select, { target: { value: '2' } }) + fireEvent.click(screen.getByText('common.save')) + await waitFor(() => expect(m.updateIndexer).toHaveBeenCalledWith(1, expect.objectContaining({ downloadClientId: 2 }))) + }) + + it('sends null when the pin is cleared back to Default', async () => { + m.updateIndexer.mockImplementation(async (_id, p) => ({ ...makeIndexer(), ...p }) as Indexer) + render() + const select = await openEdit() + expect((select as HTMLSelectElement).value).toBe('2') + fireEvent.change(select, { target: { value: '' } }) + fireEvent.click(screen.getByText('common.save')) + await waitFor(() => expect(m.updateIndexer).toHaveBeenCalledWith(1, expect.objectContaining({ downloadClientId: null }))) + }) + + it('leaves an unpinned indexer unpinned on save', async () => { + m.updateIndexer.mockImplementation(async (_id, p) => ({ ...makeIndexer(), ...p }) as Indexer) + render() + await openEdit() + fireEvent.click(screen.getByText('common.save')) + await waitFor(() => expect(m.updateIndexer).toHaveBeenCalledWith(1, expect.objectContaining({ downloadClientId: null }))) + }) + + it('drops a pin the new indexer type cannot use', async () => { + m.updateIndexer.mockImplementation(async (_id, p) => ({ ...makeIndexer(), ...p }) as Indexer) + render() + await openEdit() + fireEvent.change(screen.getByDisplayValue('settings.indexers.form.typeTorznabTorrent'), { target: { value: 'newznab' } }) + fireEvent.click(screen.getByText('common.save')) + await waitFor(() => expect(m.updateIndexer).toHaveBeenCalledWith(1, expect.objectContaining({ type: 'newznab', downloadClientId: null }))) + }) + + it('warns on the row when the pinned client is disabled, and keeps it selectable as disabled', async () => { + render() + expect(await screen.findByText('settings.indexers.pinnedClientDisabled name=qbit-off')).toBeInTheDocument() + const select = await openEdit() + const names = within(select).getAllByRole('option').map(o => o.textContent) + expect(names).toContain('settings.indexers.form.downloadClientDisabledOption name=qbit-off') + }) + + it('warns when the pinned client no longer exists', async () => { + render() + expect(await screen.findByText('settings.indexers.pinnedClientMissing')).toBeInTheDocument() + }) + + it('shows no warning for an enabled pin or no pin', async () => { + render() + await waitFor(() => expect(m.listDownloadClients).toHaveBeenCalled()) + await waitFor(() => expect(screen.queryByText(/pinnedClient/)).not.toBeInTheDocument()) + }) + + it('does not warn about a missing client while the list is still loading', () => { + m.listDownloadClients.mockReturnValue(new Promise(() => {})) + render() + expect(screen.queryByText('settings.indexers.pinnedClientMissing')).not.toBeInTheDocument() + }) + + it('sends the pin when adding an indexer', async () => { + m.addIndexer.mockImplementation(async p => ({ ...makeIndexer(), ...p, id: 9 }) as Indexer) + render() + fireEvent.click(screen.getByText('settings.indexers.addButton')) + fireEvent.change(screen.getByPlaceholderText('settings.indexers.form.namePlaceholderExample'), { target: { value: 'New' } }) + fireEvent.change(screen.getByPlaceholderText('settings.indexers.form.urlPlaceholderExample'), { target: { value: 'https://n/api' } }) + fireEvent.change(screen.getByDisplayValue('settings.indexers.form.typeNewznab'), { target: { value: 'torznab' } }) + const select = await screen.findByLabelText(label) + await waitFor(() => expect(within(select).getAllByRole('option').length).toBeGreaterThan(1)) + fireEvent.change(select, { target: { value: '1' } }) + fireEvent.click(screen.getByText('common.save')) + await waitFor(() => expect(m.addIndexer).toHaveBeenCalledWith(expect.objectContaining({ downloadClientId: 1 }))) + }) +}) diff --git a/web/src/pages/settings/IndexersTab.tsx b/web/src/pages/settings/IndexersTab.tsx index 5e94ab2b4..01bf8f6ac 100644 --- a/web/src/pages/settings/IndexersTab.tsx +++ b/web/src/pages/settings/IndexersTab.tsx @@ -1,7 +1,7 @@ -import { useState } from 'react' +import { useEffect, useState } from 'react' import { useTranslation } from 'react-i18next' import { useConfirmDialog } from '../../components/useConfirmDialog' -import { api, Indexer, IndexerTestResult, ProwlarrInstance } from '../../api/client' +import { api, DownloadClient, Indexer, IndexerTestResult, ProwlarrInstance } from '../../api/client' // client.ts re-exports types only, so the helper comes from its own module. import { indexerNeedsAttention, indexerQueryCap } from '../../api/indexers' import { inputCls } from './formStyles' @@ -220,6 +220,60 @@ function DailyQueryLimitField({ value, onChange }: { value: number | null | unde ) } +const TORRENT_CLIENT_TYPES = ['qbittorrent', 'transmission', 'deluge', 'rtorrent'] + +// clientProtocol mirrors the backend's db.ClientProtocol. +function clientProtocol(clientType: string): 'torrent' | 'usenet' { + return TORRENT_CLIENT_TYPES.includes(clientType) ? 'torrent' : 'usenet' +} + +function indexerProtocol(indexerType: string): 'torrent' | 'usenet' { + return indexerType === 'torznab' ? 'torrent' : 'usenet' +} + +// DownloadClientField pins an indexer to one download client (#3080). It lists +// only enabled clients of the indexer's protocol. A client the indexer is +// already pinned to stays listed, marked disabled, so the form does not show a +// blank that hides the real saved value. +function DownloadClientField({ value, onChange, indexerType, clients }: { + value: number | null | undefined + onChange: (v: number | null) => void + indexerType: string + clients: DownloadClient[] +}) { + const { t } = useTranslation() + const protocol = indexerProtocol(indexerType) + const options = clients.filter(c => clientProtocol(c.type) === protocol && (c.enabled || c.id === value)) + return ( +
+ + +

{t('settings.indexers.form.downloadClientHint')}

+
+ ) +} + +// clearPinOnProtocolChange drops a pin that the indexer's new type cannot use, +// so switching a form from torrent to usenet does not carry a torrent client +// along into a save the server would refuse. +function clearPinOnProtocolChange(current: number | null, newType: string, clients: DownloadClient[]): number | null { + if (current == null) return null + const c = clients.find(x => x.id === current) + return c && clientProtocol(c.type) === indexerProtocol(newType) ? current : null +} + // FreeleechOnlyField gates automatic grabbing to freeleech releases. It sits // next to the seed-ratio override because both are tracker-economics knobs. function FreeleechOnlyField({ value, onChange }: { value: boolean; onChange: (v: boolean) => void }) { @@ -271,6 +325,17 @@ export default function IndexersTab({ indexers, setIndexers, prowlarrInstances, const [indexerTestResults, setIndexerTestResults] = useState>({}) const [confirmDeleteIndexer, setConfirmDeleteIndexer] = useState(null) const [prowlarrTestResult, setProwlarrTestResult] = useState>({}) + // null until the list has loaded, so a pin is not reported missing while the + // clients are still on their way. + const [clients, setClients] = useState(null) + useEffect(() => { + let live = true + Promise.resolve() + .then(() => api.listDownloadClients()) + .then(list => { if (live) setClients(list ?? []) }) + .catch(() => { if (live) setClients([]) }) + return () => { live = false } + }, []) return (
@@ -339,6 +404,28 @@ export default function IndexersTab({ indexers, setIndexers, prowlarrInstances, )}
+ {/* A pin to a client that cannot take grabs (#3080). Grabs from + this indexer fail loudly instead of going to another client, + so the row says so before anyone has to find out the hard + way. */} + {(() => { + if (idx.downloadClientId == null || clients === null) return null + const pinned = clients.find(c => c.id === idx.downloadClientId) + if (pinned?.enabled) return null + return ( +
+ + + {pinned + ? t('settings.indexers.pinnedClientDisabled', { name: pinned.name }) + : t('settings.indexers.pinnedClientMissing')} + +
+ ) + })()} {/* How much of the daily query cap is spent (#2312). Only rendered for indexers that have one, and amber once the cap is reached, because from that point Bindery is skipping the @@ -430,6 +517,7 @@ export default function IndexersTab({ indexers, setIndexers, prowlarrInstances, {editingIndexer === idx.id && ( setEditingIndexer(null)} onSaved={(updated) => { setIndexers(indexers.map(i => i.id === updated.id ? updated : i)); setEditingIndexer(null) }} /> @@ -440,6 +528,7 @@ export default function IndexersTab({ indexers, setIndexers, prowlarrInstances, )} {showAddIndexer && ( setShowAddIndexer(false)} onAdded={(idx) => { setIndexers([...indexers, idx]); setShowAddIndexer(false) }} /> @@ -558,7 +647,7 @@ export default function IndexersTab({ indexers, setIndexers, prowlarrInstances, ) } -function EditIndexerForm({ indexer, onClose, onSaved }: { indexer: Indexer; onClose: () => void; onSaved: (idx: Indexer) => void }) { +function EditIndexerForm({ indexer, clients, onClose, onSaved }: { indexer: Indexer; clients: DownloadClient[]; onClose: () => void; onSaved: (idx: Indexer) => void }) { const { t } = useTranslation() const [name, setName] = useState(indexer.name) const [type, setType] = useState(indexer.type || 'newznab') @@ -574,12 +663,13 @@ function EditIndexerForm({ indexer, onClose, onSaved }: { indexer: Indexer; onCl const [inactiveSeedTimeMinutes, setInactiveSeedTimeMinutes] = useState(indexer.inactiveSeedTimeMinutes ?? null) const [freeleechOnly, setFreeleechOnly] = useState(indexer.freeleechOnly ?? false) const [dailyQueryLimit, setDailyQueryLimit] = useState(indexer.dailyQueryLimit ?? null) + const [downloadClientId, setDownloadClientId] = useState(indexer.downloadClientId ?? null) const [testing, setTesting] = useState(false) const [testResult, setTestResult] = useState(null) const labelCls = 'block text-xs text-slate-600 dark:text-zinc-400 mb-1' const submit = async () => { - const payload: Partial = { ...indexer, name, type, url, categories: parseCats(categories), includeParentCategories, priority: parsePriority(priority), seedRatio, seedTimeMinutes, inactiveSeedTimeMinutes, freeleechOnly, dailyQueryLimit } + const payload: Partial = { ...indexer, name, type, url, categories: parseCats(categories), includeParentCategories, priority: parsePriority(priority), seedRatio, seedTimeMinutes, inactiveSeedTimeMinutes, freeleechOnly, dailyQueryLimit, downloadClientId } delete payload.apiKey if (apiKey) payload.apiKey = apiKey const updated = await api.updateIndexer(indexer.id, payload) @@ -615,7 +705,7 @@ function EditIndexerForm({ indexer, onClose, onSaved }: { indexer: Indexer; onCl
- { setType(e.target.value); setDownloadClientId(clearPinOnProtocolChange(downloadClientId, e.target.value, clients)) }} className="w-full bg-slate-200 dark:bg-zinc-800 border border-slate-300 dark:border-zinc-700 rounded px-3 py-2 text-sm"> @@ -646,6 +736,7 @@ function EditIndexerForm({ indexer, onClose, onSaved }: { indexer: Indexer; onCl + {testResult && }
@@ -656,7 +747,7 @@ function EditIndexerForm({ indexer, onClose, onSaved }: { indexer: Indexer; onCl ) } -function AddIndexerForm({ onClose, onAdded }: { onClose: () => void; onAdded: (idx: Indexer) => void }) { +function AddIndexerForm({ clients, onClose, onAdded }: { clients: DownloadClient[]; onClose: () => void; onAdded: (idx: Indexer) => void }) { const { t } = useTranslation() const [name, setName] = useState('') const [type, setType] = useState<'newznab' | 'torznab'>('newznab') @@ -670,12 +761,13 @@ function AddIndexerForm({ onClose, onAdded }: { onClose: () => void; onAdded: (i const [inactiveSeedTimeMinutes, setInactiveSeedTimeMinutes] = useState(null) const [freeleechOnly, setFreeleechOnly] = useState(false) const [dailyQueryLimit, setDailyQueryLimit] = useState(null) + const [downloadClientId, setDownloadClientId] = useState(null) const [testing, setTesting] = useState(false) const [testResult, setTestResult] = useState(null) const labelCls = 'block text-xs text-slate-600 dark:text-zinc-400 mb-1' const submit = async () => { - const idx = await api.addIndexer({ name, url, apiKey, type, categories: parseCats(categories), includeParentCategories, priority: parsePriority(priority), enabled: true, seedRatio, seedTimeMinutes, inactiveSeedTimeMinutes, freeleechOnly, dailyQueryLimit }) + const idx = await api.addIndexer({ name, url, apiKey, type, categories: parseCats(categories), includeParentCategories, priority: parsePriority(priority), enabled: true, seedRatio, seedTimeMinutes, inactiveSeedTimeMinutes, freeleechOnly, dailyQueryLimit, downloadClientId }) onAdded(idx) } @@ -701,7 +793,7 @@ function AddIndexerForm({ onClose, onAdded }: { onClose: () => void; onAdded: (i
- { setType(e.target.value as 'newznab' | 'torznab'); setDownloadClientId(clearPinOnProtocolChange(downloadClientId, e.target.value, clients)) }} className="w-full bg-slate-200 dark:bg-zinc-800 border border-slate-300 dark:border-zinc-700 rounded px-3 py-2 text-sm"> @@ -731,6 +823,7 @@ function AddIndexerForm({ onClose, onAdded }: { onClose: () => void; onAdded: (i + {testResult && }
From 9f78c32157008950b6a92c44161483327c7cb8f6 Mon Sep 17 00:00:00 2001 From: vavallee Date: Wed, 14 Oct 2026 22:00:58 -0300 Subject: [PATCH 4/8] fix(indexer): close the no fallback gaps in per-indexer download client pins (#3080) A grab or retry with no indexer id is now tied to a pinned indexer by the release URL's host, and refused when several indexers share the host and one is pinned. A scheduled search skips a pinned indexer whose client is down and still grabs from the others, records the failure once a day per book and indexer, and notifies with the indexer name only. Saving no longer trips over a pinned client that changed protocol unless the pin or type is being changed. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- internal/api/indexer_pin_protocol_test.go | 43 +++++ internal/api/indexer_pin_unattributed_test.go | 164 ++++++++++++++++++ internal/api/indexers.go | 11 +- internal/api/queue.go | 74 ++++++++ internal/db/indexer_download_client_test.go | 32 ---- internal/db/pinned_client_test.go | 160 +++++++++++++++++ internal/indexer/newznab/client.go | 17 ++ internal/scheduler/scheduler.go | 120 ++++++++++--- .../scheduler_indexer_pin_skip_test.go | 149 ++++++++++++++++ .../scheduler/scheduler_indexer_pin_test.go | 4 +- 10 files changed, 716 insertions(+), 58 deletions(-) create mode 100644 internal/api/indexer_pin_protocol_test.go create mode 100644 internal/api/indexer_pin_unattributed_test.go create mode 100644 internal/db/pinned_client_test.go create mode 100644 internal/scheduler/scheduler_indexer_pin_skip_test.go diff --git a/internal/api/indexer_pin_protocol_test.go b/internal/api/indexer_pin_protocol_test.go new file mode 100644 index 000000000..c563ee760 --- /dev/null +++ b/internal/api/indexer_pin_protocol_test.go @@ -0,0 +1,43 @@ +package api + +import ( + "net/http" + "strconv" + "testing" +) + +// TestIndexerPin_UnrelatedUpdateWithWrongProtocolPinStillSaves: a client whose +// type was changed to the other protocol after the pin must not block enabling, +// disabling or re-prioritising the indexer. Only a change to the pin or the +// indexer type is held to the protocol rule. Grabs still fail loudly. +func TestIndexerPin_UnrelatedUpdateWithWrongProtocolPinStillSaves(t *testing.T) { + f := newPinAPIFixture(t) + torrentID := strconv.FormatInt(f.torrent.ID, 10) + created := decodeIndexer(t, f.create(`{"name":"Priv","url":"http://192.168.1.50:9117/api","type":"torznab","downloadClientId":`+torrentID+`}`)) + f.torrent.Type = "sabnzbd" + if err := f.clients.Update(t.Context(), f.torrent); err != nil { + t.Fatal(err) + } + + for _, body := range []string{`{"enabled":false}`, `{"priority":5}`, `{"name":"Renamed","downloadClientId":` + torrentID + `}`} { + rec := f.update(created.ID, body) + if rec.Code != http.StatusOK { + t.Fatalf("update %s: %d %s", body, rec.Code, rec.Body.String()) + } + if got := decodeIndexer(t, rec); got.DownloadClientID == nil || *got.DownloadClientID != f.torrent.ID { + t.Errorf("update %s lost the pin: %v", body, got.DownloadClientID) + } + } + // Changing the indexer type is a change the rule applies to. + if rec := f.update(created.ID, `{"type":"newznab"}`); rec.Code != http.StatusOK { + t.Fatalf("type change to the client's own protocol: %d %s", rec.Code, rec.Body.String()) + } + if rec := f.update(created.ID, `{"type":"torznab"}`); rec.Code != http.StatusBadRequest { + t.Fatalf("type change away from the client's protocol: %d, want 400: %s", rec.Code, rec.Body.String()) + } + // A new pin to a wrong protocol client is a change too. + other := decodeIndexer(t, f.create(`{"name":"Other","url":"http://192.168.1.51:9117/api","type":"torznab"}`)) + if rec := f.update(other.ID, `{"downloadClientId":`+torrentID+`}`); rec.Code != http.StatusBadRequest { + t.Fatalf("new wrong protocol pin: %d, want 400: %s", rec.Code, rec.Body.String()) + } +} diff --git a/internal/api/indexer_pin_unattributed_test.go b/internal/api/indexer_pin_unattributed_test.go new file mode 100644 index 000000000..090db1a59 --- /dev/null +++ b/internal/api/indexer_pin_unattributed_test.go @@ -0,0 +1,164 @@ +package api + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/vavallee/bindery/internal/auth" + "github.com/vavallee/bindery/internal/models" +) + +// grabURL runs grab and writeGrabError the way doGrab does, for a release URL +// of the test's choosing and a caller that may omit the indexer id. +func (f *pinFixture) grabURL(guid, nzbURL string, indexerID *int64, nonAdmin bool) *httptest.ResponseRecorder { + r := asAdmin(httptest.NewRequest(http.MethodPost, "/api/v1/queue/grab", nil)) + if nonAdmin { + r = r.WithContext(auth.WithUserRole(auth.WithUserID(f.ctx, 7), "user")) + } + req := grabRequest{GUID: guid, Title: "Pin Author - Pinned Book epub", NZBURL: nzbURL, Size: 100, BookID: &f.book.ID, Protocol: "usenet", IndexerID: indexerID} + w := httptest.NewRecorder() + if _, err := f.h.grab(r.Context(), req); err != nil { + writeGrabError(w, r, err) + return w + } + w.WriteHeader(http.StatusAccepted) + return w +} + +func (f *pinFixture) addIndexer(t *testing.T, name, path string, pin *int64) *models.Indexer { + t.Helper() + idx := &models.Indexer{Name: name, Type: "newznab", URL: f.idx.URL[:strings.LastIndex(f.idx.URL, "/")] + path, APIKey: "secret", Enabled: true, DownloadClientID: pin} + if err := f.indexers.Create(f.ctx, idx); err != nil { + t.Fatalf("create indexer %s: %v", name, err) + } + return idx +} + +func (f *pinFixture) wantReached(t *testing.T, w *httptest.ResponseRecorder, pub, priv int) { + t.Helper() + if f.pub.count() != pub || f.priv.count() != priv { + t.Fatalf("public=%d private=%d, want %d and %d (status %d: %s)", f.pub.count(), f.priv.count(), pub, priv, w.Code, w.Body.String()) + } +} + +// (a) A pre signed URL with no indexer id: signing returns early and attributes +// nothing, so the pin used to be lost and the release went to the public client. +func TestGrab_PreSignedURLWithoutIndexerIDStillUsesPin(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + w := f.grabURL("g-presigned", f.relURL+"?apikey=secret", nil, false) + f.wantReached(t, w, 0, 1) +} + +// (b) Two indexers share the download host and key. Signing works but nothing +// is attributed. With one of them pinned the grab cannot be tied to a single +// indexer, so it fails rather than guessing, and says why. +func TestGrab_AmbiguousHostWithAPinnedIndexerFailsWithoutFallback(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + f.addIndexer(t, "TrackerY", "/api2", nil) + + w := f.grabURL("g-ambiguous", f.relURL, nil, false) + if w.Code != http.StatusBadRequest { + t.Fatalf("status %d, want 400: %s", w.Code, w.Body.String()) + } + var body map[string]string + _ = json.Unmarshal(w.Body.Bytes(), &body) + if body["code"] != codeIndexerClientUnavailable { + t.Errorf("code = %q, want %q", body["code"], codeIndexerClientUnavailable) + } + for _, want := range []string{"could not be tied to one indexer", "TrackerX", "pinned", "was not sent"} { + if !strings.Contains(body["error"], want) { + t.Errorf("admin message %q does not mention %q", body["error"], want) + } + } + f.wantReached(t, w, 0, 0) + + // A non admin is told the release is held back, with no indexer or client names. + w = f.grabURL("g-ambiguous2", f.relURL, nil, true) + if w.Code != http.StatusBadRequest { + t.Fatalf("non admin: status %d, want 400: %s", w.Code, w.Body.String()) + } + if strings.Contains(w.Body.String(), "TrackerX") || strings.Contains(w.Body.String(), "sab-private") { + t.Errorf("non admin message leaks names: %s", w.Body.String()) + } + f.wantReached(t, w, 0, 0) +} + +// (c) A pinned indexer with no stored API key cannot sign, so signing +// attributed nothing. Its host still identifies it. +func TestGrab_PinnedIndexerWithoutAPIKeyAndNoIDStillUsesPin(t *testing.T) { + f := newPinFixture(t) + f.idx.APIKey = "" + f.pin(t, &f.privC.ID) + w := f.grabURL("g-nokey", f.relURL, nil, false) + f.wantReached(t, w, 0, 1) +} + +// An ambiguous host where no matching indexer is pinned is untouched: the grab +// proceeds exactly as before. +func TestGrab_AmbiguousHostWithNoPinnedIndexerProceeds(t *testing.T) { + f := newPinFixture(t) + f.addIndexer(t, "TrackerY", "/api2", nil) + w := f.grabURL("g-ambiguous-unpinned", f.relURL, nil, false) + f.wantReached(t, w, 1, 0) +} + +// A pinned indexer exists, but on a different host from the release: nothing ties +// the release to it. This is the documented limit (case d), the grab is +// unpinned, and the advice is to send indexerId. +func TestGrab_ReleaseOnAnUnrelatedHostIsUnpinned(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + w := f.grabURL("g-elsewhere", f.pub.srv.URL+"/other.nzb", nil, false) + f.wantReached(t, w, 1, 0) +} + +// A release from an unpinned indexer on its own host, with no id, is unaffected. +func TestGrab_UnpinnedIndexerOnItsHostWithoutIDProceeds(t *testing.T) { + f := newPinFixture(t) + w := f.grabURL("g-plain", f.relURL, nil, false) + f.wantReached(t, w, 1, 0) +} + +// Retry of a row whose indexer was deleted (indexer_id is NULL by FK SET NULL) +// finds a recreated pinned indexer again by the release URL's host. +func TestRetry_RowWithNoIndexerFindsPinnedIndexerByHost(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + dl := &models.Download{ + GUID: "g-orphan", Title: "Pin Author - Pinned Book epub", NZBURL: f.relURL + "?apikey=secret", Size: 100, + Status: models.StateFailed, Protocol: "usenet", Quality: "epub", BookID: &f.book.ID, + } + if err := f.downl.Create(f.ctx, dl); err != nil { + t.Fatal(err) + } + w := httptest.NewRecorder() + f.h.RetryDownload(w, asAdmin(retryDownloadRequest(t, dl.ID))) + if w.Code != http.StatusAccepted { + t.Fatalf("retry: status %d: %s", w.Code, w.Body.String()) + } + f.wantReached(t, w, 0, 1) +} + +// A retry that still cannot be tied to any indexer goes the way it always did. +func TestRetry_RowWithNoIndexerOnUnknownHostIsUnpinned(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + dl := &models.Download{ + GUID: "g-orphan2", Title: "Pin Author - Pinned Book epub", NZBURL: f.pub.srv.URL + "/gone.nzb", Size: 100, + Status: models.StateFailed, Protocol: "usenet", Quality: "epub", BookID: &f.book.ID, + } + if err := f.downl.Create(f.ctx, dl); err != nil { + t.Fatal(err) + } + w := httptest.NewRecorder() + f.h.RetryDownload(w, asAdmin(retryDownloadRequest(t, dl.ID))) + if w.Code != http.StatusAccepted { + t.Fatalf("retry: status %d: %s", w.Code, w.Body.String()) + } + f.wantReached(t, w, 1, 0) +} diff --git a/internal/api/indexers.go b/internal/api/indexers.go index 9a4451ee0..b7f2fd7ed 100644 --- a/internal/api/indexers.go +++ b/internal/api/indexers.go @@ -218,12 +218,17 @@ func (h *IndexerHandler) validateDownloadClientPin(ctx context.Context, idx mode if c == nil { return errPinInvalid(fmt.Sprintf("downloadClientId %d: that download client does not exist", *idx.DownloadClientID)) } - if got := db.ClientProtocol(c.Type); got != idx.Protocol() { + // A wrong protocol is refused only when this request sets the pin or the + // indexer type. Otherwise an unrelated update, such as disabling the + // indexer, would be blocked by a client changed behind the pin's back, and + // the grab fails loudly at grab time anyway. + pinChanged := stored == nil || stored.DownloadClientID == nil || *stored.DownloadClientID != *idx.DownloadClientID + typeChanged := stored == nil || stored.Type != idx.Type + if got := db.ClientProtocol(c.Type); got != idx.Protocol() && (pinChanged || typeChanged) { return errPinInvalid(fmt.Sprintf("download client %q is a %s client but this indexer is %s, so it cannot be pinned to it; pick a %s client", c.Name, got, idx.Protocol(), idx.Protocol())) } - changed := stored == nil || stored.DownloadClientID == nil || *stored.DownloadClientID != *idx.DownloadClientID - if changed && !c.Enabled { + if pinChanged && !c.Enabled { return errPinInvalid(fmt.Sprintf("download client %q is disabled; enable it first or pick another", c.Name)) } return nil diff --git a/internal/api/queue.go b/internal/api/queue.go index 7d6a9b3d0..0e2c0af8c 100644 --- a/internal/api/queue.go +++ b/internal/api/queue.go @@ -114,6 +114,24 @@ func isAdminCtx(ctx context.Context) bool { // nudge, which would be wrong when clients exist and the pin is the problem. const codeIndexerClientUnavailable = "indexer_client_unavailable" +// ambiguousPinError refuses a grab that arrived without an indexer id and that +// could not be tied to one indexer, while an indexer on the release's host is +// pinned to a download client (#3080). Sending it to the default client could +// put a private tracker's release on the public client, so it is not sent. +type ambiguousPinError struct{ pinned []string } + +func (e *ambiguousPinError) Error() string { return e.Message(true) } + +// Message names the pinned indexers only for an admin. +func (e *ambiguousPinError) Message(includeNames bool) string { + names := "" + if includeNames && len(e.pinned) > 0 { + names = " (" + strings.Join(e.pinned, ", ") + ")" + } + return "this release could not be tied to one indexer, and an indexer on its host is pinned to a download client" + names + + ". It was not sent to any client. Grab it from the search results so the indexer is known." +} + // pinnedMessage renders a pin failure for the caller's role. func pinnedMessage(ctx context.Context, perr *db.PinnedClientError) string { return perr.Message(isAdminCtx(ctx)) @@ -125,7 +143,11 @@ func pinnedMessage(ctx context.Context, perr *db.PinnedClientError) string { // the client; anything else is a storage fault and a generic 500. func writeGrabError(w http.ResponseWriter, r *http.Request, err error) { var perr *db.PinnedClientError + var aerr *ambiguousPinError switch { + case errors.As(err, &aerr): + slog.Warn("grab: the release could not be tied to one indexer and a matching indexer is pinned", "method", r.Method, "path", r.URL.Path, "error", aerr.Error()) + writeJSON(w, http.StatusBadRequest, map[string]string{"error": aerr.Message(isAdminCtx(r.Context())), "code": codeIndexerClientUnavailable}) case errors.As(err, &perr): slog.Warn("grab: the indexer's pinned download client cannot take the release", "method", r.Method, "path", r.URL.Path, "error", perr.Error()) writeJSON(w, http.StatusBadRequest, map[string]string{"error": pinnedMessage(r.Context(), perr), "code": codeIndexerClientUnavailable}) @@ -150,7 +172,10 @@ const codeNoDownloadClient = "no_download_client" // replaced. func queueItemMessage(ctx context.Context, err error) string { var perr *db.PinnedClientError + var aerr *ambiguousPinError switch { + case errors.As(err, &aerr): + return aerr.Message(isAdminCtx(ctx)) case errors.As(err, &perr): return pinnedMessage(ctx, perr) case errors.Is(err, errNoDownloadClient), errors.Is(err, errAlreadyGrabbed), @@ -1361,6 +1386,42 @@ func (h *QueueHandler) selectClient(ctx context.Context, protocol, mediaType str return db.PickClientForMediaType(candidates, mediaType), nil } +// pinnedIndexerOnHost looks for an indexer pinned to a download client among +// those on the release URL's host, for a grab that carries no indexer id. None +// pinned means the grab is unaffected (nil, nil). Exactly one indexer on the +// host, pinned, is the release's indexer and is returned so its pin applies. +// Several on the host with at least one pinned cannot be told apart, and the +// grab is refused with an *ambiguousPinError rather than guessed. A host that +// matches no indexer carries nothing to act on and is left unpinned: send the +// indexer id to have the pin apply. +func (h *QueueHandler) pinnedIndexerOnHost(ctx context.Context, rawURL string) (*models.Indexer, error) { + if h.indexers == nil || rawURL == "" { + return nil, nil + } + idxs, err := h.indexers.List(ctx) + if err != nil { + return nil, err + } + var matching []models.Indexer + var pinned []string + for _, idx := range idxs { + if !newznab.DownloadHostMatchesIndexer(rawURL, idx.URL) { + continue + } + matching = append(matching, idx) + if idx.DownloadClientID != nil { + pinned = append(pinned, idx.Name) + } + } + if len(pinned) == 0 { + return nil, nil + } + if len(matching) == 1 { + return &matching[0], nil + } + return nil, &ambiguousPinError{pinned: pinned} +} + // clientForGrab picks the client for a grab. An indexer pinned to a download // client (#3080) gets exactly that client, whatever the protocol routing, media // type or priority would have chosen, or a *db.PinnedClientError when it cannot @@ -1566,6 +1627,19 @@ func (h *QueueHandler) grab(ctx context.Context, req grabRequest) (*models.Downl if hostMatched != nil { indexerID = hostMatched } + // A grab still unattributed here may nonetheless come from a pinned + // indexer: signing cannot attribute a pre signed URL, a keyless indexer or + // two indexers sharing a host (#3080). Look at every indexer on the + // release's host before the default routing is allowed to pick a client. + if indexerID == nil { + onHost, err := h.pinnedIndexerOnHost(ctx, req.NZBURL) + if err != nil { + return nil, err + } + if onHost != nil { + indexerID = &onHost.ID + } + } // The client is chosen only now, after the host match above has settled // which indexer the release came from: an indexer pinned to a download diff --git a/internal/db/indexer_download_client_test.go b/internal/db/indexer_download_client_test.go index 2066763ab..d09351ea1 100644 --- a/internal/db/indexer_download_client_test.go +++ b/internal/db/indexer_download_client_test.go @@ -8,38 +8,6 @@ import ( "github.com/vavallee/bindery/internal/models" ) -// TestMigrate109_ExistingIndexersStayUnpinned: the column is added to a table -// that already has rows, and every one of them reads back with no pin. -func TestMigrate109_ExistingIndexersStayUnpinned(t *testing.T) { - database, err := OpenMemory() - if err != nil { - t.Fatal(err) - } - defer database.Close() - ctx := context.Background() - - // Roll the column back out, insert a row the way a pre 109 install had it, - // then add the column again exactly as the migration does. - if _, err := database.ExecContext(ctx, `ALTER TABLE indexers DROP COLUMN download_client_id`); err != nil { - t.Fatalf("drop column: %v", err) - } - if _, err := database.ExecContext(ctx, `INSERT INTO indexers (name, type, url, api_key, categories, priority, enabled, supports_search, created_at, updated_at) - VALUES ('old', 'newznab', 'http://x', 'k', '[7020]', 25, 1, 1, datetime('now'), datetime('now'))`); err != nil { - t.Fatalf("insert old row: %v", err) - } - if _, err := database.ExecContext(ctx, `ALTER TABLE indexers ADD COLUMN download_client_id INTEGER REFERENCES download_clients(id) ON DELETE RESTRICT`); err != nil { - t.Fatalf("re-add column: %v", err) - } - - got, err := NewIndexerRepo(database).List(ctx) - if err != nil || len(got) != 1 { - t.Fatalf("list: %v (%d rows)", err, len(got)) - } - if got[0].DownloadClientID != nil { - t.Errorf("an existing indexer came back pinned to %d", *got[0].DownloadClientID) - } -} - // TestIndexerRepo_PinRoundTripAndForeignKey: the pin persists through // Create, Update, GetByID and List, and the database itself refuses to delete // a client an indexer is pinned to. diff --git a/internal/db/pinned_client_test.go b/internal/db/pinned_client_test.go new file mode 100644 index 000000000..94800ebb3 --- /dev/null +++ b/internal/db/pinned_client_test.go @@ -0,0 +1,160 @@ +package db + +import ( + "context" + "database/sql" + "errors" + "sort" + "strings" + "testing" + + "github.com/vavallee/bindery/internal/models" +) + +// TestResolvePinnedClient_ProtocolOperands isolates the two protocol checks. The +// client against the indexer is reachable on its own when the release protocol +// is empty, and the client against the release on its own when the indexer +// agrees with the client. +func TestResolvePinnedClient_ProtocolOperands(t *testing.T) { + database, err := OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + ctx := context.Background() + clients := NewDownloadClientRepo(database) + usenet := &models.DownloadClient{Name: "sab", Type: "sabnzbd", Host: "h", Port: 1, Enabled: true} + if err := clients.Create(ctx, usenet); err != nil { + t.Fatal(err) + } + + cases := []struct { + name string + indexerType string + release string + wantReason string + }{ + {"client against indexer, release protocol empty", "torznab", "", PinClientWrongProtocol}, + {"client against release, indexer agrees with client", "newznab", "torrent", PinClientWrongProtocol}, + {"everything agrees", "newznab", "usenet", ""}, + {"everything agrees, release protocol empty", "newznab", "", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + idx := &models.Indexer{Name: "ix", Type: tc.indexerType, DownloadClientID: &usenet.ID} + c, err := clients.ResolvePinnedClient(ctx, idx, tc.release) + if tc.wantReason == "" { + if err != nil || c == nil || c.ID != usenet.ID { + t.Fatalf("got %v, %v; want the pinned client", c, err) + } + return + } + var perr *PinnedClientError + if !errors.As(err, &perr) || perr.Reason != tc.wantReason || c != nil { + t.Fatalf("got %v, %v; want a %q PinnedClientError", c, err, tc.wantReason) + } + }) + } +} + +// TestMigrate109_RealRunnerOnADatabaseAtVersion108 builds a database with +// migrations 1 to 108 applied by the real per migration routine, puts an +// indexer row and a download client row in it the way a pre 109 install had +// them, then lets the real runner apply the one pending migration. Existing +// rows must come back unpinned, and the foreign key must be enforced. +func TestMigrate109_RealRunnerOnADatabaseAtVersion108(t *testing.T) { + database, err := sql.Open("sqlite", ":memory:") + if err != nil { + t.Fatal(err) + } + defer database.Close() + database.SetMaxOpenConns(1) + if err := setPragmas(database); err != nil { + t.Fatal(err) + } + + entries, err := migrationsFS.ReadDir("migrations") + if err != nil { + t.Fatal(err) + } + names := make([]string, 0, len(entries)) + for _, e := range entries { + names = append(names, e.Name()) + } + sort.Strings(names) + if _, err := database.Exec(`CREATE TABLE IF NOT EXISTS schema_migrations ( + version INTEGER PRIMARY KEY, applied_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP)`); err != nil { + t.Fatal(err) + } + applied := 0 + for _, name := range names { + v, err := migrationVersion(name) + if err != nil { + t.Fatal(err) + } + if v >= 109 { + continue + } + if name == "025_multiuser.sql" { + if err := multiuserPreFlight(database); err != nil { + t.Fatal(err) + } + } + content, err := migrationsFS.ReadFile("migrations/" + name) + if err != nil { + t.Fatal(err) + } + if err := applyMigration(database, v, name, string(content)); err != nil { + t.Fatalf("apply %s: %v", name, err) + } + applied++ + } + if applied == 0 { + t.Fatal("no migrations applied") + } + var hasCol int + if err := database.QueryRow(`SELECT COUNT(*) FROM pragma_table_info('indexers') WHERE name='download_client_id'`).Scan(&hasCol); err != nil || hasCol != 0 { + t.Fatalf("the column exists before migration 109 (count %d, err %v)", hasCol, err) + } + + if _, err := database.Exec(`INSERT INTO download_clients (name, type, host, port, enabled, created_at, updated_at) + VALUES ('sab', 'sabnzbd', 'h', 1, 1, datetime('now'), datetime('now'))`); err != nil { + t.Fatalf("insert client: %v", err) + } + if _, err := database.Exec(`INSERT INTO indexers (name, type, url, api_key, categories, priority, enabled, supports_search, created_at, updated_at) + VALUES ('old', 'newznab', 'http://x', 'k', '[7020]', 25, 1, 1, datetime('now'), datetime('now'))`); err != nil { + t.Fatalf("insert indexer: %v", err) + } + + if err := migrate(database); err != nil { + t.Fatalf("migrate: %v", err) + } + var v109 int + if err := database.QueryRow(`SELECT COUNT(*) FROM schema_migrations WHERE version=109`).Scan(&v109); err != nil || v109 != 1 { + t.Fatalf("migration 109 not recorded (count %d, err %v)", v109, err) + } + if _, err := database.Exec(`PRAGMA foreign_keys=ON`); err != nil { + t.Fatal(err) + } + + ctx := context.Background() + got, err := NewIndexerRepo(database).List(ctx) + if err != nil || len(got) != 1 { + t.Fatalf("list: %v (%d rows)", err, len(got)) + } + if got[0].Name != "old" || got[0].Priority != 25 || got[0].DownloadClientID != nil { + t.Errorf("existing indexer changed or came back pinned: %+v", got[0]) + } + + // The foreign key is enforced: no pin to a client that does not exist, and + // no deleting a client that is pinned. + if _, err := database.Exec(`UPDATE indexers SET download_client_id=9999 WHERE name='old'`); err == nil || !strings.Contains(err.Error(), "FOREIGN KEY") { + t.Errorf("a pin to a missing client was accepted: %v", err) + } + if _, err := database.Exec(`UPDATE indexers SET download_client_id=(SELECT id FROM download_clients LIMIT 1) WHERE name='old'`); err != nil { + t.Fatalf("pin to a real client: %v", err) + } + if _, err := database.Exec(`DELETE FROM download_clients`); err == nil || !strings.Contains(err.Error(), "FOREIGN KEY") { + t.Errorf("a pinned client was deleted by raw SQL: %v", err) + } +} diff --git a/internal/indexer/newznab/client.go b/internal/indexer/newznab/client.go index 8ba3e2a44..b2eb0059c 100644 --- a/internal/indexer/newznab/client.go +++ b/internal/indexer/newznab/client.go @@ -193,6 +193,23 @@ func SignDownloadURLFor(rawURL, indexerURL, apiKey string) string { return signDownloadURL(rawURL, baseHost, apiKey) } +// DownloadHostMatchesIndexer reports whether a download URL is on the host the +// indexer is configured at, using the same normalisation SignDownloadURLFor +// uses. Unlike signing it needs no stored key and ignores whether the URL is +// already signed, so it answers "could this release have come from that +// indexer" for the cases signing cannot (#3080). +func DownloadHostMatchesIndexer(rawURL, indexerURL string) bool { + base, err := url.Parse(normalizeEndpointURL(indexerURL)) + if err != nil || base.Host == "" { + return false + } + u, err := url.Parse(rawURL) + if err != nil || u.Host == "" { + return false + } + return strings.EqualFold(u.Host, strings.ToLower(base.Host)) +} + // HasAPIKey reports whether a download URL already carries an apikey. // // It exists so callers can tell "this URL is already signed" apart from "this diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index aabb94d4b..a27e61688 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -115,7 +115,8 @@ type eventNotifier interface { // here to avoid having the scheduler import notifier just for two string // constants (and to keep the eventNotifier interface untied to the real type). const ( - notifierEventGrabbed = "grabbed" + notifierEventGrabbed = "grabbed" + notifierEventDownloadFailed = "downloadFailed" ) // defaultSearchInterval is the fallback wanted-book search cadence used when @@ -1017,20 +1018,73 @@ func (s *Scheduler) pinnedClient(ctx context.Context, res *newznab.SearchResult) return s.clients.ResolvePinnedClient(ctx, idx, res.Protocol) } -// recordPinnedFailure writes the refused grab to History, so it is not only a -// log line. History is readable by every user, so it names the indexer and not -// the client; the log line carries both. +// checkPin decides whether an approved release may go ahead. It returns the +// pinned client for a pinned indexer, a nil client for an unpinned one, and +// skip=true when the indexer is pinned to a client that cannot take it. The +// first failure for an indexer is logged and recorded; later releases from the +// same indexer in this sweep are skipped quietly. +func (s *Scheduler) checkPin(ctx context.Context, book models.Book, res *newznab.SearchResult, skipped map[int64]bool) (*models.DownloadClient, bool, error) { + if skipped[res.IndexerID] { + return nil, true, nil + } + c, err := s.pinnedClient(ctx, res) + if err == nil { + return c, false, nil + } + var perr *db.PinnedClientError + if !errors.As(err, &perr) { + return nil, false, err + } + slog.Warn("SearchAndGrabBook: the indexer's pinned download client cannot take the grab, skipping that indexer", + "book", book.Title, "release", res.Title, "error", perr.Error()) + skipped[res.IndexerID] = true + s.recordPinnedFailure(ctx, book.ID, res, perr) + return nil, true, nil +} + +// pinnedFailureCode marks a History event written for a pinned client failure. +const pinnedFailureCode = "indexer_client_unavailable" + +// pinnedFailureQuietPeriod is how long an identical failure for the same book +// and indexer stays recorded only once. The wanted sweep can run hourly, and a +// down client would otherwise write one row per book per sweep. +const pinnedFailureQuietPeriod = 24 * time.Hour + +// recordPinnedFailure writes the refused grab to History and the notifier, so +// it is not only a log line. History is readable by every user and a webhook +// leaves the house, so both name the indexer and not the client; the log line +// carries both. An identical failure for the same book and indexer in the last +// day is not recorded again. func (s *Scheduler) recordPinnedFailure(ctx context.Context, bookID int64, res *newznab.SearchResult, perr *db.PinnedClientError) { if s.history == nil { return } - data, _ := json.Marshal(map[string]any{"guid": res.GUID, "message": perr.Message(false)}) + message := perr.Message(false) + if events, err := s.history.ListByBook(ctx, bookID); err == nil { + cutoff := time.Now().UTC().Add(-pinnedFailureQuietPeriod) + for _, e := range events { + if e.EventType != models.HistoryEventDownloadFailed || e.CreatedAt.Before(cutoff) { + continue + } + var prior struct { + Code string `json:"code"` + IndexerID int64 `json:"indexerId"` + Message string `json:"message"` + } + if json.Unmarshal([]byte(e.Data), &prior) == nil && prior.Code == pinnedFailureCode && + prior.IndexerID == res.IndexerID && prior.Message == message { + return + } + } + } + data, _ := json.Marshal(map[string]any{"guid": res.GUID, "message": message, "code": pinnedFailureCode, "indexerId": res.IndexerID}) _ = s.history.Create(ctx, &models.HistoryEvent{ BookID: &bookID, EventType: models.HistoryEventDownloadFailed, SourceTitle: res.Title, Data: string(data), }) + s.notify(ctx, notifierEventDownloadFailed, map[string]interface{}{"title": res.Title, "message": message}) } // resolveSeedLimits looks up the per-indexer seed ratio (#883) and seed time @@ -1245,9 +1299,27 @@ func (s *Scheduler) searchAndGrabFormat(ctx context.Context, book models.Book, m } var best *newznab.SearchResult + // bestClient is the client the best release's indexer is pinned to + // (#3080), nil for an unpinned indexer. skippedPinned holds the indexers + // whose pinned client could not take a release this sweep: their other + // releases would fail the same way, so they are passed over without a second + // record, and the next approved release from a different indexer is tried. + // A pinned indexer's release is never sent to another client. + var bestClient *models.DownloadClient + skippedPinned := map[int64]bool{} for i, d := range dm.Evaluate(releases, book) { if d.Approved { + c, skip, err := s.checkPin(ctx, book, &results[i], skippedPinned) + if err != nil { + slog.Error("SearchAndGrabBook: failed to resolve the indexer's pinned download client", "indexer", results[i].IndexerName, "error", err) + outcome = "download client lookup failed" + return + } + if skip { + continue + } best = &results[i] + bestClient = c break } if dbg != nil { @@ -1285,10 +1357,26 @@ func (s *Scheduler) searchAndGrabFormat(ctx context.Context, book models.Book, m if best == nil { // Re-evaluate any existing pending releases for this book/format with the current age. best = s.checkPendingReleases(ctx, book, mediaType, dm) + if best != nil { + c, skip, err := s.checkPin(ctx, book, best, skippedPinned) + if err != nil { + slog.Error("SearchAndGrabBook: failed to resolve the indexer's pinned download client", "indexer", best.IndexerName, "error", err) + outcome = "download client lookup failed" + return + } + if skip { + best = nil + } else { + bestClient = c + } + } if best == nil { - if rawResults == 0 { + switch { + case len(skippedPinned) > 0: + outcome = "pinned download client unavailable" + case rawResults == 0: outcome = "no results" - } else { + default: outcome = "nothing approved" } return @@ -1297,21 +1385,9 @@ func (s *Scheduler) searchAndGrabFormat(ctx context.Context, book models.Book, m approved = 1 // An indexer pinned to a download client (#3080) sends every grab to that - // client or fails. There is deliberately no fallback to the default pick. - client, pinErr := s.pinnedClient(ctx, best) - if pinErr != nil { - var perr *db.PinnedClientError - if errors.As(pinErr, &perr) { - slog.Warn("SearchAndGrabBook: the indexer's pinned download client cannot take the grab", - "book", book.Title, "release", best.Title, "error", perr.Error()) - s.recordPinnedFailure(ctx, book.ID, best, perr) - outcome = "pinned download client unavailable" - return - } - slog.Error("SearchAndGrabBook: failed to resolve the indexer's pinned download client", "indexer", best.IndexerName, "error", pinErr) - outcome = "download client lookup failed" - return - } + // client or, above, was skipped. There is deliberately no fallback to the + // default pick for its releases. + client := bestClient if client == nil { candidates, err := s.clients.GetEnabledByProtocol(ctx, best.Protocol) if err != nil { diff --git a/internal/scheduler/scheduler_indexer_pin_skip_test.go b/internal/scheduler/scheduler_indexer_pin_skip_test.go new file mode 100644 index 000000000..d828b15d7 --- /dev/null +++ b/internal/scheduler/scheduler_indexer_pin_skip_test.go @@ -0,0 +1,149 @@ +package scheduler + +import ( + "context" + "encoding/json" + "strings" + "sync" + "testing" + "time" + + "github.com/vavallee/bindery/internal/indexer/newznab" + "github.com/vavallee/bindery/internal/models" +) + +type recordingNotifier struct { + mu sync.Mutex + events []map[string]interface{} + types []string +} + +func (n *recordingNotifier) Send(_ context.Context, eventType string, payload map[string]interface{}) { + n.mu.Lock() + defer n.mu.Unlock() + n.types = append(n.types, eventType) + n.events = append(n.events, payload) +} + +// pinnedFirstFixture has a pinned indexer ranked first whose client is down +// (disabled), and an unpinned indexer ranked second with its own release. +func pinnedFirstFixture(t *testing.T) (*schedPinFixture, *models.Indexer) { + t.Helper() + f := newSchedPinFixture(t) + f.pin(t, f.privC.ID) + f.privC.Enabled = false + if err := f.clients.Update(f.ctx, f.privC); err != nil { + t.Fatal(err) + } + open := &models.Indexer{Name: "OpenIdx", Type: "newznab", URL: "http://open.invalid", Enabled: true} + if err := f.indexers.Create(f.ctx, open); err != nil { + t.Fatal(err) + } + pinnedRes := f.s.searcher.(*perFormatSearcher).byFormat[models.MediaTypeEbook][0] + pinnedRes2 := pinnedRes + pinnedRes2.GUID = "g-pin-2" + pinnedRes2.Title = "Pin Author Pinned Book epub v2" + openRes := pinnedRes + openRes.GUID = "g-open" + openRes.Title = "Pin Author Pinned Book epub open" + openRes.IndexerID = open.ID + openRes.IndexerName = "OpenIdx" + f.s.searcher.(*perFormatSearcher).byFormat[models.MediaTypeEbook] = []newznab.SearchResult{pinnedRes, pinnedRes2, openRes} + return f, open +} + +func (f *schedPinFixture) pinnedFailures(t *testing.T) []models.HistoryEvent { + t.Helper() + events, err := f.history.ListByBook(f.ctx, f.book.ID) + if err != nil { + t.Fatal(err) + } + var out []models.HistoryEvent + for _, e := range events { + if e.EventType == models.HistoryEventDownloadFailed && strings.Contains(e.Data, "indexer_client_unavailable") { + out = append(out, e) + } + } + return out +} + +// TestAutoGrab_FailingPinnedReleaseDoesNotStarveUnpinnedIndexers: a pinned +// release whose client is down is skipped, the next approved release from a +// different indexer is grabbed, and nothing for the pinned indexer reaches +// another client. +func TestAutoGrab_FailingPinnedReleaseDoesNotStarveUnpinnedIndexers(t *testing.T) { + f, _ := pinnedFirstFixture(t) + f.s.SearchAndGrabBook(f.ctx, f.book) + + if f.priv.count() != 0 { + t.Fatalf("the disabled pinned client received a grab") + } + if f.pub.count() != 1 { + t.Fatalf("public client grabs = %d, want exactly the unpinned indexer's release", f.pub.count()) + } + if dl, _ := f.downloads.GetByGUID(f.ctx, "g-open"); dl == nil { + t.Errorf("the unpinned indexer's release was not grabbed") + } + for _, guid := range []string{"g-pin", "g-pin-2"} { + if dl, _ := f.downloads.GetByGUID(f.ctx, guid); dl != nil { + t.Errorf("the pinned indexer's release %s was sent somewhere: %+v", guid, dl) + } + } + // Recorded once for the book and indexer, though two of its releases were skipped. + if n := len(f.pinnedFailures(t)); n != 1 { + t.Errorf("pinned failure events = %d, want 1", n) + } +} + +// TestAutoGrab_PinnedFailureIsRecordedOncePerDay: the next sweep skips the +// same release again, and neither History nor the notifier hears about it twice. +func TestAutoGrab_PinnedFailureIsRecordedOncePerDay(t *testing.T) { + f, _ := pinnedFirstFixture(t) + n := &recordingNotifier{} + f.s.notif = n + f.s.SearchAndGrabBook(f.ctx, f.book) + // Free the second sweep from the live row of the first sweep's grab. + if dl, _ := f.downloads.GetByGUID(f.ctx, "g-open"); dl != nil { + _ = f.downloads.Delete(f.ctx, dl.ID) + } + f.s.SearchAndGrabBook(f.ctx, f.book) + + if got := len(f.pinnedFailures(t)); got != 1 { + t.Fatalf("pinned failure events after two sweeps = %d, want 1", got) + } + failed := 0 + for i, typ := range n.types { + if typ != "downloadFailed" { + continue + } + failed++ + body, _ := json.Marshal(n.events[i]) + if !strings.Contains(string(body), "TrackerX") { + t.Errorf("notification does not name the indexer: %s", body) + } + if strings.Contains(string(body), "sab-private") { + t.Errorf("notification names the client: %s", body) + } + } + if failed != 1 { + t.Errorf("downloadFailed notifications = %d, want 1", failed) + } +} + +// TestAutoGrab_PinnedFailureIsRecordedAgainAfterADay: a failure older than 24 +// hours no longer suppresses a new one. +func TestAutoGrab_PinnedFailureIsRecordedAgainAfterADay(t *testing.T) { + f, _ := pinnedFirstFixture(t) + f.s.SearchAndGrabBook(f.ctx, f.book) + old := time.Now().UTC().Add(-25 * time.Hour) + if _, err := f.database.ExecContext(f.ctx, `UPDATE history SET created_at=? WHERE event_type='downloadFailed'`, old); err != nil { + t.Fatal(err) + } + if dl, _ := f.downloads.GetByGUID(f.ctx, "g-open"); dl != nil { + _ = f.downloads.Delete(f.ctx, dl.ID) + } + f.s.SearchAndGrabBook(f.ctx, f.book) + if got := len(f.pinnedFailures(t)); got != 2 { + t.Fatalf("pinned failure events = %d, want 2", got) + } +} diff --git a/internal/scheduler/scheduler_indexer_pin_test.go b/internal/scheduler/scheduler_indexer_pin_test.go index cd4f6ebc9..24d710358 100644 --- a/internal/scheduler/scheduler_indexer_pin_test.go +++ b/internal/scheduler/scheduler_indexer_pin_test.go @@ -2,6 +2,7 @@ package scheduler import ( "context" + "database/sql" "encoding/json" "net/http" "net/http/httptest" @@ -56,6 +57,7 @@ type schedPinFixture struct { indexers *db.IndexerRepo downloads *db.DownloadRepo history *db.HistoryRepo + database *sql.DB book models.Book pub, priv *pinSAB pubC *models.DownloadClient @@ -129,7 +131,7 @@ func newSchedPinFixture(t *testing.T) *schedPinFixture { profiles: db.NewMetadataProfileRepo(database), } return &schedPinFixture{ - s: s, ctx: ctx, clients: clients, indexers: indexers, downloads: downloads, history: history, + s: s, database: database, ctx: ctx, clients: clients, indexers: indexers, downloads: downloads, history: history, book: book, pub: pub, priv: priv, pubC: pubC, privC: privC, idx: idx, } } From 2e8581db33033ddf71bbef4960a66b2dc2769cdc Mon Sep 17 00:00:00 2001 From: vavallee Date: Wed, 14 Oct 2026 22:00:59 -0300 Subject: [PATCH 5/8] feat(web): keep wrong protocol pins visible and stay quiet when clients fail to load (#3080) Also corrects the API, troubleshooting and user guide text about bulk retry, retry rows and who sees the client name, and lists the known limits. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- .../3080-per-indexer-download-client.md | 2 +- docs/API.md | 27 ++++++++++++---- docs/Troubleshooting-Wiki.md | 23 +++++++++++-- docs/User-Guide-Wiki.md | 12 ++++--- web/src/i18n/locales/en.json | 2 ++ .../pages/settings/IndexersTab.pin.test.tsx | 19 +++++++++++ web/src/pages/settings/IndexersTab.tsx | 32 ++++++++++++------- 7 files changed, 93 insertions(+), 24 deletions(-) diff --git a/changelog.d/3080-per-indexer-download-client.md b/changelog.d/3080-per-indexer-download-client.md index 57cc4fd57..405391fb1 100644 --- a/changelog.d/3080-per-indexer-download-client.md +++ b/changelog.d/3080-per-indexer-download-client.md @@ -1,2 +1,2 @@ ### Added -- **Per-indexer download client** (#3080): each indexer now has an optional **Download client** setting in Settings → Indexers. Leave it on Default and nothing changes. Pick a client and every grab from that indexer goes to it: scheduled searches, the search page, held releases in Pending and Retry download. It overrides the client's priority and category routing, so a private tracker's torrents can live on their own qBittorrent. If that client is disabled, missing, the wrong protocol or not answering, the grab fails with a message naming the indexer and the client and is never sent to another client. A download client that an indexer is pinned to cannot be deleted (the request is refused with the indexer names), and the indexer row shows a warning while its client is disabled. Prowlarr syncs keep the setting. +- **Per-indexer download client** (#3080): each indexer now has an optional **Download client** setting in Settings → Indexers. Leave it on Default and nothing changes. Pick a client and every grab from that indexer goes to it: scheduled searches, the search page, held releases in Pending and Retry download. It overrides the client's priority and category routing, so a private tracker's torrents can live on their own qBittorrent. If that client is disabled, missing, the wrong protocol or not answering, the grab fails with a message naming the indexer (and the client, for an admin) and is never sent to another client. A scheduled search skips only that indexer's releases and still grabs from your others. A download client that an indexer is pinned to cannot be deleted (the request is refused with the indexer names), and the indexer row shows a warning while its client is disabled, missing or the wrong protocol. Prowlarr syncs keep the setting. diff --git a/docs/API.md b/docs/API.md index e2bbeab68..2dd2d6d3d 100644 --- a/docs/API.md +++ b/docs/API.md @@ -550,12 +550,27 @@ indexer already pinned to a client that was disabled later still saves. A pinned grab ignores the client's priority and media type routing. If the pinned client is disabled, missing or of the wrong protocol, the grab fails and no other -client is tried: `POST /queue/grab`, `POST /queue/{id}/retry`, the bulk retry and -`POST /pending/{id}/grab` answer `400` with `code: "indexer_client_unavailable"` -and a message naming the indexer (and the client, for admins). A client that is -enabled but does not answer is the usual `502`, with the message extended to say -the indexer is pinned and no other client was tried. The field is only returned by -the admin only indexer endpoints. +client is tried: `POST /queue/grab`, `POST /queue/{id}/retry` and +`POST /pending/{id}/grab` answer `400` with `code: "indexer_client_unavailable"`. +The message names the indexer, and the client only for an admin caller. The bulk +retry returns the same text per item in its results map instead of a status. A +client that is enabled but does not answer is the usual `502`, with the message +extended to say the indexer is pinned and no other client was tried. The field is +only returned by the admin only indexer endpoints. + +A grab that arrives without an `indexerId` is tied to an indexer by its download +URL's host. One indexer on that host, pinned, applies its pin. Several indexers on +the host with at least one pinned cannot be told apart, so the grab is refused +with the same `400` and code (nothing is sent), and the message says the release +could not be tied to one indexer. A host that matches no indexer carries nothing +to act on and is routed as if unpinned, so send `indexerId` to be sure the pin +applies. A retry of a Queue row whose indexer was deleted is attributed the same +way, by the stored release URL. + +On save, a client with the wrong protocol is refused only when the request sets +the pin or changes the indexer `type`. Enabling, disabling or reprioritising an +indexer whose pinned client was changed to the other protocol later still saves; +its grabs fail at grab time. `DELETE /downloadclient/{id}` answers `409` while any indexer is pinned to the client, with `error` and an `indexers` array of their names. Disabling the client diff --git a/docs/Troubleshooting-Wiki.md b/docs/Troubleshooting-Wiki.md index 12495f69a..8f5abaeb5 100644 --- a/docs/Troubleshooting-Wiki.md +++ b/docs/Troubleshooting-Wiki.md @@ -260,11 +260,30 @@ changed after the pin). If the client is enabled but the message says the release could not be sent, the client is not answering: check that it is running and reachable from Bindery. +The message names the indexer, and the client too when you are an admin; other +users see the indexer only. + Bindery will not send the release to a different client. That is deliberate: the pin exists so a private tracker's torrents never reach your public client. To send the indexer's grabs through the normal routing again, set its **Download -client** back to *Default*. Failed scheduled grabs appear in History, and the -Queue row of a failed grab offers **Retry download** once the cause is fixed. +client** back to *Default*. A grab refused before anything was sent creates no +Queue row: from the search page the error appears on the page, and a scheduled +search records it in History (once a day per book and indexer, and as a +notification naming the indexer). Just search or grab again once the cause is +fixed. Only a grab that reached an enabled client and failed there leaves a +failed Queue row with **Retry download**. + +Limits worth knowing: + +* A scheduled search skips only the failing indexer's releases. Releases from + other indexers for the same book are still grabbed, and nothing from the + pinned indexer goes to another client. +* A grab with no `indexerId` whose download URL is on a host that matches no + indexer (a proxied link on another host) cannot be tied to a pinned indexer + and is routed as if unpinned. Grabs from the Bindery UI always send the + indexer; API callers should send `indexerId`. +* Several indexers on one host with one of them pinned: a grab without + `indexerId` is refused, because it cannot be told which indexer it came from. ### The Queue looks empty, or shorter than it should be diff --git a/docs/User-Guide-Wiki.md b/docs/User-Guide-Wiki.md index d04a05bcc..a0936a4e4 100644 --- a/docs/User-Guide-Wiki.md +++ b/docs/User-Guide-Wiki.md @@ -439,10 +439,14 @@ pin to a client of the other protocol is refused. the pinned client even if another client has an audiobook category. * **There is no fallback.** If the pinned client is disabled, missing or of the wrong protocol, or does not answer, the grab fails with a message naming the - indexer and the client, and nothing is sent to any other client. In the - scheduler the failure shows in History and the log. The indexer's row in - Settings shows an amber warning while its pinned client is disabled. Enable - the client or pick another one and the next grab goes through. + indexer (and the client too, if you are an admin), and nothing is sent to any + other client. A scheduled search skips that indexer's releases, still grabs + from your other indexers, and records the failure in History once a day per + book and indexer. The indexer's row in Settings shows an amber warning while + its pinned client is disabled, missing or the wrong protocol. Enable the + client or pick another one and the next grab goes through. A grab with no + indexer id (an API caller) is tied to its indexer by the release URL's host; + see the troubleshooting page for the limits. * **A pinned client cannot be deleted.** Deleting a download client that an indexer is pinned to is refused (HTTP 409) with the indexer names. Choose another client for those indexers, or set them back to *Default*, first. diff --git a/web/src/i18n/locales/en.json b/web/src/i18n/locales/en.json index 73dcaf59b..0535ca289 100644 --- a/web/src/i18n/locales/en.json +++ b/web/src/i18n/locales/en.json @@ -1188,9 +1188,11 @@ "downloadClient": "Download client", "downloadClientDefault": "Default (automatic routing)", "downloadClientDisabledOption": "{{name}} (disabled)", + "downloadClientWrongProtocolOption": "{{name}} (wrong protocol)", "downloadClientHint": "Send every grab from this indexer to one download client, whatever its priority or category routing says. Leave on Default to let Bindery choose. If the chosen client is disabled, missing or unreachable the grab fails with an error instead of going to another client, so private tracker traffic never reaches your public client. Only enabled clients of this indexer's protocol are listed." }, "pinnedClientDisabled": "Pinned to download client {{name}}, which is disabled. Grabs from this indexer will fail, and will not go to another client, until you enable it or choose a different one.", + "pinnedClientWrongProtocol": "Pinned to download client {{name}}, which is the wrong protocol for this indexer. Grabs from this indexer will fail, and will not go to another client, until you choose a matching client.", "pinnedClientMissing": "Pinned to a download client that no longer exists. Grabs from this indexer will fail until you choose a different one.", "quotaUsage": "{{used}} of {{limit}} queries in the last 24 hours", "quotaReached": "Daily query limit of {{limit}} reached. This indexer is being skipped until the oldest hour of queries falls out of the window.", diff --git a/web/src/pages/settings/IndexersTab.pin.test.tsx b/web/src/pages/settings/IndexersTab.pin.test.tsx index bcd758cc0..34b935830 100644 --- a/web/src/pages/settings/IndexersTab.pin.test.tsx +++ b/web/src/pages/settings/IndexersTab.pin.test.tsx @@ -148,6 +148,25 @@ describe('IndexersTab download client pin', () => { expect(screen.queryByText('settings.indexers.pinnedClientMissing')).not.toBeInTheDocument() }) + it('keeps a wrong protocol pin visible and warns about it on the row', async () => { + // The usenet client sab (id 4) pinned to a torrent indexer. + render() + expect(await screen.findByText('settings.indexers.pinnedClientWrongProtocol name=sab')).toBeInTheDocument() + const select = await openEdit() + expect((select as HTMLSelectElement).value).toBe('4') + const names = within(select).getAllByRole('option').map(o => o.textContent) + expect(names).toContain('settings.indexers.form.downloadClientWrongProtocolOption name=sab') + }) + + it('shows no pin warning at all when the client list fails to load', async () => { + m.listDownloadClients.mockRejectedValue(new Error('boom')) + render() + await waitFor(() => expect(m.listDownloadClients).toHaveBeenCalled()) + // Let the rejection settle before asserting nothing appeared. + await new Promise(r => setTimeout(r, 20)) + expect(screen.queryByText(/pinnedClient/)).not.toBeInTheDocument() + }) + it('sends the pin when adding an indexer', async () => { m.addIndexer.mockImplementation(async p => ({ ...makeIndexer(), ...p, id: 9 }) as Indexer) render() diff --git a/web/src/pages/settings/IndexersTab.tsx b/web/src/pages/settings/IndexersTab.tsx index 01bf8f6ac..63ea8a21c 100644 --- a/web/src/pages/settings/IndexersTab.tsx +++ b/web/src/pages/settings/IndexersTab.tsx @@ -233,8 +233,9 @@ function indexerProtocol(indexerType: string): 'torrent' | 'usenet' { // DownloadClientField pins an indexer to one download client (#3080). It lists // only enabled clients of the indexer's protocol. A client the indexer is -// already pinned to stays listed, marked disabled, so the form does not show a -// blank that hides the real saved value. +// already pinned to stays listed even when it is disabled or of the other +// protocol, marked as such, so the form does not show a blank that hides the +// real saved value. function DownloadClientField({ value, onChange, indexerType, clients }: { value: number | null | undefined onChange: (v: number | null) => void @@ -243,7 +244,12 @@ function DownloadClientField({ value, onChange, indexerType, clients }: { }) { const { t } = useTranslation() const protocol = indexerProtocol(indexerType) - const options = clients.filter(c => clientProtocol(c.type) === protocol && (c.enabled || c.id === value)) + const options = clients.filter(c => c.id === value || (c.enabled && clientProtocol(c.type) === protocol)) + const optionLabel = (c: DownloadClient) => { + if (!c.enabled) return t('settings.indexers.form.downloadClientDisabledOption', { name: c.name }) + if (clientProtocol(c.type) !== protocol) return t('settings.indexers.form.downloadClientWrongProtocolOption', { name: c.name }) + return c.name + } return (
@@ -256,7 +262,7 @@ function DownloadClientField({ value, onChange, indexerType, clients }: { {options.map(c => ( ))} @@ -325,15 +331,16 @@ export default function IndexersTab({ indexers, setIndexers, prowlarrInstances, const [indexerTestResults, setIndexerTestResults] = useState>({}) const [confirmDeleteIndexer, setConfirmDeleteIndexer] = useState(null) const [prowlarrTestResult, setProwlarrTestResult] = useState>({}) - // null until the list has loaded, so a pin is not reported missing while the - // clients are still on their way. + // null until the list has loaded, and left null if loading fails, so a pin + // is never reported missing or disabled on the strength of a list that is not + // there. const [clients, setClients] = useState(null) useEffect(() => { let live = true Promise.resolve() .then(() => api.listDownloadClients()) .then(list => { if (live) setClients(list ?? []) }) - .catch(() => { if (live) setClients([]) }) + .catch(() => { /* leave clients null: no warning without a list */ }) return () => { live = false } }, []) @@ -411,7 +418,8 @@ export default function IndexersTab({ indexers, setIndexers, prowlarrInstances, {(() => { if (idx.downloadClientId == null || clients === null) return null const pinned = clients.find(c => c.id === idx.downloadClientId) - if (pinned?.enabled) return null + const wrongProtocol = pinned != null && clientProtocol(pinned.type) !== indexerProtocol(idx.type) + if (pinned?.enabled && !wrongProtocol) return null return (
- {pinned - ? t('settings.indexers.pinnedClientDisabled', { name: pinned.name }) - : t('settings.indexers.pinnedClientMissing')} + {!pinned + ? t('settings.indexers.pinnedClientMissing') + : !pinned.enabled + ? t('settings.indexers.pinnedClientDisabled', { name: pinned.name }) + : t('settings.indexers.pinnedClientWrongProtocol', { name: pinned.name })}
) From 0892191b5fa147220618d4cdbe177e9344af8410 Mon Sep 17 00:00:00 2001 From: vavallee Date: Wed, 14 Oct 2026 22:22:40 -0300 Subject: [PATCH 6/8] fix(indexer): keep held releases and check every host indexer for a pin (#3080) A pending release refused by its pinned client stays pending and the next approved row is tried. The host check now runs whenever the caller named no indexer, refuses a contradicting supplied id when a pinned indexer owns the host, and compares host names ignoring case, a trailing dot and the port. An empty type in an update is no longer read as a change of type. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- docs/API.md | 30 +++- docs/Troubleshooting-Wiki.md | 19 ++- internal/api/indexer_pin_host_test.go | 115 +++++++++++++ internal/api/indexer_pin_type_test.go | 28 ++++ internal/api/indexer_pin_unattributed_test.go | 4 +- internal/api/indexers.go | 12 +- internal/api/queue.go | 102 +++++++++--- internal/indexer/newznab/client.go | 32 +++- internal/indexer/newznab/pin_host_test.go | 39 +++++ internal/scheduler/scheduler.go | 52 ++++-- .../scheduler_indexer_pin_pending_test.go | 157 ++++++++++++++++++ 11 files changed, 524 insertions(+), 66 deletions(-) create mode 100644 internal/api/indexer_pin_host_test.go create mode 100644 internal/api/indexer_pin_type_test.go create mode 100644 internal/indexer/newznab/pin_host_test.go create mode 100644 internal/scheduler/scheduler_indexer_pin_pending_test.go diff --git a/docs/API.md b/docs/API.md index 2dd2d6d3d..0b8d11fa9 100644 --- a/docs/API.md +++ b/docs/API.md @@ -559,13 +559,29 @@ extended to say the indexer is pinned and no other client was tried. The field i only returned by the admin only indexer endpoints. A grab that arrives without an `indexerId` is tied to an indexer by its download -URL's host. One indexer on that host, pinned, applies its pin. Several indexers on -the host with at least one pinned cannot be told apart, so the grab is refused -with the same `400` and code (nothing is sent), and the message says the release -could not be tied to one indexer. A host that matches no indexer carries nothing -to act on and is routed as if unpinned, so send `indexerId` to be sure the pin -applies. A retry of a Queue row whose indexer was deleted is attributed the same -way, by the stored release URL. +URL's host, whether or not the URL is already signed. One indexer on that host, +pinned, applies its pin. When another indexer on the host is pinned and the grab +cannot be tied to just one indexer, it is refused with the same `400` and code +(nothing is sent) and the message says the release could not be tied to one +indexer. That includes a pinned indexer without a stored key sharing a host with +a keyed one. On a shared Prowlarr or Jackett host, one pinned indexer therefore +makes id-less API grabs fail for every indexer on that host; the web app and the +scheduler always send the indexer, and are unaffected. + +An `indexerId` you send is taken at face value when that indexer's host matches +the release URL's host, even if another indexer on the same host is pinned. An id +that contradicts the URL's host is refused when an indexer on the URL's host is +pinned (unless the id itself is pinned, in which case its own pin applies). + +The host comparison ignores case, one trailing dot and the port, in any +combination: `prowlarr` and `prowlarr:9696` are the same host. Ignoring a +differing port is deliberate, because a spurious refusal is safer than letting a +private release reach the public client. `www.` and other subdomains, scheme-less +indexer URLs and punycode against Unicode spellings are different hosts and are +not matched. A host that matches no indexer at all carries nothing to act on and +is routed as if unpinned, so send `indexerId` to be sure the pin applies. A retry +of a Queue row whose indexer was deleted is attributed the same way, by the stored +release URL. On save, a client with the wrong protocol is refused only when the request sets the pin or changes the indexer `type`. Enabling, disabling or reprioritising an diff --git a/docs/Troubleshooting-Wiki.md b/docs/Troubleshooting-Wiki.md index 8f5abaeb5..f2ede2fa8 100644 --- a/docs/Troubleshooting-Wiki.md +++ b/docs/Troubleshooting-Wiki.md @@ -279,11 +279,20 @@ Limits worth knowing: other indexers for the same book are still grabbed, and nothing from the pinned indexer goes to another client. * A grab with no `indexerId` whose download URL is on a host that matches no - indexer (a proxied link on another host) cannot be tied to a pinned indexer - and is routed as if unpinned. Grabs from the Bindery UI always send the - indexer; API callers should send `indexerId`. -* Several indexers on one host with one of them pinned: a grab without - `indexerId` is refused, because it cannot be told which indexer it came from. + indexer (a proxied link on another host, a `www.` or other subdomain + spelling, a scheme-less indexer URL) cannot be tied to a pinned indexer and + is routed as if unpinned. The host match ignores case, a trailing dot and the + port, so the usual Prowlarr case of `prowlarr` against `prowlarr:9696` is + handled. Grabs from the Bindery UI and the scheduler always send the indexer; + API callers should send `indexerId`. +* An `indexerId` you send is taken at face value when that indexer's host + matches the release URL's host, even if another indexer on that host is + pinned. +* On a shared Prowlarr or Jackett host, one pinned indexer makes id-less API + grabs fail for every indexer on that host, because such a grab cannot be told + apart. Send `indexerId` (the UI and the scheduler already do). +* A pending (held) release from a pinned indexer whose client is down stays in + Pending and is tried again on the next sweep. It is not discarded. ### The Queue looks empty, or shorter than it should be diff --git a/internal/api/indexer_pin_host_test.go b/internal/api/indexer_pin_host_test.go new file mode 100644 index 000000000..e5ea28f68 --- /dev/null +++ b/internal/api/indexer_pin_host_test.go @@ -0,0 +1,115 @@ +package api + +import ( + "net/http" + "strconv" + "strings" + "testing" + + "github.com/vavallee/bindery/internal/models" +) + +// otherHostURL rewrites a test server's URL onto the host name localhost. The +// pin host match ignores ports on purpose, so two test servers on 127.0.0.1 are +// one host to it, and a genuinely different host is needed for the cases that +// must not match. +func otherHostURL(raw string) string { + return strings.Replace(raw, "127.0.0.1", "localhost", 1) +} + +// A keyless pinned indexer and a keyed unpinned one share the host. Signing can +// only attribute the keyed one, so an id-less grab used to be taken for the +// unpinned indexer and went to the public client. The host check now runs +// whenever the caller named no indexer, and refuses. +func TestGrab_KeylessPinnedAndKeyedUnpinnedOnOneHostIsRefusedWithoutAnID(t *testing.T) { + f := newPinFixture(t) + f.idx.APIKey = "" + f.pin(t, &f.privC.ID) + f.addIndexer(t, "TrackerQ", "/apiq", nil) + + w := f.grabURL("g-dual", f.relURL, nil, false) + if w.Code != http.StatusBadRequest || !strings.Contains(w.Body.String(), codeIndexerClientUnavailable) { + t.Fatalf("status %d, want 400 %s: %s", w.Code, codeIndexerClientUnavailable, w.Body.String()) + } + f.wantReached(t, w, 0, 0) +} + +// The same pair with the caller naming the unpinned indexer truthfully is taken +// at face value: its host matches the release, so it goes the default way. +func TestGrab_TruthfulIDOfAnUnpinnedIndexerSharingAHostWithAPinnedOneGoesPublic(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + q := f.addIndexer(t, "TrackerQ", "/apiq", nil) + + w := f.grabURL("g-truthful", f.relURL, &q.ID, false) + f.wantReached(t, w, 1, 0) +} + +// A supplied id that contradicts the release URL's host, while an indexer that +// does match the host is pinned, is refused. The URL is pre signed so nothing +// else attributes it. +func TestGrab_SuppliedIDContradictingTheURLHostForAPinnedHostIsRefused(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + elsewhere := &models.Indexer{Name: "Elsewhere", Type: "newznab", URL: otherHostURL(f.pub.srv.URL) + "/api", APIKey: "k", Enabled: true} + if err := f.indexers.Create(f.ctx, elsewhere); err != nil { + t.Fatal(err) + } + w := f.grabURL("g-contra", f.relURL+"?apikey=secret", &elsewhere.ID, false) + if w.Code != http.StatusBadRequest || !strings.Contains(w.Body.String(), codeIndexerClientUnavailable) { + t.Fatalf("status %d, want 400 %s: %s", w.Code, codeIndexerClientUnavailable, w.Body.String()) + } + f.wantReached(t, w, 0, 0) +} + +// A contradicting id with no pinned indexer on the URL's host is not a pin +// problem and proceeds. +func TestGrab_SuppliedIDContradictingTheURLHostWithNoPinnedHostProceeds(t *testing.T) { + f := newPinFixture(t) + elsewhere := &models.Indexer{Name: "Elsewhere", Type: "newznab", URL: otherHostURL(f.pub.srv.URL) + "/api", APIKey: "k", Enabled: true} + if err := f.indexers.Create(f.ctx, elsewhere); err != nil { + t.Fatal(err) + } + w := f.grabURL("g-contra-open", f.relURL+"?apikey=secret", &elsewhere.ID, false) + f.wantReached(t, w, 1, 0) +} + +// A supplied pinned indexer always applies its own pin. +func TestGrab_SuppliedPinnedIDStillAppliesItsPin(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + w := f.grabURL("g-supplied-pinned", f.relURL+"?apikey=secret", &f.idx.ID, false) + f.wantReached(t, w, 0, 1) +} + +// An indexer recorded on another port of the same host (the Prowlarr layout, +// where the application URL and the indexer URL differ by port) is still found +// by an id-less grab. +func TestGrab_IDLessGrabFindsAPinnedIndexerThatDiffersOnlyByPort(t *testing.T) { + f := newPinFixture(t) + other := strings.Replace(f.idx.URL, ":"+portOf(t, f.idx.URL), ":"+strconv.Itoa(1+atoi(t, portOf(t, f.idx.URL))), 1) + f.idx.URL = other + f.idx.APIKey = "" + f.pin(t, &f.privC.ID) + w := f.grabURL("g-port", f.relURL, nil, false) + f.wantReached(t, w, 0, 1) +} + +func portOf(t *testing.T, raw string) string { + t.Helper() + i := strings.LastIndex(raw, ":") + j := strings.Index(raw[i:], "/") + if i < 0 || j < 0 { + t.Fatalf("no port in %q", raw) + } + return raw[i+1 : i+j] +} + +func atoi(t *testing.T, s string) int { + t.Helper() + n, err := strconv.Atoi(s) + if err != nil { + t.Fatal(err) + } + return n +} diff --git a/internal/api/indexer_pin_type_test.go b/internal/api/indexer_pin_type_test.go new file mode 100644 index 000000000..40d4f5e03 --- /dev/null +++ b/internal/api/indexer_pin_type_test.go @@ -0,0 +1,28 @@ +package api + +import ( + "net/http" + "strconv" + "testing" +) + +// An update body with an empty type means the default type (usenet), which is +// what the searcher already reads it as. For an indexer that is already usenet +// it must not count as a change of type, so a pinned client that was switched +// to the other protocol does not block the save, and nothing crashes. +func TestIndexerPin_EmptyTypeInAnUpdateIsNotATypeChange(t *testing.T) { + f := newPinAPIFixture(t) + usenetID := strconv.FormatInt(f.usenet.ID, 10) + created := decodeIndexer(t, f.create(`{"name":"Usenet","url":"http://192.168.1.50:9117/api","type":"newznab","downloadClientId":`+usenetID+`}`)) + f.usenet.Type = "qbittorrent" + if err := f.clients.Update(t.Context(), f.usenet); err != nil { + t.Fatal(err) + } + rec := f.update(created.ID, `{"type":"","priority":3}`) + if rec.Code != http.StatusOK { + t.Fatalf("update with an empty type: %d %s", rec.Code, rec.Body.String()) + } + if got := decodeIndexer(t, rec); got.DownloadClientID == nil || *got.DownloadClientID != f.usenet.ID || got.Priority != 3 { + t.Errorf("the update changed the wrong things: %+v", got) + } +} diff --git a/internal/api/indexer_pin_unattributed_test.go b/internal/api/indexer_pin_unattributed_test.go index 090db1a59..cabd3655c 100644 --- a/internal/api/indexer_pin_unattributed_test.go +++ b/internal/api/indexer_pin_unattributed_test.go @@ -113,7 +113,7 @@ func TestGrab_AmbiguousHostWithNoPinnedIndexerProceeds(t *testing.T) { func TestGrab_ReleaseOnAnUnrelatedHostIsUnpinned(t *testing.T) { f := newPinFixture(t) f.pin(t, &f.privC.ID) - w := f.grabURL("g-elsewhere", f.pub.srv.URL+"/other.nzb", nil, false) + w := f.grabURL("g-elsewhere", otherHostURL(f.pub.srv.URL)+"/other.nzb", nil, false) f.wantReached(t, w, 1, 0) } @@ -149,7 +149,7 @@ func TestRetry_RowWithNoIndexerOnUnknownHostIsUnpinned(t *testing.T) { f := newPinFixture(t) f.pin(t, &f.privC.ID) dl := &models.Download{ - GUID: "g-orphan2", Title: "Pin Author - Pinned Book epub", NZBURL: f.pub.srv.URL + "/gone.nzb", Size: 100, + GUID: "g-orphan2", Title: "Pin Author - Pinned Book epub", NZBURL: otherHostURL(f.pub.srv.URL) + "/gone.nzb", Size: 100, Status: models.StateFailed, Protocol: "usenet", Quality: "epub", BookID: &f.book.ID, } if err := f.downl.Create(f.ctx, dl); err != nil { diff --git a/internal/api/indexers.go b/internal/api/indexers.go index b7f2fd7ed..47d4c378b 100644 --- a/internal/api/indexers.go +++ b/internal/api/indexers.go @@ -223,7 +223,7 @@ func (h *IndexerHandler) validateDownloadClientPin(ctx context.Context, idx mode // indexer, would be blocked by a client changed behind the pin's back, and // the grab fails loudly at grab time anyway. pinChanged := stored == nil || stored.DownloadClientID == nil || *stored.DownloadClientID != *idx.DownloadClientID - typeChanged := stored == nil || stored.Type != idx.Type + typeChanged := stored == nil || effectiveIndexerType(stored.Type) != effectiveIndexerType(idx.Type) if got := db.ClientProtocol(c.Type); got != idx.Protocol() && (pinChanged || typeChanged) { return errPinInvalid(fmt.Sprintf("download client %q is a %s client but this indexer is %s, so it cannot be pinned to it; pick a %s client", c.Name, got, idx.Protocol(), idx.Protocol())) @@ -234,6 +234,16 @@ func (h *IndexerHandler) validateDownloadClientPin(ctx context.Context, idx mode return nil } +// effectiveIndexerType reads an empty type as the default, usenet, which is what +// Indexer.Protocol and the searcher already do, so an update body that sends an +// empty type is not a change of type for an indexer that is already usenet. +func effectiveIndexerType(t string) string { + if t == "" { + return "newznab" + } + return t +} + // pinInvalidError is a 400 about the download client pin, as opposed to a // storage failure. type pinInvalidError string diff --git a/internal/api/queue.go b/internal/api/queue.go index 0e2c0af8c..7b80c9677 100644 --- a/internal/api/queue.go +++ b/internal/api/queue.go @@ -1386,15 +1386,27 @@ func (h *QueueHandler) selectClient(ctx context.Context, protocol, mediaType str return db.PickClientForMediaType(candidates, mediaType), nil } -// pinnedIndexerOnHost looks for an indexer pinned to a download client among -// those on the release URL's host, for a grab that carries no indexer id. None -// pinned means the grab is unaffected (nil, nil). Exactly one indexer on the -// host, pinned, is the release's indexer and is returned so its pin applies. -// Several on the host with at least one pinned cannot be told apart, and the -// grab is refused with an *ambiguousPinError rather than guessed. A host that -// matches no indexer carries nothing to act on and is left unpinned: send the -// indexer id to have the pin apply. -func (h *QueueHandler) pinnedIndexerOnHost(ctx context.Context, rawURL string) (*models.Indexer, error) { +// pinnedIndexerOnHost checks, for a grab, whether the release could have come +// from an indexer pinned to a download client that the grab is not already tied +// to (#3080). The indexers considered are those on the release URL's host +// (newznab.DownloadHostMatchesIndexer). It returns the indexer whose pin should +// apply when that is unambiguous, or an *ambiguousPinError when the grab must be +// refused, or (nil, nil) when nothing about the host calls for action. +// +// - The caller named no indexer (callerID nil). Signing may still have +// attributed one (signedByHost), but it only counts indexers that can sign. +// Exactly one indexer on the host, pinned and not otherwise attributed, is +// the release's indexer. Otherwise any pinned indexer on the host that is not +// the attributed one makes the grab ambiguous, and it is refused. +// - The caller named an indexer. A truthful id, one whose host matches the +// release, is taken at face value, even when a pinned indexer shares the +// host. An id that contradicts the host is refused when an indexer on the +// release's host is pinned, unless that id is itself pinned (its own pin +// applies) or signing found the host's indexer (signedByHost). +// +// A host that matches no indexer carries nothing to act on and is left unpinned: +// send the indexer id to have the pin apply. +func (h *QueueHandler) pinnedIndexerOnHost(ctx context.Context, rawURL string, callerID, currentID *int64, signedByHost bool) (*models.Indexer, error) { if h.indexers == nil || rawURL == "" { return nil, nil } @@ -1403,23 +1415,55 @@ func (h *QueueHandler) pinnedIndexerOnHost(ctx context.Context, rawURL string) ( return nil, err } var matching []models.Indexer - var pinned []string - for _, idx := range idxs { + var pinnedOnHost []models.Indexer + var supplied *models.Indexer + for i := range idxs { + idx := idxs[i] + if callerID != nil && idx.ID == *callerID { + supplied = &idxs[i] + } if !newznab.DownloadHostMatchesIndexer(rawURL, idx.URL) { continue } matching = append(matching, idx) if idx.DownloadClientID != nil { - pinned = append(pinned, idx.Name) + pinnedOnHost = append(pinnedOnHost, idx) } } - if len(pinned) == 0 { + if len(pinnedOnHost) == 0 { return nil, nil } - if len(matching) == 1 { + names := func(list []models.Indexer) []string { + out := make([]string, 0, len(list)) + for _, idx := range list { + out = append(out, idx.Name) + } + return out + } + + if callerID != nil { + if signedByHost || supplied == nil || supplied.DownloadClientID != nil { + return nil, nil + } + if newznab.DownloadHostMatchesIndexer(rawURL, supplied.URL) { + return nil, nil + } + return nil, &ambiguousPinError{pinned: names(pinnedOnHost)} + } + + if len(matching) == 1 && (currentID == nil || *currentID == matching[0].ID) { return &matching[0], nil } - return nil, &ambiguousPinError{pinned: pinned} + var others []models.Indexer + for _, idx := range pinnedOnHost { + if currentID == nil || idx.ID != *currentID { + others = append(others, idx) + } + } + if len(others) == 0 { + return nil, nil + } + return nil, &ambiguousPinError{pinned: names(others)} } // clientForGrab picks the client for a grab. An indexer pinned to a download @@ -1619,6 +1663,13 @@ func (h *QueueHandler) grab(ctx context.Context, req grabRequest) (*models.Downl // hands back an apikey-less download URL plus the indexer id; sign it // server-side here. No-op when the URL already carries a key (scheduler / // retry paths) or its host does not match the indexer. + // callerIndexerID is the indexer the caller (or the stored row, on a + // retry) named, before anything below attributes the release by host. + var callerIndexerID *int64 + if indexerID != nil { + v := *indexerID + callerIndexerID = &v + } nzbURL, hostMatched := h.signNZBURL(ctx, req.NZBURL, indexerID) // The host-match fallback knows which indexer signed the URL, so record it. // Without this the row keeps a nil (or dangling) IndexerID for precisely the @@ -1627,18 +1678,17 @@ func (h *QueueHandler) grab(ctx context.Context, req grabRequest) (*models.Downl if hostMatched != nil { indexerID = hostMatched } - // A grab still unattributed here may nonetheless come from a pinned - // indexer: signing cannot attribute a pre signed URL, a keyless indexer or - // two indexers sharing a host (#3080). Look at every indexer on the + // Signing cannot attribute a pre signed URL, a keyless indexer or two + // indexers sharing a host, and it only counts indexers that can sign, so + // an id-less grab may be tied to an unpinned indexer while a pinned one on + // the same host is the real source (#3080). Look at every indexer on the // release's host before the default routing is allowed to pick a client. - if indexerID == nil { - onHost, err := h.pinnedIndexerOnHost(ctx, req.NZBURL) - if err != nil { - return nil, err - } - if onHost != nil { - indexerID = &onHost.ID - } + onHost, err := h.pinnedIndexerOnHost(ctx, req.NZBURL, callerIndexerID, indexerID, hostMatched != nil) + if err != nil { + return nil, err + } + if onHost != nil { + indexerID = &onHost.ID } // The client is chosen only now, after the host match above has settled diff --git a/internal/indexer/newznab/client.go b/internal/indexer/newznab/client.go index b2eb0059c..1a47532a9 100644 --- a/internal/indexer/newznab/client.go +++ b/internal/indexer/newznab/client.go @@ -198,16 +198,32 @@ func SignDownloadURLFor(rawURL, indexerURL, apiKey string) string { // uses. Unlike signing it needs no stored key and ignores whether the URL is // already signed, so it answers "could this release have come from that // indexer" for the cases signing cannot (#3080). +// +// The comparison is on the host name only: case is ignored, one trailing dot is +// dropped, and the port is ignored in every combination (explicit against none, +// the scheme's default against none, two different explicit ports). Ignoring a +// differing port is deliberate and errs the safe way for a pin: Prowlarr builds +// download links from its own application URL, which need not carry the port +// the indexer was recorded with, and a spurious match only refuses a grab the +// user can retry with an indexer id, whereas a spurious miss would put a +// private tracker's release on the public client. Signing keeps its own, exact +// comparison (signDownloadURL); this function does not change it. A host that +// differs by a "www." or any other subdomain is a different host, and punycode +// is not folded to Unicode, so those remain misses. func DownloadHostMatchesIndexer(rawURL, indexerURL string) bool { - base, err := url.Parse(normalizeEndpointURL(indexerURL)) - if err != nil || base.Host == "" { - return false - } - u, err := url.Parse(rawURL) - if err != nil || u.Host == "" { - return false + base := pinHostName(normalizeEndpointURL(indexerURL)) + rel := pinHostName(rawURL) + return base != "" && base == rel +} + +// pinHostName returns the lower case host name of raw without its port or a +// single trailing dot, or "" when raw has none. +func pinHostName(raw string) string { + u, err := url.Parse(raw) + if err != nil { + return "" } - return strings.EqualFold(u.Host, strings.ToLower(base.Host)) + return strings.TrimSuffix(strings.ToLower(u.Hostname()), ".") } // HasAPIKey reports whether a download URL already carries an apikey. diff --git a/internal/indexer/newznab/pin_host_test.go b/internal/indexer/newznab/pin_host_test.go new file mode 100644 index 000000000..e2e54a97d --- /dev/null +++ b/internal/indexer/newznab/pin_host_test.go @@ -0,0 +1,39 @@ +package newznab + +import "testing" + +// DownloadHostMatchesIndexer decides which indexers a release could have come +// from for the purpose of a download client pin. It fails closed: a spurious +// match is a refused grab, a spurious miss would put a private tracker's +// release on the public client. +func TestDownloadHostMatchesIndexer(t *testing.T) { + cases := []struct { + name, release, indexer string + want bool + }{ + {"same host", "http://tracker.example/dl/1", "http://tracker.example/api", true}, + {"case differs", "http://Tracker.EXAMPLE/dl/1", "http://tracker.example/api", true}, + {"trailing dot", "http://tracker.example./dl/1", "http://tracker.example/api", true}, + {"trailing dot on the indexer", "http://tracker.example/dl/1", "http://tracker.example./api", true}, + {"explicit port against none (the Prowlarr layout)", "http://prowlarr/dl/1", "http://prowlarr:9696/1/api", true}, + {"none against explicit port", "http://prowlarr:9696/dl/1", "http://prowlarr/1/api", true}, + {"default https port against none", "https://tracker.example:443/dl/1", "https://tracker.example/api", true}, + {"default http port against none", "http://tracker.example:80/dl/1", "http://tracker.example/api", true}, + {"different explicit ports", "http://tracker.example:8080/dl/1", "http://tracker.example:9090/api", true}, + {"ipv4 with ports", "http://10.0.0.5:9117/dl/1", "http://10.0.0.5:9696/api", true}, + {"ipv6", "http://[::1]:9117/dl/1", "http://[::1]/api", true}, + {"different host", "http://other.example/dl/1", "http://tracker.example/api", false}, + {"www is a different host, a documented limit", "http://www.tracker.example/dl/1", "http://tracker.example/api", false}, + {"subdomain is a different host, a documented limit", "http://dl.tracker.example/dl/1", "http://tracker.example/api", false}, + {"empty release url", "", "http://tracker.example/api", false}, + {"unparseable release url", "http://[bad", "http://tracker.example/api", false}, + {"indexer url with no host", "http://tracker.example/dl/1", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := DownloadHostMatchesIndexer(tc.release, tc.indexer); got != tc.want { + t.Errorf("DownloadHostMatchesIndexer(%q, %q) = %v, want %v", tc.release, tc.indexer, got, tc.want) + } + }) + } +} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index a27e61688..bcf52d5e7 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1356,19 +1356,14 @@ func (s *Scheduler) searchAndGrabFormat(ctx context.Context, book models.Book, m } if best == nil { // Re-evaluate any existing pending releases for this book/format with the current age. - best = s.checkPendingReleases(ctx, book, mediaType, dm) - if best != nil { - c, skip, err := s.checkPin(ctx, book, best, skippedPinned) - if err != nil { - slog.Error("SearchAndGrabBook: failed to resolve the indexer's pinned download client", "indexer", best.IndexerName, "error", err) - outcome = "download client lookup failed" - return - } - if skip { - best = nil - } else { - bestClient = c - } + var pendErr error + best, bestClient, pendErr = s.pendingApproved(ctx, book, mediaType, dm, func(res *newznab.SearchResult) (*models.DownloadClient, bool, error) { + return s.checkPin(ctx, book, res, skippedPinned) + }) + if pendErr != nil { + slog.Error("SearchAndGrabBook: failed to resolve the indexer's pinned download client for a pending release", "error", pendErr) + outcome = "download client lookup failed" + return } if best == nil { switch { @@ -1598,12 +1593,24 @@ func (s *Scheduler) storePending(ctx context.Context, bookID int64, mediaType st // book's ebook and audiobook pending entries are evaluated independently. // If any now passes the decision engine it is returned for immediate grab. func (s *Scheduler) checkPendingReleases(ctx context.Context, book models.Book, mediaType string, dm *decision.DecisionMaker) *newznab.SearchResult { + res, _, _ := s.pendingApproved(ctx, book, mediaType, dm, nil) + return res +} + +// pendingApproved is checkPendingReleases with a gate. accept, when set, is +// asked about each approved release before it is taken: it returns the client to +// use, whether to skip the release, or an error that stops the sweep. A skipped +// release stays pending, untouched, for the next sweep (a pinned client that is +// down must not cost the user a held release), and the next approved row is +// tried. Only a release that is taken is deleted from pending. +func (s *Scheduler) pendingApproved(ctx context.Context, book models.Book, mediaType string, dm *decision.DecisionMaker, + accept func(*newznab.SearchResult) (*models.DownloadClient, bool, error)) (*newznab.SearchResult, *models.DownloadClient, error) { if s.pending == nil { - return nil + return nil, nil, nil } pendingList, err := s.pending.ListByBookAndMediaType(ctx, book.ID, mediaType) if err != nil || len(pendingList) == 0 { - return nil + return nil, nil, nil } // Re-hydrate stored releases and re-evaluate with current age. @@ -1616,16 +1623,27 @@ func (s *Scheduler) checkPendingReleases(ctx context.Context, book models.Book, rel := decision.ReleaseFromSearchResult(res) decisions := dm.Evaluate([]decision.Release{rel}, book) if len(decisions) > 0 && decisions[0].Approved { + var client *models.DownloadClient + if accept != nil { + c, skip, err := accept(&res) + if err != nil { + return nil, nil, err + } + if skip { + continue + } + client = c + } if err := s.pending.DeleteByID(ctx, pendingList[i].ID); err != nil { // If the entry survives it is re-approved and re-grabbed on the // next cycle — duplicate grabs with no visible cause. slog.Warn("failed to delete pending release after approval", "pending_id", pendingList[i].ID, "book_id", book.ID, "guid", res.GUID, "error", err) } - return &res + return &res, client, nil } } - return nil + return nil, nil, nil } func (s *Scheduler) searchWanted() { diff --git a/internal/scheduler/scheduler_indexer_pin_pending_test.go b/internal/scheduler/scheduler_indexer_pin_pending_test.go new file mode 100644 index 000000000..d11f5714f --- /dev/null +++ b/internal/scheduler/scheduler_indexer_pin_pending_test.go @@ -0,0 +1,157 @@ +package scheduler + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/indexer/newznab" + "github.com/vavallee/bindery/internal/models" +) + +// pendingPinFixture has nothing from search, so the sweep falls through to the +// pending releases, and a pinned indexer whose client is disabled. +func pendingPinFixture(t *testing.T) (*schedPinFixture, *db.PendingReleaseRepo, *models.Indexer) { + t.Helper() + f := newSchedPinFixture(t) + f.pin(t, f.privC.ID) + f.privC.Enabled = false + if err := f.clients.Update(f.ctx, f.privC); err != nil { + t.Fatal(err) + } + open := &models.Indexer{Name: "OpenIdx", Type: "newznab", URL: "http://open.invalid", Enabled: true} + if err := f.indexers.Create(f.ctx, open); err != nil { + t.Fatal(err) + } + f.s.searcher.(*perFormatSearcher).byFormat[models.MediaTypeEbook] = nil + pending := db.NewPendingReleaseRepo(f.database) + f.s.pending = pending + return f, pending, open +} + +func (f *schedPinFixture) holdPending(t *testing.T, pending *db.PendingReleaseRepo, guid string, idx *models.Indexer) { + t.Helper() + res := newznab.SearchResult{ + GUID: guid, Title: "Pin Author Pinned Book epub " + guid, NZBURL: f.pub.srv.URL + "/" + guid + ".nzb", + Protocol: "usenet", IndexerID: idx.ID, IndexerName: idx.Name, Size: 2 << 20, + } + blob, _ := json.Marshal(res) + if err := pending.Upsert(f.ctx, &models.PendingRelease{ + BookID: f.book.ID, MediaType: models.MediaTypeEbook, Title: res.Title, IndexerID: &idx.ID, + GUID: guid, Protocol: "usenet", Reason: "delay", ReleaseJSON: string(blob), + }); err != nil { + t.Fatal(err) + } +} + +// TestPending_ApprovedReleaseOfAnUnavailablePinnedClientStaysPending: the held +// release must not be deleted when the pin refuses it. It stays pending for the +// sweep after the client is back, nothing is grabbed, and the failure is +// recorded once. +func TestPending_ApprovedReleaseOfAnUnavailablePinnedClientStaysPending(t *testing.T) { + f, pending, _ := pendingPinFixture(t) + f.holdPending(t, pending, "g-held", f.idx) + + f.s.SearchAndGrabBook(f.ctx, f.book) + + rows, err := pending.ListByBookAndMediaType(f.ctx, f.book.ID, models.MediaTypeEbook) + if err != nil || len(rows) != 1 { + t.Fatalf("pending rows after the sweep = %d (err %v), want the held release to survive", len(rows), err) + } + if f.pub.count() != 0 || f.priv.count() != 0 { + t.Fatalf("a client received the grab: public=%d private=%d", f.pub.count(), f.priv.count()) + } + if n := len(f.pinnedFailures(t)); n != 1 { + t.Errorf("pinned failure events = %d, want 1", n) + } + // And once the client is back, the very same release is grabbed from it. + f.privC.Enabled = true + if err := f.clients.Update(f.ctx, f.privC); err != nil { + t.Fatal(err) + } + f.s.SearchAndGrabBook(f.ctx, f.book) + if f.priv.count() != 1 || f.pub.count() != 0 { + t.Fatalf("after the client came back: public=%d private=%d, want 0 and 1", f.pub.count(), f.priv.count()) + } +} + +// TestPending_SkipsAPinnedRowAndTriesTheNextApprovedRow: the first approved +// pending row is pinned and refused, the second is from an unpinned indexer and +// is grabbed in the same sweep. +func TestPending_SkipsAPinnedRowAndTriesTheNextApprovedRow(t *testing.T) { + f, pending, open := pendingPinFixture(t) + f.holdPending(t, pending, "g-held-open", open) + // Newest first: the pinned row is inserted last so it is tried first. + f.holdPending(t, pending, "g-held-pinned", f.idx) + + f.s.SearchAndGrabBook(f.ctx, f.book) + + if f.priv.count() != 0 || f.pub.count() != 1 { + t.Fatalf("public=%d private=%d, want the unpinned row grabbed and nothing on the pinned client", f.pub.count(), f.priv.count()) + } + if dl, _ := f.downloads.GetByGUID(f.ctx, "g-held-open"); dl == nil { + t.Errorf("the unpinned pending release was not grabbed") + } + if dl, _ := f.downloads.GetByGUID(f.ctx, "g-held-pinned"); dl != nil { + t.Errorf("the pinned pending release was sent somewhere: %+v", dl) + } + if n := len(f.pinnedFailures(t)); n != 1 { + t.Errorf("pinned failure events = %d, want 1", n) + } +} + +// TestAutoGrab_PinnedFailureRecordCarriesCodeAndIndexerButNoClient: the History +// event is machine readable for the dedupe, and neither it nor the notification +// names the client or its id. +func TestAutoGrab_PinnedFailureRecordCarriesCodeAndIndexerButNoClient(t *testing.T) { + f, _ := pinnedFirstFixture(t) + n := &recordingNotifier{} + f.s.notif = n + f.s.SearchAndGrabBook(f.ctx, f.book) + + events := f.pinnedFailures(t) + if len(events) != 1 { + t.Fatalf("pinned failure events = %d, want 1", len(events)) + } + var data map[string]any + if err := json.Unmarshal([]byte(events[0].Data), &data); err != nil { + t.Fatal(err) + } + if data["code"] != "indexer_client_unavailable" { + t.Errorf("code = %v", data["code"]) + } + if got, _ := data["indexerId"].(float64); int64(got) != f.idx.ID { + t.Errorf("indexerId = %v, want %d", data["indexerId"], f.idx.ID) + } + if !strings.Contains(events[0].Data, "TrackerX") { + t.Errorf("history message does not name the indexer: %s", events[0].Data) + } + if strings.Contains(events[0].Data, "sab-private") || strings.Contains(events[0].Data, "downloadClient") { + t.Errorf("history leaks the client: %s", events[0].Data) + } + var failed []map[string]interface{} + for i, typ := range n.types { + if typ == "downloadFailed" { + failed = append(failed, n.events[i]) + } + } + if len(failed) != 1 { + t.Fatalf("downloadFailed notifications = %d, want 1", len(failed)) + } + body, _ := json.Marshal(failed[0]) + if !strings.Contains(string(body), "TrackerX") { + t.Errorf("notification does not name the indexer: %s", body) + } + for _, banned := range []string{"sab-private", "clientId", "downloadClient"} { + if strings.Contains(string(body), banned) { + t.Errorf("notification contains %q: %s", banned, body) + } + } + // The client's numeric id must not appear as a field value either. + for k, v := range failed[0] { + if k == "clientId" || v == f.privC.ID { + t.Errorf("notification field %s carries the client id: %v", k, v) + } + } +} From 3aeb62999b7f6482371322472e889b6a902e448f Mon Sep 17 00:00:00 2001 From: vavallee Date: Wed, 14 Oct 2026 22:22:51 -0300 Subject: [PATCH 7/8] docs(indexer): note that held releases wait for a pinned client (#3080) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- changelog.d/3080-per-indexer-download-client.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/3080-per-indexer-download-client.md b/changelog.d/3080-per-indexer-download-client.md index 405391fb1..7b0498e39 100644 --- a/changelog.d/3080-per-indexer-download-client.md +++ b/changelog.d/3080-per-indexer-download-client.md @@ -1,2 +1,2 @@ ### Added -- **Per-indexer download client** (#3080): each indexer now has an optional **Download client** setting in Settings → Indexers. Leave it on Default and nothing changes. Pick a client and every grab from that indexer goes to it: scheduled searches, the search page, held releases in Pending and Retry download. It overrides the client's priority and category routing, so a private tracker's torrents can live on their own qBittorrent. If that client is disabled, missing, the wrong protocol or not answering, the grab fails with a message naming the indexer (and the client, for an admin) and is never sent to another client. A scheduled search skips only that indexer's releases and still grabs from your others. A download client that an indexer is pinned to cannot be deleted (the request is refused with the indexer names), and the indexer row shows a warning while its client is disabled, missing or the wrong protocol. Prowlarr syncs keep the setting. +- **Per-indexer download client** (#3080): each indexer now has an optional **Download client** setting in Settings → Indexers. Leave it on Default and nothing changes. Pick a client and every grab from that indexer goes to it: scheduled searches, the search page, held releases in Pending and Retry download. It overrides the client's priority and category routing, so a private tracker's torrents can live on their own qBittorrent. If that client is disabled, missing, the wrong protocol or not answering, the grab fails with a message naming the indexer (and the client, for an admin) and is never sent to another client. A scheduled search skips only that indexer's releases and still grabs from your others, and a held release from that indexer stays in Pending until its client is back. A download client that an indexer is pinned to cannot be deleted (the request is refused with the indexer names), and the indexer row shows a warning while its client is disabled, missing or the wrong protocol. Prowlarr syncs keep the setting. From de08fee082bb5a8117b883a48314a641c8225f25 Mon Sep 17 00:00:00 2001 From: vavallee Date: Wed, 14 Oct 2026 22:38:42 -0300 Subject: [PATCH 8/8] fix(indexer): decide a grab's indexer once, before signing (#3080) Replaces the interacting heuristics (signing's pick, the host check and the caller id branch) with one pure function, resolveGrabPin, and a table test of the whole matrix. Signing can no longer override the decision, an unknown id is treated as no id, and the row records the decided indexer. An explicit empty type in an indexer update keeps the stored type. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- docs/API.md | 51 ++++---- docs/Troubleshooting-Wiki.md | 27 +++-- internal/api/grab_pin.go | 125 ++++++++++++++++++++ internal/api/grab_pin_test.go | 130 +++++++++++++++++++++ internal/api/indexer_pin_leaks_test.go | 121 +++++++++++++++++++ internal/api/indexer_pin_type_test.go | 20 ++++ internal/api/indexers.go | 5 + internal/api/queue.go | 154 +++++-------------------- 8 files changed, 478 insertions(+), 155 deletions(-) create mode 100644 internal/api/grab_pin.go create mode 100644 internal/api/grab_pin_test.go create mode 100644 internal/api/indexer_pin_leaks_test.go diff --git a/docs/API.md b/docs/API.md index 0b8d11fa9..f67958e8a 100644 --- a/docs/API.md +++ b/docs/API.md @@ -558,30 +558,41 @@ client that is enabled but does not answer is the usual `502`, with the message extended to say the indexer is pinned and no other client was tried. The field is only returned by the admin only indexer endpoints. -A grab that arrives without an `indexerId` is tied to an indexer by its download -URL's host, whether or not the URL is already signed. One indexer on that host, -pinned, applies its pin. When another indexer on the host is pinned and the grab -cannot be tied to just one indexer, it is refused with the same `400` and code -(nothing is sent) and the message says the release could not be tied to one -indexer. That includes a pinned indexer without a stored key sharing a host with -a keyed one. On a shared Prowlarr or Jackett host, one pinned indexer therefore -makes id-less API grabs fail for every indexer on that host; the web app and the -scheduler always send the indexer, and are unaffected. - -An `indexerId` you send is taken at face value when that indexer's host matches -the release URL's host, even if another indexer on the same host is pinned. An id -that contradicts the URL's host is refused when an indexer on the URL's host is -pinned (unless the id itself is pinned, in which case its own pin applies). + +Which indexer a grab belongs to is decided once, from the `indexerId` you send +and the release URL's host, before the URL is signed. Signing never overrides +the decision. The rules, in order: + +1. An `indexerId` that exists and is pinned always wins. Its pin applies, + whatever the URL's host is and whoever could sign the URL. +2. An `indexerId` that exists and is not pinned is taken at face value when its + host matches the release URL's host, even if a pinned indexer shares that + host. When its host does not match the URL's host and some indexer that does + match is pinned, the grab is refused. Otherwise it stands. +3. An `indexerId` that matches no indexer (or `null`, as on a retry of a row whose + indexer was deleted) is treated as no id at all. +4. With no usable id, the indexers whose host matches the URL decide. None: the + grab is routed as if unpinned. Exactly one: that is the release's indexer, and + its pin, if any, applies (this finds a recreated indexer, and keyless + indexers). Several: if any is pinned the grab is refused, because it cannot be + told which one it came from; if none is pinned nothing changes. + +A refusal is the `400` with `code: "indexer_client_unavailable"` and nothing is +sent. The message says the release could not be tied to one indexer (or that the +named indexer does not match the release's host), and lists the pinned indexers +for an admin only. The row's `indexerId` is the decided indexer. The retry +and the pending grab go through the same decision. The host comparison ignores case, one trailing dot and the port, in any combination: `prowlarr` and `prowlarr:9696` are the same host. Ignoring a differing port is deliberate, because a spurious refusal is safer than letting a -private release reach the public client. `www.` and other subdomains, scheme-less -indexer URLs and punycode against Unicode spellings are different hosts and are -not matched. A host that matches no indexer at all carries nothing to act on and -is routed as if unpinned, so send `indexerId` to be sure the pin applies. A retry -of a Queue row whose indexer was deleted is attributed the same way, by the stored -release URL. +private release reach the public client. These do not match, so an id-less grab +through them is routed as if unpinned unless you send `indexerId`: aliases of one +machine (`localhost`, `127.0.0.1` and a DNS name), `www.` and other subdomains, +scheme-less indexer URLs, and punycode against Unicode spellings. On a shared +Prowlarr or Jackett host, one pinned indexer makes id-less API grabs fail for +every other indexer on that host; the web app and the scheduler always send the +indexer and are unaffected. On save, a client with the wrong protocol is refused only when the request sets the pin or changes the indexer `type`. Enabling, disabling or reprioritising an diff --git a/docs/Troubleshooting-Wiki.md b/docs/Troubleshooting-Wiki.md index f2ede2fa8..a31a5e3d5 100644 --- a/docs/Troubleshooting-Wiki.md +++ b/docs/Troubleshooting-Wiki.md @@ -278,19 +278,22 @@ Limits worth knowing: * A scheduled search skips only the failing indexer's releases. Releases from other indexers for the same book are still grabbed, and nothing from the pinned indexer goes to another client. -* A grab with no `indexerId` whose download URL is on a host that matches no - indexer (a proxied link on another host, a `www.` or other subdomain - spelling, a scheme-less indexer URL) cannot be tied to a pinned indexer and - is routed as if unpinned. The host match ignores case, a trailing dot and the - port, so the usual Prowlarr case of `prowlarr` against `prowlarr:9696` is - handled. Grabs from the Bindery UI and the scheduler always send the indexer; - API callers should send `indexerId`. -* An `indexerId` you send is taken at face value when that indexer's host - matches the release URL's host, even if another indexer on that host is - pinned. +* Which indexer a grab belongs to is decided from the `indexerId` sent and the + release URL's host: a pinned `indexerId` always wins; an unpinned one is taken + at face value if its host matches the URL (and refused if it contradicts the + URL's host while a pinned indexer owns that host); an unknown or missing id is + treated as no id, and then the indexers on the URL's host decide (none: + unpinned, one: that indexer, several with one pinned: refused). Grabs from the + Bindery UI and the scheduler always send the indexer. +* An id-less API grab can only be tied to a pinned indexer by host. The match + ignores case, a trailing dot and the port, so `prowlarr` against + `prowlarr:9696` works. It does not match aliases of one machine (`localhost`, + `127.0.0.1` and a DNS name), `www.` or other subdomains, scheme-less indexer + URLs or punycode spellings; such a grab is routed as if unpinned. Send + `indexerId` to be sure the pin applies. * On a shared Prowlarr or Jackett host, one pinned indexer makes id-less API - grabs fail for every indexer on that host, because such a grab cannot be told - apart. Send `indexerId` (the UI and the scheduler already do). + grabs fail for every other indexer on that host, because such a grab cannot be + told apart. Send `indexerId` (the UI and the scheduler already do). * A pending (held) release from a pinned indexer whose client is down stays in Pending and is tried again on the next sweep. It is not discarded. diff --git a/internal/api/grab_pin.go b/internal/api/grab_pin.go new file mode 100644 index 000000000..7c13f16d4 --- /dev/null +++ b/internal/api/grab_pin.go @@ -0,0 +1,125 @@ +package api + +import ( + "strings" + + "github.com/vavallee/bindery/internal/indexer/newznab" + "github.com/vavallee/bindery/internal/models" +) + +// grabPinKind is the outcome of resolveGrabPin. +type grabPinKind int + +const ( + // pinNoInfo: nothing identifies an indexer and nothing on the host calls for + // action. The grab is unpinned and routed as it always was. Signing may still + // attribute an indexer afterwards, for the seed limits. + pinNoInfo grabPinKind = iota + // pinUseIndexer: the grab is attributed to grabPinDecision.IndexerID, and that + // indexer's pin, or lack of one, decides the client. Nothing downstream may + // replace it. + pinUseIndexer + // pinRefuse: the grab cannot safely be routed and is refused. + pinRefuse +) + +// Reasons for a refusal. +const ( + pinRefuseAmbiguous = "ambiguous" + pinRefuseContradiction = "contradiction" +) + +// grabPinDecision is the one answer to "which indexer does this grab belong to, +// and may it be sent". +type grabPinDecision struct { + Kind grabPinKind + IndexerID int64 // pinUseIndexer + Reason string // pinRefuse + Pinned []string // pinRefuse: the pinned indexers that made it unsafe +} + +// resolveGrabPin decides, from the caller's indexer id (nil when none), the +// release URL and the full indexer list, which indexer a grab belongs to for the +// purpose of a download client pin (#3080). It does no signing and no IO. The +// rules, in order: +// +// a. A supplied id that exists and is pinned wins, whatever the host. +// b. A supplied id that exists and is unpinned is trusted when its host +// matches the release URL's host. When it does not match (a contradiction) +// and any pinned indexer does match the URL's host, the grab is refused. +// Otherwise the supplied indexer stands. +// c. A supplied id that matches no indexer is treated as no id. +// d. With no usable id, the indexers whose host matches the URL decide: none +// gives no information; exactly one is the release's indexer; several are +// refused when any of them is pinned (the release cannot be told apart, and +// signing's pick is a guess) and otherwise give no information. +// +// Host matching is newznab.DownloadHostMatchesIndexer: case, one trailing dot +// and the port are ignored. +func resolveGrabPin(callerID *int64, releaseURL string, idxs []models.Indexer) grabPinDecision { + matches := func(idx models.Indexer) bool { + return newznab.DownloadHostMatchesIndexer(releaseURL, idx.URL) + } + var matching, pinnedOnHost []models.Indexer + var supplied *models.Indexer + for i := range idxs { + if callerID != nil && idxs[i].ID == *callerID { + supplied = &idxs[i] + } + if matches(idxs[i]) { + matching = append(matching, idxs[i]) + if idxs[i].DownloadClientID != nil { + pinnedOnHost = append(pinnedOnHost, idxs[i]) + } + } + } + names := func(list []models.Indexer) []string { + out := make([]string, 0, len(list)) + for _, idx := range list { + out = append(out, idx.Name) + } + return out + } + + if supplied != nil { + if supplied.DownloadClientID != nil || matches(*supplied) || len(pinnedOnHost) == 0 { + return grabPinDecision{Kind: pinUseIndexer, IndexerID: supplied.ID} + } + return grabPinDecision{Kind: pinRefuse, Reason: pinRefuseContradiction, Pinned: names(pinnedOnHost)} + } + + switch { + case len(matching) == 0: + return grabPinDecision{Kind: pinNoInfo} + case len(matching) == 1: + return grabPinDecision{Kind: pinUseIndexer, IndexerID: matching[0].ID} + case len(pinnedOnHost) > 0: + return grabPinDecision{Kind: pinRefuse, Reason: pinRefuseAmbiguous, Pinned: names(pinnedOnHost)} + default: + return grabPinDecision{Kind: pinNoInfo} + } +} + +// ambiguousPinError refuses a grab that cannot safely be routed because of a +// pinned indexer (#3080). Sending it to the default client could put a private +// tracker's release on the public client, so it is not sent. +type ambiguousPinError struct { + reason string + pinned []string +} + +func (e *ambiguousPinError) Error() string { return e.Message(true) } + +// Message names the pinned indexers only for an admin. +func (e *ambiguousPinError) Message(includeNames bool) string { + names := "" + if includeNames && len(e.pinned) > 0 { + names = " (" + strings.Join(e.pinned, ", ") + ")" + } + if e.reason == pinRefuseContradiction { + return "the indexer named on this grab does not match the release's host, and an indexer on that host is pinned to a download client" + names + + ". It was not sent to any client. Grab it from the search results so the right indexer is known." + } + return "this release could not be tied to one indexer, and an indexer on its host is pinned to a download client" + names + + ". It was not sent to any client. Grab it from the search results so the indexer is known." +} diff --git a/internal/api/grab_pin_test.go b/internal/api/grab_pin_test.go new file mode 100644 index 000000000..bfd6a8d0d --- /dev/null +++ b/internal/api/grab_pin_test.go @@ -0,0 +1,130 @@ +package api + +import ( + "reflect" + "testing" + + "github.com/vavallee/bindery/internal/models" +) + +// The release URL used throughout is on host.example. Indexer ids: +// +// 1 U1 unpinned, on the URL host +// 3 U2 unpinned, on the URL host +// 2 P pinned, on the URL host +// 10 O unpinned, on another host +// 20 PX pinned, on another host +const decisionReleaseURL = "http://host.example/dl/1.nzb" + +func decisionIndexers(keyed bool, set string) []models.Indexer { + pin := int64(7) + key := "" + if keyed { + key = "secret" + } + u1 := models.Indexer{ID: 1, Name: "U1", URL: "http://host.example/api", APIKey: key} + u2 := models.Indexer{ID: 3, Name: "U2", URL: "http://host.example:9696/api2", APIKey: key} + p := models.Indexer{ID: 2, Name: "P", URL: "http://host.example/api3", APIKey: key, DownloadClientID: &pin} + o := models.Indexer{ID: 10, Name: "O", URL: "http://other.example/api", APIKey: key} + px := models.Indexer{ID: 20, Name: "PX", URL: "http://elsewhere.example/api", APIKey: key, DownloadClientID: &pin} + switch set { + case "none": + return []models.Indexer{o, px} + case "onlyUnpinned": + return []models.Indexer{u1, o, px} + case "onlyPinned": + return []models.Indexer{p, o, px} + case "both": + return []models.Indexer{u1, p, o, px} + case "severalOnePinned": + return []models.Indexer{u1, u2, p, o, px} + case "severalNonePinned": + return []models.Indexer{u1, u2, o, px} + } + panic("unknown set " + set) +} + +// TestResolveGrabPin is the whole decision matrix: the caller's id (absent, +// pinned, unpinned on the URL host, unpinned elsewhere, unknown) against what the +// URL's host matches (nothing, only unpinned indexers, only a pinned one, both, +// several with one pinned, several none pinned), each for pinned indexers with +// and without a stored key. The key plays no part in the decision, so the two +// must always agree. +func TestResolveGrabPin(t *testing.T) { + id := func(v int64) *int64 { return &v } + use := func(v int64) grabPinDecision { return grabPinDecision{Kind: pinUseIndexer, IndexerID: v} } + noInfo := grabPinDecision{Kind: pinNoInfo} + ambiguous := grabPinDecision{Kind: pinRefuse, Reason: pinRefuseAmbiguous, Pinned: []string{"P"}} + contradiction := grabPinDecision{Kind: pinRefuse, Reason: pinRefuseContradiction, Pinned: []string{"P"}} + + cases := []struct { + name string + caller *int64 + set string + want grabPinDecision + }{ + // No id supplied (rule d). + {"no id, host matches nothing", nil, "none", noInfo}, + {"no id, only an unpinned indexer on the host", nil, "onlyUnpinned", use(1)}, + {"no id, only a pinned indexer on the host", nil, "onlyPinned", use(2)}, + {"no id, a pinned and an unpinned indexer share the host", nil, "both", ambiguous}, + {"no id, several on the host and one pinned", nil, "severalOnePinned", ambiguous}, + {"no id, several on the host and none pinned", nil, "severalNonePinned", noInfo}, + + // A supplied id that exists and is pinned always wins (rule a). + {"pinned id elsewhere, host matches nothing", id(20), "none", use(20)}, + {"pinned id elsewhere, only unpinned on the host", id(20), "onlyUnpinned", use(20)}, + {"pinned id elsewhere, only a pinned on the host", id(20), "onlyPinned", use(20)}, + {"pinned id elsewhere, both on the host", id(20), "both", use(20)}, + {"pinned id elsewhere, several on the host one pinned", id(20), "severalOnePinned", use(20)}, + {"pinned id elsewhere, several on the host none pinned", id(20), "severalNonePinned", use(20)}, + {"pinned id on the host, shared with unpinned", id(2), "both", use(2)}, + {"pinned id on the host, shared with several", id(2), "severalOnePinned", use(2)}, + + // A supplied unpinned id whose host matches the URL is trusted (rule b). + {"unpinned id on the host, alone", id(1), "onlyUnpinned", use(1)}, + {"unpinned id on the host, sharing it with a pinned", id(1), "both", use(1)}, + {"unpinned id on the host, several one pinned", id(1), "severalOnePinned", use(1)}, + {"unpinned id on the host, several none pinned", id(1), "severalNonePinned", use(1)}, + + // A supplied unpinned id elsewhere contradicts the URL (rule b). + {"unpinned id elsewhere, host matches nothing", id(10), "none", use(10)}, + {"unpinned id elsewhere, only unpinned on the host", id(10), "onlyUnpinned", use(10)}, + {"unpinned id elsewhere, only a pinned on the host", id(10), "onlyPinned", contradiction}, + {"unpinned id elsewhere, both on the host", id(10), "both", contradiction}, + {"unpinned id elsewhere, several one pinned", id(10), "severalOnePinned", contradiction}, + {"unpinned id elsewhere, several none pinned", id(10), "severalNonePinned", use(10)}, + + // An id that matches no indexer is no id at all (rule c). + {"unknown id, host matches nothing", id(999), "none", noInfo}, + {"unknown id, only an unpinned on the host", id(999), "onlyUnpinned", use(1)}, + {"unknown id, only a pinned on the host", id(999), "onlyPinned", use(2)}, + {"unknown id, both on the host", id(999), "both", ambiguous}, + {"unknown id, several one pinned", id(999), "severalOnePinned", ambiguous}, + {"unknown id, several none pinned", id(999), "severalNonePinned", noInfo}, + } + for _, keyed := range []bool{true, false} { + for _, tc := range cases { + name := tc.name + ", keyless" + if keyed { + name = tc.name + ", keyed" + } + t.Run(name, func(t *testing.T) { + got := resolveGrabPin(tc.caller, decisionReleaseURL, decisionIndexers(keyed, tc.set)) + if !reflect.DeepEqual(got, tc.want) { + t.Errorf("resolveGrabPin = %+v, want %+v", got, tc.want) + } + }) + } + } +} + +// With no indexers at all the decision has nothing to say. +func TestResolveGrabPin_NoIndexers(t *testing.T) { + one := int64(1) + for _, caller := range []*int64{nil, &one} { + if got := resolveGrabPin(caller, decisionReleaseURL, nil); got.Kind != pinNoInfo { + t.Errorf("caller %v: %+v, want no information", caller, got) + } + } +} diff --git a/internal/api/indexer_pin_leaks_test.go b/internal/api/indexer_pin_leaks_test.go new file mode 100644 index 000000000..27234e1cd --- /dev/null +++ b/internal/api/indexer_pin_leaks_test.go @@ -0,0 +1,121 @@ +package api + +import ( + "net/http" + "strings" + "testing" + + "github.com/vavallee/bindery/internal/models" +) + +func (f *pinFixture) wantPinRefusal(t *testing.T, status int, body string) { + t.Helper() + if status != http.StatusBadRequest || !strings.Contains(body, codeIndexerClientUnavailable) { + t.Fatalf("status %d, want 400 %s: %s", status, codeIndexerClientUnavailable, body) + } + if f.pub.count() != 0 || f.priv.count() != 0 { + t.Fatalf("a refused grab reached a client: public=%d private=%d", f.pub.count(), f.priv.count()) + } +} + +// Leak 1: the caller names the pinned indexer Z, which has no stored key, while +// a keyed unpinned indexer Y shares the URL host. Signing used to re-attribute +// the grab to Y and Z's own pin was ignored. +func TestGrab_SuppliedPinnedIDWinsOverASigningAttributionToAnotherIndexer(t *testing.T) { + f := newPinFixture(t) + f.idx.APIKey = "" + f.pin(t, &f.privC.ID) + f.addIndexer(t, "TrackerY", "/apiy", nil) + w := f.grabURL("g-leak1", f.relURL, &f.idx.ID, false) + f.wantReached(t, w, 0, 1) +} + +// Leak 1, second form: the pinned indexer is on another host altogether and the +// URL's host has a keyed unpinned indexer. +func TestGrab_SuppliedPinnedIDOnAnotherHostStillAppliesItsPin(t *testing.T) { + f := newPinFixture(t) + f.idx.URL = otherHostURL(f.pub.srv.URL) + "/api" + f.idx.APIKey = "k" + f.pin(t, &f.privC.ID) + // Y is on the release's host, keyed and unpinned. + y := &models.Indexer{Name: "TrackerY", Type: "newznab", URL: strings.TrimSuffix(f.relURL, "/rel.nzb") + "/apiy", APIKey: "secret", Enabled: true} + if err := f.indexers.Create(f.ctx, y); err != nil { + t.Fatal(err) + } + w := f.grabURL("g-leak1b", f.relURL, &f.idx.ID, false) + f.wantReached(t, w, 0, 1) +} + +// Leak 2: keyless pinned Z and keyed unpinned Y share the URL host, and the +// caller names an unpinned X on another host. Y signs, which used to excuse the +// refusal. +func TestGrab_ContradictingIDIsRefusedEvenWhenAnotherIndexerCanSignTheURL(t *testing.T) { + f := newPinFixture(t) + f.idx.APIKey = "" + f.pin(t, &f.privC.ID) + f.addIndexer(t, "TrackerY", "/apiy", nil) + x := &models.Indexer{Name: "TrackerX2", Type: "newznab", URL: otherHostURL(f.pub.srv.URL) + "/api", APIKey: "k", Enabled: true} + if err := f.indexers.Create(f.ctx, x); err != nil { + t.Fatal(err) + } + w := f.grabURL("g-leak2", f.relURL, &x.ID, false) + f.wantPinRefusal(t, w.Code, w.Body.String()) +} + +// Leak 3: an id that matches no indexer is no id at all. With one pinned indexer +// on the URL's host it is that indexer's grab; with a pinned one among several it +// is refused. +func TestGrab_UnknownSuppliedIDIsTreatedAsNoID(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) + missing := int64(98765) + w := f.grabURL("g-leak3a", f.relURL, &missing, false) + f.wantReached(t, w, 0, 1) + + f2 := newPinFixture(t) + f2.pin(t, &f2.privC.ID) + f2.addIndexer(t, "TrackerY", "/apiy", nil) + w = f2.grabURL("g-leak3b", f2.relURL, &missing, false) + f2.wantPinRefusal(t, w.Code, w.Body.String()) +} + +// Leak 4, the opposite direction: a keyed pinned indexer and a keyless unpinned +// one share a host. An id-less grab cannot be told apart, and used to go to the +// pinned indexer's client on signing's pick. +func TestGrab_AmbiguousHostIsRefusedNotRoutedBySigningsPick(t *testing.T) { + f := newPinFixture(t) + f.pin(t, &f.privC.ID) // TrackerX: keyed, pinned + u := f.addIndexer(t, "TrackerU", "/apiu", nil) + u.APIKey = "" + if err := f.indexers.Update(f.ctx, u); err != nil { + t.Fatal(err) + } + w := f.grabURL("g-leak4", f.relURL, nil, false) + f.wantPinRefusal(t, w.Code, w.Body.String()) +} + +// The row records the decided indexer, not signing's pick. +func TestGrab_DecidedIndexerIsRecordedOnTheRow(t *testing.T) { + f := newPinFixture(t) + f.idx.APIKey = "" + f.pin(t, &f.privC.ID) + f.addIndexer(t, "TrackerY", "/apiy", nil) + w := f.grabURL("g-row", f.relURL, &f.idx.ID, false) + f.wantReached(t, w, 0, 1) + d := f.downloadFor(t, "g-row") + if d.IndexerID == nil || *d.IndexerID != f.idx.ID { + t.Errorf("row indexer = %v, want the pinned indexer %d", d.IndexerID, f.idx.ID) + } +} + +// Regression guard: with no pinned indexer anywhere, a supplied unpinned id +// whose host does not match the URL still goes the default way. +func TestGrab_NoPinnedIndexersAnywhereIsUnchanged(t *testing.T) { + f := newPinFixture(t) + x := &models.Indexer{Name: "TrackerX2", Type: "newznab", URL: otherHostURL(f.pub.srv.URL) + "/api", APIKey: "k", Enabled: true} + if err := f.indexers.Create(f.ctx, x); err != nil { + t.Fatal(err) + } + w := f.grabURL("g-nopins", f.relURL, &x.ID, false) + f.wantReached(t, w, 1, 0) +} diff --git a/internal/api/indexer_pin_type_test.go b/internal/api/indexer_pin_type_test.go index 40d4f5e03..78ea4e86b 100644 --- a/internal/api/indexer_pin_type_test.go +++ b/internal/api/indexer_pin_type_test.go @@ -6,6 +6,26 @@ import ( "testing" ) +// An explicit empty type in an update body keeps the stored type. Without this +// a torznab indexer saved with "" would be read as usenet and flip protocol. +func TestIndexerUpdate_EmptyTypeKeepsTheStoredType(t *testing.T) { + f := newPinAPIFixture(t) + torrentID := strconv.FormatInt(f.torrent.ID, 10) + created := decodeIndexer(t, f.create(`{"name":"Priv","url":"http://192.168.1.50:9117/api","type":"torznab","downloadClientId":`+torrentID+`}`)) + rec := f.update(created.ID, `{"type":"","priority":4}`) + if rec.Code != http.StatusOK { + t.Fatalf("update: %d %s", rec.Code, rec.Body.String()) + } + got := decodeIndexer(t, rec) + if got.Type != "torznab" || got.Priority != 4 || got.DownloadClientID == nil { + t.Errorf("type = %q priority = %d pin = %v, want torznab, 4 and the pin kept", got.Type, got.Priority, got.DownloadClientID) + } + stored, _ := f.ih.indexers.GetByID(t.Context(), created.ID) + if stored.Type != "torznab" { + t.Errorf("stored type = %q", stored.Type) + } +} + // An update body with an empty type means the default type (usenet), which is // what the searcher already reads it as. For an indexer that is already usenet // it must not count as a change of type, so a pinned client that was switched diff --git a/internal/api/indexers.go b/internal/api/indexers.go index 47d4c378b..7aa134950 100644 --- a/internal/api/indexers.go +++ b/internal/api/indexers.go @@ -461,6 +461,11 @@ func (h *IndexerHandler) Update(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid request body"}) return } + // An explicit empty type means "keep the stored type": saved as empty it + // would be read as usenet and flip a torznab indexer's protocol. + if idx.Type == "" { + idx.Type = existing.Type + } // The API key is write-only, so it needs the raw body: the struct decode // above cannot tell an omitted key from an explicitly blank one, and the // web app sends the redacted (blank) value straight back on every save. diff --git a/internal/api/queue.go b/internal/api/queue.go index 7b80c9677..d7cde6717 100644 --- a/internal/api/queue.go +++ b/internal/api/queue.go @@ -114,24 +114,6 @@ func isAdminCtx(ctx context.Context) bool { // nudge, which would be wrong when clients exist and the pin is the problem. const codeIndexerClientUnavailable = "indexer_client_unavailable" -// ambiguousPinError refuses a grab that arrived without an indexer id and that -// could not be tied to one indexer, while an indexer on the release's host is -// pinned to a download client (#3080). Sending it to the default client could -// put a private tracker's release on the public client, so it is not sent. -type ambiguousPinError struct{ pinned []string } - -func (e *ambiguousPinError) Error() string { return e.Message(true) } - -// Message names the pinned indexers only for an admin. -func (e *ambiguousPinError) Message(includeNames bool) string { - names := "" - if includeNames && len(e.pinned) > 0 { - names = " (" + strings.Join(e.pinned, ", ") + ")" - } - return "this release could not be tied to one indexer, and an indexer on its host is pinned to a download client" + names + - ". It was not sent to any client. Grab it from the search results so the indexer is known." -} - // pinnedMessage renders a pin failure for the caller's role. func pinnedMessage(ctx context.Context, perr *db.PinnedClientError) string { return perr.Message(isAdminCtx(ctx)) @@ -1386,86 +1368,6 @@ func (h *QueueHandler) selectClient(ctx context.Context, protocol, mediaType str return db.PickClientForMediaType(candidates, mediaType), nil } -// pinnedIndexerOnHost checks, for a grab, whether the release could have come -// from an indexer pinned to a download client that the grab is not already tied -// to (#3080). The indexers considered are those on the release URL's host -// (newznab.DownloadHostMatchesIndexer). It returns the indexer whose pin should -// apply when that is unambiguous, or an *ambiguousPinError when the grab must be -// refused, or (nil, nil) when nothing about the host calls for action. -// -// - The caller named no indexer (callerID nil). Signing may still have -// attributed one (signedByHost), but it only counts indexers that can sign. -// Exactly one indexer on the host, pinned and not otherwise attributed, is -// the release's indexer. Otherwise any pinned indexer on the host that is not -// the attributed one makes the grab ambiguous, and it is refused. -// - The caller named an indexer. A truthful id, one whose host matches the -// release, is taken at face value, even when a pinned indexer shares the -// host. An id that contradicts the host is refused when an indexer on the -// release's host is pinned, unless that id is itself pinned (its own pin -// applies) or signing found the host's indexer (signedByHost). -// -// A host that matches no indexer carries nothing to act on and is left unpinned: -// send the indexer id to have the pin apply. -func (h *QueueHandler) pinnedIndexerOnHost(ctx context.Context, rawURL string, callerID, currentID *int64, signedByHost bool) (*models.Indexer, error) { - if h.indexers == nil || rawURL == "" { - return nil, nil - } - idxs, err := h.indexers.List(ctx) - if err != nil { - return nil, err - } - var matching []models.Indexer - var pinnedOnHost []models.Indexer - var supplied *models.Indexer - for i := range idxs { - idx := idxs[i] - if callerID != nil && idx.ID == *callerID { - supplied = &idxs[i] - } - if !newznab.DownloadHostMatchesIndexer(rawURL, idx.URL) { - continue - } - matching = append(matching, idx) - if idx.DownloadClientID != nil { - pinnedOnHost = append(pinnedOnHost, idx) - } - } - if len(pinnedOnHost) == 0 { - return nil, nil - } - names := func(list []models.Indexer) []string { - out := make([]string, 0, len(list)) - for _, idx := range list { - out = append(out, idx.Name) - } - return out - } - - if callerID != nil { - if signedByHost || supplied == nil || supplied.DownloadClientID != nil { - return nil, nil - } - if newznab.DownloadHostMatchesIndexer(rawURL, supplied.URL) { - return nil, nil - } - return nil, &ambiguousPinError{pinned: names(pinnedOnHost)} - } - - if len(matching) == 1 && (currentID == nil || *currentID == matching[0].ID) { - return &matching[0], nil - } - var others []models.Indexer - for _, idx := range pinnedOnHost { - if currentID == nil || idx.ID != *currentID { - others = append(others, idx) - } - } - if len(others) == 0 { - return nil, nil - } - return nil, &ambiguousPinError{pinned: names(others)} -} - // clientForGrab picks the client for a grab. An indexer pinned to a download // client (#3080) gets exactly that client, whatever the protocol routing, media // type or priority would have chosen, or a *db.PinnedClientError when it cannot @@ -1663,33 +1565,39 @@ func (h *QueueHandler) grab(ctx context.Context, req grabRequest) (*models.Downl // hands back an apikey-less download URL plus the indexer id; sign it // server-side here. No-op when the URL already carries a key (scheduler / // retry paths) or its host does not match the indexer. - // callerIndexerID is the indexer the caller (or the stored row, on a - // retry) named, before anything below attributes the release by host. - var callerIndexerID *int64 - if indexerID != nil { - v := *indexerID - callerIndexerID = &v - } - nzbURL, hostMatched := h.signNZBURL(ctx, req.NZBURL, indexerID) - // The host-match fallback knows which indexer signed the URL, so record it. - // Without this the row keeps a nil (or dangling) IndexerID for precisely the - // API callers the fallback exists for, and both the queue's indexer - // attribution and resolveSeedLimits' per-indexer overrides are lost (#2053). - if hostMatched != nil { + // One decision says which indexer the grab belongs to, before signing gets + // a say (#3080). Signing only counts indexers that can sign and picks by + // luck of who holds a key, so it may fill in an attribution the decision + // left open but never replaces or overrides one. See resolveGrabPin. + var decision grabPinDecision + if h.indexers != nil { + idxs, err := h.indexers.List(ctx) + if err != nil { + return nil, err + } + decision = resolveGrabPin(indexerID, req.NZBURL, idxs) + } + if decision.Kind == pinRefuse { + return nil, &ambiguousPinError{reason: decision.Reason, pinned: decision.Pinned} + } + signWith := indexerID + if decision.Kind == pinUseIndexer { + id := decision.IndexerID + signWith = &id + } + nzbURL, hostMatched := h.signNZBURL(ctx, req.NZBURL, signWith) + if decision.Kind == pinUseIndexer { + // The decided indexer is the row's indexer, whoever signed the URL. + id := decision.IndexerID + indexerID = &id + } else if hostMatched != nil { + // Nothing decided: the host-match fallback knows which indexer signed + // the URL, so record it. Without this the row keeps a nil (or dangling) + // IndexerID for precisely the API callers the fallback exists for, and + // both the queue's indexer attribution and resolveSeedLimits' + // per-indexer overrides are lost (#2053). indexerID = hostMatched } - // Signing cannot attribute a pre signed URL, a keyless indexer or two - // indexers sharing a host, and it only counts indexers that can sign, so - // an id-less grab may be tied to an unpinned indexer while a pinned one on - // the same host is the real source (#3080). Look at every indexer on the - // release's host before the default routing is allowed to pick a client. - onHost, err := h.pinnedIndexerOnHost(ctx, req.NZBURL, callerIndexerID, indexerID, hostMatched != nil) - if err != nil { - return nil, err - } - if onHost != nil { - indexerID = &onHost.ID - } // The client is chosen only now, after the host match above has settled // which indexer the release came from: an indexer pinned to a download