Skip to content
This repository was archived by the owner on Aug 12, 2026. It is now read-only.

feat(subscriptions): live transfer of subscriptions from non-deleted accounts - #377

Open
lourou wants to merge 14 commits into
feature/delete-account-implfrom
feature/delete-account-live-transfer
Open

feat(subscriptions): live transfer of subscriptions from non-deleted accounts#377
lourou wants to merge 14 commits into
feature/delete-account-implfrom
feature/delete-account-live-transfer

Conversation

@lourou

@lourou lourou commented Jul 16, 2026

Copy link
Copy Markdown
Member

Subscription reclaim: live bearer-transfer tier (flagged off)

Stacked on #374 (feature/delete-account-impl). That PR ships the launch scope: tombstone restoration — reclaiming a subscription whose owning account was deleted. This PR adds the second tier that was deferred out of it: self-serve reclaim when the original account was not deleted (the ghost-account case — the user lost their keys, the old account still "owns" the subscription, and every verify returns a 409 while renewals keep enriching a wallet nobody can reach).

Every flag this PR introduces defaults off, so no claim, transfer, undo, or Google-proof behavior is reachable at merge. One piece of supporting infrastructure is live immediately and unflagged — the lastAuthAt activity stamp (see "Live at merge" below). It moves no money.

What this adds

Live bearer-transfer (POST /v2/accounts/me/subscription/claim against a live lineage):

  • Possession-of-receipt proof. The claimant must present the provider artifact (Apple JWS / Play purchase token) and the backend independently confirms with the provider that the subscription is entitled now and the artifact is the subscription's latest transaction. App Check limited-use attestation is mandatory and fails closed.
  • 72h contest window. A live-tier claim returns 202 { status: "pending", contestEndsAt } instead of transferring immediately. The current owner's devices receive a SubscriptionClaimPending push.
  • Active-owner veto. Any authenticated act by the current owner before settlement cancels the pending transfer. The veto is fail-closed end to end: lastAuthAt is stamped synchronously at token mint and on every authenticated request (a request that cannot durably stamp gets a 5xx rather than proceeding unstamped), settlement reads it under the account row lock, and a null stamp counts as a veto. Settlement also re-checks the provider at execution time — a subscription refunded inside the window cancels instead of transferring.
  • One-shot undo. The immediately previous owner can reclaim once, within 30 days, via the same endpoint. The undo executes immediately (an attacker can never be the previous owner of their own theft), is CAS-guarded so a raced undo commits exactly once, and freezes the lineage against further automated transfers (operator re-home only).
  • Unspent-credits-only movement. Transfers move value through the lineage custody ledger: the debit is capped at min(owner balance, remaining period cap - owner consumption since custody), the paired debit/credit sums to zero, and commingled promo/admin/signup credits never move. No sequence of transfer/undo/delete/restore/refund can exceed one period allotment (covered by the restored custody-cap lifecycle and conservation tests).
  • Cooldown. 30 days per lineage between transfers; the previous-owner undo is exempt.

Google claim proof path, behind its own flag: Play purchase fetch, restoration keyed to the exact latestOrderId funding event (never window arithmetic — Play reports the lifetime start time), keyless purchases fail closed into the reconciliation quarantine.

Play token-rotation tombstone absorption: a rotated token landing on a tombstoned lineage is absorbed into the lineage's alias set through the atomic conflict-detecting resolver; a token owned by another lineage quarantines instead of funding the wrong lineage.

Fixes to the restored code (bugs that were adjudicated dead with the deferred slice and would have come back alive):

  • evaluateClaimable now gates tombstoned lineages behind the tombstone flag — verify no longer advertises claimable: true for a claim the endpoint would reject.
  • CLAIM_CONTEST_WINDOW_HOURS parsing rejects fractional/invalid values instead of parseInt-truncating "0.5" to 0, which would have silently enabled instant transfer — the one configuration that requires explicit security acceptance.
  • An entitled but unrecognized Google product returns the contract 400 invalid_claim_proof instead of a 500 (mirrors the Apple-path fix that already shipped in the base PR).

Flag posture (all OFF at merge)

Flag Default Effect when off
SUBSCRIPTION_CLAIM_LIVE_TRANSFER_ENABLED false Live-lineage claims return 409 transfer_frozen; no pending rows can be created, so the settlement pass has nothing to do
SUBSCRIPTION_CLAIM_GOOGLE_ENABLED false Google claim bodies rejected before any provider call; verify's claimable stays false for Google lineages
CLAIM_CONTEST_WINDOW_HOURS 72 Only read when live transfer is enabled

SUBSCRIPTION_CLAIM_TOMBSTONE_ENABLED (default on, from the base PR) is unchanged.

Live at merge (unflagged)

The lastAuthAt activity stamp is deliberately not flag-gated, because the veto's integrity depends on the stamp history existing before the flag ever flips:

  • Every token mint and every authenticated request performs an awaited Account.lastAuthAt = now() update, throttled to once per 5 minutes per account. Inside the throttle window it instead runs a cheap pending-transfer existence check (SubscriptionTransfer by status + fromAccountId) — that table is empty while the flag is off, so the check is a no-op scan today.
  • The stamp is fail-closed: a request whose stamp write fails gets a 500 rather than proceeding unstamped, even with every flag off. This is intentional (a silently lost stamp could later cost an owner their veto), and the failure mode requires a DB that just served the auth fence read to fail the very next write — availability-only, no money impact.

Play token-rotation absorption is also unflagged, but only reachable on tombstoned lineages, which can only exist once account deletion (default off, base PR) has run.

Before enabling live transfer

Flag-off is not a kill switch for in-flight state, and two items below should be fixed or explicitly accepted as part of the enable-time security sign-off:

  • Settlement ignores the flag. settlePendingTransfers executes due pending rows regardless of SUBSCRIPTION_CLAIM_LIVE_TRANSFER_ENABLED. If the flag is enabled and later disabled mid-incident, already-created pending transfers still settle (up to 72h later, moving money while ops believes the feature is off). Recommended pre-enable change: settlement defers (does not cancel) while the flag is off. Unreachable from merge posture — no pending row can exist unless the flag was on.
  • Undo runs before the flag check. A previous owner's undo executes even with the flag off. This is deliberately protective (a victim's recovery survives a feature rollback) but should be a conscious sign-off item.
  • Consider an index on SubscriptionTransfer (status, fromAccountId) for the stamp-throttle pending lookup before real pending rows exist at volume.

Known limitations (accepted for merge; revisit at enable time):

  • Undo vs. a restoration claimant. Undo eligibility does not require the current owner to be the undone transfer's transferee: after transfer → owner-deletes → third party restores, the pre-transfer owner's undo seizes the remainder from the restoration claimant. Protective in the attacker-launder shape (delete + restore cannot defeat the victim's undo) and conservation holds, but the plan doc's "recovery is support-mediated once the lineage moves on" holds only for transfer-moves, not restore-moves.
  • Settlement fairness. The settlement pass takes 20 due rows per tick with no ordering; more than 20 perpetually-deferring rows (provider outage) could starve younger rows, and provider rechecks run per tick, unthrottled. Flag-on only.
  • Absurd contest windows. A ridiculous whole-number CLAIM_CONTEST_WINDOW_HOURS (e.g. 1e15) produces an invalid date and a 500 on the pending create. Operator error, flag-on only.
  • claimable vs. an open contest. Verify's informative claimable signal does not consider an open pending contest, so a client can be directed into a 409 pending_contest. The endpoint re-evaluates authoritatively; informational field only.
  • Veto tie-break. The veto comparison is strict and millisecond-truncated; a stamp landing in the same millisecond as the pending row does not veto. Both timestamps come from the DB clock; negligible.
  • Inherited from the base PR (not widened here): a Play fetch failure mid-chain during first resolution of a rotated tombstoned token can mint a second, truncated-root lineage that escapes the tombstone's claim controls (no money duplication — funding routes by token). Consider making the chain fetcher fail closed when a named predecessor is unfetchable.

Deploy notes

  • No new migrations. The schema already carries the transfer journal columns (SubscriptionTransfer, lineage cooldown/freeze fields) from the base PR; this PR is code-only.
  • New env vars are documented in .env.example; nothing is required at deploy time — absent vars resolve to the safe defaults above.
  • The pending-transfer settlement pass rides the existing deletion outbox sweep tick. Since the flag has never been on, no pending rows can exist at merge and the pass does nothing (note: this holds only as long as the flag has never been enabled — see "Before enabling live transfer").
  • The lastAuthAt stamp described under "Live at merge" is active from the first deploy of this code; no action needed, but be aware it adds an awaited write to the auth path (throttled per account).
  • Enabling live transfer later is an ops action (flip the flag), but it stays off pending explicit security sign-off — work the "Before enabling live transfer" checklist first; setting the contest window to 0 (instant transfer) additionally requires explicit security acceptance.

Relationship to the base PR

The base PR deferred this slice to keep the launch scope reviewable; this PR is the deferred slice rebuilt on top of it (revert of the defer commit, reconciled with the changes that landed since: root-canonicalized Google lineages, the consolidated test fixtures, the purge/fencing hardening, and the billing-grace claim seeding). Review can focus on the claim state machine (src/subscriptions/claim.ts), the custody transfer op (src/subscriptions/custody.ts), and the auth-side veto stamp (src/middleware/auth.ts, src/accounts/auth-activity.ts).

Tests: the removed live-transfer suite is restored and ported onto the shared tests/deletion/reclaim-fixtures.ts module (+27 tests over the base), including conservation, undo-race, contest-window lifecycle, veto fail-close, settlement provider recheck, Google exact-funding-event restoration, and rotation-absorption coverage.


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

Note

Add live transfer of subscriptions between non-deleted accounts with a 72h contest window

  • Introduces flagged live transfer via SUBSCRIPTION_CLAIM_LIVE_TRANSFER_ENABLED: when enabled, claiming an active subscription creates a 72h pending transfer (subscriptionTransfer row) and returns 202; instant transfer occurs when CLAIM_CONTEST_WINDOW_HOURS=0.
  • Adds settlePendingTransfers worker (run each deletion outbox sweep) that rechecks provider entitlement and veto signals after the contest window, then commits or cancels the transfer.
  • Stamps Account.lastAuthAt on every authenticated request (via authMiddleware) and on SIWE token mint; if a pending outgoing transfer exists, stamping bypasses the normal 5-minute throttle so the old owner's activity can veto settlement.
  • Adds one-shot CAS undo for the immediately previous owner within a 30-day deadline; undo executes immediately and freezes the lineage against further automated transfers.
  • Enforces a 30-day lineage cooldown between transfers and rejects new claims while a pending contest exists.
  • Adds Google Play claim support gated behind SUBSCRIPTION_CLAIM_GOOGLE_ENABLED, including quarantine for missing latestOrderId and alias absorption for tombstoned rotations.
  • Records pre-escrow wallet balance in DeletionRecord.finalBalanceCredits (new nullable BIGINT column) at account deletion time.
  • Risk: auth middleware now returns 500 on lastAuthAt stamp failure, blocking all authenticated requests if the DB write fails.

Macroscope summarized 02ea5c7.

Summary by CodeRabbit

  • New Features

    • Added subscription claims for supported Apple and Google Play purchases, with configurable launch controls.
    • Added live subscription transfers with cooldowns, contest windows, pending status, settlement, and one-time undo support.
    • Added notifications when a subscription claim is pending.
    • Improved handling of rotated Google Play tokens and deleted-account subscription events.
  • Bug Fixes

    • Prevented unresolved or invalid Google Play claims from restoring subscriptions or granting benefits.
    • Improved transfer reconciliation and compensation after provider revocations.
    • Authentication activity is now recorded more reliably during sign-in and account access.

lourou added 2 commits July 16, 2026 18:57
… Google claim proof behind launch-off flags

Reverts the defer refactor (6226698) and reconciles the restored slice
onto the moved base:

- Live bearer-transfer tier behind SUBSCRIPTION_CLAIM_LIVE_TRANSFER_ENABLED
  (off): 72h contest window (202 pending + push to the old owner's
  devices), any authenticated act by the old account vetoes settlement
  (awaited fail-closed lastAuthAt stamp at mint and on every authed
  request), execution-time provider recheck, per-lineage 30-day cooldown,
  one-shot CAS undo with post-undo freeze, pending settlement on the
  deletion outbox tick.
- Google claim proof path behind SUBSCRIPTION_CLAIM_GOOGLE_ENABLED (off),
  now documented in .env.example: Play purchase fetch, exact
  latestOrderId funding-event restoration, keyless fail-close.
- Play token-rotation tombstone absorption on the verify and RTDN paths,
  routed through the atomic conflict-detecting lineage resolver.

Reconciliation with the base that moved since the defer: the live-account
fence keeps the 500-on-DB-error split while restoring the lastAuthAt
select; the mint path keeps the backfill no-op short-circuit; verify
keeps the billing-grace renewal-info seeding on the merged claim handler.

Revived fixes that died with the deferred code: evaluateClaimable gates
tombstoned lineages behind the tombstone flag instead of advertising a
doomed claim; claimContestWindowHours rejects fractional or invalid hour
counts instead of parseInt-truncating 0.5 to 0 (instant transfer); an
entitled but unrecognized Google product claims 400 invalid_claim_proof
instead of 500, mirroring the Apple fix.

Flags all default off: merging this activates nothing.
…fixtures

Restores the test coverage removed with the deferred slice, rewritten
against tests/deletion/reclaim-fixtures.ts instead of the old per-file
fixture copies: instant transfer conservation, cooldown + one-shot undo
(including the concurrent-undo race and undo-after-spend conservation),
contest window lifecycle (202 pending, push notifier, settlement,
authenticated-act veto, stamp-throttle bypass, stamp-failure fail-close,
null-lastAuthAt defensive veto, provider revocation and unreachable
provider at settlement), replay against two targets, opposite-direction
transfer deadlock convergence, the transfer/undo/delete/restore custody
cap lifecycle, post-transfer drift compensation (incl. the 73h contested
settlement shape), Google flag-gated claims (flag-off rejection, flag-on
exact funding-event escrow release, keyless fail-close), tombstoned
rotation absorption and its conflicting-alias quarantine, and the mint
activity stamp (success + fail-closed 500).

New regression coverage for the revived fixes: a fractional contest
window falls back to 72h pending instead of an instant transfer, and an
entitled but unrecognized Google product claims 400.

The fixture wipe now also resets the claim env flags and the pending
notifier / settlement checker / stamp failure seams.
@lourou
lourou requested a review from a team as a code owner July 16, 2026 17:22
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @lourou's task —— View job


Code Review Complete

I've completed a comprehensive security-focused review of PR #377. This is a sophisticated implementation of live subscription transfers with a contest window mechanism. Below are my findings organized by severity.


🔴 Critical Security Issues

1. Race Condition in Veto Mechanism (src/accounts/auth-activity.ts:53-55)

The stampAuthActivity function performs an un-locked UPDATE on Account.lastAuthAt:

await prisma.$executeRaw`
  UPDATE "Account" SET "lastAuthAt" = now() WHERE id = ${accountId}::uuid
`;

Problem: Settlement's veto check (src/subscriptions/claim.ts:603-618) acquires Account FOR UPDATE and reads lastAuthAt under that lock. However, stampAuthActivity does NOT acquire any lock before writing. This creates a classic read-to-write race:

  1. T1 (settlement): SELECT lastAuthAt FROM Account WHERE id=X FOR UPDATE → reads old timestamp
  2. T2 (auth request): UPDATE Account SET lastAuthAt=now()waits for T1's lock
  3. T1: Veto check passes (stale read), commits transfer
  4. T2: UPDATE finally executes after T1 released the lock

Result: A legitimate owner's authenticated act during the contest window fails to veto the transfer because the stamp commits only after settlement already made its decision.

Fix Required: stampAuthActivity must acquire FOR UPDATE on the Account row BEFORE the now() write to serialize with settlement's read:

await tx.$queryRaw`SELECT id FROM "Account" WHERE id = ${accountId}::uuid FOR UPDATE`;
await tx.$executeRaw`UPDATE "Account" SET "lastAuthAt" = now() WHERE id = ${accountId}::uuid`;

This is the most critical finding as it undermines the entire veto security model.


2. Undo Can Steal from Unrelated Owners (src/subscriptions/claim.ts:145-155)

The undo eligibility check allows lastTransfer.fromAccountId === callerAccountId without verifying that the transfer led to the current owner:

const undoTarget =
  lastTransfer &&
  lastTransfer.fromAccountId === callerAccountId &&
  lastTransfer.undoDeadlineAt !== null &&
  lastTransfer.undoDeadlineAt.getTime() > Date.now()
    ? lastTransfer
    : null;

Problem: After A → B → deletion → C restores, the transfer journal has A → B as the latest committed transfer. User A can call undo and seize the subscription from C (the restoration claimant), even though A was never C's predecessor.

Fix Required: Add && lastTransfer.toAccountId === row.accountId:

const undoTarget =
  lastTransfer &&
  lastTransfer.fromAccountId === callerAccountId &&
  lastTransfer.toAccountId === row.accountId &&  // ← ADD THIS
  lastTransfer.undoDeadlineAt !== null &&
  lastTransfer.undoDeadlineAt.getTime() > Date.now()
    ? lastTransfer
    : null;

🟠 Major Issues

3. Pending-Transfer Notification is Fire-and-Forget (src/api/v2/accounts/handlers/subscription-claim.ts:570-581)

When a live transfer creates a pending row (202 response), the handler sends push notifications as fire-and-forget:

try {
  await notifier({ oldAccountId, contestEndsAt, provider });
} catch (error) {
  req.log.warn({ error }, "subscription.claim.pending_push_failed");
}

Problem: The pending transfer has already committed. If the notification fails (push service down, crash mid-delivery), there is no retry mechanism. Settlement can execute 72 hours later without the victim ever being notified.

Recommendation: Persist an outbox task atomically with the pending transfer creation. Process the notification queue independently with retries. This ensures delivery survives transient failures.


4. Settlement Ignores Rollout Flag (src/subscriptions/claim.ts:555-662)

The settlePendingTransfers worker executes all due pending rows regardless of SUBSCRIPTION_CLAIM_LIVE_TRANSFER_ENABLED.

Problem: If the flag is enabled, pending transfers are created, then the flag is flipped OFF mid-incident, those transfers still settle (moving money while ops believes the feature is disabled).

Fix: Check the flag at settlement time and defer (not cancel) pending rows when disabled:

if (!isLiveTransferEnabled()) {
  logger.warn({ transferId: pendingRow.id }, "settlement_deferred_flag_off");
  continue;  // Skip this row, will retry next sweep
}

5. Undo Bypasses the Rollout Flag (src/subscriptions/claim.ts:157)

The undo branch (lines 157-197) executes even when isLiveTransferEnabled() returns false. While this is protective for victims, it should be a conscious security sign-off item, not an accident.

Recommendation: Document this as intentional in the PR description's "Before enabling live transfer" section, or add an explicit flag check with clear reasoning in comments.


6. Provider-Unreachable Starvation (src/subscriptions/claim.ts:559-577)

Settlement fetches 20 pending rows per tick. If the first 20 repeatedly return "unknown" (provider unreachable), they consume every sweep and later due transfers never settle.

Fix: Add retry scheduling (persist nextRetryAt in the pending row and exclude rows with future retry times from the query), or use cursor-based pagination that advances past deferred rows.


🟡 Minor Issues

7. Absurd Contest Window Crashes Settlement (src/subscriptions/claim-flags.ts:34-43)

A ridiculous whole-number CLAIM_CONTEST_WINDOW_HOURS (e.g., 1e15) produces an invalid Date in the pending-transfer create and causes a 500.

Fix: Cap the maximum at the JS Date limit (~2.4 billion hours) and return the 72h fallback for out-of-range values:

const MAX_SAFE_HOURS = Math.floor(8.64e15 / 3600000);
return Number.isInteger(n) && n >= 0 && n <= MAX_SAFE_HOURS ? n : 72;

8. Play Proof Validation Returns 500 Instead of 400 (src/api/v2/accounts/handlers/subscription-claim.ts:330-364)

extractProductId, deriveStatusFromPurchase, and extractPeriodWindow throw AppError(400) for malformed Google data. These currently reach the generic 500 handler instead of mapping to invalid_claim_proof.

Fix: Wrap the Play validation calls in a try/catch that returns { status: 400 } for AppError instances.


9. claimable Signal Ignores Pending Contests (src/subscriptions/claim-eligibility.ts:43-51)

The verify endpoint's claimable: true does not check for an active pending contest, so clients can be directed into a 409 pending_contest rejection.

Fix: Query for a pending transfer before returning true:

const pending = await prisma.subscriptionTransfer.findFirst({
  where: { lineageId, status: "pending" },
  select: { id: true },
});
if (pending) return false;

✅ Strengths

  1. Comprehensive test coverage: 27 new tests covering adversarial scenarios, race conditions, conservation, veto fail-close, and settlement provider rechecks
  2. Conservative money movement: The custody system ensures D ≤ cap ≤ allotment and commingled credits never transfer
  3. Proper lock ordering: Follows the documented lineage → accounts → subscription → wallets order to prevent deadlocks
  4. Fail-closed design: Auth stamp failures return 5xx rather than proceeding unstamped
  5. Idempotent operations: Ledger operations use proper idempotency keys (sub_transfer_in_, sub_transfer_out_, etc.)
  6. Flag-gated rollout: All live-transfer features default OFF at merge

📋 API Backwards Compatibility

No breaking changes detected. The claim endpoint already exists and the PR only adds the googlePlay discriminant (which is gated behind its own flag). Old clients sending Apple proofs continue to work.


💰 Ledger/Money Handling

Follows repository rules:

  • All credit moves go through applyDeltaWithTx from @/payments/ledger
  • Idempotency keys are present and unique per operation
  • No direct UserCredits or CreditLedger writes outside approved paths
  • Conservation invariant holds: D ≤ min(balance, cap) and paired debits/credits sum to zero

🧪 Test Coverage Assessment

Excellent coverage of the new functionality:

  • Tombstone restoration with exact funding-event matching
  • Live transfer contest window lifecycle
  • Veto fail-close scenarios
  • One-shot undo with CAS race protection
  • Settlement provider rechecks
  • Google Play token rotation absorption
  • Conservation across transfer chains

Missing coverage:

  • The critical stampAuthActivity race condition (not tested)
  • Undo eligibility vs. restoration claimant (mentioned as "known limitation")
  • Settlement queue starvation under provider outages

📝 Recommendations

Before Merge

  1. Fix the veto race condition (Critical [Backend] User, Device, and Conversation tables #1) — this breaks the security model
  2. Fix the undo ownership check (Critical [Backend] Group Invites #2) — prevents theft
  3. Consider addressing Major [Backend] Push Notifications #3-6 or explicitly accepting them with sign-off

Before Enabling Live Transfer

  1. Work through the "Before enabling live transfer" checklist in the PR description
  2. Add explicit security acceptance for:
  3. Consider an index on SubscriptionTransfer (status, fromAccountId) for the stamp-throttle pending lookup

Documentation

  • The PR description is thorough and documents known limitations clearly
  • Consider adding inline comments explaining the lock-acquisition order in stampAuthActivity after the fix

Summary

This is a well-architected feature with strong conservation guarantees and comprehensive adversarial testing. However, the veto race condition (#1) and undo ownership bug (#2) are blocking security issues that must be fixed before merge. The other findings should be triaged as part of the enable-time security review.

The flag-off posture is safe for merge once the critical issues are resolved.


@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This change expands subscription reclaim with Apple and Google claim verification, feature-gated live transfers, contest-window settlement, custody movement, tombstone rotation absorption, authentication activity vetoes, and extensive adversarial coverage.

Changes

Subscription reclaim flow

Layer / File(s) Summary
Claim entry, flags, and notification contracts
.env.example, src/subscriptions/claim-flags.ts, src/subscriptions/claim-eligibility.ts, src/api/v2/accounts/handlers/subscription-claim.ts, src/api/v2/notifications/types.ts, src/subscriptions/AGENTS.md, docs/plans/delete-my-account.md, tests/deletion/adversarial-round4.test.ts
Adds runtime claim flags, Google Play proof verification, claim eligibility gates, pending-transfer push payloads, and live-transfer contract documentation.
Transfer state machine, custody, and settlement
src/subscriptions/claim.ts, src/subscriptions/custody.ts, src/accounts/deletion/outbox.ts, tests/deletion/claim.test.ts, tests/deletion/adversarial*.test.ts, tests/deletion/reclaim-fixtures.ts
Adds live ownership transfers, one-shot undo, contest-window pending rows, custody ledger movement, entitlement rechecks, settlement, reconciliation, and concurrency coverage.
Authentication activity stamping
src/accounts/auth-activity.ts, src/api/v2/auth/handlers/generate-token.ts, src/middleware/auth.ts, tests/auth-token-siwe.test.ts, tests/deletion/barrier-mint.test.ts
Stamps authenticated activity with throttling and fail-closed error handling for token generation and live-account middleware.
Tombstone rotation handling
src/subscriptions/tombstones.ts, src/subscriptions/repository.ts, src/api/v2/subscriptions/handlers/google-play-rtdn.ts, tests/deletion/tombstones.test.ts, tests/deletion/adversarial-round4.test.ts
Absorbs rotated Google keys into tombstoned lineage aliases, quarantines conflicts, and distinguishes tombstoned from unknown subscription acknowledgements.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ClaimHandler
  participant ProofVerifier
  participant ClaimStateMachine
  participant NotificationService
  participant SettlementWorker
  Client->>ClaimHandler: submit subscription claim
  ClaimHandler->>ProofVerifier: verify provider proof
  ProofVerifier-->>ClaimHandler: return verified entitlement
  ClaimHandler->>ClaimStateMachine: execute claim
  ClaimStateMachine-->>ClaimHandler: return pending transfer
  ClaimHandler->>NotificationService: notify previous account
  SettlementWorker->>ClaimStateMachine: settle expired pending transfer
  ClaimStateMachine-->>SettlementWorker: commit or cancel transfer
Loading

Possibly related PRs

Suggested reviewers: fbac

Poem

I’m a rabbit guarding claims tonight,
With Google tokens tucked in tight.
Transfers pause, then hop ahead,
While tombstone trails turn safely red.
Auth stamps veto thieves with care—
New lineage paths bloom everywhere!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: enabling live subscription transfers from accounts that were not deleted.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/delete-account-live-transfer

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

stampFailureForTests = err;
};

export const stampAuthActivity = async (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High accounts/auth-activity.ts:38

stampAuthActivity issues a bare UPDATE "Account" SET "lastAuthAt" = now() without locking the row first, so a concurrent transfer settlement that runs SELECT ... FOR UPDATE on the account can acquire the lock before this UPDATE, read the stale lastAuthAt, and commit the transfer while this authenticated request is still waiting on the UPDATE. The stamp commits only after settlement has already made its veto decision, so a real owner act during the contest window fails to cancel the pending transfer. Awaiting the UPDATE does not close the read-to-write race. The stamp needs to acquire the account row lock (e.g., SELECT id FROM "Account" WHERE id = ... FOR UPDATE) before the now() write so it serializes before settlement's read — or settlement must re-check lastAuthAt after any competing stamp.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/accounts/auth-activity.ts around line 38:

`stampAuthActivity` issues a bare `UPDATE "Account" SET "lastAuthAt" = now()` without locking the row first, so a concurrent transfer settlement that runs `SELECT ... FOR UPDATE` on the account can acquire the lock before this UPDATE, read the stale `lastAuthAt`, and commit the transfer while this authenticated request is still waiting on the UPDATE. The stamp commits only after settlement has already made its veto decision, so a real owner act during the contest window fails to cancel the pending transfer. Awaiting the UPDATE does not close the read-to-write race. The stamp needs to acquire the account row lock (e.g., `SELECT id FROM "Account" WHERE id = ... FOR UPDATE`) before the `now()` write so it serializes before settlement's read — or settlement must re-check `lastAuthAt` after any competing stamp.

req.log.warn({ error }, "subscription.claim.play_fetch_failed");
return { status: 400 };
}
const fetchedProductId = extractProductId(purchase);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium handlers/subscription-claim.ts:330

verifyPlayProof calls deriveStatusFromPurchase and extractProductId, which throw AppError(400) for pending or unknown Google Play states and for responses with no line items/product ID. These thrown errors reach the handler's outer catch and return a 500 Failed to claim subscription, but the claim contract requires a 400 invalid_claim_proof for malformed or unsupported purchase proof. Consider wrapping these calls in a try/catch inside verifyPlayProof and returning { status: 400 }, matching how verifyAppleProof handles productMapping failures.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/api/v2/accounts/handlers/subscription-claim.ts around line 330:

`verifyPlayProof` calls `deriveStatusFromPurchase` and `extractProductId`, which throw `AppError(400)` for pending or unknown Google Play states and for responses with no line items/product ID. These thrown errors reach the handler's outer catch and return a `500 Failed to claim subscription`, but the claim contract requires a `400 invalid_claim_proof` for malformed or unsupported purchase proof. Consider wrapping these calls in a try/catch inside `verifyPlayProof` and returning `{ status: 400 }`, matching how `verifyAppleProof` handles `productMapping` failures.

@macroscopeapp

macroscopeapp Bot commented Jul 16, 2026

Copy link
Copy Markdown

Approvability

Verdict: Needs human review

2 blocking correctness issues found. This PR introduces live subscription transfers with contest windows, undo, and cooldown mechanisms - a significant new feature in a security-sensitive domain. Multiple critical and major unresolved review comments identify race conditions in the auth stamp veto mechanism and missing ownership checks in the undo flow that could allow subscription theft. Human review is warranted before merge.

No code changes detected at 02ea5c7. Prior analysis still applies.

You can customize Macroscope's approvability policy. Learn more.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 12

🧹 Nitpick comments (3)
tests/deletion/tombstones.test.ts (1)

214-217: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert that absorption targets the tombstoned lineage.

A non-null alias alone would also pass if token-new were attached to the wrong lineage. Verify the referenced lineage is the expected tombstone.

Proposed assertion
 const absorbed = await prisma.lineageTokenAlias.findUnique({
   where: { token: "token-new" },
 });
 expect(absorbed).not.toBeNull();
+const absorbedLineage = await prisma.subscriptionLineage.findUnique({
+  where: { id: absorbed?.lineageId ?? "" },
+});
+expect(absorbedLineage?.state).toBe("tombstoned");
+expect(absorbedLineage?.lineageKey).toBe("token-old");
🤖 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 `@tests/deletion/tombstones.test.ts` around lines 214 - 217, Strengthen the
assertion for the alias returned by prisma.lineageTokenAlias.findUnique in the
tombstone absorption test: verify that its referenced lineage matches the
expected tombstoned lineage, not merely that the alias is non-null. Use the
existing tombstone identifier or lineage relation established by the test.
src/subscriptions/tombstones.ts (1)

34-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use an object argument for SubscriptionTombstonedError and let absorbTombstoneRotation infer its return type. Update the src/subscriptions/repository.ts throw site to pass named fields instead of relying on argument order.

🤖 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/subscriptions/tombstones.ts` around lines 34 - 41, Change
SubscriptionTombstonedError to accept a single object containing the named
constructor fields, then update the throw site in absorbTombstoneRotation within
the repository flow to pass those fields by name rather than positional
arguments. Remove any explicit return-type annotation from
absorbTombstoneRotation so its return type is inferred.

Source: Coding guidelines

src/accounts/auth-activity.ts (1)

38-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use an object parameter and inferred return type.

This new two-argument exported helper should follow the project’s src/** function conventions; update its two callers accordingly.

Proposed refactor
 export const stampAuthActivity = async (
-  accountId: string,
-  knownLastAuthAt: Date | null,
-): Promise<void> => {
+  args: { accountId: string; knownLastAuthAt: Date | null },
+) => {
+  const { accountId, knownLastAuthAt } = args;

As per coding guidelines, use object parameters in src/** and infer function return types.

🤖 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/auth-activity.ts` around lines 38 - 41, Update the exported
stampAuthActivity helper to accept a single object parameter containing
accountId and knownLastAuthAt, and remove its explicit Promise<void> return
annotation so the return type is inferred. Update both callers to pass the
corresponding values through the new object shape.

Source: Coding guidelines

🤖 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 `@docs/plans/delete-my-account.md`:
- Around line 307-316: Update the deletion persistence section to remove or
explicitly supersede the dedicated provider-key tombstone model, aligning it
with the later statement that SubscriptionLineage is the tombstone carrier.
Ensure the document presents one authoritative model for deleted subscriptions,
including how provider identities and rotated tokens are retained.

In `@src/api/v2/accounts/handlers/subscription-claim.ts`:
- Around line 330-364: Wrap the Play proof validation calls in the claim
handler—extractProductId, deriveStatusFromPurchase, and extractPeriodWindow—in
an AppError-aware try/catch that returns status 400 for validation AppErrors,
preserving the invalid_claim_proof mapping. Rethrow unexpected errors so they
continue to reach the generic error handler, and keep productMapping’s existing
handling unchanged.
- Around line 407-489: Update executeClaim and the pending-transfer notification
flow so creating the pending transfer atomically persists a durable outbox task
containing the old account, contest deadline, and provider before settlement can
proceed. Replace direct fire-and-forget reliance on
defaultPendingTransferNotifier with independent outbox processing and retries;
retain per-device failure isolation while ensuring crashes or push outages leave
the task retryable until delivery succeeds.
- Around line 323-329: Update the catch around fetchSubscriptionPurchaseV2 in
the subscription claim handler to distinguish confirmed invalid-token or 404
errors from provider and network failures. Preserve the 400 response only for
dead-token cases; log and return the established retryable 5xx response for
transient Play API failures.

In `@src/subscriptions/claim-eligibility.ts`:
- Around line 43-51: Update the claim eligibility logic around the existing
live-transfer and cooldown checks to return false when the lineage has an active
pending contest, matching executeClaim’s pending_contest rejection. Reuse the
lineage’s existing pending-transfer/contest state symbol and keep eligibility
true only when no contest is pending and all current checks pass.

In `@src/subscriptions/claim-flags.ts`:
- Around line 34-42: Update claimContestWindowHours to reject values above the
maximum safe hour bound for JavaScript Date timestamps, returning the existing
72-hour fallback for out-of-range values. Enforce the same upper bound in the
CLAIM_CONTEST_WINDOW_HOURS environment schema, while preserving acceptance of
whole non-negative hours within the limit.

In `@src/subscriptions/claim.ts`:
- Line 157: The live-transfer rollout flag must guard every live-tier mutation.
In src/subscriptions/claim.ts lines 157-157, update the undo flow around
undoTarget to reject or defer undo when live transfers are disabled; in
src/subscriptions/claim.ts lines 555-562, prevent pending transfers from being
committed while disabled and cancel or defer them according to the rollout
policy.
- Around line 559-577: Update the settlement sweep around the findMany query and
per-row checker in the subscription claim flow so rows whose entitlement is
"unknown" are persisted with a later retry time or otherwise excluded from the
current scan, allowing subsequent due transfers to be processed while retaining
a bound on provider calls. Ensure deferred rows remain eligible for future
sweeps and preserve existing committed/cancelled handling.
- Around line 145-155: Update the undoTarget eligibility check in the
subscription transfer lookup to also require lastTransfer.toAccountId ===
row.accountId, ensuring the latest committed transfer leads to the current owner
before allowing undo. Preserve the existing caller, deadline, and null handling
conditions.

In `@tests/deletion/adversarial-round3.test.ts`:
- Line 430: Update the test setup around resetAppleApiClientForTests so this
case installs an empty Apple status map via installAppleStatusMap({}) instead of
relying on the reset alone. Keep the fixture hermetic and ensure the provider
call deterministically rejects even when Apple environment variables are
present.

In `@tests/deletion/adversarial-round4.test.ts`:
- Around line 341-345: Reload lineage l1 from the database after
applyNotification and before the post-event state assertion, then assert the
reloaded record’s state is "tombstoned" instead of using the stale l1 object.
Keep the existing alias assertion unchanged.

In `@tests/deletion/claim.test.ts`:
- Around line 405-406: Update the contestEndsAt assertion in the deletion test
to retain the existing lower-bound check and add an upper-bound check confirming
the fallback is no later than 72 hours from the current time. Use the same
contestEndsAt value and allow only the intended timing tolerance.

---

Nitpick comments:
In `@src/accounts/auth-activity.ts`:
- Around line 38-41: Update the exported stampAuthActivity helper to accept a
single object parameter containing accountId and knownLastAuthAt, and remove its
explicit Promise<void> return annotation so the return type is inferred. Update
both callers to pass the corresponding values through the new object shape.

In `@src/subscriptions/tombstones.ts`:
- Around line 34-41: Change SubscriptionTombstonedError to accept a single
object containing the named constructor fields, then update the throw site in
absorbTombstoneRotation within the repository flow to pass those fields by name
rather than positional arguments. Remove any explicit return-type annotation
from absorbTombstoneRotation so its return type is inferred.

In `@tests/deletion/tombstones.test.ts`:
- Around line 214-217: Strengthen the assertion for the alias returned by
prisma.lineageTokenAlias.findUnique in the tombstone absorption test: verify
that its referenced lineage matches the expected tombstoned lineage, not merely
that the alias is non-null. Use the existing tombstone identifier or lineage
relation established by the test.
🪄 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

Run ID: 7f07e2a1-3f12-4aae-b5a5-a06193ea70f4

📥 Commits

Reviewing files that changed from the base of the PR and between ee946f6 and 5f62616.

📒 Files selected for processing (26)
  • .env.example
  • docs/plans/delete-my-account.md
  • src/accounts/auth-activity.ts
  • src/accounts/deletion/outbox.ts
  • src/api/v2/accounts/handlers/subscription-claim.ts
  • src/api/v2/auth/handlers/generate-token.ts
  • src/api/v2/notifications/types.ts
  • src/api/v2/subscriptions/handlers/google-play-rtdn.ts
  • src/middleware/auth.ts
  • src/payments/types.ts
  • src/subscriptions/AGENTS.md
  • src/subscriptions/claim-eligibility.ts
  • src/subscriptions/claim-flags.ts
  • src/subscriptions/claim.ts
  • src/subscriptions/custody.ts
  • src/subscriptions/repository.ts
  • src/subscriptions/tombstones.ts
  • tests/auth-token-siwe.test.ts
  • tests/deletion/adversarial-round3.test.ts
  • tests/deletion/adversarial-round4.test.ts
  • tests/deletion/adversarial-round5.test.ts
  • tests/deletion/adversarial.test.ts
  • tests/deletion/barrier-mint.test.ts
  • tests/deletion/claim.test.ts
  • tests/deletion/reclaim-fixtures.ts
  • tests/deletion/tombstones.test.ts

Comment thread docs/plans/delete-my-account.md
Comment on lines +323 to +329
try {
purchase = await fetchSubscriptionPurchaseV2(body.purchaseToken);
} catch (error) {
// Unknown/dead token.
req.log.warn({ error }, "subscription.claim.play_fetch_failed");
return { status: 400 };
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the target handler around the cited lines.
sed -n '280,380p' src/api/v2/accounts/handlers/subscription-claim.ts

# Find the Play fetch helper and any related error handling.
rg -n "fetchSubscriptionPurchaseV2|subscription\.claim\.play_fetch_failed|AppError\(400\)|AppError" src/api/v2 -S

# Show the implementation of the helper if present in the repo.
fd -a "subscription-claim.ts" src/api/v2
fd -a "*purchase*" src/api/v2

Repository: xmtplabs/convos-backend

Length of output: 9275


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the sibling verify handler around its Play fetch/error handling.
sed -n '280,360p' src/api/v2/accounts/handlers/subscription-verify.ts

# Inspect the Google RTDN handler using the same fetch helper.
sed -n '160,230p' src/api/v2/subscriptions/handlers/google-play-rtdn.ts

# Locate the fetch helper definition and any error classification around it.
rg -n "function fetchSubscriptionPurchaseV2|const fetchSubscriptionPurchaseV2|fetchSubscriptionPurchaseV2\\(" src -S

Repository: xmtplabs/convos-backend

Length of output: 5281


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the Play API helper and its error mapping.
sed -n '1,220p' src/subscriptions/google-play/play-api.ts

# Inspect the claim handler context around the fetch and follow-up branches.
sed -n '300,380p' src/api/v2/accounts/handlers/subscription-claim.ts

Repository: xmtplabs/convos-backend

Length of output: 6288


Differentiate Play API failures before returning 400 src/api/v2/accounts/handlers/subscription-claim.ts:323-329
fetchSubscriptionPurchaseV2() can fail for provider/network issues as well as dead tokens, but this catch turns every failure into 400. Only map confirmed invalid-token/404 cases to 400; return a retryable 5xx for transient Play API failures.

🤖 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/accounts/handlers/subscription-claim.ts` around lines 323 - 329,
Update the catch around fetchSubscriptionPurchaseV2 in the subscription claim
handler to distinguish confirmed invalid-token or 404 errors from provider and
network failures. Preserve the 400 response only for dead-token cases; log and
return the established retryable 5xx response for transient Play API failures.

Comment on lines +330 to +364
const fetchedProductId = extractProductId(purchase);
if (fetchedProductId !== body.productId) return { status: 400 };
const status = deriveStatusFromPurchase(purchase);
const entitled =
status === SubscriptionStatus.active ||
status === SubscriptionStatus.grace ||
status === SubscriptionStatus.trial;
if (!entitled) return { status: 409, reason: "not_entitled" };
if (!purchase.latestOrderId) {
// No funding-event identity: fail closed, same rule as verify/RTDN —
// park for reconciliation and reject retryably. A keyless claim would
// otherwise reach restoration with no exact escrow key.
await quarantineLineageToken(
BillingProvider.googlePlay,
body.purchaseToken,
"missing_latest_order_id",
{ source: "claim" },
);
req.log.error({}, "subscription.claim.play_missing_order_id_parked");
return { status: 409, reason: "lineage_unresolved" };
}
const playOrderId = purchase.latestOrderId;

let mapping: ReturnType<typeof productMapping>;
try {
mapping = productMapping(fetchedProductId);
} catch (error) {
req.log.warn(
{ error, productId: fetchedProductId },
"subscription.claim.unrecognized_product",
);
return { status: 400 };
}
const { tier, period } = mapping;
const window = extractPeriodWindow(purchase);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Map Play proof validation errors to invalid_claim_proof.

extractProductId, deriveStatusFromPurchase, and extractPeriodWindow throw AppError(400) for malformed, pending, or unknown provider data. These currently reach the generic handler catch and return 500. Catch validation AppErrors here and return { status: 400 }, while rethrowing unexpected errors.

As per coding guidelines, “Implement consistent error handling using AppError class.”

🤖 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/accounts/handlers/subscription-claim.ts` around lines 330 - 364,
Wrap the Play proof validation calls in the claim handler—extractProductId,
deriveStatusFromPurchase, and extractPeriodWindow—in an AppError-aware try/catch
that returns status 400 for validation AppErrors, preserving the
invalid_claim_proof mapping. Rethrow unexpected errors so they continue to reach
the generic error handler, and keep productMapping’s existing handling
unchanged.

Source: Coding guidelines

Comment on lines +407 to +489
// ---------------------------------------------------------------------------
// Pending-transfer push notification (contest window)
// ---------------------------------------------------------------------------

type PendingTransferNotifier = (args: {
oldAccountId: string;
contestEndsAt: Date;
provider: "apple" | "googlePlay";
}) => Promise<void>;

/**
* Send the contract's SubscriptionClaimPending push to every registered
* device of the old account — the one notification channel we have, and the
* structural bound on the bearer-theft residual: the legitimate owner learns
* a transfer is pending while any authenticated act still vetoes it. Each
* device send is individually caught; a push failure never fails the claim.
*/
const defaultPendingTransferNotifier: PendingTransferNotifier = async ({
oldAccountId,
contestEndsAt,
provider,
}) => {
const devices = await prisma.deviceRegistration.findMany({
where: {
accountId: oldAccountId,
disabled: false,
pushToken: { not: null },
},
select: {
deviceId: true,
pushToken: true,
pushTokenType: true,
apnsEnv: true,
},
});
logger.warn(
{ deviceCount: devices.length, contestEndsAt: contestEndsAt.toISOString() },
"subscription.claim.pending_transfer_push",
);
if (devices.length === 0) return;

const apns = createApnsService();
const fcm = createFcmService();
await Promise.all(
devices.map(async (device) => {
const payload: SubscriptionClaimPendingPayload = {
clientId: device.deviceId,
notificationType: "SubscriptionClaimPending",
notificationData: {
contestEndsAt: contestEndsAt.toISOString(),
provider,
},
};
const adapted = { ...device, id: device.deviceId };
try {
const service = device.pushTokenType === "apns" ? apns : fcm;
if (!service) {
logger.warn(
{ deviceId: device.deviceId, pushTokenType: device.pushTokenType },
"subscription.claim.pending_push_service_unavailable",
);
return;
}
const result = await service.sendPushNotification({
device: adapted,
notification: payload,
isSilent: false,
});
if (!result.success) {
logger.warn(
{ deviceId: device.deviceId, error: result.error },
"subscription.claim.pending_push_send_failed",
);
}
} catch (err) {
logger.warn(
{ err, deviceId: device.deviceId },
"subscription.claim.pending_push_send_error",
);
}
}),
);
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Make pending-transfer notification durable before allowing settlement.

executeClaim has already committed the pending transfer when notification begins, and every delivery failure is swallowed. A crash or push outage therefore leaves no retry, while settlement can later transfer ownership without notifying the old owner. Persist an outbox task atomically with the pending transfer and retry delivery independently.

Also applies to: 570-581

🤖 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/accounts/handlers/subscription-claim.ts` around lines 407 - 489,
Update executeClaim and the pending-transfer notification flow so creating the
pending transfer atomically persists a durable outbox task containing the old
account, contest deadline, and provider before settlement can proceed. Replace
direct fire-and-forget reliance on defaultPendingTransferNotifier with
independent outbox processing and retries; retain per-device failure isolation
while ensuring crashes or push outages leave the task retryable until delivery
succeeds.

Comment on lines +43 to +51
if (!isLiveTransferEnabled()) return false;
if (lineage.liveTransferFrozenAt) return false;
if (lineage.lastTransferAt) {
const cooldownMs = SUBSCRIPTION_CLAIM_COOLDOWN_DAYS * 24 * 60 * 60 * 1000;
if (Date.now() - lineage.lastTransferAt.getTime() < cooldownMs) {
return false;
}
}
return true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Hide lineages with an active pending contest.

This can return true while executeClaim rejects the same lineage with pending_contest. Check for a pending transfer before advertising the claim.

Proposed fix
   if (!isLiveTransferEnabled()) return false;
   if (lineage.liveTransferFrozenAt) return false;
+  const pending = await prisma.subscriptionTransfer.findFirst({
+    where: { lineageId, status: "pending" },
+    select: { id: true },
+  });
+  if (pending) return false;
   if (lineage.lastTransferAt) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!isLiveTransferEnabled()) return false;
if (lineage.liveTransferFrozenAt) return false;
if (lineage.lastTransferAt) {
const cooldownMs = SUBSCRIPTION_CLAIM_COOLDOWN_DAYS * 24 * 60 * 60 * 1000;
if (Date.now() - lineage.lastTransferAt.getTime() < cooldownMs) {
return false;
}
}
return true;
if (!isLiveTransferEnabled()) return false;
if (lineage.liveTransferFrozenAt) return false;
const pending = await prisma.subscriptionTransfer.findFirst({
where: { lineageId, status: "pending" },
select: { id: true },
});
if (pending) return false;
if (lineage.lastTransferAt) {
const cooldownMs = SUBSCRIPTION_CLAIM_COOLDOWN_DAYS * 24 * 60 * 60 * 1000;
if (Date.now() - lineage.lastTransferAt.getTime() < cooldownMs) {
return false;
}
}
return true;
🤖 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/subscriptions/claim-eligibility.ts` around lines 43 - 51, Update the
claim eligibility logic around the existing live-transfer and cooldown checks to
return false when the lineage has an active pending contest, matching
executeClaim’s pending_contest rejection. Reuse the lineage’s existing
pending-transfer/contest state symbol and keep eligibility true only when no
contest is pending and all current checks pass.

? lastTransfer
: null;

if (undoTarget) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Enforce the live-transfer flag across every live-tier mutation.

The rollout kill switch currently blocks new plain transfers but still permits undo and pending settlement.

  • src/subscriptions/claim.ts#L157-L157: reject or defer undo while the live-transfer tier is disabled.
  • src/subscriptions/claim.ts#L555-L562: return without committing pending transfers while disabled; cancel or defer them according to rollout policy.
📍 Affects 1 file
  • src/subscriptions/claim.ts#L157-L157 (this comment)
  • src/subscriptions/claim.ts#L555-L562
🤖 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/subscriptions/claim.ts` at line 157, The live-transfer rollout flag must
guard every live-tier mutation. In src/subscriptions/claim.ts lines 157-157,
update the undo flow around undoTarget to reject or defer undo when live
transfers are disabled; in src/subscriptions/claim.ts lines 555-562, prevent
pending transfers from being committed while disabled and cancel or defer them
according to the rollout policy.

Comment on lines +559 to +577
const due = await prisma.subscriptionTransfer.findMany({
where: { status: "pending", contestEndsAt: { lte: new Date() } },
take: 20,
});
let committed = 0;
let cancelled = 0;
for (const pendingRow of due) {
try {
// Provider recheck runs outside the transaction (third-party latency
// must not hold locks); the fetch-to-commit TOCTOU residual is the
// same one accepted for the claim path, compensated by webhooks.
const checker = settlementEntitlementChecker ?? defaultEntitlementChecker;
const entitlement = await checker(pendingRow.providerProof ?? null);
if (entitlement === "unknown") {
logger.warn(
{ transferId: pendingRow.id },
"subscription.claim.settlement_deferred_provider_unreachable",
);
continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Prevent unreachable providers from starving the settlement queue.

Only 20 unchanged pending rows are fetched. If those repeatedly return unknown, the same rows can consume every sweep while later due transfers never settle. Add persisted retry scheduling or cursor-based scanning that advances past deferred rows while bounding provider calls.

🤖 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/subscriptions/claim.ts` around lines 559 - 577, Update the settlement
sweep around the findMany query and per-row checker in the subscription claim
flow so rows whose entitlement is "unknown" are persisted with a later retry
time or otherwise excluded from the current scan, allowing subsequent due
transfers to be processed while retaining a bound on provider calls. Ensure
deferred rows remain eligible for future sweeps and preserve existing
committed/cancelled handling.

installAppleStatusMap({ [otx]: { status: 1, signedLatest: jws } });
expect((await claimRequest(claimer, jws)).status).toBe(202);

resetAppleApiClientForTests(); // provider calls now fail

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the test and fixture helpers mentioned in the review.
git ls-files 'tests/deletion/adversarial-round3.test.ts' '*apple*' '*Apple*' '*status*' '*fixture*' | sed -n '1,200p'

echo
echo '--- outline: tests/deletion/adversarial-round3.test.ts ---'
ast-grep outline tests/deletion/adversarial-round3.test.ts --view expanded || true

echo
echo '--- relevant references ---'
rg -n "resetAppleApiClientForTests|installAppleStatusMap|Apple client|provider|network|status map" tests src . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'

Repository: xmtplabs/convos-backend

Length of output: 50380


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- outline: tests/deletion/reclaim-fixtures.ts ---'
ast-grep outline tests/deletion/reclaim-fixtures.ts --view expanded || true

echo
echo '--- relevant lines: tests/deletion/reclaim-fixtures.ts ---'
sed -n '1,340p' tests/deletion/reclaim-fixtures.ts

echo
echo '--- outline: src/subscriptions/apple-server-api.ts ---'
ast-grep outline src/subscriptions/apple-server-api.ts --view expanded || true

echo
echo '--- relevant lines: src/subscriptions/apple-server-api.ts ---'
sed -n '1,260p' src/subscriptions/apple-server-api.ts

echo
echo '--- relevant lines: tests/subscriptions/apple-server-api.test.ts ---'
sed -n '1,240p' tests/subscriptions/apple-server-api.test.ts

Repository: xmtplabs/convos-backend

Length of output: 20261


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- relevant lines: tests/deletion/adversarial-round3.test.ts (400-445) ---'
sed -n '400,445p' tests/deletion/adversarial-round3.test.ts

echo
echo '--- surrounding Apple fixture usage in the file ---'
sed -n '445,470p' tests/deletion/adversarial-round3.test.ts

echo
echo '--- search for APPLE_* env in tests/setup files ---'
rg -n "APPLE_(ENV|BUNDLE_ID|API_ISSUER_ID|API_KEY_ID|API_SIGNING_KEY)" tests src . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'

Repository: xmtplabs/convos-backend

Length of output: 20243


Keep this case on a rejecting Apple fixture. resetAppleApiClientForTests() only clears the cached client; if Apple env is present, settlement can rebuild the real App Store client and stop being hermetic. Use installAppleStatusMap({}) here so the provider call fails deterministically.

🤖 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 `@tests/deletion/adversarial-round3.test.ts` at line 430, Update the test setup
around resetAppleApiClientForTests so this case installs an empty Apple status
map via installAppleStatusMap({}) instead of relying on the reset alone. Keep
the fixture hermetic and ensure the provider call deterministically rejects even
when Apple environment variables are present.

Source: Coding guidelines

Comment on lines +341 to +345
const alias = await prisma.lineageTokenAlias.findUniqueOrThrow({
where: { token: tNew },
});
expect(alias.lineageId).toBe(l2.id);
expect(l1.state).toBe("tombstoned");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reload the lineage before asserting its post-event state.

l1 was fetched before applyNotification, so expect(l1.state) always observes the original value and cannot detect an accidental state transition.

Proposed fix
     expect(alias.lineageId).toBe(l2.id);
-    expect(l1.state).toBe("tombstoned");
+    const refreshedL1 = await prisma.subscriptionLineage.findUniqueOrThrow({
+      where: { id: l1.id },
+    });
+    expect(refreshedL1.state).toBe("tombstoned");
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const alias = await prisma.lineageTokenAlias.findUniqueOrThrow({
where: { token: tNew },
});
expect(alias.lineageId).toBe(l2.id);
expect(l1.state).toBe("tombstoned");
const alias = await prisma.lineageTokenAlias.findUniqueOrThrow({
where: { token: tNew },
});
expect(alias.lineageId).toBe(l2.id);
const refreshedL1 = await prisma.subscriptionLineage.findUniqueOrThrow({
where: { id: l1.id },
});
expect(refreshedL1.state).toBe("tombstoned");
🤖 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 `@tests/deletion/adversarial-round4.test.ts` around lines 341 - 345, Reload
lineage l1 from the database after applyNotification and before the post-event
state assertion, then assert the reloaded record’s state is "tombstoned" instead
of using the stale l1 object. Keep the existing alias assertion unchanged.

Comment on lines +405 to +406
const contestEndsAt = new Date(body(res).contestEndsAt ?? "").getTime();
expect(contestEndsAt).toBeGreaterThan(Date.now() + 71 * 60 * 60 * 1000);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Bound the fallback window on both sides.

This passes if the fallback accidentally becomes 100 hours. Assert an upper bound so the test verifies the documented 72-hour default.

Proposed fix
 const contestEndsAt = new Date(body(res).contestEndsAt ?? "").getTime();
-expect(contestEndsAt).toBeGreaterThan(Date.now() + 71 * 60 * 60 * 1000);
+const remainingMs = contestEndsAt - Date.now();
+expect(remainingMs).toBeGreaterThan(71 * 60 * 60 * 1000);
+expect(remainingMs).toBeLessThan(73 * 60 * 60 * 1000);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const contestEndsAt = new Date(body(res).contestEndsAt ?? "").getTime();
expect(contestEndsAt).toBeGreaterThan(Date.now() + 71 * 60 * 60 * 1000);
const contestEndsAt = new Date(body(res).contestEndsAt ?? "").getTime();
const remainingMs = contestEndsAt - Date.now();
expect(remainingMs).toBeGreaterThan(71 * 60 * 60 * 1000);
expect(remainingMs).toBeLessThan(73 * 60 * 60 * 1000);
🤖 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 `@tests/deletion/claim.test.ts` around lines 405 - 406, Update the
contestEndsAt assertion in the deletion test to retain the existing lower-bound
check and add an upper-bound check confirming the fallback is no later than 72
hours from the current time. Use the same contestEndsAt value and allow only the
intended timing tolerance.

lourou added 5 commits July 22, 2026 16:55
…n record

Adds nullable DeletionRecord.finalBalanceCredits (display-only): a plain
read of UserCredits.balance taken inside the teardown transaction, before
any escrow debit or the wallet teardown, stable under the Account FOR
UPDATE lock. Missing wallet rows record 0 (getBalance semantics); records
written before the field stay null.
…is already gone

The notification-installation executor treated every ConnectError as
retryable, so a NotFound — or the bare HTTP 404 the Connect protocol maps
to unimplemented — retried to terminal failure and pinned the
DeletionRecord in purging. Those codes mean the installation is already
absent: classify them as success so the task completes; transient codes
(unavailable, timeouts, 5xx) keep their retry semantics.
…rdown

The escrow-before-wallet-teardown ordering in the deletion transaction was
enforced only by a comment: run the other way round, computeMoveAmount's
balance lock upserts a fresh zero wallet, escrow silently conserves 0, and
the recreated UserCredits row breaks the Account delete on its RESTRICT
FK. escrowCustody now throws EscrowWalletMissingError when the holder's
wallet row is missing (a held custody's owner always has one — funding
created it), turning the mis-ordering into a loud failure.
…t-live-transfer

# Conflicts:
#	tests/deletion/delete-account.test.ts
lourou added 4 commits July 24, 2026 15:43
…ay escrow forfeit

The 'Escrow forfeit' bullet claimed unclaimed escrow forfeits at the
30-day tombstone expiry and that the expiry sweep purges the rows. The
code does neither: expireDeletionRecords deletes only the DeletionRecord
and its DeletionTask outbox; SubscriptionLineage, LineagePeriodGrant, and
LineagePeriodCustody are on no sweep, so re-verify dedup outlives the
record. Documents the two retention clocks and flags the claim-window
bound (none in code today; 30d constants are cooldown/undo) as the open
product question.
Macroscope replay scenario: delete a subscribed account, expire the
DeletionRecord past the audit window, then re-verify the same OTX from a
fresh account. Pins that the expiry sweep leaves the tombstoned lineage,
funding registry, and escrow custody in place, the re-verify still throws
SubscriptionTombstonedError, and no subscription row or period grant is
created — so a future sweep widened to subscription-scoped rows fails CI.
lourou added 3 commits August 4, 2026 11:40
…t-live-transfer

# Conflicts:
#	src/api/v2/subscriptions/handlers/google-play-rtdn.ts
#	src/subscriptions/repository.ts
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant