fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending

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

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 = &notify
+	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)
+}

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)
+}