fix(recommendation): a nil LibrarySize must degrade, not panic
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
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
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
This commit is contained in:
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user