From 3a482bd715e3b3026c932c4c4726667efc77ddc0 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 20 Oct 2026 10:52:42 -0400 Subject: [PATCH] fix: AcoustID decides any one-album mismatch the lengths cannot, not only copies each cut to its slot (#6169) The first pass on the operator's library held 16 groups. In several, every copy's own lookup names one recording (Wombats Record Shop/Tormentor, Alchemist (2)/Prelude, Monday Chill 8/9, 404sessions), but one copy sits 3-10s from its slot, so the every-copy-fits precondition kept AcoustID out. Now: copies whose lookups all agree are that recording, whatever their fingerprints read; copies of the same audio are asked about as before; and the track list places the recording in one copy's slot, or none. Copies filed as one song are not settled here. Co-Authored-By: Claude Opus 5.5 --- internal/library/duplicate_classify_test.go | 31 +++----- internal/library/duplicate_resolve_test.go | 50 ++++++++++++ internal/library/duplicate_settle.go | 86 ++++++++++----------- 3 files changed, 103 insertions(+), 64 deletions(-) diff --git a/internal/library/duplicate_classify_test.go b/internal/library/duplicate_classify_test.go index 133f2708..f0270c51 100644 --- a/internal/library/duplicate_classify_test.go +++ b/internal/library/duplicate_classify_test.go @@ -114,9 +114,9 @@ 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) { +// The lengths cannot place a recording filed as several songs (Savant's +// Mortals, cut to each slot's length); what AcoustID calls it can (#6169). +func TestNamedCopies(t *testing.T) { track := func(title string, ms int32, rec string) dbq.MbReleaseTrack { return dbq.MbReleaseTrack{Title: title, LengthMs: ms, RecordingMbid: rec} } @@ -127,34 +127,27 @@ func TestTrimmedCopies(t *testing.T) { 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 + copies []slotCopy 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{ + {"AcoustID names Tormentor", copies, album, []string{"rec-tormentor"}, 0, []int{1}, true}, + {"one of several candidates is on the album", copies, album, []string{"rec-elsewhere", "rec-record-shop"}, 1, []int{0}, true}, + {"AcoustID knows none", copies, album, nil, -1, []int{0, 1}, true}, + {"the recording is another song on the album", copies, album, []string{"rec-ice"}, -1, []int{0, 1}, true}, + {"candidates in both copies' slots", copies, album, []string{"rec-tormentor", "rec-record-shop"}, -1, nil, false}, + {"the track list names no recording for a slot", copies, []dbq.MbReleaseTrack{ track("Tormentor", 221000, "rec-tormentor"), track("Record Shop", 180000, ""), }, []string{"rec-tormentor"}, -1, nil, false}, + {"every copy filed as the one song named", []slotCopy{copies[0], copies[0]}, album, []string{"rec-tormentor"}, -1, nil, false}, } for _, tc := range cases { - keep, losers, ok := trimmedCopies(copies, tc.tracks, tc.recordings) + keep, losers, ok := namedCopies(tc.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_test.go b/internal/library/duplicate_resolve_test.go index e3ef43a4..99328870 100644 --- a/internal/library/duplicate_resolve_test.go +++ b/internal/library/duplicate_resolve_test.go @@ -499,6 +499,56 @@ func TestResolveDuplicates_KeepsTheCopyAcoustIDNames_Integration(t *testing.T) { } } +// Copies whose own lookups all name Record Shop's recording are that +// recording, though their fingerprints differ a little and one is six seconds +// from Tormentor's length, too near for the lengths to decide (the operator's +// Wombats and Monday Chill groups, 2026-10-20). With no lookups, copies that +// far apart are not asked about. +func TestResolveDuplicates_AgreeingLookupsDecideAnAcousticGroup_Integration(t *testing.T) { + setup := func(t *testing.T) oneAlbumFixture { + f := seedTrimmedAlbum(t) + for _, stmt := range []string{ + `UPDATE duplicate_groups SET tier = 'acoustic', worst_bit_error_rate = 0.04`, + `UPDATE mb_release_tracks SET length_ms = 226000 WHERE title = 'Tormentor'`, + } { + if _, err := f.pool.Exec(context.Background(), stmt); err != nil { + t.Fatalf("exec %q: %v", stmt, err) + } + } + return f + } + t.Run("agreeing", func(t *testing.T) { + f := setup(t) + if _, err := f.pool.Exec(context.Background(), + `INSERT INTO track_acoustid_lookups (track_id, state, candidates, recording_mbid) + VALUES ($1, 'matched', 1, 'rec-record-shop'), ($2, 'matched', 1, 'rec-record-shop')`, + f.a.ID, f.b.ID); err != nil { + t.Fatal(err) + } + res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", true) + if err != nil { + t.Fatal(err) + } + if res.WrongFiles != 1 || exists(f.aPath) || !exists(f.bPath) { + t.Fatalf("result %+v, tormentor exists %v, record shop exists %v; want only Tormentor's copy removed", + res, exists(f.aPath), exists(f.bPath)) + } + }) + t.Run("unknown", func(t *testing.T) { + f := setup(t) + 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 || res.WrongFiles != 0 || !exists(f.aPath) || !exists(f.bPath) { + t.Fatalf("asked %d, result %+v; want nothing asked or removed for copies that may be two takes", asked, res) + } + }) +} + // 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) { diff --git a/internal/library/duplicate_settle.go b/internal/library/duplicate_settle.go index a3d25ab7..0ca67436 100644 --- a/internal/library/duplicate_settle.go +++ b/internal/library/duplicate_settle.go @@ -34,13 +34,16 @@ 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. -// - 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). +// - named: the lengths cannot say (Savant's Mortals held one recording as +// 16 of its 50 tracks, each cut to its own slot's length), but AcoustID +// can. When every copy's lookup names one recording, or the copies are +// the same audio and a lookup names any, the track list says whose slot +// that recording is: that copy is kept and the rest go, or every copy +// when it is in none of their slots. Same audio that AcoustID does not +// know at all is no copy's song, and every copy goes. +// +// Whenever copies go, 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. @@ -219,12 +222,15 @@ 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) { + if !ok { recordings, known := groupRecordings(ctx, logger, g, src.Identify, &identifies) if !known { continue } - keep, losers, ok = trimmedCopies(copies, tracks, recordings) + if keep, losers, ok = namedCopies(copies, tracks, recordings); !ok { + logger.Info("duplicate resolve: AcoustID's answer does not place the recording in one slot; left for review", + "group_id", syncpkg.FormatUUID(g.id), "recordings", recordings) + } } if !ok { continue @@ -252,39 +258,17 @@ 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. +// namedCopies decides a one-album mismatch the lengths could not, 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, the track list names no +// recording for a copy's slot, or the copies are all filed as one song. // // 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) { +func namedCopies(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 { @@ -316,22 +300,28 @@ func trimmedCopies(copies []slotCopy, tracks []dbq.MbReleaseTrack, recordings [] losers = append(losers, i) } } + if len(losers) == 0 { + return -1, nil, false + } 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. +// groupRecordings is what AcoustID says a group's recording may be. Copies +// whose own lookups all name one recording are that recording, however their +// fingerprints compare. Otherwise only copies that are the same audio +// (sameAudio) are asked about: the recordings any copy's lookup 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. func groupRecordings( ctx context.Context, logger *slog.Logger, g *resolveGroup, identify RecordingIdentifier, left *int, ) (recordings []string, known bool) { - noMatch := 0 + matched, noMatch := 0, 0 for _, m := range g.members { switch { case m.AcoustidState == nil: case *m.AcoustidState == lookupMatched && m.AcoustidRecording != nil: + matched++ if !slices.Contains(recordings, *m.AcoustidRecording) { recordings = append(recordings, *m.AcoustidRecording) } @@ -339,6 +329,12 @@ func groupRecordings( noMatch++ } } + if matched == len(g.members) && len(recordings) == 1 { + return recordings, true + } + if !sameAudio(g) { + return nil, false + } if len(recordings) > 0 || noMatch == len(g.members) { return recordings, true }