diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index dc4fadf4..c26615e0 100644 --- a/cmd/minstrel/main.go +++ b/cmd/minstrel/main.go @@ -363,10 +363,11 @@ func run() error { // Duplicate resolver (M498, M502): classifies the sweep's groups and, with // the operator's auto-resolve on, merges the copies that are safe to merge. - // Started after SetNotifier so its summaries reach admins. See - // internal/library/duplicate_resolve.go. + // Started after SetNotifier so its summaries reach admins. It asks the + // 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).Run(ctx) + cfg.Storage.DataDir, fpSettings, cat.RefreshReleaseGroup).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/library/duplicate_resolve.go b/internal/library/duplicate_resolve.go index e83786dc..81f4daee 100644 --- a/internal/library/duplicate_resolve.go +++ b/internal/library/duplicate_resolve.go @@ -112,10 +112,24 @@ func mergePlan(g *resolveGroup) (ok bool, survivor int) { return false, -1 } +// TrackListFetcher reads a release group's releases and track lists from +// MusicBrainz into the catalogue (catalogue.Service.RefreshReleaseGroup). +type TrackListFetcher func(ctx context.Context, releaseGroupMBID string) error + // ResolveDuplicates runs one pass. act is the operator's auto-resolve setting; -// with it off, the pass only classifies. +// with it off, the pass only classifies. It reads only track lists already in +// the catalogue; ResolveDuplicatesFetching may also fetch missing ones. 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) +} + +// 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, ) (DuplicateResolveResult, error) { if logger == nil { logger = slog.Default() @@ -182,7 +196,7 @@ func ResolveDuplicates( } res.Dismissed = dismissDistinct(ctx, pool, logger, groups, merged) - wrong, names := settleWrongSlots(ctx, pool, logger, groups, merged) + wrong, names := settleWrongSlots(ctx, pool, logger, groups, merged, fetch) res.WrongFiles = wrong mergedNames = append(mergedNames, names...) @@ -375,14 +389,18 @@ type DuplicateResolveWorker struct { logger *slog.Logger dataDir string settings *FingerprintSettingsService + fetch TrackListFetcher tick time.Duration } -// NewDuplicateResolveWorker builds a worker with the production cadence. +// NewDuplicateResolveWorker builds a worker with the production cadence. fetch +// reads a missing track list from MusicBrainz; nil reads only what is held. func NewDuplicateResolveWorker( - pool *pgxpool.Pool, logger *slog.Logger, dataDir string, settings *FingerprintSettingsService, + pool *pgxpool.Pool, logger *slog.Logger, dataDir string, settings *FingerprintSettingsService, fetch TrackListFetcher, ) *DuplicateResolveWorker { - return &DuplicateResolveWorker{pool: pool, logger: logger, dataDir: dataDir, settings: settings, tick: duplicateResolveTick} + return &DuplicateResolveWorker{ + pool: pool, logger: logger, dataDir: dataDir, settings: settings, fetch: fetch, tick: duplicateResolveTick, + } } // Run blocks until ctx is cancelled, running a pass at start and then each tick. @@ -418,7 +436,7 @@ func (w *DuplicateResolveWorker) tickOnce(ctx context.Context) { } return } - res, err := ResolveDuplicates(ctx, w.pool, w.logger, w.dataDir, w.settings.Get().AutoResolve) + res, err := ResolveDuplicatesFetching(ctx, w.pool, w.logger, w.dataDir, w.settings.Get().AutoResolve, w.fetch) 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 df99b67e..cf82e664 100644 --- a/internal/library/duplicate_resolve_test.go +++ b/internal/library/duplicate_resolve_test.go @@ -405,6 +405,33 @@ func TestResolveDuplicates_RemovesACopyFromAnotherSongsSlot_Integration(t *testi } } +// 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) { + f := newOneAlbumFixture(t, "acoustic", "Void", "Reach") + rg := "rg-" + syncpkg.FormatUUID(f.album.ID) + if _, err := f.pool.Exec(context.Background(), + `UPDATE albums SET release_group_mbid = $2 WHERE id = $1`, f.album.ID, rg); err != nil { + t.Fatal(err) + } + calls := 0 + fetch := func(_ context.Context, asked string) error { + calls++ + if asked != rg { + t.Errorf("asked for %s, want %s", asked, rg) + } + 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) + if err != nil { + t.Fatal(err) + } + if calls != 1 || res.WrongFiles != 1 || exists(f.bPath) { + t.Fatalf("fetches %d, result %+v; want one fetch and Reach's copy removed", calls, res) + } +} + // Without a track list that decides, nothing is removed. func TestResolveDuplicates_LeavesAnUndecidedSlotForTheOperator_Integration(t *testing.T) { f := newOneAlbumFixture(t, "acoustic", "Void", "Reach") diff --git a/internal/library/duplicate_settle.go b/internal/library/duplicate_settle.go index fa9a246a..bc651a79 100644 --- a/internal/library/duplicate_settle.go +++ b/internal/library/duplicate_settle.go @@ -13,6 +13,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/audit" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/musicbrainz" "git.fabledsword.com/bvandeusen/minstrel/internal/notifications" syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" ) @@ -33,6 +34,10 @@ import ( // Anything less certain stays with the operator. const ( + // trackListFetchesPerPass is how many albums' track lists a pass may ask + // MusicBrainz for, when the catalogue has never read them. The resolver + // runs hourly, so a backlog drains over a few hours without a burst. + trackListFetchesPerPass = 5 // slotFit is how close a file must be to a slot's MusicBrainz length to be // that slot's song, and slotOther how far away every length of a slot's // title must be for the file to be another song's. @@ -143,9 +148,14 @@ func dismissDistinct( // to settled. func settleWrongSlots( ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, groups []*resolveGroup, settled map[pgtype.UUID]bool, + fetch TrackListFetcher, ) (int, []string) { q := dbq.New(pool) tracksByGroup := map[string][]dbq.MbReleaseTrack{} + fetches := trackListFetchesPerPass + if fetch == nil { + fetches = 0 + } removed := 0 var names []string for _, g := range groups { @@ -163,6 +173,18 @@ func settleWrongSlots( logger.Warn("duplicate resolve: reading the album's track list failed", "release_group", rg, "err", err) continue } + if len(tracks) == 0 && fetches > 0 && neverRead(ctx, q, rg) { + fetches-- + if err := fetch(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 + } + } else if tracks, err = q.ListMBReleaseTracksForGroup(ctx, rg); err != nil { + logger.Warn("duplicate resolve: reading the album's track list failed", "release_group", rg, "err", err) + continue + } + } tracksByGroup[rg] = tracks } copies := make([]slotCopy, len(g.members)) @@ -187,6 +209,14 @@ func settleWrongSlots( return removed, 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. +func neverRead(ctx context.Context, q *dbq.Queries, rg string) bool { + g, err := q.GetMBReleaseGroup(ctx, rg) + return errors.Is(err, pgx.ErrNoRows) || (err == nil && !g.ReleasesRefreshedAt.Valid) +} + // removeWrongSlots deletes each loser's file, then marks its track missing in // 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. A