feat(library): adopt moved files instead of forking their history — #2528
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.
This commit is contained in:
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user