feat(db): deletion + subscription-lineage data model (slice 1/5 of #374) - #397
feat(db): deletion + subscription-lineage data model (slice 1/5 of #374)#397lourou wants to merge 1 commit into
Conversation
… 1/5 of #374) Additive data-model tier extracted verbatim from feature/delete-account-impl (#374): all six Prisma migrations (renumbered 20260729* so they sort after the applied otr-dev migration tip, relative order preserved), the full schema.prisma, the three schema-level test files, and a minimal tests/deletion/reclaim-fixtures.ts subset that later slices expand to the full shared fixture module. Zero behavior change: no src/ modifications.
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @lourou's task —— View job Code Review CompleteI've reviewed this PR slice thoroughly. This is an exceptionally well-engineered data model change with excellent quality across all dimensions. Below is my detailed feedback. SummaryOverall Assessment: Approve with minor observations This PR demonstrates production-grade database engineering. The migrations are additive-only, rolling-deploy-safe, include comprehensive backfills, and enforce money-conservation invariants at the DB layer. The test coverage is thorough and validates the highest-risk piece (lineage backfill). No blocking issues found. ✅ Strengths1. Excellent Migration Design
2. Money-Conservation ConstraintsThe CHECK constraints in
These align perfectly with the ledger law documented in 3. Lineage Backfill LogicThe Google token-chain root canonicalization (lines 159-243 in
4. Test Coverage
The 🔍 Code Quality ObservationsMigration 1 (
|
| -- Backfill: existing accounts start their activity clock at migration time. | ||
| -- A null lastAuthAt must never read as "inactive/no veto"; after this | ||
| -- backfill, null only ever means a brand-new account that has not minted yet. | ||
| UPDATE "Account" SET "lastAuthAt" = CURRENT_TIMESTAMP WHERE "lastAuthAt" IS NULL; |
There was a problem hiding this comment.
🟡 Medium 20260729100000_add_account_deletion/migration.sql:80
The lastAuthAt column is added as nullable with no database default, and the backfill UPDATE only runs once at migration time. Accounts that mint tokens using the currently deployed application after this migration completes but before the mint-stamping code is deployed will never get lastAuthAt set — it stays NULL permanently. This violates the migration's own stated invariant that NULL only means a brand-new account that has never minted, and causes those active accounts to be treated as having no recent-auth veto by the later claim logic. Consider adding a DEFAULT CURRENT_TIMESTAMP on the column (or a trigger) so rows minted by the old code before the writer deploys still get a non-null value, or run a second backfill when the stamping code rolls out.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @prisma/migrations/20260729100000_add_account_deletion/migration.sql around line 80:
The `lastAuthAt` column is added as nullable with no database default, and the backfill `UPDATE` only runs once at migration time. Accounts that mint tokens using the currently deployed application *after* this migration completes but *before* the mint-stamping code is deployed will never get `lastAuthAt` set — it stays `NULL` permanently. This violates the migration's own stated invariant that `NULL` only means a brand-new account that has never minted, and causes those active accounts to be treated as having no recent-auth veto by the later claim logic. Consider adding a `DEFAULT CURRENT_TIMESTAMP` on the column (or a trigger) so rows minted by the old code before the writer deploys still get a non-null value, or run a second backfill when the stamping code rolls out.
| -- transaction's start timestamp. | ||
| NEW."committedAt" := clock_timestamp(); | ||
| END IF; | ||
| ELSIF NEW.status = 'committed' THEN |
There was a problem hiding this comment.
🟡 Medium 20260729104000_harden_reconciliation_drift_cursor/migration.sql:23
The ELSIF NEW.status = 'committed' branch restamps committedAt on every update whose SET list includes status or committedAt, even when the row was already committed. Because the trigger is UPDATE OF status, "committedAt", PostgreSQL fires it whenever either column appears in the SET list regardless of whether the value changed. So an idempotent UPDATE ... SET status = 'committed' on an already-committed row overwrites committedAt with a fresh clock_timestamp(), which downstream scheduling treats as a new commitment — reopening drift monitoring, resetting retry/operator state, and extending monitorUntil. Repeated idempotent writes can keep a lineage under drift monitoring indefinitely. Guard the branch with OLD.status IS DISTINCT FROM 'committed' so only the first transition into committed is stamped.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @prisma/migrations/20260729104000_harden_reconciliation_drift_cursor/migration.sql around line 23:
The `ELSIF NEW.status = 'committed'` branch restamps `committedAt` on every update whose `SET` list includes `status` or `committedAt`, even when the row was already committed. Because the trigger is `UPDATE OF status, "committedAt"`, PostgreSQL fires it whenever either column appears in the `SET` list regardless of whether the value changed. So an idempotent `UPDATE ... SET status = 'committed'` on an already-committed row overwrites `committedAt` with a fresh `clock_timestamp()`, which downstream scheduling treats as a new commitment — reopening drift monitoring, resetting retry/operator state, and extending `monitorUntil`. Repeated idempotent writes can keep a lineage under drift monitoring indefinitely. Guard the branch with `OLD.status IS DISTINCT FROM 'committed'` so only the first transition into `committed` is stamped.
| -- transaction's start timestamp. | ||
| NEW."committedAt" := clock_timestamp(); | ||
| END IF; | ||
| ELSIF NEW.status = 'committed' THEN |
There was a problem hiding this comment.
🟡 Medium 20260729104000_harden_reconciliation_drift_cursor/migration.sql:23
The ELSIF NEW.status = 'committed' branch restamps committedAt on every update whose SET list includes status or committedAt, even when the row was already committed. Because the trigger is UPDATE OF status, "committedAt", PostgreSQL fires it whenever either column appears in the SET list regardless of whether the value changed. So an idempotent UPDATE ... SET status = 'committed' on an already-committed row overwrites committedAt with a fresh clock_timestamp(), which downstream scheduling treats as a new commitment — reopening drift monitoring, resetting retry/operator state, and extending monitorUntil. Repeated idempotent writes can keep a lineage under drift monitoring indefinitely. Guard the branch with OLD.status IS DISTINCT FROM 'committed' so only the first transition into committed is stamped.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @prisma/migrations/20260729104000_harden_reconciliation_drift_cursor/migration.sql around line 23:
The `ELSIF NEW.status = 'committed'` branch restamps `committedAt` on every update whose `SET` list includes `status` or `committedAt`, even when the row was already committed. Because the trigger is `UPDATE OF status, "committedAt"`, PostgreSQL fires it whenever either column appears in the `SET` list regardless of whether the value changed. So an idempotent `UPDATE ... SET status = 'committed'` on an already-committed row overwrites `committedAt` with a fresh `clock_timestamp()`, which downstream scheduling treats as a new commitment — reopening drift monitoring, resetting retry/operator state, and extending `monitorUntil`. Repeated idempotent writes can keep a lineage under drift monitoring indefinitely. Guard the branch with `OLD.status IS DISTINCT FROM 'committed'` so only the first transition into `committed` is stamped.
ApprovabilityVerdict: Needs human review 2 blocking correctness issues found. This PR introduces substantial new database schema for account deletion and subscription lineage tracking, including billing-related tables (custody, transfers, credits). There are unresolved review comments identifying potential bugs in the migration's backfill gap and trigger restamping behavior. Changes to billing infrastructure warrant human review. You can customize Macroscope's approvability policy. Learn more. |
What this is
Slice 1 of 5 of #374 (
feature/delete-account-impl), re-landed as a reviewable series along its test-file seams. #374 is +13.5k/-0.6k across 92 files after nine adversarial review rounds; this series splits it into independently green, independently revertable tiers. Content is extracted verbatim from the #374 tip (45cd889) — nothing was re-designed.This slice: the complete additive data-model tier (zero behavior)
20260729100000–20260729105000so they sort after the applied otr-dev migration tip (20260715140320_*); relative order preserved, including the rolling-deploy-safe reconciliation ordering (thecommittedAtmigration beforeSubscriptionDriftSchedule).add_account_deletion:DeletedIdentity,DeletionRecord,DeletionTask,SubscriptionTombstone,Account.lastAuthAtadd_subscription_lineage:SubscriptionLineage,LineageTokenAlias,LineagePeriodGrant,LineagePeriodCustody,SubscriptionTransfer,LineageQuarantine,Subscription.lineageId, money-conservation CHECK constraints, and the backfill of lineages from existing Subscription rowsadd_rate_limit_counter,add_reconciliation_progress,harden_reconciliation_drift_cursor,add_subscription_drift_scheduleprisma/schema.prismafrom the feat: account deletion (barrier, teardown, tombstones) + subscription reclaim #374 tip (byte-identical).tests/deletion/schema.test.ts,tests/deletion/schema-guards.test.ts, andtests/deletion/lineage-backfill-migration.test.ts(only the migration-folder references updated for the renumbering).tests/deletion/reclaim-fixtures.ts: minimal subset of feat: account deletion (barrier, teardown, tombstones) + subscription reclaim #374's shared fixture module (only what the backfill test imports); later slices expand it to the full version.No
src/file is touched. All new tables/columns are additive and unused by shipped code paths until the later slices land. No client-facing request schema changes (nothing forassertLegacyShapeValidatesyet — that lands with the verify/tombstone slice). No credit movement of any kind — DDL only, and the CHECK constraints strengthen the ledger law at the DB layer.Why land the schema first
The lineage backfill is the highest-risk piece of #374 (it adjudicates existing prod
Subscriptionrows). Isolating it on a small diff means prod data adjudication happens here, reviewable on its own, and every later slice becomes a pure-code diff with zero migration churn.Prod-apply note: the lineage migration hard-fails the deploy (by design,
DO $$guard) if any single lineage's rows are owned by different accounts. Before prod promote, run the read-only finder to confirm no multi-owner lineage exists in prod data (Apple: duplicateoriginalTransactionIdacross accounts; Google: token-chain roots).The series
barrier-mint,auth-require-account,router-fencingtests)ACCOUNT_DELETION_ENABLED=false(delete-account,outbox*,executorstests)claimableon the 409 (contract-pinned withassertLegacyShapeValidates) (tombstonestests)claim,adversarial*, fullreclaim-fixtures)Validation
prisma validate/prisma generateclean; full 76-migration chain replayed from scratch on an empty DB (migrate deploy), thenmigrate statusup to datetsc --noEmit,eslint,prettier --checkall cleanNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Note
Add account deletion and subscription lineage data model (slice 1/5 of #374)
DeletedIdentity,DeletionRecord,DeletionTask), subscription lineage tables (SubscriptionLineage,LineageTokenAlias,LineagePeriodGrant,LineagePeriodCustody,SubscriptionTransfer,SubscriptionDriftSchedule,LineageQuarantine), and aRateLimitCountertable.Account.lastAuthAtfor existing rows and creates Apple/Google lineages from existing subscription data via a recursive CTE that resolveslinkedPurchaseTokenchains; ambiguous or cyclic chains are quarantined.SubscriptionTransfer.committedAtis database-owned via a trigger, guaranteed non-null, and indexed for deterministic sweep selection.SubscriptionDriftScheduleis auto-maintained by a trigger onSubscriptionTransfercommits.RAISE EXCEPTIONguards that abort if different-account duplicate lineages are detected; these are irreversible once applied.📊 Macroscope summarized 11e09e5. 7 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.