Repository navigation
feat(email): add outbound provider reverse index - #1153
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>
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>
📝 WalkthroughWalkthroughThe change adds a D1 reverse index for outbound provider message IDs. Email persistence, webhook lookup, retention, account cleanup, migrations, and admin maintenance now synchronize with or report on this index. ChangesOutbound provider index
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 05:21
Contributor
|
🔎 Preview deployed: https://kody-pr-1153.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: 4
🧹 Nitpick comments (5)
packages/worker/src/email/outbound-provider-index.ts (1)
214-238: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo counters can report the same defective row.
An index row whose
user_iddoes not match its message counts inmissing_from_messages_countand again inmismatched_count.paritystays correct because it requires all counters to be zero. The counts are diagnostics only, so operators may read the totals as two separate defects. Consider documenting the overlap in the JSDoc, or restrictingmissing_from_messages_countto rows with no matchingmessage_idat all.The extra
(?1 IS NULL OR idx.user_id = ?1)predicate at Line 230 is redundant becauseindex_rowsalready applies that filter.🤖 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 214 - 238, Update the diagnostic counter query around missing_from_messages_count and mismatched_count so the same defective row is not reported in both counters; prefer restricting missing_from_messages_count to index rows with no matching message_id, while preserving the existing parity behavior. Remove the redundant (?1 IS NULL OR idx.user_id = ?1) predicate from mismatched_count because index_rows already applies the user filter.packages/worker/src/email/repo.ts (1)
920-941: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reporting stale index rows.
If the index row points at a message that is deleted, not outbound, or carries another provider message ID, the function returns
nullsilently. The stale row then stays until parity reconciliation runs. Add a log or counter for this branch so operators can detect index drift.🤖 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 920 - 941, Update getOutboundEmailMessageByProviderMessageId to report stale reverse-index rows when the referenced message is missing, not outbound, or has a different providerMessageId. Add the repository’s existing logging or counter instrumentation at this validation branch while preserving the current null return behavior.packages/worker/src/email/outbound-provider-index-atomicity.node.test.ts (1)
15-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared test schema.
createEmailDbredefinesemail_messagesandemail_outbound_provider_indexinline.packages/worker/src/email/test-schema.tsalready owns this schema for the workers tests. Two copies can drift after a migration change, and the atomicity tests would then run against an outdated shape.🤖 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-atomicity.node.test.ts` around lines 15 - 67, Update the atomicity test setup around createEmailDb to reuse the shared schema from test-schema.ts instead of redefining email_messages and email_outbound_provider_index inline. Import and invoke the existing schema helper, preserving the test database initialization while ensuring these tests stay aligned with worker schema changes.packages/worker/src/email/outbound-provider-index.workers.test.ts (1)
445-479: 🩺 Stability & Availability | 🔵 TrivialPlan detection for accepted sends that lost D1 state.
The test confirms the intended contract: the provider accepted the send, but D1 keeps
processing_status = 'stored'andprovider_message_id = null. Nothing in D1 links that message to the accepted provider ID afterward, so later delivery events for it resolve asunmatched. Add an alert onemail-outbound-terminal-persistence-failedso operators can repair these messages from the log payload.🤖 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.workers.test.ts` around lines 445 - 479, Add an alert for the accepted-send persistence failure path represented by the test, using the existing email-outbound-terminal-persistence-failed event/log mechanism. Include the messageId and accepted providerMessageId in the payload so operators can identify and repair the D1 record, while preserving the stored status, null provider ID, and send_requested-only event assertions.packages/worker/src/email/outbound.ts (1)
843-854: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord the terminal-persistence failure in usage metrics.
sendOutcomestays'success'whenOutboundEmailPersistenceErroris thrown. Thefinallyblock then recordsoutcome: 'success'for a send that left D1 in an unrepaired state. Add a distinct signal so operators can find these sends from usage data.🤖 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` around lines 843 - 854, Track terminal-persistence failure separately from sendOutcome in the catch handling OutboundEmailPersistenceError, then have the finally usage-metrics recording include that distinct signal. Preserve sendOutcome as success for the accepted send while ensuring usage data identifies sends left in an unrepaired state.
🤖 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 1762-1764: Update the architecture documentation’s cleanup
description to state that deleting email messages cascades to dependent
email_attachments and email_outbound_provider_index rows, rather than claiming
provider-index rows are explicitly deleted first. Keep the existing
thread-pruning behavior description unchanged.
In `@packages/worker/src/account/export.ts`:
- Line 181: Update accountExportSchemaVersion from 1 to 2 to reflect the
required derivedData.email_outbound_provider_index manifest field, and update
any fixtures or tests asserting the previous exported manifest version or shape.
In `@packages/worker/src/admin/mailbox-maintenance.ts`:
- Around line 448-450: Update the parity-report flow around
loadOutboundProviderIndexParityReport to enforce a single provider scope: either
add a schema/test invariant constraining email_outbound_provider_index.provider
to the compared provider, or filter index_rows using the same cloudflare-email
provider condition as the missing-message query. Ensure indexCount,
missingFromMessagesCount, and mismatchedCount all describe the same provider set
before parity is evaluated.
In `@packages/worker/src/email/outbound-provider-index-migration.node.test.ts`:
- Around line 74-88: Update the duplicate-key assertion in the migration test to
reuse an existing valid message_id from the setup instead of "other-msg",
ensuring foreign-key validation does not determine the result. Narrow the
expected error to the SQLite UNIQUE constraint for the (provider,
provider_message_id) key, removing the FOREIGN KEY alternative.
---
Nitpick comments:
In `@packages/worker/src/email/outbound-provider-index-atomicity.node.test.ts`:
- Around line 15-67: Update the atomicity test setup around createEmailDb to
reuse the shared schema from test-schema.ts instead of redefining email_messages
and email_outbound_provider_index inline. Import and invoke the existing schema
helper, preserving the test database initialization while ensuring these tests
stay aligned with worker schema changes.
In `@packages/worker/src/email/outbound-provider-index.ts`:
- Around line 214-238: Update the diagnostic counter query around
missing_from_messages_count and mismatched_count so the same defective row is
not reported in both counters; prefer restricting missing_from_messages_count to
index rows with no matching message_id, while preserving the existing parity
behavior. Remove the redundant (?1 IS NULL OR idx.user_id = ?1) predicate from
mismatched_count because index_rows already applies the user filter.
In `@packages/worker/src/email/outbound-provider-index.workers.test.ts`:
- Around line 445-479: Add an alert for the accepted-send persistence failure
path represented by the test, using the existing
email-outbound-terminal-persistence-failed event/log mechanism. Include the
messageId and accepted providerMessageId in the payload so operators can
identify and repair the D1 record, while preserving the stored status, null
provider ID, and send_requested-only event assertions.
In `@packages/worker/src/email/outbound.ts`:
- Around line 843-854: Track terminal-persistence failure separately from
sendOutcome in the catch handling OutboundEmailPersistenceError, then have the
finally usage-metrics recording include that distinct signal. Preserve
sendOutcome as success for the accepted send while ensuring usage data
identifies sends left in an unrepaired state.
In `@packages/worker/src/email/repo.ts`:
- Around line 920-941: Update getOutboundEmailMessageByProviderMessageId to
report stale reverse-index rows when the referenced message is missing, not
outbound, or has a different providerMessageId. Add the repository’s existing
logging or counter instrumentation at this validation branch while preserving
the current null return behavior.
🪄 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: ccfcc313-85d2-4b1f-9455-6a99c4572acd
📒 Files selected for processing (20)
docs/contributing/architecture/data-storage.mdpackages/worker/migrations/0128-email-outbound-provider-index.sqlpackages/worker/src/account/data-targets.tspackages/worker/src/account/export.tspackages/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-atomicity.node.test.tspackages/worker/src/email/outbound-provider-index-migration.node.test.tspackages/worker/src/email/outbound-provider-index.tspackages/worker/src/email/outbound-provider-index.workers.test.tspackages/worker/src/email/outbound.tspackages/worker/src/email/repo.tspackages/worker/src/email/service.tspackages/worker/src/email/system-email.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
Comment on lines
+1762
to
+1764
| cleanup can retry. Dependent `email_attachments` rows and derived | ||
| `email_outbound_provider_index` rows are deleted before their messages, and | ||
| threads left with no messages are pruned for the affected users. After Mailbox |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Describe provider-index cleanup as a cascade.
packages/worker/src/app/retention.ts Line 714 deletes email_messages; it does not issue a separate email_outbound_provider_index delete before that statement. The foreign-key action removes the index row as part of message deletion. Update this text to describe the cascade, or change the implementation if pre-delete ordering is required.
Proposed documentation fix
- cleanup can retry. Dependent `email_attachments` rows and derived
- `email_outbound_provider_index` rows are deleted before their messages, and
+ cleanup can retry. Dependent `email_attachments` rows are deleted before
+ their messages. `email_outbound_provider_index` rows are removed by the
+ `ON DELETE CASCADE` on `email_messages`, and📝 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
| cleanup can retry. Dependent `email_attachments` rows and derived | |
| `email_outbound_provider_index` rows are deleted before their messages, and | |
| threads left with no messages are pruned for the affected users. After Mailbox | |
| cleanup can retry. Dependent `email_attachments` rows are deleted before | |
| their messages. `email_outbound_provider_index` rows are removed by the | |
| `ON DELETE CASCADE` on `email_messages`, and | |
| threads left with no messages are pruned for the affected users. After Mailbox |
🤖 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 `@docs/contributing/architecture/data-storage.md` around lines 1762 - 1764,
Update the architecture documentation’s cleanup description to state that
deleting email messages cascades to dependent email_attachments and
email_outbound_provider_index rows, rather than claiming provider-index rows are
explicitly deleted first. Keep the existing thread-pruning behavior description
unchanged.
| derivedData: { | ||
| vectorize: string | ||
| r2: string | ||
| email_outbound_provider_index: string |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'accountExportSchemaVersion|AccountExportManifest|schemaVersion|derivedData' \
packages/worker/src/account --glob '*.ts'Repository: kentcdodds/kody
Length of output: 18041
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search consumers and schema handling =="
rg -n -C 5 \
'accountExportSchemaVersion|createAccountExportManifest|AccountExportFile|AccountExportManifest|derivedData|derived' \
packages src app --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.json' 2>/dev/null | head -n 240
echo
echo "== exact matches =="
rg -n \
'accountExportSchemaVersion|AccountExportFile|AccountExportManifest|derivedData' . \
--glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.json' --glob '*.md' \
2>/dev/null | head -n 240
echo
echo "== file excerpts around local schema usage =="
rg -n "accountExportSchemaVersion|createAccountExportManifest|readAccountExport|export.*manifest|manifest.*account" --glob '*.ts' --glob '*.tsx' --glob '*.md' . | sed -n '1,220p'Repository: kentcdodds/kody
Length of output: 20105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow search to repository root using exact symbols.
python3 - <<'PY'
import pathlib, re
symbols = {
"accountExportSchemaVersion": re.compile(r"\baccountExportSchemaVersion\b"),
"AccountExportManifest": re.compile(r"\bAccountExportManifest\b"),
"AccountExportFile": re.compile(r"\bAccountExportFile\b"),
"derivedData": re.compile(r"\bderivedData\b"),
}
patterns = (".ts", ".tsx", ".js", ".jsx", ".json", ".md", ".html")
roots = [pathlib.Path(".")]
findings = {"accountExportSchemaVersion": [], "AccountExportManifest": [], "AccountExportFile": [], "derivedData": []}
for root in roots:
for path in root.rglob("*"):
if not path.is_file():
continue
if path.suffix not in patterns:
continue
try:
text = path.read_text(encoding="utf-8", errors="ignore")
except Exception:
continue
for symbol, regex in symbols.items():
for lineno, line in enumerate(text.splitlines(), 1):
if regex.search(line):
findings[symbol].append((str(path), lineno, line.strip()))
break
for symbol, matches in findings.items():
print(f"== {symbol}: {len(matches)} matches ==")
for path, lineno, line in matches[:200]:
print(f"{path}:{lineno}: {line}")
PY
echo
echo "== account export files =="
git ls-files 'packages/worker/src/account/*' | sed -n '1,120p'
echo
echo "== export.ts around manifest =="
sed -n '168,186p;1728,1770p' packages/worker/src/account/export.ts | cat -nRepository: kentcdodds/kody
Length of output: 4259
Increment the account export manifest schema version.
derivedData.email_outbound_provider_index is a required manifest field, and accountExportSchemaVersion is still pinned at 1. Update the version to 2 and update fixture/test expectations that assert the prior exported manifest shape.
🤖 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/export.ts` at line 181, Update
accountExportSchemaVersion from 1 to 2 to reflect the required
derivedData.email_outbound_provider_index manifest field, and update any
fixtures or tests asserting the previous exported manifest version or shape.
Comment on lines
+448
to
+450
| const outboundProviderIndex = await loadOutboundProviderIndexParityReport({ | ||
| db: input.db, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'email_outbound_provider_index|emailOutboundProviderCloudflare' \
packages/worker/src packages/worker/migrationsRepository: kentcdodds/kody
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- locate file and relevant symbols ---\n'
fd -a 'mailbox-maintenance.ts|outbound-provider-index.ts|admin-mailbox-maintenance.ts|data-targets.ts|service.ts' packages/worker/src | sed 's#^\./##'
printf '\n--- mailbox-maintenance relevant section ---\n'
sed -n '410,470p' packages/worker/src/admin/mailbox-maintenance.ts
printf '\n--- outbound-provider-index implementation ---\n'
rg -n 'export const emailOutboundProviderCloudflare|export async function loadOutboundProviderIndexParityReport|export function getOutboundProviderIndexRow|INSERT INTO email_outbound_provider_index|UPDATE email_outbound_provider_index|DELETE FROM email_outbound_provider_index' packages/worker/src/email/outbound-provider-index.ts packages/worker/src/email/service.ts
sed -n '1,180p' packages/worker/src/email/outbound-provider-index.ts
printf '\n--- direct INSERT references for provider value ---\n'
python3 - <<'PY'
from pathlib import Path
import re
for p in Path('packages/worker').rglob('*'):
if not p.is_file() or '.ts' not in str(p):
continue
text=p.read_text(errors='ignore').splitlines()
for i,line in enumerate(text,1):
if 'email_outbound_provider_index' in line or 'provider_message_id' in line:
if any(s in line for s in ['INSERT', 'UPDATE email_outbound_provider_index', 'deleteOutboundProviderIndex', 'providerMessageId']):
print(f'{p}:{i}:{line.strip()}')
PYRepository: kentcdodds/kody
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- locate file and relevant symbols ---'
fd -a 'mailbox-maintenance.ts|outbound-provider-index.ts|admin-mailbox-maintenance.ts|data-targets.ts|service.ts' packages/worker/src | sed 's#^\./##'
printf '%s\n' ''
printf '%s\n' '--- mailbox-maintenance relevant section ---'
sed -n '410,470p' packages/worker/src/admin/mailbox-maintenance.ts
printf '%s\n' ''
printf '%s\n' '--- outbound-provider-index implementation ---'
rg -n 'export const emailOutboundProviderCloudflare|export async function loadOutboundProviderIndexParityReport|export function getOutboundProviderIndexRow|INSERT INTO email_outbound_provider_index|UPDATE email_outbound_provider_index|DELETE FROM email_outbound_provider_index' packages/worker/src/email/outbound-provider-index.ts packages/worker/src/email/service.ts
sed -n '1,180p' packages/worker/src/email/outbound-provider-index.ts
printf '%s\n' ''
printf '%s\n' '--- direct INSERT references for provider value ---'
python3 - <<'PY'
from pathlib import Path
import re
for p in Path('packages/worker').rglob('*'):
if not p.is_file() or '.ts' not in str(p):
continue
text=p.read_text(errors='ignore').splitlines()
for i,line in enumerate(text,1):
if 'email_outbound_provider_index' in line or 'provider_message_id' in line:
if any(s in line for s in ['INSERT', 'UPDATE email_outbound_provider_index', 'deleteOutboundProviderIndex', 'providerMessageId']):
print(f'{p}:{i}:{line.strip()}')
PYRepository: kentcdodds/kody
Length of output: 11534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- loadOutboundProviderIndexParityReport full implementation ---'
sed -n '168,260p' packages/worker/src/email/outbound-provider-index.ts
printf '%s\n' ''
printf '%s\n' '--- recordProviderEmailDeliveryEvent implementation ---'
sed -n '768,830p' packages/worker/src/email/service.ts
printf '%s\n' ''
printf '%s\n' '--- providerMessageId write call sites with provider context ---'
rg -n -C 3 'providerMessageId|prepareOutboundProviderIndexSyncStatements|senderEmail|sendOutboundEmail|recordProviderEmailDeliveryEvent' packages/worker/src/email packages/worker/src/**/*.ts 2>/dev/null | head -n 220
printf '%s\n' ''
printf '%s\n' '--- provider constant usages ---'
rg -n -C 2 'emailOutboundProviderCloudflare|Cloudflare|cloudflare-email' packages/worker/src/email/outbound-provider-index.ts packages/worker/src/email/service.ts packages/worker/src/email/repo.ts packages/worker/src/email/package-subscriptions.ts 2>/devorneRepository: kentcdodds/kody
Length of output: 21060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- provider constant usages ---'
rg -n -C 2 'emailOutboundProviderCloudflare|Cloudflare|cloudflare-email' packages/worker/src/email/outbound-provider-index.ts packages/worker/src/email/service.ts packages/worker/src/email/repo.ts packages/worker/src/email/package-subscriptions.tsRepository: kentcdodds/kody
Length of output: 5604
Add provider invariant enforcement before using the parity counts.
email_outbound_provider_index.provider is unconstrained in the schema, while the parity report counts all row providers but only compares missing messages against cloudflare-email. If a future provider writes another index value, indexCount, missingFromMessagesCount, and mismatchedCount no longer describe one provider set, so parity can be incorrect. Add a schema/test invariant or filter index_rows with the same provider comparison.
🤖 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.ts` around lines 448 - 450,
Update the parity-report flow around loadOutboundProviderIndexParityReport to
enforce a single provider scope: either add a schema/test invariant constraining
email_outbound_provider_index.provider to the compared provider, or filter
index_rows using the same cloudflare-email provider condition as the
missing-message query. Ensure indexCount, missingFromMessagesCount, and
mismatchedCount all describe the same provider set before parity is evaluated.
Comment on lines
+74
to
+88
| expect(() => { | ||
| db.prepare( | ||
| `INSERT INTO email_outbound_provider_index ( | ||
| provider, provider_message_id, user_id, message_id, inbox_id, | ||
| created_at, updated_at | ||
| ) VALUES (?, ?, ?, ?, NULL, ?, ?)`, | ||
| ).run( | ||
| emailOutboundProviderCloudflare, | ||
| 'prov-user', | ||
| 'other-user', | ||
| 'other-msg', | ||
| '2026-08-01T00:04:00.000Z', | ||
| '2026-08-01T00:04:00.000Z', | ||
| ) | ||
| }).toThrow(/UNIQUE constraint failed|FOREIGN KEY/u) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The duplicate-key assertion does not isolate the primary key.
The inserted row references message_id = 'other-msg', which does not exist in email_messages. Foreign keys are ON, so SQLite can reject the row for the FK violation instead of the (provider, provider_message_id) primary key. The regex accepts both messages, so the test passes without proving primary key enforcement. Reuse an existing message id and assert only the unique constraint.
💚 Proposed fix to isolate the primary key constraint
expect(() => {
db.prepare(
`INSERT INTO email_outbound_provider_index (
provider, provider_message_id, user_id, message_id, inbox_id,
created_at, updated_at
) VALUES (?, ?, ?, ?, NULL, ?, ?)`,
).run(
emailOutboundProviderCloudflare,
'prov-user',
'other-user',
- 'other-msg',
+ 'no-provider',
'2026-08-01T00:04:00.000Z',
'2026-08-01T00:04:00.000Z',
)
- }).toThrow(/UNIQUE constraint failed|FOREIGN KEY/u)
+ }).toThrow(/UNIQUE constraint failed/u)📝 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
| expect(() => { | |
| db.prepare( | |
| `INSERT INTO email_outbound_provider_index ( | |
| provider, provider_message_id, user_id, message_id, inbox_id, | |
| created_at, updated_at | |
| ) VALUES (?, ?, ?, ?, NULL, ?, ?)`, | |
| ).run( | |
| emailOutboundProviderCloudflare, | |
| 'prov-user', | |
| 'other-user', | |
| 'other-msg', | |
| '2026-08-01T00:04:00.000Z', | |
| '2026-08-01T00:04:00.000Z', | |
| ) | |
| }).toThrow(/UNIQUE constraint failed|FOREIGN KEY/u) | |
| expect(() => { | |
| db.prepare( | |
| `INSERT INTO email_outbound_provider_index ( | |
| provider, provider_message_id, user_id, message_id, inbox_id, | |
| created_at, updated_at | |
| ) VALUES (?, ?, ?, ?, NULL, ?, ?)`, | |
| ).run( | |
| emailOutboundProviderCloudflare, | |
| 'prov-user', | |
| 'other-user', | |
| 'no-provider', | |
| '2026-08-01T00:04:00.000Z', | |
| '2026-08-01T00:04:00.000Z', | |
| ) | |
| }).toThrow(/UNIQUE constraint failed/u) |
🤖 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-migration.node.test.ts`
around lines 74 - 88, Update the duplicate-key assertion in the migration test
to reuse an existing valid message_id from the setup instead of "other-msg",
ensuring foreign-key validation does not determine the result. Narrow the
expected error to the SQLite UNIQUE constraint for the (provider,
provider_message_id) key, removing the FOREIGN KEY alternative.
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
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
Adds/backfills an atomic D1 provider→owner/message reverse index. Provider lifecycle now resolves index-first then owner-scoped message; index rows cascade with messages.
Production verification
At 2026-08-02T05:42:14Z:
Conductor report
Summary by CodeRabbit
New Features
Bug Fixes
Documentation