fix(sca): address pr review on abandoned 3ds sweep

- Drop stripe.Key write in ReconcileStale3DSOrders; wire once in api/main.go after cfg.Load so concurrent reconcilers don't
race the SDK global.
- Add empty-PI short-circuit before paymentintent.Get to bury a future regression in a slog.Warn instead of a Stripe 4xx.
- Swap fmt.Printf for slog.Warn with k/v attrs in loadAbandoned3DSThresholdMinutes so Loki sees structured fields instead of
 literal %q/%d.
- Comment the updated_at predicate (RetryAuth bumps it via GORM auto-stamp, intentional grace-window extension) + the
conditional-UPDATE/map-form choice.
- Tests: processing/requires_capture skip, unknown-status skip, stripe-error skip, empty-PI skip, full env-parse table for
loadAbandoned3DSThresholdMinutes.

api/handlers/reconcile.go

@@ -88,12 +88,19 @@ func ReconcileOrders(stripeKey string) {
 // after re-checking the PaymentIntent with Stripe so a late
 // payment_intent.succeeded webhook is respected. Deliberately separate
 // from ReconcileOrders: the empty-PI predicate there is load-bearing for
-// the crash-min-create case (see sca-psd2-compliance.md caveat #20).
-func ReconcileStale3DSOrders(stripeKey string, threshold time.Duration) {
-	stripe.Key = stripeKey
+// the crash-mid-create case (see sca-psd2-compliance.md caveat #20).
+//
+// stripeKey is unused; stripe.Key is wired once in main.go at startup
+// so concurrent reconcilers don't race on the package-level global. Parameter
+// kept to match the other reconciler's signature for the ticker call site.
+func ReconcileStale3DSOrders(_ string, threshold time.Duration) {
 	db := database.GetDB()
 
 	var orders []models.Order
+	// Predicate keys off updated_at, not created_at: respondRequiresAction
+	// and RetryAuth both bump updated_at via GORM auto-stamping, so every
+	// customer-driven retry extends the grace window. Intentional - sweeping
+	// a row the customer just re-armed would race the new challenge.
 	cutoff := time.Now().Add(-threshold)
 	if err := db.Where("status = ? AND updated_at < ?",
 		models.OrderStatusRequiresAction, cutoff).Find(&orders).Error; err != nil {
@@ -108,6 +115,13 @@ func ReconcileStale3DSOrders(stripeKey string, threshold time.Duration) {
 
 	for i := range orders {
 		order := &orders[i]
+		if order.StripePaymentID == "" {
+			// respondRequiresAction writes stripe_payment_id and status in
+			// one UPDATE so this should be unreachable; guard anyway so a
+			// future regression noisily logs instead of 4xx'ing Stripe.
+			reconcileLog().Warn("requires_action row missing PI id; skipping", "order_id", order.ID)
+			continue
+		}
 		actual, err := paymentIntentGetFn(order.StripePaymentID, nil)
 		if err != nil {
 			reconcileLog().Error("stripe get failed",
@@ -125,7 +139,7 @@ func ReconcileStale3DSOrders(stripeKey string, threshold time.Duration) {
 				"order_id", order.ID, "pi", order.StripePaymentID)
 		case stripe.PaymentIntentStatusProcessing,
 			stripe.PaymentIntentStatusRequiresCapture:
-			reconcileLog().Info("payment intent still transitioning: skipping",
+			reconcileLog().Info("payment intent still transitioning; skipping",
 				"order_id", order.ID, "pi", order.StripePaymentID, "stripe_status", string(actual.Status))
 		case stripe.PaymentIntentStatusRequiresAction,
 			stripe.PaymentIntentStatusRequiresPaymentMethod,
@@ -144,6 +158,9 @@ func ReconcileStale3DSOrders(stripeKey string, threshold time.Duration) {
 // payment_intent.succeeded webhook that landed between the stripe get and
 // this write (caveat #20 race) wins and we silently no-op
 func flipStale3DSToFailed(db *gorm.DB, order *models.Order, stripeStatus string) {
+	// Map form so the single-field UPDATE is explicit at the call site.
+	// GORM still auto-stamps updated_at; the row exits the sweep predicate
+	// naturally on the next tick because status no longer matches anyway.
 	res := db.Model(&models.Order{}).
 		Where("id = ? AND status = ?", order.ID, models.OrderStatusRequiresAction).
 		Updates(map[string]any{"status": models.OrderStatusFailed})

api/handlers/reconcile_test.go

@@ -1,6 +1,7 @@
 package handlers
 
 import (
+	"fmt"
 	"os"
 	"testing"
 	"time"
@@ -238,6 +239,144 @@ func TestReconcileStale3DSOrders_DoesNotConsumeRetryCap(t *testing.T) {
 	}
 }
 
+// TestReconcileStale3DSOrders_SkipsTransitioningStatuses covers the
+// processing and requires_capture branches: Stripe still owns the PI's
+// next move, so the sweep must not flip even past the threshold.
+func TestReconcileStale3DSOrders_SkipsTransitioningStatuses(t *testing.T) {
+	cases := []struct {
+		name   string
+		status stripe.PaymentIntentStatus
+	}{
+		{"processing", stripe.PaymentIntentStatusProcessing},
+		{"requires_capture", stripe.PaymentIntentStatusRequiresCapture},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			testDB, user := setupCartTestDB(t)
+			defer func() { _ = os.Remove(testDB) }()
+			defer database.ResetForTesting()
+
+			db := database.GetDB()
+			order := seedRequiresActionOrder(t, user.ID,
+				"pi_transitioning_"+tc.name, time.Now().Add(-1*time.Hour))
+
+			orig := paymentIntentGetFn
+			defer func() { paymentIntentGetFn = orig }()
+			paymentIntentGetFn = func(id string, _ *stripe.PaymentIntentParams) (*stripe.PaymentIntent, error) {
+				return &stripe.PaymentIntent{ID: id, Status: tc.status}, nil
+			}
+
+			ReconcileStale3DSOrders("", 30*time.Minute)
+
+			var reloaded models.Order
+			if err := db.First(&reloaded, order.ID).Error; err != nil {
+				t.Fatalf("reload: %v", err)
+			}
+			if reloaded.Status != models.OrderStatusRequiresAction {
+				t.Fatalf("%s must stay requires_action; got %q", tc.name, reloaded.Status)
+			}
+		})
+	}
+}
+
+// TestReconcileStale3DSOrders_SkipsUnknownStatus covers the switch's default
+// arm: an unrecognized Stripe status logs and skips rather than guessing a
+// transition. Defends against new PI statuses appearing in future SDKs.
+func TestReconcileStale3DSOrders_SkipsUnknownStatus(t *testing.T) {
+	testDB, user := setupCartTestDB(t)
+	defer func() { _ = os.Remove(testDB) }()
+	defer database.ResetForTesting()
+
+	db := database.GetDB()
+	order := seedRequiresActionOrder(t, user.ID, "pi_unknown_status",
+		time.Now().Add(-1*time.Hour))
+
+	orig := paymentIntentGetFn
+	defer func() { paymentIntentGetFn = orig }()
+	paymentIntentGetFn = func(id string, _ *stripe.PaymentIntentParams) (*stripe.PaymentIntent, error) {
+		return &stripe.PaymentIntent{
+			ID:     id,
+			Status: stripe.PaymentIntentStatus("some_new_future_status"),
+		}, nil
+	}
+
+	ReconcileStale3DSOrders("", 30*time.Minute)
+
+	var reloaded models.Order
+	if err := db.First(&reloaded, order.ID).Error; err != nil {
+		t.Fatalf("reload: %v", err)
+	}
+	if reloaded.Status != models.OrderStatusRequiresAction {
+		t.Fatalf("unknown status must not flip; got %q", reloaded.Status)
+	}
+}
+
+// TestReconcileStale3DSOrders_SkipsOnStripeError verifies a Stripe Get
+// failure logs and leaves the row alone. Transient network errors must not
+// look like an abandoned 3DS.
+func TestReconcileStale3DSOrders_SkipsOnStripeError(t *testing.T) {
+	testDB, user := setupCartTestDB(t)
+	defer func() { _ = os.Remove(testDB) }()
+	defer database.ResetForTesting()
+
+	db := database.GetDB()
+	order := seedRequiresActionOrder(t, user.ID, "pi_stripe_error",
+		time.Now().Add(-1*time.Hour))
+
+	orig := paymentIntentGetFn
+	defer func() { paymentIntentGetFn = orig }()
+	paymentIntentGetFn = func(_ string, _ *stripe.PaymentIntentParams) (*stripe.PaymentIntent, error) {
+		return nil, fmt.Errorf("simulated stripe outage")
+	}
+
+	ReconcileStale3DSOrders("", 30*time.Minute)
+
+	var reloaded models.Order
+	if err := db.First(&reloaded, order.ID).Error; err != nil {
+		t.Fatalf("reload: %v", err)
+	}
+	if reloaded.Status != models.OrderStatusRequiresAction {
+		t.Fatalf("stripe error must not flip; got %q", reloaded.Status)
+	}
+}
+
+// TestReconcileStale3DSOrders_SkipsEmptyPaymentIntentID guards the
+// defensive empty-PI short-circuit: even past the threshold, a row missing
+// stripe_payment_id must not reach paymentintent.Get (which would 4xx).
+func TestReconcileStale3DSOrders_SkipsEmptyPaymentIntentID(t *testing.T) {
+	testDB, user := setupCartTestDB(t)
+	defer func() { _ = os.Remove(testDB) }()
+	defer database.ResetForTesting()
+
+	db := database.GetDB()
+	order := seedRequiresActionOrder(t, user.ID, "pi_will_be_blanked",
+		time.Now().Add(-1*time.Hour))
+	if err := db.Model(&order).UpdateColumn("stripe_payment_id", "").Error; err != nil {
+		t.Fatalf("blank PI id: %v", err)
+	}
+
+	called := false
+	orig := paymentIntentGetFn
+	defer func() { paymentIntentGetFn = orig }()
+	paymentIntentGetFn = func(_ string, _ *stripe.PaymentIntentParams) (*stripe.PaymentIntent, error) {
+		called = true
+		return &stripe.PaymentIntent{Status: stripe.PaymentIntentStatusRequiresPaymentMethod}, nil
+	}
+
+	ReconcileStale3DSOrders("", 30*time.Minute)
+
+	if called {
+		t.Fatalf("stripe Get called for empty-PI row")
+	}
+	var reloaded models.Order
+	if err := db.First(&reloaded, order.ID).Error; err != nil {
+		t.Fatalf("reload: %v", err)
+	}
+	if reloaded.Status != models.OrderStatusRequiresAction {
+		t.Fatalf("empty-PI guard must leave status alone; got %q", reloaded.Status)
+	}
+}
+
 // seedRequiresActionOrder inserts a card + order pair in the requires_action
 // state. updatedAt is forced via UpdateColumn so GORM's auto-stamping of
 // UpdatedAt on Create does not bump the row out of the sweep predicate.

api/main.go

@@ -19,6 +19,8 @@ import (
 	"terminalShop/pkg/database"
 	"terminalShop/pkg/observability"
 	"terminalShop/pkg/stripeclient"
+
+	"github.com/stripe/stripe-go/v78"
 )
 
 const version = "v0.1.0"
@@ -49,6 +51,12 @@ func main() {
 	}
 	audit.SetDB(db)
 
+	// wire the stripe SDK's package-level key once so concurrent reconcilers
+	// and the request path dont race on stripe.Key writes (handler still
+	// re-assign per request because they predate this init - pre-exiting
+	// debt, but no NEW writer is added on the reconciler path now)
+	stripe.Key = cfg.StripeSecretKey
+
 	// Seed database with initial data
 	if err := database.Seed(db); err != nil {
 		log.Fatalf("Failed to seed database: %v", err)

pkg/config/config.go

@@ -3,6 +3,7 @@ package config
 import (
 	"errors"
 	"fmt"
+	"log/slog"
 	"net/url"
 	"os"
 	"strconv"
@@ -165,7 +166,8 @@ func loadAbandoned3DSThresholdMinutes() int {
 	}
 	n, err := strconv.Atoi(raw)
 	if err != nil || n < 1 {
-		fmt.Printf("WARNING: ABANDONED_3DS_THRESHOLD_MINUTES=%q invalid, falling back to default %d\n", raw, defaultMinutes)
+		slog.Warn("invalid ABANDONED_3DS_THRESHOLD_MINUTES, falling back",
+			"raw", raw, "default", defaultMinutes)
 		return defaultMinutes
 	}
 	return n

pkg/config/config_test.go

@@ -32,3 +32,33 @@ func TestLoadMaxOrderCents(t *testing.T) {
 		})
 	}
 }
+
+// TestLoadAbandoned3DSThresholdMinutes covers the env-parsing surface for
+// the sweep threshold. Zero and negative fall back to the default (unlike
+// MaxOrderCents where zero is a deliberate off-switch) because a zero
+// threshold would sweep every requires_action row on the first tick and
+// race the customer's in-progress challenge.
+func TestLoadAbandoned3DSThresholdMinutes(t *testing.T) {
+	cases := []struct {
+		name string
+		env  string // "" means unset
+		want int
+	}{
+		{"unset uses default", "", 30},
+		{"valid override", "45", 45},
+		{"zero falls back to default", "0", 30},
+		{"negative falls back to default", "-5", 30},
+		{"garbage falls back to default", "half an hour", 30},
+		{"valid minimum", "1", 1},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Setenv("ABANDONED_3DS_THRESHOLD_MINUTES", tc.env)
+			got := loadAbandoned3DSThresholdMinutes()
+			if got != tc.want {
+				t.Errorf("loadAbandoned3DSThresholdMinutes() = %d, want %d", got, tc.want)
+			}
+		})
+	}
+}