Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions internal/purchase/execute_detached_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down Expand Up @@ -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()
Expand All @@ -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)
}

Expand Down
4 changes: 3 additions & 1 deletion internal/purchase/failure_notify.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 8 additions & 6 deletions internal/purchase/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions internal/server/scheduled_tasks.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion terraform/environments/aws/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -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)."
}
}

Expand Down
14 changes: 8 additions & 6 deletions terraform/modules/compute/aws/lambda/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -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)"
Expand All @@ -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)."
}
}

Expand Down
Loading