diff --git a/go.mod b/go.mod index 5fb3a2f61..abcf4fb6c 100644 --- a/go.mod +++ b/go.mod @@ -59,7 +59,7 @@ require ( go.opentelemetry.io/otel/metric v1.45.0 // indirect go.opentelemetry.io/otel/trace v1.45.0 // indirect golang.org/x/crypto v0.58.0 - golang.org/x/net v0.61.0 // indirect + golang.org/x/net v0.62.0 // indirect golang.org/x/oauth2 v0.37.0 golang.org/x/sync v0.24.0 golang.org/x/sys v0.49.0 // indirect diff --git a/go.sum b/go.sum index 06deac0b9..83df52e66 100644 --- a/go.sum +++ b/go.sum @@ -367,8 +367,8 @@ golang.org/x/crypto v0.58.0 h1:COkYLr4k7nDI4r6QtMML2AfzeIswWHAJthLS6K/M/54= golang.org/x/crypto v0.58.0/go.mod h1:Xh+kl5A+M0gMiWDytH2Hhr5Z7WbNCpv/eB/EFgfSk+w= golang.org/x/mod v0.42.0 h1:ICeL3t2L30ljSPHHxDnR4fqVcBt/zR2zJAb9juavmQg= golang.org/x/mod v0.42.0/go.mod h1:u5fzIrRInkL8MaO1cbRkUHT64v303YIT5KEqpNr7jRs= -golang.org/x/net v0.61.0 h1:RR+6j/BTBCrggOLugxa+66R8FvoN6MHlzDlZIbN9YFw= -golang.org/x/net v0.61.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= +golang.org/x/net v0.62.0 h1:18ELalfM5+XNRwzM2nYqXsZQ9ByS1uhj2O/U7+5jqRY= +golang.org/x/net v0.62.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA= diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 4aeca1a17..a2155ef68 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -55,7 +55,7 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun // // The resolver is best-effort: a lookup error logs and falls through // to the un-scaled total, mirroring the over-show-vs-under-show - // trade-off baked into Scheduler.applySuppressions / applyAccountOverrides. + // trade-off ListRecommendations makes for account overrides. coverageByKey := h.resolveCoverageByAccountKey(ctx, recommendations) totalSavings, byService := summarizeRecommendationsWithCoverage(recommendations, coverageByKey) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index cd58a09d0..146588da2 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2711,7 +2711,11 @@ func (h *Handler) persistRetryExecution(ctx context.Context, failedExec *config. RetryAttemptN: failedExec.RetryAttemptN + 1, } - suppressions := config.BuildSuppressions(failedExec.Recommendations, newExecutionID, h.gracePeriodConfig(ctx), time.Now()) + graceCfg, err := h.gracePeriodConfig(ctx) + if err != nil { + return nil, "", err + } + suppressions := config.BuildSuppressions(failedExec.Recommendations, newExecutionID, graceCfg, time.Now()) if err := h.persistRetrySuccessorTx(ctx, failedExec, newExecution, suppressions); err != nil { return nil, "", err @@ -3754,7 +3758,11 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio executionID := execution.ExecutionID // Load the grace-period config once before entering the tx. - suppressions := config.BuildSuppressions(execReq.Recommendations, executionID, h.gracePeriodConfig(ctx), time.Now()) + graceCfg, err := h.gracePeriodConfig(ctx) + if err != nil { + return nil, err + } + suppressions := config.BuildSuppressions(execReq.Recommendations, executionID, graceCfg, time.Now()) dupExec, err := h.persistExecutionAndSuppressions(ctx, execution, suppressions, creatorID, idempotencyKey) if err != nil { return nil, err @@ -3790,14 +3798,18 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio } // gracePeriodConfig loads the global config for BuildSuppressions' grace -// periods, read before any tx opens. A read error does not block the -// purchase: it returns nil and the grace period defaults per provider. -func (h *Handler) gracePeriodConfig(ctx context.Context) *config.GlobalConfig { +// periods, read before any tx opens. A read error is returned and fails the +// purchase before anything is persisted (audit A01-016, issue #111): falling +// back to the default grace period would write suppressions for a provider +// the operator switched off (grace 0) or with the wrong expiry. This matches +// internal/purchase (AttachResolvedRecommendations) and the other config +// reads on these paths. +func (h *Handler) gracePeriodConfig(ctx context.Context) (*config.GlobalConfig, error) { g, err := h.config.GetGlobalConfig(ctx) if err != nil { - return nil + return nil, fmt.Errorf("failed to read grace periods for suppressions: %w", err) } - return g + return g, nil } // sendApprovalAndFinalize sends the approval email for a freshly persisted diff --git a/internal/api/handler_purchases_grace_config_test.go b/internal/api/handler_purchases_grace_config_test.go new file mode 100644 index 000000000..5467bb2f4 --- /dev/null +++ b/internal/api/handler_purchases_grace_config_test.go @@ -0,0 +1,72 @@ +package api + +import ( + "context" + "errors" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Fixture/mock evidence: in-memory mocks, no database. +// +// Audit A01-016 (issue #111): the grace periods that size purchase +// suppressions come from the global config. A failed read used to fall back +// to the 7-day default, writing suppressions for a provider the operator had +// switched off (grace 0) or with the wrong expiry. Both call sites now fail +// before persisting anything, as internal/purchase does. + +func TestHandler_executePurchase_GlobalConfigErrorFailsBeforePersist(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.grantAdminPurchaser() + mockStore.On("GetGlobalConfig", ctx).Return(nil, errors.New("config db down")) + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil).Maybe() + mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil).Maybe() + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "aws", Service: "ec2", ResourceType: "m5.large", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, Savings: 50, + }) + handler := &Handler{config: mockStore, auth: mockAuth} + + result, err := handler.executePurchase(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"recommendations": [{"id": "", "provider": "aws", "service": "ec2", "resource_type": "m5.large", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 100.0, "savings": 50.0}]}`, + }) + + require.ErrorContains(t, err, "config db down") + assert.Nil(t, result) + _, isClient := IsClientError(err) + assert.False(t, isClient, "a config read failure is a server error") + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "CreateSuppressionTx", mock.Anything, mock.Anything, mock.Anything) +} + +func TestHandler_retryPurchase_GlobalConfigErrorFailsBeforePersist(t *testing.T) { + _, handler, mockConfig := retryDedupeFixture(t) + for _, c := range mockConfig.ExpectedCalls { + if c.Method == "GetGlobalConfig" { + c.Unset() + } + } + mockConfig.On("GetGlobalConfig", mock.Anything).Return(nil, errors.New("config db down")) + mockConfig.On("ListRecentSubmitsTx", mock.Anything, mock.Anything, retryCallerID, mock.AnythingOfType("time.Time")). + Return([]config.PurchaseExecution{}, nil).Maybe() + + result, err := handler.retryPurchase(context.Background(), sessionRetryReq(), retryExecID) + + require.ErrorContains(t, err, "config db down") + assert.Nil(t, result) + _, isClient := IsClientError(err) + assert.False(t, isClient, "a config read failure is a server error") + mockConfig.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + mockConfig.AssertNotCalled(t, "LinkRetryExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockConfig.AssertNotCalled(t, "CreateSuppressionTx", mock.Anything, mock.Anything, mock.Anything) +} diff --git a/internal/config/recommendation_overrides.go b/internal/config/recommendation_overrides.go index c241a880c..3217c41fe 100644 --- a/internal/config/recommendation_overrides.go +++ b/internal/config/recommendation_overrides.go @@ -80,7 +80,9 @@ func (c *globalConfigCache) lookup(ctx context.Context, store AccountConfigReade // Errors from either lookup are returned alongside the partial map so the // caller decides whether to fail the whole operation or pass through. The // scheduler / dashboard read paths choose pass-through (over-show vs. -// under-show), matching the precedent set by applySuppressions. +// under-show); the money path (ListRecommendationsStrict) fails. Suppression +// reads fail closed on every path, since they are a spend guard rather than +// a display filter (issue #111). func ResolveAccountConfigsForRecs( ctx context.Context, store AccountConfigReader, diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index defb4176d..a471d7d86 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -7,14 +7,22 @@ import "os" // IsLambda reports whether the current process is running inside an // AWS Lambda execution environment. Detection is based on -// AWS_LAMBDA_RUNTIME_API, which the Lambda runtime always sets; it's -// absent on container images, local dev runs, and the long-running -// server deploys (Cloud Run / Container Apps). +// AWS_LAMBDA_RUNTIME_API, which the Lambda service sets for every +// function, zip or container image alike (the runtime interface client +// in a Lambda container image needs it). It is absent on non-Lambda +// deployments: local dev runs and the long-running servers (Fargate, +// Cloud Run, Container Apps). // -// Callers that need to gate non-Lambda-only behavior (e.g. -// background goroutines for stale-while-revalidate) should use this -// helper rather than reading the env var directly so the detection -// rule stays consistent across call sites. +// The flag answers one question only: is the process frozen between +// invocations, so that background goroutines (e.g. the +// stale-while-revalidate refresh) cannot be relied on to finish? It +// says nothing about whether the deployment runs a single instance or +// many, so do not use it to decide whether process-local state is +// shared across requests. +// +// Callers asking the freeze question should use this helper rather +// than reading the env var directly so the detection rule stays +// consistent across call sites. func IsLambda() bool { return os.Getenv("AWS_LAMBDA_RUNTIME_API") != "" } diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index b756dd37b..291f1fe41 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1220,18 +1220,19 @@ func (s *Scheduler) ListRecommendations(ctx context.Context, filter config.Recom recs, err = s.applySuppressions(ctx, recs) if err != nil { - // A suppression read failure shouldn't block the recs list — - // the user-facing cost of that is an empty page; we'd rather - // over-show than under-show. Log and continue. - logging.Errorf("failed to apply suppressions; returning un-suppressed recs: %v", err) + // Fail closed (issue #111). Suppressions are the duplicate-purchase + // guard: without them a just-bought commitment shows at its full + // pre-purchase count with no "recently purchased" badge, inviting + // the same purchase a second time. A failed page is the safe side. + return nil, fmt.Errorf("apply suppressions: %w", err) } recs, err = s.applyAccountOverrides(ctx, recs) if err != nil { - // Same over-show-vs-under-show trade-off as applySuppressions. - // A missing global ServiceConfig or override-table read failure - // should leave the user looking at un-overridden recs, not a - // blank page. Issue #196. + // Overrides are a display filter, not a spend guard, so they keep + // the over-show trade-off: a missing global ServiceConfig or an + // override-table read failure leaves the user looking at + // un-overridden recs, not a blank page. Issue #196. logging.Errorf("failed to apply account overrides; returning un-filtered recs: %v", err) } @@ -1270,9 +1271,10 @@ func (s *Scheduler) finishCacheRead(ctx context.Context, freshness *config.Recom } // ListRecommendationsStrict is ListRecommendations for the money path: the -// same stored rows, suppressions and account overrides, but a suppression or -// override read failure is returned instead of logged. Over-showing is fine -// for a page; on a purchase it would re-buy what a prior step already bought. +// same stored rows, suppressions and account overrides, but an override read +// failure is returned instead of logged (a suppression read failure fails +// both). Over-showing an override-hidden rec is fine for a page; on a +// purchase it would buy what the operator excluded. // It has no cold-start or background-refresh side effects: an empty cache // reads as empty and the caller retries on a later tick. func (s *Scheduler) ListRecommendationsStrict(ctx context.Context, filter config.RecommendationFilter) ([]config.RecommendationRecord, error) { @@ -1298,6 +1300,8 @@ func (s *Scheduler) ListRecommendationsStrict(ctx context.Context, filter config // Suppressions are still applied: a rec whose remaining count has been // suppressed to zero is treated as gone (returns nil, nil, nil) so the caller // can render a 404 rather than a "hidden" banner for actively-dismissed recs. +// A suppression read failure is returned (issue #111) rather than serving the +// rec at its un-suppressed count. // // hiddenBy is non-nil (one or more reason strings) when the rec exists and // passes the suppression check, but would be dropped by the account-override @@ -1319,8 +1323,8 @@ func (s *Scheduler) GetRecommendationByID(ctx context.Context, id string) (rec * // the caller's perspective (the suppression is intentional user action). recs, err = s.applySuppressions(ctx, recs) if err != nil { - // Over-show: on suppression read failure treat the rec as un-suppressed. - logging.Errorf("GetRecommendationByID: suppression check failed; treating rec as un-suppressed: %v", err) + // Fail closed, as ListRecommendations does (issue #111). + return nil, nil, fmt.Errorf("GetRecommendationByID: suppression check: %w", err) } if len(recs) == 0 { return nil, nil, nil @@ -1412,10 +1416,14 @@ func (a *suppressionAgg) add(sup *config.PurchaseSuppression) { // SuppressionExpiresAt / PrimarySuppressionExecutionID fields that // drive the "recently purchased" badge on the frontend. Recs whose // remaining count hits 0 are dropped entirely. +// +// A read failure returns no recs alongside the error, so a caller that +// dropped the error would still show nothing rather than the un-suppressed +// list (issue #111). func (s *Scheduler) applySuppressions(ctx context.Context, recs []config.RecommendationRecord) ([]config.RecommendationRecord, error) { sups, err := s.config.ListActiveSuppressions(ctx) if err != nil { - return recs, err + return nil, err } if len(sups) == 0 { return recs, nil diff --git a/internal/scheduler/scheduler_overrides.go b/internal/scheduler/scheduler_overrides.go index dcfb7af08..ff242930b 100644 --- a/internal/scheduler/scheduler_overrides.go +++ b/internal/scheduler/scheduler_overrides.go @@ -24,9 +24,12 @@ import ( // - The existing applySuppressions helper sets the precedent of post-DB // filtering inside ListRecommendations. // -// Errors from the resolver are non-fatal: we log and pass the un-filtered -// list through, matching applySuppressions. Over-show is the safer default -// (an unconfigured-looking dashboard is recoverable; a blanked one is not). +// Errors from the resolver are returned with the un-filtered list, and the +// page read paths log them and pass that list through. Over-show is the safer +// default for this display filter (an unconfigured-looking dashboard is +// recoverable; a blanked one is not). Suppressions differ: they guard against +// buying the same commitment twice, so their read failure fails the page +// (issue #111). func (s *Scheduler) applyAccountOverrides(ctx context.Context, recs []config.RecommendationRecord) ([]config.RecommendationRecord, error) { if len(recs) == 0 { return recs, nil diff --git a/internal/scheduler/scheduler_scan.go b/internal/scheduler/scheduler_scan.go index cef7057de..da8980cd1 100644 --- a/internal/scheduler/scheduler_scan.go +++ b/internal/scheduler/scheduler_scan.go @@ -3,6 +3,7 @@ package scheduler import ( "context" "errors" + "fmt" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" @@ -18,7 +19,8 @@ import ( // is returned. // // It has the same cold-start and stale-while-revalidate side effects as -// ListRecommendations, and the same over-show trade-off: a suppression or +// ListRecommendations, and the same failure directions: a suppression read +// failure fails the scan before any chunk reaches fn (issue #111), while an // override read failure is logged and the affected rows pass through. // // Each chunk is one consistent read; the walk as a whole is not one snapshot. @@ -36,7 +38,22 @@ func (s *Scheduler) ScanRecommendations(ctx context.Context, filter config.Recom return err } - suppressions := s.loadSuppressionIndex(ctx) + suppressions, err := s.loadSuppressionIndex(ctx) + if err != nil { + return err + } + if err := s.scanChunks(ctx, filter, chunkSize, suppressions, fn); err != nil { + return err + } + + s.finishCacheRead(ctx, freshness) + return nil +} + +// scanChunks is ScanRecommendations' walk: it reads the cache chunk by chunk, +// applies the suppression index and account overrides to each chunk, and +// hands the non-empty survivors to fn. +func (s *Scheduler) scanChunks(ctx context.Context, filter config.RecommendationFilter, chunkSize int, suppressions map[suppressionKey][]config.PurchaseSuppression, fn func([]config.RecommendationRecord) error) error { overrides := newOverrideScanFilter(s.config) var after *config.RecommendationScanKey for { @@ -55,31 +72,27 @@ func (s *Scheduler) ScanRecommendations(ctx context.Context, filter config.Recom } } if chunk.Done || chunk.Last == nil { - break + return nil } after = chunk.Last } - - s.finishCacheRead(ctx, freshness) - return nil } // loadSuppressionIndex reads the active suppressions once for a whole scan and // groups them by 6-tuple, as applySuppressions does. Which rows of a group // apply to a rec depends only on that rec's variant (issue #816), so applying // the index chunk by chunk suppresses exactly what ListRecommendations does. -// A read failure is logged and yields no index, so the scan over-shows like -// ListRecommendations does. -func (s *Scheduler) loadSuppressionIndex(ctx context.Context) map[suppressionKey][]config.PurchaseSuppression { +// A read failure is returned, so the scan fails closed like +// ListRecommendations does (issue #111). +func (s *Scheduler) loadSuppressionIndex(ctx context.Context) (map[suppressionKey][]config.PurchaseSuppression, error) { sups, err := s.config.ListActiveSuppressions(ctx) if err != nil { - logging.Errorf("failed to apply suppressions; returning un-suppressed recs: %v", err) - return nil + return nil, fmt.Errorf("apply suppressions: %w", err) } if len(sups) == 0 { - return nil + return nil, nil } - return aggregateSuppressions(sups) + return aggregateSuppressions(sups), nil } // overrideScanFilter applies account overrides chunk by chunk, resolving each diff --git a/internal/scheduler/scheduler_scan_test.go b/internal/scheduler/scheduler_scan_test.go index 304ba86b9..8503e65f4 100644 --- a/internal/scheduler/scheduler_scan_test.go +++ b/internal/scheduler/scheduler_scan_test.go @@ -278,16 +278,31 @@ func TestScanRecommendations_StoreErrorFails(t *testing.T) { require.ErrorContains(t, err, "db down") } -// TestScanRecommendations_ReadFailuresOverShow mirrors ListRecommendations' -// trade-off: a suppression or override read failure is logged and the rows -// pass through rather than failing the page. -func TestScanRecommendations_ReadFailuresOverShow(t *testing.T) { +// TestScanRecommendations_OverrideReadFailureOverShows mirrors +// ListRecommendations' trade-off for overrides: an override read failure is +// logged and the rows pass through rather than failing the page. +func TestScanRecommendations_OverrideReadFailureOverShows(t *testing.T) { store := scanFixture() - store.supsErr = errors.New("sups down") + store.sups = nil store.getOverrideErr = errors.New("overrides down") s := &Scheduler{config: store} got, _ := collectScan(t, s, 4) - assert.Len(t, got, 30, "nothing suppressed or overridden away") + assert.Len(t, got, 30, "nothing overridden away") +} + +// TestScanRecommendations_SuppressionReadFailureFailsClosed (issue #111): a +// suppression read failure fails the scan before any chunk is read or handed +// to fn, so no covered rec reaches the page at its full pre-purchase count. +func TestScanRecommendations_SuppressionReadFailureFailsClosed(t *testing.T) { + store := scanFixture() + store.supsErr = errors.New("sups down") + s := &Scheduler{config: store} + err := s.ScanRecommendations(context.Background(), config.RecommendationFilter{}, 4, func([]config.RecommendationRecord) error { + t.Fatal("fn must not run when the suppression read fails") + return nil + }) + require.ErrorContains(t, err, "sups down") + assert.Zero(t, store.chunkReads, "no chunk is read once the suppression read has failed") } // TestScanRecommendations_RejectsBadChunkSize guards the loop bound. diff --git a/internal/scheduler/scheduler_strict_list_test.go b/internal/scheduler/scheduler_strict_list_test.go index 31678e4ee..151b63fd5 100644 --- a/internal/scheduler/scheduler_strict_list_test.go +++ b/internal/scheduler/scheduler_strict_list_test.go @@ -51,9 +51,11 @@ func TestListRecommendationsStrictAppliesSuppressions(t *testing.T) { assert.Equal(t, 2, recs[0].Count) } -// The lenient ListRecommendations logs these and over-shows; the strict -// variant must hand the error back so a purchase never sizes off a read it -// could not complete. +// The strict variant must hand every read error back so a purchase never +// sizes off a read it could not complete. The lenient ListRecommendations +// differs only on an override failure, which it logs and over-shows; a +// suppression failure fails it too (issue #111), since showing a covered +// commitment at its full count invites buying it a second time. func TestListRecommendationsStrictReturnsReadErrors(t *testing.T) { boom := errors.New("boom") cases := map[string]func(*strictStore){ @@ -69,11 +71,59 @@ func TestListRecommendationsStrictReturnsReadErrors(t *testing.T) { recs, err := s.ListRecommendationsStrict(context.Background(), config.RecommendationFilter{}) require.ErrorIs(t, err, boom) assert.Nil(t, recs) - if name != "stored list" { - lenient, lerr := s.ListRecommendations(context.Background(), config.RecommendationFilter{}) - require.NoError(t, lerr, "lenient path swallows the same failure") + lenient, lerr := s.ListRecommendations(context.Background(), config.RecommendationFilter{}) + if name == "overrides" { + require.NoError(t, lerr, "lenient path swallows an override failure") assert.NotEmpty(t, lenient) + return } + require.ErrorIs(t, lerr, boom, "lenient path fails closed on this read too") + assert.Nil(t, lenient) }) } } + +// TestListRecommendations_SuppressionReadFailureFailsClosed (issue #111): a +// suppression read failure must fail the read rather than return the covered +// rec at its full pre-purchase count with no "recently purchased" marker. +func TestListRecommendations_SuppressionReadFailureFailsClosed(t *testing.T) { + boom := errors.New("suppressions down") + st := strictFixture() + st.supErr = boom + s := &Scheduler{config: st} + + recs, err := s.ListRecommendations(context.Background(), config.RecommendationFilter{}) + + require.ErrorIs(t, err, boom) + assert.Nil(t, recs, "no rec may be returned unsuppressed") +} + +// TestGetRecommendationByID_SuppressionReadFailureFailsClosed (issue #111): +// the detail read fails closed the same way, instead of serving the rec as +// un-suppressed. +func TestGetRecommendationByID_SuppressionReadFailureFailsClosed(t *testing.T) { + boom := errors.New("suppressions down") + st := strictFixture() + st.supErr = boom + s := &Scheduler{config: st} + + rec, hiddenBy, err := s.GetRecommendationByID(context.Background(), "r1") + + require.ErrorIs(t, err, boom) + assert.Nil(t, rec, "the rec must not be served un-suppressed") + assert.Nil(t, hiddenBy) +} + +// TestApplySuppressions_ReadFailureReturnsNoRecs pins the helper itself: on a +// read error it hands back no recs, so a caller that ignored the error would +// still show nothing rather than the un-suppressed list. +func TestApplySuppressions_ReadFailureReturnsNoRecs(t *testing.T) { + st := strictFixture() + st.supErr = errors.New("suppressions down") + s := &Scheduler{config: st} + + recs, err := s.applySuppressions(context.Background(), st.recs) + + require.Error(t, err) + assert.Nil(t, recs) +}