From 484a81f7794c5d8858d7e1176ceff22c69a9b5f3 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 21 Oct 2026 11:31:57 -0400 Subject: [PATCH] fix: a release its download client will not let Minstrel follow is held back, not taken every pass (#6176) The Black Keys' Ohio Players was grabbed 40 times in a day. FabledExchange already held the torrent under another category from before Minstrel took the downloads over: the add answered "duplicate", which counted as added, and Exchange's listing, filtered to Minstrel's category, never showed it. Half an hour later the grab ended gone, and the next pass took it again. - A duplicate add that the client does not list for Minstrel is not a grab: the release is held back for a day with the client's own reason (already downloading X (category)), and the search moves on. Forced, it is refused with that reason. - A release that ends gone a second time within a week is held back for a day, whatever the cause. - release_blocks gains the reason 'gone' (migration 0098), shown as "Held back: its download client lost track of it". Co-Authored-By: Claude Opus 5.5 --- internal/acquire/grab.go | 34 +++++++++++++- internal/acquire/memory.go | 15 ++++++ internal/acquire/search_test.go | 28 +++++++++++ internal/db/dbq/lifecycle.sql.go | 19 ++++++++ .../0098_release_blocks_gone.down.sql | 4 ++ .../0098_release_blocks_gone.up.sql | 6 +++ internal/db/queries/lifecycle.sql | 6 +++ internal/lifecycle/lifecycle.go | 27 ++++++++++- internal/lifecycle/lifecycle_test.go | 47 +++++++++++++++++++ web/src/lib/api/types.ts | 2 +- .../lib/components/wanted/BlockList.svelte | 1 + 11 files changed, 186 insertions(+), 3 deletions(-) create mode 100644 internal/db/migrations/0098_release_blocks_gone.down.sql create mode 100644 internal/db/migrations/0098_release_blocks_gone.up.sql diff --git a/internal/acquire/grab.go b/internal/acquire/grab.go index 6186ebd9..20b106af 100644 --- a/internal/acquire/grab.go +++ b/internal/acquire/grab.go @@ -6,6 +6,7 @@ import ( "path" "strings" "sync" + "time" "github.com/jackc/pgx/v5/pgtype" @@ -152,7 +153,25 @@ func (s *Service) Grab(ctx context.Context, want Want, chosen *Candidate, all [] } add.SavePath = landingFor(s.Grabber.Landing, hash, chosen.Name) - if err := s.Grabber.Client.AddTorrent(ctx, add); err != nil && !errors.Is(err, downloader.ErrDuplicate) { + err := s.Grabber.Client.AddTorrent(ctx, add) + if errors.Is(err, downloader.ErrDuplicate) && s.heldElsewhere(ctx, hash) { + why := "Already in the download client outside Minstrel's category, where Minstrel cannot follow it (" + + strings.TrimPrefix(indexer.FirstLine(err.Error()), downloader.ErrDuplicate.Error()+": ") + ")" + if force { + out.Error = why + "." + return out + } + set(chosen, Blocked, Hard, CodeBlocked, why+".") + if s.Memory != nil && hash != "" { + _ = s.Memory.Block(ctx, dbq.BlockReleaseParams{ + InfoHash: hash, Name: chosen.Name, Reason: BlockGone, Detail: why + ".", WantID: want.ID, + ExpiresAt: pgtype.Timestamptz{Time: time.Now().Add(GoneBlock), Valid: true}, + }) + } + out.Refused = true + return out + } + if err != nil && !errors.Is(err, downloader.ErrDuplicate) { if errors.Is(err, downloader.ErrUnconfigured) { out.Unconfigured, out.Missing = true, MissingClient } else { @@ -191,6 +210,19 @@ func (s *Service) Grab(ctx context.Context, want Want, chosen *Candidate, all [] return out } +// heldElsewhere reports whether a release the client says it already holds is +// one it does not list for Minstrel: held under another category, from before +// Minstrel took the downloads over (#6176). Recorded as a grab, it would read +// as gone half an hour later and be taken again, every pass. A client that +// cannot answer now is given the benefit of the doubt, as the add was. +func (s *Service) heldElsewhere(ctx context.Context, hash string) bool { + if hash == "" { + return false + } + listed, err := s.Grabber.Client.Torrents(ctx, indexer.NormalizeHash(hash)) + return err == nil && len(listed) == 0 +} + // judgeFiles judges a chosen release again on its .torrent's file list, and // sets it aside when the files say it is not the album, so no later search // offers it again. force still takes it. diff --git a/internal/acquire/memory.go b/internal/acquire/memory.go index 9f408de8..7d2eac24 100644 --- a/internal/acquire/memory.go +++ b/internal/acquire/memory.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "time" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" @@ -24,8 +25,17 @@ const ( // BlockStalled: given up on because it would not finish (#5523); held // back until the block's expires_at. BlockStalled = "stalled" + // BlockGone: its download client would not let Minstrel follow it, held + // outside Minstrel's category or leaving the client twice (#6176); held + // back for GoneBlock. + BlockGone = "gone" ) +// GoneBlock is how long a release its client lost track of is held back: +// long enough that a release which keeps vanishing is tried once a day rather +// than on every pass, short enough that it is taken once the client lets go. +const GoneBlock = 24 * time.Hour + // Grab origins, matching grabs_origin_check. const ( OriginSchedule = "schedule" @@ -106,6 +116,11 @@ func BlockSentence(reason, detail string) string { return "Refused on import: " + detail } return "Refused on import: its files were not the album." + case BlockGone: + if detail != "" { + return "Held back for now: " + detail + } + return "Held back for now: its download client lost track of it." case BlockStalled: if detail != "" { return "Given up on for now: " + detail diff --git a/internal/acquire/search_test.go b/internal/acquire/search_test.go index b074e694..85ea3cf5 100644 --- a/internal/acquire/search_test.go +++ b/internal/acquire/search_test.go @@ -3,6 +3,7 @@ package acquire import ( "context" "errors" + "fmt" "strings" "testing" "time" @@ -153,6 +154,8 @@ type fakeClient struct { addErr error deleted []string progress float64 + // elsewhere: the client holds it but does not list it for Minstrel. + elsewhere bool } func (c *fakeClient) AddTorrent(_ context.Context, a downloader.Add) error { @@ -161,6 +164,9 @@ func (c *fakeClient) AddTorrent(_ context.Context, a downloader.Add) error { } func (c *fakeClient) Torrents(_ context.Context, hashes ...string) ([]downloader.Torrent, error) { + if c.elsewhere { + return nil, nil + } return []downloader.Torrent{{Hash: hashes[0], Progress: c.progress}}, nil } @@ -212,6 +218,28 @@ func TestGrab_AClientThatAlreadyHasItCounts(t *testing.T) { } } +// A duplicate the client does not list for Minstrel sits under another +// category, where it would read as gone half an hour later (#6176). It is set +// aside rather than taken; forced, it is refused with the client's reason. +func TestGrab_ADuplicateHeldOutsideMinstrelIsNotTaken(t *testing.T) { + client := &fakeClient{addErr: fmt.Errorf("%w: already downloading Ohio Players (lidarr)", downloader.ErrDuplicate), elsewhere: true} + s := &Service{Grabber: &Downloader{Client: client, Landing: "/downloads"}} + c := Candidate{Verdict: Grab, InfoHash: "aaaa", Source: indexer.Result{Magnet: "magnet:?xt=urn:btih:aaaa"}} + took := s.Grab(context.Background(), ram, &c, nil, false, Origin{By: OriginSchedule}) + if took.Added || !took.Refused { + t.Fatalf("took %+v", took) + } + if c.Verdict != Blocked || !strings.Contains(c.Reason, "(already downloading Ohio Players (lidarr))") { + t.Errorf("candidate %s: %q", c.Verdict, c.Reason) + } + + forced := Candidate{Verdict: Grab, InfoHash: "aaaa", Source: indexer.Result{Magnet: "magnet:?xt=urn:btih:aaaa"}} + took = s.Grab(context.Background(), ram, &forced, nil, true, Origin{By: OriginOperator}) + if took.Added || !strings.Contains(took.Error, "outside Minstrel's category") { + t.Errorf("forced: %+v", took) + } +} + func TestBetter_QualityThenSeedersThenYear(t *testing.T) { flac := Candidate{Verdict: Grab, Tier: release.TierLossless, Seeders: 2} mp3 := Candidate{Verdict: Grab, Tier: release.TierLossyHigh, Seeders: 90} diff --git a/internal/db/dbq/lifecycle.sql.go b/internal/db/dbq/lifecycle.sql.go index 7e02d817..36771595 100644 --- a/internal/db/dbq/lifecycle.sql.go +++ b/internal/db/dbq/lifecycle.sql.go @@ -11,6 +11,25 @@ import ( "github.com/jackc/pgx/v5/pgtype" ) +const countGoneGrabs = `-- name: CountGoneGrabs :one +SELECT count(*) FROM grabs + WHERE info_hash = $1 AND end_reason = 'gone' AND ended_at > $2 +` + +type CountGoneGrabsParams struct { + InfoHash string + Since pgtype.Timestamptz +} + +// How many of a release's grabs ended gone since a time: a release that keeps +// leaving its client is held back rather than taken on every pass (#6176). +func (q *Queries) CountGoneGrabs(ctx context.Context, arg CountGoneGrabsParams) (int64, error) { + row := q.db.QueryRow(ctx, countGoneGrabs, arg.InfoHash, arg.Since) + var count int64 + err := row.Scan(&count) + return count, err +} + const countRemediation = `-- name: CountRemediation :one UPDATE download_interventions SET remediations = remediations + 1, diff --git a/internal/db/migrations/0098_release_blocks_gone.down.sql b/internal/db/migrations/0098_release_blocks_gone.down.sql new file mode 100644 index 00000000..97d45aea --- /dev/null +++ b/internal/db/migrations/0098_release_blocks_gone.down.sql @@ -0,0 +1,4 @@ +DELETE FROM release_blocks WHERE reason = 'gone'; +ALTER TABLE release_blocks DROP CONSTRAINT release_blocks_reason_check; +ALTER TABLE release_blocks ADD CONSTRAINT release_blocks_reason_check + CHECK (reason IN ('operator', 'files', 'import', 'stalled')); diff --git a/internal/db/migrations/0098_release_blocks_gone.up.sql b/internal/db/migrations/0098_release_blocks_gone.up.sql new file mode 100644 index 00000000..b07959e5 --- /dev/null +++ b/internal/db/migrations/0098_release_blocks_gone.up.sql @@ -0,0 +1,6 @@ +-- #6176: gone, a release its download client would not let Minstrel follow: +-- held outside Minstrel's category, or leaving the client twice. Held back +-- until expires_at, as stalled is. +ALTER TABLE release_blocks DROP CONSTRAINT release_blocks_reason_check; +ALTER TABLE release_blocks ADD CONSTRAINT release_blocks_reason_check + CHECK (reason IN ('operator', 'files', 'import', 'stalled', 'gone')); diff --git a/internal/db/queries/lifecycle.sql b/internal/db/queries/lifecycle.sql index ca634f2a..18f49932 100644 --- a/internal/db/queries/lifecycle.sql +++ b/internal/db/queries/lifecycle.sql @@ -77,6 +77,12 @@ UPDATE grabs WHERE id = sqlc.arg(id) AND ended_at IS NULL AND import_state = ANY(sqlc.arg(states)::text[]) RETURNING *; +-- name: CountGoneGrabs :one +-- How many of a release's grabs ended gone since a time: a release that keeps +-- leaving its client is held back rather than taken on every pass (#6176). +SELECT count(*) FROM grabs + WHERE info_hash = sqlc.arg(info_hash) AND end_reason = 'gone' AND ended_at > sqlc.arg(since); + -- name: ListSeedingGrabs :many -- Imported downloads still sharing, oldest import first. SELECT id, client, info_hash, name, imported_at FROM grabs diff --git a/internal/lifecycle/lifecycle.go b/internal/lifecycle/lifecycle.go index 79ad280a..4e173b52 100644 --- a/internal/lifecycle/lifecycle.go +++ b/internal/lifecycle/lifecycle.go @@ -75,6 +75,8 @@ const ( // goneAfter: a client restarting can answer without a download for a // while; half an hour missing is gone. goneAfter = 30 * time.Minute + // goneWindow: a release gone twice within it is held back (#6176). + goneWindow = 7 * 24 * time.Hour ) // running are the import states of a grab still downloading: the lifecycle @@ -441,12 +443,35 @@ func (s *Service) giveUp(ctx context.Context, q *dbq.Queries, d downloader.Drive } // endGone ends a grab whose download left the client, and searches again. -// The release is not held back: nothing was found wrong with it. +// The first time, the release is not held back: nothing was found wrong with +// it. A release that leaves its client again within goneWindow is held back for +// acquire.GoneBlock (#6176): whatever keeps the client from listing it, taking +// it again on every pass found 38 grabs of one release in a day. func (s *Service) endGone(ctx context.Context, q *dbq.Queries, g dbq.ListLiveGrabsRow, rep *Report) { detail := "The download client no longer lists it: removed outside Minstrel. The album is searched for again." + before, err := q.CountGoneGrabs(ctx, dbq.CountGoneGrabsParams{ + InfoHash: g.InfoHash, Since: pgtype.Timestamptz{Time: s.now().Add(-goneWindow), Valid: true}}) + if err != nil { + s.logger.Warn("lifecycle: count earlier gone grabs", "grab", g.Name, "err", err) + } + again := before > 0 + until := s.now().Add(acquire.GoneBlock) + if again { + detail = fmt.Sprintf("The download client no longer lists it, again: held back until %s; the album is searched for again.", + until.Format("2 Jan 15:04")) + } if _, err := q.GiveUpGrab(ctx, dbq.GiveUpGrabParams{ID: g.ID, EndReason: EndGone, EndDetail: detail, States: running}); err != nil { return } + if again { + if err := acquire.NewMemory(s.pool).Block(ctx, dbq.BlockReleaseParams{ + InfoHash: g.InfoHash, Name: g.Name, Reason: acquire.BlockGone, + Detail: "It left its download client twice without finishing.", + WantID: g.WantID, ExpiresAt: pgtype.Timestamptz{Time: until, Valid: true}, + }); err != nil { + s.logger.Warn("lifecycle: hold the release back", "grab", g.Name, "err", err) + } + } if g.WantID.Valid { if err := q.SearchWantAgain(ctx, g.WantID); err != nil { s.logger.Warn("lifecycle: search again", "grab", g.Name, "err", err) diff --git a/internal/lifecycle/lifecycle_test.go b/internal/lifecycle/lifecycle_test.go index f5cd4c60..61dc617e 100644 --- a/internal/lifecycle/lifecycle_test.go +++ b/internal/lifecycle/lifecycle_test.go @@ -6,6 +6,7 @@ import ( "io" "log/slog" "os" + "strings" "testing" "time" @@ -277,6 +278,52 @@ func TestLifecycle_ADownloadGoneFromTheClientEndsAfterHalfAnHour_Integration(t * } } +// A release that leaves its client a second time is held back, whatever the +// cause (#6176): one release was taken 40 times in a day, each ending gone. +func TestLifecycle_AReleaseGoneTwiceIsHeldBack_Integration(t *testing.T) { + st := newStack(t, downloader.Torrent{Name: "Daft Punk - RAM [FLAC]", Progress: 0.3, State: "downloading", NumSeeds: 4}) + ctx := context.Background() + delete(st.client.held, hash) + st.client.held[other] = downloader.Torrent{Hash: other, Name: "Someone else's", Progress: 1, State: "uploading"} + + st.s.Pass(ctx) + st.clock = st.clock.Add(31 * time.Minute) + if rep := st.s.Pass(ctx); rep.Gone != 1 { + t.Fatalf("first gone: %+v", rep) + } + if blocks, _ := st.q.ListReleaseBlocks(ctx); len(blocks) != 0 { + t.Fatalf("held back after one gone: %+v", blocks) + } + + again, err := acquire.NewMemory(st.s.pool).Grabbed(ctx, dbq.InsertGrabParams{WantID: st.want.ID, InfoHash: hash, + Name: "Daft Punk - RAM [FLAC]", Key: hash, Client: "qbittorrent", SavePath: "/downloads", Origin: acquire.OriginSchedule}, + pgtype.UUID{}, "") + if err != nil { + t.Fatal(err) + } + // first_seen is the database's clock and the test's runs ahead of it, so + // the second grab may end on either pass; count both. + gone := st.s.Pass(ctx).Gone + st.clock = st.clock.Add(31 * time.Minute) + if gone += st.s.Pass(ctx).Gone; gone != 1 { + t.Fatalf("second grab ended gone %d times, want 1", gone) + } + blocks, err := st.q.ListReleaseBlocks(ctx) + if err != nil { + t.Fatal(err) + } + if len(blocks) != 1 || blocks[0].Reason != acquire.BlockGone || !blocks[0].ExpiresAt.Valid { + t.Fatalf("blocks = %+v, want one timed gone block", blocks) + } + g, err := st.q.GetGrab(ctx, again.ID) + if err != nil { + t.Fatal(err) + } + if g.EndReason != EndGone || !strings.Contains(g.EndDetail, "held back until") { + t.Errorf("second grab %q: %s", g.EndReason, g.EndDetail) + } +} + func TestLifecycle_AnEmptyListIsNotAMeasurement_Integration(t *testing.T) { st := newStack(t, downloader.Torrent{Name: "Daft Punk - RAM [FLAC]", Progress: 0.3, State: "downloading"}) ctx := context.Background() diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index 1bd81638..5b8848e8 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -783,7 +783,7 @@ export type DownloadsPage = { export type ReleaseBlock = { info_hash: string; name: string; - reason: 'operator' | 'files' | 'import' | 'stalled'; + reason: 'operator' | 'files' | 'import' | 'stalled' | 'gone'; detail: string; blocked_at: string; // When a release given up on may be taken again; null is for good. diff --git a/web/src/lib/components/wanted/BlockList.svelte b/web/src/lib/components/wanted/BlockList.svelte index 33b80bd3..e1343cb8 100644 --- a/web/src/lib/components/wanted/BlockList.svelte +++ b/web/src/lib/components/wanted/BlockList.svelte @@ -37,6 +37,7 @@ if (b.reason === 'operator') return 'Set aside by you'; if (b.reason === 'files') return 'Set aside for its files'; if (b.reason === 'stalled') return 'Given up on: it would not finish'; + if (b.reason === 'gone') return 'Held back: its download client lost track of it'; return 'Refused on import'; }