feat(account): shadow deletion leases in UserMeter - #1123
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
# Conflicts: # packages/worker/src/email/inbound.ts # packages/worker/src/email/outbound.ts # packages/worker/worker-configuration.d.ts Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
# Conflicts: # docs/contributing/architecture/data-storage.md # packages/worker/src/account/export.node.test.ts # packages/worker/src/account/export.ts Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
# Conflicts: # docs/contributing/architecture/data-storage.md Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
# Conflicts: # docs/contributing/architecture/data-storage.md Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughUserMeter schema version 6 adds deletion-state and account-write-lease shadows. D1 remains authoritative while deletion and lease operations synchronize to UserMeter. Exports, purge behavior, call sites, tests, and architecture documentation now cover the shadows. ChangesUserMeter deletion shadow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant D1
participant DeletionState
participant UserMeter
participant waitUntil
D1->>DeletionState: complete deletion or lease operation
DeletionState->>UserMeter: invoke shadow RPC
DeletionState->>waitUntil: schedule promise when available
UserMeter-->>DeletionState: complete or fail without changing D1 result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
@coderabbitai review |
|
🔎 Preview deployed: https://kody-pr-1123.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/worker/src/entitlements/user-meter-do.ts (1)
1400-1418: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDerive
createdfromrowsWritteninstead of value equality.
createdis currently true whenever the stored timestamp equals the input timestamp. If a row already exists with the samedeletingAtvalue, this reportscreated: truealthough no row was written.bootstrapDeletionStatemaps this flag todeletingAtApplied, so the bootstrap report can be misleading.♻️ Proposed change
- this.ctx.storage.sql.exec( + const cursor = this.ctx.storage.sql.exec( `INSERT INTO deletion_state (id, deleting_at) VALUES (?, ?) ON CONFLICT(id) DO NOTHING`, deletionStateRowId, deletingAt, ) - const stored = this.readDeletingAt() ?? deletingAt - return { deletingAt: stored, created: stored === deletingAt } + const stored = this.readDeletingAt() ?? deletingAt + return { deletingAt: stored, created: cursor.rowsWritten > 0 }🤖 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 `@packages/worker/src/entitlements/user-meter-do.ts` around lines 1400 - 1418, Update shadowMarkDeleting to derive created from the INSERT statement’s rowsWritten result rather than comparing stored and input timestamps. Preserve the existing first-write-wins behavior and return the stored timestamp, while ensuring created is false when the conflict leaves an existing row unchanged.packages/worker/src/jobs/job-schedule-watchdog.ts (1)
191-204: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftPass
waitUntilinto watchdog lease repair instead of awaiting it per stuck row.
withAccountWriteLeaseawaits the shadow write/release promises whenwaitUntilis omitted. This loop can include many stuck jobs, so each repair adds two inline lease round trips before proceeding. Threadctx.waitUntilfrom the scheduled path throughrunJobScheduleWatchdogTickand include it in the leasewrite()call so shadow work detaches from the loop.🤖 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 `@packages/worker/src/jobs/job-schedule-watchdog.ts` around lines 191 - 204, Thread ctx.waitUntil from the scheduled entry point through runJobScheduleWatchdogTick, then pass it to withAccountWriteLease in the stuck-job repair block around advanceStuckSkippedJobNextRunAt. Ensure lease shadow write and release work is scheduled through waitUntil rather than awaited inline for each repaired row.
🤖 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/contributing/architecture/data-storage.md`:
- Around line 569-585: The documentation must no longer describe
account_write_leases as unbounded. Update the lease-shadow lifecycle around
shadowReleaseWriteLease and the documented release/repair paths to add an
automated cleanup boundary for stale unreleased or orphaned rows, using
D1-backed cleanup or bounded retention, while preserving active lease rows and
existing fencing behavior.
In `@docs/contributing/architecture/entitlements.md`:
- Around line 316-320: Update UserMeter.exportCounters and the account export
path so UserMeterDeletionShadow.writeLeases excludes the raw
account_write_leases token and holder fields; retain only non-sensitive lease
information such as acquired_at, or introduce a separate sanitized export shape
when identifiers are required.
---
Nitpick comments:
In `@packages/worker/src/entitlements/user-meter-do.ts`:
- Around line 1400-1418: Update shadowMarkDeleting to derive created from the
INSERT statement’s rowsWritten result rather than comparing stored and input
timestamps. Preserve the existing first-write-wins behavior and return the
stored timestamp, while ensuring created is false when the conflict leaves an
existing row unchanged.
In `@packages/worker/src/jobs/job-schedule-watchdog.ts`:
- Around line 191-204: Thread ctx.waitUntil from the scheduled entry point
through runJobScheduleWatchdogTick, then pass it to withAccountWriteLease in the
stuck-job repair block around advanceStuckSkippedJobNextRunAt. Ensure lease
shadow write and release work is scheduled through waitUntil rather than awaited
inline for each repaired row.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c94c5c13-be67-4dd7-b97e-331a98f13a58
📒 Files selected for processing (24)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/entitlements.mddocs/contributing/architecture/primitives.yamlpackages/worker/src/account/deletion-state.node.test.tspackages/worker/src/account/deletion-state.tspackages/worker/src/account/export.node.test.tspackages/worker/src/account/export.tspackages/worker/src/account/user-owned-surfaces.tspackages/worker/src/app/account-deletion.node.test.tspackages/worker/src/app/account-deletion.tspackages/worker/src/app/account-write-lease-middleware.tspackages/worker/src/billing/stripe-plan-refresh-client.tspackages/worker/src/billing/stripe-plan-refresh-do.tspackages/worker/src/community/avatar.tspackages/worker/src/community/community-icon.tspackages/worker/src/entitlements/user-meter-do.tspackages/worker/src/entitlements/user-meter.workers.test.tspackages/worker/src/jobs/job-schedule-watchdog.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp-auth.tspackages/worker/src/mcp/capabilities/admin/admin-account-write-lease-repair.tspackages/worker/src/mcp/memory/service.tspackages/worker/src/package-registry/service.tspackages/worker/src/test-support/user-meter.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 62f6078. Configure here.
| }) | ||
| if (!input.waitUntil) await markShadowPromise | ||
| } | ||
| return activeLeases.length |
There was a problem hiding this comment.
Stale lease snapshot before shadow
Medium Severity
markAccountDeleting reads active D1 write leases once, then may await the UserMeter shadowReplaceDeletionState call (for example when deleteUserAccount passes env without waitUntil). Releases can finish in that gap while the function still returns the old lease count and pushes the same stale list into the shadow. That can trigger a false AccountDeletionWritersActiveError after deleting_at is already set, and leave ghost shadow leases until a later mark retry.
Reviewed by Cursor Bugbot for commit 62f6078. Configure here.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/src/entitlements/user-meter-do.ts (1)
1446-1504: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrefer
transactionSyncoverblockConcurrencyWhilefor this read-modify-write.
shadowReplaceDeletionStatewraps a read-modify-write sequence (readdeletingAt, conditional insert,DELETE FROM account_write_leases, then re-insert) inthis.ctx.blockConcurrencyWhile(async () => {...}). This same file already usesthis.ctx.storage.transactionSync(() => {...})for the related UPDATE+INSERT inconsumeInboundDelivery, so an established in-file convention for atomic multi-statement writes already exists.
blockConcurrencyWhileblocks every other request against this per-user UserMeter Durable Object instance — including unrelated counter, storage-bytes, and package-service-state operations — for the duration of the callback. The blockConcurrencyWhile() method guarantees that no other events are processed until the provided callback completes, even if the callback performs asynchronous I/O... Because blockConcurrencyWhile() blocks all concurrency unconditionally, it significantly reduces throughput. Reserve it for initialization and migrations, not regular request handling... For atomic read-modify-write operations during request handling, prefer transaction() over blockConcurrencyWhile().Switch to
this.ctx.storage.transactionSync(() => {...}), which wraps the same statements in an all-or-nothing SQL transaction without blocking unrelated requests to this DO instance.♻️ Proposed refactor to use `transactionSync`
- return await this.ctx.blockConcurrencyWhile(async () => { + return this.ctx.storage.transactionSync(() => { const existing = this.readDeletingAt() let created = false if (existing == null) { const cursor = this.ctx.storage.sql.exec( `INSERT INTO deletion_state (id, deleting_at) VALUES (?, ?) ON CONFLICT(id) DO NOTHING`, deletionStateRowId, deletingAt, ) created = cursor.rowsWritten > 0 } const stored = this.readDeletingAt() ?? deletingAt this.ctx.storage.sql.exec(`DELETE FROM account_write_leases`) for (const lease of leases) { this.ctx.storage.sql.exec( `INSERT INTO account_write_leases (token, holder, acquired_at) VALUES (?, ?, ?)`, lease.token, lease.holder, lease.acquiredAt, ) } return { deletingAt: stored, created, leaseCount: leases.length, } })Please verify with the Cloudflare Durable Objects SQLite storage documentation that
transactionSyncfully covers this use case (synchronous callback, noawaitinside), since it relates to an external platform API.🤖 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 `@packages/worker/src/entitlements/user-meter-do.ts` around lines 1446 - 1504, Replace the blockConcurrencyWhile wrapper in shadowReplaceDeletionState with this.ctx.storage.transactionSync, keeping the existing synchronous read, conditional insert, lease deletion, lease reinsertion, and result construction inside the transaction callback. Preserve the current validation and returned deletingAt, created, and leaseCount behavior, and do not introduce await usage in the callback.
🤖 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.
Nitpick comments:
In `@packages/worker/src/entitlements/user-meter-do.ts`:
- Around line 1446-1504: Replace the blockConcurrencyWhile wrapper in
shadowReplaceDeletionState with this.ctx.storage.transactionSync, keeping the
existing synchronous read, conditional insert, lease deletion, lease
reinsertion, and result construction inside the transaction callback. Preserve
the current validation and returned deletingAt, created, and leaseCount
behavior, and do not introduce await usage in the callback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d160d053-5ac6-4ee0-98df-1a71e94805c4
📒 Files selected for processing (11)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/entitlements.mdpackages/worker/src/account/deletion-state.node.test.tspackages/worker/src/account/deletion-state.tspackages/worker/src/account/export.node.test.tspackages/worker/src/account/export.tspackages/worker/src/account/user-owned-surfaces.tspackages/worker/src/app/account-deletion.node.test.tspackages/worker/src/entitlements/user-meter-do.tspackages/worker/src/entitlements/user-meter.workers.test.tspackages/worker/src/test-support/user-meter.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/worker/src/account/user-owned-surfaces.ts
- packages/worker/src/account/export.node.test.ts
- docs/contributing/architecture/entitlements.md
- packages/worker/src/entitlements/user-meter.workers.test.ts
- docs/contributing/architecture/data-storage.md
- packages/worker/src/test-support/user-meter.ts
- packages/worker/src/account/deletion-state.ts
- packages/worker/src/account/deletion-state.node.test.ts
- packages/worker/src/account/export.ts
3 participants
Summary
Validation
Deployment notes
Phase A expand only. D1 remains authority. The high-risk authority flip remains a separate PR.
System recap — extends User meter deletion fencing (medium risk)
Mode: recap · Base:
main@e0325f56· Head:62f6078aClassification: extends — adds non-authoritative deletion tombstone and lease shadows without changing D1 fencing authority.
System map
D1 remains the fence; successful non-email transitions shadow into UserMeter and deletion mark reconciles the shadow from D1.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Invariants
Conductor report
deleting_at, and preserve audit-first repair. It will stop green + ready-for-review without self-merge.Summary by CodeRabbit
New Features
Bug Fixes
Documentation