diff --git a/internal/scheduler/background_collection_test.go b/internal/scheduler/background_collection_test.go index f976c1ff5..6d513fa78 100644 --- a/internal/scheduler/background_collection_test.go +++ b/internal/scheduler/background_collection_test.go @@ -130,6 +130,8 @@ func TestBackgroundCollectionAllowed(t *testing.T) { assert.True(t, BackgroundCollectionAllowed(derived)) } +// Pins the isLambda guard in maybeKickBackgroundRefresh. +// // 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 @@ -137,42 +139,76 @@ func TestBackgroundCollectionAllowed(t *testing.T) { // 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. +// CollectRecommendations) signals started, then blocks until the test hands +// it a value on release. The kick claims the collecting flag synchronously, +// so a wrongly kicked refresh still holds the flag when it is checked. 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") + stale := func() *config.RecommendationsFreshness { + old := time.Now().Add(-72 * time.Hour) + return &config.RecommendationsFreshness{LastCollectedAt: &old} + } + + t.Run("lambda never kicks", func(t *testing.T) { + mockStore, started, _ := blockingRefreshStore(t) + s := &Scheduler{config: mockStore, isLambda: true} + + s.maybeKickBackgroundRefresh(stale(), time.Nanosecond) + + // The guard returns before the synchronous CAS, so a kick would + // already hold the flag here; no wall-clock wait is needed. + assert.False(t, s.collecting.Load(), "the collecting flag was claimed on Lambda") + select { + case <-started: + t.Error("a background refresh reached the store on Lambda") + default: + } + mockStore.AssertNotCalled(t, "GetGlobalConfig", mock.Anything) + }) + + // Positive control on the same harness: the refresh must start, and its + // store call must still be blocked on release (flag held) until the test + // releases it. If the harness stops blocking, nothing receives on release + // and the send below times out, so the control fails every run. + t.Run("non-lambda kicks (control)", func(t *testing.T) { + mockStore, started, release := blockingRefreshStore(t) + s := &Scheduler{config: mockStore, isLambda: false} + + s.maybeKickBackgroundRefresh(stale(), time.Nanosecond) + + select { + case <-started: + case <-time.After(5 * time.Second): + t.Fatal("expected a background refresh on a non-Lambda runtime") + } + assert.True(t, s.collecting.Load(), "the refresh must hold the collecting flag while its store call is blocked") + select { + case release <- struct{}{}: + case <-time.After(2 * time.Second): + t.Fatal("the refresh's store call did not wait for release; the harness no longer blocks it") + } + require.Eventually(t, func() bool { return !s.collecting.Load() }, 2*time.Second, time.Millisecond, + "the refresh must release the collecting flag once its store call returns") + }) +} + +// blockingRefreshStore returns a store whose GetGlobalConfig, the refresh's +// first store call, signals started and then waits for one value on release +// (or for the test to end) before failing, which ends the refresh. +func blockingRefreshStore(t *testing.T) (*MockConfigStore, <-chan struct{}, chan<- struct{}) { + t.Helper() + mockStore := new(MockConfigStore) + started := make(chan struct{}, 1) + release := make(chan struct{}) + done := make(chan struct{}) + t.Cleanup(func() { close(done) }) + mockStore.On("GetGlobalConfig", mock.Anything). + Run(func(mock.Arguments) { + started <- struct{}{} 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") + case <-release: + case <-done: } - }) - } + }). + Return(nil, errors.New("refresh started")).Maybe() + return mockStore, started, release } diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 65fe3d405..d9dfe4e37 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -1055,6 +1055,8 @@ func TestScheduler_ListRecommendations_FreshnessError(t *testing.T) { assert.Nil(t, recs) } +// Pins the outer isLambda guard in ListRecommendations (mayCollect && !s.isLambda). +// // Lambda runtime must NOT spawn a background refresh even when the // cache is stale — goroutines freeze between invocations. // @@ -1116,6 +1118,9 @@ func TestScheduler_ListRecommendations_StaleSingleFlight(t *testing.T) { _ = ctx } +// Pins the isLambda guard in kickColdStartCollection (it also fails if the +// outer guard in ListRecommendations is removed). +// // Cold-start (LastCollectedAt==nil) on Lambda marks a collection started // On Lambda, kickColdStartCollection must return immediately WITHOUT ever // calling MarkCollectionStarted at all: a goroutine started inside a Lambda