Repository navigation
refactor(account): remove legacy D1 lease runtime - #1201
Merged
Merged
Conversation
…only - withAccountWriteLease: env required; remove D1 acquire/release path, D1/DO union, waitUntil param, and active_write_count plumbing - markAccountDeleting: env required; D1 deleting_at set first (gate), then UserMeter markDeleting; no D1 lease snapshot or loading - listActiveAccountWriteLeases: (env, userId) only; UserMeter page walk; no D1 union - repairAccountWriteLease: env required; DO-only prepare/finalize; D1 audit row kept; no stale D1 clear - Remove dead waitUntil params from all call sites - D1 users.deleting_at remains the permanent point gate Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
- Remove shadow types: UserMeterWriteLeaseShadow (replaced by UserMeterWriteLeaseEntry without authority field) - Remove listDoAuthorityWriteLeases, replaceLegacyWriteLeases, and assertWriteLeaseAuthority - Remove authority discriminated union behavior from acquireWriteLease, releaseWriteLease, prepareWriteLeaseRepair, finalizeWriteLeaseRepair - Keep warm authority column/shim for schema compatibility; code treats every row as authoritative DO; will drop after schema_version >= 7 - Simplify test-support/user-meter.ts to match: remove authority field from WriteLeaseRow, remove finalizeWriteLeaseRepair authority guard Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
…ease query - DeletionParity: drop d1ActiveLeaseCount/doAuthorityLeaseCount/doLegacyLeaseCount/ tokenSetMismatches/temporaryMirrorRetired/mirrorLeaseParity; keep d1DeletingAt, meterDeletingAt, deletingAtParity, activeLeaseCount, truncated - readDeletionParity: read D1 deleting_at + UserMeter deletingAt/countActiveWriteLeases; no D1 account_write_leases query - admin-user-meter-parity: update deletionParitySchema, description, and keywords to match new DeletionParity type - Tests: remove bootstrapDeletionState/UserMeterWriteLeaseShadow, rewrite to use meter.markDeleting and new parity shape; remove split-authority tests Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
- deletion-state.node.test.ts: rewrite for meter-only mark/list/repair; remove D1-only lease paths and waitUntil; add env-required tests; verify D1 deleting_at gate and races; add export/purge tombstone preservation test - account-deletion.node.test.ts: acquire UserMeter lease to simulate active writer; verify deletion is blocked then proceeds after lease release - user-meter.workers.test.ts: remove shadowAcquireWriteLease, listDoAuthorityWriteLeases, bootstrapDeletionState, authority field - service.node.test.ts files: remove writeLeaseDb mock hooks; batch mock runs statements directly (no D1 account_write_leases queries on runtime path) Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
- account-write-lease-repair.md: remove legacy email / D1 lease paths; describe DO-only repair flow; note D1 account_write_leases quiescent - data-storage.md: note authority column warm/ignored; all callers supply env; D1 account_write_leases quiescent; write-lease rows clear on release/repair/purge - entitlements.md: rewrite Account-deletion write fencing section; mark contract complete 2026-08-03; remove split-authority/Phase-B/mirror-retired prose; update primitives table (activeLeaseCount replaces mirrorLeaseParity/doOnly) - primitives.yaml: update User meter summary to reflect authoritative DO leases 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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUserMeter is now the sole runtime authority for account write leases. D1 lease rows remain for audit and history but are not queried or written at runtime. Account deletion repair, parity reporting, integrations, tests, and documentation now use the DO-only model. ChangesUserMeter lease authority
Estimated code review effort: 4 (Complex) | ~45 minutes 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 |
Contributor
|
🔎 Preview deployed: https://kody-pr-1201.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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/account/deletion-state.ts (1)
280-289: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAwaiting the release in
finallymasks write errors and fails committed writes.Line 287 awaits
releaseWriteLeaseinsidefinally. Two problems follow.
- If
write()throws, or if the held check at Line 283 throwsAccountWriteLeaseLostError, and the release call then rejects, the release rejection replaces the original error. Callers lose the real failure reason.- If
write()succeeds and only the release round trip fails, the whole operation rejects. The write already committed. The caller sees a failure for committed work, and the lease stays active until manual repair.Handle the release failure separately from the pending error.
🐛 Proposed fix
try { const result = await input.write() const held = await meter.assertWriteLeaseHeld({ token: lease.token }) if (!held.held) throw new AccountWriteLeaseLostError() + input.frame.active = false + // Release failures on the success path must surface: the lease would + // otherwise stay active until manual repair. + await meter.releaseWriteLease({ token: lease.token }) return result - } finally { + } catch (error) { input.frame.active = false - await meter.releaseWriteLease({ token: lease.token }) + // Preserve the original failure; a release error must not replace it. + await meter + .releaseWriteLease({ token: lease.token }) + .catch(() => undefined) + throw error } }🤖 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/account/deletion-state.ts` around lines 280 - 289, Update the cleanup in the write flow surrounding input.write, the held check, and the finally block so releaseWriteLease failures are handled separately from the pending operation error: preserve and rethrow any write or lease-loss error, while not rejecting a successfully committed write solely because lease release fails. Keep input.frame.active reset unconditionally and ensure releaseWriteLease is still attempted.
🧹 Nitpick comments (6)
packages/worker/src/account/deletion-state.node.test.ts (2)
834-934: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a failing
releaseWriteLease.
createGatedDoEnvat Lines 71-130 already gatesreleaseWriteLease. Extend it with a release-failure mode. Then assert thatwithAccountWriteLeasestill surfaces the error thrown bywrite()and does not replace it with the release error. This covers the error-masking issue raised onpackages/worker/src/account/deletion-state.tsLines 280-289.🤖 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/account/deletion-state.node.test.ts` around lines 834 - 934, Add a release-failure mode to createGatedDoEnv and use it in a test covering withAccountWriteLease. Make the test verify that when write() throws and releaseWriteLease also fails, withAccountWriteLease propagates the original write error rather than masking it with the release error.
276-347: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the now-duplicated nested and detached lease tests.
The tests at Lines 276-312 and Lines 421-448 assert identical nested-reuse behavior. The detached tests at Lines 349-390 and Lines 463-483 also converge. Before this change the pairs exercised separate D1 and DO paths. Only the DO path remains, so the pairs now run the same code with different holder names. Merge each pair and keep the D1-cleanliness assertions.
Also applies to: 421-448
🤖 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/account/deletion-state.node.test.ts` around lines 276 - 347, The nested lease tests in “nested same-user lease reuses the outer lease instead of re-acquiring” and its duplicate should be consolidated into one test, preserving the shared reuse and D1-cleanliness assertions while removing redundant holder-specific coverage. Likewise merge the duplicate detached lease tests into a single test, retaining the behavior and cleanup assertions required for the remaining DO path.packages/worker/src/account/deletion-state.ts (3)
357-375: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDistinguish the two failure branches.
Line 373 and Line 375 throw the same message for different states. Line 373 means an audit row exists but the UserMeter lease is still held. Line 375 means no audit row matched. During an incident these two states require different operator actions. Keep the caller-facing message identical if the text is part of the API contract, but add a distinguishing log or error cause.
🤖 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/account/deletion-state.ts` around lines 357 - 375, The two failure branches in the lost-finalize retry flow currently throw identical errors despite representing different states. Update the existingAudit branch after assertWriteLeaseHeld, and the no-existingAudit branch after it, to preserve the caller-facing message while adding distinct diagnostic logging or error causes that identify “audit exists but lease remains held” versus “no matching audit exists.”
291-315: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the pagination loop.
The loop at Line 299 has no page limit. It accumulates every lease into one array. The admin capability at
packages/worker/src/mcp/capabilities/admin/admin-account-write-lease-list.tsLines 35-38 returns that array in full. This function exists to inspect leaked leases, so the pathological case is exactly the case with many rows. Add a page cap so a degenerate account cannot produce an unbounded response or an unbounded loop if a future DO change returns a non-advancing cursor.♻️ Proposed cap
const leases: Array<ListedAccountWriteLease> = [] let startAfter: string | null = null - for (;;) { + const maxPages = 20 + for (let page = 0; page < maxPages; page++) { const page = await meter.listWriteLeases({Rename the inner
pagevariable to avoid shadowing, then break when the cap is reached.🤖 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/account/deletion-state.ts` around lines 291 - 315, Bound pagination in listActiveAccountWriteLeases by introducing a fixed maximum page/lease limit, renaming the inner page variable to avoid shadowing, and stopping once the cap is reached or the cursor no longer advances. Ensure the returned leases array and loop remain bounded even if the service reports a non-advancing cursor.
149-184: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate the
USER_METERbinding before the D1 write.
requireUserMeterEnvat Line 179 is a pure local check with no side effect. It runs after the D1deleting_atwrite. If the binding is missing, the account is already marked for deletion in D1 and the function throws. Moving the check to the top fails fast without leaving a partially applied deletion mark. The documented "D1 gate first" ordering is preserved, because the check performs no remote call.♻️ Proposed reordering
export async function markAccountDeleting(input: { db: D1Database dbUserId: number now?: Date env: UserMeterEnv }) { + // Validate the binding first: this is a local check with no side effect, + // so it must not leave a D1 deletion mark behind on misconfiguration. + const env = requireUserMeterEnv(input.env) // D1 deleting_at is set first: it is the permanent point gate and must be // written before any UserMeter call so the gate is never missed even if // the DO call subsequently fails. const now = utcSqliteTimestamp(input.now ?? new Date()) @@ - const env = requireUserMeterEnv(input.env) const marked = await userMeterRpc({ env, userId: stableUserId, }).markDeleting({ deletingAt })🤖 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/account/deletion-state.ts` around lines 149 - 184, Move the pure requireUserMeterEnv(input.env) validation to the beginning of the account-deletion flow, before the D1 UPDATE that writes deleting_at. Reuse the validated env for userMeterRpc after the D1 gate and row validation, preserving the existing D1-before-remote-call ordering.packages/worker/src/jobs/service.node.test.ts (1)
410-416: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated sequential
batchmock into shared test support. RemovingrunWriteLeaseBatchleft three files with an identical hand-rolled loop that runs each statement in order and collects the results. None of the three simulates batch atomicity, so a future rollback expectation would need the same edit in three places.
packages/worker/src/jobs/service.node.test.ts#L410-L416: replace the inline loop with a shared helper exported frompackages/worker/src/test-support/user-meter.tsor a sibling test-support module.packages/worker/src/package-registry/service.node.test.ts#L1070-L1076: replace the inline loop with the same shared helper.packages/worker/src/mcp/run-kody-registry.node.test.ts#L631-L637: replace the inline loop with the same shared helper.🤖 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/service.node.test.ts` around lines 410 - 416, Extract the repeated sequential batch mock into one shared test-support helper, exported from packages/worker/src/test-support/user-meter.ts or a sibling test-support module. Update the batch implementations in packages/worker/src/jobs/service.node.test.ts lines 410-416, packages/worker/src/package-registry/service.node.test.ts lines 1070-1076, and packages/worker/src/mcp/run-kody-registry.node.test.ts lines 631-637 to delegate to that helper while preserving statement order and collected results; no direct loop should remain at those sites.
🤖 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 `@packages/worker/src/account/deletion-state.node.test.ts`:
- Around line 1047-1048: Update the rejection handling around held.operation in
the deletion-state test to assert that withAccountWriteLease rejects with
AccountWriteLeaseLostError after purge(), rather than swallowing any error.
Apply the same change to the matching pattern near the other referenced test
location, while preserving the existing lease-finish and operation flow.
In `@packages/worker/src/mcp/capabilities/admin/admin-user-meter-parity.ts`:
- Line 101: Update the report description string in the admin-user-meter parity
capability to replace “deletion write leases” with wording such as “deletion
state,” accurately reflecting tombstone parity and the UserMeter-only active
lease count.
---
Outside diff comments:
In `@packages/worker/src/account/deletion-state.ts`:
- Around line 280-289: Update the cleanup in the write flow surrounding
input.write, the held check, and the finally block so releaseWriteLease failures
are handled separately from the pending operation error: preserve and rethrow
any write or lease-loss error, while not rejecting a successfully committed
write solely because lease release fails. Keep input.frame.active reset
unconditionally and ensure releaseWriteLease is still attempted.
---
Nitpick comments:
In `@packages/worker/src/account/deletion-state.node.test.ts`:
- Around line 834-934: Add a release-failure mode to createGatedDoEnv and use it
in a test covering withAccountWriteLease. Make the test verify that when write()
throws and releaseWriteLease also fails, withAccountWriteLease propagates the
original write error rather than masking it with the release error.
- Around line 276-347: The nested lease tests in “nested same-user lease reuses
the outer lease instead of re-acquiring” and its duplicate should be
consolidated into one test, preserving the shared reuse and D1-cleanliness
assertions while removing redundant holder-specific coverage. Likewise merge the
duplicate detached lease tests into a single test, retaining the behavior and
cleanup assertions required for the remaining DO path.
In `@packages/worker/src/account/deletion-state.ts`:
- Around line 357-375: The two failure branches in the lost-finalize retry flow
currently throw identical errors despite representing different states. Update
the existingAudit branch after assertWriteLeaseHeld, and the no-existingAudit
branch after it, to preserve the caller-facing message while adding distinct
diagnostic logging or error causes that identify “audit exists but lease remains
held” versus “no matching audit exists.”
- Around line 291-315: Bound pagination in listActiveAccountWriteLeases by
introducing a fixed maximum page/lease limit, renaming the inner page variable
to avoid shadowing, and stopping once the cap is reached or the cursor no longer
advances. Ensure the returned leases array and loop remain bounded even if the
service reports a non-advancing cursor.
- Around line 149-184: Move the pure requireUserMeterEnv(input.env) validation
to the beginning of the account-deletion flow, before the D1 UPDATE that writes
deleting_at. Reuse the validated env for userMeterRpc after the D1 gate and row
validation, preserving the existing D1-before-remote-call ordering.
In `@packages/worker/src/jobs/service.node.test.ts`:
- Around line 410-416: Extract the repeated sequential batch mock into one
shared test-support helper, exported from
packages/worker/src/test-support/user-meter.ts or a sibling test-support module.
Update the batch implementations in
packages/worker/src/jobs/service.node.test.ts lines 410-416,
packages/worker/src/package-registry/service.node.test.ts lines 1070-1076, and
packages/worker/src/mcp/run-kody-registry.node.test.ts lines 631-637 to delegate
to that helper while preserving statement order and collected results; no direct
loop should remain at those sites.
🪄 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: 3ed979fe-951b-44dd-8c2c-6988c0231412
📒 Files selected for processing (26)
docs/contributing/account-write-lease-repair.mddocs/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/user-owned-surfaces.tspackages/worker/src/admin/user-meter-parity.node.test.tspackages/worker/src/admin/user-meter-parity.tspackages/worker/src/app/account-deletion.node.test.tspackages/worker/src/billing/stripe-plan-refresh-do.tspackages/worker/src/entitlements/user-meter-do.tspackages/worker/src/entitlements/user-meter.workers.test.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp-auth.tspackages/worker/src/mcp-auth.workers.test.tspackages/worker/src/mcp/capabilities/admin/admin-account-write-lease-list.tspackages/worker/src/mcp/capabilities/admin/admin-user-meter-parity.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-user-meter-parity.tspackages/worker/src/mcp/memory/service.node.test.tspackages/worker/src/mcp/memory/service.tspackages/worker/src/mcp/run-kody-registry.node.test.tspackages/worker/src/package-registry/service.node.test.tspackages/worker/src/package-registry/service.tspackages/worker/src/test-support/user-meter.ts
💤 Files with no reviewable changes (7)
- packages/worker/src/package-registry/service.ts
- packages/worker/src/billing/stripe-plan-refresh-do.ts
- packages/worker/src/mcp/memory/service.ts
- packages/worker/src/mcp/memory/service.node.test.ts
- packages/worker/src/mcp-auth.ts
- packages/worker/src/jobs/service.ts
- packages/worker/src/mcp-auth.workers.test.ts
Comment on lines
+1047
to
+1048
| held.finish() | ||
| await held.operation.catch(() => {}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the expected rejection instead of swallowing it.
await held.operation.catch(() => {}) accepts any failure. After purge() the lease row is gone, so assertWriteLeaseHeld returns false and withAccountWriteLease throws AccountWriteLeaseLostError. Assert that specific error. The current form would also pass if the operation failed for an unrelated reason. The same pattern appears at Line 1005.
💚 Proposed assertion
held.finish()
- await held.operation.catch(() => {})
+ await expect(held.operation).rejects.toBeInstanceOf(AccountWriteLeaseLostError)
})📝 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
| held.finish() | |
| await held.operation.catch(() => {}) | |
| held.finish() | |
| await expect(held.operation).rejects.toBeInstanceOf(AccountWriteLeaseLostError) |
🤖 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/account/deletion-state.node.test.ts` around lines 1047 -
1048, Update the rejection handling around held.operation in the deletion-state
test to assert that withAccountWriteLease rejects with
AccountWriteLeaseLostError after purge(), rather than swallowing any error.
Apply the same change to the matching pattern near the other referenced test
location, while preserving the existing lease-finish and operation flow.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Finish the runtime side of the write-lease cutover after the final email callers moved to UserMeter and production D1 lease inventory drained to zero.
Summary
USER_METERfor mark/acquire/list/repair APIs and remove the optional D1 lease branchwaitUntilplumbing, and test-only shadow/bootstrap RPCsTesting
CI=1 npm run validate— passed: 553 files / 1,899 tests, 7 E2E, 3 MCPSystem changes
System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@a90dc912· Head:41dbeedbClassification: extends — UserMeter becomes the only runtime write-lease authority and the deletion/admin contracts remove their legacy D1 variants.
Primitives touched
user-meterapp-uimcp-serverjobssaved-packagesbillingrbacSystem map
All user-owned write surfaces now fence through the per-user meter; D1 retains only the permanent deletion tombstone and quiescent pending-drop schema.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
env; D1 or UserMeter lease pathenv; UserMeter-only lease pathInvariants
users.deleting_atremains the fail-closed permanent point gate.Conductor report
95f2150eat 01:13Z. No D1 lease was acquired after deploy.e72aac17-b95c-4b3a-8afd-08e3bb9d8893); D1 active lease count was 0 at 01:18Z; target parity hadd1Only=0,legacyWithoutD1=0, tombstone parity true.envand no mailbox data-authority code changes here.authoritycolumn/index; no runtime query references it. Physical DO cleanup is deferred until all objects are confirmed >=7.account_write_leasesand dead user columns remain physically present but quiescent. Their drop is a separate destructive PR that will stop for conductor review.Summary by CodeRabbit
Reliability
Administration
Documentation