From 83459e4c5a20db87b36b025f0aafb80690c6589b Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 12:31:39 +0100 Subject: [PATCH] test(scheduler): make the Lambda no-refresh tests bite again Since #932, ListRecommendations kicks the cold-start and stale-cache collections only for a ctx marked WithBackgroundCollection. The two Lambda tests still read with a plain context.Background(), so nothing was ever collected and they passed with the isLambda guards removed. Run both with WithBackgroundCollection, so the isLambda guard is the only thing that prevents a collect, and stub the first store call each guard skips as optional so a removed guard fails a clear AssertNotCalled. maybeKickBackgroundRefresh's own isLambda guard is unreachable through ListRecommendations (the outer guard returns first) and had no test, so add a direct one with a non-Lambda positive control on the same harness. Test-only; no production change. Closes #962 Co-Authored-By: Claude Opus 5.5 --- .../scheduler/background_collection_test.go | 48 +++++++++++++++++++ internal/scheduler/scheduler_test.go | 23 ++++++++- 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/internal/scheduler/background_collection_test.go b/internal/scheduler/background_collection_test.go index ef4a62700..f976c1ff5 100644 --- a/internal/scheduler/background_collection_test.go +++ b/internal/scheduler/background_collection_test.go @@ -2,6 +2,7 @@ package scheduler import ( "context" + "errors" "testing" "time" @@ -128,3 +129,50 @@ func TestBackgroundCollectionAllowed(t *testing.T) { defer cancel() assert.True(t, BackgroundCollectionAllowed(derived)) } + +// maybeKickBackgroundRefresh carries its own isLambda guard, separate from the +// one in ListRecommendations, so a future caller that skips the outer check +// still cannot start a refresh goroutine that freezes between Lambda +// invocations (issue #962). Calling it directly is the only way to reach that +// guard: through ListRecommendations the outer guard always returns first. +// +// The refresh's first store call (GetGlobalConfig inside +// CollectRecommendations) blocks until the test ends, so on a wrongly kicked +// refresh the collecting flag, which the kick sets synchronously, is still +// held when it is checked. The non-Lambda case is the positive control: the +// same harness must see the refresh start. +func TestMaybeKickBackgroundRefresh_LambdaGuard(t *testing.T) { + for _, tc := range []struct { + name string + isLambda bool + wantKick bool + }{ + {name: "lambda never kicks", isLambda: true, wantKick: false}, + {name: "non-lambda kicks (control)", isLambda: false, wantKick: true}, + } { + t.Run(tc.name, func(t *testing.T) { + mockStore := new(MockConfigStore) + started := make(chan struct{}, 1) + release := make(chan struct{}) + t.Cleanup(func() { close(release) }) + mockStore.On("GetGlobalConfig", mock.Anything). + Run(func(mock.Arguments) { + started <- struct{}{} + <-release + }). + Return(nil, errors.New("refresh started")).Maybe() + + s := &Scheduler{config: mockStore, isLambda: tc.isLambda} + old := time.Now().Add(-72 * time.Hour) + s.maybeKickBackgroundRefresh(&config.RecommendationsFreshness{LastCollectedAt: &old}, time.Nanosecond) + + assert.Equal(t, tc.wantKick, s.collecting.Load(), "collecting flag after the kick") + select { + case <-started: + assert.True(t, tc.wantKick, "a background refresh started on Lambda") + case <-time.After(200 * time.Millisecond): + assert.False(t, tc.wantKick, "expected a background refresh on a non-Lambda runtime") + } + }) + } +} diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 08caf29a5..65fe3d405 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -1057,8 +1057,16 @@ func TestScheduler_ListRecommendations_FreshnessError(t *testing.T) { // Lambda runtime must NOT spawn a background refresh even when the // cache is stale — goroutines freeze between invocations. +// +// The read is marked WithBackgroundCollection, so the isLambda guard in +// ListRecommendations is the only thing standing between it and the refresh +// (an unmarked read never refreshes, issue #962). GetGlobalConfig is stubbed +// as optional so dropping that guard fails the AssertNotCalled below with a +// clear message: the guard skips the TTL resolution read, which is the first +// GetGlobalConfig call on this path. func TestScheduler_ListRecommendations_LambdaSkipsBackgroundRefresh(t *testing.T) { - ctx := context.Background() + ctx := WithBackgroundCollection(context.Background()) + require.True(t, BackgroundCollectionAllowed(ctx), "the read must be allowed to collect, or this test is vacuous") mockStore := new(MockConfigStore) // Cache is stale (older than the 1ns TTL set below). @@ -1067,6 +1075,8 @@ func TestScheduler_ListRecommendations_LambdaSkipsBackgroundRefresh(t *testing.T Return(&config.RecommendationsFreshness{LastCollectedAt: &old}, nil) mockStore.On("ListStoredRecommendations", ctx, mock.Anything). Return([]config.RecommendationRecord{}, nil) + mockStore.On("GetGlobalConfig", mock.Anything). + Return(&config.GlobalConfig{RecommendationsCacheStaleHours: 1}, nil).Maybe() scheduler := &Scheduler{ config: mockStore, @@ -1115,8 +1125,16 @@ func TestScheduler_ListRecommendations_StaleSingleFlight(t *testing.T) { // false-409 window on the refresh endpoint for nothing (fix-review finding // 3 on issue #106's PR). GetGlobalConfig -- only ever called by // CollectRecommendations -- must also never fire. +// +// The read is marked WithBackgroundCollection, so the isLambda guard in +// kickColdStartCollection is the only thing that keeps it from taking the +// marker (an unmarked read never even calls it, issue #962). +// MarkCollectionStarted is stubbed as optional, returning "lost the race", +// so dropping that guard fails the AssertNotCalled below with a clear message +// instead of an unexpected-call panic. func TestScheduler_ListRecommendations_ColdStart_Lambda_NeverMarksOrCollects(t *testing.T) { - ctx := context.Background() + ctx := WithBackgroundCollection(context.Background()) + require.True(t, BackgroundCollectionAllowed(ctx), "the read must be allowed to collect, or this test is vacuous") mockStore := new(MockConfigStore) t.Cleanup(func() { mockStore.AssertExpectations(t) }) @@ -1124,6 +1142,7 @@ func TestScheduler_ListRecommendations_ColdStart_Lambda_NeverMarksOrCollects(t * Return(&config.RecommendationsFreshness{LastCollectedAt: nil}, nil) mockStore.On("ListStoredRecommendations", ctx, mock.Anything). Return([]config.RecommendationRecord{}, nil) + mockStore.On("MarkCollectionStarted", mock.Anything).Return("", false, nil).Maybe() scheduler := &Scheduler{config: mockStore, isLambda: true}