From e4d9fb79dc87c4b2827b86777adba0601c65a17c Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 21:58:59 +0100 Subject: [PATCH] fix(server): thread one IsLambda value through every Application consumer NewApplicationFromDeps built the pre-DB scheduler with a live runtime.IsLambda() instead of the ApplicationConfig it was handed, while the post-DB scheduler (ensureDB) used app.appConfig.IsLambda. A caller passing ApplicationConfig{IsLambda: true} without AWS_LAMBDA_RUNTIME_API got a container-mode scheduler until ensureDB succeeded (permanently if the database stayed unreachable), so ListRecommendations could spawn the detached 5-minute background refresh inside a Lambda invocation. The API handler had the same split: the RI utilization SWR gate and the stale-execution sweep each read runtime.IsLambda() live. Now ApplicationConfig.IsLambda (captured once by LoadApplicationConfig) is the only source: the pre-DB scheduler takes cfg.IsLambda, and a new HandlerConfig.IsLambda carries it into both NewHandler calls, where the SWR cache and the sweep read h.isLambda. Scheduler.IsLambda() and Handler.IsLambda() expose the constructed mode for wiring assertions. Tests set the Lambda env vars to the opposite of the config and assert the scheduler and handler follow the config, pre-DB (unit) and after ensureDB (integration). Closes #110 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler.go | 19 ++-- internal/api/handler_history.go | 14 +-- ...dler_history_actionable_starvation_test.go | 7 +- internal/api/handler_history_test.go | 24 +++-- internal/api/ri_utilization_cache_test.go | 25 +++++ internal/api/types.go | 5 + internal/scheduler/scheduler.go | 8 ++ internal/server/app.go | 7 +- .../server/app_islambda_integration_test.go | 67 ++++++++++++ internal/server/app_islambda_test.go | 101 ++++++++++++++++++ 10 files changed, 250 insertions(+), 27 deletions(-) create mode 100644 internal/server/app_islambda_integration_test.go create mode 100644 internal/server/app_islambda_test.go diff --git a/internal/api/handler.go b/internal/api/handler.go index c4086844e..4285f405a 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -17,7 +17,6 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" "github.com/LeanerCloud/cloud-commitments-platform/internal/email" "github.com/LeanerCloud/cloud-commitments-platform/internal/oidc" - "github.com/LeanerCloud/cloud-commitments-platform/internal/runtime" "github.com/aws/aws-lambda-go/events" "github.com/aws/aws-sdk-go-v2/aws" awsconfig "github.com/aws/aws-sdk-go-v2/config" @@ -44,6 +43,7 @@ type Handler struct { signer oidc.Signer // Optional: OIDC issuer signer (backed by cloud KMS) insurance InsuranceProvider // Optional: Archera comparison (nil = off) issuerURL string // Canonical OIDC issuer URL (falls back to dashboardURL / request domain) + isLambda bool // Lambda mode from HandlerConfig.IsLambda; gates background goroutines awsCfgOnce sync.Once // guards one-time loading of the base AWS config awsCfg aws.Config // cached base AWS config (no region override) @@ -148,17 +148,23 @@ type Handler struct { // getRIUtilizationCache returns the Postgres-backed TTL cache for Cost // Explorer results, lazy-initialized on first call so tests that never -// exercise the RI Exchange paths don't need to wire it up. Lambda -// detection happens here (once) via runtime.IsLambda so SWR is gated -// off on Lambda where background goroutines freeze between -// invocations. +// exercise the RI Exchange paths don't need to wire it up. The Lambda +// mode comes from h.isLambda (HandlerConfig.IsLambda, i.e. the +// Application's captured value, issue #110) so SWR is gated off on +// Lambda where background goroutines freeze between invocations. func (h *Handler) getRIUtilizationCache() *riUtilizationCache { h.riUtilizationCacheOnce.Do(func() { - h.riUtilizationCache = newRIUtilizationCache(h.config, runtime.IsLambda()) + h.riUtilizationCache = newRIUtilizationCache(h.config, h.isLambda) }) return h.riUtilizationCache } +// IsLambda reports the Lambda mode this handler was constructed with +// (HandlerConfig.IsLambda). It never re-reads the environment. +func (h *Handler) IsLambda() bool { + return h.isLambda +} + // NewHandler creates a new API handler. func NewHandler(cfg HandlerConfig) *Handler { corsOrigin := cfg.CORSAllowedOrigin @@ -190,6 +196,7 @@ func NewHandler(cfg HandlerConfig) *Handler { insurance: cfg.Insurance, commitmentOpts: cfg.CommitmentOpts, encryptionKeySource: cfg.EncryptionKeySource, + isLambda: cfg.IsLambda, } // Pre-load API key (with a 5s timeout to avoid stalling cold-start indefinitely) diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 78242ab8c..2dc94758b 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -11,7 +11,6 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" - "github.com/LeanerCloud/cloud-commitments-platform/internal/runtime" "github.com/aws/aws-lambda-go/events" "github.com/jackc/pgx/v5" ) @@ -246,11 +245,12 @@ func isStaleExecution(exec config.PurchaseExecution) bool { // response is returned, so a background goroutine would be suspended // mid-sweep and stale rows could stay "pending" indefinitely (issue #1170). // There the sweep runs synchronously before the handler returns, mirroring -// the SWR cache's isLambda gate (ri_utilization_cache.go) and using the same -// runtime.IsLambda detection helper. The sweep is a handful of cheap UPDATEs, -// so the synchronous cost on Lambda is negligible. Errors are logged and -// skipped — a missed transition leaves the row "pending" until the next -// History load, which is better than failing the read response. +// the SWR cache's isLambda gate (ri_utilization_cache.go) and reading the same +// h.isLambda value (HandlerConfig.IsLambda, the Application's single captured +// answer, issue #110) rather than the live environment. The sweep is a handful +// of cheap UPDATEs, so the synchronous cost on Lambda is negligible. Errors are +// logged and skipped — a missed transition leaves the row "pending" until the +// next History load, which is better than failing the read response. // // The sweep is idempotent per execution ID: TransitionExecutionStatus is // guarded by the FROM-status list ("pending","notified"), so a concurrent @@ -266,7 +266,7 @@ func (h *Handler) expireStaleExecutions(staleExecs []config.PurchaseExecution) { logging.Warnf("history: sweeping %d of %d stale executions", config.DefaultListLimit, len(staleExecs)) staleExecs = staleExecs[:config.DefaultListLimit] } - if runtime.IsLambda() { + if h.isLambda { h.expireStaleExecutionsSweep(staleExecs) return } diff --git a/internal/api/handler_history_actionable_starvation_test.go b/internal/api/handler_history_actionable_starvation_test.go index 15efcfbb0..bdc611d8a 100644 --- a/internal/api/handler_history_actionable_starvation_test.go +++ b/internal/api/handler_history_actionable_starvation_test.go @@ -158,14 +158,17 @@ func TestHandler_getHistory_TruncatedPerClass(t *testing.T) { } func TestHandler_getHistory_StaleSweepBoundedToDefaultListLimit(t *testing.T) { - t.Setenv("AWS_LAMBDA_RUNTIME_API", "127.0.0.1:9001") ctx := context.Background() stale := executionsWithStatus(150, "pending", time.Now().Add(-8*24*time.Hour), "stale") store := newStarvationStore(ctx) store.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(stale, nil) store.On("ExpireExecutionAtomic", mock.Anything, mock.Anything, mock.Anything).Return(false, nil) - runStarvationHistory(t, ctx, store) + // Lambda mode (synchronous sweep) so the call count is final on return. + mockAuth, req := adminHistoryReq(ctx) + handler := &Handler{auth: mockAuth, config: store, isLambda: true} + _, err := handler.getHistory(ctx, req, map[string]string{}) + require.NoError(t, err) store.AssertNumberOfCalls(t, "ExpireExecutionAtomic", config.DefaultListLimit) } diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index 0e57053fa..eab7cfab2 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -1941,9 +1941,11 @@ func TestSummarizePurchaseHistory_CancelPendingDoesNotChangeKPIs(t *testing.T) { // execution environment freezes as soon as the response is returned, so the // sweep must complete synchronously before getHistory returns; on long-running // servers it must stay asynchronous so the read response is never blocked on -// the transitions. Detection reuses runtime.IsLambda (AWS_LAMBDA_RUNTIME_API), -// the same helper the SWR cache goroutine gate uses, so the sub-tests flip -// that env var via t.Setenv. +// the transitions. The mode comes from Handler.isLambda (HandlerConfig.IsLambda, +// the Application's single captured value), the same field the SWR cache +// goroutine gate reads. Each sub-test sets AWS_LAMBDA_RUNTIME_API to the +// opposite of the handler's mode to prove the live environment is ignored +// (issue #110). func TestHandler_getHistory_ExpireIfStale_LambdaGuard(t *testing.T) { approverEmail := "ops@example.com" staleID := "stale-exec-lambda-guard" @@ -1961,9 +1963,9 @@ func TestHandler_getHistory_ExpireIfStale_LambdaGuard(t *testing.T) { } t.Run("Lambda: sweep completes synchronously before the handler returns", func(t *testing.T) { - // Any non-empty value marks the process as running on Lambda - // (runtime.IsLambda checks presence, not content). - t.Setenv("AWS_LAMBDA_RUNTIME_API", "127.0.0.1:9001") + // The environment says "not Lambda"; the handler's configured mode + // must win (issue #110). + t.Setenv("AWS_LAMBDA_RUNTIME_API", "") ctx := context.Background() mockStore := new(MockConfigStore) @@ -1978,7 +1980,7 @@ func TestHandler_getHistory_ExpireIfStale_LambdaGuard(t *testing.T) { mockStore.On("ExpireExecutionAtomic", mock.Anything, mock.Anything, staleID).Return(true, nil).Once() mockAuth, req := adminHistoryReq(ctx) - handler := &Handler{auth: mockAuth, config: mockStore} + handler := &Handler{auth: mockAuth, config: mockStore, isLambda: true} result, err := handler.getHistory(ctx, req, map[string]string{}) require.NoError(t, err) @@ -1995,9 +1997,9 @@ func TestHandler_getHistory_ExpireIfStale_LambdaGuard(t *testing.T) { }) t.Run("non-Lambda: sweep stays asynchronous and never blocks the response", func(t *testing.T) { - // Explicitly clear the marker so the test is deterministic even if - // the outer environment sets it; t.Setenv restores it afterwards. - t.Setenv("AWS_LAMBDA_RUNTIME_API", "") + // The environment says "Lambda"; the handler's configured mode must + // win (issue #110). t.Setenv restores the variable afterwards. + t.Setenv("AWS_LAMBDA_RUNTIME_API", "127.0.0.1:9001") ctx := context.Background() mockStore := new(MockConfigStore) @@ -2020,7 +2022,7 @@ func TestHandler_getHistory_ExpireIfStale_LambdaGuard(t *testing.T) { }).Return(true, nil).Once() mockAuth, req := adminHistoryReq(ctx) - handler := &Handler{auth: mockAuth, config: mockStore} + handler := &Handler{auth: mockAuth, config: mockStore, isLambda: false} done := make(chan struct{}) var result any diff --git a/internal/api/ri_utilization_cache_test.go b/internal/api/ri_utilization_cache_test.go index 958fcff17..f66b1eeb7 100644 --- a/internal/api/ri_utilization_cache_test.go +++ b/internal/api/ri_utilization_cache_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "sync" "sync/atomic" "testing" @@ -330,3 +331,27 @@ func TestRIUtilizationCache_CorruptPayloadFallsThrough(t *testing.T) { t.Fatalf("stored payload still corrupt after upsert: %v", err) } } + +// TestHandler_getRIUtilizationCache_FollowsHandlerLambdaMode guards issue +// #110: the SWR gate must take the handler's configured Lambda mode +// (HandlerConfig.IsLambda), not a live read of AWS_LAMBDA_RUNTIME_API. Each +// case sets the environment to the opposite of the configured mode. +func TestHandler_getRIUtilizationCache_FollowsHandlerLambdaMode(t *testing.T) { + for _, isLambda := range []bool{true, false} { + t.Run(fmt.Sprintf("isLambda=%t", isLambda), func(t *testing.T) { + env := "" + if !isLambda { + env = "127.0.0.1:9001" + } + t.Setenv("AWS_LAMBDA_RUNTIME_API", env) + + h := NewHandler(HandlerConfig{CORSAllowedOrigin: "https://islambda.example.test", IsLambda: isLambda}) + if got := h.IsLambda(); got != isLambda { + t.Fatalf("Handler.IsLambda() = %t, want %t", got, isLambda) + } + if got := h.getRIUtilizationCache().isLambda; got != isLambda { + t.Fatalf("RI utilization cache isLambda = %t, want %t (must follow HandlerConfig.IsLambda)", got, isLambda) + } + }) + } +} diff --git a/internal/api/types.go b/internal/api/types.go index 9570dca8d..f29e11098 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -54,6 +54,11 @@ type HandlerConfig struct { APIKeySecretARN string EncryptionKeySource string EnableDashboard bool + // IsLambda is the caller's (Application's) single answer to "am I on + // Lambda". It gates the background goroutines (RI utilization SWR + // refresh, stale-execution sweep) that would freeze mid-flight on Lambda. + // The handler never re-reads the environment itself (issue #110). + IsLambda bool } // CommitmentOptsInterface lets us swap the real *commitmentopts.Service for diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 194e492f7..408e2cbd4 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -123,6 +123,14 @@ func NewScheduler(cfg SchedulerConfig) *Scheduler { } } +// IsLambda reports the Lambda mode this scheduler was constructed with +// (SchedulerConfig.IsLambda). It never re-reads the environment: the value is +// owned by whoever built the scheduler, so callers can assert that the mode +// they threaded in is the one that gates background goroutines (issue #110). +func (s *Scheduler) IsLambda() bool { + return s.isLambda +} + // cacheTTLFromEnv parses CUDLY_RECOMMENDATION_CACHE_TTL using // time.ParseDuration. Falls back to defaultCacheTTL on parse error (with // a warning) so a mis-set env doesn't take the feature offline. diff --git a/internal/server/app.go b/internal/server/app.go index 4a6f23103..8506659ce 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -491,7 +491,10 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext PurchaseManager: purchaseManager, EmailSender: deps.EmailSender, STSClient: deps.STSClient, - IsLambda: runtime.IsLambda(), + // cfg.IsLambda, never a live runtime.IsLambda(): the Application has + // exactly one answer to "am I on Lambda", shared with the post-DB + // scheduler in ensureDB and the API handler (issue #110). + IsLambda: cfg.IsLambda, }) purchaseManager.SetRecommendationSource(sched) @@ -531,6 +534,7 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext OIDCSigner: signer, OIDCIssuerURL: resolveOIDCIssuerURL(cfg), Insurance: archeraProvider, + IsLambda: cfg.IsLambda, }) log.Printf("CUDly Server initialization complete") @@ -899,6 +903,7 @@ func (app *Application) reinitializeAfterConnect(ctx context.Context, dbConn *da CommitmentOpts: commitmentOpts, EncryptionKeySource: app.encKeySource, Insurance: app.archera, + IsLambda: app.appConfig.IsLambda, }) if app.API == nil { return fmt.Errorf("failed to create API handler") diff --git a/internal/server/app_islambda_integration_test.go b/internal/server/app_islambda_integration_test.go new file mode 100644 index 000000000..f12c80a05 --- /dev/null +++ b/internal/server/app_islambda_integration_test.go @@ -0,0 +1,67 @@ +//go:build integration + +package server + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/require" +) + +// TestEnsureDB_IsLambdaSingleSourceOfTruth extends the issue #110 regression +// to the post-DB wiring: after ensureDB rebuilds the scheduler and API handler, +// both must still follow ApplicationConfig.IsLambda while the environment says +// the opposite. +func TestEnsureDB_IsLambdaSingleSourceOfTruth(t *testing.T) { + for _, key := range []string{ + "CREDENTIAL_ENCRYPTION_KEY_SECRET_ARN", "CREDENTIAL_ENCRYPTION_KEY_SECRET_NAME", + "CREDENTIAL_ENCRYPTION_KEY_SECRET_ID", "CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY", + "CUDLY_SIGNING_KEY_ID", "CUDLY_SIGNING_KEY_VAULT_URL", "CUDLY_SIGNING_KEY_NAME", + "CUDLY_SIGNING_KEY_RESOURCE", "AWS_PROFILE", + } { + t.Setenv(key, "") + } + t.Setenv("CREDENTIAL_ENCRYPTION_KEY", strings.Repeat("12", 32)) + t.Setenv("CUDLY_ISSUER_URL", "https://islambda.example.test") + t.Setenv("CUDLY_SOURCE_CLOUD", "aws") + t.Setenv("SCHEDULED_TASK_AUTH_MODE", "disabled") + t.Setenv("AWS_EC2_METADATA_DISABLED", "true") + t.Setenv("AWS_ACCESS_KEY_ID", "local-islambda-test") + t.Setenv("AWS_SECRET_ACCESS_KEY", "local-islambda-test") + t.Setenv("AWS_REGION", "us-east-1") + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + pg, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, pg.Cleanup(context.Background())) }) + require.NoError(t, migrations.RunMigrations(ctx, pg.DB.Pool(), "../database/postgres/migrations", "", "")) + + for _, cfgIsLambda := range []bool{true, false} { + name := "config not Lambda, environment Lambda" + if cfgIsLambda { + name = "config Lambda, environment not Lambda" + } + t.Run(name, func(t *testing.T) { + setLambdaEnv(t, !cfgIsLambda) + cfg := ApplicationConfig{ + IsLambda: cfgIsLambda, DashboardURL: "https://islambda.example.test", + DefaultTerm: 3, DefaultCoverage: 80, + } + app, appErr := NewApplicationFromDeps(ctx, cfg, ExternalDeps{ + DBConfig: pg.Config, EmailSender: &noopEmailSender{}, + }) + require.NoError(t, appErr) + t.Cleanup(func() { require.NoError(t, app.Close()) }) + + require.NoError(t, app.ensureDB(ctx)) + require.NotNil(t, app.DB, "ensureDB must have rebuilt the post-DB wiring") + assertLambdaModeWiring(t, app, cfgIsLambda) + }) + } +} diff --git a/internal/server/app_islambda_test.go b/internal/server/app_islambda_test.go new file mode 100644 index 000000000..9736c7ce8 --- /dev/null +++ b/internal/server/app_islambda_test.go @@ -0,0 +1,101 @@ +package server + +import ( + "os" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database" + "github.com/LeanerCloud/cloud-commitments-platform/internal/scheduler" + "github.com/LeanerCloud/cloud-commitments-platform/internal/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// lambdaEnvVars are the variables the AWS Lambda runtime sets. The issue #110 +// tests set or clear all of them so the ambient environment always contradicts +// the ApplicationConfig under test. +var lambdaEnvVars = []string{ + "AWS_LAMBDA_RUNTIME_API", + "AWS_LAMBDA_FUNCTION_NAME", + "AWS_EXECUTION_ENV", + "LAMBDA_TASK_ROOT", +} + +// setLambdaEnv makes the process environment look like Lambda (onLambda) or +// like a container/local run (!onLambda). t.Setenv registers the restore +// before os.Unsetenv clears the variable, so the original value comes back +// after the test either way. +func setLambdaEnv(t *testing.T, onLambda bool) { + t.Helper() + values := map[string]string{ + "AWS_LAMBDA_RUNTIME_API": "127.0.0.1:9001", + "AWS_LAMBDA_FUNCTION_NAME": "cudly-islambda-test", + "AWS_EXECUTION_ENV": "AWS_Lambda_provided.al2023", + "LAMBDA_TASK_ROOT": "/var/task", + } + for _, key := range lambdaEnvVars { + if onLambda { + t.Setenv(key, values[key]) + continue + } + t.Setenv(key, "") + require.NoError(t, os.Unsetenv(key)) + } +} + +// assertLambdaModeWiring asserts every Lambda-mode consumer the Application +// owns follows want, whatever the environment says. +func assertLambdaModeWiring(t *testing.T, app *Application, want bool) { + t.Helper() + assert.Equal(t, want, app.appConfig.IsLambda, "Application config") + + sched, ok := app.Scheduler.(*scheduler.Scheduler) + require.True(t, ok, "app.Scheduler must be a *scheduler.Scheduler, got %T", app.Scheduler) + assert.Equal(t, want, sched.IsLambda(), "scheduler must follow ApplicationConfig.IsLambda, not the live environment") + + require.NotNil(t, app.API, "API handler") + assert.Equal(t, want, app.API.IsLambda(), "API handler must follow ApplicationConfig.IsLambda, not the live environment") +} + +// TestNewApplicationFromDeps_IsLambdaSingleSourceOfTruth is the regression +// test for issue #110: NewApplicationFromDeps built the pre-DB scheduler with +// a live runtime.IsLambda() (and the API handler read it on every call), so a +// caller-supplied ApplicationConfig.IsLambda that disagreed with the +// environment put the scheduler on the wrong path until ensureDB succeeded, +// or permanently if the database stayed unreachable. +func TestNewApplicationFromDeps_IsLambdaSingleSourceOfTruth(t *testing.T) { + tests := []struct { + name string + cfgIsLambda bool + envOnLambda bool + }{ + {"config Lambda, environment not Lambda", true, false}, + {"config not Lambda, environment Lambda", false, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("SCHEDULED_TASK_AUTH_MODE", "disabled") + setLambdaEnv(t, tt.envOnLambda) + + cfg := ApplicationConfig{ + Version: "islambda-test", + DefaultTerm: 3, + DefaultCoverage: 80, + // A configured issuer URL skips the Lambda function-URL + // lookup, so the test makes no AWS call in either mode. + DashboardURL: "https://islambda.example.test", + IsLambda: tt.cfgIsLambda, + } + deps := ExternalDeps{ + EmailSender: &noopEmailSender{}, + DBConfig: &database.Config{Host: "localhost", Port: 5432, Database: "cudly_test", User: "test", Password: "test", SSLMode: "disable"}, + } + + app, err := NewApplicationFromDeps(testutil.TestContext(t), cfg, deps) + require.NoError(t, err) + require.Nil(t, app.DB, "the pre-DB wiring is what this test covers") + + assertLambdaModeWiring(t, app, tt.cfgIsLambda) + }) + } +}