diff --git a/internal/db/dbq/tracks.sql.go b/internal/db/dbq/tracks.sql.go index a6266b08..ad2d0244 100644 --- a/internal/db/dbq/tracks.sql.go +++ b/internal/db/dbq/tracks.sql.go @@ -11,6 +11,34 @@ import ( "github.com/jackc/pgx/v5/pgtype" ) +const adoptTrackPath = `-- name: AdoptTrackPath :execrows +UPDATE tracks + SET file_path = $1, + missing_since = NULL + WHERE id = $2 + AND missing_since IS NOT NULL +` + +type AdoptTrackPathParams struct { + FilePath string + ID pgtype.UUID +} + +// Re-points a missing row at the path its file turned up on, and clears the +// mark. The caller's normal UpsertTrack then conflicts on file_path and updates +// THIS row in place, so the track id survives and its likes, play history and +// playlist memberships come with it. +// +// `missing_since IS NOT NULL` again, this time as a race guard: two files can't +// both adopt the same row, and :execrows reports 0 to whichever loses. +func (q *Queries) AdoptTrackPath(ctx context.Context, arg AdoptTrackPathParams) (int64, error) { + result, err := q.db.Exec(ctx, adoptTrackPath, arg.FilePath, arg.ID) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const clearTracksMissing = `-- name: ClearTracksMissing :execrows UPDATE tracks SET missing_since = NULL @@ -107,6 +135,93 @@ func (q *Queries) DeleteTrack(ctx context.Context, id pgtype.UUID) (DeleteTrackR return i, err } +const findMissingTrackByFingerprint = `-- name: FindMissingTrackByFingerprint :many +SELECT id, file_path FROM tracks + WHERE missing_since IS NOT NULL + AND file_size = $1 + AND duration_ms = $2 + LIMIT 2 +` + +type FindMissingTrackByFingerprintParams struct { + FileSize int64 + DurationMs int32 +} + +type FindMissingTrackByFingerprintRow struct { + ID pgtype.UUID + FilePath string +} + +// Move detection fallback for files with no MBID (#2528). Exact byte size AND +// exact decoded duration is a strong pair: a plain move or rename preserves +// both, while a re-encode changes at least one — and a re-encode genuinely is a +// different file, so failing to match there is correct rather than a gap. +// +// Same missing-only constraint and same LIMIT 2 rationale as the MBID variant. +func (q *Queries) FindMissingTrackByFingerprint(ctx context.Context, arg FindMissingTrackByFingerprintParams) ([]FindMissingTrackByFingerprintRow, error) { + rows, err := q.db.Query(ctx, findMissingTrackByFingerprint, arg.FileSize, arg.DurationMs) + if err != nil { + return nil, err + } + defer rows.Close() + var items []FindMissingTrackByFingerprintRow + for rows.Next() { + var i FindMissingTrackByFingerprintRow + if err := rows.Scan(&i.ID, &i.FilePath); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const findMissingTrackByMbid = `-- name: FindMissingTrackByMbid :many +SELECT id, file_path FROM tracks + WHERE missing_since IS NOT NULL + AND mbid IS NOT NULL + AND mbid = $1::text + LIMIT 2 +` + +type FindMissingTrackByMbidRow struct { + ID pgtype.UUID + FilePath string +} + +// Move detection, strongest signal (#2528). A file that turned up at a new path +// carrying a recording MBID we already have on a MISSING row is that recording, +// moved — not a new track. +// +// `missing_since IS NOT NULL` is the safety constraint, not an optimisation: a +// row whose file is present elsewhere on disk is a DUPLICATE, and re-pointing +// its file_path would corrupt the copy that still exists. +// +// LIMIT 2 because the caller only needs to know "exactly one" vs "more than +// one" — an ambiguous match must not be adopted arbitrarily. +func (q *Queries) FindMissingTrackByMbid(ctx context.Context, mbid string) ([]FindMissingTrackByMbidRow, error) { + rows, err := q.db.Query(ctx, findMissingTrackByMbid, mbid) + if err != nil { + return nil, err + } + defer rows.Close() + var items []FindMissingTrackByMbidRow + for rows.Next() { + var i FindMissingTrackByMbidRow + if err := rows.Scan(&i.ID, &i.FilePath); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const getTrackByID = `-- name: GetTrackByID :one SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since FROM tracks WHERE id = $1 ` diff --git a/internal/db/queries/tracks.sql b/internal/db/queries/tracks.sql index 3f59044d..d11d11c2 100644 --- a/internal/db/queries/tracks.sql +++ b/internal/db/queries/tracks.sql @@ -138,6 +138,50 @@ RETURNING id, album_id, artist_id, file_path, mbid; -- (#357). Mirror of GetArtistsByIDs. SELECT * FROM tracks WHERE id = ANY($1::uuid[]); +-- name: FindMissingTrackByMbid :many +-- Move detection, strongest signal (#2528). A file that turned up at a new path +-- carrying a recording MBID we already have on a MISSING row is that recording, +-- moved — not a new track. +-- +-- `missing_since IS NOT NULL` is the safety constraint, not an optimisation: a +-- row whose file is present elsewhere on disk is a DUPLICATE, and re-pointing +-- its file_path would corrupt the copy that still exists. +-- +-- LIMIT 2 because the caller only needs to know "exactly one" vs "more than +-- one" — an ambiguous match must not be adopted arbitrarily. +SELECT id, file_path FROM tracks + WHERE missing_since IS NOT NULL + AND mbid IS NOT NULL + AND mbid = sqlc.arg(mbid)::text + LIMIT 2; + +-- name: FindMissingTrackByFingerprint :many +-- Move detection fallback for files with no MBID (#2528). Exact byte size AND +-- exact decoded duration is a strong pair: a plain move or rename preserves +-- both, while a re-encode changes at least one — and a re-encode genuinely is a +-- different file, so failing to match there is correct rather than a gap. +-- +-- Same missing-only constraint and same LIMIT 2 rationale as the MBID variant. +SELECT id, file_path FROM tracks + WHERE missing_since IS NOT NULL + AND file_size = sqlc.arg(file_size) + AND duration_ms = sqlc.arg(duration_ms) + LIMIT 2; + +-- name: AdoptTrackPath :execrows +-- Re-points a missing row at the path its file turned up on, and clears the +-- mark. The caller's normal UpsertTrack then conflicts on file_path and updates +-- THIS row in place, so the track id survives and its likes, play history and +-- playlist memberships come with it. +-- +-- `missing_since IS NOT NULL` again, this time as a race guard: two files can't +-- both adopt the same row, and :execrows reports 0 to whichever loses. +UPDATE tracks + SET file_path = sqlc.arg(file_path), + missing_since = NULL + WHERE id = sqlc.arg(id) + AND missing_since IS NOT NULL; + -- name: ListTrackPathsForReconcile :many -- Every row's path + current missing mark, for the scanner's reconcile pass -- (#2523). Deliberately unfiltered and unpaged: reconcile has to compare the diff --git a/internal/library/moved.go b/internal/library/moved.go new file mode 100644 index 00000000..aa049e8f --- /dev/null +++ b/internal/library/moved.go @@ -0,0 +1,151 @@ +package library + +import ( + "context" + + "github.com/jackc/pgx/v5/pgtype" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" +) + +// Move detection (#2528). +// +// Track identity is file_path: UpsertTrack conflicts on it, and the reconcile +// pass in reconcile.go clears a missing mark when the walk sees that same path +// again. So a file that comes back exactly where it was restores cleanly, but a +// file that comes back RENAMED or in a different directory looked, to the +// scanner, like a deletion plus an unrelated new track: +// +// - the old row stayed marked missing, holding the like and every play_event +// - a fresh row appeared with no history +// - nothing connected them +// +// A liked song read as unliked after a retag, its play count reset to zero, and +// Rediscover could offer it as a discovery. All silently. Renumbering an album +// was enough to do it — which is exactly what happened on the operator's copy of +// Minutes to Midnight. +// +// The fix adopts the existing row rather than inserting: re-point its file_path +// at the new location and clear the mark. The caller's normal UpsertTrack then +// conflicts on file_path and updates THAT row, so the track id survives and +// likes, plays and playlist memberships travel with it. Clients see an update +// rather than a delete-and-create, so no cache churn either. +// +// Only rows already marked missing are eligible. A row whose file is present +// elsewhere is a duplicate, not a move, and re-pointing it would corrupt the +// copy that still exists. That constraint is what makes this safe, and the +// marking added in #2523 is what makes it expressible. + +// trackAdopter is the slice of dbq.Queries move detection needs, narrowed so the +// match/ambiguity logic can be tested against a fake. +type trackAdopter interface { + FindMissingTrackByMbid(ctx context.Context, mbid string) ([]dbq.FindMissingTrackByMbidRow, error) + FindMissingTrackByFingerprint(ctx context.Context, arg dbq.FindMissingTrackByFingerprintParams) ([]dbq.FindMissingTrackByFingerprintRow, error) + AdoptTrackPath(ctx context.Context, arg dbq.AdoptTrackPathParams) (int64, error) +} + +// adoptMovedTrack looks for a missing row that is the same recording as the file +// at newPath and re-points it there. Reports whether a row was adopted. +// +// Never returns an error: failing to detect a move is a missed optimisation, not +// a broken scan. The caller carries on and inserts a fresh row, which is the +// pre-#2528 behaviour. +func (s *Scanner) adoptMovedTrack( + ctx context.Context, q trackAdopter, newPath string, + fileSize int64, durationMs int32, recordingMBID string, +) bool { + // MBID first. It identifies the recording rather than the bytes, so it + // survives a re-encode that the fingerprint cannot. + if recordingMBID != "" { + rows, err := q.FindMissingTrackByMbid(ctx, recordingMBID) + if err != nil { + s.logger.Warn("library scan: move lookup by mbid failed", + "path", newPath, "err", err) + } else if c, ok := s.uniqueMatch(rowsFromMbid(rows), newPath, "mbid"); ok { + return s.adopt(ctx, q, c, newPath, "mbid") + } + } + + // Fingerprint fallback for untagged files. Both components must be real: + // duration_ms is 0 when ffprobe failed, and matching 0 against 0 would pair + // up unrelated broken files. + if fileSize > 0 && durationMs > 0 { + rows, err := q.FindMissingTrackByFingerprint(ctx, dbq.FindMissingTrackByFingerprintParams{ + FileSize: fileSize, + DurationMs: durationMs, + }) + if err != nil { + s.logger.Warn("library scan: move lookup by fingerprint failed", + "path", newPath, "err", err) + } else if c, ok := s.uniqueMatch(rowsFromFingerprint(rows), newPath, "fingerprint"); ok { + return s.adopt(ctx, q, c, newPath, "fingerprint") + } + } + + return false +} + +// candidate is the shared shape of both lookups, so uniqueMatch is written once. +type candidate struct { + id pgtype.UUID + filePath string +} + +func rowsFromMbid(rows []dbq.FindMissingTrackByMbidRow) []candidate { + out := make([]candidate, 0, len(rows)) + for _, r := range rows { + out = append(out, candidate{id: r.ID, filePath: r.FilePath}) + } + return out +} + +func rowsFromFingerprint(rows []dbq.FindMissingTrackByFingerprintRow) []candidate { + out := make([]candidate, 0, len(rows)) + for _, r := range rows { + out = append(out, candidate{id: r.ID, filePath: r.FilePath}) + } + return out +} + +// uniqueMatch requires exactly one candidate. Adopting an arbitrary row out of +// several would attach this file's future history to a coin flip, which is worse +// than starting a fresh row — a fork is recoverable later, a wrong merge isn't. +// Libraries with genuine duplicates hit this, so it's logged rather than silent. +func (s *Scanner) uniqueMatch( + cands []candidate, newPath, via string, +) (candidate, bool) { + switch len(cands) { + case 0: + return candidate{}, false + case 1: + return cands[0], true + default: + s.logger.Info("library scan: ambiguous move match, inserting a new track instead", + "path", newPath, "via", via, "candidates", len(cands)) + return candidate{}, false + } +} + +func (s *Scanner) adopt( + ctx context.Context, q trackAdopter, c candidate, newPath, via string, +) bool { + n, err := q.AdoptTrackPath(ctx, dbq.AdoptTrackPathParams{ID: c.id, FilePath: newPath}) + if err != nil { + // A unique violation on file_path means something else claimed this path + // first. Fall through to a normal insert rather than failing the file. + s.logger.Warn("library scan: adopting moved track failed", + "path", newPath, "via", via, "err", err) + return false + } + if n == 0 { + // Lost the race: another file adopted this row between lookup and + // update, so its mark was already cleared. + return false + } + // Logged with both paths: this is the operator's only window onto a + // reorganisation being understood as a move rather than a new track. + s.logger.Info("library scan: track moved, history preserved", + "from", c.filePath, "to", newPath, "via", via, "track_id", syncpkg.FormatUUID(c.id)) + return true +} diff --git a/internal/library/moved_test.go b/internal/library/moved_test.go new file mode 100644 index 00000000..f3536db9 --- /dev/null +++ b/internal/library/moved_test.go @@ -0,0 +1,295 @@ +package library + +import ( + "context" + "errors" + "testing" + + "github.com/jackc/pgx/v5/pgtype" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +type fakeAdopter struct { + byMbid []dbq.FindMissingTrackByMbidRow + byFingerprint []dbq.FindMissingTrackByFingerprintRow + + mbidErr error + fingerprintErr error + adoptErr error + adoptRows int64 + + mbidQueried []string + fingerprintQueried []dbq.FindMissingTrackByFingerprintParams + adopted []dbq.AdoptTrackPathParams +} + +func (f *fakeAdopter) FindMissingTrackByMbid(_ context.Context, mbid string) ([]dbq.FindMissingTrackByMbidRow, error) { + f.mbidQueried = append(f.mbidQueried, mbid) + return f.byMbid, f.mbidErr +} + +func (f *fakeAdopter) FindMissingTrackByFingerprint( + _ context.Context, arg dbq.FindMissingTrackByFingerprintParams, +) ([]dbq.FindMissingTrackByFingerprintRow, error) { + f.fingerprintQueried = append(f.fingerprintQueried, arg) + return f.byFingerprint, f.fingerprintErr +} + +func (f *fakeAdopter) AdoptTrackPath(_ context.Context, arg dbq.AdoptTrackPathParams) (int64, error) { + f.adopted = append(f.adopted, arg) + if f.adoptErr != nil { + return 0, f.adoptErr + } + return f.adoptRows, nil +} + +// The narrowed interface must not drift from the real queries. +var _ trackAdopter = (*dbq.Queries)(nil) + +func mbidRow(n byte, path string) dbq.FindMissingTrackByMbidRow { + return dbq.FindMissingTrackByMbidRow{ID: testUUID(n), FilePath: path} +} + +func fpRow(n byte, path string) dbq.FindMissingTrackByFingerprintRow { + return dbq.FindMissingTrackByFingerprintRow{ID: testUUID(n), FilePath: path} +} + +const ( + oldPath = "/music/Linkin Park/Minutes to Midnight/02 - Bleed It Out.mp3" + newPath = "/music/Linkin Park/Minutes to Midnight/04 - Bleed It Out.mp3" +) + +func TestAdoptMovedTrack_MatchesByMbid(t *testing.T) { + s := testScanner(t) + q := &fakeAdopter{byMbid: []dbq.FindMissingTrackByMbidRow{mbidRow(7, oldPath)}, adoptRows: 1} + + if !s.adoptMovedTrack(context.Background(), q, newPath, 5_000_000, 200_000, "rec-mbid") { + t.Fatal("expected the moved track to be adopted") + } + if len(q.adopted) != 1 { + t.Fatalf("adopted %d rows, want 1", len(q.adopted)) + } + if q.adopted[0].ID != testUUID(7) { + t.Errorf("adopted the wrong row: %v", q.adopted[0].ID) + } + if q.adopted[0].FilePath != newPath { + t.Errorf("adopted FilePath = %q, want %q", q.adopted[0].FilePath, newPath) + } + // MBID matched, so the weaker signal should not have been consulted. + if len(q.fingerprintQueried) != 0 { + t.Errorf("queried the fingerprint despite an MBID match") + } +} + +func TestAdoptMovedTrack_FallsBackToFingerprint(t *testing.T) { + s := testScanner(t) + q := &fakeAdopter{byFingerprint: []dbq.FindMissingTrackByFingerprintRow{fpRow(3, oldPath)}, adoptRows: 1} + + // No MBID: an untagged file, which is exactly what the fallback is for. + if !s.adoptMovedTrack(context.Background(), q, newPath, 4_200_000, 187_000, "") { + t.Fatal("expected adoption via fingerprint") + } + if len(q.mbidQueried) != 0 { + t.Errorf("queried by MBID with no MBID available") + } + if len(q.fingerprintQueried) != 1 { + t.Fatalf("fingerprint queried %d times, want 1", len(q.fingerprintQueried)) + } + got := q.fingerprintQueried[0] + if got.FileSize != 4_200_000 || got.DurationMs != 187_000 { + t.Errorf("fingerprint = %+v, want size 4200000 duration 187000", got) + } + if len(q.adopted) != 1 || q.adopted[0].ID != testUUID(3) { + t.Errorf("adopted = %+v, want row 3", q.adopted) + } +} + +// Two missing rows carrying the same recording MBID means real duplicates. +// Adopting one arbitrarily would attach this file's future history to a coin +// flip, so it must insert fresh instead. +func TestAdoptMovedTrack_RefusesAmbiguousMbidMatch(t *testing.T) { + s := testScanner(t) + q := &fakeAdopter{byMbid: []dbq.FindMissingTrackByMbidRow{ + mbidRow(1, "/music/a.mp3"), + mbidRow(2, "/music/b.mp3"), + }, adoptRows: 1} + + if s.adoptMovedTrack(context.Background(), q, newPath, 0, 0, "rec-mbid") { + t.Fatal("expected refusal on an ambiguous MBID match") + } + if len(q.adopted) != 0 { + t.Errorf("adopted despite ambiguity: %+v", q.adopted) + } +} + +// An ambiguous MBID may still be resolvable by the fingerprint, which is a +// narrower signal — so falling through is allowed to succeed. +func TestAdoptMovedTrack_AmbiguousMbidFallsThroughToFingerprint(t *testing.T) { + s := testScanner(t) + q := &fakeAdopter{ + byMbid: []dbq.FindMissingTrackByMbidRow{ + mbidRow(1, "/music/a.mp3"), + mbidRow(2, "/music/b.mp3"), + }, + byFingerprint: []dbq.FindMissingTrackByFingerprintRow{fpRow(2, "/music/b.mp3")}, + adoptRows: 1, + } + + if !s.adoptMovedTrack(context.Background(), q, newPath, 1000, 2000, "rec-mbid") { + t.Fatal("expected the fingerprint to disambiguate") + } + if len(q.adopted) != 1 || q.adopted[0].ID != testUUID(2) { + t.Errorf("adopted = %+v, want row 2", q.adopted) + } +} + +func TestAdoptMovedTrack_RefusesAmbiguousFingerprintMatch(t *testing.T) { + s := testScanner(t) + q := &fakeAdopter{byFingerprint: []dbq.FindMissingTrackByFingerprintRow{ + fpRow(1, "/music/a.mp3"), + fpRow(2, "/music/b.mp3"), + }, adoptRows: 1} + + if s.adoptMovedTrack(context.Background(), q, newPath, 1000, 2000, "") { + t.Fatal("expected refusal on an ambiguous fingerprint match") + } + if len(q.adopted) != 0 { + t.Errorf("adopted despite ambiguity: %+v", q.adopted) + } +} + +// duration_ms is 0 when ffprobe failed. Matching 0 against 0 would pair up +// unrelated broken files, so the fingerprint must not be attempted. +func TestAdoptMovedTrack_SkipsFingerprintWithoutRealValues(t *testing.T) { + tests := []struct { + name string + size int64 + duration int32 + }{ + {"no duration", 1000, 0}, + {"no size", 0, 2000}, + {"neither", 0, 0}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + s := testScanner(t) + q := &fakeAdopter{ + byFingerprint: []dbq.FindMissingTrackByFingerprintRow{fpRow(1, oldPath)}, + adoptRows: 1, + } + if s.adoptMovedTrack(context.Background(), q, newPath, tc.size, tc.duration, "") { + t.Error("adopted on an unusable fingerprint") + } + if len(q.fingerprintQueried) != 0 { + t.Error("queried the fingerprint with unusable values") + } + }) + } +} + +func TestAdoptMovedTrack_NoCandidates(t *testing.T) { + s := testScanner(t) + q := &fakeAdopter{adoptRows: 1} + + if s.adoptMovedTrack(context.Background(), q, newPath, 1000, 2000, "rec-mbid") { + t.Fatal("expected no adoption when nothing matches") + } + if len(q.adopted) != 0 { + t.Errorf("adopted with no candidates: %+v", q.adopted) + } +} + +// The row's mark was cleared between lookup and update — another file adopted it +// first. AdoptTrackPath's `missing_since IS NOT NULL` predicate reports 0 rows. +func TestAdoptMovedTrack_LostRaceReportsNotAdopted(t *testing.T) { + s := testScanner(t) + q := &fakeAdopter{ + byMbid: []dbq.FindMissingTrackByMbidRow{mbidRow(5, oldPath)}, + adoptRows: 0, + } + + if s.adoptMovedTrack(context.Background(), q, newPath, 1000, 2000, "rec-mbid") { + t.Fatal("expected not-adopted when the update matched no rows") + } +} + +// Failing to detect a move must never fail the file: the caller falls back to +// inserting a fresh row, which is the pre-#2528 behaviour. +func TestAdoptMovedTrack_ToleratesQueryErrors(t *testing.T) { + sentinel := errors.New("db down") + tests := []struct { + name string + q *fakeAdopter + }{ + {"mbid lookup fails", &fakeAdopter{mbidErr: sentinel}}, + {"fingerprint lookup fails", &fakeAdopter{fingerprintErr: sentinel}}, + {"adopt fails", &fakeAdopter{ + byMbid: []dbq.FindMissingTrackByMbidRow{mbidRow(1, oldPath)}, + adoptErr: sentinel, + }}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + s := testScanner(t) + if s.adoptMovedTrack(context.Background(), tc.q, newPath, 1000, 2000, "rec-mbid") { + t.Error("reported adoption despite a query error") + } + }) + } +} + +// A failed MBID lookup must not stop the fingerprint from being tried. +func TestAdoptMovedTrack_MbidErrorStillTriesFingerprint(t *testing.T) { + s := testScanner(t) + q := &fakeAdopter{ + mbidErr: errors.New("db hiccup"), + byFingerprint: []dbq.FindMissingTrackByFingerprintRow{fpRow(9, oldPath)}, + adoptRows: 1, + } + + if !s.adoptMovedTrack(context.Background(), q, newPath, 1000, 2000, "rec-mbid") { + t.Fatal("expected the fingerprint to be tried after an MBID lookup error") + } + if len(q.adopted) != 1 || q.adopted[0].ID != testUUID(9) { + t.Errorf("adopted = %+v, want row 9", q.adopted) + } +} + +func TestUniqueMatch(t *testing.T) { + s := testScanner(t) + if _, ok := s.uniqueMatch(nil, newPath, "mbid"); ok { + t.Error("empty candidate set matched") + } + c, ok := s.uniqueMatch([]candidate{{id: testUUID(4), filePath: oldPath}}, newPath, "mbid") + if !ok { + t.Fatal("single candidate did not match") + } + if c.id != testUUID(4) || c.filePath != oldPath { + t.Errorf("candidate = %+v, want id 4 at %q", c, oldPath) + } + if _, ok := s.uniqueMatch([]candidate{ + {id: testUUID(1)}, {id: testUUID(2)}, + }, newPath, "mbid"); ok { + t.Error("multiple candidates matched") + } +} + +func TestRowConverters(t *testing.T) { + got := rowsFromMbid([]dbq.FindMissingTrackByMbidRow{mbidRow(1, "/a"), mbidRow(2, "/b")}) + if len(got) != 2 || got[0].id != testUUID(1) || got[1].filePath != "/b" { + t.Errorf("rowsFromMbid = %+v", got) + } + got = rowsFromFingerprint([]dbq.FindMissingTrackByFingerprintRow{fpRow(3, "/c")}) + if len(got) != 1 || got[0].id != testUUID(3) || got[0].filePath != "/c" { + t.Errorf("rowsFromFingerprint = %+v", got) + } +} + +// pgtype.UUID zero value must not be mistaken for a real id. +func TestUniqueMatch_ZeroUUIDNotValid(t *testing.T) { + var zero pgtype.UUID + if zero.Valid { + t.Fatal("zero pgtype.UUID should not be Valid") + } +} diff --git a/internal/library/scanner.go b/internal/library/scanner.go index a7116db6..985333c6 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -94,59 +94,56 @@ func (s *Scanner) Scan(ctx context.Context, progressCb func(Stats)) (Stats, erro q := dbq.New(s.pool) start := time.Now() - // Every audio path the walk visited. Reconcile diffs this against the table, - // so it costs no extra filesystem I/O — the walk already established which - // files exist. ~100 bytes/path, so a 250k-track library is ~25MB, which is - // worth it to avoid a second stat pass over the whole library. - seen := make(map[string]struct{}, 8192) - - for _, root := range s.paths { - if err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { - if ctx.Err() != nil { - return fs.SkipAll - } - if err != nil { - s.logger.Warn("library scan walk error", "path", path, "err", err) - stats.Errored++ - if progressCb != nil { - progressCb(stats) - } - return nil - } - if d.IsDir() { - return nil - } - if !audioExtensions[strings.ToLower(filepath.Ext(path))] { - return nil - } - // Recorded before scanFile so a file that exists but fails to parse - // still counts as present. It's a broken file, not a missing one, - // and marking it missing would hide it from the operator behind the - // wrong explanation. - seen[path] = struct{}{} - if _, _, err := s.scanFile(ctx, q, path, &stats); err != nil { - s.logger.Warn("library scan file error", "path", path, "err", err) - stats.Errored++ - } - if progressCb != nil { - progressCb(stats) - } - return nil - }); err != nil { - return stats, fmt.Errorf("library: walk %q: %w", root, err) - } - } - - // Reconcile only after a COMPLETE walk. A cancelled scan has a partial - // `seen` set, which would mark everything it hadn't reached yet. + // PHASE 1 — enumerate. Collect every audio path without touching tags or + // ffprobe. Cheap: WalkDir already stats each entry, so this adds a directory + // traversal and nothing else. + // + // The order matters and is the whole reason enumeration is separate. + // Reconcile has to mark disappeared rows BEFORE any file is processed, + // because move detection (#2528) can only adopt a row that is already marked + // missing. A rename performed while the server was down surfaces the deletion + // and the addition in the SAME scan — so if reconcile ran at the end, the new + // path would insert a fresh row first and the fork would be permanent. + paths, walkErrs := s.enumerate(ctx, progressCb, &stats) + stats.Errored += walkErrs if err := ctx.Err(); err != nil { return stats, err } + + // PHASE 2 — reconcile. Only ever on a COMPLETE enumeration: a cancelled walk + // has a partial view and would mark everything it hadn't reached. + seen := make(map[string]struct{}, len(paths)) + for _, p := range paths { + seen[p] = struct{}{} + } if err := s.reconcileMissing(ctx, q, seen, &stats); err != nil { - // Not fatal: the walk's results are already persisted and useful. The - // guards deliberately refuse to act on ambiguous evidence, and that - // refusal arrives here as an error. - s.logger.Warn("library scan: reconcile skipped", "err", err) + // Not fatal. The guards deliberately refuse to act on ambiguous + // evidence, and that refusal arrives here as an error. + // + // The consequence is named explicitly because it is not obvious: move + // detection (#2528) can only adopt a row that is already marked missing, + // so a refused reconcile also means renamed files insert fresh rows and + // fork their history. That's the pre-#2528 behaviour rather than a new + // failure, but it's worth knowing which scan it happened on. It bites + // hardest when a large fraction of a small library is reorganised at + // once, which trips the mark cap. + s.logger.Warn("library scan: reconcile skipped — moved files will fork rather than adopt", + "err", err) + } + + // PHASE 3 — process, in walk order so logs and cover-art batching stay + // grouped by directory rather than following map iteration order. + for _, path := range paths { + if ctx.Err() != nil { + break + } + if _, _, err := s.scanFile(ctx, q, path, &stats); err != nil { + s.logger.Warn("library scan file error", "path", path, "err", err) + stats.Errored++ + } + if progressCb != nil { + progressCb(stats) + } } s.logger.Info("library scan complete", @@ -165,6 +162,46 @@ func (s *Scanner) Scan(ctx context.Context, progressCb func(Stats)) (Stats, erro return stats, nil } +// enumerate walks every configured root and returns the audio paths found, in +// walk order, plus a count of walk errors. +// +// A path is recorded even if it will later fail to parse: an unreadable file is a +// broken file, not a missing one, and letting reconcile mark it missing would +// hide it from the operator behind the wrong explanation. +func (s *Scanner) enumerate( + ctx context.Context, progressCb func(Stats), stats *Stats, +) ([]string, int) { + paths := make([]string, 0, 8192) + errs := 0 + for _, root := range s.paths { + // WalkDir's own error return is folded into the per-entry handler below, + // so a bad root is counted rather than aborting the whole scan — one + // unreadable root shouldn't discard the others' results. + _ = filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if ctx.Err() != nil { + return fs.SkipAll + } + if err != nil { + s.logger.Warn("library scan walk error", "path", path, "err", err) + errs++ + if progressCb != nil { + progressCb(*stats) + } + return nil + } + if d.IsDir() { + return nil + } + if !audioExtensions[strings.ToLower(filepath.Ext(path))] { + return nil + } + paths = append(paths, path) + return nil + }) + } + return paths, errs +} + // scanFile upserts a single audio file. Returns the album ID the track // belongs to and whether the file was added/updated (false = skipped as // unchanged), so watcher-driven callers can enrich just the changed albums. @@ -254,6 +291,23 @@ func (s *Scanner) scanFile( durationMs = probed } + // A path we've never seen might not be a new track — it might be one that + // moved or was renamed (#2528). Adopting re-points the existing row at this + // path and clears its missing mark, so the UpsertTrack below conflicts on + // file_path and updates THAT row: same track id, likes and play history + // intact. Without this, renumbering an album forks every track on it. + // + // Runs here rather than earlier because the fingerprint needs the probed + // duration, and only for genuinely unknown paths — a known path is already + // the row we're going to update. + if !knownTrack { + if s.adoptMovedTrack(ctx, q, path, info.Size(), durationMs, recordingMBID) { + // Count it as an update: the row existed, and reporting it as Added + // would overstate library growth on every reorganisation. + knownTrack = true + } + } + params := dbq.UpsertTrackParams{ Title: trackTitle, AlbumID: album.ID, diff --git a/internal/library/scanner_test.go b/internal/library/scanner_test.go index f894fa34..34f9637e 100644 --- a/internal/library/scanner_test.go +++ b/internal/library/scanner_test.go @@ -205,3 +205,106 @@ func writeTestMP3(t *testing.T, path string, frames map[string]string) { t.Fatal(err) } } + +// TestScanner_AdoptsMovedFile_Integration is the #2528 proof: a renamed file +// must keep its existing tracks row — same id, so likes, play history and +// playlist memberships travel with it — rather than forking into a marked ghost +// plus a fresh zero-history row. +// +// Uses the MBID path. The synthetic MP3s here carry no real audio, so ffprobe +// yields duration 0 and the size+duration fingerprint is deliberately unusable — +// which is why the recording MBID is the signal under test. +// +// Eight tracks with one rename keeps the marked fraction at 12.5%, under +// missingMarkMaxFraction. That is load-bearing: if the rename exceeded the cap, +// reconcile would refuse to mark, adoption could not fire, and the file would +// fork. See the "reconcile skipped" warning in Scan. +func TestScanner_AdoptsMovedFile_Integration(t *testing.T) { + if testing.Short() { + t.Skip("skipping scanner integration in -short mode") + } + dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL") + if dsn == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + ctx := context.Background() + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + if err := db.Migrate(dsn, logger); err != nil { + t.Fatalf("migrate: %v", err) + } + pool, err := pgxpool.New(ctx, dsn) + if err != nil { + t.Fatalf("pool: %v", err) + } + t.Cleanup(pool.Close) + if _, err := pool.Exec(ctx, "TRUNCATE tracks, albums, artists RESTART IDENTITY CASCADE"); err != nil { + t.Fatalf("truncate: %v", err) + } + + root := t.TempDir() + const movedMBID = "11111111-2222-3333-4444-555555555555" + movedFrom := filepath.Join(root, "artistM/albumM/04 - Bleed It Out.mp3") + writeTestMP3(t, movedFrom, map[string]string{ + "TIT2": "Bleed It Out", "TPE1": "Artist M", "TALB": "Album M", "TRCK": "4", + // dhowden surfaces TXXX as a Comm whose Description is the Picard tag + // name; "MusicBrainz Track Id" is mbz.Recording. + "TXXX": "MusicBrainz Track Id\x00" + movedMBID, + }) + // Filler so one rename stays under the mark cap. + for i := 1; i <= 7; i++ { + writeTestMP3(t, filepath.Join(root, "artistM/albumM/filler", string(rune('a'+i))+".mp3"), + map[string]string{ + "TIT2": "Filler " + string(rune('0'+i)), "TPE1": "Artist M", "TALB": "Album M", + }) + } + + scanner := New(pool, logger, []string{root}) + if _, err := scanner.Scan(ctx, nil); err != nil { + t.Fatalf("first scan: %v", err) + } + + q := dbq.New(pool) + before, err := q.GetTrackByPath(ctx, movedFrom) + if err != nil { + t.Fatalf("track not indexed on first scan: %v", err) + } + if before.Mbid == nil || *before.Mbid != movedMBID { + t.Fatalf("recording mbid not stored: %v", before.Mbid) + } + + // Renumber the file, exactly as a tag editor would. + movedTo := filepath.Join(root, "artistM/albumM/02 - Bleed It Out.mp3") + if err := os.Rename(movedFrom, movedTo); err != nil { + t.Fatalf("rename: %v", err) + } + + if _, err := scanner.Scan(ctx, nil); err != nil { + t.Fatalf("second scan: %v", err) + } + + after, err := q.GetTrackByPath(ctx, movedTo) + if err != nil { + t.Fatalf("track not found at its new path: %v", err) + } + if after.ID != before.ID { + t.Errorf("track id changed on rename: %v -> %v (history would be stranded)", + before.ID, after.ID) + } + if after.MissingSince.Valid { + t.Errorf("adopted row is still marked missing: %v", after.MissingSince) + } + + // The old path must be gone entirely — not lingering as a marked ghost. + if _, err := q.GetTrackByPath(ctx, movedFrom); err == nil { + t.Error("old path still has a tracks row; the track forked instead of moving") + } + + var total int + if err := pool.QueryRow(ctx, "SELECT count(*) FROM tracks").Scan(&total); err != nil { + t.Fatalf("count: %v", err) + } + if total != 8 { + t.Errorf("tracks = %d, want 8 — a rename must not add a row", total) + } +}