[IAmJSD]

Root cause

executeAndFinalize runs three detached phases back to back, each with its own deadline:

1. purchase execution: purchaseExecutionTimeout = 4m (internal/purchase/manager.go);
2. terminal save: terminalSaveTimeout = 30s;
3. for a failed or partial row, the failure alert (deferred notifyRootFailure -> sendFailureNotice), which detaches again with failureNotifyTimeout = 30s (internal/purchase/failure_notify.go, added by fix(ops): alert on failed purchases and scheduled tasks; wire monitoring per environment #1094/fix(purchase): alert on failed fan-out roots and every terminal-failure writer #1245).

That adds up to 300s, which is exactly lambda_timeout in every shipped config. Nothing is left for the request work before the detach or for the response, so Lambda could end the invocation in the middle of the failure email. TestLambdaTimeoutCoversPurchaseBudget only counted phases 1 and 2, so it missed this.

Fix (owner decision D28, issue option 2)

• purchaseExecutionTimeout: 4m -> 3m30s. Worst case is now 3m30s + 30s + 30s = 270s. With the 30s lambdatimeout.Margin that fits the 300s Lambda timeout. lambda_timeout is unchanged and no deploy change is needed.
• TestLambdaTimeoutCoversPurchaseBudget now passes purchaseExecutionTimeout + terminalSaveTimeout + failureNotifyTimeout to lambdatimeout.AssertCovers. That helper reads every shipped Lambda timeout: the Terraform module and root variable defaults and validation floors, every committed lambda_timeout tfvars value and example, docs/DEPLOYMENT.md, and the CloudFormation Timeout. So raising any of the three phase budgets, or lowering any shipped timeout, fails the test.
• The literal pin on the run context in TestExecuteAndFinalize_SlowRunGetsFreshTerminalSaveBudget was 4m. It is now 3m30s, so the new value is pinned directly and not only through the constant.
• Comments and messages that said "4m execution + 30s terminal save" for purchases now state the real sum: manager.go, failure_notify.go, internal/server/scheduled_tasks.go, the comment on the timeout variable in terraform/modules/compute/aws/lambda/variables.tf, and the error_message of both Lambda timeout validations (module and terraform/environments/aws/variables.tf). Only text changed. The >= 300 floors, the defaults and the tfvars values are untouched.

Fan-out note: per-account rows send their own failure alert (a fresh 30s detach) from inside the execution phase. The root row of a fan-out logs only the marker and sends no email. So the worst case is still one 30s alert after the execution deadline, then the save: 270s total. Accounts are skipped once the run context is done, and the per-account alerts that run concurrently overlap in time instead of adding up.

Closes #1434

Verification

All of this is unit, fixture and mock evidence. Nothing ran against a real AWS account and no deploy was made.

Environment: AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.9.

Fail-before: the new test with manager.go from origin/main (4m budget), restored via git show origin/main:internal/purchase/manager.go:

go test ./internal/purchase/ -count=1 -run TestLambdaTimeoutCoversPurchaseBudget
--- FAIL: TestLambdaTimeoutCoversPurchaseBudget
    terraform/modules/compute/aws/lambda/variables.tf default: Lambda timeout 5m0s is shorter than the 5m0s detached budget plus 30s margin (want >= 5m30s)
    ... (11 failures: both variable defaults and floors, docs/DEPLOYMENT.md, every tfvars and tfvars.example)
FAIL

Pass-after (this branch):

go test ./internal/purchase/ -count=1 -run 'TestLambdaTimeoutCoversPurchaseBudget|TestExecuteAndFinalize' -v
--- PASS: TestLambdaTimeoutCoversPurchaseBudget
--- PASS: TestExecuteAndFinalize_SlowRunGetsFreshTerminalSaveBudget
... (all TestExecuteAndFinalize_* pass)

Mutation probes on this branch, each reverted afterwards:

• failureNotifyTimeout 30s -> 31s: the test fails with 11 "shorter than" errors.
• terraform/environments/aws/github-prod.tfvars lambda_timeout 300 -> 299: the test fails with github-prod.tfvars lambda_timeout: Lambda timeout 4m59s is shorter than the 4m30s detached budget plus 30s margin (want >= 5m0s).

Other checks:

• go build ./..., go vet ./..., go vet -tags integration ./...: OK
• go test -count=1 on ./internal/purchase/, ./internal/server/ and ./internal/mocks/: ok
• golangci-lint run --allow-parallel-runners ./internal/purchase/ ./internal/server/ (v2.10.1): 0 issues
• gocyclo -over 10 -ignore "_test\.go" on the changed non-test Go files: clean
• terraform fmt -check on both changed variables.tf: clean. terraform validate on modules/compute/aws/lambda and environments/aws (in a temp copy, init -backend=false): valid. terraform test -filter=tests/timeout.tftest.hcl in the Lambda module (mock provider, plan only): 5 passed.

No integration test was added. The bug is a static budget mismatch with no DB or concurrency path involved.

Combined-tree verification. Tested on a local, never-pushed tree: queue/platform @ 40dbbdaf = origin/main 6e2033cd plus:

Branch	Tested head
fix/issue-1494-listing-settlements	d7aa121
fix/issue-1529-ambiguous-return	8ecf506
fix/issue-1434-lambda-budget	8b6eebe
fix/issue-1432-retire-cfn-stack	9cb7de8
fix/issue-712-fire-default-aws-schedule	8bd7095

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers.

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests (-tags integration -count=1, one package at a time): internal/api, internal/database/postgres/migrations, internal/purchase and internal/server pass. internal/config had one failure, which comes from the fix(marketplace): close RI Marketplace poller gaps (pending rows with a listing id, superseded listings, deployment coverage) #1494 branch (a mid-claim listing gets settled); that branch is being fixed and isn't opened yet.
• Secrets: git-secrets on the changed files found nothing, and a planted fake key was flagged; scripts/test-git-secrets-allowlist.sh passes.
• Merge order: fix(cfn): CloudFormation CUDly stack can't start the current server (no DB_HOST; still provisions DynamoDB) #1432 (retire the CloudFormation stack) must merge before the fire-default branch. On its own, that branch fails two tests in the CloudFormation stack's test file, which fix(cfn): CloudFormation CUDly stack can't start the current server (no DB_HOST; still provisions DynamoDB) #1432 deletes.

Out of scope

• lambda_timeout itself (issue option 1) was left at 300 on purpose, per the owner decision.
• The RI exchange path (internal/api/ri_exchange_detached.go) still uses 4m + 30s = 270s and sends no separate detached alert, so it also fits 300s. It is unchanged.
• Trade-off: slow purchases now get 30s less execution time. Each recommendation's provider calls are still capped at 30s (perRecPurchaseTimeout).

Labels

Maintainer: please mirror the issue's triage labels onto this PR: priority/p3, severity/low, urgency/this-quarter, impact/few, effort/xs, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 6e2033c
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: c157bc04-76fc-4c0a-bd48-e089cb1c4ccc

Commits

Reviewing files that changed from the base of the PR and between 6e2033c and 8b6eebe.

Files selected for processing: 6

• internal/purchase/execute_detached_test.go
• internal/purchase/failure_notify.go
• internal/purchase/manager.go
• internal/server/scheduled_tasks.go
• terraform/environments/aws/variables.tf
• terraform/modules/compute/aws/lambda/variables.tf

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 8b6eebe0754e5399f3d1cb5c5e461352cde51de4

Blocking:

• none

Non-blocking:

• internal/execution/fanout.go:89-93 (pre-existing, not touched here): the 270s bound is not strict for a multi-account fan-out with more buying accounts than getMaxAccountParallelism(). When a slot frees after the run deadline, both sem <- struct{}{} and <-ctx.Done() are ready, and Go's select picks one at random. So a queued account can still launch with a done context. It fails at once, and its deferred notifyExecutionFailure (execution.go:313) detaches a fresh 30s for the email, holding the slot until the email finishes. With a hung mail server (fix(email): SMTP sends ignore the caller's context, so the 30s failure-alert timeout can't cut off a hung mail server #1433), each such launch can add another 30s after the deadline, and the root terminal save comes after all of them. That can cross 300s, and the root row would then strand in running until the reaper. This PR shortens the chain rather than lengthening it, so it does not block. Suggested follow-up: check ctx.Err() before the select (or prefer the Done case) so accounts really are skipped once the context is done.
• Trade-off the body already discloses: with a 30s per-rec cap and serial recs, a large single-account step gets about one fewer rec attempt before the deadline (7 instead of 8 full-length attempts). Remaining recs end the row partial, as they did before. Worth a CHANGELOG line, since it changes how much a slow run completes.
• Only internal/server/scheduled_tasks.go comments changed in internal/server, so I did not run its integration suite.

Attacked:

• Traced executeAndFinalize (manager.go:312-357): execution (DetachedWithTimeout(ctx, purchaseExecutionTimeout)), then the terminal save plus updatePlanProgress on saveCtx (30s), then the deferred notifyRootFailure → sendFailureNotice, which detaches a fresh failureNotifyTimeout. The sum is 210+30+30 = 270s. The per-account saves and the per-rec 30s cap derive from the run ctx and do not extend it. The fan-out root is marker-only, so it adds no second email. The RI exchange path has no detached alert, so 4m+30s is still fine there.
• Fail-before: with internal/purchase/manager.go from origin/main (4m) and this branch's test, TestLambdaTimeoutCoversPurchaseBudget reports 11 "shorter than" failures. Restored afterwards; tree clean.
• Grepped for stale "4m execution"/"270s"/"4m30s" wording. What remains is all RI-exchange text, or the 270s totals that are still correct.
• Ran on 8b6eebe: go build ./..., go vet ./..., go vet -tags integration ./... clean; go test -race -short -count=1 ./internal/purchase/ ./internal/server/ ok; go test -race -tags integration -count=1 ./internal/purchase/ ok (75s); golangci-lint v2.10.1 0 issues on both packages, with and without --build-tags integration.
• Issue fix(iac): purchase executor budget (4m + 30s + 30s) equals lambda_timeout (300s), leaving no headroom #1434 option 2 AC (the test counts all three phases, shipped timeouts pass, comment and validation message state the real sum): met. Closes is fine.

Body-stale:

• "Accounts are skipped once the run context is done": not guaranteed. See the fanout.go select note above; an account can still launch after the deadline when a slot frees.
• "That helper reads ... and the CloudFormation Timeout": true on main today. Once fix(cfn): retire the CloudFormation install stack #1577 merges, that check goes away along with the stack.

_{Reviewer: rM1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 8b6eebe0754e5399f3d1cb5c5e461352cde51de4

Blocking:

• none

Non-blocking:

• Out of scope, but fix(iac): fire delayed purchases by default; collect AWS recommendations at 02:00 UTC #1580 makes it more relevant: FireScheduledDelayedPurchases (internal/purchase/scheduled_fire.go) runs executeAndFinalize for each due row one after another in a single invocation, and it never checks how much time is left. The 270s budget therefore bounds one purchase, not a sweep tick. On Lambda, two slow due rows in the same tick can be cut off by the 300s timeout. Recovery exists: the reaper and the stable per-rec idempotency keys. Still, it deserves its own issue: a time-left check between rows, or one row per tick on Lambda.
• From an earlier review (IAmJSD's agent; I checked the code): internal/execution/fanout.go:89-93 predates this PR. Once the run deadline passes, a freed semaphore slot and ctx.Done() are both ready, and select picks one at random. So a queued account can still launch with a done context, and its deferred notifyExecutionFailure adds a fresh 30s detach. With more buying accounts than the parallelism limit, those 30s waits can stack past the 270s. Follow-up: check ctx.Err() before the select.
• The issue's acceptance criteria include the CloudFormation Timeout. That check goes away with fix(cfn): retire the CloudFormation install stack #1577, which is consistent with D29.

Attacked:

• Read every detached phase after the claim (executeAndFinalize, notifyRootFailure, the per-account notifyExecutionFailure inside the execution phase, failClaimedBeforeSubmission) to check the 3m30s + 30s + 30s sum. A fan-out root only logs the marker. A per-account alert started near the execution deadline adds at most one 30s detach before the save. So the worst case is 270s, as the body says. The RI exchange budget (4m + 30s, no detached alert) is unchanged and fits too.
• Fail-before: I restored internal/purchase/manager.go from origin/main. TestLambdaTimeoutCoversPurchaseBudget and TestExecuteAndFinalize_ExpiredCallerDeadline_UsesOwnBudgets both fail. Both pass at the head.
• Ran: go build ./..., go vet ./..., go vet -tags integration ./...; go test -race -short -count=1 ./internal/purchase/ ./internal/server/ (ok); go test -race -tags integration -count=1 ./internal/purchase/ (ok, 112s). I grepped for stale "4m execution" text in Go, Terraform and docs and found none for purchases.
• Combined with fix(cfn): retire the CloudFormation install stack #1577: TestLambdaTimeoutCoversPurchaseBudget passes once the CloudFormation check is gone.

Body-stale:

• "That helper reads ... and the CloudFormation Timeout" stops being true once fix(cfn): retire the CloudFormation install stack #1577 merges, because fix(cfn): retire the CloudFormation install stack #1577 removes that check. It is accurate for this branch on its own.

_{Reviewer: rM2. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 8b6eebe0754e5399f3d1cb5c5e461352cde51de4 merged onto main (6e2033cd), together with #1577, #1578, #1579 in that order (combined tree bf700d64, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on 8b6eebe0 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 8b6eebe0754e5399f3d1cb5c5e461352cde51de4.