From b27029f67482f7e48823bd4b88847c7d98ff94cc Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 1 Aug 2026 12:56:42 -0400 Subject: [PATCH] =?UTF-8?q?feat(discover):=20rotate=20the=20suggestion=20d?= =?UTF-8?q?eck=20daily=20+=20cap=20one=20seed's=20share=20=E2=80=94=20#237?= =?UTF-8?q?3?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second half of the reported symptom: suggestions "show the same artists until you request one". The ranking was `ORDER BY total_score DESC` with no randomization and no seen-state, so the only things that could ever change the deck were a candidate entering the library or the user filing a request. The tail of the ranking was unreachable — requesting was literally the only lever. No SQL change was needed. The query already takes a limit, so it over-fetches a pool (4x the slots, capped at 60) and the selection moves to Go, where it is a pure function of (pool, limit, day) — no DB, no clock — and therefore unit testable in the fast lane instead of behind the integration gate. Three rules. The best few by score always lead, so the strongest matches never rotate out of sight (For You's head/tail shape). The remaining slots are drawn by md5(mbid + day), the same daily-stable idiom the Home rows already use: stable within a day so pull-to-refresh doesn't reshuffle, different tomorrow, and no stored state. And a per-seed cap keeps roughly a quarter of the deck attributable to any one seed artist, so twelve neighbours of a single artist can't be the whole surface. The cap is a preference, not a quota. A user whose pool hangs off one or two seeds would otherwise get a three-card surface — worse than the monoculture being avoided, and exactly the vanish-or-nothing shape rule #131 exists to prevent — so a short deck tops up in score order from what the cap set aside. This is also what keeps the existing Top12Cap integration test honest: its 30 candidates share one seed, and without the top-up it would return 3. Eight unit tests, including one that had to be rewritten mid-change: the first version asserted the cap against an evenly-spread pool, where the top-N is already diverse and the assertion could not fail. It now uses a skewed pool where one seed owns the entire top of the ranking, which is the only shape that actually exercises a cap. Dropped two //nolint:gosec directives added in passing — gosec isn't in .golangci.yml, so they suppressed nothing and only implied a check that runs. Co-Authored-By: Claude Opus 5 (1M context) --- internal/recommendation/suggestions.go | 140 ++++++++++++++- .../recommendation/suggestions_select_test.go | 170 ++++++++++++++++++ 2 files changed, 308 insertions(+), 2 deletions(-) create mode 100644 internal/recommendation/suggestions_select_test.go diff --git a/internal/recommendation/suggestions.go b/internal/recommendation/suggestions.go index fbef853f..1898d35b 100644 --- a/internal/recommendation/suggestions.go +++ b/internal/recommendation/suggestions.go @@ -13,7 +13,11 @@ package recommendation import ( "context" + "crypto/md5" + "encoding/hex" "fmt" + "sort" + "time" "github.com/jackc/pgx/v5/pgtype" "github.com/jackc/pgx/v5/pgxpool" @@ -53,10 +57,14 @@ func SuggestArtists(ctx context.Context, pool *pgxpool.Pool, userID pgtype.UUID, halfLifeDays = 30 } q := dbq.New(pool) + // Over-fetch so there is something to rotate through. A deterministic + // top-N over a score ordering shows the same faces every day until one is + // requested away — the tail of the ranking was unreachable (#2367 + // mechanism 1). Selection from this pool happens in selectSuggestions. rows, err := q.SuggestArtistsForUser(ctx, dbq.SuggestArtistsForUserParams{ UserID: userID, Column2: halfLifeDays, - Limit: int32(limit), + Limit: int32(poolSizeFor(limit)), }) if err != nil { return nil, fmt.Errorf("suggest: query: %w", err) @@ -109,5 +117,133 @@ func SuggestArtists(ctx context.Context, pool *pgxpool.Pool, userID pgtype.UUID, Attribution: attribution, }) } - return out, nil + return selectSuggestions(out, limit, rotationDay(time.Now())), nil +} + +// Pool multiplier: how many scored candidates to fetch per slot shown, so the +// rotation has somewhere to rotate. 4x keeps a day's deck genuinely different +// from yesterday's without pulling the whole long tail (whose scores are noise) +// into every request. +const suggestionPoolFactor = 4 + +// Hard ceiling on the fetched pool. The scored tail past this is low-signal, so +// paying for it would buy churn rather than better suggestions. +const suggestionPoolMax = 60 + +func poolSizeFor(limit int) int { + n := limit * suggestionPoolFactor + if n > suggestionPoolMax { + return suggestionPoolMax + } + return n +} + +// rotationDay is the bucket the daily rotation hashes against. Server-local +// date, matching the `current_date` the Home rows already rotate on, so the +// whole product turns over at the same moment. +func rotationDay(now time.Time) string { + return now.Format("2006-01-02") +} + +// selectSuggestions turns the scored pool into the response. +// +// Pure by design — no DB, no clock — so the rotation and diversity rules are +// unit-testable without Postgres. Callers pass the day bucket in. +// +// Three rules, in order: +// +// 1. Diversity cap. Walking in score order, a candidate is dropped once its +// dominant seed already owns maxPerSeed slots. Nothing capped this before, +// so all twelve slots could be neighbours of one artist and the surface +// read as a single narrow cluster (#2367 mechanism 4). +// 2. Head. The best few by score always lead, so the strongest matches are +// never rotated out of sight. Mirrors For You's head/tail shape. +// 3. Tail. The rest of the slots are drawn from the remaining pool ordered by +// md5(mbid + day) — the same daily-stable idiom the Home rows use. Stable +// within a day, different tomorrow, and it needs no stored state. +func selectSuggestions(pool []ArtistSuggestion, limit int, day string) []ArtistSuggestion { + if len(pool) <= limit { + return pool + } + preferred, overflow := partitionPerSeed(pool, maxPerSeedFor(limit)) + + headCount := limit / 3 + if headCount > len(preferred) { + headCount = len(preferred) + } + out := make([]ArtistSuggestion, 0, limit) + out = append(out, preferred[:headCount]...) + + // Hash once per candidate, not once per comparison. + rest := preferred[headCount:] + tail := make([]rotatable, 0, len(rest)) + for _, s := range rest { + tail = append(tail, rotatable{key: rotationKey(s.MBID, day), suggestion: s}) + } + sort.SliceStable(tail, func(i, j int) bool { return tail[i].key < tail[j].key }) + for _, r := range tail { + if len(out) >= limit { + break + } + out = append(out, r.suggestion) + } + // Diversity is a PREFERENCE, not a quota that may starve the deck. A user + // whose whole pool hangs off one or two seeds would otherwise get a + // three-card surface, which is worse than the monoculture we were avoiding + // (and is the vanish-or-nothing shape rule #131 exists to prevent). Top up + // in score order from what the cap set aside. + for _, s := range overflow { + if len(out) >= limit { + break + } + out = append(out, s) + } + return out +} + +// rotatable pairs a candidate with its precomputed daily rotation key. +type rotatable struct { + key string + suggestion ArtistSuggestion +} + +// maxPerSeedFor keeps roughly a quarter of the deck attributable to any single +// seed artist — enough for a strong affinity to be well represented, not enough +// for it to BE the deck. Floored at 1 so a small limit still returns something. +func maxPerSeedFor(limit int) int { + n := limit / 4 + if n < 1 { + return 1 + } + return n +} + +// partitionPerSeed splits the pool by whether each candidate's dominant +// (highest-contributing) seed still has allowance left. Input must be in score +// order; both outputs preserve it. +// +// Candidates with no attribution are always preferred — there is no seed to +// attribute them to, so they cannot be the cause of a monoculture. +func partitionPerSeed(pool []ArtistSuggestion, maxPerSeed int) (preferred, overflow []ArtistSuggestion) { + perSeed := make(map[pgtype.UUID]int, len(pool)) + preferred = make([]ArtistSuggestion, 0, len(pool)) + for _, s := range pool { + if len(s.Attribution) == 0 { + preferred = append(preferred, s) + continue + } + dominant := s.Attribution[0].ArtistID + if perSeed[dominant] >= maxPerSeed { + overflow = append(overflow, s) + continue + } + perSeed[dominant]++ + preferred = append(preferred, s) + } + return preferred, overflow +} + +func rotationKey(mbid, day string) string { + sum := md5.Sum([]byte(mbid + day)) + return hex.EncodeToString(sum[:]) } diff --git a/internal/recommendation/suggestions_select_test.go b/internal/recommendation/suggestions_select_test.go new file mode 100644 index 00000000..c73ad91c --- /dev/null +++ b/internal/recommendation/suggestions_select_test.go @@ -0,0 +1,170 @@ +package recommendation + +import ( + "fmt" + "testing" + + "github.com/jackc/pgx/v5/pgtype" +) + +// selectSuggestions is deliberately pure (no DB, no clock), so the rotation and +// diversity rules from slice #2373 are covered here in the fast lane rather +// than behind the integration gate. + +func seedID(n byte) pgtype.UUID { + var u pgtype.UUID + u.Bytes[0] = n + u.Valid = true + return u +} + +// candidate builds a pool entry attributed to the given dominant seed. +func candidate(mbid string, score float64, dominant byte) ArtistSuggestion { + return ArtistSuggestion{ + MBID: mbid, + Name: mbid, + Score: score, + Attribution: []SeedContribution{ + {ArtistID: seedID(dominant), Contribution: score}, + }, + } +} + +// poolOf returns n candidates in descending score order, spread across +// `seeds` distinct dominant seeds. +func poolOf(n int, seeds int) []ArtistSuggestion { + out := make([]ArtistSuggestion, 0, n) + for i := 0; i < n; i++ { + out = append(out, candidate(fmt.Sprintf("mbid-%02d", i), 1.0-float64(i)*0.01, byte(i%seeds))) + } + return out +} + +func mbids(in []ArtistSuggestion) []string { + out := make([]string, 0, len(in)) + for _, s := range in { + out = append(out, s.MBID) + } + return out +} + +func TestSelectSuggestions_ReturnsPoolUnchangedWhenNotOverfetched(t *testing.T) { + pool := poolOf(5, 5) + got := selectSuggestions(pool, 12, "2026-08-01") + if len(got) != 5 { + t.Fatalf("len = %d, want 5 (nothing to rotate)", len(got)) + } + if got[0].MBID != "mbid-00" { + t.Errorf("first = %q, want mbid-00", got[0].MBID) + } +} + +func TestSelectSuggestions_FillsTheRequestedLimit(t *testing.T) { + got := selectSuggestions(poolOf(48, 8), 12, "2026-08-01") + if len(got) != 12 { + t.Fatalf("len = %d, want 12", len(got)) + } +} + +// The reported symptom: the deck must change day to day without the user +// requesting anything. +func TestSelectSuggestions_RotatesAcrossDays(t *testing.T) { + pool := poolOf(48, 8) + day1 := mbids(selectSuggestions(pool, 12, "2026-08-01")) + day2 := mbids(selectSuggestions(pool, 12, "2026-08-02")) + + if fmt.Sprint(day1) == fmt.Sprint(day2) { + t.Fatalf("deck identical across days: %v", day1) + } + // ...but stable WITHIN a day, or the surface would reshuffle on every + // pull-to-refresh, which reads as broken rather than fresh. + again := mbids(selectSuggestions(pool, 12, "2026-08-01")) + if fmt.Sprint(day1) != fmt.Sprint(again) { + t.Errorf("same day differed:\n %v\n %v", day1, again) + } +} + +// The strongest matches should never rotate out of sight. +func TestSelectSuggestions_HeadIsStableTopScorers(t *testing.T) { + pool := poolOf(48, 8) + for _, day := range []string{"2026-08-01", "2026-08-02", "2026-09-15"} { + got := selectSuggestions(pool, 12, day) + if got[0].MBID != "mbid-00" { + t.Errorf("day %s: first = %q, want mbid-00 (top score leads)", day, got[0].MBID) + } + } +} + +// #2367 mechanism 4: all twelve slots could be neighbours of one artist. +// +// The pool is deliberately SKEWED — one seed owns the entire top of the score +// ranking — because that is the only shape where a cap can bite. With scores +// spread evenly across seeds the top-N is already diverse and the cap is +// untestable (an earlier version of this test asserted against an even pool and +// could not fail). +func TestSelectSuggestions_CapsOneDominantSeed(t *testing.T) { + const dominantRun = 20 + pool := make([]ArtistSuggestion, 0, 48) + for i := 0; i < dominantRun; i++ { // seed 0 owns the 20 best scores + pool = append(pool, candidate(fmt.Sprintf("dom-%02d", i), 1.0-float64(i)*0.01, 0)) + } + for i := 0; i < 28; i++ { // seeds 1..9 hold everything below + pool = append(pool, candidate(fmt.Sprintf("oth-%02d", i), 0.80-float64(i)*0.01, byte(1+i%9))) + } + + got := selectSuggestions(pool, 12, "2026-08-01") + if len(got) != 12 { + t.Fatalf("len = %d, want 12", len(got)) + } + + perSeed := map[pgtype.UUID]int{} + for _, s := range got { + perSeed[s.Attribution[0].ArtistID]++ + } + // Uncapped, the top 12 by score would be 12 of seed 0's neighbours. + if n := perSeed[seedID(0)]; n > maxPerSeedFor(12) { + t.Errorf("dominant seed owns %d of 12 slots, want <= %d: %v", + n, maxPerSeedFor(12), mbids(got)) + } + if len(perSeed) < 4 { + t.Errorf("deck spans only %d seeds, want >= 4: %v", len(perSeed), mbids(got)) + } +} + +// Diversity must not starve the deck (rule #131): a pool hanging off a single +// seed should still fill, not collapse to maxPerSeed entries. +func TestSelectSuggestions_SingleSeedPoolStillFills(t *testing.T) { + got := selectSuggestions(poolOf(30, 1), 12, "2026-08-01") + if len(got) != 12 { + t.Fatalf("len = %d, want 12 (cap must not starve the deck)", len(got)) + } + if got[0].MBID != "mbid-00" { + t.Errorf("first = %q, want mbid-00", got[0].MBID) + } +} + +// A candidate with no attribution has no seed to blame, so it must never be +// held back by the diversity cap. +func TestPartitionPerSeed_UnattributedNeverCapped(t *testing.T) { + pool := []ArtistSuggestion{ + candidate("a", 0.9, 1), + candidate("b", 0.8, 1), + {MBID: "orphan", Score: 0.1}, + } + preferred, overflow := partitionPerSeed(pool, 1) + if len(preferred) != 2 || preferred[1].MBID != "orphan" { + t.Errorf("preferred = %v, want [a orphan]", mbids(preferred)) + } + if len(overflow) != 1 || overflow[0].MBID != "b" { + t.Errorf("overflow = %v, want [b]", mbids(overflow)) + } +} + +func TestPoolSizeFor_OverfetchesButIsBounded(t *testing.T) { + if got := poolSizeFor(12); got != 48 { + t.Errorf("poolSizeFor(12) = %d, want 48", got) + } + if got := poolSizeFor(50); got != suggestionPoolMax { + t.Errorf("poolSizeFor(50) = %d, want the %d ceiling", got, suggestionPoolMax) + } +}