diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index f2afe17a4..c2c5d53dc 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -143,7 +143,7 @@ compute_platform = "lambda" # Lambda configuration lambda_architecture = "arm64" lambda_memory_size = 512 -lambda_timeout = 60 +lambda_timeout = 300 # 300-900: covers the 270s exchange/purchase budget # Database configuration (Aurora Serverless v2) database_engine_version = "16.4" diff --git a/internal/api/ri_exchange_detached.go b/internal/api/ri_exchange_detached.go index 7485306e2..7b872ff61 100644 --- a/internal/api/ri_exchange_detached.go +++ b/internal/api/ri_exchange_detached.go @@ -15,7 +15,9 @@ import ( // run on contexts that keep the request's values but drop its cancellation, // each bounded by its own budget so a hung provider or database cannot pin the // goroutine forever. On Lambda the invocation itself is still bounded by the -// function timeout, which no context can extend. +// function timeout, which no context can extend, so that timeout must cover +// the sum of both budgets plus a 30s margin (lambda_timeout >= 300, enforced +// by Terraform validation and TestLambdaTimeoutCoversRIExchangeBudget, #913). // // The detaching itself is ctxutil.DetachedWithTimeout, shared with the // purchase path (#706 / #789). diff --git a/internal/api/ri_exchange_detached_test.go b/internal/api/ri_exchange_detached_test.go index 4a82ce5de..b76c89c43 100644 --- a/internal/api/ri_exchange_detached_test.go +++ b/internal/api/ri_exchange_detached_test.go @@ -9,6 +9,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" azurecompute "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/testutil/lambdatimeout" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -40,11 +41,19 @@ func (p *ctxProbe) assertDetached(t *testing.T, start time.Time) { "canceling the request must not cancel an exchange already submitted to the provider") require.True(t, p.hasDeadline, "the detached exchange must still be bounded by its own timeout") // A literal rather than riExchangeExecutionTimeout, so the test pins the - // budget itself: it must stay inside the documented lambda_timeout >= 270s - // requirement (4m execution + 30s terminal save). + // budget itself: with the terminal save it must stay inside the lambda_timeout + // >= 300s floor (4m execution + 30s terminal save + 30s margin, #913). assert.WithinDuration(t, start.Add(4*time.Minute), p.deadline, 5*time.Second) } +// TestLambdaTimeoutCoversRIExchangeBudget keeps every shipped AWS Lambda +// timeout above the detached RI exchange budget. The provider call and the +// terminal ledger write run back to back after the request, and Lambda ends +// the invocation at its timeout whatever the context deadline says (#913). +func TestLambdaTimeoutCoversRIExchangeBudget(t *testing.T) { + lambdatimeout.AssertCovers(t, "../..", riExchangeExecutionTimeout+riExchangeTerminalSaveTimeout) +} + // liveCtx matches a context that has not been canceled and carries the 30s // terminal-save budget, so a mock expectation fails when a terminal write is // attempted on the dead request context, as a real database driver would. diff --git a/internal/purchase/execute_detached_test.go b/internal/purchase/execute_detached_test.go index 023c93b7f..00c7f9e2f 100644 --- a/internal/purchase/execute_detached_test.go +++ b/internal/purchase/execute_detached_test.go @@ -9,6 +9,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/testutil/lambdatimeout" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/sts" "github.com/stretchr/testify/assert" @@ -16,6 +17,14 @@ import ( "github.com/stretchr/testify/require" ) +// TestLambdaTimeoutCoversPurchaseBudget keeps every shipped AWS Lambda timeout +// above the detached purchase budget (execution, then the terminal save). +// Lambda ends the invocation at its timeout whatever the context deadline +// says, so a shorter timeout cuts a purchase off mid-flight (#913). +func TestLambdaTimeoutCoversPurchaseBudget(t *testing.T) { + lambdatimeout.AssertCovers(t, "../..", purchaseExecutionTimeout+terminalSaveTimeout) +} + // detachFixture wires a direct single-account AWS purchase (issue #706). type detachFixture struct { manager *Manager diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 645c2a7aa..2f28c1a0c 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -229,7 +229,11 @@ func isMultiAccountAckable(execErr error) bool { } const ( - // Combined execution and persistence budgets require lambda_timeout >= 270s. + // The two budgets run back to back (270s), and on AWS Lambda the function + // timeout ends the invocation whatever these deadlines say, so the Lambda + // timeout must cover their sum plus a 30s margin (lambda_timeout >= 300, + // enforced by Terraform validation and TestLambdaTimeoutCoversPurchaseBudget, + // #913). purchaseExecutionTimeout = 4 * time.Minute terminalSaveTimeout = 30 * time.Second ) diff --git a/internal/testutil/lambdatimeout/lambdatimeout.go b/internal/testutil/lambdatimeout/lambdatimeout.go new file mode 100644 index 000000000..0436eba3f --- /dev/null +++ b/internal/testutil/lambdatimeout/lambdatimeout.go @@ -0,0 +1,150 @@ +// Package lambdatimeout pins the AWS Lambda timeouts the repository ships +// against the Go budgets for work detached from the request (#913). It is a +// separate package from testutil so packages that testutil imports (such as +// purchase) can use it without an import cycle. +package lambdatimeout + +import ( + "fmt" + "os" + "path/filepath" + "regexp" + "strconv" + "testing" + "time" +) + +// Margin is the time an AWS Lambda invocation needs on top of a +// detached budget for the request work that runs before the detach (auth, +// database reads, quote lookups) and for writing the response. +const Margin = 30 * time.Second + +var ( + tfModuleTimeoutVar = regexp.MustCompile(`(?s)variable "timeout" \{.*?\n\}`) + tfRootTimeoutVar = regexp.MustCompile(`(?s)variable "lambda_timeout" \{.*?\n\}`) + tfDefault = regexp.MustCompile(`(?m)^\s*default\s*=\s*(\d+)`) + tfModuleFloor = regexp.MustCompile(`var\.timeout\s*>=\s*(\d+)`) + tfRootFloor = regexp.MustCompile(`var\.lambda_timeout\s*>=\s*(\d+)`) + tfvarsLambdaTimeout = regexp.MustCompile(`(?m)^\s*lambda_timeout\s*=\s*(\d+)`) + cfnLambdaTimeout = regexp.MustCompile(`(?s)LambdaFunction:\s*\n\s*Type: AWS::Lambda::Function\b.*?\n\s{6}Timeout:\s*(\d+)`) +) + +// AssertCovers fails t unless every AWS Lambda timeout the +// repository ships covers budget plus Margin: the Terraform +// module and root variable defaults and their validation floors, every +// committed lambda_timeout tfvars value (including examples and the +// deployment guide), and the CloudFormation function's Timeout. Lambda ends an +// invocation at its timeout whatever a context deadline says, so detached +// work with a longer budget would be cut off mid-flight (#913). +// +// repoRoot is the repository root relative to the calling test's package. +func AssertCovers(t *testing.T, repoRoot string, budget time.Duration) { + t.Helper() + c := &checker{t: t, repoRoot: repoRoot, budget: budget, need: budget + Margin} + c.terraformVariables() + c.tfvarsAndDocs() + c.one("cloudformation/stacks/CUDly/template.yaml LambdaFunction Timeout", cfnLambdaTimeout, + c.read("cloudformation/stacks/CUDly/template.yaml")) +} + +// checker holds the state shared by the AssertCovers checks. +type checker struct { + t *testing.T + repoRoot string + budget time.Duration + need time.Duration +} + +func (c *checker) check(source string, secs int) { + c.t.Helper() + if got := time.Duration(secs) * time.Second; got < c.need { + c.t.Errorf("%s: Lambda timeout %v is shorter than the %v detached budget plus %v margin (want >= %v)", + source, got, c.budget, Margin, c.need) + } +} + +func (c *checker) read(rel string) string { + c.t.Helper() + data, err := os.ReadFile(filepath.Join(c.repoRoot, rel)) // #nosec G304 -- fixed repo-relative paths + if err != nil { + c.t.Fatalf("reading %s: %v", rel, err) + } + return string(data) +} + +func (c *checker) one(source string, re *regexp.Regexp, text string) { + c.t.Helper() + m := re.FindStringSubmatch(text) + if m == nil { + c.t.Errorf("%s: not found (pattern %s); keep this check in sync with the IaC", source, re) + return + } + c.check(source, atoi(c.t, m[1])) +} + +// terraformVariables checks the module and root variable defaults and their +// validation floors. +func (c *checker) terraformVariables() { + c.t.Helper() + for _, v := range []struct { + file string + block *regexp.Regexp + floor *regexp.Regexp + }{ + {"terraform/modules/compute/aws/lambda/variables.tf", tfModuleTimeoutVar, tfModuleFloor}, + {"terraform/environments/aws/variables.tf", tfRootTimeoutVar, tfRootFloor}, + } { + block := v.block.FindString(c.read(v.file)) + if block == "" { + c.t.Errorf("%s: Lambda timeout variable not found", v.file) + continue + } + c.one(v.file+" default", tfDefault, block) + c.one(v.file+" validation floor", v.floor, block) + } +} + +// tfvarsAndDocs checks every committed lambda_timeout value in tfvars files, +// examples and the deployment guide. +func (c *checker) tfvarsAndDocs() { + c.t.Helper() + values := 0 + for _, f := range c.tfvarsFiles() { + for _, m := range tfvarsLambdaTimeout.FindAllStringSubmatch(c.read(f), -1) { + c.check(f+" lambda_timeout", atoi(c.t, m[1])) + values++ + } + } + if values < 4 { + c.t.Errorf("found %d lambda_timeout settings in tfvars/docs, want at least 4; keep this check in sync with the IaC", values) + } +} + +// tfvarsFiles lists the repo-relative files that may set lambda_timeout. +func (c *checker) tfvarsFiles() []string { + c.t.Helper() + files := []string{"docs/DEPLOYMENT.md"} + for _, pattern := range []string{"terraform/environments/aws/*.tfvars*", "terraform/profiles/aws/*.tfvars*"} { + matches, err := filepath.Glob(filepath.Join(c.repoRoot, pattern)) + if err != nil { + c.t.Fatalf("glob %s: %v", pattern, err) + } + for _, m := range matches { + rel, relErr := filepath.Rel(c.repoRoot, m) + if relErr != nil { + c.t.Fatalf("relative path for %s: %v", m, relErr) + } + files = append(files, rel) + } + } + return files +} + +func atoi(t *testing.T, s string) int { + t.Helper() + n, err := strconv.Atoi(s) + if err != nil { + t.Fatal(fmt.Errorf("parsing %q: %w", s, err)) + } + return n +} diff --git a/terraform/environments/aws/dev.tfvars.example b/terraform/environments/aws/dev.tfvars.example index a7c026ac2..225468c6a 100644 --- a/terraform/environments/aws/dev.tfvars.example +++ b/terraform/environments/aws/dev.tfvars.example @@ -26,7 +26,7 @@ compute_platform = "lambda" # Lambda settings (when compute_platform = "lambda") lambda_architecture = "arm64" # arm64 for Graviton (cheaper) lambda_memory_size = 512 -lambda_timeout = 60 # Seconds +lambda_timeout = 300 # Seconds, 300-900 (covers the 270s exchange/purchase budget) lambda_reserved_concurrency = -1 # -1 = no limit lambda_log_retention_days = 30 diff --git a/terraform/environments/aws/variables.tf b/terraform/environments/aws/variables.tf index c0acf8168..6c73d1196 100644 --- a/terraform/environments/aws/variables.tf +++ b/terraform/environments/aws/variables.tf @@ -229,9 +229,16 @@ variable "lambda_memory_size" { } variable "lambda_timeout" { - description = "Lambda timeout in seconds" + # Feeds module.compute_lambda's timeout; see the coupling note on that + # variable (terraform/modules/compute/aws/lambda/variables.tf, #913). + description = "Lambda timeout in seconds (300-900; must cover the 270s detached exchange/purchase budget)" type = number - default = 30 + default = 300 + + validation { + condition = var.lambda_timeout >= 300 && var.lambda_timeout <= 900 + error_message = "lambda_timeout must be between 300 and 900 seconds: RI exchanges and purchases run up to 270s (4m execution + 30s terminal save) after the request, and Lambda cuts the invocation off at this timeout (#913)." + } } diff --git a/terraform/modules/compute/aws/lambda/tests/timeout.tftest.hcl b/terraform/modules/compute/aws/lambda/tests/timeout.tftest.hcl new file mode 100644 index 000000000..7cba20f5f --- /dev/null +++ b/terraform/modules/compute/aws/lambda/tests/timeout.tftest.hcl @@ -0,0 +1,72 @@ +# Lambda timeout bounds (#913). Lambda ends an invocation at its timeout +# whatever a context deadline says, so the function must outlive the 270s of +# RI exchange / purchase work detached from the request. The floor is pinned +# against the Go budgets by TestLambdaTimeoutCovers* in internal/api and +# internal/purchase. +mock_provider "aws" {} + +variables { + stack_name = "cudly-test" + permissions_boundary_arn = "arn:aws:iam::123456789012:policy/cudly-boundary" + environment = "dev" + region = "us-east-1" + image_uri = "123456789012.dkr.ecr.us-east-1.amazonaws.com/cudly:test" + database_host = "db.example.com" + database_name = "cudly" + database_username = "cudly" + database_password_secret_arn = "arn:aws:secretsmanager:us-east-1:123456789012:secret:db" + admin_email = "admin@example.com" + allowed_origins = ["https://app.example.com"] +} + +run "default_covers_detached_budget" { + command = plan + + assert { + condition = aws_lambda_function.main.timeout == 300 + error_message = "The default Lambda timeout must be 300s (270s detached budget + 30s margin)." + } +} + +run "rejects_old_30s_default" { + command = plan + + variables { + timeout = 30 + } + + expect_failures = [var.timeout] +} + +run "rejects_just_below_floor" { + command = plan + + variables { + timeout = 299 + } + + expect_failures = [var.timeout] +} + +run "rejects_above_lambda_max" { + command = plan + + variables { + timeout = 901 + } + + expect_failures = [var.timeout] +} + +run "accepts_lambda_max" { + command = plan + + variables { + timeout = 900 + } + + assert { + condition = aws_lambda_function.main.timeout == 900 + error_message = "900s (the Lambda maximum) must be accepted." + } +} diff --git a/terraform/modules/compute/aws/lambda/variables.tf b/terraform/modules/compute/aws/lambda/variables.tf index 4d858da30..532c266a1 100644 --- a/terraform/modules/compute/aws/lambda/variables.tf +++ b/terraform/modules/compute/aws/lambda/variables.tf @@ -49,9 +49,22 @@ variable "memory_size" { } variable "timeout" { - description = "Lambda timeout in seconds" + # Must cover the longest work the handler detaches from the request: an RI + # exchange or purchase gets 4m of execution plus a 30s terminal save + # (riExchangeExecutionTimeout / purchaseExecutionTimeout + their terminal + # save budgets). Lambda ends the invocation at this timeout whatever the + # context deadline says, so a shorter value cuts that work off mid-flight + # (#913). 300 = 270s of budget + 30s for the request work before it. + # Pinned against the Go constants by TestLambdaTimeoutCoversRIExchangeBudget + # and TestLambdaTimeoutCoversPurchaseBudget. + description = "Lambda timeout in seconds (300-900; must cover the 270s detached exchange/purchase budget)" type = number - default = 30 + default = 300 + + validation { + condition = var.timeout >= 300 && var.timeout <= 900 + error_message = "timeout must be between 300 and 900 seconds: RI exchanges and purchases run up to 270s (4m execution + 30s terminal save) after the request, and Lambda cuts the invocation off at this timeout (#913)." + } } variable "database_host" { diff --git a/terraform/profiles/aws/dev.tfvars.example b/terraform/profiles/aws/dev.tfvars.example index 3e90b8355..62b4b16f8 100644 --- a/terraform/profiles/aws/dev.tfvars.example +++ b/terraform/profiles/aws/dev.tfvars.example @@ -15,7 +15,7 @@ compute_platform = "lambda" # Lambda configuration lambda_architecture = "arm64" lambda_memory_size = 512 -lambda_timeout = 60 +lambda_timeout = 300 # 300-900: covers the 270s exchange/purchase budget lambda_reserved_concurrency = -1 lambda_log_retention_days = 7 # Function URL auth_type is derived from enable_cdn — see compute.tf local diff --git a/terraform/profiles/aws/prod.tfvars.example b/terraform/profiles/aws/prod.tfvars.example index a61adacfd..659716676 100644 --- a/terraform/profiles/aws/prod.tfvars.example +++ b/terraform/profiles/aws/prod.tfvars.example @@ -15,7 +15,7 @@ compute_platform = "lambda" # or "fargate" # Lambda configuration lambda_architecture = "arm64" lambda_memory_size = 1024 # More memory for production -lambda_timeout = 60 +lambda_timeout = 300 # 300-900: covers the 270s exchange/purchase budget lambda_reserved_concurrency = -1 lambda_log_retention_days = 30 # Function URL auth_type is derived from enable_cdn — see compute.tf local