From ecfa056d4d5d52014ff54b4ec916fd21d4d032ef Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 10 Sep 2026 20:59:39 -0400 Subject: [PATCH] fix(recommendation): don't shrink a candidate arm ordered by unseeded random() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the integration failure from f367eeaa: "same-day rebuild produced different track lists". Cutting RandomFill 30→10 for Songs-like broke TestBuildSystemPlaylists_DailyNonceDeterminism, and the reason is worth stating because the number is not the bug. `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 rows eligible for it — then it returns all of them, and the random order stops mattering because scoreAndSortCandidates sorts by track id before drawing jitter. Below that threshold the arm returns a random SUBSET, and two builds on the same day draw different ones. So the test was green by accident. It seeds ~20 tracks against a default RandomFill of 30; the limit exceeded the library, so the arm returned everything. Determinism held for a reason unrelated to the code being right. Which means it does NOT hold in production. Any real library is larger than 30, so same-day rebuilds have been drawing different mixes since that arm was written — invisible, because a mix changing after a refresh looks like a feature. Filed as #3889; the fix is a seeded ordering per (user, day), which needs a .sql change and sqlc regeneration and so cannot land from here. The correction: grow an arm freely, never shrink one whose ordering is unseeded random. LikesOverlap and RandomFill go back to the defaults; TasteOverlap stays halved because it sorts by `tpa.weight DESC, t.id` and is genuinely deterministic. Guarded by a test that names the reasoning, so the next person to trim these has to read why first — and it should be DELETED once #3889 lands rather than worked around. The cost is honest: the seed-independent share of the Songs-like pool falls from 29% to 20% instead of the intended cut. That matters less than it sounds. The pool only biases the draw; the songs_like WEIGHTS are what actually demote sim_score-0 candidates, and they are untouched here — a perfect match still scores 5.00 against an unrelated favourite's 2.00. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- internal/recommendation/candidates.go | 46 ++++++++++++++----- .../recommendation/songs_like_limits_test.go | 38 +++++++++++++++ 2 files changed, 73 insertions(+), 11 deletions(-) diff --git a/internal/recommendation/candidates.go b/internal/recommendation/candidates.go index d5f2de86..c2d2a4ac 100644 --- a/internal/recommendation/candidates.go +++ b/internal/recommendation/candidates.go @@ -139,22 +139,46 @@ 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". // -// 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:108) rather than -// measuring anything. It is a collaborative signal wearing similarity's -// clothes, and raising SimilarityWeight amplifies it — if real ListenBrainz -// scores commonly land below 0.6 it would outrank genuine matches. Halved -// here pending the fill-rate measurement in #3879; the honest fix is to stop -// it claiming a similarity score it did not compute. +// 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. +// +// `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. +// +// 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. +// +// 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). func SongsLikeCandidateSourceLimits() CandidateSourceLimits { return CandidateSourceLimits{ LBSimilar: 60, // tier 1 — doubled; the only arm that measures the seed - SimilarArtist: 40, // tier 2 + SimilarArtist: 40, // tier 2 — raised; growing is always safe TagOverlap: 20, // tier 2 UserCoplay: 20, // tier 2 - LikesOverlap: 10, // tier 2, halved — flat 0.6, see above - TasteOverlap: 10, // tier 3 floor — halved, not removed - RandomFill: 10, // tier 3 floor — cut hard, never to zero + 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 } } diff --git a/internal/recommendation/songs_like_limits_test.go b/internal/recommendation/songs_like_limits_test.go index 8e8bce87..568df1f5 100644 --- a/internal/recommendation/songs_like_limits_test.go +++ b/internal/recommendation/songs_like_limits_test.go @@ -59,3 +59,41 @@ 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) + } + } +}