Repository navigation
feat(email): add Mailbox parity soak gate - #1126
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>
📝 WalkthroughWalkthroughThis change adds persisted Mailbox parity state, resumable reconciliation, scheduled processing, mismatch cleanup, telemetry, and a default-off Mailbox read-cutover adapter. It also adds feature-flag registration, migration records, test fixtures, and reconciliation and cutover tests. ChangesMailbox parity and reconciliation
Estimated code review effort: 5 (Critical) | ~120 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>
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>
|
🔎 Preview deployed: https://kody-pr-1126.kody-a99.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/worker/src/email/mailbox-reconcile-mismatch-deletion-error.node.test.ts (1)
167-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the
awaitMailboxMirrorRpcmock with the real return contract.
awaitMailboxMirrorRpcinpackages/worker/src/email/mailbox-mirror.tsreturns{ ok: false; timedOut: true }on timeout. This mock returns{ ok: false, result: { status: 'timeout' } }. The production code only readsraced.ok, so the test passes today. A future consumer that readstimedOutwould still pass here and fail in production.Use the real shape.
♻️ Proposed fix
mocks.awaitMailboxMirrorRpc.mockImplementation( async (promise: Promise<unknown>) => { try { return { ok: true as const, value: await promise } } catch { - return { ok: false as const, result: { status: 'timeout' as const } } + return { ok: false as const, timedOut: true as const } } }, )🤖 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-reconcile-mismatch-deletion-error.node.test.ts` around lines 167 - 175, Update the awaitMailboxMirrorRpc mock implementation to return the production timeout shape `{ ok: false, timedOut: true }` in its catch branch, matching the contract defined by awaitMailboxMirrorRpc while preserving the successful `{ ok: true, value }` result.packages/worker/src/email/mailbox-reconcile.ts (1)
588-612: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the duplicate
persistCheckpointcalls in the content-replay branches.
stateis already assigned fromcontentPass.stateat Line 579, sopersistCheckpointwrites the frozencontentReplayUpperAtandcontentReplayCursoron every call. ThefreezeOpenedWindowguard therefore performs an extraUPDATE usersthat the following call overwrites immediately.In the
budgetbranch both calls bind identical values, so the first write has no effect at all. In thefailedbranch the second call only addslastError. Each affected tick issues two D1 writes instead of one.Remove the guarded calls. If
freezeOpenedWindowis no longer read anywhere, drop it fromcontentPhaseCheckpointas well.♻️ Proposed fix
case 'failed': - // Durably freeze a newly opened window even when this tick cannot finish it. - if (contentCheckpoint.freezeOpenedWindow) { - await persistCheckpoint({ matchingSince: null, lastError: null }) - } + // Persisted state already carries the frozen window from contentPass.state. await persistCheckpoint({ matchingSince: null, lastError: contentCheckpoint.lastError, }) @@ case 'budget': - if (contentCheckpoint.freezeOpenedWindow) { - await persistCheckpoint({ matchingSince: null, lastError: null }) - } // Freeze/retain upper + cursor; clear soak on incomplete budget ticks. await persistCheckpoint({ matchingSince: null, lastError: 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/mailbox-reconcile.ts` around lines 588 - 612, Remove the conditional persistCheckpoint calls guarded by contentCheckpoint.freezeOpenedWindow in the retryable and budget branches, leaving one persistCheckpoint call per branch with the required matchingSince and lastError values. Verify whether freezeOpenedWindow is unused afterward and remove it from contentPhaseCheckpoint if no longer needed.
🤖 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-reconcile.ts`:
- Around line 35-44: The documentation uses an outdated hourly cadence; update
the descriptions to five-minute cadence. In
packages/worker/src/email/mailbox-reconcile.ts lines 35-44,
packages/worker/src/email/mailbox-parity-repo.ts lines 4-9, and
packages/worker/migrations/0125-mailbox-parity-state.sql line 1, replace each
hourly Mailbox parity/reconcile description with the corresponding five-minute
wording.
In `@packages/worker/src/test-support/mailbox-reconcile.ts`:
- Around line 16-35: Update the module-level mocks for mailbox-live-mirror.ts,
mailbox-parity-events.ts, and mailbox-client.ts to asynchronously import and
spread each module’s original exports, while overriding only the specified mock
symbols. Preserve the existing mailbox-mirror.ts partial-mock pattern and ensure
exports such as mailboxLiveMirrorMaxEvents remain available.
---
Nitpick comments:
In
`@packages/worker/src/email/mailbox-reconcile-mismatch-deletion-error.node.test.ts`:
- Around line 167-175: Update the awaitMailboxMirrorRpc mock implementation to
return the production timeout shape `{ ok: false, timedOut: true }` in its catch
branch, matching the contract defined by awaitMailboxMirrorRpc while preserving
the successful `{ ok: true, value }` result.
In `@packages/worker/src/email/mailbox-reconcile.ts`:
- Around line 588-612: Remove the conditional persistCheckpoint calls guarded by
contentCheckpoint.freezeOpenedWindow in the retryable and budget branches,
leaving one persistCheckpoint call per branch with the required matchingSince
and lastError values. Verify whether freezeOpenedWindow is unused afterward and
remove it from contentPhaseCheckpoint if no longer needed.
🪄 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: 41172aa7-9deb-4c2a-b980-afc3a235978e
📒 Files selected for processing (24)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/feature-flags.mdpackages/worker/migrations/0125-mailbox-parity-state.sqlpackages/worker/src/app/handlers/admin-feature-flags.node.test.tspackages/worker/src/app/handlers/session-handler.node.test.tspackages/worker/src/email/mailbox-parity-phases.tspackages/worker/src/email/mailbox-parity-repo.tspackages/worker/src/email/mailbox-read-cutover.node.test.tspackages/worker/src/email/mailbox-read-cutover.tspackages/worker/src/email/mailbox-reconcile-cursor-backfill-content.node.test.tspackages/worker/src/email/mailbox-reconcile-mismatch-deletion-error.node.test.tspackages/worker/src/email/mailbox-reconcile.tspackages/worker/src/email/mailbox-reconcile.workers.test.tspackages/worker/src/feature-flags/registry.node.test.tspackages/worker/src/feature-flags/registry.tspackages/worker/src/feature-flags/service.node.test.tspackages/worker/src/index.workers.test.tspackages/worker/src/mcp/capabilities/access-control.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-feature-flag-capabilities.node.test.tspackages/worker/src/scheduled/scheduled-lanes.node.test.tspackages/worker/src/scheduled/scheduled-lanes.tspackages/worker/src/test-support/mailbox-reconcile.tspackages/worker/src/users-test-schema.tstools/migration-ledger.json
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Summary
Ships a queue-isolated, resumable Mailbox backfill/parity lane plus a default-off read-cutover gate. D1 remains authoritative; no live reader was switched.
System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@0e686a2· Head:669e93bmailboxscheduled-cronfeature-flagsd1-app-dbD1 discovers bounded work; Mailbox is never enumerated. No live read authority changes.
Conductor report
Summary by CodeRabbit
New Features
Documentation