Compare commits
11
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
d5aa081157 | ||
|
|
304de88c50 | ||
|
|
96abb48086 | ||
|
|
a094d5f8b0 | ||
|
|
481f906059 | ||
|
|
a99f855e98 | ||
|
|
24d330424f | ||
|
|
f6d1cf24f0 | ||
|
|
7e4727fc49 | ||
|
|
fd27819cdd | ||
|
|
37b396a7e4 |
@@ -9,11 +9,16 @@ import (
|
||||
|
||||
// genreCount is one row of the genre browse index (#367).
|
||||
//
|
||||
// Genres are the raw ID3 strings, split on [;,] but otherwise untouched — no
|
||||
// Genres are the tag's own strings, split on [;,] but otherwise untouched — no
|
||||
// case folding and no synonym mapping. So "Rock" and "rock" can both appear,
|
||||
// as can "Rock/Pop" alongside "Rock" and "Pop". That's deliberate for v1: the
|
||||
// alternative is a normalisation table to invent and maintain, and the raw
|
||||
// spread has to be visible before anyone can judge whether it's a problem.
|
||||
//
|
||||
// The first look at that spread found it dominated by welded tokens like
|
||||
// "Alternative RockRock" — the scanner's own bug, not the operator's tagging
|
||||
// (#2499). Judge the "is a taxonomy needed" question (#2468) only against a
|
||||
// library re-scanned since that fix.
|
||||
type genreCount struct {
|
||||
Genre string `json:"genre"`
|
||||
TrackCount int `json:"track_count"`
|
||||
|
||||
@@ -39,6 +39,14 @@ type surfaceMetric struct {
|
||||
SkipRate float64 `json:"skip_rate"` // skips / plays, [0,1]
|
||||
AvgCompletion float64 `json:"avg_completion"` // mean completion ratio, [0,1]
|
||||
LowConfidence bool `json:"low_confidence"` // plays < recMetricsLowVolume
|
||||
// SkipDelta / CompletionDelta are this row's difference from the manual
|
||||
// baseline WITH its margin of error (#2495). nil on the baseline row
|
||||
// itself, and whenever the samples are too thin for a margin to mean
|
||||
// anything. Computed server-side so both clients read the same arithmetic
|
||||
// instead of each re-deriving it — and so `low_confidence` is no longer
|
||||
// mistaken for a decision threshold, which it never was.
|
||||
SkipDelta *metricDelta `json:"skip_delta,omitempty"`
|
||||
CompletionDelta *metricDelta `json:"completion_delta,omitempty"`
|
||||
// Breakdown splits the family into the pick-kind populations its
|
||||
// builder stamped (#1249, generalized #1270): For You's taste/fresh,
|
||||
// Discover's buckets, tier1-3 for tiered mixes — plus earlier plays
|
||||
@@ -119,6 +127,10 @@ type familyAccum struct {
|
||||
// completionSum is avg*count re-expanded, so merging N raw rows
|
||||
// reduces to a single weighted division at the end.
|
||||
completionSum float64
|
||||
// completionSqSum is the sum of squared completion ratios, which is what
|
||||
// makes the variance mergeable across raw source rows (#2495). Standard
|
||||
// deviations cannot be combined; sums of squares add exactly.
|
||||
completionSqSum float64
|
||||
}
|
||||
|
||||
func (a *familyAccum) add(row dbq.RecommendationSourceMetricsForUserRow) {
|
||||
@@ -126,6 +138,7 @@ func (a *familyAccum) add(row dbq.RecommendationSourceMetricsForUserRow) {
|
||||
a.skips += row.Skips
|
||||
a.completionN += row.CompletionN
|
||||
a.completionSum += row.AvgCompletion * float64(row.CompletionN)
|
||||
a.completionSqSum += row.CompletionSqsum
|
||||
}
|
||||
|
||||
func (a *familyAccum) metric() surfaceMetric {
|
||||
@@ -145,6 +158,33 @@ func (a *familyAccum) metric() surfaceMetric {
|
||||
return m
|
||||
}
|
||||
|
||||
// completionVariance is the sample variance of this family's completion ratios.
|
||||
func (a *familyAccum) completionVariance() float64 {
|
||||
return sampleVariance(a.completionSum, a.completionSqSum, a.completionN)
|
||||
}
|
||||
|
||||
// applyDeltas attaches baseline-relative deltas + margins to a metric.
|
||||
// Split out so every row — parent surfaces and breakdown rows alike — goes
|
||||
// through the identical arithmetic; a breakdown arm is exactly where the old
|
||||
// card was most misleading, because those are the thinnest samples on screen.
|
||||
func applyDeltas(m *surfaceMetric, acc *familyAccum, baseline *familyAccum) {
|
||||
if baseline == nil || baseline.plays == 0 {
|
||||
return
|
||||
}
|
||||
m.SkipDelta = proportionDelta(
|
||||
m.SkipRate, acc.plays,
|
||||
float64(baseline.skips)/float64(baseline.plays), baseline.plays,
|
||||
)
|
||||
baseMean := 0.0
|
||||
if baseline.completionN > 0 {
|
||||
baseMean = baseline.completionSum / float64(baseline.completionN)
|
||||
}
|
||||
m.CompletionDelta = meanDelta(
|
||||
m.AvgCompletion, acc.completionVariance(), acc.completionN,
|
||||
baseMean, baseline.completionVariance(), baseline.completionN,
|
||||
)
|
||||
}
|
||||
|
||||
// handleGetRecommendationMetrics implements GET /api/me/recommendation-metrics.
|
||||
// Bucketed per-surface-family outcomes for the caller over the last `days`
|
||||
// (default 30, capped at 365), grouped by surface intent and anchored by the
|
||||
@@ -214,7 +254,7 @@ func pickKindFamily(parent recFamily, kind string) recFamily {
|
||||
// Breakdown rows. Attached only when at least one attributed play
|
||||
// exists — an all-unattributed breakdown would just repeat the parent
|
||||
// row, and families that never stamp (radio, direct plays) stay flat.
|
||||
func pickKindBreakdown(picks map[string]*familyAccum) []surfaceMetric {
|
||||
func pickKindBreakdown(picks map[string]*familyAccum, baseline *familyAccum) []surfaceMetric {
|
||||
attributed := int64(0)
|
||||
for kind, acc := range picks {
|
||||
if kind != "" {
|
||||
@@ -227,7 +267,9 @@ func pickKindBreakdown(picks map[string]*familyAccum) []surfaceMetric {
|
||||
out := make([]surfaceMetric, 0, len(picks))
|
||||
for _, kind := range pickKindOrder {
|
||||
if acc, ok := picks[kind]; ok && acc.plays > 0 {
|
||||
out = append(out, acc.metric())
|
||||
m := acc.metric()
|
||||
applyDeltas(&m, acc, baseline)
|
||||
out = append(out, m)
|
||||
}
|
||||
}
|
||||
return out
|
||||
@@ -287,7 +329,8 @@ func bucketMetricsResponse(
|
||||
for _, acc := range families {
|
||||
if acc.fam.intent == g.intent {
|
||||
m := acc.metric()
|
||||
m.Breakdown = pickKindBreakdown(picks[acc.fam.key])
|
||||
applyDeltas(&m, acc, baseline)
|
||||
m.Breakdown = pickKindBreakdown(picks[acc.fam.key], baseline)
|
||||
group.Surfaces = append(group.Surfaces, m)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,117 @@
|
||||
package api
|
||||
|
||||
import "math"
|
||||
|
||||
// Uncertainty on the deltas the recommendation-metrics card shows (#2495).
|
||||
//
|
||||
// Why this exists: the card had exactly one volume threshold,
|
||||
// recMetricsLowVolume = 20, and it was doing two jobs. Twenty plays is enough to
|
||||
// be worth DISPLAYING — below that a skip rate is anecdote — but it is nowhere
|
||||
// near enough to ACT on. Detecting the ~13pp differences that actually matter
|
||||
// needs roughly 133 plays per arm for 80% power at α=0.05.
|
||||
//
|
||||
// So Discover's taste-matched (59 plays) and random-unheard (70) both rendered as
|
||||
// full-confidence rows with a bold delta beside them, and that comparison sits at
|
||||
// p ≈ 0.06. The card said "signal"; the arithmetic said "maybe". It led directly
|
||||
// to a recommendation the data didn't support, and any reader with the same
|
||||
// numbers would have made the same call.
|
||||
//
|
||||
// The fix is to publish the margin of error next to the delta and flag when the
|
||||
// delta is smaller than it — i.e. not distinguishable from zero. Computed here,
|
||||
// server-side, so both clients agree rather than each re-deriving it.
|
||||
//
|
||||
// recMetricsLowVolume stays exactly as it was. This is a second, independent
|
||||
// signal, not a replacement: "too thin to show" and "too thin to act on" are
|
||||
// different questions and deserve different answers.
|
||||
|
||||
// deltaZ is the two-sided 95% normal critical value. Normal rather than
|
||||
// Student's t: at the sample sizes where a delta is worth acting on (n in the
|
||||
// hundreds) the difference is immaterial, and a household dashboard does not
|
||||
// need a t-table.
|
||||
const deltaZ = 1.96
|
||||
|
||||
// metricDelta is a difference from the baseline, with its uncertainty.
|
||||
//
|
||||
// Both figures are in PERCENTAGE POINTS, matching how the card reads them out —
|
||||
// a skip rate of 0.153 against a baseline of 0.270 is "-11.7", not "-0.117".
|
||||
type metricDelta struct {
|
||||
// DeltaPP is surface minus baseline. Negative skip is better; negative
|
||||
// completion is worse. The client owns that colouring.
|
||||
DeltaPP float64 `json:"delta_pp"`
|
||||
// MarginPP is the 95% margin of error on DeltaPP. Read the delta as
|
||||
// DeltaPP ± MarginPP.
|
||||
MarginPP float64 `json:"margin_pp"`
|
||||
// Distinguishable reports |DeltaPP| >= MarginPP: the interval excludes
|
||||
// zero, so the difference is worth reading as a difference. When false the
|
||||
// number may be pure noise no matter how large it looks.
|
||||
Distinguishable bool `json:"distinguishable"`
|
||||
}
|
||||
|
||||
// proportionDelta compares two rates (skips/plays) as a two-proportion
|
||||
// difference. Returns nil when either sample is empty, or when either rate is
|
||||
// degenerate (0 or 1) — a rate with no observed variation has an SE of 0 on its
|
||||
// side, which would report a spuriously narrow margin rather than an honest one.
|
||||
func proportionDelta(rate1 float64, n1 int64, rate2 float64, n2 int64) *metricDelta {
|
||||
if n1 <= 0 || n2 <= 0 {
|
||||
return nil
|
||||
}
|
||||
v1 := rate1 * (1 - rate1) / float64(n1)
|
||||
v2 := rate2 * (1 - rate2) / float64(n2)
|
||||
se := math.Sqrt(v1 + v2)
|
||||
if se <= 0 {
|
||||
// Both rates are 0 or both are 1. The delta is exactly zero and the
|
||||
// margin is meaningless; reporting nothing is more honest than
|
||||
// reporting certainty.
|
||||
return nil
|
||||
}
|
||||
return newDelta((rate1-rate2)*100, deltaZ*se*100)
|
||||
}
|
||||
|
||||
// meanDelta compares two means (average completion ratio) using Welch's
|
||||
// standard error, which does not assume equal variances between the two groups.
|
||||
//
|
||||
// Note the margins here are wider than intuition suggests, and that is correct:
|
||||
// completion is strongly bimodal — a play is either abandoned early (≈0.05) or
|
||||
// finished (≈1.0), with little in between — so its standard deviation is large
|
||||
// (~0.4) even though the mean looks stable.
|
||||
func meanDelta(mean1 float64, variance1 float64, n1 int64, mean2 float64, variance2 float64, n2 int64) *metricDelta {
|
||||
// Two observations minimum per side: a sample variance needs n-1 > 0.
|
||||
if n1 < 2 || n2 < 2 {
|
||||
return nil
|
||||
}
|
||||
se := math.Sqrt(variance1/float64(n1) + variance2/float64(n2))
|
||||
if se <= 0 || math.IsNaN(se) || math.IsInf(se, 0) {
|
||||
return nil
|
||||
}
|
||||
return newDelta((mean1-mean2)*100, deltaZ*se*100)
|
||||
}
|
||||
|
||||
func newDelta(deltaPP, marginPP float64) *metricDelta {
|
||||
return &metricDelta{
|
||||
DeltaPP: deltaPP,
|
||||
MarginPP: marginPP,
|
||||
// >= rather than >: a delta exactly equal to its margin sits on the
|
||||
// boundary, and calling the boundary "distinguishable" is the
|
||||
// conventional reading of a 95% interval that just excludes zero.
|
||||
Distinguishable: math.Abs(deltaPP) >= marginPP,
|
||||
}
|
||||
}
|
||||
|
||||
// sampleVariance recovers the sample variance from the aggregates the SQL
|
||||
// returns. sum is mean×n rather than a selected column, which keeps the query to
|
||||
// one extra expression.
|
||||
//
|
||||
// The subtraction can go very slightly negative through floating-point
|
||||
// cancellation when every observation is identical, so the result is clamped —
|
||||
// a negative variance would produce NaN downstream.
|
||||
func sampleVariance(sum, sqSum float64, n int64) float64 {
|
||||
if n < 2 {
|
||||
return 0
|
||||
}
|
||||
nf := float64(n)
|
||||
v := (sqSum - (sum * sum / nf)) / (nf - 1)
|
||||
if v < 0 {
|
||||
return 0
|
||||
}
|
||||
return v
|
||||
}
|
||||
@@ -0,0 +1,152 @@
|
||||
package api
|
||||
|
||||
import (
|
||||
"math"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestProportionDelta_ReproducesTheDiscoverCase(t *testing.T) {
|
||||
// The comparison that motivated #2495: Discover taste-matched (59 plays,
|
||||
// 15.3% skip) vs random-unheard (70 plays, 28.6%). A 13.3pp gap that the old
|
||||
// card rendered as a confident coloured number, sitting at p ≈ 0.06.
|
||||
d := proportionDelta(0.153, 59, 0.286, 70)
|
||||
if d == nil {
|
||||
t.Fatal("expected a delta for two real samples")
|
||||
}
|
||||
if math.Abs(d.DeltaPP-(-13.3)) > 0.1 {
|
||||
t.Errorf("DeltaPP = %.2f, want ≈ -13.3", d.DeltaPP)
|
||||
}
|
||||
// This is the assertion the whole task exists for: at these sample sizes the
|
||||
// margin swallows the difference.
|
||||
if d.Distinguishable {
|
||||
t.Errorf("13.3pp on n=59/70 reported as distinguishable (margin %.2f) — "+
|
||||
"this is exactly the false confidence #2495 set out to remove", d.MarginPP)
|
||||
}
|
||||
if d.MarginPP <= 13.3 {
|
||||
t.Errorf("MarginPP = %.2f, expected it to exceed the 13.3pp delta", d.MarginPP)
|
||||
}
|
||||
}
|
||||
|
||||
// Same effect size, ~10x the volume: now it is real. Proves the flag tracks
|
||||
// sample size rather than just the size of the gap.
|
||||
func TestProportionDelta_SameGapBecomesDistinguishableWithVolume(t *testing.T) {
|
||||
d := proportionDelta(0.153, 600, 0.286, 700)
|
||||
if d == nil {
|
||||
t.Fatal("expected a delta")
|
||||
}
|
||||
if !d.Distinguishable {
|
||||
t.Errorf("13.3pp on n=600/700 should be distinguishable (margin %.2f)", d.MarginPP)
|
||||
}
|
||||
}
|
||||
|
||||
func TestProportionDelta_SignAndDirection(t *testing.T) {
|
||||
// Surface skips MORE than baseline -> positive delta (worse for skip rate).
|
||||
worse := proportionDelta(0.40, 500, 0.25, 500)
|
||||
if worse == nil || worse.DeltaPP <= 0 {
|
||||
t.Fatalf("expected a positive delta, got %+v", worse)
|
||||
}
|
||||
better := proportionDelta(0.10, 500, 0.25, 500)
|
||||
if better == nil || better.DeltaPP >= 0 {
|
||||
t.Fatalf("expected a negative delta, got %+v", better)
|
||||
}
|
||||
}
|
||||
|
||||
func TestProportionDelta_EmptySamples(t *testing.T) {
|
||||
if d := proportionDelta(0.2, 0, 0.3, 100); d != nil {
|
||||
t.Errorf("n1=0 produced a delta: %+v", d)
|
||||
}
|
||||
if d := proportionDelta(0.2, 100, 0.3, 0); d != nil {
|
||||
t.Errorf("n2=0 produced a delta: %+v", d)
|
||||
}
|
||||
}
|
||||
|
||||
// Two degenerate rates have zero standard error, which would report a margin of
|
||||
// 0 and therefore "distinguishable" for a delta of exactly 0. Reporting nothing
|
||||
// is the honest answer.
|
||||
func TestProportionDelta_DegenerateRates(t *testing.T) {
|
||||
if d := proportionDelta(0, 50, 0, 50); d != nil {
|
||||
t.Errorf("both rates 0 produced a delta: %+v", d)
|
||||
}
|
||||
if d := proportionDelta(1, 50, 1, 50); d != nil {
|
||||
t.Errorf("both rates 1 produced a delta: %+v", d)
|
||||
}
|
||||
// One degenerate side is still informative — the other side carries variance.
|
||||
if d := proportionDelta(0, 200, 0.3, 200); d == nil {
|
||||
t.Error("one degenerate rate should still yield a delta")
|
||||
}
|
||||
}
|
||||
|
||||
func TestMeanDelta(t *testing.T) {
|
||||
// Completion is bimodal, so ~0.16 variance (sd ≈ 0.4) is realistic.
|
||||
const v = 0.16
|
||||
thin := meanDelta(0.82, v, 59, 0.54, v, 70)
|
||||
if thin == nil {
|
||||
t.Fatal("expected a delta")
|
||||
}
|
||||
if math.Abs(thin.DeltaPP-28.0) > 0.1 {
|
||||
t.Errorf("DeltaPP = %.2f, want ≈ 28.0", thin.DeltaPP)
|
||||
}
|
||||
// 28pp is large enough to survive even a wide margin at this n.
|
||||
if !thin.Distinguishable {
|
||||
t.Errorf("28pp on n=59/70 with sd 0.4 should be distinguishable (margin %.2f)", thin.MarginPP)
|
||||
}
|
||||
// A small completion gap at the same volume should not be.
|
||||
small := meanDelta(0.56, v, 59, 0.54, v, 70)
|
||||
if small == nil {
|
||||
t.Fatal("expected a delta")
|
||||
}
|
||||
if small.Distinguishable {
|
||||
t.Errorf("2pp on n=59/70 reported as distinguishable (margin %.2f)", small.MarginPP)
|
||||
}
|
||||
}
|
||||
|
||||
// A sample variance needs at least two observations per side.
|
||||
func TestMeanDelta_NeedsTwoObservations(t *testing.T) {
|
||||
if d := meanDelta(0.8, 0.1, 1, 0.5, 0.1, 100); d != nil {
|
||||
t.Errorf("n1=1 produced a delta: %+v", d)
|
||||
}
|
||||
if d := meanDelta(0.8, 0.1, 100, 0.5, 0.1, 1); d != nil {
|
||||
t.Errorf("n2=1 produced a delta: %+v", d)
|
||||
}
|
||||
}
|
||||
|
||||
func TestMeanDelta_ZeroVarianceBothSides(t *testing.T) {
|
||||
if d := meanDelta(0.8, 0, 50, 0.5, 0, 50); d != nil {
|
||||
t.Errorf("zero variance on both sides produced a delta: %+v", d)
|
||||
}
|
||||
}
|
||||
|
||||
func TestSampleVariance(t *testing.T) {
|
||||
// Observations 0, 1: mean 0.5, sample variance 0.5.
|
||||
if got := sampleVariance(1.0, 1.0, 2); math.Abs(got-0.5) > 1e-9 {
|
||||
t.Errorf("sampleVariance = %v, want 0.5", got)
|
||||
}
|
||||
// Identical observations -> zero variance, and must not go negative through
|
||||
// floating-point cancellation.
|
||||
if got := sampleVariance(4.0, 4.0, 4); got != 0 {
|
||||
t.Errorf("identical observations gave variance %v, want 0", got)
|
||||
}
|
||||
if got := sampleVariance(0, 0, 1); got != 0 {
|
||||
t.Errorf("n=1 gave variance %v, want 0", got)
|
||||
}
|
||||
}
|
||||
|
||||
// Clamping matters: a negative variance would become NaN in the square root and
|
||||
// propagate into the JSON as a null-ish number.
|
||||
func TestSampleVariance_NeverNegative(t *testing.T) {
|
||||
// sqSum slightly below sum²/n, as cancellation can produce.
|
||||
if got := sampleVariance(10.0, 24.999999999, 4); got < 0 {
|
||||
t.Errorf("variance went negative: %v", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestNewDelta_BoundaryCountsAsDistinguishable(t *testing.T) {
|
||||
d := newDelta(5.0, 5.0)
|
||||
if !d.Distinguishable {
|
||||
t.Error("a delta exactly equal to its margin should count as distinguishable")
|
||||
}
|
||||
d = newDelta(4.999, 5.0)
|
||||
if d.Distinguishable {
|
||||
t.Error("a delta just inside its margin should not count as distinguishable")
|
||||
}
|
||||
}
|
||||
@@ -18,6 +18,7 @@ WHERE EXISTS (
|
||||
FROM tracks
|
||||
JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true
|
||||
WHERE tracks.album_id = albums.id
|
||||
AND tracks.missing_since IS NULL
|
||||
AND trim(g.genre) = trim($1::text)
|
||||
)
|
||||
`
|
||||
@@ -97,6 +98,7 @@ WHERE EXISTS (
|
||||
FROM tracks
|
||||
JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true
|
||||
WHERE tracks.album_id = albums.id
|
||||
AND tracks.missing_since IS NULL
|
||||
AND trim(g.genre) = trim($1::text)
|
||||
)
|
||||
ORDER BY albums.sort_title, albums.id
|
||||
@@ -219,6 +221,7 @@ SELECT DISTINCT trim(g.genre) AS genre
|
||||
FROM tracks
|
||||
JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true
|
||||
WHERE tracks.album_id = $1 AND trim(g.genre) <> ''
|
||||
AND tracks.missing_since IS NULL
|
||||
ORDER BY trim(g.genre)
|
||||
`
|
||||
|
||||
@@ -251,6 +254,7 @@ SELECT DISTINCT trim(g.genre) AS genre
|
||||
FROM tracks
|
||||
JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true
|
||||
WHERE tracks.artist_id = $1 AND trim(g.genre) <> ''
|
||||
AND tracks.missing_since IS NULL
|
||||
ORDER BY trim(g.genre)
|
||||
`
|
||||
|
||||
@@ -278,10 +282,12 @@ func (q *Queries) ListGenresForArtist(ctx context.Context, artistID pgtype.UUID)
|
||||
}
|
||||
|
||||
const listGenresWithCount = `-- name: ListGenresWithCount :many
|
||||
|
||||
SELECT trim(g.genre) AS genre, COUNT(DISTINCT tracks.id)::bigint AS track_count
|
||||
FROM tracks
|
||||
JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true
|
||||
WHERE trim(g.genre) <> ''
|
||||
AND tracks.missing_since IS NULL
|
||||
GROUP BY trim(g.genre)
|
||||
ORDER BY track_count DESC, trim(g.genre)
|
||||
`
|
||||
@@ -291,6 +297,17 @@ type ListGenresWithCountRow struct {
|
||||
TrackCount int64
|
||||
}
|
||||
|
||||
// Every query in this file filters `tracks.missing_since IS NULL` (#2523).
|
||||
// A row whose file has vanished keeps its genre forever — the scanner walks the
|
||||
// filesystem, so it never revisits a path that no longer exists — which is how
|
||||
// pre-#2499 welded genres survived a full re-scan and kept showing in the index.
|
||||
// Browsing is a way of finding something to play, so a track that cannot play
|
||||
// should not shape it.
|
||||
//
|
||||
// Year queries below join albums only and are deliberately left alone: an album
|
||||
// is still a real release even if some of its tracks are gone. An album whose
|
||||
// EVERY track is missing will linger on the year axis; that's a narrower case,
|
||||
// tracked with the rest of the cleanup work.
|
||||
// Genre browse index (#367).
|
||||
//
|
||||
// Genres live inline on tracks.genre as a delimited string, so this splits on
|
||||
|
||||
@@ -15,7 +15,8 @@ const listCrossUserLikedTracksForDiscover = `-- name: ListCrossUserLikedTracksFo
|
||||
SELECT t.id, t.album_id, t.artist_id
|
||||
FROM general_likes gl
|
||||
JOIN tracks t ON t.id = gl.track_id
|
||||
WHERE gl.user_id != $1
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND gl.user_id != $1
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM play_events pe
|
||||
WHERE pe.user_id = $1
|
||||
@@ -95,7 +96,8 @@ dormant_artists AS (
|
||||
SELECT t.id, t.album_id, t.artist_id
|
||||
FROM tracks t
|
||||
JOIN dormant_artists da ON da.id = t.artist_id
|
||||
WHERE NOT EXISTS (
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM play_events pe
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.track_id = t.id
|
||||
@@ -159,7 +161,8 @@ func (q *Queries) ListDormantArtistTracksForDiscover(ctx context.Context, arg Li
|
||||
const listRandomUnheardTracksForDiscover = `-- name: ListRandomUnheardTracksForDiscover :many
|
||||
SELECT t.id, t.album_id, t.artist_id
|
||||
FROM tracks t
|
||||
WHERE NOT EXISTS (
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM play_events pe
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.track_id = t.id
|
||||
@@ -217,7 +220,8 @@ SELECT t.id, t.album_id, t.artist_id
|
||||
FROM tracks t
|
||||
JOIN LATERAL regexp_split_to_table(coalesce(t.genre, ''), '[;,]') AS g_split(g) ON true
|
||||
JOIN taste_profile_tags nt ON nt.user_id = $1 AND trim(g_split.g) = nt.tag
|
||||
WHERE nt.weight > 0
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND nt.weight > 0
|
||||
AND trim(g_split.g) <> ''
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM play_events pe
|
||||
|
||||
@@ -261,7 +261,7 @@ func (q *Queries) InsertSkipEvent(ctx context.Context, arg InsertSkipEventParams
|
||||
}
|
||||
|
||||
const listRecentSessionTracks = `-- name: ListRecentSessionTracks :many
|
||||
SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version FROM tracks t
|
||||
SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since FROM tracks t
|
||||
JOIN play_events pe ON pe.track_id = t.id
|
||||
WHERE pe.session_id = $1
|
||||
AND pe.started_at < $2
|
||||
@@ -305,6 +305,8 @@ func (q *Queries) ListRecentSessionTracks(ctx context.Context, arg ListRecentSes
|
||||
&i.UpdatedAt,
|
||||
&i.TagSource,
|
||||
&i.TagSourcesVersion,
|
||||
&i.TagReadVersion,
|
||||
&i.MissingSince,
|
||||
); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
@@ -14,7 +14,7 @@ import (
|
||||
const listUserHistory = `-- name: ListUserHistory :many
|
||||
SELECT pe.id AS event_id,
|
||||
pe.started_at,
|
||||
t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version,
|
||||
t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since,
|
||||
albums.title AS album_title,
|
||||
artists.name AS artist_name
|
||||
FROM play_events pe
|
||||
@@ -79,6 +79,8 @@ func (q *Queries) ListUserHistory(ctx context.Context, arg ListUserHistoryParams
|
||||
&i.Track.UpdatedAt,
|
||||
&i.Track.TagSource,
|
||||
&i.Track.TagSourcesVersion,
|
||||
&i.Track.TagReadVersion,
|
||||
&i.Track.MissingSince,
|
||||
&i.AlbumTitle,
|
||||
&i.ArtistName,
|
||||
); err != nil {
|
||||
|
||||
@@ -259,7 +259,7 @@ func (q *Queries) ListLikedTrackIDs(ctx context.Context, userID pgtype.UUID) ([]
|
||||
}
|
||||
|
||||
const listLikedTrackRows = `-- name: ListLikedTrackRows :many
|
||||
SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version FROM tracks t
|
||||
SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since FROM tracks t
|
||||
JOIN general_likes l ON l.track_id = t.id
|
||||
WHERE l.user_id = $1
|
||||
ORDER BY l.liked_at DESC
|
||||
@@ -299,6 +299,8 @@ func (q *Queries) ListLikedTrackRows(ctx context.Context, arg ListLikedTrackRows
|
||||
&i.UpdatedAt,
|
||||
&i.TagSource,
|
||||
&i.TagSourcesVersion,
|
||||
&i.TagReadVersion,
|
||||
&i.MissingSince,
|
||||
); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
@@ -642,6 +642,8 @@ type Track struct {
|
||||
UpdatedAt pgtype.Timestamptz
|
||||
TagSource *string
|
||||
TagSourcesVersion int32
|
||||
TagReadVersion int16
|
||||
MissingSince pgtype.Timestamptz
|
||||
}
|
||||
|
||||
type TrackSimilarity struct {
|
||||
|
||||
@@ -208,7 +208,7 @@ WITH plays AS (
|
||||
WHERE user_id = $2 AND was_skipped = false
|
||||
GROUP BY track_id
|
||||
)
|
||||
SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version,
|
||||
SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since,
|
||||
albums.title AS album_title,
|
||||
artists.name AS artist_name
|
||||
FROM plays p
|
||||
@@ -216,6 +216,7 @@ JOIN tracks t ON t.id = p.track_id
|
||||
JOIN albums ON albums.id = t.album_id
|
||||
JOIN artists ON artists.id = t.artist_id
|
||||
WHERE t.artist_id = $1
|
||||
AND t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
WHERE q.user_id = $2 AND q.track_id = t.id
|
||||
@@ -267,6 +268,8 @@ func (q *Queries) ListMostPlayedTracksForArtist(ctx context.Context, arg ListMos
|
||||
&i.Track.UpdatedAt,
|
||||
&i.Track.TagSource,
|
||||
&i.Track.TagSourcesVersion,
|
||||
&i.Track.TagReadVersion,
|
||||
&i.Track.MissingSince,
|
||||
&i.AlbumTitle,
|
||||
&i.ArtistName,
|
||||
); err != nil {
|
||||
@@ -287,14 +290,15 @@ WITH plays AS (
|
||||
WHERE user_id = $1 AND was_skipped = false
|
||||
GROUP BY track_id
|
||||
)
|
||||
SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version,
|
||||
SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since,
|
||||
albums.title AS album_title,
|
||||
artists.name AS artist_name
|
||||
FROM plays p
|
||||
JOIN tracks t ON t.id = p.track_id
|
||||
JOIN albums ON albums.id = t.album_id
|
||||
JOIN artists ON artists.id = t.artist_id
|
||||
WHERE NOT EXISTS (
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
WHERE q.user_id = $1 AND q.track_id = t.id
|
||||
)
|
||||
@@ -348,6 +352,8 @@ func (q *Queries) ListMostPlayedTracksForUser(ctx context.Context, arg ListMostP
|
||||
&i.Track.UpdatedAt,
|
||||
&i.Track.TagSource,
|
||||
&i.Track.TagSourcesVersion,
|
||||
&i.Track.TagReadVersion,
|
||||
&i.Track.MissingSince,
|
||||
&i.AlbumTitle,
|
||||
&i.ArtistName,
|
||||
); err != nil {
|
||||
@@ -685,7 +691,7 @@ func (q *Queries) ListRediscoverArtistsForUser(ctx context.Context, arg ListRedi
|
||||
|
||||
const loadRadioCandidates = `-- name: LoadRadioCandidates :many
|
||||
SELECT
|
||||
t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version,
|
||||
t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since,
|
||||
(l.user_id IS NOT NULL)::bool AS is_liked,
|
||||
pe.last_played_at::timestamptz AS last_played_at,
|
||||
pe.play_count,
|
||||
@@ -703,6 +709,7 @@ LEFT JOIN LATERAL (
|
||||
WHERE user_id = $1 AND track_id = t.id
|
||||
) pe ON true
|
||||
WHERE t.id <> $2
|
||||
AND t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM play_events
|
||||
WHERE user_id = $1 AND track_id = t.id
|
||||
@@ -763,6 +770,8 @@ func (q *Queries) LoadRadioCandidates(ctx context.Context, arg LoadRadioCandidat
|
||||
&i.Track.UpdatedAt,
|
||||
&i.Track.TagSource,
|
||||
&i.Track.TagSourcesVersion,
|
||||
&i.Track.TagReadVersion,
|
||||
&i.Track.MissingSince,
|
||||
&i.IsLiked,
|
||||
&i.LastPlayedAt,
|
||||
&i.PlayCount,
|
||||
@@ -895,7 +904,7 @@ random_fill AS (
|
||||
LIMIT $9
|
||||
)
|
||||
SELECT
|
||||
t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version,
|
||||
t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since,
|
||||
(l.user_id IS NOT NULL)::bool AS is_liked,
|
||||
pe.last_played_at::timestamptz AS last_played_at,
|
||||
pe.play_count,
|
||||
@@ -911,7 +920,7 @@ FROM (
|
||||
UNION ALL SELECT track_id, sim_score FROM coplay_artists
|
||||
UNION ALL SELECT track_id, sim_score FROM random_fill
|
||||
) u
|
||||
JOIN tracks t ON t.id = u.track_id
|
||||
JOIN tracks t ON t.id = u.track_id AND t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
JOIN albums al ON al.id = t.album_id
|
||||
LEFT JOIN general_likes l ON l.user_id = $1 AND l.track_id = t.id
|
||||
LEFT JOIN LATERAL (
|
||||
@@ -1004,6 +1013,8 @@ func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandid
|
||||
&i.Track.UpdatedAt,
|
||||
&i.Track.TagSource,
|
||||
&i.Track.TagSourcesVersion,
|
||||
&i.Track.TagReadVersion,
|
||||
&i.Track.MissingSince,
|
||||
&i.IsLiked,
|
||||
&i.LastPlayedAt,
|
||||
&i.PlayCount,
|
||||
|
||||
@@ -18,7 +18,8 @@ SELECT
|
||||
count(*)::bigint AS plays,
|
||||
count(*) FILTER (WHERE pe.was_skipped)::bigint AS skips,
|
||||
count(pe.completion_ratio)::bigint AS completion_n,
|
||||
COALESCE(avg(pe.completion_ratio), 0)::float8 AS avg_completion
|
||||
COALESCE(avg(pe.completion_ratio), 0)::float8 AS avg_completion,
|
||||
COALESCE(sum(pe.completion_ratio * pe.completion_ratio), 0)::float8 AS completion_sqsum
|
||||
FROM play_events pe
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.started_at > now() - ($2::float8 * INTERVAL '1 day')
|
||||
@@ -38,12 +39,20 @@ type RecommendationSourceMetricsForUserRow struct {
|
||||
Skips int64
|
||||
CompletionN int64
|
||||
AvgCompletion float64
|
||||
CompletionSqsum float64
|
||||
}
|
||||
|
||||
// $1 user_id, $2 window_days. plays/skips are counts; avg_completion is the
|
||||
// mean completion ratio over the completion_n plays that recorded one.
|
||||
// pick_kind splits For You plays into taste/fresh/unattributed (#1249);
|
||||
// it is NULL for every other source, so those still group to one row.
|
||||
//
|
||||
// completion_sqsum carries the sum of SQUARED completion ratios so the Go
|
||||
// handler can compute a variance — needed for the margin of error on a
|
||||
// completion delta (#2495). It is the sum rather than `stddev_samp` on purpose:
|
||||
// raw source rows get merged into surface families in Go, and sums of squares
|
||||
// add across groups exactly, whereas standard deviations cannot be combined
|
||||
// without them. Variance = (sqsum - sum²/n) / (n-1), with sum = avg × n.
|
||||
func (q *Queries) RecommendationSourceMetricsForUser(ctx context.Context, arg RecommendationSourceMetricsForUserParams) ([]RecommendationSourceMetricsForUserRow, error) {
|
||||
rows, err := q.db.Query(ctx, recommendationSourceMetricsForUser, arg.UserID, arg.Column2)
|
||||
if err != nil {
|
||||
@@ -60,6 +69,7 @@ func (q *Queries) RecommendationSourceMetricsForUser(ctx context.Context, arg Re
|
||||
&i.Skips,
|
||||
&i.CompletionN,
|
||||
&i.AvgCompletion,
|
||||
&i.CompletionSqsum,
|
||||
); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
@@ -39,7 +39,8 @@ SELECT t.id, t.album_id, t.artist_id
|
||||
JOIN affinity_artists aa ON aa.artist_id = t.artist_id
|
||||
LEFT JOIN play_counts pc ON pc.track_id = t.id
|
||||
LEFT JOIN skip_counts sc ON sc.track_id = t.id
|
||||
WHERE COALESCE(pc.c, 0) <= 2
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND COALESCE(pc.c, 0) <= 2
|
||||
AND COALESCE(sc.c, 0) < 2
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
@@ -124,7 +125,8 @@ SELECT t.id, t.album_id, t.artist_id, alt.tier::int AS tier
|
||||
FROM tracks t
|
||||
JOIN albums al ON al.id = t.album_id
|
||||
JOIN albums_tiered alt ON alt.album_id = al.id
|
||||
WHERE alt.tier IS NOT NULL
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND alt.tier IS NOT NULL
|
||||
AND NOT EXISTS (SELECT 1 FROM attempted a WHERE a.track_id = t.id)
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
@@ -225,7 +227,8 @@ albums_tiered AS (
|
||||
SELECT t.id, t.album_id, t.artist_id, alt.tier::int AS tier
|
||||
FROM tracks t
|
||||
JOIN albums_tiered alt ON alt.album_id = t.album_id
|
||||
WHERE NOT EXISTS (
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
WHERE q.user_id = $1 AND q.track_id = t.id
|
||||
)
|
||||
@@ -303,7 +306,8 @@ WITH windowed AS (
|
||||
SELECT t.id, t.album_id, t.artist_id
|
||||
FROM tracks t
|
||||
JOIN windowed w ON w.track_id = t.id
|
||||
WHERE NOT EXISTS (
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
WHERE q.user_id = $1 AND q.track_id = t.id
|
||||
)
|
||||
@@ -367,7 +371,8 @@ WITH stats AS (
|
||||
SELECT t.id, t.album_id, t.artist_id
|
||||
FROM tracks t
|
||||
JOIN stats s ON s.track_id = t.id
|
||||
WHERE s.c >= 3
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND s.c >= 3
|
||||
AND s.last_at <= now() - interval '30 days'
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
|
||||
@@ -189,6 +189,7 @@ func (q *Queries) GetSystemPlaylistRun(ctx context.Context, userID pgtype.UUID)
|
||||
|
||||
const listActiveUsersForSystemPlaylists = `-- name: ListActiveUsersForSystemPlaylists :many
|
||||
|
||||
|
||||
SELECT u.id FROM users u
|
||||
WHERE EXISTS (
|
||||
SELECT 1 FROM play_events pe
|
||||
@@ -197,6 +198,13 @@ SELECT u.id FROM users u
|
||||
)
|
||||
`
|
||||
|
||||
// Track picks here join `tracks ... AND t.missing_since IS NULL` (#2523): a
|
||||
// seed or For-You candidate has to be something that can actually play. Note
|
||||
// this only affects newly GENERATED playlists — already-stored system
|
||||
// playlists keep their rows until the next daily rebuild, which is why the
|
||||
// shared ListPlaylistTracks read path is deliberately left unfiltered (it
|
||||
// also serves user-curated playlists, where hiding a track the user added
|
||||
// themselves would be wrong).
|
||||
// M7 #352 slice 2: system-generated playlist queries.
|
||||
// Active = had a play in the last 7 days. The cron iterates this list.
|
||||
func (q *Queries) ListActiveUsersForSystemPlaylists(ctx context.Context) ([]pgtype.UUID, error) {
|
||||
@@ -298,7 +306,7 @@ recent7 AS (
|
||||
COUNT(*) FILTER (WHERE pe.was_skipped = false) AS play_count,
|
||||
0 AS tier
|
||||
FROM play_events pe
|
||||
JOIN tracks t ON t.id = pe.track_id
|
||||
JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.started_at > now() - INTERVAL '7 days'
|
||||
AND t.artist_id IS NOT NULL
|
||||
@@ -309,7 +317,7 @@ recent30 AS (
|
||||
COUNT(*) FILTER (WHERE pe.was_skipped = false) AS play_count,
|
||||
1 AS tier
|
||||
FROM play_events pe
|
||||
JOIN tracks t ON t.id = pe.track_id
|
||||
JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.started_at > now() - INTERVAL '30 days'
|
||||
AND t.artist_id IS NOT NULL
|
||||
@@ -320,7 +328,7 @@ alltime AS (
|
||||
COUNT(*) FILTER (WHERE pe.was_skipped = false) AS play_count,
|
||||
2 AS tier
|
||||
FROM play_events pe
|
||||
JOIN tracks t ON t.id = pe.track_id
|
||||
JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL
|
||||
WHERE pe.user_id = $1
|
||||
AND t.artist_id IS NOT NULL
|
||||
GROUP BY t.artist_id
|
||||
@@ -432,7 +440,7 @@ const pickTopPlayedTrackForArtistByUser = `-- name: PickTopPlayedTrackForArtistB
|
||||
SELECT COALESCE(
|
||||
(SELECT t.id
|
||||
FROM play_events pe
|
||||
JOIN tracks t ON t.id = pe.track_id
|
||||
JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL
|
||||
WHERE pe.user_id = $1
|
||||
AND t.artist_id = $2
|
||||
AND pe.started_at > now() - INTERVAL '7 days'
|
||||
@@ -472,7 +480,7 @@ const pickTopPlayedTracksForUser = `-- name: PickTopPlayedTracksForUser :many
|
||||
WITH recent AS (
|
||||
SELECT t.id, COUNT(*) AS c, 0 AS tier
|
||||
FROM play_events pe
|
||||
JOIN tracks t ON t.id = pe.track_id
|
||||
JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.started_at > now() - INTERVAL '30 days'
|
||||
AND pe.was_skipped = false
|
||||
@@ -481,7 +489,7 @@ WITH recent AS (
|
||||
alltime AS (
|
||||
SELECT t.id, COUNT(*) AS c, 1 AS tier
|
||||
FROM play_events pe
|
||||
JOIN tracks t ON t.id = pe.track_id
|
||||
JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.was_skipped = false
|
||||
GROUP BY t.id
|
||||
|
||||
+223
-10
@@ -11,6 +11,52 @@ import (
|
||||
"github.com/jackc/pgx/v5/pgtype"
|
||||
)
|
||||
|
||||
const adoptTrackPath = `-- name: AdoptTrackPath :execrows
|
||||
UPDATE tracks
|
||||
SET file_path = $1,
|
||||
missing_since = NULL
|
||||
WHERE id = $2
|
||||
AND missing_since IS NOT NULL
|
||||
`
|
||||
|
||||
type AdoptTrackPathParams struct {
|
||||
FilePath string
|
||||
ID pgtype.UUID
|
||||
}
|
||||
|
||||
// Re-points a missing row at the path its file turned up on, and clears the
|
||||
// mark. The caller's normal UpsertTrack then conflicts on file_path and updates
|
||||
// THIS row in place, so the track id survives and its likes, play history and
|
||||
// playlist memberships come with it.
|
||||
//
|
||||
// `missing_since IS NOT NULL` again, this time as a race guard: two files can't
|
||||
// both adopt the same row, and :execrows reports 0 to whichever loses.
|
||||
func (q *Queries) AdoptTrackPath(ctx context.Context, arg AdoptTrackPathParams) (int64, error) {
|
||||
result, err := q.db.Exec(ctx, adoptTrackPath, arg.FilePath, arg.ID)
|
||||
if err != nil {
|
||||
return 0, err
|
||||
}
|
||||
return result.RowsAffected(), nil
|
||||
}
|
||||
|
||||
const clearTracksMissing = `-- name: ClearTracksMissing :execrows
|
||||
UPDATE tracks
|
||||
SET missing_since = NULL
|
||||
WHERE id = ANY($1::uuid[])
|
||||
AND missing_since IS NOT NULL
|
||||
`
|
||||
|
||||
// Clears the mark on rows whose file is back. Runs independently of the mtime
|
||||
// skip check, so a file that reappears unchanged is un-marked even though the
|
||||
// scanner skips re-reading its tags.
|
||||
func (q *Queries) ClearTracksMissing(ctx context.Context, ids []pgtype.UUID) (int64, error) {
|
||||
result, err := q.db.Exec(ctx, clearTracksMissing, ids)
|
||||
if err != nil {
|
||||
return 0, err
|
||||
}
|
||||
return result.RowsAffected(), nil
|
||||
}
|
||||
|
||||
const countTracksByAlbum = `-- name: CountTracksByAlbum :one
|
||||
SELECT count(*) FROM tracks WHERE album_id = $1
|
||||
`
|
||||
@@ -89,8 +135,95 @@ func (q *Queries) DeleteTrack(ctx context.Context, id pgtype.UUID) (DeleteTrackR
|
||||
return i, err
|
||||
}
|
||||
|
||||
const findMissingTrackByFingerprint = `-- name: FindMissingTrackByFingerprint :many
|
||||
SELECT id, file_path FROM tracks
|
||||
WHERE missing_since IS NOT NULL
|
||||
AND file_size = $1
|
||||
AND duration_ms = $2
|
||||
LIMIT 2
|
||||
`
|
||||
|
||||
type FindMissingTrackByFingerprintParams struct {
|
||||
FileSize int64
|
||||
DurationMs int32
|
||||
}
|
||||
|
||||
type FindMissingTrackByFingerprintRow struct {
|
||||
ID pgtype.UUID
|
||||
FilePath string
|
||||
}
|
||||
|
||||
// Move detection fallback for files with no MBID (#2528). Exact byte size AND
|
||||
// exact decoded duration is a strong pair: a plain move or rename preserves
|
||||
// both, while a re-encode changes at least one — and a re-encode genuinely is a
|
||||
// different file, so failing to match there is correct rather than a gap.
|
||||
//
|
||||
// Same missing-only constraint and same LIMIT 2 rationale as the MBID variant.
|
||||
func (q *Queries) FindMissingTrackByFingerprint(ctx context.Context, arg FindMissingTrackByFingerprintParams) ([]FindMissingTrackByFingerprintRow, error) {
|
||||
rows, err := q.db.Query(ctx, findMissingTrackByFingerprint, arg.FileSize, arg.DurationMs)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
defer rows.Close()
|
||||
var items []FindMissingTrackByFingerprintRow
|
||||
for rows.Next() {
|
||||
var i FindMissingTrackByFingerprintRow
|
||||
if err := rows.Scan(&i.ID, &i.FilePath); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
items = append(items, i)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return items, nil
|
||||
}
|
||||
|
||||
const findMissingTrackByMbid = `-- name: FindMissingTrackByMbid :many
|
||||
SELECT id, file_path FROM tracks
|
||||
WHERE missing_since IS NOT NULL
|
||||
AND mbid IS NOT NULL
|
||||
AND mbid = $1::text
|
||||
LIMIT 2
|
||||
`
|
||||
|
||||
type FindMissingTrackByMbidRow struct {
|
||||
ID pgtype.UUID
|
||||
FilePath string
|
||||
}
|
||||
|
||||
// Move detection, strongest signal (#2528). A file that turned up at a new path
|
||||
// carrying a recording MBID we already have on a MISSING row is that recording,
|
||||
// moved — not a new track.
|
||||
//
|
||||
// `missing_since IS NOT NULL` is the safety constraint, not an optimisation: a
|
||||
// row whose file is present elsewhere on disk is a DUPLICATE, and re-pointing
|
||||
// its file_path would corrupt the copy that still exists.
|
||||
//
|
||||
// LIMIT 2 because the caller only needs to know "exactly one" vs "more than
|
||||
// one" — an ambiguous match must not be adopted arbitrarily.
|
||||
func (q *Queries) FindMissingTrackByMbid(ctx context.Context, mbid string) ([]FindMissingTrackByMbidRow, error) {
|
||||
rows, err := q.db.Query(ctx, findMissingTrackByMbid, mbid)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
defer rows.Close()
|
||||
var items []FindMissingTrackByMbidRow
|
||||
for rows.Next() {
|
||||
var i FindMissingTrackByMbidRow
|
||||
if err := rows.Scan(&i.ID, &i.FilePath); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
items = append(items, i)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return items, nil
|
||||
}
|
||||
|
||||
const getTrackByID = `-- name: GetTrackByID :one
|
||||
SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version FROM tracks WHERE id = $1
|
||||
SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since FROM tracks WHERE id = $1
|
||||
`
|
||||
|
||||
func (q *Queries) GetTrackByID(ctx context.Context, id pgtype.UUID) (Track, error) {
|
||||
@@ -114,12 +247,14 @@ func (q *Queries) GetTrackByID(ctx context.Context, id pgtype.UUID) (Track, erro
|
||||
&i.UpdatedAt,
|
||||
&i.TagSource,
|
||||
&i.TagSourcesVersion,
|
||||
&i.TagReadVersion,
|
||||
&i.MissingSince,
|
||||
)
|
||||
return i, err
|
||||
}
|
||||
|
||||
const getTrackByPath = `-- name: GetTrackByPath :one
|
||||
SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version FROM tracks WHERE file_path = $1
|
||||
SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since FROM tracks WHERE file_path = $1
|
||||
`
|
||||
|
||||
func (q *Queries) GetTrackByPath(ctx context.Context, filePath string) (Track, error) {
|
||||
@@ -143,12 +278,14 @@ func (q *Queries) GetTrackByPath(ctx context.Context, filePath string) (Track, e
|
||||
&i.UpdatedAt,
|
||||
&i.TagSource,
|
||||
&i.TagSourcesVersion,
|
||||
&i.TagReadVersion,
|
||||
&i.MissingSince,
|
||||
)
|
||||
return i, err
|
||||
}
|
||||
|
||||
const getTracksByIDs = `-- name: GetTracksByIDs :many
|
||||
SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version FROM tracks WHERE id = ANY($1::uuid[])
|
||||
SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since FROM tracks WHERE id = ANY($1::uuid[])
|
||||
`
|
||||
|
||||
// Batched lookup used by /api/library/sync to hydrate upsert payloads
|
||||
@@ -180,6 +317,8 @@ func (q *Queries) GetTracksByIDs(ctx context.Context, dollar_1 []pgtype.UUID) ([
|
||||
&i.UpdatedAt,
|
||||
&i.TagSource,
|
||||
&i.TagSourcesVersion,
|
||||
&i.TagReadVersion,
|
||||
&i.MissingSince,
|
||||
); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
@@ -192,7 +331,7 @@ func (q *Queries) GetTracksByIDs(ctx context.Context, dollar_1 []pgtype.UUID) ([
|
||||
}
|
||||
|
||||
const listArtistTracksForUser = `-- name: ListArtistTracksForUser :many
|
||||
SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version,
|
||||
SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since,
|
||||
albums.title AS album_title,
|
||||
artists.name AS artist_name
|
||||
FROM tracks t
|
||||
@@ -250,6 +389,8 @@ func (q *Queries) ListArtistTracksForUser(ctx context.Context, arg ListArtistTra
|
||||
&i.Track.UpdatedAt,
|
||||
&i.Track.TagSource,
|
||||
&i.Track.TagSourcesVersion,
|
||||
&i.Track.TagReadVersion,
|
||||
&i.Track.MissingSince,
|
||||
&i.AlbumTitle,
|
||||
&i.ArtistName,
|
||||
); err != nil {
|
||||
@@ -264,7 +405,7 @@ func (q *Queries) ListArtistTracksForUser(ctx context.Context, arg ListArtistTra
|
||||
}
|
||||
|
||||
const listRandomTracksForUser = `-- name: ListRandomTracksForUser :many
|
||||
SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version,
|
||||
SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since,
|
||||
albums.title AS album_title,
|
||||
artists.name AS artist_name
|
||||
FROM tracks t
|
||||
@@ -319,6 +460,8 @@ func (q *Queries) ListRandomTracksForUser(ctx context.Context, arg ListRandomTra
|
||||
&i.Track.UpdatedAt,
|
||||
&i.Track.TagSource,
|
||||
&i.Track.TagSourcesVersion,
|
||||
&i.Track.TagReadVersion,
|
||||
&i.Track.MissingSince,
|
||||
&i.AlbumTitle,
|
||||
&i.ArtistName,
|
||||
); err != nil {
|
||||
@@ -332,8 +475,43 @@ func (q *Queries) ListRandomTracksForUser(ctx context.Context, arg ListRandomTra
|
||||
return items, nil
|
||||
}
|
||||
|
||||
const listTrackPathsForReconcile = `-- name: ListTrackPathsForReconcile :many
|
||||
SELECT id, file_path, missing_since FROM tracks
|
||||
`
|
||||
|
||||
type ListTrackPathsForReconcileRow struct {
|
||||
ID pgtype.UUID
|
||||
FilePath string
|
||||
MissingSince pgtype.Timestamptz
|
||||
}
|
||||
|
||||
// Every row's path + current missing mark, for the scanner's reconcile pass
|
||||
// (#2523). Deliberately unfiltered and unpaged: reconcile has to compare the
|
||||
// WHOLE table against what the walk saw, and a filtered subset would let rows
|
||||
// outside it drift forever. Three narrow columns keep it cheap even on a
|
||||
// library of a few hundred thousand tracks.
|
||||
func (q *Queries) ListTrackPathsForReconcile(ctx context.Context) ([]ListTrackPathsForReconcileRow, error) {
|
||||
rows, err := q.db.Query(ctx, listTrackPathsForReconcile)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
defer rows.Close()
|
||||
var items []ListTrackPathsForReconcileRow
|
||||
for rows.Next() {
|
||||
var i ListTrackPathsForReconcileRow
|
||||
if err := rows.Scan(&i.ID, &i.FilePath, &i.MissingSince); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
items = append(items, i)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return items, nil
|
||||
}
|
||||
|
||||
const listTracksByAlbum = `-- name: ListTracksByAlbum :many
|
||||
SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version FROM tracks
|
||||
SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since FROM tracks
|
||||
WHERE album_id = $1
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
@@ -377,6 +555,8 @@ func (q *Queries) ListTracksByAlbum(ctx context.Context, arg ListTracksByAlbumPa
|
||||
&i.UpdatedAt,
|
||||
&i.TagSource,
|
||||
&i.TagSourcesVersion,
|
||||
&i.TagReadVersion,
|
||||
&i.MissingSince,
|
||||
); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
@@ -423,8 +603,31 @@ func (q *Queries) ListTracksMissingMbidWithPath(ctx context.Context, limit int32
|
||||
return items, nil
|
||||
}
|
||||
|
||||
const markTracksMissing = `-- name: MarkTracksMissing :execrows
|
||||
UPDATE tracks
|
||||
SET missing_since = now()
|
||||
WHERE id = ANY($1::uuid[])
|
||||
AND missing_since IS NULL
|
||||
`
|
||||
|
||||
// Marks rows whose file the walk did not see. `missing_since IS NULL` in the
|
||||
// predicate makes this idempotent: a row already marked keeps its ORIGINAL
|
||||
// timestamp, so "how long has it been gone" survives repeated scans. Losing
|
||||
// that would make any age-based cleanup policy meaningless.
|
||||
//
|
||||
// updated_at is deliberately NOT touched. It tracks content changes and gates
|
||||
// the scanner's mtime skip; moving it here would make a returning file look
|
||||
// newer than its own mtime and stop its tags being re-read.
|
||||
func (q *Queries) MarkTracksMissing(ctx context.Context, ids []pgtype.UUID) (int64, error) {
|
||||
result, err := q.db.Exec(ctx, markTracksMissing, ids)
|
||||
if err != nil {
|
||||
return 0, err
|
||||
}
|
||||
return result.RowsAffected(), nil
|
||||
}
|
||||
|
||||
const searchTracks = `-- name: SearchTracks :many
|
||||
SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version FROM tracks
|
||||
SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since FROM tracks
|
||||
WHERE title ILIKE '%' || $1::text || '%'
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
@@ -475,6 +678,8 @@ func (q *Queries) SearchTracks(ctx context.Context, arg SearchTracksParams) ([]T
|
||||
&i.UpdatedAt,
|
||||
&i.TagSource,
|
||||
&i.TagSourcesVersion,
|
||||
&i.TagReadVersion,
|
||||
&i.MissingSince,
|
||||
); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
@@ -507,8 +712,9 @@ func (q *Queries) SetTrackMbidIfNull(ctx context.Context, arg SetTrackMbidIfNull
|
||||
const upsertTrack = `-- name: UpsertTrack :one
|
||||
INSERT INTO tracks (
|
||||
title, album_id, artist_id, track_number, disc_number,
|
||||
duration_ms, file_path, file_size, file_format, bitrate, mbid, genre
|
||||
) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12)
|
||||
duration_ms, file_path, file_size, file_format, bitrate, mbid, genre,
|
||||
tag_read_version
|
||||
) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13)
|
||||
ON CONFLICT (file_path) DO UPDATE SET
|
||||
title = EXCLUDED.title,
|
||||
album_id = EXCLUDED.album_id,
|
||||
@@ -521,8 +727,11 @@ ON CONFLICT (file_path) DO UPDATE SET
|
||||
bitrate = EXCLUDED.bitrate,
|
||||
mbid = EXCLUDED.mbid,
|
||||
genre = EXCLUDED.genre,
|
||||
-- Stamped on update too, so a tag-repair pass marks rows as done and the
|
||||
-- next scan can short-circuit them again (#2499).
|
||||
tag_read_version = EXCLUDED.tag_read_version,
|
||||
updated_at = now()
|
||||
RETURNING id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version
|
||||
RETURNING id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since
|
||||
`
|
||||
|
||||
type UpsertTrackParams struct {
|
||||
@@ -538,6 +747,7 @@ type UpsertTrackParams struct {
|
||||
Bitrate *int32
|
||||
Mbid *string
|
||||
Genre *string
|
||||
TagReadVersion int16
|
||||
}
|
||||
|
||||
// file_path is the canonical identity for library scan; mbid is secondary.
|
||||
@@ -555,6 +765,7 @@ func (q *Queries) UpsertTrack(ctx context.Context, arg UpsertTrackParams) (Track
|
||||
arg.Bitrate,
|
||||
arg.Mbid,
|
||||
arg.Genre,
|
||||
arg.TagReadVersion,
|
||||
)
|
||||
var i Track
|
||||
err := row.Scan(
|
||||
@@ -575,6 +786,8 @@ func (q *Queries) UpsertTrack(ctx context.Context, arg UpsertTrackParams) (Track
|
||||
&i.UpdatedAt,
|
||||
&i.TagSource,
|
||||
&i.TagSourcesVersion,
|
||||
&i.TagReadVersion,
|
||||
&i.MissingSince,
|
||||
)
|
||||
return i, err
|
||||
}
|
||||
|
||||
@@ -0,0 +1,2 @@
|
||||
ALTER TABLE tracks
|
||||
DROP COLUMN tag_read_version;
|
||||
@@ -0,0 +1,15 @@
|
||||
-- Records which version of the scanner's tag-extraction logic last wrote a
|
||||
-- track's tag-derived columns (#2499).
|
||||
--
|
||||
-- DEFAULT 0 is the point of this migration: every existing row lands below the
|
||||
-- scanner's current library.tagReadVersion, so the next scan re-reads its tags
|
||||
-- instead of short-circuiting on the mtime check. That repairs genre values the
|
||||
-- old reader welded together ("Alternative Rock" + "Rock" -> "Alternative
|
||||
-- RockRock") without asking the operator to wipe and rebuild the library.
|
||||
--
|
||||
-- Bump library.tagReadVersion in Go — not this default — whenever a tag
|
||||
-- extraction fix needs to reach already-indexed files. That makes tag repairs a
|
||||
-- self-healing scan rather than a manual full rebuild, which is why this is a
|
||||
-- version number and not a boolean "needs_reread" flag.
|
||||
ALTER TABLE tracks
|
||||
ADD COLUMN tag_read_version smallint NOT NULL DEFAULT 0;
|
||||
@@ -0,0 +1,4 @@
|
||||
DROP INDEX IF EXISTS tracks_missing_since_idx;
|
||||
|
||||
ALTER TABLE tracks
|
||||
DROP COLUMN missing_since;
|
||||
@@ -0,0 +1,25 @@
|
||||
-- Marks a track whose file the scanner could no longer find (#2523).
|
||||
--
|
||||
-- NULL means present. A timestamp means the file was absent as of that scan,
|
||||
-- and is the point from which "how long has this been gone" is measured — which
|
||||
-- is what a later cleanup pass needs in order to require a grace period rather
|
||||
-- than deleting on a single missed stat.
|
||||
--
|
||||
-- Deliberately a nullable timestamp rather than a boolean: "missing" is not a
|
||||
-- state we want to act on immediately, and the age is the only thing that makes
|
||||
-- an automated deletion safe to reason about.
|
||||
--
|
||||
-- No default and no backfill. Existing rows start NULL (present) and the next
|
||||
-- full scan sets the mark where it belongs — a migration cannot check the
|
||||
-- filesystem, and guessing here would mark the whole library on a server whose
|
||||
-- media volume happens to be detached at upgrade time.
|
||||
ALTER TABLE tracks
|
||||
ADD COLUMN missing_since timestamptz;
|
||||
|
||||
-- Partial index: the only query that filters on this column positively is the
|
||||
-- admin "what's missing" list, which is a small set. Playback and browse
|
||||
-- queries filter `missing_since IS NULL`, which matches nearly every row and is
|
||||
-- better served by a sequential scan than an index lookup.
|
||||
CREATE INDEX tracks_missing_since_idx
|
||||
ON tracks (missing_since)
|
||||
WHERE missing_since IS NOT NULL;
|
||||
@@ -1,3 +1,15 @@
|
||||
-- Every query in this file filters `tracks.missing_since IS NULL` (#2523).
|
||||
-- A row whose file has vanished keeps its genre forever — the scanner walks the
|
||||
-- filesystem, so it never revisits a path that no longer exists — which is how
|
||||
-- pre-#2499 welded genres survived a full re-scan and kept showing in the index.
|
||||
-- Browsing is a way of finding something to play, so a track that cannot play
|
||||
-- should not shape it.
|
||||
--
|
||||
-- Year queries below join albums only and are deliberately left alone: an album
|
||||
-- is still a real release even if some of its tracks are gone. An album whose
|
||||
-- EVERY track is missing will linger on the year axis; that's a narrower case,
|
||||
-- tracked with the rest of the cleanup work.
|
||||
|
||||
-- name: ListGenresWithCount :many
|
||||
-- Genre browse index (#367).
|
||||
--
|
||||
@@ -22,6 +34,7 @@ SELECT trim(g.genre) AS genre, COUNT(DISTINCT tracks.id)::bigint AS track_count
|
||||
FROM tracks
|
||||
JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true
|
||||
WHERE trim(g.genre) <> ''
|
||||
AND tracks.missing_since IS NULL
|
||||
GROUP BY trim(g.genre)
|
||||
-- Ordered by the expression, not the output alias: `ORDER BY genre` is
|
||||
-- ambiguous between the alias and tracks.genre, and sqlc rejects it.
|
||||
@@ -41,6 +54,7 @@ WHERE EXISTS (
|
||||
FROM tracks
|
||||
JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true
|
||||
WHERE tracks.album_id = albums.id
|
||||
AND tracks.missing_since IS NULL
|
||||
AND trim(g.genre) = trim(sqlc.arg(genre)::text)
|
||||
)
|
||||
ORDER BY albums.sort_title, albums.id
|
||||
@@ -56,6 +70,7 @@ WHERE EXISTS (
|
||||
FROM tracks
|
||||
JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true
|
||||
WHERE tracks.album_id = albums.id
|
||||
AND tracks.missing_since IS NULL
|
||||
AND trim(g.genre) = trim(sqlc.arg(genre)::text)
|
||||
);
|
||||
|
||||
@@ -96,6 +111,7 @@ SELECT DISTINCT trim(g.genre) AS genre
|
||||
FROM tracks
|
||||
JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true
|
||||
WHERE tracks.album_id = $1 AND trim(g.genre) <> ''
|
||||
AND tracks.missing_since IS NULL
|
||||
ORDER BY trim(g.genre);
|
||||
|
||||
-- name: ListGenresForArtist :many
|
||||
@@ -106,4 +122,5 @@ SELECT DISTINCT trim(g.genre) AS genre
|
||||
FROM tracks
|
||||
JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true
|
||||
WHERE tracks.artist_id = $1 AND trim(g.genre) <> ''
|
||||
AND tracks.missing_since IS NULL
|
||||
ORDER BY trim(g.genre);
|
||||
|
||||
@@ -29,7 +29,8 @@ dormant_artists AS (
|
||||
SELECT t.id, t.album_id, t.artist_id
|
||||
FROM tracks t
|
||||
JOIN dormant_artists da ON da.id = t.artist_id
|
||||
WHERE NOT EXISTS (
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM play_events pe
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.track_id = t.id
|
||||
@@ -60,7 +61,8 @@ SELECT t.id, t.album_id, t.artist_id
|
||||
SELECT t.id, t.album_id, t.artist_id
|
||||
FROM general_likes gl
|
||||
JOIN tracks t ON t.id = gl.track_id
|
||||
WHERE gl.user_id != $1
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND gl.user_id != $1
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM play_events pe
|
||||
WHERE pe.user_id = $1
|
||||
@@ -86,7 +88,8 @@ SELECT t.id, t.album_id, t.artist_id
|
||||
-- $1 = user_id, $2 = date string for md5 ordering.
|
||||
SELECT t.id, t.album_id, t.artist_id
|
||||
FROM tracks t
|
||||
WHERE NOT EXISTS (
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM play_events pe
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.track_id = t.id
|
||||
@@ -117,7 +120,8 @@ SELECT t.id, t.album_id, t.artist_id
|
||||
FROM tracks t
|
||||
JOIN LATERAL regexp_split_to_table(coalesce(t.genre, ''), '[;,]') AS g_split(g) ON true
|
||||
JOIN taste_profile_tags nt ON nt.user_id = $1 AND trim(g_split.g) = nt.tag
|
||||
WHERE nt.weight > 0
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND nt.weight > 0
|
||||
AND trim(g_split.g) <> ''
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM play_events pe
|
||||
|
||||
@@ -24,6 +24,7 @@ LEFT JOIN LATERAL (
|
||||
WHERE user_id = $1 AND track_id = t.id
|
||||
) pe ON true
|
||||
WHERE t.id <> $2
|
||||
AND t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM play_events
|
||||
WHERE user_id = $1 AND track_id = t.id
|
||||
@@ -177,7 +178,7 @@ FROM (
|
||||
UNION ALL SELECT track_id, sim_score FROM coplay_artists
|
||||
UNION ALL SELECT track_id, sim_score FROM random_fill
|
||||
) u
|
||||
JOIN tracks t ON t.id = u.track_id
|
||||
JOIN tracks t ON t.id = u.track_id AND t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
JOIN albums al ON al.id = t.album_id
|
||||
LEFT JOIN general_likes l ON l.user_id = $1 AND l.track_id = t.id
|
||||
LEFT JOIN LATERAL (
|
||||
@@ -382,7 +383,8 @@ FROM plays p
|
||||
JOIN tracks t ON t.id = p.track_id
|
||||
JOIN albums ON albums.id = t.album_id
|
||||
JOIN artists ON artists.id = t.artist_id
|
||||
WHERE NOT EXISTS (
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
WHERE q.user_id = $1 AND q.track_id = t.id
|
||||
)
|
||||
@@ -408,6 +410,7 @@ JOIN tracks t ON t.id = p.track_id
|
||||
JOIN albums ON albums.id = t.album_id
|
||||
JOIN artists ON artists.id = t.artist_id
|
||||
WHERE t.artist_id = sqlc.arg(artist_id)
|
||||
AND t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
WHERE q.user_id = sqlc.arg(user_id) AND q.track_id = t.id
|
||||
|
||||
@@ -44,13 +44,21 @@ ORDER BY 1, 2;
|
||||
-- mean completion ratio over the completion_n plays that recorded one.
|
||||
-- pick_kind splits For You plays into taste/fresh/unattributed (#1249);
|
||||
-- it is NULL for every other source, so those still group to one row.
|
||||
--
|
||||
-- completion_sqsum carries the sum of SQUARED completion ratios so the Go
|
||||
-- handler can compute a variance — needed for the margin of error on a
|
||||
-- completion delta (#2495). It is the sum rather than `stddev_samp` on purpose:
|
||||
-- raw source rows get merged into surface families in Go, and sums of squares
|
||||
-- add across groups exactly, whereas standard deviations cannot be combined
|
||||
-- without them. Variance = (sqsum - sum²/n) / (n-1), with sum = avg × n.
|
||||
SELECT
|
||||
pe.source,
|
||||
pe.pick_kind,
|
||||
count(*)::bigint AS plays,
|
||||
count(*) FILTER (WHERE pe.was_skipped)::bigint AS skips,
|
||||
count(pe.completion_ratio)::bigint AS completion_n,
|
||||
COALESCE(avg(pe.completion_ratio), 0)::float8 AS avg_completion
|
||||
COALESCE(avg(pe.completion_ratio), 0)::float8 AS avg_completion,
|
||||
COALESCE(sum(pe.completion_ratio * pe.completion_ratio), 0)::float8 AS completion_sqsum
|
||||
FROM play_events pe
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.started_at > now() - ($2::float8 * INTERVAL '1 day')
|
||||
|
||||
@@ -40,7 +40,8 @@ SELECT t.id, t.album_id, t.artist_id
|
||||
JOIN affinity_artists aa ON aa.artist_id = t.artist_id
|
||||
LEFT JOIN play_counts pc ON pc.track_id = t.id
|
||||
LEFT JOIN skip_counts sc ON sc.track_id = t.id
|
||||
WHERE COALESCE(pc.c, 0) <= 2
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND COALESCE(pc.c, 0) <= 2
|
||||
AND COALESCE(sc.c, 0) < 2
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
@@ -70,7 +71,8 @@ WITH stats AS (
|
||||
SELECT t.id, t.album_id, t.artist_id
|
||||
FROM tracks t
|
||||
JOIN stats s ON s.track_id = t.id
|
||||
WHERE s.c >= 3
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND s.c >= 3
|
||||
AND s.last_at <= now() - interval '30 days'
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
@@ -149,7 +151,8 @@ albums_tiered AS (
|
||||
SELECT t.id, t.album_id, t.artist_id, alt.tier::int AS tier
|
||||
FROM tracks t
|
||||
JOIN albums_tiered alt ON alt.album_id = t.album_id
|
||||
WHERE NOT EXISTS (
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
WHERE q.user_id = $1 AND q.track_id = t.id
|
||||
)
|
||||
@@ -187,7 +190,8 @@ WITH windowed AS (
|
||||
SELECT t.id, t.album_id, t.artist_id
|
||||
FROM tracks t
|
||||
JOIN windowed w ON w.track_id = t.id
|
||||
WHERE NOT EXISTS (
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
WHERE q.user_id = $1 AND q.track_id = t.id
|
||||
)
|
||||
@@ -240,7 +244,8 @@ SELECT t.id, t.album_id, t.artist_id, alt.tier::int AS tier
|
||||
FROM tracks t
|
||||
JOIN albums al ON al.id = t.album_id
|
||||
JOIN albums_tiered alt ON alt.album_id = al.id
|
||||
WHERE alt.tier IS NOT NULL
|
||||
WHERE t.missing_since IS NULL -- #2523: never offer a file that is gone
|
||||
AND alt.tier IS NOT NULL
|
||||
AND NOT EXISTS (SELECT 1 FROM attempted a WHERE a.track_id = t.id)
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM lidarr_quarantine q
|
||||
|
||||
@@ -1,3 +1,11 @@
|
||||
-- Track picks here join `tracks ... AND t.missing_since IS NULL` (#2523): a
|
||||
-- seed or For-You candidate has to be something that can actually play. Note
|
||||
-- this only affects newly GENERATED playlists — already-stored system
|
||||
-- playlists keep their rows until the next daily rebuild, which is why the
|
||||
-- shared ListPlaylistTracks read path is deliberately left unfiltered (it
|
||||
-- also serves user-curated playlists, where hiding a track the user added
|
||||
-- themselves would be wrong).
|
||||
|
||||
-- M7 #352 slice 2: system-generated playlist queries.
|
||||
|
||||
-- name: ListActiveUsersForSystemPlaylists :many
|
||||
@@ -72,7 +80,7 @@ recent7 AS (
|
||||
COUNT(*) FILTER (WHERE pe.was_skipped = false) AS play_count,
|
||||
0 AS tier
|
||||
FROM play_events pe
|
||||
JOIN tracks t ON t.id = pe.track_id
|
||||
JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.started_at > now() - INTERVAL '7 days'
|
||||
AND t.artist_id IS NOT NULL
|
||||
@@ -83,7 +91,7 @@ recent30 AS (
|
||||
COUNT(*) FILTER (WHERE pe.was_skipped = false) AS play_count,
|
||||
1 AS tier
|
||||
FROM play_events pe
|
||||
JOIN tracks t ON t.id = pe.track_id
|
||||
JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.started_at > now() - INTERVAL '30 days'
|
||||
AND t.artist_id IS NOT NULL
|
||||
@@ -94,7 +102,7 @@ alltime AS (
|
||||
COUNT(*) FILTER (WHERE pe.was_skipped = false) AS play_count,
|
||||
2 AS tier
|
||||
FROM play_events pe
|
||||
JOIN tracks t ON t.id = pe.track_id
|
||||
JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL
|
||||
WHERE pe.user_id = $1
|
||||
AND t.artist_id IS NOT NULL
|
||||
GROUP BY t.artist_id
|
||||
@@ -139,7 +147,7 @@ SELECT c.artist_id,
|
||||
WITH recent AS (
|
||||
SELECT t.id, COUNT(*) AS c, 0 AS tier
|
||||
FROM play_events pe
|
||||
JOIN tracks t ON t.id = pe.track_id
|
||||
JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.started_at > now() - INTERVAL '30 days'
|
||||
AND pe.was_skipped = false
|
||||
@@ -148,7 +156,7 @@ WITH recent AS (
|
||||
alltime AS (
|
||||
SELECT t.id, COUNT(*) AS c, 1 AS tier
|
||||
FROM play_events pe
|
||||
JOIN tracks t ON t.id = pe.track_id
|
||||
JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL
|
||||
WHERE pe.user_id = $1
|
||||
AND pe.was_skipped = false
|
||||
GROUP BY t.id
|
||||
@@ -181,7 +189,7 @@ SELECT id
|
||||
SELECT COALESCE(
|
||||
(SELECT t.id
|
||||
FROM play_events pe
|
||||
JOIN tracks t ON t.id = pe.track_id
|
||||
JOIN tracks t ON t.id = pe.track_id AND t.missing_since IS NULL
|
||||
WHERE pe.user_id = $1
|
||||
AND t.artist_id = $2
|
||||
AND pe.started_at > now() - INTERVAL '7 days'
|
||||
|
||||
@@ -2,8 +2,9 @@
|
||||
-- file_path is the canonical identity for library scan; mbid is secondary.
|
||||
INSERT INTO tracks (
|
||||
title, album_id, artist_id, track_number, disc_number,
|
||||
duration_ms, file_path, file_size, file_format, bitrate, mbid, genre
|
||||
) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12)
|
||||
duration_ms, file_path, file_size, file_format, bitrate, mbid, genre,
|
||||
tag_read_version
|
||||
) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13)
|
||||
ON CONFLICT (file_path) DO UPDATE SET
|
||||
title = EXCLUDED.title,
|
||||
album_id = EXCLUDED.album_id,
|
||||
@@ -16,6 +17,9 @@ ON CONFLICT (file_path) DO UPDATE SET
|
||||
bitrate = EXCLUDED.bitrate,
|
||||
mbid = EXCLUDED.mbid,
|
||||
genre = EXCLUDED.genre,
|
||||
-- Stamped on update too, so a tag-repair pass marks rows as done and the
|
||||
-- next scan can short-circuit them again (#2499).
|
||||
tag_read_version = EXCLUDED.tag_read_version,
|
||||
updated_at = now()
|
||||
RETURNING *;
|
||||
|
||||
@@ -133,3 +137,78 @@ RETURNING id, album_id, artist_id, file_path, mbid;
|
||||
-- Batched lookup used by /api/library/sync to hydrate upsert payloads
|
||||
-- (#357). Mirror of GetArtistsByIDs.
|
||||
SELECT * FROM tracks WHERE id = ANY($1::uuid[]);
|
||||
|
||||
-- name: FindMissingTrackByMbid :many
|
||||
-- Move detection, strongest signal (#2528). A file that turned up at a new path
|
||||
-- carrying a recording MBID we already have on a MISSING row is that recording,
|
||||
-- moved — not a new track.
|
||||
--
|
||||
-- `missing_since IS NOT NULL` is the safety constraint, not an optimisation: a
|
||||
-- row whose file is present elsewhere on disk is a DUPLICATE, and re-pointing
|
||||
-- its file_path would corrupt the copy that still exists.
|
||||
--
|
||||
-- LIMIT 2 because the caller only needs to know "exactly one" vs "more than
|
||||
-- one" — an ambiguous match must not be adopted arbitrarily.
|
||||
SELECT id, file_path FROM tracks
|
||||
WHERE missing_since IS NOT NULL
|
||||
AND mbid IS NOT NULL
|
||||
AND mbid = sqlc.arg(mbid)::text
|
||||
LIMIT 2;
|
||||
|
||||
-- name: FindMissingTrackByFingerprint :many
|
||||
-- Move detection fallback for files with no MBID (#2528). Exact byte size AND
|
||||
-- exact decoded duration is a strong pair: a plain move or rename preserves
|
||||
-- both, while a re-encode changes at least one — and a re-encode genuinely is a
|
||||
-- different file, so failing to match there is correct rather than a gap.
|
||||
--
|
||||
-- Same missing-only constraint and same LIMIT 2 rationale as the MBID variant.
|
||||
SELECT id, file_path FROM tracks
|
||||
WHERE missing_since IS NOT NULL
|
||||
AND file_size = sqlc.arg(file_size)
|
||||
AND duration_ms = sqlc.arg(duration_ms)
|
||||
LIMIT 2;
|
||||
|
||||
-- name: AdoptTrackPath :execrows
|
||||
-- Re-points a missing row at the path its file turned up on, and clears the
|
||||
-- mark. The caller's normal UpsertTrack then conflicts on file_path and updates
|
||||
-- THIS row in place, so the track id survives and its likes, play history and
|
||||
-- playlist memberships come with it.
|
||||
--
|
||||
-- `missing_since IS NOT NULL` again, this time as a race guard: two files can't
|
||||
-- both adopt the same row, and :execrows reports 0 to whichever loses.
|
||||
UPDATE tracks
|
||||
SET file_path = sqlc.arg(file_path),
|
||||
missing_since = NULL
|
||||
WHERE id = sqlc.arg(id)
|
||||
AND missing_since IS NOT NULL;
|
||||
|
||||
-- name: ListTrackPathsForReconcile :many
|
||||
-- Every row's path + current missing mark, for the scanner's reconcile pass
|
||||
-- (#2523). Deliberately unfiltered and unpaged: reconcile has to compare the
|
||||
-- WHOLE table against what the walk saw, and a filtered subset would let rows
|
||||
-- outside it drift forever. Three narrow columns keep it cheap even on a
|
||||
-- library of a few hundred thousand tracks.
|
||||
SELECT id, file_path, missing_since FROM tracks;
|
||||
|
||||
-- name: MarkTracksMissing :execrows
|
||||
-- Marks rows whose file the walk did not see. `missing_since IS NULL` in the
|
||||
-- predicate makes this idempotent: a row already marked keeps its ORIGINAL
|
||||
-- timestamp, so "how long has it been gone" survives repeated scans. Losing
|
||||
-- that would make any age-based cleanup policy meaningless.
|
||||
--
|
||||
-- updated_at is deliberately NOT touched. It tracks content changes and gates
|
||||
-- the scanner's mtime skip; moving it here would make a returning file look
|
||||
-- newer than its own mtime and stop its tags being re-read.
|
||||
UPDATE tracks
|
||||
SET missing_since = now()
|
||||
WHERE id = ANY(sqlc.arg(ids)::uuid[])
|
||||
AND missing_since IS NULL;
|
||||
|
||||
-- name: ClearTracksMissing :execrows
|
||||
-- Clears the mark on rows whose file is back. Runs independently of the mtime
|
||||
-- skip check, so a file that reappears unchanged is un-marked even though the
|
||||
-- scanner skips re-reading its tags.
|
||||
UPDATE tracks
|
||||
SET missing_since = NULL
|
||||
WHERE id = ANY(sqlc.arg(ids)::uuid[])
|
||||
AND missing_since IS NOT NULL;
|
||||
|
||||
@@ -0,0 +1,179 @@
|
||||
package library
|
||||
|
||||
import (
|
||||
"io"
|
||||
"strconv"
|
||||
"strings"
|
||||
|
||||
"github.com/dhowden/tag"
|
||||
)
|
||||
|
||||
// genreDelimiter is what we join multi-value genres with on the way into
|
||||
// tracks.genre. It has to be one of the characters the read side already splits
|
||||
// on — internal/taste and internal/recommendation both split on [;,], as do
|
||||
// browse.sql, recommendation.sql and discover.sql. Storing values joined with
|
||||
// ";" means the entire fix lands in the scanner and no query changes.
|
||||
const genreDelimiter = ";"
|
||||
|
||||
// extractGenres returns the genre values for a file, normalised and
|
||||
// deduplicated, ready to be joined with genreDelimiter.
|
||||
//
|
||||
// fellBack reports that an ID3v2 file's genre frame could not be parsed and the
|
||||
// value came from dhowden/tag instead. That path yields the old welded string,
|
||||
// so it is worth logging — but it is still the best available answer, and
|
||||
// degrading to it beats storing no genre at all.
|
||||
func extractGenres(meta tag.Metadata, rs io.ReadSeeker) (genres []string, fellBack bool) {
|
||||
switch meta.Format() {
|
||||
case tag.ID3v2_2, tag.ID3v2_3, tag.ID3v2_4:
|
||||
values, err := readID3v2GenreValues(rs)
|
||||
if err == nil {
|
||||
return normaliseGenres(values), false
|
||||
}
|
||||
// No frame at all is the common case for untagged files, and
|
||||
// dhowden/tag will have nothing either — not worth flagging.
|
||||
fellBack = meta.Genre() != ""
|
||||
default:
|
||||
// Vorbis comments (FLAC/OGG/Opus) and MP4 atoms don't go through
|
||||
// dhowden's welding path, so its value is already a faithful read of
|
||||
// the primary genre. Multi-value handling for those containers is a
|
||||
// separate, unproven concern — see #2500.
|
||||
}
|
||||
return normaliseGenres([]string{meta.Genre()}), fellBack
|
||||
}
|
||||
|
||||
// normaliseGenres expands each raw value, then drops case-insensitive
|
||||
// duplicates while keeping the first spelling seen. Duplicates are common once
|
||||
// numeric references are resolved: "(40)AlternRock" declares the same genre
|
||||
// twice, and so does a file tagged both "Rock" and "rock".
|
||||
func normaliseGenres(values []string) []string {
|
||||
out := make([]string, 0, len(values))
|
||||
seen := make(map[string]struct{}, len(values))
|
||||
for _, v := range values {
|
||||
for _, g := range normaliseGenreValue(v) {
|
||||
key := strings.ToLower(g)
|
||||
if _, dup := seen[key]; dup {
|
||||
continue
|
||||
}
|
||||
seen[key] = struct{}{}
|
||||
out = append(out, g)
|
||||
}
|
||||
}
|
||||
if len(out) == 0 {
|
||||
return nil
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// normaliseGenreValue turns one raw tag value into zero or more genre names,
|
||||
// resolving the ID3 numeric-reference syntax.
|
||||
//
|
||||
// A value may be:
|
||||
// - plain text ("Alternative Rock") — passed through
|
||||
// - a bare ID3v1 index ("17") — resolved to "Rock". This is what the spec
|
||||
// says a numeric TCON means, and what ffmpeg does. It is why the operator's
|
||||
// library showed genres like "4017" and "526617": several numeric values
|
||||
// welded together by the old reader.
|
||||
// - ID3v2.3 refinement syntax ("(17)", "(51)(39)", "(17)Hard Rock", "(RX)")
|
||||
// — each parenthesised index becomes its own genre, and trailing text
|
||||
// becomes one more.
|
||||
//
|
||||
// Values that are numeric but out of range carry no meaning as a label, so they
|
||||
// are dropped rather than stored as digits.
|
||||
func normaliseGenreValue(v string) []string {
|
||||
v = strings.TrimSpace(v)
|
||||
if v == "" {
|
||||
return nil
|
||||
}
|
||||
|
||||
var out []string
|
||||
for strings.HasPrefix(v, "(") {
|
||||
// "((" is the spec's escape for a literal "(" — the rest is plain text.
|
||||
if strings.HasPrefix(v, "((") {
|
||||
return append(out, strings.TrimSpace(v[1:]))
|
||||
}
|
||||
end := strings.IndexByte(v, ')')
|
||||
if end < 0 {
|
||||
break
|
||||
}
|
||||
inner := strings.TrimSpace(v[1:end])
|
||||
switch {
|
||||
case strings.EqualFold(inner, "RX"):
|
||||
out = append(out, "Remix")
|
||||
case strings.EqualFold(inner, "CR"):
|
||||
out = append(out, "Cover")
|
||||
default:
|
||||
n, err := strconv.Atoi(inner)
|
||||
if err != nil {
|
||||
// Parenthesised but not a reference, e.g. "(Live)". Keep the
|
||||
// whole remainder as written.
|
||||
return append(out, v)
|
||||
}
|
||||
if name, ok := id3v1GenreName(n); ok {
|
||||
out = append(out, name)
|
||||
}
|
||||
}
|
||||
v = strings.TrimSpace(v[end+1:])
|
||||
}
|
||||
|
||||
if v == "" {
|
||||
return out
|
||||
}
|
||||
if n, err := strconv.Atoi(v); err == nil {
|
||||
if name, ok := id3v1GenreName(n); ok {
|
||||
return append(out, name)
|
||||
}
|
||||
return out
|
||||
}
|
||||
return append(out, v)
|
||||
}
|
||||
|
||||
func id3v1GenreName(n int) (string, bool) {
|
||||
if n < 0 || n >= len(id3v1Genres) {
|
||||
return "", false
|
||||
}
|
||||
return id3v1Genres[n], true
|
||||
}
|
||||
|
||||
// id3v1Genres is the ID3v1 genre index: entries 0-79 are the original list,
|
||||
// 80-125 were added by Winamp, and 126-191 later still. Index is meaningful, so
|
||||
// never reorder or remove an entry — a numeric tag written years ago resolves
|
||||
// through this table by position.
|
||||
//
|
||||
// Entry 133 is "Afro-Punk"; the 1990s list used a slur there, and no file in
|
||||
// practice depends on the original spelling.
|
||||
var id3v1Genres = []string{
|
||||
"Blues", "Classic Rock", "Country", "Dance", "Disco", "Funk", "Grunge",
|
||||
"Hip-Hop", "Jazz", "Metal", "New Age", "Oldies", "Other", "Pop", "R&B",
|
||||
"Rap", "Reggae", "Rock", "Techno", "Industrial", "Alternative", "Ska",
|
||||
"Death Metal", "Pranks", "Soundtrack", "Euro-Techno", "Ambient",
|
||||
"Trip-Hop", "Vocal", "Jazz+Funk", "Fusion", "Trance", "Classical",
|
||||
"Instrumental", "Acid", "House", "Game", "Sound Clip", "Gospel", "Noise",
|
||||
"AlternRock", "Bass", "Soul", "Punk", "Space", "Meditative",
|
||||
"Instrumental Pop", "Instrumental Rock", "Ethnic", "Gothic", "Darkwave",
|
||||
"Techno-Industrial", "Electronic", "Pop-Folk", "Eurodance", "Dream",
|
||||
"Southern Rock", "Comedy", "Cult", "Gangsta", "Top 40", "Christian Rap",
|
||||
"Pop/Funk", "Jungle", "Native American", "Cabaret", "New Wave",
|
||||
"Psychadelic", "Rave", "Showtunes", "Trailer", "Lo-Fi", "Tribal",
|
||||
"Acid Punk", "Acid Jazz", "Polka", "Retro", "Musical", "Rock & Roll",
|
||||
"Hard Rock", "Folk", "Folk-Rock", "National Folk", "Swing", "Fast Fusion",
|
||||
"Bebob", "Latin", "Revival", "Celtic", "Bluegrass", "Avantgarde",
|
||||
"Gothic Rock", "Progressive Rock", "Psychedelic Rock", "Symphonic Rock",
|
||||
"Slow Rock", "Big Band", "Chorus", "Easy Listening", "Acoustic", "Humour",
|
||||
"Speech", "Chanson", "Opera", "Chamber Music", "Sonata", "Symphony",
|
||||
"Booty Bass", "Primus", "Porn Groove", "Satire", "Slow Jam", "Club",
|
||||
"Tango", "Samba", "Folklore", "Ballad", "Power Ballad", "Rhythmic Soul",
|
||||
"Freestyle", "Duet", "Punk Rock", "Drum Solo", "A capella", "Euro-House",
|
||||
"Dance Hall", "Goa", "Drum & Bass", "Club-House", "Hardcore", "Terror",
|
||||
"Indie", "BritPop", "Afro-Punk", "Polsk Punk", "Beat",
|
||||
"Christian Gangsta Rap", "Heavy Metal", "Black Metal", "Crossover",
|
||||
"Contemporary Christian", "Christian Rock", "Merengue", "Salsa",
|
||||
"Thrash Metal", "Anime", "JPop", "Synthpop", "Abstract", "Art Rock",
|
||||
"Baroque", "Bhangra", "Big Beat", "Breakbeat", "Chillout", "Downtempo",
|
||||
"Dub", "EBM", "Eclectic", "Electro", "Electroclash", "Emo",
|
||||
"Experimental", "Garage", "Global", "IDM", "Illbient", "Industro-Goth",
|
||||
"Jam Band", "Krautrock", "Leftfield", "Lounge", "Math Rock",
|
||||
"New Romantic", "Nu-Breakz", "Post-Punk", "Post-Rock", "Psytrance",
|
||||
"Shoegaze", "Space Rock", "Trop Rock", "World Music", "Neoclassical",
|
||||
"Audiobook", "Audio Theatre", "Neue Deutsche Welle", "Podcast",
|
||||
"Indie Rock", "G-Funk", "Dubstep", "Garage Rock", "Psybient",
|
||||
}
|
||||
@@ -0,0 +1,421 @@
|
||||
package library
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"encoding/binary"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/dhowden/tag"
|
||||
)
|
||||
|
||||
// rawFrame is a frame with a byte-exact payload, so tests can express encoding
|
||||
// bytes and embedded nulls that a string-keyed helper can't.
|
||||
type rawFrame struct {
|
||||
id string
|
||||
payload []byte
|
||||
}
|
||||
|
||||
// buildID3v2 assembles a tag for the given major version. Frame size encoding
|
||||
// differs per version (2.4 is synchsafe, 2.2/2.3 are plain), which is exactly
|
||||
// the kind of detail a parser gets subtly wrong, so tests build all three.
|
||||
func buildID3v2(t *testing.T, major byte, frames ...rawFrame) []byte {
|
||||
t.Helper()
|
||||
var body bytes.Buffer
|
||||
for _, f := range frames {
|
||||
switch major {
|
||||
case 2:
|
||||
if len(f.id) != 3 {
|
||||
t.Fatalf("v2.2 frame id %q must be 3 bytes", f.id)
|
||||
}
|
||||
body.WriteString(f.id)
|
||||
n := len(f.payload)
|
||||
body.Write([]byte{byte(n >> 16), byte(n >> 8), byte(n)})
|
||||
case 3:
|
||||
body.WriteString(f.id)
|
||||
_ = binary.Write(&body, binary.BigEndian, uint32(len(f.payload)))
|
||||
body.Write([]byte{0x00, 0x00})
|
||||
case 4:
|
||||
body.WriteString(f.id)
|
||||
body.Write(synchsafeBytes(len(f.payload)))
|
||||
body.Write([]byte{0x00, 0x00})
|
||||
}
|
||||
body.Write(f.payload)
|
||||
}
|
||||
var out bytes.Buffer
|
||||
out.WriteString("ID3")
|
||||
out.Write([]byte{major, 0x00, 0x00})
|
||||
out.Write(synchsafeBytes(body.Len()))
|
||||
out.Write(body.Bytes())
|
||||
// A few bytes of MPEG sync so dhowden/tag accepts the file shape.
|
||||
out.Write([]byte{0xFF, 0xFB, 0x90, 0x00})
|
||||
return out.Bytes()
|
||||
}
|
||||
|
||||
func synchsafeBytes(n int) []byte {
|
||||
return []byte{
|
||||
byte((n >> 21) & 0x7F),
|
||||
byte((n >> 14) & 0x7F),
|
||||
byte((n >> 7) & 0x7F),
|
||||
byte(n & 0x7F),
|
||||
}
|
||||
}
|
||||
|
||||
// utf8Frame builds a text-frame payload: encoding byte 3 (UTF-8) followed by
|
||||
// values joined with the null separator ID3v2 uses for multiple values.
|
||||
func utf8Frame(values ...string) []byte {
|
||||
return append([]byte{0x03}, []byte(strings.Join(values, "\x00"))...)
|
||||
}
|
||||
|
||||
// TestReadID3v2GenreValues_MultiValue is the #2499 regression. dhowden/tag
|
||||
// rejoins these values with the empty string, producing "Alternative RockRock";
|
||||
// the whole point of our own reader is that they stay separate.
|
||||
func TestReadID3v2GenreValues_MultiValue(t *testing.T) {
|
||||
for _, major := range []byte{2, 3, 4} {
|
||||
id := "TCON"
|
||||
if major == 2 {
|
||||
id = "TCO"
|
||||
}
|
||||
data := buildID3v2(t, major, rawFrame{id, utf8Frame("Alternative Rock", "Rock")})
|
||||
got, err := readID3v2GenreValues(bytes.NewReader(data))
|
||||
if err != nil {
|
||||
t.Fatalf("v2.%d: %v", major, err)
|
||||
}
|
||||
want := []string{"Alternative Rock", "Rock"}
|
||||
if !equalStrings(got, want) {
|
||||
t.Errorf("v2.%d genres = %q, want %q", major, got, want)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// The operator's worst case: eight values welded into one 70-character token.
|
||||
func TestReadID3v2GenreValues_ManyValues(t *testing.T) {
|
||||
values := []string{
|
||||
"Boom Bap", "Downtempo", "Hip Hop", "Instrumental",
|
||||
"Lo-Fi", "Lo-Fi Hip Hop", "Chillwave", "Instrumental Hip Hop",
|
||||
}
|
||||
data := buildID3v2(t, 4, rawFrame{"TCON", utf8Frame(values...)})
|
||||
got, err := readID3v2GenreValues(bytes.NewReader(data))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !equalStrings(got, values) {
|
||||
t.Errorf("genres = %q, want %q", got, values)
|
||||
}
|
||||
}
|
||||
|
||||
// A trailing null terminator is legal and must not produce an empty value.
|
||||
func TestReadID3v2GenreValues_TrailingTerminator(t *testing.T) {
|
||||
data := buildID3v2(t, 4, rawFrame{"TCON", append(utf8Frame("Jazz"), 0x00)})
|
||||
got, err := readID3v2GenreValues(bytes.NewReader(data))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !equalStrings(got, []string{"Jazz"}) {
|
||||
t.Errorf("genres = %q, want [Jazz]", got)
|
||||
}
|
||||
}
|
||||
|
||||
// UTF-16 uses a TWO-byte separator. Splitting it on single nulls would cut
|
||||
// every ASCII character in half, so this guards the width handling.
|
||||
func TestReadID3v2GenreValues_UTF16(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
payload []byte
|
||||
}{
|
||||
{
|
||||
// Spec-correct: encoding 1 with a BOM on every value.
|
||||
name: "utf16le, BOM on each value",
|
||||
payload: concat([]byte{0x01},
|
||||
[]byte{0xFF, 0xFE}, utf16LE("Rock"),
|
||||
[]byte{0x00, 0x00},
|
||||
[]byte{0xFF, 0xFE}, utf16LE("Pop")),
|
||||
},
|
||||
{
|
||||
// Sloppy but common: BOM only on the first value. Without carrying
|
||||
// the byte order forward, "Pop" decodes byte-swapped to CJK.
|
||||
name: "utf16le, BOM only on the first value",
|
||||
payload: concat([]byte{0x01},
|
||||
[]byte{0xFF, 0xFE}, utf16LE("Rock"),
|
||||
[]byte{0x00, 0x00}, utf16LE("Pop")),
|
||||
},
|
||||
{
|
||||
// Encoding 2: big-endian, no BOM anywhere.
|
||||
name: "utf16be no BOM",
|
||||
payload: concat([]byte{0x02},
|
||||
utf16BE("Rock"), []byte{0x00, 0x00}, utf16BE("Pop")),
|
||||
},
|
||||
}
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
data := buildID3v2(t, 4, rawFrame{"TCON", tc.payload})
|
||||
got, err := readID3v2GenreValues(bytes.NewReader(data))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !equalStrings(got, []string{"Rock", "Pop"}) {
|
||||
t.Errorf("genres = %q, want [Rock Pop]", got)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// ISO-8859-1 must be widened, not reinterpreted as UTF-8 — "Bj\xf6rk" would
|
||||
// otherwise come back as invalid bytes.
|
||||
func TestReadID3v2GenreValues_Latin1(t *testing.T) {
|
||||
payload := append([]byte{0x00}, []byte("Chanson Fran\xe7aise")...)
|
||||
data := buildID3v2(t, 3, rawFrame{"TCON", payload})
|
||||
got, err := readID3v2GenreValues(bytes.NewReader(data))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !equalStrings(got, []string{"Chanson Française"}) {
|
||||
t.Errorf("genres = %q, want [Chanson Française]", got)
|
||||
}
|
||||
}
|
||||
|
||||
// Frames before TCON must be walked over correctly. If the size field were
|
||||
// decoded with the wrong scheme the walk lands mid-frame and TCON is missed.
|
||||
func TestReadID3v2GenreValues_SkipsPrecedingFrames(t *testing.T) {
|
||||
for _, major := range []byte{3, 4} {
|
||||
data := buildID3v2(t, major,
|
||||
rawFrame{"TIT2", utf8Frame("Some Title")},
|
||||
rawFrame{"TPE1", utf8Frame("Some Artist")},
|
||||
rawFrame{"TCON", utf8Frame("Shoegaze", "Dream Pop")},
|
||||
)
|
||||
got, err := readID3v2GenreValues(bytes.NewReader(data))
|
||||
if err != nil {
|
||||
t.Fatalf("v2.%d: %v", major, err)
|
||||
}
|
||||
if !equalStrings(got, []string{"Shoegaze", "Dream Pop"}) {
|
||||
t.Errorf("v2.%d genres = %q, want [Shoegaze Dream Pop]", major, got)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestReadID3v2GenreValues_NoGenreFrame(t *testing.T) {
|
||||
data := buildID3v2(t, 4, rawFrame{"TIT2", utf8Frame("Only A Title")})
|
||||
if _, err := readID3v2GenreValues(bytes.NewReader(data)); err == nil {
|
||||
t.Fatal("expected an error when no genre frame is present")
|
||||
}
|
||||
}
|
||||
|
||||
func TestReadID3v2GenreValues_NotAnID3File(t *testing.T) {
|
||||
if _, err := readID3v2GenreValues(bytes.NewReader([]byte("not a tag at all"))); err == nil {
|
||||
t.Fatal("expected an error for a file with no ID3v2 tag")
|
||||
}
|
||||
}
|
||||
|
||||
// Padding after the last frame is zero bytes; the walk must stop rather than
|
||||
// read a frame id of "\x00\x00\x00\x00".
|
||||
func TestReadID3v2GenreValues_StopsAtPadding(t *testing.T) {
|
||||
tagged := buildID3v2(t, 4, rawFrame{"TCON", utf8Frame("Rock")})
|
||||
// Splice 32 padding bytes in before the MPEG sync trailer, growing the
|
||||
// declared tag size to match.
|
||||
body := tagged[10 : len(tagged)-4]
|
||||
padded := append(append([]byte{}, body...), make([]byte, 32)...)
|
||||
var out bytes.Buffer
|
||||
out.WriteString("ID3")
|
||||
out.Write([]byte{4, 0x00, 0x00})
|
||||
out.Write(synchsafeBytes(len(padded)))
|
||||
out.Write(padded)
|
||||
got, err := readID3v2GenreValues(bytes.NewReader(out.Bytes()))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !equalStrings(got, []string{"Rock"}) {
|
||||
t.Errorf("genres = %q, want [Rock]", got)
|
||||
}
|
||||
}
|
||||
|
||||
// Unsynchronisation inserts 0xFF 0x00 pairs that must be collapsed before the
|
||||
// frame list is walked, or every offset past the first pair is wrong.
|
||||
func TestReadID3v2GenreValues_TagUnsynchronisation(t *testing.T) {
|
||||
// Latin-1 so a genre can legitimately contain the byte 0xFF ("ÿ"). Once
|
||||
// unsynchronised that becomes 0xFF 0x00 — which is indistinguishable from a
|
||||
// value separator until the collapse runs, so this fails loudly if
|
||||
// undoUnsynchronisation is skipped.
|
||||
payload := concat([]byte{0x00}, []byte("Ro\xffck"), []byte{0x00}, []byte("Pop"))
|
||||
inner := buildID3v2(t, 3, rawFrame{"TCON", payload})
|
||||
body := inner[10 : len(inner)-4]
|
||||
encoded := bytes.ReplaceAll(body, []byte{0xFF}, []byte{0xFF, 0x00})
|
||||
if bytes.Equal(encoded, body) {
|
||||
t.Fatal("test is vacuous: nothing was unsynchronised")
|
||||
}
|
||||
var out bytes.Buffer
|
||||
out.WriteString("ID3")
|
||||
out.Write([]byte{3, 0x00, 0x80}) // 0x80 = unsynchronisation
|
||||
out.Write(synchsafeBytes(len(encoded)))
|
||||
out.Write(encoded)
|
||||
|
||||
got, err := readID3v2GenreValues(bytes.NewReader(out.Bytes()))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !equalStrings(got, []string{"Roÿck", "Pop"}) {
|
||||
t.Errorf("genres = %q, want [Roÿck Pop]", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestNormaliseGenreValue(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
in string
|
||||
want []string
|
||||
}{
|
||||
{"plain text", "Alternative Rock", []string{"Alternative Rock"}},
|
||||
{"trims whitespace", " Jazz ", []string{"Jazz"}},
|
||||
{"empty", "", nil},
|
||||
{"whitespace only", " ", nil},
|
||||
|
||||
// The operator's digit soup, one value at a time.
|
||||
{"bare numeric", "17", []string{"Rock"}},
|
||||
{"bare numeric pop", "13", []string{"Pop"}},
|
||||
{"bare numeric electronic", "52", []string{"Electronic"}},
|
||||
{"winamp extension range", "187", []string{"Indie Rock"}},
|
||||
{"numeric out of range", "9999", nil},
|
||||
{"negative", "-1", nil},
|
||||
|
||||
// ID3v2.3 refinement syntax.
|
||||
{"parenthesised", "(17)", []string{"Rock"}},
|
||||
{"parenthesised repeated", "(51)(39)", []string{"Techno-Industrial", "Noise"}},
|
||||
{"parenthesised with refinement", "(17)Hard Rock", []string{"Rock", "Hard Rock"}},
|
||||
{"remix", "(RX)", []string{"Remix"}},
|
||||
{"cover", "(CR)", []string{"Cover"}},
|
||||
{"escaped open paren", "((Weird", []string{"(Weird"}},
|
||||
{"parenthesised non-numeric", "(Live)", []string{"(Live)"}},
|
||||
|
||||
// A label that merely starts with digits is text, not a reference.
|
||||
{"digits in a name", "1980s", []string{"1980s"}},
|
||||
{"hyphenated", "Lo-Fi Hip Hop", []string{"Lo-Fi Hip Hop"}},
|
||||
}
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
got := normaliseGenreValue(tc.in)
|
||||
if !equalStrings(got, tc.want) {
|
||||
t.Errorf("normaliseGenreValue(%q) = %q, want %q", tc.in, got, tc.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestNormaliseGenres_DedupesCaseInsensitively(t *testing.T) {
|
||||
got := normaliseGenres([]string{"Rock", "rock", "ROCK", "Pop"})
|
||||
// First spelling wins — we are not imposing a canonical case here, only
|
||||
// removing values that repeat within a single file.
|
||||
if !equalStrings(got, []string{"Rock", "Pop"}) {
|
||||
t.Errorf("genres = %q, want [Rock Pop]", got)
|
||||
}
|
||||
}
|
||||
|
||||
// "(40)AlternRock" declares the same genre twice — numerically and in text.
|
||||
func TestNormaliseGenres_DedupesResolvedNumeric(t *testing.T) {
|
||||
got := normaliseGenres([]string{"(40)AlternRock"})
|
||||
if !equalStrings(got, []string{"AlternRock"}) {
|
||||
t.Errorf("genres = %q, want [AlternRock]", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestNormaliseGenres_AllJunkYieldsNil(t *testing.T) {
|
||||
if got := normaliseGenres([]string{"", " ", "9999"}); got != nil {
|
||||
t.Errorf("genres = %q, want nil", got)
|
||||
}
|
||||
}
|
||||
|
||||
// End-to-end through dhowden/tag, which is what the scanner actually calls.
|
||||
// Proves the welded value never reaches the caller.
|
||||
func TestExtractGenres_EndToEnd(t *testing.T) {
|
||||
data := buildID3v2(t, 4,
|
||||
rawFrame{"TIT2", utf8Frame("A Song")},
|
||||
rawFrame{"TCON", utf8Frame("Alternative Rock", "Rock")},
|
||||
)
|
||||
rs := bytes.NewReader(data)
|
||||
meta, err := tag.ReadFrom(rs)
|
||||
if err != nil {
|
||||
t.Fatalf("tag.ReadFrom: %v", err)
|
||||
}
|
||||
// Confirm the upstream behaviour this fix exists for is still present —
|
||||
// if dhowden ever fixes it, this test tells us the workaround can go.
|
||||
if welded := meta.Genre(); welded != "Alternative RockRock" {
|
||||
t.Logf("note: dhowden/tag no longer welds multi-values (got %q)", welded)
|
||||
}
|
||||
|
||||
genres, fellBack := extractGenres(meta, rs)
|
||||
if fellBack {
|
||||
t.Error("fellBack = true, want false — the TCON frame is parseable")
|
||||
}
|
||||
if !equalStrings(genres, []string{"Alternative Rock", "Rock"}) {
|
||||
t.Errorf("genres = %q, want [Alternative Rock Rock]", genres)
|
||||
}
|
||||
if joined := strings.Join(genres, genreDelimiter); joined != "Alternative Rock;Rock" {
|
||||
t.Errorf("stored value = %q, want %q", joined, "Alternative Rock;Rock")
|
||||
}
|
||||
}
|
||||
|
||||
// The digit-soup case, end to end: numeric references resolve to names.
|
||||
func TestExtractGenres_ResolvesNumericReferences(t *testing.T) {
|
||||
data := buildID3v2(t, 4, rawFrame{"TCON", utf8Frame("40", "17")})
|
||||
rs := bytes.NewReader(data)
|
||||
meta, err := tag.ReadFrom(rs)
|
||||
if err != nil {
|
||||
t.Fatalf("tag.ReadFrom: %v", err)
|
||||
}
|
||||
genres, _ := extractGenres(meta, rs)
|
||||
if !equalStrings(genres, []string{"AlternRock", "Rock"}) {
|
||||
t.Errorf("genres = %q, want [AlternRock Rock]", genres)
|
||||
}
|
||||
}
|
||||
|
||||
// A file with no genre at all must yield nothing and must NOT be reported as a
|
||||
// fallback — that would log a warning for every untagged file in the library.
|
||||
func TestExtractGenres_NoGenreIsNotAFallback(t *testing.T) {
|
||||
data := buildID3v2(t, 4, rawFrame{"TIT2", utf8Frame("A Song")})
|
||||
rs := bytes.NewReader(data)
|
||||
meta, err := tag.ReadFrom(rs)
|
||||
if err != nil {
|
||||
t.Fatalf("tag.ReadFrom: %v", err)
|
||||
}
|
||||
genres, fellBack := extractGenres(meta, rs)
|
||||
if len(genres) != 0 {
|
||||
t.Errorf("genres = %q, want none", genres)
|
||||
}
|
||||
if fellBack {
|
||||
t.Error("fellBack = true for an untagged file; would log on every such file")
|
||||
}
|
||||
}
|
||||
|
||||
func concat(parts ...[]byte) []byte {
|
||||
var out []byte
|
||||
for _, p := range parts {
|
||||
out = append(out, p...)
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
func utf16LE(s string) []byte {
|
||||
out := make([]byte, 0, len(s)*2)
|
||||
for _, r := range s {
|
||||
out = append(out, byte(r), byte(r>>8))
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
func utf16BE(s string) []byte {
|
||||
out := make([]byte, 0, len(s)*2)
|
||||
for _, r := range s {
|
||||
out = append(out, byte(r>>8), byte(r))
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
func equalStrings(a, b []string) bool {
|
||||
if len(a) != len(b) {
|
||||
return false
|
||||
}
|
||||
for i := range a {
|
||||
if a[i] != b[i] {
|
||||
return false
|
||||
}
|
||||
}
|
||||
return true
|
||||
}
|
||||
@@ -0,0 +1,388 @@
|
||||
package library
|
||||
|
||||
import (
|
||||
"encoding/binary"
|
||||
"errors"
|
||||
"io"
|
||||
"strings"
|
||||
"unicode/utf16"
|
||||
)
|
||||
|
||||
// Why this file exists at all: github.com/dhowden/tag reads every other field
|
||||
// we need correctly, but its text-frame reader destroys multi-value frames.
|
||||
// readTFrame does
|
||||
//
|
||||
// strings.Join(strings.Split(txt, string(singleZero)), "")
|
||||
//
|
||||
// — it splits on the ID3v2 null separator and rejoins with the EMPTY string, so
|
||||
// a file tagged "Alternative Rock" + "Rock" comes back as the single token
|
||||
// "Alternative RockRock" (#2499). We stored that verbatim, which corrupted the
|
||||
// genre browse axis and polluted the taste profile's tag vocabulary.
|
||||
//
|
||||
// ffprobe is not an escape hatch either: ffmpeg's read_ttag calls decode_str
|
||||
// exactly once with no loop, so it keeps only the FIRST value and silently
|
||||
// discards the rest. Truncating multi-genre tags would blunt genre similarity,
|
||||
// which is the main thing genre feeds.
|
||||
//
|
||||
// So the TCON frame is parsed here directly. Only the genre frame — everything
|
||||
// else still comes from dhowden/tag, which handles it fine.
|
||||
|
||||
// maxID3TagSize caps how much of a file we'll buffer looking for TCON. Real
|
||||
// tags are kilobytes; embedded cover art pushes them to a few megabytes. The
|
||||
// cap exists so a corrupt or hostile size field can't make the scanner
|
||||
// allocate wildly on a file it was only asked to index.
|
||||
const maxID3TagSize = 16 << 20
|
||||
|
||||
// errNoGenreFrame means the file carries no readable genre frame. It is an
|
||||
// expected outcome (plenty of files are untagged), not a failure.
|
||||
var errNoGenreFrame = errors.New("library: no ID3v2 genre frame")
|
||||
|
||||
// readID3v2GenreValues returns the raw, still-unnormalised values of the ID3v2
|
||||
// genre frame — one entry per value the tag actually declares. Numeric ID3v1
|
||||
// references are left alone here; normaliseGenreValue resolves them.
|
||||
//
|
||||
// rs is seeked to the start, so it is safe to call after dhowden/tag has
|
||||
// already consumed the reader.
|
||||
func readID3v2GenreValues(rs io.ReadSeeker) ([]string, error) {
|
||||
if _, err := rs.Seek(0, io.SeekStart); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
var hdr [10]byte
|
||||
if _, err := io.ReadFull(rs, hdr[:]); err != nil {
|
||||
return nil, errNoGenreFrame
|
||||
}
|
||||
if string(hdr[0:3]) != "ID3" {
|
||||
return nil, errNoGenreFrame
|
||||
}
|
||||
major := hdr[3]
|
||||
// 2.2, 2.3 and 2.4 are the versions in the wild. A future 2.5 would very
|
||||
// likely move the frame layout, so refuse rather than misparse it.
|
||||
if major < 2 || major > 4 {
|
||||
return nil, errNoGenreFrame
|
||||
}
|
||||
tagFlags := hdr[5]
|
||||
size := syncsafeInt(hdr[6:10])
|
||||
if size <= 0 || size > maxID3TagSize {
|
||||
return nil, errNoGenreFrame
|
||||
}
|
||||
|
||||
body := make([]byte, size)
|
||||
if _, err := io.ReadFull(rs, body); err != nil {
|
||||
// A truncated tag is still worth parsing as far as it goes — frame
|
||||
// walking stops cleanly at the end of what we managed to read.
|
||||
return nil, errNoGenreFrame
|
||||
}
|
||||
|
||||
// 2.2 used flag 0x40 for whole-tag compression with a scheme that was
|
||||
// never actually specified. Nothing can read those.
|
||||
if major == 2 && tagFlags&0x40 != 0 {
|
||||
return nil, errNoGenreFrame
|
||||
}
|
||||
if tagFlags&0x80 != 0 {
|
||||
// Whole-tag unsynchronisation (2.2/2.3). 2.4 moved this per-frame, but
|
||||
// some writers still set it at tag level, and undoing it twice is
|
||||
// harmless: after the first pass no 0xFF 0x00 pairs remain.
|
||||
body = undoUnsynchronisation(body)
|
||||
}
|
||||
if major >= 3 && tagFlags&0x40 != 0 {
|
||||
var ok bool
|
||||
if body, ok = skipExtendedHeader(body, major); !ok {
|
||||
return nil, errNoGenreFrame
|
||||
}
|
||||
}
|
||||
|
||||
return findGenreFrame(body, major)
|
||||
}
|
||||
|
||||
// findGenreFrame walks the frame list and decodes the genre frame's values.
|
||||
func findGenreFrame(body []byte, major byte) ([]string, error) {
|
||||
// 2.2 frames: 3-byte id + 3-byte size, no flags. 2.3/2.4: 4-byte id +
|
||||
// 4-byte size + 2-byte flags. The size field is the other difference that
|
||||
// matters — see frameSize.
|
||||
idLen, sizeLen, flagLen := 4, 4, 2
|
||||
wantID := "TCON"
|
||||
if major == 2 {
|
||||
idLen, sizeLen, flagLen = 3, 3, 0
|
||||
wantID = "TCO"
|
||||
}
|
||||
hdrLen := idLen + sizeLen + flagLen
|
||||
|
||||
for off := 0; off+hdrLen <= len(body); {
|
||||
id := string(body[off : off+idLen])
|
||||
// A zero byte where a frame id belongs means we've reached the padding
|
||||
// that fills out the tag. Everything after it is zeros.
|
||||
if body[off] == 0 {
|
||||
break
|
||||
}
|
||||
size := frameSize(body[off+idLen:off+idLen+sizeLen], major)
|
||||
if size <= 0 || off+hdrLen+size > len(body) {
|
||||
// Bogus length — we can't trust any offset past this point.
|
||||
break
|
||||
}
|
||||
if id == wantID {
|
||||
var flags uint16
|
||||
if flagLen == 2 {
|
||||
flags = binary.BigEndian.Uint16(body[off+idLen+sizeLen : off+hdrLen])
|
||||
}
|
||||
data, ok := frameData(body[off+hdrLen:off+hdrLen+size], major, flags)
|
||||
if !ok {
|
||||
return nil, errNoGenreFrame
|
||||
}
|
||||
return decodeTextValues(data), nil
|
||||
}
|
||||
off += hdrLen + size
|
||||
}
|
||||
return nil, errNoGenreFrame
|
||||
}
|
||||
|
||||
// frameSize decodes a frame's length field. 2.4 made it syncsafe (7 bits per
|
||||
// byte); 2.2 and 2.3 are plain big-endian. Reading a 2.3 size as syncsafe (or
|
||||
// the reverse) yields a plausible-looking wrong offset rather than an obvious
|
||||
// error, which is exactly how frame-walking bugs go unnoticed.
|
||||
func frameSize(b []byte, major byte) int {
|
||||
switch major {
|
||||
case 2:
|
||||
return int(b[0])<<16 | int(b[1])<<8 | int(b[2])
|
||||
case 3:
|
||||
n := binary.BigEndian.Uint32(b)
|
||||
if n > maxID3TagSize {
|
||||
return -1
|
||||
}
|
||||
return int(n)
|
||||
default:
|
||||
return syncsafeInt(b)
|
||||
}
|
||||
}
|
||||
|
||||
// frameData strips per-frame wrappers and reports whether the payload is
|
||||
// readable at all. Compressed and encrypted frames are not (we have no
|
||||
// zlib-in-frame or key handling, and neither is meaningful for a genre tag).
|
||||
func frameData(data []byte, major byte, flags uint16) ([]byte, bool) {
|
||||
if major == 3 {
|
||||
// 2.3 flags: %abc00000 %ijk00000 — i compression, j encryption,
|
||||
// k grouping.
|
||||
if flags&0x0080 != 0 || flags&0x0040 != 0 {
|
||||
return nil, false
|
||||
}
|
||||
if flags&0x0020 != 0 {
|
||||
if len(data) < 1 {
|
||||
return nil, false
|
||||
}
|
||||
data = data[1:] // group identifier
|
||||
}
|
||||
return data, true
|
||||
}
|
||||
if major == 4 {
|
||||
// 2.4 flags: %0abc0000 %0h00kmnp — h grouping, k compression,
|
||||
// m encryption, n unsynchronisation, p data-length indicator.
|
||||
if flags&0x0008 != 0 || flags&0x0004 != 0 {
|
||||
return nil, false
|
||||
}
|
||||
if flags&0x0040 != 0 {
|
||||
if len(data) < 1 {
|
||||
return nil, false
|
||||
}
|
||||
data = data[1:]
|
||||
}
|
||||
if flags&0x0001 != 0 {
|
||||
if len(data) < 4 {
|
||||
return nil, false
|
||||
}
|
||||
data = data[4:] // syncsafe expanded size; we don't need it
|
||||
}
|
||||
if flags&0x0002 != 0 {
|
||||
data = undoUnsynchronisation(data)
|
||||
}
|
||||
return data, true
|
||||
}
|
||||
return data, true // 2.2 has no frame flags
|
||||
}
|
||||
|
||||
// decodeTextValues splits a text frame's payload into its individual values and
|
||||
// decodes each according to the frame's encoding byte.
|
||||
//
|
||||
// This is the whole point of the file: ID3v2 separates multiple values in one
|
||||
// text frame with a null, and that separator is two bytes wide for the UTF-16
|
||||
// encodings. Splitting a UTF-16 payload on single nulls would cut every ASCII
|
||||
// character in half.
|
||||
func decodeTextValues(data []byte) []string {
|
||||
if len(data) == 0 {
|
||||
return nil
|
||||
}
|
||||
encoding := data[0]
|
||||
payload := data[1:]
|
||||
|
||||
switch encoding {
|
||||
case 0: // ISO-8859-1
|
||||
return mapChunks(splitOnNul(payload, 1), decodeLatin1)
|
||||
case 3: // UTF-8
|
||||
return mapChunks(splitOnNul(payload, 1), func(b []byte) string { return string(b) })
|
||||
case 1, 2: // UTF-16 with BOM / UTF-16BE without
|
||||
chunks := splitOnNul(payload, 2)
|
||||
// Encoding 2 is big-endian by definition. Encoding 1 carries a byte
|
||||
// order mark, which the spec says must appear on EVERY value in a
|
||||
// multi-value frame — but writers that emit one only on the first value
|
||||
// are common. Take the first BOM found as the default for values that
|
||||
// lack their own, otherwise everything after the first value decodes
|
||||
// byte-swapped into CJK gibberish.
|
||||
defaultBE := true
|
||||
if encoding == 1 {
|
||||
for _, c := range chunks {
|
||||
if be, ok := bomOrder(c); ok {
|
||||
defaultBE = be
|
||||
break
|
||||
}
|
||||
}
|
||||
}
|
||||
out := make([]string, 0, len(chunks))
|
||||
for _, c := range chunks {
|
||||
be := defaultBE
|
||||
if encoding == 1 {
|
||||
if o, ok := bomOrder(c); ok {
|
||||
be, c = o, c[2:]
|
||||
}
|
||||
}
|
||||
if s := strings.TrimSpace(decodeUTF16(c, be)); s != "" {
|
||||
out = append(out, s)
|
||||
}
|
||||
}
|
||||
return out
|
||||
default:
|
||||
// Unknown encoding byte. Treating it as Latin-1 recovers ASCII text,
|
||||
// which is better than dropping the frame.
|
||||
return mapChunks(splitOnNul(payload, 1), decodeLatin1)
|
||||
}
|
||||
}
|
||||
|
||||
// splitOnNul splits on a null of the given width, honouring alignment so a
|
||||
// 2-byte-wide separator can't match across a character boundary.
|
||||
func splitOnNul(b []byte, width int) [][]byte {
|
||||
var out [][]byte
|
||||
start := 0
|
||||
for i := 0; i+width <= len(b); i += width {
|
||||
if !isNul(b[i : i+width]) {
|
||||
continue
|
||||
}
|
||||
out = append(out, b[start:i])
|
||||
start = i + width
|
||||
}
|
||||
if start < len(b) {
|
||||
out = append(out, b[start:])
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
func isNul(b []byte) bool {
|
||||
for _, c := range b {
|
||||
if c != 0 {
|
||||
return false
|
||||
}
|
||||
}
|
||||
return true
|
||||
}
|
||||
|
||||
func mapChunks(chunks [][]byte, decode func([]byte) string) []string {
|
||||
out := make([]string, 0, len(chunks))
|
||||
for _, c := range chunks {
|
||||
if s := strings.TrimSpace(decode(c)); s != "" {
|
||||
out = append(out, s)
|
||||
}
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// decodeLatin1 widens ISO-8859-1 bytes to runes. A plain string() conversion
|
||||
// would treat the bytes as UTF-8 and mangle every accented character.
|
||||
func decodeLatin1(b []byte) string {
|
||||
runes := make([]rune, len(b))
|
||||
for i, c := range b {
|
||||
runes[i] = rune(c)
|
||||
}
|
||||
return string(runes)
|
||||
}
|
||||
|
||||
// bomOrder reports the byte order a UTF-16 byte-order mark declares, and
|
||||
// whether one is present at all.
|
||||
func bomOrder(b []byte) (bigEndian, ok bool) {
|
||||
if len(b) < 2 {
|
||||
return false, false
|
||||
}
|
||||
switch {
|
||||
case b[0] == 0xFE && b[1] == 0xFF:
|
||||
return true, true
|
||||
case b[0] == 0xFF && b[1] == 0xFE:
|
||||
return false, true
|
||||
}
|
||||
return false, false
|
||||
}
|
||||
|
||||
// decodeUTF16 decodes UTF-16 code units in the given byte order. Any BOM has
|
||||
// already been consumed by the caller.
|
||||
func decodeUTF16(b []byte, bigEndian bool) string {
|
||||
if len(b) < 2 {
|
||||
return ""
|
||||
}
|
||||
units := make([]uint16, 0, len(b)/2)
|
||||
for i := 0; i+1 < len(b); i += 2 {
|
||||
if bigEndian {
|
||||
units = append(units, uint16(b[i])<<8|uint16(b[i+1]))
|
||||
} else {
|
||||
units = append(units, uint16(b[i+1])<<8|uint16(b[i]))
|
||||
}
|
||||
}
|
||||
return string(utf16.Decode(units))
|
||||
}
|
||||
|
||||
// skipExtendedHeader advances past the optional extended header. The two
|
||||
// versions disagree about whether the size field counts itself, which is worth
|
||||
// spelling out because getting it wrong offsets the entire frame list by four
|
||||
// bytes and makes every frame id look like padding.
|
||||
func skipExtendedHeader(body []byte, major byte) ([]byte, bool) {
|
||||
if len(body) < 4 {
|
||||
return nil, false
|
||||
}
|
||||
if major == 3 {
|
||||
// 2.3: size EXCLUDES the four size bytes themselves.
|
||||
size := int(binary.BigEndian.Uint32(body[0:4]))
|
||||
if size < 0 || 4+size > len(body) {
|
||||
return nil, false
|
||||
}
|
||||
return body[4+size:], true
|
||||
}
|
||||
// 2.4: syncsafe size INCLUDING the size bytes.
|
||||
size := syncsafeInt(body[0:4])
|
||||
if size < 4 || size > len(body) {
|
||||
return nil, false
|
||||
}
|
||||
return body[size:], true
|
||||
}
|
||||
|
||||
// syncsafeInt decodes a 4-byte synchsafe integer (7 significant bits per byte).
|
||||
func syncsafeInt(b []byte) int {
|
||||
if len(b) < 4 {
|
||||
return -1
|
||||
}
|
||||
// A set high bit means this isn't a valid synchsafe integer. Some writers
|
||||
// emit a plain big-endian size here; refusing is safer than silently
|
||||
// dropping bits and walking to a wrong offset.
|
||||
for _, c := range b[:4] {
|
||||
if c&0x80 != 0 {
|
||||
return -1
|
||||
}
|
||||
}
|
||||
return int(b[0])<<21 | int(b[1])<<14 | int(b[2])<<7 | int(b[3])
|
||||
}
|
||||
|
||||
// undoUnsynchronisation collapses the 0xFF 0x00 pairs that unsynchronisation
|
||||
// inserts to stop a tag from looking like an MPEG frame sync.
|
||||
func undoUnsynchronisation(b []byte) []byte {
|
||||
out := make([]byte, 0, len(b))
|
||||
for i := 0; i < len(b); i++ {
|
||||
out = append(out, b[i])
|
||||
if b[i] == 0xFF && i+1 < len(b) && b[i+1] == 0x00 {
|
||||
i++
|
||||
}
|
||||
}
|
||||
return out
|
||||
}
|
||||
@@ -0,0 +1,151 @@
|
||||
package library
|
||||
|
||||
import (
|
||||
"context"
|
||||
|
||||
"github.com/jackc/pgx/v5/pgtype"
|
||||
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
|
||||
syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync"
|
||||
)
|
||||
|
||||
// Move detection (#2528).
|
||||
//
|
||||
// Track identity is file_path: UpsertTrack conflicts on it, and the reconcile
|
||||
// pass in reconcile.go clears a missing mark when the walk sees that same path
|
||||
// again. So a file that comes back exactly where it was restores cleanly, but a
|
||||
// file that comes back RENAMED or in a different directory looked, to the
|
||||
// scanner, like a deletion plus an unrelated new track:
|
||||
//
|
||||
// - the old row stayed marked missing, holding the like and every play_event
|
||||
// - a fresh row appeared with no history
|
||||
// - nothing connected them
|
||||
//
|
||||
// A liked song read as unliked after a retag, its play count reset to zero, and
|
||||
// Rediscover could offer it as a discovery. All silently. Renumbering an album
|
||||
// was enough to do it — which is exactly what happened on the operator's copy of
|
||||
// Minutes to Midnight.
|
||||
//
|
||||
// The fix adopts the existing row rather than inserting: re-point its file_path
|
||||
// at the new location and clear the mark. The caller's normal UpsertTrack then
|
||||
// conflicts on file_path and updates THAT row, so the track id survives and
|
||||
// likes, plays and playlist memberships travel with it. Clients see an update
|
||||
// rather than a delete-and-create, so no cache churn either.
|
||||
//
|
||||
// Only rows already marked missing are eligible. A row whose file is present
|
||||
// elsewhere is a duplicate, not a move, and re-pointing it would corrupt the
|
||||
// copy that still exists. That constraint is what makes this safe, and the
|
||||
// marking added in #2523 is what makes it expressible.
|
||||
|
||||
// trackAdopter is the slice of dbq.Queries move detection needs, narrowed so the
|
||||
// match/ambiguity logic can be tested against a fake.
|
||||
type trackAdopter interface {
|
||||
FindMissingTrackByMbid(ctx context.Context, mbid string) ([]dbq.FindMissingTrackByMbidRow, error)
|
||||
FindMissingTrackByFingerprint(ctx context.Context, arg dbq.FindMissingTrackByFingerprintParams) ([]dbq.FindMissingTrackByFingerprintRow, error)
|
||||
AdoptTrackPath(ctx context.Context, arg dbq.AdoptTrackPathParams) (int64, error)
|
||||
}
|
||||
|
||||
// adoptMovedTrack looks for a missing row that is the same recording as the file
|
||||
// at newPath and re-points it there. Reports whether a row was adopted.
|
||||
//
|
||||
// Never returns an error: failing to detect a move is a missed optimisation, not
|
||||
// a broken scan. The caller carries on and inserts a fresh row, which is the
|
||||
// pre-#2528 behaviour.
|
||||
func (s *Scanner) adoptMovedTrack(
|
||||
ctx context.Context, q trackAdopter, newPath string,
|
||||
fileSize int64, durationMs int32, recordingMBID string,
|
||||
) bool {
|
||||
// MBID first. It identifies the recording rather than the bytes, so it
|
||||
// survives a re-encode that the fingerprint cannot.
|
||||
if recordingMBID != "" {
|
||||
rows, err := q.FindMissingTrackByMbid(ctx, recordingMBID)
|
||||
if err != nil {
|
||||
s.logger.Warn("library scan: move lookup by mbid failed",
|
||||
"path", newPath, "err", err)
|
||||
} else if c, ok := s.uniqueMatch(rowsFromMbid(rows), newPath, "mbid"); ok {
|
||||
return s.adopt(ctx, q, c, newPath, "mbid")
|
||||
}
|
||||
}
|
||||
|
||||
// Fingerprint fallback for untagged files. Both components must be real:
|
||||
// duration_ms is 0 when ffprobe failed, and matching 0 against 0 would pair
|
||||
// up unrelated broken files.
|
||||
if fileSize > 0 && durationMs > 0 {
|
||||
rows, err := q.FindMissingTrackByFingerprint(ctx, dbq.FindMissingTrackByFingerprintParams{
|
||||
FileSize: fileSize,
|
||||
DurationMs: durationMs,
|
||||
})
|
||||
if err != nil {
|
||||
s.logger.Warn("library scan: move lookup by fingerprint failed",
|
||||
"path", newPath, "err", err)
|
||||
} else if c, ok := s.uniqueMatch(rowsFromFingerprint(rows), newPath, "fingerprint"); ok {
|
||||
return s.adopt(ctx, q, c, newPath, "fingerprint")
|
||||
}
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
// candidate is the shared shape of both lookups, so uniqueMatch is written once.
|
||||
type candidate struct {
|
||||
id pgtype.UUID
|
||||
filePath string
|
||||
}
|
||||
|
||||
func rowsFromMbid(rows []dbq.FindMissingTrackByMbidRow) []candidate {
|
||||
out := make([]candidate, 0, len(rows))
|
||||
for _, r := range rows {
|
||||
out = append(out, candidate{id: r.ID, filePath: r.FilePath})
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
func rowsFromFingerprint(rows []dbq.FindMissingTrackByFingerprintRow) []candidate {
|
||||
out := make([]candidate, 0, len(rows))
|
||||
for _, r := range rows {
|
||||
out = append(out, candidate{id: r.ID, filePath: r.FilePath})
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// uniqueMatch requires exactly one candidate. Adopting an arbitrary row out of
|
||||
// several would attach this file's future history to a coin flip, which is worse
|
||||
// than starting a fresh row — a fork is recoverable later, a wrong merge isn't.
|
||||
// Libraries with genuine duplicates hit this, so it's logged rather than silent.
|
||||
func (s *Scanner) uniqueMatch(
|
||||
cands []candidate, newPath, via string,
|
||||
) (candidate, bool) {
|
||||
switch len(cands) {
|
||||
case 0:
|
||||
return candidate{}, false
|
||||
case 1:
|
||||
return cands[0], true
|
||||
default:
|
||||
s.logger.Info("library scan: ambiguous move match, inserting a new track instead",
|
||||
"path", newPath, "via", via, "candidates", len(cands))
|
||||
return candidate{}, false
|
||||
}
|
||||
}
|
||||
|
||||
func (s *Scanner) adopt(
|
||||
ctx context.Context, q trackAdopter, c candidate, newPath, via string,
|
||||
) bool {
|
||||
n, err := q.AdoptTrackPath(ctx, dbq.AdoptTrackPathParams{ID: c.id, FilePath: newPath})
|
||||
if err != nil {
|
||||
// A unique violation on file_path means something else claimed this path
|
||||
// first. Fall through to a normal insert rather than failing the file.
|
||||
s.logger.Warn("library scan: adopting moved track failed",
|
||||
"path", newPath, "via", via, "err", err)
|
||||
return false
|
||||
}
|
||||
if n == 0 {
|
||||
// Lost the race: another file adopted this row between lookup and
|
||||
// update, so its mark was already cleared.
|
||||
return false
|
||||
}
|
||||
// Logged with both paths: this is the operator's only window onto a
|
||||
// reorganisation being understood as a move rather than a new track.
|
||||
s.logger.Info("library scan: track moved, history preserved",
|
||||
"from", c.filePath, "to", newPath, "via", via, "track_id", syncpkg.FormatUUID(c.id))
|
||||
return true
|
||||
}
|
||||
@@ -0,0 +1,295 @@
|
||||
package library
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"testing"
|
||||
|
||||
"github.com/jackc/pgx/v5/pgtype"
|
||||
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
|
||||
)
|
||||
|
||||
type fakeAdopter struct {
|
||||
byMbid []dbq.FindMissingTrackByMbidRow
|
||||
byFingerprint []dbq.FindMissingTrackByFingerprintRow
|
||||
|
||||
mbidErr error
|
||||
fingerprintErr error
|
||||
adoptErr error
|
||||
adoptRows int64
|
||||
|
||||
mbidQueried []string
|
||||
fingerprintQueried []dbq.FindMissingTrackByFingerprintParams
|
||||
adopted []dbq.AdoptTrackPathParams
|
||||
}
|
||||
|
||||
func (f *fakeAdopter) FindMissingTrackByMbid(_ context.Context, mbid string) ([]dbq.FindMissingTrackByMbidRow, error) {
|
||||
f.mbidQueried = append(f.mbidQueried, mbid)
|
||||
return f.byMbid, f.mbidErr
|
||||
}
|
||||
|
||||
func (f *fakeAdopter) FindMissingTrackByFingerprint(
|
||||
_ context.Context, arg dbq.FindMissingTrackByFingerprintParams,
|
||||
) ([]dbq.FindMissingTrackByFingerprintRow, error) {
|
||||
f.fingerprintQueried = append(f.fingerprintQueried, arg)
|
||||
return f.byFingerprint, f.fingerprintErr
|
||||
}
|
||||
|
||||
func (f *fakeAdopter) AdoptTrackPath(_ context.Context, arg dbq.AdoptTrackPathParams) (int64, error) {
|
||||
f.adopted = append(f.adopted, arg)
|
||||
if f.adoptErr != nil {
|
||||
return 0, f.adoptErr
|
||||
}
|
||||
return f.adoptRows, nil
|
||||
}
|
||||
|
||||
// The narrowed interface must not drift from the real queries.
|
||||
var _ trackAdopter = (*dbq.Queries)(nil)
|
||||
|
||||
func mbidRow(n byte, path string) dbq.FindMissingTrackByMbidRow {
|
||||
return dbq.FindMissingTrackByMbidRow{ID: testUUID(n), FilePath: path}
|
||||
}
|
||||
|
||||
func fpRow(n byte, path string) dbq.FindMissingTrackByFingerprintRow {
|
||||
return dbq.FindMissingTrackByFingerprintRow{ID: testUUID(n), FilePath: path}
|
||||
}
|
||||
|
||||
const (
|
||||
oldPath = "/music/Linkin Park/Minutes to Midnight/02 - Bleed It Out.mp3"
|
||||
newPath = "/music/Linkin Park/Minutes to Midnight/04 - Bleed It Out.mp3"
|
||||
)
|
||||
|
||||
func TestAdoptMovedTrack_MatchesByMbid(t *testing.T) {
|
||||
s := testScanner(t)
|
||||
q := &fakeAdopter{byMbid: []dbq.FindMissingTrackByMbidRow{mbidRow(7, oldPath)}, adoptRows: 1}
|
||||
|
||||
if !s.adoptMovedTrack(context.Background(), q, newPath, 5_000_000, 200_000, "rec-mbid") {
|
||||
t.Fatal("expected the moved track to be adopted")
|
||||
}
|
||||
if len(q.adopted) != 1 {
|
||||
t.Fatalf("adopted %d rows, want 1", len(q.adopted))
|
||||
}
|
||||
if q.adopted[0].ID != testUUID(7) {
|
||||
t.Errorf("adopted the wrong row: %v", q.adopted[0].ID)
|
||||
}
|
||||
if q.adopted[0].FilePath != newPath {
|
||||
t.Errorf("adopted FilePath = %q, want %q", q.adopted[0].FilePath, newPath)
|
||||
}
|
||||
// MBID matched, so the weaker signal should not have been consulted.
|
||||
if len(q.fingerprintQueried) != 0 {
|
||||
t.Errorf("queried the fingerprint despite an MBID match")
|
||||
}
|
||||
}
|
||||
|
||||
func TestAdoptMovedTrack_FallsBackToFingerprint(t *testing.T) {
|
||||
s := testScanner(t)
|
||||
q := &fakeAdopter{byFingerprint: []dbq.FindMissingTrackByFingerprintRow{fpRow(3, oldPath)}, adoptRows: 1}
|
||||
|
||||
// No MBID: an untagged file, which is exactly what the fallback is for.
|
||||
if !s.adoptMovedTrack(context.Background(), q, newPath, 4_200_000, 187_000, "") {
|
||||
t.Fatal("expected adoption via fingerprint")
|
||||
}
|
||||
if len(q.mbidQueried) != 0 {
|
||||
t.Errorf("queried by MBID with no MBID available")
|
||||
}
|
||||
if len(q.fingerprintQueried) != 1 {
|
||||
t.Fatalf("fingerprint queried %d times, want 1", len(q.fingerprintQueried))
|
||||
}
|
||||
got := q.fingerprintQueried[0]
|
||||
if got.FileSize != 4_200_000 || got.DurationMs != 187_000 {
|
||||
t.Errorf("fingerprint = %+v, want size 4200000 duration 187000", got)
|
||||
}
|
||||
if len(q.adopted) != 1 || q.adopted[0].ID != testUUID(3) {
|
||||
t.Errorf("adopted = %+v, want row 3", q.adopted)
|
||||
}
|
||||
}
|
||||
|
||||
// Two missing rows carrying the same recording MBID means real duplicates.
|
||||
// Adopting one arbitrarily would attach this file's future history to a coin
|
||||
// flip, so it must insert fresh instead.
|
||||
func TestAdoptMovedTrack_RefusesAmbiguousMbidMatch(t *testing.T) {
|
||||
s := testScanner(t)
|
||||
q := &fakeAdopter{byMbid: []dbq.FindMissingTrackByMbidRow{
|
||||
mbidRow(1, "/music/a.mp3"),
|
||||
mbidRow(2, "/music/b.mp3"),
|
||||
}, adoptRows: 1}
|
||||
|
||||
if s.adoptMovedTrack(context.Background(), q, newPath, 0, 0, "rec-mbid") {
|
||||
t.Fatal("expected refusal on an ambiguous MBID match")
|
||||
}
|
||||
if len(q.adopted) != 0 {
|
||||
t.Errorf("adopted despite ambiguity: %+v", q.adopted)
|
||||
}
|
||||
}
|
||||
|
||||
// An ambiguous MBID may still be resolvable by the fingerprint, which is a
|
||||
// narrower signal — so falling through is allowed to succeed.
|
||||
func TestAdoptMovedTrack_AmbiguousMbidFallsThroughToFingerprint(t *testing.T) {
|
||||
s := testScanner(t)
|
||||
q := &fakeAdopter{
|
||||
byMbid: []dbq.FindMissingTrackByMbidRow{
|
||||
mbidRow(1, "/music/a.mp3"),
|
||||
mbidRow(2, "/music/b.mp3"),
|
||||
},
|
||||
byFingerprint: []dbq.FindMissingTrackByFingerprintRow{fpRow(2, "/music/b.mp3")},
|
||||
adoptRows: 1,
|
||||
}
|
||||
|
||||
if !s.adoptMovedTrack(context.Background(), q, newPath, 1000, 2000, "rec-mbid") {
|
||||
t.Fatal("expected the fingerprint to disambiguate")
|
||||
}
|
||||
if len(q.adopted) != 1 || q.adopted[0].ID != testUUID(2) {
|
||||
t.Errorf("adopted = %+v, want row 2", q.adopted)
|
||||
}
|
||||
}
|
||||
|
||||
func TestAdoptMovedTrack_RefusesAmbiguousFingerprintMatch(t *testing.T) {
|
||||
s := testScanner(t)
|
||||
q := &fakeAdopter{byFingerprint: []dbq.FindMissingTrackByFingerprintRow{
|
||||
fpRow(1, "/music/a.mp3"),
|
||||
fpRow(2, "/music/b.mp3"),
|
||||
}, adoptRows: 1}
|
||||
|
||||
if s.adoptMovedTrack(context.Background(), q, newPath, 1000, 2000, "") {
|
||||
t.Fatal("expected refusal on an ambiguous fingerprint match")
|
||||
}
|
||||
if len(q.adopted) != 0 {
|
||||
t.Errorf("adopted despite ambiguity: %+v", q.adopted)
|
||||
}
|
||||
}
|
||||
|
||||
// duration_ms is 0 when ffprobe failed. Matching 0 against 0 would pair up
|
||||
// unrelated broken files, so the fingerprint must not be attempted.
|
||||
func TestAdoptMovedTrack_SkipsFingerprintWithoutRealValues(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
size int64
|
||||
duration int32
|
||||
}{
|
||||
{"no duration", 1000, 0},
|
||||
{"no size", 0, 2000},
|
||||
{"neither", 0, 0},
|
||||
}
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
s := testScanner(t)
|
||||
q := &fakeAdopter{
|
||||
byFingerprint: []dbq.FindMissingTrackByFingerprintRow{fpRow(1, oldPath)},
|
||||
adoptRows: 1,
|
||||
}
|
||||
if s.adoptMovedTrack(context.Background(), q, newPath, tc.size, tc.duration, "") {
|
||||
t.Error("adopted on an unusable fingerprint")
|
||||
}
|
||||
if len(q.fingerprintQueried) != 0 {
|
||||
t.Error("queried the fingerprint with unusable values")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestAdoptMovedTrack_NoCandidates(t *testing.T) {
|
||||
s := testScanner(t)
|
||||
q := &fakeAdopter{adoptRows: 1}
|
||||
|
||||
if s.adoptMovedTrack(context.Background(), q, newPath, 1000, 2000, "rec-mbid") {
|
||||
t.Fatal("expected no adoption when nothing matches")
|
||||
}
|
||||
if len(q.adopted) != 0 {
|
||||
t.Errorf("adopted with no candidates: %+v", q.adopted)
|
||||
}
|
||||
}
|
||||
|
||||
// The row's mark was cleared between lookup and update — another file adopted it
|
||||
// first. AdoptTrackPath's `missing_since IS NOT NULL` predicate reports 0 rows.
|
||||
func TestAdoptMovedTrack_LostRaceReportsNotAdopted(t *testing.T) {
|
||||
s := testScanner(t)
|
||||
q := &fakeAdopter{
|
||||
byMbid: []dbq.FindMissingTrackByMbidRow{mbidRow(5, oldPath)},
|
||||
adoptRows: 0,
|
||||
}
|
||||
|
||||
if s.adoptMovedTrack(context.Background(), q, newPath, 1000, 2000, "rec-mbid") {
|
||||
t.Fatal("expected not-adopted when the update matched no rows")
|
||||
}
|
||||
}
|
||||
|
||||
// Failing to detect a move must never fail the file: the caller falls back to
|
||||
// inserting a fresh row, which is the pre-#2528 behaviour.
|
||||
func TestAdoptMovedTrack_ToleratesQueryErrors(t *testing.T) {
|
||||
sentinel := errors.New("db down")
|
||||
tests := []struct {
|
||||
name string
|
||||
q *fakeAdopter
|
||||
}{
|
||||
{"mbid lookup fails", &fakeAdopter{mbidErr: sentinel}},
|
||||
{"fingerprint lookup fails", &fakeAdopter{fingerprintErr: sentinel}},
|
||||
{"adopt fails", &fakeAdopter{
|
||||
byMbid: []dbq.FindMissingTrackByMbidRow{mbidRow(1, oldPath)},
|
||||
adoptErr: sentinel,
|
||||
}},
|
||||
}
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
s := testScanner(t)
|
||||
if s.adoptMovedTrack(context.Background(), tc.q, newPath, 1000, 2000, "rec-mbid") {
|
||||
t.Error("reported adoption despite a query error")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// A failed MBID lookup must not stop the fingerprint from being tried.
|
||||
func TestAdoptMovedTrack_MbidErrorStillTriesFingerprint(t *testing.T) {
|
||||
s := testScanner(t)
|
||||
q := &fakeAdopter{
|
||||
mbidErr: errors.New("db hiccup"),
|
||||
byFingerprint: []dbq.FindMissingTrackByFingerprintRow{fpRow(9, oldPath)},
|
||||
adoptRows: 1,
|
||||
}
|
||||
|
||||
if !s.adoptMovedTrack(context.Background(), q, newPath, 1000, 2000, "rec-mbid") {
|
||||
t.Fatal("expected the fingerprint to be tried after an MBID lookup error")
|
||||
}
|
||||
if len(q.adopted) != 1 || q.adopted[0].ID != testUUID(9) {
|
||||
t.Errorf("adopted = %+v, want row 9", q.adopted)
|
||||
}
|
||||
}
|
||||
|
||||
func TestUniqueMatch(t *testing.T) {
|
||||
s := testScanner(t)
|
||||
if _, ok := s.uniqueMatch(nil, newPath, "mbid"); ok {
|
||||
t.Error("empty candidate set matched")
|
||||
}
|
||||
c, ok := s.uniqueMatch([]candidate{{id: testUUID(4), filePath: oldPath}}, newPath, "mbid")
|
||||
if !ok {
|
||||
t.Fatal("single candidate did not match")
|
||||
}
|
||||
if c.id != testUUID(4) || c.filePath != oldPath {
|
||||
t.Errorf("candidate = %+v, want id 4 at %q", c, oldPath)
|
||||
}
|
||||
if _, ok := s.uniqueMatch([]candidate{
|
||||
{id: testUUID(1)}, {id: testUUID(2)},
|
||||
}, newPath, "mbid"); ok {
|
||||
t.Error("multiple candidates matched")
|
||||
}
|
||||
}
|
||||
|
||||
func TestRowConverters(t *testing.T) {
|
||||
got := rowsFromMbid([]dbq.FindMissingTrackByMbidRow{mbidRow(1, "/a"), mbidRow(2, "/b")})
|
||||
if len(got) != 2 || got[0].id != testUUID(1) || got[1].filePath != "/b" {
|
||||
t.Errorf("rowsFromMbid = %+v", got)
|
||||
}
|
||||
got = rowsFromFingerprint([]dbq.FindMissingTrackByFingerprintRow{fpRow(3, "/c")})
|
||||
if len(got) != 1 || got[0].id != testUUID(3) || got[0].filePath != "/c" {
|
||||
t.Errorf("rowsFromFingerprint = %+v", got)
|
||||
}
|
||||
}
|
||||
|
||||
// pgtype.UUID zero value must not be mistaken for a real id.
|
||||
func TestUniqueMatch_ZeroUUIDNotValid(t *testing.T) {
|
||||
var zero pgtype.UUID
|
||||
if zero.Valid {
|
||||
t.Fatal("zero pgtype.UUID should not be Valid")
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,155 @@
|
||||
package library
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"os"
|
||||
|
||||
"github.com/jackc/pgx/v5/pgtype"
|
||||
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
|
||||
)
|
||||
|
||||
// trackReconciler is the slice of dbq.Queries reconcileMissing needs. Narrowed
|
||||
// to an interface so the guard logic — which is the part that can do damage —
|
||||
// is unit-testable against a fake without a database.
|
||||
type trackReconciler interface {
|
||||
ListTrackPathsForReconcile(ctx context.Context) ([]dbq.ListTrackPathsForReconcileRow, error)
|
||||
MarkTracksMissing(ctx context.Context, ids []pgtype.UUID) (int64, error)
|
||||
ClearTracksMissing(ctx context.Context, ids []pgtype.UUID) (int64, error)
|
||||
}
|
||||
|
||||
// Reconcile marks tracks whose files have disappeared (#2523).
|
||||
//
|
||||
// Why this exists: nothing in Minstrel used to notice a deleted file. The walk
|
||||
// only visits paths that exist, so a row whose file is gone was never scanned,
|
||||
// never errored, never counted — permanently invisible. The watcher ignores
|
||||
// removals by design (see classifyEvent), and the safety-net scan is the same
|
||||
// walk, so it covers additions only. Rows accumulated forever, kept being
|
||||
// offered to recommendations, and failed at playback.
|
||||
//
|
||||
// Why it MARKS rather than deletes: a missing file is a claim about the
|
||||
// filesystem, and the filesystem lies transiently — an unmounted volume, a
|
||||
// network-storage blip, a container that started before its media mount
|
||||
// attached. Every other sweep in this codebase (internal/gc) resolves a truth
|
||||
// *inside* the database and is safe to run blind. This one isn't, so the
|
||||
// destructive step is deliberately not here. Marking is reversible: the next
|
||||
// good scan clears it.
|
||||
|
||||
// missingMarkMaxFraction caps how much of the library one reconcile may newly
|
||||
// mark missing. A partially-attached mount is the failure this defends against:
|
||||
// the roots resolve, the walk succeeds, and it legitimately sees only part of
|
||||
// the library — evidence indistinguishable from a mass deletion.
|
||||
//
|
||||
// A quarter is deliberately conservative. A genuine bulk deletion trips it and
|
||||
// gets logged rather than applied, which needs a second scan (or operator
|
||||
// action) to take effect. That's the right trade: the cost of over-refusing is
|
||||
// a stale row and a log line, and the cost of over-marking is a chunk of the
|
||||
// library silently vanishing from every mix.
|
||||
const missingMarkMaxFraction = 0.25
|
||||
|
||||
// reconcileMissing diffs the paths the walk saw against every row in the table.
|
||||
// Rows not seen get marked; rows seen that carry a mark get cleared.
|
||||
//
|
||||
// seen must come from a COMPLETE walk of every configured root. Callers with a
|
||||
// partial view must not call this.
|
||||
func (s *Scanner) reconcileMissing(
|
||||
ctx context.Context, q trackReconciler, seen map[string]struct{}, stats *Stats,
|
||||
) error {
|
||||
if err := s.verifyRootsPresent(); err != nil {
|
||||
return err
|
||||
}
|
||||
// Roots resolved but the walk found nothing. Either the library is genuinely
|
||||
// empty — in which case there is nothing to reconcile — or the mount is
|
||||
// hollow. Both mean: don't act.
|
||||
if len(seen) == 0 {
|
||||
return errors.New("walk saw no audio files; refusing to reconcile")
|
||||
}
|
||||
|
||||
rows, err := q.ListTrackPathsForReconcile(ctx)
|
||||
if err != nil {
|
||||
return fmt.Errorf("list track paths: %w", err)
|
||||
}
|
||||
if len(rows) == 0 {
|
||||
return nil
|
||||
}
|
||||
|
||||
var toMark, toClear []pgtype.UUID
|
||||
for _, row := range rows {
|
||||
_, present := seen[row.FilePath]
|
||||
switch {
|
||||
case !present && !row.MissingSince.Valid:
|
||||
toMark = append(toMark, row.ID)
|
||||
case present && row.MissingSince.Valid:
|
||||
toClear = append(toClear, row.ID)
|
||||
}
|
||||
}
|
||||
|
||||
// Clear before marking, and unconditionally. Restoring a file is never the
|
||||
// dangerous direction, so it must not be blocked by the guard below —
|
||||
// otherwise a library that tripped the cap once could never recover its
|
||||
// marks even after the mount came back.
|
||||
if len(toClear) > 0 {
|
||||
n, err := q.ClearTracksMissing(ctx, toClear)
|
||||
if err != nil {
|
||||
return fmt.Errorf("clear missing marks: %w", err)
|
||||
}
|
||||
stats.Restored = int(n)
|
||||
s.logger.Info("library scan: files returned", "count", n)
|
||||
}
|
||||
|
||||
if len(toMark) == 0 {
|
||||
return nil
|
||||
}
|
||||
if fraction := float64(len(toMark)) / float64(len(rows)); fraction > missingMarkMaxFraction {
|
||||
return fmt.Errorf(
|
||||
"refusing to mark %d of %d tracks missing (%.0f%% > %.0f%% cap): "+
|
||||
"this looks like an unavailable mount rather than a deletion",
|
||||
len(toMark), len(rows), fraction*100, missingMarkMaxFraction*100,
|
||||
)
|
||||
}
|
||||
|
||||
n, err := q.MarkTracksMissing(ctx, toMark)
|
||||
if err != nil {
|
||||
return fmt.Errorf("mark tracks missing: %w", err)
|
||||
}
|
||||
stats.Missing = int(n)
|
||||
// Warn, not Info: every one of these is a library entry the operator
|
||||
// probably didn't intend to lose, and the only place it surfaces today is
|
||||
// this line.
|
||||
s.logger.Warn("library scan: tracks marked missing (files not found)",
|
||||
"count", n, "library_total", len(rows))
|
||||
return nil
|
||||
}
|
||||
|
||||
// verifyRootsPresent is the first and most important guard. If a configured root
|
||||
// doesn't resolve to a readable directory, the walk beneath it found nothing and
|
||||
// every row under it would look deleted. An unmounted media volume is the
|
||||
// obvious case, and it is common enough — a container restart racing its volume
|
||||
// mount does exactly this.
|
||||
func (s *Scanner) verifyRootsPresent() error {
|
||||
if len(s.paths) == 0 {
|
||||
return errors.New("no scan roots configured")
|
||||
}
|
||||
for _, root := range s.paths {
|
||||
info, err := os.Stat(root)
|
||||
if err != nil {
|
||||
return fmt.Errorf("scan root %q unavailable: %w", root, err)
|
||||
}
|
||||
if !info.IsDir() {
|
||||
return fmt.Errorf("scan root %q is not a directory", root)
|
||||
}
|
||||
entries, err := os.ReadDir(root)
|
||||
if err != nil {
|
||||
return fmt.Errorf("scan root %q unreadable: %w", root, err)
|
||||
}
|
||||
// An empty root is the signature of a mount point with nothing mounted
|
||||
// on it. `os.Stat` succeeds on the bare directory, so this is the only
|
||||
// cheap way to tell the two apart.
|
||||
if len(entries) == 0 {
|
||||
return fmt.Errorf("scan root %q is empty; refusing to reconcile", root)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
@@ -0,0 +1,326 @@
|
||||
package library
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"log/slog"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
|
||||
"github.com/jackc/pgx/v5/pgtype"
|
||||
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
|
||||
)
|
||||
|
||||
// fakeReconciler records what reconcileMissing decided to do, so the guards can
|
||||
// be tested without a database. The guards are the whole point of this pass —
|
||||
// they are what stands between an unmounted volume and the library disappearing
|
||||
// from every mix — so they get tested directly rather than via integration.
|
||||
type fakeReconciler struct {
|
||||
rows []dbq.ListTrackPathsForReconcileRow
|
||||
marked []pgtype.UUID
|
||||
cleared []pgtype.UUID
|
||||
listErr error
|
||||
markErr error
|
||||
clearErr error
|
||||
}
|
||||
|
||||
func (f *fakeReconciler) ListTrackPathsForReconcile(context.Context) ([]dbq.ListTrackPathsForReconcileRow, error) {
|
||||
return f.rows, f.listErr
|
||||
}
|
||||
|
||||
func (f *fakeReconciler) MarkTracksMissing(_ context.Context, ids []pgtype.UUID) (int64, error) {
|
||||
if f.markErr != nil {
|
||||
return 0, f.markErr
|
||||
}
|
||||
f.marked = append(f.marked, ids...)
|
||||
return int64(len(ids)), nil
|
||||
}
|
||||
|
||||
func (f *fakeReconciler) ClearTracksMissing(_ context.Context, ids []pgtype.UUID) (int64, error) {
|
||||
if f.clearErr != nil {
|
||||
return 0, f.clearErr
|
||||
}
|
||||
f.cleared = append(f.cleared, ids...)
|
||||
return int64(len(ids)), nil
|
||||
}
|
||||
|
||||
// Compile-time proof the real queries still satisfy what reconcile needs — the
|
||||
// interface exists to narrow dbq.Queries, not to diverge from it.
|
||||
var _ trackReconciler = (*dbq.Queries)(nil)
|
||||
|
||||
func testUUID(n byte) pgtype.UUID {
|
||||
var u pgtype.UUID
|
||||
u.Bytes[15] = n
|
||||
u.Valid = true
|
||||
return u
|
||||
}
|
||||
|
||||
func markedAt() pgtype.Timestamptz {
|
||||
return pgtype.Timestamptz{Valid: true}
|
||||
}
|
||||
|
||||
func row(n byte, path string, missing bool) dbq.ListTrackPathsForReconcileRow {
|
||||
r := dbq.ListTrackPathsForReconcileRow{ID: testUUID(n), FilePath: path}
|
||||
if missing {
|
||||
r.MissingSince = markedAt()
|
||||
}
|
||||
return r
|
||||
}
|
||||
|
||||
// populatedRoot returns a directory containing one file, so verifyRootsPresent
|
||||
// treats it as a real, mounted library root.
|
||||
func populatedRoot(t *testing.T) string {
|
||||
t.Helper()
|
||||
dir := t.TempDir()
|
||||
if err := os.WriteFile(filepath.Join(dir, "a.mp3"), []byte("x"), 0o600); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
return dir
|
||||
}
|
||||
|
||||
func testScanner(t *testing.T, roots ...string) *Scanner {
|
||||
t.Helper()
|
||||
return &Scanner{
|
||||
logger: slog.New(slog.NewTextHandler(io.Discard, nil)),
|
||||
paths: roots,
|
||||
}
|
||||
}
|
||||
|
||||
func TestReconcileMissing_MarksRowsTheWalkDidNotSee(t *testing.T) {
|
||||
root := populatedRoot(t)
|
||||
s := testScanner(t, root)
|
||||
|
||||
// 10 rows with 2 absent — 20%, deliberately under missingMarkMaxFraction so
|
||||
// this exercises marking rather than the cap. (An earlier version of this
|
||||
// test used 2-of-4 and was really testing the guard by accident.)
|
||||
rows := make([]dbq.ListTrackPathsForReconcileRow, 0, 10)
|
||||
seen := map[string]struct{}{}
|
||||
for i := 0; i < 10; i++ {
|
||||
p := fmt.Sprintf("/music/track-%02d.mp3", i)
|
||||
rows = append(rows, row(byte(i), p, false))
|
||||
if i >= 2 {
|
||||
seen[p] = struct{}{}
|
||||
}
|
||||
}
|
||||
q := &fakeReconciler{rows: rows}
|
||||
|
||||
var stats Stats
|
||||
if err := s.reconcileMissing(context.Background(), q, seen, &stats); err != nil {
|
||||
t.Fatalf("reconcile: %v", err)
|
||||
}
|
||||
if len(q.marked) != 2 {
|
||||
t.Fatalf("marked %d rows, want 2", len(q.marked))
|
||||
}
|
||||
if q.marked[0] != testUUID(0) || q.marked[1] != testUUID(1) {
|
||||
t.Errorf("marked the wrong rows: %v", q.marked)
|
||||
}
|
||||
if stats.Missing != 2 {
|
||||
t.Errorf("stats.Missing = %d, want 2", stats.Missing)
|
||||
}
|
||||
if len(q.cleared) != 0 {
|
||||
t.Errorf("cleared %d rows, want 0", len(q.cleared))
|
||||
}
|
||||
}
|
||||
|
||||
func TestReconcileMissing_ClearsRowsWhoseFileReturned(t *testing.T) {
|
||||
root := populatedRoot(t)
|
||||
s := testScanner(t, root)
|
||||
q := &fakeReconciler{rows: []dbq.ListTrackPathsForReconcileRow{
|
||||
row(1, "/music/back.mp3", true),
|
||||
row(2, "/music/still-here.mp3", false),
|
||||
}}
|
||||
seen := map[string]struct{}{
|
||||
"/music/back.mp3": {},
|
||||
"/music/still-here.mp3": {},
|
||||
}
|
||||
|
||||
var stats Stats
|
||||
if err := s.reconcileMissing(context.Background(), q, seen, &stats); err != nil {
|
||||
t.Fatalf("reconcile: %v", err)
|
||||
}
|
||||
if len(q.cleared) != 1 || q.cleared[0] != testUUID(1) {
|
||||
t.Fatalf("cleared = %v, want just row 1", q.cleared)
|
||||
}
|
||||
if stats.Restored != 1 {
|
||||
t.Errorf("stats.Restored = %d, want 1", stats.Restored)
|
||||
}
|
||||
if len(q.marked) != 0 {
|
||||
t.Errorf("marked %d rows, want 0", len(q.marked))
|
||||
}
|
||||
}
|
||||
|
||||
// An already-marked row must not be re-marked: the timestamp is the "how long
|
||||
// has this been gone" clock that any future cleanup policy depends on.
|
||||
func TestReconcileMissing_DoesNotRemarkAlreadyMissingRows(t *testing.T) {
|
||||
root := populatedRoot(t)
|
||||
s := testScanner(t, root)
|
||||
q := &fakeReconciler{rows: []dbq.ListTrackPathsForReconcileRow{
|
||||
row(1, "/music/long-gone.mp3", true),
|
||||
row(2, "/music/present.mp3", false),
|
||||
}}
|
||||
seen := map[string]struct{}{"/music/present.mp3": {}}
|
||||
|
||||
var stats Stats
|
||||
if err := s.reconcileMissing(context.Background(), q, seen, &stats); err != nil {
|
||||
t.Fatalf("reconcile: %v", err)
|
||||
}
|
||||
if len(q.marked) != 0 {
|
||||
t.Errorf("re-marked an already-missing row: %v", q.marked)
|
||||
}
|
||||
if len(q.cleared) != 0 {
|
||||
t.Errorf("cleared = %v, want none", q.cleared)
|
||||
}
|
||||
}
|
||||
|
||||
// The guard that matters most. A half-attached mount makes the walk succeed
|
||||
// while seeing only part of the library — evidence indistinguishable from a mass
|
||||
// deletion, so reconcile must refuse rather than guess.
|
||||
func TestReconcileMissing_RefusesWhenTooMuchWouldBeMarked(t *testing.T) {
|
||||
root := populatedRoot(t)
|
||||
s := testScanner(t, root)
|
||||
rows := make([]dbq.ListTrackPathsForReconcileRow, 0, 100)
|
||||
seen := map[string]struct{}{}
|
||||
for i := 0; i < 100; i++ {
|
||||
p := fmt.Sprintf("/music/track-%03d.mp3", i)
|
||||
rows = append(rows, row(byte(i), p, false))
|
||||
// Only 60 of 100 present -> 40% would be marked, over the 25% cap.
|
||||
if i < 60 {
|
||||
seen[p] = struct{}{}
|
||||
}
|
||||
}
|
||||
q := &fakeReconciler{rows: rows}
|
||||
|
||||
var stats Stats
|
||||
err := s.reconcileMissing(context.Background(), q, seen, &stats)
|
||||
if err == nil {
|
||||
t.Fatal("expected reconcile to refuse, got nil error")
|
||||
}
|
||||
if len(q.marked) != 0 {
|
||||
t.Errorf("marked %d rows despite refusing", len(q.marked))
|
||||
}
|
||||
if stats.Missing != 0 {
|
||||
t.Errorf("stats.Missing = %d, want 0", stats.Missing)
|
||||
}
|
||||
}
|
||||
|
||||
// Restoring is never the dangerous direction, so it must survive the cap —
|
||||
// otherwise a library that tripped the cap once could never clear its marks
|
||||
// even after the volume came back.
|
||||
func TestReconcileMissing_ClearsEvenWhenMarkCapTrips(t *testing.T) {
|
||||
root := populatedRoot(t)
|
||||
s := testScanner(t, root)
|
||||
rows := []dbq.ListTrackPathsForReconcileRow{row(1, "/music/back.mp3", true)}
|
||||
seen := map[string]struct{}{"/music/back.mp3": {}}
|
||||
// Add enough absent rows to blow the cap.
|
||||
for i := 2; i < 10; i++ {
|
||||
rows = append(rows, row(byte(i), fmt.Sprintf("/music/absent-%02d.mp3", i), false))
|
||||
}
|
||||
q := &fakeReconciler{rows: rows}
|
||||
|
||||
var stats Stats
|
||||
if err := s.reconcileMissing(context.Background(), q, seen, &stats); err == nil {
|
||||
t.Fatal("expected the mark cap to trip")
|
||||
}
|
||||
if len(q.cleared) != 1 {
|
||||
t.Errorf("cleared %d rows, want 1 — restores must not be blocked by the cap", len(q.cleared))
|
||||
}
|
||||
if stats.Restored != 1 {
|
||||
t.Errorf("stats.Restored = %d, want 1", stats.Restored)
|
||||
}
|
||||
}
|
||||
|
||||
func TestReconcileMissing_RefusesOnEmptyWalk(t *testing.T) {
|
||||
root := populatedRoot(t)
|
||||
s := testScanner(t, root)
|
||||
q := &fakeReconciler{rows: []dbq.ListTrackPathsForReconcileRow{
|
||||
row(1, "/music/a.mp3", false),
|
||||
}}
|
||||
|
||||
var stats Stats
|
||||
if err := s.reconcileMissing(context.Background(), q, map[string]struct{}{}, &stats); err == nil {
|
||||
t.Fatal("expected refusal when the walk saw no files")
|
||||
}
|
||||
if len(q.marked) != 0 {
|
||||
t.Errorf("marked rows on an empty walk: %v", q.marked)
|
||||
}
|
||||
}
|
||||
|
||||
// The unmounted-volume case: the configured root doesn't exist at all.
|
||||
func TestReconcileMissing_RefusesWhenRootMissing(t *testing.T) {
|
||||
s := testScanner(t, filepath.Join(t.TempDir(), "not-mounted"))
|
||||
q := &fakeReconciler{rows: []dbq.ListTrackPathsForReconcileRow{
|
||||
row(1, "/music/a.mp3", false),
|
||||
}}
|
||||
|
||||
var stats Stats
|
||||
if err := s.reconcileMissing(context.Background(), q, map[string]struct{}{"/x": {}}, &stats); err == nil {
|
||||
t.Fatal("expected refusal when a scan root is absent")
|
||||
}
|
||||
if len(q.marked) != 0 {
|
||||
t.Errorf("marked rows with an absent root: %v", q.marked)
|
||||
}
|
||||
}
|
||||
|
||||
// A mount point that exists but has nothing mounted on it: os.Stat succeeds on
|
||||
// the bare directory, which is why emptiness is checked separately.
|
||||
func TestReconcileMissing_RefusesWhenRootEmpty(t *testing.T) {
|
||||
s := testScanner(t, t.TempDir())
|
||||
q := &fakeReconciler{rows: []dbq.ListTrackPathsForReconcileRow{
|
||||
row(1, "/music/a.mp3", false),
|
||||
}}
|
||||
|
||||
var stats Stats
|
||||
if err := s.reconcileMissing(context.Background(), q, map[string]struct{}{"/x": {}}, &stats); err == nil {
|
||||
t.Fatal("expected refusal when a scan root is empty")
|
||||
}
|
||||
}
|
||||
|
||||
// Several roots, one detached. Marking must not proceed on partial evidence just
|
||||
// because the other roots looked fine.
|
||||
func TestReconcileMissing_RefusesWhenAnyRootMissing(t *testing.T) {
|
||||
good := populatedRoot(t)
|
||||
s := testScanner(t, good, filepath.Join(t.TempDir(), "detached"))
|
||||
q := &fakeReconciler{rows: []dbq.ListTrackPathsForReconcileRow{
|
||||
row(1, "/music/a.mp3", false),
|
||||
}}
|
||||
|
||||
var stats Stats
|
||||
if err := s.reconcileMissing(context.Background(), q, map[string]struct{}{"/x": {}}, &stats); err == nil {
|
||||
t.Fatal("expected refusal when one of several roots is absent")
|
||||
}
|
||||
}
|
||||
|
||||
func TestReconcileMissing_NoRowsIsNotAnError(t *testing.T) {
|
||||
root := populatedRoot(t)
|
||||
s := testScanner(t, root)
|
||||
q := &fakeReconciler{}
|
||||
|
||||
var stats Stats
|
||||
if err := s.reconcileMissing(context.Background(), q, map[string]struct{}{"/x": {}}, &stats); err != nil {
|
||||
t.Fatalf("empty library should reconcile cleanly, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestReconcileMissing_PropagatesListError(t *testing.T) {
|
||||
root := populatedRoot(t)
|
||||
s := testScanner(t, root)
|
||||
sentinel := errors.New("boom")
|
||||
q := &fakeReconciler{listErr: sentinel}
|
||||
|
||||
var stats Stats
|
||||
err := s.reconcileMissing(context.Background(), q, map[string]struct{}{"/x": {}}, &stats)
|
||||
if !errors.Is(err, sentinel) {
|
||||
t.Fatalf("err = %v, want it to wrap %v", err, sentinel)
|
||||
}
|
||||
}
|
||||
|
||||
func TestVerifyRootsPresent_NoRootsConfigured(t *testing.T) {
|
||||
s := testScanner(t)
|
||||
if err := s.verifyRootsPresent(); err == nil {
|
||||
t.Fatal("expected an error with no scan roots configured")
|
||||
}
|
||||
}
|
||||
+174
-32
@@ -39,12 +39,32 @@ var audioExtensions = map[string]bool{
|
||||
".wav": true,
|
||||
}
|
||||
|
||||
// tagReadVersion is the version of this package's tag-extraction logic. Rows
|
||||
// whose tracks.tag_read_version is lower get their tags re-read on the next
|
||||
// scan even when the file itself hasn't changed, so a fix reaches an existing
|
||||
// library without the operator rebuilding it (migration 0054).
|
||||
//
|
||||
// Bump this whenever a change to tag extraction should reach already-indexed
|
||||
// files, and say why below.
|
||||
//
|
||||
// 1: genre read from the ID3v2 TCON frame directly and stored ";"-delimited.
|
||||
// dhowden/tag welds null-separated multi-values into one token
|
||||
// ("Alternative Rock" + "Rock" -> "Alternative RockRock"), which corrupted
|
||||
// the genre browse axis and polluted the taste profile's tag vocabulary,
|
||||
// and left bare ID3v1 numeric references unresolved (#2499).
|
||||
const tagReadVersion int16 = 1
|
||||
|
||||
type Stats struct {
|
||||
Scanned int `json:"scanned"`
|
||||
Added int `json:"added"`
|
||||
Updated int `json:"updated"`
|
||||
Skipped int `json:"skipped"`
|
||||
Errored int `json:"errored"`
|
||||
// Missing / Restored come from the reconcile pass, not the walk (#2523):
|
||||
// rows whose file the walk didn't find, and rows whose file came back.
|
||||
// Only a full Scan sets these — see reconcileMissing.
|
||||
Missing int `json:"missing"`
|
||||
Restored int `json:"restored"`
|
||||
}
|
||||
|
||||
type Scanner struct {
|
||||
@@ -61,6 +81,12 @@ func New(pool *pgxpool.Pool, logger *slog.Logger, paths []string) *Scanner {
|
||||
// newer than the existing row's updated_at. Walk errors and per-file errors
|
||||
// are logged + counted; the scan keeps going.
|
||||
//
|
||||
// It then reconciles: rows whose file the walk never saw get marked missing,
|
||||
// and rows whose file has come back get un-marked (#2523). Only a FULL scan may
|
||||
// do this — the walk's set of seen paths is the evidence, and a partial
|
||||
// (watcher-driven) scan has no basis for concluding anything about files it
|
||||
// didn't look at. That's why ScanFiles does not reconcile.
|
||||
//
|
||||
// progressCb (may be nil) receives the current Stats snapshot after each
|
||||
// processed file. Used by the orchestrator to drive partial-tally writes.
|
||||
func (s *Scanner) Scan(ctx context.Context, progressCb func(Stats)) (Stats, error) {
|
||||
@@ -68,24 +94,48 @@ func (s *Scanner) Scan(ctx context.Context, progressCb func(Stats)) (Stats, erro
|
||||
q := dbq.New(s.pool)
|
||||
start := time.Now()
|
||||
|
||||
for _, root := range s.paths {
|
||||
if err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error {
|
||||
// PHASE 1 — enumerate. Collect every audio path without touching tags or
|
||||
// ffprobe. Cheap: WalkDir already stats each entry, so this adds a directory
|
||||
// traversal and nothing else.
|
||||
//
|
||||
// The order matters and is the whole reason enumeration is separate.
|
||||
// Reconcile has to mark disappeared rows BEFORE any file is processed,
|
||||
// because move detection (#2528) can only adopt a row that is already marked
|
||||
// missing. A rename performed while the server was down surfaces the deletion
|
||||
// and the addition in the SAME scan — so if reconcile ran at the end, the new
|
||||
// path would insert a fresh row first and the fork would be permanent.
|
||||
paths, walkErrs := s.enumerate(ctx, progressCb, &stats)
|
||||
stats.Errored += walkErrs
|
||||
if err := ctx.Err(); err != nil {
|
||||
return stats, err
|
||||
}
|
||||
|
||||
// PHASE 2 — reconcile. Only ever on a COMPLETE enumeration: a cancelled walk
|
||||
// has a partial view and would mark everything it hadn't reached.
|
||||
seen := make(map[string]struct{}, len(paths))
|
||||
for _, p := range paths {
|
||||
seen[p] = struct{}{}
|
||||
}
|
||||
if err := s.reconcileMissing(ctx, q, seen, &stats); err != nil {
|
||||
// Not fatal. The guards deliberately refuse to act on ambiguous
|
||||
// evidence, and that refusal arrives here as an error.
|
||||
//
|
||||
// The consequence is named explicitly because it is not obvious: move
|
||||
// detection (#2528) can only adopt a row that is already marked missing,
|
||||
// so a refused reconcile also means renamed files insert fresh rows and
|
||||
// fork their history. That's the pre-#2528 behaviour rather than a new
|
||||
// failure, but it's worth knowing which scan it happened on. It bites
|
||||
// hardest when a large fraction of a small library is reorganised at
|
||||
// once, which trips the mark cap.
|
||||
s.logger.Warn("library scan: reconcile skipped — moved files will fork rather than adopt",
|
||||
"err", err)
|
||||
}
|
||||
|
||||
// PHASE 3 — process, in walk order so logs and cover-art batching stay
|
||||
// grouped by directory rather than following map iteration order.
|
||||
for _, path := range paths {
|
||||
if ctx.Err() != nil {
|
||||
return fs.SkipAll
|
||||
}
|
||||
if err != nil {
|
||||
s.logger.Warn("library scan walk error", "path", path, "err", err)
|
||||
stats.Errored++
|
||||
if progressCb != nil {
|
||||
progressCb(stats)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
if d.IsDir() {
|
||||
return nil
|
||||
}
|
||||
if !audioExtensions[strings.ToLower(filepath.Ext(path))] {
|
||||
return nil
|
||||
break
|
||||
}
|
||||
if _, _, err := s.scanFile(ctx, q, path, &stats); err != nil {
|
||||
s.logger.Warn("library scan file error", "path", path, "err", err)
|
||||
@@ -94,10 +144,6 @@ func (s *Scanner) Scan(ctx context.Context, progressCb func(Stats)) (Stats, erro
|
||||
if progressCb != nil {
|
||||
progressCb(stats)
|
||||
}
|
||||
return nil
|
||||
}); err != nil {
|
||||
return stats, fmt.Errorf("library: walk %q: %w", root, err)
|
||||
}
|
||||
}
|
||||
|
||||
s.logger.Info("library scan complete",
|
||||
@@ -106,6 +152,8 @@ func (s *Scanner) Scan(ctx context.Context, progressCb func(Stats)) (Stats, erro
|
||||
"updated", stats.Updated,
|
||||
"skipped", stats.Skipped,
|
||||
"errored", stats.Errored,
|
||||
"missing", stats.Missing,
|
||||
"restored", stats.Restored,
|
||||
"duration_ms", time.Since(start).Milliseconds(),
|
||||
)
|
||||
if err := ctx.Err(); err != nil {
|
||||
@@ -114,6 +162,46 @@ func (s *Scanner) Scan(ctx context.Context, progressCb func(Stats)) (Stats, erro
|
||||
return stats, nil
|
||||
}
|
||||
|
||||
// enumerate walks every configured root and returns the audio paths found, in
|
||||
// walk order, plus a count of walk errors.
|
||||
//
|
||||
// A path is recorded even if it will later fail to parse: an unreadable file is a
|
||||
// broken file, not a missing one, and letting reconcile mark it missing would
|
||||
// hide it from the operator behind the wrong explanation.
|
||||
func (s *Scanner) enumerate(
|
||||
ctx context.Context, progressCb func(Stats), stats *Stats,
|
||||
) ([]string, int) {
|
||||
paths := make([]string, 0, 8192)
|
||||
errs := 0
|
||||
for _, root := range s.paths {
|
||||
// WalkDir's own error return is folded into the per-entry handler below,
|
||||
// so a bad root is counted rather than aborting the whole scan — one
|
||||
// unreadable root shouldn't discard the others' results.
|
||||
_ = filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error {
|
||||
if ctx.Err() != nil {
|
||||
return fs.SkipAll
|
||||
}
|
||||
if err != nil {
|
||||
s.logger.Warn("library scan walk error", "path", path, "err", err)
|
||||
errs++
|
||||
if progressCb != nil {
|
||||
progressCb(*stats)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
if d.IsDir() {
|
||||
return nil
|
||||
}
|
||||
if !audioExtensions[strings.ToLower(filepath.Ext(path))] {
|
||||
return nil
|
||||
}
|
||||
paths = append(paths, path)
|
||||
return nil
|
||||
})
|
||||
}
|
||||
return paths, errs
|
||||
}
|
||||
|
||||
// scanFile upserts a single audio file. Returns the album ID the track
|
||||
// belongs to and whether the file was added/updated (false = skipped as
|
||||
// unchanged), so watcher-driven callers can enrich just the changed albums.
|
||||
@@ -133,12 +221,15 @@ func (s *Scanner) scanFile(
|
||||
if err != nil && !errors.Is(err, pgx.ErrNoRows) {
|
||||
return pgtype.UUID{}, false, fmt.Errorf("lookup: %w", err)
|
||||
}
|
||||
// Incremental skip: only when the file hasn't changed AND we already have
|
||||
// a real duration. The second clause lets older scans that recorded
|
||||
// duration_ms=0 (before ffprobe was wired) get backfilled without forcing
|
||||
// the operator to wipe the library. Once duration is set, subsequent
|
||||
// scans short-circuit as before.
|
||||
if knownTrack && !existing.UpdatedAt.Time.Before(mtime) && existing.DurationMs > 0 {
|
||||
// Incremental skip: only when the file hasn't changed AND we already have a
|
||||
// real duration AND the row's tag-derived columns were written by the
|
||||
// current extraction logic. The duration clause lets older scans that
|
||||
// recorded duration_ms=0 (before ffprobe was wired) get backfilled without
|
||||
// forcing the operator to wipe the library; the tag-version clause does the
|
||||
// same job for tag-extraction fixes (#2499). Once both are current,
|
||||
// subsequent scans short-circuit as before.
|
||||
unchanged := knownTrack && !existing.UpdatedAt.Time.Before(mtime)
|
||||
if unchanged && existing.DurationMs > 0 && existing.TagReadVersion >= tagReadVersion {
|
||||
stats.Skipped++
|
||||
return pgtype.UUID{}, false, nil
|
||||
}
|
||||
@@ -180,14 +271,43 @@ func (s *Scanner) scanFile(
|
||||
|
||||
trackNum, _ := meta.Track()
|
||||
discNum, _ := meta.Disc()
|
||||
durationMs, err := probeDurationMs(ctx, path)
|
||||
if err != nil {
|
||||
|
||||
// An unchanged file being re-read only to refresh tag-derived columns
|
||||
// doesn't need another ffprobe: the stored duration is still accurate, and
|
||||
// the file's bytes haven't moved. This keeps a library-wide tag-repair pass
|
||||
// (a tagReadVersion bump) bound by tag reads rather than costing one
|
||||
// fork+exec per file.
|
||||
var durationMs int32
|
||||
if unchanged && existing.DurationMs > 0 {
|
||||
durationMs = existing.DurationMs
|
||||
} else {
|
||||
probed, perr := probeDurationMs(ctx, path)
|
||||
if perr != nil {
|
||||
// Missing duration is degraded UX (clients can't scrub) but not a
|
||||
// blocker for ingestion. Record the file with 0ms; the next scan
|
||||
// will retry via the backfill clause in the skip check above.
|
||||
s.logger.Warn("library scan: ffprobe failed", "path", path, "err", err)
|
||||
durationMs = 0
|
||||
s.logger.Warn("library scan: ffprobe failed", "path", path, "err", perr)
|
||||
}
|
||||
durationMs = probed
|
||||
}
|
||||
|
||||
// A path we've never seen might not be a new track — it might be one that
|
||||
// moved or was renamed (#2528). Adopting re-points the existing row at this
|
||||
// path and clears its missing mark, so the UpsertTrack below conflicts on
|
||||
// file_path and updates THAT row: same track id, likes and play history
|
||||
// intact. Without this, renumbering an album forks every track on it.
|
||||
//
|
||||
// Runs here rather than earlier because the fingerprint needs the probed
|
||||
// duration, and only for genuinely unknown paths — a known path is already
|
||||
// the row we're going to update.
|
||||
if !knownTrack {
|
||||
if s.adoptMovedTrack(ctx, q, path, info.Size(), durationMs, recordingMBID) {
|
||||
// Count it as an update: the row existed, and reporting it as Added
|
||||
// would overstate library growth on every reorganisation.
|
||||
knownTrack = true
|
||||
}
|
||||
}
|
||||
|
||||
params := dbq.UpsertTrackParams{
|
||||
Title: trackTitle,
|
||||
AlbumID: album.ID,
|
||||
@@ -196,6 +316,8 @@ func (s *Scanner) scanFile(
|
||||
FilePath: path,
|
||||
FileSize: info.Size(),
|
||||
FileFormat: strings.TrimPrefix(strings.ToLower(filepath.Ext(path)), "."),
|
||||
// Stamped so a future extraction fix can find this row again.
|
||||
TagReadVersion: tagReadVersion,
|
||||
}
|
||||
if trackNum > 0 {
|
||||
v := int32(trackNum)
|
||||
@@ -205,7 +327,14 @@ func (s *Scanner) scanFile(
|
||||
v := int32(discNum)
|
||||
params.DiscNumber = &v
|
||||
}
|
||||
if g := meta.Genre(); g != "" {
|
||||
if genres, fellBack := extractGenres(meta, f); len(genres) > 0 {
|
||||
if fellBack {
|
||||
// dhowden/tag's welded value — see genre.go. Logged because the
|
||||
// stored genre for this file is the old, corrupt shape.
|
||||
s.logger.Warn("library scan: genre frame unreadable, using fallback",
|
||||
"path", path, "genre", meta.Genre())
|
||||
}
|
||||
g := strings.Join(genres, genreDelimiter)
|
||||
params.Genre = &g
|
||||
}
|
||||
// Recording MBID feeds the ListenBrainz similarity pipeline.
|
||||
@@ -285,8 +414,21 @@ func (s *Scanner) resolveArtist(ctx context.Context, q *dbq.Queries, name, mbid
|
||||
ID: existing.ID,
|
||||
Mbid: &m,
|
||||
}); uerr != nil {
|
||||
if isUniqueViolation(uerr) {
|
||||
// Another artist row already owns this MBID — two rows that
|
||||
// should be merged (usually two spellings of one name).
|
||||
// Expected, not a fault: leave NULL and let the operator
|
||||
// merge. Mirrors resolveAlbum, which has always handled it
|
||||
// this way — without this branch the identical benign
|
||||
// condition logged a generic warning plus a Postgres ERROR
|
||||
// line on every scan, which teaches an operator to ignore
|
||||
// database errors (#2524).
|
||||
s.logger.Info("library scan: duplicate artist mbid (canonical row already owns it)",
|
||||
"artist_id", existing.ID, "artist", name, "mbid", mbid)
|
||||
} else {
|
||||
s.logger.Warn("library scan: heal artist mbid failed",
|
||||
"artist_id", existing.ID, "err", uerr)
|
||||
}
|
||||
} else {
|
||||
existing.Mbid = &m
|
||||
}
|
||||
|
||||
@@ -205,3 +205,106 @@ func writeTestMP3(t *testing.T, path string, frames map[string]string) {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}
|
||||
|
||||
// TestScanner_AdoptsMovedFile_Integration is the #2528 proof: a renamed file
|
||||
// must keep its existing tracks row — same id, so likes, play history and
|
||||
// playlist memberships travel with it — rather than forking into a marked ghost
|
||||
// plus a fresh zero-history row.
|
||||
//
|
||||
// Uses the MBID path. The synthetic MP3s here carry no real audio, so ffprobe
|
||||
// yields duration 0 and the size+duration fingerprint is deliberately unusable —
|
||||
// which is why the recording MBID is the signal under test.
|
||||
//
|
||||
// Eight tracks with one rename keeps the marked fraction at 12.5%, under
|
||||
// missingMarkMaxFraction. That is load-bearing: if the rename exceeded the cap,
|
||||
// reconcile would refuse to mark, adoption could not fire, and the file would
|
||||
// fork. See the "reconcile skipped" warning in Scan.
|
||||
func TestScanner_AdoptsMovedFile_Integration(t *testing.T) {
|
||||
if testing.Short() {
|
||||
t.Skip("skipping scanner integration in -short mode")
|
||||
}
|
||||
dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL")
|
||||
if dsn == "" {
|
||||
t.Skip("MINSTREL_TEST_DATABASE_URL not set")
|
||||
}
|
||||
ctx := context.Background()
|
||||
logger := slog.New(slog.NewTextHandler(io.Discard, nil))
|
||||
|
||||
if err := db.Migrate(dsn, logger); err != nil {
|
||||
t.Fatalf("migrate: %v", err)
|
||||
}
|
||||
pool, err := pgxpool.New(ctx, dsn)
|
||||
if err != nil {
|
||||
t.Fatalf("pool: %v", err)
|
||||
}
|
||||
t.Cleanup(pool.Close)
|
||||
if _, err := pool.Exec(ctx, "TRUNCATE tracks, albums, artists RESTART IDENTITY CASCADE"); err != nil {
|
||||
t.Fatalf("truncate: %v", err)
|
||||
}
|
||||
|
||||
root := t.TempDir()
|
||||
const movedMBID = "11111111-2222-3333-4444-555555555555"
|
||||
movedFrom := filepath.Join(root, "artistM/albumM/04 - Bleed It Out.mp3")
|
||||
writeTestMP3(t, movedFrom, map[string]string{
|
||||
"TIT2": "Bleed It Out", "TPE1": "Artist M", "TALB": "Album M", "TRCK": "4",
|
||||
// dhowden surfaces TXXX as a Comm whose Description is the Picard tag
|
||||
// name; "MusicBrainz Track Id" is mbz.Recording.
|
||||
"TXXX": "MusicBrainz Track Id\x00" + movedMBID,
|
||||
})
|
||||
// Filler so one rename stays under the mark cap.
|
||||
for i := 1; i <= 7; i++ {
|
||||
writeTestMP3(t, filepath.Join(root, "artistM/albumM/filler", string(rune('a'+i))+".mp3"),
|
||||
map[string]string{
|
||||
"TIT2": "Filler " + string(rune('0'+i)), "TPE1": "Artist M", "TALB": "Album M",
|
||||
})
|
||||
}
|
||||
|
||||
scanner := New(pool, logger, []string{root})
|
||||
if _, err := scanner.Scan(ctx, nil); err != nil {
|
||||
t.Fatalf("first scan: %v", err)
|
||||
}
|
||||
|
||||
q := dbq.New(pool)
|
||||
before, err := q.GetTrackByPath(ctx, movedFrom)
|
||||
if err != nil {
|
||||
t.Fatalf("track not indexed on first scan: %v", err)
|
||||
}
|
||||
if before.Mbid == nil || *before.Mbid != movedMBID {
|
||||
t.Fatalf("recording mbid not stored: %v", before.Mbid)
|
||||
}
|
||||
|
||||
// Renumber the file, exactly as a tag editor would.
|
||||
movedTo := filepath.Join(root, "artistM/albumM/02 - Bleed It Out.mp3")
|
||||
if err := os.Rename(movedFrom, movedTo); err != nil {
|
||||
t.Fatalf("rename: %v", err)
|
||||
}
|
||||
|
||||
if _, err := scanner.Scan(ctx, nil); err != nil {
|
||||
t.Fatalf("second scan: %v", err)
|
||||
}
|
||||
|
||||
after, err := q.GetTrackByPath(ctx, movedTo)
|
||||
if err != nil {
|
||||
t.Fatalf("track not found at its new path: %v", err)
|
||||
}
|
||||
if after.ID != before.ID {
|
||||
t.Errorf("track id changed on rename: %v -> %v (history would be stranded)",
|
||||
before.ID, after.ID)
|
||||
}
|
||||
if after.MissingSince.Valid {
|
||||
t.Errorf("adopted row is still marked missing: %v", after.MissingSince)
|
||||
}
|
||||
|
||||
// The old path must be gone entirely — not lingering as a marked ghost.
|
||||
if _, err := q.GetTrackByPath(ctx, movedFrom); err == nil {
|
||||
t.Error("old path still has a tracks row; the track forked instead of moving")
|
||||
}
|
||||
|
||||
var total int
|
||||
if err := pool.QueryRow(ctx, "SELECT count(*) FROM tracks").Scan(&total); err != nil {
|
||||
t.Fatalf("count: %v", err)
|
||||
}
|
||||
if total != 8 {
|
||||
t.Errorf("tracks = %d, want 8 — a rename must not add a row", total)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -24,6 +24,11 @@ type LibraryStageTallies struct {
|
||||
Updated int `json:"updated"`
|
||||
Skipped int `json:"skipped"`
|
||||
Errored int `json:"errored"`
|
||||
// Reconcile results (#2523). Surfaced in the scan record because a track
|
||||
// disappearing from the library is something the operator should be able to
|
||||
// see happened, rather than discovering it when a mix comes up short.
|
||||
Missing int `json:"missing"`
|
||||
Restored int `json:"restored"`
|
||||
}
|
||||
|
||||
// MBIDBackfillStageTallies wires BackfillMBIDsResult into the scan_runs jsonb column.
|
||||
|
||||
@@ -54,9 +54,13 @@ func uuidString(u pgtype.UUID) string {
|
||||
// splitGenres splits a track's denormalized genre string on the common
|
||||
// multi-genre delimiters (`;`, `,`) used by various tag editors. Trims
|
||||
// whitespace; drops empty fragments. Strings with no delimiter come back
|
||||
// as a single-element slice. Concatenated-without-separator inputs (e.g.
|
||||
// "ElectronicComplextroGlitch Hop" from broken tag-editor output) cannot
|
||||
// be split without a genre dictionary and stay as one opaque tag.
|
||||
// as a single-element slice.
|
||||
//
|
||||
// This comment used to blame concatenated inputs like
|
||||
// "ElectronicComplextroGlitch Hop" on broken tag editors. They were ours: the
|
||||
// scanner stored dhowden/tag's welded multi-value frames verbatim. Fixed in
|
||||
// #2499 — the scanner now writes ";"-delimited values, so such tokens only
|
||||
// survive on rows not yet re-scanned.
|
||||
func splitGenres(s string) []string {
|
||||
parts := strings.FieldsFunc(s, func(r rune) bool {
|
||||
return r == ';' || r == ','
|
||||
|
||||
@@ -4,6 +4,20 @@ import { api } from './client';
|
||||
// Mirrors internal/api/me_recommendation_metrics.go: raw play sources are
|
||||
// bucketed server-side into stable surface families, grouped by intent, and
|
||||
// anchored by the manual-plays baseline (milestone #127).
|
||||
// A difference from the baseline, with its uncertainty (#2495). Both figures
|
||||
// are already in percentage points — the server does the arithmetic so both
|
||||
// clients read the same numbers.
|
||||
//
|
||||
// `distinguishable: false` means |delta_pp| < margin_pp: the delta cannot be
|
||||
// told apart from zero, however large it looks. That distinction is the whole
|
||||
// point of this type — `low_confidence` answers "is this worth showing?", which
|
||||
// is a much lower bar than "is this worth acting on?".
|
||||
export type MetricDelta = {
|
||||
delta_pp: number;
|
||||
margin_pp: number;
|
||||
distinguishable: boolean;
|
||||
};
|
||||
|
||||
export type SurfaceMetric = {
|
||||
key: string;
|
||||
label: string;
|
||||
@@ -12,6 +26,10 @@ export type SurfaceMetric = {
|
||||
skip_rate: number;
|
||||
avg_completion: number;
|
||||
low_confidence: boolean;
|
||||
// Absent on the baseline row itself, and whenever the samples are too thin
|
||||
// for a margin to mean anything.
|
||||
skip_delta?: MetricDelta;
|
||||
completion_delta?: MetricDelta;
|
||||
// Present when the surface's builder stamps pick-kind provenance and
|
||||
// the window holds attributed plays (#1249, generalized #1270): For
|
||||
// You's taste/fresh split, Discover's candidate buckets, the tiered
|
||||
|
||||
@@ -216,9 +216,17 @@
|
||||
return plays > 0 ? hits / plays : 0;
|
||||
}
|
||||
|
||||
function latest(s: TrendSeries): { skip: number; completion: number } {
|
||||
// The "latest" columns are ONE WEEK while the Plays column is the whole
|
||||
// window, which is a trap: a 40% skip rate off 17 plays sat next to a
|
||||
// four-figure Plays total and read as a solid signal. It isn't — I misread
|
||||
// exactly this and briefly concluded Deep cuts was the worst surface, when
|
||||
// over 180 days it's one of the best (#2495). So the week's own play count
|
||||
// comes back with the rates and is rendered beside them.
|
||||
function latest(s: TrendSeries): { skip: number; completion: number; plays: number } {
|
||||
const last = s.points[s.points.length - 1];
|
||||
return last ? { skip: last.skip_rate, completion: last.avg_completion } : { skip: 0, completion: 0 };
|
||||
return last
|
||||
? { skip: last.skip_rate, completion: last.avg_completion, plays: last.plays }
|
||||
: { skip: 0, completion: 0, plays: 0 };
|
||||
}
|
||||
|
||||
function pct(v: number): string {
|
||||
@@ -423,6 +431,9 @@
|
||||
Skip rate per surface over the last {trends?.weeks ?? 12} weeks (lower is better; all
|
||||
users aggregated, rates only). Dashed ticks mark tuning changes. Taste hit is the share
|
||||
of plays whose artist fits the current taste profile.
|
||||
<span class="font-medium">The skip and completion columns show the most recent week
|
||||
alone</span>, not the whole window — the figure after the skip rate is that week's
|
||||
play count, so a rate drawn from a handful of listens reads as what it is.
|
||||
</p>
|
||||
</div>
|
||||
{#if trendsFailed}
|
||||
@@ -439,10 +450,10 @@
|
||||
<tr class="text-left text-text-secondary">
|
||||
<th class="py-1 font-medium">Surface</th>
|
||||
<th class="py-1 font-medium">Skip rate by week</th>
|
||||
<th class="py-1 text-right font-medium">Plays</th>
|
||||
<th class="py-1 text-right font-medium">Latest skip</th>
|
||||
<th class="py-1 text-right font-medium">Latest completion</th>
|
||||
<th class="py-1 text-right font-medium">Taste hit</th>
|
||||
<th class="py-1 text-right font-medium">Plays<span class="font-normal text-xs"> (window)</span></th>
|
||||
<th class="py-1 text-right font-medium">Skip<span class="font-normal text-xs"> (last wk)</span></th>
|
||||
<th class="py-1 text-right font-medium">Completion<span class="font-normal text-xs"> (last wk)</span></th>
|
||||
<th class="py-1 text-right font-medium">Taste hit<span class="font-normal text-xs"> (window)</span></th>
|
||||
</tr>
|
||||
</thead>
|
||||
<tbody>
|
||||
@@ -493,7 +504,10 @@
|
||||
</svg>
|
||||
</td>
|
||||
<td class="py-1.5 text-right tabular-nums">{s.plays}</td>
|
||||
<td class="py-1.5 text-right tabular-nums">{pct(latest(s).skip)}</td>
|
||||
<td class="py-1.5 text-right tabular-nums">
|
||||
{pct(latest(s).skip)}
|
||||
<span class="text-xs text-text-secondary">/{latest(s).plays}</span>
|
||||
</td>
|
||||
<td class="py-1.5 text-right tabular-nums">{pct(latest(s).completion)}</td>
|
||||
<td class="py-1.5 text-right tabular-nums">{pct(windowTasteHitRate(s))}</td>
|
||||
</tr>
|
||||
|
||||
@@ -195,9 +195,29 @@ describe('Admin tuning page', () => {
|
||||
await waitFor(() => expect(screen.getByText('Weekly trends')).toBeInTheDocument());
|
||||
expect(screen.getByTestId('sparkline-radio')).toBeInTheDocument();
|
||||
expect(screen.getByTestId('sparkline-discover')).toBeInTheDocument();
|
||||
// Latest skip rate column for radio = 40% (also discover's latest
|
||||
// completion, hence getAllBy).
|
||||
expect(screen.getAllByText('40%').length).toBeGreaterThan(0);
|
||||
// The skip column is the LAST WEEK's rate, now carrying that week's play
|
||||
// count so a rate off a handful of plays reads as what it is (#2495).
|
||||
// Radio's latest week: 40% skip over 15 plays; Discover's: 60% over 5.
|
||||
expect(screen.getByText('/15')).toBeInTheDocument();
|
||||
expect(screen.getByText('/5')).toBeInTheDocument();
|
||||
// Completion columns are unchanged and still bare percentages — radio 70%.
|
||||
expect(screen.getByText('70%')).toBeInTheDocument();
|
||||
// '40%' is now genuinely ambiguous: radio's latest SKIP rate and discover's
|
||||
// latest COMPLETION are both 40%. testing-library matches an element's own
|
||||
// direct text nodes, so the skip cell still matches despite its trailing
|
||||
// play-count span. Assert the count rather than pretending it's unique.
|
||||
expect(screen.getAllByText('40%')).toHaveLength(2);
|
||||
// The window/last-week distinction has to be visible in the headers, or the
|
||||
// Plays total reads as the denominator of the skip rate. That misreading is
|
||||
// what #2495 was filed over.
|
||||
//
|
||||
// Queried as column headers rather than by text: the caption below also
|
||||
// mentions "Plays", and matching on the word finds the prose too.
|
||||
// Exact accessible names: /^Skip/ also matches the "Skip rate by week"
|
||||
// sparkline column, and /Plays/ matched the caption prose before that.
|
||||
expect(screen.getByRole('columnheader', { name: 'Plays (window)' })).toBeInTheDocument();
|
||||
expect(screen.getByRole('columnheader', { name: 'Skip (last wk)' })).toBeInTheDocument();
|
||||
expect(screen.getByRole('columnheader', { name: 'Completion (last wk)' })).toBeInTheDocument();
|
||||
// The knob turn is listed under the chart AND tooltipped on each
|
||||
// sparkline's marker tick, hence getAllBy.
|
||||
expect(
|
||||
|
||||
@@ -12,7 +12,7 @@
|
||||
createRecommendationMetricsQuery,
|
||||
type RecommendationMetrics,
|
||||
type SurfaceIntent,
|
||||
type SurfaceMetric
|
||||
type MetricDelta
|
||||
} from '$lib/api/metrics';
|
||||
import { theme, setTheme, type ThemePreference } from '$lib/stores/theme.svelte';
|
||||
import { player, setCrossfade } from '$lib/player/store.svelte';
|
||||
@@ -45,20 +45,39 @@
|
||||
return `${(v * 100).toFixed(0)}%`;
|
||||
}
|
||||
|
||||
// Delta in percentage points vs the baseline, signed ("+12" / "−5").
|
||||
function deltaPts(value: number, baseline: number): string {
|
||||
const pts = Math.round((value - baseline) * 100);
|
||||
return pts > 0 ? `+${pts}` : `${pts}`;
|
||||
// Deltas come from the server with their margin of error (#2495). The client
|
||||
// no longer subtracts rates itself: the margin needs the sample sizes and
|
||||
// variances, and having both clients re-derive it invites them to disagree.
|
||||
//
|
||||
// A delta that isn't distinguishable from zero is prefixed "≈" and dimmed.
|
||||
// That is the point of this whole change — the card used to render a −12 on
|
||||
// 59 plays exactly as boldly as a −6 on 400, and the first of those is noise.
|
||||
function deltaText(d: MetricDelta | undefined): string {
|
||||
if (!d) return '';
|
||||
const pts = Math.round(d.delta_pp);
|
||||
const signed = pts > 0 ? `+${pts}` : `${pts}`;
|
||||
return d.distinguishable ? signed : `≈${signed}`;
|
||||
}
|
||||
|
||||
// A surface's skip delta is "worse" when it skips more than the
|
||||
// baseline; completion delta is "worse" when it completes less.
|
||||
function skipDeltaClass(m: SurfaceMetric, baseline: SurfaceMetric): string {
|
||||
return m.skip_rate > baseline.skip_rate ? 'text-danger' : 'text-text-secondary';
|
||||
function deltaTitle(d: MetricDelta | undefined): string | undefined {
|
||||
if (!d) return undefined;
|
||||
const range = `${d.delta_pp.toFixed(1)} ± ${d.margin_pp.toFixed(1)} points vs baseline`;
|
||||
return d.distinguishable
|
||||
? `${range} (95% confidence)`
|
||||
: `${range} — not distinguishable from zero at 95% confidence, so read this as no measured difference.`;
|
||||
}
|
||||
|
||||
function completionDeltaClass(m: SurfaceMetric, baseline: SurfaceMetric): string {
|
||||
return m.avg_completion < baseline.avg_completion ? 'text-danger' : 'text-text-secondary';
|
||||
// A skip delta is "worse" above the baseline; a completion delta is "worse"
|
||||
// below it. Neither gets a colour unless it's distinguishable — colouring
|
||||
// noise red is what made the old card misleading.
|
||||
function skipDeltaClass(d: MetricDelta | undefined): string {
|
||||
if (!d?.distinguishable) return 'text-text-secondary opacity-60';
|
||||
return d.delta_pp > 0 ? 'text-danger' : 'text-text-secondary';
|
||||
}
|
||||
|
||||
function completionDeltaClass(d: MetricDelta | undefined): string {
|
||||
if (!d?.distinguishable) return 'text-text-secondary opacity-60';
|
||||
return d.delta_pp < 0 ? 'text-danger' : 'text-text-secondary';
|
||||
}
|
||||
|
||||
// Pick-kind breakdowns are collapsed by default (#1270): with every
|
||||
@@ -384,18 +403,18 @@
|
||||
<td class="py-1 text-right tabular-nums">{m.plays}</td>
|
||||
<td class="py-1 text-right tabular-nums">
|
||||
{pct(m.skip_rate)}
|
||||
{#if baseline}
|
||||
<span class="ml-1 text-xs {skipDeltaClass(m, baseline)}">
|
||||
{deltaPts(m.skip_rate, baseline.skip_rate)}
|
||||
</span>
|
||||
{#if m.skip_delta}
|
||||
<span class="ml-1 text-xs {skipDeltaClass(m.skip_delta)}"
|
||||
data-testid="skip-delta-{m.key}"
|
||||
title={deltaTitle(m.skip_delta)}>{deltaText(m.skip_delta)}</span>
|
||||
{/if}
|
||||
</td>
|
||||
<td class="py-1 text-right tabular-nums">
|
||||
{pct(m.avg_completion)}
|
||||
{#if baseline}
|
||||
<span class="ml-1 text-xs {completionDeltaClass(m, baseline)}">
|
||||
{deltaPts(m.avg_completion, baseline.avg_completion)}
|
||||
</span>
|
||||
{#if m.completion_delta}
|
||||
<span class="ml-1 text-xs {completionDeltaClass(m.completion_delta)}"
|
||||
data-testid="completion-delta-{m.key}"
|
||||
title={deltaTitle(m.completion_delta)}>{deltaText(m.completion_delta)}</span>
|
||||
{/if}
|
||||
</td>
|
||||
</tr>
|
||||
@@ -420,18 +439,18 @@
|
||||
<td class="py-1 text-right text-xs tabular-nums">{b.plays}</td>
|
||||
<td class="py-1 text-right text-xs tabular-nums">
|
||||
{pct(b.skip_rate)}
|
||||
{#if baseline}
|
||||
<span class="ml-1 {skipDeltaClass(b, baseline)}">
|
||||
{deltaPts(b.skip_rate, baseline.skip_rate)}
|
||||
</span>
|
||||
{#if b.skip_delta}
|
||||
<span class="ml-1 {skipDeltaClass(b.skip_delta)}"
|
||||
data-testid="skip-delta-{b.key}"
|
||||
title={deltaTitle(b.skip_delta)}>{deltaText(b.skip_delta)}</span>
|
||||
{/if}
|
||||
</td>
|
||||
<td class="py-1 text-right text-xs tabular-nums">
|
||||
{pct(b.avg_completion)}
|
||||
{#if baseline}
|
||||
<span class="ml-1 {completionDeltaClass(b, baseline)}">
|
||||
{deltaPts(b.avg_completion, baseline.avg_completion)}
|
||||
</span>
|
||||
{#if b.completion_delta}
|
||||
<span class="ml-1 {completionDeltaClass(b.completion_delta)}"
|
||||
data-testid="completion-delta-{b.key}"
|
||||
title={deltaTitle(b.completion_delta)}>{deltaText(b.completion_delta)}</span>
|
||||
{/if}
|
||||
</td>
|
||||
</tr>
|
||||
@@ -442,6 +461,12 @@
|
||||
</table>
|
||||
</div>
|
||||
{/each}
|
||||
<p class="text-xs text-text-secondary">
|
||||
Deltas compare each surface with your manual plays. A delta marked
|
||||
<span class="opacity-60">≈</span> is smaller than its own margin of error at this
|
||||
sample size — it can't be told apart from no difference, however big it looks.
|
||||
Hover any delta for its range.
|
||||
</p>
|
||||
{:else}
|
||||
<p class="text-sm text-text-secondary">
|
||||
No plays recorded yet. Play something from For You, Discover, or a mix.
|
||||
|
||||
@@ -241,6 +241,74 @@ describe('Settings page — Recommendation metrics card', () => {
|
||||
expect(screen.queryByText(/Taste picks/)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
// #2495: the card used to render a delta computed client-side with no notion
|
||||
// of uncertainty, so a -12 on 59 plays looked exactly as solid as a -6 on 400.
|
||||
// Deltas now arrive from the server with a margin, and an indistinguishable
|
||||
// one is marked with "≈" and dimmed rather than coloured.
|
||||
test('a delta smaller than its margin is marked as indistinguishable', async () => {
|
||||
setupPage();
|
||||
metricsMock.data = {
|
||||
window_days: 30,
|
||||
baseline: metric('manual', 'Manual library plays', { plays: 400, skip_rate: 0.27 }),
|
||||
groups: [
|
||||
{
|
||||
intent: 'discovery',
|
||||
label: 'Discovery mixes',
|
||||
surfaces: [
|
||||
metric('discover', 'Discover', {
|
||||
plays: 59,
|
||||
skip_rate: 0.153,
|
||||
// 13.3pp gap, but the margin at n=59 is wider than the gap.
|
||||
skip_delta: { delta_pp: -13.3, margin_pp: 14.5, distinguishable: false },
|
||||
completion_delta: { delta_pp: 28.0, margin_pp: 12.1, distinguishable: true }
|
||||
})
|
||||
]
|
||||
}
|
||||
]
|
||||
};
|
||||
render(SettingsPage);
|
||||
await waitFor(() => expect(screen.getByText('Discover')).toBeInTheDocument());
|
||||
|
||||
// Targeted by test id rather than text: the legend below the table also
|
||||
// contains a "≈", so matching on the glyph finds the explanation instead of
|
||||
// the delta. (It did, on the first attempt at this test.)
|
||||
const skip = screen.getByTestId('skip-delta-discover');
|
||||
expect(skip).toHaveTextContent('≈-13');
|
||||
expect(skip).toHaveAttribute('title', expect.stringContaining('not distinguishable from zero'));
|
||||
// It must NOT be coloured as a real regression/improvement.
|
||||
expect(skip.className).toContain('opacity-60');
|
||||
|
||||
// The completion delta clears its margin, so it renders plainly.
|
||||
const completion = screen.getByTestId('completion-delta-discover');
|
||||
expect(completion).toHaveTextContent('+28');
|
||||
expect(completion.className).not.toContain('opacity-60');
|
||||
|
||||
// And the legend explains the glyph rather than leaving it a mystery.
|
||||
expect(screen.getByText(/smaller than its own margin of error/i)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
// A delta is omitted entirely when the samples are too thin for a margin to
|
||||
// mean anything — the server decides that, and the cell must simply show the
|
||||
// rate rather than a bare "0".
|
||||
test('a surface with no delta shows its rate and nothing else', async () => {
|
||||
setupPage();
|
||||
metricsMock.data = {
|
||||
window_days: 30,
|
||||
baseline: metric('manual', 'Manual library plays', { plays: 400 }),
|
||||
groups: [
|
||||
{
|
||||
intent: 'go_to',
|
||||
label: 'Go-to surfaces',
|
||||
surfaces: [metric('radio', 'Radio', { plays: 1, skip_rate: 0 })]
|
||||
}
|
||||
]
|
||||
};
|
||||
render(SettingsPage);
|
||||
await waitFor(() => expect(screen.getByText('Radio')).toBeInTheDocument());
|
||||
expect(screen.queryByTestId('skip-delta-radio')).not.toBeInTheDocument();
|
||||
expect(screen.queryByTestId('completion-delta-radio')).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
test('surfaces without a breakdown render no toggle and no sub-rows', async () => {
|
||||
setupPage();
|
||||
metricsMock.data = {
|
||||
|
||||
Reference in New Issue
Block a user