diff --git a/internal/api/radio.go b/internal/api/radio.go index b39ae73f..8f4d194c 100644 --- a/internal/api/radio.go +++ b/internal/api/radio.go @@ -108,7 +108,15 @@ func (h *handlers) handleRadio(w http.ResponseWriter, r *http.Request) { // Scoring weights come from the DB-backed tuning lab (#1250) — // read per request so an admin change takes effect live. weights := h.recSettings.Weights(recsettings.ScopeRadio) - picks := recommendation.Shuffle(candidates, weights, time.Now().UTC(), h.rng, limit-1) + // Diversity caps (#3882). Radio had none while every sibling surface did, + // which is how a whole session could come back from one artist. Scaled to + // the requested length so a 20-track radio and a 200-track one are capped + // alike; Shuffle relaxes them rather than returning a short radio. + // + // limit-1 because the seed track occupies the first slot and is prepended + // below — the caps govern the tracks that FOLLOW it. + caps := recommendation.RadioDiversityCaps(limit - 1) + picks := recommendation.Shuffle(candidates, weights, time.Now().UTC(), h.rng, limit-1, caps) out := make([]TrackRef, 0, len(picks)+1) out = append(out, trackRefFrom(track, album.Title, artist.Name)) diff --git a/internal/recommendation/shuffle.go b/internal/recommendation/shuffle.go index d81eedfe..b7033d24 100644 --- a/internal/recommendation/shuffle.go +++ b/internal/recommendation/shuffle.go @@ -4,6 +4,8 @@ import ( "sort" "time" + "github.com/jackc/pgx/v5/pgtype" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" ) @@ -13,15 +15,72 @@ type Candidate struct { Inputs ScoringInputs } -// Shuffle scores each candidate, sorts descending by score, and returns -// the top `limit` candidates. limit <= 0 returns nil; nil input returns -// nil. Pure — no IO, no global state beyond the rng callback. +// DiversityCaps bounds how much of one selection a single artist or album +// may occupy. A zero value means "no cap", which is what Shuffle did +// unconditionally before #3882. +type DiversityCaps struct { + MaxPerArtist int + MaxPerAlbum int +} + +// radioCapArtistPer25 / radioCapAlbumPer25 hold the caps at the same +// PROPORTION the system mixes already use — 3 artist / 2 album tracks in a +// 25-track mix — so a 20-track radio and a 200-track one feel alike rather +// than one of them being effectively uncapped. +// +// A fixed count cannot do that. Three-per-artist is a reasonable 12% of a +// 25-track mix and an absurd 1.5% of a 200-track radio, where it would put +// the selection permanently in the relaxation path below and quietly undo +// the cap it was meant to enforce. +const ( + radioCapArtistPer25 = 3 + radioCapAlbumPer25 = 2 +) + +// RadioDiversityCaps scales the diversity caps to the requested radio +// length. Floors of 2 and 1 keep a very short radio from being capped into +// a single track per artist, which would be its own kind of wrong. +func RadioDiversityCaps(limit int) DiversityCaps { + artist := limit * radioCapArtistPer25 / 25 + if artist < 2 { + artist = 2 + } + album := limit * radioCapAlbumPer25 / 25 + if album < 1 { + album = 1 + } + return DiversityCaps{MaxPerArtist: artist, MaxPerAlbum: album} +} + +// Shuffle scores each candidate, sorts descending by score, and returns the +// top `limit` candidates, preferring artist/album diversity. limit <= 0 +// returns nil; nil input returns nil. Pure — no IO, no global state beyond +// the rng callback. +// +// THE COUNT IS NEVER REDUCED BY THE CAPS. Selection runs in two passes: the +// first takes candidates that fit under the caps, and the second fills any +// remaining slots from those the first pass skipped, still in score order. +// So the result holds min(limit, len(candidates)) either way — the caps +// change WHICH tracks are chosen, never HOW MANY. +// +// That two-pass shape is the whole design, and a hard cap would have been +// the easy mistake. Radio asks for 50 tracks by default and 200 at most; a +// pool concentrated on a few artists would return six tracks and call it a +// radio. Rule 131's principle — degrade, never vanish — applies past the +// system mixes it was written for. +// +// Before #3882 there was no cap here at all, while discover.go, +// you_might_like.go and home.go all had one. That asymmetry is what let a +// radio session come back entirely from a single artist: nothing between +// the pool and the output bounded any artist's share, so a pool dominated +// by one artist produced an output dominated by it too. func Shuffle( candidates []Candidate, weights ScoringWeights, now time.Time, rng func() float64, limit int, + caps DiversityCaps, ) []Candidate { if len(candidates) == 0 || limit <= 0 { return nil @@ -40,9 +99,36 @@ func Shuffle( if limit > len(scored) { limit = len(scored) } - out := make([]Candidate, limit) - for i := 0; i < limit; i++ { - out[i] = scored[i].c + + out := make([]Candidate, 0, limit) + deferred := make([]Candidate, 0, len(scored)-limit) + artistCount := map[pgtype.UUID]int{} + albumCount := map[pgtype.UUID]int{} + + for _, s := range scored { + if len(out) == limit { + break + } + overArtist := caps.MaxPerArtist > 0 && artistCount[s.c.Track.ArtistID] >= caps.MaxPerArtist + overAlbum := caps.MaxPerAlbum > 0 && albumCount[s.c.Track.AlbumID] >= caps.MaxPerAlbum + if overArtist || overAlbum { + // Held back, not discarded — pass two may still need it. + deferred = append(deferred, s.c) + continue + } + artistCount[s.c.Track.ArtistID]++ + albumCount[s.c.Track.AlbumID]++ + out = append(out, s.c) + } + + // Pass two: the caps could not fill the request, so relax them rather + // than hand back a short radio. Still score order, so the best of the + // held-back candidates go first. + for _, c := range deferred { + if len(out) == limit { + break + } + out = append(out, c) } return out } diff --git a/internal/recommendation/shuffle_caps_test.go b/internal/recommendation/shuffle_caps_test.go new file mode 100644 index 00000000..9ba78ecd --- /dev/null +++ b/internal/recommendation/shuffle_caps_test.go @@ -0,0 +1,197 @@ +package recommendation + +import ( + "fmt" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgtype" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// candBy builds a candidate with a real artist and album identity, which +// `cand` deliberately leaves zero. +func candBy(t *testing.T, id, artist, album string, in ScoringInputs) Candidate { + t.Helper() + tr := dbq.Track{Title: id} + if err := tr.ID.Scan("00000000-0000-0000-0000-" + id); err != nil { + t.Fatalf("track id %q: %v", id, err) + } + if err := tr.ArtistID.Scan("00000000-0000-0000-0001-" + artist); err != nil { + t.Fatalf("artist id %q: %v", artist, err) + } + if err := tr.AlbumID.Scan("00000000-0000-0000-0002-" + album); err != nil { + t.Fatalf("album id %q: %v", album, err) + } + return Candidate{Track: tr, Inputs: in} +} + +// artistKey is the map key artistsOf produces for a given fixture artist, +// derived the same way the fixture builds the UUID. Hand-writing the hex is +// how the first version of this test went wrong: the artist id is not all +// zeros — it carries 0001 in its fourth group — so the literal did not match +// and the assertion measured nothing. +func artistKey(t *testing.T, artist string) string { + t.Helper() + var id pgtype.UUID + if err := id.Scan("00000000-0000-0000-0001-" + artist); err != nil { + t.Fatalf("artist id %q: %v", artist, err) + } + return fmt.Sprintf("%x", id.Bytes) +} + +// artistsOf counts how many picks each artist contributed. +func artistsOf(picks []Candidate) map[string]int { + out := map[string]int{} + for _, p := range picks { + out[fmt.Sprintf("%x", p.Track.ArtistID.Bytes)]++ + } + return out +} + +// THE BUG. Operator, 2026-09-10: started radio from a song and "literally +// all of the songs in the playlist after that were from a single artist". +// +// One artist's tracks all outscore everything else, and there are more of +// them than the radio has room for. Without a cap the output is entirely +// that artist — nothing between the pool and the result bounded its share. +func TestShuffle_CapStopsOneArtistTakingTheWholeRadio(t *testing.T) { + var cs []Candidate + // 20 tracks by artist A, all liked so they sort to the top. + for i := 0; i < 20; i++ { + cs = append(cs, candBy(t, fmt.Sprintf("%012d", i), "00000000000a", + fmt.Sprintf("%012d", i), ScoringInputs{IsGeneralLiked: true})) + } + // 10 tracks by 10 other artists, none liked, so they all rank below. + for i := 0; i < 10; i++ { + cs = append(cs, candBy(t, fmt.Sprintf("%012d", 100+i), + fmt.Sprintf("%012d", 200+i), fmt.Sprintf("%012d", 100+i), + ScoringInputs{IsGeneralLiked: false})) + } + + const limit = 10 + caps := DiversityCaps{MaxPerArtist: 3} + picks := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), limit, caps) + + if len(picks) != limit { + t.Fatalf("len = %d, want %d", len(picks), limit) + } + byArtist := artistsOf(picks) + artistA := artistKey(t, "00000000000a") + if got := byArtist[artistA]; got != 3 { + t.Errorf("artist A contributed %d of %d picks, cap is 3", got, limit) + } + if len(byArtist) < 8 { + t.Errorf("only %d distinct artists in a %d-track radio; the cap is not "+ + "spreading the selection", len(byArtist), limit) + } + + // The same pool with NO cap is the regression this exists to catch. If + // this stops holding, the test above is no longer proving anything. + uncapped := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), limit, DiversityCaps{}) + if artistsOf(uncapped)[artistA] != limit { + t.Errorf("uncapped selection was not single-artist, so this fixture no " + + "longer reproduces the bug being fixed") + } +} + +// A CAP IS A BOUND, NOT AN EXCLUSION. The operator asked for the opposite of +// removal: "again it should be able to add songs from the same artist." +func TestShuffle_CappedArtistIsStillRepresented(t *testing.T) { + var cs []Candidate + for i := 0; i < 20; i++ { + cs = append(cs, candBy(t, fmt.Sprintf("%012d", i), "00000000000a", + fmt.Sprintf("%012d", i), ScoringInputs{IsGeneralLiked: true})) + } + for i := 0; i < 10; i++ { + cs = append(cs, candBy(t, fmt.Sprintf("%012d", 100+i), + fmt.Sprintf("%012d", 200+i), fmt.Sprintf("%012d", 100+i), + ScoringInputs{IsGeneralLiked: false})) + } + picks := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10, + DiversityCaps{MaxPerArtist: 3}) + if artistsOf(picks)[artistKey(t, "00000000000a")] == 0 { + t.Error("the dominant artist was excluded entirely; the cap should bound " + + "its share, not remove it") + } +} + +// RULE 131, applied past the system mixes it was written for: degrade, never +// vanish. A hard cap over a pool with few artists would hand back a six-track +// "radio" for a fifty-track request. The caps must change WHICH tracks are +// picked, never HOW MANY. +func TestShuffle_CapsNeverShortenTheResult(t *testing.T) { + for _, tc := range []struct { + name string + artists int + perArt int + limit int + }{ + {"one artist owns the entire pool", 1, 30, 10}, + {"two artists, tight cap", 2, 15, 20}, + {"pool smaller than the request", 3, 2, 50}, + } { + t.Run(tc.name, func(t *testing.T) { + var cs []Candidate + n := 0 + for a := 0; a < tc.artists; a++ { + for k := 0; k < tc.perArt; k++ { + cs = append(cs, candBy(t, fmt.Sprintf("%012d", n), + fmt.Sprintf("%012d", 300+a), fmt.Sprintf("%012d", n), + ScoringInputs{})) + n++ + } + } + want := tc.limit + if len(cs) < want { + want = len(cs) + } + picks := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), tc.limit, + DiversityCaps{MaxPerArtist: 3, MaxPerAlbum: 2}) + if len(picks) != want { + t.Errorf("len = %d, want %d — the caps shortened the result instead "+ + "of relaxing to fill it", len(picks), want) + } + // No duplicates: a candidate deferred in pass one must not also be + // taken in pass two. + seen := map[[16]byte]bool{} + for _, p := range picks { + if seen[p.Track.ID.Bytes] { + t.Fatalf("track %x appears twice; pass two re-added a pick", p.Track.ID.Bytes) + } + seen[p.Track.ID.Bytes] = true + } + }) + } +} + +// A fixed count cannot serve both a 20-track radio and a 200-track one: three +// per artist is 12% of the first and 1.5% of the second, which would leave the +// long radio permanently in the relaxation path and effectively uncapped. +func TestRadioDiversityCaps_ScaleWithTheRequestedLength(t *testing.T) { + short := RadioDiversityCaps(25) + long := RadioDiversityCaps(200) + + if short.MaxPerArtist != 3 { + t.Errorf("a 25-track radio caps artists at %d, want 3 — the same "+ + "proportion the system mixes use", short.MaxPerArtist) + } + if long.MaxPerArtist <= short.MaxPerArtist { + t.Errorf("a 200-track radio caps artists at %d, no higher than a 25-track "+ + "one at %d; the cap is not scaling", long.MaxPerArtist, short.MaxPerArtist) + } + // Proportion held, not just "bigger". + if long.MaxPerArtist != 24 { + t.Errorf("200-track artist cap = %d, want 24 (3 per 25)", long.MaxPerArtist) + } + + // Floors: a tiny radio must not be capped down to one track per artist. + tiny := RadioDiversityCaps(1) + if tiny.MaxPerArtist < 2 { + t.Errorf("tiny radio artist cap = %d, want at least 2", tiny.MaxPerArtist) + } + if tiny.MaxPerAlbum < 1 { + t.Errorf("tiny radio album cap = %d, want at least 1", tiny.MaxPerAlbum) + } +} diff --git a/internal/recommendation/shuffle_test.go b/internal/recommendation/shuffle_test.go index ec3c2341..8b54107b 100644 --- a/internal/recommendation/shuffle_test.go +++ b/internal/recommendation/shuffle_test.go @@ -21,7 +21,7 @@ func TestShuffle_LikedRanksAboveUnliked(t *testing.T) { cand("000000000001", ScoringInputs{IsGeneralLiked: false}), cand("000000000002", ScoringInputs{IsGeneralLiked: true}), } - out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10) + out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{}) if out[0].Track.Title != "000000000002" { t.Errorf("liked track did not rank first: %+v", out) } @@ -33,7 +33,7 @@ func TestShuffle_HighSkipRanksLast(t *testing.T) { cand("000000000002", ScoringInputs{PlayCount: 10, SkipCount: 0}), // ratio 0 cand("000000000003", ScoringInputs{PlayCount: 10, SkipCount: 5}), // ratio 0.5 } - out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10) + out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{}) if out[0].Track.Title != "000000000002" || out[2].Track.Title != "000000000001" { t.Errorf("skip-ratio ordering broken: %v", titles(out)) } @@ -44,7 +44,7 @@ func TestShuffle_LimitTruncates(t *testing.T) { for i := range cs { cs[i] = cand("00000000000"+string(rune('a'+i%26)), ScoringInputs{}) } - out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10) + out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{}) if len(out) != 10 { t.Errorf("len = %d, want 10", len(out)) } @@ -58,7 +58,7 @@ func TestShuffle_JitterDoesNotFlipStructuralWinner(t *testing.T) { cand("000000000001", ScoringInputs{IsGeneralLiked: false}), cand("000000000002", ScoringInputs{IsGeneralLiked: true}), } - out := Shuffle(cs, defaultWeights(), time.Now(), r.Float64, 10) + out := Shuffle(cs, defaultWeights(), time.Now(), r.Float64, 10, DiversityCaps{}) if out[0].Track.Title != "000000000002" { t.Fatalf("iter %d: liked did not rank first; out=%v", i, titles(out)) } @@ -66,7 +66,7 @@ func TestShuffle_JitterDoesNotFlipStructuralWinner(t *testing.T) { } func TestShuffle_Empty_ReturnsEmpty(t *testing.T) { - out := Shuffle(nil, defaultWeights(), time.Now(), fixedRNG(0.5), 10) + out := Shuffle(nil, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{}) if len(out) != 0 { t.Errorf("len = %d, want 0", len(out)) }