diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index f80f3554e..5251e8a3f 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2308,7 +2308,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, @@ -3211,9 +3214,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; @@ -3221,9 +3222,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" @@ -3232,16 +3239,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. @@ -3644,11 +3668,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) +}