diff --git a/internal/db/dbq/browse.sql.go b/internal/db/dbq/browse.sql.go index be5de0f4..bf428524 100644 --- a/internal/db/dbq/browse.sql.go +++ b/internal/db/dbq/browse.sql.go @@ -18,6 +18,7 @@ WHERE EXISTS ( FROM tracks JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true WHERE tracks.album_id = albums.id + AND tracks.missing_since IS NULL AND trim(g.genre) = trim($1::text) ) ` @@ -97,6 +98,7 @@ WHERE EXISTS ( FROM tracks JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true WHERE tracks.album_id = albums.id + AND tracks.missing_since IS NULL AND trim(g.genre) = trim($1::text) ) ORDER BY albums.sort_title, albums.id @@ -219,6 +221,7 @@ SELECT DISTINCT trim(g.genre) AS genre FROM tracks JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true WHERE tracks.album_id = $1 AND trim(g.genre) <> '' + AND tracks.missing_since IS NULL ORDER BY trim(g.genre) ` @@ -251,6 +254,7 @@ SELECT DISTINCT trim(g.genre) AS genre FROM tracks JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true WHERE tracks.artist_id = $1 AND trim(g.genre) <> '' + AND tracks.missing_since IS NULL ORDER BY trim(g.genre) ` @@ -278,10 +282,12 @@ func (q *Queries) ListGenresForArtist(ctx context.Context, artistID pgtype.UUID) } const listGenresWithCount = `-- name: ListGenresWithCount :many + SELECT trim(g.genre) AS genre, COUNT(DISTINCT tracks.id)::bigint AS track_count FROM tracks JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true WHERE trim(g.genre) <> '' + AND tracks.missing_since IS NULL GROUP BY trim(g.genre) ORDER BY track_count DESC, trim(g.genre) ` @@ -291,6 +297,17 @@ type ListGenresWithCountRow struct { TrackCount int64 } +// Every query in this file filters `tracks.missing_since IS NULL` (#2523). +// A row whose file has vanished keeps its genre forever — the scanner walks the +// filesystem, so it never revisits a path that no longer exists — which is how +// pre-#2499 welded genres survived a full re-scan and kept showing in the index. +// Browsing is a way of finding something to play, so a track that cannot play +// should not shape it. +// +// Year queries below join albums only and are deliberately left alone: an album +// is still a real release even if some of its tracks are gone. An album whose +// EVERY track is missing will linger on the year axis; that's a narrower case, +// tracked with the rest of the cleanup work. // Genre browse index (#367). // // Genres live inline on tracks.genre as a delimited string, so this splits on diff --git a/internal/db/dbq/discover.sql.go b/internal/db/dbq/discover.sql.go index 69161ec1..7abd362a 100644 --- a/internal/db/dbq/discover.sql.go +++ b/internal/db/dbq/discover.sql.go @@ -15,7 +15,8 @@ const listCrossUserLikedTracksForDiscover = `-- name: ListCrossUserLikedTracksFo SELECT t.id, t.album_id, t.artist_id FROM general_likes gl JOIN tracks t ON t.id = gl.track_id - WHERE gl.user_id != $1 + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND gl.user_id != $1 AND NOT EXISTS ( SELECT 1 FROM play_events pe WHERE pe.user_id = $1 @@ -95,7 +96,8 @@ dormant_artists AS ( SELECT t.id, t.album_id, t.artist_id FROM tracks t JOIN dormant_artists da ON da.id = t.artist_id - WHERE NOT EXISTS ( + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND NOT EXISTS ( SELECT 1 FROM play_events pe WHERE pe.user_id = $1 AND pe.track_id = t.id @@ -159,7 +161,8 @@ func (q *Queries) ListDormantArtistTracksForDiscover(ctx context.Context, arg Li const listRandomUnheardTracksForDiscover = `-- name: ListRandomUnheardTracksForDiscover :many SELECT t.id, t.album_id, t.artist_id FROM tracks t - WHERE NOT EXISTS ( + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND NOT EXISTS ( SELECT 1 FROM play_events pe WHERE pe.user_id = $1 AND pe.track_id = t.id @@ -217,7 +220,8 @@ SELECT t.id, t.album_id, t.artist_id FROM tracks t JOIN LATERAL regexp_split_to_table(coalesce(t.genre, ''), '[;,]') AS g_split(g) ON true JOIN taste_profile_tags nt ON nt.user_id = $1 AND trim(g_split.g) = nt.tag - WHERE nt.weight > 0 + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND nt.weight > 0 AND trim(g_split.g) <> '' AND NOT EXISTS ( SELECT 1 FROM play_events pe diff --git a/internal/db/dbq/events.sql.go b/internal/db/dbq/events.sql.go index 837edfe1..64b35b72 100644 --- a/internal/db/dbq/events.sql.go +++ b/internal/db/dbq/events.sql.go @@ -261,7 +261,7 @@ func (q *Queries) InsertSkipEvent(ctx context.Context, arg InsertSkipEventParams } const listRecentSessionTracks = `-- name: ListRecentSessionTracks :many -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version FROM tracks t +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since FROM tracks t JOIN play_events pe ON pe.track_id = t.id WHERE pe.session_id = $1 AND pe.started_at < $2 @@ -306,6 +306,7 @@ func (q *Queries) ListRecentSessionTracks(ctx context.Context, arg ListRecentSes &i.TagSource, &i.TagSourcesVersion, &i.TagReadVersion, + &i.MissingSince, ); err != nil { return nil, err } diff --git a/internal/db/dbq/history.sql.go b/internal/db/dbq/history.sql.go index ef652a45..26f99fc9 100644 --- a/internal/db/dbq/history.sql.go +++ b/internal/db/dbq/history.sql.go @@ -14,7 +14,7 @@ import ( const listUserHistory = `-- name: ListUserHistory :many SELECT pe.id AS event_id, pe.started_at, - t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, + t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, albums.title AS album_title, artists.name AS artist_name FROM play_events pe @@ -80,6 +80,7 @@ func (q *Queries) ListUserHistory(ctx context.Context, arg ListUserHistoryParams &i.Track.TagSource, &i.Track.TagSourcesVersion, &i.Track.TagReadVersion, + &i.Track.MissingSince, &i.AlbumTitle, &i.ArtistName, ); err != nil { diff --git a/internal/db/dbq/likes.sql.go b/internal/db/dbq/likes.sql.go index 369eddb1..f5eaa5e6 100644 --- a/internal/db/dbq/likes.sql.go +++ b/internal/db/dbq/likes.sql.go @@ -259,7 +259,7 @@ func (q *Queries) ListLikedTrackIDs(ctx context.Context, userID pgtype.UUID) ([] } const listLikedTrackRows = `-- name: ListLikedTrackRows :many -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version FROM tracks t +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since FROM tracks t JOIN general_likes l ON l.track_id = t.id WHERE l.user_id = $1 ORDER BY l.liked_at DESC @@ -300,6 +300,7 @@ func (q *Queries) ListLikedTrackRows(ctx context.Context, arg ListLikedTrackRows &i.TagSource, &i.TagSourcesVersion, &i.TagReadVersion, + &i.MissingSince, ); err != nil { return nil, err } diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index e8ae8630..e4e01981 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -643,6 +643,7 @@ type Track struct { TagSource *string TagSourcesVersion int32 TagReadVersion int16 + MissingSince pgtype.Timestamptz } type TrackSimilarity struct { diff --git a/internal/db/dbq/recommendation.sql.go b/internal/db/dbq/recommendation.sql.go index 96f4947b..f290ec35 100644 --- a/internal/db/dbq/recommendation.sql.go +++ b/internal/db/dbq/recommendation.sql.go @@ -208,7 +208,7 @@ WITH plays AS ( WHERE user_id = $2 AND was_skipped = false GROUP BY track_id ) -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, albums.title AS album_title, artists.name AS artist_name FROM plays p @@ -216,6 +216,7 @@ JOIN tracks t ON t.id = p.track_id JOIN albums ON albums.id = t.album_id JOIN artists ON artists.id = t.artist_id WHERE t.artist_id = $1 + AND t.missing_since IS NULL -- #2523: never offer a file that is gone AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q WHERE q.user_id = $2 AND q.track_id = t.id @@ -268,6 +269,7 @@ func (q *Queries) ListMostPlayedTracksForArtist(ctx context.Context, arg ListMos &i.Track.TagSource, &i.Track.TagSourcesVersion, &i.Track.TagReadVersion, + &i.Track.MissingSince, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -288,14 +290,15 @@ WITH plays AS ( WHERE user_id = $1 AND was_skipped = false GROUP BY track_id ) -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, albums.title AS album_title, artists.name AS artist_name FROM plays p JOIN tracks t ON t.id = p.track_id JOIN albums ON albums.id = t.album_id JOIN artists ON artists.id = t.artist_id -WHERE NOT EXISTS ( +WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q WHERE q.user_id = $1 AND q.track_id = t.id ) @@ -350,6 +353,7 @@ func (q *Queries) ListMostPlayedTracksForUser(ctx context.Context, arg ListMostP &i.Track.TagSource, &i.Track.TagSourcesVersion, &i.Track.TagReadVersion, + &i.Track.MissingSince, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -687,7 +691,7 @@ func (q *Queries) ListRediscoverArtistsForUser(ctx context.Context, arg ListRedi const loadRadioCandidates = `-- name: LoadRadioCandidates :many SELECT - t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, + t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, (l.user_id IS NOT NULL)::bool AS is_liked, pe.last_played_at::timestamptz AS last_played_at, pe.play_count, @@ -705,6 +709,7 @@ LEFT JOIN LATERAL ( WHERE user_id = $1 AND track_id = t.id ) pe ON true WHERE t.id <> $2 + AND t.missing_since IS NULL -- #2523: never offer a file that is gone AND NOT EXISTS ( SELECT 1 FROM play_events WHERE user_id = $1 AND track_id = t.id @@ -766,6 +771,7 @@ func (q *Queries) LoadRadioCandidates(ctx context.Context, arg LoadRadioCandidat &i.Track.TagSource, &i.Track.TagSourcesVersion, &i.Track.TagReadVersion, + &i.Track.MissingSince, &i.IsLiked, &i.LastPlayedAt, &i.PlayCount, @@ -898,7 +904,7 @@ random_fill AS ( LIMIT $9 ) SELECT - t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, + t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, (l.user_id IS NOT NULL)::bool AS is_liked, pe.last_played_at::timestamptz AS last_played_at, pe.play_count, @@ -914,7 +920,7 @@ FROM ( UNION ALL SELECT track_id, sim_score FROM coplay_artists UNION ALL SELECT track_id, sim_score FROM random_fill ) u -JOIN tracks t ON t.id = u.track_id +JOIN tracks t ON t.id = u.track_id AND t.missing_since IS NULL -- #2523: never offer a file that is gone JOIN albums al ON al.id = t.album_id LEFT JOIN general_likes l ON l.user_id = $1 AND l.track_id = t.id LEFT JOIN LATERAL ( @@ -1008,6 +1014,7 @@ func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandid &i.Track.TagSource, &i.Track.TagSourcesVersion, &i.Track.TagReadVersion, + &i.Track.MissingSince, &i.IsLiked, &i.LastPlayedAt, &i.PlayCount, diff --git a/internal/db/dbq/system_mixes.sql.go b/internal/db/dbq/system_mixes.sql.go index ce98bd19..d8046f1b 100644 --- a/internal/db/dbq/system_mixes.sql.go +++ b/internal/db/dbq/system_mixes.sql.go @@ -39,7 +39,8 @@ SELECT t.id, t.album_id, t.artist_id JOIN affinity_artists aa ON aa.artist_id = t.artist_id LEFT JOIN play_counts pc ON pc.track_id = t.id LEFT JOIN skip_counts sc ON sc.track_id = t.id - WHERE COALESCE(pc.c, 0) <= 2 + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND COALESCE(pc.c, 0) <= 2 AND COALESCE(sc.c, 0) < 2 AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q @@ -124,7 +125,8 @@ SELECT t.id, t.album_id, t.artist_id, alt.tier::int AS tier FROM tracks t JOIN albums al ON al.id = t.album_id JOIN albums_tiered alt ON alt.album_id = al.id - WHERE alt.tier IS NOT NULL + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND alt.tier IS NOT NULL AND NOT EXISTS (SELECT 1 FROM attempted a WHERE a.track_id = t.id) AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q @@ -225,7 +227,8 @@ albums_tiered AS ( SELECT t.id, t.album_id, t.artist_id, alt.tier::int AS tier FROM tracks t JOIN albums_tiered alt ON alt.album_id = t.album_id - WHERE NOT EXISTS ( + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q WHERE q.user_id = $1 AND q.track_id = t.id ) @@ -303,7 +306,8 @@ WITH windowed AS ( SELECT t.id, t.album_id, t.artist_id FROM tracks t JOIN windowed w ON w.track_id = t.id - WHERE NOT EXISTS ( + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q WHERE q.user_id = $1 AND q.track_id = t.id ) @@ -367,7 +371,8 @@ WITH stats AS ( SELECT t.id, t.album_id, t.artist_id FROM tracks t JOIN stats s ON s.track_id = t.id - WHERE s.c >= 3 + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND s.c >= 3 AND s.last_at <= now() - interval '30 days' AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q diff --git a/internal/db/dbq/system_playlists.sql.go b/internal/db/dbq/system_playlists.sql.go index a86564a9..f76d10ff 100644 --- a/internal/db/dbq/system_playlists.sql.go +++ b/internal/db/dbq/system_playlists.sql.go @@ -189,6 +189,7 @@ func (q *Queries) GetSystemPlaylistRun(ctx context.Context, userID pgtype.UUID) const listActiveUsersForSystemPlaylists = `-- name: ListActiveUsersForSystemPlaylists :many + SELECT u.id FROM users u WHERE EXISTS ( SELECT 1 FROM play_events pe @@ -197,6 +198,13 @@ SELECT u.id FROM users u ) ` +// Track picks here join `tracks ... AND t.missing_since IS NULL` (#2523): a +// seed or For-You candidate has to be something that can actually play. Note +// this only affects newly GENERATED playlists — already-stored system +// playlists keep their rows until the next daily rebuild, which is why the +// shared ListPlaylistTracks read path is deliberately left unfiltered (it +// also serves user-curated playlists, where hiding a track the user added +// themselves would be wrong). // M7 #352 slice 2: system-generated playlist queries. // Active = had a play in the last 7 days. The cron iterates this list. func (q *Queries) ListActiveUsersForSystemPlaylists(ctx context.Context) ([]pgtype.UUID, error) { @@ -298,7 +306,7 @@ recent7 AS ( COUNT(*) FILTER (WHERE pe.was_skipped = false) AS play_count, 0 AS tier FROM play_events pe - JOIN tracks t ON t.id = pe.track_id + JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL WHERE pe.user_id = $1 AND pe.started_at > now() - INTERVAL '7 days' AND t.artist_id IS NOT NULL @@ -309,7 +317,7 @@ recent30 AS ( COUNT(*) FILTER (WHERE pe.was_skipped = false) AS play_count, 1 AS tier FROM play_events pe - JOIN tracks t ON t.id = pe.track_id + JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL WHERE pe.user_id = $1 AND pe.started_at > now() - INTERVAL '30 days' AND t.artist_id IS NOT NULL @@ -320,7 +328,7 @@ alltime AS ( COUNT(*) FILTER (WHERE pe.was_skipped = false) AS play_count, 2 AS tier FROM play_events pe - JOIN tracks t ON t.id = pe.track_id + JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL WHERE pe.user_id = $1 AND t.artist_id IS NOT NULL GROUP BY t.artist_id @@ -432,7 +440,7 @@ const pickTopPlayedTrackForArtistByUser = `-- name: PickTopPlayedTrackForArtistB SELECT COALESCE( (SELECT t.id FROM play_events pe - JOIN tracks t ON t.id = pe.track_id + JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL WHERE pe.user_id = $1 AND t.artist_id = $2 AND pe.started_at > now() - INTERVAL '7 days' @@ -472,7 +480,7 @@ const pickTopPlayedTracksForUser = `-- name: PickTopPlayedTracksForUser :many WITH recent AS ( SELECT t.id, COUNT(*) AS c, 0 AS tier FROM play_events pe - JOIN tracks t ON t.id = pe.track_id + JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL WHERE pe.user_id = $1 AND pe.started_at > now() - INTERVAL '30 days' AND pe.was_skipped = false @@ -481,7 +489,7 @@ WITH recent AS ( alltime AS ( SELECT t.id, COUNT(*) AS c, 1 AS tier FROM play_events pe - JOIN tracks t ON t.id = pe.track_id + JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL WHERE pe.user_id = $1 AND pe.was_skipped = false GROUP BY t.id diff --git a/internal/db/dbq/tracks.sql.go b/internal/db/dbq/tracks.sql.go index 9c3b0daa..ad2d0244 100644 --- a/internal/db/dbq/tracks.sql.go +++ b/internal/db/dbq/tracks.sql.go @@ -11,6 +11,52 @@ 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 + WHERE id = ANY($1::uuid[]) + AND missing_since IS NOT NULL +` + +// Clears the mark on rows whose file is back. Runs independently of the mtime +// skip check, so a file that reappears unchanged is un-marked even though the +// scanner skips re-reading its tags. +func (q *Queries) ClearTracksMissing(ctx context.Context, ids []pgtype.UUID) (int64, error) { + result, err := q.db.Exec(ctx, clearTracksMissing, ids) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const countTracksByAlbum = `-- name: CountTracksByAlbum :one SELECT count(*) FROM tracks WHERE album_id = $1 ` @@ -89,8 +135,95 @@ 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 FROM tracks WHERE id = $1 +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 ` func (q *Queries) GetTrackByID(ctx context.Context, id pgtype.UUID) (Track, error) { @@ -115,12 +248,13 @@ func (q *Queries) GetTrackByID(ctx context.Context, id pgtype.UUID) (Track, erro &i.TagSource, &i.TagSourcesVersion, &i.TagReadVersion, + &i.MissingSince, ) return i, err } const getTrackByPath = `-- name: GetTrackByPath :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 FROM tracks WHERE file_path = $1 +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 file_path = $1 ` func (q *Queries) GetTrackByPath(ctx context.Context, filePath string) (Track, error) { @@ -145,12 +279,13 @@ func (q *Queries) GetTrackByPath(ctx context.Context, filePath string) (Track, e &i.TagSource, &i.TagSourcesVersion, &i.TagReadVersion, + &i.MissingSince, ) return i, err } const getTracksByIDs = `-- name: GetTracksByIDs :many -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 FROM tracks WHERE id = ANY($1::uuid[]) +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 = ANY($1::uuid[]) ` // Batched lookup used by /api/library/sync to hydrate upsert payloads @@ -183,6 +318,7 @@ func (q *Queries) GetTracksByIDs(ctx context.Context, dollar_1 []pgtype.UUID) ([ &i.TagSource, &i.TagSourcesVersion, &i.TagReadVersion, + &i.MissingSince, ); err != nil { return nil, err } @@ -195,7 +331,7 @@ func (q *Queries) GetTracksByIDs(ctx context.Context, dollar_1 []pgtype.UUID) ([ } const listArtistTracksForUser = `-- name: ListArtistTracksForUser :many -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, albums.title AS album_title, artists.name AS artist_name FROM tracks t @@ -254,6 +390,7 @@ func (q *Queries) ListArtistTracksForUser(ctx context.Context, arg ListArtistTra &i.Track.TagSource, &i.Track.TagSourcesVersion, &i.Track.TagReadVersion, + &i.Track.MissingSince, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -268,7 +405,7 @@ func (q *Queries) ListArtistTracksForUser(ctx context.Context, arg ListArtistTra } const listRandomTracksForUser = `-- name: ListRandomTracksForUser :many -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, albums.title AS album_title, artists.name AS artist_name FROM tracks t @@ -324,6 +461,7 @@ func (q *Queries) ListRandomTracksForUser(ctx context.Context, arg ListRandomTra &i.Track.TagSource, &i.Track.TagSourcesVersion, &i.Track.TagReadVersion, + &i.Track.MissingSince, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -337,8 +475,43 @@ func (q *Queries) ListRandomTracksForUser(ctx context.Context, arg ListRandomTra return items, nil } +const listTrackPathsForReconcile = `-- name: ListTrackPathsForReconcile :many +SELECT id, file_path, missing_since FROM tracks +` + +type ListTrackPathsForReconcileRow struct { + ID pgtype.UUID + FilePath string + MissingSince pgtype.Timestamptz +} + +// Every row's path + current missing mark, for the scanner's reconcile pass +// (#2523). Deliberately unfiltered and unpaged: reconcile has to compare the +// WHOLE table against what the walk saw, and a filtered subset would let rows +// outside it drift forever. Three narrow columns keep it cheap even on a +// library of a few hundred thousand tracks. +func (q *Queries) ListTrackPathsForReconcile(ctx context.Context) ([]ListTrackPathsForReconcileRow, error) { + rows, err := q.db.Query(ctx, listTrackPathsForReconcile) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListTrackPathsForReconcileRow + for rows.Next() { + var i ListTrackPathsForReconcileRow + if err := rows.Scan(&i.ID, &i.FilePath, &i.MissingSince); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listTracksByAlbum = `-- name: ListTracksByAlbum :many -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 FROM tracks +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 album_id = $1 AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q @@ -383,6 +556,7 @@ func (q *Queries) ListTracksByAlbum(ctx context.Context, arg ListTracksByAlbumPa &i.TagSource, &i.TagSourcesVersion, &i.TagReadVersion, + &i.MissingSince, ); err != nil { return nil, err } @@ -429,8 +603,31 @@ func (q *Queries) ListTracksMissingMbidWithPath(ctx context.Context, limit int32 return items, nil } +const markTracksMissing = `-- name: MarkTracksMissing :execrows +UPDATE tracks + SET missing_since = now() + WHERE id = ANY($1::uuid[]) + AND missing_since IS NULL +` + +// Marks rows whose file the walk did not see. `missing_since IS NULL` in the +// predicate makes this idempotent: a row already marked keeps its ORIGINAL +// timestamp, so "how long has it been gone" survives repeated scans. Losing +// that would make any age-based cleanup policy meaningless. +// +// updated_at is deliberately NOT touched. It tracks content changes and gates +// the scanner's mtime skip; moving it here would make a returning file look +// newer than its own mtime and stop its tags being re-read. +func (q *Queries) MarkTracksMissing(ctx context.Context, ids []pgtype.UUID) (int64, error) { + result, err := q.db.Exec(ctx, markTracksMissing, ids) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const searchTracks = `-- name: SearchTracks :many -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 FROM tracks +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 title ILIKE '%' || $1::text || '%' AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q @@ -482,6 +679,7 @@ func (q *Queries) SearchTracks(ctx context.Context, arg SearchTracksParams) ([]T &i.TagSource, &i.TagSourcesVersion, &i.TagReadVersion, + &i.MissingSince, ); err != nil { return nil, err } @@ -533,7 +731,7 @@ ON CONFLICT (file_path) DO UPDATE SET -- next scan can short-circuit them again (#2499). tag_read_version = EXCLUDED.tag_read_version, updated_at = now() -RETURNING 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 +RETURNING 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 ` type UpsertTrackParams struct { @@ -589,6 +787,7 @@ func (q *Queries) UpsertTrack(ctx context.Context, arg UpsertTrackParams) (Track &i.TagSource, &i.TagSourcesVersion, &i.TagReadVersion, + &i.MissingSince, ) return i, err } diff --git a/internal/db/migrations/0055_track_missing_since.down.sql b/internal/db/migrations/0055_track_missing_since.down.sql new file mode 100644 index 00000000..d9bc5bc1 --- /dev/null +++ b/internal/db/migrations/0055_track_missing_since.down.sql @@ -0,0 +1,4 @@ +DROP INDEX IF EXISTS tracks_missing_since_idx; + +ALTER TABLE tracks + DROP COLUMN missing_since; diff --git a/internal/db/migrations/0055_track_missing_since.up.sql b/internal/db/migrations/0055_track_missing_since.up.sql new file mode 100644 index 00000000..11952ade --- /dev/null +++ b/internal/db/migrations/0055_track_missing_since.up.sql @@ -0,0 +1,25 @@ +-- Marks a track whose file the scanner could no longer find (#2523). +-- +-- NULL means present. A timestamp means the file was absent as of that scan, +-- and is the point from which "how long has this been gone" is measured — which +-- is what a later cleanup pass needs in order to require a grace period rather +-- than deleting on a single missed stat. +-- +-- Deliberately a nullable timestamp rather than a boolean: "missing" is not a +-- state we want to act on immediately, and the age is the only thing that makes +-- an automated deletion safe to reason about. +-- +-- No default and no backfill. Existing rows start NULL (present) and the next +-- full scan sets the mark where it belongs — a migration cannot check the +-- filesystem, and guessing here would mark the whole library on a server whose +-- media volume happens to be detached at upgrade time. +ALTER TABLE tracks + ADD COLUMN missing_since timestamptz; + +-- Partial index: the only query that filters on this column positively is the +-- admin "what's missing" list, which is a small set. Playback and browse +-- queries filter `missing_since IS NULL`, which matches nearly every row and is +-- better served by a sequential scan than an index lookup. +CREATE INDEX tracks_missing_since_idx + ON tracks (missing_since) + WHERE missing_since IS NOT NULL; diff --git a/internal/db/queries/browse.sql b/internal/db/queries/browse.sql index 3a1e3385..00d47d29 100644 --- a/internal/db/queries/browse.sql +++ b/internal/db/queries/browse.sql @@ -1,3 +1,15 @@ +-- Every query in this file filters `tracks.missing_since IS NULL` (#2523). +-- A row whose file has vanished keeps its genre forever — the scanner walks the +-- filesystem, so it never revisits a path that no longer exists — which is how +-- pre-#2499 welded genres survived a full re-scan and kept showing in the index. +-- Browsing is a way of finding something to play, so a track that cannot play +-- should not shape it. +-- +-- Year queries below join albums only and are deliberately left alone: an album +-- is still a real release even if some of its tracks are gone. An album whose +-- EVERY track is missing will linger on the year axis; that's a narrower case, +-- tracked with the rest of the cleanup work. + -- name: ListGenresWithCount :many -- Genre browse index (#367). -- @@ -22,6 +34,7 @@ SELECT trim(g.genre) AS genre, COUNT(DISTINCT tracks.id)::bigint AS track_count FROM tracks JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true WHERE trim(g.genre) <> '' + AND tracks.missing_since IS NULL GROUP BY trim(g.genre) -- Ordered by the expression, not the output alias: `ORDER BY genre` is -- ambiguous between the alias and tracks.genre, and sqlc rejects it. @@ -41,6 +54,7 @@ WHERE EXISTS ( FROM tracks JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true WHERE tracks.album_id = albums.id + AND tracks.missing_since IS NULL AND trim(g.genre) = trim(sqlc.arg(genre)::text) ) ORDER BY albums.sort_title, albums.id @@ -56,6 +70,7 @@ WHERE EXISTS ( FROM tracks JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true WHERE tracks.album_id = albums.id + AND tracks.missing_since IS NULL AND trim(g.genre) = trim(sqlc.arg(genre)::text) ); @@ -96,6 +111,7 @@ SELECT DISTINCT trim(g.genre) AS genre FROM tracks JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true WHERE tracks.album_id = $1 AND trim(g.genre) <> '' + AND tracks.missing_since IS NULL ORDER BY trim(g.genre); -- name: ListGenresForArtist :many @@ -106,4 +122,5 @@ SELECT DISTINCT trim(g.genre) AS genre FROM tracks JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true WHERE tracks.artist_id = $1 AND trim(g.genre) <> '' + AND tracks.missing_since IS NULL ORDER BY trim(g.genre); diff --git a/internal/db/queries/discover.sql b/internal/db/queries/discover.sql index ed44c378..86385609 100644 --- a/internal/db/queries/discover.sql +++ b/internal/db/queries/discover.sql @@ -29,7 +29,8 @@ dormant_artists AS ( SELECT t.id, t.album_id, t.artist_id FROM tracks t JOIN dormant_artists da ON da.id = t.artist_id - WHERE NOT EXISTS ( + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND NOT EXISTS ( SELECT 1 FROM play_events pe WHERE pe.user_id = $1 AND pe.track_id = t.id @@ -60,7 +61,8 @@ SELECT t.id, t.album_id, t.artist_id SELECT t.id, t.album_id, t.artist_id FROM general_likes gl JOIN tracks t ON t.id = gl.track_id - WHERE gl.user_id != $1 + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND gl.user_id != $1 AND NOT EXISTS ( SELECT 1 FROM play_events pe WHERE pe.user_id = $1 @@ -86,7 +88,8 @@ SELECT t.id, t.album_id, t.artist_id -- $1 = user_id, $2 = date string for md5 ordering. SELECT t.id, t.album_id, t.artist_id FROM tracks t - WHERE NOT EXISTS ( + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND NOT EXISTS ( SELECT 1 FROM play_events pe WHERE pe.user_id = $1 AND pe.track_id = t.id @@ -117,7 +120,8 @@ SELECT t.id, t.album_id, t.artist_id FROM tracks t JOIN LATERAL regexp_split_to_table(coalesce(t.genre, ''), '[;,]') AS g_split(g) ON true JOIN taste_profile_tags nt ON nt.user_id = $1 AND trim(g_split.g) = nt.tag - WHERE nt.weight > 0 + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND nt.weight > 0 AND trim(g_split.g) <> '' AND NOT EXISTS ( SELECT 1 FROM play_events pe diff --git a/internal/db/queries/recommendation.sql b/internal/db/queries/recommendation.sql index fcb35e8b..20fbf7f7 100644 --- a/internal/db/queries/recommendation.sql +++ b/internal/db/queries/recommendation.sql @@ -24,6 +24,7 @@ LEFT JOIN LATERAL ( WHERE user_id = $1 AND track_id = t.id ) pe ON true WHERE t.id <> $2 + AND t.missing_since IS NULL -- #2523: never offer a file that is gone AND NOT EXISTS ( SELECT 1 FROM play_events WHERE user_id = $1 AND track_id = t.id @@ -177,7 +178,7 @@ FROM ( UNION ALL SELECT track_id, sim_score FROM coplay_artists UNION ALL SELECT track_id, sim_score FROM random_fill ) u -JOIN tracks t ON t.id = u.track_id +JOIN tracks t ON t.id = u.track_id AND t.missing_since IS NULL -- #2523: never offer a file that is gone JOIN albums al ON al.id = t.album_id LEFT JOIN general_likes l ON l.user_id = $1 AND l.track_id = t.id LEFT JOIN LATERAL ( @@ -382,7 +383,8 @@ FROM plays p JOIN tracks t ON t.id = p.track_id JOIN albums ON albums.id = t.album_id JOIN artists ON artists.id = t.artist_id -WHERE NOT EXISTS ( +WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q WHERE q.user_id = $1 AND q.track_id = t.id ) @@ -408,6 +410,7 @@ JOIN tracks t ON t.id = p.track_id JOIN albums ON albums.id = t.album_id JOIN artists ON artists.id = t.artist_id WHERE t.artist_id = sqlc.arg(artist_id) + AND t.missing_since IS NULL -- #2523: never offer a file that is gone AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q WHERE q.user_id = sqlc.arg(user_id) AND q.track_id = t.id diff --git a/internal/db/queries/system_mixes.sql b/internal/db/queries/system_mixes.sql index 7503a122..6469ae06 100644 --- a/internal/db/queries/system_mixes.sql +++ b/internal/db/queries/system_mixes.sql @@ -40,7 +40,8 @@ SELECT t.id, t.album_id, t.artist_id JOIN affinity_artists aa ON aa.artist_id = t.artist_id LEFT JOIN play_counts pc ON pc.track_id = t.id LEFT JOIN skip_counts sc ON sc.track_id = t.id - WHERE COALESCE(pc.c, 0) <= 2 + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND COALESCE(pc.c, 0) <= 2 AND COALESCE(sc.c, 0) < 2 AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q @@ -70,7 +71,8 @@ WITH stats AS ( SELECT t.id, t.album_id, t.artist_id FROM tracks t JOIN stats s ON s.track_id = t.id - WHERE s.c >= 3 + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND s.c >= 3 AND s.last_at <= now() - interval '30 days' AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q @@ -149,7 +151,8 @@ albums_tiered AS ( SELECT t.id, t.album_id, t.artist_id, alt.tier::int AS tier FROM tracks t JOIN albums_tiered alt ON alt.album_id = t.album_id - WHERE NOT EXISTS ( + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q WHERE q.user_id = $1 AND q.track_id = t.id ) @@ -187,7 +190,8 @@ WITH windowed AS ( SELECT t.id, t.album_id, t.artist_id FROM tracks t JOIN windowed w ON w.track_id = t.id - WHERE NOT EXISTS ( + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q WHERE q.user_id = $1 AND q.track_id = t.id ) @@ -240,7 +244,8 @@ SELECT t.id, t.album_id, t.artist_id, alt.tier::int AS tier FROM tracks t JOIN albums al ON al.id = t.album_id JOIN albums_tiered alt ON alt.album_id = al.id - WHERE alt.tier IS NOT NULL + WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone + AND alt.tier IS NOT NULL AND NOT EXISTS (SELECT 1 FROM attempted a WHERE a.track_id = t.id) AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q diff --git a/internal/db/queries/system_playlists.sql b/internal/db/queries/system_playlists.sql index b57fd079..41c1cff5 100644 --- a/internal/db/queries/system_playlists.sql +++ b/internal/db/queries/system_playlists.sql @@ -1,3 +1,11 @@ +-- Track picks here join `tracks ... AND t.missing_since IS NULL` (#2523): a +-- seed or For-You candidate has to be something that can actually play. Note +-- this only affects newly GENERATED playlists — already-stored system +-- playlists keep their rows until the next daily rebuild, which is why the +-- shared ListPlaylistTracks read path is deliberately left unfiltered (it +-- also serves user-curated playlists, where hiding a track the user added +-- themselves would be wrong). + -- M7 #352 slice 2: system-generated playlist queries. -- name: ListActiveUsersForSystemPlaylists :many @@ -72,7 +80,7 @@ recent7 AS ( COUNT(*) FILTER (WHERE pe.was_skipped = false) AS play_count, 0 AS tier FROM play_events pe - JOIN tracks t ON t.id = pe.track_id + JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL WHERE pe.user_id = $1 AND pe.started_at > now() - INTERVAL '7 days' AND t.artist_id IS NOT NULL @@ -83,7 +91,7 @@ recent30 AS ( COUNT(*) FILTER (WHERE pe.was_skipped = false) AS play_count, 1 AS tier FROM play_events pe - JOIN tracks t ON t.id = pe.track_id + JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL WHERE pe.user_id = $1 AND pe.started_at > now() - INTERVAL '30 days' AND t.artist_id IS NOT NULL @@ -94,7 +102,7 @@ alltime AS ( COUNT(*) FILTER (WHERE pe.was_skipped = false) AS play_count, 2 AS tier FROM play_events pe - JOIN tracks t ON t.id = pe.track_id + JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL WHERE pe.user_id = $1 AND t.artist_id IS NOT NULL GROUP BY t.artist_id @@ -139,7 +147,7 @@ SELECT c.artist_id, WITH recent AS ( SELECT t.id, COUNT(*) AS c, 0 AS tier FROM play_events pe - JOIN tracks t ON t.id = pe.track_id + JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL WHERE pe.user_id = $1 AND pe.started_at > now() - INTERVAL '30 days' AND pe.was_skipped = false @@ -148,7 +156,7 @@ WITH recent AS ( alltime AS ( SELECT t.id, COUNT(*) AS c, 1 AS tier FROM play_events pe - JOIN tracks t ON t.id = pe.track_id + JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL WHERE pe.user_id = $1 AND pe.was_skipped = false GROUP BY t.id @@ -181,7 +189,7 @@ SELECT id SELECT COALESCE( (SELECT t.id FROM play_events pe - JOIN tracks t ON t.id = pe.track_id + JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL WHERE pe.user_id = $1 AND t.artist_id = $2 AND pe.started_at > now() - INTERVAL '7 days' diff --git a/internal/db/queries/tracks.sql b/internal/db/queries/tracks.sql index c16e7108..d11d11c2 100644 --- a/internal/db/queries/tracks.sql +++ b/internal/db/queries/tracks.sql @@ -137,3 +137,78 @@ RETURNING id, album_id, artist_id, file_path, mbid; -- Batched lookup used by /api/library/sync to hydrate upsert payloads -- (#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 +-- WHOLE table against what the walk saw, and a filtered subset would let rows +-- outside it drift forever. Three narrow columns keep it cheap even on a +-- library of a few hundred thousand tracks. +SELECT id, file_path, missing_since FROM tracks; + +-- name: MarkTracksMissing :execrows +-- Marks rows whose file the walk did not see. `missing_since IS NULL` in the +-- predicate makes this idempotent: a row already marked keeps its ORIGINAL +-- timestamp, so "how long has it been gone" survives repeated scans. Losing +-- that would make any age-based cleanup policy meaningless. +-- +-- updated_at is deliberately NOT touched. It tracks content changes and gates +-- the scanner's mtime skip; moving it here would make a returning file look +-- newer than its own mtime and stop its tags being re-read. +UPDATE tracks + SET missing_since = now() + WHERE id = ANY(sqlc.arg(ids)::uuid[]) + AND missing_since IS NULL; + +-- name: ClearTracksMissing :execrows +-- Clears the mark on rows whose file is back. Runs independently of the mtime +-- skip check, so a file that reappears unchanged is un-marked even though the +-- scanner skips re-reading its tags. +UPDATE tracks + SET missing_since = NULL + WHERE id = ANY(sqlc.arg(ids)::uuid[]) + AND missing_since IS NOT NULL; 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/reconcile.go b/internal/library/reconcile.go new file mode 100644 index 00000000..6c406cf2 --- /dev/null +++ b/internal/library/reconcile.go @@ -0,0 +1,155 @@ +package library + +import ( + "context" + "errors" + "fmt" + "os" + + "github.com/jackc/pgx/v5/pgtype" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// trackReconciler is the slice of dbq.Queries reconcileMissing needs. Narrowed +// to an interface so the guard logic — which is the part that can do damage — +// is unit-testable against a fake without a database. +type trackReconciler interface { + ListTrackPathsForReconcile(ctx context.Context) ([]dbq.ListTrackPathsForReconcileRow, error) + MarkTracksMissing(ctx context.Context, ids []pgtype.UUID) (int64, error) + ClearTracksMissing(ctx context.Context, ids []pgtype.UUID) (int64, error) +} + +// Reconcile marks tracks whose files have disappeared (#2523). +// +// Why this exists: nothing in Minstrel used to notice a deleted file. The walk +// only visits paths that exist, so a row whose file is gone was never scanned, +// never errored, never counted — permanently invisible. The watcher ignores +// removals by design (see classifyEvent), and the safety-net scan is the same +// walk, so it covers additions only. Rows accumulated forever, kept being +// offered to recommendations, and failed at playback. +// +// Why it MARKS rather than deletes: a missing file is a claim about the +// filesystem, and the filesystem lies transiently — an unmounted volume, a +// network-storage blip, a container that started before its media mount +// attached. Every other sweep in this codebase (internal/gc) resolves a truth +// *inside* the database and is safe to run blind. This one isn't, so the +// destructive step is deliberately not here. Marking is reversible: the next +// good scan clears it. + +// missingMarkMaxFraction caps how much of the library one reconcile may newly +// mark missing. A partially-attached mount is the failure this defends against: +// the roots resolve, the walk succeeds, and it legitimately sees only part of +// the library — evidence indistinguishable from a mass deletion. +// +// A quarter is deliberately conservative. A genuine bulk deletion trips it and +// gets logged rather than applied, which needs a second scan (or operator +// action) to take effect. That's the right trade: the cost of over-refusing is +// a stale row and a log line, and the cost of over-marking is a chunk of the +// library silently vanishing from every mix. +const missingMarkMaxFraction = 0.25 + +// reconcileMissing diffs the paths the walk saw against every row in the table. +// Rows not seen get marked; rows seen that carry a mark get cleared. +// +// seen must come from a COMPLETE walk of every configured root. Callers with a +// partial view must not call this. +func (s *Scanner) reconcileMissing( + ctx context.Context, q trackReconciler, seen map[string]struct{}, stats *Stats, +) error { + if err := s.verifyRootsPresent(); err != nil { + return err + } + // Roots resolved but the walk found nothing. Either the library is genuinely + // empty — in which case there is nothing to reconcile — or the mount is + // hollow. Both mean: don't act. + if len(seen) == 0 { + return errors.New("walk saw no audio files; refusing to reconcile") + } + + rows, err := q.ListTrackPathsForReconcile(ctx) + if err != nil { + return fmt.Errorf("list track paths: %w", err) + } + if len(rows) == 0 { + return nil + } + + var toMark, toClear []pgtype.UUID + for _, row := range rows { + _, present := seen[row.FilePath] + switch { + case !present && !row.MissingSince.Valid: + toMark = append(toMark, row.ID) + case present && row.MissingSince.Valid: + toClear = append(toClear, row.ID) + } + } + + // Clear before marking, and unconditionally. Restoring a file is never the + // dangerous direction, so it must not be blocked by the guard below — + // otherwise a library that tripped the cap once could never recover its + // marks even after the mount came back. + if len(toClear) > 0 { + n, err := q.ClearTracksMissing(ctx, toClear) + if err != nil { + return fmt.Errorf("clear missing marks: %w", err) + } + stats.Restored = int(n) + s.logger.Info("library scan: files returned", "count", n) + } + + if len(toMark) == 0 { + return nil + } + if fraction := float64(len(toMark)) / float64(len(rows)); fraction > missingMarkMaxFraction { + return fmt.Errorf( + "refusing to mark %d of %d tracks missing (%.0f%% > %.0f%% cap): "+ + "this looks like an unavailable mount rather than a deletion", + len(toMark), len(rows), fraction*100, missingMarkMaxFraction*100, + ) + } + + n, err := q.MarkTracksMissing(ctx, toMark) + if err != nil { + return fmt.Errorf("mark tracks missing: %w", err) + } + stats.Missing = int(n) + // Warn, not Info: every one of these is a library entry the operator + // probably didn't intend to lose, and the only place it surfaces today is + // this line. + s.logger.Warn("library scan: tracks marked missing (files not found)", + "count", n, "library_total", len(rows)) + return nil +} + +// verifyRootsPresent is the first and most important guard. If a configured root +// doesn't resolve to a readable directory, the walk beneath it found nothing and +// every row under it would look deleted. An unmounted media volume is the +// obvious case, and it is common enough — a container restart racing its volume +// mount does exactly this. +func (s *Scanner) verifyRootsPresent() error { + if len(s.paths) == 0 { + return errors.New("no scan roots configured") + } + for _, root := range s.paths { + info, err := os.Stat(root) + if err != nil { + return fmt.Errorf("scan root %q unavailable: %w", root, err) + } + if !info.IsDir() { + return fmt.Errorf("scan root %q is not a directory", root) + } + entries, err := os.ReadDir(root) + if err != nil { + return fmt.Errorf("scan root %q unreadable: %w", root, err) + } + // An empty root is the signature of a mount point with nothing mounted + // on it. `os.Stat` succeeds on the bare directory, so this is the only + // cheap way to tell the two apart. + if len(entries) == 0 { + return fmt.Errorf("scan root %q is empty; refusing to reconcile", root) + } + } + return nil +} diff --git a/internal/library/reconcile_test.go b/internal/library/reconcile_test.go new file mode 100644 index 00000000..a5248f73 --- /dev/null +++ b/internal/library/reconcile_test.go @@ -0,0 +1,326 @@ +package library + +import ( + "context" + "errors" + "fmt" + "io" + "log/slog" + "os" + "path/filepath" + "testing" + + "github.com/jackc/pgx/v5/pgtype" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// fakeReconciler records what reconcileMissing decided to do, so the guards can +// be tested without a database. The guards are the whole point of this pass — +// they are what stands between an unmounted volume and the library disappearing +// from every mix — so they get tested directly rather than via integration. +type fakeReconciler struct { + rows []dbq.ListTrackPathsForReconcileRow + marked []pgtype.UUID + cleared []pgtype.UUID + listErr error + markErr error + clearErr error +} + +func (f *fakeReconciler) ListTrackPathsForReconcile(context.Context) ([]dbq.ListTrackPathsForReconcileRow, error) { + return f.rows, f.listErr +} + +func (f *fakeReconciler) MarkTracksMissing(_ context.Context, ids []pgtype.UUID) (int64, error) { + if f.markErr != nil { + return 0, f.markErr + } + f.marked = append(f.marked, ids...) + return int64(len(ids)), nil +} + +func (f *fakeReconciler) ClearTracksMissing(_ context.Context, ids []pgtype.UUID) (int64, error) { + if f.clearErr != nil { + return 0, f.clearErr + } + f.cleared = append(f.cleared, ids...) + return int64(len(ids)), nil +} + +// Compile-time proof the real queries still satisfy what reconcile needs — the +// interface exists to narrow dbq.Queries, not to diverge from it. +var _ trackReconciler = (*dbq.Queries)(nil) + +func testUUID(n byte) pgtype.UUID { + var u pgtype.UUID + u.Bytes[15] = n + u.Valid = true + return u +} + +func markedAt() pgtype.Timestamptz { + return pgtype.Timestamptz{Valid: true} +} + +func row(n byte, path string, missing bool) dbq.ListTrackPathsForReconcileRow { + r := dbq.ListTrackPathsForReconcileRow{ID: testUUID(n), FilePath: path} + if missing { + r.MissingSince = markedAt() + } + return r +} + +// populatedRoot returns a directory containing one file, so verifyRootsPresent +// treats it as a real, mounted library root. +func populatedRoot(t *testing.T) string { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "a.mp3"), []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + return dir +} + +func testScanner(t *testing.T, roots ...string) *Scanner { + t.Helper() + return &Scanner{ + logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + paths: roots, + } +} + +func TestReconcileMissing_MarksRowsTheWalkDidNotSee(t *testing.T) { + root := populatedRoot(t) + s := testScanner(t, root) + + // 10 rows with 2 absent — 20%, deliberately under missingMarkMaxFraction so + // this exercises marking rather than the cap. (An earlier version of this + // test used 2-of-4 and was really testing the guard by accident.) + rows := make([]dbq.ListTrackPathsForReconcileRow, 0, 10) + seen := map[string]struct{}{} + for i := 0; i < 10; i++ { + p := fmt.Sprintf("/music/track-%02d.mp3", i) + rows = append(rows, row(byte(i), p, false)) + if i >= 2 { + seen[p] = struct{}{} + } + } + q := &fakeReconciler{rows: rows} + + var stats Stats + if err := s.reconcileMissing(context.Background(), q, seen, &stats); err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(q.marked) != 2 { + t.Fatalf("marked %d rows, want 2", len(q.marked)) + } + if q.marked[0] != testUUID(0) || q.marked[1] != testUUID(1) { + t.Errorf("marked the wrong rows: %v", q.marked) + } + if stats.Missing != 2 { + t.Errorf("stats.Missing = %d, want 2", stats.Missing) + } + if len(q.cleared) != 0 { + t.Errorf("cleared %d rows, want 0", len(q.cleared)) + } +} + +func TestReconcileMissing_ClearsRowsWhoseFileReturned(t *testing.T) { + root := populatedRoot(t) + s := testScanner(t, root) + q := &fakeReconciler{rows: []dbq.ListTrackPathsForReconcileRow{ + row(1, "/music/back.mp3", true), + row(2, "/music/still-here.mp3", false), + }} + seen := map[string]struct{}{ + "/music/back.mp3": {}, + "/music/still-here.mp3": {}, + } + + var stats Stats + if err := s.reconcileMissing(context.Background(), q, seen, &stats); err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(q.cleared) != 1 || q.cleared[0] != testUUID(1) { + t.Fatalf("cleared = %v, want just row 1", q.cleared) + } + if stats.Restored != 1 { + t.Errorf("stats.Restored = %d, want 1", stats.Restored) + } + if len(q.marked) != 0 { + t.Errorf("marked %d rows, want 0", len(q.marked)) + } +} + +// An already-marked row must not be re-marked: the timestamp is the "how long +// has this been gone" clock that any future cleanup policy depends on. +func TestReconcileMissing_DoesNotRemarkAlreadyMissingRows(t *testing.T) { + root := populatedRoot(t) + s := testScanner(t, root) + q := &fakeReconciler{rows: []dbq.ListTrackPathsForReconcileRow{ + row(1, "/music/long-gone.mp3", true), + row(2, "/music/present.mp3", false), + }} + seen := map[string]struct{}{"/music/present.mp3": {}} + + var stats Stats + if err := s.reconcileMissing(context.Background(), q, seen, &stats); err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(q.marked) != 0 { + t.Errorf("re-marked an already-missing row: %v", q.marked) + } + if len(q.cleared) != 0 { + t.Errorf("cleared = %v, want none", q.cleared) + } +} + +// The guard that matters most. A half-attached mount makes the walk succeed +// while seeing only part of the library — evidence indistinguishable from a mass +// deletion, so reconcile must refuse rather than guess. +func TestReconcileMissing_RefusesWhenTooMuchWouldBeMarked(t *testing.T) { + root := populatedRoot(t) + s := testScanner(t, root) + rows := make([]dbq.ListTrackPathsForReconcileRow, 0, 100) + seen := map[string]struct{}{} + for i := 0; i < 100; i++ { + p := fmt.Sprintf("/music/track-%03d.mp3", i) + rows = append(rows, row(byte(i), p, false)) + // Only 60 of 100 present -> 40% would be marked, over the 25% cap. + if i < 60 { + seen[p] = struct{}{} + } + } + q := &fakeReconciler{rows: rows} + + var stats Stats + err := s.reconcileMissing(context.Background(), q, seen, &stats) + if err == nil { + t.Fatal("expected reconcile to refuse, got nil error") + } + if len(q.marked) != 0 { + t.Errorf("marked %d rows despite refusing", len(q.marked)) + } + if stats.Missing != 0 { + t.Errorf("stats.Missing = %d, want 0", stats.Missing) + } +} + +// Restoring is never the dangerous direction, so it must survive the cap — +// otherwise a library that tripped the cap once could never clear its marks +// even after the volume came back. +func TestReconcileMissing_ClearsEvenWhenMarkCapTrips(t *testing.T) { + root := populatedRoot(t) + s := testScanner(t, root) + rows := []dbq.ListTrackPathsForReconcileRow{row(1, "/music/back.mp3", true)} + seen := map[string]struct{}{"/music/back.mp3": {}} + // Add enough absent rows to blow the cap. + for i := 2; i < 10; i++ { + rows = append(rows, row(byte(i), fmt.Sprintf("/music/absent-%02d.mp3", i), false)) + } + q := &fakeReconciler{rows: rows} + + var stats Stats + if err := s.reconcileMissing(context.Background(), q, seen, &stats); err == nil { + t.Fatal("expected the mark cap to trip") + } + if len(q.cleared) != 1 { + t.Errorf("cleared %d rows, want 1 — restores must not be blocked by the cap", len(q.cleared)) + } + if stats.Restored != 1 { + t.Errorf("stats.Restored = %d, want 1", stats.Restored) + } +} + +func TestReconcileMissing_RefusesOnEmptyWalk(t *testing.T) { + root := populatedRoot(t) + s := testScanner(t, root) + q := &fakeReconciler{rows: []dbq.ListTrackPathsForReconcileRow{ + row(1, "/music/a.mp3", false), + }} + + var stats Stats + if err := s.reconcileMissing(context.Background(), q, map[string]struct{}{}, &stats); err == nil { + t.Fatal("expected refusal when the walk saw no files") + } + if len(q.marked) != 0 { + t.Errorf("marked rows on an empty walk: %v", q.marked) + } +} + +// The unmounted-volume case: the configured root doesn't exist at all. +func TestReconcileMissing_RefusesWhenRootMissing(t *testing.T) { + s := testScanner(t, filepath.Join(t.TempDir(), "not-mounted")) + q := &fakeReconciler{rows: []dbq.ListTrackPathsForReconcileRow{ + row(1, "/music/a.mp3", false), + }} + + var stats Stats + if err := s.reconcileMissing(context.Background(), q, map[string]struct{}{"/x": {}}, &stats); err == nil { + t.Fatal("expected refusal when a scan root is absent") + } + if len(q.marked) != 0 { + t.Errorf("marked rows with an absent root: %v", q.marked) + } +} + +// A mount point that exists but has nothing mounted on it: os.Stat succeeds on +// the bare directory, which is why emptiness is checked separately. +func TestReconcileMissing_RefusesWhenRootEmpty(t *testing.T) { + s := testScanner(t, t.TempDir()) + q := &fakeReconciler{rows: []dbq.ListTrackPathsForReconcileRow{ + row(1, "/music/a.mp3", false), + }} + + var stats Stats + if err := s.reconcileMissing(context.Background(), q, map[string]struct{}{"/x": {}}, &stats); err == nil { + t.Fatal("expected refusal when a scan root is empty") + } +} + +// Several roots, one detached. Marking must not proceed on partial evidence just +// because the other roots looked fine. +func TestReconcileMissing_RefusesWhenAnyRootMissing(t *testing.T) { + good := populatedRoot(t) + s := testScanner(t, good, filepath.Join(t.TempDir(), "detached")) + q := &fakeReconciler{rows: []dbq.ListTrackPathsForReconcileRow{ + row(1, "/music/a.mp3", false), + }} + + var stats Stats + if err := s.reconcileMissing(context.Background(), q, map[string]struct{}{"/x": {}}, &stats); err == nil { + t.Fatal("expected refusal when one of several roots is absent") + } +} + +func TestReconcileMissing_NoRowsIsNotAnError(t *testing.T) { + root := populatedRoot(t) + s := testScanner(t, root) + q := &fakeReconciler{} + + var stats Stats + if err := s.reconcileMissing(context.Background(), q, map[string]struct{}{"/x": {}}, &stats); err != nil { + t.Fatalf("empty library should reconcile cleanly, got %v", err) + } +} + +func TestReconcileMissing_PropagatesListError(t *testing.T) { + root := populatedRoot(t) + s := testScanner(t, root) + sentinel := errors.New("boom") + q := &fakeReconciler{listErr: sentinel} + + var stats Stats + err := s.reconcileMissing(context.Background(), q, map[string]struct{}{"/x": {}}, &stats) + if !errors.Is(err, sentinel) { + t.Fatalf("err = %v, want it to wrap %v", err, sentinel) + } +} + +func TestVerifyRootsPresent_NoRootsConfigured(t *testing.T) { + s := testScanner(t) + if err := s.verifyRootsPresent(); err == nil { + t.Fatal("expected an error with no scan roots configured") + } +} diff --git a/internal/library/scanner.go b/internal/library/scanner.go index bd988986..985333c6 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -60,6 +60,11 @@ type Stats struct { Updated int `json:"updated"` Skipped int `json:"skipped"` Errored int `json:"errored"` + // Missing / Restored come from the reconcile pass, not the walk (#2523): + // rows whose file the walk didn't find, and rows whose file came back. + // Only a full Scan sets these — see reconcileMissing. + Missing int `json:"missing"` + Restored int `json:"restored"` } type Scanner struct { @@ -76,6 +81,12 @@ func New(pool *pgxpool.Pool, logger *slog.Logger, paths []string) *Scanner { // newer than the existing row's updated_at. Walk errors and per-file errors // are logged + counted; the scan keeps going. // +// It then reconciles: rows whose file the walk never saw get marked missing, +// and rows whose file has come back get un-marked (#2523). Only a FULL scan may +// do this — the walk's set of seen paths is the evidence, and a partial +// (watcher-driven) scan has no basis for concluding anything about files it +// didn't look at. That's why ScanFiles does not reconcile. +// // progressCb (may be nil) receives the current Stats snapshot after each // processed file. Used by the orchestrator to drive partial-tally writes. func (s *Scanner) Scan(ctx context.Context, progressCb func(Stats)) (Stats, error) { @@ -83,35 +94,55 @@ func (s *Scanner) Scan(ctx context.Context, progressCb func(Stats)) (Stats, erro q := dbq.New(s.pool) start := time.Now() - 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 - } - 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) + // 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 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) } } @@ -121,6 +152,8 @@ func (s *Scanner) Scan(ctx context.Context, progressCb func(Stats)) (Stats, erro "updated", stats.Updated, "skipped", stats.Skipped, "errored", stats.Errored, + "missing", stats.Missing, + "restored", stats.Restored, "duration_ms", time.Since(start).Milliseconds(), ) if err := ctx.Err(); err != nil { @@ -129,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. @@ -218,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) + } +} diff --git a/internal/library/scanrun.go b/internal/library/scanrun.go index 5f42ecb5..e907eaab 100644 --- a/internal/library/scanrun.go +++ b/internal/library/scanrun.go @@ -24,6 +24,11 @@ type LibraryStageTallies struct { Updated int `json:"updated"` Skipped int `json:"skipped"` Errored int `json:"errored"` + // Reconcile results (#2523). Surfaced in the scan record because a track + // disappearing from the library is something the operator should be able to + // see happened, rather than discovering it when a mix comes up short. + Missing int `json:"missing"` + Restored int `json:"restored"` } // MBIDBackfillStageTallies wires BackfillMBIDsResult into the scan_runs jsonb column.