Repository navigation
feat(email): move internal reads to Mailbox - #1160
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughMailbox becomes authoritative for USER email reads, exports, R2 references, deletion, and retention. D1 email tables remain compatibility projections and deletion targets. Tombstones and retry records fence stale writes and support retryable blob cleanup. ChangesMailbox-authoritative email migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant InternalRead
participant Mailbox
participant R2
participant D1
Caller->>InternalRead: request USER email data
InternalRead->>Mailbox: owner-bound read or export
Mailbox-->>InternalRead: authoritative metadata and blob references
InternalRead->>R2: resolve or delete referenced objects
R2-->>InternalRead: object result
InternalRead->>D1: remove compatibility projection when required
InternalRead-->>Caller: aggregate 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>
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>
kentcdodds
marked this pull request as ready for review
August 2, 2026 21:07
Contributor
|
🔎 Preview deployed: https://kody-pr-1160.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/r2-export.ts (1)
264-281: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winAdd a non-advancing pagination guard to prevent an infinite loop.
If
page.referencesis empty whilepage.truncatedis true, the code advancesstartAftertopage.nextStartAfterwithout checking whether it differs from the currentstartAfter. If Mailbox ever returns a non-advancingnextStartAfterin that state,listMailboxEmailObjectRefPageis called again with the identicalstartAfter, producing the identical result. Thewhile (!cursor.current)loop infindNextRefthen repeats synchronously without bound, hanging the request.
visitMailboxBlobReferencePagesinpackages/worker/src/account/mailbox-r2-references.tsguards this exact case by throwing whenpage.nextStartAfter == null || page.nextStartAfter === startAfter. Apply the same guard here.🛠️ Proposed fix for the non-advancing pagination guard
const reference = page.references[0] if (!reference) { if (page.truncated && page.nextStartAfter != null) { + if (page.nextStartAfter === startAfter) { + throw new Error( + 'Mailbox blob-reference pagination did not advance during R2 export.', + ) + } cursor = { v: accountR2CursorVersion, state: { stage: 'mailbox_email_blob', startAfter: page.nextStartAfter, }, } break }🤖 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/r2-export.ts` around lines 264 - 281, In the empty-reference branch of the mailbox pagination loop, add the same non-advancing cursor guard used by visitMailboxBlobReferencePages: before updating cursor with page.nextStartAfter, throw when it is null or equals the current startAfter. Only advance to the next page when the value is different, preserving the existing done-cursor behavior otherwise.
🧹 Nitpick comments (3)
packages/worker/src/account/mailbox-r2-references.node.test.ts (1)
70-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
pageSize > 1inlistMailboxEmailObjectRefPage.The current test only exercises
pageSize: 1. OncelistMailboxEmailObjectRefPageis fixed to track a per-itemstartAfter(see the companion comment onpackages/worker/src/account/mailbox-r2-references.ts, lines 38-61), add a case withpageSize: 2or higher that asserts each returned reference carries a distinct, correctsource.startAftervalue. This guards against the same regression recurring.🤖 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/mailbox-r2-references.node.test.ts` around lines 70 - 98, Extend the test around listMailboxEmailObjectRefPage to request at least two items and assert every returned reference has the correct distinct source.startAfter value for its position. Preserve the existing resolution assertions where applicable, and ensure the coverage would detect reuse of a single startAfter across multiple references.packages/worker/src/admin/mailbox-maintenance.node.test.ts (1)
103-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
canonicalMailboxMessageBlobReferencesin the mock.This block re-implements the production reference derivation from
packages/worker/src/email/mailbox-retention.ts. If the production rules change, for example for theraw_mimedirection condition or the attachment key equality check, the mock keeps the old behavior and the tests still pass.Call the exported helper with the D1-derived attachment list instead.
♻️ Proposed fix
- const blobReferences = [ - ...(message.direction === 'inbound' - ? [ - { - kind: 'raw_mime' as const, - key: emailRawMimeKey(userId, messageId), - messageId, - attachmentId: null, - }, - ] - : []), - ...attachments.flatMap((attachment) => { - if (attachment.storageKind !== 'external') return [] - const expected = emailAttachmentBlobKey( - userId, - messageId, - attachment.id, - ) - return attachment.storageKey === expected - ? [ - { - kind: 'attachment' as const, - key: expected, - messageId, - attachmentId: attachment.id, - }, - ] - : [] - }), - ] + const blobReferences = canonicalMailboxMessageBlobReferences({ + ownerId: userId, + messageId, + direction: message.direction, + attachments: attachments.map((attachment) => ({ + id: attachment.id, + storage_key: attachment.storageKey, + })), + })Add the import:
import { canonicalMailboxMessageBlobReferences } from '`#worker/email/mailbox-retention.ts`'🤖 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/admin/mailbox-maintenance.node.test.ts` around lines 103 - 132, Replace the duplicated blobReferences derivation in the test setup with a call to the exported canonicalMailboxMessageBlobReferences helper from mailbox-retention, passing the D1-derived attachment list and required message context. Add the helper import and remove the local direction, storageKind, and storageKey checks so the mock follows production rules.packages/worker/src/email/repo.ts (1)
1348-1355: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueFence the attachment delete on the owner too.
The ownership check at Line 1340 and the batch at Line 1348 are separate reads and writes. The message delete re-checks
user_id, but the attachment delete uses onlymessage_id. If ownership changes between the two steps, the attachment rows are still removed while the message row is retained.The window is narrow, so this is defensive hardening rather than an active defect.
♻️ Proposed fix
await input.db.batch([ input.db - .prepare(`DELETE FROM email_attachments WHERE message_id = ?`) - .bind(input.messageId), + .prepare( + `DELETE FROM email_attachments + WHERE message_id = ? + AND EXISTS ( + SELECT 1 FROM email_messages + WHERE id = ? AND user_id = ? + )`, + ) + .bind(input.messageId, input.messageId, input.expectedUserId), input.db .prepare(`DELETE FROM email_messages WHERE id = ? AND user_id = ?`) .bind(input.messageId, input.expectedUserId), ])🤖 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/repo.ts` around lines 1348 - 1355, Update the attachment DELETE in the batch within the message deletion flow to also constrain the target by the expected owner, matching the ownership predicate used by the email_messages DELETE. Bind the corresponding user identifier so attachments are removed only when the message belongs to input.expectedUserId.
🤖 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/mailbox-r2-references.ts`:
- Around line 38-61: Update listMailboxEmailObjectRefPage so each mapped
reference’s source.startAfter identifies the item immediately preceding that
reference, rather than reusing input.startAfter for the entire page. Preserve
the source reference and pagination metadata, and ensure the cursor advances per
item so resolveMailboxEmailObjectRef revalidation with pageSize 1 selects the
matching reference for any page size.
In `@packages/worker/src/email/mailbox-do.ts`:
- Around line 972-983: Update purgeForParityRebuild to wrap the transactionSync
purge, alarm deletion, retentionAlarmArmed reset, and retentionIdleConfirmed
update inside the existing blockConcurrencyWhile input gate, matching purge’s
serialization behavior; return { ok: true } after the gated operation completes.
---
Outside diff comments:
In `@packages/worker/src/account/r2-export.ts`:
- Around line 264-281: In the empty-reference branch of the mailbox pagination
loop, add the same non-advancing cursor guard used by
visitMailboxBlobReferencePages: before updating cursor with page.nextStartAfter,
throw when it is null or equals the current startAfter. Only advance to the next
page when the value is different, preserving the existing done-cursor behavior
otherwise.
---
Nitpick comments:
In `@packages/worker/src/account/mailbox-r2-references.node.test.ts`:
- Around line 70-98: Extend the test around listMailboxEmailObjectRefPage to
request at least two items and assert every returned reference has the correct
distinct source.startAfter value for its position. Preserve the existing
resolution assertions where applicable, and ensure the coverage would detect
reuse of a single startAfter across multiple references.
In `@packages/worker/src/admin/mailbox-maintenance.node.test.ts`:
- Around line 103-132: Replace the duplicated blobReferences derivation in the
test setup with a call to the exported canonicalMailboxMessageBlobReferences
helper from mailbox-retention, passing the D1-derived attachment list and
required message context. Add the helper import and remove the local direction,
storageKind, and storageKey checks so the mock follows production rules.
In `@packages/worker/src/email/repo.ts`:
- Around line 1348-1355: Update the attachment DELETE in the batch within the
message deletion flow to also constrain the target by the expected owner,
matching the ownership predicate used by the email_messages DELETE. Bind the
corresponding user identifier so attachments are removed only when the message
belongs to input.expectedUserId.
🪄 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: 1fa81a7d-63bc-4a7e-be8b-0160462821b7
📒 Files selected for processing (54)
docs/contributing/architecture/data-storage.mddocs/contributing/decisions/0002-data-placement.mdpackages/worker/src/account/data-targets.tspackages/worker/src/account/export.node.test.tspackages/worker/src/account/export.tspackages/worker/src/account/mailbox-r2-references.node.test.tspackages/worker/src/account/mailbox-r2-references.tspackages/worker/src/account/r2-export.tspackages/worker/src/account/r2-inventory.tspackages/worker/src/account/user-owned-surfaces.node.test.tspackages/worker/src/account/user-owned-surfaces.tspackages/worker/src/admin/mailbox-maintenance.node.test.tspackages/worker/src/admin/mailbox-maintenance.tspackages/worker/src/admin/user-usage-data.tspackages/worker/src/app/account-deletion.node.test.tspackages/worker/src/app/account-deletion.tspackages/worker/src/app/account-email-data.node.test.tspackages/worker/src/app/account-email-data.tspackages/worker/src/app/account-usage-data.node.test.tspackages/worker/src/app/handlers/account-email.node.test.tspackages/worker/src/email/inbound-effects.tspackages/worker/src/email/inbound-entitlements.workers.test.tspackages/worker/src/email/inbound-mailbox-mirror.workers.test.tspackages/worker/src/email/inbound.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/mailbox-delivery-events.tspackages/worker/src/email/mailbox-do.tspackages/worker/src/email/mailbox-do.workers.test.tspackages/worker/src/email/mailbox-inbound-authority-guard.workers.test.tspackages/worker/src/email/mailbox-inbound-ledger.tspackages/worker/src/email/mailbox-inbound-ledger.workers.test.tspackages/worker/src/email/mailbox-internal-read.node.test.tspackages/worker/src/email/mailbox-internal-read.tspackages/worker/src/email/mailbox-message-deletion-tombstones.tspackages/worker/src/email/mailbox-mutations.tspackages/worker/src/email/mailbox-mutations.workers.test.tspackages/worker/src/email/mailbox-read-cutover.tspackages/worker/src/email/mailbox-reconcile.node.test.tspackages/worker/src/email/mailbox-reconcile.tspackages/worker/src/email/mailbox-reconcile.workers.test.tspackages/worker/src/email/mailbox-record-mappers.tspackages/worker/src/email/mailbox-retention.tspackages/worker/src/email/mailbox-retention.workers.test.tspackages/worker/src/email/mailbox-schema.tspackages/worker/src/email/mailbox-store.tspackages/worker/src/email/mailbox-types.tspackages/worker/src/email/package-subscriptions.node.test.tspackages/worker/src/email/package-subscriptions.tspackages/worker/src/email/repo.tspackages/worker/src/entitlements/service.tspackages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.tspackages/worker/src/mcp/capabilities/email/email-usage-get.tspackages/worker/src/mcp/capabilities/email/email-usage-get.workers.test.tspackages/worker/src/test-support/mailbox-reconcile.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This was referenced Aug 2, 2026
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
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.
Summary
Moves USER internal email metadata consumers and account lifecycle surfaces to fail-closed Mailbox reads:
No D1 email writes/tables are removed in this step.
System recap — extends Mailbox and account lifecycle (medium risk)
Mode: recap · Base:
main@2b0b60e2· Head:ea0cca9bClassification: extends — moves existing USER internal/account reads onto Mailbox and hardens authoritative deletion; no primitive is added.
Primitives touched
mailboxaccount-exportemailemail-blobs-r2entitlementsSystem map
USER internal reads and account lifecycle flow through Mailbox; system email and provider reverse lookup remain D1 exceptions.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Invariants
Verification
ea0cca9b: all required checks pass; Bugbot and CodeRabbit pass with no unresolved threadsConductor report
7787f8c9…remains recordedNote
Medium Risk
Touches account deletion, export, and authoritative email metadata/R2 ordering on a staged migration; changes are bounded by retained D1 projections and fail-closed Mailbox RPCs rather than dropping D1 writers in this step.
Overview
Step 3 cutover: USER internal consumers (inbound effects, package payloads, signed-in
stored_email_messagesusage, account export) route throughmailbox-internal-read.tswith no feature flag or D1 fallback;system:emailstays explicitly D1-backed.Account export & R2: The four USER D1
email_*graph tables are excluded from export (includeInExport: falseon data targets). Themailboxsection andMailbox.listBlobReferencesare the sole USER graph and email-byte inventory;r2_objecttraversal is cursor v2 (legacy v1 cursors must restart withoutstartAfter).Deletion & lifecycle: Account deletion pages blob references, deletes canonical/defensive R2 keys, and only purges Mailbox after email cleanup completes with no warnings. USER admin
delete_messageuses owner-bound R2-before-metadata (deleteMessageWithBlobs); D1 projection cleanup follows. Deletion tombstones and paritypurgeForParityRebuildblock delayed mirror/parity from resurrecting deleted messages.Mailbox DO: Retention processes one R2-backed message per turn with per-message hourly retry rows to avoid head-of-line blocking; mirrors and delivery writes detach tombstoned
message_ids.Docs/ADR updates describe step 3 ordering and the thin D1 reverse-index pattern for externally keyed events.
Reviewed by Cursor Bugbot for commit ea0cca9. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation