diff --git a/internal/api/admin_duplicates.go b/internal/api/admin_duplicates.go index 8e730ad3..755799c6 100644 --- a/internal/api/admin_duplicates.go +++ b/internal/api/admin_duplicates.go @@ -165,7 +165,9 @@ func (h *handlers) handleListDuplicates(w http.ResponseWriter, r *http.Request) } // resolvedActions are the audit actions the "Resolved automatically" tab lists. -var resolvedActions = []string{string(audit.ActionDuplicateMerge)} +var resolvedActions = []string{ + string(audit.ActionDuplicateMerge), string(audit.ActionDuplicateDismiss), string(audit.ActionDuplicateWrongFile), +} func duplicateCounts(ctx context.Context, q *dbq.Queries) (duplicateViewCounts, error) { var c duplicateViewCounts diff --git a/internal/api/admin_summary_test.go b/internal/api/admin_summary_test.go index 263dc999..0d1abc3c 100644 --- a/internal/api/admin_summary_test.go +++ b/internal/api/admin_summary_test.go @@ -62,3 +62,31 @@ func TestAdminSummary_CountsTheQueuesAndSinceWhen(t *testing.T) { t.Errorf("a listener got %d, want 403", code) } } + +// Duplicates count what needs review: a cross-release group is kept and +// linked by the resolver and waits on nobody (operator, 2026-10-19: the +// headline said 3,479 when 536 needed review). +func TestAdminSummary_DuplicatesCountOnlyWhatNeedsReview(t *testing.T) { + h, _ := testHandlers(t) + truncateLibrary(t, h.pool) + admin := seedUser(t, h.pool, "dupsum-admin", "pw", true) + ctx := context.Background() + a := seedQuarantineTrack(t, h, "dupsum-a") + b := seedQuarantineTrack(t, h, "dupsum-b") + for _, g := range []struct{ key, class string }{{"dupsum-review", "review"}, {"dupsum-cross", "cross_release"}} { + var id [16]byte + if err := h.pool.QueryRow(ctx, + `INSERT INTO duplicate_groups (member_key, tier, class) VALUES ($1, 'acoustic', $2) RETURNING id`, + g.key, g.class).Scan(&id); err != nil { + t.Fatal(err) + } + if _, err := h.pool.Exec(ctx, `INSERT INTO duplicate_group_members (group_id, track_id) VALUES ($1, $2), ($1, $3)`, + id, a.ID, b.ID); err != nil { + t.Fatal(err) + } + } + _, got := getAdminSummary(t, h, admin) + if got.Duplicates.Count != 1 { + t.Errorf("duplicates = %d, want 1: the cross-release group needs nobody", got.Duplicates.Count) + } +} diff --git a/internal/audit/audit.go b/internal/audit/audit.go index b9e92c5d..ff870fe8 100644 --- a/internal/audit/audit.go +++ b/internal/audit/audit.go @@ -60,6 +60,12 @@ const ( // and its history moved onto the copy kept. The metadata names both, so the // log can answer "where did that file go" long after the report is gone. ActionDuplicateMerge Action = "duplicate_merge" + // The resolver settling a group without a merge (M498, 2026-10-19): + // dismissing two versions of a song that are not duplicates, and removing + // copies that filled another song's track slot (their tracks are kept, + // marked missing, and fetched again). + ActionDuplicateDismiss Action = "duplicate_dismiss" + ActionDuplicateWrongFile Action = "duplicate_wrong_file" // Subsonic password (#5026): generated in Settings for t/s-only clients. ActionSubsonicPasswordSet Action = "subsonic_password_set" diff --git a/internal/audit/audit_test.go b/internal/audit/audit_test.go index 46998384..19b0c07e 100644 --- a/internal/audit/audit_test.go +++ b/internal/audit/audit_test.go @@ -169,6 +169,8 @@ func TestWrite_AllActionConstantsArePersisted(t *testing.T) { audit.ActionForgotPasswordInit, audit.ActionPasswordResetByEmail, audit.ActionDuplicateMerge, + audit.ActionDuplicateDismiss, + audit.ActionDuplicateWrongFile, audit.ActionSubsonicPasswordSet, audit.ActionSubsonicPasswordClear, } diff --git a/internal/db/dbq/admin_summary.sql.go b/internal/db/dbq/admin_summary.sql.go index 6f682bdc..d1242e2f 100644 --- a/internal/db/dbq/admin_summary.sql.go +++ b/internal/db/dbq/admin_summary.sql.go @@ -26,11 +26,13 @@ SELECT SELECT created_at AS at FROM quarantine UNION ALL SELECT occurred_at FROM playback_errors WHERE resolved_at IS NULL) r)::timestamptz AS reports_since, + -- Duplicates needing review: a cross-release group is kept and linked by + -- the resolver and waits on nobody (the report's review view). (SELECT count(*) FROM duplicate_groups g - WHERE g.status = 'pending' + WHERE g.status = 'pending' AND g.class IS DISTINCT FROM 'cross_release' AND (SELECT count(*) FROM duplicate_group_members m WHERE m.group_id = g.id) >= 2)::bigint AS duplicates, (SELECT min(g.detected_at) FROM duplicate_groups g - WHERE g.status = 'pending' + WHERE g.status = 'pending' AND g.class IS DISTINCT FROM 'cross_release' AND (SELECT count(*) FROM duplicate_group_members m WHERE m.group_id = g.id) >= 2)::timestamptz AS duplicates_since, (SELECT count(*) FROM tracks WHERE source_verdict = 'suspect' AND missing_since IS NULL)::bigint AS suspect, (SELECT min(source_verdict_at) FROM tracks WHERE source_verdict = 'suspect' AND missing_since IS NULL)::timestamptz AS suspect_since, diff --git a/internal/db/dbq/duplicates.sql.go b/internal/db/dbq/duplicates.sql.go index 0ad1dd83..f6bf98d3 100644 --- a/internal/db/dbq/duplicates.sql.go +++ b/internal/db/dbq/duplicates.sql.go @@ -293,61 +293,10 @@ func (q *Queries) ListDuplicateCandidates(ctx context.Context, arg ListDuplicate return items, nil } -const listDuplicateGroupsDetectedSince = `-- name: ListDuplicateGroupsDetectedSince :many -SELECT ar.name AS artist_name, t.title, count(*) OVER ()::bigint AS total - FROM duplicate_groups g - JOIN LATERAL (SELECT m.track_id - FROM duplicate_group_members m - WHERE m.group_id = g.id - ORDER BY m.track_id - LIMIT 1) one ON true - JOIN tracks t ON t.id = one.track_id - JOIN artists ar ON ar.id = t.artist_id - WHERE g.status = 'pending' - AND g.detected_at >= $1 - ORDER BY g.detected_at DESC, g.id - LIMIT $2 -` - -type ListDuplicateGroupsDetectedSinceParams struct { - Since pgtype.Timestamptz - N int32 -} - -type ListDuplicateGroupsDetectedSinceRow struct { - ArtistName string - Title string - Total int64 -} - -// Pending proposals first made at or after `since`: what a sweep that started -// then found for the first time, each named by one of its songs, and `total` -// of them however many are listed. A refreshed proposal keeps its detected_at, -// so a sweep that only re-finds known groups lists none (M489: admins are told -// about new duplicates, not reminded of the same ones every sweep). -func (q *Queries) ListDuplicateGroupsDetectedSince(ctx context.Context, arg ListDuplicateGroupsDetectedSinceParams) ([]ListDuplicateGroupsDetectedSinceRow, error) { - rows, err := q.db.Query(ctx, listDuplicateGroupsDetectedSince, arg.Since, arg.N) - if err != nil { - return nil, err - } - defer rows.Close() - var items []ListDuplicateGroupsDetectedSinceRow - for rows.Next() { - var i ListDuplicateGroupsDetectedSinceRow - if err := rows.Scan(&i.ArtistName, &i.Title, &i.Total); err != nil { - return nil, err - } - items = append(items, i) - } - if err := rows.Err(); err != nil { - return nil, err - } - return items, nil -} - const listDuplicateGroupsForResolve = `-- name: ListDuplicateGroupsForResolve :many SELECT g.id AS group_id, g.tier, + g.class AS prior_class, t.id AS track_id, t.album_id, t.title, @@ -356,6 +305,7 @@ SELECT g.id AS group_id, t.file_path, t.file_format, t.file_size, + t.duration_ms, t.added_at, t.disc_number, t.track_number, @@ -364,32 +314,38 @@ SELECT g.id AS group_id, WHERE o.album_id = t.album_id AND o.id <> t.id AND o.missing_since IS NULL 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 + 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 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' WHERE g.status = 'pending' AND t.missing_since IS NULL ORDER BY g.id, t.id ` type ListDuplicateGroupsForResolveRow struct { - GroupID pgtype.UUID - Tier string - TrackID pgtype.UUID - AlbumID pgtype.UUID - Title string - SourceVerdict *string - ArtistName string - FilePath string - FileFormat string - FileSize int64 - AddedAt pgtype.Timestamptz - DiscNumber *int32 - TrackNumber *int32 - HasMbid bool - PositionClash bool + 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 } // Every pending group's present copies, for the resolver (M498 #5435): what it @@ -398,6 +354,9 @@ type ListDuplicateGroupsForResolveRow struct { // same album claims the same disc and track number: tags that do not fit the // album, which the survivor rule weighs against a copy. source_verdict says // whether the copy is a video rip ('suspect') or one the operator kept ('fine'). +// 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. func (q *Queries) ListDuplicateGroupsForResolve(ctx context.Context) ([]ListDuplicateGroupsForResolveRow, error) { rows, err := q.db.Query(ctx, listDuplicateGroupsForResolve) if err != nil { @@ -410,6 +369,7 @@ func (q *Queries) ListDuplicateGroupsForResolve(ctx context.Context) ([]ListDupl if err := rows.Scan( &i.GroupID, &i.Tier, + &i.PriorClass, &i.TrackID, &i.AlbumID, &i.Title, @@ -418,11 +378,13 @@ func (q *Queries) ListDuplicateGroupsForResolve(ctx context.Context) ([]ListDupl &i.FilePath, &i.FileFormat, &i.FileSize, + &i.DurationMs, &i.AddedAt, &i.DiscNumber, &i.TrackNumber, &i.HasMbid, &i.PositionClash, + &i.ReleaseGroupMbid, ); err != nil { return nil, err } @@ -633,6 +595,27 @@ func (q *Queries) SetDuplicateGroupClasses(ctx context.Context, arg SetDuplicate return err } +const settleDuplicateGroupAutomatically = `-- name: SettleDuplicateGroupAutomatically :execrows +UPDATE duplicate_groups + SET status = $1, resolved_at = now(), resolved_automatically = true + WHERE id = $2 AND status = 'pending' +` + +type SettleDuplicateGroupAutomaticallyParams struct { + Status string + ID pgtype.UUID +} + +// The resolver closing a group itself: 'dismissed' for versions that are not +// duplicates, 'merged' when it removed the wrong copies. Only a pending group. +func (q *Queries) SettleDuplicateGroupAutomatically(ctx context.Context, arg SettleDuplicateGroupAutomaticallyParams) (int64, error) { + result, err := q.db.Exec(ctx, settleDuplicateGroupAutomatically, arg.Status, arg.ID) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const startDuplicateSweep = `-- name: StartDuplicateSweep :one INSERT INTO duplicate_sweeps DEFAULT VALUES RETURNING id, started_at ` diff --git a/internal/db/migrations/0094_duplicate_distinct.down.sql b/internal/db/migrations/0094_duplicate_distinct.down.sql new file mode 100644 index 00000000..e5e44233 --- /dev/null +++ b/internal/db/migrations/0094_duplicate_distinct.down.sql @@ -0,0 +1,4 @@ +UPDATE duplicate_groups SET class = 'review' WHERE class = 'distinct'; +ALTER TABLE duplicate_groups DROP CONSTRAINT duplicate_groups_class_check; +ALTER TABLE duplicate_groups ADD CONSTRAINT duplicate_groups_class_check + CHECK (class IN ('same_release', 'cross_release', 'mismatch', 'review')); diff --git a/internal/db/migrations/0094_duplicate_distinct.up.sql b/internal/db/migrations/0094_duplicate_distinct.up.sql new file mode 100644 index 00000000..4f685da8 --- /dev/null +++ b/internal/db/migrations/0094_duplicate_distinct.up.sql @@ -0,0 +1,11 @@ +-- 0094_duplicate_distinct.up.sql — duplicates settle themselves (Scribe +-- milestone #498, operator 2026-10-19: "this needs to be automated away"). +-- +-- distinct: one album holds two versions of a song that sound alike ("Lovesick" +-- and "Lovesick (Instrumental)"). They are not duplicates, and the resolver +-- dismisses them itself. mismatch now also covers one album holding the same +-- audio in several track slots, the resolver's wrong-file case. +-- Rule 36: the new value swaps the constraint in the same migration. +ALTER TABLE duplicate_groups DROP CONSTRAINT duplicate_groups_class_check; +ALTER TABLE duplicate_groups ADD CONSTRAINT duplicate_groups_class_check + CHECK (class IN ('same_release', 'cross_release', 'mismatch', 'distinct', 'review')); diff --git a/internal/db/queries/admin_summary.sql b/internal/db/queries/admin_summary.sql index 97d91f86..5bcc1a72 100644 --- a/internal/db/queries/admin_summary.sql +++ b/internal/db/queries/admin_summary.sql @@ -18,11 +18,13 @@ SELECT SELECT created_at AS at FROM quarantine UNION ALL SELECT occurred_at FROM playback_errors WHERE resolved_at IS NULL) r)::timestamptz AS reports_since, + -- Duplicates needing review: a cross-release group is kept and linked by + -- the resolver and waits on nobody (the report's review view). (SELECT count(*) FROM duplicate_groups g - WHERE g.status = 'pending' + WHERE g.status = 'pending' AND g.class IS DISTINCT FROM 'cross_release' AND (SELECT count(*) FROM duplicate_group_members m WHERE m.group_id = g.id) >= 2)::bigint AS duplicates, (SELECT min(g.detected_at) FROM duplicate_groups g - WHERE g.status = 'pending' + WHERE g.status = 'pending' AND g.class IS DISTINCT FROM 'cross_release' AND (SELECT count(*) FROM duplicate_group_members m WHERE m.group_id = g.id) >= 2)::timestamptz AS duplicates_since, (SELECT count(*) FROM tracks WHERE source_verdict = 'suspect' AND missing_since IS NULL)::bigint AS suspect, (SELECT min(source_verdict_at) FROM tracks WHERE source_verdict = 'suspect' AND missing_since IS NULL)::timestamptz AS suspect_since, diff --git a/internal/db/queries/duplicates.sql b/internal/db/queries/duplicates.sql index 71902168..9a5d17c0 100644 --- a/internal/db/queries/duplicates.sql +++ b/internal/db/queries/duplicates.sql @@ -184,26 +184,6 @@ UPDATE duplicate_groups SET status = 'dismissed', resolved_at = now() WHERE id = sqlc.arg(id) AND status = 'pending'; --- name: ListDuplicateGroupsDetectedSince :many --- Pending proposals first made at or after `since`: what a sweep that started --- then found for the first time, each named by one of its songs, and `total` --- of them however many are listed. A refreshed proposal keeps its detected_at, --- so a sweep that only re-finds known groups lists none (M489: admins are told --- about new duplicates, not reminded of the same ones every sweep). -SELECT ar.name AS artist_name, t.title, count(*) OVER ()::bigint AS total - FROM duplicate_groups g - JOIN LATERAL (SELECT m.track_id - FROM duplicate_group_members m - WHERE m.group_id = g.id - ORDER BY m.track_id - LIMIT 1) one ON true - JOIN tracks t ON t.id = one.track_id - JOIN artists ar ON ar.id = t.artist_id - WHERE g.status = 'pending' - AND g.detected_at >= sqlc.arg(since) - ORDER BY g.detected_at DESC, g.id - LIMIT sqlc.arg(n); - -- name: ListDuplicateGroupsForResolve :many -- Every pending group's present copies, for the resolver (M498 #5435): what it -- needs to classify the group and to choose the copy to keep. One row per copy, @@ -211,8 +191,12 @@ SELECT ar.name AS artist_name, t.title, count(*) OVER ()::bigint AS total -- same album claims the same disc and track number: tags that do not fit the -- album, which the survivor rule weighs against a copy. source_verdict says -- whether the copy is a video rip ('suspect') or one the operator kept ('fine'). +-- 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. SELECT g.id AS group_id, g.tier, + g.class AS prior_class, t.id AS track_id, t.album_id, t.title, @@ -221,6 +205,7 @@ SELECT g.id AS group_id, t.file_path, t.file_format, t.file_size, + t.duration_ms, t.added_at, t.disc_number, t.track_number, @@ -229,11 +214,14 @@ SELECT g.id AS group_id, WHERE o.album_id = t.album_id AND o.id <> t.id AND o.missing_since IS NULL 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 + 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 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' WHERE g.status = 'pending' AND t.missing_since IS NULL ORDER BY g.id, t.id; @@ -251,5 +239,12 @@ UPDATE duplicate_groups g AND g.status = 'pending' AND (g.class IS DISTINCT FROM v.class OR g.resolve_note IS DISTINCT FROM NULLIF(v.note, '')); +-- name: SettleDuplicateGroupAutomatically :execrows +-- The resolver closing a group itself: 'dismissed' for versions that are not +-- duplicates, 'merged' when it removed the wrong copies. Only a pending group. +UPDATE duplicate_groups + SET status = sqlc.arg(status), resolved_at = now(), resolved_automatically = true + WHERE id = sqlc.arg(id) AND status = 'pending'; + -- name: MarkDuplicateGroupResolvedAutomatically :exec UPDATE duplicate_groups SET resolved_automatically = true WHERE id = sqlc.arg(id); diff --git a/internal/library/duplicate_classify.go b/internal/library/duplicate_classify.go index f27220f8..b8356b4d 100644 --- a/internal/library/duplicate_classify.go +++ b/internal/library/duplicate_classify.go @@ -6,21 +6,26 @@ import ( "unicode" ) -// Duplicate classification (M498 #5435). +// Duplicate classification (M498 #5435; settled further 2026-10-19). // // A duplicate group is one recording found more than once. What should happen -// to it depends on where the copies sit, and the measured library (milestone -// 498's body) splits into four shapes: +// to it depends on where the copies sit: // -// - same_release: every copy is on one album. An album holds a song once, so -// the extra copies are the ones to fold away. -// - cross_release: the copies are on different albums under one title. A -// single and the album it came from, a deluxe edition and the standard one: -// each copy belongs to its own release, so none is deleted (unless it is a -// video rip; the resolver decides). -// - mismatch: byte-identical audio on different albums under different -// titles. One file carries another song's tags — a wrong-file import. -// - review: anything else. The operator decides. +// - same_release: every copy is on one album under one title. An album holds +// a song once, so the extra copies are the ones to fold away. +// - cross_release: the copies are on different albums. A single and the +// album it came from, a deluxe edition and the standard one, or one +// recording a later release retitled ("The Ragtime Cat", "F.O.D. (live)", +// a romanized title): each copy belongs to its own release, so none is +// deleted (unless it is a video rip; the resolver decides). The measured +// library (2026-10-19) had 451 such groups waiting on the operator under +// different titles, and they were retitles, not different songs. +// - distinct: one album, two versions of a song that sound alike, named as +// such ("Lovesick", "Lovesick (Instrumental)"). Not duplicates. +// - mismatch: one album holds the same audio under different song titles: +// one file filling several track slots, a broken download. Which slot is +// right is the resolver's to decide from MusicBrainz's track lengths. +// - review: what none of these fits. The operator decides. // DuplicateClass is a group's shape. The values are duplicate_groups.class. type DuplicateClass string @@ -28,6 +33,7 @@ type DuplicateClass string const ( ClassSameRelease DuplicateClass = "same_release" ClassCrossRelease DuplicateClass = "cross_release" + ClassDistinct DuplicateClass = "distinct" ClassMismatch DuplicateClass = "mismatch" ClassReview DuplicateClass = "review" ) @@ -44,17 +50,17 @@ type ClassifyMember struct { // // Titles are compared by MatchTitleKey, which drops what a release or an upload // adds to a title (featuring credits, remaster notes, video markers) and keeps -// what names a different recording (live, demo, remix, instrumental). So one -// album holding "WWW" and "WWW (instrumental)" as an acoustic match is review, -// not a copy to fold away: the #3885 case, where they were in fact different -// recordings mislabelled. Byte-identical copies on one album are the same file -// twice whatever their tags say. +// what names a different recording (live, demo, remix, instrumental). On one +// album, titles that differ only by such a word are two versions (distinct) +// unless the bytes are identical, which is one file twice: the #3885 case of +// "WWW" and "WWW (instrumental)" holding the same file is a wrong slot. func ClassifyDuplicateGroup(tier string, members []ClassifyMember) DuplicateClass { if len(members) < 2 { return ClassReview } - oneAlbum, oneTitle := true, true + oneAlbum, oneTitle, oneSong := true, true, true key := MatchTitleKey(members[0].Title, members[0].ArtistName) + song := songKey(members[0].Title, members[0].ArtistName) for _, m := range members[1:] { if m.AlbumID != members[0].AlbumID { oneAlbum = false @@ -62,22 +68,33 @@ func ClassifyDuplicateGroup(tier string, members []ClassifyMember) DuplicateClas if MatchTitleKey(m.Title, m.ArtistName) != key { oneTitle = false } + if songKey(m.Title, m.ArtistName) != song { + oneSong = false + } } exact := tier == string(tierExact) switch { - case oneAlbum && (oneTitle || exact): - return ClassSameRelease - case oneAlbum: - return ClassReview - case oneTitle: + case !oneAlbum: return ClassCrossRelease - case exact: - return ClassMismatch + case oneTitle: + return ClassSameRelease + case oneSong && !exact: + return ClassDistinct default: - return ClassReview + return ClassMismatch } } +// songKey is MatchTitleKey with every bracketed segment dropped: the song a +// title names, whatever version of it. "Lovesick (Instrumental)" and +// "Lovesick" share one. +func songKey(title, artist string) string { + if key := MatchTitleKey(bracketed.ReplaceAllString(title, " "), artist); key != "" { + return key + } + return MatchTitleKey(title, artist) +} + var ( // A bracketed credit: "(feat. Panda Bear)", "[ft. X]", "(featuring Y)". featBracket = regexp.MustCompile(`(?i)[(\[][^)\]]*\b(feat\.?|ft\.?|featuring)\s[^)\]]*[)\]]`) diff --git a/internal/library/duplicate_classify_test.go b/internal/library/duplicate_classify_test.go index 07b38216..e9c21db6 100644 --- a/internal/library/duplicate_classify_test.go +++ b/internal/library/duplicate_classify_test.go @@ -1,6 +1,11 @@ package library -import "testing" +import ( + "fmt" + "testing" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) // The shapes measured on the operator's library (milestone 498's body). func TestClassifyDuplicateGroup(t *testing.T) { @@ -17,18 +22,22 @@ func TestClassifyDuplicateGroup(t *testing.T) { []ClassifyMember{m("humanz", "Saturnz Barz"), m("humanz", "Saturnz Barz (Official Video)")}, ClassSameRelease}, {"one album, video upload title with artist prefix", "acoustic", []ClassifyMember{m("humanz", "Andromeda"), m("humanz", "Gorillaz - Andromeda (Official Audio)")}, ClassSameRelease}, - {"one album, identical bytes under different titles", "exact", - []ClassifyMember{m("www", "WWW"), m("www", "WWW (instrumental)")}, ClassSameRelease}, - {"one album, acoustic match, different recordings named", "acoustic", - []ClassifyMember{m("www", "WWW"), m("www", "WWW (instrumental)")}, ClassReview}, + {"one album, identical bytes under a version title: one file in two slots", "exact", + []ClassifyMember{m("www", "WWW"), m("www", "WWW (instrumental)")}, ClassMismatch}, + {"one album, acoustic match, two versions named", "acoustic", + []ClassifyMember{m("moe", "Lovesick"), m("moe", "Lovesick (Instrumental)")}, ClassDistinct}, + {"Void: one album, one audio under three song titles", "acoustic", + []ClassifyMember{m("void", "Slimewar (into the void)"), m("void", "Void"), m("void", "Reach")}, ClassMismatch}, + {"Slasher and Slasher II are not versions of one song", "acoustic", + []ClassifyMember{m("slasher", "Slasher"), m("slasher", "Slasher II")}, ClassMismatch}, {"Big Me: the single and the album", "exact", []ClassifyMember{m("single", "Big Me"), m("album", "Big Me")}, ClassCrossRelease}, {"remaster and featuring credits are one song", "acoustic", []ClassifyMember{m("a", "Feel Good Inc. (2017 Remaster)"), m("b", "Feel Good Inc feat. De La Soul")}, ClassCrossRelease}, - {"Nervosa / Anorexia: identical bytes, different songs", "exact", - []ClassifyMember{m("a", "Nervosa"), m("b", "Anorexia")}, ClassMismatch}, - {"studio and live across albums", "acoustic", - []ClassifyMember{m("studio", "Charger"), m("live", "Charger (live)")}, ClassReview}, + {"a later release retitles the recording", "acoustic", + []ClassifyMember{m("coco", "Ragtime Cat"), m("single", "The Ragtime Cat")}, ClassCrossRelease}, + {"identical bytes on two albums under two titles", "exact", + []ClassifyMember{m("dookie", "F.O.D."), m("live", "F.O.D. (live)")}, ClassCrossRelease}, {"a lone copy", "exact", []ClassifyMember{m("a", "Song")}, ClassReview}, } for _, tc := range cases { @@ -59,3 +68,38 @@ func TestMatchTitleKey(t *testing.T) { } } } + +func TestWrongSlots(t *testing.T) { + track := func(title string, ms int32) dbq.MbReleaseTrack { return dbq.MbReleaseTrack{Title: title, LengthMs: ms} } + copies := func(titles ...string) []slotCopy { + out := make([]slotCopy, len(titles)) + for i, title := range titles { + out[i] = slotCopy{Title: title, Artist: "Savant", DurationMs: 220000} + } + return out + } + void := []dbq.MbReleaseTrack{track("Slimewar (Into the Void)", 300000), track("Reach", 281000), track("Void", 220500)} + cases := []struct { + name string + copies []slotCopy + tracks []dbq.MbReleaseTrack + keep int + losers []int + ok bool + }{ + {"Void: the file is Void's; the other two slots are other songs", copies("Slimewar (into the void)", "Void", "Reach"), void, 1, []int{0, 2}, true}, + {"no track list", copies("Void", "Reach"), nil, -1, nil, false}, + {"a title the track list does not hold", copies("Void", "Bonus"), void, -1, nil, false}, + {"two slots fit the file", copies("Void", "Reach"), []dbq.MbReleaseTrack{track("Void", 220000), track("Reach", 221500)}, -1, nil, false}, + {"a slot near but not within the fit", copies("Void", "Reach"), []dbq.MbReleaseTrack{track("Void", 220000), track("Reach", 226000)}, -1, nil, false}, + {"no slot fits", copies("Reach", "Slimewar (into the void)"), void, -1, nil, false}, + {"an unknown length counts for nothing", copies("Void", "Reach"), []dbq.MbReleaseTrack{track("Void", 220000), track("Reach", 0)}, -1, nil, false}, + } + for _, tc := range cases { + keep, losers, ok := wrongSlots(tc.copies, tc.tracks) + 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 acb4e0d4..e83786dc 100644 --- a/internal/library/duplicate_resolve.go +++ b/internal/library/duplicate_resolve.go @@ -31,6 +31,11 @@ import ( // - Except that a video rip is no release's copy. When every copy but one is // a rip, across releases or under another title, the rips are merged into // the one that is not. +// - 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). // - Anything else waits for the operator. const ( @@ -50,6 +55,9 @@ type DuplicateResolveResult struct { MergeFailed int // SongsLinked counts cross-release groups newly linked as one song. SongsLinked int + // Dismissed counts groups of distinct versions dismissed; WrongFiles the + // copies removed from track slots that were not theirs. + Dismissed, WrongFiles int // HeldBack counts tracks newly held back from radio and the mixes as video // rips; Released counts held-back tracks whose name no longer says so. HeldBack, Released int64 @@ -96,7 +104,7 @@ func mergePlan(g *resolveGroup) (ok bool, survivor int) { switch g.class { case ClassSameRelease: return true, -1 - case ClassCrossRelease, ClassMismatch: + case ClassCrossRelease: if i := g.lone(); i >= 0 { return true, i } @@ -136,6 +144,7 @@ func ResolveDuplicates( } if !act { + notifyNewForReview(ctx, groups, nil) return res, nil } @@ -172,17 +181,23 @@ func ResolveDuplicates( } } + res.Dismissed = dismissDistinct(ctx, pool, logger, groups, merged) + wrong, names := settleWrongSlots(ctx, pool, logger, groups, merged) + res.WrongFiles = wrong + mergedNames = append(mergedNames, names...) + linked, err := linkCrossRelease(ctx, pool, logger, groups, merged) res.SongsLinked = linked if err != nil { return res, err } - if res.Merged > 0 { + if settledCopies := res.Merged + res.WrongFiles; settledCopies > 0 { notifyAdmins(ctx, notifications.KindDuplicatesResolved, notifications.Payload{ - Count: int64(res.Merged), Items: mergedNames, + Count: int64(settledCopies), Items: mergedNames[:min(len(mergedNames), notifyItems)], }) } + notifyNewForReview(ctx, groups, merged) return res, nil } @@ -276,7 +291,7 @@ func resolveNote(g *resolveGroup) string { return "Each copy belongs to its own release, so none is removed; they count as one song." } case ClassMismatch: - return "Identical audio filed under different titles: one file carries another song's tags." + return "One recording fills several of this album's track slots, and MusicBrainz's track lengths could not say which is its own." } return "" } @@ -409,5 +424,6 @@ func (w *DuplicateResolveWorker) tickOnce(ctx context.Context) { } w.logger.Info("duplicate resolve complete", "groups", res.Groups, "merged", res.Merged, "merge_failed", res.MergeFailed, - "songs_linked", res.SongsLinked, "held_back", res.HeldBack, "released", res.Released) + "songs_linked", res.SongsLinked, "dismissed", res.Dismissed, "wrong_files", res.WrongFiles, + "held_back", res.HeldBack, "released", res.Released) } diff --git a/internal/library/duplicate_resolve_test.go b/internal/library/duplicate_resolve_test.go index 82d52f77..df99b67e 100644 --- a/internal/library/duplicate_resolve_test.go +++ b/internal/library/duplicate_resolve_test.go @@ -242,9 +242,10 @@ func TestResolveDuplicates_KeepsCleanCopiesOnTwoReleases_Integration(t *testing. } } -// Identical audio under two titles on two albums, neither a rip: which tags -// are wrong is the operator's call, so nothing is merged or linked. -func TestResolveDuplicates_LeavesAMismatchForTheOperator_Integration(t *testing.T) { +// Identical audio under two titles on two albums, neither a rip: a later +// release retitled the recording ("F.O.D." and "F.O.D. (live)"). Each copy +// belongs to its release, so nothing is removed and they count as one song. +func TestResolveDuplicates_LinksOneRecordingUnderTwoTitles_Integration(t *testing.T) { f := newCrossReleaseRipFixture(t) ctx := context.Background() if _, err := f.pool.Exec(ctx, `UPDATE duplicate_groups SET tier = 'exact' WHERE id = $1`, f.groupID); err != nil { @@ -258,11 +259,166 @@ func TestResolveDuplicates_LeavesAMismatchForTheOperator_Integration(t *testing. if err != nil { t.Fatal(err) } - if res.Classes[ClassMismatch] != 1 || res.Merged != 0 || res.SongsLinked != 0 { - t.Fatalf("classes %v, merged %d, linked %d; want a mismatch left alone", res.Classes, res.Merged, res.SongsLinked) + if res.Classes[ClassCrossRelease] != 1 || res.Merged != 0 || res.SongsLinked != 1 { + t.Fatalf("classes %v, merged %d, linked %d; want the pair linked and kept", res.Classes, res.Merged, res.SongsLinked) } - if status, _, note, _ := (resolveFixture{pool: f.pool, groupID: f.groupID}).group(t); status != "pending" || note == nil || *note == "" { - t.Errorf("group = (%s, note %v), want pending with a note", status, note) +} + +// oneAlbumFixture is one album holding two tracks in one pending group, each +// with a file on disk. +type oneAlbumFixture struct { + pool *pgxpool.Pool + album dbq.Album + a, b dbq.Track + aPath string + bPath string + groupID pgtype.UUID +} + +func newOneAlbumFixture(t *testing.T, tier, titleA, titleB string) oneAlbumFixture { + t.Helper() + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + dir := filepath.Join(t.TempDir(), "Savant", "Void") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + f := oneAlbumFixture{pool: pool, aPath: filepath.Join(dir, "15 - a.m4a"), bPath: filepath.Join(dir, "17 - b.m4a")} + for _, p := range []string{f.aPath, f.bPath} { + if err := os.WriteFile(p, []byte("audio"), 0o644); err != nil { + t.Fatal(err) + } + } + var artist dbq.Artist + f.a, f.album, artist = seedTrack(t, pool, f.aPath) + b, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ + Title: titleB, AlbumID: f.album.ID, ArtistID: artist.ID, + DurationMs: 220000, FilePath: f.bPath, FileSize: 100, FileFormat: "m4a", + }) + if err != nil { + t.Fatal(err) + } + f.b = b + if _, err := pool.Exec(ctx, `UPDATE tracks SET title = $2, duration_ms = 220000 WHERE id = $1`, f.a.ID, titleA); err != nil { + t.Fatal(err) + } + if err := pool.QueryRow(ctx, + `INSERT INTO duplicate_groups (member_key, tier) VALUES ($1, $2) RETURNING id`, t.Name(), tier, + ).Scan(&f.groupID); err != nil { + t.Fatal(err) + } + if _, err := pool.Exec(ctx, `INSERT INTO duplicate_group_members (group_id, track_id) VALUES ($1, $2), ($1, $3)`, + f.groupID, f.a.ID, f.b.ID); err != nil { + t.Fatal(err) + } + return f +} + +func auditCount(t *testing.T, pool *pgxpool.Pool, action string, groupID pgtype.UUID) int { + t.Helper() + var n int + if err := pool.QueryRow(context.Background(), + `SELECT count(*) FROM audit_log WHERE action = $1 AND metadata->>'group_id' = $2`, + action, syncpkg.FormatUUID(groupID)).Scan(&n); err != nil { + t.Fatal(err) + } + return n +} + +// Two versions of a song on one album that sound alike are not duplicates: +// the group is dismissed and both files stay. +func TestResolveDuplicates_DismissesTwoVersionsOfASong_Integration(t *testing.T) { + f := newOneAlbumFixture(t, "acoustic", "Lovesick", "Lovesick (Instrumental)") + res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", true) + if err != nil { + t.Fatal(err) + } + if res.Dismissed != 1 || res.Merged != 0 || !exists(f.aPath) || !exists(f.bPath) { + t.Fatalf("result %+v; want the group dismissed and both files kept", res) + } + if status, _, _, auto := (resolveFixture{pool: f.pool, groupID: f.groupID}).group(t); status != "dismissed" || !auto { + t.Errorf("group = (%s, auto=%v), want dismissed by the resolver", status, auto) + } + if n := auditCount(t, f.pool, "duplicate_dismiss", f.groupID); n != 1 { + t.Errorf("dismiss audit rows = %d, want 1", n) + } +} + +// seedTrackList holds a release group whose one release lists the songs with +// their lengths, and points the album at it. +func seedTrackList(t *testing.T, pool *pgxpool.Pool, album dbq.Album, songs map[string]int) { + t.Helper() + ctx := context.Background() + rg := "rg-" + syncpkg.FormatUUID(album.ID) + rel := "rel-" + syncpkg.FormatUUID(album.ID) + for _, stmt := range []struct { + sql string + args []any + }{ + {`INSERT INTO mb_release_groups (mbid, title) VALUES ($1, 'Void')`, []any{rg}}, + {`INSERT INTO mb_releases (mbid, release_group_mbid, title) VALUES ($1, $2, 'Void')`, []any{rel, rg}}, + {`UPDATE albums SET release_group_mbid = $2 WHERE id = $1`, []any{album.ID, rg}}, + } { + if _, err := pool.Exec(ctx, stmt.sql, stmt.args...); err != nil { + t.Fatalf("exec %q: %v", stmt.sql, err) + } + } + pos := 1 + for title, ms := range songs { + if _, err := pool.Exec(ctx, + `INSERT INTO mb_release_tracks (release_mbid, medium_position, position, title, length_ms) VALUES ($1, 1, $2, $3, $4)`, + rel, pos, title, ms); err != nil { + t.Fatal(err) + } + pos++ + } +} + +// One recording in two slots of an album: MusicBrainz says Void is as long as +// the file and Reach is a minute longer, so the copy in Reach's slot goes. Its +// track stays, marked missing, for the album to be fetched again. +func TestResolveDuplicates_RemovesACopyFromAnotherSongsSlot_Integration(t *testing.T) { + f := newOneAlbumFixture(t, "acoustic", "Void", "Reach") + seedTrackList(t, f.pool, f.album, map[string]int{"Void": 220500, "Reach": 281000}) + 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, void exists %v, reach exists %v; want only Reach's copy removed", + res, exists(f.aPath), exists(f.bPath)) + } + var missing bool + if err := f.pool.QueryRow(context.Background(), + `SELECT missing_since IS NOT NULL FROM tracks WHERE id = $1`, f.b.ID).Scan(&missing); err != nil { + t.Fatalf("Reach's track is gone: %v", err) + } + if !missing { + t.Error("Reach's track is not marked missing, so nothing will fetch it again") + } + if status, _, _, auto := (resolveFixture{pool: f.pool, groupID: f.groupID}).group(t); status != "merged" || !auto { + t.Errorf("group = (%s, auto=%v), want settled by the resolver", status, auto) + } + if n := auditCount(t, f.pool, "duplicate_wrong_file", f.groupID); n != 1 { + t.Errorf("wrong-file audit rows = %d, want 1", n) + } +} + +// Without a track list that decides, nothing is removed. +func TestResolveDuplicates_LeavesAnUndecidedSlotForTheOperator_Integration(t *testing.T) { + f := newOneAlbumFixture(t, "acoustic", "Void", "Reach") + seedTrackList(t, f.pool, f.album, map[string]int{"Void": 220500, "Reach": 221000}) + res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", true) + if err != nil { + t.Fatal(err) + } + if res.WrongFiles != 0 || !exists(f.aPath) || !exists(f.bPath) { + t.Fatalf("result %+v; want both copies kept when the lengths cannot tell", res) + } + if status, class, _, _ := (resolveFixture{pool: f.pool, groupID: f.groupID}).group(t); status != "pending" || + class == nil || *class != string(ClassMismatch) { + t.Errorf("group = (%s, %v), want a pending mismatch", status, class) } } diff --git a/internal/library/duplicate_settle.go b/internal/library/duplicate_settle.go new file mode 100644 index 00000000..cea06672 --- /dev/null +++ b/internal/library/duplicate_settle.go @@ -0,0 +1,262 @@ +package library + +import ( + "context" + "errors" + "fmt" + "log/slog" + "os" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/audit" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/notifications" + syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" +) + +// Duplicates that settle without a merge (M498, operator 2026-10-19: "this +// needs to be automated away"). The measured library had 536 groups waiting +// on the operator; most were retitles across releases, now linked like any +// cross-release group. What is left on one album is two shapes: +// +// - distinct: two versions of a song that sound alike ("Lovesick" and +// "Lovesick (Instrumental)"). Not duplicates, so the group is dismissed. +// - mismatch: one file filling several track slots (Savant's Void held one +// recording as tracks 15, 17 and 20). The album's MusicBrainz track list +// says how long each slot's song is. When exactly one slot's song is as +// long as the file and every other slot's song is clearly not, the copies +// 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. +// Anything less certain stays with the operator. + +const ( + // 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. + slotFit = 3000 + slotOther = 10000 +) + +// slotCopy is what deciding a wrong slot needs to know about one copy. +type slotCopy struct { + Title, Artist string + DurationMs int32 +} + +// wrongSlots decides which copies of a one-album mismatch sit in a slot that +// is not theirs. keep is the copy whose slot fits the recording; ok is false +// when the track list cannot say, and nothing should be removed. +func wrongSlots(copies []slotCopy, tracks []dbq.MbReleaseTrack) (keep int, losers []int, ok bool) { + keep = -1 + for i, c := range copies { + key := MatchTitleKey(c.Title, c.Artist) + fits, other, known := false, true, false + for _, t := range tracks { + if t.LengthMs <= 0 || MatchTitleKey(t.Title, c.Artist) != key { + continue + } + known = true + d := absMs(t.LengthMs - c.DurationMs) + if d <= slotFit { + fits = true + } + if d <= slotOther { + other = false + } + } + switch { + case fits: + if keep >= 0 { + return -1, nil, false + } + keep = i + case known && other: + losers = append(losers, i) + default: + return -1, nil, false + } + } + if keep < 0 || len(losers) == 0 { + return -1, nil, false + } + return keep, losers, true +} + +func absMs(d int32) int32 { + if d < 0 { + return -d + } + return d +} + +// dismissDistinct dismisses every group of distinct versions and records it +// for the Resolved automatically tab. It adds what it settled to settled. +func dismissDistinct( + ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, groups []*resolveGroup, settled map[pgtype.UUID]bool, +) int { + q := dbq.New(pool) + n := 0 + for _, g := range groups { + if g.class != ClassDistinct || settled[g.id] || ctx.Err() != nil { + continue + } + rows, err := q.SettleDuplicateGroupAutomatically(ctx, dbq.SettleDuplicateGroupAutomaticallyParams{ + ID: g.id, Status: "dismissed", + }) + if err != nil { + logger.Warn("duplicate resolve: dismiss failed", "group_id", syncpkg.FormatUUID(g.id), "err", err) + continue + } + if rows == 0 { + continue + } + settled[g.id] = true + n++ + titles := make([]string, len(g.members)) + for i, m := range g.members { + titles[i] = m.Title + } + audit.WriteOrLog(ctx, pool, logger, pgtype.UUID{}, pgtype.UUID{}, audit.ActionDuplicateDismiss, map[string]any{ + "automatic": true, + "group_id": syncpkg.FormatUUID(g.id), + "artist": g.members[0].ArtistName, + "titles": titles, + }) + } + return n +} + +// 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. +func settleWrongSlots( + ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, groups []*resolveGroup, settled map[pgtype.UUID]bool, +) (int, []string) { + q := dbq.New(pool) + tracksByGroup := map[string][]dbq.MbReleaseTrack{} + removed := 0 + var names []string + for _, g := range groups { + if g.class != ClassMismatch || settled[g.id] || ctx.Err() != nil { + continue + } + rg := g.members[0].ReleaseGroupMbid + if rg == "" { + continue + } + tracks, seen := tracksByGroup[rg] + if !seen { + var err error + 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)) + for i, m := range g.members { + copies[i] = slotCopy{Title: m.Title, Artist: m.ArtistName, DurationMs: m.DurationMs} + } + keep, losers, ok := wrongSlots(copies, tracks) + if !ok { + continue + } + n, err := removeWrongSlots(ctx, pool, logger, g, keep, losers) + removed += n + if err != nil { + logger.Warn("duplicate resolve: removing wrong-slot copies failed", "group_id", syncpkg.FormatUUID(g.id), "err", err) + continue + } + settled[g.id] = true + for _, i := range losers { + names = append(names, TrackLabel(g.members[i].ArtistName, g.members[i].Title)) + } + } + return removed, names +} + +// 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 +// 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, +) (int, error) { + var gone []pgtype.UUID + var removedFiles []map[string]string + var removeErr error + for _, i := range losers { + m := g.members[i] + if err := removeFile(m.FilePath); err != nil && !errors.Is(err, os.ErrNotExist) { + removeErr = fmt.Errorf("remove %s: %w", m.FilePath, err) + break + } + gone = append(gone, m.TrackID) + removedFiles = append(removedFiles, map[string]string{ + "track_id": syncpkg.FormatUUID(m.TrackID), "file_path": m.FilePath, "title": m.Title, + }) + } + if len(gone) == 0 { + return 0, removeErr + } + err := pgx.BeginFunc(ctx, pool, func(tx pgx.Tx) error { + tq := dbq.New(tx) + if _, err := tq.MarkTracksMissing(ctx, gone); err != nil { + return fmt.Errorf("mark missing: %w", err) + } + if err := logTrackChanges(ctx, tq, gone); err != nil { + return fmt.Errorf("log changes: %w", err) + } + if removeErr != nil { + return nil + } + _, err := tq.SettleDuplicateGroupAutomatically(ctx, dbq.SettleDuplicateGroupAutomaticallyParams{ + ID: g.id, Status: "merged", + }) + return err + }) + if err != nil { + return 0, err + } + kept := g.members[keep] + audit.WriteOrLog(ctx, pool, logger, pgtype.UUID{}, pgtype.UUID{}, audit.ActionDuplicateWrongFile, map[string]any{ + "automatic": true, + "group_id": syncpkg.FormatUUID(g.id), + "artist": kept.ArtistName, + "kept_title": kept.Title, + "kept_path": kept.FilePath, + "removed": removedFiles, + }) + logger.Info("duplicate resolve: removed copies from slots that were not theirs", + "group_id", syncpkg.FormatUUID(g.id), "kept", kept.FilePath, "removed", len(gone)) + return len(gone), removeErr +} + +// notifyNewForReview tells admins about groups this pass classified for the +// first time and left for them. A group the resolver settled, linked or will +// link is not news: telling admins about those was most of what the notice +// used to say. +func notifyNewForReview(ctx context.Context, groups []*resolveGroup, settled map[pgtype.UUID]bool) { + var count int64 + var items []string + for _, g := range groups { + if g.members[0].PriorClass != nil || settled[g.id] { + continue + } + if g.class != ClassReview && g.class != ClassMismatch { + continue + } + count++ + if len(items) < notifyItems { + items = append(items, TrackLabel(g.members[0].ArtistName, g.members[0].Title)) + } + } + if count > 0 { + notifyAdmins(ctx, notifications.KindDuplicatesFound, notifications.Payload{Count: count, Items: items}) + } +} diff --git a/internal/library/duplicate_sweep.go b/internal/library/duplicate_sweep.go index 42e500e5..34dd2cb8 100644 --- a/internal/library/duplicate_sweep.go +++ b/internal/library/duplicate_sweep.go @@ -14,7 +14,6 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" - "git.fabledsword.com/bvandeusen/minstrel/internal/notifications" syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" ) @@ -102,38 +101,12 @@ func runDuplicateSweep( } } - if runErr == nil { - notifyNewDuplicates(finishCtx, q, sweep.StartedAt, logger) - } - logger.Info("duplicate sweep complete", "candidates", res.Candidates, "groups", res.Groups, "proposed", res.Proposed, "suppressed", res.Suppressed, "retired", res.Retired, "oversize", res.Oversize, "err", runErr) return res, runErr } -// notifyNewDuplicates tells admins when a sweep proposed groups it had not -// proposed before (M489), counting and naming those (M542). A sweep that only -// re-finds known groups says nothing, so reading the notice once is enough -// until something new turns up. -func notifyNewDuplicates(ctx context.Context, q *dbq.Queries, sweepStarted pgtype.Timestamptz, logger *slog.Logger) { - fresh, err := q.ListDuplicateGroupsDetectedSince(ctx, dbq.ListDuplicateGroupsDetectedSinceParams{ - Since: sweepStarted, N: notifyItems, - }) - if err != nil { - logger.Warn("duplicate sweep: listing new proposals failed", "err", err) - return - } - if len(fresh) == 0 { - return - } - items := make([]string, 0, len(fresh)) - for _, g := range fresh { - items = append(items, TrackLabel(g.ArtistName, g.Title)) - } - notifyAdmins(ctx, notifications.KindDuplicatesFound, notifications.Payload{Count: fresh[0].Total, Items: items}) -} - func sweepDuplicates( ctx context.Context, q *dbq.Queries, sweepID pgtype.UUID, cfg FingerprintSettings, pageSize int32, ) (DuplicateSweepResult, error) { diff --git a/internal/library/notify_test.go b/internal/library/notify_test.go index e3c51163..9308f95f 100644 --- a/internal/library/notify_test.go +++ b/internal/library/notify_test.go @@ -1,7 +1,6 @@ package library import ( - "bytes" "context" "encoding/json" "fmt" @@ -155,68 +154,54 @@ func TestNotifyStillMissing_NamesOnlyTheTracksStillGone(t *testing.T) { } } -func TestDuplicateSweep_NotifiesOnlyWhenSomethingNewIsProposed(t *testing.T) { +// Admins hear about a duplicate group once, when a resolver pass first leaves +// it for them. A group the resolver settles or links is never news, and a +// group already seen by an earlier pass is not news again. +func TestResolveDuplicates_TellsAdminsOnlyOfNewGroupsLeftForThem(t *testing.T) { pool := newPool(t) admin := notifyingAdmin(t, pool) ctx := context.Background() q := dbq.New(pool) dir := t.TempDir() - logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - - _, album, artist := seedTrack(t, pool, filepath.Join(dir, "seed.mp3")) - // Same audio-stream hash: an exact pair, whatever the prints say. - pair := func(name string, b byte, seed uint64) { - t.Helper() - for i := 1; i <= 2; i++ { - tr, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ - Title: name, AlbumID: album.ID, ArtistID: artist.ID, DurationMs: 200000, - FilePath: filepath.Join(dir, fmt.Sprintf("%s-%d.mp3", name, i)), FileSize: 100, FileFormat: "mp3", - }) - if err != nil { - t.Fatalf("track %s: %v", name, err) - } - if err := q.UpsertTrackFingerprint(ctx, dbq.UpsertTrackFingerprintParams{ - TrackID: tr.ID, AudioStreamSha256: bytes.Repeat([]byte{b}, 32), - Chromaprint: randomPrint(seed+uint64(i), printLen), FingerprintVersion: fingerprintVersion, - ChromaprintLengthSec: defaultChromaprintLengthSec, - }); err != nil { - t.Fatalf("fingerprint %s: %v", name, err) - } - } + first, album, artist := seedTrack(t, pool, filepath.Join(dir, "a.mp3")) + if _, err := pool.Exec(ctx, `UPDATE tracks SET title = 'Alpha' WHERE id = $1`, first.ID); err != nil { + t.Fatal(err) } - sweep := func() { - t.Helper() - if _, err := runDuplicateSweep(ctx, pool, logger, DefaultFingerprintSettings, duplicateCandidatePage); err != nil { - t.Fatalf("sweep: %v", err) - } + second, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ + Title: "Beta", AlbumID: album.ID, ArtistID: artist.ID, + DurationMs: 1000, FilePath: filepath.Join(dir, "b.mp3"), FileSize: 100, FileFormat: "mp3", + }) + if err != nil { + t.Fatal(err) } - markAllRead := func() { - t.Helper() - if _, err := q.MarkAllNotificationsRead(ctx, dbq.MarkAllNotificationsReadParams{UserID: admin.ID}); err != nil { - t.Fatalf("mark read: %v", err) - } + // One album, one recording under two song titles, no track list to decide. + var groupID pgtype.UUID + if err := pool.QueryRow(ctx, + `INSERT INTO duplicate_groups (member_key, tier) VALUES ('notify-review', 'exact') RETURNING id`, + ).Scan(&groupID); err != nil { + t.Fatal(err) + } + if _, err := pool.Exec(ctx, `INSERT INTO duplicate_group_members (group_id, track_id) VALUES ($1, $2), ($1, $3)`, + groupID, first.ID, second.ID); err != nil { + t.Fatal(err) } - pair("first", 1, 100) - sweep() + if _, err := ResolveDuplicates(ctx, pool, nil, "", true); err != nil { + t.Fatal(err) + } got := unreadNotices(t, pool, admin) if len(got) != 1 || got[0].kind != string(notifications.KindDuplicatesFound) || got[0].count != 1 || - len(got[0].items) != 1 || got[0].items[0] != "Delete Test Artist – first" { - t.Fatalf("after the first sweep notices = %+v, want one duplicates_found naming the first pair", got) + len(got[0].items) != 1 || got[0].items[0] == "" { + t.Fatalf("after the first pass notices = %+v, want one duplicates_found naming the group", got) } - // Read, then swept again with nothing new: no reminder. - markAllRead() - sweep() + if _, err := q.MarkAllNotificationsRead(ctx, dbq.MarkAllNotificationsReadParams{UserID: admin.ID}); err != nil { + t.Fatal(err) + } + if _, err := ResolveDuplicates(ctx, pool, nil, "", true); err != nil { + t.Fatal(err) + } if got := unreadNotices(t, pool, admin); len(got) != 0 { - t.Fatalf("a sweep that found nothing new notified: %+v", got) - } - - // A new pair is news, and the notice is about it alone: the first was read. - pair("second", 2, 200) - sweep() - got = unreadNotices(t, pool, admin) - if len(got) != 1 || got[0].count != 1 || len(got[0].items) != 1 || got[0].items[0] != "Delete Test Artist – second" { - t.Fatalf("after a new pair notices = %+v, want one naming only the new pair", got) + t.Fatalf("a group an earlier pass saw was told again: %+v", got) } } diff --git a/internal/notifications/render.go b/internal/notifications/render.go index 96aeac20..7e618bf5 100644 --- a/internal/notifications/render.go +++ b/internal/notifications/render.go @@ -136,7 +136,7 @@ func Render(kind Kind, payload []byte) Rendered { body := "Extra copies of a song were merged into the one the album keeps." switch names := nameList(p.Items, p.Count); { case names != "": - body = "Extra copies merged into the one the album keeps: " + names + "." + body = "Extra copies removed, the right one kept: " + names + "." case p.Detail != "": body = p.Detail } diff --git a/internal/notifications/render_test.go b/internal/notifications/render_test.go index f4509f73..5ecd299b 100644 --- a/internal/notifications/render_test.go +++ b/internal/notifications/render_test.go @@ -56,7 +56,7 @@ func TestRender_NamesWhatChanged(t *testing.T) { require.Equal(t, "Held more than once: A, B, C and 9 more.", render(KindDuplicatesFound, Payload{Count: 12, Items: []string{"A", "B", "C", "D", "E"}}).Body) require.Equal(t, "12 new duplicates to review", render(KindDuplicatesFound, Payload{Count: 12}).Title) - require.Equal(t, "Extra copies merged into the one the album keeps: A and B.", + require.Equal(t, "Extra copies removed, the right one kept: A and B.", render(KindDuplicatesResolved, Payload{Count: 2, Items: []string{"A", "B"}}).Body) require.Equal(t, "Failed to play: Weezer – Buddy Holly (alice).", render(KindPlaybackErrors, Payload{Count: 1, Items: []string{"Weezer – Buddy Holly (alice)"}}).Body) diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index af84ca84..b81363ab 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -448,7 +448,7 @@ export type AdminDuplicateMember = { }; // The resolver's verdict on a group (M498). null until its first pass. -export type DuplicateClass = 'same_release' | 'cross_release' | 'mismatch' | 'review'; +export type DuplicateClass = 'same_release' | 'cross_release' | 'distinct' | 'mismatch' | 'review'; // exact: identical encoded audio. acoustic: the same recording, differently // encoded; worst_bit_error_rate is the weakest link between any two members. @@ -504,14 +504,20 @@ export type AdminDuplicatesResponse = { export type DuplicatesView = 'review' | 'cross_release'; // One merge the resolver made on its own (M498), from the audit log. +// What the resolver did by itself: merged a group, dismissed two versions of a +// song as not duplicates, or removed copies that filled another song's track +// slot (their tracks stay, marked missing, and the album is fetched again). export type ResolvedDuplicateActivity = { id: string; - action: 'duplicate_merge'; + action: 'duplicate_merge' | 'duplicate_dismiss' | 'duplicate_wrong_file'; created_at: string; details: { survivor_path?: string; survivor_reason?: string; - removed?: { track_id: string; file_path: string }[]; + removed?: { track_id: string; file_path: string; title?: string }[]; + artist?: string; + titles?: string[]; + kept_title?: string; }; }; diff --git a/web/src/routes/collection/duplicates/+page.svelte b/web/src/routes/collection/duplicates/+page.svelte index fc42523b..c1e054bd 100644 --- a/web/src/routes/collection/duplicates/+page.svelte +++ b/web/src/routes/collection/duplicates/+page.svelte @@ -188,8 +188,10 @@

- Tracks that hold the same recording more than once. A song held twice on one album is merged on - its own, and so is a video rip with a clean copy elsewhere. What's left here needs you. + Tracks that hold the same recording more than once. Most settle on their own: a song held twice + on one album is merged, copies on different releases count as one song, two versions of a song + are left alone, and a file filling another song's track slot is removed so the album is fetched + again. What's left here needs you.

@@ -233,7 +235,7 @@

Nothing resolved automatically yet.

- Merges made on their own are listed here. + What Minstrel settled on its own is listed here.

{:else} @@ -241,16 +243,31 @@ {#each activity as item (item.id)} {@const removed = item.details.removed ?? []}
  • -

    - Kept {basename(item.details.survivor_path ?? '')} - {#if item.details.survivor_reason} - ({item.details.survivor_reason}) - {/if} - and removed {plural(removed.length, 'file', 'files')}. -

    - {#each removed as r (r.track_id)} -

    {r.file_path}

    - {/each} + {#if item.action === 'duplicate_dismiss'} +

    + {item.details.artist}: {(item.details.titles ?? []).join(' and ')} are different versions, + not duplicates. Both kept. +

    + {:else if item.action === 'duplicate_wrong_file'} +

    + {item.details.artist}: the recording of {item.details.kept_title} also filled the + {plural(removed.length, 'slot', 'slots')} of {removed.map((r) => r.title).join(', ')}. + Removed; the album will be fetched again. +

    + {:else} +

    + Kept {basename(item.details.survivor_path ?? '')} + {#if item.details.survivor_reason} + ({item.details.survivor_reason}) + {/if} + and removed {plural(removed.length, 'file', 'files')}. +

    + {/if} + {#if item.action !== 'duplicate_dismiss'} + {#each removed as r (r.track_id)} +

    {r.file_path}

    + {/each} + {/if}

    {relativeTime(item.created_at)}

    • {/each} diff --git a/web/src/routes/collection/duplicates/duplicates.test.ts b/web/src/routes/collection/duplicates/duplicates.test.ts index 14dcbd76..4de1dead 100644 --- a/web/src/routes/collection/duplicates/duplicates.test.ts +++ b/web/src/routes/collection/duplicates/duplicates.test.ts @@ -354,6 +354,34 @@ describe('admin duplicates', () => { expect(items[0]).toContain('Kept 03 - Saturnz Barz.mp3 (the copy that is not a video rip) and removed 1 file.'); expect(screen.queryByTestId('duplicate-group')).toBeNull(); }); + + test('Resolved automatically says what was dismissed and which slots were emptied', async () => { + renderWith(response(), [ + { + id: 'a-2', + action: 'duplicate_dismiss', + created_at: ago(HOUR), + details: { artist: 'Moe Shop', titles: ['Lovesick', 'Lovesick (Instrumental)'] } + }, + { + id: 'a-3', + action: 'duplicate_wrong_file', + created_at: ago(HOUR), + details: { + artist: 'Savant', + kept_title: 'Void', + removed: [{ track_id: 't-9', file_path: '/data/Music/Savant/Void/17 - Reach.m4a', title: 'Reach' }] + } + } + ]); + await fireEvent.click(screen.getByRole('tab', { name: /resolved automatically/i })); + const items = screen.getAllByTestId('resolved-item').map(text); + expect(items[0]).toContain('Moe Shop: Lovesick and Lovesick (Instrumental) are different versions, not duplicates. Both kept.'); + expect(items[1]).toContain( + 'Savant: the recording of Void also filled the slot of Reach. Removed; the album will be fetched again.' + ); + expect(items[1]).toContain('/data/Music/Savant/Void/17 - Reach.m4a'); + }); }); describe("the resolver's note", () => {