Skip to content

Commit e8f88e6

Browse files
IAmJSDclaude
andauthored
fix(server,scheduler): first slice of the 2026-07-28 review findings (#1275)
- scheduler: an unknown provider name in EnabledProviders is now a failed provider, so it lands in FailedProviders and last_collection_error instead of reading as a clean collection of zero recommendations. - config: UpsertRecommendations sets last_collected_at with GREATEST, so an older collection that commits after a newer one can no longer move freshness backwards and trigger another full collection. - server: /health and /ready report a fixed message plus a stable reason code (db_unreachable, auth_store_unreachable, migration_failed) and keep the raw pgx/migrate error in the server log only. - server: runScheduledTask rejects a task name outside the roster before connecting or taking an advisory lock keyed on it (covers --task). - cmd/server: an explicit RUNTIME_MODE now outranks Lambda auto-detection; RUNTIME_MODE=auto is treated as unset rather than unrecognized. Refs #111 Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent bb7cfc7 commit e8f88e6

13 files changed

Lines changed: 354 additions & 24 deletions

‎cmd/server/main.go‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -105,29 +105,36 @@ func getTaskTimeout() time.Duration {
105105
}
106106

107107
// determineRuntimeMode determines the runtime mode based on flags and environment.
108+
//
109+
// Precedence: the --mode flag, then an explicit RUNTIME_MODE, then Lambda
110+
// auto-detection. Explicit configuration outranks ambient detection (issue
111+
// #111): with RUNTIME_MODE=http under the Lambda Runtime Interface Emulator,
112+
// which sets AWS_LAMBDA_RUNTIME_API, the operator gets the HTTP server they
113+
// asked for rather than a silent Lambda start. RUNTIME_MODE=auto (the
114+
// Dockerfile default) or an empty value means auto-detect.
108115
func determineRuntimeMode(modeFlag string) string {
109116
// If mode is explicitly set, use it
110117
if modeFlag != "auto" {
111118
return modeFlag
112119
}
113120

121+
// Check for explicit RUNTIME_MODE environment variable
122+
switch runtimeMode := os.Getenv("RUNTIME_MODE"); runtimeMode {
123+
case "lambda", "http":
124+
return runtimeMode
125+
case "", "auto":
126+
// Not set, or explicitly auto: fall through to detection below.
127+
default:
128+
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
129+
}
130+
114131
// Auto-detect based on environment using the canonical runtime helper,
115132
// which encapsulates the detection rule so future changes stay consistent
116133
// across all call sites (issue 04-M5).
117134
if runtime.IsLambda() {
118135
return "lambda"
119136
}
120137

121-
// Check for explicit RUNTIME_MODE environment variable
122-
if runtimeMode := os.Getenv("RUNTIME_MODE"); runtimeMode != "" {
123-
switch runtimeMode {
124-
case "lambda", "http":
125-
return runtimeMode
126-
default:
127-
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
128-
}
129-
}
130-
131138
// Default to HTTP mode for containers (Fargate, Cloud Run, Container Apps)
132139
return "http"
133140
}

‎cmd/server/runtime_mode_test.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
package main
2+
3+
import "testing"
4+
5+
// TestDetermineRuntimeMode pins issue #111: an explicit RUNTIME_MODE wins
6+
// over Lambda auto-detection, and the --mode flag wins over both.
7+
func TestDetermineRuntimeMode(t *testing.T) {
8+
tests := []struct {
9+
name string
10+
modeFlag string
11+
runtimeMode string
12+
lambdaAPI string
13+
want string
14+
}{
15+
{name: "RUNTIME_MODE=http beats Lambda detection", modeFlag: "auto", runtimeMode: "http", lambdaAPI: "127.0.0.1:9001", want: "http"},
16+
{name: "RUNTIME_MODE=lambda without Lambda env", modeFlag: "auto", runtimeMode: "lambda", want: "lambda"},
17+
{name: "auto detects Lambda", modeFlag: "auto", runtimeMode: "auto", lambdaAPI: "127.0.0.1:9001", want: "lambda"},
18+
{name: "unset detects Lambda", modeFlag: "auto", lambdaAPI: "127.0.0.1:9001", want: "lambda"},
19+
{name: "unset outside Lambda defaults to http", modeFlag: "auto", want: "http"},
20+
{name: "unrecognized value falls back to detection", modeFlag: "auto", runtimeMode: "bogus", lambdaAPI: "127.0.0.1:9001", want: "lambda"},
21+
{name: "flag beats RUNTIME_MODE", modeFlag: "lambda", runtimeMode: "http", want: "lambda"},
22+
}
23+
for _, tt := range tests {
24+
t.Run(tt.name, func(t *testing.T) {
25+
t.Setenv("RUNTIME_MODE", tt.runtimeMode)
26+
t.Setenv("AWS_LAMBDA_RUNTIME_API", tt.lambdaAPI)
27+
if got := determineRuntimeMode(tt.modeFlag); got != tt.want {
28+
t.Errorf("determineRuntimeMode(%q) = %q, want %q", tt.modeFlag, got, tt.want)
29+
}
30+
})
31+
}
32+
}

‎internal/api/openapi.yaml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4369,6 +4369,10 @@ components:
43694369
enum: [healthy, unhealthy]
43704370
message:
43714371
type: string
4372+
description: Fixed human-readable text; never a raw error string.
4373+
reason:
4374+
type: string
4375+
description: Stable code for a failed check, e.g. db_unreachable or migration_failed.
43724376

43734377
# -- Info ---------------------------------------------------------------
43744378
PublicInfoResponse:

‎internal/config/store_postgres_recommendations.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,16 @@ func (s *PostgresStore) UpsertRecommendations(ctx context.Context, collectedAt t
7272
}
7373
}
7474

75+
// last_collected_at only moves forward (issue #111). Two collections can
76+
// overlap (cron and a user refresh), and the one that started first can
77+
// commit last; an unconditional SET would then move freshness backwards
78+
// and make the next read kick another full collection. GREATEST ignores
79+
// the NULL of a never-collected row. Under READ COMMITTED a writer that
80+
// waited on the row lock re-evaluates GREATEST against the committed
81+
// value, so concurrent commits keep the newest timestamp.
7582
if _, err := tx.Exec(ctx, `
7683
UPDATE recommendations_state
77-
SET last_collected_at = $1,
84+
SET last_collected_at = GREATEST(last_collected_at, $1),
7885
last_collection_error = NULL,
7986
last_collection_partial = NULL
8087
WHERE id = 1
Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
//go:build integration
2+
// +build integration
3+
4+
package config_test
5+
6+
import (
7+
"context"
8+
"testing"
9+
"time"
10+
11+
"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
12+
"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers"
13+
"github.com/stretchr/testify/assert"
14+
"github.com/stretchr/testify/require"
15+
)
16+
17+
// requireLastCollectedAt asserts recommendations_state.last_collected_at
18+
// equals want.
19+
func requireLastCollectedAt(ctx context.Context, t *testing.T, store *config.PostgresStore, want time.Time) {
20+
t.Helper()
21+
fr, err := store.GetRecommendationsFreshness(ctx)
22+
require.NoError(t, err)
23+
require.NotNil(t, fr.LastCollectedAt)
24+
assert.True(t, want.Equal(*fr.LastCollectedAt),
25+
"last_collected_at = %s, want %s", fr.LastCollectedAt.UTC(), want)
26+
}
27+
28+
// TestPostgresStore_UpsertRecommendations_LastCollectedAtNeverMovesBackwards
29+
// pins issue #111: a collection that started earlier but commits later must
30+
// not move last_collected_at back to its older timestamp.
31+
func TestPostgresStore_UpsertRecommendations_LastCollectedAtNeverMovesBackwards(t *testing.T) {
32+
ctx := context.Background()
33+
store := setupRecommendationsStore(ctx, t)
34+
35+
newer := time.Now().UTC().Truncate(time.Second)
36+
older := newer.Add(-3 * time.Minute)
37+
38+
require.NoError(t, store.UpsertRecommendations(ctx, newer, nil, nil))
39+
requireLastCollectedAt(ctx, t, store, newer)
40+
41+
require.NoError(t, store.UpsertRecommendations(ctx, older, nil, nil))
42+
requireLastCollectedAt(ctx, t, store, newer)
43+
44+
// A genuinely newer collection still advances it.
45+
newest := newer.Add(time.Minute)
46+
require.NoError(t, store.UpsertRecommendations(ctx, newest, nil, nil))
47+
requireLastCollectedAt(ctx, t, store, newest)
48+
}
49+
50+
// TestPostgresStore_UpsertRecommendations_LastCollectedAtConcurrentCommit
51+
// covers the race in issue #111 on real Postgres: the newer collection holds
52+
// the state row's lock and commits while the older one is blocked on it. The
53+
// blocked UPDATE must re-evaluate GREATEST against the committed newer value
54+
// rather than overwrite it with the older timestamp.
55+
func TestPostgresStore_UpsertRecommendations_LastCollectedAtConcurrentCommit(t *testing.T) {
56+
ctx := context.Background()
57+
db := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()).DB
58+
store := config.NewPostgresStore(db)
59+
60+
base := time.Now().UTC().Truncate(time.Second)
61+
older := base.Add(-2 * time.Minute)
62+
newer := base
63+
64+
require.NoError(t, store.UpsertRecommendations(ctx, base.Add(-time.Hour), nil, nil))
65+
66+
// The newer run's state write, held open so it owns the row lock.
67+
tx, err := db.Begin(ctx)
68+
require.NoError(t, err)
69+
t.Cleanup(func() { _ = tx.Rollback(context.Background()) })
70+
_, err = tx.Exec(ctx, `UPDATE recommendations_state SET last_collected_at = GREATEST(last_collected_at, $1) WHERE id = 1`, newer)
71+
require.NoError(t, err)
72+
73+
// The older run commits after it: its UPDATE blocks on the row lock.
74+
done := make(chan error, 1)
75+
go func() { done <- store.UpsertRecommendations(ctx, older, nil, nil) }()
76+
77+
require.Eventually(t, func() bool {
78+
var waiting int
79+
if qerr := db.QueryRow(ctx, `SELECT count(*) FROM pg_locks WHERE NOT granted`).Scan(&waiting); qerr != nil {
80+
return false
81+
}
82+
return waiting > 0
83+
}, 10*time.Second, 20*time.Millisecond, "older run's UPDATE never blocked on the state row lock")
84+
85+
require.NoError(t, tx.Commit(ctx))
86+
87+
select {
88+
case err := <-done:
89+
require.NoError(t, err)
90+
case <-time.After(10 * time.Second):
91+
t.Fatal("older run's UpsertRecommendations did not finish after the newer run committed")
92+
}
93+
94+
requireLastCollectedAt(ctx, t, store, newer)
95+
}

‎internal/scheduler/scheduler.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,12 @@ func joinProviderErrors(failed map[string]string) string {
467467
// CloudAccountID. The *sweepGap is non-nil when some account's sweep was
468468
// incomplete (its rows are kept; see tolerateIncompleteSweep). Provider-level
469469
// errors propagate unchanged.
470+
//
471+
// A name that is not one of the known providers (e.g. "AWS" or "azure " left
472+
// in EnabledProviders) is an error, so the merge records it as a failed
473+
// provider and persistCollection writes last_collection_error. Returning nil
474+
// here reported a provider that was never contacted as a clean, successful
475+
// collection of zero recommendations (issue #111).
470476
func (s *Scheduler) collectProviderRecommendations(ctx context.Context, providerName string, globalCfg *config.GlobalConfig) ([]config.RecommendationRecord, []string, *sweepGap, error) {
471477
switch providerName {
472478
case "aws":
@@ -476,8 +482,7 @@ func (s *Scheduler) collectProviderRecommendations(ctx context.Context, provider
476482
case "gcp":
477483
return s.collectGCPRecommendations(ctx, globalCfg)
478484
default:
479-
logging.Warnf("Unknown provider: %s", providerName)
480-
return nil, nil, nil, nil
485+
return nil, nil, nil, fmt.Errorf("unknown provider %q (valid: aws, azure, gcp)", providerName)
481486
}
482487
}
483488

‎internal/scheduler/scheduler_test.go‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -466,17 +466,25 @@ func TestScheduler_CollectRecommendations_ParallelProviders(t *testing.T) {
466466
})
467467
}
468468

469+
// TestScheduler_CollectRecommendations_UnknownProvider pins issue #111: a
470+
// provider name the scheduler does not recognize (wrong case, trailing space)
471+
// must be reported as a failed provider and recorded in
472+
// last_collection_error, not as a clean collection of zero recommendations.
469473
func TestScheduler_CollectRecommendations_UnknownProvider(t *testing.T) {
470474
ctx := context.Background()
471475
mockStore := new(MockConfigStore)
472476
mockEmail := new(MockEmailSender)
473477
mockFactory := new(MockProviderFactory)
474478

475479
globalCfg := &config.GlobalConfig{
476-
EnabledProviders: []string{"unknown_provider"},
480+
EnabledProviders: []string{"AWS", "azure "},
477481
}
478482

479483
mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil)
484+
var recordedErr string
485+
mockStore.On("SetRecommendationsCollectionError", mock.Anything, mock.AnythingOfType("string")).
486+
Run(func(args mock.Arguments) { recordedErr = args.String(1) }).
487+
Return(nil).Once()
480488

481489
scheduler := &Scheduler{
482490
config: mockStore,
@@ -489,6 +497,12 @@ func TestScheduler_CollectRecommendations_UnknownProvider(t *testing.T) {
489497
require.NoError(t, err)
490498

491499
assert.Equal(t, 0, result.Recommendations)
500+
assert.Empty(t, result.SuccessfulProviders, "an unknown provider was never contacted, so it must not count as successful")
501+
assert.Contains(t, result.FailedProviders, "AWS")
502+
assert.Contains(t, result.FailedProviders, "azure ")
503+
mockStore.AssertCalled(t, "SetRecommendationsCollectionError", mock.Anything, mock.AnythingOfType("string"))
504+
assert.Contains(t, recordedErr, `unknown provider "AWS"`)
505+
assert.Contains(t, recordedErr, `unknown provider "azure "`)
492506
}
493507

494508
func TestScheduler_CollectAWSRecommendations(t *testing.T) {
@@ -576,10 +590,10 @@ func TestScheduler_CollectProviderRecommendations(t *testing.T) {
576590
provider string
577591
expectError bool
578592
}{
579-
{"aws", true}, // ambient fallback fails via factory error
580-
{"azure", false}, // no accounts → nil, nil
581-
{"gcp", false}, // no accounts → nil, nil
582-
{"unknown", false},
593+
{"aws", true}, // ambient fallback fails via factory error
594+
{"azure", false}, // no accounts → nil, nil
595+
{"gcp", false}, // no accounts → nil, nil
596+
{"unknown", true}, // not a provider the scheduler can collect (#111)
583597
}
584598

585599
for _, tt := range tests {

‎internal/server/handler.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,13 @@ var scheduledEventActions = map[string]ScheduledTaskType{
100100
func (app *Application) runScheduledTask(ctx context.Context, taskType ScheduledTaskType, params ScheduledTaskParams) (any, error) {
101101
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
102102

103+
// Reject an unknown task before connecting or taking an advisory lock
104+
// keyed on its name. The Lambda and HTTP transports already validate at
105+
// their boundary; this covers the --task CLI and any future caller (#111).
106+
if _, err := ParseScheduledTaskType(string(taskType)); err != nil {
107+
return nil, err
108+
}
109+
103110
if err := app.ensureDB(ctx); err != nil {
104111
return nil, fmt.Errorf("database connection failed: %w", err)
105112
}

‎internal/server/health.go‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,25 @@ type HealthStatus struct {
2222
}
2323

2424
// CheckResult represents the result of a health check.
25+
//
26+
// /health and /ready are unauthenticated, so Message is always a fixed string
27+
// and never carries a raw error: pgx and golang-migrate errors name the
28+
// database host, user, migration file and schema objects (issue #111). A
29+
// failed check sets Reason to a stable code instead, and the error itself goes
30+
// to the server log only.
2531
type CheckResult struct {
2632
Status string `json:"status"`
2733
Message string `json:"message,omitempty"`
34+
Reason string `json:"reason,omitempty"`
2835
}
2936

37+
// Stable reason codes a failed health check reports in CheckResult.Reason.
38+
const (
39+
healthReasonMigrationFailed = "migration_failed"
40+
healthReasonDBUnreachable = "db_unreachable"
41+
healthReasonAuthUnreachable = "auth_store_unreachable"
42+
)
43+
3044
// handleHealthCheck returns the liveness status of the application.
3145
//
3246
// This endpoint is the LIVENESS contract: it always answers HTTP 200 so
@@ -160,7 +174,8 @@ func (app *Application) checkMigrations() CheckResult {
160174
case finishedAt.IsZero():
161175
return CheckResult{Status: "pending", Message: "migrations have not run yet"}
162176
case err != nil:
163-
return CheckResult{Status: "failed", Message: err.Error()}
177+
// ensureDB logged the error when the run failed.
178+
return CheckResult{Status: "failed", Message: "Database migrations failed", Reason: healthReasonMigrationFailed}
164179
default:
165180
return CheckResult{
166181
Status: "healthy",
@@ -191,9 +206,11 @@ func (app *Application) checkConfigStore(ctx context.Context, configStore config
191206
// If using PostgreSQL, check database connection health
192207
if db != nil {
193208
if err := db.HealthCheck(ctx); err != nil {
209+
log.Printf("health: database health check failed: %v", err)
194210
return CheckResult{
195211
Status: "unhealthy",
196-
Message: fmt.Sprintf("Database health check failed: %v", err),
212+
Message: "Database health check failed",
213+
Reason: healthReasonDBUnreachable,
197214
}
198215
}
199216
}
@@ -240,9 +257,11 @@ func (app *Application) checkAuthStore(ctx context.Context, authService *auth.Se
240257

241258
// Ping the database to verify connection is healthy
242259
if err := authService.Ping(ctx); err != nil {
260+
log.Printf("health: auth store ping failed: %v", err)
243261
return CheckResult{
244262
Status: "unhealthy",
245-
Message: fmt.Sprintf("Auth store ping failed: %v", err),
263+
Message: "Auth store ping failed",
264+
Reason: healthReasonAuthUnreachable,
246265
}
247266
}
248267

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
//go:build integration
2+
3+
package server
4+
5+
import (
6+
"context"
7+
"testing"
8+
"time"
9+
10+
"github.com/LeanerCloud/cloud-commitments-platform/internal/database"
11+
"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers"
12+
"github.com/stretchr/testify/assert"
13+
"github.com/stretchr/testify/require"
14+
)
15+
16+
// TestCheckConfigStore_DBFailureHidesRawError drives the config-store check
17+
// against a real pgx pool whose ping fails, and pins issue #111: the result
18+
// carries a fixed message and the db_unreachable reason, not the pgx error.
19+
func TestCheckConfigStore_DBFailureHidesRawError(t *testing.T) {
20+
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
21+
defer cancel()
22+
pg := testhelpers.RequirePostgresContainer(ctx, t)
23+
24+
conn, err := database.NewConnection(ctx, pg.Config, nil)
25+
require.NoError(t, err)
26+
conn.Close()
27+
require.Error(t, conn.HealthCheck(ctx), "a closed pool must fail its health check")
28+
29+
// Through collectHealth rather than checkConfigStore directly, so the
30+
// test does not depend on that helper's parameter list.
31+
app := &Application{Config: &mockConfigStoreForHealth{}, DB: conn, Auth: createHealthyAuthService()}
32+
got := app.collectHealth(ctx)
33+
assert.Equal(t, "degraded", got.Status)
34+
assert.Equal(t, CheckResult{Status: "unhealthy", Message: "Database health check failed", Reason: healthReasonDBUnreachable}, got.Checks["config_store"])
35+
}

0 commit comments

Comments
 (0)