From b9684e6d12c4f5f39989e803c99daf09598647a6 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 21 Oct 2026 13:47:43 -0400 Subject: [PATCH] fix: a group of one recording in several slots is settled by getting the album again, not merged (#6184) The duplicates review offered only Merge for a wrong-slot group. A merge keeps one copy's title whether or not it is the audio's, and neither sets the release aside nor wants the missing song back. For class mismatch the page now asks which song the recording is. "Get it again" (the operator can't tell) removes every copy; "Keep " keeps that copy and removes the ones under other titles. Either way it runs the resolver's own path: the release the files came from is set aside, the removed copies' tracks stay marked missing and emptied, and the album is wanted again. - library.SettleWrongSlot, POST /api/admin/library/duplicates/{id}/refetch - ListDuplicateGroupsForResolve takes an optional group_id - removeWrongSlots takes the acting operator (manual: not automatic, actor in the audit row) and removes files through removeTrackFileOnDisk, so a read-only library answers library_not_writable Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- internal/api/admin_duplicates.go | 87 ++++++++ internal/api/api.go | 1 + internal/db/dbq/duplicates.sql.go | 8 +- internal/db/queries/duplicates.sql | 4 +- internal/library/duplicate_resolve.go | 19 +- internal/library/duplicate_settle.go | 103 +++++++++- internal/library/duplicate_settle_test.go | 148 +++++++++++++ web/src/lib/api/admin.ts | 13 ++ web/src/lib/api/types.ts | 9 + .../routes/collection/duplicates/+page.svelte | 194 +++++++++++++++--- .../collection/duplicates/duplicates.test.ts | 65 +++++- 11 files changed, 601 insertions(+), 50 deletions(-) create mode 100644 internal/library/duplicate_settle_test.go diff --git a/internal/api/admin_duplicates.go b/internal/api/admin_duplicates.go index de8fe2e1..87260c05 100644 --- a/internal/api/admin_duplicates.go +++ b/internal/api/admin_duplicates.go @@ -449,3 +449,90 @@ func (h *handlers) handleMergeDuplicateGroup(w http.ResponseWriter, r *http.Requ } writeJSON(w, http.StatusOK, resp) } + +// refetchDuplicateRequest is the operator's answer for one recording filling +// several of an album's slots. keep_track_id is the copy whose title the +// recording is; empty means they cannot say, and every copy goes. +type refetchDuplicateRequest struct { + KeepTrackID string `json:"keep_track_id"` +} + +// refetchDuplicateResponse reports what went. Blocked names the releases set +// aside; empty when the album came from before Minstrel downloaded anything. +type refetchDuplicateResponse struct { + KeptTrackID *string `json:"kept_track_id"` + RemovedPaths []string `json:"removed_paths"` + Blocked []string `json:"blocked"` +} + +// handleRefetchDuplicateGroup implements POST +// /api/admin/library/duplicates/{id}/refetch (#6184): settle a group of one +// recording in several slots of an album the way the resolver would, with the +// operator's answer. The copies in slots that are not the recording's are +// removed and marked missing, the release they came from is set aside, and the +// album is wanted again. Merge is the wrong action there: it keeps one title +// whether or not it is the audio's, and fetches nothing. +// +// Errors: +// - 409 library_not_writable / 500 file_delete_failed when a file could not be +// removed (fileRemoveAPIError) +// - 404 duplicate_group_not_pending when the group was already resolved +// - 400 not_wrong_slot when the group is not one recording in several slots +// - 400 survivor_not_in_group, invalid_id, invalid_body +func (h *handlers) handleRefetchDuplicateGroup(w http.ResponseWriter, r *http.Request) { + admin, ok := requireUser(w, r) + if !ok { + return + } + groupID, ok := parseUUID(chi.URLParam(r, "id")) + if !ok { + writeAdminJSONErr(w, http.StatusBadRequest, "invalid_id") + return + } + var body refetchDuplicateRequest + if err := json.NewDecoder(http.MaxBytesReader(w, r.Body, mergeRequestBodyLimit)).Decode(&body); err != nil && !errors.Is(err, io.EOF) { + writeAdminJSONErr(w, http.StatusBadRequest, "invalid_body") + return + } + var keepID pgtype.UUID // invalid: every copy goes + if body.KeepTrackID != "" { + if keepID, ok = parseUUID(body.KeepTrackID); !ok { + writeAdminJSONErr(w, http.StatusBadRequest, "invalid_id") + return + } + } + + res, err := library.SettleWrongSlot(r.Context(), h.pool, h.logger, groupID, keepID, admin.ID) + if err != nil { + if apiErr, ok := fileRemoveAPIError(err); ok { + logFileRemoveFailure(h.logger, apiErr, "group_id", uuidToString(groupID)) + writeErr(w, apiErr) + return + } + switch { + case errors.Is(err, library.ErrDuplicateGroupNotPending): + writeAdminJSONErr(w, http.StatusNotFound, "duplicate_group_not_pending") + case errors.Is(err, library.ErrNotWrongSlot): + writeAdminJSONErr(w, http.StatusBadRequest, "not_wrong_slot") + case errors.Is(err, library.ErrSurvivorNotInGroup): + writeAdminJSONErr(w, http.StatusBadRequest, "survivor_not_in_group") + default: + h.logger.Error("admin: refetch duplicate group", "group_id", uuidToString(groupID), "err", err) + writeAdminJSONErr(w, http.StatusInternalServerError, "server_error") + } + return + } + + resp := refetchDuplicateResponse{RemovedPaths: make([]string, 0, len(res.Removed)), Blocked: res.Blocked} + if resp.Blocked == nil { + resp.Blocked = []string{} + } + if res.Kept != nil { + id := uuidToString(res.Kept.TrackID) + resp.KeptTrackID = &id + } + for _, c := range res.Removed { + resp.RemovedPaths = append(resp.RemovedPaths, c.FilePath) + } + writeJSON(w, http.StatusOK, resp) +} diff --git a/internal/api/api.go b/internal/api/api.go index 7dca451c..45d85b2c 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -333,6 +333,7 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev admin.Post("/library/duplicates/sweep", h.handleRunDuplicateSweep) admin.Post("/library/duplicates/{id}/dismiss", h.handleDismissDuplicateGroup) admin.Post("/library/duplicates/{id}/merge", h.handleMergeDuplicateGroup) + admin.Post("/library/duplicates/{id}/refetch", h.handleRefetchDuplicateGroup) admin.Get("/invites", h.handleListInvites) admin.Post("/invites", h.handleCreateInvite) diff --git a/internal/db/dbq/duplicates.sql.go b/internal/db/dbq/duplicates.sql.go index 880f34da..9e7b70d2 100644 --- a/internal/db/dbq/duplicates.sql.go +++ b/internal/db/dbq/duplicates.sql.go @@ -328,6 +328,7 @@ SELECT g.id AS group_id, LEFT JOIN track_acoustid_lookups l ON l.track_id = t.id WHERE g.status = 'pending' AND t.missing_since IS NULL + AND ($1::uuid IS NULL OR g.id = $1::uuid) ORDER BY g.id, t.id ` @@ -366,9 +367,10 @@ type ListDuplicateGroupsForResolveRow struct { // tags or the library listing's match; ” when neither names one. // acoustid_state and acoustid_recording are what AcoustID said of the file, // when it was looked up (#6169: which song one recording cut to several slots -// is). -func (q *Queries) ListDuplicateGroupsForResolve(ctx context.Context) ([]ListDuplicateGroupsForResolveRow, error) { - rows, err := q.db.Query(ctx, listDuplicateGroupsForResolve) +// is). group_id narrows it to one group, for the operator settling it (#6184); +// NULL reads them all. +func (q *Queries) ListDuplicateGroupsForResolve(ctx context.Context, groupID pgtype.UUID) ([]ListDuplicateGroupsForResolveRow, error) { + rows, err := q.db.Query(ctx, listDuplicateGroupsForResolve, groupID) if err != nil { return nil, err } diff --git a/internal/db/queries/duplicates.sql b/internal/db/queries/duplicates.sql index d38c6e89..12cc6f14 100644 --- a/internal/db/queries/duplicates.sql +++ b/internal/db/queries/duplicates.sql @@ -197,7 +197,8 @@ UPDATE duplicate_groups -- tags or the library listing's match; '' when neither names one. -- acoustid_state and acoustid_recording are what AcoustID said of the file, -- when it was looked up (#6169: which song one recording cut to several slots --- is). +-- is). group_id narrows it to one group, for the operator settling it (#6184); +-- NULL reads them all. SELECT g.id AS group_id, g.tier, g.worst_bit_error_rate, @@ -232,6 +233,7 @@ SELECT g.id AS group_id, LEFT JOIN track_acoustid_lookups l ON l.track_id = t.id WHERE g.status = 'pending' AND t.missing_since IS NULL + AND (sqlc.narg(group_id)::uuid IS NULL OR g.id = sqlc.narg(group_id)::uuid) ORDER BY g.id, t.id; -- name: SetDuplicateGroupClasses :exec diff --git a/internal/library/duplicate_resolve.go b/internal/library/duplicate_resolve.go index 14f71163..2fc39e05 100644 --- a/internal/library/duplicate_resolve.go +++ b/internal/library/duplicate_resolve.go @@ -160,7 +160,7 @@ func ResolveDuplicatesWith( q := dbq.New(pool) res := DuplicateResolveResult{Classes: map[DuplicateClass]int{}} - rows, err := q.ListDuplicateGroupsForResolve(ctx) + rows, err := q.ListDuplicateGroupsForResolve(ctx, pgtype.UUID{}) if err != nil { return res, fmt.Errorf("list duplicate groups: %w", err) } @@ -168,12 +168,7 @@ func ResolveDuplicatesWith( res.Groups = len(groups) for _, g := range groups { - cm := make([]ClassifyMember, len(g.members)) - for i, m := range g.members { - cm[i] = ClassifyMember{AlbumID: syncpkg.FormatUUID(m.AlbumID), Title: m.Title, ArtistName: m.ArtistName} - } - g.class = ClassifyDuplicateGroup(g.tier, cm) - g.note = resolveNote(g) + g.classify() res.Classes[g.class]++ } if err := writeResolveVerdicts(ctx, q, groups); err != nil { @@ -444,6 +439,16 @@ func foldResolveGroups(rows []dbq.ListDuplicateGroupsForResolveRow) []*resolveGr return out } +// classify sets the group's class and the note the report shows beside it. +func (g *resolveGroup) classify() { + cm := make([]ClassifyMember, len(g.members)) + for i, m := range g.members { + cm[i] = ClassifyMember{AlbumID: syncpkg.FormatUUID(m.AlbumID), Title: m.Title, ArtistName: m.ArtistName} + } + g.class = ClassifyDuplicateGroup(g.tier, cm) + g.note = resolveNote(g) +} + // resolveNote is what the report says beside a group, when there is something // to say. func resolveNote(g *resolveGroup) string { diff --git a/internal/library/duplicate_settle.go b/internal/library/duplicate_settle.go index b73ac791..f6812b5d 100644 --- a/internal/library/duplicate_settle.go +++ b/internal/library/duplicate_settle.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "log/slog" - "os" "slices" "github.com/jackc/pgx/v5" @@ -45,7 +44,8 @@ import ( // Whenever copies go, the release the files came from is set aside, so the // album fetched again is not the same files (#6169). // -// Anything less certain stays with the operator. +// Anything less certain stays with the operator, who settles it the same way +// with their own answer (SettleWrongSlot). const ( // trackListFetchesPerPass is how many albums' track lists a pass may ask @@ -239,7 +239,7 @@ func settleWrongSlots( // still does not fetch the same files again. blocked := blockReleasesHolding(ctx, pool, logger, g) emptied := emptiedSlots(ctx, q, logger, g, losers, tracks) - n, err := removeWrongSlots(ctx, pool, logger, g, keep, losers, emptied, blocked) + n, err := removeWrongSlots(ctx, pool, logger, g, keep, losers, emptied, blocked, pgtype.UUID{}) removed += n if err != nil { logger.Warn("duplicate resolve: removing wrong-slot copies failed", "group_id", syncpkg.FormatUUID(g.id), "err", err) @@ -253,6 +253,86 @@ func settleWrongSlots( return removed, names } +// ErrNotWrongSlot means a group is not one recording filling several of an +// album's track slots, so there is no slot to empty and nothing to fetch again. +var ErrNotWrongSlot = errors.New("library: duplicate group is not one recording in several slots") + +// WrongSlotResult is what settling a wrong slot by hand did. Kept is nil when +// every copy went. +type WrongSlotResult struct { + Kept *MergedCopy + Removed []MergedCopy + // Blocked names the releases set aside. None when the album was imported + // before Minstrel downloaded anything: the same files may come back. + Blocked []string +} + +// SettleWrongSlot settles a one-album mismatch the resolver left for the +// operator (#6184), with their answer in place of MusicBrainz's lengths and +// AcoustID's. keepID is the copy whose title they say the recording is; an +// invalid id means they cannot say, and every copy goes. Either way it is the +// resolver's own path: the release the files came from is set aside, the +// copies removed are marked missing, and the album is wanted again for them. +// A merge does none of that, and keeps one copy's title whether or not it is +// the audio's. +// +// Copies sharing the kept copy's title are one song held twice and stay, as in +// wrongSlots: the next sweep finds the pair and the resolver merges it. +func SettleWrongSlot( + ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, groupID, keepID, actorID pgtype.UUID, +) (WrongSlotResult, error) { + if logger == nil { + logger = slog.Default() + } + q := dbq.New(pool) + rows, err := q.ListDuplicateGroupsForResolve(ctx, groupID) + if err != nil { + return WrongSlotResult{}, fmt.Errorf("read duplicate group: %w", err) + } + groups := foldResolveGroups(rows) + if len(groups) == 0 { + return WrongSlotResult{}, ErrDuplicateGroupNotPending + } + g := groups[0] + if g.classify(); g.class != ClassMismatch { + return WrongSlotResult{}, ErrNotWrongSlot + } + keep := -1 + if keepID.Valid { + keep = slices.IndexFunc(g.members, func(m dbq.ListDuplicateGroupsForResolveRow) bool { return m.TrackID == keepID }) + if keep < 0 { + return WrongSlotResult{}, ErrSurvivorNotInGroup + } + } + var losers []int + for i, m := range g.members { + if keep >= 0 && MatchTitleKey(m.Title, m.ArtistName) == MatchTitleKey(g.members[keep].Title, m.ArtistName) { + continue + } + losers = append(losers, i) + } + if len(losers) == 0 { + return WrongSlotResult{}, ErrNotWrongSlot + } + var tracks []dbq.MbReleaseTrack + if rg := g.members[0].ReleaseGroupMbid; rg != "" { + if tracks, err = q.ListMBReleaseTracksForGroup(ctx, rg); err != nil { + return WrongSlotResult{}, fmt.Errorf("read the album's track list: %w", err) + } + } + + res := WrongSlotResult{Blocked: blockReleasesHolding(ctx, pool, logger, g)} + emptied := emptiedSlots(ctx, q, logger, g, losers, tracks) + n, err := removeWrongSlots(ctx, pool, logger, g, keep, losers, emptied, res.Blocked, actorID) + if keep >= 0 { + res.Kept = &MergedCopy{TrackID: g.members[keep].TrackID, FilePath: g.members[keep].FilePath} + } + for _, i := range losers[:n] { + res.Removed = append(res.Removed, MergedCopy{TrackID: g.members[i].TrackID, FilePath: g.members[i].FilePath}) + } + return res, err +} + // sameAudio reports whether a group's copies are one recording, cut however: // identical audio, or fingerprints all but identical (sameAudioBER). func sameAudio(g *resolveGroup) bool { @@ -442,18 +522,19 @@ func neverRead(ctx context.Context, q *dbq.Queries, rg string) bool { // are stamped to wait for a copy fetched again (emptiedSlots). blocked names // the releases set aside, for the record. A file that cannot be removed stops // the group; the ones already gone are still marked, and the group waits for -// the next pass. +// the next pass. actorID is the operator settling it by hand, invalid for the +// resolver. func removeWrongSlots( ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, g *resolveGroup, keep int, losers []int, - emptied []pgtype.UUID, blocked []string, + emptied []pgtype.UUID, blocked []string, actorID pgtype.UUID, ) (int, error) { var gone []pgtype.UUID var removedFiles []map[string]string var removeErr error for _, i := range losers { m := g.members[i] - if err := removeFile(m.FilePath); err != nil && !errors.Is(err, os.ErrNotExist) { - removeErr = fmt.Errorf("remove %s: %w", m.FilePath, err) + if err := removeTrackFileOnDisk(m.FilePath); err != nil { + removeErr = err break } gone = append(gone, m.TrackID) @@ -481,6 +562,10 @@ func removeWrongSlots( if removeErr != nil { return nil } + if actorID.Valid { + _, err := tq.MarkDuplicateGroupMerged(ctx, g.id) + return err + } _, err := tq.SettleDuplicateGroupAutomatically(ctx, dbq.SettleDuplicateGroupAutomaticallyParams{ ID: g.id, Status: "merged", }) @@ -490,7 +575,7 @@ func removeWrongSlots( return 0, err } details := map[string]any{ - "automatic": true, + "automatic": !actorID.Valid, "group_id": syncpkg.FormatUUID(g.id), "artist": g.members[0].ArtistName, "removed": removedFiles, @@ -504,7 +589,7 @@ func removeWrongSlots( details["kept_title"], details["kept_path"] = kept.Title, kept.FilePath keptPath = kept.FilePath } - audit.WriteOrLog(ctx, pool, logger, pgtype.UUID{}, pgtype.UUID{}, audit.ActionDuplicateWrongFile, details) + audit.WriteOrLog(ctx, pool, logger, actorID, pgtype.UUID{}, audit.ActionDuplicateWrongFile, details) logger.Info("duplicate resolve: removed copies from slots that were not theirs", "group_id", syncpkg.FormatUUID(g.id), "kept", keptPath, "removed", len(gone)) return len(gone), removeErr diff --git a/internal/library/duplicate_settle_test.go b/internal/library/duplicate_settle_test.go new file mode 100644 index 00000000..8c623ff4 --- /dev/null +++ b/internal/library/duplicate_settle_test.go @@ -0,0 +1,148 @@ +package library + +import ( + "context" + "errors" + "path/filepath" + "strings" + "testing" + + "github.com/jackc/pgx/v5/pgtype" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/dbtest" + syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" +) + +// undecidedSlot is one recording filed as Void and as Reach, two songs +// MusicBrainz lists at lengths that cannot tell them apart, from a download +// Minstrel imported: the resolver leaves it pending for the operator (#6184). +func undecidedSlot(t *testing.T) (oneAlbumFixture, dbq.User) { + t.Helper() + f := newOneAlbumFixture(t, "acoustic", "Void", "Reach") + ctx := context.Background() + seedTrackList(t, f.pool, f.album, map[string]int{"Void": 220500, "Reach": 221000}) + if _, err := f.pool.Exec(ctx, + `INSERT INTO grabs (info_hash, name, key, client, save_path, origin, import_state, destination) + VALUES ('abc123', 'Savant - Void [WEB]', 'k', 'qbit', '/dl', 'schedule', 'imported', $1)`, + filepath.Dir(f.aPath)); err != nil { + t.Fatal(err) + } + if res, err := ResolveDuplicates(ctx, f.pool, nil, "", true); err != nil || res.WrongFiles != 0 { + t.Fatalf("resolve = %+v, %v; want the group left for the operator", res, err) + } + name := strings.NewReplacer("/", "-", " ", "-").Replace(t.Name()) + u, err := dbq.New(f.pool).CreateUser(ctx, dbq.CreateUserParams{ + Username: dbtest.TestUserPrefix + name, PasswordHash: "x", ApiTokenHash: name + "-token", + }) + if err != nil { + t.Fatal(err) + } + return f, u +} + +// emptiedTrack says whether a copy's track is marked missing and waiting for +// the album to be fetched again. +func emptiedTrack(t *testing.T, f oneAlbumFixture, id pgtype.UUID) bool { + t.Helper() + var emptied bool + if err := f.pool.QueryRow(context.Background(), + `SELECT missing_since IS NOT NULL AND emptied_at IS NOT NULL FROM tracks WHERE id = $1`, id).Scan(&emptied); err != nil { + t.Fatalf("track %s is gone: %v", syncpkg.FormatUUID(id), err) + } + return emptied +} + +// settledByHand checks the group closed as the operator's and the audit row +// names them. +func settledByHand(t *testing.T, f oneAlbumFixture, actor dbq.User) { + t.Helper() + if status, _, _, auto := (resolveFixture{pool: f.pool, groupID: f.groupID}).group(t); status != "merged" || auto { + t.Errorf("group = (%s, auto=%v), want merged by the operator", status, auto) + } + var n int + if err := f.pool.QueryRow(context.Background(), + `SELECT count(*) FROM audit_log WHERE action = 'duplicate_wrong_file' AND actor_id = $1 + AND metadata->>'group_id' = $2 AND NOT (metadata->>'automatic')::boolean`, + actor.ID, syncpkg.FormatUUID(f.groupID)).Scan(&n); err != nil { + t.Fatal(err) + } + if n != 1 { + t.Errorf("operator's wrong-file audit rows = %d, want 1", n) + } +} + +// "Get it again": the operator cannot say which song the recording is, so +// every copy goes, the download they came from is set aside, and both slots +// wait for the album fetched again. A merge would have kept one title. +func TestSettleWrongSlot_GetItAgainRemovesEveryCopy_Integration(t *testing.T) { + f, op := undecidedSlot(t) + ctx := context.Background() + res, err := SettleWrongSlot(ctx, f.pool, nil, f.groupID, pgtype.UUID{}, op.ID) + if err != nil { + t.Fatal(err) + } + if res.Kept != nil || len(res.Removed) != 2 || exists(f.aPath) || exists(f.bPath) { + t.Fatalf("result %+v, void exists %v, reach exists %v; want both copies removed", + res, exists(f.aPath), exists(f.bPath)) + } + for _, id := range []pgtype.UUID{f.a.ID, f.b.ID} { + if !emptiedTrack(t, f, id) { + t.Errorf("track %s is not waiting to be fetched again", syncpkg.FormatUUID(id)) + } + } + if !releaseBlocked(t, f.pool, "abc123") || len(res.Blocked) != 1 { + t.Errorf("blocked %v; want the download the files came from set aside", res.Blocked) + } + settledByHand(t, f, op) + + if _, err := SettleWrongSlot(ctx, f.pool, nil, f.groupID, pgtype.UUID{}, op.ID); !errors.Is(err, ErrDuplicateGroupNotPending) { + t.Errorf("settling it twice: err = %v, want ErrDuplicateGroupNotPending", err) + } +} + +// "This one is Reach": the copy the operator names stays, and only Void's slot +// is emptied and wanted again. +func TestSettleWrongSlot_KeepsTheCopyTheOperatorNames_Integration(t *testing.T) { + f, op := undecidedSlot(t) + res, err := SettleWrongSlot(context.Background(), f.pool, nil, f.groupID, f.b.ID, op.ID) + if err != nil { + t.Fatal(err) + } + if res.Kept == nil || res.Kept.TrackID != f.b.ID || len(res.Removed) != 1 || exists(f.aPath) || !exists(f.bPath) { + t.Fatalf("result %+v, void exists %v, reach exists %v; want only Void's copy removed", + res, exists(f.aPath), exists(f.bPath)) + } + if !emptiedTrack(t, f, f.a.ID) { + t.Error("Void's track is not waiting to be fetched again") + } + if !releaseBlocked(t, f.pool, "abc123") { + t.Error("the download the files came from is not set aside") + } + settledByHand(t, f, op) +} + +// Only a group of one recording in several slots is settled this way, and only +// with a copy from it. +func TestSettleWrongSlot_RefusesWhatItCannotSettle_Integration(t *testing.T) { + ctx := context.Background() + t.Run("two versions of a song", func(t *testing.T) { + f := newOneAlbumFixture(t, "acoustic", "Lovesick", "Lovesick (Instrumental)") + if _, err := SettleWrongSlot(ctx, f.pool, nil, f.groupID, pgtype.UUID{}, pgtype.UUID{}); !errors.Is(err, ErrNotWrongSlot) { + t.Errorf("err = %v, want ErrNotWrongSlot", err) + } + if !exists(f.aPath) || !exists(f.bPath) { + t.Error("a file was removed") + } + }) + t.Run("a copy from another group", func(t *testing.T) { + f, op := undecidedSlot(t) + other := pgtype.UUID{Bytes: [16]byte{1}, Valid: true} + if _, err := SettleWrongSlot(ctx, f.pool, nil, f.groupID, other, op.ID); !errors.Is(err, ErrSurvivorNotInGroup) { + t.Errorf("err = %v, want ErrSurvivorNotInGroup", err) + } + if !exists(f.aPath) || !exists(f.bPath) || releaseBlocked(t, f.pool, "abc123") { + t.Error("something changed") + } + }) +} diff --git a/web/src/lib/api/admin.ts b/web/src/lib/api/admin.ts index 317a25b2..8e71dfb1 100644 --- a/web/src/lib/api/admin.ts +++ b/web/src/lib/api/admin.ts @@ -9,6 +9,7 @@ import type { AdminDuplicatesResponse, DuplicatesView, MergeDuplicateResult, + RefetchDuplicateResult, ResolvedDuplicatesResponse, AdminPlaybackError, AdminQuarantineRow, @@ -893,6 +894,18 @@ export async function mergeDuplicateGroup( ); } +// One recording in several of an album's slots (#6184): keep the copy whose +// title it is, or none (empty keep_track_id), and fetch the album again. +export async function refetchDuplicateGroup( + id: string, + body: { keep_track_id: string } +): Promise<RefetchDuplicateResult> { + return api.post<RefetchDuplicateResult>( + `/api/admin/library/duplicates/${encodeURIComponent(id)}/refetch`, + body + ); +} + export async function dismissDuplicateGroup(id: string): Promise<void> { await api.post(`/api/admin/library/duplicates/${encodeURIComponent(id)}/dismiss`, {}); } diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index 5b8848e8..48dbd69e 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -485,6 +485,15 @@ export type MergeDuplicateResult = { removed_paths: string[]; }; +// What settling one recording in several slots did (#6184). kept_track_id is +// null when every copy went; blocked names the releases set aside, empty when +// the album came from before Minstrel downloaded anything. +export type RefetchDuplicateResult = { + kept_track_id: string | null; + removed_paths: string[]; + blocked: string[]; +}; + export type AdminDuplicatesResponse = { sweep: AdminDuplicateSweep; fingerprints: { diff --git a/web/src/routes/collection/duplicates/+page.svelte b/web/src/routes/collection/duplicates/+page.svelte index 63ffd1c3..9510eba8 100644 --- a/web/src/routes/collection/duplicates/+page.svelte +++ b/web/src/routes/collection/duplicates/+page.svelte @@ -9,7 +9,8 @@ createResolvedDuplicatesQuery, runDuplicateSweep, dismissDuplicateGroup, - mergeDuplicateGroup + mergeDuplicateGroup, + refetchDuplicateGroup } from '#lib/api/admin.js'; import { errMessage } from '#lib/api/errors.js'; import { pushToast } from '#lib/stores/toast.svelte.js'; @@ -38,6 +39,14 @@ // 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. + // + // Except for one recording filling several of an album's slots (class + // mismatch, #6184), where a merge is the wrong answer: it keeps one copy's + // title whether or not it is the audio's, and fetches nothing. There the + // operator says which song the recording is, or that they can't tell, and the + // group settles as the resolver would have: copies in other songs' slots are + // removed, the release they came from is set aside, and the album is fetched + // again. Operator, 2026-10-21: "for some of these I don't have any idea". type Tab = DuplicatesView | 'resolved'; const fromUrl = page.url.searchParams.get('view'); @@ -53,6 +62,11 @@ let keepChoice = $state<Record<string, string>>({}); let confirmingMerge = $state<string | null>(null); let merging = $state<string | null>(null); + // Per wrong-slot group: the copy whose title the recording is, absent when + // the operator can't tell, and the same two-click confirm. + let slotPick = $state<Record<string, string>>({}); + let confirmingRefetch = $state<string | null>(null); + let refetching = $state<string | null>(null); const queryStore = $derived(createDuplicatesQuery(groupView)); const query = $derived($queryStore); @@ -126,6 +140,51 @@ } } + function wrongSlot(g: AdminDuplicateGroup): boolean { + return g.class === 'mismatch'; + } + + function pickedOf(g: AdminDuplicateGroup): AdminDuplicateMember | undefined { + return g.members.find((m) => m.track_id === slotPick[g.id]); + } + + // Close to the server's MatchTitleKey: case, "&" and punctuation aside. + function songKey(title: string): string { + return title.toLowerCase().replace(/&/g, 'and').replace(/[^\p{L}\p{N}]/gu, ''); + } + + // The copies that go: every one when the operator can't tell, else those + // under another title. A copy under the picked title is the song held twice, + // and the next sweep merges it. + function refetchRemoves(g: AdminDuplicateGroup): AdminDuplicateMember[] { + const picked = pickedOf(g); + if (!picked) return g.members; + return g.members.filter((m) => songKey(m.title) !== songKey(picked.title)); + } + + async function onRefetch(group: AdminDuplicateGroup) { + if (confirmingRefetch !== group.id) { + confirmingRefetch = group.id; + return; + } + confirmingRefetch = null; + refetching = group.id; + try { + const result = await refetchDuplicateGroup(group.id, { keep_track_id: slotPick[group.id] ?? '' }); + const removed = `Removed ${plural(result.removed_paths.length, 'file', 'files')}`; + pushToast( + result.blocked.length > 0 + ? `${removed} and set aside the release they came from. The album will be fetched again.` + : `${removed}. The album will be fetched again, but Minstrel didn't download these files, so the same ones may come back.` + ); + query.refetch(); + } catch (e: unknown) { + pushToast(errMessage(e), 'error'); + } finally { + refetching = null; + } + } + // "Identical audio" and "same recording" are different claims, and an // operator deciding whether to merge needs to know which one they are // looking at before anything else. @@ -377,27 +436,49 @@ <button type="button" onclick={() => onDismiss(group)} - disabled={dismissing === group.id || merging === group.id} + disabled={dismissing === group.id || merging === group.id || refetching === group.id} class="rounded-md border border-border px-3 py-1.5 text-sm text-text-secondary hover:bg-surface-hover hover:text-text-primary disabled:opacity-50" > {tab === 'cross_release' ? 'Not the same song' : 'Not duplicates'} </button> - <button - type="button" - onclick={() => onMerge(group)} - disabled={merging === group.id} - class="rounded-md px-3 py-1.5 text-sm disabled:opacity-50 {confirmingMerge === group.id - ? 'bg-action-destructive text-action-fg hover:opacity-90' - : 'border border-border text-text-primary hover:bg-surface-hover'}" - > - {#if merging === group.id} - Merging… - {:else if confirmingMerge === group.id} - Remove {plural(group.members.length - 1, 'file', 'files')} and merge - {:else} - Merge… - {/if} - </button> + {#if wrongSlot(group)} + {@const picked = pickedOf(group)} + <button + type="button" + onclick={() => onRefetch(group)} + disabled={refetching === group.id} + class="rounded-md px-3 py-1.5 text-sm disabled:opacity-50 {confirmingRefetch === group.id + ? 'bg-action-destructive text-action-fg hover:opacity-90' + : 'border border-border text-text-primary hover:bg-surface-hover'}" + > + {#if refetching === group.id} + Removing… + {:else if confirmingRefetch === group.id} + Remove {plural(refetchRemoves(group).length, 'file', 'files')} and get it again + {:else if picked} + Keep “{picked.title}”… + {:else} + Get it again… + {/if} + </button> + {:else} + <button + type="button" + onclick={() => onMerge(group)} + disabled={merging === group.id} + class="rounded-md px-3 py-1.5 text-sm disabled:opacity-50 {confirmingMerge === group.id + ? 'bg-action-destructive text-action-fg hover:opacity-90' + : 'border border-border text-text-primary hover:bg-surface-hover'}" + > + {#if merging === group.id} + Merging… + {:else if confirmingMerge === group.id} + Remove {plural(group.members.length - 1, 'file', 'files')} and merge + {:else} + Merge… + {/if} + </button> + {/if} </div> </div> @@ -424,12 +505,40 @@ <span> {group.tier === 'exact' ? 'The audio is identical' : 'The music is the same'}, but the copies are titled {quotedList(titles)}. If those name different recordings, one file is - mislabelled and the other recording isn't in your library. Play each and keep the copy - whose title matches what you hear. + mislabelled and the other recording isn't in your library. + {#if wrongSlot(group)} + Play them. If you know which song this is, pick the copy with that title. If you can't + tell, get the album again: every copy goes, and it's fetched from another release. + {:else} + Play each and keep the copy whose title matches what you hear. + {/if} </span> </p> {/if} + {#if confirmingRefetch === group.id} + {@const picked = pickedOf(group)} + <div class="space-y-2 border-b border-border bg-surface-hover px-4 py-3 text-sm" data-testid="refetch-confirm"> + <p class="text-text-primary"> + {#if picked} + The copy titled “{picked.title}” stays, and {plural(refetchRemoves(group).length, 'copy', 'copies')} + under other titles will be deleted from disk. + {:else} + All {plural(group.members.length, 'copy', 'copies')} will be deleted from disk. + {/if} + Their tracks stay, marked missing, with their likes and plays. The release the files came + from is set aside, and the album is fetched again from another one. + </p> + <button + type="button" + class="text-xs text-text-secondary underline hover:text-text-primary" + onclick={() => (confirmingRefetch = null)} + > + Cancel + </button> + </div> + {/if} + {#if confirmingMerge === group.id} <!-- What the second click will do, said plainly before it is done. --> <div class="space-y-2 border-b border-border bg-surface-hover px-4 py-3 text-sm" data-testid="merge-confirm"> @@ -449,20 +558,38 @@ <ul class="divide-y divide-border"> {#each group.members as m (m.track_id)} - {@const keep = keeperOf(group) === m.track_id} + {@const keep = !wrongSlot(group) && keeperOf(group) === m.track_id} {@const proposed = m.track_id === group.survivor_track_id} <li class="flex items-start gap-3 px-4 py-3" data-testid="duplicate-member"> - <input - type="radio" - name="keep-{group.id}" - class="mt-1" - checked={keeperOf(group) === m.track_id} - onchange={() => (keepChoice[group.id] = m.track_id)} - aria-label="Keep {m.title}, {m.file_format.toUpperCase()}, {m.file_path}" - /> + {#if wrongSlot(group)} + <!-- No copy is proposed: which song the recording is, is the + question, and nothing here can answer it. --> + <input + type="radio" + name="song-{group.id}" + class="mt-1" + checked={slotPick[group.id] === m.track_id} + onchange={() => (slotPick[group.id] = m.track_id)} + aria-label="This is {m.title}, {m.file_path}" + /> + {:else} + <input + type="radio" + name="keep-{group.id}" + class="mt-1" + checked={keeperOf(group) === m.track_id} + onchange={() => (keepChoice[group.id] = m.track_id)} + aria-label="Keep {m.title}, {m.file_format.toUpperCase()}, {m.file_path}" + /> + {/if} <div class="min-w-0 flex-1 space-y-0.5"> <div class="flex flex-wrap items-center gap-2"> <span class="truncate text-sm text-text-primary">{m.title}</span> + {#if wrongSlot(group) && slotPick[group.id] === m.track_id} + <span class="shrink-0 rounded-full bg-accent-tint px-2 py-0.5 text-xs text-accent-fg" data-testid="song-badge"> + The recording is this song + </span> + {/if} {#if keep} <!-- The copy the merge will keep: the proposed one with the rule that chose it, or the one the operator picked. The @@ -499,6 +626,15 @@ </li> {/each} </ul> + {#if wrongSlot(group) && pickedOf(group)} + <button + type="button" + class="w-full border-t border-border px-4 py-2 text-left text-xs text-text-secondary underline hover:text-text-primary" + onclick={() => delete slotPick[group.id]} + > + I can't tell which song it is + </button> + {/if} </li> {/each} </ul> diff --git a/web/src/routes/collection/duplicates/duplicates.test.ts b/web/src/routes/collection/duplicates/duplicates.test.ts index ac4a4bfb..fb361bdf 100644 --- a/web/src/routes/collection/duplicates/duplicates.test.ts +++ b/web/src/routes/collection/duplicates/duplicates.test.ts @@ -15,6 +15,11 @@ vi.mock('#lib/api/admin.js', () => ({ mergeDuplicateGroup: vi.fn().mockResolvedValue({ survivor_track_id: 'www-01', removed_paths: ['/music/Moe Shop/WWW (2020)/www-02.mp3'] + }), + refetchDuplicateGroup: vi.fn().mockResolvedValue({ + kept_track_id: null, + removed_paths: ['/music/Moe Shop/WWW (2020)/www-01.mp3', '/music/Moe Shop/WWW (2020)/www-02.mp3'], + blocked: ['Moe Shop - WWW [WEB]'] }) })); vi.mock('#lib/player/store.svelte.js', () => ({ playQueue: vi.fn() })); @@ -26,6 +31,7 @@ import { createResolvedDuplicatesQuery, dismissDuplicateGroup, mergeDuplicateGroup, + refetchDuplicateGroup, runDuplicateSweep } from '#lib/api/admin.js'; @@ -256,7 +262,64 @@ describe('admin duplicates', () => { renderWith(r); const notice = text(screen.getByTestId('title-mismatch')); expect(notice).toContain('The music is the same, but the copies are titled “WWW” and “WWW (instrumental)”'); - expect(notice).toContain('Play each'); + expect(notice).toContain("If you can't tell, get the album again"); + }); + }); + + // One recording in several of an album's slots (#6184). A merge would keep + // one title whether or not it is the audio's and fetch nothing, so it is not + // offered: the operator says which song it is, or that they can't tell. + describe('one recording in several slots', () => { + function slots() { + const r = response(); + r.groups[0] = { + ...r.groups[0], + tier: 'acoustic', + worst_bit_error_rate: 0.04, + class: 'mismatch', + members: [member('www-01'), member('www-02', { title: 'WWW (instrumental)' })] + }; + return r; + } + + test('offers Get it again instead of Merge, with no copy proposed', () => { + renderWith(slots()); + expect(screen.queryByRole('button', { name: 'Merge…' })).toBeNull(); + expect(screen.getByRole('button', { name: 'Get it again…' })).toBeTruthy(); + expect(screen.getAllByRole('radio').some((r) => (r as HTMLInputElement).checked)).toBe(false); + expect(screen.queryByTestId('survivor-badge')).toBeNull(); + }); + + test("can't tell: every copy goes, after a second click", async () => { + renderWith(slots()); + await fireEvent.click(screen.getByRole('button', { name: 'Get it again…' })); + expect(refetchDuplicateGroup).not.toHaveBeenCalled(); + const confirm = text(screen.getByTestId('refetch-confirm')); + expect(confirm).toContain('All 2 copies will be deleted from disk'); + expect(confirm).toContain('the album is fetched again from another one'); + + await fireEvent.click(screen.getByRole('button', { name: 'Remove 2 files and get it again' })); + expect(refetchDuplicateGroup).toHaveBeenCalledWith('g-1', { keep_track_id: '' }); + }); + + test('naming the song keeps that copy and removes the others', async () => { + renderWith(slots()); + await fireEvent.click(screen.getByRole('radio', { name: /This is WWW \(instrumental\)/ })); + expect(screen.getByTestId('song-badge')).toBeTruthy(); + await fireEvent.click(screen.getByRole('button', { name: 'Keep “WWW (instrumental)”…' })); + expect(text(screen.getByTestId('refetch-confirm'))).toContain( + 'The copy titled “WWW (instrumental)” stays, and 1 copy under other titles will be deleted from disk' + ); + await fireEvent.click(screen.getByRole('button', { name: 'Remove 1 file and get it again' })); + expect(refetchDuplicateGroup).toHaveBeenCalledWith('g-1', { keep_track_id: 'www-02' }); + }); + + test("I can't tell takes the pick back", async () => { + renderWith(slots()); + await fireEvent.click(screen.getAllByRole('radio')[0]); + await fireEvent.click(screen.getByRole('button', { name: "I can't tell which song it is" })); + expect(screen.getByRole('button', { name: 'Get it again…' })).toBeTruthy(); + expect(screen.queryByTestId('song-badge')).toBeNull(); }); });