From 5306a261c1e76f5e431206ff67ef9cc9d13033c9 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 9 Oct 2026 20:12:49 -0400 Subject: [PATCH] feat: duplicates resolve themselves without Lidarr (M502 #5517) Lidarr is off, and with it the resolver had stopped merging: it only merged when Lidarr had said which copies it mapped. Minstrel is now the only program writing the library, so the rule is about the copies: - A song held twice on one album: the extra copies merge into the one whose tags fit best (then lossless, larger, older). - Across releases or under another title: when every copy but one is a video rip (held back, or named like one and not marked fine), the rips merge into the one that is not. - Clean copies on different releases are linked as one song, as before. - Anything else waits for the operator. Gone with Lidarr: release switches and their settle window, the unmapped-file read, the rip search, the merge guard and its 409/503, lidarr.Client's release methods, and duplicate_group_members.lidarr_state (migration 0078). The duplicates page drops the per-copy Lidarr badge and the blocked-merge notice; past release changes stay in its history. Co-Authored-By: Claude Opus 5.5 --- cmd/minstrel/main.go | 16 +- internal/api/admin_duplicates.go | 22 +- internal/api/admin_duplicates_test.go | 19 +- internal/db/dbq/duplicates.sql.go | 97 +-- internal/db/dbq/models.go | 5 +- internal/db/dbq/tracks.sql.go | 72 -- .../0078_drop_duplicate_lidarr_state.down.sql | 3 + .../0078_drop_duplicate_lidarr_state.up.sql | 9 + internal/db/queries/duplicates.sql | 28 +- internal/db/queries/tracks.sql | 37 - internal/library/duplicate_classify.go | 13 +- internal/library/duplicate_classify_test.go | 10 - internal/library/duplicate_merge.go | 28 - internal/library/duplicate_resolve.go | 649 +++--------------- internal/library/duplicate_resolve_test.go | 486 +++---------- internal/library/duplicate_survivor.go | 15 +- internal/library/duplicate_survivor_test.go | 17 +- internal/library/song_link_test.go | 6 +- internal/lidarr/releases.go | 175 ----- internal/lidarr/releases_test.go | 138 ---- internal/server/server.go | 11 - internal/tracks/service.go | 50 +- web/src/lib/api/types.ts | 4 - .../components/FingerprintSettingsCard.svelte | 7 +- web/src/lib/styles/error-copy.json | 2 - web/src/routes/admin/duplicates/+page.svelte | 79 +-- .../admin/duplicates/duplicates.test.ts | 46 +- 27 files changed, 301 insertions(+), 1743 deletions(-) create mode 100644 internal/db/migrations/0078_drop_duplicate_lidarr_state.down.sql create mode 100644 internal/db/migrations/0078_drop_duplicate_lidarr_state.up.sql delete mode 100644 internal/lidarr/releases.go delete mode 100644 internal/lidarr/releases_test.go diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index 95df672f..5036d8d1 100644 --- a/cmd/minstrel/main.go +++ b/cmd/minstrel/main.go @@ -325,18 +325,12 @@ func run() error { library.SetNotifier(notifier) go lidarrReconciler.Run(ctx) - // Duplicate resolver (M498): classifies the sweep's groups and, with the - // operator's auto-resolve on, merges copies Lidarr does not map and moves a - // Lidarr release that lists songs twice. Started after SetNotifier so its - // summaries reach admins. See internal/library/duplicate_resolve.go. - resolverLidarr := func() library.LidarrLibrary { - if c := lidarrClientFn(); c != nil { - return c - } - return nil // a nil *lidarr.Client must not become a non-nil interface - } + // 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. go library.NewDuplicateResolveWorker(pool, logger.With("component", "duplicate_resolve"), - cfg.Storage.DataDir, fpSettings, resolverLidarr).Run(ctx) + cfg.Storage.DataDir, fpSettings).Run(ctx) // Missing-file re-acquisition (milestone #290). Turns albums whose files // have been gone longer than the grace window into Lidarr requests, on an diff --git a/internal/api/admin_duplicates.go b/internal/api/admin_duplicates.go index 0bc48c42..a21bd08b 100644 --- a/internal/api/admin_duplicates.go +++ b/internal/api/admin_duplicates.go @@ -14,7 +14,6 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/audit" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/library" - "git.fabledsword.com/bvandeusen/minstrel/internal/tracks" ) // duplicateMemberView is one copy in a proposed duplicate group. LikeCount and @@ -33,12 +32,8 @@ type duplicateMemberView struct { AddedAt string `json:"added_at"` LikeCount int64 `json:"like_count"` PlayCount int64 `json:"play_count"` - // LidarrState is Lidarr's view of the file as of the resolver's last pass - // (M498): "tracked" (mapped to a track, so it cannot be removed without - // Lidarr downloading it again), "unmapped", or null when not asked. - LidarrState *string `json:"lidarr_state"` - DiscNumber *int32 `json:"disc_number"` - TrackNumber *int32 `json:"track_number"` + DiscNumber *int32 `json:"disc_number"` + TrackNumber *int32 `json:"track_number"` } // duplicateGroupView is one proposal. SurvivorTrackID and SurvivorReason are @@ -291,14 +286,12 @@ func foldDuplicateGroups(rows []dbq.ListPendingDuplicateGroupMembersRow) []dupli AddedAt: formatTimestamp(row.AddedAt), LikeCount: row.LikeCount, PlayCount: row.PlayCount, - LidarrState: row.LidarrState, DiscNumber: row.DiscNumber, TrackNumber: row.TrackNumber, }) candidates[n] = append(candidates[n], library.SurvivorCandidate{ TrackID: trackID, FileFormat: row.FileFormat, FileSize: row.FileSize, AddedAt: row.AddedAt.Time, - LidarrTracked: row.LidarrState != nil && *row.LidarrState == "tracked", - TagFit: library.TagFitScore(row.TrackNumber != nil, row.PositionClash, row.FilePath, row.HasMbid), + TagFit: library.TagFitScore(row.TrackNumber != nil, row.PositionClash, row.FilePath, row.HasMbid), }) } for i := range groups { @@ -398,10 +391,6 @@ const mergeRequestBodyLimit = 1 << 16 // Errors: // - 409 library_not_writable / 500 file_delete_failed when a file could not be // removed — nothing was changed (fileRemoveAPIError) -// - 409 copy_tracked_by_lidarr when a copy to remove is one Lidarr maps to a -// track: removing it would make Lidarr download it again (M498) -// - 503 lidarr_unavailable when Lidarr is on but did not answer, so that -// cannot be checked // - 404 duplicate_group_not_pending when the group was already resolved // - 400 survivor_not_in_group, invalid_id, invalid_body func (h *handlers) handleMergeDuplicateGroup(w http.ResponseWriter, r *http.Request) { @@ -439,11 +428,6 @@ func (h *handlers) handleMergeDuplicateGroup(w http.ResponseWriter, r *http.Requ writeAdminJSONErr(w, http.StatusNotFound, "duplicate_group_not_pending") case errors.Is(err, library.ErrSurvivorNotInGroup): writeAdminJSONErr(w, http.StatusBadRequest, "survivor_not_in_group") - case errors.Is(err, library.ErrCopyTrackedByLidarr): - writeAdminJSONErr(w, http.StatusConflict, "copy_tracked_by_lidarr") - case errors.Is(err, tracks.ErrLidarrUnavailable): - h.logger.Warn("admin: merge duplicate group: lidarr unavailable", "group_id", uuidToString(groupID), "err", err) - writeAdminJSONErr(w, http.StatusServiceUnavailable, "lidarr_unavailable") default: h.logger.Error("admin: merge duplicate group", "group_id", uuidToString(groupID), "err", err) writeAdminJSONErr(w, http.StatusInternalServerError, "server_error") diff --git a/internal/api/admin_duplicates_test.go b/internal/api/admin_duplicates_test.go index e67cfe56..26f4e814 100644 --- a/internal/api/admin_duplicates_test.go +++ b/internal/api/admin_duplicates_test.go @@ -65,27 +65,26 @@ func TestFoldDuplicateGroups(t *testing.T) { } // M498: the proposal shown beside a group follows the resolver's rule. The copy -// Lidarr tracks is proposed even when the other is lossless, because removing -// the tracked one would make Lidarr download it again. -func TestFoldDuplicateGroups_ProposesTheCopyLidarrTracks(t *testing.T) { +// whose tags fit the album is proposed even when the other is lossless. +func TestFoldDuplicateGroups_ProposesTheCopyWhoseTagsFit(t *testing.T) { at := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC) - tracked, unmapped := "tracked", "unmapped" class := "same_release" + three := int32(3) rows := []dbq.ListPendingDuplicateGroupMembersRow{ {GroupID: dupUUID(3), Tier: "acoustic", DetectedAt: dupTS(at), TrackID: dupUUID(30), Class: &class, - Title: "Song", FileFormat: "flac", FileSize: 30_000_000, AddedAt: dupTS(at), LidarrState: &unmapped}, + Title: "Song", FileFormat: "flac", FileSize: 30_000_000, AddedAt: dupTS(at)}, {GroupID: dupUUID(3), Tier: "acoustic", DetectedAt: dupTS(at), TrackID: dupUUID(31), Class: &class, - Title: "Song", FileFormat: "mp3", FileSize: 9_000_000, AddedAt: dupTS(at), LidarrState: &tracked}, + Title: "Song", FileFormat: "mp3", FileSize: 9_000_000, AddedAt: dupTS(at), TrackNumber: &three, HasMbid: true}, } got := foldDuplicateGroups(rows) if len(got) != 1 { t.Fatalf("folded %d groups, want 1", len(got)) } g := got[0] - if g.SurvivorTrackID != uuidToString(dupUUID(31)) || g.SurvivorReason != "the copy Lidarr tracks" { - t.Errorf("survivor = (%s, %q), want the tracked mp3", g.SurvivorTrackID, g.SurvivorReason) + if g.SurvivorTrackID != uuidToString(dupUUID(31)) || g.SurvivorReason != "tags fit the album" { + t.Errorf("survivor = (%s, %q), want the tagged mp3", g.SurvivorTrackID, g.SurvivorReason) } - if g.Class == nil || *g.Class != "same_release" || g.Members[1].LidarrState == nil || *g.Members[1].LidarrState != "tracked" { - t.Errorf("class or lidarr state not carried: %+v", g) + if g.Class == nil || *g.Class != "same_release" { + t.Errorf("class not carried: %+v", g) } } diff --git a/internal/db/dbq/duplicates.sql.go b/internal/db/dbq/duplicates.sql.go index 51aaf8be..422b5e58 100644 --- a/internal/db/dbq/duplicates.sql.go +++ b/internal/db/dbq/duplicates.sql.go @@ -210,32 +210,6 @@ func (q *Queries) GetLatestFingerprintComputedAt(ctx context.Context) (pgtype.Ti return latest, err } -const listAlbumPresentTrackTitles = `-- name: ListAlbumPresentTrackTitles :many -SELECT title FROM tracks WHERE album_id = $1 AND missing_since IS NULL -` - -// The titles an album has on disk, for choosing the Lidarr release that covers -// them best (M498 #5437). -func (q *Queries) ListAlbumPresentTrackTitles(ctx context.Context, albumID pgtype.UUID) ([]string, error) { - rows, err := q.db.Query(ctx, listAlbumPresentTrackTitles, albumID) - if err != nil { - return nil, err - } - defer rows.Close() - var items []string - for rows.Next() { - var title string - if err := rows.Scan(&title); err != nil { - return nil, err - } - items = append(items, title) - } - if err := rows.Err(); err != nil { - return nil, err - } - return items, nil -} - const listDismissedDuplicateMemberSets = `-- name: ListDismissedDuplicateMemberSets :many SELECT g.id, array_agg(m.track_id ORDER BY m.track_id)::uuid[] AS track_ids FROM duplicate_groups g @@ -343,6 +317,7 @@ SELECT g.id AS group_id, t.id AS track_id, t.album_id, t.title, + t.source_verdict, artists.name AS artist_name, t.file_path, t.file_format, @@ -351,8 +326,6 @@ SELECT g.id AS group_id, t.disc_number, t.track_number, (t.mbid IS NOT NULL)::boolean AS has_mbid, - albums.release_group_mbid, - albums.title AS album_title, EXISTS (SELECT 1 FROM tracks o 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 @@ -361,7 +334,6 @@ SELECT g.id AS group_id, 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 albums ON albums.id = t.album_id JOIN artists ON artists.id = t.artist_id WHERE g.status = 'pending' AND t.missing_since IS NULL @@ -369,29 +341,29 @@ SELECT g.id AS group_id, ` type ListDuplicateGroupsForResolveRow struct { - GroupID pgtype.UUID - Tier string - TrackID pgtype.UUID - AlbumID pgtype.UUID - Title string - ArtistName string - FilePath string - FileFormat string - FileSize int64 - AddedAt pgtype.Timestamptz - DiscNumber *int32 - TrackNumber *int32 - HasMbid bool - ReleaseGroupMbid *string - AlbumTitle string - PositionClash bool + 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 } // 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, // grouped by group. position_clash is true when another present track on the // same album claims the same disc and track number: tags that do not fit the -// album, which the survivor rule weighs against a copy. +// 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'). func (q *Queries) ListDuplicateGroupsForResolve(ctx context.Context) ([]ListDuplicateGroupsForResolveRow, error) { rows, err := q.db.Query(ctx, listDuplicateGroupsForResolve) if err != nil { @@ -407,6 +379,7 @@ func (q *Queries) ListDuplicateGroupsForResolve(ctx context.Context) ([]ListDupl &i.TrackID, &i.AlbumID, &i.Title, + &i.SourceVerdict, &i.ArtistName, &i.FilePath, &i.FileFormat, @@ -415,8 +388,6 @@ func (q *Queries) ListDuplicateGroupsForResolve(ctx context.Context) ([]ListDupl &i.DiscNumber, &i.TrackNumber, &i.HasMbid, - &i.ReleaseGroupMbid, - &i.AlbumTitle, &i.PositionClash, ); err != nil { return nil, err @@ -487,7 +458,6 @@ SELECT p.id AS group_id, p.detected_at, p.class, p.resolve_note, - m.lidarr_state, t.disc_number, t.track_number, (t.mbid IS NOT NULL)::boolean AS has_mbid, @@ -529,7 +499,6 @@ type ListPendingDuplicateGroupMembersRow struct { DetectedAt pgtype.Timestamptz Class *string ResolveNote *string - LidarrState *string DiscNumber *int32 TrackNumber *int32 HasMbid bool @@ -552,8 +521,7 @@ type ListPendingDuplicateGroupMembersRow struct { // newest first, flattened to one row per member so the handler folds them // without a query per group. What each copy carries — likes and plays from // every user — is here because it is what the operator weighs when deciding -// which copy to keep; Lidarr's view of it, because a copy Lidarr tracks cannot -// be removed without Lidarr downloading it again. +// which copy to keep. func (q *Queries) ListPendingDuplicateGroupMembers(ctx context.Context, arg ListPendingDuplicateGroupMembersParams) ([]ListPendingDuplicateGroupMembersRow, error) { rows, err := q.db.Query(ctx, listPendingDuplicateGroupMembers, arg.View, arg.PageOffset, arg.PageLimit) if err != nil { @@ -570,7 +538,6 @@ func (q *Queries) ListPendingDuplicateGroupMembers(ctx context.Context, arg List &i.DetectedAt, &i.Class, &i.ResolveNote, - &i.LidarrState, &i.DiscNumber, &i.TrackNumber, &i.HasMbid, @@ -632,30 +599,6 @@ func (q *Queries) SetDuplicateGroupClasses(ctx context.Context, arg SetDuplicate return err } -const setDuplicateMemberLidarrStates = `-- name: SetDuplicateMemberLidarrStates :exec -UPDATE duplicate_group_members m - SET lidarr_state = NULLIF(v.state, '') - FROM (SELECT unnest($1::uuid[]) AS group_id, - unnest($2::uuid[]) AS track_id, - unnest($3::text[]) AS state) v - WHERE m.group_id = v.group_id - AND m.track_id = v.track_id - AND m.lidarr_state IS DISTINCT FROM NULLIF(v.state, '') -` - -type SetDuplicateMemberLidarrStatesParams struct { - GroupIds []pgtype.UUID - TrackIds []pgtype.UUID - States []string -} - -// Lidarr's view of each copy as of this pass; an empty state clears it (Lidarr -// not consulted). Unchanged rows are left alone. -func (q *Queries) SetDuplicateMemberLidarrStates(ctx context.Context, arg SetDuplicateMemberLidarrStatesParams) error { - _, err := q.db.Exec(ctx, setDuplicateMemberLidarrStates, arg.GroupIds, arg.TrackIds, arg.States) - return err -} - const startDuplicateSweep = `-- name: StartDuplicateSweep :one INSERT INTO duplicate_sweeps DEFAULT VALUES RETURNING id, started_at ` diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index 556c54c9..bcaa80ca 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -337,9 +337,8 @@ type DuplicateGroup struct { } type DuplicateGroupMember struct { - GroupID pgtype.UUID - TrackID pgtype.UUID - LidarrState *string + GroupID pgtype.UUID + TrackID pgtype.UUID } type DuplicateSweep struct { diff --git a/internal/db/dbq/tracks.sql.go b/internal/db/dbq/tracks.sql.go index 9cc88467..05387d86 100644 --- a/internal/db/dbq/tracks.sql.go +++ b/internal/db/dbq/tracks.sql.go @@ -405,78 +405,6 @@ func (q *Queries) GetTracksByIDs(ctx context.Context, dollar_1 []pgtype.UUID) ([ return items, nil } -const listAlbumsWithSoleCopyRips = `-- name: ListAlbumsWithSoleCopyRips :many -SELECT a.id AS album_id, - a.release_group_mbid::text AS release_group_mbid, - a.title AS album_title, - ar.name AS artist_name, - count(*)::bigint AS rips - FROM tracks t - JOIN albums a ON a.id = t.album_id - JOIN artists ar ON ar.id = a.artist_id - WHERE t.source_verdict = 'suspect' - AND t.missing_since IS NULL - AND a.release_group_mbid IS NOT NULL - AND NOT EXISTS ( - SELECT 1 FROM tracks c - WHERE c.song_key = t.song_key AND c.id <> t.id - AND c.missing_since IS NULL AND c.source_verdict IS DISTINCT FROM 'suspect') - AND NOT EXISTS ( - SELECT 1 FROM duplicate_group_members m - JOIN duplicate_groups g ON g.id = m.group_id AND g.status = 'pending' - JOIN duplicate_group_members m2 ON m2.group_id = g.id AND m2.track_id <> t.id - JOIN tracks c ON c.id = m2.track_id - WHERE m.track_id = t.id - AND c.missing_since IS NULL AND c.source_verdict IS DISTINCT FROM 'suspect') - AND NOT EXISTS ( - SELECT 1 FROM audit_log l - WHERE l.action = 'lidarr_rip_search' - AND l.metadata->>'album_id' = a.id::text - AND l.created_at > now() - interval '7 days') - GROUP BY a.id, a.release_group_mbid, a.title, ar.name - ORDER BY count(*) DESC, a.id - LIMIT $1 -` - -type ListAlbumsWithSoleCopyRipsRow struct { - AlbumID pgtype.UUID - ReleaseGroupMbid string - AlbumTitle string - ArtistName string - Rips int64 -} - -// Albums holding a held-back video rip that is the only copy of its song -// (#5447): no present copy that is not a rip shares its song, and no pending -// duplicate group pairs it with one. Lidarr counts such a track fulfilled, so -// the resolver asks it to search the album for a better release. Albums -// searched within the last week are left out, most rips first. -func (q *Queries) ListAlbumsWithSoleCopyRips(ctx context.Context, pageLimit int32) ([]ListAlbumsWithSoleCopyRipsRow, error) { - rows, err := q.db.Query(ctx, listAlbumsWithSoleCopyRips, pageLimit) - if err != nil { - return nil, err - } - defer rows.Close() - var items []ListAlbumsWithSoleCopyRipsRow - for rows.Next() { - var i ListAlbumsWithSoleCopyRipsRow - if err := rows.Scan( - &i.AlbumID, - &i.ReleaseGroupMbid, - &i.AlbumTitle, - &i.ArtistName, - &i.Rips, - ); err != nil { - return nil, err - } - items = append(items, i) - } - if err := rows.Err(); err != nil { - return nil, err - } - return items, nil -} - const listArtistTracksForUser = `-- name: ListArtistTracksForUser :many SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, t.song_id, t.song_key, t.source_verdict, t.source_verdict_at, albums.title AS album_title, diff --git a/internal/db/migrations/0078_drop_duplicate_lidarr_state.down.sql b/internal/db/migrations/0078_drop_duplicate_lidarr_state.down.sql new file mode 100644 index 00000000..b325605d --- /dev/null +++ b/internal/db/migrations/0078_drop_duplicate_lidarr_state.down.sql @@ -0,0 +1,3 @@ +ALTER TABLE duplicate_group_members ADD COLUMN lidarr_state text; +ALTER TABLE duplicate_group_members ADD CONSTRAINT duplicate_group_members_lidarr_state_check + CHECK (lidarr_state IN ('tracked', 'unmapped')); diff --git a/internal/db/migrations/0078_drop_duplicate_lidarr_state.up.sql b/internal/db/migrations/0078_drop_duplicate_lidarr_state.up.sql new file mode 100644 index 00000000..a7c2637d --- /dev/null +++ b/internal/db/migrations/0078_drop_duplicate_lidarr_state.up.sql @@ -0,0 +1,9 @@ +-- 0078_drop_duplicate_lidarr_state.up.sql — the duplicate resolver works +-- without Lidarr (Scribe milestone #502, #5517). +-- +-- 0075 recorded Lidarr's view of each duplicate copy (tracked or unmapped) +-- because a copy Lidarr mapped could not be removed without Lidarr downloading +-- it again. Lidarr is gone: Minstrel is the only program writing the library, +-- so no copy is anyone else's to keep, and the column has nothing to say. +ALTER TABLE duplicate_group_members DROP CONSTRAINT IF EXISTS duplicate_group_members_lidarr_state_check; +ALTER TABLE duplicate_group_members DROP COLUMN IF EXISTS lidarr_state; diff --git a/internal/db/queries/duplicates.sql b/internal/db/queries/duplicates.sql index 7ca041be..d003bd71 100644 --- a/internal/db/queries/duplicates.sql +++ b/internal/db/queries/duplicates.sql @@ -131,8 +131,7 @@ SELECT count(*)::bigint -- newest first, flattened to one row per member so the handler folds them -- without a query per group. What each copy carries — likes and plays from -- every user — is here because it is what the operator weighs when deciding --- which copy to keep; Lidarr's view of it, because a copy Lidarr tracks cannot --- be removed without Lidarr downloading it again. +-- which copy to keep. WITH page AS ( SELECT g.id, g.tier, g.worst_bit_error_rate, g.detected_at, g.class, g.resolve_note FROM duplicate_groups g @@ -151,7 +150,6 @@ SELECT p.id AS group_id, p.detected_at, p.class, p.resolve_note, - m.lidarr_state, t.disc_number, t.track_number, (t.mbid IS NOT NULL)::boolean AS has_mbid, @@ -201,12 +199,14 @@ SELECT count(*)::bigint -- needs to classify the group and to choose the copy to keep. One row per copy, -- grouped by group. position_clash is true when another present track on the -- same album claims the same disc and track number: tags that do not fit the --- album, which the survivor rule weighs against a copy. +-- 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'). SELECT g.id AS group_id, g.tier, t.id AS track_id, t.album_id, t.title, + t.source_verdict, artists.name AS artist_name, t.file_path, t.file_format, @@ -215,8 +215,6 @@ SELECT g.id AS group_id, t.disc_number, t.track_number, (t.mbid IS NOT NULL)::boolean AS has_mbid, - albums.release_group_mbid, - albums.title AS album_title, EXISTS (SELECT 1 FROM tracks o 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 @@ -225,7 +223,6 @@ SELECT g.id AS group_id, 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 albums ON albums.id = t.album_id JOIN artists ON artists.id = t.artist_id WHERE g.status = 'pending' AND t.missing_since IS NULL @@ -244,22 +241,5 @@ 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: SetDuplicateMemberLidarrStates :exec --- Lidarr's view of each copy as of this pass; an empty state clears it (Lidarr --- not consulted). Unchanged rows are left alone. -UPDATE duplicate_group_members m - SET lidarr_state = NULLIF(v.state, '') - FROM (SELECT unnest(sqlc.arg(group_ids)::uuid[]) AS group_id, - unnest(sqlc.arg(track_ids)::uuid[]) AS track_id, - unnest(sqlc.arg(states)::text[]) AS state) v - WHERE m.group_id = v.group_id - AND m.track_id = v.track_id - AND m.lidarr_state IS DISTINCT FROM NULLIF(v.state, ''); - -- name: MarkDuplicateGroupResolvedAutomatically :exec UPDATE duplicate_groups SET resolved_automatically = true WHERE id = sqlc.arg(id); - --- name: ListAlbumPresentTrackTitles :many --- The titles an album has on disk, for choosing the Lidarr release that covers --- them best (M498 #5437). -SELECT title FROM tracks WHERE album_id = sqlc.arg(album_id) AND missing_since IS NULL; diff --git a/internal/db/queries/tracks.sql b/internal/db/queries/tracks.sql index dd7b367d..449ac0fb 100644 --- a/internal/db/queries/tracks.sql +++ b/internal/db/queries/tracks.sql @@ -334,40 +334,3 @@ UPDATE tracks SET source_verdict = sqlc.arg(verdict)::text, source_verdict_at = now() WHERE id = sqlc.arg(id) AND source_verdict IS NOT NULL; - --- name: ListAlbumsWithSoleCopyRips :many --- Albums holding a held-back video rip that is the only copy of its song --- (#5447): no present copy that is not a rip shares its song, and no pending --- duplicate group pairs it with one. Lidarr counts such a track fulfilled, so --- the resolver asks it to search the album for a better release. Albums --- searched within the last week are left out, most rips first. -SELECT a.id AS album_id, - a.release_group_mbid::text AS release_group_mbid, - a.title AS album_title, - ar.name AS artist_name, - count(*)::bigint AS rips - FROM tracks t - JOIN albums a ON a.id = t.album_id - JOIN artists ar ON ar.id = a.artist_id - WHERE t.source_verdict = 'suspect' - AND t.missing_since IS NULL - AND a.release_group_mbid IS NOT NULL - AND NOT EXISTS ( - SELECT 1 FROM tracks c - WHERE c.song_key = t.song_key AND c.id <> t.id - AND c.missing_since IS NULL AND c.source_verdict IS DISTINCT FROM 'suspect') - AND NOT EXISTS ( - SELECT 1 FROM duplicate_group_members m - JOIN duplicate_groups g ON g.id = m.group_id AND g.status = 'pending' - JOIN duplicate_group_members m2 ON m2.group_id = g.id AND m2.track_id <> t.id - JOIN tracks c ON c.id = m2.track_id - WHERE m.track_id = t.id - AND c.missing_since IS NULL AND c.source_verdict IS DISTINCT FROM 'suspect') - AND NOT EXISTS ( - SELECT 1 FROM audit_log l - WHERE l.action = 'lidarr_rip_search' - AND l.metadata->>'album_id' = a.id::text - AND l.created_at > now() - interval '7 days') - GROUP BY a.id, a.release_group_mbid, a.title, ar.name - ORDER BY count(*) DESC, a.id - LIMIT sqlc.arg(page_limit); diff --git a/internal/library/duplicate_classify.go b/internal/library/duplicate_classify.go index 70430d73..f27220f8 100644 --- a/internal/library/duplicate_classify.go +++ b/internal/library/duplicate_classify.go @@ -13,11 +13,11 @@ import ( // 498's body) splits into four shapes: // // - same_release: every copy is on one album. An album holds a song once, so -// the extra copies are the ones to fold away — when Lidarr does not need -// them (the resolver checks). +// 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 fulfils its own release in Lidarr, so none is deleted. +// 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. @@ -123,13 +123,6 @@ func MatchTitleKey(title, artist string) string { return alnumKey(title) } -// ReleaseTitleKey is the lighter comparison for asking whether one release -// lists a song twice: case, "&" and punctuation only. Every word stays, so a -// box set's "Song" and "Song (demo)" are two songs, as the release means them. -func ReleaseTitleKey(title string) string { - return alnumKey(title) -} - // alnumKey lowercases, reads "&" as "and", and keeps only letters and digits. func alnumKey(s string) string { s = strings.ReplaceAll(strings.ToLower(s), "&", " and ") diff --git a/internal/library/duplicate_classify_test.go b/internal/library/duplicate_classify_test.go index 81f4e9f6..07b38216 100644 --- a/internal/library/duplicate_classify_test.go +++ b/internal/library/duplicate_classify_test.go @@ -59,13 +59,3 @@ func TestMatchTitleKey(t *testing.T) { } } } - -// The release check keeps every word: a box set's demo is its own song. -func TestReleaseTitleKey(t *testing.T) { - if ReleaseTitleKey("Song (demo)") == ReleaseTitleKey("Song") { - t.Error("ReleaseTitleKey collapsed a demo into the song") - } - if ReleaseTitleKey("Hallelujah Money") != ReleaseTitleKey("hallelujah money!") { - t.Error("ReleaseTitleKey kept case or punctuation") - } -} diff --git a/internal/library/duplicate_merge.go b/internal/library/duplicate_merge.go index aa5e411b..1856b514 100644 --- a/internal/library/duplicate_merge.go +++ b/internal/library/duplicate_merge.go @@ -25,16 +25,6 @@ var ErrDuplicateGroupNotPending = errors.New("library: duplicate group is not pe // ErrSurvivorNotInGroup means the copy chosen to keep is not a member of the group. var ErrSurvivorNotInGroup = errors.New("library: survivor is not a member of the group") -// ErrCopyTrackedByLidarr means a copy the merge would remove is one Lidarr maps -// to a track of the release it monitors (M498). Removing it would open a hole -// that Lidarr fills by downloading the song again, so the merge refuses and -// changes nothing. -var ErrCopyTrackedByLidarr = errors.New("library: a copy to remove is tracked by Lidarr") - -// RemovableFunc reports whether the file at filePath may be removed. A merge -// asks it about every copy it would remove, before touching anything. -type RemovableFunc func(filePath string) bool - // MergedCopy is one copy a merge kept or removed. type MergedCopy struct { TrackID pgtype.UUID @@ -85,17 +75,6 @@ type MergeResult struct { func MergeDuplicateGroup( ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string, groupID, survivorID pgtype.UUID, -) (MergeResult, error) { - return MergeDuplicateGroupGuarded(ctx, pool, logger, dataDir, groupID, survivorID, nil) -} - -// MergeDuplicateGroupGuarded is MergeDuplicateGroup with a check on what it may -// remove: when removable says no to any copy that would go, the merge returns -// ErrCopyTrackedByLidarr (wrapped with the path) and changes nothing. A nil -// removable allows every copy. -func MergeDuplicateGroupGuarded( - ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string, - groupID, survivorID pgtype.UUID, removable RemovableFunc, ) (MergeResult, error) { if logger == nil { logger = slog.Default() @@ -130,13 +109,6 @@ func MergeDuplicateGroupGuarded( return MergeResult{}, err } - if removable != nil { - for _, l := range losers { - if !removable(l.FilePath) { - return MergeResult{}, fmt.Errorf("%w: %s", ErrCopyTrackedByLidarr, l.FilePath) - } - } - } for _, l := range losers { if err := removeTrackFileOnDisk(l.FilePath); err != nil { return MergeResult{}, err diff --git a/internal/library/duplicate_resolve.go b/internal/library/duplicate_resolve.go index 7344b969..8a81d80d 100644 --- a/internal/library/duplicate_resolve.go +++ b/internal/library/duplicate_resolve.go @@ -2,12 +2,9 @@ package library import ( "context" - "encoding/json" "errors" "fmt" "log/slog" - "sort" - "strings" "time" "github.com/jackc/pgx/v5" @@ -16,100 +13,46 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/audit" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" - "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" "git.fabledsword.com/bvandeusen/minstrel/internal/notifications" syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" ) -// Duplicate resolver (M498 #5435, #5437). +// Duplicate resolver (M498 #5435, #5437; without Lidarr since M502 #5517). // // The sweep proposes duplicate groups; the resolver decides what each one is -// and settles the ones that are safe to settle without asking. Safety is -// Lidarr's to define: Lidarr maps one file to each track of the release it -// monitors, and re-downloads any mapped file that disappears. So: +// and settles the ones that are safe to settle without asking. Minstrel is the +// only program writing the library, so no copy is kept because something else +// depends on it. What decides is what the copies are: // -// - A copy Lidarr maps is never removed. -// - On one album, copies Lidarr does not map are merged into the copy it does -// (or, when it maps none, into the best by ProposeSurvivor). -// - When Lidarr maps every copy on one album, the release it monitors lists -// the song twice: the Humanz box set, 14 vinyl sides then the digital -// medium. The resolver moves Lidarr to a release of that album that lists -// each song once. Lidarr then rescans the folder, maps one copy and leaves -// the rest unmapped, and the next pass merges them. -// -// The release change has a fixed point (lesson #4183): it happens only while -// the monitored release repeats a title, and the release it chooses repeats -// none, so the next pass finds nothing to change. - -// LidarrLibrary is the part of Lidarr's API the resolver uses. *lidarr.Client -// satisfies it. -type LidarrLibrary interface { - ListUnmappedTrackFiles(ctx context.Context) ([]lidarr.TrackFile, error) - LookupAlbumByMBID(ctx context.Context, mbid string) (lidarr.LidarrAlbum, error) - ListAlbumTracks(ctx context.Context, albumID int) ([]lidarr.ReleaseTrack, error) - GetAlbumReleases(ctx context.Context, albumID int) ([]lidarr.AlbumRelease, error) - ListReleaseTracks(ctx context.Context, releaseID int) ([]lidarr.ReleaseTrack, error) - SetMonitoredRelease(ctx context.Context, albumID, releaseID int) error - SearchAlbums(ctx context.Context, albumIDs []int) error -} +// - On one album, a song is held once: the extra copies are merged into the +// best by ProposeSurvivor. +// - Across releases, each copy belongs to its own release (a single and its +// album): nothing is removed, and the copies are linked as one song. +// - 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. +// - Anything else waits for the operator. const ( // resolveMergeCap bounds the merges one pass makes. The first pass after - // deploy meets the whole backlog (433 groups measured on 2026-10-09); a - // cap spreads it over a few hours, each pass short. + // deploy meets the whole backlog; a cap spreads it over a few hours, each + // pass short. resolveMergeCap = 200 - // resolveReleaseCap bounds the Lidarr release changes one pass makes. Each - // makes Lidarr rescan an artist folder. - resolveReleaseCap = 10 - // releaseChangeSettle is how long after a release change the resolver - // leaves that album's groups alone. Lidarr unmaps every file of the album - // and rescans; until the rescan lands, every copy reads as unmapped, and a - // merge then could remove the very file Lidarr is about to map. - releaseChangeSettle = 24 * time.Hour // duplicateResolveTick is how often the worker runs a pass. duplicateResolveTick = time.Hour ) -const ( - lidarrStateTracked = "tracked" - lidarrStateUnmapped = "unmapped" -) - -// ReleaseChange is one Lidarr release the resolver switched. -type ReleaseChange struct { - AlbumTitle string - ArtistName string - From, To lidarr.AlbumRelease -} - // DuplicateResolveResult tallies one pass. type DuplicateResolveResult struct { - Groups int - Classes map[DuplicateClass]int - LidarrConsulted bool - Merged int - MergeFailed int - ReleaseChanges []ReleaseChange + Groups int + Classes map[DuplicateClass]int + Merged int + MergeFailed int // SongsLinked counts cross-release groups newly linked as one song. SongsLinked 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 - // RipSearches counts albums Lidarr was asked to search for a better copy - // of a rip that has no clean copy (#5447). - RipSearches int -} - -// LidarrPathKey is the part of a path Minstrel and Lidarr agree on: the last -// three components (artist folder, album folder, file). Both see the library -// under their own mount; on the operator's deploy both happen to say /music, -// and the key keeps the match from depending on that. -func LidarrPathKey(p string) string { - parts := strings.Split(strings.Trim(p, "/"), "/") - if len(parts) > 3 { - parts = parts[len(parts)-3:] - } - return strings.Join(parts, "/") } // resolveGroup is one pending group and its present copies. @@ -119,53 +62,52 @@ type resolveGroup struct { members []dbq.ListDuplicateGroupsForResolveRow class DuplicateClass note string - states []string // per member: tracked, unmapped, or "" when Lidarr was not asked } -// autoMergeable says whether the resolver may merge the group on its own: when -// every copy it would remove is one Lidarr does not map (D-a rule 1). On one -// album that is any group with at most one mapped copy. Across releases each -// mapped copy fulfils its own release (D-b), so only a group with exactly one -// mapped copy qualifies: the others fulfil nothing — a rip beside the clean -// copy on another release, or a wrong-file import Lidarr holds unmapped -// (#5439) — and the mapped copy is the clear one to keep. -func autoMergeable(g *resolveGroup) bool { +// isRip says whether a copy is a video rip: held back as one, or named like +// one and not kept by the operator. The name is read as well as the verdict +// because a pass reads the groups before it flags new rips. +func isRip(m dbq.ListDuplicateGroupsForResolveRow) bool { + if m.SourceVerdict != nil { + return *m.SourceVerdict == "suspect" + } + return len(SourceMarkersFor(m.FilePath)) > 0 +} + +// lone returns the index of the group's only copy that is not a rip, or -1 +// when there is none or more than one. +func (g *resolveGroup) lone() int { + at := -1 + for i, m := range g.members { + if isRip(m) { + continue + } + if at >= 0 { + return -1 + } + at = i + } + return at +} + +// mergePlan says whether the resolver may merge the group on its own, and into +// which copy (an index into members; -1 means ProposeSurvivor decides). +func mergePlan(g *resolveGroup) (ok bool, survivor int) { switch g.class { case ClassSameRelease: - return g.tracked() <= 1 + return true, -1 case ClassCrossRelease, ClassMismatch: - return g.tracked() == 1 - default: - return false - } -} - -// settling says whether any of the group's albums had its Lidarr release -// changed recently, so Lidarr may still be remapping its files. -func (g *resolveGroup) settling(albums map[string]bool) bool { - for _, m := range g.members { - if albums[syncpkg.FormatUUID(m.AlbumID)] { - return true + if i := g.lone(); i >= 0 { + return true, i } } - return false + return false, -1 } -func (g *resolveGroup) tracked() int { - n := 0 - for _, s := range g.states { - if s == lidarrStateTracked { - n++ - } - } - return n -} - -// ResolveDuplicates runs one pass. lid is nil when Lidarr is disabled; act is -// the operator's auto-resolve setting. Without Lidarr, or with act off, the -// pass only classifies: nothing is removed when Lidarr cannot say what is safe. +// ResolveDuplicates runs one pass. act is the operator's auto-resolve setting; +// with it off, the pass only classifies. func ResolveDuplicates( - ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string, lid LidarrLibrary, act bool, + ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string, act bool, ) (DuplicateResolveResult, error) { if logger == nil { logger = slog.Default() @@ -180,32 +122,10 @@ func ResolveDuplicates( groups := foldResolveGroups(rows) res.Groups = len(groups) - var unmapped map[string]bool - if lid != nil { - files, err := lid.ListUnmappedTrackFiles(ctx) - if err != nil { - logger.Warn("duplicate resolve: Lidarr's unmapped files unavailable; classifying only", "err", err) - } else { - unmapped = make(map[string]bool, len(files)) - for _, f := range files { - unmapped[LidarrPathKey(f.Path)] = true - } - res.LidarrConsulted = true - } - } - for _, g := range groups { cm := make([]ClassifyMember, len(g.members)) - g.states = make([]string, len(g.members)) for i, m := range g.members { cm[i] = ClassifyMember{AlbumID: syncpkg.FormatUUID(m.AlbumID), Title: m.Title, ArtistName: m.ArtistName} - if res.LidarrConsulted { - if unmapped[LidarrPathKey(m.FilePath)] { - g.states[i] = lidarrStateUnmapped - } else { - g.states[i] = lidarrStateTracked - } - } } g.class = ClassifyDuplicateGroup(g.tier, cm) g.note = resolveNote(g) @@ -219,31 +139,15 @@ func ResolveDuplicates( return res, nil } - // Holding rips back and linking songs remove nothing and need nothing from - // Lidarr. if res.HeldBack, err = q.FlagSuspectSourceTracks(ctx, SuspectSourcePattern); err != nil { return res, fmt.Errorf("hold back video rips: %w", err) } if res.Released, err = q.ClearStaleSuspectSourceTracks(ctx, SuspectSourcePattern); err != nil { return res, fmt.Errorf("release renamed tracks: %w", err) } - linked, err := linkCrossRelease(ctx, pool, logger, groups) - res.SongsLinked = linked - if err != nil { - return res, err - } - if !res.LidarrConsulted { - return res, nil - } - - settling, err := recentlyChangedAlbums(ctx, q, time.Now()) - if err != nil { - return res, err - } - - // Merges first, on the state Lidarr reported at the start of the pass. - removable := func(p string) bool { return unmapped[LidarrPathKey(p)] } + // Merges before links, so a rip merged away is not linked first. + merged := map[pgtype.UUID]bool{} for _, g := range groups { if res.Merged >= resolveMergeCap { break @@ -251,35 +155,29 @@ func ResolveDuplicates( if ctx.Err() != nil { return res, ctx.Err() } - if !autoMergeable(g) || g.settling(settling) { + ok, survivor := mergePlan(g) + if !ok { continue } - if err := autoMerge(ctx, pool, logger, dataDir, g, removable); err != nil { + if err := autoMerge(ctx, pool, logger, dataDir, g, survivor); err != nil { res.MergeFailed++ logger.Warn("duplicate resolve: merge failed", "group_id", syncpkg.FormatUUID(g.id), "err", err) continue } + merged[g.id] = true res.Merged++ } - // Then release changes, for albums where Lidarr maps every copy. - changes, err := changeRepeatingReleases(ctx, q, pool, logger, lid, groups, settling) - res.ReleaseChanges = changes + linked, err := linkCrossRelease(ctx, pool, logger, groups, merged) + res.SongsLinked = linked if err != nil { return res, err } - // Last, rips with no clean copy anywhere: ask Lidarr for something better. - searched, err := searchSoleCopyRips(ctx, q, pool, logger, lid) - res.RipSearches = searched - if err != nil { - logger.Warn("duplicate resolve: rip search failed", "err", err) - } - - if res.Merged > 0 || len(changes) > 0 { + if res.Merged > 0 { notifyAdmins(ctx, notifications.KindDuplicatesResolved, notifications.Payload{ - Count: int64(res.Merged + len(changes)), - Detail: resolvedDetail(res.Merged, changes), + Count: int64(res.Merged), + Detail: fmt.Sprintf("Merged %d duplicate %s.", res.Merged, pluralWord(res.Merged, "copy", "copies")), }) } return res, nil @@ -287,13 +185,15 @@ func ResolveDuplicates( // linkCrossRelease links each cross-release group's copies as one song // (#5438): a like on one is a like on all, and the Liked list, shuffle and the -// mixes count the song once. Linking an already-linked group changes nothing, +// mixes count the song once. A group the pass merged has nothing left to link. Linking an already-linked group changes nothing, // so this runs every pass; likes are shared only when a link is new, since a // like made after it reaches every copy as it is made. -func linkCrossRelease(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, groups []*resolveGroup) (int, error) { +func linkCrossRelease( + ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, groups []*resolveGroup, merged map[pgtype.UUID]bool, +) (int, error) { linked := 0 for _, g := range groups { - if g.class != ClassCrossRelease { + if g.class != ClassCrossRelease || merged[g.id] { continue } if ctx.Err() != nil { @@ -345,61 +245,6 @@ func linkSong(ctx context.Context, pool *pgxpool.Pool, ids []pgtype.UUID) (bool, return true, tx.Commit(ctx) } -// ripSearchCap bounds the albums one pass asks Lidarr to search, so the first -// pass after deploy does not queue the whole backlog of rips at once. -const ripSearchCap = 10 - -// searchSoleCopyRips asks Lidarr to search each album holding a video rip that -// is the only copy of its song (#5447, operator's choice: option 2). While -// Lidarr maps the rip it counts the track fulfilled, so nothing else asks for -// better. An album search grabs only when the quality profile allows an -// upgrade, so this removes nothing and may change nothing: the rip stays held -// back either way. An album is searched at most once a week (the query reads -// the audit log), and an album Lidarr does not hold is skipped. -func searchSoleCopyRips( - ctx context.Context, q *dbq.Queries, pool *pgxpool.Pool, logger *slog.Logger, lid LidarrLibrary, -) (int, error) { - albums, err := q.ListAlbumsWithSoleCopyRips(ctx, ripSearchCap) - if err != nil { - return 0, fmt.Errorf("list albums with sole-copy rips: %w", err) - } - type found struct { - row dbq.ListAlbumsWithSoleCopyRipsRow - lidarrID int - } - var targets []found - for _, a := range albums { - la, err := lid.LookupAlbumByMBID(ctx, a.ReleaseGroupMbid) - if errors.Is(err, lidarr.ErrNotFound) { - continue - } - if err != nil { - return 0, fmt.Errorf("look up %s: %w", a.ReleaseGroupMbid, err) - } - targets = append(targets, found{row: a, lidarrID: la.ID}) - } - if len(targets) == 0 { - return 0, nil - } - ids := make([]int, len(targets)) - for i, t := range targets { - ids[i] = t.lidarrID - } - if err := lid.SearchAlbums(ctx, ids); err != nil { - return 0, fmt.Errorf("search albums: %w", err) - } - for _, t := range targets { - audit.WriteOrLog(ctx, pool, logger, pgtype.UUID{}, pgtype.UUID{}, audit.ActionLidarrRipSearch, map[string]any{ - "album_id": syncpkg.FormatUUID(t.row.AlbumID), - "album_title": t.row.AlbumTitle, - "artist_name": t.row.ArtistName, - "lidarr_album_id": t.lidarrID, - "rips": t.row.Rips, - }) - } - return len(targets), nil -} - func foldResolveGroups(rows []dbq.ListDuplicateGroupsForResolveRow) []*resolveGroup { var groups []*resolveGroup for _, r := range rows { @@ -419,16 +264,14 @@ func foldResolveGroups(rows []dbq.ListDuplicateGroupsForResolveRow) []*resolveGr return out } -// resolveNote is what the report says beside a group the resolver leaves for -// the operator, when there is something to say. +// resolveNote is what the report says beside a group, when there is something +// to say. func resolveNote(g *resolveGroup) string { switch g.class { - case ClassSameRelease: - if g.tracked() > 1 { - return "Lidarr tracks every copy as its own track, so removing one would make Lidarr download it again." - } case ClassCrossRelease: - return "Each copy belongs to its own release, and Lidarr keeps each one. Nothing is removed; they count as one song." + if g.lone() < 0 { + 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." } @@ -437,16 +280,10 @@ func resolveNote(g *resolveGroup) string { func writeResolveVerdicts(ctx context.Context, q *dbq.Queries, groups []*resolveGroup) error { classes := dbq.SetDuplicateGroupClassesParams{} - states := dbq.SetDuplicateMemberLidarrStatesParams{} for _, g := range groups { classes.GroupIds = append(classes.GroupIds, g.id) classes.Classes = append(classes.Classes, string(g.class)) classes.Notes = append(classes.Notes, g.note) - for i, m := range g.members { - states.GroupIds = append(states.GroupIds, g.id) - states.TrackIds = append(states.TrackIds, m.TrackID) - states.States = append(states.States, g.states[i]) - } } if len(classes.GroupIds) == 0 { return nil @@ -454,9 +291,6 @@ func writeResolveVerdicts(ctx context.Context, q *dbq.Queries, groups []*resolve if err := q.SetDuplicateGroupClasses(ctx, classes); err != nil { return fmt.Errorf("write duplicate classes: %w", err) } - if err := q.SetDuplicateMemberLidarrStates(ctx, states); err != nil { - return fmt.Errorf("write lidarr states: %w", err) - } return nil } @@ -465,29 +299,31 @@ func survivorCandidates(g *resolveGroup) []SurvivorCandidate { cands := make([]SurvivorCandidate, len(g.members)) for i, m := range g.members { cands[i] = SurvivorCandidate{ - TrackID: syncpkg.FormatUUID(m.TrackID), - FileFormat: m.FileFormat, - FileSize: m.FileSize, - AddedAt: m.AddedAt.Time, - LidarrTracked: g.states[i] == lidarrStateTracked, - TagFit: TagFitScore(m.TrackNumber != nil, m.PositionClash, m.FilePath, m.HasMbid), + TrackID: syncpkg.FormatUUID(m.TrackID), + FileFormat: m.FileFormat, + FileSize: m.FileSize, + AddedAt: m.AddedAt.Time, + TagFit: TagFitScore(m.TrackNumber != nil, m.PositionClash, m.FilePath, m.HasMbid), } } return cands } -// autoMerge merges one group into the copy ProposeSurvivor picks, which is the -// copy Lidarr tracks when there is one. removable is the guard: a copy Lidarr -// maps can never be among those removed, whatever the survivor rule said. +// autoMerge merges one group into the copy at index survivor, or the copy +// ProposeSurvivor picks when survivor is -1, and records the merge as the +// resolver's. func autoMerge( - ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string, g *resolveGroup, removable RemovableFunc, + ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string, g *resolveGroup, survivor int, ) error { survivorKey, reason := ProposeSurvivor(survivorCandidates(g)) + if survivor >= 0 { + survivorKey, reason = syncpkg.FormatUUID(g.members[survivor].TrackID), "the copy that is not a video rip" + } var survivorID pgtype.UUID if err := survivorID.Scan(survivorKey); err != nil { return fmt.Errorf("parse survivor id: %w", err) } - res, err := MergeDuplicateGroupGuarded(ctx, pool, logger, dataDir, g.id, survivorID, removable) + res, err := MergeDuplicateGroup(ctx, pool, logger, dataDir, g.id, survivorID) if err != nil { return err } @@ -515,294 +351,6 @@ func autoMerge( return nil } -// recentlyChangedAlbums is the Minstrel albums whose Lidarr release the -// resolver changed within releaseChangeSettle, read from the audit log so a -// restart does not forget them. -func recentlyChangedAlbums(ctx context.Context, q *dbq.Queries, now time.Time) (map[string]bool, error) { - rows, err := q.ListAuditLogByActions(ctx, dbq.ListAuditLogByActionsParams{ - Actions: []string{string(audit.ActionLidarrReleaseChange)}, SystemOnly: true, - PageLimit: 200, PageOffset: 0, - }) - if err != nil { - return nil, fmt.Errorf("read recent release changes: %w", err) - } - out := map[string]bool{} - for _, r := range rows { - if now.Sub(r.CreatedAt.Time) > releaseChangeSettle { - break // newest first - } - var meta struct { - AlbumID string `json:"album_id"` - } - if json.Unmarshal(r.Metadata, &meta) == nil && meta.AlbumID != "" { - out[meta.AlbumID] = true - } - } - return out, nil -} - -// repeatingAlbum is a Minstrel album where Lidarr maps more than one copy of a -// song, and the groups that showed it. -type repeatingAlbum struct { - albumID pgtype.UUID - releaseGroupMbid string - title, artist string - groups []*resolveGroup -} - -func changeRepeatingReleases( - ctx context.Context, q *dbq.Queries, pool *pgxpool.Pool, logger *slog.Logger, - lid LidarrLibrary, groups []*resolveGroup, settling map[string]bool, -) ([]ReleaseChange, error) { - albums := map[string]*repeatingAlbum{} - for _, g := range groups { - if g.class != ClassSameRelease || g.tracked() < 2 { - continue - } - m := g.members[0] - key := syncpkg.FormatUUID(m.AlbumID) - if settling[key] || m.ReleaseGroupMbid == nil || *m.ReleaseGroupMbid == "" { - continue - } - a := albums[key] - if a == nil { - a = &repeatingAlbum{albumID: m.AlbumID, releaseGroupMbid: *m.ReleaseGroupMbid, title: m.AlbumTitle, artist: m.ArtistName} - albums[key] = a - } - a.groups = append(a.groups, g) - } - keys := make([]string, 0, len(albums)) - for k := range albums { - keys = append(keys, k) - } - sort.Strings(keys) - - var changes []ReleaseChange - for _, k := range keys { - if len(changes) >= resolveReleaseCap { - break - } - if ctx.Err() != nil { - return changes, ctx.Err() - } - a := albums[k] - change, note, err := changeRelease(ctx, q, lid, a) - if err != nil { - logger.Warn("duplicate resolve: Lidarr release check failed", "album", a.title, "err", err) - continue - } - if change != nil { - changes = append(changes, *change) - audit.WriteOrLog(ctx, pool, logger, pgtype.UUID{}, pgtype.UUID{}, audit.ActionLidarrReleaseChange, map[string]any{ - "automatic": true, - "album_id": k, - "album_title": a.title, - "artist_name": a.artist, - "from_release": releaseLabel(change.From), - "to_release": releaseLabel(change.To), - }) - } - if note != "" { - if err := setGroupNotes(ctx, q, a.groups, note); err != nil { - return changes, err - } - } - } - return changes, nil -} - -// changeRelease checks one album's monitored release and, when it lists a -// song twice, moves Lidarr to the release that lists each song once and best -// covers what is on disk. It returns the change made (nil when none) and the -// note for the album's groups. -func changeRelease( - ctx context.Context, q *dbq.Queries, lid LidarrLibrary, a *repeatingAlbum, -) (*ReleaseChange, string, error) { - la, err := lid.LookupAlbumByMBID(ctx, a.releaseGroupMbid) - if errors.Is(err, lidarr.ErrNotFound) { - return nil, "Lidarr does not hold this album, so its copies are left for you.", nil - } - if err != nil { - return nil, "", err - } - current, err := lid.ListAlbumTracks(ctx, la.ID) - if err != nil { - return nil, "", err - } - if !releaseRepeats(current) { - return nil, "Lidarr maps each copy to a different track of this album, so none can go without a download.", nil - } - releases, err := lid.GetAlbumReleases(ctx, la.ID) - if err != nil { - return nil, "", err - } - titles, err := q.ListAlbumPresentTrackTitles(ctx, a.albumID) - if err != nil { - return nil, "", fmt.Errorf("list album titles: %w", err) - } - onDisk := map[string]bool{} - for _, t := range titles { - if k := ReleaseTitleKey(t); k != "" { - onDisk[k] = true - } - } - - var from lidarr.AlbumRelease - var candidates []releaseCandidate - for _, r := range releases { - if r.Monitored { - from = r - continue - } - tracks, err := lid.ListReleaseTracks(ctx, r.ID) - if err != nil { - return nil, "", err - } - if len(tracks) == 0 || releaseRepeats(tracks) { - continue - } - candidates = append(candidates, releaseCandidate{release: r, coverage: coverage(tracks, onDisk)}) - } - best, ok := pickRelease(candidates, coverage(current, onDisk)) - if !ok { - return nil, "Lidarr's release of this album lists songs twice, and no other release lists each once while keeping what is on disk.", nil - } - if err := lid.SetMonitoredRelease(ctx, la.ID, best.ID); err != nil { - // Lidarr's album update unlinks the album's files and queues the rescan - // before it answers, and on a large album that outlasts the client's - // timeout while the change still lands (Cracker Island on the first - // deploy pass). An unanswered PUT is read back: when the release did - // change, it is recorded like any other, so the album gets its settle - // window and the report shows it. - if !errors.Is(err, lidarr.ErrUnreachable) || !releaseMonitored(ctx, lid, la.ID, best.ID) { - return nil, "", err - } - } - note := fmt.Sprintf("Lidarr now monitors the %s release, which lists each song once. The extra copies are merged once Lidarr has rescanned.", releaseLabel(best)) - return &ReleaseChange{AlbumTitle: a.title, ArtistName: a.artist, From: from, To: best}, note, nil -} - -// releaseMonitored reads whether the album's monitored release is now -// releaseID. A failed read answers false: the change is not assumed. -func releaseMonitored(ctx context.Context, lid LidarrLibrary, albumID, releaseID int) bool { - releases, err := lid.GetAlbumReleases(ctx, albumID) - if err != nil { - return false - } - for _, r := range releases { - if r.ID == releaseID { - return r.Monitored - } - } - return false -} - -type releaseCandidate struct { - release lidarr.AlbumRelease - coverage int -} - -// pickRelease chooses among releases that list each song once: the one -// covering most of the titles on disk, then the fewest tracks (the plainest -// edition that holds them), then a digital one, then the lowest id so the -// choice is stable. It refuses a release covering fewer titles on disk than -// the current one does: the change must not drop a song Lidarr now keeps. -func pickRelease(cands []releaseCandidate, currentCoverage int) (lidarr.AlbumRelease, bool) { - if len(cands) == 0 { - return lidarr.AlbumRelease{}, false - } - sort.SliceStable(cands, func(i, j int) bool { - a, b := cands[i], cands[j] - if a.coverage != b.coverage { - return a.coverage > b.coverage - } - if a.release.TrackCount != b.release.TrackCount { - return a.release.TrackCount < b.release.TrackCount - } - if da, db := isDigital(a.release), isDigital(b.release); da != db { - return da - } - return a.release.ID < b.release.ID - }) - if cands[0].coverage < currentCoverage { - return lidarr.AlbumRelease{}, false - } - return cands[0].release, true -} - -func isDigital(r lidarr.AlbumRelease) bool { - return strings.Contains(strings.ToLower(r.Format), "digital") -} - -// releaseRepeats reports whether a release lists one title more than once. -func releaseRepeats(tracks []lidarr.ReleaseTrack) bool { - seen := map[string]bool{} - for _, t := range tracks { - k := ReleaseTitleKey(t.Title) - if k == "" { - continue - } - if seen[k] { - return true - } - seen[k] = true - } - return false -} - -// coverage counts the distinct titles on disk that the release lists. -func coverage(tracks []lidarr.ReleaseTrack, onDisk map[string]bool) int { - hit := map[string]bool{} - for _, t := range tracks { - if k := ReleaseTitleKey(t.Title); onDisk[k] { - hit[k] = true - } - } - return len(hit) -} - -func releaseLabel(r lidarr.AlbumRelease) string { - label := r.Format - if label == "" { - label = r.Title - } - if r.Disambiguation != "" { - label = r.Disambiguation + ", " + label - } - return fmt.Sprintf("%s (%d tracks)", label, r.TrackCount) -} - -func setGroupNotes(ctx context.Context, q *dbq.Queries, groups []*resolveGroup, note string) error { - p := dbq.SetDuplicateGroupClassesParams{} - for _, g := range groups { - g.note = note - p.GroupIds = append(p.GroupIds, g.id) - p.Classes = append(p.Classes, string(g.class)) - p.Notes = append(p.Notes, note) - } - if err := q.SetDuplicateGroupClasses(ctx, p); err != nil { - return fmt.Errorf("write duplicate notes: %w", err) - } - return nil -} - -func resolvedDetail(merged int, changes []ReleaseChange) string { - var parts []string - if merged > 0 { - parts = append(parts, fmt.Sprintf("Merged %d %s Lidarr does not need.", merged, pluralWord(merged, "copy", "copies"))) - } - switch len(changes) { - case 0: - case 1: - c := changes[0] - parts = append(parts, fmt.Sprintf("Lidarr now monitors the %s release of %s by %s, which lists each song once.", - releaseLabel(c.To), c.AlbumTitle, c.ArtistName)) - default: - parts = append(parts, fmt.Sprintf("Lidarr now monitors a release that lists each song once for %d albums.", len(changes))) - } - return strings.Join(parts, " ") -} - func pluralWord(n int, one, many string) string { if n == 1 { return one @@ -816,20 +364,14 @@ type DuplicateResolveWorker struct { logger *slog.Logger dataDir string settings *FingerprintSettingsService - lidarr func() LidarrLibrary tick time.Duration } // NewDuplicateResolveWorker builds a worker with the production cadence. -// lidarrFn is asked each pass, so a Lidarr setting saved in admin takes effect -// without a restart; it returns nil while Lidarr is disabled. func NewDuplicateResolveWorker( - pool *pgxpool.Pool, logger *slog.Logger, dataDir string, - settings *FingerprintSettingsService, lidarrFn func() LidarrLibrary, + pool *pgxpool.Pool, logger *slog.Logger, dataDir string, settings *FingerprintSettingsService, ) *DuplicateResolveWorker { - return &DuplicateResolveWorker{ - pool: pool, logger: logger, dataDir: dataDir, settings: settings, lidarr: lidarrFn, tick: duplicateResolveTick, - } + return &DuplicateResolveWorker{pool: pool, logger: logger, dataDir: dataDir, settings: settings, tick: duplicateResolveTick} } // Run blocks until ctx is cancelled, running a pass at start and then each tick. @@ -865,16 +407,11 @@ func (w *DuplicateResolveWorker) tickOnce(ctx context.Context) { } return } - var lid LidarrLibrary - if w.lidarr != nil { - lid = w.lidarr() - } - res, err := ResolveDuplicates(ctx, w.pool, w.logger, w.dataDir, lid, w.settings.Get().AutoResolve) + res, err := ResolveDuplicates(ctx, w.pool, w.logger, w.dataDir, w.settings.Get().AutoResolve) if err != nil && ctx.Err() == nil { w.logger.Warn("duplicate resolve: pass failed", "err", err) } w.logger.Info("duplicate resolve complete", - "groups", res.Groups, "lidarr", res.LidarrConsulted, "merged", res.Merged, - "merge_failed", res.MergeFailed, "release_changes", len(res.ReleaseChanges), "songs_linked", res.SongsLinked, - "held_back", res.HeldBack, "released", res.Released, "rip_searches", res.RipSearches) + "groups", res.Groups, "merged", res.Merged, "merge_failed", res.MergeFailed, + "songs_linked", res.SongsLinked, "held_back", res.HeldBack, "released", res.Released) } diff --git a/internal/library/duplicate_resolve_test.go b/internal/library/duplicate_resolve_test.go index 135e55ca..6f184056 100644 --- a/internal/library/duplicate_resolve_test.go +++ b/internal/library/duplicate_resolve_test.go @@ -2,8 +2,6 @@ package library import ( "context" - "errors" - "fmt" "os" "path/filepath" "testing" @@ -12,128 +10,9 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" - "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" ) -func TestLidarrPathKey(t *testing.T) { - // Minstrel and Lidarr see the library under different mounts; the key is - // what they agree on. - a := LidarrPathKey("/music/Gorillaz/Humanz (2017)/Gorillaz - Humanz - 01 - Ascension.mp3") - b := LidarrPathKey("/data/media/music/Gorillaz/Humanz (2017)/Gorillaz - Humanz - 01 - Ascension.mp3") - if a != b || a != "Gorillaz/Humanz (2017)/Gorillaz - Humanz - 01 - Ascension.mp3" { - t.Errorf("keys differ or are wrong: %q, %q", a, b) - } - if LidarrPathKey("/music/Gorillaz/Humanz (2017)/a.mp3") == LidarrPathKey("/music/Gorillaz/Demon Days (2005)/a.mp3") { - t.Error("two albums' files share a key") - } -} - -func rt(titles ...string) []lidarr.ReleaseTrack { - out := make([]lidarr.ReleaseTrack, len(titles)) - for i, title := range titles { - out[i] = lidarr.ReleaseTrack{ID: i + 1, Title: title} - } - return out -} - -func TestReleaseRepeats(t *testing.T) { - if !releaseRepeats(rt("Ascension", "Strobelite", "Ascension")) { - t.Error("the box set lists Ascension twice, but no repeat was found") - } - if releaseRepeats(rt("Charger", "Charger (demo)")) { - t.Error("a demo was read as a repeat of the song") - } -} - -// The Humanz releases as Lidarr lists them (2026-10-09): the box set is -// monitored, and the 40-track digital release is the one that lists each song -// once and covers what is on disk. -func TestPickRelease(t *testing.T) { - digital40 := lidarr.AlbumRelease{ID: 10149, Format: "Digital Media", TrackCount: 40} - deluxe26 := lidarr.AlbumRelease{ID: 10150, Format: "CD", TrackCount: 26} - standard20 := lidarr.AlbumRelease{ID: 10151, Format: "Digital Media", TrackCount: 20} - - got, ok := pickRelease([]releaseCandidate{ - {release: standard20, coverage: 20}, {release: deluxe26, coverage: 26}, {release: digital40, coverage: 34}, - }, 34) - if !ok || got.ID != digital40.ID { - t.Errorf("picked %+v (ok=%v), want the 40-track digital release", got, ok) - } - - // Equal coverage: the plainer edition, then digital. - cd := lidarr.AlbumRelease{ID: 1, Format: "CD", TrackCount: 26} - web := lidarr.AlbumRelease{ID: 2, Format: "Digital Media", TrackCount: 26} - if got, _ := pickRelease([]releaseCandidate{{release: cd, coverage: 26}, {release: web, coverage: 26}}, 26); got.ID != web.ID { - t.Errorf("picked %+v, want the digital one of two equal releases", got) - } - - // Never a release that keeps fewer of the songs on disk than the current one. - if got, ok := pickRelease([]releaseCandidate{{release: deluxe26, coverage: 26}}, 34); ok { - t.Errorf("picked %+v, which covers fewer songs on disk than the current release", got) - } -} - -// fakeLidarr is one Lidarr album with several releases. Switching the -// monitored release changes what ListAlbumTracks answers, as in Lidarr. -type fakeLidarr struct { - unmapped []string - unmappedErr error - album lidarr.LidarrAlbum - releases []lidarr.AlbumRelease - releaseTracks map[int][]lidarr.ReleaseTrack - setCalls [][2]int - searchCalls [][]int - // setErr is what SetMonitoredRelease answers after applying the change: - // Lidarr timing out on a change that landed. - setErr error -} - -func (f *fakeLidarr) ListUnmappedTrackFiles(context.Context) ([]lidarr.TrackFile, error) { - out := make([]lidarr.TrackFile, len(f.unmapped)) - for i, p := range f.unmapped { - out[i] = lidarr.TrackFile{ID: i + 1, Path: p} - } - return out, f.unmappedErr -} - -func (f *fakeLidarr) LookupAlbumByMBID(_ context.Context, mbid string) (lidarr.LidarrAlbum, error) { - if f.album.ForeignAlbumID != mbid { - return lidarr.LidarrAlbum{}, lidarr.ErrNotFound - } - return f.album, nil -} - -func (f *fakeLidarr) ListAlbumTracks(context.Context, int) ([]lidarr.ReleaseTrack, error) { - for _, r := range f.releases { - if r.Monitored { - return f.releaseTracks[r.ID], nil - } - } - return nil, nil -} - -func (f *fakeLidarr) GetAlbumReleases(context.Context, int) ([]lidarr.AlbumRelease, error) { - return append([]lidarr.AlbumRelease(nil), f.releases...), nil -} - -func (f *fakeLidarr) ListReleaseTracks(_ context.Context, releaseID int) ([]lidarr.ReleaseTrack, error) { - return f.releaseTracks[releaseID], nil -} - -func (f *fakeLidarr) SearchAlbums(_ context.Context, albumIDs []int) error { - f.searchCalls = append(f.searchCalls, albumIDs) - return nil -} - -func (f *fakeLidarr) SetMonitoredRelease(_ context.Context, albumID, releaseID int) error { - f.setCalls = append(f.setCalls, [2]int{albumID, releaseID}) - for i := range f.releases { - f.releases[i].Monitored = f.releases[i].ID == releaseID - } - return f.setErr -} - // resolveFixture is the Humanz shape: one album, one song twice, both copies on // disk under an artist/album folder, in one pending exact-tier group. type resolveFixture struct { @@ -178,7 +57,6 @@ func newResolveFixture(t *testing.T) resolveFixture { } } exec(`UPDATE tracks SET title = 'Saturnz Barz' WHERE id = $1`, f.clean.ID) - exec(`UPDATE albums SET release_group_mbid = 'rg-humanz' WHERE id = $1`, album.ID) if err := pool.QueryRow(ctx, `INSERT INTO duplicate_groups (member_key, tier) VALUES ('resolve-fixture', 'exact') RETURNING id`, ).Scan(&f.groupID); err != nil { @@ -188,9 +66,6 @@ func newResolveFixture(t *testing.T) resolveFixture { return f } -// lidarrPath is where Lidarr, mounted elsewhere, sees a fixture file. -func lidarrPath(p string) string { return "/music/" + LidarrPathKey(p) } - func (f resolveFixture) group(t *testing.T) (status string, class *string, note *string, auto bool) { t.Helper() if err := f.pool.QueryRow(context.Background(), @@ -203,18 +78,32 @@ func (f resolveFixture) group(t *testing.T) (status string, class *string, note func exists(p string) bool { _, err := os.Stat(p); return err == nil } -// Lidarr maps the clean copy and holds the rip unmapped: the rip goes, the -// clean copy stays, and the merge is recorded as the resolver's. -func TestResolveDuplicates_MergesTheUnmappedCopy_Integration(t *testing.T) { - f := newResolveFixture(t) - lid := &fakeLidarr{unmapped: []string{lidarrPath(f.ripPath)}} +// autoMergeAudit reads the resolver's audit row for a group: how many there +// are, and the reason the last one gave for its survivor. +func autoMergeAudit(t *testing.T, pool *pgxpool.Pool, groupID pgtype.UUID) (n int, reason string) { + t.Helper() + if err := pool.QueryRow(context.Background(), + `SELECT count(*), coalesce(max(metadata->>'survivor_reason'), '') FROM audit_log + WHERE action = 'duplicate_merge' AND actor_id IS NULL + AND metadata->>'group_id' = $1 AND (metadata->>'automatic')::boolean`, + syncpkg.FormatUUID(groupID)).Scan(&n, &reason); err != nil { + t.Fatal(err) + } + return n, reason +} - res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", lid, true) +// One album holding a song twice: the extra copy goes, the copy whose name and +// tags fit the album stays, and the merge is recorded as the resolver's. No +// Lidarr is asked (M502 #5517): Minstrel is the only program writing the +// library. +func TestResolveDuplicates_MergesASameReleaseCopy_Integration(t *testing.T) { + f := newResolveFixture(t) + res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", true) if err != nil { t.Fatalf("resolve: %v", err) } - if res.Merged != 1 || !res.LidarrConsulted { - t.Fatalf("result = %+v, want one merge with Lidarr consulted", res) + if res.Merged != 1 || res.Classes[ClassSameRelease] != 1 { + t.Fatalf("result = %+v, want the same-release group merged", res) } if exists(f.ripPath) || !exists(f.cleanPath) { t.Errorf("rip exists=%v clean exists=%v; want only the clean copy left", exists(f.ripPath), exists(f.cleanPath)) @@ -223,138 +112,24 @@ func TestResolveDuplicates_MergesTheUnmappedCopy_Integration(t *testing.T) { if status != "merged" || class == nil || *class != string(ClassSameRelease) || !auto { t.Errorf("group = (%s, %v, auto=%v), want merged same_release by the resolver", status, class, auto) } - var audits int - if err := f.pool.QueryRow(context.Background(), - `SELECT count(*) FROM audit_log WHERE action = 'duplicate_merge' AND actor_id IS NULL - AND metadata->>'group_id' = $1 AND (metadata->>'automatic')::boolean`, - syncpkg.FormatUUID(f.groupID)).Scan(&audits); err != nil { - t.Fatal(err) - } - if audits != 1 { - t.Errorf("automatic merge audit rows = %d, want 1", audits) + if n, _ := autoMergeAudit(t, f.pool, f.groupID); n != 1 { + t.Errorf("automatic merge audit rows = %d, want 1", n) } } -// Lidarr maps both copies, and its release lists the song twice: nothing is -// deleted, the release moves to the one listing each song once, and the next -// pass leaves it there (lesson #4183's fixed point). -func TestResolveDuplicates_ChangesARepeatingRelease_Integration(t *testing.T) { +// With auto-resolve off, the pass classifies and removes nothing. +func TestResolveDuplicates_ClassifiesOnlyWithoutPermission_Integration(t *testing.T) { f := newResolveFixture(t) - lid := &fakeLidarr{ - album: lidarr.LidarrAlbum{ID: 4723, ForeignAlbumID: "rg-humanz", Title: "Humanz"}, - releases: []lidarr.AlbumRelease{ - {ID: 10148, Format: "14x12\" Vinyl, Digital Media", TrackCount: 4, Monitored: true}, - {ID: 10149, Format: "Digital Media", TrackCount: 2}, - }, - releaseTracks: map[int][]lidarr.ReleaseTrack{ - 10148: rt("Saturnz Barz", "Ascension", "Saturnz Barz", "Ascension"), - 10149: rt("Saturnz Barz", "Ascension"), - }, - } - - res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", lid, true) - if err != nil { - t.Fatalf("resolve: %v", err) - } - if res.Merged != 0 || !exists(f.ripPath) || !exists(f.cleanPath) { - t.Fatalf("merged %d; a copy Lidarr maps was touched", res.Merged) - } - if len(lid.setCalls) != 1 || lid.setCalls[0] != [2]int{4723, 10149} { - t.Fatalf("release changes = %v, want album 4723 to release 10149", lid.setCalls) - } - status, _, note, _ := f.group(t) - if status != "pending" || note == nil || *note == "" { - t.Errorf("group = (%s, note %v), want pending with a note saying what changed", status, note) - } - - // The next pass: Lidarr has not rescanned yet, so both copies still read - // as tracked. The album is settling, so nothing changes. - if _, err := ResolveDuplicates(context.Background(), f.pool, nil, "", lid, true); err != nil { - t.Fatalf("second resolve: %v", err) - } - if len(lid.setCalls) != 1 { - t.Errorf("release changed again: %v", lid.setCalls) - } - // And once it has settled, the check itself finds nothing to do: the - // release it chose lists each song once. - change, _, err := changeRelease(context.Background(), dbq.New(f.pool), lid, - &repeatingAlbum{albumID: f.clean.AlbumID, releaseGroupMbid: "rg-humanz"}) - if err != nil || change != nil || len(lid.setCalls) != 1 { - t.Errorf("after the change, changeRelease = (%+v, %v) with %d calls; want no change", change, err, len(lid.setCalls)) - } -} - -// After a release change Lidarr unmaps every file of the album until it has -// rescanned. A pass in that window must not read "every copy unmapped" as -// licence to merge: the copy it would remove may be the one Lidarr maps next. -func TestResolveDuplicates_LeavesAnAlbumAloneWhileLidarrRescans_Integration(t *testing.T) { - f := newResolveFixture(t) - lid := &fakeLidarr{ - album: lidarr.LidarrAlbum{ID: 4723, ForeignAlbumID: "rg-humanz"}, - releases: []lidarr.AlbumRelease{ - {ID: 10148, TrackCount: 2, Monitored: true}, {ID: 10149, TrackCount: 1}, - }, - releaseTracks: map[int][]lidarr.ReleaseTrack{10148: rt("Saturnz Barz", "Saturnz Barz"), 10149: rt("Saturnz Barz")}, - } - if _, err := ResolveDuplicates(context.Background(), f.pool, nil, "", lid, true); err != nil { - t.Fatal(err) - } - lid.unmapped = []string{lidarrPath(f.cleanPath), lidarrPath(f.ripPath)} - res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", lid, true) + res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", false) if err != nil { t.Fatal(err) } - if res.Merged != 0 || !exists(f.ripPath) || !exists(f.cleanPath) { - t.Errorf("merged %d during the rescan window", res.Merged) + if res.Merged != 0 || !exists(f.ripPath) { + t.Errorf("merged %d", res.Merged) } -} - -// Without Lidarr, or with auto-resolve off, the pass classifies and removes -// nothing. -func TestResolveDuplicates_ClassifiesOnlyWithoutLidarrOrPermission_Integration(t *testing.T) { - for name, run := range map[string]func(resolveFixture) (DuplicateResolveResult, error){ - "lidarr disabled": func(f resolveFixture) (DuplicateResolveResult, error) { - return ResolveDuplicates(context.Background(), f.pool, nil, "", nil, true) - }, - "lidarr unreachable": func(f resolveFixture) (DuplicateResolveResult, error) { - return ResolveDuplicates(context.Background(), f.pool, nil, "", &fakeLidarr{unmappedErr: errors.New("timeout")}, true) - }, - "auto-resolve off": func(f resolveFixture) (DuplicateResolveResult, error) { - lid := &fakeLidarr{unmapped: []string{lidarrPath(f.ripPath)}} - return ResolveDuplicates(context.Background(), f.pool, nil, "", lid, false) - }, - } { - t.Run(name, func(t *testing.T) { - f := newResolveFixture(t) - res, err := run(f) - if err != nil { - t.Fatal(err) - } - if res.Merged != 0 || !exists(f.ripPath) { - t.Errorf("merged %d", res.Merged) - } - status, class, _, _ := f.group(t) - if status != "pending" || class == nil || *class != string(ClassSameRelease) { - t.Errorf("group = (%s, %v), want pending and classified", status, class) - } - }) - } -} - -// The guard itself: a merge asked to remove a copy the check refuses changes -// nothing, file or row. -func TestMergeDuplicateGroupGuarded_RefusesATrackedCopy_Integration(t *testing.T) { - f := newResolveFixture(t) - refuse := func(p string) bool { return p != f.ripPath } - _, err := MergeDuplicateGroupGuarded(context.Background(), f.pool, nil, "", f.groupID, f.clean.ID, refuse) - if !errors.Is(err, ErrCopyTrackedByLidarr) { - t.Fatalf("err = %v, want ErrCopyTrackedByLidarr", err) - } - if !exists(f.ripPath) { - t.Error("the refused copy's file was removed") - } - if status, _, _, _ := f.group(t); status != "pending" { - t.Errorf("group status = %s, want pending", status) + status, class, _, _ := f.group(t) + if status != "pending" || class == nil || *class != string(ClassSameRelease) { + t.Errorf("group = (%s, %v), want pending and classified", status, class) } } @@ -416,12 +191,12 @@ func newCrossReleaseRipFixture(t *testing.T) crossReleaseRipFixture { return f } -// The rip fulfils nothing in Lidarr while the clean copy on the album does: -// the rip goes and the album copy stays (#5439, D-a rule 1). -func TestResolveDuplicates_MergesAnUnmappedRipAcrossReleases_Integration(t *testing.T) { +// A video rip on one release with the clean copy on another: the rip is no +// release's copy, so it goes and the album cut stays (#5439, now without +// Lidarr). +func TestResolveDuplicates_MergesARipAcrossReleases_Integration(t *testing.T) { f := newCrossReleaseRipFixture(t) - lid := &fakeLidarr{unmapped: []string{lidarrPath(f.ripPath)}} - res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", lid, true) + res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", true) if err != nil { t.Fatal(err) } @@ -431,21 +206,65 @@ func TestResolveDuplicates_MergesAnUnmappedRipAcrossReleases_Integration(t *test if exists(f.ripPath) || !exists(f.cleanPath) { t.Errorf("rip exists %v, clean exists %v; want only the clean copy", exists(f.ripPath), exists(f.cleanPath)) } + if _, reason := autoMergeAudit(t, f.pool, f.groupID); reason != "the copy that is not a video rip" { + t.Errorf("survivor reason %q, want the rip rule", reason) + } } -// When Lidarr maps both copies, each fulfils its own release: nothing is -// removed, and the copies are linked as one song instead. -func TestResolveDuplicates_KeepsCopiesLidarrMapsOnTwoReleases_Integration(t *testing.T) { +// Copies on two releases that are both clean each belong to their release: +// nothing is removed, and they are linked as one song. A rip the operator +// marked fine counts as clean. +func TestResolveDuplicates_KeepsCleanCopiesOnTwoReleases_Integration(t *testing.T) { + for name, prepare := range map[string]func(f crossReleaseRipFixture) (string, []any){ + "renamed": func(f crossReleaseRipFixture) (string, []any) { + return `UPDATE tracks SET title = 'Feel Good Inc.', file_path = $2 WHERE id = $1`, + []any{f.rip.ID, filepath.Join(filepath.Dir(f.ripPath), "01 - Feel Good Inc.mp3")} + }, + "kept as fine": func(f crossReleaseRipFixture) (string, []any) { + return `UPDATE tracks SET source_verdict = 'fine' WHERE id = $1`, []any{f.rip.ID} + }, + } { + t.Run(name, func(t *testing.T) { + f := newCrossReleaseRipFixture(t) + sql, args := prepare(f) + if _, err := f.pool.Exec(context.Background(), sql, args...); err != nil { + t.Fatal(err) + } + res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", true) + if err != nil { + t.Fatal(err) + } + if res.Merged != 0 || !exists(f.ripPath) { + t.Errorf("merged %d; want both copies kept", res.Merged) + } + if res.SongsLinked != 1 { + t.Errorf("linked %d, want the pair linked as one song", res.SongsLinked) + } + }) + } +} + +// 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) { f := newCrossReleaseRipFixture(t) - res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", &fakeLidarr{}, true) + ctx := context.Background() + if _, err := f.pool.Exec(ctx, `UPDATE duplicate_groups SET tier = 'exact' WHERE id = $1`, f.groupID); err != nil { + t.Fatal(err) + } + if _, err := f.pool.Exec(ctx, `UPDATE tracks SET title = 'DARE', file_path = $2 WHERE id = $1`, + f.rip.ID, filepath.Join(filepath.Dir(f.ripPath), "01 - DARE.mp3")); err != nil { + t.Fatal(err) + } + res, err := ResolveDuplicates(ctx, f.pool, nil, "", true) if err != nil { t.Fatal(err) } - if res.Merged != 0 || !exists(f.ripPath) { - t.Errorf("merged %d; want both copies kept", res.Merged) + 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.SongsLinked != 1 { - t.Errorf("linked %d, want the pair linked as one song", 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) } } @@ -455,6 +274,11 @@ func TestResolveDuplicates_HoldsBackRips_Integration(t *testing.T) { f := newCrossReleaseRipFixture(t) ctx := context.Background() q := dbq.New(f.pool) + // Without its group the rip has no clean copy to merge into, so it stays + // on disk and only its verdict changes. + if _, err := f.pool.Exec(ctx, `DELETE FROM duplicate_groups WHERE id = $1`, f.groupID); err != nil { + t.Fatal(err) + } verdict := func(id pgtype.UUID) *string { t.Helper() var v *string @@ -465,7 +289,7 @@ func TestResolveDuplicates_HoldsBackRips_Integration(t *testing.T) { } resolve := func() DuplicateResolveResult { t.Helper() - res, err := ResolveDuplicates(ctx, f.pool, nil, "", &fakeLidarr{}, true) + res, err := ResolveDuplicates(ctx, f.pool, nil, "", true) if err != nil { t.Fatal(err) } @@ -512,123 +336,3 @@ func TestResolveDuplicates_HoldsBackRips_Integration(t *testing.T) { t.Errorf("set on an unflagged track: %d, %v; want 0 rows", n, err) } } - -// A rip that is the only copy of its song: Lidarr is asked to search its album -// for a better release (#5447), once a week at most, and the rip is not touched. -// A rip with a clean copy elsewhere is not searched for. -func TestResolveDuplicates_SearchesForASoleCopyRip_Integration(t *testing.T) { - f := newCrossReleaseRipFixture(t) - ctx := context.Background() - exec := func(sql string, args ...any) { - t.Helper() - if _, err := f.pool.Exec(ctx, sql, args...); err != nil { - t.Fatalf("exec %q: %v", sql, err) - } - } - // The single's rip has a clean copy on the album: never searched for. - exec(`UPDATE albums SET release_group_mbid = 'rg-single' WHERE id = $1`, f.rip.AlbumID) - lid := &fakeLidarr{album: lidarr.LidarrAlbum{ID: 77, ForeignAlbumID: "rg-single"}} - if _, err := ResolveDuplicates(ctx, f.pool, nil, "", lid, true); err != nil { - t.Fatal(err) - } - if len(lid.searchCalls) != 0 { - t.Fatalf("searched %v while a clean copy exists", lid.searchCalls) - } - - // Without the group and the song link, the rip is the only copy. - exec(`DELETE FROM duplicate_groups WHERE id = $1`, f.groupID) - exec(`UPDATE tracks SET song_id = NULL WHERE id IN ($1, $2)`, f.rip.ID, f.clean.ID) - res, err := ResolveDuplicates(ctx, f.pool, nil, "", lid, true) - if err != nil { - t.Fatal(err) - } - if res.RipSearches != 1 || len(lid.searchCalls) != 1 || len(lid.searchCalls[0]) != 1 || lid.searchCalls[0][0] != 77 { - t.Fatalf("rip searches %d, calls %v; want album 77 searched once", res.RipSearches, lid.searchCalls) - } - if !exists(f.ripPath) { - t.Error("the rip was removed; a search must not touch it") - } - - // The next pass, inside the week, leaves the album alone. - if res, err := ResolveDuplicates(ctx, f.pool, nil, "", lid, true); err != nil || res.RipSearches != 0 { - t.Errorf("second pass searched %d (%v), want 0", res.RipSearches, err) - } - var searchedAt pgtype.Timestamptz - rows, err := dbq.New(f.pool).ListSuspectSourceTracks(ctx, dbq.ListSuspectSourceTracksParams{ - Pattern: SuspectSourcePattern, PageLimit: 10, - }) - if err != nil { - t.Fatal(err) - } - for _, r := range rows { - if r.ID == f.rip.ID { - searchedAt = r.SearchedAt - } - } - if !searchedAt.Valid { - t.Error("the suspect-sources report doesn't show the search") - } -} - -// Lidarr applies the release change but the PUT times out (Cracker Island on -// the first deploy pass). The change is read back and recorded, so the album -// settles; when it did not land, the error stands and nothing is recorded. -func TestResolveDuplicates_RecordsAReleaseChangeLidarrDidNotAnswer_Integration(t *testing.T) { - for name, landed := range map[string]bool{"landed": true, "did not land": false} { - t.Run(name, func(t *testing.T) { - f := newResolveFixture(t) - lid := &fakeLidarr{ - album: lidarr.LidarrAlbum{ID: 4706, ForeignAlbumID: "rg-humanz", Title: "Cracker Island"}, - releases: []lidarr.AlbumRelease{ - {ID: 1, Format: "Digital Media", TrackCount: 4, Monitored: true}, - {ID: 2, Format: "Digital Media", TrackCount: 2}, - }, - releaseTracks: map[int][]lidarr.ReleaseTrack{ - 1: rt("Saturnz Barz", "Ascension", "Saturnz Barz", "Ascension"), - 2: rt("Saturnz Barz", "Ascension"), - }, - setErr: fmt.Errorf("%w: context deadline exceeded", lidarr.ErrUnreachable), - } - if !landed { - lid.setErr = fmt.Errorf("%w: connection refused", lidarr.ErrUnreachable) - } - // ResetDB keeps audit_log, so other tests' release changes are - // still in it: count what this pass adds. - audited := func() (n int) { - t.Helper() - if err := f.pool.QueryRow(context.Background(), - `SELECT count(*) FROM audit_log WHERE action = 'lidarr_release_change'`).Scan(&n); err != nil { - t.Fatal(err) - } - return n - } - before := audited() - res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", &undoingLidarr{fakeLidarr: lid, undo: !landed}, true) - if landed { - if err != nil || len(res.ReleaseChanges) != 1 { - t.Fatalf("changes %d (%v), want the landed change recorded", len(res.ReleaseChanges), err) - } - } else if len(res.ReleaseChanges) != 0 { - t.Fatalf("recorded %d changes that did not land", len(res.ReleaseChanges)) - } - if got, want := audited()-before, map[bool]int{true: 1, false: 0}[landed]; got != want { - t.Errorf("audited %d release changes, want %d", got, want) - } - }) - } -} - -// undoingLidarr is a Lidarr whose release change fails without landing when -// undo is set: the PUT never reached it. -type undoingLidarr struct { - *fakeLidarr - undo bool -} - -func (u *undoingLidarr) SetMonitoredRelease(ctx context.Context, albumID, releaseID int) error { - if !u.undo { - return u.fakeLidarr.SetMonitoredRelease(ctx, albumID, releaseID) - } - u.setCalls = append(u.setCalls, [2]int{albumID, releaseID}) - return u.setErr -} diff --git a/internal/library/duplicate_survivor.go b/internal/library/duplicate_survivor.go index aaaa0d28..7383d4f8 100644 --- a/internal/library/duplicate_survivor.go +++ b/internal/library/duplicate_survivor.go @@ -14,10 +14,6 @@ type SurvivorCandidate struct { FileSize int64 AddedAt time.Time - // LidarrTracked: Lidarr maps this file to a track of the release it - // monitors. Removing it opens a hole Lidarr downloads again (M498), so a - // tracked copy outranks everything else. False when Lidarr was not asked. - LidarrTracked bool // TagFit is TagFitScore: how well the copy's tags and name fit the album. TagFit int } @@ -56,9 +52,9 @@ var losslessFormats = map[string]bool{"flac": true, "wav": true} // the report shows it and the merge (#3911) lets the operator choose another. // // In order: -// 0. the copy Lidarr maps, then the copy whose tags fit the album best -// (TagFitScore). Both are zero for every copy when the caller does not -// know them, and the order falls through to the quality rules below +// 0. the copy whose tags fit the album best (TagFitScore). It is zero for +// every copy when the caller does not know it, and the order falls through +// to the quality rules below // 1. lossless over lossy — the one difference no later step can recover // 2. the larger file — for one recording at one duration that is the higher // bitrate. The scanner does not record bitrate (tracks.bitrate is never @@ -81,8 +77,6 @@ func ProposeSurvivor(cands []SurvivorCandidate) (trackID, reason string) { // runner-up — the rule that actually decided, not every rule it passed. next := ranked[1] switch { - case best.LidarrTracked != next.LidarrTracked: - return best.TrackID, "the copy Lidarr tracks" case best.TagFit != next.TagFit: return best.TrackID, "tags fit the album" case isLossless(best) != isLossless(next): @@ -97,9 +91,6 @@ func ProposeSurvivor(cands []SurvivorCandidate) (trackID, reason string) { } func survivorBefore(a, b SurvivorCandidate) bool { - if a.LidarrTracked != b.LidarrTracked { - return a.LidarrTracked - } if a.TagFit != b.TagFit { return a.TagFit > b.TagFit } diff --git a/internal/library/duplicate_survivor_test.go b/internal/library/duplicate_survivor_test.go index 4062716f..09368977 100644 --- a/internal/library/duplicate_survivor_test.go +++ b/internal/library/duplicate_survivor_test.go @@ -90,11 +90,10 @@ func TestProposeSurvivor_ReasonIsTheDecidingRule(t *testing.T) { } } -// M498 #5436: the copy Lidarr maps outranks everything, then the copy whose -// tags fit the album; quality decides only between copies equal on both. Each -// case is built so the older rules would choose the other copy, so a reorder -// that drops the new rules fails. -func TestProposeSurvivor_LidarrThenTagFit(t *testing.T) { +// M498 #5436: the copy whose tags fit the album outranks quality, which decides +// only between copies whose tags fit equally. Each case is built so the quality +// rules would choose the other copy, so a reorder that drops tag fit fails. +func TestProposeSurvivor_TagFitFirst(t *testing.T) { at := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC) cases := []struct { name string @@ -103,12 +102,12 @@ func TestProposeSurvivor_LidarrThenTagFit(t *testing.T) { wantReason string }{ { - name: "tracked beats lossless and better tags", + name: "tag fit beats lossless", cands: []SurvivorCandidate{ - {TrackID: "flac", FileFormat: "flac", FileSize: 40_000_000, AddedAt: at, TagFit: 3}, - {TrackID: "tracked", FileFormat: "mp3", FileSize: 5_000_000, AddedAt: at, TagFit: 1, LidarrTracked: true}, + {TrackID: "flac", FileFormat: "flac", FileSize: 40_000_000, AddedAt: at, TagFit: 1}, + {TrackID: "fits", FileFormat: "mp3", FileSize: 5_000_000, AddedAt: at, TagFit: 3}, }, - wantID: "tracked", wantReason: "the copy Lidarr tracks", + wantID: "fits", wantReason: "tags fit the album", }, { // The Humanz shape: two rips a few bytes apart, one carrying a diff --git a/internal/library/song_link_test.go b/internal/library/song_link_test.go index 8d9179b7..3ced2aa1 100644 --- a/internal/library/song_link_test.go +++ b/internal/library/song_link_test.go @@ -78,7 +78,7 @@ func TestCrossReleaseCopiesCountAsOneSong_Integration(t *testing.T) { q := dbq.New(pool) f := newSongLinkFixture(t, pool) - res, err := ResolveDuplicates(ctx, pool, nil, "", nil, true) + res, err := ResolveDuplicates(ctx, pool, nil, "", true) if err != nil { t.Fatal(err) } @@ -118,7 +118,7 @@ func TestCrossReleaseCopiesCountAsOneSong_Integration(t *testing.T) { } // A second pass changes nothing. - if res, err := ResolveDuplicates(ctx, pool, nil, "", nil, true); err != nil || res.SongsLinked != 0 { + if res, err := ResolveDuplicates(ctx, pool, nil, "", true); err != nil || res.SongsLinked != 0 { t.Errorf("second pass linked %d (%v), want 0", res.SongsLinked, err) } @@ -155,7 +155,7 @@ func TestCrossReleaseLinkNeedsAutoResolve_Integration(t *testing.T) { ctx := context.Background() q := dbq.New(pool) f := newSongLinkFixture(t, pool) - if _, err := ResolveDuplicates(ctx, pool, nil, "", nil, false); err != nil { + if _, err := ResolveDuplicates(ctx, pool, nil, "", false); err != nil { t.Fatal(err) } keys := songKeys(t, q, f.single.ID, f.albumCut.ID) diff --git a/internal/lidarr/releases.go b/internal/lidarr/releases.go deleted file mode 100644 index a952d52e..00000000 --- a/internal/lidarr/releases.go +++ /dev/null @@ -1,175 +0,0 @@ -package lidarr - -import ( - "context" - "encoding/json" - "fmt" - "net/url" - "strconv" -) - -// What Lidarr holds on disk and how it maps it (Scribe milestone #498). -// -// Lidarr maps one file to each track of the ONE release it monitors per album. -// A file it holds but maps to no track is "unmapped": removing it opens no hole. -// Removing a mapped file does, and Lidarr fills the hole by downloading again — -// which is why Minstrel's duplicate resolver asks before it deletes anything. - -// TrackFile is a file Lidarr holds on disk. -type TrackFile struct { - ID int `json:"id"` - Path string `json:"path"` - AlbumID int `json:"albumId"` -} - -// ListUnmappedTrackFiles hits GET /api/v1/trackfile?unmapped=true: files under -// Lidarr's root folders that it has scanned but matched to no track. -func (c *Client) ListUnmappedTrackFiles(ctx context.Context) ([]TrackFile, error) { - resp, err := c.get(ctx, "/api/v1/trackfile", url.Values{"unmapped": {"true"}}) - if err != nil { - return nil, err - } - defer func() { _ = resp.Body.Close() }() - var out []TrackFile - if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { - return nil, fmt.Errorf("%w: decode unmapped track files: %v", ErrInvalidPayload, err) - } - return out, nil -} - -// AlbumRelease is one edition of an album as Lidarr knows it: the 14×12" vinyl -// box set, the 2×CD deluxe, the digital standard. Exactly one is Monitored. -type AlbumRelease struct { - ID int `json:"id"` - Title string `json:"title"` - Disambiguation string `json:"disambiguation"` - Format string `json:"format"` - TrackCount int `json:"trackCount"` - MediumCount int `json:"mediumCount"` - Monitored bool `json:"monitored"` -} - -// ReleaseTrack is one track of a release. -type ReleaseTrack struct { - ID int `json:"id"` - Title string `json:"title"` - MediumNumber int `json:"mediumNumber"` - TrackFileID int `json:"trackFileId"` -} - -// GetAlbumReleases hits GET /api/v1/album/{id} and returns the album's releases. -func (c *Client) GetAlbumReleases(ctx context.Context, albumID int) ([]AlbumRelease, error) { - resp, err := c.get(ctx, "/api/v1/album/"+strconv.Itoa(albumID), nil) - if err != nil { - return nil, err - } - defer func() { _ = resp.Body.Close() }() - var album struct { - Releases []AlbumRelease `json:"releases"` - } - if err := json.NewDecoder(resp.Body).Decode(&album); err != nil { - return nil, fmt.Errorf("%w: decode album: %v", ErrInvalidPayload, err) - } - return album.Releases, nil -} - -// ListAlbumTracks hits GET /api/v1/track?albumId= : the tracks of the release -// Lidarr monitors for that album. -func (c *Client) ListAlbumTracks(ctx context.Context, albumID int) ([]ReleaseTrack, error) { - return c.listTracks(ctx, url.Values{"albumId": {strconv.Itoa(albumID)}}) -} - -// ListReleaseTracks hits GET /api/v1/track?albumReleaseId= : the tracks of one -// release, monitored or not. -func (c *Client) ListReleaseTracks(ctx context.Context, releaseID int) ([]ReleaseTrack, error) { - return c.listTracks(ctx, url.Values{"albumReleaseId": {strconv.Itoa(releaseID)}}) -} - -func (c *Client) listTracks(ctx context.Context, q url.Values) ([]ReleaseTrack, error) { - resp, err := c.get(ctx, "/api/v1/track", q) - if err != nil { - return nil, err - } - defer func() { _ = resp.Body.Close() }() - var out []ReleaseTrack - if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { - return nil, fmt.Errorf("%w: decode tracks: %v", ErrInvalidPayload, err) - } - return out, nil -} - -// SetMonitoredRelease makes releaseID the album's monitored release. -// -// It is the round trip Lidarr's own "edit album" makes: GET the album resource, -// flip `monitored` on its releases, PUT the whole resource back. Lidarr's -// UpdateAlbum publishes AlbumEditedEvent, and when the monitored release changed, -// AlbumEditedService unlinks the album's track files and queues -// RescanFoldersCommand for the artist's folder, which maps the files on disk to -// the new release's tracks. Files the new release has no track for come out of -// that rescan unmapped. (Lidarr source, develop: Music/Services/ -// AlbumEditedService.cs and AlbumService.UpdateAlbum.) -// -// The resource is kept as raw JSON so every field Lidarr sent goes back -// unchanged; only the releases' monitored flags differ. Returns ErrNotFound when -// the album has no release with that id. -func (c *Client) SetMonitoredRelease(ctx context.Context, albumID, releaseID int) error { - path := "/api/v1/album/" + strconv.Itoa(albumID) - resp, err := c.get(ctx, path, nil) - if err != nil { - return err - } - var album map[string]any - err = json.NewDecoder(resp.Body).Decode(&album) - _ = resp.Body.Close() - if err != nil { - return fmt.Errorf("%w: decode album: %v", ErrInvalidPayload, err) - } - - releases, _ := album["releases"].([]any) - found := false - for _, r := range releases { - rel, ok := r.(map[string]any) - if !ok { - continue - } - id, _ := rel["id"].(float64) - match := int(id) == releaseID - rel["monitored"] = match - found = found || match - } - if !found { - return fmt.Errorf("%w: release %d on album %d", ErrNotFound, releaseID, albumID) - } - - body, err := json.Marshal(album) - if err != nil { - return fmt.Errorf("%w: marshal album: %v", ErrInvalidPayload, err) - } - putResp, err := c.put(ctx, path, body) - if err != nil { - return err - } - _ = putResp.Body.Close() - return nil -} - -// SearchAlbums asks Lidarr to search its indexers for the albums -// (POST /api/v1/command, AlbumSearch). Lidarr grabs a release only when it -// beats the files the album already has under the quality profile, so for an -// album whose tracks are all present this is an upgrade search: harmless when -// nothing better exists. The command is queued; the call does not wait for it. -func (c *Client) SearchAlbums(ctx context.Context, albumIDs []int) error { - if len(albumIDs) == 0 { - return nil - } - body, err := json.Marshal(map[string]any{"name": "AlbumSearch", "albumIds": albumIDs}) - if err != nil { - return fmt.Errorf("%w: marshal command: %v", ErrInvalidPayload, err) - } - resp, err := c.post(ctx, "/api/v1/command", body) - if err != nil { - return err - } - _ = resp.Body.Close() - return nil -} diff --git a/internal/lidarr/releases_test.go b/internal/lidarr/releases_test.go deleted file mode 100644 index e89a39d2..00000000 --- a/internal/lidarr/releases_test.go +++ /dev/null @@ -1,138 +0,0 @@ -package lidarr - -import ( - "context" - "encoding/json" - "errors" - "io" - "net/http" - "testing" -) - -func TestListUnmappedTrackFiles(t *testing.T) { - c, srv := newTestClient(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/api/v1/trackfile" || r.URL.Query().Get("unmapped") != "true" { - t.Errorf("request = %s?%s, want /api/v1/trackfile?unmapped=true", r.URL.Path, r.URL.RawQuery) - } - _, _ = w.Write([]byte(`[{"id":7,"path":"/music/A/B (2017)/A - B - 02 - X.mp3","albumId":0}]`)) - }) - defer srv.Close() - - got, err := c.ListUnmappedTrackFiles(context.Background()) - if err != nil { - t.Fatalf("ListUnmappedTrackFiles: %v", err) - } - if len(got) != 1 || got[0].ID != 7 || got[0].Path != "/music/A/B (2017)/A - B - 02 - X.mp3" { - t.Errorf("got %+v", got) - } -} - -func TestReleaseReads(t *testing.T) { - c, srv := newTestClient(func(w http.ResponseWriter, r *http.Request) { - switch { - case r.URL.Path == "/api/v1/album/4723": - _, _ = w.Write([]byte(`{"id":4723,"releases":[ - {"id":10148,"title":"Humanz","format":"14x12\" Vinyl, Digital Media","trackCount":68,"mediumCount":15,"monitored":true}, - {"id":10149,"title":"Humanz","format":"Digital Media","trackCount":40,"mediumCount":1,"monitored":false}]}`)) - case r.URL.Path == "/api/v1/track" && r.URL.Query().Get("albumId") == "4723": - _, _ = w.Write([]byte(`[{"id":1,"title":"Ascension","mediumNumber":1,"trackFileId":9}]`)) - case r.URL.Path == "/api/v1/track" && r.URL.Query().Get("albumReleaseId") == "10149": - _, _ = w.Write([]byte(`[{"id":2,"title":"Ascension","mediumNumber":1,"trackFileId":0}]`)) - default: - t.Errorf("unexpected request %s?%s", r.URL.Path, r.URL.RawQuery) - w.WriteHeader(http.StatusNotFound) - } - }) - defer srv.Close() - ctx := context.Background() - - rels, err := c.GetAlbumReleases(ctx, 4723) - if err != nil { - t.Fatalf("GetAlbumReleases: %v", err) - } - if len(rels) != 2 || !rels[0].Monitored || rels[0].MediumCount != 15 || rels[1].TrackCount != 40 { - t.Errorf("releases = %+v", rels) - } - tr, err := c.ListAlbumTracks(ctx, 4723) - if err != nil || len(tr) != 1 || tr[0].TrackFileID != 9 { - t.Errorf("ListAlbumTracks = %+v, %v", tr, err) - } - tr, err = c.ListReleaseTracks(ctx, 10149) - if err != nil || len(tr) != 1 || tr[0].Title != "Ascension" { - t.Errorf("ListReleaseTracks = %+v, %v", tr, err) - } -} - -// The PUT carries the whole resource back, with only the releases' monitored -// flags changed: exactly the chosen release is monitored, and a field Minstrel -// does not model survives the round trip. -func TestSetMonitoredRelease_RoundTripsTheResource(t *testing.T) { - var put map[string]any - c, srv := newTestClient(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/api/v1/album/4723" { - t.Errorf("path = %q", r.URL.Path) - } - switch r.Method { - case http.MethodGet: - _, _ = w.Write([]byte(`{"id":4723,"anyReleaseOk":false,"secondaryTypes":["Studio"],"releases":[ - {"id":10148,"monitored":true},{"id":10149,"monitored":false}]}`)) - case http.MethodPut: - b, _ := io.ReadAll(r.Body) - if err := json.Unmarshal(b, &put); err != nil { - t.Errorf("PUT body: %v", err) - } - w.WriteHeader(http.StatusAccepted) - } - }) - defer srv.Close() - - if err := c.SetMonitoredRelease(context.Background(), 4723, 10149); err != nil { - t.Fatalf("SetMonitoredRelease: %v", err) - } - rels := put["releases"].([]any) - if m := rels[0].(map[string]any)["monitored"]; m != false { - t.Errorf("old release monitored = %v, want false", m) - } - if m := rels[1].(map[string]any)["monitored"]; m != true { - t.Errorf("new release monitored = %v, want true", m) - } - if st, _ := put["secondaryTypes"].([]any); len(st) != 1 || st[0] != "Studio" { - t.Errorf("secondaryTypes = %v, want it carried through unchanged", put["secondaryTypes"]) - } -} - -func TestSetMonitoredRelease_UnknownReleaseSendsNothing(t *testing.T) { - c, srv := newTestClient(func(w http.ResponseWriter, r *http.Request) { - if r.Method == http.MethodPut { - t.Error("PUT sent for a release the album does not have") - } - _, _ = w.Write([]byte(`{"id":4723,"releases":[{"id":10148,"monitored":true}]}`)) - }) - defer srv.Close() - - if err := c.SetMonitoredRelease(context.Background(), 4723, 999); !errors.Is(err, ErrNotFound) { - t.Errorf("err = %v, want ErrNotFound", err) - } -} - -func TestSearchAlbums_PostsTheCommand(t *testing.T) { - var got map[string]any - c, srv := newTestClient(func(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodPost || r.URL.Path != "/api/v1/command" { - t.Errorf("request = %s %s, want POST /api/v1/command", r.Method, r.URL.Path) - } - b, _ := io.ReadAll(r.Body) - _ = json.Unmarshal(b, &got) - w.WriteHeader(http.StatusCreated) - _, _ = w.Write([]byte(`{"id":1,"name":"AlbumSearch"}`)) - }) - defer srv.Close() - - if err := c.SearchAlbums(context.Background(), []int{4723, 12}); err != nil { - t.Fatalf("SearchAlbums: %v", err) - } - ids, _ := got["albumIds"].([]any) - if got["name"] != "AlbumSearch" || len(ids) != 2 || ids[0] != float64(4723) || ids[1] != float64(12) { - t.Errorf("command = %v", got) - } -} diff --git a/internal/server/server.go b/internal/server/server.go index 978234bb..bb77f503 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -57,17 +57,6 @@ func (a lidarrUnmonitorAdapter) UnmonitorTrack(ctx context.Context, trackMbid, a return c.UnmonitorTrack(ctx, trackMbid, albumMbid) } -// ListUnmappedTrackFiles lets the duplicate merge refuse to remove a copy -// Lidarr maps (M498). tracks.ErrLidarrDisabled tells it there is no Lidarr to -// protect. -func (a lidarrUnmonitorAdapter) ListUnmappedTrackFiles(ctx context.Context) ([]lidarr.TrackFile, error) { - c := a.fn() - if c == nil { - return nil, tracks.ErrLidarrDisabled - } - return c.ListUnmappedTrackFiles(ctx) -} - // ScanTrigger is the subset of the scanner the HTTP handler needs. Kept as an // interface so tests can stub it without touching the DB. The progressCb // parameter (added in m7-scan-progress) lets the orchestrator drive partial- diff --git a/internal/tracks/service.go b/internal/tracks/service.go index 1d460677..1d576bd1 100644 --- a/internal/tracks/service.go +++ b/internal/tracks/service.go @@ -31,7 +31,6 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/audit" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/library" - "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" ) @@ -41,22 +40,6 @@ import ( // same pointer. var ErrNotFound = apierror.ErrNotFound -// ErrLidarrDisabled is what a LidarrUnmonitorer that also lists Lidarr's -// unmapped files returns when Lidarr is turned off: there is then no Lidarr -// record a removal could break, and a merge goes ahead unguarded. -var ErrLidarrDisabled = errors.New("tracks: lidarr disabled") - -// ErrLidarrUnavailable means Lidarr is on but did not answer, so a merge cannot -// confirm the copies it would remove are ones Lidarr does not need (M498), and -// refuses rather than guess. -var ErrLidarrUnavailable = errors.New("tracks: lidarr did not answer") - -// lidarrFileLister is the optional part of the Lidarr dependency a merge uses to -// learn which files Lidarr holds without mapping to any track. -type lidarrFileLister interface { - ListUnmappedTrackFiles(ctx context.Context) ([]lidarr.TrackFile, error) -} - // LidarrUnmonitorer is the subset of *lidarr.Client RemoveTrack uses. // Defined as an interface so tests can stub without spinning up a Lidarr // httptest server. UnmonitorTrack failures are non-fatal at the service @@ -168,9 +151,7 @@ func (s *Service) RemoveTrack( } // MergeDuplicates merges a duplicate group into the copy to keep -// (library.MergeDuplicateGroupGuarded, refusing to remove a copy Lidarr maps: -// library.ErrCopyTrackedByLidarr, or ErrLidarrUnavailable when Lidarr cannot -// say), then — when asked — tells Lidarr to stop +// (library.MergeDuplicateGroup), then — when asked — tells Lidarr to stop // monitoring the removed copies so it does not download them again, and records // the merge in the audit log. // @@ -180,11 +161,7 @@ func (s *Service) RemoveTrack( func (s *Service) MergeDuplicates( ctx context.Context, groupID, survivorID, actorID pgtype.UUID, unmonitor bool, ) (res library.MergeResult, lidarrUnmonitorFailed bool, err error) { - removable, err := s.lidarrRemovable(ctx) - if err != nil { - return res, false, err - } - res, err = library.MergeDuplicateGroupGuarded(ctx, s.pool, s.logger, s.dataDir, groupID, survivorID, removable) + res, err = library.MergeDuplicateGroup(ctx, s.pool, s.logger, s.dataDir, groupID, survivorID) if err != nil { return res, false, err } @@ -226,29 +203,6 @@ func (s *Service) MergeDuplicates( return res, lidarrUnmonitorFailed, nil } -// lidarrRemovable is the merge's guard (M498): only a copy Lidarr holds without -// mapping it to a track may be removed, because removing a mapped one makes -// Lidarr download it again. Nil, allowing everything, when Lidarr is disabled -// or the dependency cannot list files. -func (s *Service) lidarrRemovable(ctx context.Context) (library.RemovableFunc, error) { - lister, ok := s.lidarr.(lidarrFileLister) - if !ok { - return nil, nil - } - files, err := lister.ListUnmappedTrackFiles(ctx) - if errors.Is(err, ErrLidarrDisabled) { - return nil, nil - } - if err != nil { - return nil, fmt.Errorf("%w: %v", ErrLidarrUnavailable, err) - } - unmapped := make(map[string]bool, len(files)) - for _, f := range files { - unmapped[library.LidarrPathKey(f.Path)] = true - } - return func(p string) bool { return unmapped[library.LidarrPathKey(p)] }, nil -} - // sameLidarrTrack reports whether two copies are the same Lidarr track: one // recording on one album. Lidarr monitors per album track, so when the removed // copy is a second file of the kept copy's own album track — the #3885 case — diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index 2fef805d..8cfc208a 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -479,10 +479,6 @@ export type AdminDuplicateMember = { added_at: string; like_count: number; play_count: number; - // Lidarr's view of the file as of the resolver's last pass (M498): tracked - // means Lidarr maps it to a track, so removing it would make Lidarr download - // it again. null when Lidarr wasn't asked. - lidarr_state: 'tracked' | 'unmapped' | null; disc_number: number | null; track_number: number | null; }; diff --git a/web/src/lib/components/FingerprintSettingsCard.svelte b/web/src/lib/components/FingerprintSettingsCard.svelte index 8bc410e5..186cb80f 100644 --- a/web/src/lib/components/FingerprintSettingsCard.svelte +++ b/web/src/lib/components/FingerprintSettingsCard.svelte @@ -120,10 +120,9 @@ Resolve duplicates automatically - Merges copies Lidarr doesn't use into the one it does, and moves a Lidarr album off a - release that lists songs twice. A song kept on several releases counts as one song, and - video rips are kept out of radio and the mixes. A copy Lidarr uses is never removed. - Off, every group waits here for you. + Merges a song held twice on one album, and a video rip with a clean copy elsewhere, into + the copy that fits best. A song kept on several releases counts as one song, and video + rips are kept out of radio and the mixes. Off, every group waits here for you. diff --git a/web/src/lib/styles/error-copy.json b/web/src/lib/styles/error-copy.json index 2af87301..b3b56229 100644 --- a/web/src/lib/styles/error-copy.json +++ b/web/src/lib/styles/error-copy.json @@ -46,8 +46,6 @@ "sweep_in_progress": "A duplicate sweep is already running.", "duplicate_group_not_pending": "That group has already been resolved.", "survivor_not_in_group": "That copy isn't part of this group any more.", - "copy_tracked_by_lidarr": "Lidarr uses a copy this would remove, and would download it again. Keep that copy instead.", - "lidarr_unavailable": "Lidarr didn't answer, so it isn't safe to remove a copy yet. Try again shortly.", "suspect_track_not_found": "That track isn't flagged any more. Refresh the list.", "invalid_verdict": "That isn't a choice for a flagged track.", "invalid_setting": "That setting is out of range.", diff --git a/web/src/routes/admin/duplicates/+page.svelte b/web/src/routes/admin/duplicates/+page.svelte index 7a1dca40..35f39834 100644 --- a/web/src/routes/admin/duplicates/+page.svelte +++ b/web/src/routes/admin/duplicates/+page.svelte @@ -23,17 +23,17 @@ import TabStrip from '#lib/components/TabStrip.svelte'; // Tracks the duplicate sweep believes hold one recording (#3912). The - // resolver (M498) settles what is safe on its own: copies Lidarr doesn't use - // are merged into the one it does, and a Lidarr release that lists songs - // twice is changed. What is left comes in two tabs: groups that need the - // operator, and the same song on different releases, which is kept on - // purpose. A third tab lists what the resolver did. + // resolver (M498, M502) settles what is safe on its own: a song held twice on + // one album, and a video rip with a clean copy elsewhere. What is left comes + // in two tabs: groups that need the operator, and the same song on different + // releases, which is kept on purpose. A third tab lists what the resolver + // did. // // Dismissing a group says "these are not duplicates", and the sweep will not // propose that set again. On the cross-release tab it reads "Not the same // song" and also undoes the link that made the copies one song (#5438). Merging (#3911) keeps one copy, moves the others' // likes, plays and playlist entries onto it, and deletes their files, so it - // asks twice. The server refuses to remove a copy Lidarr uses. + // asks twice. type Tab = DuplicatesView | 'resolved'; const fromUrl = page.url.searchParams.get('view'); @@ -108,13 +108,6 @@ return keepChoice[group.id] ?? group.survivor_track_id; } - // Copies the merge would remove that Lidarr uses. The server refuses these, - // so the confirm says so before the click rather than after. - function trackedLosers(group: AdminDuplicateGroup): AdminDuplicateMember[] { - const keep = keeperOf(group); - return group.members.filter((m) => m.track_id !== keep && m.lidarr_state === 'tracked'); - } - function fileCountLabel(n: number): string { return n === 1 ? '1 file' : `${n} files`; } @@ -223,8 +216,8 @@

- Tracks that hold the same recording more than once. Copies Lidarr doesn't use are merged on - their own, and a copy Lidarr uses is never removed. What's left here needs you. + 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.

@@ -265,7 +258,7 @@

Nothing resolved automatically yet.

- Merges and Lidarr release changes made on their own are listed here. + Merges made on their own are listed here.

{:else} @@ -279,7 +272,7 @@ {#if item.details.survivor_reason} ({item.details.survivor_reason}) {/if} - and removed {fileCountLabel(removed.length)} Lidarr doesn't use. + and removed {fileCountLabel(removed.length)}.

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

{r.file_path}

@@ -354,7 +347,6 @@
    {#each groups as group (group.id)} {@const titles = differingTitles(group)} - {@const blocked = trackedLosers(group)}
  • @@ -373,7 +365,7 @@
    {m.artist_name} · {m.album_title}{position(m) ? ` · ${position(m)}` : ''} diff --git a/web/src/routes/admin/duplicates/duplicates.test.ts b/web/src/routes/admin/duplicates/duplicates.test.ts index 4f452295..1060fca1 100644 --- a/web/src/routes/admin/duplicates/duplicates.test.ts +++ b/web/src/routes/admin/duplicates/duplicates.test.ts @@ -66,7 +66,6 @@ function member(id: string, extra: Partial { expect(dismissDuplicateGroup).toHaveBeenCalledWith('g-1'); }); - test('Resolved automatically lists merges and release changes', async () => { + // Release changes stopped with Lidarr (M502), but the ones made before stay + // in the history. + test('Resolved automatically lists merges and past release changes', async () => { renderWith(response(), [ { id: 'a-1', @@ -340,7 +341,7 @@ describe('admin duplicates', () => { created_at: ago(HOUR), details: { survivor_path: '/music/Gorillaz/Humanz (2017)/03 - Saturnz Barz.mp3', - survivor_reason: 'the copy Lidarr tracks', + survivor_reason: 'the copy that is not a video rip', removed: [{ track_id: 't-2', file_path: '/music/Gorillaz/Humanz (2017)/03 - Saturnz Barz (Official Video).mp3' }] } }, @@ -358,47 +359,26 @@ describe('admin duplicates', () => { ]); await fireEvent.click(screen.getByRole('tab', { name: /resolved automatically/i })); const items = screen.getAllByTestId('resolved-item').map(text); - expect(items[0]).toContain('Kept 03 - Saturnz Barz.mp3 (the copy Lidarr tracks) and removed 1 file'); + expect(items[0]).toContain('Kept 03 - Saturnz Barz.mp3 (the copy that is not a video rip) and removed 1 file.'); expect(items[1]).toContain('Lidarr now monitors Digital Media (40 tracks) of Humanz by Gorillaz'); expect(screen.queryByTestId('duplicate-group')).toBeNull(); }); }); - describe("Lidarr's view of each copy", () => { - function tracked() { + describe("the resolver's note", () => { + test('shows above the group it explains', () => { const r = response(); - r.groups[0] = { - ...r.groups[0], - resolve_note: 'Lidarr tracks every copy as its own track.', - members: [member('www-01', { lidarr_state: 'tracked' }), member('www-02', { lidarr_state: 'unmapped' })] - }; - return r; - } - - test('each copy says whether Lidarr uses it, and the resolver note shows', () => { - renderWith(tracked()); - expect(screen.getAllByTestId('lidarr-state').map(text)).toEqual([ - 'Lidarr uses this', - "Lidarr doesn't use this" - ]); - expect(text(screen.getByTestId('resolve-note'))).toBe('Lidarr tracks every copy as its own track.'); + r.groups[0] = { ...r.groups[0], resolve_note: 'Identical audio filed under different titles.' }; + renderWith(r); + expect(text(screen.getByTestId('resolve-note'))).toBe('Identical audio filed under different titles.'); }); - // Keeping the unmapped copy would remove the one Lidarr uses. The server - // refuses that; the confirm says so first and offers no button to press. - test('a merge that would remove the copy Lidarr uses is blocked before it is sent', async () => { - renderWith(tracked()); + test('a merge confirms and sends the copy chosen to keep', async () => { + renderWith(response()); await fireEvent.click(screen.getAllByRole('radio')[1]); await fireEvent.click(screen.getByRole('button', { name: 'Merge…' })); - expect(text(screen.getByTestId('merge-blocked'))).toContain('Lidarr uses a copy this would remove'); - const confirm = screen.getByRole('button', { name: 'Remove 1 file and merge' }) as HTMLButtonElement; - expect(confirm.disabled).toBe(true); - - // Choosing the copy Lidarr uses clears it. - await fireEvent.click(screen.getAllByRole('radio')[0]); - expect(screen.queryByTestId('merge-blocked')).toBeNull(); await fireEvent.click(screen.getByRole('button', { name: 'Remove 1 file and merge' })); - expect(mergeDuplicateGroup).toHaveBeenCalledWith('g-1', { survivor_track_id: 'www-01', unmonitor: false }); + expect(mergeDuplicateGroup).toHaveBeenCalledWith('g-1', { survivor_track_id: 'www-02', unmonitor: false }); }); }); });