From 17654ccf3937a803d11bfff67e58e9c942838e9e Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 01:26:03 +0100 Subject: [PATCH] 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) +}