diff --git a/api/handlers/reconcile.go b/api/handlers/reconcile.go index 908b44c..451c489 100644 --- a/api/handlers/reconcile.go +++ b/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}) diff --git a/api/handlers/reconcile_test.go b/api/handlers/reconcile_test.go index 2f3f446..b92481b 100644 --- a/api/handlers/reconcile_test.go +++ b/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. diff --git a/api/main.go b/api/main.go index a104ad2..43ecaa3 100644 --- a/api/main.go +++ b/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) diff --git a/pkg/config/config.go b/pkg/config/config.go index c4eeca2..e0dbf4e 100644 --- a/pkg/config/config.go +++ b/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 diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 7592f6b..91acb82 100644 --- a/pkg/config/config_test.go +++ b/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) + } + }) + } +}