fix(email): hand storage reservations to UserMeter - #1136
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>
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>
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>
Restore deleteEmailMessageById to D1 batch then immediate R2 cleanup with no Mailbox env/waitUntil/mirror. Restore insertEmailMessageWithAttachments signature without mirror forwarding. Drop PR-only delete mirror tests and update data-storage.md: live explicit/retention deletes are repaired by parity purge/rebuild; direct delete wiring remains pending. 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>
…doff Add a focused workers test that sends a successful outbound email, waits for the best-effort D1→UserMeter storage shadow, and asserts readStorageBytes matches authoritative users.d1_storage_bytes. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Exercise handleInboundEmail storage-byte reservation with real UserMeter: capture ExecutionContext waitUntil, drain shadow sync, and assert readStorageBytes matches authoritative users.d1_storage_bytes. Also cover over-quota rejection without meter drift and pre-commit retry without double-reserving D1 storage. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughInbound and outbound email flows now use ChangesEmail storage metering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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>
|
🔎 Preview deployed: https://kody-pr-1136.kody-a99.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/worker/src/email/inbound.ts (1)
479-491: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the single
waitUntiladapter instead of building it twice.Lines 554-556 create the same
ctx-bound adapter as lines 488-490. Move thatconst waitUntildeclaration above this block and reuse it here. This keeps one definition of the deferral contract inhandleInboundEmail.♻️ Proposed refactor
+ const waitUntil = ctx + ? (promise: Promise<unknown>) => ctx.waitUntil(promise) + : undefined let delivery = activeWindow ?? candidateDeliveryawait assertWithinStorageBytesEntitlement({ db: env.APP_DB, env, userId, email: account.email, requested: estimateInboundEmailStorageBytes({ message, recipient, }), - waitUntil: ctx - ? (promise: Promise<unknown>) => ctx.waitUntil(promise) - : undefined, + waitUntil, })Then remove the later duplicate declaration at lines 554-556.
🤖 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/email/inbound.ts` around lines 479 - 491, In handleInboundEmail, hoist the ctx-bound waitUntil adapter above the assertWithinStorageBytesEntitlement call and pass that shared variable into the request. Remove the duplicate waitUntil declaration later in the function so the deferral contract has one definition.packages/worker/src/email/inbound-storage-meter.workers.test.ts (2)
108-296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit this test into separate
testcases per scenario.One test now covers five independent scenarios: happy-path shadowing, over-quota rejection, blob-failure reservation retention, retry without double reservation, and single-message persistence. The scenarios share no required state, because each one seeds its own user. A failure in the first scenario hides the remaining four, and the 30-second timeout applies to the whole chain.
Extract the over-quota scenario (lines 172-213) and the retry scenario (lines 215-293) into their own tests. Move
ensureEmailTestSchemaandensureUsageRollupsTestSchemainto a shared setup 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/email/inbound-storage-meter.workers.test.ts` around lines 108 - 296, Split the combined inbound storage test into independent test cases for happy-path shadowing, over-quota rejection, and retry behavior, preserving each scenario’s existing assertions and isolated seeded user state. Extract the over-quota block and retry block from the current test into separate tests, and add a shared setup helper that performs ensureEmailTestSchema and ensureUsageRollupsTestSchema before each case.
45-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the production estimator instead of re-implementing it.
inbound.tsalready definesestimateInboundEmailStorageBytes, but it is not exported. Make it available toinbound-storage-meter.workers.test.tsand import it here so the test exercises the same byte formula used by production.🤖 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/email/inbound-storage-meter.workers.test.ts` around lines 45 - 60, Export estimateInboundEmailStorageBytes from inbound.ts and replace the duplicate local implementation in inbound-storage-meter.workers.test.ts with an import of that production function, ensuring the test uses the same storage-byte calculation.
🤖 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/email/inbound.ts`:
- Around line 479-491: Update the inbound email entitlement flow around
assertWithinStorageBytesEntitlement to ensure UserMeter shadow work completes
without an ExecutionContext: pass ctx.waitUntil when ctx exists, and otherwise
await the scheduled shadow promise before the request exits. Preserve the
existing entitlement validation and use the existing
scheduleUserMeterStorageBytesShadow path rather than bypassing it.
---
Nitpick comments:
In `@packages/worker/src/email/inbound-storage-meter.workers.test.ts`:
- Around line 108-296: Split the combined inbound storage test into independent
test cases for happy-path shadowing, over-quota rejection, and retry behavior,
preserving each scenario’s existing assertions and isolated seeded user state.
Extract the over-quota block and retry block from the current test into separate
tests, and add a shared setup helper that performs ensureEmailTestSchema and
ensureUsageRollupsTestSchema before each case.
- Around line 45-60: Export estimateInboundEmailStorageBytes from inbound.ts and
replace the duplicate local implementation in
inbound-storage-meter.workers.test.ts with an import of that production
function, ensuring the test uses the same storage-byte calculation.
In `@packages/worker/src/email/inbound.ts`:
- Around line 479-491: In handleInboundEmail, hoist the ctx-bound waitUntil
adapter above the assertWithinStorageBytesEntitlement call and pass that shared
variable into the request. Remove the duplicate waitUntil declaration later in
the function so the deferral contract has one definition.
🪄 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: f333e065-60c0-4366-ac94-7863eb3fb3e9
📒 Files selected for processing (4)
packages/worker/src/email/inbound-storage-meter.workers.test.tspackages/worker/src/email/inbound.tspackages/worker/src/email/outbound-storage-shadow.workers.test.tspackages/worker/src/email/outbound.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/email/outbound.ts`:
- Line 572: Update the outbound email flow around reserveEmailStorageBytes to
compensate every reservation when consumeDailyEntitlement, message persistence,
or attachment storage fails before data is committed. Add an idempotent
refund-or-commit workflow for each pre-persistence failure path, and
specifically refund the attachment reservation when attachment storage fails,
while preserving successful committed reservations.
🪄 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: 439736d0-5a96-45d1-81e3-99974bf37986
📒 Files selected for processing (3)
packages/worker/src/email/inbound.tspackages/worker/src/email/outbound.tspackages/worker/src/email/storage-reservation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/email/inbound.ts
| references: input.references ?? [], | ||
| }) | ||
| await assertWithinStorageBytesEntitlement({ | ||
| await reserveEmailStorageBytes({ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Compensate storage reservations when the reserved bytes are not persisted.
reserveEmailStorageBytes durably increments users.d1_storage_bytes before consumeDailyEntitlement at Line 592 and before message persistence at Line 618. If the daily-send check or a later write fails, the request retains storage bytes for data that does not exist. If attachment storage fails at Line 660, the reservation also retains attachment bytes that were not stored.
Add an idempotent refund or commit workflow for every pre-persistence failure path. Refund the attachment portion when attachment storage fails.
🤖 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/email/outbound.ts` at line 572, Update the outbound email
flow around reserveEmailStorageBytes to compensate every reservation when
consumeDailyEntitlement, message persistence, or attachment storage fails before
data is committed. Add an idempotent refund-or-commit workflow for each
pre-persistence failure path, and specifically refund the attachment reservation
when attachment storage fails, while preserving successful committed
reservations.
Summary
Routes inbound/outbound email storage-byte reservations through the existing UserMeter accounting interface. Inbound uses Email Routing
waitUntil(or awaits when no context); outbound awaits the caught accounting task. UserMeter internals, limits, retry/refund behavior, and Mailbox reads are unchanged.Conductor report
reserveEmailStorageBytesSummary by CodeRabbit
Bug Fixes
Tests