fix(recommendation): make the candidate draw reproducible, not accidentally so
test-go / test (push) Successful in 1m18s
test-go / integration (push) Successful in 4m52s
release / Build signed APK (releases and dev) (push) Successful in 6m9s
release / Build + push container image (push) Successful in 2m5s
release / Verify release artifacts (tag releases only) (push) Skipped
test-go / test (push) Successful in 1m18s
test-go / integration (push) Successful in 4m52s
release / Build signed APK (releases and dev) (push) Successful in 6m9s
release / Build + push container image (push) Successful in 2m5s
release / Verify release artifacts (tag releases only) (push) Skipped
Four arms of the candidate query ended in a bare `ORDER BY random()` with no seed: similar_artists, likes_overlap, coplay_artists and random_fill. 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 order stops mattering, because scoreAndSortCandidates sorts by track id before drawing jitter. Below that threshold it returns a random SUBSET, and two builds on the same day draw different ones. So daily determinism held BY ACCIDENT, and only for libraries smaller than the limits. Any real library is larger, which means same-day rebuilds have been producing different mixes since those arms were written — invisible, because a mix that changes after a refresh looks like a feature rather than a broken promise. Found by breaking it: cutting RandomFill to 10 while tuning Songs-like turned TestBuildSystemPlaylists_DailyNonceDeterminism red. That test seeds ~20 tracks against a default RandomFill of 30, so its determinism came from the limit exceeding the library, not from the code being right. It is now a real guard. The arms order by md5(id || $12) instead. The CALLER decides what that means, which is the point: system mixes pass a per-(user, day) seed and get the determinism they promise, radio passes a fresh value per request and keeps varying, which is what a radio should do. Same shape the browse queries in this file already use (`md5(id::text || current_date::text)`) — existing idiom, not a new one. This also unblocks the trim that #3881 wanted and could not have. Shrinking a randomly-ordered arm was what broke membership; a seeded one takes a smaller but REPRODUCIBLE slice. Songs-like's seed-independent share drops from 29% to 12%, which was the original intent before determinism forced it back to 20%. TestSongsLikeLimits_DoNotShrinkTheUnseededRandomArms is DELETED rather than kept passing. It existed to stop anyone trimming those arms while the ordering was broken; the ordering is fixed, so the constraint is gone and a guard enforcing it would now forbid correct code. Was filed as blocked on tooling. It was not: `make generate-go` runs sqlc as a pinned Go tool and is the same path CI takes. One thing worth knowing for next time: three files in internal/db/dbq are owned by root, left by `make generate` running sqlc in Docker. sqlc errored on the first it could not write. They are untouched by this change and the regeneration of recommendation.sql.go completed, but `make generate` will keep failing until they are chowned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH
This commit is contained in:
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user