From c8e855ae4af62009b8e1c0d293a8c808ad20000a Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 20 Oct 2026 09:02:53 -0400 Subject: [PATCH] feat: the duplicate resolver fetches a missing album track list from MusicBrainz to decide a wrong slot, five albums a pass (M498) 20 of the 34 groups left for review had no cached track list for their album: the catalogue only reads one when the wanted list or the importer needs it. The resolver now asks catalogue.RefreshReleaseGroup for an album it must decide and the catalogue has never read, through the shared MusicBrainz pacer, at most five a pass (hourly). An unreachable MusicBrainz stops the pass's fetching; a group read and found empty is not asked about again. Co-Authored-By: Claude Opus 5.5 --- cmd/minstrel/main.go | 7 ++--- internal/library/duplicate_resolve.go | 30 +++++++++++++++++----- internal/library/duplicate_resolve_test.go | 27 +++++++++++++++++++ internal/library/duplicate_settle.go | 30 ++++++++++++++++++++++ 4 files changed, 85 insertions(+), 9 deletions(-) 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