From eff3d8893171f0d855a37669a2e5e97f71c9055f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 11 Sep 2026 08:44:33 -0400 Subject: [PATCH 1/5] fix(recommendation): make the candidate draw reproducible, not accidentally so MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four arms of the candidate query ended in a bare `ORDER BY random()` with no seed: similar_artists, likes_overlap, coplay_artists and random_fill. Such an arm returns a STABLE set only while its LIMIT exceeds the rows eligible for it — at that point it returns all of them and the order stops mattering, because scoreAndSortCandidates sorts by track id before drawing jitter. Below that threshold it returns a random SUBSET, and two builds on the same day draw different ones. So daily determinism held BY ACCIDENT, and only for libraries smaller than the limits. Any real library is larger, which means same-day rebuilds have been producing different mixes since those arms were written — invisible, because a mix that changes after a refresh looks like a feature rather than a broken promise. Found by breaking it: cutting RandomFill to 10 while tuning Songs-like turned TestBuildSystemPlaylists_DailyNonceDeterminism red. That test seeds ~20 tracks against a default RandomFill of 30, so its determinism came from the limit exceeding the library, not from the code being right. It is now a real guard. The arms order by md5(id || $12) instead. The CALLER decides what that means, which is the point: system mixes pass a per-(user, day) seed and get the determinism they promise, radio passes a fresh value per request and keeps varying, which is what a radio should do. Same shape the browse queries in this file already use (`md5(id::text || current_date::text)`) — existing idiom, not a new one. This also unblocks the trim that #3881 wanted and could not have. Shrinking a randomly-ordered arm was what broke membership; a seeded one takes a smaller but REPRODUCIBLE slice. Songs-like's seed-independent share drops from 29% to 12%, which was the original intent before determinism forced it back to 20%. TestSongsLikeLimits_DoNotShrinkTheUnseededRandomArms is DELETED rather than kept passing. It existed to stop anyone trimming those arms while the ordering was broken; the ordering is fixed, so the constraint is gone and a guard enforcing it would now forbid correct code. Was filed as blocked on tooling. It was not: `make generate-go` runs sqlc as a pinned Go tool and is the same path CI takes. One thing worth knowing for next time: three files in internal/db/dbq are owned by root, left by `make generate` running sqlc in Docker. sqlc errored on the first it could not write. They are untouched by this change and the regeneration of recommendation.sql.go completed, but `make generate` will keep failing until they are chowned. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- internal/api/radio.go | 4 + internal/db/dbq/recommendation.sql.go | 48 ++++--- internal/db/queries/recommendation.sql | 25 +++- internal/playlists/system.go | 13 ++ internal/playlists/you_might_like.go | 1 + internal/recommendation/candidates.go | 68 +++++----- internal/recommendation/candidates_v2_test.go | 120 ++++++++++++++++-- .../recommendation/songs_like_limits_test.go | 38 ------ 8 files changed, 216 insertions(+), 101 deletions(-) diff --git a/internal/api/radio.go b/internal/api/radio.go index 218dadd8..d8558bb8 100644 --- a/internal/api/radio.go +++ b/internal/api/radio.go @@ -101,6 +101,10 @@ func (h *handlers) handleRadio(w http.ResponseWriter, r *http.Request) { candidates, err := recommendation.LoadCandidatesFromSimilarity( r.Context(), q, user.ID, seedID, h.recCfg.RecentlyPlayedHours, currentVec, exclude, limits, + // A fresh seed per request (#3889): radio is a new session each time + // and SHOULD draw differently. The system mixes are the surfaces that + // promise repeatability; this is not one of them. + strconv.FormatInt(time.Now().UnixNano(), 36), ) if err != nil { h.logger.Warn("api: radio: similarity-pool failed; falling back to whole-library", "err", err) diff --git a/internal/db/dbq/recommendation.sql.go b/internal/db/dbq/recommendation.sql.go index f290ec35..bd798644 100644 --- a/internal/db/dbq/recommendation.sql.go +++ b/internal/db/dbq/recommendation.sql.go @@ -829,7 +829,7 @@ similar_artists AS ( JOIN seed_artist sa ON asim.artist_a_id = sa.artist_id WHERE asim.source = 'listenbrainz' AND t.id NOT IN (SELECT id FROM excluded_ids) - ORDER BY asim.score DESC, random() + ORDER BY asim.score DESC, md5(t.id::text || $12::text) LIMIT $6 ), tag_overlap AS ( @@ -857,7 +857,7 @@ likes_overlap AS ( WHERE t.id = gl.track_id AND trim(g_overlap.g) IN (SELECT tag FROM seed_tags) ) - ORDER BY random() + ORDER BY md5(gl.track_id::text || $12::text) LIMIT $8 ), taste_overlap AS ( @@ -884,7 +884,7 @@ coplay_artists AS ( WHERE asim.source = 'user_cooccurrence' AND t.id NOT IN (SELECT id FROM excluded_ids) AND t.id <> $2 - ORDER BY asim.score DESC, random() + ORDER BY asim.score DESC, md5(t.id::text || $12::text) LIMIT $11 ), random_fill AS ( @@ -900,7 +900,7 @@ random_fill AS ( UNION SELECT track_id FROM taste_overlap UNION SELECT track_id FROM coplay_artists ) - ORDER BY random() + ORDER BY md5(t.id::text || $12::text) LIMIT $9 ) SELECT @@ -938,17 +938,18 @@ GROUP BY t.id, t.title, t.album_id, t.artist_id, t.duration_ms, t.file_path, ` type LoadRadioCandidatesV2Params struct { - UserID pgtype.UUID - ID pgtype.UUID - Column3 interface{} - Column4 []pgtype.UUID - Limit int32 - Limit_2 int32 - Limit_3 int32 - Limit_4 int32 - Limit_5 int32 - Limit_6 int32 - Limit_7 int32 + UserID pgtype.UUID + ID pgtype.UUID + Column3 interface{} + Column4 []pgtype.UUID + Limit int32 + Limit_2 int32 + Limit_3 int32 + Limit_4 int32 + Limit_5 int32 + Limit_6 int32 + Limit_7 int32 + Column12 string } type LoadRadioCandidatesV2Row struct { @@ -971,8 +972,22 @@ type LoadRadioCandidatesV2Row struct { // enter the pool even when the similarity/random arms miss them; scored // in Go via TasteMatch, so sim_score here is 0 pool-inclusion), // $11 coplay_artists K (#1533 — tracks by artists co-played across the -// instance with the seed's artist; source='user_cooccurrence'). +// instance with the seed's artist; source='user_cooccurrence'), +// $12 order_seed (text) — see below. // +// $12 REPLACES `ORDER BY random()` IN FOUR ARMS (#3889). Those arms returned +// a stable set only while their LIMIT exceeded the rows eligible for them: at +// that point they returned all of them and the order stopped mattering, +// because the caller sorts by track id before scoring. Below that threshold +// they returned a random SUBSET, and two builds on the same day drew +// different ones — so "daily determinism" held by accident, and only for +// libraries smaller than the limits. +// +// md5(id || seed) keeps the intent — an arbitrary spread that changes when +// the seed does — while making it reproducible for a given seed. The CALLER +// decides what that means: system mixes pass a per-(user, day) string and get +// the determinism they promise; radio passes a fresh value per request and +// keeps varying, which is what a radio should do. // Returns same shape as LoadRadioCandidates plus similarity_score column. func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandidatesV2Params) ([]LoadRadioCandidatesV2Row, error) { rows, err := q.db.Query(ctx, loadRadioCandidatesV2, @@ -987,6 +1002,7 @@ func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandid arg.Limit_5, arg.Limit_6, arg.Limit_7, + arg.Column12, ) if err != nil { return nil, err diff --git a/internal/db/queries/recommendation.sql b/internal/db/queries/recommendation.sql index 20fbf7f7..7d9bfa87 100644 --- a/internal/db/queries/recommendation.sql +++ b/internal/db/queries/recommendation.sql @@ -45,7 +45,22 @@ WHERE t.id <> $2 -- enter the pool even when the similarity/random arms miss them; scored -- in Go via TasteMatch, so sim_score here is 0 pool-inclusion), -- $11 coplay_artists K (#1533 — tracks by artists co-played across the --- instance with the seed's artist; source='user_cooccurrence'). +-- instance with the seed's artist; source='user_cooccurrence'), +-- $12 order_seed (text) — see below. +-- +-- $12 REPLACES `ORDER BY random()` IN FOUR ARMS (#3889). Those arms returned +-- a stable set only while their LIMIT exceeded the rows eligible for them: at +-- that point they returned all of them and the order stopped mattering, +-- because the caller sorts by track id before scoring. Below that threshold +-- they returned a random SUBSET, and two builds on the same day drew +-- different ones — so "daily determinism" held by accident, and only for +-- libraries smaller than the limits. +-- +-- md5(id || seed) keeps the intent — an arbitrary spread that changes when +-- the seed does — while making it reproducible for a given seed. The CALLER +-- decides what that means: system mixes pass a per-(user, day) string and get +-- the determinism they promise; radio passes a fresh value per request and +-- keeps varying, which is what a radio should do. -- Returns same shape as LoadRadioCandidates plus similarity_score column. WITH @@ -87,7 +102,7 @@ similar_artists AS ( JOIN seed_artist sa ON asim.artist_a_id = sa.artist_id WHERE asim.source = 'listenbrainz' AND t.id NOT IN (SELECT id FROM excluded_ids) - ORDER BY asim.score DESC, random() + ORDER BY asim.score DESC, md5(t.id::text || $12::text) LIMIT $6 ), tag_overlap AS ( @@ -115,7 +130,7 @@ likes_overlap AS ( WHERE t.id = gl.track_id AND trim(g_overlap.g) IN (SELECT tag FROM seed_tags) ) - ORDER BY random() + ORDER BY md5(gl.track_id::text || $12::text) LIMIT $8 ), taste_overlap AS ( @@ -142,7 +157,7 @@ coplay_artists AS ( WHERE asim.source = 'user_cooccurrence' AND t.id NOT IN (SELECT id FROM excluded_ids) AND t.id <> $2 - ORDER BY asim.score DESC, random() + ORDER BY asim.score DESC, md5(t.id::text || $12::text) LIMIT $11 ), random_fill AS ( @@ -158,7 +173,7 @@ random_fill AS ( UNION SELECT track_id FROM taste_overlap UNION SELECT track_id FROM coplay_artists ) - ORDER BY random() + ORDER BY md5(t.id::text || $12::text) LIMIT $9 ) SELECT diff --git a/internal/playlists/system.go b/internal/playlists/system.go index f0708793..420c0811 100644 --- a/internal/playlists/system.go +++ b/internal/playlists/system.go @@ -276,6 +276,17 @@ func SetTasteConfig(c taste.Config) { systemTasteConfig = c } +// dailyOrderSeed is the value the randomised candidate arms order by (#3889). +// +// Per (user, day) so a same-day rebuild draws the SAME set — which is what +// TestBuildSystemPlaylists_DailyNonceDeterminism asserts and what those arms +// only ever achieved by accident before, when their limits happened to exceed +// the eligible rows. It changes on the day boundary, so the mixes still move +// daily. +func dailyOrderSeed(userID pgtype.UUID, dateStr string) string { + return uuidStringPL(userID) + ":" + dateStr +} + func currentSongsLikeWeights() recommendation.ScoringWeights { systemTuningMu.RLock() defer systemTuningMu.RUnlock() @@ -629,6 +640,7 @@ func produceForYou( zeroVec, seeds, systemForYouSourceLimits(), + dailyOrderSeed(userID, dateStr), ) if cerr != nil { logger.Warn("system playlist: for-you candidates load failed for seed; continuing", @@ -716,6 +728,7 @@ func produceSeedMixes( recommendation.ScaleForLibrary( recommendation.SongsLikeCandidateSourceLimits(), librarySize, ), + dailyOrderSeed(userID, dateStr), ) if cerr != nil { logger.Warn("system playlist: seed candidates load failed; skipping", diff --git a/internal/playlists/you_might_like.go b/internal/playlists/you_might_like.go index 27ccaf58..2e4d4bc1 100644 --- a/internal/playlists/you_might_like.go +++ b/internal/playlists/you_might_like.go @@ -102,6 +102,7 @@ func buildYouMightLike( cands, err := recommendation.LoadCandidatesFromSimilarity( ctx, q, userID, seed, 1, zeroVec, []pgtype.UUID{seed}, ymlLimits, + dailyOrderSeed(userID, dateStr), ) if err != nil { logger.Warn("you-might-like: candidate load failed; skipping", diff --git a/internal/recommendation/candidates.go b/internal/recommendation/candidates.go index c2d2a4ac..ecd1d7e2 100644 --- a/internal/recommendation/candidates.go +++ b/internal/recommendation/candidates.go @@ -139,46 +139,41 @@ func DefaultCandidateSourceLimits() CandidateSourceLimits { // would produce a short mix or none at all, and "no playlist" is a worse // answer than "a few tracks further from the seed than we would like". // -// DO NOT SHRINK AN ARM ORDERED BY UNSEEDED random(). This is the constraint -// that shapes the numbers below, and it is not obvious from reading them. +// The seed-independent arms are trimmed hardest, because on this surface they +// are noise: `taste_overlap` (tracks by the user's top taste artists) and +// `random_fill` (any track not already in the pool) both carry +// `0.0::float8 AS sim_score`, so nearly a third of the default pool had no +// relationship to the seed at all. // -// `likes_overlap` and `random_fill` both end in a bare `ORDER BY random()` -// (recommendation.sql:118, :161) with no daily seed. Such an arm returns a -// STABLE set only while its LIMIT exceeds the rows eligible for it — at that -// point it returns all of them and the random order is irrelevant, because -// the caller sorts by id before scoring. Drop the limit below the eligible -// count and the arm starts returning a random SUBSET, which differs between -// two builds on the same day. +// THESE TRIMS WERE BLOCKED UNTIL #3889. `likes_overlap` and `random_fill` +// used to end in a bare `ORDER BY random()`, which made their output a stable +// SET only while the limit exceeded the eligible rows — so SHRINKING them +// changed pool membership between same-day rebuilds and broke daily +// determinism. Those arms now order by md5(id || seed), so a smaller limit +// takes a smaller but REPRODUCIBLE slice, and the trim is safe. // -// That is a real defect (#3889) rather than a quirk of this function, and it -// bit here: cutting RandomFill to 10 broke -// TestBuildSystemPlaylists_DailyNonceDeterminism, whose library is smaller -// than the default limit and whose determinism was therefore accidental. -// Growing an arm is always safe; only shrinking one is. +// Reduced, never removed. Rule 131: the two seed-independent arms are the +// tier-3 floor, and zeroing them would leave a seed with thin ListenBrainz +// coverage producing a short mix or none at all. The weights (SimilarityWeight +// 4.0, everything seed-independent demoted) keep them ranked last, so they +// surface only when the closer tiers cannot fill the mix. // -// So the seed-independent arms are trimmed only where the ordering is -// deterministic: `taste_overlap` sorts by `tpa.weight DESC, t.id` and can be -// cut, `random_fill` cannot. The reduction is consequently modest — and it -// matters less than it looks, because the WEIGHTS are what demote sim_score-0 -// candidates now. The pool change biases the draw; the songs_like profile is -// what actually keeps unrelated tracks out of the result. -// -// One arm is left alone that arguably should not be: `likes_overlap` assigns -// a FLAT 0.6 sim_score (recommendation.sql:108) rather than measuring -// anything — a collaborative signal wearing similarity's clothes, which a -// raised SimilarityWeight amplifies. If real ListenBrainz scores commonly -// land below 0.6 it will outrank genuine matches. It cannot be trimmed here -// without the determinism fix landing first; the honest repair is to stop it -// claiming a similarity score it never computed (#3879). +// likes_overlap is cut hardest of the tier-2 arms for a specific reason: its +// SQL assigns a FLAT 0.6 sim_score (recommendation.sql) rather than measuring +// anything. It is a collaborative signal wearing similarity's clothes, and a +// raised SimilarityWeight amplifies it — if real ListenBrainz scores commonly +// land below 0.6 it would outrank genuine matches. Halved pending the +// fill-rate measurement in #3879; the honest fix is to stop it claiming a +// similarity score it never computed. func SongsLikeCandidateSourceLimits() CandidateSourceLimits { return CandidateSourceLimits{ LBSimilar: 60, // tier 1 — doubled; the only arm that measures the seed SimilarArtist: 40, // tier 2 — raised; growing is always safe TagOverlap: 20, // tier 2 UserCoplay: 20, // tier 2 - LikesOverlap: 20, // tier 2 — NOT trimmed: unseeded random(), see above - TasteOverlap: 10, // tier 3 floor — halved; deterministic ordering, safe - RandomFill: 30, // tier 3 floor — NOT trimmed: unseeded random(), see above + LikesOverlap: 10, // tier 2, halved — flat 0.6 sim_score, see above + TasteOverlap: 10, // tier 3 floor — halved, not removed + RandomFill: 10, // tier 3 floor — cut hard, never to zero } } @@ -187,6 +182,10 @@ func SongsLikeCandidateSourceLimits() CandidateSourceLimits { // likes-overlap / random fill) + dedup-by-max sim_score. Returns // []Candidate (same shape as LoadCandidates) so Shuffle is unchanged. // +// orderSeed decides whether the randomised arms repeat their draw — see +// Column12 below and #3889. Pass a stable per-(user, day) value where the +// selection must be reproducible, and a varying one where it should not be. +// // Caller (radio handler) falls back to LoadCandidates on error. func LoadCandidatesFromSimilarity( ctx context.Context, @@ -196,6 +195,7 @@ func LoadCandidatesFromSimilarity( currentVector SessionVector, exclude []pgtype.UUID, limits CandidateSourceLimits, + orderSeed string, ) ([]Candidate, error) { if exclude == nil { exclude = []pgtype.UUID{} @@ -212,6 +212,12 @@ func LoadCandidatesFromSimilarity( Limit_5: int32(limits.RandomFill), Limit_6: int32(limits.TasteOverlap), Limit_7: int32(limits.UserCoplay), + // #3889. Four arms used to end in a bare ORDER BY random(), which made + // their output a stable SET only while the limit exceeded the eligible + // rows. They now order by md5(id || this), so the caller decides + // whether the draw repeats: a per-(user, day) seed for the system + // mixes that promise daily determinism, a fresh one per radio request. + Column12: orderSeed, }) if err != nil { return nil, err diff --git a/internal/recommendation/candidates_v2_test.go b/internal/recommendation/candidates_v2_test.go index 0fa19b55..af654157 100644 --- a/internal/recommendation/candidates_v2_test.go +++ b/internal/recommendation/candidates_v2_test.go @@ -2,6 +2,10 @@ package recommendation import ( "context" + "fmt" + "reflect" + "sort" + "strings" "testing" "github.com/jackc/pgx/v5/pgtype" @@ -50,7 +54,7 @@ func TestLoadCandidatesFromSimilarity_LBSimilarSourceContributes(t *testing.T) { target := f.tracks[1] helperLBSimilarity(t, f, seed.ID, target.ID, 0.85) got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -82,7 +86,7 @@ func TestLoadCandidatesFromSimilarity_SimilarArtistTracksContribute(t *testing.T }) helperArtistSimilarity(t, f, seed.ArtistID, otherArtist.ID, 0.8) got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -106,7 +110,7 @@ func TestLoadCandidatesFromSimilarity_TagOverlapContributes(t *testing.T) { helperSetTrackGenre(t, f, seed.ID, "Rock; Pop") helperSetTrackGenre(t, f, target.ID, "Rock") got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -133,7 +137,7 @@ func TestLoadCandidatesFromSimilarity_LikesOverlapContributes(t *testing.T) { t.Fatalf("like: %v", err) } got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -155,7 +159,7 @@ func TestLoadCandidatesFromSimilarity_RandomFillReturnsTracks(t *testing.T) { f := newFixture(t, 10) // 10 tracks; no similarity data seed := f.tracks[0] got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -176,7 +180,7 @@ func TestLoadCandidatesFromSimilarity_ExcludeListRespected(t *testing.T) { excluded := f.tracks[1].ID got, err := LoadCandidatesFromSimilarity( context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, - []pgtype.UUID{excluded}, defaultLimits(), + []pgtype.UUID{excluded}, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -192,7 +196,7 @@ func TestLoadCandidatesFromSimilarity_SeedAlwaysExcluded(t *testing.T) { f := newFixture(t, 5) seed := f.tracks[0] got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -222,7 +226,7 @@ func TestLoadCandidatesFromSimilarity_RecentlyPlayedExcluded(t *testing.T) { t.Fatalf("play_event: %v", err) } got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -242,7 +246,7 @@ func TestLoadCandidatesFromSimilarity_DedupTakesMaxScore(t *testing.T) { helperSetTrackGenre(t, f, target.ID, "Rock") // jaccard 1/1 = 1.0 from tag-overlap helperLBSimilarity(t, f, seed.ID, target.ID, 0.5) // weaker LB signal got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -295,7 +299,7 @@ func TestLoadCandidatesFromSimilarity_TasteOverlapArm(t *testing.T) { // Only the taste_overlap arm is enabled. limits := CandidateSourceLimits{TasteOverlap: 10} got, err := LoadCandidatesFromSimilarity( - ctx, f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, limits, + ctx, f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, limits, "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -321,7 +325,7 @@ func TestLoadCandidatesFromSimilarity_EmptyLibrary_NoError(t *testing.T) { f := newFixture(t, 1) // just the seed seed := f.tracks[0] got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -331,3 +335,97 @@ func TestLoadCandidatesFromSimilarity_EmptyLibrary_NoError(t *testing.T) { t.Errorf("got %d candidates from seed-only library, want 0", len(got)) } } + +// The randomised arms must draw REPRODUCIBLY for a given seed (#3889). +// +// Four arms used to end in a bare `ORDER BY random()`. That returned a stable +// set only while the arm's LIMIT exceeded the rows eligible for it — at that +// point it returned all of them and the order stopped mattering, because the +// caller sorts by track id before scoring. Below that threshold it returned a +// random SUBSET, so two calls drew different candidates. +// +// It therefore held by ACCIDENT, and only for libraries smaller than the +// limits. Any real library is larger, so same-day rebuilds had been drawing +// different mixes since the arm was written — invisible, because a mix that +// changes after a refresh looks like a feature. +// +// Limits deliberately smaller than the fixture, because that is the only +// regime where the bug existed at all: with limits above the eligible count +// the old code passes this too. +func TestLoadCandidatesFromSimilarity_SameSeedDrawsTheSameSet(t *testing.T) { + f := newFixture(t, 12) + seed := f.tracks[0] + + tight := CandidateSourceLimits{ + LBSimilar: 2, SimilarArtist: 2, TagOverlap: 2, + LikesOverlap: 2, RandomFill: 3, TasteOverlap: 2, UserCoplay: 2, + } + ids := func(cs []Candidate) []string { + out := make([]string, 0, len(cs)) + for _, c := range cs { + out = append(out, fmt.Sprintf("%x", c.Track.ID.Bytes)) + } + sort.Strings(out) // membership, not order — order is settled downstream + return out + } + + first, err := LoadCandidatesFromSimilarity( + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, tight, "day-one", + ) + if err != nil { + t.Fatalf("load: %v", err) + } + if len(first) == 0 { + t.Fatal("no candidates, so this test asserts nothing") + } + + for i := 0; i < 3; i++ { + again, err := LoadCandidatesFromSimilarity( + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, tight, "day-one", + ) + if err != nil { + t.Fatalf("load %d: %v", i, err) + } + if !reflect.DeepEqual(ids(first), ids(again)) { + t.Fatalf("same seed drew a different set on call %d:\n first %v\n again %v", + i, ids(first), ids(again)) + } + } +} + +// ...and a different seed is free to draw differently, or the ordering would +// be fixed rather than seeded and every day would serve the same mix. +// +// Asserted as "not pinned to one answer" rather than "always differs": with a +// small fixture two seeds can legitimately collide, so requiring a difference +// on any single pair would be flaky. Several seeds producing exactly one +// distinct set is the real regression — that is what a constant ORDER BY +// looks like. +func TestLoadCandidatesFromSimilarity_DifferentSeedsCanDrawDifferently(t *testing.T) { + f := newFixture(t, 12) + seed := f.tracks[0] + tight := CandidateSourceLimits{ + LBSimilar: 2, SimilarArtist: 2, TagOverlap: 2, + LikesOverlap: 2, RandomFill: 3, TasteOverlap: 2, UserCoplay: 2, + } + + seen := map[string]bool{} + for _, orderSeed := range []string{"a", "b", "c", "d", "e", "f"} { + cs, err := LoadCandidatesFromSimilarity( + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, tight, orderSeed, + ) + if err != nil { + t.Fatalf("load %q: %v", orderSeed, err) + } + ids := make([]string, 0, len(cs)) + for _, c := range cs { + ids = append(ids, fmt.Sprintf("%x", c.Track.ID.Bytes)) + } + sort.Strings(ids) + seen[strings.Join(ids, ",")] = true + } + if len(seen) < 2 { + t.Errorf("six different seeds produced %d distinct set(s); the ordering is not "+ + "varying with the seed at all", len(seen)) + } +} diff --git a/internal/recommendation/songs_like_limits_test.go b/internal/recommendation/songs_like_limits_test.go index 568df1f5..8e8bce87 100644 --- a/internal/recommendation/songs_like_limits_test.go +++ b/internal/recommendation/songs_like_limits_test.go @@ -59,41 +59,3 @@ func TestSongsLikeLimits_KeepThePoolRoughlyTheSameSize(t *testing.T) { "this was meant to re-weight the pool, not starve it", s, d) } } - -// The constraint that is invisible in the numbers, and that this file exists -// to keep visible. -// -// `likes_overlap` and `random_fill` end in a bare `ORDER BY random()` with no -// daily seed (recommendation.sql:118, :161). Such an arm returns a stable set -// only while its LIMIT exceeds the eligible rows; below that it returns a -// random SUBSET that differs between two builds on the same day, and the -// daily-determinism promise quietly stops holding. -// -// This is not hypothetical — it is how this change first failed CI. Cutting -// RandomFill to 10 broke TestBuildSystemPlaylists_DailyNonceDeterminism, -// whose library is smaller than the default limit and whose determinism was -// therefore an accident of the limit exceeding the library. -// -// Growing these arms is always safe. Only shrinking is, and the fix that -// would make shrinking safe is a seeded ordering (#3889), not a smaller -// number here. -func TestSongsLikeLimits_DoNotShrinkTheUnseededRandomArms(t *testing.T) { - d := DefaultCandidateSourceLimits() - s := SongsLikeCandidateSourceLimits() - - for _, tc := range []struct { - arm string - songsLike, dflt int - }{ - {"RandomFill", s.RandomFill, d.RandomFill}, - {"LikesOverlap", s.LikesOverlap, d.LikesOverlap}, - } { - if tc.songsLike < tc.dflt { - t.Errorf("%s cut from %d to %d. That arm is ordered by unseeded random(), "+ - "so a smaller limit makes pool membership vary between same-day "+ - "rebuilds — it breaks daily determinism rather than merely narrowing "+ - "the mix. Fix the ordering (#3889) before trimming this.", - tc.arm, tc.dflt, tc.songsLike) - } - } -} From cba77a518799dd46eb63d4227110314231df265a Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 11 Sep 2026 13:21:15 -0400 Subject: [PATCH 2/5] =?UTF-8?q?feat(library):=20fingerprint=20every=20new?= =?UTF-8?q?=20or=20changed=20file=20=E2=80=94=20M400=20#3905-#3907?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two identities per track, because they answer different questions: - audio_stream_sha256: SHA-256 of the ENCODED audio packets (ffmpeg -map 0:a -c:a copy -f hash). Equal means identical audio whatever the tags say. Measured against the #3885 pair: the two WWW files hash identically here and differently as whole files. Packets rather than decoded samples, so an ffmpeg upgrade cannot silently change every stored hash, and nothing is decoded. - chromaprint: fpcalc -raw -signed. The same recording at another bitrate or codec, for the acoustic tier. fpcalc ships in the image (libchromaprint-tools); shelled out because CGO_ENABLED=0 rules out bindings. Stored in a track_fingerprints table rather than on tracks: eight queries read tracks with SELECT *, including album pages, search and the Subsonic surface, and a ~4 KB array there would be de-TOASTed on every one of them. The scan fingerprints only bytes it has not seen (a new path, or mtime past the row's). A tag-repair pass leaves fingerprints alone, and unchanged files with no fingerprint are the backfill's job (#3908). Folding that into the skip check would re-decode the whole library on the first scan after upgrade and push a sync change per track. A failure that says nothing about the file (timeout, cancelled scan, tool not installed) is never stored, and on changed bytes it removes the old row. A tool that rejects the file stores NULL at the current version, so the backfill does not retry it every boot. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- Dockerfile | 8 +- internal/db/dbq/fingerprints.sql.go | 59 ++++ internal/db/dbq/models.go | 8 + .../0058_track_fingerprints.down.sql | 1 + .../migrations/0058_track_fingerprints.up.sql | 38 +++ internal/db/queries/fingerprints.sql | 22 ++ internal/dbtest/reset.go | 1 + internal/library/fingerprint.go | 296 ++++++++++++++++++ internal/library/fingerprint_scan_test.go | 174 ++++++++++ internal/library/fingerprint_test.go | 194 ++++++++++++ internal/library/scanner.go | 29 +- 11 files changed, 828 insertions(+), 2 deletions(-) create mode 100644 internal/db/dbq/fingerprints.sql.go create mode 100644 internal/db/migrations/0058_track_fingerprints.down.sql create mode 100644 internal/db/migrations/0058_track_fingerprints.up.sql create mode 100644 internal/db/queries/fingerprints.sql create mode 100644 internal/library/fingerprint.go create mode 100644 internal/library/fingerprint_scan_test.go create mode 100644 internal/library/fingerprint_test.go diff --git a/Dockerfile b/Dockerfile index 7aef6079..55b3690d 100644 --- a/Dockerfile +++ b/Dockerfile @@ -33,8 +33,14 @@ RUN go build -trimpath \ -o /out/minstrel ./cmd/minstrel FROM debian:bookworm-slim +# ffmpeg: duration probes and the exact-tier audio hash (a SHA-256 of the +# encoded audio packets, so no decode). libchromaprint-tools: fpcalc, the +# acoustic fingerprint that tells the same recording at two bitrates apart +# from two different recordings (M400). Both are baked in at build time so a +# deployed instance never fetches either (rule 164); fpcalc is shelled out +# rather than bound because CGO_ENABLED=0 above rules out cgo. RUN apt-get update \ - && apt-get install -y --no-install-recommends ca-certificates ffmpeg \ + && apt-get install -y --no-install-recommends ca-certificates ffmpeg libchromaprint-tools \ && rm -rf /var/lib/apt/lists/* RUN groupadd --system --gid 1000 minstrel \ diff --git a/internal/db/dbq/fingerprints.sql.go b/internal/db/dbq/fingerprints.sql.go new file mode 100644 index 00000000..f1b5e244 --- /dev/null +++ b/internal/db/dbq/fingerprints.sql.go @@ -0,0 +1,59 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: fingerprints.sql + +package dbq + +import ( + "context" + + "github.com/jackc/pgx/v5/pgtype" +) + +const deleteTrackFingerprint = `-- name: DeleteTrackFingerprint :exec +DELETE FROM track_fingerprints WHERE track_id = $1 +` + +// A file changed but could not be fingerprinted, for a reason unrelated to the +// file. The stored row describes the OLD bytes, so it goes and the backfill +// re-derives it — nothing may keep trusting a stale identity. +func (q *Queries) DeleteTrackFingerprint(ctx context.Context, trackID pgtype.UUID) error { + _, err := q.db.Exec(ctx, deleteTrackFingerprint, trackID) + return err +} + +const upsertTrackFingerprint = `-- name: UpsertTrackFingerprint :exec +INSERT INTO track_fingerprints ( + track_id, audio_stream_sha256, chromaprint, fingerprint_version +) VALUES ( + $1, $2, $3, + $4 +) +ON CONFLICT (track_id) DO UPDATE SET + audio_stream_sha256 = EXCLUDED.audio_stream_sha256, + chromaprint = EXCLUDED.chromaprint, + fingerprint_version = EXCLUDED.fingerprint_version, + computed_at = now() +` + +type UpsertTrackFingerprintParams struct { + TrackID pgtype.UUID + AudioStreamSha256 []byte + Chromaprint []int32 + FingerprintVersion int16 +} + +// Written whenever a track's fingerprint is derived: by the scan when a file is +// new or its bytes changed, and by the backfill (#3908) for rows derived by an +// older method. Replaces the row wholesale — a fingerprint of the old bytes has +// no standing once the file has changed. +func (q *Queries) UpsertTrackFingerprint(ctx context.Context, arg UpsertTrackFingerprintParams) error { + _, err := q.db.Exec(ctx, upsertTrackFingerprint, + arg.TrackID, + arg.AudioStreamSha256, + arg.Chromaprint, + arg.FingerprintVersion, + ) + return err +} diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index d0d4a5f1..07ca6f75 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -667,6 +667,14 @@ type Track struct { MissingSince pgtype.Timestamptz } +type TrackFingerprint struct { + TrackID pgtype.UUID + AudioStreamSha256 []byte + Chromaprint []int32 + FingerprintVersion int16 + ComputedAt pgtype.Timestamptz +} + type TrackSimilarity struct { TrackAID pgtype.UUID TrackBID pgtype.UUID diff --git a/internal/db/migrations/0058_track_fingerprints.down.sql b/internal/db/migrations/0058_track_fingerprints.down.sql new file mode 100644 index 00000000..c04a359c --- /dev/null +++ b/internal/db/migrations/0058_track_fingerprints.down.sql @@ -0,0 +1 @@ +DROP TABLE track_fingerprints; diff --git a/internal/db/migrations/0058_track_fingerprints.up.sql b/internal/db/migrations/0058_track_fingerprints.up.sql new file mode 100644 index 00000000..aa0f8c9c --- /dev/null +++ b/internal/db/migrations/0058_track_fingerprints.up.sql @@ -0,0 +1,38 @@ +-- 0058_track_fingerprints.up.sql — an acoustic identity per track (Scribe +-- milestone #400: #3905, #3906). +-- +-- A table of its own rather than columns on tracks, for the hot path's sake: +-- tracks is read with SELECT * by eight queries, among them ListTracksByAlbum, +-- SearchTracks and GetTracksByIDs — album pages, search, the Subsonic surface. +-- A ~4 KB chromaprint column on tracks would be de-TOASTed on every one of +-- those reads to carry a value only the duplicate sweep ever looks at. +-- +-- What a row means, which the backfill depends on: +-- no row never fingerprinted +-- fingerprint_version < current derived by an older method; re-derive it +-- fingerprint_version = current attempted; a NULL value means that tool +-- failed on this file, and it is not retried +-- until the file changes +-- A failure that says nothing about the file — a timeout, a cancelled scan, a +-- missing binary — writes no row at all, so the backfill tries again. +CREATE TABLE track_fingerprints ( + -- CASCADE is right here, unlike for the likes and play history M400's + -- merge has to carry across: a fingerprint describes one file's bytes and + -- means nothing once that file's row is gone. + track_id uuid PRIMARY KEY REFERENCES tracks (id) ON DELETE CASCADE, + -- SHA-256 of the ENCODED audio packets (ffmpeg -c:a copy -f hash), not of + -- decoded samples. internal/library/fingerprint.go says why. + audio_stream_sha256 bytea + CHECK (audio_stream_sha256 IS NULL OR octet_length(audio_stream_sha256) = 32), + -- fpcalc -raw -signed: the same 32 bits per item, stored signed because + -- integer is. + chromaprint integer[], + fingerprint_version smallint NOT NULL, + computed_at timestamptz NOT NULL DEFAULT now() +); + +-- The exact duplicate tier is an equality match on this column. Partial +-- because a NULL is never looked up — it only means the hash was not taken. +CREATE INDEX track_fingerprints_audio_stream_sha256 + ON track_fingerprints (audio_stream_sha256) + WHERE audio_stream_sha256 IS NOT NULL; diff --git a/internal/db/queries/fingerprints.sql b/internal/db/queries/fingerprints.sql new file mode 100644 index 00000000..ef0276b5 --- /dev/null +++ b/internal/db/queries/fingerprints.sql @@ -0,0 +1,22 @@ +-- name: UpsertTrackFingerprint :exec +-- Written whenever a track's fingerprint is derived: by the scan when a file is +-- new or its bytes changed, and by the backfill (#3908) for rows derived by an +-- older method. Replaces the row wholesale — a fingerprint of the old bytes has +-- no standing once the file has changed. +INSERT INTO track_fingerprints ( + track_id, audio_stream_sha256, chromaprint, fingerprint_version +) VALUES ( + sqlc.arg(track_id), sqlc.narg(audio_stream_sha256), sqlc.narg(chromaprint), + sqlc.arg(fingerprint_version) +) +ON CONFLICT (track_id) DO UPDATE SET + audio_stream_sha256 = EXCLUDED.audio_stream_sha256, + chromaprint = EXCLUDED.chromaprint, + fingerprint_version = EXCLUDED.fingerprint_version, + computed_at = now(); + +-- name: DeleteTrackFingerprint :exec +-- A file changed but could not be fingerprinted, for a reason unrelated to the +-- file. The stored row describes the OLD bytes, so it goes and the backfill +-- re-derives it — nothing may keep trusting a stale identity. +DELETE FROM track_fingerprints WHERE track_id = $1; diff --git a/internal/dbtest/reset.go b/internal/dbtest/reset.go index c71b9aae..a99db739 100644 --- a/internal/dbtest/reset.go +++ b/internal/dbtest/reset.go @@ -87,6 +87,7 @@ var dataTables = []string{ // pristine Discover knobs rather than whatever a previous test tuned. "discover_tuning", "recommendation_tuning_audit", + "track_fingerprints", // M400 "tracks", "albums", "artists", diff --git a/internal/library/fingerprint.go b/internal/library/fingerprint.go new file mode 100644 index 00000000..59ac7cb7 --- /dev/null +++ b/internal/library/fingerprint.go @@ -0,0 +1,296 @@ +package library + +import ( + "bytes" + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "os/exec" + "strconv" + "strings" + "time" + + "github.com/jackc/pgx/v5/pgtype" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// Acoustic identity (M400). +// +// Two values per track, because they answer different questions: +// +// audio_stream_sha256 a SHA-256 of the ENCODED audio packets. Equal means the +// same audio bytes, whatever the tags or container around +// them say. No threshold and no false positives — this is +// what catches two copies of one MP3 that differ only in +// their ID3 (#3885). +// +// chromaprint fpcalc's raw fingerprint. Close means the same +// recording, even at another bitrate or in another codec +// — the case an exact hash cannot see. +// +// Both shell out, in the shape probeDurationMs already set: a deadline on every +// call, and a failure that leaves the value unset rather than failing the file. +// A track with no fingerprint is never a duplicate candidate; it is still a +// track. + +// fingerprintTimeout bounds one ffmpeg hash or fpcalc call. Longer than +// probeTimeout because both read the audio rather than a header: the hash reads +// every packet and fpcalc decodes up to its -length. 60s leaves room for a large +// lossless file on a slow network mount; a call needing more is a stall, not a +// big file. +const fingerprintTimeout = 60 * time.Second + +// fingerprintWaitDelay bounds how long Output may keep waiting on the tool's +// pipes after the deadline has killed it. Without it, a child that left a +// descendant holding stdout open would block the scan past its own timeout. +const fingerprintWaitDelay = 5 * time.Second + +// fingerprintVersion stamps how a track_fingerprints row was derived. Bump it +// whenever the derivation changes — the hash arguments, fpcalc's flags or its +// length — and the backfill re-derives every row below it. Fingerprints taken +// by two methods are not comparable, and nothing else would reveal that the +// library held a mix. +const fingerprintVersion int16 = 1 + +// errFingerprintTimeout marks a tool that ran out of time. Distinct from a +// failed exit because a stall is a fact about the mount, not about the file. +var errFingerprintTimeout = errors.New("fingerprint tool timed out") + +// defaultChromaprintLengthSec is how many seconds of audio fpcalc fingerprints. +// 120 is fpcalc's own default. Fingerprints taken at different lengths are not +// comparable, so changing this has to re-derive every stored one. +const defaultChromaprintLengthSec = 120 + +// fpcalcStderrTail caps how much of a failing tool's stderr reaches the log. +const fpcalcStderrTail = 512 + +// streamHashArgs hashes the encoded audio packets, never decoded samples. +// +// -c:a copy is the point, not an optimisation. A decoded hash of a lossy file +// depends on the decoder's float maths and sample conversion, which can move +// between ffmpeg releases — so an image upgrade could silently change every +// stored hash, and yesterday's duplicate would stop matching today's copy. +// Packet bytes do not move. It is also far cheaper: demux only, no decode. +// +// -map 0:a keeps embedded cover art (an attached-picture video stream) out of +// the hash, so two copies of one recording carrying different art still match. +func streamHashArgs(path string) []string { + return []string{ + "-v", "error", + "-i", path, + "-map", "0:a", + "-c:a", "copy", + "-f", "hash", "-hash", "sha256", + "-", + } +} + +// fpcalcArgs asks for the raw fingerprint as SIGNED integers. +// +// -raw because the matcher compares items bit by bit, which the compressed form +// cannot do without being unpacked first. -signed because the column is Postgres +// integer[], which is signed: fpcalc's default prints uint32, and half of those +// values do not fit. Signed output is the same 32 bits with no reinterpretation +// step left to get wrong. +func fpcalcArgs(path string, lengthSec int) []string { + return []string{ + "-raw", "-signed", + "-length", strconv.Itoa(lengthSec), + path, + } +} + +// fingerprintResult is one attempt at both halves of a track's identity. They +// fail independently: a file ffmpeg can demux may still defeat fpcalc. +type fingerprintResult struct { + streamSHA256 []byte + chromaprint []int32 + hashErr error + printErr error +} + +// computeFingerprint derives both halves for the file at path. +func computeFingerprint(ctx context.Context, path string) fingerprintResult { + var r fingerprintResult + r.streamSHA256, r.hashErr = computeAudioStreamSHA256(ctx, path) + r.chromaprint, r.printErr = computeChromaprint(ctx, path, defaultChromaprintLengthSec) + return r +} + +// inconclusive reports whether either half failed for a reason that says +// nothing about the file. Such a result must never be stored: stamped at the +// current version it would read as "tried, and this file cannot be +// fingerprinted", and the backfill would never try it again. +func (r fingerprintResult) inconclusive() bool { + return isInconclusive(r.hashErr) || isInconclusive(r.printErr) +} + +// isInconclusive names the failures that are not a verdict on the file: a +// stall, a cancelled scan, and a tool that is not installed. The last matters +// outside the image — a dev binary run without fpcalc on PATH must not stamp +// every track in the library as unfingerprintable. +func isInconclusive(err error) bool { + return errors.Is(err, errFingerprintTimeout) || + errors.Is(err, context.Canceled) || + errors.Is(err, context.DeadlineExceeded) || + errors.Is(err, exec.ErrNotFound) +} + +// fingerprintFile runs the scanner's fingerprinter. A Scanner built without New +// gets the real tools rather than a nil-func panic halfway through a scan. +func (s *Scanner) fingerprintFile(ctx context.Context, path string) fingerprintResult { + if s.fingerprint == nil { + return computeFingerprint(ctx, path) + } + return s.fingerprint(ctx, path) +} + +// storeFingerprint records one attempt for a track whose bytes are new or have +// changed. It never fails the scan: a missing fingerprint only keeps a track +// out of duplicate detection, which is not worth dropping the track over. +func (s *Scanner) storeFingerprint( + ctx context.Context, q *dbq.Queries, trackID pgtype.UUID, path string, fp fingerprintResult, +) { + if fp.hashErr != nil { + s.logger.Warn("library scan: audio stream hash failed", "path", path, "err", fp.hashErr) + } + if fp.printErr != nil { + s.logger.Warn("library scan: chromaprint failed", "path", path, "err", fp.printErr) + } + if fp.inconclusive() { + // Any row this track holds describes its PREVIOUS bytes. Drop it and + // leave the track to the backfill, rather than stamping a failure that + // says nothing about this file. + if err := q.DeleteTrackFingerprint(ctx, trackID); err != nil { + s.logger.Warn("library scan: clearing stale fingerprint failed", "path", path, "err", err) + } + return + } + // A NULL half here is a verdict — the tool ran and rejected this file — and + // is stamped at the current version so the backfill does not retry it on + // every boot. It is retried when the file changes. + if err := q.UpsertTrackFingerprint(ctx, dbq.UpsertTrackFingerprintParams{ + TrackID: trackID, + AudioStreamSha256: fp.streamSHA256, + Chromaprint: fp.chromaprint, + FingerprintVersion: fingerprintVersion, + }); err != nil { + s.logger.Warn("library scan: storing fingerprint failed", "path", path, "err", err) + } +} + +// computeAudioStreamSHA256 returns the SHA-256 of the file's encoded audio. +func computeAudioStreamSHA256(ctx context.Context, path string) ([]byte, error) { + out, err := runFingerprintTool(ctx, "ffmpeg", streamHashArgs(path)) + if err != nil { + return nil, err + } + return parseStreamHash(out) +} + +// computeChromaprint returns the raw acoustic fingerprint of the first +// lengthSec seconds of the file. +func computeChromaprint(ctx context.Context, path string, lengthSec int) ([]int32, error) { + out, err := runFingerprintTool(ctx, "fpcalc", fpcalcArgs(path, lengthSec)) + if err != nil { + return nil, err + } + return parseFpcalcRaw(out) +} + +// runFingerprintTool runs one tool under fingerprintTimeout. +// +// Any non-zero exit is an error, and that deliberately includes fpcalc's exit 3: +// "reading failed, but here is a fingerprint of what I got". A partial +// fingerprint of a damaged file is not that file's identity. Stored, it would +// score against a healthy copy over whatever prefix survived, and could group +// or fail to group either way. Absent is better than wrong. +func runFingerprintTool(ctx context.Context, name string, args []string) ([]byte, error) { + runCtx, cancel := context.WithTimeout(ctx, fingerprintTimeout) + defer cancel() + + cmd := exec.CommandContext(runCtx, name, args...) + cmd.WaitDelay = fingerprintWaitDelay + out, err := cmd.Output() + if err == nil { + return out, nil + } + // The caller gave up (a cancelled scan). Report that rather than the + // signal-killed exit it caused, so it is never mistaken for a verdict on + // the file. + if ctx.Err() != nil { + return nil, fmt.Errorf("%s: %w", name, ctx.Err()) + } + // Named separately so a stall reads as a stall, not as a crash. + if errors.Is(runCtx.Err(), context.DeadlineExceeded) { + return nil, fmt.Errorf("%s: no result within %s: %w", name, fingerprintTimeout, errFingerprintTimeout) + } + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + return nil, fmt.Errorf("%s exited %d: %s", name, exitErr.ExitCode(), stderrTail(exitErr.Stderr)) + } + return nil, fmt.Errorf("%s: %w", name, err) +} + +// stderrTail keeps the END of a failing tool's stderr. ffmpeg and fpcalc print +// the actual reason last, after any banner or per-frame warnings, so a cap that +// kept the head would log the noise and drop the cause. +func stderrTail(stderr []byte) []byte { + stderr = bytes.TrimSpace(stderr) + if len(stderr) > fpcalcStderrTail { + stderr = stderr[len(stderr)-fpcalcStderrTail:] + } + return stderr +} + +// parseStreamHash reads the ffmpeg hash muxer's "SHA256=" line. +func parseStreamHash(out []byte) ([]byte, error) { + for _, line := range strings.Split(string(out), "\n") { + hexed, ok := strings.CutPrefix(strings.TrimSpace(line), "SHA256=") + if !ok { + continue + } + sum, err := hex.DecodeString(hexed) + if err != nil { + return nil, fmt.Errorf("stream hash %q: %w", hexed, err) + } + if len(sum) != sha256.Size { + return nil, fmt.Errorf("stream hash is %d bytes, want %d", len(sum), sha256.Size) + } + return sum, nil + } + return nil, errors.New("ffmpeg printed no SHA256= line") +} + +// parseFpcalcRaw reads fpcalc's text output: +// +// DURATION= +// FINGERPRINT=,,... +func parseFpcalcRaw(out []byte) ([]int32, error) { + for _, line := range strings.Split(string(out), "\n") { + list, ok := strings.CutPrefix(strings.TrimSpace(line), "FINGERPRINT=") + if !ok { + continue + } + if list == "" { + return nil, errors.New("fpcalc returned an empty fingerprint") + } + items := strings.Split(list, ",") + fp := make([]int32, len(items)) + for i, item := range items { + // ParseInt at 32 bits, not ParseUint: a value past int32 means the + // output was unsigned — -signed went missing from the invocation — + // and nothing downstream would reinterpret it. Refuse it here. + v, err := strconv.ParseInt(item, 10, 32) + if err != nil { + return nil, fmt.Errorf("fingerprint item %d %q: %w", i, item, err) + } + fp[i] = int32(v) + } + return fp, nil + } + return nil, errors.New("fpcalc printed no FINGERPRINT= line") +} diff --git a/internal/library/fingerprint_scan_test.go b/internal/library/fingerprint_scan_test.go new file mode 100644 index 00000000..b2edb800 --- /dev/null +++ b/internal/library/fingerprint_scan_test.go @@ -0,0 +1,174 @@ +package library + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "log/slog" + "os" + "path/filepath" + "slices" + "testing" + "time" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db" +) + +// TestScanner_FingerprintsOnlyNewOrChangedBytes_Integration pins WHEN the scan +// fingerprints. The cost of getting it wrong is asymmetric and invisible: a +// scan that re-fingerprints unchanged files still produces correct rows, just +// by decoding the entire library on every tag-repair pass. +// +// The fingerprinter is stubbed. CI has no real audio, and the tools' output is +// covered by the parser tests; this covers the scan's decisions. +func TestScanner_FingerprintsOnlyNewOrChangedBytes_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() + a := filepath.Join(root, "artist/album/01.mp3") + b := filepath.Join(root, "artist/album/02.mp3") + writeTestMP3(t, a, map[string]string{"TIT2": "One", "TPE1": "Artist", "TALB": "Album", "TRCK": "1"}) + writeTestMP3(t, b, map[string]string{"TIT2": "Two", "TPE1": "Artist", "TALB": "Album", "TRCK": "2"}) + + sum := bytes.Repeat([]byte{0xAB}, 32) + chroma := []int32{7, -7, 2147483647} + result := fingerprintResult{streamSHA256: sum, chromaprint: chroma} + calls := map[string]int{} + + scanner := New(pool, logger, []string{root}) + scanner.fingerprint = func(_ context.Context, path string) fingerprintResult { + calls[path]++ + return result + } + + scan := func(step string) Stats { + t.Helper() + st, err := scanner.Scan(ctx, nil) + if err != nil { + t.Fatalf("%s: scan: %v", step, err) + } + return st + } + type row struct { + sha []byte + chroma []int32 + version int16 + } + stored := func(path string) (row, bool) { + t.Helper() + var r row + err := pool.QueryRow(ctx, ` + SELECT f.audio_stream_sha256, f.chromaprint, f.fingerprint_version + FROM track_fingerprints f JOIN tracks t ON t.id = f.track_id + WHERE t.file_path = $1`, path).Scan(&r.sha, &r.chroma, &r.version) + if errors.Is(err, pgx.ErrNoRows) { + return row{}, false + } + if err != nil { + t.Fatalf("read fingerprint for %s: %v", path, err) + } + return r, true + } + // A later step moves mtime forward past the row's updated_at, which is + // what the scan reads as "these bytes changed". + touch := func(path string, ahead time.Duration) { + t.Helper() + when := time.Now().Add(ahead) + if err := os.Chtimes(path, when, when); err != nil { + t.Fatalf("chtimes %s: %v", path, err) + } + } + + // 1. New files are fingerprinted, and stored at the current version. + scan("first scan") + if calls[a] != 1 || calls[b] != 1 { + t.Fatalf("first scan fingerprint calls = %v, want one per file", calls) + } + got, ok := stored(a) + if !ok { + t.Fatal("first scan stored no fingerprint") + } + if !bytes.Equal(got.sha, sum) || !slices.Equal(got.chroma, chroma) || got.version != fingerprintVersion { + t.Fatalf("stored %+v, want sha %x chromaprint %v version %d", got, sum, chroma, fingerprintVersion) + } + + // 2. A tag-repair pass re-reads every unchanged file and must not + // fingerprint any of them again. + // + // The Updated count is what makes this able to fail. Without it, a scan + // that simply SKIPPED both files would also leave the call counts at one, + // and the assertion would pass without the re-read path ever running. + if _, err := pool.Exec(ctx, "UPDATE tracks SET duration_ms = 1000, tag_read_version = 0"); err != nil { + t.Fatalf("force tag re-read: %v", err) + } + if st := scan("tag-repair scan"); st.Updated != 2 || st.Skipped != 0 { + t.Fatalf("tag-repair scan stats = %+v, want both files re-read (Updated=2 Skipped=0)", st) + } + if calls[a] != 1 || calls[b] != 1 { + t.Fatalf("tag-repair scan re-fingerprinted unchanged files: calls = %v", calls) + } + if _, ok := stored(a); !ok { + t.Fatal("tag-repair scan dropped a stored fingerprint") + } + + // 3. Bytes that changed are fingerprinted again, and only those. + touch(a, time.Hour) + scan("changed-file scan") + if calls[a] != 2 || calls[b] != 1 { + t.Fatalf("changed-file scan calls = %v, want a=2 b=1", calls) + } + + // 4. A changed file whose attempt is inconclusive loses its old row: that + // row describes the previous bytes, and a stall says nothing about the new + // ones. + result = fingerprintResult{streamSHA256: sum, printErr: fmt.Errorf("fpcalc: %w", errFingerprintTimeout)} + touch(a, 2*time.Hour) + scan("inconclusive scan") + if _, ok := stored(a); ok { + t.Fatal("inconclusive attempt left the previous bytes' fingerprint in place") + } + if _, ok := stored(b); !ok { + t.Fatal("inconclusive attempt on one file removed another file's fingerprint") + } + + // 5. A file the tools reject gets a row at the current version with both + // halves NULL — a verdict, so the backfill does not retry it every boot. + result = fingerprintResult{ + hashErr: errors.New("ffmpeg exited 1"), + printErr: errors.New("fpcalc exited 2"), + } + touch(a, 3*time.Hour) + scan("rejected scan") + got, ok = stored(a) + if !ok { + t.Fatal("a file the tools rejected got no row, so the backfill would retry it forever") + } + if got.sha != nil || got.chroma != nil || got.version != fingerprintVersion { + t.Fatalf("rejected file stored %+v, want both halves NULL at version %d", got, fingerprintVersion) + } +} diff --git a/internal/library/fingerprint_test.go b/internal/library/fingerprint_test.go new file mode 100644 index 00000000..e4b5cee1 --- /dev/null +++ b/internal/library/fingerprint_test.go @@ -0,0 +1,194 @@ +package library + +import ( + "bytes" + "context" + "errors" + "fmt" + "os/exec" + "slices" + "strings" + "testing" +) + +func TestParseFpcalcRaw(t *testing.T) { + cases := []struct { + name string + out string + want []int32 + wantErr string + }{ + { + name: "signed output with negatives", + out: "DURATION=213\nFINGERPRINT=-1453821711,17,0,2147483647,-2147483648\n", + want: []int32{-1453821711, 17, 0, 2147483647, -2147483648}, + }, + { + name: "fingerprint line need not come second", + out: "FINGERPRINT=5,6\nDURATION=1\n", + want: []int32{5, 6}, + }, + { + // fpcalc's default is uint32. This value only appears when -signed + // is missing, and storing it would need a reinterpretation nothing + // performs. + name: "unsigned output is refused", + out: "DURATION=213\nFINGERPRINT=2841145585,17\n", + wantErr: "item 0", + }, + {name: "empty fingerprint", out: "DURATION=0\nFINGERPRINT=\n", wantErr: "empty fingerprint"}, + {name: "no fingerprint line", out: "DURATION=213\n", wantErr: "no FINGERPRINT= line"}, + {name: "non-numeric item", out: "FINGERPRINT=1,x,3\n", wantErr: "item 1"}, + {name: "trailing comma", out: "FINGERPRINT=1,2,\n", wantErr: "item 2"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := parseFpcalcRaw([]byte(tc.out)) + if tc.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("err = %v, want one containing %q", err, tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if !slices.Equal(got, tc.want) { + t.Fatalf("got %v, want %v", got, tc.want) + } + }) + } +} + +func TestParseStreamHash(t *testing.T) { + // The real value ffmpeg printed for both files of the #3885 pair. + const www = "24e2daa3b4a534ff1a8d1a76f67810205869daf89f728d83a16625da4d28a18e" + + got, err := parseStreamHash([]byte("SHA256=" + www + "\n")) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if len(got) != 32 || got[0] != 0x24 || got[31] != 0x8e { + t.Fatalf("decoded %x, want %s", got, www) + } + + for name, out := range map[string]string{ + "no hash line": "", + "other hash": "MD5=" + www[:32] + "\n", + "not hex": "SHA256=" + strings.Repeat("zz", 32) + "\n", + "short digest": "SHA256=" + www[:62] + "\n", + "odd hex chars": "SHA256=" + www[:63] + "\n", + } { + if _, err := parseStreamHash([]byte(out)); err == nil { + t.Errorf("%s: parsed %q without error", name, out) + } + } +} + +// followedBy reports whether flag appears in args immediately followed by value. +func followedBy(args []string, flag, value string) bool { + for i := 0; i+1 < len(args); i++ { + if args[i] == flag && args[i+1] == value { + return true + } + } + return false +} + +// The exact tier's stored hashes must stay comparable across ffmpeg upgrades, +// which only holds while the packets are copied rather than decoded. A decoded +// hash still matches within one ffmpeg build, so nothing else would notice the +// change until an image upgrade silently broke every stored value. +func TestStreamHashArgs_HashPacketsNotSamples(t *testing.T) { + args := streamHashArgs("/music/a.mp3") + for _, pair := range [][2]string{ + {"-c:a", "copy"}, // no decode + {"-map", "0:a"}, // audio only: cover art stays out of the hash + {"-f", "hash"}, // the hash muxer, not a file + {"-hash", "sha256"}, + {"-i", "/music/a.mp3"}, + } { + if !followedBy(args, pair[0], pair[1]) { + t.Errorf("streamHashArgs lacks %s %s: %v", pair[0], pair[1], args) + } + } +} + +func TestFpcalcArgs_RequestSignedRawOutput(t *testing.T) { + args := fpcalcArgs("/music/a.flac", 90) + for _, flag := range []string{"-raw", "-signed"} { + if !slices.Contains(args, flag) { + t.Errorf("fpcalcArgs lacks %s: %v", flag, args) + } + } + if !followedBy(args, "-length", "90") { + t.Errorf("fpcalcArgs does not pass the requested length: %v", args) + } + // fpcalc takes the file as its trailing positional argument. + if args[len(args)-1] != "/music/a.flac" { + t.Errorf("path is not last: %v", args) + } +} + +func TestStderrTail_KeepsTheCauseNotTheBanner(t *testing.T) { + banner := bytes.Repeat([]byte("warning: skipping frame\n"), fpcalcStderrTail) + got := stderrTail(append(banner, []byte("ERROR: could not decode\n")...)) + if len(got) != fpcalcStderrTail { + t.Fatalf("tail is %d bytes, want the %d-byte cap", len(got), fpcalcStderrTail) + } + if !bytes.HasSuffix(got, []byte("ERROR: could not decode")) { + t.Fatalf("tail dropped the final line: ...%q", got[len(got)-40:]) + } + + if got := stderrTail([]byte(" short \n")); string(got) != "short" { + t.Fatalf("short stderr = %q, want it trimmed and whole", got) + } +} + +// A stored failure is permanent until the file changes, so the classification +// decides whether a track is ever retried. Every inconclusive case here would, +// if misfiled as a verdict, silently exclude that track from duplicate +// detection for good. +func TestIsInconclusive(t *testing.T) { + notInstalled := fmt.Errorf("fpcalc: %w", &exec.Error{Name: "fpcalc", Err: exec.ErrNotFound}) + cases := []struct { + name string + err error + want bool + }{ + {"timeout", fmt.Errorf("fpcalc: no result: %w", errFingerprintTimeout), true}, + {"scan cancelled", fmt.Errorf("ffmpeg: %w", context.Canceled), true}, + {"caller deadline", fmt.Errorf("ffmpeg: %w", context.DeadlineExceeded), true}, + {"tool not installed", notInstalled, true}, + {"tool rejected the file", errors.New("fpcalc exited 2: could not decode"), false}, + {"unparseable output", errors.New("fpcalc printed no FINGERPRINT= line"), false}, + {"success", nil, false}, + } + for _, tc := range cases { + if got := isInconclusive(tc.err); got != tc.want { + t.Errorf("%s: isInconclusive = %v, want %v", tc.name, got, tc.want) + } + } +} + +// Either half being inconclusive taints the whole result: storing the half that +// succeeded would stamp the row at the current version with the other half +// NULL, and that NULL would then read as a verdict. +func TestFingerprintResult_InconclusiveIfEitherHalfIs(t *testing.T) { + stall := fmt.Errorf("fpcalc: %w", errFingerprintTimeout) + rejected := errors.New("fpcalc exited 2") + for name, tc := range map[string]struct { + r fingerprintResult + want bool + }{ + "both succeeded": {fingerprintResult{streamSHA256: []byte{1}, chromaprint: []int32{1}}, false}, + "hash ok, print stalled": {fingerprintResult{streamSHA256: []byte{1}, printErr: stall}, true}, + "hash stalled, print ok": {fingerprintResult{hashErr: stall, chromaprint: []int32{1}}, true}, + "hash ok, print rejected": {fingerprintResult{streamSHA256: []byte{1}, printErr: rejected}, false}, + "both rejected by the file": {fingerprintResult{hashErr: rejected, printErr: rejected}, false}, + } { + if got := tc.r.inconclusive(); got != tc.want { + t.Errorf("%s: inconclusive = %v, want %v", name, got, tc.want) + } + } +} diff --git a/internal/library/scanner.go b/internal/library/scanner.go index 634c45a4..c23dbd94 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -75,10 +75,15 @@ type Scanner struct { pool *pgxpool.Pool logger *slog.Logger paths []string + // fingerprint derives a file's acoustic identity (M400). A field so an + // integration test can substitute a deterministic one: CI has no real audio + // to fingerprint, and what the test pins is WHEN the scan fingerprints, not + // what the tools print. Call it through fingerprintFile. + fingerprint func(ctx context.Context, path string) fingerprintResult } func New(pool *pgxpool.Pool, logger *slog.Logger, paths []string) *Scanner { - return &Scanner{pool: pool, logger: logger, paths: paths} + return &Scanner{pool: pool, logger: logger, paths: paths, fingerprint: computeFingerprint} } // Scan walks every configured root and upserts any audio file whose mtime is @@ -295,6 +300,25 @@ func (s *Scanner) scanFile( durationMs = probed } + // Fingerprint only bytes this row has not seen: a new path, or a file whose + // mtime moved past the row's. An unchanged file re-read for a tag repair + // keeps its stored fingerprint, for the same reason it keeps its duration + // above — a tagReadVersion bump must stay bound by tag reads, not become a + // decode of the whole library. + // + // An unchanged file with NO fingerprint yet is the backfill's job (#3908), + // deliberately not the scan's. Folding it into the skip check would make + // the first scan after an upgrade re-decode every track and push a sync + // change to every client for each one. + // + // Computed before move adoption so adoption can match on the audio hash + // (#3914); stored after the upsert, once the row id is known. + var fp fingerprintResult + fingerprinted := !unchanged + if fingerprinted { + fp = s.fingerprintFile(ctx, path) + } + // 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 @@ -359,6 +383,9 @@ func (s *Scanner) scanFile( // touches this track will re-emit the change. s.logger.Warn("library scan: LogChange track upsert failed", "track_id", track.ID, "err", err) } + if fingerprinted { + s.storeFingerprint(ctx, q, track.ID, path, fp) + } if knownTrack { stats.Updated++ From d7a8e5f300df675ef2d59c89f25027818848849b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 11 Sep 2026 14:23:01 -0400 Subject: [PATCH 3/5] =?UTF-8?q?fix(library):=20a=20track=20delete=20that?= =?UTF-8?q?=20cannot=20remove=20its=20file=20deletes=20nothing=20=E2=80=94?= =?UTF-8?q?=20#3918?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two delete paths had opposite failure policies. tracks.RemoveTrack logged a failed os.Remove and deleted the row anyway, which CASCADEs likes, plays, playlist memberships and tags, while the file survived for the next scan to re-import as a stranger. library.DeleteTrackFile stopped correctly but reported it as a bare 500 nobody could read. One path now: library.DeleteTrackFile removes the file first and, on anything but ErrNotExist, returns *FileRemoveError with nothing deleted. Only then does it delete the row and tidy an emptied album and artist in one transaction, log the sync change and clear orphaned artist art. RemoveTrack calls it, which also fixes RemoveTrack never logging a sync change. Quarantine Delete file now tidies emptied albums and artists too. Both endpoints answer an unwritable library (EROFS, EACCES, EPERM) with 409 library_not_writable. The message names the directory (removal writes to the parent), the uid:gid the server runs as, and that nothing was deleted. Other remove errors are 500 file_delete_failed with the path. The reachable surface is quarantine Delete file, which failed silently: no copy for the code on either client, and Android swallowed the exception so the row just reappeared. Web and Android now have copy for both codes and append the server message for exactly those two. Android's quarantine screen shows it in a snackbar. DELETE /api/admin/tracks/{id} has had no client since f7278f24, which kept it on purpose for a safer admin surface, so its history loss was latent. Fixed rather than removed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- .../admin/ui/AdminQuarantineScreen.kt | 11 ++ .../admin/ui/AdminQuarantineViewModel.kt | 15 +- .../com/fabledsword/minstrel/api/ErrorCopy.kt | 33 +++- .../fabledsword/minstrel/api/ErrorCopyTest.kt | 60 ++++++ internal/api/admin_quarantine.go | 7 + internal/api/admin_quarantine_test.go | 2 +- internal/api/admin_tracks.go | 15 +- internal/api/auth_test.go | 2 +- internal/api/file_remove_error.go | 58 ++++++ internal/api/file_remove_error_test.go | 96 ++++++++++ internal/library/delete.go | 175 ++++++++++++++---- internal/library/delete_test.go | 122 +++++++++++- internal/lidarrquarantine/service.go | 20 +- internal/lidarrquarantine/service_test.go | 26 +-- internal/server/server.go | 2 +- internal/tracks/service.go | 100 +++------- web/src/lib/api/errors.test.ts | 34 ++++ web/src/lib/api/errors.ts | 16 +- web/src/lib/styles/error-copy.json | 2 + 19 files changed, 647 insertions(+), 149 deletions(-) create mode 100644 android/app/src/test/java/com/fabledsword/minstrel/api/ErrorCopyTest.kt create mode 100644 internal/api/file_remove_error.go create mode 100644 internal/api/file_remove_error_test.go diff --git a/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineScreen.kt b/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineScreen.kt index 4031a5fc..586c4ce5 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineScreen.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineScreen.kt @@ -15,10 +15,14 @@ import androidx.compose.material3.HorizontalDivider import androidx.compose.material3.MaterialTheme import androidx.compose.material3.OutlinedButton import androidx.compose.material3.Scaffold +import androidx.compose.material3.SnackbarHost +import androidx.compose.material3.SnackbarHostState import androidx.compose.material3.Text import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue +import androidx.compose.runtime.remember import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.text.style.TextOverflow @@ -42,6 +46,12 @@ fun AdminQuarantineScreen( viewModel: AdminQuarantineViewModel = hiltViewModel(), ) { val state by viewModel.uiState.collectAsStateWithLifecycle() + val snackbarHostState = remember { SnackbarHostState() } + LaunchedEffect(Unit) { + viewModel.transientMessages.collect { msg -> + snackbarHostState.showSnackbar(msg) + } + } Scaffold( contentWindowInsets = ShellContentWindowInsets, modifier = Modifier.fillMaxSize(), @@ -53,6 +63,7 @@ fun AdminQuarantineScreen( onBack = { navController.popBackStack() }, ) }, + snackbarHost = { SnackbarHost(snackbarHostState) }, ) { inner -> PullToRefreshScaffold( onRefresh = { viewModel.refresh().join() }, diff --git a/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineViewModel.kt b/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineViewModel.kt index 3e05b469..85e83b8e 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineViewModel.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/admin/ui/AdminQuarantineViewModel.kt @@ -10,10 +10,13 @@ import com.fabledsword.minstrel.events.EventsStream import com.fabledsword.minstrel.models.AdminQuarantineItemRef import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.Job +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.filter +import kotlinx.coroutines.flow.receiveAsFlow import kotlinx.coroutines.launch import javax.inject.Inject @@ -34,6 +37,15 @@ class AdminQuarantineViewModel @Inject constructor( private val internal = MutableStateFlow(AdminQuarantineUiState.Loading) val uiState: StateFlow = internal.asStateFlow() + /** + * One-shot messages for the screen's snackbar. A failed action has to say + * why: the row quietly reappearing reads as a glitch, and for a Delete + * file refused by a read-only library it hides the one thing the + * operator can fix (#3918). + */ + private val transientMessagesChannel = Channel(Channel.BUFFERED) + val transientMessages: Flow = transientMessagesChannel.receiveAsFlow() + init { refresh() viewModelScope.launch { @@ -86,8 +98,9 @@ class AdminQuarantineViewModel @Inject constructor( try { action(trackId) } catch ( - @Suppress("TooGenericExceptionCaught", "SwallowedException") e: Throwable, + @Suppress("TooGenericExceptionCaught") e: Throwable, ) { + transientMessagesChannel.trySend(ErrorCopy.fromThrowable(e)) refresh() } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/api/ErrorCopy.kt b/android/app/src/main/java/com/fabledsword/minstrel/api/ErrorCopy.kt index 8456fad9..3f345851 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/api/ErrorCopy.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/api/ErrorCopy.kt @@ -37,18 +37,35 @@ object ErrorCopy { * as connection failures. */ fun fromThrowable(t: Throwable): String = when (t) { - is HttpException -> messageFor(codeFromHttp(t)) + is HttpException -> fromHttp(t) is IOException -> messageFor("connection_refused") else -> TABLE.getValue("unknown") } - private fun codeFromHttp(e: HttpException): String { + /** + * Codes whose server message carries specifics the operator needs in + * order to act — which directory, which uid — that fixed copy cannot say. + * For these the message follows the copy (#3918). Kept to a named set on + * purpose: most server messages are internal detail. Mirrors web's + * errors.ts. + */ + private val DETAIL_CODES = setOf("library_not_writable", "file_delete_failed") + + private fun fromHttp(e: HttpException): String { + val body = bodyFromHttp(e) + val copy = messageFor(body.code.ifEmpty { "unknown" }) + return if (body.code in DETAIL_CODES && body.message.isNotBlank()) { + "$copy ${body.message}" + } else { + copy + } + } + + private fun bodyFromHttp(e: HttpException): Body { val raw = runCatching { e.response()?.errorBody()?.string() }.getOrNull() - ?: return "unknown" - val code = runCatching { json.decodeFromString(raw).error?.code } - .getOrNull() - .orEmpty() - return code.ifEmpty { "unknown" } + ?: return Body() + return runCatching { json.decodeFromString(raw).error } + .getOrNull() ?: Body() } private val TABLE: Map = mapOf( @@ -99,6 +116,8 @@ object ErrorCopy { "request_not_pending" to "This request is no longer pending.", "request_not_found" to "That request no longer exists.", "track_not_found" to "That track no longer exists.", + "library_not_writable" to "The music library isn't writable by the server.", + "file_delete_failed" to "The file couldn't be deleted.", "album_not_found" to "That album no longer exists.", "artist_not_found" to "That artist no longer exists.", "playlist_not_found" to "That playlist no longer exists.", diff --git a/android/app/src/test/java/com/fabledsword/minstrel/api/ErrorCopyTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/api/ErrorCopyTest.kt new file mode 100644 index 00000000..c7b7d67a --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/api/ErrorCopyTest.kt @@ -0,0 +1,60 @@ +package com.fabledsword.minstrel.api + +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.ResponseBody.Companion.toResponseBody +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test +import retrofit2.HttpException +import retrofit2.Response +import java.io.IOException + +class ErrorCopyTest { + private fun httpError(status: Int, body: String): HttpException = + HttpException( + Response.error(status, body.toResponseBody("application/json".toMediaType())), + ) + + @Test + fun libraryNotWritableAppendsTheServerDetail() { + val detail = "Minstrel runs as uid 1000, gid 1000 and cannot delete from /music/A " + + "(read-only file system). The library mount must be writable by that user. " + + "Nothing was deleted." + val e = httpError(409, """{"error":{"code":"library_not_writable","message":"$detail"}}""") + + assertEquals( + "${ErrorCopy.messageFor("library_not_writable")} $detail", + ErrorCopy.fromThrowable(e), + ) + } + + @Test + fun detailCodeWithoutAMessageShowsTheCopyAlone() { + val e = httpError(409, """{"error":{"code":"library_not_writable","message":""}}""") + + assertEquals(ErrorCopy.messageFor("library_not_writable"), ErrorCopy.fromThrowable(e)) + } + + // Server messages are usually internal detail; appending them for every + // code would leak driver errors into snackbars. This pins the scope. + @Test + fun otherCodesNeverCarryTheServerMessage() { + val e = httpError(404, """{"error":{"code":"track_not_found","message":"pgx: no rows"}}""") + + assertEquals(ErrorCopy.messageFor("track_not_found"), ErrorCopy.fromThrowable(e)) + } + + @Test + fun anUnparseableBodyFallsBackToUnknown() { + val e = httpError(500, "not json") + + assertEquals(ErrorCopy.messageFor("unknown"), ErrorCopy.fromThrowable(e)) + } + + @Test + fun transportFailureMapsToConnectionRefused() { + assertEquals( + ErrorCopy.messageFor("connection_refused"), + ErrorCopy.fromThrowable(IOException("refused")), + ) + } +} diff --git a/internal/api/admin_quarantine.go b/internal/api/admin_quarantine.go index 116f9048..bd372d3f 100644 --- a/internal/api/admin_quarantine.go +++ b/internal/api/admin_quarantine.go @@ -133,6 +133,13 @@ func (h *handlers) handleDeleteQuarantineFile(w http.ResponseWriter, r *http.Req } action, err := h.lidarrQuarantine.DeleteFile(r.Context(), id, admin.ID) if err != nil { + // Written in the enveloped shape, not writeAdminJSONErr's bare code: the + // message is the part that tells the operator which directory and uid. + if apiErr, ok := fileRemoveAPIError(err); ok { + logFileRemoveFailure(h.logger, apiErr, "track_id", uuidToString(id)) + writeErr(w, apiErr) + return + } switch { case errors.Is(err, lidarrquarantine.ErrTrackNotFound): writeAdminJSONErr(w, http.StatusNotFound, "track_not_found") diff --git a/internal/api/admin_quarantine_test.go b/internal/api/admin_quarantine_test.go index 59cd7b49..5318d5b3 100644 --- a/internal/api/admin_quarantine_test.go +++ b/internal/api/admin_quarantine_test.go @@ -69,7 +69,7 @@ func installQuarantineClientFn(t *testing.T, h *handlers) { } return lidarr.NewClient(c.BaseURL, c.APIKey) } - h.lidarrQuarantine = lidarrquarantine.NewService(h.pool, cfg, clientFn) + h.lidarrQuarantine = lidarrquarantine.NewService(h.pool, cfg, clientFn, h.dataDir) } // flagDirect bypasses the HTTP handler to seed a quarantine row via the diff --git a/internal/api/admin_tracks.go b/internal/api/admin_tracks.go index 251d287b..7630fa27 100644 --- a/internal/api/admin_tracks.go +++ b/internal/api/admin_tracks.go @@ -23,15 +23,17 @@ type removeTrackResponse struct { // handleRemoveTrack implements DELETE /api/admin/tracks/{id}?unmonitor=true|false. // -// Admin-only (gated by auth.RequireAdmin on the /admin route group). Always -// deletes the file + DB row and runs the album/artist cascade tidy-up. When +// Admin-only (gated by auth.RequireAdmin on the /admin route group). Deletes the +// file, then the DB row, and runs the album/artist cascade tidy-up — and deletes +// nothing at all when the file cannot be removed (#3918). When // unmonitor=true and the track has an mbid, also calls Lidarr.UnmonitorTrack // — failure there is non-fatal (the destructive part already completed) and // surfaces as `lidarr_unmonitor_failed: true` in the success envelope. // // Per spec §5, Lidarr-side errors during the unmonitor step do NOT map to -// wire error codes; the only error codes this handler emits are not_found, -// server_error, plus the auth codes the middleware emits upstream. +// wire error codes. The codes this handler emits are not_found, +// library_not_writable (409) and file_delete_failed when the file could not be +// removed, server_error, plus the auth codes the middleware emits upstream. func (h *handlers) handleRemoveTrack(w http.ResponseWriter, r *http.Request) { idStr := chi.URLParam(r, "id") trackID, ok := parseUUID(idStr) @@ -66,6 +68,11 @@ func (h *handlers) handleRemoveTrack(w http.ResponseWriter, r *http.Request) { writeErr(w, &apierror.Error{Status: http.StatusNotFound, Code: "not_found", Message: "track not found"}) return } + if apiErr, ok := fileRemoveAPIError(err); ok { + logFileRemoveFailure(h.logger, apiErr, "track_id", idStr) + writeErr(w, apiErr) + return + } h.logger.Error("api: remove track failed", "err", err, "track_id", idStr) writeErr(w, apierror.InternalMsg("remove failed", err)) return diff --git a/internal/api/auth_test.go b/internal/api/auth_test.go index ae564b9c..171c0f8a 100644 --- a/internal/api/auth_test.go +++ b/internal/api/auth_test.go @@ -65,7 +65,7 @@ func testHandlers(t *testing.T) (*handlers, *pgxpool.Pool) { } lidarrCfg := lidarrconfig.New(pool) lidarrReqs := lidarrrequests.NewService(pool, lidarrCfg, nil, nil) - lidarrQuar := lidarrquarantine.NewService(pool, lidarrCfg, nil) + lidarrQuar := lidarrquarantine.NewService(pool, lidarrCfg, nil, "") // tracks.Service has no Lidarr unmonitorer in tests by default; the // admin-tracks tests below override h.tracks via installTracksLidarrStub // when they need a stubbed Lidarr. diff --git a/internal/api/file_remove_error.go b/internal/api/file_remove_error.go new file mode 100644 index 00000000..3b2441c2 --- /dev/null +++ b/internal/api/file_remove_error.go @@ -0,0 +1,58 @@ +package api + +import ( + "errors" + "fmt" + "log/slog" + "net/http" + + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" + "git.fabledsword.com/bvandeusen/minstrel/internal/library" +) + +// fileRemoveAPIError answers a delete that could not reach the track's file +// (#3918). Both delete endpoints use it, so the operator gets the same +// explanation from the admin remove-track action and from quarantine's Delete +// file. +// +// The unwritable case is a 409 rather than a 500 because nothing is broken: the +// request conflicts with how the library is mounted, and the fix is the +// operator's. The message names the directory — removal writes to the parent, +// not the file — and the uid/gid the process runs as, which is the half of a +// permission problem invisible from the host. Every case says nothing was +// deleted, because that is exactly what the operator will be worried about. +func fileRemoveAPIError(err error) (*apierror.Error, bool) { + var fre *library.FileRemoveError + if !errors.As(err, &fre) { + return nil, false + } + if fre.NotWritable() { + return &apierror.Error{ + Status: http.StatusConflict, + Code: "library_not_writable", + Message: fmt.Sprintf( + "Minstrel runs as uid %d, gid %d and cannot delete from %s (%s). "+ + "The library mount must be writable by that user. Nothing was deleted.", + fre.UID, fre.GID, fre.Dir(), fre.Reason()), + Cause: err, + }, true + } + return &apierror.Error{ + Status: http.StatusInternalServerError, + Code: "file_delete_failed", + Message: fmt.Sprintf("Could not delete %s (%s). Nothing was deleted.", fre.Path, fre.Reason()), + Cause: err, + }, true +} + +// logFileRemoveFailure records a delete that could not reach its file. An +// unwritable library is an environment fact the operator can fix, so it is a +// Warn; anything else is a real fault. +func logFileRemoveFailure(logger *slog.Logger, apiErr *apierror.Error, attrs ...any) { + attrs = append(attrs, "code", apiErr.Code, "err", apiErr.Cause) + if apiErr.Status == http.StatusConflict { + logger.Warn("api: track file could not be deleted", attrs...) + return + } + logger.Error("api: track file could not be deleted", attrs...) +} diff --git a/internal/api/file_remove_error_test.go b/internal/api/file_remove_error_test.go new file mode 100644 index 00000000..1da062da --- /dev/null +++ b/internal/api/file_remove_error_test.go @@ -0,0 +1,96 @@ +package api + +import ( + "errors" + "fmt" + "io/fs" + "net/http" + "strings" + "syscall" + "testing" + + "git.fabledsword.com/bvandeusen/minstrel/internal/library" +) + +const removeTestPath = "/music/Moe Shop/WWW (2020)/01 - WWW.mp3" + +// removeFailure builds the error a delete service returns when the file would +// not go, wrapped the way lidarrquarantine.DeleteFile and tracks.RemoveTrack +// wrap it — the mapping has to see through that. +func removeFailure(errno syscall.Errno) error { + return fmt.Errorf("delete file: %w", &library.FileRemoveError{ + Path: removeTestPath, UID: 1000, GID: 1000, + Err: &fs.PathError{Op: "remove", Path: removeTestPath, Err: errno}, + }) +} + +func TestFileRemoveAPIError(t *testing.T) { + cases := []struct { + name string + errno syscall.Errno + wantStatus int + wantCode string + wantIn []string + }{ + { + name: "read-only mount", errno: syscall.EROFS, + wantStatus: http.StatusConflict, wantCode: "library_not_writable", + wantIn: []string{"uid 1000, gid 1000", "/music/Moe Shop/WWW (2020)", "read-only file system", "Nothing was deleted"}, + }, + { + name: "permission denied", errno: syscall.EACCES, + wantStatus: http.StatusConflict, wantCode: "library_not_writable", + wantIn: []string{"permission denied", "Nothing was deleted"}, + }, + { + name: "operation not permitted", errno: syscall.EPERM, + wantStatus: http.StatusConflict, wantCode: "library_not_writable", + wantIn: []string{"operation not permitted"}, + }, + { + name: "i/o error", errno: syscall.EIO, + wantStatus: http.StatusInternalServerError, wantCode: "file_delete_failed", + wantIn: []string{removeTestPath, "input/output error", "Nothing was deleted"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + apiErr, ok := fileRemoveAPIError(removeFailure(tc.errno)) + if !ok { + t.Fatal("a wrapped *library.FileRemoveError was not recognised") + } + if apiErr.Status != tc.wantStatus || apiErr.Code != tc.wantCode { + t.Fatalf("got %d %s, want %d %s", apiErr.Status, apiErr.Code, tc.wantStatus, tc.wantCode) + } + for _, want := range tc.wantIn { + if !strings.Contains(apiErr.Message, want) { + t.Errorf("message %q lacks %q", apiErr.Message, want) + } + } + }) + } +} + +// The unwritable answer must name the DIRECTORY. Removal needs write access to +// the parent, so a message naming the file would send the operator to fix the +// wrong permissions. The directory is a prefix of the file path, which is why a +// plain "contains the directory" check could never catch that regression. +func TestFileRemoveAPIError_NotWritableNamesTheDirectoryNotTheFile(t *testing.T) { + apiErr, _ := fileRemoveAPIError(removeFailure(syscall.EROFS)) + if strings.Contains(apiErr.Message, "01 - WWW.mp3") { + t.Fatalf("message names the file rather than its directory: %q", apiErr.Message) + } +} + +func TestFileRemoveAPIError_IgnoresOtherErrors(t *testing.T) { + for name, err := range map[string]error{ + "nil": nil, + "plain error": errors.New("delete track: connection reset"), + "path error": &fs.PathError{Op: "remove", Path: removeTestPath, Err: syscall.EROFS}, + "not found": library.ErrTrackNotFound, + } { + if _, ok := fileRemoveAPIError(err); ok { + t.Errorf("%s: mapped as a file-remove failure", name) + } + } +} diff --git a/internal/library/delete.go b/internal/library/delete.go index 81f5bd58..70d3ece5 100644 --- a/internal/library/delete.go +++ b/internal/library/delete.go @@ -5,12 +5,16 @@ import ( "errors" "fmt" "io/fs" + "log/slog" "os" + "path/filepath" + "syscall" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" "github.com/jackc/pgx/v5/pgxpool" + "git.fabledsword.com/bvandeusen/minstrel/internal/coverart" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" ) @@ -19,54 +23,159 @@ import ( // that has no row in tracks. var ErrTrackNotFound = errors.New("library: track not found") -// DeleteTrackFile removes a track file from disk and its row from the -// tracks table. Album and artist rows are left untouched. +// removeFile is os.Remove behind a variable so a test can make removal fail the +// way a read-only mount or a wrongly-owned directory does. A chmod-based test +// cannot stand in for that: root ignores permission bits, so in a CI container +// running as root it would pass without ever exercising the failure. +var removeFile = os.Remove + +// FileRemoveError reports that a track's file exists but could not be removed. +// When DeleteTrackFile returns one, NOTHING was deleted: the row, its likes, its +// play history and its playlist memberships are all intact. +type FileRemoveError struct { + Path string + // UID and GID are the identity the server process runs as — the half of a + // permission problem the operator cannot see from the host side. + UID, GID int + Err error +} + +func (e *FileRemoveError) Error() string { return fmt.Sprintf("remove track file: %v", e.Err) } + +func (e *FileRemoveError) Unwrap() error { return e.Err } + +// Dir is the directory removal needs write access to. Unlinking a file writes to +// its PARENT, so a world-writable file inside a read-only directory still cannot +// be removed — naming the file's own permissions would send the operator to the +// wrong place. +func (e *FileRemoveError) Dir() string { return filepath.Dir(e.Path) } + +// NotWritable reports whether the library is unwritable for this process — a +// read-only mount or a permission denial — rather than an I/O fault. It is the +// case the operator can fix, so callers answer it differently. +func (e *FileRemoveError) NotWritable() bool { + return errors.Is(e.Err, fs.ErrPermission) || errors.Is(e.Err, syscall.EROFS) +} + +// Reason is the underlying cause without the path os.Remove already wrapped +// around it, for messages that name the directory themselves. +func (e *FileRemoveError) Reason() string { + var pathErr *fs.PathError + if errors.As(e.Err, &pathErr) { + return pathErr.Err.Error() + } + return e.Err.Error() +} + +// DeletedTrack reports what a delete tidied away beyond the track itself. +type DeletedTrack struct { + // AlbumID is set when the track was its album's last, so the album went too. + AlbumID *pgtype.UUID + // ArtistID is set when that album was its artist's last, so the artist went too. + ArtistID *pgtype.UUID +} + +// DeleteTrackFile removes a track's file from disk and then its row, tidying +// away an album or artist the delete leaves empty. It is the ONLY path that +// deletes a track file: the admin remove-track endpoint and quarantine's Delete +// file both come through here (#3918). // -// Steps: -// 1. Look up the track to get its file_path. -// 2. Remove the file from disk. fs.ErrNotExist is OK — already gone. -// 3. Delete the tracks row. +// Order is the whole contract. The file goes first, and if it cannot go — a +// read-only mount, a permission denial, an I/O error — nothing else happens and +// a *FileRemoveError comes back. Proceeding past that failure is how #3918 lost +// history: tracks CASCADEs to play_events, general_likes, contextual_likes, +// playlist_tracks, track_tags and playback_errors, so the row and everything +// hanging off it were destroyed while the file survived, and the next scan +// re-imported it as a brand-new track with none of it. // -// 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. +// A file that is already gone (fs.ErrNotExist) is not a failure; the row is +// removed as asked. // -// 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). +// This is NOT the missing-file path. That lifecycle is deliberately +// non-destructive: reconcile stamps missing_since (#2523), selection paths +// filter on it, and a returning file is un-marked or adopted (#2528). This is the +// explicit, irreversible "remove this recording", never the way to tidy up a row +// whose file merely went away. // -// 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 { +// dataDir, when set, also clears the cached art of an artist the delete removed. +// logger may be nil. +func DeleteTrackFile( + ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string, trackID pgtype.UUID, +) (DeletedTrack, error) { + if logger == nil { + logger = slog.Default() + } q := dbq.New(pool) track, err := q.GetTrackByID(ctx, trackID) if err != nil { if errors.Is(err, pgx.ErrNoRows) { - return ErrTrackNotFound + return DeletedTrack{}, ErrTrackNotFound } - return fmt.Errorf("get track: %w", err) + return DeletedTrack{}, fmt.Errorf("get track: %w", err) } - if err := os.Remove(track.FilePath); err != nil && !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("remove file: %w", err) + if err := removeFile(track.FilePath); err != nil && !errors.Is(err, fs.ErrNotExist) { + return DeletedTrack{}, &FileRemoveError{ + Path: track.FilePath, UID: os.Getuid(), GID: os.Getgid(), Err: err, + } } - if _, err := pool.Exec(ctx, "DELETE FROM tracks WHERE id = $1", trackID); err != nil { - return fmt.Errorf("delete row: %w", err) + // The row and any album or artist it empties go together, so a failure + // partway cannot leave a deleted track with a ghost album behind it. + tx, err := pool.Begin(ctx) + if err != nil { + return DeletedTrack{}, fmt.Errorf("begin tx: %w", err) } - // Log the change after the delete succeeds. Best-effort: a Warn-level - // failure here would leave the cache index orphaned on offline clients - // until the next scan touches the surrounding album. + defer func() { _ = tx.Rollback(ctx) }() + tq := dbq.New(tx) + + deleted, err := tq.DeleteTrack(ctx, trackID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + // Removed by someone else between the lookup and here. + return DeletedTrack{}, ErrTrackNotFound + } + return DeletedTrack{}, fmt.Errorf("delete track: %w", err) + } + + var out DeletedTrack + album, err := tq.DeleteAlbumIfEmpty(ctx, deleted.AlbumID) + switch { + case err == nil: + albumID := album.ID + out.AlbumID = &albumID + artistID, aerr := tq.DeleteArtistIfEmpty(ctx, album.ArtistID) + switch { + case aerr == nil: + out.ArtistID = &artistID + case errors.Is(aerr, pgx.ErrNoRows): + // The artist still has other albums or stray tracks. + default: + return DeletedTrack{}, fmt.Errorf("delete artist if empty: %w", aerr) + } + case errors.Is(err, pgx.ErrNoRows): + // The album still has other tracks. + default: + return DeletedTrack{}, fmt.Errorf("delete album if empty: %w", err) + } + + if err := tx.Commit(ctx); err != nil { + return DeletedTrack{}, fmt.Errorf("commit: %w", err) + } + + // Both of these run after the delete has committed, so neither may fail + // it: the recording is gone either way. An unlogged change leaves the track + // in offline clients' caches until the next scan touches its album; a + // leftover art directory is only disk. if err := syncpkg.LogChange(ctx, pool, syncpkg.EntityTrack, syncpkg.FormatUUID(trackID), syncpkg.OpDelete); err != nil { - return fmt.Errorf("log change: %w", err) + logger.Warn("track delete: LogChange failed", "track_id", syncpkg.FormatUUID(trackID), "err", err) } - return nil + if out.ArtistID != nil && dataDir != "" { + if err := coverart.CleanupArtistArt(dataDir, *out.ArtistID); err != nil { + logger.Warn("track delete: artist-art cleanup failed", + "artist_id", syncpkg.FormatUUID(*out.ArtistID), "err", err) + } + } + return out, nil } diff --git a/internal/library/delete_test.go b/internal/library/delete_test.go index 9952bffd..76cf9ae2 100644 --- a/internal/library/delete_test.go +++ b/internal/library/delete_test.go @@ -4,9 +4,11 @@ import ( "context" "errors" "io" + "io/fs" "log/slog" "os" "path/filepath" + "syscall" "testing" "github.com/jackc/pgx/v5/pgtype" @@ -64,6 +66,15 @@ func seedTrack(t *testing.T, pool *pgxpool.Pool, filePath string) (dbq.Track, db return track, album, artist } +// stubRemoveFile makes file removal fail (or succeed) on demand for one test. +// See removeFile for why this is a seam rather than a chmod. +func stubRemoveFile(t *testing.T, fn func(string) error) { + t.Helper() + orig := removeFile + removeFile = fn + t.Cleanup(func() { removeFile = orig }) +} + func TestDeleteTrackFile_HappyPath(t *testing.T) { pool := newPool(t) q := dbq.New(pool) @@ -73,9 +84,18 @@ func TestDeleteTrackFile_HappyPath(t *testing.T) { if err := os.WriteFile(path, []byte("payload"), 0o644); err != nil { t.Fatalf("write file: %v", err) } - track, album, _ := seedTrack(t, pool, path) + track, album, artist := seedTrack(t, pool, path) + // A sibling keeps the album non-empty, so this case pins that the tidy-up + // only removes an album the delete actually emptied. + if _, err := q.UpsertTrack(context.Background(), dbq.UpsertTrackParams{ + Title: "Sibling", AlbumID: album.ID, ArtistID: artist.ID, + DurationMs: 1000, FilePath: filepath.Join(dir, "sibling.mp3"), FileSize: 100, FileFormat: "mp3", + }); err != nil { + t.Fatalf("sibling: %v", err) + } - if err := DeleteTrackFile(context.Background(), pool, track.ID); err != nil { + got, err := DeleteTrackFile(context.Background(), pool, nil, "", track.ID) + if err != nil { t.Fatalf("DeleteTrackFile: %v", err) } @@ -85,9 +105,99 @@ func TestDeleteTrackFile_HappyPath(t *testing.T) { if _, err := q.GetTrackByID(context.Background(), track.ID); err == nil { t.Errorf("track row still exists") } - // Album row preserved (other tracks may reference it). if _, err := q.GetAlbumByID(context.Background(), album.ID); err != nil { - t.Errorf("album row vanished: %v", err) + t.Errorf("album with a remaining track vanished: %v", err) + } + if got.AlbumID != nil || got.ArtistID != nil { + t.Errorf("reported tidy-up %+v for an album that still has a track", got) + } +} + +func TestDeleteTrackFile_EmptiedAlbumAndArtistGoToo(t *testing.T) { + pool := newPool(t) + q := dbq.New(pool) + + path := filepath.Join(t.TempDir(), "lone.mp3") + if err := os.WriteFile(path, []byte("payload"), 0o644); err != nil { + t.Fatalf("write file: %v", err) + } + track, album, artist := seedTrack(t, pool, path) + + got, err := DeleteTrackFile(context.Background(), pool, nil, "", track.ID) + if err != nil { + t.Fatalf("DeleteTrackFile: %v", err) + } + if got.AlbumID == nil || *got.AlbumID != album.ID { + t.Errorf("AlbumID = %v, want %v", got.AlbumID, album.ID) + } + if got.ArtistID == nil || *got.ArtistID != artist.ID { + t.Errorf("ArtistID = %v, want %v", got.ArtistID, artist.ID) + } + if _, err := q.GetAlbumByID(context.Background(), album.ID); err == nil { + t.Errorf("emptied album row still exists") + } + if _, err := q.GetArtistByID(context.Background(), artist.ID); err == nil { + t.Errorf("emptied artist row still exists") + } +} + +// The #3918 proof. A file that cannot be removed must leave EVERYTHING in place: +// the row is what carries likes, plays and playlist memberships, and the file +// surviving means the next scan would re-import it as a stranger. +func TestDeleteTrackFile_UnremovableFileDeletesNothing(t *testing.T) { + cases := []struct { + name string + errno syscall.Errno + notWritable bool + }{ + {"read-only mount", syscall.EROFS, true}, + {"permission denied", syscall.EACCES, true}, + {"operation not permitted", syscall.EPERM, true}, + {"i/o error", syscall.EIO, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + pool := newPool(t) + q := dbq.New(pool) + + dir := t.TempDir() + path := filepath.Join(dir, "track.mp3") + if err := os.WriteFile(path, []byte("payload"), 0o644); err != nil { + t.Fatalf("write file: %v", err) + } + track, album, _ := seedTrack(t, pool, path) + stubRemoveFile(t, func(name string) error { + return &fs.PathError{Op: "remove", Path: name, Err: tc.errno} + }) + + _, err := DeleteTrackFile(context.Background(), pool, nil, "", track.ID) + + var fre *FileRemoveError + if !errors.As(err, &fre) { + t.Fatalf("err = %v, want a *FileRemoveError", err) + } + if fre.NotWritable() != tc.notWritable { + t.Errorf("NotWritable = %v, want %v", fre.NotWritable(), tc.notWritable) + } + if fre.Dir() != dir { + t.Errorf("Dir = %q, want the parent directory %q", fre.Dir(), dir) + } + if fre.Reason() != tc.errno.Error() { + t.Errorf("Reason = %q, want %q", fre.Reason(), tc.errno.Error()) + } + if fre.UID != os.Getuid() || fre.GID != os.Getgid() { + t.Errorf("identity = %d:%d, want this process's %d:%d", fre.UID, fre.GID, os.Getuid(), os.Getgid()) + } + if _, err := q.GetTrackByID(context.Background(), track.ID); err != nil { + t.Errorf("track row was deleted although its file was not: %v", err) + } + if _, err := q.GetAlbumByID(context.Background(), album.ID); err != nil { + t.Errorf("album row was deleted although the track's file was not: %v", err) + } + if _, err := os.Stat(path); err != nil { + t.Errorf("file gone although removal was refused: %v", err) + } + }) } } @@ -97,7 +207,7 @@ func TestDeleteTrackFile_FileAlreadyGoneSucceeds(t *testing.T) { track, _, _ := seedTrack(t, pool, "/no/such/file/anywhere.mp3") - if err := DeleteTrackFile(context.Background(), pool, track.ID); err != nil { + if _, err := DeleteTrackFile(context.Background(), pool, nil, "", track.ID); err != nil { t.Fatalf("DeleteTrackFile with missing file: %v", err) } if _, err := q.GetTrackByID(context.Background(), track.ID); err == nil { @@ -112,7 +222,7 @@ func TestDeleteTrackFile_NotFoundReturnsErr(t *testing.T) { bogus.Bytes = [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16} bogus.Valid = true - err := DeleteTrackFile(context.Background(), pool, bogus) + _, err := DeleteTrackFile(context.Background(), pool, nil, "", bogus) if !errors.Is(err, ErrTrackNotFound) { t.Errorf("err = %v, want ErrTrackNotFound", err) } diff --git a/internal/lidarrquarantine/service.go b/internal/lidarrquarantine/service.go index 3a402342..54a97abe 100644 --- a/internal/lidarrquarantine/service.go +++ b/internal/lidarrquarantine/service.go @@ -35,6 +35,9 @@ var ( // config changes in lidarrconfig take effect immediately. clientFn returns // nil when Lidarr is disabled. type Service struct { + // dataDir lets a Delete file that empties an artist clear that artist's + // cached art, the same as the admin remove-track path. + dataDir string pool *pgxpool.Pool lidarrCfg *lidarrconfig.Service clientFn func() *lidarr.Client @@ -42,11 +45,11 @@ type Service struct { // NewService constructs a Service. Pass nil for clientFn to disable the // Lidarr-using methods (DeleteViaLidarr will return ErrLidarrDisabled). -func NewService(pool *pgxpool.Pool, cfg *lidarrconfig.Service, clientFn func() *lidarr.Client) *Service { +func NewService(pool *pgxpool.Pool, cfg *lidarrconfig.Service, clientFn func() *lidarr.Client, dataDir string) *Service { if clientFn == nil { clientFn = func() *lidarr.Client { return nil } } - return &Service{pool: pool, lidarrCfg: cfg, clientFn: clientFn} + return &Service{pool: pool, lidarrCfg: cfg, clientFn: clientFn, dataDir: dataDir} } // Flag inserts or updates a quarantine row for the caller. Re-flagging @@ -245,10 +248,13 @@ func (s *Service) snapshot(ctx context.Context, q *dbq.Queries, track dbq.Track) }, nil } -// DeleteFile removes the track file from disk and the tracks row, then -// (via FK ON DELETE CASCADE) clears all per-user quarantine rows for that -// track and writes an audit row. If the file deletion fails, the per-user -// rows stay so admin can retry. No partial state. +// DeleteFile removes the track file from disk and the tracks row (tidying away +// an album or artist that leaves empty), then — via FK ON DELETE CASCADE — +// clears every per-user quarantine row for that track and writes an audit row. +// +// If the file cannot be removed, nothing is deleted and the per-user rows stay +// so the admin can retry: the error wraps a *library.FileRemoveError, which the +// handler turns into an answer naming the cause (#3918). No partial state. func (s *Service) DeleteFile(ctx context.Context, trackID, adminID pgtype.UUID) (dbq.LidarrQuarantineAction, error) { q := dbq.New(s.pool) track, err := q.GetTrackByID(ctx, trackID) @@ -267,7 +273,7 @@ func (s *Service) DeleteFile(ctx context.Context, trackID, adminID pgtype.UUID) if err != nil { return dbq.LidarrQuarantineAction{}, fmt.Errorf("count: %w", err) } - if err := library.DeleteTrackFile(ctx, s.pool, trackID); err != nil { + if _, err := library.DeleteTrackFile(ctx, s.pool, nil, s.dataDir, trackID); err != nil { return dbq.LidarrQuarantineAction{}, fmt.Errorf("delete file: %w", err) } // tracks row is gone; ON DELETE CASCADE on lidarr_quarantine.track_id diff --git a/internal/lidarrquarantine/service_test.go b/internal/lidarrquarantine/service_test.go index cca8e26c..9f14ae37 100644 --- a/internal/lidarrquarantine/service_test.go +++ b/internal/lidarrquarantine/service_test.go @@ -87,7 +87,7 @@ func TestFlag_HappyPath(t *testing.T) { user := seedUser(t, pool, "alice") track, _, _ := seedTrack(t, pool, "Bad Track", "abc") - svc := NewService(pool, lidarrconfig.New(pool), nil) + svc := NewService(pool, lidarrconfig.New(pool), nil, "") row, err := svc.Flag(context.Background(), user.ID, track.ID, "bad_rip", "crackly") if err != nil { t.Fatalf("Flag: %v", err) @@ -105,7 +105,7 @@ func TestFlag_UpsertOnSecondFlag(t *testing.T) { user := seedUser(t, pool, "alice") track, _, _ := seedTrack(t, pool, "T", "x") - svc := NewService(pool, lidarrconfig.New(pool), nil) + svc := NewService(pool, lidarrconfig.New(pool), nil, "") if _, err := svc.Flag(context.Background(), user.ID, track.ID, "bad_rip", "first"); err != nil { t.Fatalf("first flag: %v", err) } @@ -129,7 +129,7 @@ func TestFlag_NonexistentTrackReturnsErrTrackNotFound(t *testing.T) { bogus.Bytes = [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16} bogus.Valid = true - svc := NewService(pool, lidarrconfig.New(pool), nil) + svc := NewService(pool, lidarrconfig.New(pool), nil, "") _, err := svc.Flag(context.Background(), user.ID, bogus, "bad_rip", "") if !errors.Is(err, ErrTrackNotFound) { t.Errorf("err = %v, want ErrTrackNotFound", err) @@ -141,7 +141,7 @@ func TestFlag_BadReasonRejected(t *testing.T) { user := seedUser(t, pool, "alice") track, _, _ := seedTrack(t, pool, "T", "x") - svc := NewService(pool, lidarrconfig.New(pool), nil) + svc := NewService(pool, lidarrconfig.New(pool), nil, "") _, err := svc.Flag(context.Background(), user.ID, track.ID, "garbage", "") if !errors.Is(err, ErrBadReason) { t.Errorf("err = %v, want ErrBadReason", err) @@ -153,7 +153,7 @@ func TestUnflag_DeletesRow(t *testing.T) { user := seedUser(t, pool, "alice") track, _, _ := seedTrack(t, pool, "T", "x") - svc := NewService(pool, lidarrconfig.New(pool), nil) + svc := NewService(pool, lidarrconfig.New(pool), nil, "") if _, err := svc.Flag(context.Background(), user.ID, track.ID, "bad_rip", ""); err != nil { t.Fatalf("Flag: %v", err) } @@ -171,7 +171,7 @@ func TestListMine_OrderedNewestFirst(t *testing.T) { t1, _, _ := seedTrack(t, pool, "T1", "x") t2, _, _ := seedTrack(t, pool, "T2", "y") - svc := NewService(pool, lidarrconfig.New(pool), nil) + svc := NewService(pool, lidarrconfig.New(pool), nil, "") if _, err := svc.Flag(context.Background(), user.ID, t1.ID, "bad_rip", ""); err != nil { t.Fatalf("Flag t1: %v", err) } @@ -199,7 +199,7 @@ func TestListAdminQueue_AggregatesByTrackWithReasonCounts(t *testing.T) { carol := seedUser(t, pool, "carol") track, _, _ := seedTrack(t, pool, "Hot Mess", "abc") - svc := NewService(pool, lidarrconfig.New(pool), nil) + svc := NewService(pool, lidarrconfig.New(pool), nil, "") if _, err := svc.Flag(context.Background(), alice.ID, track.ID, "bad_rip", ""); err != nil { t.Fatalf("alice flag: %v", err) } @@ -235,7 +235,7 @@ func TestResolve_ClearsRowsAndWritesAudit(t *testing.T) { bob := seedUser(t, pool, "bob") track, _, _ := seedTrack(t, pool, "T", "x") - svc := NewService(pool, lidarrconfig.New(pool), nil) + svc := NewService(pool, lidarrconfig.New(pool), nil, "") if _, err := svc.Flag(context.Background(), alice.ID, track.ID, "bad_rip", ""); err != nil { t.Fatalf("alice flag: %v", err) } @@ -267,7 +267,7 @@ func TestResolve_NoExistingRowsStillWritesAudit(t *testing.T) { user := seedUser(t, pool, "alice") track, _, _ := seedTrack(t, pool, "T", "x") - svc := NewService(pool, lidarrconfig.New(pool), nil) + svc := NewService(pool, lidarrconfig.New(pool), nil, "") // No flags applied — resolve a track with zero quarantine rows. audit, err := svc.Resolve(context.Background(), track.ID, user.ID) if err != nil { @@ -291,7 +291,7 @@ func TestResolve_TrackNotFound(t *testing.T) { bogus.Bytes = [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16} bogus.Valid = true - svc := NewService(pool, lidarrconfig.New(pool), nil) + svc := NewService(pool, lidarrconfig.New(pool), nil, "") _, err := svc.Resolve(context.Background(), bogus, user.ID) if !errors.Is(err, ErrTrackNotFound) { t.Errorf("err = %v, want ErrTrackNotFound", err) @@ -315,7 +315,7 @@ func TestDeleteFile_RemovesFileAndAuditsAffected(t *testing.T) { DurationMs: 1000, FilePath: path, FileSize: 1, FileFormat: "mp3", }) - svc := NewService(pool, lidarrconfig.New(pool), nil) + svc := NewService(pool, lidarrconfig.New(pool), nil, "") if _, err := svc.Flag(context.Background(), user.ID, track.ID, "bad_rip", ""); err != nil { t.Fatalf("Flag: %v", err) } @@ -424,7 +424,7 @@ func TestDeleteViaLidarr_LidarrDisabled(t *testing.T) { user := seedUser(t, pool, "alice") track, _, _ := seedTrack(t, pool, "T", "x") - svc := NewService(pool, lidarrconfig.New(pool), nil) + svc := NewService(pool, lidarrconfig.New(pool), nil, "") _, _, err := svc.DeleteViaLidarr(context.Background(), track.ID, user.ID) if !errors.Is(err, ErrLidarrDisabled) { t.Errorf("err = %v, want ErrLidarrDisabled", err) @@ -499,7 +499,7 @@ func TestDeleteFile_TrackNotFound(t *testing.T) { bogus.Bytes = [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16} bogus.Valid = true - svc := NewService(pool, lidarrconfig.New(pool), nil) + svc := NewService(pool, lidarrconfig.New(pool), nil, "") _, err := svc.DeleteFile(context.Background(), bogus, user.ID) if !errors.Is(err, ErrTrackNotFound) { t.Errorf("err = %v, want ErrTrackNotFound", err) diff --git a/internal/server/server.go b/internal/server/server.go index af4780c0..37b13da2 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -160,7 +160,7 @@ func (s *Server) Router() http.Handler { if raErr != nil { s.Logger.Warn("reacquisition settings unavailable; serving defaults", "err", raErr) } - lidarrQuar := lidarrquarantine.NewService(s.Pool, lidarrCfg, lidarrClientFn) + lidarrQuar := lidarrquarantine.NewService(s.Pool, lidarrCfg, lidarrClientFn, s.DataDir) tracksSvc := tracks.NewService(s.Pool, s.Logger, lidarrUnmonitorAdapter{fn: lidarrClientFn}, s.DataDir) playlistsSvc := playlists.NewService(s.Pool, s.Logger, s.DataDir) smtpSender := mailer.NewSMTPSender(s.Pool, s.Logger.With("component", "mailer")) diff --git a/internal/tracks/service.go b/internal/tracks/service.go index 27194e48..7abb98ec 100644 --- a/internal/tracks/service.go +++ b/internal/tracks/service.go @@ -1,9 +1,9 @@ -// Package tracks owns the track-level admin actions exposed by the -// M7 #372 track-actions menu. Today that's RemoveTrack: the destructive -// part is always handled directly by Minstrel (os.Remove + DB delete + -// cascade); when the operator opts in via `unmonitor=true` the service -// also tells Lidarr to flip the track's monitored flag off so Lidarr -// doesn't search for a replacement. +// Package tracks owns the track-level admin actions behind DELETE +// /api/admin/tracks/{id}. Today that's RemoveTrack: the destructive part goes +// through library.DeleteTrackFile — the one path that deletes a track file — +// and when the operator opts in via `unmonitor=true` the service also tells +// Lidarr to flip the track's monitored flag off so Lidarr doesn't search for a +// replacement. // // History: an earlier shape (commit 50a231f, since rewritten) routed // Lidarr-managed tracks through lidarrquarantine.DeleteViaLidarr — but @@ -12,6 +12,9 @@ // drop sibling tracks the operator didn't ask to remove. The current // shape per spec revision 723eee9 is "always direct delete; opt-in // Lidarr unmonitor for replacement-suppression." +// +// The web track-kebab entry that called this was removed in f7278f24; the +// endpoint was kept deliberately for a safer admin surface to rebind. package tracks import ( @@ -19,15 +22,14 @@ import ( "errors" "fmt" "log/slog" - "os" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" - "git.fabledsword.com/bvandeusen/minstrel/internal/coverart" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/library" ) // ErrNotFound is returned when the track id doesn't resolve. Aliased @@ -69,10 +71,10 @@ func NewService(pool *pgxpool.Pool, logger *slog.Logger, lidarr LidarrUnmonitore return &Service{pool: pool, logger: logger, lidarr: lidarr, dataDir: dataDir} } -// RemoveTrack deletes the file from disk and the DB rows, runs the -// album-empty / artist-empty cascade tidy-up, and (when unmonitor is -// true and the track is Lidarr-managed) tells Lidarr to flip the track's -// monitored flag off so it won't search for a replacement. +// RemoveTrack deletes the track's file and then its row, tidies away an album +// or artist the delete empties, and (when unmonitor is true and the track is +// Lidarr-managed) tells Lidarr to flip the track's monitored flag off so it +// won't search for a replacement. // // Returns: // - deletedAlbumID: non-nil when removing the track left the album @@ -81,9 +83,10 @@ func NewService(pool *pgxpool.Pool, logger *slog.Logger, lidarr LidarrUnmonitore // empty (only set if deletedAlbumID is also set). // - lidarrUnmonitorFailed: true when the operator requested unmonitor // and the Lidarr call failed; the file + DB delete still succeeded. -// - err: only for failures *before* the destructive part completes. -// A failed os.Remove is logged and tolerated. A failed Lidarr -// unmonitor is reflected in the bool flag, not the error. +// - err: ErrNotFound, or a failure before anything was deleted. When the +// file cannot be removed it is a *library.FileRemoveError and NOTHING was +// deleted — see library.DeleteTrackFile for why that order is the contract +// (#3918). A failed Lidarr unmonitor is reflected in the bool, not here. // // adminID is currently unused — the cascade audit-log line that would // reference it isn't wired in this slice. It's threaded through the @@ -104,10 +107,9 @@ func (s *Service) RemoveTrack( return nil, nil, false, fmt.Errorf("get track: %w", err) } - // Capture the album's mbid *before* the cascade-delete transaction. - // If removing this track empties the album, DeleteAlbumIfEmpty - // removes the row and a post-commit GetAlbumByID would return - // pgx.ErrNoRows — leaving the unmonitor walk with no album mbid. + // Capture the album's mbid *before* the delete. If removing this track + // empties the album, its row is gone afterwards and the unmonitor walk + // would have no album mbid to name. var albumMbid string if track.Mbid != nil && *track.Mbid != "" && unmonitor && s.lidarr != nil { alb, alerr := q.GetAlbumByID(ctx, track.AlbumID) @@ -118,64 +120,14 @@ func (s *Service) RemoveTrack( // "no albumMbid → can't unmonitor → flag failure." } - // Always: remove the file. Tolerate already-missing. - if track.FilePath != "" { - if rerr := os.Remove(track.FilePath); rerr != nil && !errors.Is(rerr, os.ErrNotExist) { - s.logger.Warn("track delete: file remove failed", - "path", track.FilePath, "track_id", trackID, "err", rerr) - // Proceed: DB consistency is the priority. + deleted, err := library.DeleteTrackFile(ctx, s.pool, s.logger, s.dataDir, trackID) + if err != nil { + if errors.Is(err, library.ErrTrackNotFound) { + return nil, nil, false, ErrNotFound } - } - - // DB cleanup in a transaction so a midway failure leaves things consistent. - tx, err := s.pool.Begin(ctx) - if err != nil { - return nil, nil, false, fmt.Errorf("begin tx: %w", err) - } - defer func() { _ = tx.Rollback(ctx) }() - - tq := dbq.New(tx) - - deleted, err := tq.DeleteTrack(ctx, trackID) - if err != nil { return nil, nil, false, fmt.Errorf("delete track: %w", err) } - albumRow, aerr := tq.DeleteAlbumIfEmpty(ctx, deleted.AlbumID) - switch { - case aerr == nil: - albumID := albumRow.ID - deletedAlbumID = &albumID - artistID, arerr := tq.DeleteArtistIfEmpty(ctx, albumRow.ArtistID) - switch { - case arerr == nil: - id := artistID - deletedArtistID = &id - case errors.Is(arerr, pgx.ErrNoRows): - // Artist still has other albums or stray tracks. OK. - default: - return nil, nil, false, fmt.Errorf("delete artist if empty: %w", arerr) - } - case errors.Is(aerr, pgx.ErrNoRows): - // Album still has other tracks. OK. - default: - return nil, nil, false, fmt.Errorf("delete album if empty: %w", aerr) - } - - if err := tx.Commit(ctx); err != nil { - return nil, nil, false, fmt.Errorf("commit: %w", err) - } - - // Cleanup artist-art filesystem cache if the delete cascade orphaned - // an artist. Non-fatal — the destructive part is done; we just want - // to keep the dataDir tidy. - if deletedArtistID != nil && s.dataDir != "" { - if err := coverart.CleanupArtistArt(s.dataDir, *deletedArtistID); err != nil { - s.logger.Warn("track delete: artist-art cleanup failed", - "artist_id", *deletedArtistID, "err", err) - } - } - // Lidarr unmonitor — non-fatal. The destructive part is done; any // failure here is informational so the operator can retry manually. if unmonitor && track.Mbid != nil && *track.Mbid != "" && s.lidarr != nil { @@ -193,5 +145,5 @@ func (s *Service) RemoveTrack( } } - return deletedAlbumID, deletedArtistID, lidarrUnmonitorFailed, nil + return deleted.AlbumID, deleted.ArtistID, lidarrUnmonitorFailed, nil } diff --git a/web/src/lib/api/errors.test.ts b/web/src/lib/api/errors.test.ts index d12f1e0b..d728c1fc 100644 --- a/web/src/lib/api/errors.test.ts +++ b/web/src/lib/api/errors.test.ts @@ -49,3 +49,37 @@ describe('errMessage', () => { expect(result.length).toBeGreaterThan(0); }); }); + +describe('errMessage detail codes (#3918)', () => { + const detail = + 'Minstrel runs as uid 1000, gid 1000 and cannot delete from /music/A (read-only file system). ' + + 'The library mount must be writable by that user. Nothing was deleted.'; + + test('library_not_writable appends the server message to the copy', () => { + expect(errMessage({ code: 'library_not_writable', message: detail })).toBe( + `${ERROR_COPY.library_not_writable} ${detail}` + ); + }); + + test('file_delete_failed appends the server message to the copy', () => { + const msg = 'Could not delete /music/A/01.mp3 (input/output error). Nothing was deleted.'; + expect(errMessage({ code: 'file_delete_failed', message: msg })).toBe( + `${ERROR_COPY.file_delete_failed} ${msg}` + ); + }); + + test('a detail code with no message shows the copy alone', () => { + expect(errMessage({ code: 'library_not_writable' })).toBe(ERROR_COPY.library_not_writable); + expect(errMessage({ code: 'library_not_writable', message: ' ' })).toBe( + ERROR_COPY.library_not_writable + ); + }); + + // Server messages are usually internal detail. Appending them for every code + // would leak things like driver errors into toasts; this pins the scope. + test('other codes never carry the server message', () => { + expect(errMessage({ code: 'track_not_found', message: 'pgx: no rows in result set' })).toBe( + ERROR_COPY.track_not_found + ); + }); +}); diff --git a/web/src/lib/api/errors.ts b/web/src/lib/api/errors.ts index a7417811..1f09f55d 100644 --- a/web/src/lib/api/errors.ts +++ b/web/src/lib/api/errors.ts @@ -8,11 +8,25 @@ export function errCode(err: unknown): string { return (err as { code?: string })?.code ?? 'unknown'; } +/** + * Codes whose server message carries specifics the operator needs in order to + * act — which directory, which uid — that fixed copy cannot say. For these the + * message follows the copy (#3918). Kept to a named list on purpose: most + * server messages are internal detail and must never reach a toast. Mirrored + * in Android's ErrorCopy. + */ +const DETAIL_CODES: ReadonlySet = new Set(['library_not_writable', 'file_delete_failed']); + /** * Returns user-facing copy for an unknown error value. Looks up the * error's code in the error-copy map; falls back to the supplied * fallback (default: "Something went wrong.") when the code is unknown. + * For a DETAIL_CODES code, the server's message is appended. */ export function errMessage(err: unknown, fallback = 'Something went wrong.'): string { - return copyForCode(errCode(err)) ?? fallback; + const code = errCode(err); + const copy = copyForCode(code) ?? fallback; + if (!DETAIL_CODES.has(code)) return copy; + const detail = (err as { message?: unknown })?.message; + return typeof detail === 'string' && detail.trim() !== '' ? `${copy} ${detail}` : copy; } diff --git a/web/src/lib/styles/error-copy.json b/web/src/lib/styles/error-copy.json index c18961fd..80b94860 100644 --- a/web/src/lib/styles/error-copy.json +++ b/web/src/lib/styles/error-copy.json @@ -40,6 +40,8 @@ "request_not_pending": "This request is no longer pending.", "request_not_found": "That request no longer exists.", "track_not_found": "That track no longer exists.", + "library_not_writable": "The music library isn't writable by the server.", + "file_delete_failed": "The file couldn't be deleted.", "album_not_found": "That album no longer exists.", "artist_not_found": "That artist no longer exists.", "playlist_not_found": "That playlist no longer exists.", From 702b48ce364fc42f281e17b3d4844ae7cda91649 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 11 Sep 2026 14:34:28 -0400 Subject: [PATCH 4/5] fix(lidarrquarantine): pass dataDir at the four stub-client test constructors d7a8e5f3 added a dataDir parameter to NewService and updated the 13 call sites spelled NewService(pool, lidarrconfig.New(pool), nil). Four more build their client from a Lidarr stub, NewService(pool, cfg, clientFn), and were missed, so the package's tests did not compile and run 6495 failed both go vet and the integration build. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- internal/lidarrquarantine/service_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/lidarrquarantine/service_test.go b/internal/lidarrquarantine/service_test.go index 9f14ae37..8129b01a 100644 --- a/internal/lidarrquarantine/service_test.go +++ b/internal/lidarrquarantine/service_test.go @@ -366,7 +366,7 @@ func TestDeleteViaLidarr_FullCascade(t *testing.T) { t.Fatalf("save config: %v", err) } clientFn := func() *lidarr.Client { return lidarr.NewClient(stub.URL, "k") } - svc := NewService(pool, cfg, clientFn) + svc := NewService(pool, cfg, clientFn, "") q := dbq.New(pool) artist, _ := q.UpsertArtist(context.Background(), dbq.UpsertArtistParams{Name: "A", SortName: "A"}) @@ -456,7 +456,7 @@ func TestDeleteViaLidarr_AlbumMBIDMissing(t *testing.T) { cfg := lidarrconfig.New(pool) _ = cfg.Save(context.Background(), lidarrconfig.Config{Enabled: true, BaseURL: stub.URL, APIKey: "k"}) clientFn := func() *lidarr.Client { return lidarr.NewClient(stub.URL, "k") } - svc := NewService(pool, cfg, clientFn) + svc := NewService(pool, cfg, clientFn, "") _, _ = svc.Flag(context.Background(), user.ID, track.ID, "bad_rip", "") _, _, err := svc.DeleteViaLidarr(context.Background(), track.ID, user.ID) @@ -481,7 +481,7 @@ func TestDeleteViaLidarr_LidarrAlbumNotFound(t *testing.T) { cfg := lidarrconfig.New(pool) _ = cfg.Save(context.Background(), lidarrconfig.Config{Enabled: true, BaseURL: stub.URL, APIKey: "k"}) clientFn := func() *lidarr.Client { return lidarr.NewClient(stub.URL, "k") } - svc := NewService(pool, cfg, clientFn) + svc := NewService(pool, cfg, clientFn, "") track, _, _ := seedTrack(t, pool, "T", "x") _, _ = svc.Flag(context.Background(), user.ID, track.ID, "bad_rip", "") @@ -521,7 +521,7 @@ func TestDeleteViaLidarr_TrackNotFound(t *testing.T) { t.Fatalf("save config: %v", err) } clientFn := func() *lidarr.Client { return lidarr.NewClient(stub.URL, "k") } - svc := NewService(pool, cfg, clientFn) + svc := NewService(pool, cfg, clientFn, "") var bogus pgtype.UUID bogus.Bytes = [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16} From 71d4335584452a3c0073de5b308d76883e9f077c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 11 Sep 2026 14:34:28 -0400 Subject: [PATCH 5/5] =?UTF-8?q?docs(readme):=20the=20music=20mount=20is=20?= =?UTF-8?q?writable=20=E2=80=94=20Minstrel=20deletes=20when=20asked?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The quickstart mounted the library :ro and promised "Minstrel never writes to your library". That stopped being true long before #3918: quarantine's Delete file removes files, and under :ro it failed. The operator has accepted delete ownership (Scribe note #3926). The quickstart now mounts it writable and says exactly what Minstrel writes: it deletes a file when an admin asks, and never moves, renames or retags. It notes that uid 1000 needs write access, and that :ro still works, with deletes refusing and explaining why. Reorganising and tag writes stay out, pending whether Minstrel absorbs Lidarr's role. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- README.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 4571d523..3f0ac051 100644 --- a/README.md +++ b/README.md @@ -37,8 +37,12 @@ services: ports: ['4533:4533'] volumes: # Your music library. Point ./music at wherever your audio files - # live. Mounted read-only — Minstrel never writes to your library. - - ./music:/music:ro + # live. Writable, because Minstrel deletes a file when an admin asks + # it to (for example, quarantine's "Delete file"). It never moves, + # renames or retags anything. The container runs as uid 1000, so that + # user needs write access to the folders. Mount it :ro to forbid even + # deletes: those actions then refuse, say why, and delete nothing. + - ./music:/music # Generated data: playlist cover collages, artist art, caches. # The path must match MINSTREL_STORAGE_DATA_DIR, which the image # sets to /app/data — keep this mount on /app/data or your cache @@ -47,7 +51,7 @@ services: environment: MINSTREL_DATABASE_URL: postgres://minstrel:minstrel@db:5432/minstrel?sslmode=disable # Colon-separated library roots to scan; must match the container - # path of the read-only music mount above (/music here). + # path of the music mount above (/music here). MINSTREL_LIBRARY_SCAN_PATHS: /music depends_on: [db]