This repository was archived by the owner on Aug 12, 2026. It is now read-only.
feat: account deletion (barrier, teardown, tombstones) + subscription reclaim - #374
Open
lourou wants to merge 50 commits into
Open
feat: account deletion (barrier, teardown, tombstones) + subscription reclaim#374lourou wants to merge 50 commits into
lourou wants to merge 50 commits into
Conversation
Plan for an authenticated account-deletion endpoint: a durable deletion barrier at SIWE token mint (auto-provisioning currently recreates deleted accounts), transactional RESTRICT-ordered teardown with a direct ClientIdentifier sweep, pseudonymized financial-record retention, provider-key billing tombstones across webhooks and verify, async external purges (S3, notification server, Composio, analytics) behind a transactional outbox with a purge SLA, idempotency via a client operation id, and open decision points. Companion client plan lives in convos-ios docs/plans.
Resolutions from the PR 358 review triage: - idempotent-retry auth carve-out for the deletion route, so repeat calls are not bounced by fail-closed requireAccount - concurrent-writer fencing: barrier covers every account-attaching writer, Account row lock, final locked sweep feeding the purge outbox - Option A transfer requires an explicit one-time ownership claim on top of the tombstone gate - teardown routes UserCredits/CreditLedger deletes through the ledger module per src/payments/AGENTS.md
… shared lock Closes the TOCTOU macroscope flagged: a writer could pass the barrier check before the deletion transaction commits and attach a no-FK row after the sweep. Writers now take SELECT ... FOR KEY SHARE on the Account row inside their own transaction via a shared requireLiveAccount helper; the deletion transaction locks the row FOR UPDATE as its first statement, not via the final DELETE, because the sweep runs before the Account row is deleted and would otherwise stay racy. Barrier plus final sweep remain as defense in depth.
…odels Additive migration for account deletion: DeletedIdentity (permanent keyed-hash barrier), DeletionRecord (operationId-keyed idempotency record), DeletionTask (transactional purge outbox), and SubscriptionTombstone (provider-key billing tombstone), plus an Account.lastAuthAt activity stamp written at token mint. No behavior change yet; consumers land in follow-up commits.
…ve-account fencing The deletion barrier (DeletedIdentity, HMAC keyed by the new required DELETION_HASH_SECRET) is consulted at POST /v2/auth/token after full SIWE validation: a barred identity gets the terminal 410 identity_deleted response and never re-provisions an account or signup bonus. Successful SIWE mints stamp Account.lastAuthAt (activity recency; carries no gating semantics on its own). requireAccount now fails closed: the account row must exist, a deleted account holding an unexpired token gets a generic 401 (the mint 410 stays the only deletion-confirmation channel). No caching - every check hits the database. requireLiveAccount (SELECT ... FOR KEY SHARE on the Account row) fences the FK-less account-linked writers - the ClientIdentifier upsert in notifications subscribe and AdminAudit inserts - against a concurrent deletion's FOR UPDATE lock. FK-backed writers already get the same lock implicitly from referential integrity. The Composio link-completion fence is the fail-closed requireAccount on the /v2/connections routes.
…hooks Consult SubscriptionTombstone before any subscription state can attach to a provider key that belonged to a deleted account: - verify with no live row for a tombstoned key (or its Play rotation predecessor) returns the existing 409 subscription_account_mismatch envelope with the additive claimable: true field - no row created, no entitlement. A live row for the key always wins over a tombstone so a future claim re-home keeps verifying normally. - ownership-mismatch 409s carry claimable (informative only, evaluated by the shared claim-eligibility module; live-owner transfer stays disabled until the claim endpoint ships). - Apple S2S and Play RTDN ack tombstoned keys as counted no-ops (subscription.ssn.tombstoned_noop / play.rtdn.tombstoned_noop) and absorb Play token rotation onto the tombstone set instead of letting it escape. - a deletion racing an in-flight notification converges to the same no-op: FK/missing-row failures (P2003/P2025) re-check the tombstones instead of bubbling a 500. Tombstone rows are written by the delete-account teardown, which lands next - ordering the commits this way means no point in history has an active delete path without tombstone-aware verify/webhooks.
The teardown runs in one transaction whose first statement locks the
Account row FOR UPDATE - the serialization point for every concurrent
account-linked writer. Children go before parents: billing receipts,
subscriptions (converted to provider-key tombstones), the wallet and
ledger (via the new deleteWalletForAccountWithTx helper inside the
payments module, preserving the single-writer law), builder templates
and generations, device registrations, the direct ClientIdentifier
accountId sweep, and auth methods (each erecting a permanent barrier
row); Account goes last, cascading connection grants. Entitled periods
are forfeited before the wallet is removed.
The transaction also writes the durable DeletionRecord (operationId-
keyed), snapshots the external-purge outbox (notification-server
installations, S3 avatars and build attachments, Composio user,
PostHog person), and records a retained AdminAudit entry under a
sentinel account id with the keyed accountRef.
Response contract: 200 {status, operationId, deletedAt,
purgeWindowHours: 24}. Replays - same or different operationId, via
the endpoint-specific carve-out that accepts an unexpired pre-deletion
token for record lookup only - return the stored record. 5/15min rate
limits per IP and per account.
Lock-order law: applyDeltaWithTx, the verify/notification transactions,
and the device backfill now take their Account lock (requireLiveAccount
or FK KEY SHARE) before any other row lock, so the teardown's
Account-first ordering is deadlock-free; racing ledger writes surface
as AccountNotLiveError, mapped to the routes' existing account-gone
responses.
A per-minute sweep (same setInterval lifecycle as the generation and telemetry sweeps) drains pending DeletionTasks with exponential backoff (30s base, 1h cap, 10 attempts then terminal failed + deletion.task.terminal_failure operator alert), completes DeletionRecords whose tasks all finished (stamping a 30-day record expiry), alerts on records still purging past the 24h window (deletion.purge.sla_breach), and removes expired records with their task rows. Executors, one per task kind, all idempotent: - s3_object: public avatars (key derived from the stored URL) and private build attachments, via DeleteObjectCommand; - notification_installation: notification-server delete-installation per snapshotted ClientIdentifier; - composio_user: post-commit re-discovery via list-for-user, deleting every returned connection and re-listing until empty (grant-less connections are only discoverable remotely); - posthog_person: person deletion through the PostHog private API (new optional POSTHOG_PERSONAL_API_KEY / POSTHOG_PROJECT_ID envs); when analytics is active but the credentials are missing the task retries and pages ops instead of silently skipping.
…oint Implements the reclaim v3 design (lineage/custody model, bounded bearer-transfer, contest window, one-shot undo). Model: SubscriptionLineage is the canonical first lock for verify, webhooks, claims, and deletion (global order: lineage -> accounts sorted -> subscription -> wallets sorted; documented in src/subscriptions/AGENTS.md). Google token chains resolve recursively with loop detection into LineageTokenAlias; conflicting chains quarantine, never auto-merge. LineagePeriodGrant is the global once-per-funding-event registry (apple_txn_<id> / play_order_<id> keys, new-period gate so mid-period upgrades never double-fund), and LineagePeriodCustody tracks each funded period's remaining value with conservative moves: D = min(lockedBalance, max(0, cap - consumesSince)) then cap := D, so promo/admin credits never move and no chain of operations exceeds one allotment. Tombstones are now lineage state; the SubscriptionTombstone table is dropped and its consumers rewritten. Deletion teardown escrows the custody remainder (journaled) and flips lineages to tombstoned; expiry/refund/revoke compensate the current custody holder (correct post-transfer, where account-scoped sub_grant discovery finds nothing); Play voided purchases now compensate instead of being discarded; renewals while tombstoned fund escrow directly. POST /v2/accounts/me/subscription/claim: fail-closed requireAccount + mandatory consumed App Check attestation (single 403 app_check_required, no app_attest_enabled bypass), authoritative entitled-now + latest-transaction proof (no signedDate window), per-IP/per-account/ global rate limits. Tombstone restoration releases the escrow (never a second grant). Live transfers sit behind SUBSCRIPTION_CLAIM_LIVE_TRANSFER_ENABLED (off at launch) with a 72h contest window (202 pending + push notification to the old account's devices; authenticated old-account activity vetoes at settlement), 30-day per-lineage cooldown, and a one-shot CAS undo for the previous owner that executes immediately and freezes further automated transfers. DELETE /v2/accounts/me gains the account_deletion_enabled runtime-config kill switch.
…ments Release-blocker test coverage for the claim design: App Check fails closed with app_attest_enabled=false and rejects replayed limited-use tokens; the same JWS claimed concurrently for two accounts commits exactly one transfer with total credits conserved; undo after attacker spend returns only the unspent remainder and is one-shot (undo_consumed); refunds after a transfer compensate the current custody holder; renewals while tombstoned fund escrow once and restoration releases it once; voided purchases while tombstoned invalidate escrow without wallet moves; Google T1->T2->T3 chains resolve to one lineage with conflicting chains quarantined; mid-period upgrades never double-fund; deletion racing verify converges with no recreated state. Schema guards: RESTRICT still bites at the DB layer, and an inventory test fails when a new account-correlatable model is not accounted for in the teardown/retained/ownerless sets. Spec doc amended to the as-built design: the tombstone-only transfer section is superseded by the lineage claim design (deviation called out), and the decided cross-repo contract shapes and retention/flag defaults are recorded.
…invariants - Keep the SubscriptionTombstone table (and its Prisma model): lineage state supersedes it, but dropping the relation broke rollback safety. - Unique index on Subscription.lineageId (one live row per lineage) with a defensive dedupe, so claim/webhook lookups are deterministic. - CHECK constraints on custody/transfer/lineage state vocabularies, non-negative caps and conserved credits, and escrow-iff-ownerless. - Backfill Account.lastAuthAt to migration time (null must never read as "inactive/no veto"). - New RateLimitCounter table backing shared-store rate limits.
…resolution, lock-safe custody moves Google period accounting derives from the renewal order identity, never the lifetime startTime: the new-period gate compares window ends, the per-account ledger key for Google grants comes from play_order_<id>, and custody windows clamp their start to the previous period end. Renewals with an unchanged startTime now fund; a missing latestOrderId fails closed (event parked in LineageQuarantine, no synthesized key) on both verify and RTDN. Lineage creation is a single atomic insert-or-adopt transaction with bounded restarts; chain loops, depth overflow, and cross-lineage conflicts quarantine instead of silently adopting a truncated root, and verify/webhooks resolve the full chain like claim does. Webhook applies re-read the subscription under the lineage lock before any staleness/renewal decision; the tombstone probe re-checks lineage state under the lock (retrying on the live path after a restoration), invalidates escrow custody on terminal events while tombstoned, and funds tombstoned renewals against an end-gated window. Terminal events and Play voided purchases target the exact funding-key custody row, so a late void claws only its own period. Custody transfers prelock both wallets in sorted account order, claims lock the subscription row before wallets, and every multi-lock money transaction retries bounded on 40P01/40001. The legacy custody bootstrap now writes its funding-registry row. Pending-transfer settlement hardening: both accounts locked FOR UPDATE in sorted order with the veto's lastAuthAt read under that lock, a null lastAuthAt treated as a veto, and an execution-time provider entitlement recheck (injectable; provider-unreachable defers the row) before any stored transfer executes.
…ute, default deletion off The auto-provisioning upsert now takes a per-identity advisory lock and re-checks the DeletedIdentity barrier inside its own transaction; the teardown takes the same lock before barring/deleting the identity, so a mint racing a delete can never re-create a permanently deleted account (surfaces as IdentityBarredError -> 410 identity_deleted). JWT authentication itself now enforces the deletion fence: any token carrying an accountId claim is only honored while the Account row exists (generic 401 otherwise), with DELETE /v2/accounts/me as the single carve-out for idempotent deletion-record replays. This closes every route registration that skipped requireAccount (/auth-check, invite redemption, attachment presigning, assets, notifications, ...). Live authenticated requests also stamp lastAuthAt (throttled), so the claim contest window's veto covers any authenticated act, not only mints. account_deletion_enabled now defaults to false: deletion stays off until ops flips the RuntimeConfig after migrations and a full rollout.
… shared-store global limiter - claimAppCheckMiddleware reads app_attest_enabled directly: when the flag is false the route is OFF — even a valid token is rejected with 403 app_check_required (no appCheckOnlyMiddleware bypass semantics). - The pending-transfer notifier now sends the contract's SubscriptionClaimPending push (new NotificationType + payload) to the old owner's registered devices through the existing APNs/FCM services, carrying contestEndsAt and the provider. - The global claims-per-hour ceiling moves to a Postgres-backed express-rate-limit store (RateLimitCounter) so it holds across replicas; per-IP/per-account limiters stay in-process.
… replay-flake diagnostics New adversarial coverage for the invariants the round-3 review found untested (each maps to a fixed defect): - app_attest_enabled=false rejects a VALID limited-use token (flag wins over the verifier). - Google renewals with unchanged startTime and a new latestOrderId fund on both webhook and verify paths; same-order replays and same-window upgrades fund nothing. - Keyless Google events (no latestOrderId) fail closed on verify (502) and RTDN (parked ack), each leaving a quarantine row. - A refund of a tombstoned renewal invalidates exactly that period's escrow; the deletion escrow for the earlier period is untouched. - A real concurrent undo race commits exactly one undo journal row, and the undo row itself is never an undo target. - Contest settlement cancels when the provider revoked entitlement inside the window, defers when the provider is unreachable, and treats a null lastAuthAt as a veto. - withDeadlockRetry unit coverage (bounded retry on 40P01/40001/P2034, immediate rethrow otherwise) plus opposite-direction transfers across two lineages converging under the sorted wallet prelock. - Cumulative custody-cap invariant across transfer -> spend -> undo -> delete -> restore (cap monotone non-increasing, one registry row, one escrow release, total movement bounded by one allotment). - Mint-vs-delete: the upsert observes the barrier inside its own transaction, and a racing mint can never re-create the account. - Google chain loops and depth overflow quarantine; concurrent first resolution of overlapping chains converges on one lineage. Router fencing audit (tests/deletion/router-fencing.test.ts): a source audit pins verifyJwtToken call sites to the fenced middlewares, and the REAL /v2 router is probed with a deleted account's unexpired JWT across every surface the review named (all generic 401), including the DELETE /v2/accounts/me replay carve-out and a live-account control. The delete-replay tests now attach response bodies and deletion-record state to their assertions so the rare flake, if it recurs, retains the actual failure. Agent-template read tests create their synthetic reader accounts (fail-closed auth requires the accountId claim to reference a live row).
…, Google provider gate Tombstone restoration now releases ONLY the escrow row whose providerPeriodKey matches the provider-verified current funding event (apple_txn_<latest tx> / play_order_<latestOrderId>), carried through the claim proof into executeClaim. Window-covering selection is gone from the provider-keyed path: Google reports the lifetime startTime as the period start, so an expired period's escrow could cover that timestamp while the current period's escrow stayed stranded. The covering fallback survives only for legacy_ custody bootstrapped from pre-lineage periods, which no provider event can name. A Google claim with no latestOrderId now fails closed like verify/RTDN: parked in LineageQuarantine and rejected 409 lineage_unresolved (retryable) before executeClaim — a keyless restoration was the direct enabler of the wrong-escrow release. New provider scope flag SUBSCRIPTION_CLAIM_GOOGLE_ENABLED (default false): the product is Apple-only today, so googlePlay claim bodies are rejected with contract not-claimable semantics (409 transfer_frozen) before any provider call, and verify's claimable signal stays false for Google lineages. Verify/RTDN ingest and Google money accounting stay fully on. Test fixtures now fund restoration scenarios under the same transactionId the claim presents as Apple's latest (as in production when no renewal intervened), matching the exact-key selection.
… void fallback absorbTombstoneRotation no longer performs a bare alias upsert (which silently no-opped when the presented token already belonged to another lineage, letting the event mutate the wrong one). It routes through the atomic conflict-detecting lineage resolver: a token resolving elsewhere, a chain conflict, loop, or depth overflow quarantines and returns "conflict" — and the tombstone probe then acks the event WITHOUT any funding or invalidation effect (resolution strictly precedes money effects; the reconciliation sweep owns the parked row). compensateVoidedPurchase fails closed on unmatched orders: a void with no orderId, or whose exact play_order_<orderId> custody row is absent (pre-lineage legacy period, unseen order), proves nothing about the current period — it is parked in LineageQuarantine (voided_purchase_keyless / voided_purchase_unmatched_order) instead of revoking current entitlement or clawing covering-now custody. The RTDN handler logs parked voids at error level for ops visibility.
…l-closed global ceiling The lastAuthAt activity stamp is now reliable exactly when the contest veto depends on it: the write is awaited before the authenticated handler proceeds (no fire-and-forget landing after settlement's locked read), the timestamp is database now() — the same clock as the pending row's createdAt, closing app/DB clock-skew inversions — and the 5-minute throttle is bypassed whenever the account has a pending outgoing transfer, so no authenticated victim act inside the window can ever go unrecorded. The mint-path stamp uses database now() too. The delete-replay carve-out matches the exact route (DELETE /api/v2/accounts/me), not an endsWith suffix; tests mount the production path. The global claims-per-hour ceiling is now a dedicated middleware over the shared RateLimitCounter table with security-ceiling semantics: it fails CLOSED (503) on any counter-store error instead of waving requests through, and window identity derives from database time, so replicas with skewed app clocks cannot split the counter at a boundary. Replaces the fail-open express-rate-limit store.
… post-transfer drift) The consumer for everything the online paths fail closed into, modeled on the deletion outbox drain (bounded batches, idempotent re-runs, observable counts) and riding the same tick, self-throttled to hourly (provider-calling). Quarantine drain: retryable LineageQuarantine rows (keyless verify/RTDN/claim events, keyless or unmatched voids) are re-driven against fresh provider state through the SAME hardened code paths — the atomic resolver and applyNotification with its receipt/registry idempotency keys — so re-running never double-applies; recovered rows are stamped resolvedAt. Conflict-class reasons (chain conflicts, loops, rotation mismatches) are never auto-merged: counted for an operator and left in place. Post-transfer drift: lineages with a committed transfer/restore/undo in the last 24 hours are re-checked against authoritative provider state; a non-entitled result invalidates the current held custody (bounded, conservative claw from the current owner) and raises an ops-alert log. This closes what the settlement recheck cannot: lost/delayed terminal webhooks, post-settlement revocations, and immediate tombstone restorations that never pass through contest settlement.
…, parked voids, ceiling, sweep - Google restoration with two tombstoned periods releases exactly the claimed order's escrow (the lifetime-startTime window would have released the old period's). - Keyless Google claim: 409 lineage_unresolved + quarantine row, lineage stays tombstoned. - Google provider gate: googlePlay claims rejected before any provider call while SUBSCRIPTION_CLAIM_GOOGLE_ENABLED is off (default), claimable=false for gated Google lineages, Apple unaffected. - Tombstoned rotation with a conflicting alias quarantines, funds nothing, and never repoints the existing alias. - Activity veto through a REAL authenticated request inside the stamp-throttle window (codex's exact bypass shape) cancels the pending transfer; without a pending transfer the throttle still suppresses. - Voids: keyless and unmatched-order voids park (nothing revoked, no claw); a matched order still compensates exactly its period. - Global ceiling: 503 on counter-store failure (fail closed); concurrent requests count exactly once each in the shared counter and the limit splits 200/429 precisely. - Reconciliation sweep: parked keyless renewal recovers once the order identity appears (idempotent second run), post-transfer provider revocation is drift-compensated exactly once, and conflict-class quarantine rows are never auto-merged.
A transient DB error during the stamp write (or the pending-transfer lookup that gates the throttle) used to be swallowed while the request proceeded - a legitimate owner's authenticated act during a contest window could silently fail to veto a pending transfer. stampAuthActivity now propagates failures; the auth middleware and the token-mint path answer 5xx so the client retries and no act ever succeeds unstamped. The mint path reuses stampAuthActivity (null knownLastAuthAt = always stamp) instead of its own inline raw update. Also drops the stale fire-and-forget wording from the middleware doc comment.
LineageQuarantine gains per-row retry state (attempts, nextAttemptAt, needsOperatorAt) so the sweep can back persistent rows off and escalate them to an operator instead of reselecting a fixed oldest window. SubscriptionTransfer gains committedAt - stamped whenever a journal reaches committed (settlement for contested transfers, creation for instant moves, restores, and deletion escrows), backfilled from updatedAt - so drift reconciliation can cursor on settlement time: a default 72h-contested transfer's createdAt is 72 hours old by the time it settles and can never drive selection.
A tombstone restoration whose provider-proven current funding event has no matching escrow row (exact key or legacy-window fallback) used to restore anyway: a live lineage minted with zero released credits, which the drift sweep - seeing the provider as entitled - would never backfill. The claim now parks the lineage in LineageQuarantine (restoration_missing_funding_event, deduped while unresolved, ops-alert logged) and rejects with the retryable lineage_unresolved reason, leaving the lineage tombstoned.
… cursor, version fence, lease Quarantine drain selects only actionable rows (retryable reasons, due nextAttemptAt, not escalated); persistent rows back off exponentially and escalate to an operator after enough attempts, so they can never starve newer recoverable rows. Keyless voids resolve only when fresh provider state shows the subscription no longer entitled (terminal apply + clawback through applyNotification, which no longer carries the provider-truncated period end past the staleness guard); while entitled the voided order is unidentifiable and escalates instead of being mislabeled recovered. Unmatched-order voids for the current latest order fall through to the same terminal path (legacy-window custody fallback) instead of waiting forever for an exact play_order_ row. Drift selection cursors on journal committedAt with a persisted watermark (RuntimeConfig) instead of a 24h createdAt window, so a 72h-contested transfer is swept after settlement and a provider outage postpones - never loses - a journal (deferred rows hold the watermark; advancement is capped below a commit-visibility margin). Compensation re-reads the subscription under the lineage lock and fences on identity+version: a renewal landing between the provider fetch and the lock defers instead of being clawed on the stale answer. The affected custody is the current-window row or, when the period just ended with a lost terminal event, the latest held row - no covering-now escape - and the Subscription row is updated to the provider-derived terminal state alongside the clawback. The whole sweep runs under a pg_try_advisory_xact_lock lease so exactly one replica executes an interval (money operations stay idempotent if a lease is lost mid-sweep).
…ence, lease, void e2e Covers every round-5 named failure mode: an activity-stamp DB failure during a contest window fails the request (injected via a test seam in stampAuthActivity) and the retried act still vetoes at settlement; a default 72h-contested transfer settles and is then drift-swept off its committedAt (createdAt 73h old - the shape the old 24h createdAt window missed); 30 persistent quarantine rows back off instead of starving a newer recoverable row; a renewal interleaved between the sweep's provider fetch and the lineage lock survives the version fence and the deferred journal is re-checked entitled next sweep; keyless voids resolve end-to-end when provider state shows the void (terminal state + exact-period clawback) and escalate to an operator when still entitled; two concurrent sweep runners serialize on the advisory-lock lease with exactly one making provider calls; a custody period that expired just before the sweep (lost terminal event) is still compensated; and a restoration whose funding event has no escrow parks (deduped) and rejects instead of minting a zero-credit live lineage.
…lling-safe migration Replace the timestamp-only committedAt > watermark drift sweep with a deterministic (committedAt, id) keyset cursor so more than DRIFT_BATCH rows sharing one millisecond can no longer be permanently skipped. committedAt is now stamped exclusively by a database trigger (clock_timestamp()), never application new Date(), so a slow-clock replica can't backdate a journal behind the watermark. The migration backfills existing NULLs, installs a DEFAULT + NOT NULL, and the trigger stamps every insert/transition, so old replicas mid-rollout can no longer create a permanently invisible journal. Cursor advancement stays capped below a two-minute commit-visibility margin (a journal can become visible up to a transaction's lifetime after its timestamp is assigned), and a completed scan cycles back to the moving 24-hour floor instead of retiring a lineage after its first entitled result, so a revoke discovered hours later inside the drift window is still caught. The sweep's advisory-lock lease moves to Postgres's two-integer namespace, structurally disjoint from the identity barrier's single-bigint hash locks.
Add coverage for the round-6 drift cursor hardening: 51 journals sharing one committedAt millisecond are each swept exactly once across keyset batches; a raw insert attempting to backdate committedAt gets overridden by the DB trigger; an old-replica-style NULL committedAt insert is stamped and swept; and a provider revocation discovered on a later sweep, after an earlier sweep already saw the lineage as entitled but still inside the 24-hour drift window, still claws back. Also covers the previously-uncovered token-mint activity-stamp failure path, which must return 500 without minting a JWT.
…fe migration order Replace the global (committedAt, id) drift cursor with a per-lineage SubscriptionDriftSchedule table (nextDriftCheckAt/monitorUntil/attempts/ needsOperatorAt), driven by a DB trigger on SubscriptionTransfer so every committed transfer/restore/undo schedules or extends its lineage independently. One unreachable-provider lineage now backs off and escalates on its own schedule instead of head-of-line-blocking every later journal, and due lineages are served most-overdue-first so sustained volume above the batch cap can't starve older in-window lineages. Reorder migration 20260715170000 so the committedAt trigger/default install before the backfill and NOT NULL constraint, closing a race where a concurrent old-replica writer could land between the backfill and the constraint during a rolling deploy. Also read the database's wall clock once per sweep tick and use that single value for every window/due-time comparison, so a fast application replica can no longer age a DB-stamped journal out of its monitoring window ahead of schedule.
…log bounds Fix the revoke-after-entitled test to actually push the lineage's schedule past the commit-visibility margin before re-sweeping, since the prior version re-swept immediately and could never distinguish a working fix from a no-op. Add coverage for a permanently deferred lineage not blocking a healthy same-batch compensation, for its own backoff/escalation reaching an operator after the retry budget, and for a sustained 3x-batch backlog being fully checked across exactly three sweep calls. Update the equal-millisecond and DB-clock tests to assert against the new per-lineage schedule state instead of the retired RuntimeConfig watermark. Register the new SubscriptionDriftSchedule model in the deletion-inventory guard as retained: it carries only lineage-scoped scheduling metadata, no direct account identifier, and cascades with its already-retained SubscriptionLineage row.
…e fixtures Remove tests whose source was deferred (live transfer, contest window, undo, Google claim proof, Play rotation absorption) and rewrite the cases that guarded surviving invariants onto the surviving surface: the custody cap lifecycle now runs spend/delete/restore cycles, the six drift reconciliation cases build committed state through delete-then-restore instead of a live transfer, the Google claim gate pins the permanent fail-closed contract, and the alias-conflict quarantine goes through the live funding resolver. Extract the repeated reclaim fixtures (period constants, account and JWT helpers, StoreKit signing, Apple status stubs, claim requests, cleanup) into tests/deletion/reclaim-fixtures.ts, consolidate the outbox task-success and record-completion duplicates into the sweep and retry tests, and drop the superseded duplicate cases.
…ining Avatar purge no longer trusts the client-supplied avatar URL: the deletion task snapshots the owning account id, the executor validates the configured public-asset origin, derives a canonical account-scoped object key, and treats foreign, cross-account, or malformed URLs as successful no-ops instead of retrying to terminal failure. PostHog purge calls use a dedicated private-API host setting (POSTHOG_API_HOST, default https://us.posthog.com) instead of the ingestion host, and both lookup and delete requests carry timeouts. Claiming an entitled but unrecognized Apple product returns the contract 400 invalid_claim_proof instead of a 500. The live-account fence distinguishes infrastructure failures (500) from a missing account (401). Google renewal notifications carry mapped tier and product so tombstoned renewals fund escrow. Reconciliation defers unknown-subscription events instead of resolving their quarantine row. Deletion outbox draining takes a cross-replica advisory lease and finalizes tasks conditionally so overlapping runners cannot revert a completed task. Global-ceiling failures log through the request logger.
Avatar uploads now mint account-scoped object keys (a/<accountId>/<uuid>) through an authenticated presign path, so teardown can safely delete exactly the deleting account's objects; legacy unscoped keys are logged, counted skips since ownership cannot be proven, with bucket lifecycle policy as the ops path for those. Notification subscribe now persists the client identifier inside a transaction that holds the owning account lock across remote registration, closing the race where a deletion could commit between remote registration and the identifier write and leave an unpurgeable installation; failed cleanup commits the row as a durable purge target and alerts. Outbox tasks are claimed atomically (pending to processing) before execution and finalized only from processing, with stale claims reclaimed after a timeout, so an expired lease can no longer double-run a task; a failed processing task returns to pending with backoff. Verify's tombstone response consults the claim eligibility evaluator instead of always advertising claimable, dead test references to the removed claim env vars are gone, and migration, source, and docs comments describe behavior instead of planning history (including the deletion kill switch defaulting to off).
…ation cleanup Outbox claims now increment the attempt counter at claim time and every finalization (done, terminal failure, retry with backoff) matches the claimed generation, so a worker resuming after its stale claim was reclaimed can no longer overwrite a newer worker's active claim. Notification subscribe locks the JWT account, device owner, and prior identifier owner in sorted order, persists the identifier in a short database-only transaction committed before any remote call, then registers remotely and re-verifies generation, ownership, and account liveness afterwards; an invalidated registration is compensated remotely and cleanup failures emit an operator alert. Deletion snapshots the union of account-owned identifiers and identifiers attached to account-owned devices, so a device cascade cannot destroy another account's identifier without a purge task, and the purge executor skips identifiers that have been re-registered by a live account; re-registering an identifier with an unfinished purge returns 503 with Retry-After.
…eardown Notification purge RPCs carry a 10-second deadline, kept well under the outbox stale-claim threshold so an in-flight delete can never outlive a reclaim and land on a re-registered installation; tasks already at the attempt cap fail terminally without another execution. All notification-server mutations (subscribe registration and compensation, unregister, unsubscribe, webhook push-failure cleanup, outbox purge, topic updates) serialize on a per-installation advisory lock and re-verify generation and ownership under it before calling out, so a stalled older request cannot overwrite or delete a newer registration; failed compensation keeps the identifier row as the durable purge target and alerts. Device registration migrations lock the involved accounts in sorted order, take the installation locks, and revalidate liveness before attaching an identifier, so a migration either lands before a deletion snapshot (and is purged with the device cascade) or fails closed against a mid-teardown account.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Member
Author
|
Status of the 13 review items after the follow-up commits. Context: the PR was resized to the Apple-only launch surface; the deferred paths (live-lineage transfer, Google claim proof, Play tombstone rotation absorption) are parked on
|
…nd consolidate duplicate chain rows Keying each row on its immediate predecessor gave a twice-rotated Google chain two lineage identities, letting a fractured lineage bypass the global once-per-period funding registry. The backfill now walks every linkedPurchaseToken chain to its recursively discovered root (loops and depth overflows are quarantined, never guessed) and aliases every chain member. Root canonicalization can land several rows of one chain on one lineage. Detaching the extras left them addressable by token, so a later verify resolved the same lineage and hit the one-row-per-lineage unique index as an unrecognized P2002 (HTTP 500; silently dropped on the notification path). Same-account duplicates now consolidate onto the newest-entitlement row (receipts moved, losers deleted); same-chain rows owned by different accounts fail the migration loudly with row-level diagnostics.
A keyless void names no order, and a purchase fetch can only describe the subscription's current state - it can never say which historical order the void hit. The sweep previously treated any non-entitled current state as proof the void hit the current order and resolved the row as recovered, so a natural expiry unrelated to the void silently abandoned the void's clawback and its older custody. Keyless-void rows now escalate straight to an operator (needsOperatorAt stamp + ops alert) and are never resolved by terminal current state; independently proven expiry stays with the ordinary drift/terminal reconciliation paths.
…renewal info A claim presented during Apple billing grace (status 4) matches a latest transaction that has already lapsed, so deriving the restored row's status from the transaction seeded it as expired: the row serialized as expired/free tier and only a successful renewal healed it. The grace state and its authoritative future deadline live in the status item's signedRenewalInfo. Grace claims now verify and decode it, seed the restored row as grace with gracePeriodExpiresDate as gracePeriodEnd, and use the transaction only for period and funding identity. Missing or undecodable renewal info fails closed as an invalid proof; an already-elapsed grace deadline rejects as not entitled.
3 tasks
Converts the deletion rollout barrier from the RuntimeConfig row account_deletion_enabled to the ACCOUNT_DELETION_ENABLED env var, read via loadAccountDeletionEnabled() in src/config.ts. Strict fail-closed semantics: only the exact string "true" enables the endpoint; unset, empty, or garbage reads as off and keeps the exact 503 response. Env is fixed at process start, so a flip is now an infra PR + task-definition roll instead of a 30s config-cache expiry — the accepted trade-off for a deploy-audited switch. The RuntimeConfig helper stays (app_attest_enabled still uses it); only this key's usage is removed. Flag tests now set the env var and pin the fail-closed matrix (unset/false/1/TRUE all 503).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/notifications/installation-mutation-fence.ts (1)
45-88: 🚀 Performance & Scalability | 🔵 TrivialOperational note: monitor connection-pool pressure for the fence.
Each fenced mutation retains a pooled DB connection while a remote RPC runs (bounded to 10s under a 15s tx timeout). This is a deliberate, documented tradeoff, but on hot paths (subscribe, webhook cleanup fan-out) concurrent in-flight mutations equal to the pool size can exhaust it and manifest as
maxWait(5s) transaction acquisition timeouts → 500s. Consider tracking pool saturation /maxWaittimeout metrics and sizing the pool accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/notifications/installation-mutation-fence.ts` around lines 45 - 88, Instrument with metrics or logging around the prisma.$transaction acquisition in withInstallationMutationFence, specifically tracking maxWait timeout failures and concurrent in-flight fenced mutations. Use these signals to monitor pool saturation on subscribe and webhook cleanup paths, and document or adjust connection-pool sizing as needed without changing the existing fence behavior or timeout bounds.src/accounts/deletion/outbox.ts (3)
52-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplicit return-type annotations on exported functions.
drainDeletionTasksUnderLease,drainDeletionTasks, andrunDeletionOutboxSweepall declare: Promise<...>return types. As per coding guidelines,src/**/*.{ts,tsx}should "Don't specify return type on functions. Prefer inferring the value in TypeScript."Also applies to: 214-214, 307-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/accounts/deletion/outbox.ts` at line 52, Remove the explicit Promise return-type annotations from drainDeletionTasksUnderLease, drainDeletionTasks, and runDeletionOutboxSweep, allowing TypeScript to infer their return types while preserving their existing implementations.Source: Coding guidelines
307-340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing JSDoc on
runDeletionOutboxSweep.Sibling exports in this file (
drainDeletionTasks, the test seam) carry JSDoc; this orchestrator — the main entry point tied to SLA alerting per the PR objectives — doesn't. As per coding guidelines,src/**/*.{ts,tsx}should "Add JSDoc comments for public APIs."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/accounts/deletion/outbox.ts` around lines 307 - 340, Add a JSDoc comment immediately before the exported runDeletionOutboxSweep function, documenting it as the public orchestrator for the deletion outbox sweep and its draining, completion, expiry, and reconciliation passes.Source: Coding guidelines
85-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate terminal-failure transition/logging.
The "mark failed + log terminal_failure" pattern (Lines 86-108 for pre-claim exhaustion, Lines 149-170 for post-executor exhaustion) is duplicated with only the
whereclause andattemptsvalue differing. Extracting a small helper would reduce the chance the two paths drift (e.g., one keepslastErrorin sync and the other doesn't).♻️ Suggested helper to de-duplicate terminal-failure handling
+const markTaskFailed = async (params: { + taskId: string; + operationId: string; + kind: string; + where: Prisma.DeletionTaskWhereInput; + lastError: string; + attempts: number; +}) => { + const transitioned = await prisma.deletionTask.updateMany({ + where: { id: params.taskId, ...params.where }, + data: { status: "failed", lastError: params.lastError }, + }); + if (transitioned.count === 0) return 0; + logger.error( + { + taskId: params.taskId, + operationId: params.operationId, + kind: params.kind, + attempts: params.attempts, + lastError: params.lastError, + }, + "deletion.task.terminal_failure", + ); + return transitioned.count; +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/accounts/deletion/outbox.ts` around lines 85 - 198, Extract the duplicated terminal-failure transition and logging from the drain flow into a small helper, using the task identity, applicable attempt count, matching status/claim conditions, and lastError as parameters. Update both the pre-claim exhaustion branch and the post-executor exhaustion branch to call this helper, preserving their existing counters, conditional transition behavior, and terminal_failure logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Line 157: Reorder the environment keys in .env.example to satisfy
dotenv-linter’s alphabetical ordering: move ACCOUNT_DELETION_ENABLED before
DELETION_HASH_SECRET, and order POSTHOG_API_HOST, POSTHOG_PERSONAL_API_KEY,
POSTHOG_PROJECT_ID consistently relative to each other and
SIWE_ALLOWED_CHAIN_IDS. Apply the resulting ordering across the surrounding
entries, preserving all key names and values.
In `@src/accounts/deletion/outbox.ts`:
- Around line 207-233: The transaction wrapper in drainDeletionTasks can reject
after drainDeletionTasksUnderLease has completed and populated counts,
discarding those results. Track whether the lease drain completed, catch the
transaction rejection, and return the computed counts when completion occurred;
rethrow failures that happen before the drain finishes so runDeletionOutboxSweep
still reports genuine execution errors.
---
Nitpick comments:
In `@src/accounts/deletion/outbox.ts`:
- Line 52: Remove the explicit Promise return-type annotations from
drainDeletionTasksUnderLease, drainDeletionTasks, and runDeletionOutboxSweep,
allowing TypeScript to infer their return types while preserving their existing
implementations.
- Around line 307-340: Add a JSDoc comment immediately before the exported
runDeletionOutboxSweep function, documenting it as the public orchestrator for
the deletion outbox sweep and its draining, completion, expiry, and
reconciliation passes.
- Around line 85-198: Extract the duplicated terminal-failure transition and
logging from the drain flow into a small helper, using the task identity,
applicable attempt count, matching status/claim conditions, and lastError as
parameters. Update both the pre-claim exhaustion branch and the post-executor
exhaustion branch to call this helper, preserving their existing counters,
conditional transition behavior, and terminal_failure logging.
In `@src/notifications/installation-mutation-fence.ts`:
- Around line 45-88: Instrument with metrics or logging around the
prisma.$transaction acquisition in withInstallationMutationFence, specifically
tracking maxWait timeout failures and concurrent in-flight fenced mutations. Use
these signals to monitor pool saturation on subscribe and webhook cleanup paths,
and document or adjust connection-pool sizing as needed without changing the
existing fence behavior or timeout bounds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 056d5df4-cee3-4db0-871b-eb558fa51b7c
📒 Files selected for processing (57)
.env.exampledocs/plans/delete-my-account.mdprisma/migrations/20260715094310_add_account_deletion/migration.sqlprisma/migrations/20260715104500_add_subscription_lineage/migration.sqlsrc/accounts/deletion/executors.tssrc/accounts/deletion/outbox.tssrc/accounts/deletion/service.tssrc/api/v2/accounts/handlers/account-delete.tssrc/api/v2/accounts/handlers/subscription-claim.tssrc/api/v2/accounts/handlers/subscription-verify.tssrc/api/v2/agents/assets/agent-assets.router.tssrc/api/v2/agents/assets/handlers/get-presigned-url.tssrc/api/v2/auth/handlers/generate-token.tssrc/api/v2/device/handlers/register.tssrc/api/v2/index.tssrc/api/v2/notifications/handlers/subscribe.tssrc/api/v2/notifications/handlers/unregister.tssrc/api/v2/notifications/handlers/unsubscribe.tssrc/api/v2/notifications/handlers/webhook.tssrc/api/v2/subscriptions/handlers/google-play-rtdn.tssrc/config.tssrc/middleware/auth.tssrc/middleware/claimGlobalCeiling.tssrc/notifications/client.tssrc/notifications/installation-mutation-fence.tssrc/payments/types.tssrc/subscriptions/AGENTS.mdsrc/subscriptions/claim-eligibility.tssrc/subscriptions/claim-flags.tssrc/subscriptions/claim.tssrc/subscriptions/custody.tssrc/subscriptions/google-play/notification-mapping.tssrc/subscriptions/jws-verifier.tssrc/subscriptions/lineage.tssrc/subscriptions/reconciliation.tssrc/subscriptions/repository.tssrc/subscriptions/tombstones.tstests/account-auth-check.test.tstests/agent-assets-presigned.test.tstests/builder-deps-env.test.tstests/deletion/adversarial-round3.test.tstests/deletion/adversarial-round4.test.tstests/deletion/adversarial-round5.test.tstests/deletion/adversarial.test.tstests/deletion/barrier-mint.test.tstests/deletion/claim.test.tstests/deletion/delete-account.test.tstests/deletion/delete-endpoint-ratelimit.test.tstests/deletion/executors.test.tstests/deletion/lineage-backfill-migration.test.tstests/deletion/outbox.test.tstests/deletion/reclaim-fixtures.tstests/deletion/router-fencing.test.tstests/deletion/tombstones.test.tstests/device-register-deletion-fence.test.tstests/notifications-subscribe-fencing.test.tstests/subscriptions/google-play/notification-mapping.test.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- src/payments/types.ts
- src/api/v2/auth/handlers/generate-token.ts
- tests/account-auth-check.test.ts
- src/config.ts
- tests/deletion/delete-endpoint-ratelimit.test.ts
- src/middleware/claimGlobalCeiling.ts
- src/api/v2/subscriptions/handlers/google-play-rtdn.ts
- src/api/v2/accounts/handlers/subscription-verify.ts
- src/api/v2/accounts/handlers/account-delete.ts
- prisma/migrations/20260715094310_add_account_deletion/migration.sql
- src/subscriptions/reconciliation.ts
- src/subscriptions/lineage.ts
- src/accounts/deletion/executors.ts
- tests/deletion/tombstones.test.ts
- src/accounts/deletion/service.ts
- tests/deletion/delete-account.test.ts
- src/subscriptions/repository.ts
…ount-impl # Conflicts: # docs/plans/delete-my-account.md # src/api/v2/credits-admin/audit-repository.ts # src/api/v2/notifications/handlers/subscribe.ts # src/subscriptions/grants.ts # src/subscriptions/repository.ts
… fails The outbox drain's transaction exists solely to hold the advisory lock; per-task writes commit on the pooled client. A lease timeout on close after the drain finished used to reject the whole call, discarding the computed counts and logging a generic drain_failed even though the work was durable. Track completion: a post-drain close failure now logs deletion.outbox.lease_commit_failed with the counts and returns them; a failure before the drain completes still propagates unchanged.
ACCOUNT_DELETION_ENABLED now precedes DELETION_HASH_SECRET and the PostHog trio is alphabetized (POSTHOG_API_HOST, POSTHOG_PERSONAL_API_KEY, POSTHOG_PROJECT_ID). Feature-section grouping and comments unchanged.
…se guard drainCompleted is assigned inside the transaction closure; TS flow analysis cannot see closure execution from the catch block and narrows the read to its literal initializer, so no-unnecessary-condition misfires. Targeted disable with rationale, matching existing precedent.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/v2/credits-admin/audit-repository.ts (1)
57-72: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire canonical ISO timestamps before decoding cursors.
new Date()accepts formats the encoder never produces, so non-canonical timestamps like2026-07-14 00:00:00.000can pass and later be normalized by the query. Validate both the decoded timestamp and UUID with a strict Zod schema so malformed cursors returninvalid_cursor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/v2/credits-admin/audit-repository.ts` around lines 57 - 72, The decodeAuditCursor function currently accepts non-canonical timestamps through new Date(). Replace the ad hoc timestamp and UUID checks with a strict Zod schema that validates the decoded timestamp in the exact ISO format emitted by the cursor encoder and the UUID id, returning null for any schema failure so callers produce invalid_cursor.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (2)
src/api/v2/credits-admin/audit-repository.ts (1)
48-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign exported cursor APIs with repository conventions.
Add JSDoc for the exported cursor type/helpers and let TypeScript infer the explicit return types.
As per coding guidelines, “Add JSDoc comments for public APIs” and “Don't specify return type on functions. Prefer inferring the value in TypeScript.”
Also applies to: 102-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/v2/credits-admin/audit-repository.ts` around lines 48 - 78, Update the exported RecentAuditCursor, encodeAuditCursor, and decodeAuditCursor APIs with concise JSDoc describing their purpose and usage. Remove their explicit return-type annotations so TypeScript infers the return types, while preserving the existing cursor encoding and decoding behavior.Source: Coding guidelines
docs/plans/delete-my-account.md (1)
570-571: 🔒 Security & Privacy | 🔵 TrivialConsider documenting a compromise-response path for the non-rotatable
DELETION_HASH_SECRET.Pinning the barrier hash to a secret that "must never rotate" is reasonable to keep hash lookups stable, but it leaves no documented recourse if the secret ever leaks (recomputing barrier hashes for an attacker, or needing a coordinated re-hash migration). Worth a short note on the incident-response plan for this specific secret, even if rotation itself stays out of scope for v1.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/delete-my-account.md` around lines 570 - 571, Add a brief incident-response note near the DELETION_HASH_SECRET barrier documentation describing the contingency if the non-rotatable secret is compromised, including coordinated barrier-hash re-computation or migration. Keep routine secret rotation out of v1 scope while documenting the response path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/api/v2/credits-admin/audit-repository.ts`:
- Around line 57-72: The decodeAuditCursor function currently accepts
non-canonical timestamps through new Date(). Replace the ad hoc timestamp and
UUID checks with a strict Zod schema that validates the decoded timestamp in the
exact ISO format emitted by the cursor encoder and the UUID id, returning null
for any schema failure so callers produce invalid_cursor.
---
Nitpick comments:
In `@docs/plans/delete-my-account.md`:
- Around line 570-571: Add a brief incident-response note near the
DELETION_HASH_SECRET barrier documentation describing the contingency if the
non-rotatable secret is compromised, including coordinated barrier-hash
re-computation or migration. Keep routine secret rotation out of v1 scope while
documenting the response path.
In `@src/api/v2/credits-admin/audit-repository.ts`:
- Around line 48-78: Update the exported RecentAuditCursor, encodeAuditCursor,
and decodeAuditCursor APIs with concise JSDoc describing their purpose and
usage. Remove their explicit return-type annotations so TypeScript infers the
return types, while preserving the existing cursor encoding and decoding
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ac3ce783-e6cc-4ca2-9d89-73cf1a74f139
📒 Files selected for processing (15)
.env.exampledocs/plans/delete-my-account.mdprisma/schema.prismasrc/accounts/deletion/outbox.tssrc/api/v2/credits-admin/audit-repository.tssrc/api/v2/notifications/handlers/subscribe.tssrc/payments/AGENTS.mdsrc/subscriptions/custody.tssrc/subscriptions/grants.tssrc/subscriptions/repository.tstests/credits-admin/audit-repository.test.tstests/deletion/outbox-lease-commit.test.tstests/notifications-subscribe-fencing.test.tstests/notifications-subscribe-identity.test.tstests/subscriptions/repository.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/credits-admin/audit-repository.test.ts
- src/payments/AGENTS.md
- tests/notifications-subscribe-fencing.test.ts
- src/subscriptions/grants.ts
- prisma/schema.prisma
- src/subscriptions/custody.ts
- src/subscriptions/repository.ts
- src/api/v2/notifications/handlers/subscribe.ts
lourou
marked this pull request as ready for review
July 27, 2026 16:12
This was referenced Jul 29, 2026
…ount-impl # Conflicts: # src/api/v2/subscriptions/handlers/google-play-rtdn.ts # src/subscriptions/repository.ts
| const result = await applyNotificationOnce(input); | ||
| if (result.kind !== "retry_live") return result; | ||
| } | ||
| // Restored again while retrying — ack; the next provider event (or a |
There was a problem hiding this comment.
🟠 High subscriptions/repository.ts:1214
When the retry loop in applyNotification exhausts both attempts with retry_live, it records the delivery as dropped and returns unknown_subscription, which causes the webhook handler to ack 200. A refund/revoke that races two delete/restore transitions is then permanently skipped — the provider stops retrying an acknowledged event, and there may be no later notification or client verify to apply the state change. The exhausted path should either continue against the now-live row or return an error so the provider redelivers, rather than acknowledging an unapplied notification.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/subscriptions/repository.ts around line 1214:
When the retry loop in `applyNotification` exhausts both attempts with `retry_live`, it records the delivery as dropped and returns `unknown_subscription`, which causes the webhook handler to ack 200. A refund/revoke that races two delete/restore transitions is then permanently skipped — the provider stops retrying an acknowledged event, and there may be no later notification or client verify to apply the state change. The exhausted path should either continue against the now-live row or return an error so the provider redelivers, rather than acknowledging an unapplied notification.
| // refreshed Play purchase) precisely so this guard has a period to | ||
| // compare; updates that omit it (no period drift possible) fall | ||
| // through and apply as before. | ||
| if ( |
There was a problem hiding this comment.
🟠 High subscriptions/repository.ts:1328
The staleness guard at input.update.currentPeriodEnd < current.currentPeriodEnd returns { kind: "applied" } before reaching the terminal-money branch. A late refund/revoke for an older funded period — arriving after a later renewal already advanced the live subscription — records the receipt but never looks up or invalidates that event's LineagePeriodCustody row and never compensates its current holder. The stale event's custody value is left held/escrowed indefinitely. The guard correctly prevents overwriting the subscription's current entitlement state, but the terminal custody lookup and invalidation for notificationProviderPeriodKey(input) must still run before returning.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/subscriptions/repository.ts around line 1328:
The staleness guard at `input.update.currentPeriodEnd < current.currentPeriodEnd` returns `{ kind: "applied" }` before reaching the terminal-money branch. A late refund/revoke for an older funded period — arriving after a later renewal already advanced the live subscription — records the receipt but never looks up or invalidates that event's `LineagePeriodCustody` row and never compensates its current holder. The stale event's custody value is left held/escrowed indefinitely. The guard correctly prevents overwriting the subscription's current entitlement state, but the terminal custody lookup and invalidation for `notificationProviderPeriodKey(input)` must still run before returning.
…ount-impl # Conflicts: # src/subscriptions/repository.ts
… the purge API is unconfigured PostHog persons carry only pseudonymous identifiers (accountId UUID, HKDF-derived device hashes) and behavioral counters — no direct PII — and the accountId mapping dies with the Account row in the teardown transaction. Failing the purge task (503 + retry, paging ops) until a human-scoped personal API key is parked in the task environment was a worse trade than the orphaned pseudonymous residue it removes. Missing POSTHOG_PERSONAL_API_KEY / POSTHOG_PROJECT_ID (or analytics disabled entirely) now completes the task with a distinct deletion.purge.posthog_person_skipped log (reason: purge_api_not_configured / analytics_disabled) so DeletionRecord can reach completed. When both are set, the person purge runs unchanged.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
feat: account deletion (barrier, teardown, tombstones) + Apple subscription reclaim
Implements the delete-my-account spec on this stack's plan branch, plus the subscription reclaim design that
heals the
subscription_account_mismatch409 class (paid subscriptions orphaned on deleted accounts). The specdoc is amended to the as-built design. The reclaim architecture is the settled result of nine adversarial review
rounds, then deliberately resized to the Apple-only launch surface: everything behind a launch-off flag
(live-lineage transfer with its contest window and undo, the Google claim proof path, Play token-rotation
tombstone absorption) was deferred to a follow-up branch (
feature/delete-account-impl-fullholds the completeimplementation) so this PR ships only code that can execute at launch.
Deletion architecture
DeletedIdentitystores a keyed HMAC (DELETION_HASH_SECRET) of the auth method'stype:externalKey.POST /v2/auth/tokenconsults it only after full SIWE validation succeeds: a barredidentity gets the terminal 410
identity_deletedand never re-provisions (bad nonce/signature never revealsdeletion state). Mint and teardown co-serialize on a shared
pg_advisory_xact_lock(hashtext(identityHash))and the mint re-checks the barrier inside its own transaction, so a delete racing a mint can never recreate
the account. The barrier is permanent.
accountIdclaim ishonored only while the Account row exists, generic 401 otherwise, no positive caching; an infrastructure
failure during the check returns 500, never a 401 that could make a client wipe its session. The sole
carve-out is
DELETE /v2/accounts/me(method + path), accepting an unexpired pre-deletion token for recordlookup only; a router-fencing test audits every authed
/v2surface.DELETE /v2/accounts/melocks the Account rowFOR UPDATEfirst, thendeletes children before parents (wallet/ledger via a dedicated payments-module helper preserving the
single-writer law; entitled periods escrowed first). It writes the operationId-keyed
DeletionRecord(allreplays return the stored record), snapshots the purge outbox, tombstones lineages, erects barrier rows, and
records a retained AdminAudit entry under a sentinel id. A global lock order (lineage -> accounts sorted ->
subscription -> wallets sorted; every account-linked writer takes its Account lock first) plus bounded
jittered retry make it deadlock-free.
plus the additive
claimable: true; Apple S2S and Play RTDN ack tombstoned keys as counted no-ops that neverrecreate account-linked state (money-relevant events still keep the retained escrow correct: a tombstoned
renewal funds escrow, a tombstoned refund invalidates it).
(10 attempts, then terminal failure + operator alert) runs idempotent purges (S3 objects, notification
installations, Composio user, PostHog person), finalizes tasks conditionally so overlapping runners cannot
revert a completed task, alerts past the 24h SLA, and expires records at 30 days. The avatar purge never
trusts the client-supplied avatar URL: the task snapshots the owning account id and the executor validates
the configured public-asset origin and derives a canonical account-scoped object key; foreign, cross-account,
or malformed URLs are successful no-ops.
Subscription reclaim (lineage custody)
SubscriptionLineage— one row per purchase lineage (AppleoriginalTransactionId;Google
linkedPurchaseTokenchains resolved recursively with loop/depth bounds intoLineageTokenAlias;conflicting chains quarantine, never auto-merge) — is the canonical first lock and the scope for the grant
registry and tombstone state.
LineagePeriodGrantis a global once-per-funding-event registry (apple_txn_<id>/play_order_<latestOrderId>, with a period-advance gate so mid-period upgrades and replays never double-fund).LineagePeriodCustodycaps each period — promo/admin/signup credits never move, and no chain ofrestore/refund/deletion exceeds one allotment. DB CHECK constraints enforce these.
server-side (single 403
app_check_requiredfor every failure;app_attest_enabled=falsefails closed — theendpoint is off, never open), authoritative entitled-now + latest-transaction proof (no
signedDatewindow),per-IP/per-account limits, and a Postgres-backed global claims-per-hour ceiling that fails closed on store
error. An entitled but unrecognized product returns the contract 400
invalid_claim_proof.provably deleted) restores exactly once, releasing the exact funding-event escrow — never a second grant — and
a restoration matching no escrow row parks in quarantine and rejects
lineage_unresolvedrather than minting azero-credit row; a live lineage already owned by the caller replays 200 with no double credit; any other
live-lineage claim deterministically fails closed. Live-owner transfer is not in this PR (see Deferred).
Reconciliation
Drift is tracked by a durable per-lineage
SubscriptionDriftSchedule(FK'd to the lineage,committedAt-ownedby a DB trigger). A bounded sweep runs a real provider check per due row, most-overdue-first, under an advisory
lease: entitled reschedules, not-entitled claws custody back onto the current owner, unknown escalates with
backoff. Deadline-expired rows get one final check before resolution (a late revoke is still clawed back), and a
work cap with an observable backlog counter defers overflow to the next tick rather than silently expiring it.
Google void notifications compensate the current holder; conflict and unknown-subscription quarantine rows stay
open until an apply actually succeeds (a reconcile pass that cannot persist the purchase defers instead of
resolving), and escalate to an operator, never auto-merge.
Security model
Provider proof is bearer-shaped — neither store offers claimant binding — so the design bounds consequences
instead of pretending to bind. The launch posture removes the live-owner attack surface entirely: only lineages
whose previous owner deleted their account are claimable, so a claim can never take anything from a live user.
An attacker would still need the victim's current-period latest JWS, a genuine attested app instance able to
mint a fresh limited-use App Check token per attempt (server consumes it; replay fails), and a live Convos
account — and must clear per-caller limits and the fail-closed global ceiling.
Deferred to the follow-up branch
The complete implementation, reviewed across the same rounds, is parked on
feature/delete-account-impl-fulland will come back as its own PR with its own security sign-off:
authenticated-activity veto, the one-shot undo + lineage freeze, and the transfer settlement worker.
fully on in this PR so the books are correct whenever claims ship).
counted no-op here; the absorption bookkeeping ships with the follow-up).
The schema keeps the deferred features' tables (
SubscriptionTransferand friends): migrations stay additiveand the follow-up branch reuses them unchanged.
Deployment / flags
DELETION_HASH_SECRET— new, required, never rotate (barrier hashes must stay stable forever; deliberatelyseparate from the freely rotatable nonce secret).
account_deletion_enableddefaults FALSE — ops flips it to true only after the migrationsare deployed and every replica runs this build; it doubles as the emergency kill switch.
SUBSCRIPTION_CLAIM_TOMBSTONE_ENABLED=trueis the only claim flag; the live-transfer, Google-claim, andcontest-window settings were removed with their code paths.
POSTHOG_PERSONAL_API_KEY/POSTHOG_PROJECT_ID, plusPOSTHOG_API_HOST(defaults to the privateAPI host
https://us.posthog.com— distinct from the ingestion host; both purge calls carry timeouts). Whenanalytics is active but creds are missing, purge tasks retry and page ops rather than silently skipping.
committedAtcolumnwith its DB-time stamp trigger, then
SubscriptionDriftSchedule) install triggers beforebackfill/constraints so mixed-version replicas stay correct with no drain; apply
committedAtfirst, then theschedule, before deploying code that queries the schedule.
Tests
Full suite ~1574 passing across 182 files; the only failure is the documented pre-existing telemetry-metrics
machine flake (reproduces identically on the untouched base commit). Lint, format, typecheck, and
prisma validateclean. Adversarial release-blocker invariants all present: credit conservation (promo nevermoves, no delete/restore/renewal chain exceeds one allotment); concurrent claims of one JWS commit exactly one
restoration; App Check fails closed with
app_attest_enabled=falseand rejects replayed tokens; unknown claimkey 404 with the attestation still consumed; drift deadline final-check and bounded backlog on
delete-then-restore fixtures; sweep and outbox single-runner leases; avatar purge origin/namespace validation
(malicious and garbage URLs); migration-chain replay from scratch. Schema guards: RESTRICT still bites, and an
inventory test flags any new account-correlatable model. Repeated fixtures were consolidated into a shared
module (
tests/deletion/reclaim-fixtures.ts).Commits
Chunks 1-7 (
ac86f51,4f0fffb,f791707,e9f01f4,9e505d1,ff1c55f,e56591a): models, barrier,fail-closed auth, tombstone-aware verify/webhooks, teardown, purge outbox, the lineage/custody claim endpoint,
first invariant suite. Review-hardening rounds then followed:
ce9179b,c2e8cec,d6db53a,9a364bc,f725250): 16 blockers + 4 should-fix — atomiclineage/alias creation, DB-enforced money invariants, mint/delete co-serialization, route fencing, App Check
fail-closed, deletion default-off.
3727d99,2bc0935,2ce3974,5ee7214,2e028a3): exact funding-event restoration, keyless-claimfail-close, lossless veto, fail-closed ceiling, the sweep.
3464e24,c673617,296142e,90a316c,98f4de5): quarantine backoff/escalation, drift cursor,TOCTOU fence, expired-custody comp, single-runner lease.
e415a82,53e389e,a302528,6f3b657,4c4ab71,4c343ab): composite cursor -> durableper-lineage
SubscriptionDriftSchedule-> deadline final check + bounded backlog drain; DB-owned commit time;rolling-safe migration ordering; matching tests.
6226698,6524bb6,827f717): defer the flag-off paths to the follow-up branch, retargetthe test suite at the launch scope (drift and conservation cases rebuilt on delete-then-restore fixtures;
shared fixture module), and apply the review-bot fixes — account-scoped S3 avatar purge, PostHog private-API
host + timeouts, claim 400 mapping for unmapped products, auth 500-vs-401 on infrastructure errors, Google
renewal tier/product mapping so tombstoned renewals fund escrow, reconciliation deferral of unpersisted
purchases, the outbox advisory lease, and request-correlated ceiling logging.
f4788ba,1cf9743,4678ba1,90b578d): account-scoped avatar uploadkeys with legacy-key skip semantics, outbox atomic claim with generation-fenced finalization, stale-claim
reclaim, claim-time attempt cap, and deadline-bounded purge RPCs (an in-flight delete can never outlive a
reclaim), notification subscribe/deletion fencing redesign (sorted multi-owner locks,
commit-before-remote-RPC, post-registration liveness verification with remote compensation that retains the
durable purge target on failure, device-cascade-aware purge snapshot, purge executor re-registration guard),
a per-installation mutation fence so every remote installation writer (subscribe, compensation, unregister,
unsubscribe, webhook cleanup, outbox purge, topic updates) serializes and re-verifies generation/ownership
before calling out, device-register participation in the deletion lock protocol (sorted account locks +
liveness revalidation before identifier migration), eligibility-gated
claimableon verify, anddocs/comment cleanup.
Known non-blocking follow-ups
recurrence retains the failure body.
account_deletion_enabled, off atlaunch): the fence transaction's timeout budget starts before the blocking advisory-lock acquisition, so a
writer that waited several seconds on a contended installation lock can have its transaction expire
(releasing the lock) while its 10-second remote call is still in flight, briefly re-opening the
overwrite/delete race for that one installation id. Requires sustained per-installation contention plus a
slow remote call. Fix direction: acquire the lock before starting the timeout budget, or size the budget to
cover lock-wait plus the RPC deadline.
was reviewed and accepted with deadlock-retry wrappers on both sides; tightening it to the documented order
remains optional hardening.
How to test
pnpm test(orpnpm test:localto reset the DB first); deletion, claim, andreconciliation suites live under
tests/deletion/and the subscriptions directory. Aprisma migrate deployagainst a fresh DB replays the full chain end to end.
DELETE /v2/accounts/me-> 200 with the stored record on every replay; re-mint -> 410identity_deleted; verify a tombstoned key -> 409claimable: true; claim it -> escrow released exactly once,journal row written; claim again as the new owner -> 200 replay, no double credit.
Summary by CodeRabbit
Note
Add authenticated account deletion endpoint with deletion barriers, subscription tombstones, and reclaim flow
DELETE /v2/accounts/me(account-delete.ts) gated byACCOUNT_DELETION_ENABLED, returning 200 with a deletion record or replaying idempotently; missing accounts return 401, disabled flag returns 503.POST /v2/accounts/me/subscription/claimendpoint restores tombstoned lineages to a new owner.DeletionTaskrows with exponential backoff for async purges (S3, PostHog, Composio, notification installations).DeletedIdentity,DeletionRecord,DeletionTask,SubscriptionLineage,LineagePeriodCustody,LineagePeriodGrant,LineageTokenAlias,RateLimitCounter, and others across multiple migrations.authMiddlewareandrequireAccountnow perform a database read on every authenticated request; the deletion carve-out forDELETE /api/v2/accounts/memust remain exact or deleted-account tokens will be permanently locked out of the replay path.Macroscope summarized 24e65af.