feat(email): move inbound authority to Mailbox - #1157
Merged
Merged
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>
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>
📝 WalkthroughWalkthroughUSER inbound delivery state and effects now use Mailbox CAS as the authority. D1 stores synchronous compatibility snapshots. The change adds migration and bootstrap support, authority-based processing, reconciliation, cleanup leases, effect handling, and integration tests. ChangesUSER inbound delivery authority
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant Authority as UserInboundDeliveryAuthority
participant Mailbox
participant UserMeter as USER_METER
participant D1
Worker->>Authority: charge inbound delivery
Authority->>Mailbox: claim dedupe window and create pending delivery
Authority->>UserMeter: charge quota
Authority->>D1: mirror delivery snapshot
Worker->>Authority: claim and finalize storage
Authority->>Mailbox: apply delivery CAS transition
Authority->>D1: mirror finalized snapshot
Worker->>Authority: claim and complete effects
Authority->>Mailbox: apply effect lease transition
Authority->>D1: mirror effect state
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>
kody-bot
marked this pull request as ready for review
August 2, 2026 16:51
Contributor
|
🔎 Preview deployed: https://kody-pr-1157.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: 9
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/email/inbound.ts (1)
573-576: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale comment about the charge authority.
The comment states that the candidate object is returned when the invocation "won the atomic D1 charge".
authority.chargeno longer charges in D1. It claims the dedupe window in Mailbox, consumes quota in UserMeter, and inserts the charged pending delivery through Mailbox CAS. The reference-identity logic on Line 576 is still correct, but the stated mechanism is wrong.📝 Proposed comment update
- // The charge helper returns the candidate object only when this - // invocation won the atomic D1 charge. A concurrently committed - // delivery is returned as a separately parsed object. + // The authority returns the candidate object only when this + // invocation won the Mailbox dedupe claim and its CAS insert. + // A concurrently committed delivery is returned as a separately + // projected object.🤖 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 573 - 576, Update the comment immediately above the chargedReceive assignment to describe authority.charge’s current behavior: it claims the dedupe window in Mailbox, consumes quota in UserMeter, and inserts the charged pending delivery through Mailbox CAS. Preserve the explanation that a concurrently committed delivery is returned as a separately parsed object, and leave the reference-identity logic unchanged.
🧹 Nitpick comments (7)
packages/worker/src/email/mailbox-inbound-ledger.workers.test.ts (1)
344-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
'deleted'outcome and assert the retry delay.Line 348 uses
outcome: 'delete-failed'.markMailboxInboundDeliveryOrphanCleanedselects a different retry delay per outcome:mailboxInboundOrphanVerificationMsfor'deleted'andmailboxInboundReconciliationRetryMsfor'delete-failed'(seepackages/worker/src/email/mailbox-inbound-cleanup-ledger.tslines 160-163). The'deleted'branch is untested, and it is the branch that drives thecleanedcounter inreconcileUserStaleInboundDeliveries.Add an assertion on the resulting
cleanupRetryAtso both delay constants are pinned.🤖 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/mailbox-inbound-ledger.workers.test.ts` around lines 344 - 351, The test around markInboundDeliveryOrphanCleaned currently covers only the 'delete-failed' outcome; add a separate or parameterized case for outcome 'deleted' and assert cleanupRetryAt uses mailboxInboundOrphanVerificationMs, while retaining coverage that 'delete-failed' uses mailboxInboundReconciliationRetryMs.packages/worker/src/email/test-schema.ts (1)
192-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMirror the two new partial indexes from migration 0129 in the test schema.
Migration 0129 creates
idx_email_delivery_events_user_state_createdandidx_email_delivery_events_user_dedupe_expires. The test schema creates neither. Tests that exercise owner discovery and dedupe pruning therefore run against a different index set than production. Index absence does not change query results, but it lets index-name or partial-predicate regressions pass unnoticed in the worker tests.♻️ Proposed additions
`CREATE TABLE IF NOT EXISTS email_inbound_usage_effects ( user_id TEXT NOT NULL, delivery_id TEXT NOT NULL, finalization_token TEXT NOT NULL, created_at TEXT NOT NULL, PRIMARY KEY (user_id, delivery_id, finalization_token), FOREIGN KEY (delivery_id) REFERENCES email_delivery_events(id) ON DELETE CASCADE );`, + `CREATE INDEX IF NOT EXISTS idx_email_delivery_events_user_state_created +ON email_delivery_events(user_id, state, created_at, id) +WHERE provider = 'cloudflare-email-routing' AND state IS NOT NULL;`, + `CREATE INDEX IF NOT EXISTS idx_email_delivery_events_user_dedupe_expires +ON email_delivery_events(user_id, dedupe_expires_at, id) +WHERE provider = 'cloudflare-email-routing-dedupe' + AND dedupe_expires_at IS NOT NULL;`,🤖 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/test-schema.ts` around lines 192 - 204, The test schema’s SQL definitions must mirror the two partial indexes added by migration 0129. In the schema statement collection near idx_email_delivery_events_pending_effects, add idx_email_delivery_events_user_state_created and idx_email_delivery_events_user_dedupe_expires with the same columns and partial predicates as production.packages/worker/src/email/inbound-delivery-authority.ts (1)
237-262: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a read path that does not write to D1 on every point read.
getandgetWindowmirror to D1 on every successful Mailbox read, even when the snapshot is unchanged.bootstrapPreDeployDueRowsinpackages/worker/src/email/inbound-delivery-reconciliation-authority.tscallsauthority.get(row.id)in a sequential loop over a stale batch. Each iteration then costs one Mailbox RPC plus one D1 upsert, serially.The
updated_atfence inmirrorUserInboundDeliverySnapshotToD1already makes a repeat mirror a no-op write, so the write is pure overhead when the snapshot has not advanced. Two options: skip the mirror when the snapshotupdatedAtmatches the D1 row, or expose a read-only variant for bulk reconciliation loops.🤖 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-delivery-authority.ts` around lines 237 - 262, Adjust the read paths in get and getWindow to avoid mirroring unchanged Mailbox snapshots to D1 on every successful read. Reuse the existing updated_at comparison or expose a read-only authority variant for bootstrapPreDeployDueRows, while preserving mirroring when the snapshot has advanced or must be bootstrapped.packages/worker/migrations/0129-email-inbound-mailbox-authority-mirror.sql (1)
115-122: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd an index on
email_inbound_usage_effects(delivery_id)for the cascade path.The primary key is
(user_id, delivery_id, finalization_token). SQLite cannot use that index for a lookup keyed ondelivery_idalone. EveryON DELETE CASCADEfromemail_delivery_eventstherefore scans the whole child table. The 90-day delivery-event retention job deletes rows in bulk, so this scan repeats per parent row.♻️ Proposed index
PRIMARY KEY (user_id, delivery_id, finalization_token), FOREIGN KEY (delivery_id) REFERENCES email_delivery_events(id) ON DELETE CASCADE ); + +CREATE INDEX IF NOT EXISTS idx_email_inbound_usage_effects_delivery +ON email_inbound_usage_effects(delivery_id);Note: changing this file changes its SHA-256, so update
tools/migration-ledger.jsonas well.🤖 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/migrations/0129-email-inbound-mailbox-authority-mirror.sql` around lines 115 - 122, Add a dedicated index on email_inbound_usage_effects(delivery_id) to support the email_delivery_events ON DELETE CASCADE lookup, while keeping the existing primary key unchanged. After modifying the migration, update its corresponding SHA-256 entry in tools/migration-ledger.json.packages/worker/src/email/inbound-mailbox-authority-mirror-migration.node.test.ts (1)
110-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a non-inbound fixture row to cover the second backfill statement.
All four fixture rows use
cloudflare-email-routingorcloudflare-email-routing-dedupe. Migration statement 2 (UPDATE ... SET updated_at = created_at WHERE updated_at IS NULL) applies to every other row, and thesystem:emailpath must keepstateNULL while still receivingupdated_at. That invariant is central to the D1-only system path and is not asserted today.💚 Proposed fixture and assertion
insert.run( 'email-inbound-dedupe:fingerprint-dedupe', 'user-dedupe', 'receive_started', 'cloudflare-email-routing-dedupe', JSON.stringify({ state: 'pending', fingerprint: 'fingerprint-dedupe', dedupeExpiresAt: '2026-08-01T01:00:00.000Z', }), 0, null, createdAt, ) + insert.run( + 'delivery-system', + 'system:email', + 'sent', + 'kody', + JSON.stringify({ state: 'received', fingerprint: 'fingerprint-system' }), + 0, + null, + createdAt, + )expect( db .prepare( `SELECT state, fingerprint, updated_at FROM email_delivery_events WHERE id = 'delivery-system'`, ) .get(), ).toEqual({ state: null, fingerprint: null, updated_at: createdAt })🤖 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-mailbox-authority-mirror-migration.node.test.ts` around lines 110 - 126, Add a non-inbound `system:email` fixture row such as `delivery-system` before applying `authorityMirrorMigration`, ensuring its `state` and `fingerprint` are NULL and `updated_at` starts NULL. Extend the migration assertions to query this row and verify `state` and `fingerprint` remain NULL while `updated_at` is populated from `createdAt`.packages/worker/src/account/data-targets.ts (1)
159-159: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExclude the inbound usage-effects ledger from account export.
includeInExport: falsecombined withsurfaceandreasonkeeps deletion coverage while documenting that this internal idempotency bookkeeping table has no user content to export. The table contains onlyuser_id,delivery_id,finalization_token, andcreated_at.🤖 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/data-targets.ts` at line 159, Update the email_inbound_usage_effects entry in the account data-target configuration to set includeInExport to false while retaining its surface and reason metadata. Keep the user_id key and deletion coverage unchanged, and document that this internal idempotency ledger contains no user-exportable content.Source: Coding guidelines
packages/worker/src/email/inbound-mailbox-mirror.workers.test.ts (1)
93-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a Proxy forwarder instead of the hand-written method list.
createLedgerBackedMailboxStubforwards 15 ledger RPCs by name. When the authority gains a new ledger RPC, this list silently omits it and the affected test fails with an opaque "not a function" error instead of a clear signal. AProxyforwards every method automatically while still allowing the graph-mirror overrides applied on Lines 194-204.♻️ Proposed refactor
function createLedgerBackedMailboxStub( mailbox: ReturnType<typeof env.MAILBOX.get>, + overrides: Record<string, unknown> = {}, ) { - return { - async getInboundDelivery( - ...args: Parameters<typeof mailbox.getInboundDelivery> - ) { - return await mailbox.getInboundDelivery(...args) - }, - // ...remaining explicit forwarders... - } + return new Proxy(overrides, { + get(target, property, receiver) { + if (property in target) return Reflect.get(target, property, receiver) + const value = (mailbox as Record<string | symbol, unknown>)[property] + return typeof value === 'function' ? value.bind(mailbox) : value + }, + }) }🤖 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-mailbox-mirror.workers.test.ts` around lines 93 - 173, Replace the hand-written forwarding methods in createLedgerBackedMailboxStub with a Proxy that dynamically forwards any missing mailbox property or method to the underlying mailbox. Preserve the existing graph-mirror overrides applied after this helper, ensuring explicitly overridden properties continue to take precedence.
🤖 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 717-747: Reconcile the stale phase and durability sections with
the USER inbound authority model: update the sections around the phase
description and inbound commit boundary so Mailbox owner-bound CAS is
authoritative, D1 is only the synchronous mirror, and retries follow Mailbox
state. Preserve explicit exceptions for system:email and the one-time
D1-to-Mailbox bootstrap bridge; if these sections describe future behavior
instead, label them clearly as a future phase.
In `@packages/worker/src/email/inbound-delivery-authority.ts`:
- Around line 331-334: Update the return logic after claimWindow so newly
inserted deliveries always return toUserInboundDelivery(userId,
result.delivery), including when the claimed deliveryId matches
chargeInput.delivery.deliveryId; preserve the existing handling for non-inserted
results.
In `@packages/worker/src/email/inbound-delivery-projection.ts`:
- Around line 200-254: The inbound projection write currently treats both
owner/provider conflicts and stale snapshots as failures. Update the mirror
logic around the D1 upsert and its changes check to re-read the existing row
when no update occurs, return successfully when the stored row is at least as
current as the snapshot, and throw only if the existing owner or provider
differs from the incoming values; preserve the owner/provider isolation fence
and existing error behavior for genuine conflicts.
In `@packages/worker/src/email/inbound-delivery-reconciliation-authority.ts`:
- Around line 181-188: Guard the await of authority.deferReconciliation in the
catch handler for inbound delivery recovery so a rejection is swallowed,
matching reconcileStaleInboundDeliveries. Ensure reconciliation continues
processing remaining deliveries and preserves accumulated counters.
- Around line 262-265: Validate each row’s detail_json in the
pruneUserExpiredInboundDedupePointers loop before calling authority.claimWindow:
safely handle NULL or malformed JSON without aborting remaining rows, and reject
parsed deliveries whose userId differs from input.userId. Only pass validated,
user-owned InboundDelivery values to claimWindow, matching the legacy
pruneExpiredInboundDedupePointers behavior.
In `@packages/worker/src/email/inbound-effects.ts`:
- Around line 732-744: Update the bridge-row loop in the reconciliation flow to
catch failures from each authority.get(row.id), count each failed row using the
existing error-counting mechanism, and continue processing subsequent rows so
listDueEffects still runs. Keep successful reads and existing due-processing
behavior unchanged.
In `@packages/worker/src/email/inbound-mailbox.ts`:
- Around line 103-115: Move createUserInboundDeliveryAuthority inside an async
promise wrapper in scheduleInboundRejectedTerminalWork so both synchronous
construction errors and the subsequent get(input.deliveryId) failure are handled
by the existing catch. Preserve the isSystemEmailOwner early return and the
current error logging, ensuring the function never propagates errors to its
caller.
In `@packages/worker/src/email/mailbox-inbound-ledger.ts`:
- Around line 186-203: Update claimInboundDeliveryCleanup so its
compare-and-swap only claims rows that still satisfy the same stale eligibility
predicate enforced by listDueStaleInboundDeliveries, rather than every pending
row. Persist or pass the stale eligibility token as needed and validate it
atomically before transitioning the delivery to cleaning, preventing newly
pending deliveries from entering orphan cleanup.
In `@packages/worker/src/email/mailbox-live-mirror.ts`:
- Around line 137-138: Protect the Mailbox authority boundary across
packages/worker/src/email/mailbox-live-mirror.ts:137-138,
packages/worker/src/email/mailbox-live-mirror.ts:175-177, and
packages/worker/src/email/mailbox-types.ts:502-503. Require explicit
event-mirroring intent in the live-mirror API or reject USER inbound and
system:email events, retain fail-closed behavior based on that intent, and
separate bootstrap upserts from normal Mailbox RPCs or enforce bootstrap-only
and missing-row checks at runtime.
---
Outside diff comments:
In `@packages/worker/src/email/inbound.ts`:
- Around line 573-576: Update the comment immediately above the chargedReceive
assignment to describe authority.charge’s current behavior: it claims the dedupe
window in Mailbox, consumes quota in UserMeter, and inserts the charged pending
delivery through Mailbox CAS. Preserve the explanation that a concurrently
committed delivery is returned as a separately parsed object, and leave the
reference-identity logic unchanged.
---
Nitpick comments:
In `@packages/worker/migrations/0129-email-inbound-mailbox-authority-mirror.sql`:
- Around line 115-122: Add a dedicated index on
email_inbound_usage_effects(delivery_id) to support the email_delivery_events ON
DELETE CASCADE lookup, while keeping the existing primary key unchanged. After
modifying the migration, update its corresponding SHA-256 entry in
tools/migration-ledger.json.
In `@packages/worker/src/account/data-targets.ts`:
- Line 159: Update the email_inbound_usage_effects entry in the account
data-target configuration to set includeInExport to false while retaining its
surface and reason metadata. Keep the user_id key and deletion coverage
unchanged, and document that this internal idempotency ledger contains no
user-exportable content.
In `@packages/worker/src/email/inbound-delivery-authority.ts`:
- Around line 237-262: Adjust the read paths in get and getWindow to avoid
mirroring unchanged Mailbox snapshots to D1 on every successful read. Reuse the
existing updated_at comparison or expose a read-only authority variant for
bootstrapPreDeployDueRows, while preserving mirroring when the snapshot has
advanced or must be bootstrapped.
In
`@packages/worker/src/email/inbound-mailbox-authority-mirror-migration.node.test.ts`:
- Around line 110-126: Add a non-inbound `system:email` fixture row such as
`delivery-system` before applying `authorityMirrorMigration`, ensuring its
`state` and `fingerprint` are NULL and `updated_at` starts NULL. Extend the
migration assertions to query this row and verify `state` and `fingerprint`
remain NULL while `updated_at` is populated from `createdAt`.
In `@packages/worker/src/email/inbound-mailbox-mirror.workers.test.ts`:
- Around line 93-173: Replace the hand-written forwarding methods in
createLedgerBackedMailboxStub with a Proxy that dynamically forwards any missing
mailbox property or method to the underlying mailbox. Preserve the existing
graph-mirror overrides applied after this helper, ensuring explicitly overridden
properties continue to take precedence.
In `@packages/worker/src/email/mailbox-inbound-ledger.workers.test.ts`:
- Around line 344-351: The test around markInboundDeliveryOrphanCleaned
currently covers only the 'delete-failed' outcome; add a separate or
parameterized case for outcome 'deleted' and assert cleanupRetryAt uses
mailboxInboundOrphanVerificationMs, while retaining coverage that
'delete-failed' uses mailboxInboundReconciliationRetryMs.
In `@packages/worker/src/email/test-schema.ts`:
- Around line 192-204: The test schema’s SQL definitions must mirror the two
partial indexes added by migration 0129. In the schema statement collection near
idx_email_delivery_events_pending_effects, add
idx_email_delivery_events_user_state_created and
idx_email_delivery_events_user_dedupe_expires with the same columns and partial
predicates as production.
🪄 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: 91afb047-7927-4bd2-bba2-09e2f93b92e5
📒 Files selected for processing (26)
docs/contributing/architecture/data-storage.mdpackages/worker/migrations/0129-email-inbound-mailbox-authority-mirror.sqlpackages/worker/src/account/data-targets.tspackages/worker/src/email/inbound-delivery-authority.node.test.tspackages/worker/src/email/inbound-delivery-authority.tspackages/worker/src/email/inbound-delivery-projection.tspackages/worker/src/email/inbound-delivery-reconciliation-authority.tspackages/worker/src/email/inbound-effects.tspackages/worker/src/email/inbound-mailbox-authority-mirror-migration.node.test.tspackages/worker/src/email/inbound-mailbox-mirror.workers.test.tspackages/worker/src/email/inbound-mailbox.node.test.tspackages/worker/src/email/inbound-mailbox.tspackages/worker/src/email/inbound.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/mailbox-do.tspackages/worker/src/email/mailbox-inbound-cleanup-ledger.tspackages/worker/src/email/mailbox-inbound-effect-ledger.tspackages/worker/src/email/mailbox-inbound-ledger-shared.tspackages/worker/src/email/mailbox-inbound-ledger.tspackages/worker/src/email/mailbox-inbound-ledger.workers.test.tspackages/worker/src/email/mailbox-live-mirror.tspackages/worker/src/email/mailbox-types.tspackages/worker/src/email/reconcile-inbound-deliveries.tspackages/worker/src/email/service.tspackages/worker/src/email/test-schema.tstools/migration-ledger.json
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/src/email/mailbox-inbound-cleanup-ledger.ts (1)
44-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate eligibility logic between the JS predicate and the SQL
WHEREclause.The
claimablepredicate (lines 47-61) and the SQLWHEREclause (lines 81-89) encode the same staleness and state-eligibility rules independently. If a future change updates one side and not the other, the JS-side early return at Line 62 causes an incorrectnot-claimedresult whenever the SQL predicate would actually have allowed the claim, because theUPDATEis never attempted in that case. The post-write verification at lines 106-110 cannot catch this, since it only runs after theUPDATEexecutes.Consider removing the JS-side pre-check and relying solely on the SQL
WHEREclause plus the existing post-write verification. A non-matchingUPDATEis cheap and the finalafter-based check already determines the correct result.♻️ Proposed direction
- const claimable = - current.state === expectedState && - current.updatedAt === expectedUpdatedAt && - current.createdAt < staleBefore && - (current.reconcileAfter == null || current.reconcileAfter <= now) && - (current.state === 'pending' || - (current.state === 'storing' && - current.storageLeaseAt != null && - current.storageLeaseAt < leaseExpiredBefore) || - (current.state === 'cleaning' && - current.cleanupLeaseAt != null && - current.cleanupLeaseAt < leaseExpiredBefore) || - (current.state === 'orphan-cleaned' && - current.cleanupRetryAt != null && - current.cleanupRetryAt <= now)) - if (!claimable) return { status: 'not-claimed', delivery: current } - const cleanupLease = crypto.randomUUID()Let the SQL
UPDATEand the existingafter-based verification (lines 105-113) be the single source of truth for eligibility.Also applies to: 72-90
🤖 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/mailbox-inbound-cleanup-ledger.ts` around lines 44 - 62, Remove the duplicated claimable predicate and its early return from the claim flow around the mailbox inbound cleanup ledger update. Let the SQL UPDATE WHERE clause remain the sole eligibility check, then use the existing post-update after-based verification to return the correct claimed or not-claimed result.
🤖 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/email/mailbox-inbound-cleanup-ledger.ts`:
- Around line 44-62: Remove the duplicated claimable predicate and its early
return from the claim flow around the mailbox inbound cleanup ledger update. Let
the SQL UPDATE WHERE clause remain the sole eligibility check, then use the
existing post-update after-based verification to return the correct claimed or
not-claimed result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c5e1fff2-af40-4b69-aac7-392143b93f78
📒 Files selected for processing (31)
docs/contributing/architecture/data-storage.mdpackages/worker/migrations/0129-email-inbound-mailbox-authority-mirror.sqlpackages/worker/src/account/data-targets.tspackages/worker/src/email/inbound-delivery-authority.node.test.tspackages/worker/src/email/inbound-delivery-authority.tspackages/worker/src/email/inbound-delivery-projection.tspackages/worker/src/email/inbound-delivery-reconciliation-authority.tspackages/worker/src/email/inbound-delivery.tspackages/worker/src/email/inbound-effects.tspackages/worker/src/email/inbound-mailbox-authority-mirror-migration.node.test.tspackages/worker/src/email/inbound-mailbox-mirror.workers.test.tspackages/worker/src/email/inbound-mailbox.node.test.tspackages/worker/src/email/inbound-mailbox.tspackages/worker/src/email/inbound.tspackages/worker/src/email/mailbox-do.tspackages/worker/src/email/mailbox-do.workers.test.tspackages/worker/src/email/mailbox-inbound-bootstrap.tspackages/worker/src/email/mailbox-inbound-cleanup-ledger.tspackages/worker/src/email/mailbox-inbound-ledger.tspackages/worker/src/email/mailbox-inbound-ledger.workers.test.tspackages/worker/src/email/mailbox-live-mirror.tspackages/worker/src/email/mailbox-mirror.tspackages/worker/src/email/mailbox-snapshot-repo.tspackages/worker/src/email/mailbox-snapshots.tspackages/worker/src/email/mailbox-store.tspackages/worker/src/email/mailbox-types.tspackages/worker/src/email/reconcile-inbound-deliveries.tspackages/worker/src/email/service.tspackages/worker/src/email/system-inbound-delivery-authority.tspackages/worker/src/email/test-schema.tstools/migration-ledger.json
🚧 Files skipped from review as they are similar to previous changes (15)
- tools/migration-ledger.json
- packages/worker/src/email/mailbox-types.ts
- packages/worker/src/email/test-schema.ts
- packages/worker/src/email/inbound-mailbox-mirror.workers.test.ts
- packages/worker/src/account/data-targets.ts
- packages/worker/src/email/reconcile-inbound-deliveries.ts
- packages/worker/src/email/service.ts
- packages/worker/migrations/0129-email-inbound-mailbox-authority-mirror.sql
- packages/worker/src/email/inbound-delivery-reconciliation-authority.ts
- packages/worker/src/email/inbound-mailbox.ts
- packages/worker/src/email/inbound-mailbox.node.test.ts
- packages/worker/src/email/inbound-mailbox-authority-mirror-migration.node.test.ts
- packages/worker/src/email/mailbox-inbound-ledger.ts
- packages/worker/src/email/inbound.ts
- packages/worker/src/email/inbound-effects.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
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 a5f3952. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/worker/src/email/mailbox-mirror.ts (1)
371-376: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePartition the page in one pass.
Both
filtercalls evaluateisUserInboundLegacyAuthoritySnapshotfor every event. That predicate runsJSON.parseondetailJsonfor each inbound-provider event, so each such event is parsed twice per page. A single loop removes the duplicate work and keeps the two subsets in input order.♻️ Proposed refactor
- const bootstrapEvents = input.events.filter( - isUserInboundLegacyAuthoritySnapshot, - ) - const normalEvents = input.events.filter( - (event) => !isUserInboundLegacyAuthoritySnapshot(event), - ) + const bootstrapEvents: Array<MailboxDeliveryEventInput> = [] + const normalEvents: Array<MailboxDeliveryEventInput> = [] + for (const event of input.events) { + if (isUserInboundLegacyAuthoritySnapshot(event)) bootstrapEvents.push(event) + else normalEvents.push(event) + }🤖 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/mailbox-mirror.ts` around lines 371 - 376, Update the event partitioning around bootstrapEvents and normalEvents to iterate over input.events once, evaluate isUserInboundLegacyAuthoritySnapshot only once per event, and append each event to the corresponding subset while preserving input order.
🤖 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/mailbox-inbound-bootstrap.ts`:
- Around line 175-236: Update assertUserInboundLegacyBootstrapSnapshot to
validate cleanupRetryAt and reconcileAfter against the delivery detail,
requiring both fields to be null rather than accepting persisted event values.
Add these checks alongside the existing optionalMatches assertions without
changing validation of the other snapshot fields.
---
Nitpick comments:
In `@packages/worker/src/email/mailbox-mirror.ts`:
- Around line 371-376: Update the event partitioning around bootstrapEvents and
normalEvents to iterate over input.events once, evaluate
isUserInboundLegacyAuthoritySnapshot only once per event, and append each event
to the corresponding subset while preserving input order.
🪄 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: 37459523-23d4-4ba2-b76d-1216165077f5
📒 Files selected for processing (20)
docs/contributing/architecture/data-storage.mddocs/contributing/disaster-recovery.mdpackages/worker/src/email/inbound-delivery-authority.node.test.tspackages/worker/src/email/inbound-delivery-authority.tspackages/worker/src/email/inbound-delivery-projection.tspackages/worker/src/email/inbound-mailbox-mirror.workers.test.tspackages/worker/src/email/mailbox-delivery-event-bootstrap.tspackages/worker/src/email/mailbox-delivery-event-upsert.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-bootstrap.tspackages/worker/src/email/mailbox-inbound-ledger.tspackages/worker/src/email/mailbox-inbound-ledger.workers.test.tspackages/worker/src/email/mailbox-mirror.node.test.tspackages/worker/src/email/mailbox-mirror.tspackages/worker/src/email/mailbox-parity-phases.tspackages/worker/src/email/mailbox-reconcile.workers.test.tspackages/worker/src/email/mailbox-types.tspackages/worker/src/email/service.ts
💤 Files with no reviewable changes (1)
- packages/worker/src/email/mailbox-do.workers.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/worker/src/email/service.ts
- packages/worker/src/email/mailbox-inbound-ledger.workers.test.ts
- packages/worker/src/email/mailbox-inbound-ledger.ts
- packages/worker/src/email/inbound-delivery-authority.ts
- packages/worker/src/email/inbound-delivery-projection.ts
Comment on lines
+175
to
+236
| !optionalMatches(input.event.storageLease, delivery.storageLease) || | ||
| !optionalMatches(input.event.storageLeaseAt, delivery.storageLeaseAt) || | ||
| !optionalMatches(input.event.cleanupLease, delivery.cleanupLease) || | ||
| !optionalMatches(input.event.cleanupLeaseAt, delivery.cleanupLeaseAt) || | ||
| !optionalMatches( | ||
| input.event.expectedAttachmentCount, | ||
| delivery.expectedAttachmentCount, | ||
| ) || | ||
| !optionalMatches( | ||
| input.event.finalizationToken, | ||
| delivery.finalizationToken, | ||
| ) || | ||
| !optionalMatches(input.event.dedupeExpiresAt, delivery.dedupeExpiresAt) || | ||
| !optionalMatches( | ||
| input.event.usageEffectRecordedAt, | ||
| delivery.usageEffectRecordedAt, | ||
| ) || | ||
| !optionalMatches( | ||
| input.event.usageEffectSuppressedAt, | ||
| delivery.usageEffectSuppressedAt, | ||
| ) || | ||
| !optionalMatches(input.event.usageStartedAt, delivery.usageStartedAt) || | ||
| !optionalMatches(input.event.usageMonth, delivery.usageMonth) || | ||
| !optionalMatches(input.event.usageBytes, delivery.usageBytes) || | ||
| !optionalMatches(input.event.usageDurationMs, delivery.usageDurationMs) || | ||
| !optionalMatches( | ||
| input.event.usageEffectRetryAt, | ||
| delivery.usageEffectRetryAt, | ||
| ) || | ||
| !optionalMatches(input.event.usageEffectLease, delivery.usageEffectLease) || | ||
| !optionalMatches( | ||
| input.event.usageEffectLeaseAt, | ||
| delivery.usageEffectLeaseAt, | ||
| ) || | ||
| !optionalMatches( | ||
| input.event.subscriptionEffectState, | ||
| delivery.subscriptionEffectState, | ||
| ) || | ||
| !optionalMatches( | ||
| input.event.subscriptionEffectLease, | ||
| delivery.subscriptionEffectLease, | ||
| ) || | ||
| !optionalMatches( | ||
| input.event.subscriptionEffectLeaseAt, | ||
| delivery.subscriptionEffectLeaseAt, | ||
| ) || | ||
| !optionalMatches( | ||
| input.event.subscriptionEffectRetryAt, | ||
| delivery.subscriptionEffectRetryAt, | ||
| ) || | ||
| !optionalMatches( | ||
| input.event.subscriptionEffectAttemptCount, | ||
| delivery.subscriptionEffectAttemptCount, | ||
| ) || | ||
| !optionalMatches( | ||
| input.event.subscriptionEffectDeadLetterAt, | ||
| delivery.subscriptionEffectDeadLetterAt, | ||
| ) || | ||
| !optionalMatches( | ||
| input.event.subscriptionEffectLastError, | ||
| delivery.subscriptionEffectLastError, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Resolve the InboundDelivery type and check for the two fields.
fd -t f 'inbound-delivery.ts' packages/worker/src/email
ast-grep run --pattern 'export type InboundDelivery = $_' --lang typescript packages/worker/src/email
rg -nP -C2 '\b(reconcileAfter|cleanupRetryAt)\b' packages/worker/src/email/inbound-delivery.tsRepository: kentcdodds/kody
Length of output: 4722
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== mailbox-inbound-bootstrap nearby =="
sed -n '120,245p' packages/worker/src/email/mailbox-inbound-bootstrap.ts
echo
echo "== inbound-delivery summary =="
sed -n '1,100p' packages/worker/src/email/inbound-delivery.ts
echo
echo "== write/read event shape references =="
rg -nP -C3 'cleanupRetryAt|reconcileAfter' packages/worker/src/email/mailbox-delivery-events.ts packages/worker/src/email/mailbox-inbound-bootstrap.ts packages/worker/src/email/inbound-delivery.tsRepository: kentcdodds/kody
Length of output: 12689
Validate scheduling fields against the D1 detail.
cleanupRetryAt and reconcileAfter are authority-bearing fields and can be persisted from bootstrap events, but assertUserInboundLegacyBootstrapSnapshot does not compare them. Assert cleanupRetryAt == null and reconcileAfter == null; do not accept values that the D1 detail does not represent as a complete owner-bound snapshot.
🤖 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/mailbox-inbound-bootstrap.ts` around lines 175 -
236, Update assertUserInboundLegacyBootstrapSnapshot to validate cleanupRetryAt
and reconcileAfter against the delivery detail, requiring both fields to be null
rather than accepting persisted event values. Add these checks alongside the
existing optionalMatches assertions without changing validation of the other
snapshot fields.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This was referenced Aug 3, 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
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.
Summary
Flips USER inbound delivery/effect authority from D1 to owner-bound Mailbox CAS RPCs while preserving retry safety, compatibility projections, legacy cutover, and the D1-only
system:emailexception.Migration
0129is additive. No user metadata rows/tables are dropped. Usage-effect replay records cascade with the 90-day delivery-event lifecycle.Conductor findings
upsertDeliveryEvent(s)still rejects USER lifecycle authority overwrites. The parity mirror now partitions mixed batches: validated lifecycle/dedupe history goes through owner-boundbootstrapDeliveryEvents(missing-row-only); pre-claim audit/non-authority rows use normal upsert; malformed legacy rows are skipped/count-visible rather than rolling back audit backfills. Tests seed production-shapedreceive_started(pending/storing),receivedwith finalization/effects, dedupe pointer, and bounded rejection audit rows, then run the actual reconciliation lane from an empty Mailbox. They prove first-pass convergence, second-pass idempotency, exact schedule preservation, malformed-row isolation, and that stale D1 cannot overwrite an existing newer Mailbox row.updated_atrollback writes are not automatically ordered against existing Mailbox rows.data-storage.mdanddisaster-recovery.mdnow state this explicitly and provide the backup-gated repair: verify sealed backup SHA-256 prefix7787f8c9…, quiesce ingress/queues/schedules, inspect each owner, purge only the affected owner Mailbox metadata, rebuild from inspected D1 through missing-row bootstrap/parity, verify lifecycle/effect/finalization/message counts, redeploy, canary, then resume. The docs explicitly say code does not infer that rollback D1 is newer.System recap — extends Email, Mailbox, and D1 (medium structural / high operational risk)
Mode: recap · Base:
main@7d762dae· Head:cfe74bf3Classification: extends — changes USER inbound authority and compatibility contracts across existing primitives; no new primitive is added.
Primitives touched
emailmailboxd1-app-dbSystem map
USER inbound mail selects a dedupe winner, consumes UserMeter quota, commits Mailbox CAS state, and projects snapshots to D1 where the existing message graph still uses storage fences; system mail remains D1-only.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Invariants
system:emailnever enters a per-user Mailbox object.Verification
0130cfe74bf3: all required checks pass, including Workers, Node, MCP, Static, and E2EConductor report
7787f8c9…origin/mainSummary by CodeRabbit
New Features
Bug Fixes
Documentation