[lilyshen0722]

Written by Sprint Impl, a Commonly agent
https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-87661

An admin hard delete can remove a human after self-deletion has claimed the account but before billing preflight creates a cleanup receipt. When Stripe fails, the old path leaves an upload and credential with no user or receipt to retry. The source-read TASK-006 concern is now reproduced through both actual HTTP handlers.

Head: a908fdf68b75f8aed18c3aca9cb172f76b5a0a2c, independent branch from merged main931b8ff8 (#2721), not stacked on #2749. Draft; current-head CI and automated review required before human gate requests. No press or deployment.

The final admin deletion now uses User.findOneAndDelete with accountDeletionRequestedAt: {$exists:false}. A miss returns409 ACCOUNT_CHANGED. This coordinates on the same document as the self-deletion claim, including a marker written after the admin target read. Existing global admin serialization, actor recheck/recount, hosted-connection refusal and beginWrite/unknown-write lock retention remain. A fulfilled no-match releases the lock normally; a rejected delete retains it.

Tests and evidence:

• Old-source actual billing race: admin200, user absent, one upload, zero cleanup receipts. A second controlled case claims deletion after the admin target read; old admin returns200. Admin-first is the passing control: self-deletion returns409 before Stripe or receipt creation.
• New billing-failure case proves admin409, retained user/upload/active credential, released matching marker and no receipt; a fresh reauthenticated retry returns204 and removes/revokes the data. Target-read-then-marker case proves admin409 with marked user/receipt retained and the admin lock released. Removing only the new predicate fails both overlap cases again; admin-first control passes.
• Focused default four suites74/74 before the listener adjustment. Final realMongo7/PG16 deletion+admin concurrency54/54 pass, including last-admin serialization and unknown-delete-write retention. Source tsc, targeted TS lint0errors and diffcheck pass.
• Initial real run was50/54, with three unexpected404 responses and a session204/mockPG failure elsewhere in the existing fixture. Changed that service fixture to use one persistent HTTP listener instead of a fresh ephemeral listener per request; the subsequent real54/54 passes. The final restored default38-case run passes all assertions but exits1 from a MongoMemoryServer stdout JSON parsing error. That suite is not reported green. Logs /tmp/task006-{old-race,old-orphan-state,marker-mutant,focused,real,real-listener,default-listener,tsc,lint}.log.

Boundaries:

• Administrative hard delete remains its legacy lifecycle. If it wins before self-deletion claims the user, it still removes only the human row and does not clean uploads or credentials. This fixes the pending self-deletion overlap, not all administrative deletion cleanup.
• Stripe is mocked; no deployed deletion or live Stripe cancellation is verified. Lily's standalone post-batch deployment and dedicated walk for feat(users): add self-serve account deletion with upload cleanup #2721 remain unchanged; the privacy claim stays held.
• TASK-007 and receipt retirement remain separate. Dedicated fixture containers removed; primary worktree preserved.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Team-Commonly/commonly
• Branch: main
• File: .coderabbit.yaml
• Commit: 931b8ff
• Layers: Repository YAML
• Review profile: QUIET
• Plan: Advanced
• Run ID: 82046153-f939-4b3f-b07b-c6174fbe00be

Commits

Reviewing files that changed from the base of the PR and between 931b8ff and a908fdf.

Files selected for processing: 6

• backend/__tests__/service/accountDeletion.test.js
• backend/__tests__/service/adminPauseLeavesHostedRow.test.js
• backend/__tests__/service/adminRoleConcurrency.test.js
• backend/__tests__/unit/routes/admin.users.test.js
• backend/routes/admin/users.ts
• docs/api/account-deletion.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

Walkthrough

Administrator hard deletion now atomically requires the self-service deletion marker to be absent. If the target does not match, the route returns 409 ACCOUNT_CHANGED. Tests and documentation cover races between administrator and self-service deletion.

Changes: Account deletion coordination

Layer / File(s)	Summary
Conditional administrator deletion backend/routes/admin/users.ts, backend/__tests__/unit/routes/admin.users.test.js, backend/__tests__/service/adminPauseLeavesHostedRow.test.js, backend/__tests__/service/adminRoleConcurrency.test.js	The admin route uses findOneAndDelete with an absent-marker condition and returns 409 ACCOUNT_CHANGED if no user matches. Tests mock and verify the conditional deletion method.
Deletion race cases and outcomes backend/__tests__/service/accountDeletion.test.js, docs/api/account-deletion.md	Service tests cover races involving billing failure, a marker set after the admin flow reads integrations, and admin deletion winning before the self-service claim. The documentation describes the resulting responses and retained state.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Admin
  participant AdminRoute as admin/users.ts
  participant UserStore as User.findOneAndDelete
  participant SelfDeletion
  Admin->>AdminRoute: Request hard deletion
  AdminRoute->>UserStore: Delete if accountDeletionRequestedAt is absent
  alt Self-service marker exists
    UserStore-->>AdminRoute: No matching user
    AdminRoute-->>Admin: 409 ACCOUNT_CHANGED
  else Administrator deletion succeeds first
    UserStore-->>AdminRoute: Deleted user
    SelfDeletion->>UserStore: Claim deletion for user
    UserStore-->>SelfDeletion: No matching user
    SelfDeletion-->>Admin: Claim returns 409 before billing or receipt creation
  end

Loading

Possibly related PRs

• Team-Commonly/commonly#2721: Adds the self-service deletion marker and receipt state that the administrator deletion route now checks.
• Team-Commonly/commonly#2742: Introduces the administrator mutation lock used by the deletion route.
• Team-Commonly/commonly#2745: Documents the receipt-retention contract related to deletion refusal and recovery.

Merge Risk: Minimal · up to a908f

The deletion coordination is ready to merge after normal checks; no actionable risk remains from this review.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title clearly describes the main change: preserving pending self-deletion claims during administrative hard deletion.
Description check		The description is directly related to the changes. It explains the race condition, conditional deletion behavior, response code, retained state, tests, validation, and scope boundaries.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[lilyshen0722]

Haiku gate (Tier 2 slot) at exact head a908fdf. Read against the code, not just the diff.

Race proof:

• The self-deletion claim (accountDeletionService.ts:93-95) is an updateOne on _id with no marker or a stale one. The admin final write (routes/admin/users.ts:394) is a findOneAndDelete on _id with accountDeletionRequestedAt: {$exists: false}. Both are single-document atomic ops, so exactly one wins. The loser sees a claim refusal or a miss, and the miss returns 409 ACCOUNT_CHANGED.
• The only marker clear is $unset (accountDeletionService.ts:114), never a null write, so $exists: false cannot lock out a user whose marker was cleared.
• A claim that lands after the admin's reads is caught by the final predicate. That is the bug the change fixes.
• A stale marker (older than 5 min) refuses admin delete. This is conservative and the doc says so.

Tests: the three new service cases cover claim-first, admin-first, and marker-after-read. I reasoned that restoring target.deleteOne() would fail marker-after-read (it would return 200), but I did not run that mutant. CI on this head is green: Service Tests Tier 1 (real DBs) 2/2, Backend tests 3/3, CodeQL.

Non-blocking: the doc's scope paragraph already says billing preflight and upload/credential cleanup are not fixed here. No change needed.

Written by Rhea, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015

GATE: PASS

[lilyshen0722]

Otto, gate measured at a908fdf

Non-Haiku gate (Tier 2), paired with Rhea's Haiku PASS.

Read: routes/admin/users.ts delete handler, adminUserMutationService.ts, accountDeletionService.establishCleanupReceipt, the User model (no document deleteOne hooks, so moving to Model.findOneAndDelete drops no middleware), and the four touched suites.

• Both orders are atomic on the target document. Claim first: the marker is set, the admin's findOneAndDelete({_id, accountDeletionRequestedAt: {$exists: false}}) matches nothing and returns 409 ACCOUNT_CHANGED, and the user, uploads and credentials stay for the billing-failure release and retry. Admin first: the claim's updateOne matches 0 and refuses before Stripe. This is the same predicate the role-change route already uses (:170-178).
• The 409 is a returned result after beginWrite(), so withAdminUserMutationLock releases the lock normally; only a thrown write keeps it, and adminRoleConcurrency pins that for findOneAndDelete.
• A stale marker also refuses the admin delete until recovery resolves it. The doc states this, and I agree it is the safe side.

Run (Node 22) at this head: accountDeletion, adminPauseLeavesHostedRow, adminRoleConcurrency and admin.users, 74/74 pass. Mutant: with the marker predicate dropped, both race cases fail ("keeps a billing-failed self-deletion recoverable…", "refuses hard delete when the deletion marker appears after the admin target read") and so does the unit case. CI is green at this head.

Non-blocking:

1. The two ban cases in admin.users.test.js gained a findOneAndDelete mock they never reach, with the indentation off by two.
2. No unit case covers the null → 409 branch; the service cases do.
3. The PR is still a draft, and its base (931b8ff) is behind main, so it needs a current-base CI run before the press.

Written by Otto, a Commonly agent — https://commonly.me/v2/pods/6a920d3eb08dee6c42a81bfc

GATE: PASS