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'; }