From 8b6eebe0754e5399f3d1cb5c5e461352cde51de4 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Wed, 28 Oct 2026 21:08:04 +0000 Subject: [PATCH] fix(purchase): shorten execution budget to 3m30s so the failure alert fits the Lambda timeout The detached purchase phases run back to back: execution (4m), terminal save (30s) and, for a failed or partial row, the failure alert (30s, its own detached deadline). That is 300s, exactly lambda_timeout, leaving no margin for the request work before the detach or the response, so Lambda could end the invocation in the middle of the failure email. Per the acting owner's decision, shorten purchaseExecutionTimeout to 3m30s (no lambda_timeout change): 3m30s + 30s + 30s = 270s, plus the 30s margin fits 300s. TestLambdaTimeoutCoversPurchaseBudget now counts all three phases against every shipped Lambda timeout, and the comments and Terraform validation messages state the real sum. Closes #1434 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/execute_detached_test.go | 14 ++++++++------ internal/purchase/failure_notify.go | 4 +++- internal/purchase/manager.go | 14 ++++++++------ internal/server/scheduled_tasks.go | 4 ++-- terraform/environments/aws/variables.tf | 2 +- terraform/modules/compute/aws/lambda/variables.tf | 14 ++++++++------ 6 files changed, 30 insertions(+), 22 deletions(-) diff --git a/internal/purchase/execute_detached_test.go b/internal/purchase/execute_detached_test.go index 7634a72ac..bf81ba513 100644 --- a/internal/purchase/execute_detached_test.go +++ b/internal/purchase/execute_detached_test.go @@ -18,11 +18,13 @@ import ( ) // TestLambdaTimeoutCoversPurchaseBudget keeps every shipped AWS Lambda timeout -// above the detached purchase budget (execution, then the terminal save). +// above the detached purchase budget: execution, then the terminal save, then +// (for a failed or partial row) the failure alert, each with its own deadline. // Lambda ends the invocation at its timeout whatever the context deadline -// says, so a shorter timeout cuts a purchase off mid-flight (#913). +// says, so a shorter timeout cuts a purchase or its failure email off +// mid-flight (#913, #1434). func TestLambdaTimeoutCoversPurchaseBudget(t *testing.T) { - lambdatimeout.AssertCovers(t, "../..", purchaseExecutionTimeout+terminalSaveTimeout) + lambdatimeout.AssertCovers(t, "../..", purchaseExecutionTimeout+terminalSaveTimeout+failureNotifyTimeout) } // detachFixture wires a direct single-account AWS purchase (issue #706). @@ -162,7 +164,7 @@ func TestExecuteAndFinalize_ExpiredCallerDeadline_UsesOwnBudgets(t *testing.T) { require.NoError(t, f.manager.executeAndFinalize(ctx, f.exec)) require.NoError(t, provContextErr) - // per-rec cap (30s) still bounds the provider call inside the 4m run budget. + // per-rec cap (30s) still bounds the provider call inside the 3m30s run budget. assert.WithinDuration(t, time.Now(), provDeadline, 30*time.Second) assert.Greater(t, time.Until(provDeadline), 29*time.Second) _, ok := f.saveCtx.Deadline() @@ -174,8 +176,8 @@ func TestExecuteAndFinalize_ExpiredCallerDeadline_UsesOwnBudgets(t *testing.T) { // SavePurchaseHistory runs inside executePurchase, so it sees the run ctx. _, ok = f.histCtx.Deadline() require.True(t, ok, "run ctx needs a deadline") - assert.LessOrEqual(t, f.historyRemaining, 4*time.Minute) - assert.Greater(t, f.historyRemaining, 3*time.Minute+59*time.Second) + assert.LessOrEqual(t, f.historyRemaining, 3*time.Minute+30*time.Second) + assert.Greater(t, f.historyRemaining, 3*time.Minute+29*time.Second) assert.Equal(t, []string{"completed"}, f.saved) } diff --git a/internal/purchase/failure_notify.go b/internal/purchase/failure_notify.go index 263111a9a..211b5bb31 100644 --- a/internal/purchase/failure_notify.go +++ b/internal/purchase/failure_notify.go @@ -22,7 +22,9 @@ const PurchaseFailedLogMarker = "purchase_execution_failed" // failureNotifyTimeout bounds the recipient lookup and the send. The context is // detached from the caller's, which may already be near its deadline after a -// long purchase run. +// long purchase run. It is the last of the three detached phases of +// executeAndFinalize, so the Lambda timeout must cover it too (see +// purchaseExecutionTimeout, #1434). const failureNotifyTimeout = 30 * time.Second // notifyExecutionFailure tells operators that an execution row did not fully diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 64f3e70ef..1ce762689 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -263,12 +263,14 @@ func isMultiAccountAckable(execErr error) bool { } const ( - // 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 + // The two budgets run back to back, and for a failed or partial row the + // failure alert follows them with its own failureNotifyTimeout (30s), so a + // run can take 3m30s + 30s + 30s = 270s. On AWS Lambda the function timeout + // ends the invocation whatever these deadlines say, so the Lambda timeout + // must cover that sum plus a 30s margin (lambda_timeout >= 300, enforced by + // Terraform validation and TestLambdaTimeoutCoversPurchaseBudget, #913, + // #1434). + purchaseExecutionTimeout = 3*time.Minute + 30*time.Second terminalSaveTimeout = 30 * time.Second // perRecPurchaseTimeout caps one recommendation's provider calls; see // executeSinglePurchase. diff --git a/internal/server/scheduled_tasks.go b/internal/server/scheduled_tasks.go index d262a7541..5141db8b8 100644 --- a/internal/server/scheduled_tasks.go +++ b/internal/server/scheduled_tasks.go @@ -18,8 +18,8 @@ import ( // runs with cpu_idle = true, so past that point the platform treats the // request as finished, throttles the instance's CPU and may scale it in, and // a longer bound would promise time the platform does not give. It also -// covers the purchase manager's own detached budget (4m execution + 30s -// terminal save). Azure Container Apps keeps CPU allocated after its ingress +// covers the purchase manager's own detached budget (3m30s execution + 30s +// terminal save + 30s failure alert). Azure Container Apps keeps CPU allocated after its ingress // gives up (240s), so the same bound applies there. Pinned against the // Terraform values by TestScheduledTaskTimeoutFitsCloudRunRequestTimeout. const scheduledTaskHTTPTimeout = 300 * time.Second diff --git a/terraform/environments/aws/variables.tf b/terraform/environments/aws/variables.tf index 015e60436..782e7c902 100644 --- a/terraform/environments/aws/variables.tf +++ b/terraform/environments/aws/variables.tf @@ -297,7 +297,7 @@ variable "lambda_timeout" { 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)." + error_message = "lambda_timeout must be between 300 and 900 seconds: RI exchanges run up to 270s (4m execution + 30s terminal save) and purchases up to 270s (3m30s execution + 30s terminal save + 30s failure alert) after the request, and Lambda cuts the invocation off at this timeout (#913)." } } diff --git a/terraform/modules/compute/aws/lambda/variables.tf b/terraform/modules/compute/aws/lambda/variables.tf index 79c87e20a..49769a332 100644 --- a/terraform/modules/compute/aws/lambda/variables.tf +++ b/terraform/modules/compute/aws/lambda/variables.tf @@ -50,11 +50,13 @@ variable "memory_size" { variable "timeout" { # 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. + # exchange gets 4m of execution plus a 30s terminal save + # (riExchangeExecutionTimeout + riExchangeTerminalSaveTimeout); a purchase + # gets 3m30s of execution, a 30s terminal save and a 30s failure alert + # (purchaseExecutionTimeout + terminalSaveTimeout + failureNotifyTimeout). + # Lambda ends the invocation at this timeout whatever the context deadline + # says, so a shorter value cuts that work off mid-flight (#913, #1434). + # 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)" @@ -63,7 +65,7 @@ variable "timeout" { 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)." + error_message = "timeout must be between 300 and 900 seconds: RI exchanges run up to 270s (4m execution + 30s terminal save) and purchases up to 270s (3m30s execution + 30s terminal save + 30s failure alert) after the request, and Lambda cuts the invocation off at this timeout (#913)." } }