From 17654ccf3937a803d11bfff67e58e9c942838e9e Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 01:26:03 +0100 Subject: [PATCH 1/2] fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending finalizePurchaseStatus stamped "failed" over a freshly inserted execution with an unguarded full-row save whenever the approval email could not be sent. A session approve (pending -> running claim), cancel or pause can land while the email is in flight; the unguarded write then overwrote the row, so a commitment that was being bought ended up "failed" and eligible for retry, risking a second purchase. The write now goes through SaveExecutionIfStatus with "pending" as the only expected status (row lock + status check + write in one tx). A lost guard is a no-op with a warning: the row keeps the other writer's status, the caller's in-memory copy is left untouched, and the response reports the row's re-read status instead of a stale "failed". Other save errors keep the existing best-effort "pending" contract. Tests: mock unit tests pin the guarded call and the lost-guard behavior; integration tests on real Postgres drive the real send path with an approve claim landing during a blocked email send, and race the failed write against the approve claim for 40 rounds. Refs #1232 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_purchases.go | 35 ++++- ...urchases_finalize_race_integration_test.go | 147 ++++++++++++++++++ .../api/handler_purchases_finalize_test.go | 103 ++++++++++++ 3 files changed, 281 insertions(+), 4 deletions(-) create mode 100644 internal/api/handler_purchases_finalize_race_integration_test.go create mode 100644 internal/api/handler_purchases_finalize_test.go diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index af7c61b96..4012c77c7 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -3089,17 +3089,44 @@ func validateExecutePurchaseRecommendations(recs []config.RecommendationRecord) // response should carry. Returns the original "pending" when email_sent is // true or when the failed-state write itself fails (best-effort — the UI // still gets email_sent=false and can point the user at History). +// +// The write is guarded on the row still being "pending" (issue #1232). A +// session approve, cancel or pause can land while the email is being sent; +// an unguarded write would then stamp "failed" over a row that was already +// approved and bought, which makes it eligible for retry and a second +// purchase. A lost guard is a no-op: the row keeps the status the other +// writer gave it, and that status is what the response carries. func (h *Handler) finalizePurchaseStatus(ctx context.Context, execution *config.PurchaseExecution, emailSent bool, emailReason string) string { if emailSent { return "pending" } - execution.Status = "failed" - execution.Error = emailReason - if err := h.config.SavePurchaseExecution(ctx, execution); err != nil { + failed := *execution + failed.Status = "failed" + failed.Error = emailReason + err := h.config.SaveExecutionIfStatus(ctx, &failed, []string{"pending"}) + switch { + case err == nil: + execution.Status, execution.Error = failed.Status, failed.Error + return "failed" + case errors.Is(err, config.ErrExecutionNotInExpectedStatus): + logging.Warnf("execution %s left pending while its approval email was being sent; not marking it failed: %v", execution.ExecutionID, err) + return h.currentExecutionStatus(ctx, execution.ExecutionID) + default: logging.Errorf("failed to mark execution %s as failed after email send error: %v", execution.ExecutionID, err) return "pending" } - return "failed" +} + +// currentExecutionStatus re-reads an execution's stored status for a response +// whose in-memory copy went stale. Falls back to "pending" (the status the row +// was created with) when the read fails. +func (h *Handler) currentExecutionStatus(ctx context.Context, executionID string) string { + row, err := h.config.GetExecutionByID(ctx, executionID) + if err != nil || row == nil { + logging.Errorf("failed to re-read execution %s status: %v", executionID, err) + return "pending" + } + return row.Status } // validateAndTotalRecommendations validates each recommendation and returns diff --git a/internal/api/handler_purchases_finalize_race_integration_test.go b/internal/api/handler_purchases_finalize_race_integration_test.go new file mode 100644 index 000000000..d79713267 --- /dev/null +++ b/internal/api/handler_purchases_finalize_race_integration_test.go @@ -0,0 +1,147 @@ +//go:build integration + +package api + +import ( + "context" + "errors" + "sync" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/email" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// blockingApprovalNotifier holds SendPurchaseApprovalRequest until the test +// releases it, then fails the send, so a test can land another writer on the +// row while the approval email is in flight (issue #1232). +type blockingApprovalNotifier struct { + stubEmailNotifier + entered, release chan struct{} +} + +func (n *blockingApprovalNotifier) SendPurchaseApprovalRequest(ctx context.Context, _ email.NotificationData) error { + close(n.entered) + select { + case <-n.release: + case <-ctx.Done(): + return ctx.Err() + } + return errors.New("ses throttled") +} + +// claimRunningLikeApprove is the claim CAS a session approve runs before it +// calls the provider (purchase.Manager.claimRunning): pending|notified -> +// running. Winning it means the commitment is being bought. +func claimRunningLikeApprove(ctx context.Context, store *config.PostgresStore, id string) error { + _, err := store.TransitionExecutionStatus(ctx, id, []string{"pending", "notified"}, "running", nil) + return err +} + +func storedStatus(t *testing.T, store *config.PostgresStore, id string) *config.PurchaseExecution { + t.Helper() + row, err := store.GetExecutionByID(context.Background(), id) + require.NoError(t, err) + return row +} + +// TestFinalizePurchaseStatus_ApproveDuringEmailSend drives the real send path: +// the submit inserts a pending row, the approval email blocks, an approve +// claims the row (running) while it is in flight, then the send fails. Before +// the fix finalizePurchaseStatus stamped "failed" over the running row, which +// made a commitment that was being bought eligible for retry. +func TestFinalizePurchaseStatus_ApproveDuringEmailSend(t *testing.T) { + _, store := setupSubmitDB(t) + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + cfg, err := store.GetGlobalConfig(ctx) + require.NoError(t, err) + notify := "approvals@example.com" + cfg.NotificationEmail = ¬ify + require.NoError(t, store.SaveGlobalConfig(ctx, cfg)) + + recs, key := submitFixture(t) + exec := newSubmitExecution(t, recs) + notifier := &blockingApprovalNotifier{entered: make(chan struct{}), release: make(chan struct{})} + h := &Handler{config: store, emailNotifier: notifier} + dup, err := h.persistExecutionAndSuppressions(ctx, exec, nil, "", key) + require.NoError(t, err) + require.Nil(t, dup) + + type outcome struct { + emailSent bool + status string + } + done := make(chan outcome, 1) + go func() { + sent, reason, _ := h.sendPurchaseApprovalEmail(ctx, &events.LambdaFunctionURLRequest{}, exec, "raw-token", recs, 0, 0) + done <- outcome{sent, h.finalizePurchaseStatus(ctx, exec, sent, reason)} + }() + select { + case <-notifier.entered: + case <-ctx.Done(): + t.Fatal("approval email send never started") + } + require.NoError(t, claimRunningLikeApprove(ctx, store, exec.ExecutionID), "approve must win a pending row") + close(notifier.release) + + var got outcome + select { + case got = <-done: + case <-ctx.Done(): + t.Fatal("send + finalize did not finish") + } + assert.False(t, got.emailSent) + assert.Equal(t, "running", got.status, "the response must report the row's real status") + row := storedStatus(t, store, exec.ExecutionID) + assert.Equal(t, "running", row.Status, "a failed email must not overwrite an approved, running row") + assert.Empty(t, row.Error) +} + +// TestFinalizePurchaseStatus_ConcurrentWithApproveClaim races the guarded +// failed write against the approve claim on real Postgres. Exactly one may +// win: either the row is failed and the approve was refused, or the approve +// claimed it and the row stays running. Before the fix the unguarded write +// could land after a won claim, leaving a failed row for a live purchase. +func TestFinalizePurchaseStatus_ConcurrentWithApproveClaim(t *testing.T) { + _, store := setupSubmitDB(t) + ctx := context.Background() + h := &Handler{config: store} + const rounds = 40 + claimWins := 0 + for i := 0; i < rounds; i++ { + recs, _ := submitFixture(t) + exec := newSubmitExecution(t, recs) + require.NoError(t, store.SavePurchaseExecution(ctx, exec)) + + start := make(chan struct{}) + var wg sync.WaitGroup + var claimErr error + var status string + wg.Add(2) + go func() { defer wg.Done(); <-start; claimErr = claimRunningLikeApprove(ctx, store, exec.ExecutionID) }() + go func() { + defer wg.Done() + <-start + status = h.finalizePurchaseStatus(ctx, exec, false, "send failed: ses throttled") + }() + close(start) + wg.Wait() + + row := storedStatus(t, store, exec.ExecutionID) + if claimErr == nil { + claimWins++ + assert.Equal(t, "running", row.Status, "round %d: a won approve claim must not be overwritten with failed", i) + assert.Equal(t, "running", status, "round %d", i) + continue + } + assert.ErrorIs(t, claimErr, config.ErrExecutionNotInExpectedStatus, "round %d", i) + assert.Equal(t, "failed", row.Status, "round %d", i) + assert.Equal(t, "failed", status, "round %d", i) + } + t.Logf("approve claim won %d of %d rounds", claimWins, rounds) +} diff --git a/internal/api/handler_purchases_finalize_test.go b/internal/api/handler_purchases_finalize_test.go new file mode 100644 index 000000000..77d5691b9 --- /dev/null +++ b/internal/api/handler_purchases_finalize_test.go @@ -0,0 +1,103 @@ +package api + +import ( + "context" + "errors" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +const finalizeExecID = "11111111-2222-4333-8444-555555555555" + +func newFinalizeExecution() *config.PurchaseExecution { + return &config.PurchaseExecution{ExecutionID: finalizeExecID, Status: "pending"} +} + +// TestFinalizePurchaseStatus_GuardsFailedWriteOnPending pins issue #1232: the +// email-failure write must go through the status-guarded save with "pending" +// as the only expected status, never through the unguarded full-row save. +func TestFinalizePurchaseStatus_GuardsFailedWriteOnPending(t *testing.T) { + store := new(MockConfigStore) + store.On("SaveExecutionIfStatus", mock.Anything, mock.MatchedBy(func(e *config.PurchaseExecution) bool { + return e.ExecutionID == finalizeExecID && e.Status == "failed" && e.Error == "send failed: throttled" + }), []string{"pending"}).Return(nil).Once() + h := &Handler{config: store} + exec := newFinalizeExecution() + + status := h.finalizePurchaseStatus(context.Background(), exec, false, "send failed: throttled") + + assert.Equal(t, "failed", status) + assert.Equal(t, "failed", exec.Status, "the caller's copy reflects the committed write") + assert.Equal(t, "send failed: throttled", exec.Error) + store.AssertExpectations(t) + store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) +} + +// TestFinalizePurchaseStatus_LostGuardIsNoOp is the approve-during-send case: +// another writer moved the row off "pending" while the email was in flight. +// The response must carry the row's real status, the caller's copy must stay +// untouched, and nothing may be written over the row. +func TestFinalizePurchaseStatus_LostGuardIsNoOp(t *testing.T) { + for _, current := range []string{"approved", "running", "completed", "canceled", "paused"} { + t.Run(current, func(t *testing.T) { + store := new(MockConfigStore) + store.On("SaveExecutionIfStatus", mock.Anything, mock.Anything, []string{"pending"}). + Return(config.ErrExecutionNotInExpectedStatus).Once() + store.On("GetExecutionByID", mock.Anything, finalizeExecID). + Return(&config.PurchaseExecution{ExecutionID: finalizeExecID, Status: current}, nil).Once() + h := &Handler{config: store} + exec := newFinalizeExecution() + + status := h.finalizePurchaseStatus(context.Background(), exec, false, "send failed: throttled") + + assert.Equal(t, current, status) + assert.Equal(t, "pending", exec.Status, "a lost guard must not mutate the caller's copy") + assert.Empty(t, exec.Error) + store.AssertExpectations(t) + store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + }) + } +} + +// TestFinalizePurchaseStatus_LostGuardReReadFails falls back to "pending" when +// the stored status cannot be re-read after a lost guard. +func TestFinalizePurchaseStatus_LostGuardReReadFails(t *testing.T) { + store := new(MockConfigStore) + store.On("SaveExecutionIfStatus", mock.Anything, mock.Anything, []string{"pending"}). + Return(config.ErrExecutionNotInExpectedStatus).Once() + store.On("GetExecutionByID", mock.Anything, finalizeExecID).Return(nil, errors.New("db down")).Once() + h := &Handler{config: store} + + assert.Equal(t, "pending", h.finalizePurchaseStatus(context.Background(), newFinalizeExecution(), false, "x")) + store.AssertExpectations(t) +} + +// TestFinalizePurchaseStatus_WriteErrorKeepsPending keeps the pre-existing +// best-effort contract: any other save error is logged and reported as +// "pending" without a re-read. +func TestFinalizePurchaseStatus_WriteErrorKeepsPending(t *testing.T) { + store := new(MockConfigStore) + store.On("SaveExecutionIfStatus", mock.Anything, mock.Anything, []string{"pending"}). + Return(errors.New("connection reset")).Once() + h := &Handler{config: store} + exec := newFinalizeExecution() + + assert.Equal(t, "pending", h.finalizePurchaseStatus(context.Background(), exec, false, "x")) + assert.Equal(t, "pending", exec.Status) + store.AssertExpectations(t) + store.AssertNotCalled(t, "GetExecutionByID", mock.Anything, mock.Anything) +} + +// TestFinalizePurchaseStatus_EmailSentWritesNothing: a sent email leaves the +// row pending and touches the store not at all. +func TestFinalizePurchaseStatus_EmailSentWritesNothing(t *testing.T) { + store := new(MockConfigStore) + h := &Handler{config: store} + require.Equal(t, "pending", h.finalizePurchaseStatus(context.Background(), newFinalizeExecution(), true, "")) + store.AssertNotCalled(t, "SaveExecutionIfStatus", mock.Anything, mock.Anything, mock.Anything) + store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) +} From 91827b9ab72c2e2f4a1e8533b61a214f30842347 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 04:38:23 +0000 Subject: [PATCH 2/2] fix(api/purchases): return a 500 when the approval-email failure cannot be recorded When the approval email failed, finalizePurchaseStatus tried to mark the new execution failed. If that guarded save itself errored (anything other than a lost status guard), it logged and returned "pending", so submit and retry answered 200 {"status":"pending","email_sent":false}. The row really was left pending, with no error text and a valid approval token, and looked exactly like a healthy request waiting on its approver. finalizePurchaseStatus now returns (string, error). On a save failure it returns a 500 client error whose message starts "AUDIT LOSS:" and whose body carries execution_id, email_sent=false and email_reason, so the caller can find and cancel the row. The store error is wrapped (still reachable with errors.Is) but kept out of the response body. executePurchase and retryPurchase propagate it. The guarded write and the lost-guard branch are unchanged. The submit-path tail moved into sendApprovalAndFinalize to keep executePurchase within gocyclo 10. Tests: mock unit tests for the save-error and missing-row cases, handler tests for submit and retry that answered 200 "pending" before the fix, and an integration test on real Postgres where the failure write runs on a canceled context and the row stays pending. Closes #54 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_purchases.go | 65 ++++++-- ...urchases_finalize_race_integration_test.go | 42 +++++- .../api/handler_purchases_finalize_test.go | 140 ++++++++++++++++-- 3 files changed, 219 insertions(+), 28 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 4012c77c7..c2359677d 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2249,7 +2249,10 @@ func (h *Handler) retryPurchase(ctx context.Context, req *events.LambdaFunctionU // the original row is unaffected (it points at the failed-again // successor, which is exactly the audit trail we want). emailSent, emailReason, recipient := h.sendPurchaseApprovalEmail(ctx, req, newExecution, rawApprovalToken, failedExec.Recommendations, totalUpfront, totalSavings) - status := h.finalizePurchaseStatus(ctx, newExecution, emailSent, emailReason) + status, err := h.finalizePurchaseStatus(ctx, newExecution, emailSent, emailReason) + if err != nil { + return nil, err + } resp := map[string]any{ "execution_id": newExecution.ExecutionID, @@ -3086,9 +3089,7 @@ func validateExecutePurchaseRecommendations(recs []config.RecommendationRecord) // finalizePurchaseStatus flips an execution's stored status to "failed" if // the approval email couldn't send, and returns the status string the API -// response should carry. Returns the original "pending" when email_sent is -// true or when the failed-state write itself fails (best-effort — the UI -// still gets email_sent=false and can point the user at History). +// response should carry. Returns "pending" when email_sent is true. // // The write is guarded on the row still being "pending" (issue #1232). A // session approve, cancel or pause can land while the email is being sent; @@ -3096,9 +3097,15 @@ func validateExecutePurchaseRecommendations(recs []config.RecommendationRecord) // approved and bought, which makes it eligible for retry and a second // purchase. A lost guard is a no-op: the row keeps the status the other // writer gave it, and that status is what the response carries. -func (h *Handler) finalizePurchaseStatus(ctx context.Context, execution *config.PurchaseExecution, emailSent bool, emailReason string) string { +// +// Any other save error is returned as a 500 (issue #54). The row then +// still reads "pending" with no error text and a valid approval token while +// no approval email exists, so answering "pending" would make it look like +// a healthy request waiting on its approver. The error names the execution +// so the caller can cancel it. +func (h *Handler) finalizePurchaseStatus(ctx context.Context, execution *config.PurchaseExecution, emailSent bool, emailReason string) (string, error) { if emailSent { - return "pending" + return "pending", nil } failed := *execution failed.Status = "failed" @@ -3107,16 +3114,33 @@ func (h *Handler) finalizePurchaseStatus(ctx context.Context, execution *config. switch { case err == nil: execution.Status, execution.Error = failed.Status, failed.Error - return "failed" + return "failed", nil case errors.Is(err, config.ErrExecutionNotInExpectedStatus): logging.Warnf("execution %s left pending while its approval email was being sent; not marking it failed: %v", execution.ExecutionID, err) - return h.currentExecutionStatus(ctx, execution.ExecutionID) + return h.currentExecutionStatus(ctx, execution.ExecutionID), nil default: - logging.Errorf("failed to mark execution %s as failed after email send error: %v", execution.ExecutionID, err) - return "pending" + logging.Errorf("AUDIT LOSS: failed to mark execution %s as failed after email send error (%s): %v", execution.ExecutionID, emailReason, err) + return "", emailFailureNotRecordedError(execution.ExecutionID, emailReason, err) } } +// emailFailureNotRecordedError is the 500 finalizePurchaseStatus returns when +// the approval email failed and the write recording that failure failed too +// (issue #54). The body names the execution and carries the email reason so +// the caller can find the row and cancel it. The store error stays out of +// the body but is wrapped so it remains reachable with errors.Is. +func emailFailureNotRecordedError(executionID, emailReason string, saveErr error) error { + ce := NewClientErrorWithDetails(500, + fmt.Sprintf("AUDIT LOSS: the approval email for execution %s could not be sent, and saving that failure on the execution also failed; "+ + "it may still show as pending with no approval email on the way, so cancel it from History", executionID), + map[string]any{ + "execution_id": executionID, + "email_sent": false, + "email_reason": emailReason, + }) + return fmt.Errorf("%w: %w", ce, saveErr) +} + // currentExecutionStatus re-reads an execution's stored status for a response // whose in-memory copy went stale. Falls back to "pending" (the status the row // was created with) when the read fails. @@ -3514,11 +3538,24 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio // actual outcome. The DB write above is the source of truth — email is // best-effort and never blocks the response body; the returned // email_sent / email_reason fields let the UI tell the user whether they - // should wait for an inbox or cancel/retry manually. - emailSent, emailReason, recipient := h.sendPurchaseApprovalEmail(ctx, req, execution, rawApprovalToken, execReq.Recommendations, totalUpfront, totalSavings) - status := h.finalizePurchaseStatus(ctx, execution, emailSent, emailReason) + // should wait for an inbox or cancel/retry manually. The one exception is + // a send failure that cannot be recorded on the row: that answers 500 + // (issue #54). + return h.sendApprovalAndFinalize(ctx, req, execution, rawApprovalToken, execReq.Recommendations, totalUpfront, totalSavings, paymentAdjustments) +} - return withPaymentAdjustments(buildApprovalPendingResponse(executionID, status, len(execReq.Recommendations), totalUpfront, totalSavings, emailSent, emailReason, recipient), paymentAdjustments), nil +// sendApprovalAndFinalize sends the approval email for a freshly persisted +// pending execution, records a send failure on the row through +// finalizePurchaseStatus, and builds the approval-pending response. A failed +// failure write comes back as an error rather than a response (issue #54). +// Extracted from executePurchase to keep it under the gocyclo budget. +func (h *Handler) sendApprovalAndFinalize(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, rawApprovalToken string, recs []config.RecommendationRecord, totalUpfront, totalSavings float64, paymentAdjustments []PaymentAdjustment) (any, error) { + emailSent, emailReason, recipient := h.sendPurchaseApprovalEmail(ctx, req, execution, rawApprovalToken, recs, totalUpfront, totalSavings) + status, err := h.finalizePurchaseStatus(ctx, execution, emailSent, emailReason) + if err != nil { + return nil, err + } + return withPaymentAdjustments(buildApprovalPendingResponse(execution.ExecutionID, status, len(recs), totalUpfront, totalSavings, emailSent, emailReason, recipient), paymentAdjustments), nil } // withPaymentAdjustments attaches the payment-option coercion notices to an diff --git a/internal/api/handler_purchases_finalize_race_integration_test.go b/internal/api/handler_purchases_finalize_race_integration_test.go index d79713267..e0c54e8f1 100644 --- a/internal/api/handler_purchases_finalize_race_integration_test.go +++ b/internal/api/handler_purchases_finalize_race_integration_test.go @@ -75,11 +75,13 @@ func TestFinalizePurchaseStatus_ApproveDuringEmailSend(t *testing.T) { type outcome struct { emailSent bool status string + err error } done := make(chan outcome, 1) go func() { sent, reason, _ := h.sendPurchaseApprovalEmail(ctx, &events.LambdaFunctionURLRequest{}, exec, "raw-token", recs, 0, 0) - done <- outcome{sent, h.finalizePurchaseStatus(ctx, exec, sent, reason)} + status, err := h.finalizePurchaseStatus(ctx, exec, sent, reason) + done <- outcome{sent, status, err} }() select { case <-notifier.entered: @@ -96,6 +98,7 @@ func TestFinalizePurchaseStatus_ApproveDuringEmailSend(t *testing.T) { t.Fatal("send + finalize did not finish") } assert.False(t, got.emailSent) + require.NoError(t, got.err) assert.Equal(t, "running", got.status, "the response must report the row's real status") row := storedStatus(t, store, exec.ExecutionID) assert.Equal(t, "running", row.Status, "a failed email must not overwrite an approved, running row") @@ -120,17 +123,18 @@ func TestFinalizePurchaseStatus_ConcurrentWithApproveClaim(t *testing.T) { start := make(chan struct{}) var wg sync.WaitGroup - var claimErr error + var claimErr, finalizeErr error var status string wg.Add(2) go func() { defer wg.Done(); <-start; claimErr = claimRunningLikeApprove(ctx, store, exec.ExecutionID) }() go func() { defer wg.Done() <-start - status = h.finalizePurchaseStatus(ctx, exec, false, "send failed: ses throttled") + status, finalizeErr = h.finalizePurchaseStatus(ctx, exec, false, "send failed: ses throttled") }() close(start) wg.Wait() + require.NoError(t, finalizeErr, "round %d: a lost guard is not a save failure", i) row := storedStatus(t, store, exec.ExecutionID) if claimErr == nil { @@ -145,3 +149,35 @@ func TestFinalizePurchaseStatus_ConcurrentWithApproveClaim(t *testing.T) { } t.Logf("approve claim won %d of %d rounds", claimWins, rounds) } + +// TestFinalizePurchaseStatus_SaveFailureSurfaces is issue #54 on real +// Postgres: the approval email fails and the write recording that fails too +// (here the request context is canceled before it, as when the Lambda deadline +// hits). The row is left pending with no error text, indistinguishable from a +// healthy pending request, so the response must be the 500 naming the +// execution rather than "pending". +func TestFinalizePurchaseStatus_SaveFailureSurfaces(t *testing.T) { + _, store := setupSubmitDB(t) + ctx := context.Background() + recs, key := submitFixture(t) + exec := newSubmitExecution(t, recs) + h := &Handler{config: store} + dup, err := h.persistExecutionAndSuppressions(ctx, exec, nil, "", key) + require.NoError(t, err) + require.Nil(t, dup) + + sent, reason, _ := h.sendPurchaseApprovalEmail(ctx, &events.LambdaFunctionURLRequest{}, exec, "raw-token", recs, 0, 0) + require.False(t, sent, "no notifier is wired, so the send must fail") + require.NotEmpty(t, reason) + + deadCtx, cancel := context.WithCancel(ctx) + cancel() + status, err := h.finalizePurchaseStatus(deadCtx, exec, sent, reason) + + assert.Empty(t, status) + requireEmailFailureNotRecorded(t, err, exec.ExecutionID, reason) + assert.ErrorIs(t, err, context.Canceled) + row := storedStatus(t, store, exec.ExecutionID) + assert.Equal(t, "pending", row.Status, "the failed write never landed") + assert.Empty(t, row.Error) +} diff --git a/internal/api/handler_purchases_finalize_test.go b/internal/api/handler_purchases_finalize_test.go index 77d5691b9..4d179831b 100644 --- a/internal/api/handler_purchases_finalize_test.go +++ b/internal/api/handler_purchases_finalize_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -28,8 +29,9 @@ func TestFinalizePurchaseStatus_GuardsFailedWriteOnPending(t *testing.T) { h := &Handler{config: store} exec := newFinalizeExecution() - status := h.finalizePurchaseStatus(context.Background(), exec, false, "send failed: throttled") + status, err := h.finalizePurchaseStatus(context.Background(), exec, false, "send failed: throttled") + require.NoError(t, err) assert.Equal(t, "failed", status) assert.Equal(t, "failed", exec.Status, "the caller's copy reflects the committed write") assert.Equal(t, "send failed: throttled", exec.Error) @@ -52,8 +54,9 @@ func TestFinalizePurchaseStatus_LostGuardIsNoOp(t *testing.T) { h := &Handler{config: store} exec := newFinalizeExecution() - status := h.finalizePurchaseStatus(context.Background(), exec, false, "send failed: throttled") + status, err := h.finalizePurchaseStatus(context.Background(), exec, false, "send failed: throttled") + require.NoError(t, err) assert.Equal(t, current, status) assert.Equal(t, "pending", exec.Status, "a lost guard must not mutate the caller's copy") assert.Empty(t, exec.Error) @@ -72,32 +75,147 @@ func TestFinalizePurchaseStatus_LostGuardReReadFails(t *testing.T) { store.On("GetExecutionByID", mock.Anything, finalizeExecID).Return(nil, errors.New("db down")).Once() h := &Handler{config: store} - assert.Equal(t, "pending", h.finalizePurchaseStatus(context.Background(), newFinalizeExecution(), false, "x")) + status, err := h.finalizePurchaseStatus(context.Background(), newFinalizeExecution(), false, "x") + require.NoError(t, err) + assert.Equal(t, "pending", status) store.AssertExpectations(t) } -// TestFinalizePurchaseStatus_WriteErrorKeepsPending keeps the pre-existing -// best-effort contract: any other save error is logged and reported as -// "pending" without a re-read. -func TestFinalizePurchaseStatus_WriteErrorKeepsPending(t *testing.T) { +// TestFinalizePurchaseStatus_WriteErrorSurfaces pins issue #54: when the +// failed write itself errors, the function must not report "pending" (the row +// would look like a healthy request waiting on its approver). It returns a +// 500 that names the execution and carries the email reason, keeps the store +// error reachable with errors.Is, and leaves the caller's copy untouched. +func TestFinalizePurchaseStatus_WriteErrorSurfaces(t *testing.T) { store := new(MockConfigStore) + saveErr := errors.New("connection reset") store.On("SaveExecutionIfStatus", mock.Anything, mock.Anything, []string{"pending"}). - Return(errors.New("connection reset")).Once() + Return(saveErr).Once() h := &Handler{config: store} exec := newFinalizeExecution() - assert.Equal(t, "pending", h.finalizePurchaseStatus(context.Background(), exec, false, "x")) - assert.Equal(t, "pending", exec.Status) + status, err := h.finalizePurchaseStatus(context.Background(), exec, false, "send failed: throttled") + + assert.Empty(t, status) + requireEmailFailureNotRecorded(t, err, finalizeExecID, "send failed: throttled") + assert.ErrorIs(t, err, saveErr) + assert.Equal(t, "pending", exec.Status, "a failed write must not mutate the caller's copy") + assert.Empty(t, exec.Error) store.AssertExpectations(t) store.AssertNotCalled(t, "GetExecutionByID", mock.Anything, mock.Anything) } +// TestFinalizePurchaseStatus_MissingRowSurfaces: a row that vanished between +// the insert and the failed write (ErrNotFound from the guarded save) is a +// save failure too, not a lost guard, so it gets the same 500. +func TestFinalizePurchaseStatus_MissingRowSurfaces(t *testing.T) { + store := new(MockConfigStore) + store.On("SaveExecutionIfStatus", mock.Anything, mock.Anything, []string{"pending"}). + Return(config.ErrNotFound).Once() + h := &Handler{config: store} + + _, err := h.finalizePurchaseStatus(context.Background(), newFinalizeExecution(), false, "x") + + requireEmailFailureNotRecorded(t, err, finalizeExecID, "x") + assert.ErrorIs(t, err, config.ErrNotFound) + store.AssertNotCalled(t, "GetExecutionByID", mock.Anything, mock.Anything) +} + +// requireEmailFailureNotRecorded asserts err is the issue #54 500 for +// executionID, and that the response body the router builds from it names +// the execution and the email reason but not the store error. +func requireEmailFailureNotRecorded(t *testing.T, err error, executionID, emailReason string) { + t.Helper() + require.Error(t, err, "a failed failure write must not be reported as a normal response") + ce, ok := IsClientError(err) + require.True(t, ok, "want a client-facing error, got %T: %v", err, err) + assert.Equal(t, 500, ce.code) + assert.Contains(t, ce.message, "AUDIT LOSS") + assert.Contains(t, ce.message, executionID) + + code, body := (&Handler{}).handleRequestError(err) + assert.Equal(t, 500, code) + m, ok := body.(map[string]any) + require.True(t, ok, "want a structured body, got %T", body) + assert.Equal(t, executionID, m["execution_id"]) + assert.Equal(t, false, m["email_sent"]) + assert.Equal(t, emailReason, m["email_reason"]) + assert.NotContains(t, m["error"], "connection reset", "the store error must stay out of the response body") +} + // TestFinalizePurchaseStatus_EmailSentWritesNothing: a sent email leaves the // row pending and touches the store not at all. func TestFinalizePurchaseStatus_EmailSentWritesNothing(t *testing.T) { store := new(MockConfigStore) h := &Handler{config: store} - require.Equal(t, "pending", h.finalizePurchaseStatus(context.Background(), newFinalizeExecution(), true, "")) + status, err := h.finalizePurchaseStatus(context.Background(), newFinalizeExecution(), true, "") + require.NoError(t, err) + require.Equal(t, "pending", status) store.AssertNotCalled(t, "SaveExecutionIfStatus", mock.Anything, mock.Anything, mock.Anything) store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) } + +// TestExecutePurchase_EmailFailureNotRecorded_Returns500 drives the submit +// path (issue #54): no notifier is wired, so the approval email fails, and +// the guarded write recording that failure errors too. Before the fix the +// handler answered 200 {"status":"pending","email_sent":false}. +func TestExecutePurchase_EmailFailureNotRecorded_Returns500(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + authSvc := new(MockAuthService) + authSvc.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + authSvc.grantAdminPurchaser() + var inserted *config.PurchaseExecution + store.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { inserted = args.Get(1).(*config.PurchaseExecution) }). + Return(nil).Once() + store.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + store.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) + expectStoredRecs(store, config.RecommendationRecord{Provider: "aws", Service: "ec2", ResourceType: "m5.large", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, Savings: 50}) + saveErr := errors.New("connection reset") + store.On("SaveExecutionIfStatus", ctx, mock.Anything, []string{"pending"}).Return(saveErr).Once() + h := &Handler{config: store, auth: authSvc} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"recommendations": [{"id": "", "provider": "aws", "service": "ec2", "resource_type": "m5.large", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 100.0, "savings": 50.0}]}`, + } + + result, err := h.executePurchase(ctx, req) + + assert.Nil(t, result, "no success body may accompany the error") + require.NotNil(t, inserted, "the pending row must have been inserted before the send") + requireEmailFailureNotRecorded(t, err, inserted.ExecutionID, "email notifier not configured for this deployment") + assert.ErrorIs(t, err, saveErr) + store.AssertExpectations(t) +} + +// TestRetryPurchase_EmailFailureNotRecorded_Returns500 is the retry-path +// twin: the successor row is inserted and linked, its approval email fails, +// and recording that failure errors. The response must be the 500 naming the +// successor, not a 200 that reports it as pending. +func TestRetryPurchase_EmailFailureNotRecorded_Returns500(t *testing.T) { + creator := retryOtherID + failedExec := &config.PurchaseExecution{ + ExecutionID: retryExecID, + Status: "failed", + Error: "send failed: transient SES throttle", + CreatedByUserID: &creator, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, + } + h, store, _ := buildSessionRetryHandler(failedExec, &Session{UserID: retryCallerID, Email: "admin@example.com"}, true, false) + var successor *config.PurchaseExecution + store.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { successor = args.Get(1).(*config.PurchaseExecution) }). + Return(nil).Once() + store.On("LinkRetryExecutionAtomic", mock.Anything, mock.Anything, retryExecID, mock.AnythingOfType("string")).Return(true, nil).Once() + saveErr := errors.New("connection reset") + store.On("SaveExecutionIfStatus", mock.Anything, mock.Anything, []string{"pending"}).Return(saveErr).Once() + + result, err := h.retryPurchase(context.Background(), sessionRetryReq(), retryExecID) + + assert.Nil(t, result, "no success body may accompany the error") + require.NotNil(t, successor, "the successor row must have been inserted before the send") + requireEmailFailureNotRecorded(t, err, successor.ExecutionID, "email notifier not configured for this deployment") + assert.ErrorIs(t, err, saveErr) + store.AssertExpectations(t) +}