Recommendation relevance, the rollback unit, and a version that names what shipped #131

Merged

bvandeusen merged 11 commits from dev into main 2026-09-10 23:37:57 -04:00

Conversation 0 Commits 11 Files Changed 32

+2224 -208

11 Commits

Author	SHA1	Message	Date
bvandeusenandClaude Opus 5	4ce47397a9	fix(recommendation): a nil LibrarySize must degrade, not panic ... Fixes the integration failure from 72115484: a SIGSEGV inside handleRadio took down TestHandleRadio_ColdStart_OnlySeedReturned. recommendation.(*LibrarySize).Get(0x0, ...) library_scale.go:146 api.(*handlers).handleRadio(...) radio.go:95 internal/api builds its handlers struct directly in a dozen tests, none of which know about every field, so librarySize arrives nil there. Get took l.mu.Lock() straight off the nil receiver. The shape of the bug is what matters more than the nil check. This value's entire contract is that it degrades — an errored count keeps the last known number, a never-counted cache returns 0, and 0 scales to the base limits, i.e. today's behaviour. A pool-sizing HINT then turned a request into a crash, which is the precise opposite of that. A nil receiver is now VALID and means "no cache": the count still runs, it is just not memoised. Correct-but-uncached rather than zero, so a wiring miss in production would cost a query per request, not silently unscale every pool — a performance bug is findable, a quietly-wrong pool is not. Patching the test constructors was the alternative and is worse: a dozen call sites, and the next test to build a handlers literal reintroduces it. Guarded with the nil path exercised directly, including that it counts again rather than memoising, and still returns 0 on a failed count. The old shape fails it by panicking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH	2026-09-10 23:29:35 -04:00
bvandeusenandClaude Opus 5	721154847e	fix(recommendation): size the candidate pool to the library ... Operator, 2026-09-10: "is the pool that we draw from somehow scaled to the amount of music in the library... my earlier understanding of the tuning and work may have been skewed by what was in my library." It was not. DefaultCandidateSourceLimits returns what its own comment calls "the v1 hardcoded constants per spec" — ~170 candidates for a 500-track library and a 100,000-track one alike. The pool therefore samples a shrinking FRACTION of a growing collection: 17% of 1,000 tracks, 1.7% of 10,000, 0.17% of 100,000. RandomFill, whose whole job is exploration, becomes a thinner and noisier slice at exactly the moment a library gets more diverse — which is the "starting to feel weird" being reported. 1,000 tracks -> pool 170 (unchanged) 5,000 -> pool 170 (unchanged) 20,000 -> pool 280 80,000 -> pool 500 (ceiling) THE SCALING IS PER-ARM, and that is the substance rather than a refinement. A limit only matters if there are rows for it to cut off, so what an arm is BOUNDED BY decides whether library size can help it. LBSimilar, SimilarArtist, TagOverlap and RandomFill grow: they are bounded by similarity/tag data and by the library itself. LikesOverlap, UserCoplay and TasteOverlap do not: they are bounded by the user's likes, the instance's co-play graph and the taste profile, none of which grow when the library does. Raising those would sample more of a set that did not change — churn, not reach. It also keeps this from inflating the sim_score-0 share, since TasteOverlap is one of the two zero-similarity arms. sqrt, not linear: linear would put a 100,000-track library at a 3,400-candidate pool, long past where more candidates improve the answer. A 4x ceiling bounds it at ~500. Never shrinks an arm. The base limits are a floor, and #3889 makes that load-bearing rather than tidy — shrinking an arm ordered by unseeded random() changes pool membership between same-day rebuilds. Library size comes from a TTL-cached count reusing CountTracksMatching with an empty pattern (rule 28 — a new query would need sqlc regeneration, which is blocked). The ILIKE defeats every index, so it is a full scan and must not run per request. It degrades rather than fails: an error keeps the last known value, a never-counted cache returns 0, and 0 scales to the base limits — today's behaviour exactly. Nothing about sizing a pool justifies failing the request it is sizing. Bounded by a 3s deadline (rule 156), and a failed refresh does not stamp the clock, so a blip cannot pin a stale value for the whole TTL. THE REFERENCE IS ASSUMED, NOT MEASURED. libraryScaleReference = 5000 is where growth starts, and the size the v1 constants were really tuned against is unrecorded. #3879 should replace it; until then that constant is the one thing to change. Deliberately conservative: below it nothing scales at all, so no existing install changes behaviour. Falsification caught a weak guard: the sqrt-vs-linear assertion was written at SIXTEEN times the reference, where linear has already been clamped by the ceiling and both curves land on 4x. It proved nothing. Moved to four times the reference, below the ceiling for both, where sqrt gives 2x and linear would give 4x. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH	2026-09-10 23:23:04 -04:00
bvandeusenandClaude Opus 5	633d4f591f	fix(radio): cap any one artist's share of a radio session ... Operator, 2026-09-10: started radio from a song and "literally all of the songs in the playlist after that were from a single artist which was not expected." There was no per-artist cap anywhere in the radio path. radio.go built the pool and handed it straight to Shuffle, which scores, sorts and takes the top N — nothing between those steps bounded any artist's share, so a pool dominated by one artist produced an output dominated by it. The asymmetry was the tell: discover.go, you_might_like.go and home.go all cap; radio never got one. With the fixture that reproduces it — 20 liked tracks by one artist plus 10 by ten others — the old path returns 10 tracks from 1 artist. It now returns 10 from 8. TWO PASSES, and that is the whole design. A hard cap was the easy mistake: radio asks for 50 tracks by default and 200 at most, so capping at three per artist over a concentrated pool would hand back a six-track "radio". Pass one takes candidates that fit under the caps; pass two fills any remaining slots from those it skipped, still in score order. The result always holds min(limit, len(candidates)) — the caps change WHICH tracks are picked, never HOW MANY. Rule 131's principle past the system mixes it was written for. The caps SCALE with the requested length rather than being a constant. Three-per-artist is a sensible 12% of a 25-track mix and an absurd 1.5% of a 200-track radio, where every selection would sit in the relaxation path and the cap would be decorative. RadioDiversityCaps holds the system mixes' proportion at any length: 3/2 at 25, 6/4 at 50, 24/16 at 200, with floors so a very short radio is not capped down to one track per artist. A BOUND, NOT AN EXCLUSION — the operator asked for the opposite of removal: "again it should be able to add songs from the same artist." The dominant artist still appears, just not exclusively. Guarded, because the tempting wrong fix is the filter songs-like used to carry. Shuffle grew the parameter rather than gaining a capped twin: radio is its only production caller, so a second function would have left the original dead (rule 22). Falsified against each named regression: uncapped gives 10/10 to one artist; a hard cap returns 3 of 10 on a single-artist pool; a cap-as-exclusion drops the artist entirely; a fixed cap stays 3 where the scaled one reaches 24. Caught while writing the guards: the artist-key constant was hand-written hex and wrong — the fixture's artist UUID carries 0001 in its fourth group, so the lookup missed and the assertion measured nothing. Derived from the same construction the fixture uses now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH	2026-09-10 22:14:42 -04:00
bvandeusenandClaude Opus 5	f5dd4462de	test(playlists): the same-artist guard needed a fixture that has same artists ... Fixes the integration failure from 31190657. The test was wrong, not the code: it could not have passed whatever produceSeedMixes did. seedActiveLibrary builds its tracks through seedTrack, whose own comment says "artist and album are not deduplicated across calls (mbid-less upsert)". So every track gets a fresh artist row despite sharing a name — 4 artists x 5 tracks is really 20 artists with one track each. A seed artist's only track IS the seed, which is excluded from its own mix, so "does this mix contain a track by its seed artist" was structurally answerable only as no. That is the failure mode worth naming: the assertion was measuring the fixture, not the behaviour, and it reported the behaviour as broken. seedSharedArtistLibrary upserts each artist ONCE and reuses the id across its tracks, so a seed artist genuinely owns five others. Albums are still not deduplicated, which suits this test — the per-album cap never binds, so the per-artist cap (3) is unambiguously what is under test. Noted in the fixture, because "tidying" the album titles into something shared would silently change which cap the assertion measures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH	2026-09-10 21:25:49 -04:00
bvandeusenandClaude Opus 5	31190657d8	feat(recommendation): Songs-like can include the seed artist's own music ... Operator, 2026-09-10: "it should also be able to include music from the same artist." Completes #3881 — the weights and pool landed in f367eeaa; this is the eligibility half. produceSeedMixes filtered the seed artist out entirely: // "Songs like X" excludes X's own songs. if !pgtypeUUIDEqual(c.Track.ArtistID, artistID) { ... } That reads as obviously right and is not. The seed is a TRACK — the artist's top-played one — and the tracks most likely to sound like it are usually the rest of that artist's catalogue. The filter threw away the seed's nearest neighbours, then reached FURTHER OUT to replace them. On the one surface whose job is staying in a neighbourhood, that is backwards, and it worked against the coherence tuning rather than with it. Domination is bounded by the cap instead of by exclusion, which is the distinction that makes this safe rather than a new problem: capCandidatesByAlbumAndArtist already allows at most 3 tracks per artist in a 25-track mix, so the seed artist gets 12% at most — a presence, not a takeover. Without that bound this would just be the radio failure (#3882) arriving on a different surface. The seed track itself still cannot appear; it is passed to LoadCandidatesFromSimilarity as an exclusion. Guarded end-to-end rather than by reading the source, for two reasons: the check has to survive the filter returning in a different shape, and an absence check would now match the comment that explains why the filter is gone — rule 167's prose trap exactly. The test asserts both directions, that at least one mix contains its seed artist and that none exceeds the cap. Its falsification is by construction rather than by execution: under the previous code every mix's own-artist count was necessarily zero, so the assertion could not have passed. Running it needs Postgres, which is the integration lane's job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH	2026-09-10 21:13:32 -04:00
bvandeusenandClaude Opus 5	ecfa056d4d	fix(recommendation): don't shrink a candidate arm ordered by unseeded random() ... 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH	2026-09-10 20:59:39 -04:00
bvandeusenandClaude Opus 5	f367eeaa9d	fix(recommendation): Songs-like gets its own profile so it stops wandering ... Operator, 2026-09-10: "when I play it I'm expecting to get a consistent sound and style from the experience... I was getting a seeming wide variety of music from each one when I was hoping to stay in a certain neighborhood." Songs-like shared the `daily_mix` weight profile with For-You, and that sharing WAS the bug. The two surfaces want opposite things: For-You answers "what will they enjoy today" and is supposed to roam; Songs-like answers "what sounds like THIS". Under one profile the broad answer wins. The arithmetic, from the shared weights: unrelated track, liked, not played recently → 1.0 + 2.0 + 1.0 = 4.0 PERFECT similarity match, not liked → 1.0 + 1.5 = 2.5 Liking something outranked sounding like the seed, because LikeBoost (2.0) exceeded SimilarityWeight's whole range (1.5) and TasteWeight (1.5, and seed-INDEPENDENT) matched it outright. Under the new profile the same pair scores 5.00 vs 2.00. Two levers, because either alone leaves the other's failure intact: POOL. Songs-like now takes its own CandidateSourceLimits. The default gave ~29% of candidates a sim_score of literally zero — `taste_overlap` and `random_fill` are both `0.0::float8` in recommendation.sql, seed-independent by construction. Same total pool size; composition shifts to arms that measure distance from the seed, LBSimilar doubled. WEIGHTS. A third profile beside radio and daily_mix, DB-backed and live per rule 25, with the property that similarity's range exceeds the combined range of every seed-independent differentiator — so a closer match cannot be beaten on likes, freshness and taste alone, while tracks within ~0.39 similarity of each other still get ordered by what the user likes. Rule 131 changed the pool design mid-way and for the better. Zeroing the two seed-independent arms was the first instinct and is exactly the vanish-or-nothing shape that rule forbids: a seed with thin ListenBrainz coverage would yield a short mix or none. They are the tier-3 FLOOR — cut hard, never removed — and the weights keep them at the bottom of the ranking rather than out of the pool. "A few tracks further from the seed than we'd like" beats "no playlist". Caught while wiring it: switching only pickTopN's final Score would have been nearly INERT. scoreAndSortCandidates does the selection sort, and the caller caps and truncates in that order — so the playlist would still have been chosen by daily_mix and merely relabelled with songs_like numbers. It now takes the profile as a parameter, and each surface passes its own. Also corrects the daily_mix card's blurb, which claimed Songs-like as one of its surfaces and no longer is. Guards pin behaviour rather than the numbers, since numbers get retuned: that similarity beats an unrelated liked track, that daily_mix still DOESN'T (or the split buys nothing), that the tier-3 floor is non-zero, and that the UI card shows its own values rather than falling back. Each falsified against its named regression first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH	2026-09-10 20:51:15 -04:00
bvandeusenandClaude Opus 5	270ad7a71b	fix(ci): tests do not ship, so they must not re-version an artifact ... Completes the pathspec. 17212e9e excluded CI, docs and tooling but left tests in the shipped set, so its own commit re-versioned the image on the strength of a _test.go file. `go build` drops *_test.go outright and the Vite build never imports a .test.ts — neither reaches an image or an APK. Globs over files rather than a directory exclusion, because this repo has no tests/ tree to exclude: Go tests sit inline beside the code they cover (158 files) and the web suite beside its modules (115). Patterns match what exists and nothing speculative — there are no .spec.* files, no __tests__/ directories and no androidTest/ tree. If any appear they re-version until named, which is the harmless direction and the point of a denylist. The guard that matters is not "a test-only commit is inert" — it is that a commit touching a test AND its source still moves the version. `':!internal'` would satisfy every inertness assertion while silently excluding the entire server, which is the stale-version-on-changed-artifact failure this whole derivation exists to prevent. Falsified: drop the Go exclusion and a _test.go commit moves the version; drop the web one and a .test.ts does; replace the globs with `':!internal'` and the source-alongside-test case breaks. That last check failed first time, on a bug in the FIXTURE rather than the derivation, and it is worth recording because it makes a test pass for the wrong reason. Both commit helpers wrote the constant "x\n", so re-writing a file with identical bytes recorded NOTHING — the "source and test together" commit actually contained only the test, and the assertion was quietly checking the case it was meant to contrast against. Content is now derived from the commit's epoch, and the test asserts HEAD really contains both paths before drawing any conclusion from it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH	2026-09-10 18:21:12 -04:00
bvandeusenandClaude Opus 5	17212e9eb4	fix(ci): version derives from the shipped set; untrack an 18MB binary ... Three build-hygiene fixes that turned up while explaining the pathspec. **version.sh derives from what SHIPPED.** It read bare HEAD, so any commit moved the version — including one touching only CI or a README. Rules 148 and 149 both specify the pathspec form. Now a denylist, and the direction is the point: as an allowlist the list must be updated by whoever adds a directory and nothing fails if they don't, so the failure mode is a changed artifact keeping its old version silently on a green run. Inverted, new content counts by default. android/ is deliberately NOT excluded, and that is the subtle part. This repo ships TWO artifacts from ONE derivation: android/ is in no server image, but it is the APK's entire source, and excluding it would stop an Android-only commit from moving the APK's own version — the silent downgrade the versioning rework exists to prevent. So the list is the union: exclude only what ships in neither, and accept that an Android commit also nudges the server's reported version. Over-inclusion across the two, which is the harmless direction. roundtable/roundtable-android each keep tighter lists because they are one-artifact repos; don't copy theirs. **.dockerignore excluded the wrong CI directory.** It named .forgejo/ and .github/, neither of which this repo has. Gitea Actions reads .gitea/, so the one directory that exists was the one not excluded. The "Flutter mobile client" block had also lost its PATTERN when flutter_client/ was deleted, leaving a comment describing an exclusion that was not happening — android/ never took its place, so 4.1MB of Gradle project entered the context and busted the `COPY . .` layer on every Android-only change. bin/ excluded too. **bin/minstrel was tracked** — an 18MB binary last refreshed by a commit about web test mocks, and re-dirtied by every `make build` since. Untracked and ignored; the file stays on disk. Guards are behavioural rather than textual: they build throwaway repos with pinned commit timestamps and run version.sh against them, so they break when the derivation changes rather than when the wording does. Falsified — drop the .gitea exclusion and the CI-only commit moves the version; add an android exclusion and an Android commit stops moving it; exclude everything and a source commit refuses. One honest note on the refusal test: the script already refused an empty result via the downstream date check, so the new explicit check improves the diagnostic ("no commit touches the shipped file set — shallow clone?") and not the safety. The test pins the property, which is defended in depth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH	2026-09-10 17:55:36 -04:00
bvandeusenandClaude Opus 5	8f4b76a638	fix(ci): artifacts move to stock upload-artifact@v7 / download-artifact@v8 ... android.yml's debug upload and release.yml's minstrel-apk pair went through the bvandeusen fork mirrors, with comments saying stock actions refuse this hostname, that the pair had to be matched on the bundled @actions/artifact major, and that download v7 was off-limits for node24. None of that holds on gitea/runner 3.x: the runner edits the GHES refusal out of the action bundles, every download major v4-v8 reads every upload major v4-v7 (Scribe spike #3843, CI-runner run 6312), and every CI image carries Node 24. The mirror pair itself was last verified at tag run 6286. Same artifact names, paths and if-no-files-found. ci-requirements.md drops the pairing table and keeps what is still true: @v3 is invisible. Scribe snippet #2271, milestone 395. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DwoKYuw3qJmUUYsJeNherB	2026-09-10 17:25:10 -04:00
bvandeusenandClaude Opus 5	aeb8781c4e	fix(release): drop version image tags, mint the rollback unit on main ... The image tag map was the inverse of family rules 145 and 147 on every count: it published :vYYYY.MM.DD.HHMM that nobody pinned, published :main that rule 147 says should not exist, and published no commit-addressable image at all — so the rollback unit the rule names did not exist in this repo. A bad main push had nothing to roll back to but the previous release tag, which may be many commits back. The whole map is now: dev → :dev main → :latest + :<sha> tag → :latest A release refreshes the channel and mints nothing else. The tag build rebuilds the SAME SOURCE as main's build minutes earlier, differing only in which APK is baked in, so rule 145's immutability clause applies directly: move the channel tag, never re-push a commit-addressable one. :latest has to move here rather than waiting for the next main push, or the channel would carry the previous release's APK indefinitely — a channel that cannot refresh itself (rule 146). Two consequences that are not optional: The verify job asserted the :<version> image existed. With version tags gone that would fail every release for a tag nothing mints. Re-pointed at the :<sha> image rather than deleted — deleting it is the tempting way to make a failing guard go green, and it earns its keep twice now: it still catches an image push that silently did not happen, and it additionally proves the ordering, since a tag cut on a commit whose main build never completed has no rollback target. The server's self-reported version was the literal string "main" or "dev". That was survivable while :vYYYY.MM.DD.HHMM existed to identify a build; with version tags gone it is the ONLY thing that says which build is running, and two dev images months apart were indistinguishable. It now carries the derived name from ci/version.sh on every lane, with the channel as a sibling field (rule 149) rather than folded into the string. Surfaced at /healthz and beside the version in Settings. Guards added for each arm of the policy, and every one was falsified against the specific regression it names before committing. That caught two real bugs in the guards themselves: stepBody cut at the next `- name:`, which returns an EMPTY body for the last step in a job and made the assertions pass vacuously, and its replacement cut at any blank line followed by indentation, which truncated a step mid-run-block. The helper now refuses an empty body outright. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH	2026-09-10 15:10:15 -04:00