feat(email): detach provider index from legacy graph - #1167
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThe PR detaches the outbound provider-index foreign key, adds compatibility and explicit deletion cleanup, rejects system-email rows, exposes detachment status, updates tests, records the migration, and documents the staged USER graph transition. ChangesOutbound provider index migration
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 |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
🔎 Preview deployed: https://kody-pr-1167.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: 1
🧹 Nitpick comments (1)
packages/worker/src/app/retention.ts (1)
704-721: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd explicit provider-index cleanup here to match the pattern used in
repo.ts.This function relies solely on the compatibility trigger to clean up
email_outbound_provider_indexrows when messages are deleted. The migration comment states this trigger is temporary: "Step 5b removes this compatibility trigger with the legacy table." Once that trigger is removed, this retention path will stop cleaning up provider-index rows for pruned messages, leaving orphaned rows.
repo.ts'sdeleteEmailMessageByIdanddeleteEmailMessageProjectionByIdalready add an explicitDELETE FROM email_outbound_provider_index WHERE message_id = ?in the same atomic batch, specifically so cleanup does not depend on the trigger. Apply the same explicit-deletion pattern here, batched with theemail_attachments/email_messagesdeletes, so this path does not silently regress when the trigger is removed.🤖 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/app/retention.ts` around lines 704 - 721, Add explicit batched deletion of email_outbound_provider_index rows for messageIds in the retention deletion flow, alongside the existing email_attachments and email_messages operations. Follow the atomic cleanup pattern used by deleteEmailMessageById and deleteEmailMessageProjectionById, and do not rely on the compatibility trigger.
🤖 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-provider-index.ts`:
- Around line 29-41: Update isOutboundProviderIndexForeignKeyDetached to default
the optional result.results array to an empty array before calling .some(),
preserving the existing foreign-key matching logic and boolean result.
---
Nitpick comments:
In `@packages/worker/src/app/retention.ts`:
- Around line 704-721: Add explicit batched deletion of
email_outbound_provider_index rows for messageIds in the retention deletion
flow, alongside the existing email_attachments and email_messages operations.
Follow the atomic cleanup pattern used by deleteEmailMessageById and
deleteEmailMessageProjectionById, and do not rely on the compatibility trigger.
🪄 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: ece5ac85-2082-43d7-a68e-d64d0bacc764
📒 Files selected for processing (15)
docs/contributing/architecture/data-storage.mdpackages/worker/migrations/0132-email-outbound-provider-index-detach.sqlpackages/worker/src/admin/mailbox-maintenance.node.test.tspackages/worker/src/admin/mailbox-maintenance.tspackages/worker/src/app/retention.node.test.tspackages/worker/src/app/retention.tspackages/worker/src/email/outbound-provider-index-detach-migration.node.test.tspackages/worker/src/email/outbound-provider-index.tspackages/worker/src/email/outbound-provider-index.workers.test.tspackages/worker/src/email/repo.tspackages/worker/src/email/system-email-authority.workers.test.tspackages/worker/src/email/test-schema.tspackages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.tstools/migration-ledger.json
| export async function isOutboundProviderIndexForeignKeyDetached( | ||
| db: D1Database, | ||
| ): Promise<boolean> { | ||
| const result = await db | ||
| .prepare(`PRAGMA foreign_key_list(email_outbound_provider_index)`) | ||
| .all<{ table: string; from: string; to: string }>() | ||
| return !result.results.some( | ||
| (foreignKey) => | ||
| foreignKey.table === 'email_messages' && | ||
| foreignKey.from === 'message_id' && | ||
| foreignKey.to === 'id', | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
fd -a 'outbound-provider-index|repo\.ts|retention\.ts|package\.json|pnpm-lock|package-lock|yarn\.lock' . | sed 's#^\./##' | head -200
echo
echo "== find D1Result definitions/usages in installed deps/types if present =="
fd -a 'index.d.ts|cloudflare' . | rg '(`@cloudflare/workers-types`|workers-types)' || true
echo
echo "== inspect target file =="
target='packages/worker/src/email/outbound-provider-index.ts'
if [ -f "$target" ]; then
wc -l "$target"
cat -n "$target" | sed -n '1,120p'
fi
echo
echo "== inspect .all call sites in reviewed-ish files =="
for f in $(fd 'repo\.ts|retention\.ts' packages); do
echo "--- $f"
rg -n '\.all\(|result\.results|results\?' "$f" || true
doneRepository: kentcdodds/kody
Length of output: 9973
🌐 Web query:
@cloudflare/workers-types D1Result results optional results?: T[]
💡 Result:
In the @cloudflare/workers-types definitions, the results property within the D1Result type is defined as optional (results?: T[]) [1][2]. This design reflects the nature of D1 operations: 1. Consistency with Runtime: The D1Result object is returned by methods like run and batch [3][4]. While a successful query often populates the results array with an array of objects [3][4], write operations (such as INSERT, UPDATE, or DELETE) may not return rows, in which case the results property may be empty or not present in the same way depending on the specific execution context [4]. 2. TypeScript Usage: Because the property is optional (marked with?), TypeScript users must account for the possibility that results might be undefined [1][2]. When processing a D1Result, it is standard practice to check for the existence of the property or handle it as an array (e.g., results || []) before iterating over it [4]. 3. Documentation vs. Types: While official Cloudflare documentation often illustrates D1Result with a results array [3][5][4], the TypeScript definition provided in @cloudflare/workers-types explicitly includes the optional modifier to ensure type safety, acknowledging that not every D1 operation yields a result set [1][2].
Citations:
- 1: https://github.com/cloudflare/workers-types/blob/master/index.d.ts
- 2: https://github.com/cloudflare/workers-types/blob/master/overrides/d1.d.ts
- 3: https://developers.cloudflare.com/d1/worker-api/return-object/
- 4: https://developers.cloudflare.com/d1/worker-api/prepared-statements/
- 5: https://developers.cloudflare.com/d1/worker-api/return-object/index.md
🌐 Web query:
WorkerD1Types@cloudflare/workers-types D1Result
💡 Result:
The D1Result interface is a standard return type for specific Cloudflare D1 operations in the @cloudflare/workers-types package [1][2]. It represents the outcome of database queries executed via D1PreparedStatement.run or D1Database.batch [3][4]. Key Characteristics: - The D1Result type is generic, where T represents the expected shape of the database rows [1][2]. - It is returned by D1PreparedStatement.run and D1Database.batch [3][4]. Note that D1Database.exec returns a different type, D1ExecResult [4][5]. Typical Structure: While implementation details in type definition files may vary slightly based on versioning, the official D1 documentation defines the D1Result object as follows [4][5]: { success: boolean, // true if the operation was successful meta: { served_by: string, // Cloudflare backend version served_by_region: string, // Region of the execution served_by_primary: boolean, // Whether the primary instance was used timings: { sql_duration_ms: number }, // SQL execution duration duration: number, // Total duration in ms changes: number, // Number of changes made last_row_id: number, // Last inserted row ID changed_db: boolean, // Whether the database was changed size_after: number, // Database size after the query rows_read: number, // Rows scanned rows_written: number, // Rows written total_attempts: number // Number of attempts including retries }, results: array | null // Array of rows, or null if not applicable } Usage Recommendation: Cloudflare now recommends using the wrangler types command to generate types based on your Worker's compatibility date and configuration flags, rather than relying solely on the @cloudflare/workers-types package [6][7]. This ensures that the type definitions accurately match the runtime APIs available to your specific Worker [6].
Citations:
- 1: https://github.com/cloudflare/workers-types/blob/master/overrides/d1.d.ts
- 2: https://github.com/cloudflare/workers-types/blob/master/index.d.ts
- 3: https://github.com/cloudflare/cloudflare-docs/blob/production/src/content/docs/d1/worker-api/return-object.mdx
- 4: https://developers.cloudflare.com/d1/worker-api/return-object/
- 5: https://developers.cloudflare.com/d1/worker-api/return-object/index.md
- 6: https://developers.cloudflare.com/workers/languages/typescript/index.md
- 7: https://registry.npmjs.org/%40cloudflare%2Fworkers-types
Guard the results array before calling .some().
D1Result<T>.results is optional, and this function calls result.results.some(...) directly. Use result.results ?? [] so isOutboundProviderIndexForeignKeyDetached does not throw when the D1 result lacks results.
🤖 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-provider-index.ts` around lines 29 - 41,
Update isOutboundProviderIndexForeignKeyDetached to default the optional
result.results array to an empty array before calling .some(), preserving the
existing foreign-key matching logic and boolean result.
2 participants
Summary
Step 5a prerequisite: rebuilds
email_outbound_provider_indexwithout its FK to the USER D1 message graph while preserving all rows, keys, and indexes.foreignKeyDetachedfor production gatingemail_messagesDELETE trigger preserves the previous worker’s cascade semantics; no live authority flips hereNo data/table drop; no USER behavior change.
Verification
Conductor report
7787f8c9…; fresh pre-drop backup deferred until destructive 5bNote
Medium Risk
Non-destructive D1 table rebuild on a global webhook lookup table with documented rollback constraints; incorrect deploy ordering or skipping the
foreignKeyDetachedgate could leave stale index rows or unsafe rollback assumptions.Overview
Step 5a prerequisite for the Mailbox USER graph cutover: migration
0132-email-outbound-provider-index-detach.sqlrebuildsemail_outbound_provider_indexwithout themessage_idforeign key to legacyemail_messages, copies all rows, and adds auser_id <> 'system:email'CHECK so operator mail cannot enter the global provider reverse index.A compatibility trigger on
email_messagesDELETE keeps the old FK cascade behavior for rollback-era code paths; explicit message deletes inrepo.tsand retention now also remove matching index rows in the samedb.batch. Admin mailbox maintenancestatusaddsoutboundProviderIndex.foreignKeyDetached(viaPRAGMA foreign_key_list) as the production gate before later 5a authority flips.Architecture docs spell out the ordered 5a/5b cutover and rollback rules (frozen D1 graph after Mailbox-only writes; trigger removed in 5b). No live USER authority change in this deploy.
Reviewed by Cursor Bugbot for commit dd8e7e5. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation