Repository navigation
Conversation
The AWS Lambda module and root env defaulted the function timeout to 30s, but RI exchanges and purchases run up to 270s after the request on detached contexts (4m execution + 30s terminal save). Lambda ends the invocation at its timeout whatever the context deadline says, so on a default deployment that work was cut off mid-flight. - Default timeout / lambda_timeout to 300s (270s budget + 30s margin) and validate 300-900 in both the module and the root env variable. - Pin every shipped timeout (TF defaults, validation floors, committed tfvars and examples, docs/DEPLOYMENT.md, CloudFormation) against the Go budget constants with TestLambdaTimeoutCoversRIExchangeBudget and TestLambdaTimeoutCoversPurchaseBudget, via a small cycle-free internal/testutil/lambdatimeout helper. - Add a mock-provider terraform test for the validation bounds. - Bump the 60s example/doc values to 300 and document the coupling next to the Go constants. Closes LeanerCloud#913 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 12 billable files and costs up to $3.00. Or wait 38 minutes for your next included review. View limit details
Comment |
Independent review verdict: CLEAN at 8046724Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. Read-only, with no cloud credentials and no plan or apply against a real backend. The PR body was treated as a claim. Coordinator: claude-cc-reviewer. Blocking findings: none. Detached budgets vs the Lambda timeout:
That leaves a 30s margin everywhere. No lower override remains, and prod was already 300. Also verified:
Note (non-blocking): What was attacked and how:
Deploy impact, so merge is held for an explicit owner OK:
Gate: labels are mirrored from #913. |
…o limit AssertCovers lives in a non-_test.go file, so the pre-commit gocyclo hook checks it (complexity 13). Split the Terraform-variable, tfvars/docs and CloudFormation checks into methods on a small checker type; behavior and messages are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
CI fix at 0692254. Run pre-commit hooks failed on gocyclo:
🤖 Generated with Claude Code |
|
Delta verdict, CLEAN at 0692254 (same reviewer, scope |
Root cause
On AWS Lambda the function timeout is a hard stop: Lambda ends the invocation when it is reached, whatever the Go context deadline says. The Lambda module (
terraform/modules/compute/aws/lambda/variables.tf,timeout) and the root env (terraform/environments/aws/variables.tf,lambda_timeout) both defaulted to 30s. RI exchanges (riExchangeExecutionTimeout4m +riExchangeTerminalSaveTimeout30s, #897) and purchases (purchaseExecutionTimeout4m +terminalSaveTimeout30s, #789) run up to 270s back to back on detached contexts after the request. On a default deployment that work was cut off at 30s, possibly after the provider call had been submitted but before the terminal ledger write.Two notes on scope:
scheduledTaskHTTPTimeoutfrom fix(server): detach HTTP-triggered scheduled tasks from the request context #895 applies only on the container HTTP server path (/api/scheduled/*ininternal/server/http.go, used by Cloud Run, Fargate and Container Apps). On Lambda, scheduled tasks come in as EventBridge invocations and run on the invocation context, so 270s is the largest detached budget on Lambda.Timeout: 900, so it needs no change. It is now covered by the same static check.Fix
timeout/lambda_timeoutgoes from 30 to 300s: the 270s budget plus 30s for the request work before the detach (auth, DB reads, quote lookup) and the response. This matches whatgithub-{dev,staging,prod}.tfvarsalready deploy, so CI-managed environments see no change.validationblock on both variables requires 300 to 900 seconds (900 is the Lambda maximum).TestLambdaTimeoutCoversRIExchangeBudget(internal/api) andTestLambdaTimeoutCoversPurchaseBudget(internal/purchase) compute the budget from the Go constants and check it, plus a 30s margin, against: both TF defaults, both validation floors, every committedlambda_timeoutinterraform/environments/aws/*.tfvars*,terraform/profiles/aws/*.tfvars*anddocs/DEPLOYMENT.md, and the CloudFormationLambdaFunctionTimeout. If someone raises a Go budget, CI fails until the IaC floor is raised too. The shared helper lives ininternal/testutil/lambdatimeout, which has no dependencies, becauseinternal/testutilimportspurchaseand would create an import cycle.terraform/modules/compute/aws/lambda/tests/timeout.tftest.hcl(mock provider) checks the default (300), rejects 30, 299 and 901, and accepts 900.docs/DEPLOYMENT.mdbecome 300, since they would now fail validation. The coupling is documented next to the Go constants and the TF variable.Cost and concurrency
Lambda bills for actual duration, not for the configured timeout, so a normal request costs the same. What changes is the worst case: a hung invocation can now bill and hold a concurrency slot for up to 300s instead of 30s. The default
lambda_reserved_concurrency = -1means it draws from the account's unreserved pool. CloudFront or the client may still give up on a long HTTP request at its own timeout (CloudFront's origin read timeout defaults to 30s). That is fine: the work is detached, and the invocation keeps running until it finishes or reaches the Lambda timeout.Breaking for operators
Anyone with a
lambda_timeoutbelow 300 in a local, gitignored tfvars file will now failterraform planwith a clear validation error. That is intended (#913). Raise it to 300 or more and re-apply. Nothing was applied or planned against a real account. Operators have to re-apply for the new default to take effect.Verification (static and local only)
TestLambdaTimeoutCoversRIExchangeBudgetfails on both defaults (30s), the missing floors and the four 60s example/doc values. The tftest failsdefault_covers_detached_budgetandrejects_old_30s_default.Timeout900 to 60,github-prod.tfvars300 to 30.terraform fmt -check,init -backend=false+validate(temporaryTF_DATA_DIR) onterraform/modules/compute/aws/lambdaandterraform/environments/aws: clean.terraform teston the lambda module: 7 passed, including the existing sweeps tests. Note that the lambda module's tftests are not wired into CI today; the Go tests are.go build ./...;go test -count=1for./internal/api/,./internal/purchase/,./internal/mocks/,./terraform/modules/compute/...,./terraform/environments/aws/: all pass.go vetand golangci-lint v2.10.1 on the touched packages: 0 issues.scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL. cfn-lint is not installed locally, and the CFN template is unchanged.Out of scope
TestScheduledTaskTimeoutFitsCloudRunRequestTimeout(fix(server): detach HTTP-triggered scheduled tasks from the request context #895).Closes #913
🤖 Generated with Claude Code