fix(api): stop dropping subscription and invoice webhook events

[paulvanbrenk]

The bug

StripeWebhook only acted on events whose data object carried metadata["app"] == "patchnotes". That tag was set in exactly one place — SessionCreateOptions.Metadata in SubscriptionRoutes.cs — and Stripe does not copy a Checkout Session's own metadata onto the subscription it creates. subscription_data.metadata is a separate parameter, and it was never set. Invoices never carry it at all.

Four of the five handled event types were therefore dropped:

Event	Data object	app metadata	Before
checkout.session.completed	Session	set at creation	handled
customer.subscription.updated	Subscription	empty	dropped
customer.subscription.deleted	Subscription	empty	dropped
invoice.payment_failed	Invoice	empty	dropped
invoice.payment_succeeded	Invoice	empty	dropped

A subscriber activated correctly at checkout and was then frozen. Renewals never extended SubscriptionExpiresAt, so a paying customer silently lost Pro after one billing period. Cancellations never set canceled; failed payments never set past_due.

It stayed invisible because the ignore path returns 200 OK — Stripe's dashboard showed a healthy endpoint at 100% delivery success.

The fix

Events are recognised two ways, because neither covers everything:

• Metadata — SubscriptionData.Metadata is now set at checkout, so new subscriptions and their events carry the tag regardless of what order Stripe delivers them in.
• Known customer — anything untagged is matched against a StripeCustomerId we already store. Without this, every subscription created before this PR would stay broken forever; the tag can't be applied retroactively.

Also: stop trusting the snapshot payload

Snapshot events are eventually consistent — Stripe recommends re-fetching. HandleCheckoutSessionCompleted and HandlePaymentSucceeded already did; the two subscription handlers didn't. They now re-read the subscription and use the payload only to identify it.

Re-fetching alone doesn't fix out-of-order delivery, because a cancelled subscription still reads back as cancelled. A delayed deleted for a subscription the user has already replaced is declined explicitly instead of cancelling the current one.

Tests

7 new tests drive the real signature-verification path. Three fail against the previous implementation, verified by stashing the fix and re-running:

• GivenInvoiceForKnownCustomer_IsProcessed — the core regression
• GivenDuplicateEventId_ReturnsDuplicateFlag — old code ignored the first delivery, so the second was never a duplicate
• GivenDeleteForSupersededSubscription_LeavesUserAlone — asserts the event is recorded, so it can't pass on an implementation that drops subscription events wholesale

The other four pass on both by design: they prove the fix didn't over-widen the filter (unknown customers and foreign app tags are still ignored, the metadata path still works, bad signatures still rejected).

Not covered: the re-fetch itself calls SubscriptionService directly, which would need the Stripe client injected to fake.

Follow-ups, not in this PR

• ProcessedWebhookEvent stores only EventId/ProcessedAt. Adding EventType would have made this self-diagnosing — worth doing, needs a migration on both providers.
• Bump Stripe.net, Stytch.net and Svix #693 (Stripe.net 50.3.0 → 52.4.1) is a separate problem: it crosses 2026-01-28.clover → 2026-08-26.dahlia, so Program.cs:29's startup assertion needs updating (its Backend CI is red on exactly that) and the dashboard webhook endpoint's API version needs flipping, or throwOnApiVersionMismatch: true rejects every delivery.

🤖 Generated with Claude Code

[paulvanbrenk]

The old filter guaranteed Data.Object had non-null Metadata before any handler ran. The known-customer branch removes that guarantee, and HandleCheckoutSessionCompleted still does an unguarded session.Metadata.TryGetValue("stytch_user_id", ...).

A checkout.session.completed whose payload has no metadata key at all, for a customer already in Users, now reaches that line and throws NullReferenceException -> the generic catch returns Results.Problem (500) -> Stripe retries the event forever and flags the endpoint as failing.

Verified locally by posting a signed checkout.session.completed with {"id":"cs_test1","object":"checkout.session","customer":"cus_known123"} for a seeded user: STATUS=InternalServerError, System.NullReferenceException in the log.

Fix: session.Metadata?.TryGetValue(...) != true in the handler.

[paulvanbrenk]

Confirmed and fixed. `session.Metadata is not { } metadata || !metadata.TryGetValue(...)` at the point of use.

Root cause is your altitude comment below: the old filter was doing double duty as an admission gate and an implicit null guarantee, and widening it silently dropped the second job. Regression test HandleWebhook_GivenSessionWithoutMetadata_IsHandledNotCrashed, verified by reverting just the null check.

[paulvanbrenk]

This early return is a silent no-op, but control still falls through to ProcessedWebhookEvents.Add(...) at line 106 — the event is permanently marked processed and any redelivery returns duplicate.

Before this PR that was harmless: subscription events never got past the filter, so they were never recorded. Now SubscriptionData.Metadata makes them pass, and Stripe routinely delivers customer.subscription.created/updated before checkout.session.completed — i.e. before user.StripeCustomerId has been stored. The event is consumed and can never be replayed.

Verified: a signed customer.subscription.updated tagged app=patchnotes for cus_brandnew, with a User row that has no StripeCustomerId, returns STATUS=OK BODY={"received":true} RECORDED=1 USERSUB=null USERSTATUS=null.

Two cheap fixes: (a) fall back to payload.Metadata["stytch_user_id"] — this PR now puts it on the subscription, so the user is resolvable; or (b) do not record the event when no handler actually applied it.

[paulvanbrenk]

Both, in the end.

(a) ResolveUserAsync falls back to stytch_user_id from subscription_data.metadata when the customer id is not stored yet — as you say, this PR now puts it there.

(b) Handlers return whether they applied, and only applied events are recorded. A row means "never process this again", which is not true of an event nothing acted on, and it is what makes replay from the dashboard possible.

Tests: GivenUnappliedEvent_IsNotRecordedSoItStaysReplayable and GivenAppliedEvent_IsRecorded.

[paulvanbrenk]

IsSupersededSubscription treats any id mismatch as "old", but the stored id is not a high-water mark — nothing ever clears it, HandleSubscriptionDeleted leaves it pointing at the cancelled subscription, and only HandleCheckoutSessionCompleted ever advances it.

So a genuinely new subscription is misclassified as superseded whenever its events land before checkout.session.completed: user cancels sub_A (status canceled, StripeSubscriptionId still sub_A), resubscribes as sub_B; the customer.subscription.updated for sub_B hits user.StripeSubscriptionId == "sub_A" != "sub_B" -> dropped, and recorded as processed so it can never be replayed. If the checkout event is itself dropped (no stytch_user_id, user not found), the account is stuck on the cancelled subscription permanently.

The ordering signal is in the event, not in our row: compare the payload/fetched Created against the stored subscription, or keep a per-user event watermark, rather than assuming "different id == older".

[paulvanbrenk]

Correct, and the failure mode is worse than stale-drop: it strands the account on the cancelled subscription.

I did not take the Created comparison — it needs the stored subscription fetched to compare against, and a watermark needs a column. The subscription's own status carries the same signal without either: a live subscription (active, trialing, past_due, unpaid, incomplete) is the one the customer is on, anything else is history. ShouldAdopt now adopts when the stored id is empty, the ids match, or the incoming subscription is live.

Your exact scenario is ShouldAdopt_GivenReplacementSubscriptionAfterCancellation_AdoptsIt.

[paulvanbrenk]

The guard is applied to two of the four handlers that write subscription state. HandlePaymentSucceeded (line ~298) resolves invoice.Parent.SubscriptionDetails.SubscriptionId, fetches it, and overwrites SubscriptionStatus and SubscriptionExpiresAt with no check that it is the subscription the user is actually on.

These invoice events were dropped entirely before this PR, so the asymmetry is new: a late or retried invoice.payment_succeeded for the replaced sub_A now rewrites SubscriptionExpiresAt back to sub_As period end while the user is paying on sub_B -> Pro expires early for a paying customer.

The same check (against invoiceSubscriptionId) belongs here.

[paulvanbrenk]

Fixed — IsForCurrentSubscription is applied in both invoice handlers.

You are right that the asymmetry is new, and it is the same mistake as the filter: a check that exists in one place reads as a check that exists. It now returns false for an invoice on a different subscription and for one with no subscription at all, which is not ours to act on either.

[paulvanbrenk]

IsKnownCustomerAsync admits every event about a stored customer, and HandlePaymentFailed (line ~279) then sets SubscriptionStatus = "past_due" for any failing invoice, with no check of which subscription it belongs to.

Concretely: user cancels sub_A leaving an open invoice, resubscribes as sub_B (active). Stripe smart retries keep hammering the sub_A invoice for up to ~3 weeks; each failure now flips the user to past_due. /api/subscription/status reports a dunning state to a customer in good standing, and once SubscriptionExpiresAt passes without a corrective customer.subscription.updated, User.IsPro goes false.

It should ignore invoices whose subscription is not user.StripeSubscriptionId (and ideally read the status off the subscription rather than hardcoding past_due).

[paulvanbrenk]

Fixed — HandlePaymentFailed now goes through IsForCurrentSubscription, so a retry against a replaced subscription is ignored instead of flipping a paying customer to past_due. Test GivenInvoiceForReplacedSubscription_IsIgnored, verified by short-circuiting the predicate.

I did not take the second half. Reading the status off the subscription would add a network call to the one handler that currently makes none, and with the identity check in place the literal only applies to the subscription the user is actually on. Happy to do it if you would rather have the real status — it does mean unpaid and incomplete stop being reported as past_due.

[paulvanbrenk]

This unconditionally overwrites the expiry, so when Items.Data comes back empty the line sets SubscriptionExpiresAt = null — the opposite of the comment above it. User.IsPro requires SubscriptionStatus == "canceled" && SubscriptionExpiresAt > UtcNow, so a null wipes the remaining paid period instantly.

The re-fetch added in this PR makes that reachable in a new way: the value now comes from a second, independent payload rather than the one that was signature-verified. Only overwrite when a period end was actually found:

Suggested change

	user.SubscriptionExpiresAt = subscription.Items.Data.FirstOrDefault()?.CurrentPeriodEnd;
	user.SubscriptionExpiresAt =
	subscription.Items.Data.FirstOrDefault()?.CurrentPeriodEnd ?? user.SubscriptionExpiresAt;

[paulvanbrenk]

Applied. Both writes now use ?? user.SubscriptionExpiresAt, via a shared ApplySubscription, and the delete handler does the same with the payload.

Your point about the re-fetch making it newly reachable is the part I had missed: the value stopped coming from the payload we verified. Test GivenDeleteWithNoPeriodEnd_KeepsTheExistingExpiry, verified by dropping the coalesce.

[paulvanbrenk]

For the deleted handler the re-fetch buys nothing: the status is hardcoded "canceled" on the next line, so the only thing read off the network is CurrentPeriodEnd, which the signature-verified payload already carries.

What it does buy is a new failure mode. GET /v1/subscriptions/{id} throwing (Stripe outage, rate limit, a subscription Stripe has since purged) is caught as StripeException -> 503 -> Stripe retries; until it succeeds the cancellation is never recorded. A cancellation that used to be processed offline now depends on Stripe being up.

The eventual-consistency argument in the doc comment applies to updated (where the status matters), not to deleted. Consider dropping the fetch here and using payload.Items.

[paulvanbrenk]

Agreed — the fetch is gone from the delete handler.

The argument I had written into the doc comment was about updated, where the status is genuinely in question, and I applied it to deleted without re-checking whether it held. It does not: the status is already known and the period end is in the signature-verified payload, so the only thing the fetch added was a dependency on Stripe being reachable for a cancellation to be recorded at all.

Kept for updated, dropped here.

[paulvanbrenk]

This test does not cover what its name and comment claim. The customer is KnownCustomerId, which line 91 just seeded — so the event passes the filter through IsKnownCustomerAsync, and the test goes green with HasAppMetadata deleted entirely. The metadata half of the new two-way filter has no coverage.

To make it bite, use a customer that is not in Users:

Suggested change

	"customer": "{{KnownCustomerId}}",
	"customer": "cus_notyetlinked",

[paulvanbrenk]

You are right, and it is worse than under-covering: the test asserted something it structurally could not observe, so it read as coverage while providing none.

The premise is gone with the positive filter, so I replaced it rather than repointing the customer. GivenForeignAppTag_IsIgnoredBeforeAnyHandler covers what is left of the metadata path, and GivenUntaggedInvoiceForCurrentSubscription_IsApplied covers the untagged case that the original bug was about.

[paulvanbrenk]

Altitude: the known-customer fallback widens the global filter rather than fixing ownership per event type, and it is a strictly weaker signal than the app tag — "this customer exists in our table" is not "this event is ours".

Every handler already resolves the user itself (FirstOrDefaultAsync(u => u.StripeCustomerId == ...) / by stytch_user_id) and already returns early when it finds none, so this pre-check duplicates the lookup and then hands unvetted events to handlers that assume they were vetted (see the null-metadata NRE and the past_due case). Deleting the filter and letting each handler own its own admission decision is both simpler and safer than adding a second, looser gate in front of it.

[paulvanbrenk]

Agreed, and this turned out to be the root of half the others — so the positive gate is gone.

The only global check left is the negative one: reject events explicitly tagged for another app. An absent tag is not evidence either way (invoices never carry one), so it is not treated as rejection. Everything else reaches a handler, and each handler resolves its own subject and returns false without writing when it cannot.

You were right that the pre-check duplicated the lookup, but the more expensive part was the false assurance: the NRE here and the past_due case below were both handlers trusting a gate that had stopped guaranteeing what they assumed.

[paulvanbrenk]

This test only stays offline while IsSupersededSubscription returns true. The moment that guard regresses, HandleSubscriptionDeleted reaches FetchCurrentAsync and the test makes a real HTTPS call to api.stripe.com with the fixtures sk_test_placeholder key — slow and network-dependent in CI, and the failure it reports is a 503 rather than the state assertion that actually regressed.

Since SubscriptionService is constructed inline and cannot be faked (as the PR description notes), consider at least pointing StripeConfiguration.ApiBase at a loopback address in the fixture so a regression fails fast and locally instead of dialling out.

[paulvanbrenk]

Good catch, and it applies more broadly than the one test — nothing in the suite should be able to dial out.

PatchNotesApiFixture now points the SDK at a closed loopback port via StripeConfiguration.StripeClient. It has to be set after the host is built: Program.cs assigns StripeConfiguration.ApiKey, and that discards any client set before it, so a static constructor would have been silently undone.

Verified by temporarily reintroducing a fetch into the delete handler — the request goes to 127.0.0.1:1 and is refused immediately, with no api.stripe.com in the log.

(ApiBase does not exist on StripeConfiguration in 50.x, so this goes through the StripeClient constructor.)

[paulvanbrenk]

Efficiency: this runs a Users query for every untagged event the endpoint receives, including the event types the switch below does not handle at all (charge.*, payment_intent.*, customer.subscription.created, ...). Ownership is only meaningful for the five handled types.

Moving the type check first — or gating this on stripeEvent.Type being one of the handled cases — removes a DB round-trip from the majority of deliveries.

[paulvanbrenk]

Resolved by the redesign rather than by reordering: IsKnownCustomerAsync is gone entirely, so there is no longer a Users query in the admission path at all.

What remains is IsTaggedForAnotherApp, which is pure. An unhandled type now reaches the switch, hits default, and returns without touching the database — so the round-trip you describe is removed for every delivery, not just the unhandled ones.

[paulvanbrenk]

Reuse: this is now the third copy of new SubscriptionService().GetAsync(id) in the file — HandleCheckoutSessionCompleted (line ~211) and HandlePaymentSucceeded (line ~301) still hand-roll it. The helper only uses payload.Id, so taking a string subscriptionId instead of a Subscription would let both of those call it too, and would give a single place to inject a fake service later.

[paulvanbrenk]

Done. FetchCurrentAsync takes a string subscriptionId now, and the checkout and payment-succeeded handlers both call it instead of constructing their own service.

The single injection point is the more useful half of this — it is where a fake would go once SubscriptionService stops being constructed inline.

[paulvanbrenk]

Simplification: after this PR HandleSubscriptionUpdated and HandleSubscriptionDeleted are the same nine lines — same is not Subscription payload guard, same user lookup, same warning, same superseded check, same re-fetch, same Items.Data.FirstOrDefault()?.CurrentPeriodEnd. Only the status assignment differs (subscription.Status vs the literal "canceled").

One method taking the status to write (or Func<Subscription, string>) removes the copy and stops the two drifting — which is exactly how HandlePaymentSucceeded/HandlePaymentFailed ended up without the new guard.

[paulvanbrenk]

The premise held when you wrote it, but the other fixes have pulled the two apart: deleted no longer re-fetches (your comment below), matches on subscription id rather than status, and writes the literal from the payload, while updated fetches, runs ShouldAdopt, and may backfill the customer id. Folding them together now would mean a method with a flag selecting which half to run.

Your underlying point is the one that mattered, and I took it a different way: the shared pieces are extracted as ApplySubscription, IsForCurrentSubscription and ResolveUserAsync, so the invariants live in one place each. That is what stops the PaymentSucceeded/PaymentFailed drift recurring — it was a shared rule going unenforced, not shared lines going uncopied.

[paulvanbrenk]

Cleanup: async + Task.FromResult(...FirstOrDefault(...)) is a synchronous query wearing an async signature. FirstOrDefaultAsync is already used everywhere else in the suite.

Suggested change

	db.Users.FirstOrDefault(u => u.StripeCustomerId == customerId));
	return await db.Users.FirstOrDefaultAsync(u => u.StripeCustomerId == customerId);

[paulvanbrenk]

Already gone — the helper was rewritten in this round and uses FirstOrDefaultAsync directly.

[paulvanbrenk]

Cleanup: this dictionary is a verbatim copy of Metadata eight lines up, and the whole point of the PR is that the two tags must agree. Building it once and assigning it to both makes drift impossible:

var appMetadata = new Dictionary<string, string>
{
    { "stytch_user_id", stytchUserId },
    { "app", "patchnotes" },
};
// ... Metadata = appMetadata, SubscriptionData = new() { Metadata = appMetadata },

[paulvanbrenk]

Applied, with the dictionary built once above sessionOptions and assigned to both. Your reasoning is the reason it is worth doing: the tags agreeing is the whole point of the PR, so the copy was the one place drift would have been silent.