fix(billing): derive effective plan, record churn, add webhook idempotency - #98
Merged
Merged
Conversation
…tency Migration 0014 plus the admin query rewrite behind it. Status vocabulary platform-api's webhook wrote status='cancelled' (en-GB); every reader compares against 'canceled' (en-US, the spelling 0006 documents). Since 'cancelled' != 'canceled' is TRUE, all five `status != 'canceled'` filters were silent no-ops and canceled subscriptions were treated as live. 0014 normalises stored rows and adds a CHECK constraint so the two spellings cannot diverge again. The writer is fixed separately in platform-api. Effective plan Subscribing UPGRADES the existing free row in place, so a canceled row leaves no free row to fall back to -- the admin list rendered a churned user as "payg". plan_name is now derived: 'free' when the latest subscription is canceled, with the paid plan preserved separately as previous_plan_name alongside canceled_at, so churned-but-previously-paid users stay identifiable for win-back campaigns. Filters list/count now share one predicate set and accept plan, auth_method, status and previously_paid, plus sort by created/last_active/email (fixed ORDER BY strings, no caller input interpolated). Both correlated subqueries are replaced by a single LATERAL, and the token count moves to its own LATERAL, dropping GROUP BY entirely. Also adds the first indexes on subscriptions (user_id, status). Verified against Postgres 16: all 12 migrations apply clean, and a seeded churned/active/free trio returns the expected effective plans and filter results. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
marvin-agent-rockflow
approved these changes
Jul 25, 2026
marvin-agent-rockflow
left a comment
Collaborator
There was a problem hiding this comment.
Hermes Agent Review
Head: c1cd57e · Files: 3 · +240 / -55
Verdict: Approve
Solid root-cause fix for the en-GB/en-US status split, with the right supporting schema (CHECK, canceled_at, stripe_events, indexes) and a clean admin-query rewrite.
Critical
- None.
Warnings
- None blocking. Deploy order called out in the PR body is mandatory: this migration must land before platform-api starts writing
canceled_at/ the constrained status set (paired with joaoh82/rustunnel-web#53).
Suggestions
- Dynamic SQL via
format!: Safe today becauseAdminUserSort::order_by()returns only&'static strliterals and filters are bound params — good. A one-line comment nearlist_admin_usersnoting "ORDER BY is never caller-controlled" would lock that invariant for future readers. previously_paid/ effective plan:billing_model <> 'free'correctly excludes never-paid free rows. If a free-plan row can ever sit instatus = 'canceled'(legacy/data quirks), it will correctly stay out of the win-back list — worth a quick assert in the seeded verification set.- Multi-sub edge: LATERAL picks latest
subscriptionsbycreated_at DESC. Fine if subscribe upgrades in place; if a user can ever accumulate multiple live rows, document that "latest wins" is intentional. - No automated regression test for the spelling/
CHECKconstraint — the manual PG 16 matrix in the PR body is convincing; a tiny SQL test or migration smoke would guard against a future writer reintroducing'cancelled'.
Looks good
- Migration 0014: normalize
cancelled→canceled, CHECK vocabulary,canceled_atbackfill,stripe_eventsPK for idempotency, first indexes onsubscriptions. - Effective plan derivation (
canceled→free+previous_plan_name) matches the in-place upgrade model so churned users no longer render as paid. - Filter/sort API: bind parameters only; unknown
sortfalls back to default rather than 500. - Correlated subqueries → single LATERAL + token LATERAL; drops
GROUP BY; list/count share the same predicate set. AdminUserfields align with the admin-dashboard consumer PR.
Automated hourly review by marvin-agent-rockflow (Hermes). Will re-review only if new commits land.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deploy this first. platform-api's changes write
canceled_atand must satisfy the newCHECKconstraint, both added here.Why
A paying customer canceled on 29 June. The admin dashboard still listed them as
payg, and nothing anywhere told us they'd gone. Root cause turned out to be a spelling mismatch.platform-api's webhook wrote
status = 'cancelled'(en-GB) — the only place that spelling was ever written. Five queries filter with!= 'canceled'(en-US, the spelling migration 0006 documents). Since'cancelled' != 'canceled'is TRUE, the canceled row passed every filter.All five
!= 'canceled'filters were silent no-ops. Canceled subscriptions were treated as live everywhere they were read.What's here
Migration 0014
canceled, plus aCHECKconstraint so the two spellings can't diverge againsubscriptions.canceled_at(nothing recorded when a subscription ended — the cancel webhook didn't even touchupdated_at), backfilled fromcurrent_period_endstripe_eventsfor webhook idempotency (consumed by the platform-api PR)subscriptions— it had noneEffective plan. Fixing the spelling alone would have made it worse: the row would then be excluded,
plan_namewould goNULL, and the list would render—. Subscribing UPGRADES the free row in place, so a canceled row leaves no free row to fall back to.plan_nameis now derived (freewhen canceled) withprevious_plan_name+canceled_atpreserved separately, so churned-but-previously-paid users stay identifiable for win-back.Filters.
plan,auth_method,status,previously_paid, and sort bycreated/last_active/email(fixed ORDER BY strings — no caller input interpolated). Both correlated subqueries collapse into one LATERAL; the token count moves to its own, droppingGROUP BYentirely.Verification
Against Postgres 16 — all 12 migrations apply clean, then with seeded churned/active/free users:
freepaygfreepaygEvery filter combination returns the expected set;
previously_paidreturns exactly the churned user.UPDATE ... SET status='cancelled'is now rejected by the constraint.cargo check,fmt --check,clippy -D warningsall pass.🤖 Generated with Claude Code