fix(scheduler,api): fail closed when suppressions or grace periods can't be read

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

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=

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)

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

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

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,

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") != ""
 }

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

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

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