test(scheduler): make the LambdaGuard control catch a non-blocking harness (#971 follow-ups)

internal/scheduler/background_collection_test.go

@@ -130,49 +130,85 @@ 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
 // 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.
+// 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
 }

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