From 24d330424ffb65ff0142da684dd63d8fc0fcb24f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 6 Aug 2026 15:56:07 -0400 Subject: [PATCH] =?UTF-8?q?feat(library):=20adopt=20moved=20files=20instea?= =?UTF-8?q?d=20of=20forking=20their=20history=20=E2=80=94=20#2528?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Track identity was file_path, so a file that came back renamed or in a different directory looked like a deletion plus an unrelated new track: the old row kept the like and every play_event while a fresh zero-history row appeared, and nothing connected them. A liked song read as unliked, its play count reset, and Rediscover could offer it as a discovery — silently. Renumbering an album was enough, which is what happened to the operator's copy of Minutes to Midnight. Adoption re-points the existing row's file_path at the new location and clears its missing mark. The 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 — and clients see an update rather than a delete-and-create, so no cache churn either. Matching is MBID first (identifies the recording, so it survives a re-encode), then file_size + duration_ms for untagged files. Both fingerprint components must be non-zero: duration_ms is 0 when ffprobe failed, and matching 0 against 0 would pair up unrelated broken files. 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. An ambiguous match inserts fresh rather than adopting one arbitrarily: a fork is recoverable later, a wrong merge isn't. Scan is now three phases, and the order is the point. Adoption can only claim a row that is ALREADY marked missing, but reconcile previously ran after processing — so a rename performed while the server was down surfaced the deletion and the addition in the same scan, the new path inserted first, and the fork became permanent. Enumeration is therefore separated from processing so reconcile can run between them: walk (paths only, no tag reads or probes) -> reconcile -> process in walk order. Consequence worth knowing: when reconcile refuses (an absent root, or a reorganisation exceeding the 25% mark cap) adoption cannot fire and renamed files fork as before. That's the pre-#2528 behaviour rather than a new failure, and the warning now names it. The old outer walk-error branch was unreachable — the callback always returned nil, so WalkDir never surfaced an error — and verifyRootsPresent is the real protection, so enumerate counts walk errors instead of pretending to abort on them. --- internal/db/dbq/tracks.sql.go | 115 ++++++++++++ internal/db/queries/tracks.sql | 44 +++++ internal/library/moved.go | 151 ++++++++++++++++ internal/library/moved_test.go | 295 +++++++++++++++++++++++++++++++ internal/library/scanner.go | 152 +++++++++++----- internal/library/scanner_test.go | 103 +++++++++++ 6 files changed, 811 insertions(+), 49 deletions(-) create mode 100644 internal/library/moved.go create mode 100644 internal/library/moved_test.go 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) + } +}