fix(server,scheduler): first slice of the 2026-07-28 review findings (#111)

cmd/server/main.go

@@ -105,29 +105,36 @@ func getTaskTimeout() time.Duration {
 }
 
 // determineRuntimeMode determines the runtime mode based on flags and environment.
+//
+// Precedence: the --mode flag, then an explicit RUNTIME_MODE, then Lambda
+// auto-detection. Explicit configuration outranks ambient detection (issue
+// #111): with RUNTIME_MODE=http under the Lambda Runtime Interface Emulator,
+// which sets AWS_LAMBDA_RUNTIME_API, the operator gets the HTTP server they
+// asked for rather than a silent Lambda start. RUNTIME_MODE=auto (the
+// Dockerfile default) or an empty value means auto-detect.
 func determineRuntimeMode(modeFlag string) string {
 	// If mode is explicitly set, use it
 	if modeFlag != "auto" {
 		return modeFlag
 	}
 
+	// Check for explicit RUNTIME_MODE environment variable
+	switch runtimeMode := os.Getenv("RUNTIME_MODE"); runtimeMode {
+	case "lambda", "http":
+		return runtimeMode
+	case "", "auto":
+		// Not set, or explicitly auto: fall through to detection below.
+	default:
+		log.Printf("Warning: unrecognized RUNTIME_MODE %q, falling back to auto-detection", runtimeMode) // #nosec G706 -- RUNTIME_MODE env var is operator-controlled; logged for diagnostics
+	}
+
 	// Auto-detect based on environment using the canonical runtime helper,
 	// which encapsulates the detection rule so future changes stay consistent
 	// across all call sites (issue 04-M5).
 	if runtime.IsLambda() {
 		return "lambda"
 	}
 
-	// Check for explicit RUNTIME_MODE environment variable
-	if runtimeMode := os.Getenv("RUNTIME_MODE"); runtimeMode != "" {
-		switch runtimeMode {
-		case "lambda", "http":
-			return runtimeMode
-		default:
-			log.Printf("Warning: unrecognized RUNTIME_MODE %q, falling back to auto-detection", runtimeMode) // #nosec G706 -- RUNTIME_MODE env var is operator-controlled; logged for diagnostics
-		}
-	}
-
 	// Default to HTTP mode for containers (Fargate, Cloud Run, Container Apps)
 	return "http"
 }

cmd/server/runtime_mode_test.go

@@ -0,0 +1,32 @@
+package main
+
+import "testing"
+
+// TestDetermineRuntimeMode pins issue #111: an explicit RUNTIME_MODE wins
+// over Lambda auto-detection, and the --mode flag wins over both.
+func TestDetermineRuntimeMode(t *testing.T) {
+	tests := []struct {
+		name        string
+		modeFlag    string
+		runtimeMode string
+		lambdaAPI   string
+		want        string
+	}{
+		{name: "RUNTIME_MODE=http beats Lambda detection", modeFlag: "auto", runtimeMode: "http", lambdaAPI: "127.0.0.1:9001", want: "http"},
+		{name: "RUNTIME_MODE=lambda without Lambda env", modeFlag: "auto", runtimeMode: "lambda", want: "lambda"},
+		{name: "auto detects Lambda", modeFlag: "auto", runtimeMode: "auto", lambdaAPI: "127.0.0.1:9001", want: "lambda"},
+		{name: "unset detects Lambda", modeFlag: "auto", lambdaAPI: "127.0.0.1:9001", want: "lambda"},
+		{name: "unset outside Lambda defaults to http", modeFlag: "auto", want: "http"},
+		{name: "unrecognized value falls back to detection", modeFlag: "auto", runtimeMode: "bogus", lambdaAPI: "127.0.0.1:9001", want: "lambda"},
+		{name: "flag beats RUNTIME_MODE", modeFlag: "lambda", runtimeMode: "http", want: "lambda"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv("RUNTIME_MODE", tt.runtimeMode)
+			t.Setenv("AWS_LAMBDA_RUNTIME_API", tt.lambdaAPI)
+			if got := determineRuntimeMode(tt.modeFlag); got != tt.want {
+				t.Errorf("determineRuntimeMode(%q) = %q, want %q", tt.modeFlag, got, tt.want)
+			}
+		})
+	}
+}

internal/api/openapi.yaml

@@ -4197,6 +4197,10 @@ components:
           enum: [healthy, unhealthy]
         message:
           type: string
+          description: Fixed human-readable text; never a raw error string.
+        reason:
+          type: string
+          description: Stable code for a failed check, e.g. db_unreachable or migration_failed.
 
     # -- Info ---------------------------------------------------------------
     PublicInfoResponse:

internal/config/store_postgres_recommendations.go

@@ -108,9 +108,16 @@ func (s *PostgresStore) UpsertRecommendations(ctx context.Context, collectedAt t
 		}
 	}
 
+	// last_collected_at only moves forward (issue #111). Two collections can
+	// overlap (cron and a user refresh), and the one that started first can
+	// commit last; an unconditional SET would then move freshness backwards
+	// and make the next read kick another full collection. GREATEST ignores
+	// the NULL of a never-collected row. Under READ COMMITTED a writer that
+	// waited on the row lock re-evaluates GREATEST against the committed
+	// value, so concurrent commits keep the newest timestamp.
 	if _, err := tx.Exec(ctx, `
 		UPDATE recommendations_state
-		   SET last_collected_at       = $1,
+		   SET last_collected_at       = GREATEST(last_collected_at, $1),
 		       last_collection_error   = NULL,
 		       last_collection_partial = NULL
 		 WHERE id = 1

internal/config/store_postgres_recommendations_freshness_test.go

@@ -0,0 +1,95 @@
+//go:build integration
+// +build integration
+
+package config_test
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// requireLastCollectedAt asserts recommendations_state.last_collected_at
+// equals want.
+func requireLastCollectedAt(ctx context.Context, t *testing.T, store *config.PostgresStore, want time.Time) {
+	t.Helper()
+	fr, err := store.GetRecommendationsFreshness(ctx)
+	require.NoError(t, err)
+	require.NotNil(t, fr.LastCollectedAt)
+	assert.True(t, want.Equal(*fr.LastCollectedAt),
+		"last_collected_at = %s, want %s", fr.LastCollectedAt.UTC(), want)
+}
+
+// TestPostgresStore_UpsertRecommendations_LastCollectedAtNeverMovesBackwards
+// pins issue #111: a collection that started earlier but commits later must
+// not move last_collected_at back to its older timestamp.
+func TestPostgresStore_UpsertRecommendations_LastCollectedAtNeverMovesBackwards(t *testing.T) {
+	ctx := context.Background()
+	store := setupRecommendationsStore(ctx, t)
+
+	newer := time.Now().UTC().Truncate(time.Second)
+	older := newer.Add(-3 * time.Minute)
+
+	require.NoError(t, store.UpsertRecommendations(ctx, newer, nil, nil))
+	requireLastCollectedAt(ctx, t, store, newer)
+
+	require.NoError(t, store.UpsertRecommendations(ctx, older, nil, nil))
+	requireLastCollectedAt(ctx, t, store, newer)
+
+	// A genuinely newer collection still advances it.
+	newest := newer.Add(time.Minute)
+	require.NoError(t, store.UpsertRecommendations(ctx, newest, nil, nil))
+	requireLastCollectedAt(ctx, t, store, newest)
+}
+
+// TestPostgresStore_UpsertRecommendations_LastCollectedAtConcurrentCommit
+// covers the race in issue #111 on real Postgres: the newer collection holds
+// the state row's lock and commits while the older one is blocked on it. The
+// blocked UPDATE must re-evaluate GREATEST against the committed newer value
+// rather than overwrite it with the older timestamp.
+func TestPostgresStore_UpsertRecommendations_LastCollectedAtConcurrentCommit(t *testing.T) {
+	ctx := context.Background()
+	db := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()).DB
+	store := config.NewPostgresStore(db)
+
+	base := time.Now().UTC().Truncate(time.Second)
+	older := base.Add(-2 * time.Minute)
+	newer := base
+
+	require.NoError(t, store.UpsertRecommendations(ctx, base.Add(-time.Hour), nil, nil))
+
+	// The newer run's state write, held open so it owns the row lock.
+	tx, err := db.Begin(ctx)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = tx.Rollback(context.Background()) })
+	_, err = tx.Exec(ctx, `UPDATE recommendations_state SET last_collected_at = GREATEST(last_collected_at, $1) WHERE id = 1`, newer)
+	require.NoError(t, err)
+
+	// The older run commits after it: its UPDATE blocks on the row lock.
+	done := make(chan error, 1)
+	go func() { done <- store.UpsertRecommendations(ctx, older, nil, nil) }()
+
+	require.Eventually(t, func() bool {
+		var waiting int
+		if qerr := db.QueryRow(ctx, `SELECT count(*) FROM pg_locks WHERE NOT granted`).Scan(&waiting); qerr != nil {
+			return false
+		}
+		return waiting > 0
+	}, 10*time.Second, 20*time.Millisecond, "older run's UPDATE never blocked on the state row lock")
+
+	require.NoError(t, tx.Commit(ctx))
+
+	select {
+	case err := <-done:
+		require.NoError(t, err)
+	case <-time.After(10 * time.Second):
+		t.Fatal("older run's UpsertRecommendations did not finish after the newer run committed")
+	}
+
+	requireLastCollectedAt(ctx, t, store, newer)
+}

internal/scheduler/scheduler.go

@@ -465,6 +465,12 @@ func joinProviderErrors(failed map[string]string) string {
 // CloudAccountID. The *sweepGap is non-nil when some account's sweep was
 // incomplete (its rows are kept; see tolerateIncompleteSweep). Provider-level
 // errors propagate unchanged.
+//
+// A name that is not one of the known providers (e.g. "AWS" or "azure " left
+// in EnabledProviders) is an error, so the merge records it as a failed
+// provider and persistCollection writes last_collection_error. Returning nil
+// here reported a provider that was never contacted as a clean, successful
+// collection of zero recommendations (issue #111).
 func (s *Scheduler) collectProviderRecommendations(ctx context.Context, providerName string, globalCfg *config.GlobalConfig) ([]config.RecommendationRecord, []string, *sweepGap, error) {
 	switch providerName {
 	case "aws":
@@ -474,8 +480,7 @@ func (s *Scheduler) collectProviderRecommendations(ctx context.Context, provider
 	case "gcp":
 		return s.collectGCPRecommendations(ctx, globalCfg)
 	default:
-		logging.Warnf("Unknown provider: %s", providerName)
-		return nil, nil, nil, nil
+		return nil, nil, nil, fmt.Errorf("unknown provider %q (valid: aws, azure, gcp)", providerName)
 	}
 }
 

internal/scheduler/scheduler_test.go

@@ -465,17 +465,25 @@ func TestScheduler_CollectRecommendations_ParallelProviders(t *testing.T) {
 	})
 }
 
+// TestScheduler_CollectRecommendations_UnknownProvider pins issue #111: a
+// provider name the scheduler does not recognize (wrong case, trailing space)
+// must be reported as a failed provider and recorded in
+// last_collection_error, not as a clean collection of zero recommendations.
 func TestScheduler_CollectRecommendations_UnknownProvider(t *testing.T) {
 	ctx := context.Background()
 	mockStore := new(MockConfigStore)
 	mockEmail := new(MockEmailSender)
 	mockFactory := new(MockProviderFactory)
 
 	globalCfg := &config.GlobalConfig{
-		EnabledProviders: []string{"unknown_provider"},
+		EnabledProviders: []string{"AWS", "azure "},
 	}
 
 	mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil)
+	var recordedErr string
+	mockStore.On("SetRecommendationsCollectionError", mock.Anything, mock.AnythingOfType("string")).
+		Run(func(args mock.Arguments) { recordedErr = args.String(1) }).
+		Return(nil).Once()
 
 	scheduler := &Scheduler{
 		config:          mockStore,
@@ -488,6 +496,12 @@ func TestScheduler_CollectRecommendations_UnknownProvider(t *testing.T) {
 	require.NoError(t, err)
 
 	assert.Equal(t, 0, result.Recommendations)
+	assert.Empty(t, result.SuccessfulProviders, "an unknown provider was never contacted, so it must not count as successful")
+	assert.Contains(t, result.FailedProviders, "AWS")
+	assert.Contains(t, result.FailedProviders, "azure ")
+	mockStore.AssertCalled(t, "SetRecommendationsCollectionError", mock.Anything, mock.AnythingOfType("string"))
+	assert.Contains(t, recordedErr, `unknown provider "AWS"`)
+	assert.Contains(t, recordedErr, `unknown provider "azure "`)
 }
 
 func TestScheduler_CollectAWSRecommendations(t *testing.T) {
@@ -575,10 +589,10 @@ func TestScheduler_CollectProviderRecommendations(t *testing.T) {
 		provider    string
 		expectError bool
 	}{
-		{"aws", true},    // ambient fallback fails via factory error
-		{"azure", false}, // no accounts → nil, nil
-		{"gcp", false},   // no accounts → nil, nil
-		{"unknown", false},
+		{"aws", true},     // ambient fallback fails via factory error
+		{"azure", false},  // no accounts → nil, nil
+		{"gcp", false},    // no accounts → nil, nil
+		{"unknown", true}, // not a provider the scheduler can collect (#111)
 	}
 
 	for _, tt := range tests {

internal/server/handler.go

@@ -100,6 +100,13 @@ var scheduledEventActions = map[string]ScheduledTaskType{
 func (app *Application) runScheduledTask(ctx context.Context, taskType ScheduledTaskType, params ScheduledTaskParams) (any, error) {
 	log.Printf("Handling scheduled task: %q", taskType) // #nosec G706 -- taskType is looked up from a known-value map; %q quotes the value to prevent CR/LF log injection
 
+	// Reject an unknown task before connecting or taking an advisory lock
+	// keyed on its name. The Lambda and HTTP transports already validate at
+	// their boundary; this covers the --task CLI and any future caller (#111).
+	if _, err := ParseScheduledTaskType(string(taskType)); err != nil {
+		return nil, err
+	}
+
 	if err := app.ensureDB(ctx); err != nil {
 		return nil, fmt.Errorf("database connection failed: %w", err)
 	}

internal/server/health.go

@@ -22,11 +22,25 @@ type HealthStatus struct {
 }
 
 // CheckResult represents the result of a health check.
+//
+// /health and /ready are unauthenticated, so Message is always a fixed string
+// and never carries a raw error: pgx and golang-migrate errors name the
+// database host, user, migration file and schema objects (issue #111). A
+// failed check sets Reason to a stable code instead, and the error itself goes
+// to the server log only.
 type CheckResult struct {
 	Status  string `json:"status"`
 	Message string `json:"message,omitempty"`
+	Reason  string `json:"reason,omitempty"`
 }
 
+// Stable reason codes a failed health check reports in CheckResult.Reason.
+const (
+	healthReasonMigrationFailed = "migration_failed"
+	healthReasonDBUnreachable   = "db_unreachable"
+	healthReasonAuthUnreachable = "auth_store_unreachable"
+)
+
 // handleHealthCheck returns the liveness status of the application.
 //
 // This endpoint is the LIVENESS contract: it always answers HTTP 200 so
@@ -160,7 +174,8 @@ func (app *Application) checkMigrations() CheckResult {
 	case finishedAt.IsZero():
 		return CheckResult{Status: "pending", Message: "migrations have not run yet"}
 	case err != nil:
-		return CheckResult{Status: "failed", Message: err.Error()}
+		// ensureDB logged the error when the run failed.
+		return CheckResult{Status: "failed", Message: "Database migrations failed", Reason: healthReasonMigrationFailed}
 	default:
 		return CheckResult{
 			Status:  "healthy",
@@ -191,9 +206,11 @@ func (app *Application) checkConfigStore(ctx context.Context, configStore config
 	// If using PostgreSQL, check database connection health
 	if db != nil {
 		if err := db.HealthCheck(ctx); err != nil {
+			log.Printf("health: database health check failed: %v", err)
 			return CheckResult{
 				Status:  "unhealthy",
-				Message: fmt.Sprintf("Database health check failed: %v", err),
+				Message: "Database health check failed",
+				Reason:  healthReasonDBUnreachable,
 			}
 		}
 	}
@@ -240,9 +257,11 @@ func (app *Application) checkAuthStore(ctx context.Context, authService *auth.Se
 
 	// Ping the database to verify connection is healthy
 	if err := authService.Ping(ctx); err != nil {
+		log.Printf("health: auth store ping failed: %v", err)
 		return CheckResult{
 			Status:  "unhealthy",
-			Message: fmt.Sprintf("Auth store ping failed: %v", err),
+			Message: "Auth store ping failed",
+			Reason:  healthReasonAuthUnreachable,
 		}
 	}
 

internal/server/health_error_redaction_integration_test.go

@@ -0,0 +1,35 @@
+//go:build integration
+
+package server
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/database"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestCheckConfigStore_DBFailureHidesRawError drives the config-store check
+// against a real pgx pool whose ping fails, and pins issue #111: the result
+// carries a fixed message and the db_unreachable reason, not the pgx error.
+func TestCheckConfigStore_DBFailureHidesRawError(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+	defer cancel()
+	pg := testhelpers.RequirePostgresContainer(ctx, t)
+
+	conn, err := database.NewConnection(ctx, pg.Config, nil)
+	require.NoError(t, err)
+	conn.Close()
+	require.Error(t, conn.HealthCheck(ctx), "a closed pool must fail its health check")
+
+	// Through collectHealth rather than checkConfigStore directly, so the
+	// test does not depend on that helper's parameter list.
+	app := &Application{Config: &mockConfigStoreForHealth{}, DB: conn, Auth: createHealthyAuthService()}
+	got := app.collectHealth(ctx)
+	assert.Equal(t, "degraded", got.Status)
+	assert.Equal(t, CheckResult{Status: "unhealthy", Message: "Database health check failed", Reason: healthReasonDBUnreachable}, got.Checks["config_store"])
+}