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) + } +}