diff --git a/internal/api/admin_suspect_sources.go b/internal/api/admin_suspect_sources.go index 7e6c36d5..cca97934 100644 --- a/internal/api/admin_suspect_sources.go +++ b/internal/api/admin_suspect_sources.go @@ -1,7 +1,11 @@ package api import ( + "encoding/json" "net/http" + "time" + + "github.com/go-chi/chi/v5" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/library" @@ -22,6 +26,11 @@ type suspectTrackView struct { DiscNumber *int32 `json:"disc_number"` TrackNumber *int32 `json:"track_number"` Markers []string `json:"markers"` + // Verdict is "suspect" while the track is held back from radio and the + // mixes, "fine" once the operator let it back, null before the resolver's + // first pass (M498 #5439). VerdictAt is when it was last set. + Verdict *string `json:"verdict"` + VerdictAt *string `json:"verdict_at"` } // suspectGroupView is a folder's worth of flagged tracks. As on the @@ -42,9 +51,10 @@ type adminSuspectSourcesResponse struct { // handleListSuspectSources implements GET /api/admin/library/suspect-sources. // -// Read-only. A marker is a reason to look, not proof: a band can title a song -// "Music Video". What to do about a flagged file — merge it in Duplicates, -// quarantine it, replace it in Lidarr — stays the operator's call. +// A marker is a reason to look, not proof: a band can title a song "Music +// Video". The duplicate resolver holds a flagged track back from radio and the +// mixes and merges it away where a clean copy exists (M498 #5439); this report +// is where the operator sees that and can say a track is fine. func (h *handlers) handleListSuspectSources(w http.ResponseWriter, r *http.Request) { limit, offset, err := parsePaging(r.URL.Query()) if err != nil { @@ -96,6 +106,11 @@ func groupSuspectByDirectory(rows []dbq.ListSuspectSourceTracksRow) []suspectGro DiscNumber: row.DiscNumber, TrackNumber: row.TrackNumber, Markers: library.SourceMarkersFor(row.FilePath), + Verdict: row.SourceVerdict, + } + if row.SourceVerdictAt.Valid { + at := row.SourceVerdictAt.Time.UTC().Format(time.RFC3339) + t.VerdictAt = &at } if n := len(groups); n > 0 && groups[n-1].Directory == row.Directory { groups[n-1].Tracks = append(groups[n-1].Tracks, t) @@ -108,3 +123,39 @@ func groupSuspectByDirectory(rows []dbq.ListSuspectSourceTracksRow) []suspectGro } return groups } + +type suspectVerdictRequest struct { + Verdict string `json:"verdict"` +} + +// handleSetSuspectSourceVerdict implements PUT +// /api/admin/library/suspect-sources/{id}: {"verdict": "fine"} lets a flagged +// track back into radio and the mixes for good, {"verdict": "suspect"} holds +// it back again. 404 suspect_track_not_found when the track was never flagged. +func (h *handlers) handleSetSuspectSourceVerdict(w http.ResponseWriter, r *http.Request) { + id, ok := parseUUID(chi.URLParam(r, "id")) + if !ok { + writeAdminJSONErr(w, http.StatusBadRequest, "invalid_id") + return + } + var req suspectVerdictRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + writeAdminJSONErr(w, http.StatusBadRequest, "invalid_body") + return + } + if req.Verdict != "fine" && req.Verdict != "suspect" { + writeAdminJSONErr(w, http.StatusBadRequest, "invalid_verdict") + return + } + n, err := dbq.New(h.pool).SetTrackSourceVerdict(r.Context(), dbq.SetTrackSourceVerdictParams{Verdict: req.Verdict, ID: id}) + if err != nil { + h.logger.Error("admin: set suspect-source verdict", "err", err) + writeAdminJSONErr(w, http.StatusInternalServerError, "server_error") + return + } + if n == 0 { + writeAdminJSONErr(w, http.StatusNotFound, "suspect_track_not_found") + return + } + writeJSON(w, http.StatusOK, map[string]string{"verdict": req.Verdict}) +} diff --git a/internal/api/api.go b/internal/api/api.go index de6eef0a..1162ec25 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -253,6 +253,7 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev // not be mistaken for it (#2527). admin.Get("/library/missing", h.handleListMissingTracks) admin.Get("/library/suspect-sources", h.handleListSuspectSources) + admin.Put("/library/suspect-sources/{id}", h.handleSetSuspectSourceVerdict) admin.Get("/library/coverage", h.handleGetLibraryCoverage) admin.Get("/library/fingerprints", h.handleGetFingerprintCoverage) diff --git a/internal/db/dbq/events.sql.go b/internal/db/dbq/events.sql.go index bbd3673f..ab656653 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, t.missing_since, t.mbid_source, t.song_id, t.song_key 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, t.mbid_source, t.song_id, t.song_key, t.source_verdict, t.source_verdict_at FROM tracks t JOIN play_events pe ON pe.track_id = t.id WHERE pe.session_id = $1 AND pe.started_at < $2 @@ -310,6 +310,8 @@ func (q *Queries) ListRecentSessionTracks(ctx context.Context, arg ListRecentSes &i.MbidSource, &i.SongID, &i.SongKey, + &i.SourceVerdict, + &i.SourceVerdictAt, ); err != nil { return nil, err } diff --git a/internal/db/dbq/history.sql.go b/internal/db/dbq/history.sql.go index 7a493b16..32943d28 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.missing_since, t.mbid_source, t.song_id, t.song_key, + 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, t.mbid_source, t.song_id, t.song_key, t.source_verdict, t.source_verdict_at, albums.title AS album_title, artists.name AS artist_name FROM play_events pe @@ -84,6 +84,8 @@ func (q *Queries) ListUserHistory(ctx context.Context, arg ListUserHistoryParams &i.Track.MbidSource, &i.Track.SongID, &i.Track.SongKey, + &i.Track.SourceVerdict, + &i.Track.SourceVerdictAt, &i.AlbumTitle, &i.ArtistName, ); err != nil { diff --git a/internal/db/dbq/likes.sql.go b/internal/db/dbq/likes.sql.go index f5d73817..d7480f00 100644 --- a/internal/db/dbq/likes.sql.go +++ b/internal/db/dbq/likes.sql.go @@ -281,12 +281,13 @@ 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, t.missing_since, t.mbid_source, t.song_id, t.song_key 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, t.mbid_source, t.song_id, t.song_key, t.source_verdict, t.source_verdict_at FROM tracks t JOIN (SELECT DISTINCT ON (lt.song_key) l.track_id, l.liked_at FROM general_likes l JOIN tracks lt ON lt.id = l.track_id WHERE l.user_id = $1 - ORDER BY lt.song_key, (lt.missing_since IS NOT NULL), l.liked_at, lt.id) l + ORDER BY lt.song_key, (lt.missing_since IS NOT NULL), + (lt.source_verdict IS NOT DISTINCT FROM 'suspect'), l.liked_at, lt.id) l ON l.track_id = t.id ORDER BY l.liked_at DESC, t.id LIMIT $2 OFFSET $3 @@ -298,8 +299,8 @@ type ListLikedTrackRowsParams struct { Offset int32 } -// One row per liked song: of its copies, the one present on disk, then the -// one liked first. +// One row per liked song: of its copies, the one present on disk, then one +// that is not a held-back video rip, then the one liked first. func (q *Queries) ListLikedTrackRows(ctx context.Context, arg ListLikedTrackRowsParams) ([]Track, error) { rows, err := q.db.Query(ctx, listLikedTrackRows, arg.UserID, arg.Limit, arg.Offset) if err != nil { @@ -332,6 +333,8 @@ func (q *Queries) ListLikedTrackRows(ctx context.Context, arg ListLikedTrackRows &i.MbidSource, &i.SongID, &i.SongKey, + &i.SourceVerdict, + &i.SourceVerdictAt, ); err != nil { return nil, err } diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index 88ce634a..556c54c9 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -756,6 +756,8 @@ type Track struct { MbidSource *string SongID pgtype.UUID SongKey pgtype.UUID + SourceVerdict *string + SourceVerdictAt pgtype.Timestamptz } type TrackAcoustidLookup struct { diff --git a/internal/db/dbq/recommendation.sql.go b/internal/db/dbq/recommendation.sql.go index b0938825..442fafee 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, t.missing_since, t.mbid_source, t.song_id, t.song_key, +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, t.mbid_source, t.song_id, t.song_key, t.source_verdict, t.source_verdict_at, albums.title AS album_title, artists.name AS artist_name FROM plays p @@ -273,6 +273,8 @@ func (q *Queries) ListMostPlayedTracksForArtist(ctx context.Context, arg ListMos &i.Track.MbidSource, &i.Track.SongID, &i.Track.SongKey, + &i.Track.SourceVerdict, + &i.Track.SourceVerdictAt, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -293,7 +295,7 @@ 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, t.missing_since, t.mbid_source, t.song_id, t.song_key, +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, t.mbid_source, t.song_id, t.song_key, t.source_verdict, t.source_verdict_at, albums.title AS album_title, artists.name AS artist_name FROM plays p @@ -360,6 +362,8 @@ func (q *Queries) ListMostPlayedTracksForUser(ctx context.Context, arg ListMostP &i.Track.MbidSource, &i.Track.SongID, &i.Track.SongKey, + &i.Track.SourceVerdict, + &i.Track.SourceVerdictAt, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -699,7 +703,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.missing_since, t.mbid_source, t.song_id, t.song_key, + 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, t.mbid_source, t.song_id, t.song_key, t.source_verdict, t.source_verdict_at, (l.user_id IS NOT NULL)::bool AS is_liked, pe.last_played_at::timestamptz AS last_played_at, pe.play_count, @@ -783,6 +787,8 @@ func (q *Queries) LoadRadioCandidates(ctx context.Context, arg LoadRadioCandidat &i.Track.MbidSource, &i.Track.SongID, &i.Track.SongKey, + &i.Track.SourceVerdict, + &i.Track.SourceVerdictAt, &i.IsLiked, &i.LastPlayedAt, &i.PlayCount, @@ -931,7 +937,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.missing_since, t.mbid_source, t.song_id, t.song_key, + 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, t.mbid_source, t.song_id, t.song_key, t.source_verdict, t.source_verdict_at, (l.user_id IS NOT NULL)::bool AS is_liked, pe.last_played_at::timestamptz AS last_played_at, pe.play_count, @@ -1061,6 +1067,8 @@ func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandid &i.Track.MbidSource, &i.Track.SongID, &i.Track.SongKey, + &i.Track.SourceVerdict, + &i.Track.SourceVerdictAt, &i.IsLiked, &i.LastPlayedAt, &i.PlayCount, diff --git a/internal/db/dbq/songs.sql.go b/internal/db/dbq/songs.sql.go index fc19bd00..0820427d 100644 --- a/internal/db/dbq/songs.sql.go +++ b/internal/db/dbq/songs.sql.go @@ -46,14 +46,18 @@ func (q *Queries) LinkTracksAsSong(ctx context.Context, trackIds []pgtype.UUID) } const listTrackSongKeys = `-- name: ListTrackSongKeys :many -SELECT id, song_key FROM tracks WHERE id = ANY($1::uuid[]) +SELECT id, song_key, (source_verdict IS NOT DISTINCT FROM 'suspect')::boolean AS held_back + FROM tracks WHERE id = ANY($1::uuid[]) ` type ListTrackSongKeysRow struct { - ID pgtype.UUID - SongKey pgtype.UUID + ID pgtype.UUID + SongKey pgtype.UUID + HeldBack bool } +// What the mix writer needs to place a track: its song, and whether it is a +// video rip held back from the mixes (#5439). func (q *Queries) ListTrackSongKeys(ctx context.Context, ids []pgtype.UUID) ([]ListTrackSongKeysRow, error) { rows, err := q.db.Query(ctx, listTrackSongKeys, ids) if err != nil { @@ -63,7 +67,7 @@ func (q *Queries) ListTrackSongKeys(ctx context.Context, ids []pgtype.UUID) ([]L var items []ListTrackSongKeysRow for rows.Next() { var i ListTrackSongKeysRow - if err := rows.Scan(&i.ID, &i.SongKey); err != nil { + if err := rows.Scan(&i.ID, &i.SongKey, &i.HeldBack); err != nil { return nil, err } items = append(items, i) diff --git a/internal/db/dbq/tracks.sql.go b/internal/db/dbq/tracks.sql.go index a1210c55..6a9de912 100644 --- a/internal/db/dbq/tracks.sql.go +++ b/internal/db/dbq/tracks.sql.go @@ -39,6 +39,23 @@ func (q *Queries) AdoptTrackPath(ctx context.Context, arg AdoptTrackPathParams) return result.RowsAffected(), nil } +const clearStaleSuspectSourceTracks = `-- name: ClearStaleSuspectSourceTracks :execrows +UPDATE tracks + SET source_verdict = NULL, source_verdict_at = NULL + WHERE source_verdict = 'suspect' + AND NOT regexp_replace(file_path, '^.*/', '') ~* $1::text +` + +// A held-back track whose name no longer carries a marker (renamed, retagged +// by Lidarr) is released again. +func (q *Queries) ClearStaleSuspectSourceTracks(ctx context.Context, pattern string) (int64, error) { + result, err := q.db.Exec(ctx, clearStaleSuspectSourceTracks, pattern) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const clearTracksMissing = `-- name: ClearTracksMissing :execrows UPDATE tracks SET missing_since = NULL @@ -246,8 +263,27 @@ func (q *Queries) FindMissingTrackByMbid(ctx context.Context, mbid string) ([]Fi return items, nil } +const flagSuspectSourceTracks = `-- name: FlagSuspectSourceTracks :execrows +UPDATE tracks + SET source_verdict = 'suspect', source_verdict_at = now() + WHERE missing_since IS NULL + AND source_verdict IS NULL + AND regexp_replace(file_path, '^.*/', '') ~* $1::text +` + +// Holds back present tracks whose basename carries a video-rip marker (M498 +// #5439), the same pattern the report lists. A track the operator called fine +// keeps that verdict. +func (q *Queries) FlagSuspectSourceTracks(ctx context.Context, pattern string) (int64, error) { + result, err := q.db.Exec(ctx, flagSuspectSourceTracks, pattern) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const getTrackByID = `-- name: GetTrackByID :one -SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since, mbid_source, song_id, song_key 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, mbid_source, song_id, song_key, source_verdict, source_verdict_at FROM tracks WHERE id = $1 ` func (q *Queries) GetTrackByID(ctx context.Context, id pgtype.UUID) (Track, error) { @@ -276,12 +312,14 @@ func (q *Queries) GetTrackByID(ctx context.Context, id pgtype.UUID) (Track, erro &i.MbidSource, &i.SongID, &i.SongKey, + &i.SourceVerdict, + &i.SourceVerdictAt, ) 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, missing_since, mbid_source, song_id, song_key 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, mbid_source, song_id, song_key, source_verdict, source_verdict_at FROM tracks WHERE file_path = $1 ` func (q *Queries) GetTrackByPath(ctx context.Context, filePath string) (Track, error) { @@ -310,12 +348,14 @@ func (q *Queries) GetTrackByPath(ctx context.Context, filePath string) (Track, e &i.MbidSource, &i.SongID, &i.SongKey, + &i.SourceVerdict, + &i.SourceVerdictAt, ) 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, missing_since, mbid_source, song_id, song_key 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, mbid_source, song_id, song_key, source_verdict, source_verdict_at FROM tracks WHERE id = ANY($1::uuid[]) ` // Batched lookup used by /api/library/sync to hydrate upsert payloads @@ -352,6 +392,8 @@ func (q *Queries) GetTracksByIDs(ctx context.Context, dollar_1 []pgtype.UUID) ([ &i.MbidSource, &i.SongID, &i.SongKey, + &i.SourceVerdict, + &i.SourceVerdictAt, ); err != nil { return nil, err } @@ -364,7 +406,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, t.missing_since, t.mbid_source, t.song_id, t.song_key, +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, t.mbid_source, t.song_id, t.song_key, t.source_verdict, t.source_verdict_at, albums.title AS album_title, artists.name AS artist_name FROM tracks t @@ -427,6 +469,8 @@ func (q *Queries) ListArtistTracksForUser(ctx context.Context, arg ListArtistTra &i.Track.MbidSource, &i.Track.SongID, &i.Track.SongKey, + &i.Track.SourceVerdict, + &i.Track.SourceVerdictAt, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -527,7 +571,7 @@ func (q *Queries) ListMissingTracks(ctx context.Context, arg ListMissingTracksPa } 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, t.missing_since, t.mbid_source, t.song_id, t.song_key, +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, t.mbid_source, t.song_id, t.song_key, t.source_verdict, t.source_verdict_at, albums.title AS album_title, artists.name AS artist_name FROM tracks t @@ -587,6 +631,8 @@ func (q *Queries) ListRandomTracksForUser(ctx context.Context, arg ListRandomTra &i.Track.MbidSource, &i.Track.SongID, &i.Track.SongKey, + &i.Track.SourceVerdict, + &i.Track.SourceVerdictAt, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -611,7 +657,9 @@ SELECT t.id, albums.id AS album_id, albums.title AS album_title, artists.id AS artist_id, - artists.name AS artist_name + artists.name AS artist_name, + t.source_verdict, + t.source_verdict_at FROM tracks t JOIN albums ON albums.id = t.album_id JOIN artists ON artists.id = t.artist_id @@ -628,17 +676,19 @@ type ListSuspectSourceTracksParams struct { } type ListSuspectSourceTracksRow struct { - ID pgtype.UUID - Title string - FilePath string - Directory string - DurationMs int32 - DiscNumber *int32 - TrackNumber *int32 - AlbumID pgtype.UUID - AlbumTitle string - ArtistID pgtype.UUID - ArtistName string + ID pgtype.UUID + Title string + FilePath string + Directory string + DurationMs int32 + DiscNumber *int32 + TrackNumber *int32 + AlbumID pgtype.UUID + AlbumTitle string + ArtistID pgtype.UUID + ArtistName string + SourceVerdict *string + SourceVerdictAt pgtype.Timestamptz } // The admin report of present tracks whose filename looks like a video rip @@ -672,6 +722,8 @@ func (q *Queries) ListSuspectSourceTracks(ctx context.Context, arg ListSuspectSo &i.AlbumTitle, &i.ArtistID, &i.ArtistName, + &i.SourceVerdict, + &i.SourceVerdictAt, ); err != nil { return nil, err } @@ -719,7 +771,7 @@ func (q *Queries) ListTrackPathsForReconcile(ctx context.Context) ([]ListTrackPa } 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, missing_since, mbid_source, song_id, song_key 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, mbid_source, song_id, song_key, source_verdict, source_verdict_at FROM tracks WHERE album_id = $1 AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q @@ -768,6 +820,8 @@ func (q *Queries) ListTracksByAlbum(ctx context.Context, arg ListTracksByAlbumPa &i.MbidSource, &i.SongID, &i.SongKey, + &i.SourceVerdict, + &i.SourceVerdictAt, ); err != nil { return nil, err } @@ -841,7 +895,7 @@ func (q *Queries) MarkTracksMissing(ctx context.Context, ids []pgtype.UUID) (int } 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, missing_since, mbid_source, song_id, song_key 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, mbid_source, song_id, song_key, source_verdict, source_verdict_at FROM tracks WHERE title ILIKE '%' || $1::text || '%' AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q @@ -897,6 +951,8 @@ func (q *Queries) SearchTracks(ctx context.Context, arg SearchTracksParams) ([]T &i.MbidSource, &i.SongID, &i.SongKey, + &i.SourceVerdict, + &i.SourceVerdictAt, ); err != nil { return nil, err } @@ -926,6 +982,28 @@ func (q *Queries) SetTrackMbidIfNull(ctx context.Context, arg SetTrackMbidIfNull return err } +const setTrackSourceVerdict = `-- name: SetTrackSourceVerdict :execrows +UPDATE tracks + SET source_verdict = $1::text, source_verdict_at = now() + WHERE id = $2 + AND source_verdict IS NOT NULL +` + +type SetTrackSourceVerdictParams struct { + Verdict string + ID pgtype.UUID +} + +// The operator's call on a flagged track: 'fine' lets it back into radio and +// the mixes for good; 'suspect' holds it back again. +func (q *Queries) SetTrackSourceVerdict(ctx context.Context, arg SetTrackSourceVerdictParams) (int64, error) { + result, err := q.db.Exec(ctx, setTrackSourceVerdict, arg.Verdict, arg.ID) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const upsertTrack = `-- name: UpsertTrack :one INSERT INTO tracks ( title, album_id, artist_id, track_number, disc_number, @@ -953,7 +1031,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, missing_since, mbid_source, song_id, song_key +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, mbid_source, song_id, song_key, source_verdict, source_verdict_at ` type UpsertTrackParams struct { @@ -1015,6 +1093,8 @@ func (q *Queries) UpsertTrack(ctx context.Context, arg UpsertTrackParams) (Track &i.MbidSource, &i.SongID, &i.SongKey, + &i.SourceVerdict, + &i.SourceVerdictAt, ) return i, err } diff --git a/internal/db/migrations/0077_track_source_verdict.down.sql b/internal/db/migrations/0077_track_source_verdict.down.sql new file mode 100644 index 00000000..17b626c6 --- /dev/null +++ b/internal/db/migrations/0077_track_source_verdict.down.sql @@ -0,0 +1,3 @@ +DROP INDEX IF EXISTS tracks_source_suspect_idx; +ALTER TABLE tracks DROP COLUMN IF EXISTS source_verdict_at; +ALTER TABLE tracks DROP COLUMN IF EXISTS source_verdict; diff --git a/internal/db/migrations/0077_track_source_verdict.up.sql b/internal/db/migrations/0077_track_source_verdict.up.sql new file mode 100644 index 00000000..c1361d5f --- /dev/null +++ b/internal/db/migrations/0077_track_source_verdict.up.sql @@ -0,0 +1,15 @@ +-- 0077_track_source_verdict.up.sql — video rips are held back from mixes and +-- radio until the operator says otherwise (Scribe milestone #498, #5439; +-- operator decision D-d). +-- +-- A file whose name carries a video-rip marker ("(Official Video)", "[HD]", +-- #5410) holds the audio of an upload, which can carry intros, skits or a +-- different take. The duplicate resolver sets 'suspect' on such a present +-- track each pass, and clears it when the name no longer matches. 'suspect' +-- keeps the track out of radio and the system mixes; it still plays when +-- chosen. 'fine' is the operator's "this one is fine" and is never +-- overwritten by the resolver. +ALTER TABLE tracks ADD COLUMN source_verdict text + CONSTRAINT tracks_source_verdict_check CHECK (source_verdict IN ('suspect', 'fine')); +ALTER TABLE tracks ADD COLUMN source_verdict_at timestamptz; +CREATE INDEX tracks_source_suspect_idx ON tracks (id) WHERE source_verdict = 'suspect'; diff --git a/internal/db/queries/likes.sql b/internal/db/queries/likes.sql index eedabc3b..350b7f59 100644 --- a/internal/db/queries/likes.sql +++ b/internal/db/queries/likes.sql @@ -20,14 +20,15 @@ DELETE FROM general_likes RETURNING track_id; -- name: ListLikedTrackRows :many --- One row per liked song: of its copies, the one present on disk, then the --- one liked first. +-- One row per liked song: of its copies, the one present on disk, then one +-- that is not a held-back video rip, then the one liked first. SELECT t.* FROM tracks t JOIN (SELECT DISTINCT ON (lt.song_key) l.track_id, l.liked_at FROM general_likes l JOIN tracks lt ON lt.id = l.track_id WHERE l.user_id = $1 - ORDER BY lt.song_key, (lt.missing_since IS NOT NULL), l.liked_at, lt.id) l + ORDER BY lt.song_key, (lt.missing_since IS NOT NULL), + (lt.source_verdict IS NOT DISTINCT FROM 'suspect'), l.liked_at, lt.id) l ON l.track_id = t.id ORDER BY l.liked_at DESC, t.id LIMIT $2 OFFSET $3; diff --git a/internal/db/queries/songs.sql b/internal/db/queries/songs.sql index d52ba873..bc3e200a 100644 --- a/internal/db/queries/songs.sql +++ b/internal/db/queries/songs.sql @@ -40,7 +40,10 @@ UPDATE tracks SET song_id = NULL AND id IN (SELECT track_id FROM duplicate_group_members WHERE group_id = sqlc.arg(group_id)); -- name: ListTrackSongKeys :many -SELECT id, song_key FROM tracks WHERE id = ANY(sqlc.arg(ids)::uuid[]); +-- What the mix writer needs to place a track: its song, and whether it is a +-- video rip held back from the mixes (#5439). +SELECT id, song_key, (source_verdict IS NOT DISTINCT FROM 'suspect')::boolean AS held_back + FROM tracks WHERE id = ANY(sqlc.arg(ids)::uuid[]); -- name: MergeInheritSongLink :exec -- A merge keeps the removed copy's song link: when the copy being removed was diff --git a/internal/db/queries/tracks.sql b/internal/db/queries/tracks.sql index 23e1b603..b8e7d9d3 100644 --- a/internal/db/queries/tracks.sql +++ b/internal/db/queries/tracks.sql @@ -285,7 +285,9 @@ SELECT t.id, albums.id AS album_id, albums.title AS album_title, artists.id AS artist_id, - artists.name AS artist_name + artists.name AS artist_name, + t.source_verdict, + t.source_verdict_at FROM tracks t JOIN albums ON albums.id = t.album_id JOIN artists ON artists.id = t.artist_id @@ -299,3 +301,29 @@ SELECT t.id, SELECT COUNT(*) FROM tracks t WHERE t.missing_since IS NULL AND regexp_replace(t.file_path, '^.*/', '') ~* sqlc.arg(pattern)::text; + +-- name: FlagSuspectSourceTracks :execrows +-- Holds back present tracks whose basename carries a video-rip marker (M498 +-- #5439), the same pattern the report lists. A track the operator called fine +-- keeps that verdict. +UPDATE tracks + SET source_verdict = 'suspect', source_verdict_at = now() + WHERE missing_since IS NULL + AND source_verdict IS NULL + AND regexp_replace(file_path, '^.*/', '') ~* sqlc.arg(pattern)::text; + +-- name: ClearStaleSuspectSourceTracks :execrows +-- A held-back track whose name no longer carries a marker (renamed, retagged +-- by Lidarr) is released again. +UPDATE tracks + SET source_verdict = NULL, source_verdict_at = NULL + WHERE source_verdict = 'suspect' + AND NOT regexp_replace(file_path, '^.*/', '') ~* sqlc.arg(pattern)::text; + +-- name: SetTrackSourceVerdict :execrows +-- The operator's call on a flagged track: 'fine' lets it back into radio and +-- the mixes for good; 'suspect' holds it back again. +UPDATE tracks + SET source_verdict = sqlc.arg(verdict)::text, source_verdict_at = now() + WHERE id = sqlc.arg(id) + AND source_verdict IS NOT NULL; diff --git a/internal/library/duplicate_resolve.go b/internal/library/duplicate_resolve.go index c16520ab..5dd4b9ee 100644 --- a/internal/library/duplicate_resolve.go +++ b/internal/library/duplicate_resolve.go @@ -91,6 +91,9 @@ type DuplicateResolveResult struct { ReleaseChanges []ReleaseChange // SongsLinked counts cross-release groups newly linked as one song. SongsLinked int + // HeldBack counts tracks newly held back from radio and the mixes as video + // rips; Released counts held-back tracks whose name no longer says so. + HeldBack, Released int64 } // LidarrPathKey is the part of a path Minstrel and Lidarr agree on: the last @@ -115,6 +118,35 @@ type resolveGroup struct { states []string // per member: tracked, unmapped, or "" when Lidarr was not asked } +// autoMergeable says whether the resolver may merge the group on its own: when +// every copy it would remove is one Lidarr does not map (D-a rule 1). On one +// album that is any group with at most one mapped copy. Across releases each +// mapped copy fulfils its own release (D-b), so only a group with exactly one +// mapped copy qualifies: the others fulfil nothing — a rip beside the clean +// copy on another release, or a wrong-file import Lidarr holds unmapped +// (#5439) — and the mapped copy is the clear one to keep. +func autoMergeable(g *resolveGroup) bool { + switch g.class { + case ClassSameRelease: + return g.tracked() <= 1 + case ClassCrossRelease, ClassMismatch: + return g.tracked() == 1 + default: + return false + } +} + +// settling says whether any of the group's albums had its Lidarr release +// changed recently, so Lidarr may still be remapping its files. +func (g *resolveGroup) settling(albums map[string]bool) bool { + for _, m := range g.members { + if albums[syncpkg.FormatUUID(m.AlbumID)] { + return true + } + } + return false +} + func (g *resolveGroup) tracked() int { n := 0 for _, s := range g.states { @@ -183,7 +215,14 @@ func ResolveDuplicates( return res, nil } - // Linking removes nothing and needs nothing from Lidarr. + // Holding rips back and linking songs remove nothing and need nothing from + // Lidarr. + if res.HeldBack, err = q.FlagSuspectSourceTracks(ctx, SuspectSourcePattern); err != nil { + return res, fmt.Errorf("hold back video rips: %w", err) + } + if res.Released, err = q.ClearStaleSuspectSourceTracks(ctx, SuspectSourcePattern); err != nil { + return res, fmt.Errorf("release renamed tracks: %w", err) + } linked, err := linkCrossRelease(ctx, pool, logger, groups) res.SongsLinked = linked if err != nil { @@ -208,7 +247,7 @@ func ResolveDuplicates( if ctx.Err() != nil { return res, ctx.Err() } - if g.class != ClassSameRelease || g.tracked() > 1 || settling[syncpkg.FormatUUID(g.members[0].AlbumID)] { + if !autoMergeable(g) || g.settling(settling) { continue } if err := autoMerge(ctx, pool, logger, dataDir, g, removable); err != nil { @@ -747,5 +786,6 @@ func (w *DuplicateResolveWorker) tickOnce(ctx context.Context) { } w.logger.Info("duplicate resolve complete", "groups", res.Groups, "lidarr", res.LidarrConsulted, "merged", res.Merged, - "merge_failed", res.MergeFailed, "release_changes", len(res.ReleaseChanges), "songs_linked", res.SongsLinked) + "merge_failed", res.MergeFailed, "release_changes", len(res.ReleaseChanges), "songs_linked", res.SongsLinked, + "held_back", res.HeldBack, "released", res.Released) } diff --git a/internal/library/duplicate_resolve_test.go b/internal/library/duplicate_resolve_test.go index 6570a961..d1b67a83 100644 --- a/internal/library/duplicate_resolve_test.go +++ b/internal/library/duplicate_resolve_test.go @@ -347,3 +347,158 @@ func TestMergeDuplicateGroupGuarded_RefusesATrackedCopy_Integration(t *testing.T t.Errorf("group status = %s, want pending", status) } } + +// crossReleaseRipFixture is a video rip on one release with the clean copy on +// another: the album cut, and the same song's video filed as a single. +type crossReleaseRipFixture struct { + pool *pgxpool.Pool + clean, rip dbq.Track + cleanPath, ripPath string + groupID pgtype.UUID +} + +func newCrossReleaseRipFixture(t *testing.T) crossReleaseRipFixture { + t.Helper() + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + root := t.TempDir() + f := crossReleaseRipFixture{pool: pool} + f.cleanPath = filepath.Join(root, "Gorillaz", "Demon Days (2005)", "06 - Feel Good Inc.mp3") + f.ripPath = filepath.Join(root, "Gorillaz", "Feel Good Inc (2005)", "01 - Feel Good Inc. (Official Video).mp3") + artist, err := q.UpsertArtist(ctx, dbq.UpsertArtistParams{Name: "Gorillaz", SortName: "Gorillaz"}) + if err != nil { + t.Fatal(err) + } + track := func(albumTitle, title, path string) dbq.Track { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("audio"), 0o644); err != nil { + t.Fatal(err) + } + album, err := q.UpsertAlbum(ctx, dbq.UpsertAlbumParams{Title: albumTitle, SortTitle: albumTitle, ArtistID: artist.ID}) + if err != nil { + t.Fatal(err) + } + tr, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ + Title: title, AlbumID: album.ID, ArtistID: artist.ID, + DurationMs: 1000, FilePath: path, FileSize: 100, FileFormat: "mp3", + }) + if err != nil { + t.Fatal(err) + } + return tr + } + f.clean = track("Demon Days", "Feel Good Inc.", f.cleanPath) + f.rip = track("Feel Good Inc.", "Feel Good Inc. (Official Video)", f.ripPath) + if err := pool.QueryRow(ctx, + `INSERT INTO duplicate_groups (member_key, tier) VALUES ('cross-rip', 'acoustic') RETURNING id`, + ).Scan(&f.groupID); err != nil { + t.Fatal(err) + } + if _, err := pool.Exec(ctx, + `INSERT INTO duplicate_group_members (group_id, track_id) VALUES ($1, $2), ($1, $3)`, + f.groupID, f.clean.ID, f.rip.ID); err != nil { + t.Fatal(err) + } + return f +} + +// The rip fulfils nothing in Lidarr while the clean copy on the album does: +// the rip goes and the album copy stays (#5439, D-a rule 1). +func TestResolveDuplicates_MergesAnUnmappedRipAcrossReleases_Integration(t *testing.T) { + f := newCrossReleaseRipFixture(t) + lid := &fakeLidarr{unmapped: []string{lidarrPath(f.ripPath)}} + res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", lid, true) + if err != nil { + t.Fatal(err) + } + if res.Classes[ClassCrossRelease] != 1 || res.Merged != 1 { + t.Fatalf("classes %v, merged %d; want the cross-release group merged", res.Classes, res.Merged) + } + if exists(f.ripPath) || !exists(f.cleanPath) { + t.Errorf("rip exists %v, clean exists %v; want only the clean copy", exists(f.ripPath), exists(f.cleanPath)) + } +} + +// When Lidarr maps both copies, each fulfils its own release: nothing is +// removed, and the copies are linked as one song instead. +func TestResolveDuplicates_KeepsCopiesLidarrMapsOnTwoReleases_Integration(t *testing.T) { + f := newCrossReleaseRipFixture(t) + res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", &fakeLidarr{}, true) + if err != nil { + t.Fatal(err) + } + if res.Merged != 0 || !exists(f.ripPath) { + t.Errorf("merged %d; want both copies kept", res.Merged) + } + if res.SongsLinked != 1 { + t.Errorf("linked %d, want the pair linked as one song", res.SongsLinked) + } +} + +// A rip is held back from radio and the mixes; the operator's "fine" sticks +// through later passes; a renamed file is released. +func TestResolveDuplicates_HoldsBackRips_Integration(t *testing.T) { + f := newCrossReleaseRipFixture(t) + ctx := context.Background() + q := dbq.New(f.pool) + verdict := func(id pgtype.UUID) *string { + t.Helper() + var v *string + if err := f.pool.QueryRow(ctx, `SELECT source_verdict FROM tracks WHERE id = $1`, id).Scan(&v); err != nil { + t.Fatal(err) + } + return v + } + resolve := func() DuplicateResolveResult { + t.Helper() + res, err := ResolveDuplicates(ctx, f.pool, nil, "", &fakeLidarr{}, true) + if err != nil { + t.Fatal(err) + } + return res + } + + if res := resolve(); res.HeldBack != 1 { + t.Errorf("held back %d, want the rip", res.HeldBack) + } + if v := verdict(f.rip.ID); v == nil || *v != "suspect" { + t.Errorf("rip verdict %v, want suspect", v) + } + if v := verdict(f.clean.ID); v != nil { + t.Errorf("clean copy verdict %v, want none", *v) + } + + if n, err := q.SetTrackSourceVerdict(ctx, dbq.SetTrackSourceVerdictParams{Verdict: "fine", ID: f.rip.ID}); err != nil || n != 1 { + t.Fatalf("set fine: %d, %v", n, err) + } + if res := resolve(); res.HeldBack != 0 { + t.Errorf("held back %d after the operator's fine, want 0", res.HeldBack) + } + if v := verdict(f.rip.ID); v == nil || *v != "fine" { + t.Errorf("rip verdict %v, want fine kept", v) + } + + // Held back again, then renamed: the next pass lets it go. + if _, err := q.SetTrackSourceVerdict(ctx, dbq.SetTrackSourceVerdictParams{Verdict: "suspect", ID: f.rip.ID}); err != nil { + t.Fatal(err) + } + if _, err := f.pool.Exec(ctx, `UPDATE tracks SET file_path = $2 WHERE id = $1`, + f.rip.ID, filepath.Join(filepath.Dir(f.ripPath), "01 - Feel Good Inc.mp3")); err != nil { + t.Fatal(err) + } + if res := resolve(); res.Released != 1 { + t.Errorf("released %d, want the renamed track", res.Released) + } + if v := verdict(f.rip.ID); v != nil { + t.Errorf("renamed track verdict %v, want none", *v) + } + + // The clean copy can't be flagged by hand: there is nothing to judge. + if n, err := q.SetTrackSourceVerdict(ctx, dbq.SetTrackSourceVerdictParams{Verdict: "fine", ID: f.clean.ID}); err != nil || n != 0 { + t.Errorf("set on an unflagged track: %d, %v; want 0 rows", n, err) + } +} diff --git a/internal/playlists/song_copies_test.go b/internal/playlists/song_copies_test.go new file mode 100644 index 00000000..68f52d4f --- /dev/null +++ b/internal/playlists/song_copies_test.go @@ -0,0 +1,49 @@ +package playlists + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5/pgtype" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// A mix holds a song once (M498 #5438) and never a held-back video rip +// (#5439): of a single and its album cut, the better-ranked copy stays; a rip +// is left out and the next copy of its song takes the place. +func TestOneCopyPerSong_Integration(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + single := seedTrack(t, pool, "copies-single", "copies-artist") + albumCut := seedTrack(t, pool, "copies-album-cut", "copies-artist") + rip := seedTrack(t, pool, "copies-rip", "copies-artist") + clean := seedTrack(t, pool, "copies-clean", "copies-artist") + other := seedTrack(t, pool, "copies-other", "copies-artist") + for _, pair := range [][]pgtype.UUID{{single.ID, albumCut.ID}, {rip.ID, clean.ID}} { + if _, err := q.LinkTracksAsSong(ctx, pair); err != nil { + t.Fatal(err) + } + } + if _, err := pool.Exec(ctx, `UPDATE tracks SET source_verdict = 'suspect' WHERE id = $1`, rip.ID); err != nil { + t.Fatal(err) + } + + in := []rankedCandidate{ + {TrackID: single.ID}, {TrackID: rip.ID}, {TrackID: albumCut.ID}, {TrackID: other.ID}, {TrackID: clean.ID}, + } + out, err := oneCopyPerSong(ctx, q, in) + if err != nil { + t.Fatal(err) + } + want := []pgtype.UUID{single.ID, other.ID, clean.ID} + if len(out) != len(want) { + t.Fatalf("got %d tracks, want %d", len(out), len(want)) + } + for i, w := range want { + if out[i].TrackID != w { + t.Errorf("position %d = %s, want %s", i, uuidString(out[i].TrackID), uuidString(w)) + } + } +} diff --git a/internal/playlists/system.go b/internal/playlists/system.go index f670977c..807d38f8 100644 --- a/internal/playlists/system.go +++ b/internal/playlists/system.go @@ -1101,7 +1101,9 @@ func insertSystemPlaylist(ctx context.Context, qtx *dbq.Queries, userID pgtype.U } // oneCopyPerSong keeps the first of a song's copies on different releases -// (M498 #5438): a mix holds the song once, in its best-ranked place. +// (M498 #5438): a mix holds the song once, in its best-ranked place. A video +// rip held back from the mixes (#5439) is left out, and a clean copy of the +// same song can take its place. func oneCopyPerSong(ctx context.Context, qtx *dbq.Queries, tracks []rankedCandidate) ([]rankedCandidate, error) { ids := make([]pgtype.UUID, len(tracks)) for i, t := range tracks { @@ -1111,14 +1113,18 @@ func oneCopyPerSong(ctx context.Context, qtx *dbq.Queries, tracks []rankedCandid if err != nil { return nil, fmt.Errorf("song keys: %w", err) } - songOf := make(map[pgtype.UUID]pgtype.UUID, len(rows)) + songOf := make(map[pgtype.UUID]dbq.ListTrackSongKeysRow, len(rows)) for _, r := range rows { - songOf[r.ID] = r.SongKey + songOf[r.ID] = r } seen := make(map[pgtype.UUID]bool, len(tracks)) out := tracks[:0:0] for _, t := range tracks { - song, ok := songOf[t.TrackID] + row, ok := songOf[t.TrackID] + if ok && row.HeldBack { + continue + } + song := row.SongKey if ok && seen[song] { continue } diff --git a/internal/recommendation/shuffle.go b/internal/recommendation/shuffle.go index dd33b6d5..91173c22 100644 --- a/internal/recommendation/shuffle.go +++ b/internal/recommendation/shuffle.go @@ -52,6 +52,12 @@ func RadioDiversityCaps(limit int) DiversityCaps { return DiversityCaps{MaxPerArtist: artist, MaxPerAlbum: album} } +// heldBack says whether the candidate is a video rip held back from radio +// (M498 #5439): it plays when chosen, but radio does not choose it. +func heldBack(c Candidate) bool { + return c.Track.SourceVerdict != nil && *c.Track.SourceVerdict == "suspect" +} + // Shuffle scores each candidate, sorts descending by score, and returns the // top `limit` candidates, preferring artist/album diversity. limit <= 0 // returns nil; nil input returns nil. Pure — no IO, no global state beyond @@ -61,7 +67,9 @@ func RadioDiversityCaps(limit int) DiversityCaps { // first takes candidates that fit under the caps, and the second fills any // remaining slots from those the first pass skipped, still in score order. // So the result holds min(limit, len(candidates)) either way — the caps -// change WHICH tracks are chosen, never HOW MANY. +// change WHICH tracks are chosen, never HOW MANY. (What is not a candidate at +// all is dropped first: a held-back video rip, and a second copy of a song +// already chosen from another release, M498.) // // That two-pass shape is the whole design, and a hard cap would have been // the easy mistake. Radio asks for 50 tracks by default and 200 at most; a @@ -119,7 +127,7 @@ func Shuffle( if len(out) == limit { break } - if repeat(s.c) { + if heldBack(s.c) || repeat(s.c) { continue } overArtist := caps.MaxPerArtist > 0 && artistCount[s.c.Track.ArtistID] >= caps.MaxPerArtist diff --git a/internal/recommendation/shuffle_test.go b/internal/recommendation/shuffle_test.go index 3406da13..9e36fe22 100644 --- a/internal/recommendation/shuffle_test.go +++ b/internal/recommendation/shuffle_test.go @@ -111,3 +111,18 @@ func TestShuffle_OneCopyPerSong(t *testing.T) { t.Errorf("got %v, want two tracks: one copy of the song", titles(out)) } } + +// A video rip held back from radio (M498 #5439) is never chosen, even to fill +// the request. +func TestShuffle_SkipsHeldBackRips(t *testing.T) { + suspect := "suspect" + fine := "fine" + rip := cand("000000000001", ScoringInputs{IsGeneralLiked: true}) + rip.Track.SourceVerdict = &suspect + cleared := cand("000000000002", ScoringInputs{}) + cleared.Track.SourceVerdict = &fine + out := Shuffle([]Candidate{rip, cleared}, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{}) + if got := titles(out); len(got) != 1 || got[0] != "000000000002" { + t.Errorf("got %v, want only the track marked fine", got) + } +} diff --git a/web/src/lib/api/admin.ts b/web/src/lib/api/admin.ts index 06a80e30..77e1a3b7 100644 --- a/web/src/lib/api/admin.ts +++ b/web/src/lib/api/admin.ts @@ -5,6 +5,7 @@ import type { ActionResult, AdminMissingResponse, AdminSuspectResponse, + SuspectVerdict, AdminDuplicatesResponse, DuplicatesView, MergeDuplicateResult, @@ -961,6 +962,12 @@ export function createSuspectSourcesQuery() { }); } +// 'fine' lets a flagged track back into radio and the mixes; 'suspect' holds +// it back again (M498 #5439). +export async function setSuspectVerdict(trackId: string, verdict: SuspectVerdict): Promise { + await api.put(`/api/admin/library/suspect-sources/${encodeURIComponent(trackId)}`, { verdict }); +} + // Missing-file re-acquisition (#2527 / milestone #290) ---------------------- export type ReacquisitionSettings = { diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index e18795eb..f752b91e 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -439,8 +439,14 @@ export type AdminSuspectTrack = { disc_number: number | null; track_number: number | null; markers: string[]; + // 'suspect': held back from radio and the mixes. 'fine': the operator let it + // back. null: the resolver hasn't looked yet (M498 #5439). + verdict: SuspectVerdict | null; + verdict_at: string | null; }; +export type SuspectVerdict = 'suspect' | 'fine'; + // A folder's worth of flagged tracks; the server orders and groups by folder. export type AdminSuspectGroup = { directory: string; diff --git a/web/src/lib/components/FingerprintSettingsCard.svelte b/web/src/lib/components/FingerprintSettingsCard.svelte index b4f49557..8bc410e5 100644 --- a/web/src/lib/components/FingerprintSettingsCard.svelte +++ b/web/src/lib/components/FingerprintSettingsCard.svelte @@ -121,8 +121,9 @@ Resolve duplicates automatically Merges copies Lidarr doesn't use into the one it does, and moves a Lidarr album off a - release that lists songs twice. A copy Lidarr uses is never removed. Off, every group - waits here for you. + release that lists songs twice. A song kept on several releases counts as one song, and + video rips are kept out of radio and the mixes. A copy Lidarr uses is never removed. + Off, every group waits here for you. diff --git a/web/src/lib/styles/error-copy.json b/web/src/lib/styles/error-copy.json index 5a02f580..2af87301 100644 --- a/web/src/lib/styles/error-copy.json +++ b/web/src/lib/styles/error-copy.json @@ -48,6 +48,8 @@ "survivor_not_in_group": "That copy isn't part of this group any more.", "copy_tracked_by_lidarr": "Lidarr uses a copy this would remove, and would download it again. Keep that copy instead.", "lidarr_unavailable": "Lidarr didn't answer, so it isn't safe to remove a copy yet. Try again shortly.", + "suspect_track_not_found": "That track isn't flagged any more. Refresh the list.", + "invalid_verdict": "That isn't a choice for a flagged track.", "invalid_setting": "That setting is out of range.", "invalid_public_url": "That address isn't valid.", "album_not_found": "That album no longer exists.", diff --git a/web/src/routes/admin/suspect-sources/+page.svelte b/web/src/routes/admin/suspect-sources/+page.svelte index dc57a7df..101362fb 100644 --- a/web/src/routes/admin/suspect-sources/+page.svelte +++ b/web/src/routes/admin/suspect-sources/+page.svelte @@ -1,17 +1,23 @@