From 32631560670522bf52f1bd862b0ae94f0ef552e1 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Mon, 24 Aug 2026 20:41:36 -0500 Subject: [PATCH] billing: key entitlements on the Stripe product, not the price Tier resolution matched a subscription's price ID against the configured stripe_price_monthly/stripe_price_yearly, which inverts what a price change is supposed to do. Stripe prices are immutable, so changing what a tier costs means creating a new price, and Stripe never migrates existing subscribers off the old one. Updating the config to the new price IDs therefore un-tiers precisely the subscribers a price change is meant to leave alone. They did not even drop cleanly to free. An unresolved tier logs a warning, returns nil, and the event is recorded in stripe_processed_events -- so Stripe answers 200, never redelivers, and a later dashboard Resend is swallowed by the idempotency check. Reproduced against the sandbox: a subscription on a price the config does not list granted nothing, and the event could not be replayed afterwards. A tier has one product and many prices over its life, so the product is the durable key for an entitlement. Tiers gain a stripe_product field, and resolution tries the product first, falling back to the price IDs so configs without it keep working unchanged. Checkout still keys on price -- that direction has to name a specific price to charge. Verified live: a subscription on a price created outside the config, under the Pro product, resolved to tierName=Pro tierRank=2 and landed on the hold. The same shape with an unknown product produced the silent no-op before. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB --- config-appview.example.yaml | 4 + pkg/billing/billing.go | 40 ++++++- pkg/billing/config.go | 31 +++++ pkg/billing/tier_resolution_test.go | 180 ++++++++++++++++++++++++++++ 4 files changed, 252 insertions(+), 3 deletions(-) create mode 100644 pkg/billing/tier_resolution_test.go diff --git a/config-appview.example.yaml b/config-appview.example.yaml index 73932b1..d18e340 100644 --- a/config-appview.example.yaml +++ b/config-appview.example.yaml @@ -146,6 +146,8 @@ billing: stripe_price_monthly: "" # Stripe price ID for yearly billing. stripe_price_yearly: "price_1SmK1mRROAC4bYmSwhTQ7RY9" + # Stripe product ID for this tier. Entitlements resolve on this, so changing a price does not remove features from existing subscribers. Falls back to the price IDs when empty. + stripe_product: "prod_TjndpygrZWXHzS" # Maximum webhooks for this tier (-1 = unlimited). max_webhooks: 1 # Allow all webhook trigger types (not just first-scan). @@ -164,6 +166,8 @@ billing: stripe_price_monthly: "price_1SmK4QRROAC4bYmSxpr35HUl" # Stripe price ID for yearly billing. stripe_price_yearly: "price_1SmJuLRROAC4bYmSUgVCwZWo" + # Stripe product ID for this tier. Entitlements resolve on this, so changing a price does not remove features from existing subscribers. Falls back to the price IDs when empty. + stripe_product: "prod_TjnVZ1HcRm3dLL" # Maximum webhooks for this tier (-1 = unlimited). max_webhooks: 10 # Allow all webhook trigger types (not just first-scan). diff --git a/pkg/billing/billing.go b/pkg/billing/billing.go index fa379c3..7737a71 100644 --- a/pkg/billing/billing.go +++ b/pkg/billing/billing.go @@ -632,8 +632,7 @@ func (m *Manager) handleSubscriptionChange(event stripe.Event) error { switch sub.Status { case stripe.SubscriptionStatusActive, stripe.SubscriptionStatusTrialing: if sub.Items != nil && len(sub.Items.Data) > 0 { - priceID := sub.Items.Data[0].Price.ID - tierName, tierRank = m.cfg.GetTierByPriceID(priceID) + tierName, tierRank = m.resolveTier(sub.Items.Data[0].Price) } case stripe.SubscriptionStatusPastDue: @@ -671,7 +670,12 @@ func (m *Manager) handleSubscriptionChange(event stripe.Event) error { } if tierName == "" { - slog.Warn("Could not resolve tier from subscription", "priceID", sub.Items.Data[0].Price.ID) + slog.Warn("Could not resolve tier from subscription", + "priceID", sub.Items.Data[0].Price.ID, + "productID", priceProductID(sub.Items.Data[0].Price), + "subscriptionID", sub.ID, + "customerID", sub.Customer.ID, + ) return nil } @@ -710,6 +714,36 @@ func (m *Manager) handleSubscriptionChange(event stripe.Event) error { return nil } +// resolveTier maps a subscription's price onto a configured tier. +// +// The product is tried first and the price ID is the fallback. That ordering is +// what makes grandfathering work: a tier's prices change over its life, its +// product does not, and Stripe leaves existing subscribers on the price they +// signed up at forever. Matching on price alone would drop those subscribers +// out of their tier the moment a new price was introduced — the opposite of +// what a price change is meant to do. +// +// The fallback keeps configs that predate stripe_product resolving unchanged. +func (m *Manager) resolveTier(price *stripe.Price) (string, int) { + if price == nil { + return "", -1 + } + if name, rank := m.cfg.GetTierByProductID(priceProductID(price)); name != "" { + return name, rank + } + return m.cfg.GetTierByPriceID(price.ID) +} + +// priceProductID returns the product ID behind a price, or "" if absent. +// Stripe sends price.product as a bare ID string unless it is expanded, which +// stripe-go unmarshals into a Product carrying only that ID. +func priceProductID(price *stripe.Price) string { + if price == nil || price.Product == nil { + return "" + } + return price.Product.ID +} + // handleInvoicePaymentFailed logs a failed invoice payment. // No tier change: handleSubscriptionChange reacts to the subsequent // past_due → unpaid transition. Stripe Smart Retries handle retry cadence diff --git a/pkg/billing/config.go b/pkg/billing/config.go index 4e917a0..33cd6fb 100644 --- a/pkg/billing/config.go +++ b/pkg/billing/config.go @@ -42,6 +42,10 @@ type BillingTierConfig struct { // Stripe price ID for yearly billing. StripePriceYearly string `yaml:"stripe_price_yearly,omitempty" comment:"Stripe price ID for yearly billing."` + // Stripe product ID backing this tier. Entitlements resolve on this, not + // on the price IDs above, so a price change does not strand subscribers. + StripeProduct string `yaml:"stripe_product,omitempty" comment:"Stripe product ID for this tier. Entitlements resolve on this, so changing a price does not remove features from existing subscribers. Falls back to the price IDs when empty."` + // Maximum number of webhooks for this tier (-1 = unlimited). MaxWebhooks int `yaml:"max_webhooks" comment:"Maximum webhooks for this tier (-1 = unlimited)."` @@ -55,8 +59,35 @@ type BillingTierConfig struct { SupporterBadge bool `yaml:"supporter_badge" comment:"Show supporter badge on user profiles for subscribers at this tier."` } +// GetTierByProductID finds the tier backed by the given Stripe product ID. +// Returns the tier name and rank, or empty string and -1 if not found. +// +// Product rather than price is the durable key for an entitlement. Stripe +// prices are immutable, so changing what a tier costs means creating a new +// price under the same product, and existing subscribers keep billing on the +// old one indefinitely — Stripe never migrates them. Resolving entitlements by +// price ID therefore un-tiers precisely the subscribers a price change is +// meant to leave alone. Resolving by product grandfathers them for free. +// +// Prices stay the key in the other direction: checkout has to name a specific +// price to charge. +func (c *Config) GetTierByProductID(productID string) (string, int) { + if c == nil || productID == "" { + return "", -1 + } + for i, tier := range c.Tiers { + if tier.StripeProduct == productID { + return tier.Name, i + } + } + return "", -1 +} + // GetTierByPriceID finds the tier that contains the given Stripe price ID. // Returns the tier name and rank, or empty string and -1 if not found. +// +// Kept as the fallback for tiers with no stripe_product configured, so an +// existing config keeps resolving unchanged. func (c *Config) GetTierByPriceID(priceID string) (string, int) { if c == nil || priceID == "" { return "", -1 diff --git a/pkg/billing/tier_resolution_test.go b/pkg/billing/tier_resolution_test.go new file mode 100644 index 0000000..1803a3b --- /dev/null +++ b/pkg/billing/tier_resolution_test.go @@ -0,0 +1,180 @@ +//go:build billing + +package billing + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "atcr.io/pkg/atproto" + "github.com/bluesky-social/indigo/atproto/atcrypto" + "github.com/stripe/stripe-go/v84" +) + +// Tier resolution used to key on the price ID alone, which quietly inverted +// what a price change is supposed to do. Stripe prices are immutable, so +// raising a price means creating a new one, and Stripe never moves existing +// subscribers off the old one. Matching on price therefore dropped exactly the +// subscribers a price change is meant to leave untouched — and dropped them +// into the silent branch, where the event is answered 200, recorded as +// processed, and can never be redelivered. + +func productPrice(priceID, productID string) *stripe.Price { + p := &stripe.Price{ID: priceID} + if productID != "" { + p.Product = &stripe.Product{ID: productID} + } + return p +} + +func tierTestConfig() *Config { + return &Config{Tiers: []BillingTierConfig{ + {Name: "free"}, + { + Name: "supporter", + StripeProduct: "prod_supporter", + StripePriceMonthly: "price_supporter_v2", + }, + { + Name: "pro", + StripePriceMonthly: "price_pro_only", + }, + }} +} + +// TestResolveTier_GrandfatheredPriceResolvesByProduct is the whole point of +// keying on the product: a subscriber still billing on price_supporter_v1, +// which no longer appears anywhere in the config, keeps their tier. +func TestResolveTier_GrandfatheredPriceResolvesByProduct(t *testing.T) { + m := &Manager{cfg: tierTestConfig()} + + name, rank := m.resolveTier(productPrice("price_supporter_v1_retired", "prod_supporter")) + if name != "supporter" || rank != 1 { + t.Errorf("resolveTier(retired price, live product) = (%q, %d), want (\"supporter\", 1) — "+ + "a subscriber on the old price lost their tier", name, rank) + } +} + +// TestResolveTier_FallsBackToPriceWhenNoProductConfigured: a config written +// before stripe_product existed must keep resolving exactly as it did. +func TestResolveTier_FallsBackToPriceWhenNoProductConfigured(t *testing.T) { + m := &Manager{cfg: tierTestConfig()} + + name, rank := m.resolveTier(productPrice("price_pro_only", "prod_pro_unconfigured")) + if name != "pro" || rank != 2 { + t.Errorf("resolveTier = (%q, %d), want (\"pro\", 2) — the price fallback stopped working", name, rank) + } +} + +// TestResolveTier_ProductWinsOverPrice: when both could match, the product +// decides. Otherwise a price ID left behind on the wrong tier could outvote the +// product and grant the wrong entitlement. +func TestResolveTier_ProductWinsOverPrice(t *testing.T) { + m := &Manager{cfg: &Config{Tiers: []BillingTierConfig{ + {Name: "free"}, + {Name: "supporter", StripeProduct: "prod_supporter"}, + {Name: "pro", StripePriceMonthly: "price_shared"}, + }}} + + name, _ := m.resolveTier(productPrice("price_shared", "prod_supporter")) + if name != "supporter" { + t.Errorf("resolveTier = %q, want \"supporter\" — the price ID outvoted the product", name) + } +} + +// TestResolveTier_UnknownEverythingIsUnresolved keeps the negative honest: a +// price on a product the config has never heard of must not resolve to a tier. +func TestResolveTier_UnknownEverythingIsUnresolved(t *testing.T) { + m := &Manager{cfg: tierTestConfig()} + + if name, rank := m.resolveTier(productPrice("price_unknown", "prod_unknown")); name != "" || rank != -1 { + t.Errorf("resolveTier(unknown) = (%q, %d), want (\"\", -1)", name, rank) + } + if name, rank := m.resolveTier(nil); name != "" || rank != -1 { + t.Errorf("resolveTier(nil) = (%q, %d), want (\"\", -1)", name, rank) + } +} + +// TestHandleSubscriptionChange_GrandfatheredSubscriberKeepsTier runs the same +// property through the real webhook path, so it covers the payload shape too: +// Stripe sends price.product as a bare ID string, and this fails if that is +// ever read wrongly. +// +// It asserts on the rank the managed hold is actually asked to apply. An +// earlier draft asserted that the event was recorded as processed, and passed +// against price-only resolution — because an unresolved tier ALSO records the +// event and returns nil. That is the defect itself, so any assertion it +// satisfies cannot be measuring the fix. +func TestHandleSubscriptionChange_GrandfatheredSubscriberKeepsTier(t *testing.T) { + atproto.SetTestMode(true) + t.Cleanup(func() { atproto.SetTestMode(false) }) + + const secret = "whsec_grandfather_test" + m, _ := newTestManager(t, secret) + m.cfg.Tiers = tierTestConfig().Tiers + + stripeAPIReturning(t, http.StatusOK, + `{"id":"cus_gf","object":"customer","metadata":{"user_did":"did:plc:grandfathered"}}`) + + type tierPush struct { + UserDID string `json:"userDid"` + TierRank int `json:"tierRank"` + } + pushes := make(chan tierPush, 4) + hold := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var got tierPush + _ = json.NewDecoder(r.Body).Decode(&got) + pushes <- got + w.WriteHeader(http.StatusOK) + })) + defer hold.Close() + m.managedHolds = []string{"did:web:" + strings.ReplaceAll( + strings.TrimPrefix(hold.URL, "http://"), ":", "%3A")} + + priv, err := atcrypto.GeneratePrivateKeyP256() + if err != nil { + t.Fatalf("generate key: %v", err) + } + m.privateKey = priv + + // price_supporter_v1_retired appears nowhere in the config. Only its + // product does — this is the subscriber a price change left behind. + payload := fmt.Sprintf(`{ + "id": "evt_grandfathered", + "object": "event", + "api_version": %q, + "type": "customer.subscription.updated", + "created": %d, + "data": {"object": { + "id": "sub_gf", + "object": "subscription", + "status": "active", + "customer": "cus_gf", + "items": {"object":"list","data":[{"price":{ + "id":"price_supporter_v1_retired", + "product":"prod_supporter" + }}]} + }} + }`, stripe.APIVersion, time.Now().Unix()) + + if err := postWebhook(t, m, secret, []byte(payload)); err != nil { + t.Fatalf("webhook: %v", err) + } + + select { + case got := <-pushes: + if got.TierRank != 1 { + t.Errorf("hold was asked for tierRank %d, want 1 (supporter)", got.TierRank) + } + if got.UserDID != "did:plc:grandfathered" { + t.Errorf("hold was asked to update %q, want did:plc:grandfathered", got.UserDID) + } + case <-time.After(2 * time.Second): + t.Fatal("no tier push reached the hold — the grandfathered subscriber's tier was never applied") + } +}