From 3f2dafa17b844a9b3879891e4ad4eb29e892dda2 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 12:52:50 +0100 Subject: [PATCH] test(scheduler): make the LambdaGuard control catch a non-blocking harness Review follow-up on #971: - The non-Lambda control now hands the blocked store call a value on an unbuffered release channel after asserting the collecting flag is held. If the harness stops blocking, nothing receives and the control fails every run instead of passing by timing. - The Lambda subtest drops its 200ms wait: the guard returns before the synchronous CAS, so the flag check plus a non-blocking receive on started are enough. - Each Lambda test names the isLambda guard it pins. Test-only; no production change. Refs #962 Co-Authored-By: Claude Opus 5.5 --- .../scheduler/background_collection_test.go | 106 ++++++++++++------ internal/scheduler/scheduler_test.go | 5 + 2 files changed, 76 insertions(+), 35 deletions(-) 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