From b416f3e4db1044b5f491712b1342c6765949da80 Mon Sep 17 00:00:00 2001 From: Sheng Kun Chang Date: Mon, 6 Jul 2026 16:00:23 +0800 Subject: [PATCH 1/2] feat: service-block eligibility gate (migrations 038-040) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Block an account's services when its payment standing is unsafe, per the product gate: an account is ELIGIBLE iff (1) it has >= 1 non-fraud card on file, (2) its first charge did not fail, and (3) its consecutive failed-charge streak is < 2. Any failing gate blocks. Mirrors the pure-policy posture of internal/account/collection: - internal/account/eligibility — pure Evaluate(Signals) -> Verdict, no DB/Stripe/clock, exhaustive table tests. Grace edges: a brand-new account (no charge yet) and one whose first invoice is still retrying pass gate 2; only an uncollectible first charge blocks. Gate 1 (a card) is unconditional. - migrations 038-040 (+ init-db wiring): payment_methods_mirror.fraud_blocked (+reason/flagged_at, defaults non-fraud so the gate works immediately; the fraud-setting webhook is a follow-up), invoices.ever_failed (sticky per-invoice latch), accounts.failed_charge_streak (recoverable counter). - webhook: invoice.payment_failed advances the streak once per distinct invoice (ever_failed latch dedups Stripe's per-retry events); invoice.paid resets it to 0 (auto-cure). Both idempotent under replay. - billing.GetServiceStatus RPC (internal-secret) — gathers the three signals in one read and returns {blocked, reason, reasons} for the platform's gate. No account yet => blocked on the card gate. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/account-api/main.go | 8 + internal/account/billing/service.go | 73 ++++++++ internal/account/billing/service_test.go | 107 +++++++++++ internal/account/billing/store.go | 19 ++ internal/account/billing/types.go | 26 +++ internal/account/db/billing.sql.go | 60 +++++++ internal/account/db/models.go | 10 ++ internal/account/db/queries/billing.sql | 46 +++++ internal/account/db/queries/webhook.sql | 39 ++++ internal/account/db/webhook.sql.go | 66 +++++++ internal/account/eligibility/eligibility.go | 128 ++++++++++++++ .../account/eligibility/eligibility_test.go | 166 ++++++++++++++++++ internal/account/webhook/handlers.go | 34 +++- internal/account/webhook/router.go | 16 ++ internal/account/webhook/router_test.go | 100 +++++++++++ internal/account/webhook/store.go | 51 ++++++ .../account/webhook/store_integration_test.go | 100 +++++++++++ internal/account/webhook/webhooktest/fakes.go | 22 +++ .../billing/038_payment_method_fraud.down.sql | 5 + .../billing/038_payment_method_fraud.up.sql | 42 +++++ .../billing/039_invoice_ever_failed.down.sql | 3 + .../billing/039_invoice_ever_failed.up.sql | 31 ++++ .../040_account_failed_charge_streak.down.sql | 3 + .../040_account_failed_charge_streak.up.sql | 30 ++++ scripts/init-db.sql | 6 + 25 files changed, 1188 insertions(+), 3 deletions(-) create mode 100644 internal/account/eligibility/eligibility.go create mode 100644 internal/account/eligibility/eligibility_test.go create mode 100644 migrations/billing/038_payment_method_fraud.down.sql create mode 100644 migrations/billing/038_payment_method_fraud.up.sql create mode 100644 migrations/billing/039_invoice_ever_failed.down.sql create mode 100644 migrations/billing/039_invoice_ever_failed.up.sql create mode 100644 migrations/billing/040_account_failed_charge_streak.down.sql create mode 100644 migrations/billing/040_account_failed_charge_streak.up.sql diff --git a/cmd/account-api/main.go b/cmd/account-api/main.go index 777b664..55f9c9c 100644 --- a/cmd/account-api/main.go +++ b/cmd/account-api/main.go @@ -104,6 +104,13 @@ func (d *dispatcher) dispatch(ctx context.Context, action string, requestPayload } return d.svc.GetPaymentMethods(ctx, req) + case "GetServiceStatus": + var req billing.GetServiceStatusRequest + if err := json.Unmarshal(requestPayload, &req); err != nil { + return nil, billing.InvalidInput("malformed request payload: " + err.Error()) + } + return d.svc.GetServiceStatus(ctx, req) + case "DetachPaymentMethod": var req billing.DetachPaymentMethodRequest if err := json.Unmarshal(requestPayload, &req); err != nil { @@ -350,6 +357,7 @@ func buildRouter(d *dispatcher) *chi.Mux { r.Post("/v1/billing.StartAddPaymentMethod", makeHTTPHandler(d, "StartAddPaymentMethod")) r.Post("/v1/billing.FinishAddPaymentMethod", makeHTTPHandler(d, "FinishAddPaymentMethod")) r.Post("/v1/billing.GetPaymentMethods", makeHTTPHandler(d, "GetPaymentMethods")) + r.Post("/v1/billing.GetServiceStatus", makeHTTPHandler(d, "GetServiceStatus")) r.Post("/v1/billing.DetachPaymentMethod", makeHTTPHandler(d, "DetachPaymentMethod")) r.Post("/v1/billing.SetDefaultPaymentMethod", makeHTTPHandler(d, "SetDefaultPaymentMethod")) // Usage RPCs invoked by the platform control plane (manifest metric diff --git a/internal/account/billing/service.go b/internal/account/billing/service.go index a6b111a..54f79d2 100644 --- a/internal/account/billing/service.go +++ b/internal/account/billing/service.go @@ -6,6 +6,7 @@ import ( "github.com/google/uuid" + "github.com/mirrorstack-ai/billing-engine/internal/account/eligibility" billingstripe "github.com/mirrorstack-ai/billing-engine/internal/shared/stripe" ) @@ -298,6 +299,78 @@ func (s *Service) GetPaymentMethods(ctx context.Context, req GetPaymentMethodsRe return &GetPaymentMethodsResponse{PaymentMethods: methods}, nil } +// GetServiceStatus is the read behind the platform's service-block gate: it +// returns whether the account's services should be BLOCKED on payment standing +// (no usable non-fraud card, a failed first charge, or a failed-charge streak +// >= 2), per internal/account/eligibility. Read-only; never touches Stripe. +// +// A user with no billing account yet has no card on file, so the gate blocks on +// the card requirement (ReasonNoUsableCard) rather than 404-ing — the caller +// reads one Blocked flag and never has to special-case "not set up". A real +// account gathers the three signals in one store read and hands them to the +// pure Evaluate. +func (s *Service) GetServiceStatus(ctx context.Context, req GetServiceStatusRequest) (*GetServiceStatusResponse, error) { + if req.UserID == uuid.Nil { + return nil, InvalidInput("user_id required") + } + + accountID, found, err := s.store.AccountByUser(ctx, req.UserID) + if err != nil { + return nil, Internal("account lookup failed", err) + } + if !found { + // No account ⇒ no card ⇒ blocked on the (unconditional) card gate. + return serviceStatusResponse(eligibility.Verdict{ + Blocked: true, + Reason: eligibility.ReasonNoUsableCard, + Reasons: []eligibility.Reason{eligibility.ReasonNoUsableCard}, + }), nil + } + + sig, err := s.store.ServiceBlockSignals(ctx, accountID) + if err != nil { + return nil, Internal("service-block signals read failed", err) + } + verdict := eligibility.Evaluate(eligibility.Signals{ + UsableNonFraudCardCount: sig.UsableCardCount, + FirstCharge: firstChargeState(sig.FirstChargeStatus), + FailedChargeStreak: sig.FailedChargeStreak, + }) + return serviceStatusResponse(verdict), nil +} + +// firstChargeState maps the earliest real invoice's Stripe status (from +// ServiceBlockSignals) to the eligibility enum. The query already excludes +// draft/void, so only paid/open/uncollectible/"" reach here; "" (no charge yet) +// and any unrecognized status fall through to FirstChargeNone (graced). +func firstChargeState(status string) eligibility.FirstChargeState { + switch status { + case "paid": + return eligibility.FirstChargeSucceeded + case "open": + return eligibility.FirstChargePending + case "uncollectible": + return eligibility.FirstChargeFailed + default: + return eligibility.FirstChargeNone + } +} + +// serviceStatusResponse serializes an eligibility.Verdict into the wire +// response, stringifying the Reason codes. An eligible verdict carries an empty +// Reasons slice, which the omitempty tag drops from the JSON. +func serviceStatusResponse(v eligibility.Verdict) *GetServiceStatusResponse { + reasons := make([]string, len(v.Reasons)) + for i, r := range v.Reasons { + reasons[i] = string(r) + } + return &GetServiceStatusResponse{ + Blocked: v.Blocked, + Reason: string(v.Reason), + Reasons: reasons, + } +} + // DetachPaymentMethod detaches a saved card from the user's Stripe // Customer. Ownership is enforced via PaymentMethodTarget — the PM must // belong to the caller's account. The mirror row is soft-deleted diff --git a/internal/account/billing/service_test.go b/internal/account/billing/service_test.go index cf38952..16529c8 100644 --- a/internal/account/billing/service_test.go +++ b/internal/account/billing/service_test.go @@ -20,6 +20,7 @@ type fakeStore struct { hasUsablePM map[uuid.UUID]bool hasUnpaidInvoice map[uuid.UUID]bool paymentMethodsBy map[uuid.UUID][]billing.PaymentMethod + serviceSignals map[uuid.UUID]billing.ServiceSignals addCardRequests map[uuid.UUID]*fakeAddCardRequest // PM-target lookups for detach / set-default, keyed by payment method id. @@ -32,6 +33,7 @@ type fakeStore struct { errHasUsablePaymentMx error errHasUnpaidInvoice error errListPaymentMethods error + errServiceSignals error errPaymentMethodTarget error errInsertAddCardRequest error errSetSetupIntent error @@ -62,6 +64,7 @@ func newFakeStore() *fakeStore { hasUsablePM: map[uuid.UUID]bool{}, hasUnpaidInvoice: map[uuid.UUID]bool{}, paymentMethodsBy: map[uuid.UUID][]billing.PaymentMethod{}, + serviceSignals: map[uuid.UUID]billing.ServiceSignals{}, pmTargets: map[uuid.UUID]pmTarget{}, addCardRequests: map[uuid.UUID]*fakeAddCardRequest{}, } @@ -125,6 +128,13 @@ func (s *fakeStore) ListPaymentMethods(_ context.Context, accountID uuid.UUID) ( return s.paymentMethodsBy[accountID], nil } +func (s *fakeStore) ServiceBlockSignals(_ context.Context, accountID uuid.UUID) (billing.ServiceSignals, error) { + if s.errServiceSignals != nil { + return billing.ServiceSignals{}, s.errServiceSignals + } + return s.serviceSignals[accountID], nil +} + func (s *fakeStore) PaymentMethodTarget(_ context.Context, _ uuid.UUID, paymentMethodID uuid.UUID) (string, string, bool, bool, error) { if s.errPaymentMethodTarget != nil { return "", "", false, false, s.errPaymentMethodTarget @@ -621,6 +631,103 @@ func TestGetPaymentMethods_HasMethods_ReturnsAll(t *testing.T) { require.True(t, resp.PaymentMethods[0].IsDefault) } +// --- GetServiceStatus (service-block gate) -------------------------------- + +func TestGetServiceStatus_MissingUserID_InvalidInput(t *testing.T) { + svc := billing.NewService(newFakeStore(), &fakeStripe{}, "") + + _, err := svc.GetServiceStatus(context.Background(), billing.GetServiceStatusRequest{}) + + var be *billing.Error + require.ErrorAs(t, err, &be) + require.Equal(t, billing.CodeInvalidInput, be.Code) +} + +func TestGetServiceStatus_NoAccount_BlockedNoCard(t *testing.T) { + // A user with no billing account has no card on file → blocked on the card + // gate, not a 404. + svc := billing.NewService(newFakeStore(), &fakeStripe{}, "") + + resp, err := svc.GetServiceStatus(context.Background(), billing.GetServiceStatusRequest{UserID: uuid.New()}) + + require.NoError(t, err) + require.True(t, resp.Blocked) + require.Equal(t, "NO_USABLE_CARD", resp.Reason) + require.Equal(t, []string{"NO_USABLE_CARD"}, resp.Reasons) +} + +func TestGetServiceStatus_Eligible(t *testing.T) { + store := newFakeStore() + userID, accountID := uuid.New(), uuid.New() + store.accountsByUser[userID] = fakeAccount{id: accountID, stripeCustomerID: "cus_x"} + store.serviceSignals[accountID] = billing.ServiceSignals{ + UsableCardCount: 1, FailedChargeStreak: 1, FirstChargeStatus: "paid", + } + svc := billing.NewService(store, &fakeStripe{}, "") + + resp, err := svc.GetServiceStatus(context.Background(), billing.GetServiceStatusRequest{UserID: userID}) + + require.NoError(t, err) + require.False(t, resp.Blocked) + require.Equal(t, "ELIGIBLE", resp.Reason) + require.Empty(t, resp.Reasons) +} + +func TestGetServiceStatus_NewAccountWithCardIsGraced(t *testing.T) { + // No charge yet ("" first-charge status) + a card → eligible (grace). + store := newFakeStore() + userID, accountID := uuid.New(), uuid.New() + store.accountsByUser[userID] = fakeAccount{id: accountID} + store.serviceSignals[accountID] = billing.ServiceSignals{UsableCardCount: 1, FirstChargeStatus: ""} + svc := billing.NewService(store, &fakeStripe{}, "") + + resp, err := svc.GetServiceStatus(context.Background(), billing.GetServiceStatusRequest{UserID: userID}) + + require.NoError(t, err) + require.False(t, resp.Blocked) +} + +func TestGetServiceStatus_MapsSignalsToBlockingReasons(t *testing.T) { + cases := []struct { + name string + signals billing.ServiceSignals + wantReason string + }{ + {"no card", billing.ServiceSignals{UsableCardCount: 0, FirstChargeStatus: "paid"}, "NO_USABLE_CARD"}, + {"first charge uncollectible", billing.ServiceSignals{UsableCardCount: 1, FirstChargeStatus: "uncollectible"}, "FIRST_CHARGE_FAILED"}, + {"streak of two", billing.ServiceSignals{UsableCardCount: 1, FirstChargeStatus: "paid", FailedChargeStreak: 2}, "TOO_MANY_FAILURES"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + store := newFakeStore() + userID, accountID := uuid.New(), uuid.New() + store.accountsByUser[userID] = fakeAccount{id: accountID} + store.serviceSignals[accountID] = tc.signals + svc := billing.NewService(store, &fakeStripe{}, "") + + resp, err := svc.GetServiceStatus(context.Background(), billing.GetServiceStatusRequest{UserID: userID}) + + require.NoError(t, err) + require.True(t, resp.Blocked) + require.Equal(t, tc.wantReason, resp.Reason) + }) + } +} + +func TestGetServiceStatus_SignalsError_Internal(t *testing.T) { + store := newFakeStore() + userID, accountID := uuid.New(), uuid.New() + store.accountsByUser[userID] = fakeAccount{id: accountID} + store.errServiceSignals = errors.New("conn dropped") + svc := billing.NewService(store, &fakeStripe{}, "") + + _, err := svc.GetServiceStatus(context.Background(), billing.GetServiceStatusRequest{UserID: userID}) + + var be *billing.Error + require.ErrorAs(t, err, &be) + require.Equal(t, billing.CodeInternal, be.Code) +} + func TestDetachPaymentMethod_OwnedPM_DetachesFromStripe(t *testing.T) { store := newFakeStore() pmID := uuid.New() diff --git a/internal/account/billing/store.go b/internal/account/billing/store.go index 9533564..726fcd8 100644 --- a/internal/account/billing/store.go +++ b/internal/account/billing/store.go @@ -52,6 +52,13 @@ type Store interface { // none exist. ListPaymentMethods(ctx context.Context, accountID uuid.UUID) ([]PaymentMethod, error) + // ServiceBlockSignals reads, in one round-trip, the three inputs the + // service-block eligibility gate reasons over for an account: the usable + // non-fraud card count, the consecutive failed-charge streak, and the + // earliest real charge's status ("" when the account has none yet). See + // db.ServiceBlockSignals. + ServiceBlockSignals(ctx context.Context, accountID uuid.UUID) (ServiceSignals, error) + // PaymentMethodTarget resolves an active payment method owned by the // user, returning its Stripe PM id, the account's Stripe customer // id, and whether the row is currently the default. found=false when @@ -205,6 +212,18 @@ func (s *pgxStore) HasUnpaidInvoice(ctx context.Context, accountID uuid.UUID) (b return s.q.AccountHasUnpaidInvoice(ctx, accountID.String()) } +func (s *pgxStore) ServiceBlockSignals(ctx context.Context, accountID uuid.UUID) (ServiceSignals, error) { + row, err := s.q.ServiceBlockSignals(ctx, accountID.String()) + if err != nil { + return ServiceSignals{}, err + } + return ServiceSignals{ + UsableCardCount: int(row.UsableCardCount), + FailedChargeStreak: int(row.FailedChargeStreak), + FirstChargeStatus: row.FirstChargeStatus, + }, nil +} + func (s *pgxStore) ListPaymentMethods(ctx context.Context, accountID uuid.UUID) ([]PaymentMethod, error) { rows, err := s.q.ListPaymentMethods(ctx, accountID.String()) if err != nil { diff --git a/internal/account/billing/types.go b/internal/account/billing/types.go index 78c755d..ed9c4d1 100644 --- a/internal/account/billing/types.go +++ b/internal/account/billing/types.go @@ -179,6 +179,32 @@ type GetPaymentMethodsResponse struct { PaymentMethods []PaymentMethod `json:"payment_methods"` } +// ServiceSignals are the raw service-block gate inputs read for one account +// (db.ServiceBlockSignals). The service maps these into eligibility.Signals: +// FirstChargeStatus is the earliest real invoice's Stripe status ("" when the +// account has no charge yet), mapped to the FirstChargeState enum. +type ServiceSignals struct { + UsableCardCount int + FailedChargeStreak int + FirstChargeStatus string +} + +// GetServiceStatusRequest is the payload of GetServiceStatus — the account is +// addressed by owner, the same as Ensure / GetPaymentMethods. +type GetServiceStatusRequest struct { + UserID uuid.UUID `json:"user_id"` +} + +// GetServiceStatusResponse is the service-block verdict for the account. Blocked +// is the single field a caller must read to gate service; Reason is the primary +// machine-readable cause (eligibility.Reason, "ELIGIBLE" when not blocked); +// Reasons lists every failing gate (omitted when eligible). +type GetServiceStatusResponse struct { + Blocked bool `json:"blocked"` + Reason string `json:"reason"` + Reasons []string `json:"reasons,omitempty"` +} + // PaymentMethod is the projection of a payment_methods_mirror row // returned to UI consumers. Card-only in v1. type PaymentMethod struct { diff --git a/internal/account/db/billing.sql.go b/internal/account/db/billing.sql.go index f9f4d8c..b3c5287 100644 --- a/internal/account/db/billing.sql.go +++ b/internal/account/db/billing.sql.go @@ -197,6 +197,66 @@ func (q *Queries) SelectAccountByUser(ctx context.Context, ownerUserID pgtype.UU return i, err } +const serviceBlockSignals = `-- name: ServiceBlockSignals :one +SELECT + (SELECT COUNT(*) + FROM ms_billing.payment_methods_mirror pmm + WHERE pmm.account_id = a.id + AND pmm.deleted_at IS NULL + AND NOT pmm.fraud_blocked + AND (pmm.exp_year, pmm.exp_month) >= ( + EXTRACT(YEAR FROM current_date)::INT, + EXTRACT(MONTH FROM current_date)::INT + ))::int AS usable_card_count, + a.failed_charge_streak, + COALESCE(( + SELECT inv.status + FROM ms_billing.invoices inv + WHERE inv.account_id = a.id + AND inv.status NOT IN ('draft', 'void') + ORDER BY inv.created_at ASC, inv.id ASC + LIMIT 1 + ), '')::text AS first_charge_status +FROM ms_billing.accounts a +WHERE a.id = $1 +` + +type ServiceBlockSignalsRow struct { + UsableCardCount int32 `json:"usable_card_count"` + FailedChargeStreak int32 `json:"failed_charge_streak"` + FirstChargeStatus string `json:"first_charge_status"` +} + +// ServiceBlockSignals reads, in ONE round-trip, the three inputs the +// service-block eligibility gate (internal/account/eligibility) reasons over, +// all scoped to one already-resolved account id: +// +// usable_card_count — active (deleted_at IS NULL), non-fraud +// (NOT fraud_blocked, migration 038), NOT-expired cards. +// Reuses HasUsablePaymentMethod's expiry predicate, +// COUNT instead of EXISTS. The gate blocks at 0. +// failed_charge_streak — the account's consecutive failed-charge counter +// (migration 040), read verbatim. The gate blocks at >= 2. +// first_charge_status — the status of the account's EARLIEST real charge: +// the oldest invoice that is not 'draft' (never +// finalized) or 'void' (cancelled, never a real charge +// attempt), by (created_at, id) ASC. '' when the account +// has no such invoice yet (brand new — the gate graces +// it as long as a card is present). The Go layer maps +// '' / paid / open / uncollectible → the FirstChargeState +// enum (none / succeeded / pending / failed). +// +// Scalar subqueries (not JOINs) so each signal is independent and a NULL card +// count is impossible (COUNT is 0, not NULL); first_charge_status COALESCEs the +// no-invoice case to ”. One row per account id (or none → caller maps to the +// not-found verdict). +func (q *Queries) ServiceBlockSignals(ctx context.Context, id string) (ServiceBlockSignalsRow, error) { + row := q.db.QueryRow(ctx, serviceBlockSignals, id) + var i ServiceBlockSignalsRow + err := row.Scan(&i.UsableCardCount, &i.FailedChargeStreak, &i.FirstChargeStatus) + return i, err +} + const setStripeCustomer = `-- name: SetStripeCustomer :execrows UPDATE ms_billing.accounts SET stripe_customer_id = $2 WHERE id = $1 ` diff --git a/internal/account/db/models.go b/internal/account/db/models.go index 883f5f1..4b15d01 100644 --- a/internal/account/db/models.go +++ b/internal/account/db/models.go @@ -333,6 +333,8 @@ type MsBillingAccount struct { ActivatedAt pgtype.Timestamptz `json:"activated_at"` // Per-account size threshold (micro-USD) above which a SUCCESSFUL off-session charge is disclosed as "large" on the billing page. NULL = platform default ($100 = 100000000 micros). Resolved at charge time; pure disclosure, changes no charging behaviour. AutoCollectThresholdMicros pgtype.Int8 `json:"auto_collect_threshold_micros"` + // Consecutive distinct failed invoices, maintained by the invoice webhook (+1 on the first payment_failed of a row via invoices.ever_failed, reset to 0 on invoice.paid). The service-block gate blocks at >= 2. Recoverable — self-heals to 0 on the next successful charge. + FailedChargeStreak int32 `json:"failed_charge_streak"` } type MsBillingAddCardRequest struct { @@ -469,6 +471,8 @@ type MsBillingInvoice struct { InvoicePdf pgtype.Text `json:"invoice_pdf"` // Server-computed at invoice-create time: true iff the charged amount (netted arrears + advance base, micros) exceeded the account auto_collect_threshold_micros (or the default when NULL) that applied WHEN THE CHARGE FIRED. Post-hoc disclosure only. IsLargeAutoCollect bool `json:"is_large_auto_collect"` + // Sticky: set true on the FIRST invoice.payment_failed for this row and never cleared. The service-block gate uses it to count DISTINCT failed invoices (not per-retry events) into accounts.failed_charge_streak. Independent of the current status — survives a later flip to paid. + EverFailed bool `json:"ever_failed"` } type MsBillingMetricDefinition struct { @@ -511,6 +515,12 @@ type MsBillingPaymentMethodsMirror struct { AttachedAt time.Time `json:"attached_at"` DeletedAt pgtype.Timestamptz `json:"deleted_at"` Fingerprint pgtype.Text `json:"fingerprint"` + // True once Stripe flags this card as fraud/dispute risk (set by the radar.early_fraud_warning / charge.dispute webhook — follow-up PR). The service-block gate EXCLUDES fraud_blocked cards from the usable-card count. Defaults false so every existing card is treated non-fraud. + FraudBlocked bool `json:"fraud_blocked"` + // Audit: the signal that set fraud_blocked (e.g. early_fraud_warning, dispute). NULL until flagged. + FraudReason pgtype.Text `json:"fraud_reason"` + // Audit: when fraud_blocked was set. NULL until flagged. + FraudFlaggedAt pgtype.Timestamptz `json:"fraud_flagged_at"` } type MsBillingUsageAggregate struct { diff --git a/internal/account/db/queries/billing.sql b/internal/account/db/queries/billing.sql index fe75c41..7bfbc30 100644 --- a/internal/account/db/queries/billing.sql +++ b/internal/account/db/queries/billing.sql @@ -80,3 +80,49 @@ SELECT EXISTS ( WHERE account_id = $1 AND status IN ('open', 'uncollectible') ) AS has_unpaid; + +-- ServiceBlockSignals reads, in ONE round-trip, the three inputs the +-- service-block eligibility gate (internal/account/eligibility) reasons over, +-- all scoped to one already-resolved account id: +-- +-- usable_card_count — active (deleted_at IS NULL), non-fraud +-- (NOT fraud_blocked, migration 038), NOT-expired cards. +-- Reuses HasUsablePaymentMethod's expiry predicate, +-- COUNT instead of EXISTS. The gate blocks at 0. +-- failed_charge_streak — the account's consecutive failed-charge counter +-- (migration 040), read verbatim. The gate blocks at >= 2. +-- first_charge_status — the status of the account's EARLIEST real charge: +-- the oldest invoice that is not 'draft' (never +-- finalized) or 'void' (cancelled, never a real charge +-- attempt), by (created_at, id) ASC. '' when the account +-- has no such invoice yet (brand new — the gate graces +-- it as long as a card is present). The Go layer maps +-- '' / paid / open / uncollectible → the FirstChargeState +-- enum (none / succeeded / pending / failed). +-- +-- Scalar subqueries (not JOINs) so each signal is independent and a NULL card +-- count is impossible (COUNT is 0, not NULL); first_charge_status COALESCEs the +-- no-invoice case to ''. One row per account id (or none → caller maps to the +-- not-found verdict). +-- name: ServiceBlockSignals :one +SELECT + (SELECT COUNT(*) + FROM ms_billing.payment_methods_mirror pmm + WHERE pmm.account_id = a.id + AND pmm.deleted_at IS NULL + AND NOT pmm.fraud_blocked + AND (pmm.exp_year, pmm.exp_month) >= ( + EXTRACT(YEAR FROM current_date)::INT, + EXTRACT(MONTH FROM current_date)::INT + ))::int AS usable_card_count, + a.failed_charge_streak, + COALESCE(( + SELECT inv.status + FROM ms_billing.invoices inv + WHERE inv.account_id = a.id + AND inv.status NOT IN ('draft', 'void') + ORDER BY inv.created_at ASC, inv.id ASC + LIMIT 1 + ), '')::text AS first_charge_status +FROM ms_billing.accounts a +WHERE a.id = $1; diff --git a/internal/account/db/queries/webhook.sql b/internal/account/db/queries/webhook.sql index cc8ec9b..838a217 100644 --- a/internal/account/db/queries/webhook.sql +++ b/internal/account/db/queries/webhook.sql @@ -223,3 +223,42 @@ WHERE a.id = ( WHERE i.account_id = a.id AND i.status IN ('open', 'uncollectible') ); + +-- MarkInvoiceEverFailed sets the sticky ever_failed flag (migration 039) on the +-- first invoice.payment_failed for a row. The `AND NOT ever_failed` guard makes +-- it a one-way latch: :execrows returns 1 ONLY on the first flip, 0 on Stripe's +-- subsequent retry events (distinct event ids, so the webhook's event-level +-- dedup does not collapse them). The webhook increments the account streak ONLY +-- when this returns 1, so a single invoice's retries count as one failure. +-- name: MarkInvoiceEverFailed :execrows +UPDATE ms_billing.invoices +SET ever_failed = true +WHERE stripe_invoice_id = $1 + AND NOT ever_failed; + +-- IncrementFailedChargeStreak bumps the account's consecutive failed-charge +-- streak (migration 040) by one, resolving the account via the invoice mirror. +-- Called by the webhook ONLY after MarkInvoiceEverFailed reports a fresh flip, +-- so it counts distinct failed invoices, not per-retry events. :execrows so the +-- caller can log a drift no-op when the invoice has no mirror row yet. +-- name: IncrementFailedChargeStreak :execrows +UPDATE ms_billing.accounts +SET failed_charge_streak = failed_charge_streak + 1 +WHERE id = ( + SELECT inv.account_id FROM ms_billing.invoices AS inv + WHERE inv.stripe_invoice_id = $1 + ); + +-- ResetFailedChargeStreak clears the account's consecutive failed-charge streak +-- to 0 on a successful charge (invoice.paid) — the auto-cure that unblocks a +-- previously-blocked account. Resolves the account via the invoice mirror. The +-- `AND failed_charge_streak <> 0` guard makes it a no-op (0 rows) when the +-- streak is already clean, so a replayed/redundant paid event is idempotent. +-- name: ResetFailedChargeStreak :execrows +UPDATE ms_billing.accounts +SET failed_charge_streak = 0 +WHERE id = ( + SELECT inv.account_id FROM ms_billing.invoices AS inv + WHERE inv.stripe_invoice_id = $1 + ) + AND failed_charge_streak <> 0; diff --git a/internal/account/db/webhook.sql.go b/internal/account/db/webhook.sql.go index de769ab..60c4206 100644 --- a/internal/account/db/webhook.sql.go +++ b/internal/account/db/webhook.sql.go @@ -133,6 +133,28 @@ func (q *Queries) DuplicateFingerprintPM(ctx context.Context, arg DuplicateFinge return id, err } +const incrementFailedChargeStreak = `-- name: IncrementFailedChargeStreak :execrows +UPDATE ms_billing.accounts +SET failed_charge_streak = failed_charge_streak + 1 +WHERE id = ( + SELECT inv.account_id FROM ms_billing.invoices AS inv + WHERE inv.stripe_invoice_id = $1 + ) +` + +// IncrementFailedChargeStreak bumps the account's consecutive failed-charge +// streak (migration 040) by one, resolving the account via the invoice mirror. +// Called by the webhook ONLY after MarkInvoiceEverFailed reports a fresh flip, +// so it counts distinct failed invoices, not per-retry events. :execrows so the +// caller can log a drift no-op when the invoice has no mirror row yet. +func (q *Queries) IncrementFailedChargeStreak(ctx context.Context, stripeInvoiceID string) (int64, error) { + result, err := q.db.Exec(ctx, incrementFailedChargeStreak, stripeInvoiceID) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const insertPaymentMethod = `-- name: InsertPaymentMethod :execrows WITH acct AS ( SELECT id FROM ms_billing.accounts WHERE stripe_customer_id = $1 @@ -223,6 +245,27 @@ func (q *Queries) MarkEventProcessed(ctx context.Context, arg MarkEventProcessed return result.RowsAffected(), nil } +const markInvoiceEverFailed = `-- name: MarkInvoiceEverFailed :execrows +UPDATE ms_billing.invoices +SET ever_failed = true +WHERE stripe_invoice_id = $1 + AND NOT ever_failed +` + +// MarkInvoiceEverFailed sets the sticky ever_failed flag (migration 039) on the +// first invoice.payment_failed for a row. The `AND NOT ever_failed` guard makes +// it a one-way latch: :execrows returns 1 ONLY on the first flip, 0 on Stripe's +// subsequent retry events (distinct event ids, so the webhook's event-level +// dedup does not collapse them). The webhook increments the account streak ONLY +// when this returns 1, so a single invoice's retries count as one failure. +func (q *Queries) MarkInvoiceEverFailed(ctx context.Context, stripeInvoiceID string) (int64, error) { + result, err := q.db.Exec(ctx, markInvoiceEverFailed, stripeInvoiceID) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const mirrorRowByStripePM = `-- name: MirrorRowByStripePM :one SELECT id, account_id, fingerprint FROM ms_billing.payment_methods_mirror @@ -289,6 +332,29 @@ func (q *Queries) RelaxCollectionOnPaidInvoice(ctx context.Context, stripeInvoic return result.RowsAffected(), nil } +const resetFailedChargeStreak = `-- name: ResetFailedChargeStreak :execrows +UPDATE ms_billing.accounts +SET failed_charge_streak = 0 +WHERE id = ( + SELECT inv.account_id FROM ms_billing.invoices AS inv + WHERE inv.stripe_invoice_id = $1 + ) + AND failed_charge_streak <> 0 +` + +// ResetFailedChargeStreak clears the account's consecutive failed-charge streak +// to 0 on a successful charge (invoice.paid) — the auto-cure that unblocks a +// previously-blocked account. Resolves the account via the invoice mirror. The +// `AND failed_charge_streak <> 0` guard makes it a no-op (0 rows) when the +// streak is already clean, so a replayed/redundant paid event is idempotent. +func (q *Queries) ResetFailedChargeStreak(ctx context.Context, stripeInvoiceID string) (int64, error) { + result, err := q.db.Exec(ctx, resetFailedChargeStreak, stripeInvoiceID) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const resolveAddCardRequest = `-- name: ResolveAddCardRequest :exec UPDATE ms_billing.add_card_requests SET status = $2::ms_billing.add_card_request_status, diff --git a/internal/account/eligibility/eligibility.go b/internal/account/eligibility/eligibility.go new file mode 100644 index 0000000..dba1819 --- /dev/null +++ b/internal/account/eligibility/eligibility.go @@ -0,0 +1,128 @@ +// Package eligibility implements the SERVICE-BLOCK gate: the pure decision of +// whether an account's services may run, given its payment standing. It is a +// sibling of the collection package (internal/account/collection) and follows +// the same posture — DELIBERATELY PURE: the verdict is a total function of +// explicit Signals, with NO DB / Stripe / clock access. The caller (the +// account-api billing service) gathers the signals (via db.ServiceBlockSignals) +// and calls Evaluate; persistence + transport live in the caller. This keeps +// the policy unit-testable in isolation and puts the gate rules in one obvious, +// finance-legible place. +// +// The gate (product spec): an account is ELIGIBLE for service iff ALL of — +// +// (1) it has at least one NON-FRAUD card on file, +// (2) its FIRST charge did not fail, and +// (3) its consecutive failed-charge streak is < 2. +// +// If any gate fails, the account is BLOCKED. The three gates map one-for-one to +// the three Signals fields; see Evaluate for the exact boolean logic and the +// grace edges (a brand-new account with no charge yet is graced, not blocked). +package eligibility + +// MaxFailedChargeStreak is the consecutive failed-charge count at which services +// are blocked. The spec is "< 2 failed" (2 excluded), so a streak of 0 or 1 +// passes and 2+ blocks. The streak is RECOVERABLE — it resets to 0 on the next +// successful charge (accounts.failed_charge_streak, migration 040) — so a block +// here self-heals when the account pays. Hardcoded (mirrors collection's inline +// finance thresholds); promote to a per-account column only if a knob is needed. +const MaxFailedChargeStreak = 2 + +// FirstChargeState is the outcome of the account's EARLIEST real charge — the +// oldest invoice that is not draft (never finalized) or void (cancelled). It is +// the enum form of ServiceBlockSignals.first_charge_status, resolved by the +// caller from the invoice mirror. +type FirstChargeState string + +const ( + // FirstChargeNone: the account has no finalized charge yet (brand new). GRACE + // — eligible as long as a card is present, so arrears/usage billing isn't a + // bootstrap deadlock (the account must run to generate a charge at all). + FirstChargeNone FirstChargeState = "none" + // FirstChargeSucceeded: the earliest real invoice is 'paid'. Passes gate 2. + FirstChargeSucceeded FirstChargeState = "succeeded" + // FirstChargePending: the earliest real invoice is still 'open' (Stripe is + // smart-retrying). GRACE during the retry window — not yet a failure. + FirstChargePending FirstChargeState = "pending" + // FirstChargeFailed: the earliest real invoice went 'uncollectible' (Stripe + // gave up). This is the only first-charge state that BLOCKS (fails gate 2). + FirstChargeFailed FirstChargeState = "failed" +) + +// Signals are the live inputs the gate reasons over, gathered by the caller in +// one DB read (db.ServiceBlockSignals). None are derived here. +type Signals struct { + // UsableNonFraudCardCount is the number of active (not soft-deleted), + // not-fraud-flagged, not-expired cards on the account. Gate 1 requires >= 1. + UsableNonFraudCardCount int + // FirstCharge is the outcome of the account's earliest real charge. Gate 2 + // blocks only on FirstChargeFailed. + FirstCharge FirstChargeState + // FailedChargeStreak is the account's consecutive failed-charge counter + // (resets to 0 on a successful charge). Gate 3 blocks at >= MaxFailedChargeStreak. + FailedChargeStreak int +} + +// Reason is a stable, machine-readable code for a Verdict — the primary block +// cause (or ReasonEligible), for the wire contract, the UI, and test assertions. +type Reason string + +const ( + // ReasonEligible: not blocked — all gates pass. + ReasonEligible Reason = "ELIGIBLE" + // ReasonNoUsableCard: gate 1 failed — zero non-fraud usable cards on file. + ReasonNoUsableCard Reason = "NO_USABLE_CARD" + // ReasonFirstChargeFailed: gate 2 failed — the account's first charge went + // uncollectible. + ReasonFirstChargeFailed Reason = "FIRST_CHARGE_FAILED" + // ReasonTooManyFailures: gate 3 failed — the consecutive failed-charge streak + // reached MaxFailedChargeStreak. + ReasonTooManyFailures Reason = "TOO_MANY_FAILURES" +) + +// Verdict is the gate's decision. Blocked is the single field a caller must read +// to gate service; Reason is the primary cause (ReasonEligible when !Blocked); +// Reasons lists EVERY failing gate (empty when eligible) so a UI can tell a +// customer everything they must fix, not just the first thing. +type Verdict struct { + Blocked bool + Reason Reason + Reasons []Reason +} + +// Evaluate applies the three gates in a fixed priority order and returns the +// Verdict. The gates, and the exact boolean each checks: +// +// gate 1 (card): UsableNonFraudCardCount >= 1 +// gate 2 (first charge): FirstCharge != FirstChargeFailed +// gate 3 (failures): FailedChargeStreak < MaxFailedChargeStreak +// Blocked = NOT (gate1 AND gate2 AND gate3) +// +// Priority (card → first charge → failures) fixes which cause becomes the +// primary Reason when more than one gate fails; Reasons collects them all in the +// same order. The order is purely presentational — Blocked is the AND of all +// three regardless. +// +// Grace edges (why gate 2 is "!= failed", not "== succeeded"): a brand-new +// account (FirstChargeNone) and one whose first invoice is still retrying +// (FirstChargePending) both PASS gate 2 — blocking them would deadlock an +// account that must run before it can ever be charged. Only an outright failed +// first charge (uncollectible) blocks. Gate 1 is unconditional, so even a graced +// new account still needs a card. +func Evaluate(s Signals) Verdict { + var reasons []Reason + + if s.UsableNonFraudCardCount < 1 { + reasons = append(reasons, ReasonNoUsableCard) + } + if s.FirstCharge == FirstChargeFailed { + reasons = append(reasons, ReasonFirstChargeFailed) + } + if s.FailedChargeStreak >= MaxFailedChargeStreak { + reasons = append(reasons, ReasonTooManyFailures) + } + + if len(reasons) == 0 { + return Verdict{Blocked: false, Reason: ReasonEligible} + } + return Verdict{Blocked: true, Reason: reasons[0], Reasons: reasons} +} diff --git a/internal/account/eligibility/eligibility_test.go b/internal/account/eligibility/eligibility_test.go new file mode 100644 index 0000000..923af23 --- /dev/null +++ b/internal/account/eligibility/eligibility_test.go @@ -0,0 +1,166 @@ +package eligibility + +import ( + "reflect" + "testing" +) + +// card returns Signals with a good first charge and clean streak, varying only +// the usable-card count — so a case exercises exactly the gate it names. +func card(n int) Signals { + return Signals{UsableNonFraudCardCount: n, FirstCharge: FirstChargeSucceeded, FailedChargeStreak: 0} +} + +func TestEvaluate(t *testing.T) { + cases := []struct { + name string + in Signals + wantBlocked bool + wantReason Reason + wantReasons []Reason + }{ + // --- fully eligible --- + { + name: "one card, first charge paid, no failures", + in: Signals{UsableNonFraudCardCount: 1, FirstCharge: FirstChargeSucceeded, FailedChargeStreak: 0}, + wantReason: ReasonEligible, + }, + { + name: "many cards, streak of one still under the cap", + in: Signals{UsableNonFraudCardCount: 3, FirstCharge: FirstChargeSucceeded, FailedChargeStreak: 1}, + wantReason: ReasonEligible, + }, + + // --- gate 1: card --- + { + name: "zero cards blocks even with a clean charge history", + in: Signals{UsableNonFraudCardCount: 0, FirstCharge: FirstChargeSucceeded, FailedChargeStreak: 0}, + wantBlocked: true, + wantReason: ReasonNoUsableCard, + wantReasons: []Reason{ReasonNoUsableCard}, + }, + { + name: "negative card count is treated as no card (defensive)", + in: Signals{UsableNonFraudCardCount: -1, FirstCharge: FirstChargeSucceeded, FailedChargeStreak: 0}, + wantBlocked: true, + wantReason: ReasonNoUsableCard, + wantReasons: []Reason{ReasonNoUsableCard}, + }, + + // --- gate 2: first charge (grace edges) --- + { + name: "brand-new account, no charge yet, with a card is graced", + in: Signals{UsableNonFraudCardCount: 1, FirstCharge: FirstChargeNone, FailedChargeStreak: 0}, + wantReason: ReasonEligible, + }, + { + name: "first charge still pending (retrying) with a card is graced", + in: Signals{UsableNonFraudCardCount: 1, FirstCharge: FirstChargePending, FailedChargeStreak: 0}, + wantReason: ReasonEligible, + }, + { + name: "new account with NO card is blocked on the card, not graced away", + in: Signals{UsableNonFraudCardCount: 0, FirstCharge: FirstChargeNone, FailedChargeStreak: 0}, + wantBlocked: true, + wantReason: ReasonNoUsableCard, + wantReasons: []Reason{ReasonNoUsableCard}, + }, + { + name: "first charge failed blocks", + in: Signals{UsableNonFraudCardCount: 1, FirstCharge: FirstChargeFailed, FailedChargeStreak: 1}, + wantBlocked: true, + wantReason: ReasonFirstChargeFailed, + wantReasons: []Reason{ReasonFirstChargeFailed}, + }, + + // --- gate 3: failure streak boundary (< 2, so 2 excluded) --- + { + name: "streak of one is allowed (boundary, still under 2)", + in: Signals{UsableNonFraudCardCount: 1, FirstCharge: FirstChargeSucceeded, FailedChargeStreak: 1}, + wantReason: ReasonEligible, + }, + { + name: "streak of exactly two blocks (2 excluded by < 2)", + in: Signals{UsableNonFraudCardCount: 1, FirstCharge: FirstChargeSucceeded, FailedChargeStreak: 2}, + wantBlocked: true, + wantReason: ReasonTooManyFailures, + wantReasons: []Reason{ReasonTooManyFailures}, + }, + { + name: "streak above two blocks", + in: Signals{UsableNonFraudCardCount: 1, FirstCharge: FirstChargeSucceeded, FailedChargeStreak: 5}, + wantBlocked: true, + wantReason: ReasonTooManyFailures, + wantReasons: []Reason{ReasonTooManyFailures}, + }, + + // --- multiple gates fail: priority + all-reasons --- + { + name: "no card AND first charge failed: primary is card, both reported", + in: Signals{UsableNonFraudCardCount: 0, FirstCharge: FirstChargeFailed, FailedChargeStreak: 1}, + wantBlocked: true, + wantReason: ReasonNoUsableCard, + wantReasons: []Reason{ReasonNoUsableCard, ReasonFirstChargeFailed}, + }, + { + name: "all three gates fail: card is primary, all three reported in order", + in: Signals{UsableNonFraudCardCount: 0, FirstCharge: FirstChargeFailed, FailedChargeStreak: 3}, + wantBlocked: true, + wantReason: ReasonNoUsableCard, + wantReasons: []Reason{ReasonNoUsableCard, ReasonFirstChargeFailed, ReasonTooManyFailures}, + }, + { + name: "first charge failed AND too many failures (card ok): first-charge is primary", + in: Signals{UsableNonFraudCardCount: 2, FirstCharge: FirstChargeFailed, FailedChargeStreak: 2}, + wantBlocked: true, + wantReason: ReasonFirstChargeFailed, + wantReasons: []Reason{ReasonFirstChargeFailed, ReasonTooManyFailures}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := Evaluate(tc.in) + if got.Blocked != tc.wantBlocked { + t.Errorf("Blocked = %v, want %v", got.Blocked, tc.wantBlocked) + } + if got.Reason != tc.wantReason { + t.Errorf("Reason = %q, want %q", got.Reason, tc.wantReason) + } + if !reflect.DeepEqual(got.Reasons, tc.wantReasons) { + t.Errorf("Reasons = %v, want %v", got.Reasons, tc.wantReasons) + } + }) + } +} + +// TestEvaluate_EligibleHasNoReasons pins the contract that an eligible verdict +// carries an empty Reasons slice (a UI iterates it to show "what to fix"). +func TestEvaluate_EligibleHasNoReasons(t *testing.T) { + got := Evaluate(card(1)) + if got.Blocked { + t.Fatalf("expected eligible, got blocked: %+v", got) + } + if len(got.Reasons) != 0 { + t.Errorf("eligible verdict must have no Reasons, got %v", got.Reasons) + } +} + +// TestEvaluate_PrimaryReasonIsFirstOfReasons pins that, whenever blocked, the +// primary Reason is exactly Reasons[0] — the invariant a caller relies on when +// it shows one headline cause but keeps the full list. +func TestEvaluate_PrimaryReasonIsFirstOfReasons(t *testing.T) { + for _, n := range []int{0, 1} { + for _, fc := range []FirstChargeState{FirstChargeNone, FirstChargeSucceeded, FirstChargePending, FirstChargeFailed} { + for _, streak := range []int{0, 1, 2, 4} { + v := Evaluate(Signals{UsableNonFraudCardCount: n, FirstCharge: fc, FailedChargeStreak: streak}) + if v.Blocked && v.Reason != v.Reasons[0] { + t.Errorf("blocked verdict %+v: Reason %q != Reasons[0] %q", v, v.Reason, v.Reasons[0]) + } + if !v.Blocked && v.Reason != ReasonEligible { + t.Errorf("unblocked verdict must read ELIGIBLE, got %q", v.Reason) + } + } + } + } +} diff --git a/internal/account/webhook/handlers.go b/internal/account/webhook/handlers.go index 6dd3e57..fca31eb 100644 --- a/internal/account/webhook/handlers.go +++ b/internal/account/webhook/handlers.go @@ -256,9 +256,28 @@ func (r *Router) handleInvoiceLifecycle(ctx context.Context, event stripego.Even return Result{HTTPStatus: 200, Status: StatusDriftWarning} } - // RELAX driver — invoice.paid only. A failure here is surfaced (500) so Stripe - // retries: the relax UPDATE is idempotent (a no-op once the account is already - // arrears), so a retry after the mirror already advanced is safe. + // SERVICE-BLOCK streak — invoice.payment_failed advances the account's + // consecutive failed-charge streak (migration 040), gated so a single + // invoice's Stripe retries count once (RecordFailedCharge's ever_failed + // latch). Reached only past the found guard above, so a late payment_failed + // against an already-terminal invoice (rejected by the monotonic guard) never + // counts. A failure here is surfaced (500) so Stripe retries; the latch makes + // the retry idempotent (no double increment). + if event.Type == stripego.EventTypeInvoicePaymentFailed { + counted, err := r.store.RecordFailedCharge(ctx, inv.ID) + if err != nil { + r.log.ErrorContext(ctx, "invoice.payment_failed record failed-charge streak failed", "event_id", event.ID, "stripe_invoice_id", inv.ID, "error", err) + return Result{HTTPStatus: 500, Status: StatusInternal} + } + if counted { + r.log.InfoContext(ctx, "invoice.payment_failed advanced failed-charge streak", "event_id", event.ID, "stripe_invoice_id", inv.ID) + } + } + + // RELAX + streak auto-cure — invoice.paid only. A failure here is surfaced + // (500) so Stripe retries: both UPDATEs are idempotent (relax is a no-op once + // arrears; reset is a no-op once the streak is already 0), so a retry after + // the mirror already advanced is safe. if event.Type == stripego.EventTypeInvoicePaid { relaxed, err := r.store.RelaxCollectionOnPaidInvoice(ctx, inv.ID) if err != nil { @@ -268,6 +287,15 @@ func (r *Router) handleInvoiceLifecycle(ctx context.Context, event stripego.Even if relaxed { r.log.InfoContext(ctx, "invoice.paid relaxed account prepaid → arrears", "event_id", event.ID, "stripe_invoice_id", inv.ID) } + + reset, err := r.store.ResetFailedChargeStreak(ctx, inv.ID) + if err != nil { + r.log.ErrorContext(ctx, "invoice.paid reset failed-charge streak failed", "event_id", event.ID, "stripe_invoice_id", inv.ID, "error", err) + return Result{HTTPStatus: 500, Status: StatusInternal} + } + if reset { + r.log.InfoContext(ctx, "invoice.paid cleared failed-charge streak (service-block auto-cure)", "event_id", event.ID, "stripe_invoice_id", inv.ID) + } } return Result{HTTPStatus: 200, Status: StatusOK} } diff --git a/internal/account/webhook/router.go b/internal/account/webhook/router.go index 845e78e..d3c7c2e 100644 --- a/internal/account/webhook/router.go +++ b/internal/account/webhook/router.go @@ -128,6 +128,22 @@ type Store interface { // no-op (the account was not prepaid, is still delinquent, or has no mirror // row), never an error. It NEVER charges — relax and charge are decoupled. RelaxCollectionOnPaidInvoice(ctx context.Context, stripeInvoiceID string) (relaxed bool, err error) + + // RecordFailedCharge maintains the service-block failed-charge streak on + // invoice.payment_failed: it flips the sticky per-invoice ever_failed marker + // (migration 039) and, ONLY on the FIRST failure of a distinct invoice, + // increments accounts.failed_charge_streak (migration 040) — so Stripe's + // per-retry payment_failed events (distinct event ids, past the event-level + // dedup) count as a single failure. Returns (counted bool, error): + // counted=true only when this was the first failure for the invoice and the + // streak advanced; false is a retry no-op or a not-yet-mirrored invoice. + RecordFailedCharge(ctx context.Context, stripeInvoiceID string) (counted bool, err error) + + // ResetFailedChargeStreak clears accounts.failed_charge_streak to 0 on + // invoice.paid — the auto-cure that unblocks an account the streak had + // blocked. Returns (reset bool, error): reset=false (0 rows) is a no-op — the + // streak was already clean or the invoice has no mirror row. + ResetFailedChargeStreak(ctx context.Context, stripeInvoiceID string) (reset bool, err error) } // ApplyInvoiceStatusParams carries the columns ApplyInvoiceStatus reconciles diff --git a/internal/account/webhook/router_test.go b/internal/account/webhook/router_test.go index 45d08f5..8c60975 100644 --- a/internal/account/webhook/router_test.go +++ b/internal/account/webhook/router_test.go @@ -460,6 +460,106 @@ func TestProcess_InvoicePaymentFailed_StaysOpen(t *testing.T) { require.Equal(t, int64(0), s.AppliedInvoices[0].AmountPaidCents) } +// --- service-block failed-charge streak (migrations 039/040) -------------- + +func TestProcess_InvoicePaymentFailed_RecordsFailedCharge(t *testing.T) { + // payment_failed drives the service-block streak: the handler calls + // RecordFailedCharge with the invoice id (the store's ever_failed latch does + // the distinct-invoice dedup). paid is NOT involved, so no streak reset. + v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_pf_streak", "invoice.payment_failed", "in_1", "open", 0, 1200)} + s := webhooktest.NewFakeStore() + s.FailCounted = true + r := newRouter(v, s) + + res := r.Process(context.Background(), []byte(`{}`), "sig") + + require.Equal(t, 200, res.HTTPStatus) + require.Equal(t, webhook.StatusOK, res.Status) + require.Equal(t, []string{"in_1"}, s.FailedInvoices, "payment_failed must record the failed charge") + require.Empty(t, s.ResetInvoices, "payment_failed must not reset the streak") +} + +func TestProcess_InvoicePaid_ResetsFailedChargeStreak(t *testing.T) { + // invoice.paid is the auto-cure: it resets the failed-charge streak (and never + // records a failure). + v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_paid_reset", "invoice.paid", "in_1", "paid", 1200, 1200)} + s := webhooktest.NewFakeStore() + s.StreakReset = true + r := newRouter(v, s) + + res := r.Process(context.Background(), []byte(`{}`), "sig") + + require.Equal(t, 200, res.HTTPStatus) + require.Equal(t, webhook.StatusOK, res.Status) + require.Equal(t, []string{"in_1"}, s.ResetInvoices, "invoice.paid must reset the failed-charge streak") + require.Empty(t, s.FailedInvoices, "invoice.paid must not record a failure") +} + +func TestProcess_NonFailureInvoiceEvent_DoesNotRecordFailure(t *testing.T) { + // Only invoice.payment_failed feeds the streak. Every other invoice.* event + // lands the mirror but never touches RecordFailedCharge. + for _, ev := range []struct{ name, typ, status string }{ + {"created", "invoice.created", "draft"}, + {"finalized", "invoice.finalized", "open"}, + {"paid", "invoice.paid", "paid"}, + {"voided", "invoice.voided", "void"}, + {"uncollectible", "invoice.marked_uncollectible", "uncollectible"}, + } { + t.Run(ev.name, func(t *testing.T) { + v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_nf_"+ev.name, ev.typ, "in_x", ev.status, 0, 1200)} + s := webhooktest.NewFakeStore() + r := newRouter(v, s) + + res := r.Process(context.Background(), []byte(`{}`), "sig") + + require.Equal(t, webhook.StatusOK, res.Status) + require.Empty(t, s.FailedInvoices, "only payment_failed records a failed charge") + }) + } +} + +func TestProcess_InvoicePaymentFailed_DriftMirror_DoesNotRecord(t *testing.T) { + // No mirror row (drift) short-circuits at the found guard, before the streak + // step — a payment_failed for an unmirrored invoice never advances the streak. + v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_pf_drift", "invoice.payment_failed", "in_orphan", "open", 0, 1200)} + s := webhooktest.NewFakeStore() + s.InvoiceFound = false + r := newRouter(v, s) + + res := r.Process(context.Background(), []byte(`{}`), "sig") + + require.Equal(t, webhook.StatusDriftWarning, res.Status) + require.Empty(t, s.FailedInvoices, "drift short-circuits before the streak step") +} + +func TestProcess_InvoicePaymentFailed_RecordError_Internal(t *testing.T) { + // A streak-store error surfaces as 500 so Stripe retries; the ever_failed + // latch makes the retry idempotent (no double count). + v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_pf_err", "invoice.payment_failed", "in_1", "open", 0, 1200)} + s := webhooktest.NewFakeStore() + s.ErrRecordFailed = errors.New("db down") + r := newRouter(v, s) + + res := r.Process(context.Background(), []byte(`{}`), "sig") + + require.Equal(t, 500, res.HTTPStatus) + require.Equal(t, webhook.StatusInternal, res.Status) +} + +func TestProcess_InvoicePaid_ResetError_Internal(t *testing.T) { + // A streak-reset store error on invoice.paid surfaces as 500 so Stripe + // retries (the reset is idempotent once the streak is 0). + v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_paid_rst_err", "invoice.paid", "in_1", "paid", 1200, 1200)} + s := webhooktest.NewFakeStore() + s.ErrResetStreak = errors.New("db down") + r := newRouter(v, s) + + res := r.Process(context.Background(), []byte(`{}`), "sig") + + require.Equal(t, 500, res.HTTPStatus) + require.Equal(t, webhook.StatusInternal, res.Status) +} + func TestProcess_InvoiceVoided_AppliesVoid(t *testing.T) { // An admin/Stripe void (invoice.voided, status 'void') MUST reach the // reconciler so the mirror leaves 'open' and the delinquency signal clears. diff --git a/internal/account/webhook/store.go b/internal/account/webhook/store.go index fc76cee..c01028f 100644 --- a/internal/account/webhook/store.go +++ b/internal/account/webhook/store.go @@ -295,6 +295,57 @@ func (s *pgxStore) RelaxCollectionOnPaidInvoice(ctx context.Context, stripeInvoi return rows > 0, nil } +// RecordFailedCharge flips the sticky ever_failed marker on the invoice and, on +// the first failure of a distinct invoice ONLY, advances the account's +// consecutive failed-charge streak (see the query docs). The flip GATES the +// increment: MarkInvoiceEverFailed returns rows=1 exactly once per invoice (a +// one-way latch), so Stripe's per-retry payment_failed events never double-count. +// +// The flip and the increment MUST be atomic. They run in ONE transaction so the +// latch is never visible unless the increment also commits — otherwise a crash +// or transient error between two autocommits would leave ever_failed=true with +// the streak un-advanced, and the latch would then permanently suppress the +// increment on every retry/redelivery (event-id dedup short-circuits the same +// event; a later distinct event sees ever_failed already true). That would +// silently UNDER-count the streak and let a should-be-blocked account read +// eligible. On any error the tx rolls back, so the handler's 500 → Stripe +// redelivery re-flips and re-increments correctly. Returns (counted, error) +// where counted=true means the streak advanced. +func (s *pgxStore) RecordFailedCharge(ctx context.Context, stripeInvoiceID string) (bool, error) { + tx, err := s.pool.Begin(ctx) + if err != nil { + return false, err + } + defer func() { _ = tx.Rollback(ctx) }() + qtx := s.q.WithTx(tx) + + flipped, err := qtx.MarkInvoiceEverFailed(ctx, stripeInvoiceID) + if err != nil { + return false, err + } + if flipped == 0 { + // Already-failed invoice (a retry) or no mirror row yet — the streak + // counts distinct failed invoices, not events, so do not increment. + // Commit the (empty) tx so the caller sees a clean no-op. + return false, tx.Commit(ctx) + } + if _, err := qtx.IncrementFailedChargeStreak(ctx, stripeInvoiceID); err != nil { + return false, err + } + return true, tx.Commit(ctx) +} + +// ResetFailedChargeStreak clears the account's failed-charge streak to 0 on a +// paid invoice (auto-cure). Returns (reset, error): reset=false (0 rows) is a +// no-op — the streak was already clean or the invoice has no mirror row. +func (s *pgxStore) ResetFailedChargeStreak(ctx context.Context, stripeInvoiceID string) (bool, error) { + rows, err := s.q.ResetFailedChargeStreak(ctx, stripeInvoiceID) + if err != nil { + return false, err + } + return rows > 0, nil +} + // text wraps a non-null Go string in the pgtype.Text the generated // queries expect for nullable TEXT columns. func text(s string) pgtype.Text { diff --git a/internal/account/webhook/store_integration_test.go b/internal/account/webhook/store_integration_test.go index 2f3813c..4d57d8f 100644 --- a/internal/account/webhook/store_integration_test.go +++ b/internal/account/webhook/store_integration_test.go @@ -715,3 +715,103 @@ func readMode(t *testing.T, pool *pgxpool.Pool, accountID uuid.UUID) string { accountID.String()).Scan(&mode)) return mode } + +// --- service-block failed-charge streak (migrations 039/040) -------------- + +func readFailedStreak(t *testing.T, pool *pgxpool.Pool, accountID uuid.UUID) int { + t.Helper() + var streak int + require.NoError(t, pool.QueryRow(context.Background(), + `SELECT failed_charge_streak FROM ms_billing.accounts WHERE id = $1`, + accountID.String()).Scan(&streak)) + return streak +} + +func TestPgxStore_RecordFailedCharge_StreakLifecycle(t *testing.T) { + // The full streak lifecycle over the REAL store (exercises the atomic + // latch+increment tx): first failure counts once, a retry of the SAME + // invoice does not re-count, a second DISTINCT invoice advances to the + // block threshold, and a paid invoice resets to 0 (auto-cure). + pool := testutil.NewTestDB(t) + store := webhook.NewStore(pool) + accountID := seedAccount(t, pool, "cus_streak") + ctx := context.Background() + + seedInvoice(t, pool, accountID, "in_A", "open", 0, 1200) + counted, err := store.RecordFailedCharge(ctx, "in_A") + require.NoError(t, err) + require.True(t, counted, "first failure of a distinct invoice counts") + require.Equal(t, 1, readFailedStreak(t, pool, accountID)) + + // Retry of the SAME invoice (a distinct Stripe event) — ever_failed latch + // suppresses a second increment. + counted, err = store.RecordFailedCharge(ctx, "in_A") + require.NoError(t, err) + require.False(t, counted, "a retry of the same invoice must not re-count") + require.Equal(t, 1, readFailedStreak(t, pool, accountID)) + + // A second DISTINCT failed invoice advances the streak to the block threshold. + seedInvoice(t, pool, accountID, "in_B", "open", 0, 1200) + counted, err = store.RecordFailedCharge(ctx, "in_B") + require.NoError(t, err) + require.True(t, counted) + require.Equal(t, 2, readFailedStreak(t, pool, accountID)) + + // A successful charge clears the streak (auto-cure), and a repeat reset is + // an idempotent no-op. + reset, err := store.ResetFailedChargeStreak(ctx, "in_B") + require.NoError(t, err) + require.True(t, reset) + require.Equal(t, 0, readFailedStreak(t, pool, accountID)) + + reset, err = store.ResetFailedChargeStreak(ctx, "in_B") + require.NoError(t, err) + require.False(t, reset, "reset is idempotent once the streak is 0") +} + +func TestPgxStore_RecordFailedCharge_NoMirrorRow_IsNoOp(t *testing.T) { + // A payment_failed for an un-mirrored invoice must not advance any streak + // (the latch flips zero rows) and must not error. + pool := testutil.NewTestDB(t) + store := webhook.NewStore(pool) + ctx := context.Background() + + counted, err := store.RecordFailedCharge(ctx, "in_orphan") + require.NoError(t, err) + require.False(t, counted) +} + +func TestPgxStore_ServiceBlockSignals_ReflectsCardStreakAndFirstCharge(t *testing.T) { + // The billing store's one-shot signal read reflects the usable non-fraud + // card count, the failed-charge streak, and the earliest real charge. + pool := testutil.NewTestDB(t) + bstore := billing.NewStore(pool) + accountID := seedAccount(t, pool, "cus_sig") + ctx := context.Background() + + // Brand new: no card, no charge, clean streak. + sig, err := bstore.ServiceBlockSignals(ctx, accountID) + require.NoError(t, err) + require.Equal(t, 0, sig.UsableCardCount) + require.Equal(t, 0, sig.FailedChargeStreak) + require.Equal(t, "", sig.FirstChargeStatus) + + // A usable non-fraud card, a fraud-blocked card (excluded), and an + // uncollectible earliest invoice. + _, err = pool.Exec(ctx, + `INSERT INTO ms_billing.payment_methods_mirror + (account_id, stripe_payment_method_id, brand, last4, exp_month, exp_year) + VALUES ($1, 'pm_good', 'visa', '4242', 12, 2999)`, accountID) + require.NoError(t, err) + _, err = pool.Exec(ctx, + `INSERT INTO ms_billing.payment_methods_mirror + (account_id, stripe_payment_method_id, brand, last4, exp_month, exp_year, fraud_blocked) + VALUES ($1, 'pm_fraud', 'visa', '0002', 12, 2999, true)`, accountID) + require.NoError(t, err) + seedInvoice(t, pool, accountID, "in_first", "uncollectible", 0, 1200) + + sig, err = bstore.ServiceBlockSignals(ctx, accountID) + require.NoError(t, err) + require.Equal(t, 1, sig.UsableCardCount, "fraud-blocked card is excluded from the usable count") + require.Equal(t, "uncollectible", sig.FirstChargeStatus) +} diff --git a/internal/account/webhook/webhooktest/fakes.go b/internal/account/webhook/webhooktest/fakes.go index 7d469f7..95e7b09 100644 --- a/internal/account/webhook/webhooktest/fakes.go +++ b/internal/account/webhook/webhooktest/fakes.go @@ -49,6 +49,8 @@ type FakeStore struct { AppliedInvoices []webhook.ApplyInvoiceStatusParams // captured calls to ApplyInvoiceStatus RelaxedInvoices []string // stripe_invoice_id values from RelaxCollectionOnPaidInvoice + FailedInvoices []string // stripe_invoice_id values from RecordFailedCharge + ResetInvoices []string // stripe_invoice_id values from ResetFailedChargeStreak // Found-flag knobs TouchedFound bool // returned by TouchAccountByStripeCustomer @@ -56,6 +58,8 @@ type FakeStore struct { SoftDelFound bool // returned by SoftDeletePaymentMethod InvoiceFound bool // returned by ApplyInvoiceStatus Relaxed bool // returned by RelaxCollectionOnPaidInvoice + FailCounted bool // returned by RecordFailedCharge (counted) + StreakReset bool // returned by ResetFailedChargeStreak (reset) ActivatedNew bool // returned by StampAccountActivated (firstBind) // Error injection @@ -68,6 +72,8 @@ type FakeStore struct { ErrResolve error // from ResolvePendingAddCardRequest ErrApplyInvoice error // from ApplyInvoiceStatus ErrRelax error // from RelaxCollectionOnPaidInvoice + ErrRecordFailed error // from RecordFailedCharge + ErrResetStreak error // from ResetFailedChargeStreak ErrActivate error // from StampAccountActivated } @@ -170,6 +176,22 @@ func (s *FakeStore) RelaxCollectionOnPaidInvoice(_ context.Context, stripeInvoic return s.Relaxed, nil } +func (s *FakeStore) RecordFailedCharge(_ context.Context, stripeInvoiceID string) (bool, error) { + if s.ErrRecordFailed != nil { + return false, s.ErrRecordFailed + } + s.FailedInvoices = append(s.FailedInvoices, stripeInvoiceID) + return s.FailCounted, nil +} + +func (s *FakeStore) ResetFailedChargeStreak(_ context.Context, stripeInvoiceID string) (bool, error) { + if s.ErrResetStreak != nil { + return false, s.ErrResetStreak + } + s.ResetInvoices = append(s.ResetInvoices, stripeInvoiceID) + return s.StreakReset, nil +} + // SilentLogger returns a slog.Logger that discards all output. Useful // for tests that don't make logging assertions. func SilentLogger() *slog.Logger { diff --git a/migrations/billing/038_payment_method_fraud.down.sql b/migrations/billing/038_payment_method_fraud.down.sql new file mode 100644 index 0000000..748916d --- /dev/null +++ b/migrations/billing/038_payment_method_fraud.down.sql @@ -0,0 +1,5 @@ +-- 038 down: drop the fraud-flag columns from the payment-method mirror. +ALTER TABLE ms_billing.payment_methods_mirror + DROP COLUMN IF EXISTS fraud_blocked, + DROP COLUMN IF EXISTS fraud_reason, + DROP COLUMN IF EXISTS fraud_flagged_at; diff --git a/migrations/billing/038_payment_method_fraud.up.sql b/migrations/billing/038_payment_method_fraud.up.sql new file mode 100644 index 0000000..0ba7ee2 --- /dev/null +++ b/migrations/billing/038_payment_method_fraud.up.sql @@ -0,0 +1,42 @@ +-- Migration 038 — fraud flag on the payment-method mirror (service-block gate). +-- +-- The service-block eligibility gate (internal/account/eligibility) requires +-- "at least one NON-FRAUD card on file". payment_methods_mirror (002/005) held +-- no risk state at all — only brand/last4/exp + is_default + deleted_at — so a +-- card that Stripe later flags (an early-fraud warning or a dispute) could not +-- be excluded from the usable-card count. These columns add that risk state. +-- +-- fraud_blocked is the authoritative "exclude this card" flag the gate reads; +-- fraud_reason / fraud_flagged_at are the audit trail (which signal, when). +-- All default to the non-fraud state, so EVERY existing row is treated as a +-- good card the moment this lands — the gate is fully functional on the card +-- count immediately. The webhook that SETS fraud_blocked=true +-- (radar.early_fraud_warning.created + charge.dispute.created) is a follow-up +-- PR: shipping the column now with no writer just means every card stays +-- non-fraud until that path lands, which is the safe interim (never a false +-- block). +-- +-- fraud_blocked is a ONE-WAY latch in practice (a flagged card is not +-- auto-cleared) — a disputed/fraudulent card is detached + re-added, not +-- un-flagged in place. No index: cards-per-account is tiny and the count query +-- already rides the pmm_account_active_idx (002) partial index on account_id. +-- +-- Born clean at slot 038 (next free after 037). sqlc picks up the columns from +-- migrations/billing/ automatically. + +ALTER TABLE ms_billing.payment_methods_mirror + ADD COLUMN fraud_blocked BOOLEAN NOT NULL DEFAULT false, + ADD COLUMN fraud_reason TEXT NULL, + ADD COLUMN fraud_flagged_at TIMESTAMPTZ NULL; + +COMMENT ON COLUMN ms_billing.payment_methods_mirror.fraud_blocked IS + 'True once Stripe flags this card as fraud/dispute risk (set by the ' + 'radar.early_fraud_warning / charge.dispute webhook — follow-up PR). The ' + 'service-block gate EXCLUDES fraud_blocked cards from the usable-card count. ' + 'Defaults false so every existing card is treated non-fraud.'; + +COMMENT ON COLUMN ms_billing.payment_methods_mirror.fraud_reason IS + 'Audit: the signal that set fraud_blocked (e.g. early_fraud_warning, dispute). NULL until flagged.'; + +COMMENT ON COLUMN ms_billing.payment_methods_mirror.fraud_flagged_at IS + 'Audit: when fraud_blocked was set. NULL until flagged.'; diff --git a/migrations/billing/039_invoice_ever_failed.down.sql b/migrations/billing/039_invoice_ever_failed.down.sql new file mode 100644 index 0000000..b6d9aff --- /dev/null +++ b/migrations/billing/039_invoice_ever_failed.down.sql @@ -0,0 +1,3 @@ +-- 039 down: drop the sticky per-invoice failure marker. +ALTER TABLE ms_billing.invoices + DROP COLUMN IF EXISTS ever_failed; diff --git a/migrations/billing/039_invoice_ever_failed.up.sql b/migrations/billing/039_invoice_ever_failed.up.sql new file mode 100644 index 0000000..8d3c347 --- /dev/null +++ b/migrations/billing/039_invoice_ever_failed.up.sql @@ -0,0 +1,31 @@ +-- Migration 039 — sticky per-invoice "ever failed" marker (service-block gate). +-- +-- The service-block gate counts an account's CONSECUTIVE failed-charge streak +-- (accounts.failed_charge_streak, migration 040): +1 the first time an invoice +-- fails, reset to 0 on the next successful charge, block at >= 2. Stripe smart- +-- retries a failed invoice and fires invoice.payment_failed on EACH attempt +-- (distinct event ids, so the webhook's event-level dedup does NOT collapse +-- them). Counting every payment_failed event would over-count a single +-- invoice's retries. +-- +-- ever_failed is the per-invoice guard that makes the streak count DISTINCT +-- invoices: the webhook flips it false->true on the first payment_failed for a +-- row (an UPDATE ... WHERE NOT ever_failed, so RowsAffected=1 only once) and +-- increments the account streak ONLY on that flip. It is never cleared — it is +-- a historical "this invoice failed at least once" fact, independent of the +-- invoice's later status (a failed-then-paid invoice keeps ever_failed=true +-- while the account streak resets). Distinct from the derived delinquency +-- signal (AccountHasUnpaidInvoice, which reflects CURRENT status only). +-- +-- Every historic row defaults false; only a NEW payment_failed sets it. Born +-- clean at slot 039 (next free after 038). sqlc picks up the column +-- automatically. updated_at is trigger-maintained (001). + +ALTER TABLE ms_billing.invoices + ADD COLUMN ever_failed BOOLEAN NOT NULL DEFAULT false; + +COMMENT ON COLUMN ms_billing.invoices.ever_failed IS + 'Sticky: set true on the FIRST invoice.payment_failed for this row and never ' + 'cleared. The service-block gate uses it to count DISTINCT failed invoices ' + '(not per-retry events) into accounts.failed_charge_streak. Independent of ' + 'the current status — survives a later flip to paid.'; diff --git a/migrations/billing/040_account_failed_charge_streak.down.sql b/migrations/billing/040_account_failed_charge_streak.down.sql new file mode 100644 index 0000000..da3117b --- /dev/null +++ b/migrations/billing/040_account_failed_charge_streak.down.sql @@ -0,0 +1,3 @@ +-- 040 down: drop the consecutive failed-charge streak column. +ALTER TABLE ms_billing.accounts + DROP COLUMN IF EXISTS failed_charge_streak; diff --git a/migrations/billing/040_account_failed_charge_streak.up.sql b/migrations/billing/040_account_failed_charge_streak.up.sql new file mode 100644 index 0000000..b80da89 --- /dev/null +++ b/migrations/billing/040_account_failed_charge_streak.up.sql @@ -0,0 +1,30 @@ +-- Migration 040 — consecutive failed-charge streak on the billing account +-- (service-block gate). +-- +-- The service-block gate (internal/account/eligibility) blocks an account whose +-- failed-charge count reaches 2 ("< 2 failed" — 2 excluded). The streak is +-- RECOVERABLE, not a lifetime tally: it counts CONSECUTIVE distinct failed +-- invoices and RESETS to 0 on the next successful charge, so an account self- +-- heals the moment it pays (product decision: auto-cure on next success). +-- +-- Maintained entirely by the invoice.* webhook: +-- invoice.payment_failed -> +1, but only on the first failure of a distinct +-- invoice (gated by invoices.ever_failed, migration +-- 039, so Stripe's per-retry events don't over-count). +-- invoice.paid -> reset to 0. +-- The gate READS this column directly (no aggregation) alongside the usable-card +-- count and the first-charge outcome. +-- +-- Defaults 0 for every existing account (a clean streak). Born clean at slot 040 +-- (next free after 039). updated_at is trigger-maintained (001); sqlc picks up +-- the column automatically. + +ALTER TABLE ms_billing.accounts + ADD COLUMN failed_charge_streak INT NOT NULL DEFAULT 0 + CHECK (failed_charge_streak >= 0); + +COMMENT ON COLUMN ms_billing.accounts.failed_charge_streak IS + 'Consecutive distinct failed invoices, maintained by the invoice webhook ' + '(+1 on the first payment_failed of a row via invoices.ever_failed, reset to ' + '0 on invoice.paid). The service-block gate blocks at >= 2. Recoverable — ' + 'self-heals to 0 on the next successful charge.'; diff --git a/scripts/init-db.sql b/scripts/init-db.sql index f4ebd08..b1fe58a 100644 --- a/scripts/init-db.sql +++ b/scripts/init-db.sql @@ -54,3 +54,9 @@ \i migrations/billing/035_billing_run_frozen_charge.up.sql \i migrations/billing/036_charge_attempt_markers.up.sql \i migrations/billing/037_apps_name.up.sql + +-- 038–040: service-block eligibility gate — card fraud flag, sticky per-invoice +-- failure marker, and the account's consecutive failed-charge streak. +\i migrations/billing/038_payment_method_fraud.up.sql +\i migrations/billing/039_invoice_ever_failed.up.sql +\i migrations/billing/040_account_failed_charge_streak.up.sql From 0bd74559d31d4b71027d46c720b5e4c601956477 Mon Sep 17 00:00:00 2001 From: Sheng Kun Chang Date: Tue, 7 Jul 2026 04:33:27 +0800 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20harden=20service-block=20streak=20+?= =?UTF-8?q?=20cure=20uncollectible=E2=86=92paid=20(review)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial re-review (Fable5 find / Opus4.8 verify) surfaced two real defects plus an ordering fragility in the failed-charge streak. Fixes: M1 (was HIGH) — ApplyInvoiceStatus ranked paid/void/uncollectible all at 2, so an uncollectible→paid transition (customer settles on the hosted invoice page) was rejected by the monotonic guard, and the found-guard then skipped the invoice.paid side-effects. A paying customer stayed delinquent AND service-blocked forever. Fix: rank paid(3) above void/uncollectible(2) so a genuine payment lands (relax + block cure run) while void/uncollectible still can't overwrite a paid row. Stripe never emits those after a real payment, so terminal-once-paid holds. S1–S3 consolidation — the streak was a stored counter (accounts.failed_charge_ streak, migration 040) mutated by two webhook UPDATEs in Stripe DELIVERY order, which could over/under-count under out-of-order or lost-then-redelivered events (and the counter needed a tx to stay atomic). Replaced it with a READ-TIME DERIVATION in ServiceBlockSignals: count (ever_failed OR uncollectible) invoices created after the account's most-recent paid invoice. This is delivery-order immune, makes "reset on next success" fall out of the created_at cutoff, and removes the counter, the two increment/reset queries, the pgx tx, and migration 040 entirely. invoice.paid needs no streak write. The webhook now only latches ever_failed (set-only) on payment_failed / marked_uncollectible, BEFORE the found-guard so an out-of-order terminal still marks the invoice (S3). Net −96 lines. Integration tests cover uncollectible→paid cure, the order-immune derivation, and the failure latch. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/account/db/billing.sql.go | 29 +++- internal/account/db/models.go | 4 +- internal/account/db/queries/billing.sql | 29 +++- internal/account/db/queries/webhook.sql | 73 ++++------ internal/account/db/webhook.sql.go | 95 +++++-------- internal/account/eligibility/eligibility.go | 9 +- internal/account/webhook/handlers.go | 51 +++---- internal/account/webhook/router.go | 24 ++-- internal/account/webhook/router_test.go | 101 ++++++-------- internal/account/webhook/store.go | 63 ++------- .../account/webhook/store_integration_test.go | 131 +++++++++++------- internal/account/webhook/webhooktest/fakes.go | 24 +--- .../billing/039_invoice_ever_failed.up.sql | 42 +++--- .../040_account_failed_charge_streak.down.sql | 3 - .../040_account_failed_charge_streak.up.sql | 30 ---- scripts/init-db.sql | 6 +- 16 files changed, 309 insertions(+), 405 deletions(-) delete mode 100644 migrations/billing/040_account_failed_charge_streak.down.sql delete mode 100644 migrations/billing/040_account_failed_charge_streak.up.sql diff --git a/internal/account/db/billing.sql.go b/internal/account/db/billing.sql.go index b3c5287..aabd0e4 100644 --- a/internal/account/db/billing.sql.go +++ b/internal/account/db/billing.sql.go @@ -208,7 +208,16 @@ SELECT EXTRACT(YEAR FROM current_date)::INT, EXTRACT(MONTH FROM current_date)::INT ))::int AS usable_card_count, - a.failed_charge_streak, + (SELECT COUNT(*) + FROM ms_billing.invoices f + WHERE f.account_id = a.id + AND (f.ever_failed OR f.status = 'uncollectible') + AND f.created_at > COALESCE( + (SELECT MAX(p.created_at) + FROM ms_billing.invoices p + WHERE p.account_id = a.id AND p.status = 'paid'), + '-infinity'::timestamptz + ))::int AS failed_charge_streak, COALESCE(( SELECT inv.status FROM ms_billing.invoices inv @@ -235,8 +244,22 @@ type ServiceBlockSignalsRow struct { // (NOT fraud_blocked, migration 038), NOT-expired cards. // Reuses HasUsablePaymentMethod's expiry predicate, // COUNT instead of EXISTS. The gate blocks at 0. -// failed_charge_streak — the account's consecutive failed-charge counter -// (migration 040), read verbatim. The gate blocks at >= 2. +// failed_charge_streak — the account's consecutive failed-charge count, +// DERIVED at read time from the timestamped invoice +// mirror: the number of distinct FAILED invoices +// (ever_failed, migration 039, OR currently +// 'uncollectible') created AFTER the account's most- +// recent PAID invoice. The gate blocks at >= 2. +// Deriving it (vs a stored counter mutated by two +// out-of-transaction webhook UPDATEs in Stripe DELIVERY +// order) makes it immune to at-least-once + out-of-order +// delivery: reorder the paid/failed events however you +// like, the count over the immutable (status, ever_failed, +// created_at) facts is the same. "Reset on the next +// successful charge" falls out for free — a newer paid +// invoice moves the cutoff forward, excluding older +// failures. A failed-then-paid invoice is excluded (its +// created_at is not AFTER its own paid instant). // first_charge_status — the status of the account's EARLIEST real charge: // the oldest invoice that is not 'draft' (never // finalized) or 'void' (cancelled, never a real charge diff --git a/internal/account/db/models.go b/internal/account/db/models.go index 4b15d01..04cf7c8 100644 --- a/internal/account/db/models.go +++ b/internal/account/db/models.go @@ -333,8 +333,6 @@ type MsBillingAccount struct { ActivatedAt pgtype.Timestamptz `json:"activated_at"` // Per-account size threshold (micro-USD) above which a SUCCESSFUL off-session charge is disclosed as "large" on the billing page. NULL = platform default ($100 = 100000000 micros). Resolved at charge time; pure disclosure, changes no charging behaviour. AutoCollectThresholdMicros pgtype.Int8 `json:"auto_collect_threshold_micros"` - // Consecutive distinct failed invoices, maintained by the invoice webhook (+1 on the first payment_failed of a row via invoices.ever_failed, reset to 0 on invoice.paid). The service-block gate blocks at >= 2. Recoverable — self-heals to 0 on the next successful charge. - FailedChargeStreak int32 `json:"failed_charge_streak"` } type MsBillingAddCardRequest struct { @@ -471,7 +469,7 @@ type MsBillingInvoice struct { InvoicePdf pgtype.Text `json:"invoice_pdf"` // Server-computed at invoice-create time: true iff the charged amount (netted arrears + advance base, micros) exceeded the account auto_collect_threshold_micros (or the default when NULL) that applied WHEN THE CHARGE FIRED. Post-hoc disclosure only. IsLargeAutoCollect bool `json:"is_large_auto_collect"` - // Sticky: set true on the FIRST invoice.payment_failed for this row and never cleared. The service-block gate uses it to count DISTINCT failed invoices (not per-retry events) into accounts.failed_charge_streak. Independent of the current status — survives a later flip to paid. + // Sticky, set-only: true once this invoice failed a payment (payment_failed / marked_uncollectible), never cleared. Lets the service-block gate DERIVE the failed-charge streak at read time — counting (ever_failed OR uncollectible) invoices created after the last paid one — so it survives a later flip to paid and is immune to webhook delivery order. EverFailed bool `json:"ever_failed"` } diff --git a/internal/account/db/queries/billing.sql b/internal/account/db/queries/billing.sql index 7bfbc30..4332586 100644 --- a/internal/account/db/queries/billing.sql +++ b/internal/account/db/queries/billing.sql @@ -89,8 +89,22 @@ SELECT EXISTS ( -- (NOT fraud_blocked, migration 038), NOT-expired cards. -- Reuses HasUsablePaymentMethod's expiry predicate, -- COUNT instead of EXISTS. The gate blocks at 0. --- failed_charge_streak — the account's consecutive failed-charge counter --- (migration 040), read verbatim. The gate blocks at >= 2. +-- failed_charge_streak — the account's consecutive failed-charge count, +-- DERIVED at read time from the timestamped invoice +-- mirror: the number of distinct FAILED invoices +-- (ever_failed, migration 039, OR currently +-- 'uncollectible') created AFTER the account's most- +-- recent PAID invoice. The gate blocks at >= 2. +-- Deriving it (vs a stored counter mutated by two +-- out-of-transaction webhook UPDATEs in Stripe DELIVERY +-- order) makes it immune to at-least-once + out-of-order +-- delivery: reorder the paid/failed events however you +-- like, the count over the immutable (status, ever_failed, +-- created_at) facts is the same. "Reset on the next +-- successful charge" falls out for free — a newer paid +-- invoice moves the cutoff forward, excluding older +-- failures. A failed-then-paid invoice is excluded (its +-- created_at is not AFTER its own paid instant). -- first_charge_status — the status of the account's EARLIEST real charge: -- the oldest invoice that is not 'draft' (never -- finalized) or 'void' (cancelled, never a real charge @@ -115,7 +129,16 @@ SELECT EXTRACT(YEAR FROM current_date)::INT, EXTRACT(MONTH FROM current_date)::INT ))::int AS usable_card_count, - a.failed_charge_streak, + (SELECT COUNT(*) + FROM ms_billing.invoices f + WHERE f.account_id = a.id + AND (f.ever_failed OR f.status = 'uncollectible') + AND f.created_at > COALESCE( + (SELECT MAX(p.created_at) + FROM ms_billing.invoices p + WHERE p.account_id = a.id AND p.status = 'paid'), + '-infinity'::timestamptz + ))::int AS failed_charge_streak, COALESCE(( SELECT inv.status FROM ms_billing.invoices inv diff --git a/internal/account/db/queries/webhook.sql b/internal/account/db/queries/webhook.sql index 838a217..80e6b07 100644 --- a/internal/account/db/queries/webhook.sql +++ b/internal/account/db/queries/webhook.sql @@ -152,16 +152,24 @@ WHERE stripe_pm_id = $1 AND status = 'pending'; -- Out-of-order + at-least-once safety: Stripe delivers webhooks at-least-once -- and NOT in guaranteed order, so a late invoice.finalized (open) can arrive -- after invoice.paid (paid). The WHERE guard enforces a MONOTONIC status --- transition via a rank ladder — draft(0) < open(1) < paid/void/uncollectible(2, --- terminal) — so the row's status can only move forward. The new status's rank +-- transition via a rank ladder — draft(0) < open(1) < void/uncollectible(2) < +-- paid(3) — so the row's status can only move forward. The new status's rank -- must be strictly greater than the current rank (a forward transition), with -- one exception: an identical re-apply of the SAME status is allowed through so --- a replayed paid can refresh amount_paid/amount_due idempotently. A terminal --- status (rank 2) is never overwritten by a different status because no rank --- exceeds 2 and the equal-rank branch requires status equality, so paid→void --- (both rank 2) is a no-op — terminal-once-set holds. The CASE ladder is inline --- (not a stored ENUM) so a new Stripe status maps to the bottom rung (-1) and --- can never silently regress a known terminal state. +-- a replayed paid can refresh amount_paid/amount_due idempotently. +-- +-- paid outranks void/uncollectible (rank 3 vs 2) DELIBERATELY: paying an +-- invoice that Stripe already marked uncollectible (the customer settles it on +-- the hosted page) transitions it uncollectible→paid, which MUST land so the +-- delinquency signal clears and the collection relax + service-block cure run +-- off the invoice.paid handler. Ranking paid at 2 (its old value) rejected that +-- transition (2 > 2 false) and trapped a paying customer in delinquent/blocked +-- state forever. void/uncollectible stay at 2 and so still cannot overwrite a +-- paid(3) row (2 > 3 false; the equal-rank branch requires status equality), and +-- Stripe never emits void/uncollectible AFTER a real payment — so terminal-once- +-- paid still holds while the genuine recovery is no longer blocked. The CASE +-- ladder is inline (not a stored ENUM) so a new Stripe status maps to the bottom +-- rung (-1) and can never silently regress a known terminal state. -- -- :execrows → >0 means the row existed AND the guard let the update through; 0 -- means either no mirror row (drift: the charge spine's UpsertInvoice hasn't @@ -181,12 +189,12 @@ WHERE i.stripe_invoice_id = @stripe_invoice_id -- forward transition: incoming rank strictly above the stored rank … (CASE @status::text WHEN 'draft' THEN 0 WHEN 'open' THEN 1 - WHEN 'paid' THEN 2 WHEN 'void' THEN 2 WHEN 'uncollectible' THEN 2 + WHEN 'void' THEN 2 WHEN 'uncollectible' THEN 2 WHEN 'paid' THEN 3 ELSE -1 END) > (CASE i.status WHEN 'draft' THEN 0 WHEN 'open' THEN 1 - WHEN 'paid' THEN 2 WHEN 'void' THEN 2 WHEN 'uncollectible' THEN 2 + WHEN 'void' THEN 2 WHEN 'uncollectible' THEN 2 WHEN 'paid' THEN 3 ELSE -1 END) -- … OR an identical re-apply (idempotent amount refresh on replay). OR i.status = @status @@ -224,41 +232,18 @@ WHERE a.id = ( AND i.status IN ('open', 'uncollectible') ); --- MarkInvoiceEverFailed sets the sticky ever_failed flag (migration 039) on the --- first invoice.payment_failed for a row. The `AND NOT ever_failed` guard makes --- it a one-way latch: :execrows returns 1 ONLY on the first flip, 0 on Stripe's --- subsequent retry events (distinct event ids, so the webhook's event-level --- dedup does not collapse them). The webhook increments the account streak ONLY --- when this returns 1, so a single invoice's retries count as one failure. --- name: MarkInvoiceEverFailed :execrows +-- MarkInvoiceFailed sets the sticky ever_failed flag (migration 039) on an +-- invoice that failed a payment (invoice.payment_failed / marked_uncollectible). +-- ever_failed is what lets ServiceBlockSignals count an invoice that is still +-- 'open' after a failed charge (a currently-'uncollectible' invoice the +-- derivation already catches by status). :execrows, but the caller ignores the +-- count: the flag is a set-only latch (a second failure event for the same +-- invoice re-sets it to the same value — 0 rows, a harmless no-op), and the +-- failed-charge STREAK is derived at read time from these facts + created_at, +-- not maintained as a counter, so there is nothing to double-count. Keyed on the +-- invoice, so it is a safe no-op when the mirror row has not landed yet. +-- name: MarkInvoiceFailed :execrows UPDATE ms_billing.invoices SET ever_failed = true WHERE stripe_invoice_id = $1 AND NOT ever_failed; - --- IncrementFailedChargeStreak bumps the account's consecutive failed-charge --- streak (migration 040) by one, resolving the account via the invoice mirror. --- Called by the webhook ONLY after MarkInvoiceEverFailed reports a fresh flip, --- so it counts distinct failed invoices, not per-retry events. :execrows so the --- caller can log a drift no-op when the invoice has no mirror row yet. --- name: IncrementFailedChargeStreak :execrows -UPDATE ms_billing.accounts -SET failed_charge_streak = failed_charge_streak + 1 -WHERE id = ( - SELECT inv.account_id FROM ms_billing.invoices AS inv - WHERE inv.stripe_invoice_id = $1 - ); - --- ResetFailedChargeStreak clears the account's consecutive failed-charge streak --- to 0 on a successful charge (invoice.paid) — the auto-cure that unblocks a --- previously-blocked account. Resolves the account via the invoice mirror. The --- `AND failed_charge_streak <> 0` guard makes it a no-op (0 rows) when the --- streak is already clean, so a replayed/redundant paid event is idempotent. --- name: ResetFailedChargeStreak :execrows -UPDATE ms_billing.accounts -SET failed_charge_streak = 0 -WHERE id = ( - SELECT inv.account_id FROM ms_billing.invoices AS inv - WHERE inv.stripe_invoice_id = $1 - ) - AND failed_charge_streak <> 0; diff --git a/internal/account/db/webhook.sql.go b/internal/account/db/webhook.sql.go index 60c4206..f49710f 100644 --- a/internal/account/db/webhook.sql.go +++ b/internal/account/db/webhook.sql.go @@ -37,12 +37,12 @@ WHERE i.stripe_invoice_id = $7 -- forward transition: incoming rank strictly above the stored rank … (CASE $1::text WHEN 'draft' THEN 0 WHEN 'open' THEN 1 - WHEN 'paid' THEN 2 WHEN 'void' THEN 2 WHEN 'uncollectible' THEN 2 + WHEN 'void' THEN 2 WHEN 'uncollectible' THEN 2 WHEN 'paid' THEN 3 ELSE -1 END) > (CASE i.status WHEN 'draft' THEN 0 WHEN 'open' THEN 1 - WHEN 'paid' THEN 2 WHEN 'void' THEN 2 WHEN 'uncollectible' THEN 2 + WHEN 'void' THEN 2 WHEN 'uncollectible' THEN 2 WHEN 'paid' THEN 3 ELSE -1 END) -- … OR an identical re-apply (idempotent amount refresh on replay). OR i.status = $1 @@ -76,16 +76,24 @@ type ApplyInvoiceStatusParams struct { // Out-of-order + at-least-once safety: Stripe delivers webhooks at-least-once // and NOT in guaranteed order, so a late invoice.finalized (open) can arrive // after invoice.paid (paid). The WHERE guard enforces a MONOTONIC status -// transition via a rank ladder — draft(0) < open(1) < paid/void/uncollectible(2, -// terminal) — so the row's status can only move forward. The new status's rank +// transition via a rank ladder — draft(0) < open(1) < void/uncollectible(2) < +// paid(3) — so the row's status can only move forward. The new status's rank // must be strictly greater than the current rank (a forward transition), with // one exception: an identical re-apply of the SAME status is allowed through so -// a replayed paid can refresh amount_paid/amount_due idempotently. A terminal -// status (rank 2) is never overwritten by a different status because no rank -// exceeds 2 and the equal-rank branch requires status equality, so paid→void -// (both rank 2) is a no-op — terminal-once-set holds. The CASE ladder is inline -// (not a stored ENUM) so a new Stripe status maps to the bottom rung (-1) and -// can never silently regress a known terminal state. +// a replayed paid can refresh amount_paid/amount_due idempotently. +// +// paid outranks void/uncollectible (rank 3 vs 2) DELIBERATELY: paying an +// invoice that Stripe already marked uncollectible (the customer settles it on +// the hosted page) transitions it uncollectible→paid, which MUST land so the +// delinquency signal clears and the collection relax + service-block cure run +// off the invoice.paid handler. Ranking paid at 2 (its old value) rejected that +// transition (2 > 2 false) and trapped a paying customer in delinquent/blocked +// state forever. void/uncollectible stay at 2 and so still cannot overwrite a +// paid(3) row (2 > 3 false; the equal-rank branch requires status equality), and +// Stripe never emits void/uncollectible AFTER a real payment — so terminal-once- +// paid still holds while the genuine recovery is no longer blocked. The CASE +// ladder is inline (not a stored ENUM) so a new Stripe status maps to the bottom +// rung (-1) and can never silently regress a known terminal state. // // :execrows → >0 means the row existed AND the guard let the update through; 0 // means either no mirror row (drift: the charge spine's UpsertInvoice hasn't @@ -133,28 +141,6 @@ func (q *Queries) DuplicateFingerprintPM(ctx context.Context, arg DuplicateFinge return id, err } -const incrementFailedChargeStreak = `-- name: IncrementFailedChargeStreak :execrows -UPDATE ms_billing.accounts -SET failed_charge_streak = failed_charge_streak + 1 -WHERE id = ( - SELECT inv.account_id FROM ms_billing.invoices AS inv - WHERE inv.stripe_invoice_id = $1 - ) -` - -// IncrementFailedChargeStreak bumps the account's consecutive failed-charge -// streak (migration 040) by one, resolving the account via the invoice mirror. -// Called by the webhook ONLY after MarkInvoiceEverFailed reports a fresh flip, -// so it counts distinct failed invoices, not per-retry events. :execrows so the -// caller can log a drift no-op when the invoice has no mirror row yet. -func (q *Queries) IncrementFailedChargeStreak(ctx context.Context, stripeInvoiceID string) (int64, error) { - result, err := q.db.Exec(ctx, incrementFailedChargeStreak, stripeInvoiceID) - if err != nil { - return 0, err - } - return result.RowsAffected(), nil -} - const insertPaymentMethod = `-- name: InsertPaymentMethod :execrows WITH acct AS ( SELECT id FROM ms_billing.accounts WHERE stripe_customer_id = $1 @@ -245,21 +231,25 @@ func (q *Queries) MarkEventProcessed(ctx context.Context, arg MarkEventProcessed return result.RowsAffected(), nil } -const markInvoiceEverFailed = `-- name: MarkInvoiceEverFailed :execrows +const markInvoiceFailed = `-- name: MarkInvoiceFailed :execrows UPDATE ms_billing.invoices SET ever_failed = true WHERE stripe_invoice_id = $1 AND NOT ever_failed ` -// MarkInvoiceEverFailed sets the sticky ever_failed flag (migration 039) on the -// first invoice.payment_failed for a row. The `AND NOT ever_failed` guard makes -// it a one-way latch: :execrows returns 1 ONLY on the first flip, 0 on Stripe's -// subsequent retry events (distinct event ids, so the webhook's event-level -// dedup does not collapse them). The webhook increments the account streak ONLY -// when this returns 1, so a single invoice's retries count as one failure. -func (q *Queries) MarkInvoiceEverFailed(ctx context.Context, stripeInvoiceID string) (int64, error) { - result, err := q.db.Exec(ctx, markInvoiceEverFailed, stripeInvoiceID) +// MarkInvoiceFailed sets the sticky ever_failed flag (migration 039) on an +// invoice that failed a payment (invoice.payment_failed / marked_uncollectible). +// ever_failed is what lets ServiceBlockSignals count an invoice that is still +// 'open' after a failed charge (a currently-'uncollectible' invoice the +// derivation already catches by status). :execrows, but the caller ignores the +// count: the flag is a set-only latch (a second failure event for the same +// invoice re-sets it to the same value — 0 rows, a harmless no-op), and the +// failed-charge STREAK is derived at read time from these facts + created_at, +// not maintained as a counter, so there is nothing to double-count. Keyed on the +// invoice, so it is a safe no-op when the mirror row has not landed yet. +func (q *Queries) MarkInvoiceFailed(ctx context.Context, stripeInvoiceID string) (int64, error) { + result, err := q.db.Exec(ctx, markInvoiceFailed, stripeInvoiceID) if err != nil { return 0, err } @@ -332,29 +322,6 @@ func (q *Queries) RelaxCollectionOnPaidInvoice(ctx context.Context, stripeInvoic return result.RowsAffected(), nil } -const resetFailedChargeStreak = `-- name: ResetFailedChargeStreak :execrows -UPDATE ms_billing.accounts -SET failed_charge_streak = 0 -WHERE id = ( - SELECT inv.account_id FROM ms_billing.invoices AS inv - WHERE inv.stripe_invoice_id = $1 - ) - AND failed_charge_streak <> 0 -` - -// ResetFailedChargeStreak clears the account's consecutive failed-charge streak -// to 0 on a successful charge (invoice.paid) — the auto-cure that unblocks a -// previously-blocked account. Resolves the account via the invoice mirror. The -// `AND failed_charge_streak <> 0` guard makes it a no-op (0 rows) when the -// streak is already clean, so a replayed/redundant paid event is idempotent. -func (q *Queries) ResetFailedChargeStreak(ctx context.Context, stripeInvoiceID string) (int64, error) { - result, err := q.db.Exec(ctx, resetFailedChargeStreak, stripeInvoiceID) - if err != nil { - return 0, err - } - return result.RowsAffected(), nil -} - const resolveAddCardRequest = `-- name: ResolveAddCardRequest :exec UPDATE ms_billing.add_card_requests SET status = $2::ms_billing.add_card_request_status, diff --git a/internal/account/eligibility/eligibility.go b/internal/account/eligibility/eligibility.go index dba1819..77ffbfc 100644 --- a/internal/account/eligibility/eligibility.go +++ b/internal/account/eligibility/eligibility.go @@ -21,10 +21,11 @@ package eligibility // MaxFailedChargeStreak is the consecutive failed-charge count at which services // are blocked. The spec is "< 2 failed" (2 excluded), so a streak of 0 or 1 -// passes and 2+ blocks. The streak is RECOVERABLE — it resets to 0 on the next -// successful charge (accounts.failed_charge_streak, migration 040) — so a block -// here self-heals when the account pays. Hardcoded (mirrors collection's inline -// finance thresholds); promote to a per-account column only if a knob is needed. +// passes and 2+ blocks. The streak is RECOVERABLE — it is derived (by +// ServiceBlockSignals) as the failures since the account's last successful +// charge, so paying moves the cutoff forward and self-heals the block. +// Hardcoded (mirrors collection's inline finance thresholds); promote to a +// per-account column only if a knob is needed. const MaxFailedChargeStreak = 2 // FirstChargeState is the outcome of the account's EARLIEST real charge — the diff --git a/internal/account/webhook/handlers.go b/internal/account/webhook/handlers.go index fca31eb..030f955 100644 --- a/internal/account/webhook/handlers.go +++ b/internal/account/webhook/handlers.go @@ -229,6 +229,21 @@ func (r *Router) handleInvoiceLifecycle(ctx context.Context, event stripego.Even return Result{HTTPStatus: 400, Status: StatusInvalidBody} } + // SERVICE-BLOCK failure latch — set BEFORE the found-guard and the status + // reconcile, on BOTH failure signals (payment_failed leaves the invoice + // 'open'; marked_uncollectible is a terminal that may arrive first under + // out-of-order delivery). ever_failed is invoice-keyed and set-only, so this + // is order-independent and a harmless no-op when the mirror row hasn't landed + // — the read-time streak derivation (ServiceBlockSignals) does the rest, so + // there is no counter to advance here and no reset on invoice.paid. A failure + // here is surfaced (500) so Stripe retries; the latch makes the retry a no-op. + if event.Type == stripego.EventTypeInvoicePaymentFailed || event.Type == stripego.EventTypeInvoiceMarkedUncollectible { + if err := r.store.MarkInvoiceFailed(ctx, inv.ID); err != nil { + r.log.ErrorContext(ctx, "invoice failure latch (ever_failed) failed", "event_id", event.ID, "type", event.Type, "stripe_invoice_id", inv.ID, "error", err) + return Result{HTTPStatus: 500, Status: StatusInternal} + } + } + found, err := r.store.ApplyInvoiceStatus(ctx, ApplyInvoiceStatusParams{ StripeInvoiceID: inv.ID, // Stripe invoice status mirrored verbatim (draft/open/paid/ @@ -256,28 +271,11 @@ func (r *Router) handleInvoiceLifecycle(ctx context.Context, event stripego.Even return Result{HTTPStatus: 200, Status: StatusDriftWarning} } - // SERVICE-BLOCK streak — invoice.payment_failed advances the account's - // consecutive failed-charge streak (migration 040), gated so a single - // invoice's Stripe retries count once (RecordFailedCharge's ever_failed - // latch). Reached only past the found guard above, so a late payment_failed - // against an already-terminal invoice (rejected by the monotonic guard) never - // counts. A failure here is surfaced (500) so Stripe retries; the latch makes - // the retry idempotent (no double increment). - if event.Type == stripego.EventTypeInvoicePaymentFailed { - counted, err := r.store.RecordFailedCharge(ctx, inv.ID) - if err != nil { - r.log.ErrorContext(ctx, "invoice.payment_failed record failed-charge streak failed", "event_id", event.ID, "stripe_invoice_id", inv.ID, "error", err) - return Result{HTTPStatus: 500, Status: StatusInternal} - } - if counted { - r.log.InfoContext(ctx, "invoice.payment_failed advanced failed-charge streak", "event_id", event.ID, "stripe_invoice_id", inv.ID) - } - } - - // RELAX + streak auto-cure — invoice.paid only. A failure here is surfaced - // (500) so Stripe retries: both UPDATEs are idempotent (relax is a no-op once - // arrears; reset is a no-op once the streak is already 0), so a retry after - // the mirror already advanced is safe. + // RELAX driver — invoice.paid only. A failure here is surfaced (500) so Stripe + // retries: the relax UPDATE is idempotent (a no-op once the account is already + // arrears), so a retry after the mirror already advanced is safe. The + // service-block auto-cure needs no write here — paying an invoice moves the + // ServiceBlockSignals streak cutoff (most-recent paid) forward automatically. if event.Type == stripego.EventTypeInvoicePaid { relaxed, err := r.store.RelaxCollectionOnPaidInvoice(ctx, inv.ID) if err != nil { @@ -287,15 +285,6 @@ func (r *Router) handleInvoiceLifecycle(ctx context.Context, event stripego.Even if relaxed { r.log.InfoContext(ctx, "invoice.paid relaxed account prepaid → arrears", "event_id", event.ID, "stripe_invoice_id", inv.ID) } - - reset, err := r.store.ResetFailedChargeStreak(ctx, inv.ID) - if err != nil { - r.log.ErrorContext(ctx, "invoice.paid reset failed-charge streak failed", "event_id", event.ID, "stripe_invoice_id", inv.ID, "error", err) - return Result{HTTPStatus: 500, Status: StatusInternal} - } - if reset { - r.log.InfoContext(ctx, "invoice.paid cleared failed-charge streak (service-block auto-cure)", "event_id", event.ID, "stripe_invoice_id", inv.ID) - } } return Result{HTTPStatus: 200, Status: StatusOK} } diff --git a/internal/account/webhook/router.go b/internal/account/webhook/router.go index d3c7c2e..71d9494 100644 --- a/internal/account/webhook/router.go +++ b/internal/account/webhook/router.go @@ -129,21 +129,15 @@ type Store interface { // row), never an error. It NEVER charges — relax and charge are decoupled. RelaxCollectionOnPaidInvoice(ctx context.Context, stripeInvoiceID string) (relaxed bool, err error) - // RecordFailedCharge maintains the service-block failed-charge streak on - // invoice.payment_failed: it flips the sticky per-invoice ever_failed marker - // (migration 039) and, ONLY on the FIRST failure of a distinct invoice, - // increments accounts.failed_charge_streak (migration 040) — so Stripe's - // per-retry payment_failed events (distinct event ids, past the event-level - // dedup) count as a single failure. Returns (counted bool, error): - // counted=true only when this was the first failure for the invoice and the - // streak advanced; false is a retry no-op or a not-yet-mirrored invoice. - RecordFailedCharge(ctx context.Context, stripeInvoiceID string) (counted bool, err error) - - // ResetFailedChargeStreak clears accounts.failed_charge_streak to 0 on - // invoice.paid — the auto-cure that unblocks an account the streak had - // blocked. Returns (reset bool, error): reset=false (0 rows) is a no-op — the - // streak was already clean or the invoice has no mirror row. - ResetFailedChargeStreak(ctx context.Context, stripeInvoiceID string) (reset bool, err error) + // MarkInvoiceFailed latches the sticky ever_failed flag (migration 039) on an + // invoice that failed a payment (invoice.payment_failed / marked_uncollectible), + // so ServiceBlockSignals' read-time streak derivation counts an invoice still + // 'open' after a failed charge. Set-only + invoice-keyed, so it is idempotent + // under Stripe's at-least-once + out-of-order delivery and a safe no-op when + // the mirror row has not landed. The failed-charge STREAK is DERIVED at read + // time (not a maintained counter), so there is no account write and nothing to + // reset on invoice.paid. + MarkInvoiceFailed(ctx context.Context, stripeInvoiceID string) error } // ApplyInvoiceStatusParams carries the columns ApplyInvoiceStatus reconciles diff --git a/internal/account/webhook/router_test.go b/internal/account/webhook/router_test.go index 8c60975..b38bd10 100644 --- a/internal/account/webhook/router_test.go +++ b/internal/account/webhook/router_test.go @@ -460,98 +460,75 @@ func TestProcess_InvoicePaymentFailed_StaysOpen(t *testing.T) { require.Equal(t, int64(0), s.AppliedInvoices[0].AmountPaidCents) } -// --- service-block failed-charge streak (migrations 039/040) -------------- +// --- service-block failure latch (migration 039; streak derived at read time) --- -func TestProcess_InvoicePaymentFailed_RecordsFailedCharge(t *testing.T) { - // payment_failed drives the service-block streak: the handler calls - // RecordFailedCharge with the invoice id (the store's ever_failed latch does - // the distinct-invoice dedup). paid is NOT involved, so no streak reset. - v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_pf_streak", "invoice.payment_failed", "in_1", "open", 0, 1200)} - s := webhooktest.NewFakeStore() - s.FailCounted = true - r := newRouter(v, s) +func TestProcess_FailureEvents_LatchEverFailed(t *testing.T) { + // Both failure signals latch ever_failed via MarkInvoiceFailed — and BEFORE + // the found-guard, so an out-of-order marked_uncollectible (or a not-yet- + // mirrored invoice) still latches. No account write happens; the streak is + // derived at read time. + for _, ev := range []struct{ name, typ, status string }{ + {"payment_failed", "invoice.payment_failed", "open"}, + {"marked_uncollectible", "invoice.marked_uncollectible", "uncollectible"}, + } { + t.Run(ev.name, func(t *testing.T) { + v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_lat_"+ev.name, ev.typ, "in_1", ev.status, 0, 1200)} + s := webhooktest.NewFakeStore() + r := newRouter(v, s) - res := r.Process(context.Background(), []byte(`{}`), "sig") + res := r.Process(context.Background(), []byte(`{}`), "sig") - require.Equal(t, 200, res.HTTPStatus) - require.Equal(t, webhook.StatusOK, res.Status) - require.Equal(t, []string{"in_1"}, s.FailedInvoices, "payment_failed must record the failed charge") - require.Empty(t, s.ResetInvoices, "payment_failed must not reset the streak") + require.Equal(t, 200, res.HTTPStatus) + require.Equal(t, webhook.StatusOK, res.Status) + require.Equal(t, []string{"in_1"}, s.FailedInvoices, "failure events must latch ever_failed") + }) + } } -func TestProcess_InvoicePaid_ResetsFailedChargeStreak(t *testing.T) { - // invoice.paid is the auto-cure: it resets the failed-charge streak (and never - // records a failure). - v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_paid_reset", "invoice.paid", "in_1", "paid", 1200, 1200)} +func TestProcess_FailureLatch_RunsEvenOnDriftMirror(t *testing.T) { + // The latch is set BEFORE the found-guard, so a payment_failed for a not-yet- + // mirrored invoice still calls MarkInvoiceFailed (a store-side no-op) — this + // is what makes the read-time streak order-independent. The event still ACKs + // drift (the status reconcile found no row). + v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_lat_drift", "invoice.payment_failed", "in_orphan", "open", 0, 1200)} s := webhooktest.NewFakeStore() - s.StreakReset = true + s.InvoiceFound = false r := newRouter(v, s) res := r.Process(context.Background(), []byte(`{}`), "sig") - require.Equal(t, 200, res.HTTPStatus) - require.Equal(t, webhook.StatusOK, res.Status) - require.Equal(t, []string{"in_1"}, s.ResetInvoices, "invoice.paid must reset the failed-charge streak") - require.Empty(t, s.FailedInvoices, "invoice.paid must not record a failure") + require.Equal(t, webhook.StatusDriftWarning, res.Status) + require.Equal(t, []string{"in_orphan"}, s.FailedInvoices, "latch runs ahead of the found-guard") } -func TestProcess_NonFailureInvoiceEvent_DoesNotRecordFailure(t *testing.T) { - // Only invoice.payment_failed feeds the streak. Every other invoice.* event - // lands the mirror but never touches RecordFailedCharge. +func TestProcess_NonFailureInvoiceEvent_DoesNotLatch(t *testing.T) { + // Only the two failure signals latch ever_failed. created/finalized/paid/void + // land the mirror but never mark the invoice failed. for _, ev := range []struct{ name, typ, status string }{ {"created", "invoice.created", "draft"}, {"finalized", "invoice.finalized", "open"}, {"paid", "invoice.paid", "paid"}, {"voided", "invoice.voided", "void"}, - {"uncollectible", "invoice.marked_uncollectible", "uncollectible"}, } { t.Run(ev.name, func(t *testing.T) { - v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_nf_"+ev.name, ev.typ, "in_x", ev.status, 0, 1200)} + v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_nl_"+ev.name, ev.typ, "in_x", ev.status, 0, 1200)} s := webhooktest.NewFakeStore() r := newRouter(v, s) res := r.Process(context.Background(), []byte(`{}`), "sig") require.Equal(t, webhook.StatusOK, res.Status) - require.Empty(t, s.FailedInvoices, "only payment_failed records a failed charge") + require.Empty(t, s.FailedInvoices, "only failure events latch ever_failed") }) } } -func TestProcess_InvoicePaymentFailed_DriftMirror_DoesNotRecord(t *testing.T) { - // No mirror row (drift) short-circuits at the found guard, before the streak - // step — a payment_failed for an unmirrored invoice never advances the streak. - v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_pf_drift", "invoice.payment_failed", "in_orphan", "open", 0, 1200)} - s := webhooktest.NewFakeStore() - s.InvoiceFound = false - r := newRouter(v, s) - - res := r.Process(context.Background(), []byte(`{}`), "sig") - - require.Equal(t, webhook.StatusDriftWarning, res.Status) - require.Empty(t, s.FailedInvoices, "drift short-circuits before the streak step") -} - -func TestProcess_InvoicePaymentFailed_RecordError_Internal(t *testing.T) { - // A streak-store error surfaces as 500 so Stripe retries; the ever_failed - // latch makes the retry idempotent (no double count). - v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_pf_err", "invoice.payment_failed", "in_1", "open", 0, 1200)} - s := webhooktest.NewFakeStore() - s.ErrRecordFailed = errors.New("db down") - r := newRouter(v, s) - - res := r.Process(context.Background(), []byte(`{}`), "sig") - - require.Equal(t, 500, res.HTTPStatus) - require.Equal(t, webhook.StatusInternal, res.Status) -} - -func TestProcess_InvoicePaid_ResetError_Internal(t *testing.T) { - // A streak-reset store error on invoice.paid surfaces as 500 so Stripe - // retries (the reset is idempotent once the streak is 0). - v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_paid_rst_err", "invoice.paid", "in_1", "paid", 1200, 1200)} +func TestProcess_FailureLatchError_Internal(t *testing.T) { + // A latch store error surfaces as 500 so Stripe retries; the set-only latch + // makes the retry a harmless no-op. + v := &webhooktest.FakeVerifier{Event: invoiceEvent("evt_lat_err", "invoice.payment_failed", "in_1", "open", 0, 1200)} s := webhooktest.NewFakeStore() - s.ErrResetStreak = errors.New("db down") + s.ErrMarkFailed = errors.New("db down") r := newRouter(v, s) res := r.Process(context.Background(), []byte(`{}`), "sig") diff --git a/internal/account/webhook/store.go b/internal/account/webhook/store.go index c01028f..2fca8e0 100644 --- a/internal/account/webhook/store.go +++ b/internal/account/webhook/store.go @@ -295,55 +295,20 @@ func (s *pgxStore) RelaxCollectionOnPaidInvoice(ctx context.Context, stripeInvoi return rows > 0, nil } -// RecordFailedCharge flips the sticky ever_failed marker on the invoice and, on -// the first failure of a distinct invoice ONLY, advances the account's -// consecutive failed-charge streak (see the query docs). The flip GATES the -// increment: MarkInvoiceEverFailed returns rows=1 exactly once per invoice (a -// one-way latch), so Stripe's per-retry payment_failed events never double-count. -// -// The flip and the increment MUST be atomic. They run in ONE transaction so the -// latch is never visible unless the increment also commits — otherwise a crash -// or transient error between two autocommits would leave ever_failed=true with -// the streak un-advanced, and the latch would then permanently suppress the -// increment on every retry/redelivery (event-id dedup short-circuits the same -// event; a later distinct event sees ever_failed already true). That would -// silently UNDER-count the streak and let a should-be-blocked account read -// eligible. On any error the tx rolls back, so the handler's 500 → Stripe -// redelivery re-flips and re-increments correctly. Returns (counted, error) -// where counted=true means the streak advanced. -func (s *pgxStore) RecordFailedCharge(ctx context.Context, stripeInvoiceID string) (bool, error) { - tx, err := s.pool.Begin(ctx) - if err != nil { - return false, err - } - defer func() { _ = tx.Rollback(ctx) }() - qtx := s.q.WithTx(tx) - - flipped, err := qtx.MarkInvoiceEverFailed(ctx, stripeInvoiceID) - if err != nil { - return false, err - } - if flipped == 0 { - // Already-failed invoice (a retry) or no mirror row yet — the streak - // counts distinct failed invoices, not events, so do not increment. - // Commit the (empty) tx so the caller sees a clean no-op. - return false, tx.Commit(ctx) - } - if _, err := qtx.IncrementFailedChargeStreak(ctx, stripeInvoiceID); err != nil { - return false, err - } - return true, tx.Commit(ctx) -} - -// ResetFailedChargeStreak clears the account's failed-charge streak to 0 on a -// paid invoice (auto-cure). Returns (reset, error): reset=false (0 rows) is a -// no-op — the streak was already clean or the invoice has no mirror row. -func (s *pgxStore) ResetFailedChargeStreak(ctx context.Context, stripeInvoiceID string) (bool, error) { - rows, err := s.q.ResetFailedChargeStreak(ctx, stripeInvoiceID) - if err != nil { - return false, err - } - return rows > 0, nil +// MarkInvoiceFailed latches the sticky ever_failed flag on an invoice that +// failed a payment (invoice.payment_failed / marked_uncollectible). It is a +// single set-only UPDATE — NO account counter, NO transaction: the failed-charge +// streak is DERIVED at read time by ServiceBlockSignals from ever_failed + +// status + created_at, so there is nothing to increment atomically and nothing +// a reordered/duplicate delivery can double-count. The rows-affected count is +// irrelevant (a repeat failure event for the same invoice is a harmless 0-row +// no-op), so the method returns only an error. A no-op when the mirror row has +// not landed yet (keyed on the invoice) — the currently-'uncollectible' case is +// counted by status regardless, and an 'open'-after-failure invoice is remarked +// by the next payment_failed once the row exists. +func (s *pgxStore) MarkInvoiceFailed(ctx context.Context, stripeInvoiceID string) error { + _, err := s.q.MarkInvoiceFailed(ctx, stripeInvoiceID) + return err } // text wraps a non-null Go string in the pgtype.Text the generated diff --git a/internal/account/webhook/store_integration_test.go b/internal/account/webhook/store_integration_test.go index 4d57d8f..94fddb8 100644 --- a/internal/account/webhook/store_integration_test.go +++ b/internal/account/webhook/store_integration_test.go @@ -716,88 +716,88 @@ func readMode(t *testing.T, pool *pgxpool.Pool, accountID uuid.UUID) string { return mode } -// --- service-block failed-charge streak (migrations 039/040) -------------- +// --- service-block failure latch + read-time streak derivation (migration 039) --- -func readFailedStreak(t *testing.T, pool *pgxpool.Pool, accountID uuid.UUID) int { +// seedInvoiceAt inserts an invoice with an explicit created_at + ever_failed so +// the read-time streak derivation (ordered on created_at) can be exercised +// deterministically without relying on wall-clock insert order. +func seedInvoiceAt(t *testing.T, pool *pgxpool.Pool, accountID uuid.UUID, stripeInvoiceID, status string, everFailed bool, createdAt string) { t.Helper() - var streak int - require.NoError(t, pool.QueryRow(context.Background(), - `SELECT failed_charge_streak FROM ms_billing.accounts WHERE id = $1`, - accountID.String()).Scan(&streak)) - return streak + _, err := pool.Exec(context.Background(), + `INSERT INTO ms_billing.invoices + (account_id, stripe_invoice_id, status, amount_due, currency, ever_failed, created_at) + VALUES ($1, $2, $3, 1200, 'usd', $4, $5::timestamptz)`, + accountID, stripeInvoiceID, status, everFailed, createdAt, + ) + require.NoError(t, err) } -func TestPgxStore_RecordFailedCharge_StreakLifecycle(t *testing.T) { - // The full streak lifecycle over the REAL store (exercises the atomic - // latch+increment tx): first failure counts once, a retry of the SAME - // invoice does not re-count, a second DISTINCT invoice advances to the - // block threshold, and a paid invoice resets to 0 (auto-cure). +func TestPgxStore_MarkInvoiceFailed_LatchesEverFailed(t *testing.T) { + // The set-only latch flips ever_failed and is an idempotent no-op on repeat / + // on an un-mirrored invoice. pool := testutil.NewTestDB(t) store := webhook.NewStore(pool) - accountID := seedAccount(t, pool, "cus_streak") + accountID := seedAccount(t, pool, "cus_latch") ctx := context.Background() - seedInvoice(t, pool, accountID, "in_A", "open", 0, 1200) - counted, err := store.RecordFailedCharge(ctx, "in_A") - require.NoError(t, err) - require.True(t, counted, "first failure of a distinct invoice counts") - require.Equal(t, 1, readFailedStreak(t, pool, accountID)) - // Retry of the SAME invoice (a distinct Stripe event) — ever_failed latch - // suppresses a second increment. - counted, err = store.RecordFailedCharge(ctx, "in_A") - require.NoError(t, err) - require.False(t, counted, "a retry of the same invoice must not re-count") - require.Equal(t, 1, readFailedStreak(t, pool, accountID)) - - // A second DISTINCT failed invoice advances the streak to the block threshold. - seedInvoice(t, pool, accountID, "in_B", "open", 0, 1200) - counted, err = store.RecordFailedCharge(ctx, "in_B") - require.NoError(t, err) - require.True(t, counted) - require.Equal(t, 2, readFailedStreak(t, pool, accountID)) + require.NoError(t, store.MarkInvoiceFailed(ctx, "in_A")) + var everFailed bool + require.NoError(t, pool.QueryRow(ctx, + `SELECT ever_failed FROM ms_billing.invoices WHERE stripe_invoice_id = 'in_A'`).Scan(&everFailed)) + require.True(t, everFailed) - // A successful charge clears the streak (auto-cure), and a repeat reset is - // an idempotent no-op. - reset, err := store.ResetFailedChargeStreak(ctx, "in_B") - require.NoError(t, err) - require.True(t, reset) - require.Equal(t, 0, readFailedStreak(t, pool, accountID)) - - reset, err = store.ResetFailedChargeStreak(ctx, "in_B") - require.NoError(t, err) - require.False(t, reset, "reset is idempotent once the streak is 0") + require.NoError(t, store.MarkInvoiceFailed(ctx, "in_A"), "repeat is a no-op") + require.NoError(t, store.MarkInvoiceFailed(ctx, "in_orphan"), "un-mirrored invoice is a no-op") } -func TestPgxStore_RecordFailedCharge_NoMirrorRow_IsNoOp(t *testing.T) { - // A payment_failed for an un-mirrored invoice must not advance any streak - // (the latch flips zero rows) and must not error. +func TestPgxStore_ServiceBlockSignals_StreakDerivation_IsDeliveryOrderImmune(t *testing.T) { + // The failed-charge streak is DERIVED from (ever_failed OR uncollectible) + + // created_at relative to the most-recent paid invoice — so it depends only on + // the immutable facts, never on webhook delivery order. Timeline: A failed + // (t1), B PAID (t2), C failed (t3). Only C is after the last paid, so the + // streak is 1 regardless of the order these rows/latches landed. pool := testutil.NewTestDB(t) - store := webhook.NewStore(pool) + bstore := billing.NewStore(pool) + accountID := seedAccount(t, pool, "cus_deriv") ctx := context.Background() - counted, err := store.RecordFailedCharge(ctx, "in_orphan") + seedInvoiceAt(t, pool, accountID, "in_A", "open", true, "2026-01-01T00:00:00Z") + seedInvoiceAt(t, pool, accountID, "in_B", "paid", false, "2026-02-01T00:00:00Z") + seedInvoiceAt(t, pool, accountID, "in_C", "open", true, "2026-03-01T00:00:00Z") + + sig, err := bstore.ServiceBlockSignals(ctx, accountID) require.NoError(t, err) - require.False(t, counted) + require.Equal(t, 1, sig.FailedChargeStreak, "only failures after the last paid invoice count") + + // Two distinct failures after the last paid reach the block threshold. + seedInvoiceAt(t, pool, accountID, "in_D", "uncollectible", false, "2026-04-01T00:00:00Z") + sig, err = bstore.ServiceBlockSignals(ctx, accountID) + require.NoError(t, err) + require.Equal(t, 2, sig.FailedChargeStreak) + + // A newer paid invoice moves the cutoff forward and clears the streak (the + // auto-cure) — no counter to reset. + seedInvoiceAt(t, pool, accountID, "in_E", "paid", false, "2026-05-01T00:00:00Z") + sig, err = bstore.ServiceBlockSignals(ctx, accountID) + require.NoError(t, err) + require.Equal(t, 0, sig.FailedChargeStreak, "a later paid invoice cures the streak") } -func TestPgxStore_ServiceBlockSignals_ReflectsCardStreakAndFirstCharge(t *testing.T) { - // The billing store's one-shot signal read reflects the usable non-fraud - // card count, the failed-charge streak, and the earliest real charge. +func TestPgxStore_ServiceBlockSignals_ReflectsCardAndFirstCharge(t *testing.T) { + // The one-shot read reflects the usable non-fraud card count (fraud-blocked + // excluded) and the earliest real charge's status. pool := testutil.NewTestDB(t) bstore := billing.NewStore(pool) accountID := seedAccount(t, pool, "cus_sig") ctx := context.Background() - // Brand new: no card, no charge, clean streak. sig, err := bstore.ServiceBlockSignals(ctx, accountID) require.NoError(t, err) require.Equal(t, 0, sig.UsableCardCount) require.Equal(t, 0, sig.FailedChargeStreak) require.Equal(t, "", sig.FirstChargeStatus) - // A usable non-fraud card, a fraud-blocked card (excluded), and an - // uncollectible earliest invoice. _, err = pool.Exec(ctx, `INSERT INTO ms_billing.payment_methods_mirror (account_id, stripe_payment_method_id, brand, last4, exp_month, exp_year) @@ -815,3 +815,28 @@ func TestPgxStore_ServiceBlockSignals_ReflectsCardStreakAndFirstCharge(t *testin require.Equal(t, 1, sig.UsableCardCount, "fraud-blocked card is excluded from the usable count") require.Equal(t, "uncollectible", sig.FirstChargeStatus) } + +func TestPgxStore_ApplyInvoiceStatus_UncollectibleToPaid_Cures(t *testing.T) { + // M1: paying an uncollectible invoice (customer settles on the hosted page) + // must land — paid outranks uncollectible — so the delinquency/block clears. + pool := testutil.NewTestDB(t) + store := webhook.NewStore(pool) + accountID := seedAccount(t, pool, "cus_cure") + seedInvoice(t, pool, accountID, "in_unc", "uncollectible", 0, 1200) + ctx := context.Background() + + found, err := store.ApplyInvoiceStatus(ctx, webhook.ApplyInvoiceStatusParams{ + StripeInvoiceID: "in_unc", Status: "paid", AmountPaidCents: 1200, AmountDueCents: 1200, + }) + require.NoError(t, err) + require.True(t, found, "uncollectible→paid must be accepted") + require.Equal(t, "paid", readInvoiceStatus(t, pool, "in_unc")) + + // And a stray late uncollectible after paid must NOT regress it. + found, err = store.ApplyInvoiceStatus(ctx, webhook.ApplyInvoiceStatusParams{ + StripeInvoiceID: "in_unc", Status: "uncollectible", AmountPaidCents: 0, AmountDueCents: 1200, + }) + require.NoError(t, err) + require.False(t, found, "a late uncollectible must not overwrite paid") + require.Equal(t, "paid", readInvoiceStatus(t, pool, "in_unc")) +} diff --git a/internal/account/webhook/webhooktest/fakes.go b/internal/account/webhook/webhooktest/fakes.go index 95e7b09..fcb2ce0 100644 --- a/internal/account/webhook/webhooktest/fakes.go +++ b/internal/account/webhook/webhooktest/fakes.go @@ -49,8 +49,7 @@ type FakeStore struct { AppliedInvoices []webhook.ApplyInvoiceStatusParams // captured calls to ApplyInvoiceStatus RelaxedInvoices []string // stripe_invoice_id values from RelaxCollectionOnPaidInvoice - FailedInvoices []string // stripe_invoice_id values from RecordFailedCharge - ResetInvoices []string // stripe_invoice_id values from ResetFailedChargeStreak + FailedInvoices []string // stripe_invoice_id values from MarkInvoiceFailed // Found-flag knobs TouchedFound bool // returned by TouchAccountByStripeCustomer @@ -58,8 +57,6 @@ type FakeStore struct { SoftDelFound bool // returned by SoftDeletePaymentMethod InvoiceFound bool // returned by ApplyInvoiceStatus Relaxed bool // returned by RelaxCollectionOnPaidInvoice - FailCounted bool // returned by RecordFailedCharge (counted) - StreakReset bool // returned by ResetFailedChargeStreak (reset) ActivatedNew bool // returned by StampAccountActivated (firstBind) // Error injection @@ -72,8 +69,7 @@ type FakeStore struct { ErrResolve error // from ResolvePendingAddCardRequest ErrApplyInvoice error // from ApplyInvoiceStatus ErrRelax error // from RelaxCollectionOnPaidInvoice - ErrRecordFailed error // from RecordFailedCharge - ErrResetStreak error // from ResetFailedChargeStreak + ErrMarkFailed error // from MarkInvoiceFailed ErrActivate error // from StampAccountActivated } @@ -176,20 +172,12 @@ func (s *FakeStore) RelaxCollectionOnPaidInvoice(_ context.Context, stripeInvoic return s.Relaxed, nil } -func (s *FakeStore) RecordFailedCharge(_ context.Context, stripeInvoiceID string) (bool, error) { - if s.ErrRecordFailed != nil { - return false, s.ErrRecordFailed +func (s *FakeStore) MarkInvoiceFailed(_ context.Context, stripeInvoiceID string) error { + if s.ErrMarkFailed != nil { + return s.ErrMarkFailed } s.FailedInvoices = append(s.FailedInvoices, stripeInvoiceID) - return s.FailCounted, nil -} - -func (s *FakeStore) ResetFailedChargeStreak(_ context.Context, stripeInvoiceID string) (bool, error) { - if s.ErrResetStreak != nil { - return false, s.ErrResetStreak - } - s.ResetInvoices = append(s.ResetInvoices, stripeInvoiceID) - return s.StreakReset, nil + return nil } // SilentLogger returns a slog.Logger that discards all output. Useful diff --git a/migrations/billing/039_invoice_ever_failed.up.sql b/migrations/billing/039_invoice_ever_failed.up.sql index 8d3c347..0c21dbd 100644 --- a/migrations/billing/039_invoice_ever_failed.up.sql +++ b/migrations/billing/039_invoice_ever_failed.up.sql @@ -1,23 +1,24 @@ -- Migration 039 — sticky per-invoice "ever failed" marker (service-block gate). -- --- The service-block gate counts an account's CONSECUTIVE failed-charge streak --- (accounts.failed_charge_streak, migration 040): +1 the first time an invoice --- fails, reset to 0 on the next successful charge, block at >= 2. Stripe smart- --- retries a failed invoice and fires invoice.payment_failed on EACH attempt --- (distinct event ids, so the webhook's event-level dedup does NOT collapse --- them). Counting every payment_failed event would over-count a single --- invoice's retries. +-- The service-block gate needs to count an account's failed-charge STREAK. That +-- streak is DERIVED at read time (ServiceBlockSignals) as the number of distinct +-- FAILED invoices created after the account's most-recent PAID invoice — a +-- delivery-order-immune reading of the invoice mirror, not a mutable counter. -- --- ever_failed is the per-invoice guard that makes the streak count DISTINCT --- invoices: the webhook flips it false->true on the first payment_failed for a --- row (an UPDATE ... WHERE NOT ever_failed, so RowsAffected=1 only once) and --- increments the account streak ONLY on that flip. It is never cleared — it is --- a historical "this invoice failed at least once" fact, independent of the --- invoice's later status (a failed-then-paid invoice keeps ever_failed=true --- while the account streak resets). Distinct from the derived delinquency --- signal (AccountHasUnpaidInvoice, which reflects CURRENT status only). +-- ever_failed is the fact that read needs: an invoice can sit at status 'open' +-- AFTER a failed payment (Stripe keeps retrying), so status alone can't tell an +-- open-because-just-finalized invoice from an open-because-it-failed one. The +-- webhook latches ever_failed=true on a failure event (payment_failed / +-- marked_uncollectible); the derivation then counts (ever_failed OR currently +-- 'uncollectible') invoices. It is set-only and never cleared — a historical +-- "this invoice failed at least once" fact that survives a later flip to paid +-- (that invoice is excluded from the streak by the created_at cutoff, not by +-- clearing the flag). Distinct from the delinquency signal (AccountHasUnpaidInvoice, +-- CURRENT status only). -- --- Every historic row defaults false; only a NEW payment_failed sets it. Born +-- Set-only + invoice-keyed makes the webhook write idempotent under Stripe's +-- at-least-once + out-of-order delivery — there is no counter to double-count. +-- Every historic row defaults false; only a NEW failure event sets it. Born -- clean at slot 039 (next free after 038). sqlc picks up the column -- automatically. updated_at is trigger-maintained (001). @@ -25,7 +26,8 @@ ALTER TABLE ms_billing.invoices ADD COLUMN ever_failed BOOLEAN NOT NULL DEFAULT false; COMMENT ON COLUMN ms_billing.invoices.ever_failed IS - 'Sticky: set true on the FIRST invoice.payment_failed for this row and never ' - 'cleared. The service-block gate uses it to count DISTINCT failed invoices ' - '(not per-retry events) into accounts.failed_charge_streak. Independent of ' - 'the current status — survives a later flip to paid.'; + 'Sticky, set-only: true once this invoice failed a payment ' + '(payment_failed / marked_uncollectible), never cleared. Lets the ' + 'service-block gate DERIVE the failed-charge streak at read time — counting ' + '(ever_failed OR uncollectible) invoices created after the last paid one — ' + 'so it survives a later flip to paid and is immune to webhook delivery order.'; diff --git a/migrations/billing/040_account_failed_charge_streak.down.sql b/migrations/billing/040_account_failed_charge_streak.down.sql deleted file mode 100644 index da3117b..0000000 --- a/migrations/billing/040_account_failed_charge_streak.down.sql +++ /dev/null @@ -1,3 +0,0 @@ --- 040 down: drop the consecutive failed-charge streak column. -ALTER TABLE ms_billing.accounts - DROP COLUMN IF EXISTS failed_charge_streak; diff --git a/migrations/billing/040_account_failed_charge_streak.up.sql b/migrations/billing/040_account_failed_charge_streak.up.sql deleted file mode 100644 index b80da89..0000000 --- a/migrations/billing/040_account_failed_charge_streak.up.sql +++ /dev/null @@ -1,30 +0,0 @@ --- Migration 040 — consecutive failed-charge streak on the billing account --- (service-block gate). --- --- The service-block gate (internal/account/eligibility) blocks an account whose --- failed-charge count reaches 2 ("< 2 failed" — 2 excluded). The streak is --- RECOVERABLE, not a lifetime tally: it counts CONSECUTIVE distinct failed --- invoices and RESETS to 0 on the next successful charge, so an account self- --- heals the moment it pays (product decision: auto-cure on next success). --- --- Maintained entirely by the invoice.* webhook: --- invoice.payment_failed -> +1, but only on the first failure of a distinct --- invoice (gated by invoices.ever_failed, migration --- 039, so Stripe's per-retry events don't over-count). --- invoice.paid -> reset to 0. --- The gate READS this column directly (no aggregation) alongside the usable-card --- count and the first-charge outcome. --- --- Defaults 0 for every existing account (a clean streak). Born clean at slot 040 --- (next free after 039). updated_at is trigger-maintained (001); sqlc picks up --- the column automatically. - -ALTER TABLE ms_billing.accounts - ADD COLUMN failed_charge_streak INT NOT NULL DEFAULT 0 - CHECK (failed_charge_streak >= 0); - -COMMENT ON COLUMN ms_billing.accounts.failed_charge_streak IS - 'Consecutive distinct failed invoices, maintained by the invoice webhook ' - '(+1 on the first payment_failed of a row via invoices.ever_failed, reset to ' - '0 on invoice.paid). The service-block gate blocks at >= 2. Recoverable — ' - 'self-heals to 0 on the next successful charge.'; diff --git a/scripts/init-db.sql b/scripts/init-db.sql index b1fe58a..f4c5f2b 100644 --- a/scripts/init-db.sql +++ b/scripts/init-db.sql @@ -55,8 +55,8 @@ \i migrations/billing/036_charge_attempt_markers.up.sql \i migrations/billing/037_apps_name.up.sql --- 038–040: service-block eligibility gate — card fraud flag, sticky per-invoice --- failure marker, and the account's consecutive failed-charge streak. +-- 038–039: service-block eligibility gate — card fraud flag + sticky per-invoice +-- failure marker (the failed-charge streak is DERIVED at read time from these +-- facts in ServiceBlockSignals, so it needs no stored column). \i migrations/billing/038_payment_method_fraud.up.sql \i migrations/billing/039_invoice_ever_failed.up.sql -\i migrations/billing/040_account_failed_charge_streak.up.sql