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) + } +}