From 633d4f591f15726128f4986c8761924e5dc4f726 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 10 Sep 2026 22:14:42 -0400 Subject: [PATCH] fix(radio): cap any one artist's share of a radio session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 which was not expected." There was no per-artist cap anywhere in the radio path. radio.go built the pool and handed it straight to Shuffle, which scores, sorts and takes the top N — nothing between those steps bounded any artist's share, so a pool dominated by one artist produced an output dominated by it. The asymmetry was the tell: discover.go, you_might_like.go and home.go all cap; radio never got one. With the fixture that reproduces it — 20 liked tracks by one artist plus 10 by ten others — the old path returns 10 tracks from 1 artist. It now returns 10 from 8. TWO PASSES, and that is the whole design. A hard cap was the easy mistake: radio asks for 50 tracks by default and 200 at most, so capping at three per artist over a concentrated pool would hand back a six-track "radio". Pass one takes candidates that fit under the caps; pass two fills any remaining slots from those it skipped, still in score order. The result always holds min(limit, len(candidates)) — the caps change WHICH tracks are picked, never HOW MANY. Rule 131's principle past the system mixes it was written for. The caps SCALE with the requested length rather than being a constant. Three-per-artist is a sensible 12% of a 25-track mix and an absurd 1.5% of a 200-track radio, where every selection would sit in the relaxation path and the cap would be decorative. RadioDiversityCaps holds the system mixes' proportion at any length: 3/2 at 25, 6/4 at 50, 24/16 at 200, with floors so a very short radio is not capped down to one track per artist. 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." The dominant artist still appears, just not exclusively. Guarded, because the tempting wrong fix is the filter songs-like used to carry. Shuffle grew the parameter rather than gaining a capped twin: radio is its only production caller, so a second function would have left the original dead (rule 22). Falsified against each named regression: uncapped gives 10/10 to one artist; a hard cap returns 3 of 10 on a single-artist pool; a cap-as-exclusion drops the artist entirely; a fixed cap stays 3 where the scaled one reaches 24. Caught while writing the guards: the artist-key constant was hand-written hex and wrong — the fixture's artist UUID carries 0001 in its fourth group, so the lookup missed and the assertion measured nothing. Derived from the same construction the fixture uses now. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- internal/api/radio.go | 10 +- internal/recommendation/shuffle.go | 98 ++++++++- internal/recommendation/shuffle_caps_test.go | 197 +++++++++++++++++++ internal/recommendation/shuffle_test.go | 10 +- 4 files changed, 303 insertions(+), 12 deletions(-) create mode 100644 internal/recommendation/shuffle_caps_test.go 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)) }