test(scheduler): make the Lambda no-refresh tests bite again

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")
+			}
+		})
+	}
+}

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,15 +1125,24 @@ 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) })
 
 	mockStore.On("GetRecommendationsFreshness", ctx).
 		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}