diff --git a/internal/purchase/execute_detached_test.go b/internal/purchase/execute_detached_test.go index 7634a72a..bf81ba51 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 263111a9..211b5bb3 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 64f3e70e..1ce76268 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 d262a754..5141db8b 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 015e6043..782e7c90 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 79c87e20..49769a33 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)." } }