Files
minstrel/internal/recommendation/library_scale_test.go
T
bvandeusenandClaude Opus 5 4ce47397a9
test-go / test (push) Successful in 1m4s
test-go / integration (push) Successful in 3m41s
release / Build signed APK (releases and dev) (push) Successful in 4m37s
release / Build + push container image (push) Successful in 1m58s
release / Verify release artifacts (tag releases only) (push) Skipped
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

217 lines
8.7 KiB
Go

package recommendation
import (
"context"
"errors"
"testing"
"time"
)
// Small libraries must behave exactly as before. Scaling is meant to help a
// growing collection, not to change what every existing install already does.
func TestScaleForLibrary_SmallLibrariesAreUntouched(t *testing.T) {
base := DefaultCandidateSourceLimits()
for _, size := range []int64{0, 1, 500, libraryScaleReference} {
if got := ScaleForLibrary(base, size); got != base {
t.Errorf("library of %d changed the limits: %+v, want %+v", size, got, base)
}
}
}
// The complaint, restated as a property: a bigger library must reach further
// into itself. Operator, 2026-09-10 — "is the pool that we draw from somehow
// scaled to the amount of music in the library".
func TestScaleForLibrary_BigLibrariesGetABiggerPool(t *testing.T) {
base := DefaultCandidateSourceLimits()
small := ScaleForLibrary(base, libraryScaleReference)
big := ScaleForLibrary(base, libraryScaleReference*16)
if big.RandomFill <= small.RandomFill {
t.Errorf("RandomFill %d did not grow for a 16x library (was %d); that arm "+
"IS the fraction-of-library problem", big.RandomFill, small.RandomFill)
}
if big.LBSimilar <= small.LBSimilar {
t.Errorf("LBSimilar %d did not grow for a 16x library (was %d)",
big.LBSimilar, small.LBSimilar)
}
}
// THE SUBSTANCE: scaling is per-arm, decided by what each arm is BOUNDED BY.
// Raising a limit only helps if there are rows for it to cut off, so arms
// bounded by the user's likes, the instance's co-play graph or the taste
// profile gain nothing from a bigger library — raising them would sample more
// of a set that did not grow.
//
// A uniform scale would look right and be wrong, which is why this is pinned
// separately from "the pool got bigger".
func TestScaleForLibrary_OnlyLibraryBoundedArmsGrow(t *testing.T) {
base := DefaultCandidateSourceLimits()
big := ScaleForLibrary(base, libraryScaleReference*16)
for _, tc := range []struct {
arm string
got, want int
why string
}{
{"LikesOverlap", big.LikesOverlap, base.LikesOverlap, "bounded by the user's likes"},
{"UserCoplay", big.UserCoplay, base.UserCoplay, "bounded by the instance's co-play graph"},
{"TasteOverlap", big.TasteOverlap, base.TasteOverlap, "bounded by the taste profile's artists"},
} {
if tc.got != tc.want {
t.Errorf("%s scaled to %d (base %d) but is %s — a bigger library gives it "+
"nothing more to return", tc.arm, tc.got, tc.want, tc.why)
}
}
}
// Growth is bounded, or a huge library turns every recommendation query into
// a huge scan. sqrt keeps it proportional; the ceiling keeps it affordable.
func TestScaleForLibrary_GrowthIsBounded(t *testing.T) {
base := DefaultCandidateSourceLimits()
huge := ScaleForLibrary(base, 100_000_000)
if huge.RandomFill > int(float64(base.RandomFill)*maxLibraryScale) {
t.Errorf("RandomFill %d exceeds the %.0fx ceiling on base %d",
huge.RandomFill, maxLibraryScale, base.RandomFill)
}
// sqrt, not linear — and the test point matters. At SIXTEEN times the
// reference both curves land on 4x, because linear has already been
// clamped by the ceiling; asserting there proves nothing. Four times the
// reference is below the ceiling for both, so the curves separate: sqrt
// gives 2x, linear would give 4x.
four := ScaleForLibrary(base, libraryScaleReference*4)
if four.RandomFill != base.RandomFill*2 {
t.Errorf("4x library gave RandomFill %d, want %d — sqrt growth (linear "+
"would give %d)", four.RandomFill, base.RandomFill*2, base.RandomFill*4)
}
}
// Never below the base. The base limits are a floor, not a midpoint — and
// #3889 makes this load-bearing rather than tidy: shrinking an arm ordered by
// unseeded random() changes pool membership between same-day rebuilds.
func TestScaleForLibrary_NeverShrinksAnArm(t *testing.T) {
base := DefaultCandidateSourceLimits()
for _, size := range []int64{0, 1, 100, 4999, 5001, 1_000_000} {
got := ScaleForLibrary(base, size)
for _, tc := range []struct {
arm string
got, want int
}{
{"LBSimilar", got.LBSimilar, base.LBSimilar},
{"SimilarArtist", got.SimilarArtist, base.SimilarArtist},
{"TagOverlap", got.TagOverlap, base.TagOverlap},
{"RandomFill", got.RandomFill, base.RandomFill},
{"LikesOverlap", got.LikesOverlap, base.LikesOverlap},
{"UserCoplay", got.UserCoplay, base.UserCoplay},
{"TasteOverlap", got.TasteOverlap, base.TasteOverlap},
} {
if tc.got < tc.want {
t.Errorf("library %d shrank %s to %d (base %d)", size, tc.arm, tc.got, tc.want)
}
}
}
}
// A pool-sizing hint must never fail the request it is sizing. A count that
// errors leaves the previous value in place, and a never-counted cache
// returns 0 — which scales to the base limits, i.e. exactly today's
// behaviour.
func TestLibrarySize_DegradesOnFailure(t *testing.T) {
clock := time.Now()
c := NewLibrarySize(func() time.Time { return clock })
boom := func(context.Context) (int64, error) { return 0, errors.New("db is down") }
if got := c.Get(context.Background(), boom); got != 0 {
t.Errorf("first failure returned %d, want 0 (which scales to the base limits)", got)
}
if got := ScaleForLibrary(DefaultCandidateSourceLimits(), 0); got != DefaultCandidateSourceLimits() {
t.Error("a zero library size did not scale to the base limits")
}
// A good count, then a failure: the last known value survives.
if got := c.Get(context.Background(), func(context.Context) (int64, error) { return 40_000, nil }); got != 40_000 {
t.Fatalf("got %d, want 40000", got)
}
clock = clock.Add(librarySizeTTL + time.Second)
if got := c.Get(context.Background(), boom); got != 40_000 {
t.Errorf("a failed refresh returned %d, discarding the last known 40000", got)
}
}
// The count is a full table scan, so it must not run per request.
func TestLibrarySize_CachesWithinTheTTL(t *testing.T) {
clock := time.Now()
c := NewLibrarySize(func() time.Time { return clock })
calls := 0
load := func(context.Context) (int64, error) { calls++; return 1234, nil }
for i := 0; i < 5; i++ {
c.Get(context.Background(), load)
}
if calls != 1 {
t.Errorf("counted %d times within the TTL, want 1 — this is a full table scan", calls)
}
clock = clock.Add(librarySizeTTL + time.Second)
c.Get(context.Background(), load)
if calls != 2 {
t.Errorf("counted %d times after the TTL expired, want 2 — the size never refreshes", calls)
}
}
// A transient failure must not pin a stale value for the whole TTL: the
// failed refresh does not stamp the clock, so the next call retries.
func TestLibrarySize_RetriesAfterAFailedRefresh(t *testing.T) {
clock := time.Now()
c := NewLibrarySize(func() time.Time { return clock })
c.Get(context.Background(), func(context.Context) (int64, error) { return 100, nil })
clock = clock.Add(librarySizeTTL + time.Second)
c.Get(context.Background(), func(context.Context) (int64, error) { return 0, errors.New("blip") })
// Immediately after, with no clock movement — a stamped failure would
// serve the stale 100 until the TTL expired again.
if got := c.Get(context.Background(), func(context.Context) (int64, error) { return 900, nil }); got != 900 {
t.Errorf("got %d after a failed refresh, want 900 — the failure pinned a stale value", got)
}
}
// A nil cache must not panic, and must still be CORRECT — uncached, not
// broken.
//
// This is not a hypothetical hardening. internal/api builds its handlers
// struct directly in a dozen tests, none of which know about every field, so
// librarySize arrives nil there. The first version of this panicked inside
// handleRadio and took down TestHandleRadio_ColdStart_OnlySeedReturned with a
// SIGSEGV — turning a missing pool-sizing HINT into a request-killing crash,
// which is the opposite of what a value that "degrades rather than fails" is
// supposed to do.
func TestLibrarySize_NilReceiverStillCounts(t *testing.T) {
var c *LibrarySize // deliberately not constructed
calls := 0
got := c.Get(context.Background(), func(context.Context) (int64, error) {
calls++
return 40_000, nil
})
if got != 40_000 {
t.Errorf("nil cache returned %d, want 40000 — it should still count, just not memoise", got)
}
if calls != 1 {
t.Errorf("nil cache called the loader %d times, want 1", calls)
}
// Uncached: a second call counts again rather than reusing anything.
c.Get(context.Background(), func(context.Context) (int64, error) { calls++; return 40_000, nil })
if calls != 2 {
t.Errorf("nil cache memoised across calls (%d loads); it has nowhere to store a value", calls)
}
// And it still degrades on error rather than panicking.
if got := c.Get(context.Background(), func(context.Context) (int64, error) {
return 0, errors.New("db is down")
}); got != 0 {
t.Errorf("nil cache returned %d on a failed count, want 0 (base limits)", got)
}
}