From 4ce47397a9c573b786cf9436c0944a13340ca0f0 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 10 Sep 2026 23:29:35 -0400 Subject: [PATCH] fix(recommendation): a nil LibrarySize must degrade, not panic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- internal/recommendation/library_scale.go | 30 +++++++++++--- internal/recommendation/library_scale_test.go | 39 +++++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/internal/recommendation/library_scale.go b/internal/recommendation/library_scale.go index d648171f..a5298244 100644 --- a/internal/recommendation/library_scale.go +++ b/internal/recommendation/library_scale.go @@ -118,8 +118,7 @@ func CountLibraryTracks(ctx context.Context, q *dbq.Queries) (int64, error) { // per request. const librarySizeTTL = 5 * time.Minute -// librarySizeTimeout bounds the count itself. Rule 156: the caller is a user -// waiting on a radio, and a pool-sizing hint is never worth hanging for. +// librarySizeTimeout bounds the count itself — see loadWithDeadline. const librarySizeTimeout = 3 * time.Second // LibrarySize memoises the library track count. @@ -142,7 +141,22 @@ func NewLibrarySize(now func() time.Time) *LibrarySize { } // Get returns the cached size, refreshing through load when stale. +// +// A NIL RECEIVER IS VALID and means "no cache": the count still runs, it is +// just not memoised. That is deliberate rather than defensive habit. The api +// handlers struct is built directly by a dozen tests that cannot know about +// every field, and a nil here previously panicked inside a radio request — +// turning a missing pool-sizing HINT into a 500. Uncached-but-correct is the +// right failure for something whose whole contract is that it degrades. func (l *LibrarySize) Get(ctx context.Context, load func(context.Context) (int64, error)) int64 { + if l == nil { + n, err := loadWithDeadline(ctx, load) + if err != nil { + return 0 // scales to the base limits + } + return n + } + l.mu.Lock() defer l.mu.Unlock() @@ -154,9 +168,7 @@ func (l *LibrarySize) Get(ctx context.Context, load func(context.Context) (int64 return l.size } - cctx, cancel := context.WithTimeout(ctx, librarySizeTimeout) - defer cancel() - n, err := load(cctx) + n, err := loadWithDeadline(ctx, load) if err != nil { // Keep the last known value and re-try at the next call rather than // stamping `at`, so a transient failure does not pin a stale number @@ -166,3 +178,11 @@ func (l *LibrarySize) Get(ctx context.Context, load func(context.Context) (int64 l.size, l.at = n, now() return l.size } + +// loadWithDeadline bounds the count. Rule 156: the caller is a user waiting +// on a radio, and a pool-sizing hint is never worth hanging for. +func loadWithDeadline(ctx context.Context, load func(context.Context) (int64, error)) (int64, error) { + cctx, cancel := context.WithTimeout(ctx, librarySizeTimeout) + defer cancel() + return load(cctx) +} diff --git a/internal/recommendation/library_scale_test.go b/internal/recommendation/library_scale_test.go index 00cd6745..918ce409 100644 --- a/internal/recommendation/library_scale_test.go +++ b/internal/recommendation/library_scale_test.go @@ -175,3 +175,42 @@ func TestLibrarySize_RetriesAfterAFailedRefresh(t *testing.T) { 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) + } +}