[kentcdodds]

Summary

Ships audited Mailbox status/reconcile and owner-gated D1-before-DO natural retention controls.

Production evidence

Forced one complete retention page across all 3 tracked owners: D1 and Mailbox both pruned zero naturally eligible rows; Mailbox counts stayed 100 threads / 185 messages / 48 attachments / 269 events; no blob failures, expired remaining, pending D1 owners, or owner failures. Production currently has no messages older than 365 days and no delivery events older than 90 days, so actual age-qualified blob-before-row deletion remains elapsed-time/test coverage.

Conductor report

• STATUS: done
• What shipped: bounded status/reconcile/retention maintenance
• Risk self-assessment: medium — audited natural maintenance only
• Merged/deployed: yes — PR #1139, merge, deploy containing change
• Read cutover: unchanged/off
• Sibling scope spill: none

  

Summary by CodeRabbit

• New Features

  • Added audited admin mailbox maintenance with status checks, parity reconciliation, and retention actions.
  • Added bounded processing, pagination, concurrency controls, and aggregated cleanup metrics.
  • Added owner-authorized on-demand mailbox retention with deletion and remaining-work results.

• Bug Fixes

  • Improved retention safety by processing expired database records before mailbox cleanup and isolating individual failures.
  • Preserved fresh mailbox data when retention runs encounter cleanup errors.

[coderabbitai]

📝 Walkthrough

Walkthrough

Mailbox retention now uses a shared owner-bound pass for alarms and immediate execution. An audited admin MCP capability adds status, reconciliation, and bounded retention actions with aggregate metrics, pagination, concurrency limits, and time budgets.

Changes

Mailbox retention execution

Layer / File(s)	Summary
Shared owner-bound retention pass packages/worker/src/email/mailbox-types.ts, packages/worker/src/email/mailbox-do.ts, packages/worker/src/email/mailbox-retention.workers.test.ts, docs/contributing/architecture/data-storage.md	alarm() and runRetentionNow() share retention logic. The RPC validates the owner and returns mailbox counts, blob-delete failures, and remaining expired rows.

Admin maintenance workflows

Layer / File(s)	Summary
Bounded admin maintenance service packages/worker/src/admin/mailbox-maintenance.ts, packages/worker/src/admin/mailbox-maintenance.node.test.ts, docs/contributing/architecture/data-storage.md	The service reports parity status, reconciles owners, prunes D1 rows, and runs Mailbox retention with keyset pagination, bounded concurrency, failure isolation, and wall-clock limits.

Admin MCP integration

Layer / File(s)	Summary
Audited admin capability packages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.ts, packages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.node.test.ts, packages/worker/src/mcp/capabilities/admin/domain.ts, packages/worker/src/mcp/capabilities/registry.node.test.ts	The admin_mailbox_maintenance capability validates status, reconcile, and retention actions, dispatches them to the maintenance service, records audit results, and remains restricted to admin callers.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AdminMCP
  participant MaintenanceService
  participant D1
  participant MailboxDO
  AdminMCP->>MaintenanceService: retention request
  MaintenanceService->>D1: prune expired rows and find eligible owners
  D1-->>MaintenanceService: deletion totals and owner pages
  MaintenanceService->>MailboxDO: runRetentionNow(ownerId)
  MailboxDO-->>MaintenanceService: mailbox counts and cleanup status
  MaintenanceService-->>AdminMCP: aggregate metrics and continuation cursor

Loading

Possibly related PRs

• kentcdodds/kody#1124: Both changes modify Mailbox retention behavior.
• kentcdodds/kody#1126: Both changes modify Mailbox parity and retention workflows.
• kentcdodds/kody#1127: This change builds on Mailbox parity and repair infrastructure.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: adding admin-only Mailbox maintenance controls.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch cursor/mailbox-do-810a

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔎 Preview deployed: https://kody-pr-1139.kody-a99.workers.dev

Worker: kody-pr-1139
D1: kody-pr-1139-db
KV: kody-pr-1139-oauth-kv

Mocks:

• cloudflare: https://kody-pr-1139-mock-cloudflare.kody-a99.workers.dev/__mocks (kody-pr-1139-mock-cloudflare)

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

packages/worker/src/admin/mailbox-maintenance.node.test.ts (1)

16-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reset the hoisted mocks between tests.

mocks.mailboxRpc, mocks.reconcileMailboxParity, and the two prune mocks keep their implementations and call history across tests in this file. Test order then affects results. For example, retention concurrency stays at most 4 relies on mocks.mailboxRpc being set by itself, but a future test that omits the setup would silently reuse the previous implementation.

Add a beforeEach that calls vi.resetAllMocks() and re-applies the shared defaults, or enable mockReset in the Vitest config.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/admin/mailbox-maintenance.node.test.ts` around lines 16 -
21, Add a beforeEach in the test setup that calls vi.resetAllMocks() and
restores the shared default implementations for mocks.mailboxRpc,
mocks.reconcileMailboxParity, mocks.pruneUserEmailMessagesForRetention, and
mocks.pruneEmailDeliveryEventsForRetention, ensuring each test is isolated.

🤖 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/admin/mailbox-maintenance.ts`:
- Around line 254-296: Bound each launched owner pass in the maintenance worker
around the existing runOwner flow so runOwner cannot keep the returned promise
pending indefinitely. Add a per-owner timeout using input.ownerTimeoutMs when
provided, otherwise the remaining deadline, and resolve timed-out passes with a
failed OwnerRetentionOutcome; clear the timer after either path settles. Keep
the existing concurrency, outcome collection, and deadline-based launch behavior
intact.
- Around line 588-592: Update the surrounding maintenance flow to retain the
hasMore result from pruneUserEmailMessagesForRetention and include it when
computing truncated, alongside stoppedByBudget and pageFull. Ensure
nextStartAfter remains non-null whenever remaining D1 work exists so subsequent
sweeps continue processing expired rows.

---

Nitpick comments:
In `@packages/worker/src/admin/mailbox-maintenance.node.test.ts`:
- Around line 16-21: Add a beforeEach in the test setup that calls
vi.resetAllMocks() and restores the shared default implementations for
mocks.mailboxRpc, mocks.reconcileMailboxParity,
mocks.pruneUserEmailMessagesForRetention, and
mocks.pruneEmailDeliveryEventsForRetention, ensuring each test is isolated.

🪄 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: 83de44cc-8826-4f1c-8d98-a831d950b326

📥 Commits

Reviewing files that changed from the base of the PR and between 38f3be9 and bb0d830.

📒 Files selected for processing (10)

• docs/contributing/architecture/data-storage.md
• packages/worker/src/admin/mailbox-maintenance.node.test.ts
• packages/worker/src/admin/mailbox-maintenance.ts
• packages/worker/src/email/mailbox-do.ts
• packages/worker/src/email/mailbox-retention.workers.test.ts
• packages/worker/src/email/mailbox-types.ts
• packages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.node.test.ts
• packages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.ts
• packages/worker/src/mcp/capabilities/admin/domain.ts
• packages/worker/src/mcp/capabilities/registry.node.test.ts

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

The wall-clock budget does not bound in-flight owner passes.

deadlineMs only stops new launches. The returned promise resolves only after every launched runOwner settles. If one Mailbox.runRetentionNow RPC hangs, this function waits indefinitely, and adminMailboxMaintenanceRetentionBudgetMs provides no upper bound on the admin request.

Add a per-owner timeout so each launched pass settles within the budget.

🛡️ Proposed per-owner timeout

 async function runOwnersWithBudget(input: {
 	owners: ReadonlyArray<{ userId: string }>
 	concurrency: number
 	deadlineMs: number
 	nowMs: () => number
 	runOwner: (userId: string) => Promise<OwnerRetentionOutcome>
+	ownerTimeoutMs?: number
 }): Promise<{

 				void input
 					.runOwner(owner.userId)
+					.then((outcome) => outcome)
 					.then((outcome) => {
 						outcomes.push(outcome)
 					})

Wrap the call so it races a timer, for example:

const withTimeout = async (userId: string) => {
	const timeoutMs = input.ownerTimeoutMs ?? Math.max(0, input.deadlineMs - input.nowMs())
	let timer: ReturnType<typeof setTimeout> | undefined
	try {
		return await Promise.race([
			input.runOwner(userId),
			new Promise<OwnerRetentionOutcome>((resolve) => {
				timer = setTimeout(() => resolve({ status: 'failed' as const }), timeoutMs)
			}),
		])
	} finally {
		if (timer) clearTimeout(timer)
	}
}

🤖 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 254 - 296,
Bound each launched owner pass in the maintenance worker around the existing
runOwner flow so runOwner cannot keep the returned promise pending indefinitely.
Add a per-owner timeout using input.ownerTimeoutMs when provided, otherwise the
remaining deadline, and resolve timed-out passes with a failed
OwnerRetentionOutcome; clear the timer after either path settles. Keep the
existing concurrency, outcome collection, and deadline-based launch behavior
intact.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

truncated ignores remaining D1 work, so a caller can stop early while expired rows remain.

pruneUserEmailMessagesForRetention returns hasMore (line 497). This function discards it. truncated reflects only the owner page and the budget.

Consider this sequence: the D1 message prune fills its batch and reports hasMore: true; the owner page is shorter than limit; the budget holds. The response then sets truncated: false and nextStartAfter: null. An operator reads that as "sweep complete", but expired D1 rows remain, and every affected owner is still gated by ownerHasPendingExpiredD1Retention, so no Durable Object pass runs for them.

Fold the D1 signal into truncated.

🐛 Proposed fix

 	const pageFull = owners.length >= limit
-	const truncated = stoppedByBudget || pageFull
+	const d1HasMore = d1Messages.hasMore === true
+	const truncated = stoppedByBudget || pageFull || d1HasMore
 	const nextStartAfter = truncated
 		? (lastAttemptedUserId ?? startAfterUserId)
 		: null

📝 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

	const pageFull = owners.length >= limit
	const truncated = stoppedByBudget || pageFull
	const nextStartAfter = truncated
	? (lastAttemptedUserId ?? startAfterUserId)
	: null
	const pageFull = owners.length >= limit
	const d1HasMore = d1Messages.hasMore === true
	const truncated = stoppedByBudget || pageFull || d1HasMore
	const nextStartAfter = truncated
	? (lastAttemptedUserId ?? startAfterUserId)
	: 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/admin/mailbox-maintenance.ts` around lines 588 - 592,
Update the surrounding maintenance flow to retain the hasMore result from
pruneUserEmailMessagesForRetention and include it when computing truncated,
alongside stoppedByBudget and pageFull. Ensure nextStartAfter remains non-null
whenever remaining D1 work exists so subsequent sweeps continue processing
expired rows.