From c3f3a17c6db46d3ba6872b3894b535af5eccd920 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 16 Aug 2026 11:43:05 -0400 Subject: [PATCH] =?UTF-8?q?feat(library):=20a=20missing=20file=20stays=20i?= =?UTF-8?q?n=20the=20playlist,=20greyed=20and=20unplayable=20=E2=80=94=20#?= =?UTF-8?q?2527?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every browse, discover and mix query filters missing_since, so a track whose file vanished disappears from the places Minstrel chooses music. A playlist is different: the entry is there because the user put it there, and silently dropping it rewrites their list behind their back. So playlists keep the row and mark it instead. ListPlaylistTracks now carries missing_since (still deliberately unfiltered), the service layer surfaces it as PlaylistTrack.Unavailable, and the wire gains "unavailable" on each entry. A missing entry also loses its stream_url. Refusing to hand out a URL that cannot serve is stronger than trusting every client to honour the flag, and "stream_url": null is a shape the clients already model -- PlaylistWire.streamUrl is documented nullable for the track-removed case -- so an older build degrades to "present but not playable" with no change. Nothing is deleted here and nothing should be: the row, its play history, its likes and its taste contribution all survive a file going missing, because the file may come back (and #2528 will adopt it if it comes back renamed). Also corrects two comments that had drifted into lying. delete.go still claimed the file-gone case was NOT auto-reconciled and told admins to delete rows by hand -- untrue since f6d1cf24, and that exact staleness is what produced drift #572. It now says what DeleteTrackFile really is: the destructive admin action, which CASCADEs play_events and likes, and is emphatically not the missing-file path. watcher.go claimed the safety-net scan "covers anything missed"; the walk only covers additions, and it is reconcile that covers removals. --- internal/api/playlists.go | 17 ++++++++++++-- internal/db/dbq/playlists.sql.go | 39 +++++++++++++++++++------------ internal/db/queries/playlists.sql | 13 ++++++++--- internal/library/delete.go | 23 ++++++++++++------ internal/library/watcher.go | 8 +++++++ internal/playlists/service.go | 6 +++++ 6 files changed, 79 insertions(+), 27 deletions(-) diff --git a/internal/api/playlists.go b/internal/api/playlists.go index 6efba6ee..af262bd7 100644 --- a/internal/api/playlists.go +++ b/internal/api/playlists.go @@ -60,6 +60,11 @@ type playlistTrackView struct { DurationSec int32 `json:"duration_sec"` StreamURL *string `json:"stream_url"` AddedAt string `json:"added_at"` + // Unavailable marks an entry whose file is missing from disk (#2527). + // The track is still a real, known track — its history and likes are + // intact and the file may come back — so the entry keeps its place and + // its text; clients render it greyed out and skip over it on playback. + Unavailable bool `json:"unavailable"` } // playlistDetailView extends playlistRowView with the ordered track list. @@ -472,12 +477,20 @@ func playlistDetailToView(d *playlists.PlaylistDetail) playlistDetailView { AlbumTitle: t.AlbumTitle, DurationSec: t.DurationSec, AddedAt: formatTimestamp(t.AddedAt), + Unavailable: t.Unavailable, } if t.TrackID != nil { s := uuidToString(*t.TrackID) v.TrackID = &s - url := streamURL(*t.TrackID) - v.StreamURL = &url + // No stream URL for a missing file. The wire refuses to offer + // a URL that cannot serve rather than trusting every client to + // honour Unavailable — and `"stream_url": null` is a shape the + // clients already handle (the track-removed case), so an older + // build degrades to "present but not playable" on its own. + if !t.Unavailable { + url := streamURL(*t.TrackID) + v.StreamURL = &url + } } if t.AlbumID != nil { s := uuidToString(*t.AlbumID) diff --git a/internal/db/dbq/playlists.sql.go b/internal/db/dbq/playlists.sql.go index e4a9855e..ce928088 100644 --- a/internal/db/dbq/playlists.sql.go +++ b/internal/db/dbq/playlists.sql.go @@ -262,9 +262,10 @@ func (q *Queries) ListAllPlaylistTracksForCollage(ctx context.Context, arg ListA const listPlaylistTracks = `-- name: ListPlaylistTracks :many SELECT pt.playlist_id, pt.position, pt.track_id, pt.title, pt.artist_name, pt.album_title, pt.duration_sec, pt.added_at, pt.pick_kind, - t.id AS live_track_id, - albums.id AS album_id, - artists.id AS artist_id + t.id AS live_track_id, + t.missing_since AS missing_since, + albums.id AS album_id, + artists.id AS artist_id FROM playlist_tracks pt LEFT JOIN tracks t ON t.id = pt.track_id LEFT JOIN albums ON albums.id = t.album_id @@ -274,24 +275,31 @@ ORDER BY pt.position ` type ListPlaylistTracksRow struct { - PlaylistID pgtype.UUID - Position int32 - TrackID pgtype.UUID - Title string - ArtistName string - AlbumTitle string - DurationSec int32 - AddedAt pgtype.Timestamptz - PickKind *string - LiveTrackID pgtype.UUID - AlbumID pgtype.UUID - ArtistID pgtype.UUID + PlaylistID pgtype.UUID + Position int32 + TrackID pgtype.UUID + Title string + ArtistName string + AlbumTitle string + DurationSec int32 + AddedAt pgtype.Timestamptz + PickKind *string + LiveTrackID pgtype.UUID + MissingSince pgtype.Timestamptz + AlbumID pgtype.UUID + ArtistID pgtype.UUID } // Joined to tracks for the live track id (the service layer derives the // stream URL from it); LEFT JOIN preserves the row when track_id is NULL // (track was removed from the library). The denormalized snapshot fields // on playlist_tracks remain authoritative for title/artist/album text. +// +// Deliberately NOT filtered on missing_since (#2527), unlike every browse / +// discover / mix query. A playlist entry is something the user put here on +// purpose, so a missing file stays in the list and renders as a dead row +// rather than silently vanishing; missing_since rides along so the service +// layer can mark it unplayable. func (q *Queries) ListPlaylistTracks(ctx context.Context, playlistID pgtype.UUID) ([]ListPlaylistTracksRow, error) { rows, err := q.db.Query(ctx, listPlaylistTracks, playlistID) if err != nil { @@ -312,6 +320,7 @@ func (q *Queries) ListPlaylistTracks(ctx context.Context, playlistID pgtype.UUID &i.AddedAt, &i.PickKind, &i.LiveTrackID, + &i.MissingSince, &i.AlbumID, &i.ArtistID, ); err != nil { diff --git a/internal/db/queries/playlists.sql b/internal/db/queries/playlists.sql index 058a1ffa..fa414aaa 100644 --- a/internal/db/queries/playlists.sql +++ b/internal/db/queries/playlists.sql @@ -54,10 +54,17 @@ RETURNING id, cover_path; -- stream URL from it); LEFT JOIN preserves the row when track_id is NULL -- (track was removed from the library). The denormalized snapshot fields -- on playlist_tracks remain authoritative for title/artist/album text. +-- +-- Deliberately NOT filtered on missing_since (#2527), unlike every browse / +-- discover / mix query. A playlist entry is something the user put here on +-- purpose, so a missing file stays in the list and renders as a dead row +-- rather than silently vanishing; missing_since rides along so the service +-- layer can mark it unplayable. SELECT pt.*, - t.id AS live_track_id, - albums.id AS album_id, - artists.id AS artist_id + t.id AS live_track_id, + t.missing_since AS missing_since, + albums.id AS album_id, + artists.id AS artist_id FROM playlist_tracks pt LEFT JOIN tracks t ON t.id = pt.track_id LEFT JOIN albums ON albums.id = t.album_id diff --git a/internal/library/delete.go b/internal/library/delete.go index f496871e..81f5bd58 100644 --- a/internal/library/delete.go +++ b/internal/library/delete.go @@ -28,13 +28,22 @@ var ErrTrackNotFound = errors.New("library: track not found") // 3. Delete the tracks row. // // Order matters: file first, then DB. If the file delete fails (permission, -// I/O error), we leave the DB row alone so the admin can retry. The reverse -// failure mode — file gone, DB row still present — is currently NOT -// auto-reconciled (drift #572 audit found the misleading prior claim -// that a scan would clean it up — the scanner only walks + upserts; -// it does not enumerate orphan rows). An admin must re-trigger -// DeleteTrackFile or delete the row manually. A scanrun orphan-row -// sweep is tracked as future work in the audit queue. +// I/O error), we leave the DB row alone so the admin can retry. +// +// The reverse failure mode — file gone, DB row still present — IS reconciled +// now, and not by this function: the scan's reconcile pass stamps +// tracks.missing_since (#2523), every selection path filters on it, and a file +// that returns is un-marked or adopted at its new path (#2528). That is the +// normal life of a vanished file and it is deliberately non-destructive: the +// row, its play history and its likes survive, because a missing file is a +// track Minstrel still knows about (#2527). +// +// So this function is NOT the missing-file path. It is the explicit admin +// action "remove this recording from disk and from the library", and it is +// irreversible: tracks CASCADEs to play_events, general_likes_tracks, +// contextual_likes, track_tags and playback_errors. Reach for it when the +// operator means to destroy the record, never to tidy up a row whose file +// merely went away. func DeleteTrackFile(ctx context.Context, pool *pgxpool.Pool, trackID pgtype.UUID) error { q := dbq.New(pool) track, err := q.GetTrackByID(ctx, trackID) diff --git a/internal/library/watcher.go b/internal/library/watcher.go index 94a66900..dc6946c7 100644 --- a/internal/library/watcher.go +++ b/internal/library/watcher.go @@ -55,6 +55,14 @@ func classifyEvent(op fsnotify.Op, isDir, isAudio bool) watchAction { // It is recursive: a watch is added per directory, and new directories get a // watch as they appear. inotify watch-limit exhaustion on huge libraries is // logged, not fatal -- the periodic safety-net scan covers anything missed. +// +// "Covers anything missed" is true in two different ways, worth separating +// because the walk alone only ever gave one of them: the walk re-visits every +// path that EXISTS, which catches additions and edits, and the reconcile pass +// that runs with it compares the whole tracks table against what the walk saw, +// which is what catches removals (#2523). classifyEvent still ignores fsnotify +// removals by design, so a deleted file surfaces at the next scan, not the +// next inotify event. type Watcher struct { scanner *Scanner enricher *coverart.Enricher diff --git a/internal/playlists/service.go b/internal/playlists/service.go index f919abb8..b6ae36fe 100644 --- a/internal/playlists/service.go +++ b/internal/playlists/service.go @@ -110,6 +110,11 @@ type PlaylistTrack struct { AlbumTitle string DurationSec int32 AddedAt pgtype.Timestamptz + // Unavailable reports that the track still exists in the library but + // its file is currently missing from disk (tracks.missing_since is + // set, #2527). The entry keeps its place in the playlist — the user + // put it there — but nothing should try to play it. + Unavailable bool } // Create makes a new playlist owned by userID. @@ -173,6 +178,7 @@ func (s *Service) Get(ctx context.Context, callerID, playlistID pgtype.UUID) (*P AlbumTitle: t.AlbumTitle, DurationSec: t.DurationSec, AddedAt: t.AddedAt, + Unavailable: t.MissingSince.Valid, } // pt.track_id (snapshot FK, ON DELETE SET NULL) and the joined // live_track_id should agree on validity in normal operation —