diff --git a/internal/api/radio.go b/internal/api/radio.go index 218dadd8..d8558bb8 100644 --- a/internal/api/radio.go +++ b/internal/api/radio.go @@ -101,6 +101,10 @@ func (h *handlers) handleRadio(w http.ResponseWriter, r *http.Request) { candidates, err := recommendation.LoadCandidatesFromSimilarity( r.Context(), q, user.ID, seedID, h.recCfg.RecentlyPlayedHours, currentVec, exclude, limits, + // A fresh seed per request (#3889): radio is a new session each time + // and SHOULD draw differently. The system mixes are the surfaces that + // promise repeatability; this is not one of them. + strconv.FormatInt(time.Now().UnixNano(), 36), ) if err != nil { h.logger.Warn("api: radio: similarity-pool failed; falling back to whole-library", "err", err) diff --git a/internal/db/dbq/recommendation.sql.go b/internal/db/dbq/recommendation.sql.go index f290ec35..bd798644 100644 --- a/internal/db/dbq/recommendation.sql.go +++ b/internal/db/dbq/recommendation.sql.go @@ -829,7 +829,7 @@ similar_artists AS ( JOIN seed_artist sa ON asim.artist_a_id = sa.artist_id WHERE asim.source = 'listenbrainz' AND t.id NOT IN (SELECT id FROM excluded_ids) - ORDER BY asim.score DESC, random() + ORDER BY asim.score DESC, md5(t.id::text || $12::text) LIMIT $6 ), tag_overlap AS ( @@ -857,7 +857,7 @@ likes_overlap AS ( WHERE t.id = gl.track_id AND trim(g_overlap.g) IN (SELECT tag FROM seed_tags) ) - ORDER BY random() + ORDER BY md5(gl.track_id::text || $12::text) LIMIT $8 ), taste_overlap AS ( @@ -884,7 +884,7 @@ coplay_artists AS ( WHERE asim.source = 'user_cooccurrence' AND t.id NOT IN (SELECT id FROM excluded_ids) AND t.id <> $2 - ORDER BY asim.score DESC, random() + ORDER BY asim.score DESC, md5(t.id::text || $12::text) LIMIT $11 ), random_fill AS ( @@ -900,7 +900,7 @@ random_fill AS ( UNION SELECT track_id FROM taste_overlap UNION SELECT track_id FROM coplay_artists ) - ORDER BY random() + ORDER BY md5(t.id::text || $12::text) LIMIT $9 ) SELECT @@ -938,17 +938,18 @@ GROUP BY t.id, t.title, t.album_id, t.artist_id, t.duration_ms, t.file_path, ` type LoadRadioCandidatesV2Params struct { - UserID pgtype.UUID - ID pgtype.UUID - Column3 interface{} - Column4 []pgtype.UUID - Limit int32 - Limit_2 int32 - Limit_3 int32 - Limit_4 int32 - Limit_5 int32 - Limit_6 int32 - Limit_7 int32 + UserID pgtype.UUID + ID pgtype.UUID + Column3 interface{} + Column4 []pgtype.UUID + Limit int32 + Limit_2 int32 + Limit_3 int32 + Limit_4 int32 + Limit_5 int32 + Limit_6 int32 + Limit_7 int32 + Column12 string } type LoadRadioCandidatesV2Row struct { @@ -971,8 +972,22 @@ type LoadRadioCandidatesV2Row struct { // enter the pool even when the similarity/random arms miss them; scored // in Go via TasteMatch, so sim_score here is 0 pool-inclusion), // $11 coplay_artists K (#1533 — tracks by artists co-played across the -// instance with the seed's artist; source='user_cooccurrence'). +// instance with the seed's artist; source='user_cooccurrence'), +// $12 order_seed (text) — see below. // +// $12 REPLACES `ORDER BY random()` IN FOUR ARMS (#3889). Those arms returned +// a stable set only while their LIMIT exceeded the rows eligible for them: at +// that point they returned all of them and the order stopped mattering, +// because the caller sorts by track id before scoring. Below that threshold +// they returned a random SUBSET, and two builds on the same day drew +// different ones — so "daily determinism" held by accident, and only for +// libraries smaller than the limits. +// +// md5(id || seed) keeps the intent — an arbitrary spread that changes when +// the seed does — while making it reproducible for a given seed. The CALLER +// decides what that means: system mixes pass a per-(user, day) string and get +// the determinism they promise; radio passes a fresh value per request and +// keeps varying, which is what a radio should do. // Returns same shape as LoadRadioCandidates plus similarity_score column. func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandidatesV2Params) ([]LoadRadioCandidatesV2Row, error) { rows, err := q.db.Query(ctx, loadRadioCandidatesV2, @@ -987,6 +1002,7 @@ func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandid arg.Limit_5, arg.Limit_6, arg.Limit_7, + arg.Column12, ) if err != nil { return nil, err diff --git a/internal/db/queries/recommendation.sql b/internal/db/queries/recommendation.sql index 20fbf7f7..7d9bfa87 100644 --- a/internal/db/queries/recommendation.sql +++ b/internal/db/queries/recommendation.sql @@ -45,7 +45,22 @@ WHERE t.id <> $2 -- enter the pool even when the similarity/random arms miss them; scored -- in Go via TasteMatch, so sim_score here is 0 pool-inclusion), -- $11 coplay_artists K (#1533 — tracks by artists co-played across the --- instance with the seed's artist; source='user_cooccurrence'). +-- instance with the seed's artist; source='user_cooccurrence'), +-- $12 order_seed (text) — see below. +-- +-- $12 REPLACES `ORDER BY random()` IN FOUR ARMS (#3889). Those arms returned +-- a stable set only while their LIMIT exceeded the rows eligible for them: at +-- that point they returned all of them and the order stopped mattering, +-- because the caller sorts by track id before scoring. Below that threshold +-- they returned a random SUBSET, and two builds on the same day drew +-- different ones — so "daily determinism" held by accident, and only for +-- libraries smaller than the limits. +-- +-- md5(id || seed) keeps the intent — an arbitrary spread that changes when +-- the seed does — while making it reproducible for a given seed. The CALLER +-- decides what that means: system mixes pass a per-(user, day) string and get +-- the determinism they promise; radio passes a fresh value per request and +-- keeps varying, which is what a radio should do. -- Returns same shape as LoadRadioCandidates plus similarity_score column. WITH @@ -87,7 +102,7 @@ similar_artists AS ( JOIN seed_artist sa ON asim.artist_a_id = sa.artist_id WHERE asim.source = 'listenbrainz' AND t.id NOT IN (SELECT id FROM excluded_ids) - ORDER BY asim.score DESC, random() + ORDER BY asim.score DESC, md5(t.id::text || $12::text) LIMIT $6 ), tag_overlap AS ( @@ -115,7 +130,7 @@ likes_overlap AS ( WHERE t.id = gl.track_id AND trim(g_overlap.g) IN (SELECT tag FROM seed_tags) ) - ORDER BY random() + ORDER BY md5(gl.track_id::text || $12::text) LIMIT $8 ), taste_overlap AS ( @@ -142,7 +157,7 @@ coplay_artists AS ( WHERE asim.source = 'user_cooccurrence' AND t.id NOT IN (SELECT id FROM excluded_ids) AND t.id <> $2 - ORDER BY asim.score DESC, random() + ORDER BY asim.score DESC, md5(t.id::text || $12::text) LIMIT $11 ), random_fill AS ( @@ -158,7 +173,7 @@ random_fill AS ( UNION SELECT track_id FROM taste_overlap UNION SELECT track_id FROM coplay_artists ) - ORDER BY random() + ORDER BY md5(t.id::text || $12::text) LIMIT $9 ) SELECT diff --git a/internal/playlists/system.go b/internal/playlists/system.go index f0708793..420c0811 100644 --- a/internal/playlists/system.go +++ b/internal/playlists/system.go @@ -276,6 +276,17 @@ func SetTasteConfig(c taste.Config) { systemTasteConfig = c } +// dailyOrderSeed is the value the randomised candidate arms order by (#3889). +// +// Per (user, day) so a same-day rebuild draws the SAME set — which is what +// TestBuildSystemPlaylists_DailyNonceDeterminism asserts and what those arms +// only ever achieved by accident before, when their limits happened to exceed +// the eligible rows. It changes on the day boundary, so the mixes still move +// daily. +func dailyOrderSeed(userID pgtype.UUID, dateStr string) string { + return uuidStringPL(userID) + ":" + dateStr +} + func currentSongsLikeWeights() recommendation.ScoringWeights { systemTuningMu.RLock() defer systemTuningMu.RUnlock() @@ -629,6 +640,7 @@ func produceForYou( zeroVec, seeds, systemForYouSourceLimits(), + dailyOrderSeed(userID, dateStr), ) if cerr != nil { logger.Warn("system playlist: for-you candidates load failed for seed; continuing", @@ -716,6 +728,7 @@ func produceSeedMixes( recommendation.ScaleForLibrary( recommendation.SongsLikeCandidateSourceLimits(), librarySize, ), + dailyOrderSeed(userID, dateStr), ) if cerr != nil { logger.Warn("system playlist: seed candidates load failed; skipping", diff --git a/internal/playlists/you_might_like.go b/internal/playlists/you_might_like.go index 27ccaf58..2e4d4bc1 100644 --- a/internal/playlists/you_might_like.go +++ b/internal/playlists/you_might_like.go @@ -102,6 +102,7 @@ func buildYouMightLike( cands, err := recommendation.LoadCandidatesFromSimilarity( ctx, q, userID, seed, 1, zeroVec, []pgtype.UUID{seed}, ymlLimits, + dailyOrderSeed(userID, dateStr), ) if err != nil { logger.Warn("you-might-like: candidate load failed; skipping", diff --git a/internal/recommendation/candidates.go b/internal/recommendation/candidates.go index c2d2a4ac..ecd1d7e2 100644 --- a/internal/recommendation/candidates.go +++ b/internal/recommendation/candidates.go @@ -139,46 +139,41 @@ func DefaultCandidateSourceLimits() CandidateSourceLimits { // would produce a short mix or none at all, and "no playlist" is a worse // answer than "a few tracks further from the seed than we would like". // -// DO NOT SHRINK AN ARM ORDERED BY UNSEEDED random(). This is the constraint -// that shapes the numbers below, and it is not obvious from reading them. +// The seed-independent arms are trimmed hardest, because on this surface they +// are noise: `taste_overlap` (tracks by the user's top taste artists) and +// `random_fill` (any track not already in the pool) both carry +// `0.0::float8 AS sim_score`, so nearly a third of the default pool had no +// relationship to the seed at all. // -// `likes_overlap` and `random_fill` both end in a bare `ORDER BY random()` -// (recommendation.sql:118, :161) with no daily seed. Such an arm returns a -// STABLE set only while its LIMIT exceeds the rows eligible for it — at that -// point it returns all of them and the random order is irrelevant, because -// the caller sorts by id before scoring. Drop the limit below the eligible -// count and the arm starts returning a random SUBSET, which differs between -// two builds on the same day. +// THESE TRIMS WERE BLOCKED UNTIL #3889. `likes_overlap` and `random_fill` +// used to end in a bare `ORDER BY random()`, which made their output a stable +// SET only while the limit exceeded the eligible rows — so SHRINKING them +// changed pool membership between same-day rebuilds and broke daily +// determinism. Those arms now order by md5(id || seed), so a smaller limit +// takes a smaller but REPRODUCIBLE slice, and the trim is safe. // -// That is a real defect (#3889) rather than a quirk of this function, and it -// bit here: cutting RandomFill to 10 broke -// TestBuildSystemPlaylists_DailyNonceDeterminism, whose library is smaller -// than the default limit and whose determinism was therefore accidental. -// Growing an arm is always safe; only shrinking one is. +// Reduced, never removed. Rule 131: the two seed-independent arms are the +// tier-3 floor, and zeroing them would leave a seed with thin ListenBrainz +// coverage producing a short mix or none at all. The weights (SimilarityWeight +// 4.0, everything seed-independent demoted) keep them ranked last, so they +// surface only when the closer tiers cannot fill the mix. // -// So the seed-independent arms are trimmed only where the ordering is -// deterministic: `taste_overlap` sorts by `tpa.weight DESC, t.id` and can be -// cut, `random_fill` cannot. The reduction is consequently modest — and it -// matters less than it looks, because the WEIGHTS are what demote sim_score-0 -// candidates now. The pool change biases the draw; the songs_like profile is -// what actually keeps unrelated tracks out of the result. -// -// One arm is left alone that arguably should not be: `likes_overlap` assigns -// a FLAT 0.6 sim_score (recommendation.sql:108) rather than measuring -// anything — a collaborative signal wearing similarity's clothes, which a -// raised SimilarityWeight amplifies. If real ListenBrainz scores commonly -// land below 0.6 it will outrank genuine matches. It cannot be trimmed here -// without the determinism fix landing first; the honest repair is to stop it -// claiming a similarity score it never computed (#3879). +// likes_overlap is cut hardest of the tier-2 arms for a specific reason: its +// SQL assigns a FLAT 0.6 sim_score (recommendation.sql) rather than measuring +// anything. It is a collaborative signal wearing similarity's clothes, and a +// raised SimilarityWeight amplifies it — if real ListenBrainz scores commonly +// land below 0.6 it would outrank genuine matches. Halved pending the +// fill-rate measurement in #3879; the honest fix is to stop it claiming a +// similarity score it never computed. func SongsLikeCandidateSourceLimits() CandidateSourceLimits { return CandidateSourceLimits{ LBSimilar: 60, // tier 1 — doubled; the only arm that measures the seed SimilarArtist: 40, // tier 2 — raised; growing is always safe TagOverlap: 20, // tier 2 UserCoplay: 20, // tier 2 - LikesOverlap: 20, // tier 2 — NOT trimmed: unseeded random(), see above - TasteOverlap: 10, // tier 3 floor — halved; deterministic ordering, safe - RandomFill: 30, // tier 3 floor — NOT trimmed: unseeded random(), see above + LikesOverlap: 10, // tier 2, halved — flat 0.6 sim_score, see above + TasteOverlap: 10, // tier 3 floor — halved, not removed + RandomFill: 10, // tier 3 floor — cut hard, never to zero } } @@ -187,6 +182,10 @@ func SongsLikeCandidateSourceLimits() CandidateSourceLimits { // likes-overlap / random fill) + dedup-by-max sim_score. Returns // []Candidate (same shape as LoadCandidates) so Shuffle is unchanged. // +// orderSeed decides whether the randomised arms repeat their draw — see +// Column12 below and #3889. Pass a stable per-(user, day) value where the +// selection must be reproducible, and a varying one where it should not be. +// // Caller (radio handler) falls back to LoadCandidates on error. func LoadCandidatesFromSimilarity( ctx context.Context, @@ -196,6 +195,7 @@ func LoadCandidatesFromSimilarity( currentVector SessionVector, exclude []pgtype.UUID, limits CandidateSourceLimits, + orderSeed string, ) ([]Candidate, error) { if exclude == nil { exclude = []pgtype.UUID{} @@ -212,6 +212,12 @@ func LoadCandidatesFromSimilarity( Limit_5: int32(limits.RandomFill), Limit_6: int32(limits.TasteOverlap), Limit_7: int32(limits.UserCoplay), + // #3889. Four arms used to end in a bare ORDER BY random(), which made + // their output a stable SET only while the limit exceeded the eligible + // rows. They now order by md5(id || this), so the caller decides + // whether the draw repeats: a per-(user, day) seed for the system + // mixes that promise daily determinism, a fresh one per radio request. + Column12: orderSeed, }) if err != nil { return nil, err diff --git a/internal/recommendation/candidates_v2_test.go b/internal/recommendation/candidates_v2_test.go index 0fa19b55..af654157 100644 --- a/internal/recommendation/candidates_v2_test.go +++ b/internal/recommendation/candidates_v2_test.go @@ -2,6 +2,10 @@ package recommendation import ( "context" + "fmt" + "reflect" + "sort" + "strings" "testing" "github.com/jackc/pgx/v5/pgtype" @@ -50,7 +54,7 @@ func TestLoadCandidatesFromSimilarity_LBSimilarSourceContributes(t *testing.T) { target := f.tracks[1] helperLBSimilarity(t, f, seed.ID, target.ID, 0.85) got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -82,7 +86,7 @@ func TestLoadCandidatesFromSimilarity_SimilarArtistTracksContribute(t *testing.T }) helperArtistSimilarity(t, f, seed.ArtistID, otherArtist.ID, 0.8) got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -106,7 +110,7 @@ func TestLoadCandidatesFromSimilarity_TagOverlapContributes(t *testing.T) { helperSetTrackGenre(t, f, seed.ID, "Rock; Pop") helperSetTrackGenre(t, f, target.ID, "Rock") got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -133,7 +137,7 @@ func TestLoadCandidatesFromSimilarity_LikesOverlapContributes(t *testing.T) { t.Fatalf("like: %v", err) } got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -155,7 +159,7 @@ func TestLoadCandidatesFromSimilarity_RandomFillReturnsTracks(t *testing.T) { f := newFixture(t, 10) // 10 tracks; no similarity data seed := f.tracks[0] got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -176,7 +180,7 @@ func TestLoadCandidatesFromSimilarity_ExcludeListRespected(t *testing.T) { excluded := f.tracks[1].ID got, err := LoadCandidatesFromSimilarity( context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, - []pgtype.UUID{excluded}, defaultLimits(), + []pgtype.UUID{excluded}, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -192,7 +196,7 @@ func TestLoadCandidatesFromSimilarity_SeedAlwaysExcluded(t *testing.T) { f := newFixture(t, 5) seed := f.tracks[0] got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -222,7 +226,7 @@ func TestLoadCandidatesFromSimilarity_RecentlyPlayedExcluded(t *testing.T) { t.Fatalf("play_event: %v", err) } got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -242,7 +246,7 @@ func TestLoadCandidatesFromSimilarity_DedupTakesMaxScore(t *testing.T) { helperSetTrackGenre(t, f, target.ID, "Rock") // jaccard 1/1 = 1.0 from tag-overlap helperLBSimilarity(t, f, seed.ID, target.ID, 0.5) // weaker LB signal got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -295,7 +299,7 @@ func TestLoadCandidatesFromSimilarity_TasteOverlapArm(t *testing.T) { // Only the taste_overlap arm is enabled. limits := CandidateSourceLimits{TasteOverlap: 10} got, err := LoadCandidatesFromSimilarity( - ctx, f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, limits, + ctx, f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, limits, "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -321,7 +325,7 @@ func TestLoadCandidatesFromSimilarity_EmptyLibrary_NoError(t *testing.T) { f := newFixture(t, 1) // just the seed seed := f.tracks[0] got, err := LoadCandidatesFromSimilarity( - context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, defaultLimits(), "test-seed", ) if err != nil { t.Fatalf("load: %v", err) @@ -331,3 +335,97 @@ func TestLoadCandidatesFromSimilarity_EmptyLibrary_NoError(t *testing.T) { t.Errorf("got %d candidates from seed-only library, want 0", len(got)) } } + +// The randomised arms must draw REPRODUCIBLY for a given seed (#3889). +// +// Four arms used to end in a bare `ORDER BY random()`. That returned a stable +// set only while the arm's LIMIT exceeded the rows eligible for it — at that +// point it returned all of them and the order stopped mattering, because the +// caller sorts by track id before scoring. Below that threshold it returned a +// random SUBSET, so two calls drew different candidates. +// +// It therefore held by ACCIDENT, and only for libraries smaller than the +// limits. Any real library is larger, so same-day rebuilds had been drawing +// different mixes since the arm was written — invisible, because a mix that +// changes after a refresh looks like a feature. +// +// Limits deliberately smaller than the fixture, because that is the only +// regime where the bug existed at all: with limits above the eligible count +// the old code passes this too. +func TestLoadCandidatesFromSimilarity_SameSeedDrawsTheSameSet(t *testing.T) { + f := newFixture(t, 12) + seed := f.tracks[0] + + tight := CandidateSourceLimits{ + LBSimilar: 2, SimilarArtist: 2, TagOverlap: 2, + LikesOverlap: 2, RandomFill: 3, TasteOverlap: 2, UserCoplay: 2, + } + ids := func(cs []Candidate) []string { + out := make([]string, 0, len(cs)) + for _, c := range cs { + out = append(out, fmt.Sprintf("%x", c.Track.ID.Bytes)) + } + sort.Strings(out) // membership, not order — order is settled downstream + return out + } + + first, err := LoadCandidatesFromSimilarity( + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, tight, "day-one", + ) + if err != nil { + t.Fatalf("load: %v", err) + } + if len(first) == 0 { + t.Fatal("no candidates, so this test asserts nothing") + } + + for i := 0; i < 3; i++ { + again, err := LoadCandidatesFromSimilarity( + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, tight, "day-one", + ) + if err != nil { + t.Fatalf("load %d: %v", i, err) + } + if !reflect.DeepEqual(ids(first), ids(again)) { + t.Fatalf("same seed drew a different set on call %d:\n first %v\n again %v", + i, ids(first), ids(again)) + } + } +} + +// ...and a different seed is free to draw differently, or the ordering would +// be fixed rather than seeded and every day would serve the same mix. +// +// Asserted as "not pinned to one answer" rather than "always differs": with a +// small fixture two seeds can legitimately collide, so requiring a difference +// on any single pair would be flaky. Several seeds producing exactly one +// distinct set is the real regression — that is what a constant ORDER BY +// looks like. +func TestLoadCandidatesFromSimilarity_DifferentSeedsCanDrawDifferently(t *testing.T) { + f := newFixture(t, 12) + seed := f.tracks[0] + tight := CandidateSourceLimits{ + LBSimilar: 2, SimilarArtist: 2, TagOverlap: 2, + LikesOverlap: 2, RandomFill: 3, TasteOverlap: 2, UserCoplay: 2, + } + + seen := map[string]bool{} + for _, orderSeed := range []string{"a", "b", "c", "d", "e", "f"} { + cs, err := LoadCandidatesFromSimilarity( + context.Background(), f.q, f.user, seed.ID, 1, SessionVector{Seed: true}, nil, tight, orderSeed, + ) + if err != nil { + t.Fatalf("load %q: %v", orderSeed, err) + } + ids := make([]string, 0, len(cs)) + for _, c := range cs { + ids = append(ids, fmt.Sprintf("%x", c.Track.ID.Bytes)) + } + sort.Strings(ids) + seen[strings.Join(ids, ",")] = true + } + if len(seen) < 2 { + t.Errorf("six different seeds produced %d distinct set(s); the ordering is not "+ + "varying with the seed at all", len(seen)) + } +} diff --git a/internal/recommendation/songs_like_limits_test.go b/internal/recommendation/songs_like_limits_test.go index 568df1f5..8e8bce87 100644 --- a/internal/recommendation/songs_like_limits_test.go +++ b/internal/recommendation/songs_like_limits_test.go @@ -59,41 +59,3 @@ func TestSongsLikeLimits_KeepThePoolRoughlyTheSameSize(t *testing.T) { "this was meant to re-weight the pool, not starve it", s, d) } } - -// The constraint that is invisible in the numbers, and that this file exists -// to keep visible. -// -// `likes_overlap` and `random_fill` end in a bare `ORDER BY random()` with no -// daily seed (recommendation.sql:118, :161). Such an arm returns a stable set -// only while its LIMIT exceeds the eligible rows; below that it returns a -// random SUBSET that differs between two builds on the same day, and the -// daily-determinism promise quietly stops holding. -// -// This is not hypothetical — it is how this change first failed CI. Cutting -// RandomFill to 10 broke TestBuildSystemPlaylists_DailyNonceDeterminism, -// whose library is smaller than the default limit and whose determinism was -// therefore an accident of the limit exceeding the library. -// -// Growing these arms is always safe. Only shrinking is, and the fix that -// would make shrinking safe is a seeded ordering (#3889), not a smaller -// number here. -func TestSongsLikeLimits_DoNotShrinkTheUnseededRandomArms(t *testing.T) { - d := DefaultCandidateSourceLimits() - s := SongsLikeCandidateSourceLimits() - - for _, tc := range []struct { - arm string - songsLike, dflt int - }{ - {"RandomFill", s.RandomFill, d.RandomFill}, - {"LikesOverlap", s.LikesOverlap, d.LikesOverlap}, - } { - if tc.songsLike < tc.dflt { - t.Errorf("%s cut from %d to %d. That arm is ordered by unseeded random(), "+ - "so a smaller limit makes pool membership vary between same-day "+ - "rebuilds — it breaks daily determinism rather than merely narrowing "+ - "the mix. Fix the ordering (#3889) before trimming this.", - tc.arm, tc.dflt, tc.songsLike) - } - } -}