diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index c26615e0..2298f3fa 100644 --- a/cmd/minstrel/main.go +++ b/cmd/minstrel/main.go @@ -367,7 +367,9 @@ func run() error { // catalogue for the track list of an album whose wrong slot it must // decide, a few a pass. See internal/library/duplicate_resolve.go. go library.NewDuplicateResolveWorker(pool, logger.With("component", "duplicate_resolve"), - cfg.Storage.DataDir, fpSettings, cat.RefreshReleaseGroup).Run(ctx) + cfg.Storage.DataDir, fpSettings, library.ResolveSources{ + TrackLists: cat.RefreshReleaseGroup, Identify: acoustIDLookup.IdentifyRecordings, + }).Run(ctx) // Missing-file re-acquisition (milestone #290, M502 #5520). Wants back // albums whose files have been gone longer than the grace window; the diff --git a/internal/db/dbq/duplicates.sql.go b/internal/db/dbq/duplicates.sql.go index 800a28f8..880f34da 100644 --- a/internal/db/dbq/duplicates.sql.go +++ b/internal/db/dbq/duplicates.sql.go @@ -296,6 +296,7 @@ func (q *Queries) ListDuplicateCandidates(ctx context.Context, arg ListDuplicate const listDuplicateGroupsForResolve = `-- name: ListDuplicateGroupsForResolve :many SELECT g.id AS group_id, g.tier, + g.worst_bit_error_rate, g.class AS prior_class, t.id AS track_id, t.album_id, @@ -315,37 +316,43 @@ SELECT g.id AS group_id, AND t.track_number IS NOT NULL AND o.disc_number IS NOT DISTINCT FROM t.disc_number AND o.track_number = t.track_number)::boolean AS position_clash, - COALESCE(NULLIF(al.release_group_mbid, ''), lam.release_group_mbid, '')::text AS release_group_mbid + COALESCE(NULLIF(al.release_group_mbid, ''), lam.release_group_mbid, '')::text AS release_group_mbid, + l.state AS acoustid_state, + l.recording_mbid AS acoustid_recording FROM duplicate_groups g JOIN duplicate_group_members m ON m.group_id = g.id JOIN tracks t ON t.id = m.track_id JOIN artists ON artists.id = t.artist_id JOIN albums al ON al.id = t.album_id LEFT JOIN library_album_matches lam ON lam.album_id = t.album_id AND lam.outcome = 'listed' + LEFT JOIN track_acoustid_lookups l ON l.track_id = t.id WHERE g.status = 'pending' AND t.missing_since IS NULL ORDER BY g.id, t.id ` type ListDuplicateGroupsForResolveRow struct { - GroupID pgtype.UUID - Tier string - PriorClass *string - TrackID pgtype.UUID - AlbumID pgtype.UUID - Title string - SourceVerdict *string - ArtistName string - FilePath string - FileFormat string - FileSize int64 - DurationMs int32 - AddedAt pgtype.Timestamptz - DiscNumber *int32 - TrackNumber *int32 - HasMbid bool - PositionClash bool - ReleaseGroupMbid string + GroupID pgtype.UUID + Tier string + WorstBitErrorRate *float32 + PriorClass *string + TrackID pgtype.UUID + AlbumID pgtype.UUID + Title string + SourceVerdict *string + ArtistName string + FilePath string + FileFormat string + FileSize int64 + DurationMs int32 + AddedAt pgtype.Timestamptz + DiscNumber *int32 + TrackNumber *int32 + HasMbid bool + PositionClash bool + ReleaseGroupMbid string + AcoustidState *string + AcoustidRecording *string } // Every pending group's present copies, for the resolver (M498 #5435): what it @@ -357,6 +364,9 @@ type ListDuplicateGroupsForResolveRow struct { // prior_class is the class the last pass gave the group, NULL for a group no // pass has seen. release_group_mbid is the album's MusicBrainz group, from its // tags or the library listing's match; ” when neither names one. +// acoustid_state and acoustid_recording are what AcoustID said of the file, +// when it was looked up (#6169: which song one recording cut to several slots +// is). func (q *Queries) ListDuplicateGroupsForResolve(ctx context.Context) ([]ListDuplicateGroupsForResolveRow, error) { rows, err := q.db.Query(ctx, listDuplicateGroupsForResolve) if err != nil { @@ -369,6 +379,7 @@ func (q *Queries) ListDuplicateGroupsForResolve(ctx context.Context) ([]ListDupl if err := rows.Scan( &i.GroupID, &i.Tier, + &i.WorstBitErrorRate, &i.PriorClass, &i.TrackID, &i.AlbumID, @@ -385,6 +396,8 @@ func (q *Queries) ListDuplicateGroupsForResolve(ctx context.Context) ([]ListDupl &i.HasMbid, &i.PositionClash, &i.ReleaseGroupMbid, + &i.AcoustidState, + &i.AcoustidRecording, ); err != nil { return nil, err } diff --git a/internal/db/dbq/imports.sql.go b/internal/db/dbq/imports.sql.go index e6920393..56f78285 100644 --- a/internal/db/dbq/imports.sql.go +++ b/internal/db/dbq/imports.sql.go @@ -348,6 +348,50 @@ func (q *Queries) ListGrabs(ctx context.Context, arg ListGrabsParams) ([]ListGra return items, nil } +const listImportedGrabsHolding = `-- name: ListImportedGrabsHolding :many +SELECT id, info_hash, name, want_id + FROM grabs + WHERE import_state = 'imported' + AND info_hash <> '' + AND destination <> '' + AND starts_with($1::text, rtrim(destination, '/') || '/') +` + +type ListImportedGrabsHoldingRow struct { + ID pgtype.UUID + InfoHash string + Name string + WantID pgtype.UUID +} + +// The downloads Minstrel imported into the folder holding path: the releases a +// file there came from, to block when the files proved not to be the album +// (#6169). A file imported before Minstrel downloaded anything has none. +func (q *Queries) ListImportedGrabsHolding(ctx context.Context, path string) ([]ListImportedGrabsHoldingRow, error) { + rows, err := q.db.Query(ctx, listImportedGrabsHolding, path) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListImportedGrabsHoldingRow + for rows.Next() { + var i ListImportedGrabsHoldingRow + if err := rows.Scan( + &i.ID, + &i.InfoHash, + &i.Name, + &i.WantID, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listLiveGrabs = `-- name: ListLiveGrabs :many SELECT g.id, g.want_id, g.info_hash, g.name, g.key, g.indexer, g.source, g.client, g.save_path, g.quality, g.origin, g.forced, g.grabbed_by, g.grabbed_at, g.ended_at, g.end_reason, g.end_detail, g.progress, g.import_state, g.import_reason, g.import_detail, g.import_forced, g.import_attempts, g.import_updated_at, g.imported_at, g.imported_files, g.destination, g.seeding, g.ratio, g.seed_released_at, w.release_group_mbid, w.artist_mbid, w.artist_name, w.title AS want_title, diff --git a/internal/db/queries/duplicates.sql b/internal/db/queries/duplicates.sql index 113e354c..d38c6e89 100644 --- a/internal/db/queries/duplicates.sql +++ b/internal/db/queries/duplicates.sql @@ -195,8 +195,12 @@ UPDATE duplicate_groups -- prior_class is the class the last pass gave the group, NULL for a group no -- pass has seen. release_group_mbid is the album's MusicBrainz group, from its -- tags or the library listing's match; '' when neither names one. +-- acoustid_state and acoustid_recording are what AcoustID said of the file, +-- when it was looked up (#6169: which song one recording cut to several slots +-- is). SELECT g.id AS group_id, g.tier, + g.worst_bit_error_rate, g.class AS prior_class, t.id AS track_id, t.album_id, @@ -216,13 +220,16 @@ SELECT g.id AS group_id, AND t.track_number IS NOT NULL AND o.disc_number IS NOT DISTINCT FROM t.disc_number AND o.track_number = t.track_number)::boolean AS position_clash, - COALESCE(NULLIF(al.release_group_mbid, ''), lam.release_group_mbid, '')::text AS release_group_mbid + COALESCE(NULLIF(al.release_group_mbid, ''), lam.release_group_mbid, '')::text AS release_group_mbid, + l.state AS acoustid_state, + l.recording_mbid AS acoustid_recording FROM duplicate_groups g JOIN duplicate_group_members m ON m.group_id = g.id JOIN tracks t ON t.id = m.track_id JOIN artists ON artists.id = t.artist_id JOIN albums al ON al.id = t.album_id LEFT JOIN library_album_matches lam ON lam.album_id = t.album_id AND lam.outcome = 'listed' + LEFT JOIN track_acoustid_lookups l ON l.track_id = t.id WHERE g.status = 'pending' AND t.missing_since IS NULL ORDER BY g.id, t.id; diff --git a/internal/db/queries/imports.sql b/internal/db/queries/imports.sql index c677ab07..a44bd1ea 100644 --- a/internal/db/queries/imports.sql +++ b/internal/db/queries/imports.sql @@ -163,3 +163,14 @@ UPDATE wants -- A track whose file was replaced in place is no longer the rip it was. UPDATE tracks SET source_verdict = NULL, source_verdict_at = NULL WHERE id = $1 AND source_verdict = 'suspect'; + +-- name: ListImportedGrabsHolding :many +-- The downloads Minstrel imported into the folder holding path: the releases a +-- file there came from, to block when the files proved not to be the album +-- (#6169). A file imported before Minstrel downloaded anything has none. +SELECT id, info_hash, name, want_id + FROM grabs + WHERE import_state = 'imported' + AND info_hash <> '' + AND destination <> '' + AND starts_with(sqlc.arg(path)::text, rtrim(destination, '/') || '/'); diff --git a/internal/library/acoustid_lookup.go b/internal/library/acoustid_lookup.go index 0f4dfa15..dda1fa87 100644 --- a/internal/library/acoustid_lookup.go +++ b/internal/library/acoustid_lookup.go @@ -345,6 +345,46 @@ func (w *AcoustIDLookupWorker) lookupTrack( return w.store(ctx, row, chooseRecording(cands, cfg.MinScore, row.Title, row.DurationMs)), nil } +// ErrIdentifyUnavailable says AcoustID cannot be asked now: the lookup is off +// or has no key, the key was refused, or AcoustID could not be reached. The +// caller waits and asks again on a later pass. +var ErrIdentifyUnavailable = errors.New("acoustid: cannot be asked now") + +// IdentifyRecordings asks AcoustID which recordings a file may be, for the +// duplicate resolver (#6169): every candidate at or above the minimum score, +// best first. None is an answer: AcoustID does not know the audio. Nothing is +// stored; the file's own lookup is the worker's. +func (w *AcoustIDLookupWorker) IdentifyRecordings(ctx context.Context, path string) ([]string, error) { + if w == nil { + return nil, ErrIdentifyUnavailable + } + cfg := w.settings.Get() + if !cfg.Ready() { + return nil, ErrIdentifyUnavailable + } + fp, err := w.fingerprint(ctx, path) + if err != nil { + if isInconclusive(err) { + return nil, fmt.Errorf("%w: %w", ErrIdentifyUnavailable, err) + } + return nil, fmt.Errorf("fingerprint: %w", err) + } + cands, err := w.client.Lookup(ctx, cfg.APIKey, fp.fingerprint, fp.durationSec) + switch { + case errors.Is(err, acoustid.ErrInvalidKey), errors.Is(err, acoustid.ErrUnavailable): + return nil, fmt.Errorf("%w: %w", ErrIdentifyUnavailable, err) + case err != nil: + return nil, err + } + var ids []string + for _, c := range cands { + if float32(c.Score) >= cfg.MinScore { + ids = append(ids, c.ID) + } + } + return ids, nil +} + // store records a decision and applies it to tracks.mbid in one transaction, // so the gauge never shows a match whose id was not written. A failed write // stores nothing and the track is tried again. diff --git a/internal/library/duplicate_classify_test.go b/internal/library/duplicate_classify_test.go index 55a289bf..133f2708 100644 --- a/internal/library/duplicate_classify_test.go +++ b/internal/library/duplicate_classify_test.go @@ -113,3 +113,51 @@ func TestWrongSlots(t *testing.T) { } } } + +// One recording cut to each of several slots' lengths (Savant's Mortals): the +// lengths cannot place it, and what AcoustID calls it does (#6169). +func TestTrimmedCopies(t *testing.T) { + track := func(title string, ms int32, rec string) dbq.MbReleaseTrack { + return dbq.MbReleaseTrack{Title: title, LengthMs: ms, RecordingMbid: rec} + } + copies := []slotCopy{ + {Title: "Tormentor", Artist: "The Wombats", DurationMs: 220000}, + {Title: "Record Shop", Artist: "The Wombats", DurationMs: 181000}, + } + album := []dbq.MbReleaseTrack{ + track("Tormentor", 221000, "rec-tormentor"), track("Record Shop", 180000, "rec-record-shop"), track("Ice Cream", 200000, "rec-ice"), + } + if !trimmedToSlots(copies, album) { + t.Fatal("each copy fits its own slot: trimmedToSlots should hold the group") + } + if trimmedToSlots(copies[:1], album) { + t.Error("one title is not several songs' slots") + } + if trimmedToSlots([]slotCopy{copies[0], {Title: "Record Shop", Artist: "The Wombats", DurationMs: 220000}}, album) { + t.Error("a copy that does not fit its slot is wrongSlots' to decide") + } + cases := []struct { + name string + tracks []dbq.MbReleaseTrack + recordings []string + keep int + losers []int + ok bool + }{ + {"AcoustID names Tormentor", album, []string{"rec-tormentor"}, 0, []int{1}, true}, + {"one of several candidates is on the album", album, []string{"rec-elsewhere", "rec-record-shop"}, 1, []int{0}, true}, + {"AcoustID knows none", album, nil, -1, []int{0, 1}, true}, + {"the recording is another song on the album", album, []string{"rec-ice"}, -1, []int{0, 1}, true}, + {"candidates in both copies' slots", album, []string{"rec-tormentor", "rec-record-shop"}, -1, nil, false}, + {"the track list names no recording for a slot", []dbq.MbReleaseTrack{ + track("Tormentor", 221000, "rec-tormentor"), track("Record Shop", 180000, ""), + }, []string{"rec-tormentor"}, -1, nil, false}, + } + for _, tc := range cases { + keep, losers, ok := trimmedCopies(copies, tc.tracks, tc.recordings) + if keep != tc.keep || ok != tc.ok || fmt.Sprint(losers) != fmt.Sprint(tc.losers) { + t.Errorf("%s: got keep=%d losers=%v ok=%v, want keep=%d losers=%v ok=%v", + tc.name, keep, losers, ok, tc.keep, tc.losers, tc.ok) + } + } +} diff --git a/internal/library/duplicate_resolve.go b/internal/library/duplicate_resolve.go index 81f4daee..c7bf6625 100644 --- a/internal/library/duplicate_resolve.go +++ b/internal/library/duplicate_resolve.go @@ -34,8 +34,9 @@ import ( // - Two versions of a song on one album that sound alike are not duplicates: // the group is dismissed (duplicate_settle.go). // - One file filling several of an album's track slots: MusicBrainz's track -// lengths say which slot it belongs to, and the copies in the other slots -// are removed so the album is fetched again (duplicate_settle.go). +// lengths say which slot it belongs to, or AcoustID which song it is when +// each copy was cut to its own slot's length, and the copies in the other +// slots are removed so the album is fetched again (duplicate_settle.go). // - Anything else waits for the operator. const ( @@ -65,11 +66,14 @@ type DuplicateResolveResult struct { // resolveGroup is one pending group and its present copies. type resolveGroup struct { - id pgtype.UUID - tier string - members []dbq.ListDuplicateGroupsForResolveRow - class DuplicateClass - note string + id pgtype.UUID + tier string + // worstBER is how far apart the least alike copies' fingerprints are, for + // an acoustic group; nil for an exact one. + worstBER *float32 + members []dbq.ListDuplicateGroupsForResolveRow + class DuplicateClass + note string } // isRip says whether a copy is a video rip: held back as one, or named like @@ -116,20 +120,33 @@ func mergePlan(g *resolveGroup) (ok bool, survivor int) { // MusicBrainz into the catalogue (catalogue.Service.RefreshReleaseGroup). type TrackListFetcher func(ctx context.Context, releaseGroupMBID string) error +// RecordingIdentifier asks AcoustID which recordings a file may be +// (AcoustIDLookupWorker.IdentifyRecordings). +type RecordingIdentifier func(ctx context.Context, path string) ([]string, error) + +// ResolveSources are what a pass may ask beyond the library, a few times per +// pass each. Either may be nil, and the pass decides from what is held. +type ResolveSources struct { + // TrackLists reads the track list of an album whose wrong slot the pass + // must decide and the catalogue has never read (trackListFetchesPerPass). + TrackLists TrackListFetcher + // Identify names the recording cut to several slots' lengths, when no + // copy's own lookup has (identifiesPerPass). + Identify RecordingIdentifier +} + // ResolveDuplicates runs one pass. act is the operator's auto-resolve setting; -// with it off, the pass only classifies. It reads only track lists already in -// the catalogue; ResolveDuplicatesFetching may also fetch missing ones. +// with it off, the pass only classifies. It decides from what the library and +// catalogue hold; ResolveDuplicatesWith may also ask MusicBrainz and AcoustID. func ResolveDuplicates( ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string, act bool, ) (DuplicateResolveResult, error) { - return ResolveDuplicatesFetching(ctx, pool, logger, dataDir, act, nil) + return ResolveDuplicatesWith(ctx, pool, logger, dataDir, act, ResolveSources{}) } -// ResolveDuplicatesFetching is ResolveDuplicates that asks MusicBrainz, through -// fetch, for the track list of an album whose wrong slot it must decide and -// the catalogue has never read: a few per pass (trackListFetchesPerPass). -func ResolveDuplicatesFetching( - ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string, act bool, fetch TrackListFetcher, +// ResolveDuplicatesWith is ResolveDuplicates that may ask src. +func ResolveDuplicatesWith( + ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string, act bool, src ResolveSources, ) (DuplicateResolveResult, error) { if logger == nil { logger = slog.Default() @@ -196,7 +213,7 @@ func ResolveDuplicatesFetching( } res.Dismissed = dismissDistinct(ctx, pool, logger, groups, merged) - wrong, names := settleWrongSlots(ctx, pool, logger, groups, merged, fetch) + wrong, names := settleWrongSlots(ctx, pool, logger, groups, merged, src) res.WrongFiles = wrong mergedNames = append(mergedNames, names...) @@ -281,7 +298,7 @@ func foldResolveGroups(rows []dbq.ListDuplicateGroupsForResolveRow) []*resolveGr var groups []*resolveGroup for _, r := range rows { if n := len(groups); n == 0 || groups[n-1].id != r.GroupID { - groups = append(groups, &resolveGroup{id: r.GroupID, tier: r.Tier}) + groups = append(groups, &resolveGroup{id: r.GroupID, tier: r.Tier, worstBER: r.WorstBitErrorRate}) } g := groups[len(groups)-1] g.members = append(g.members, r) @@ -389,17 +406,17 @@ type DuplicateResolveWorker struct { logger *slog.Logger dataDir string settings *FingerprintSettingsService - fetch TrackListFetcher + src ResolveSources tick time.Duration } -// NewDuplicateResolveWorker builds a worker with the production cadence. fetch -// reads a missing track list from MusicBrainz; nil reads only what is held. +// NewDuplicateResolveWorker builds a worker with the production cadence. src +// is what a pass may ask beyond the library. func NewDuplicateResolveWorker( - pool *pgxpool.Pool, logger *slog.Logger, dataDir string, settings *FingerprintSettingsService, fetch TrackListFetcher, + pool *pgxpool.Pool, logger *slog.Logger, dataDir string, settings *FingerprintSettingsService, src ResolveSources, ) *DuplicateResolveWorker { return &DuplicateResolveWorker{ - pool: pool, logger: logger, dataDir: dataDir, settings: settings, fetch: fetch, tick: duplicateResolveTick, + pool: pool, logger: logger, dataDir: dataDir, settings: settings, src: src, tick: duplicateResolveTick, } } @@ -436,7 +453,7 @@ func (w *DuplicateResolveWorker) tickOnce(ctx context.Context) { } return } - res, err := ResolveDuplicatesFetching(ctx, w.pool, w.logger, w.dataDir, w.settings.Get().AutoResolve, w.fetch) + res, err := ResolveDuplicatesWith(ctx, w.pool, w.logger, w.dataDir, w.settings.Get().AutoResolve, w.src) if err != nil && ctx.Err() == nil { w.logger.Warn("duplicate resolve: pass failed", "err", err) } diff --git a/internal/library/duplicate_resolve_test.go b/internal/library/duplicate_resolve_test.go index d594a583..e3ef43a4 100644 --- a/internal/library/duplicate_resolve_test.go +++ b/internal/library/duplicate_resolve_test.go @@ -436,6 +436,108 @@ func TestResolveDuplicates_RemovesEveryCopyWhenNoSlotIsTheirs_Integration(t *tes } } +// seedTrimmedAlbum is newOneAlbumFixture for one recording cut to both its +// slots' lengths: identical audio, as Tormentor and as Record Shop, each as +// long as its own slot's song, and the track list naming each slot's +// recording. The files came from a download Minstrel imported. +func seedTrimmedAlbum(t *testing.T) oneAlbumFixture { + t.Helper() + f := newOneAlbumFixture(t, "exact", "Tormentor", "Record Shop") + ctx := context.Background() + seedTrackList(t, f.pool, f.album, map[string]int{"Tormentor": 221000, "Record Shop": 219000}) + for _, stmt := range []struct { + sql string + args []any + }{ + {`UPDATE mb_release_tracks SET recording_mbid = 'rec-' || lower(replace(title, ' ', '-'))`, nil}, + {`INSERT INTO grabs (info_hash, name, key, client, save_path, origin, import_state, destination) + VALUES ('abc123', 'The Wombats - Void [WEB]', 'k', 'qbit', '/dl', 'schedule', 'imported', $1)`, + []any{filepath.Dir(f.aPath)}}, + } { + if _, err := f.pool.Exec(ctx, stmt.sql, stmt.args...); err != nil { + t.Fatalf("exec %q: %v", stmt.sql, err) + } + } + return f +} + +func releaseBlocked(t *testing.T, pool *pgxpool.Pool, hash string) bool { + t.Helper() + var n int + if err := pool.QueryRow(context.Background(), + `SELECT count(*) FROM release_blocks WHERE info_hash = $1 AND reason = 'files'`, hash).Scan(&n); err != nil { + t.Fatal(err) + } + return n == 1 +} + +// AcoustID matched one copy to Tormentor's recording: that copy stays, Record +// Shop's goes, and the download they came from is set aside (#6169). +func TestResolveDuplicates_KeepsTheCopyAcoustIDNames_Integration(t *testing.T) { + f := seedTrimmedAlbum(t) + if _, err := f.pool.Exec(context.Background(), + `INSERT INTO track_acoustid_lookups (track_id, state, candidates, recording_mbid) VALUES ($1, 'matched', 1, 'rec-tormentor')`, + f.a.ID); err != nil { + t.Fatal(err) + } + asked := 0 + res, err := ResolveDuplicatesWith(context.Background(), f.pool, nil, "", true, ResolveSources{ + Identify: func(context.Context, string) ([]string, error) { asked++; return nil, nil }, + }) + if err != nil { + t.Fatal(err) + } + if asked != 0 { + t.Errorf("AcoustID asked %d times; the copy's own lookup already answered", asked) + } + if res.WrongFiles != 1 || !exists(f.aPath) || exists(f.bPath) { + t.Fatalf("result %+v, tormentor exists %v, record shop exists %v; want only Record Shop's copy removed", + res, exists(f.aPath), exists(f.bPath)) + } + if !releaseBlocked(t, f.pool, "abc123") { + t.Error("the download the files came from was not set aside, so the album would be fetched from it again") + } +} + +// AcoustID knows the recording as no song of the album: every copy goes. With +// AcoustID unreachable, nothing is decided and the group waits. +func TestResolveDuplicates_TrimmedCopiesAcoustIDCannotName_Integration(t *testing.T) { + t.Run("no match", func(t *testing.T) { + f := seedTrimmedAlbum(t) + asked := 0 + res, err := ResolveDuplicatesWith(context.Background(), f.pool, nil, "", true, ResolveSources{ + Identify: func(context.Context, string) ([]string, error) { + asked++ + return []string{"rec-elsewhere"}, nil + }, + }) + if err != nil { + t.Fatal(err) + } + if asked != 1 || res.WrongFiles != 2 || exists(f.aPath) || exists(f.bPath) { + t.Fatalf("asked %d, result %+v; want one lookup and both copies removed", asked, res) + } + if status, _, _, _ := (resolveFixture{pool: f.pool, groupID: f.groupID}).group(t); status != "merged" { + t.Errorf("group = %s, want settled", status) + } + }) + t.Run("unreachable", func(t *testing.T) { + f := seedTrimmedAlbum(t) + res, err := ResolveDuplicatesWith(context.Background(), f.pool, nil, "", true, ResolveSources{ + Identify: func(context.Context, string) ([]string, error) { return nil, ErrIdentifyUnavailable }, + }) + if err != nil { + t.Fatal(err) + } + if res.WrongFiles != 0 || !exists(f.aPath) || !exists(f.bPath) || releaseBlocked(t, f.pool, "abc123") { + t.Fatalf("result %+v; want nothing removed or set aside while AcoustID cannot answer", res) + } + if status, _, _, _ := (resolveFixture{pool: f.pool, groupID: f.groupID}).group(t); status != "pending" { + t.Errorf("group = %s, want pending", status) + } + }) +} + // An album whose track list the catalogue never read: the resolver asks for // it, once, and decides with what comes back. func TestResolveDuplicates_FetchesAMissingTrackList_Integration(t *testing.T) { @@ -454,7 +556,7 @@ func TestResolveDuplicates_FetchesAMissingTrackList_Integration(t *testing.T) { seedTrackList(t, f.pool, f.album, map[string]int{"Void": 220500, "Reach": 281000}) return nil } - res, err := ResolveDuplicatesFetching(context.Background(), f.pool, nil, "", true, fetch) + res, err := ResolveDuplicatesWith(context.Background(), f.pool, nil, "", true, ResolveSources{TrackLists: fetch}) if err != nil { t.Fatal(err) } diff --git a/internal/library/duplicate_settle.go b/internal/library/duplicate_settle.go index 5358fbac..a3d25ab7 100644 --- a/internal/library/duplicate_settle.go +++ b/internal/library/duplicate_settle.go @@ -6,11 +6,13 @@ import ( "fmt" "log/slog" "os" + "slices" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" "github.com/jackc/pgx/v5/pgxpool" + "git.fabledsword.com/bvandeusen/minstrel/internal/acquire" "git.fabledsword.com/bvandeusen/minstrel/internal/audit" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/musicbrainz" @@ -32,7 +34,15 @@ import ( // in the other slots are removed and their tracks marked missing, so the // reacquisition sweeper wants the album back and a good copy fills them. // When every slot's song is clearly not the file, every copy goes. -// Anything less certain stays with the operator. +// - trimmed: one recording cut to each of several slots' lengths (Savant's +// Mortals held one as 16 of its 50 tracks), so every slot fits and the +// lengths cannot say. AcoustID names the recording and the track list +// names each slot's: the copy in that recording's slot is kept and the +// rest go, or every copy when the recording is in none of their slots. +// The release the files came from is set aside, so the album fetched +// again is not the same files (#6169). +// +// Anything less certain stays with the operator. const ( // trackListFetchesPerPass is how many albums' track lists a pass may ask @@ -44,6 +54,14 @@ const ( // title must be for the file to be another song's. slotFit = 3000 slotOther = 10000 + // identifiesPerPass is how many groups a pass may ask AcoustID about. The + // client keeps to AcoustID's rate; this keeps a pass short. + identifiesPerPass = 5 + // sameAudioBER is the worst bit error rate at which an acoustic group's + // copies are one recording cut differently rather than takes that sound + // alike: the groups measured as one recording trimmed to several slots + // read 0.000, and two takes of one song read far above this (#6169). + sameAudioBER = 0.02 ) // slotCopy is what deciding a wrong slot needs to know about one copy. @@ -149,19 +167,22 @@ func dismissDistinct( } // settleWrongSlots removes the copies that sit in another song's slot, for -// every one-album mismatch the track list decides. It returns how many copies -// went and the songs whose slots were emptied, and adds the groups it settled -// to settled. +// every one-album mismatch the track list, or AcoustID with it, decides. It +// returns how many copies went and the songs whose slots were emptied, and +// adds the groups it settled to settled. func settleWrongSlots( ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, groups []*resolveGroup, settled map[pgtype.UUID]bool, - fetch TrackListFetcher, + src ResolveSources, ) (int, []string) { q := dbq.New(pool) tracksByGroup := map[string][]dbq.MbReleaseTrack{} - fetches := trackListFetchesPerPass - if fetch == nil { + fetches, identifies := trackListFetchesPerPass, identifiesPerPass + if src.TrackLists == nil { fetches = 0 } + if src.Identify == nil { + identifies = 0 + } removed := 0 var names []string for _, g := range groups { @@ -181,7 +202,7 @@ func settleWrongSlots( } if len(tracks) == 0 && fetches > 0 && neverRead(ctx, q, rg) { fetches-- - if err := fetch(ctx, rg); err != nil { + if err := src.TrackLists(ctx, rg); err != nil { logger.Info("duplicate resolve: fetching the album's track list failed", "release_group", rg, "err", err) if errors.Is(err, musicbrainz.ErrUnreachable) { fetches = 0 @@ -198,10 +219,20 @@ func settleWrongSlots( copies[i] = slotCopy{Title: m.Title, Artist: m.ArtistName, DurationMs: m.DurationMs} } keep, losers, ok := wrongSlots(copies, tracks) + if !ok && sameAudio(g) && trimmedToSlots(copies, tracks) { + recordings, known := groupRecordings(ctx, logger, g, src.Identify, &identifies) + if !known { + continue + } + keep, losers, ok = trimmedCopies(copies, tracks, recordings) + } if !ok { continue } - n, err := removeWrongSlots(ctx, pool, logger, g, keep, losers) + // Set aside before the files go: a pass that dies between the two + // still does not fetch the same files again. + blocked := blockReleasesHolding(ctx, pool, logger, g) + n, err := removeWrongSlots(ctx, pool, logger, g, keep, losers, blocked) removed += n if err != nil { logger.Warn("duplicate resolve: removing wrong-slot copies failed", "group_id", syncpkg.FormatUUID(g.id), "err", err) @@ -215,6 +246,149 @@ func settleWrongSlots( return removed, names } +// sameAudio reports whether a group's copies are one recording, cut however: +// identical audio, or fingerprints all but identical (sameAudioBER). +func sameAudio(g *resolveGroup) bool { + return g.tier == "exact" || (g.worstBER != nil && *g.worstBER <= sameAudioBER) +} + +// trimmedToSlots reports whether every copy is as long as its own slot's song +// and they are filed as more than one song: one recording cut to several +// slots' lengths, which the lengths alone cannot place. +func trimmedToSlots(copies []slotCopy, tracks []dbq.MbReleaseTrack) bool { + keys := map[string]bool{} + for _, c := range copies { + key := MatchTitleKey(c.Title, c.Artist) + fits := false + for _, t := range tracks { + if t.LengthMs > 0 && MatchTitleKey(t.Title, c.Artist) == key && absMs(t.LengthMs-c.DurationMs) <= slotFit { + fits = true + break + } + } + if !fits { + return false + } + keys[key] = true + } + return len(keys) > 1 +} + +// trimmedCopies decides a group trimmedToSlots holds, from the recordings +// AcoustID says it may be. The track list names each slot's recording: the +// copy in a slot of one of them is kept and the others are losers. When none +// of the copies' slots is one of them, or AcoustID knows none (recordings +// empty), every copy is another song's and all go. ok is false when they fall +// in more than one copy's slot, or the track list names no recording for a +// copy's slot. +// +// Copies sharing the kept copy's title are one song held twice, as in +// wrongSlots: neither kept nor lost here. +func trimmedCopies(copies []slotCopy, tracks []dbq.MbReleaseTrack, recordings []string) (keep int, losers []int, ok bool) { + keys := make([]string, len(copies)) + theirs := map[string]bool{} + for i, c := range copies { + keys[i] = MatchTitleKey(c.Title, c.Artist) + named := false + for _, t := range tracks { + if t.RecordingMbid == "" || MatchTitleKey(t.Title, c.Artist) != keys[i] { + continue + } + named = true + if slices.Contains(recordings, t.RecordingMbid) { + theirs[keys[i]] = true + } + } + if !named { + return -1, nil, false + } + } + if len(theirs) > 1 { + return -1, nil, false + } + keep = -1 + for i, key := range keys { + switch { + case theirs[key] && keep < 0: + keep = i + case theirs[key]: + default: + losers = append(losers, i) + } + } + return keep, losers, true +} + +// groupRecordings is what AcoustID says a group's recording may be: the +// recordings the copies' own lookups matched, none when every copy's lookup +// found no match, or else a lookup of one copy now, while the pass has +// lookups left. known is false when there is no answer yet, and the group +// waits for a later pass. +func groupRecordings( + ctx context.Context, logger *slog.Logger, g *resolveGroup, identify RecordingIdentifier, left *int, +) (recordings []string, known bool) { + noMatch := 0 + for _, m := range g.members { + switch { + case m.AcoustidState == nil: + case *m.AcoustidState == lookupMatched && m.AcoustidRecording != nil: + if !slices.Contains(recordings, *m.AcoustidRecording) { + recordings = append(recordings, *m.AcoustidRecording) + } + case *m.AcoustidState == lookupNoMatch: + noMatch++ + } + } + if len(recordings) > 0 || noMatch == len(g.members) { + return recordings, true + } + if identify == nil || *left <= 0 { + return nil, false + } + *left-- + ids, err := identify(ctx, g.members[0].FilePath) + if err != nil { + if errors.Is(err, ErrIdentifyUnavailable) { + *left = 0 + } + logger.Info("duplicate resolve: asking AcoustID which song a recording is failed", + "group_id", syncpkg.FormatUUID(g.id), "err", err) + return nil, false + } + return ids, true +} + +// blockReleasesHolding sets aside the downloads a group's files were imported +// from, and returns their names. An album imported before Minstrel downloaded +// anything has none. +func blockReleasesHolding(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, g *resolveGroup) []string { + q := dbq.New(pool) + seen := map[string]bool{} + var names []string + for _, m := range g.members { + grabs, err := q.ListImportedGrabsHolding(ctx, m.FilePath) + if err != nil { + logger.Warn("duplicate resolve: finding the download a file came from failed", "path", m.FilePath, "err", err) + continue + } + for _, gr := range grabs { + if seen[gr.InfoHash] { + continue + } + seen[gr.InfoHash] = true + if err := acquire.NewMemory(pool).Block(ctx, dbq.BlockReleaseParams{ + InfoHash: gr.InfoHash, Name: gr.Name, Reason: acquire.BlockFiles, + Detail: "one recording fills several of the album's tracks.", WantID: gr.WantID, + }); err != nil { + logger.Warn("duplicate resolve: setting the release aside failed", "release", gr.Name, "err", err) + continue + } + names = append(names, gr.Name) + } + } + return names +} + // neverRead reports whether the catalogue has never read a release group's // releases. A group read and found empty is not asked about again: the // catalogue's own refresh revisits it. @@ -227,11 +401,12 @@ func neverRead(ctx context.Context, q *dbq.Queries, rg string) bool { // the same transaction that closes the group. The row is kept, with its likes // and plays: it is the slot, and a good copy fetched again takes it over. Its // recording MBID is dropped, being the wrong file's, so missing-pair folding -// does not fold the slot into the copy that was kept. A -// file that cannot be removed stops the group; the ones already gone are still -// marked, and the group waits for the next pass. +// does not fold the slot into the copy that was kept. blocked names the +// releases set aside, for the record. A file that cannot be removed stops the +// group; the ones already gone are still marked, and the group waits for the +// next pass. func removeWrongSlots( - ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, g *resolveGroup, keep int, losers []int, + ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, g *resolveGroup, keep int, losers []int, blocked []string, ) (int, error) { var gone []pgtype.UUID var removedFiles []map[string]string @@ -278,6 +453,9 @@ func removeWrongSlots( "artist": g.members[0].ArtistName, "removed": removedFiles, } + if len(blocked) > 0 { + details["blocked"] = blocked + } keptPath := "" if keep >= 0 { kept := g.members[keep]