Skip to content

fix(dr): make backups restorable, reliable, complete, and alerting - #986

Merged
kody-bot merged 5 commits into
mainfrom
cursor/dr-backup-fixes-933e
Jul 28, 2026
Merged

kody-bot merged 5 commits into
mainfrom
cursor/dr-backup-fixes-933e

Conversation

@kentcdodds

@kentcdodds kentcdodds commented Jul 27, 2026 •

Copy link
Copy Markdown
Owner

Fixes the three root causes found while evaluating disaster recovery with live restore drills of the 2026-07-26 production export (plus the silent-failure gap):

  1. Backups were un-importable — D1's import path rejects statements above its ~100 KB limit (statement too long: SQLITE_TOOBIG), and production had rows (up to 1.28 MB package_invocations.response_json, oldest from April 30) whose single-row INSERTs exceeded it. Every backup ever taken failed restore drills on this. A drill with statements ≤100 KB filtered out passed end-to-end (PRAGMA quick_check = ok, 66 tables), proving this was the only remaining restore blocker after fix(dr): make D1 restore imports FK-safe with foreign_keys=OFF prelude #943.
    • New shared bound packages/shared/src/backup-restore-safety.ts (64 KiB per large text column).
    • Oversized invocation replay caches are dropped (duplicates get the existing idempotency_response_unavailable outcome); stored email bodies are truncated (raw MIME in R2 stays canonical); value_set rejects oversized values with a storage-bucket hint.
    • Migration 0105 bounds existing rows (81 oversized invocation rows, 2 email bodies in production).
  2. The D1 backup lane failed roughly every other night (July 25 and 27 errored terminally; July 26 passed only on its last catch-up restart). The finalize step re-downloaded the export from D1 and required byte-equality with the stored object; when the short-lived poll result expired, the refresh started a new export of a newer database state, so the comparison failed whenever production wrote anything in between. Finalization now verifies the stored object against the durable upload-step digest (size, ETag, full SHA-256 re-read) and never polls D1 again. It also measures statement lengths while streaming and persists <objectKey>.stats.json; oversized statements log backup-unrestorable-statements (failure status).
  3. No day was ever sealed because the staging exporter's 00:30–02:10 UTC window never fit a full night at production scale (live progress files show every night ending mid-artifacts phase; exporter/summary.json was never written, and daily/full/ is empty). The window is now 00:30–06:10 (~22 min of budget; completed days no-op via the already-complete check).
  4. Failures were silent — a new 06:15 UTC watchdog lane reports a missing staging summary to Sentry (window exhaustion previously produced no error anywhere), and the runbook's alert list now includes backup-unrestorable-statements.

Also refreshes TRUSTED_RESTORE_BASELINE_SHA256 (stale since #904) and documents its recipe plus the new schedule and row-size contract in docs/contributing/disaster-recovery.md.

Operational note: the oversized user values have since been moved out of D1 values entirely — solarHistoryFull now lives as a solar_history_full table in the tesla-solar archive storage bucket, and the environment package caches moved to package storage — so production data is fully under the new bounds once migration 0105 runs.

System recap — extends existing primitives (medium risk)

Mode: recap · Base: main · Head: b8a43985

Classification: extends — changes the backup control plane's finalization contract, bounds D1 row sizes at three write paths, widens the staging cron window, and adds a watchdog lane. No new primitives.

Primitives touched

Primitive Group Impact
backup-control-plane storage extends — finalize verifies stored object, statement stats, wider DR window
d1-app-db storage extends — migration 0105 bounds oversized rows; write-time caps
email assistant extends — stored body copies truncated at 64 KiB (raw MIME stays canonical)
mcp-server surfaces extends — value_set rejects values over 64 KiB
scheduled-cron surfaces extends — DR export window 00:30–06:10 + dr_export_watchdog lane
email-blobs-r2 storage composes — referenced as the canonical home of truncated email bodies

System map

Nightly staging runs longer and gains a watchdog; the D1 backup finalization stops round-tripping through D1; write paths bound row sizes so exports import cleanly.

Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).

flowchart LR
	scheduledCron["scheduled-cron<br/>Scheduled cron"]:::extended
	backupControlPlane["backup-control-plane<br/>Production backup control plane"]:::extended
	d1AppDb["d1-app-db<br/>D1 app database"]:::extended
	email["email<br/>Email inbox"]:::extended
	mcpServer["mcp-server<br/>MCP endpoint"]:::extended
	emailBlobs["email-blobs-r2<br/>Email raw MIME blobs"]:::untouched
	sentry["Sentry (alerting)"]:::untouched
	scheduledCron -->|"dr_export window 00:30–06:10"| backupControlPlane
	scheduledCron -->|"06:15 dr_export_watchdog: missing summary fails lane"| sentry
	backupControlPlane -->|"finalize: verify stored object vs upload digest; write objectKey.stats.json"| backupControlPlane
	mcpServer -->|"value_set 64 KiB cap"| d1AppDb
	email -->|"insertEmailMessage truncates bodies at 64 KiB"| d1AppDb
	email -->|"full message stays in raw_mime_key"| emailBlobs
	d1AppDb -->|"migration 0105 bounds existing rows"| d1AppDb
	classDef touched fill:#1a7f37,color:#fff
	classDef extended fill:#9a6700,color:#fff
	classDef added fill:#cf222e,color:#fff
	classDef untouched fill:#57606a,color:#fff
Loading

Invariants

Immutable backup objects and manifests remain append-only; the finalize change only alters how an absent manifest is verified (stored-object digest instead of a fresh D1 download). Per-user isolation is untouched — all bounded writes stay scoped to the owning userId.

Open in Web Open in Cursor 

Summary by CodeRabbit

  • New Features

    • Added disaster-recovery watchdog monitoring to detect incomplete exports and missing summaries.
    • Extended nightly export coverage through 06:10 UTC, with watchdog checks beginning at 06:15 UTC.
    • Added backup SQL size metrics and reporting for statements that may prevent restoration.
  • Bug Fixes

    • Prevented oversized email, invocation, and stored-value data from making backups unrestorable.
    • Improved backup finalization and retry handling for safer recovery from interrupted or repeated operations.
  • Documentation

    • Documented restore-safe row limits, alerts, schedules, and trusted restore baseline updates.

cursoragent and others added 3 commits July 27, 2026 18:06
D1's import path rejects statements above its ~100 KB limit
(SQLITE_TOOBIG) and exports write one INSERT per row, so a single
oversized row made every production backup un-importable (verified with
live restore drills of the 2026-07-26 export).

- add shared restore-safety limits (packages/shared/src/backup-restore-safety.ts)
- drop oversized package-invocation replay caches instead of storing them
  (duplicates get the existing idempotency_response_unavailable outcome)
- truncate stored email body copies at 64 KiB (raw MIME in R2 stays canonical)
- reject oversized value_set writes with a storage-bucket hint
- migration 0102 bounds existing rows (81 oversized invocation rows in
  production, 2 oversized email bodies)

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
The finalize step re-polled D1 with the cached bookmark and required the
fresh download to byte-match the stored object. When the short-lived
poll result had expired, the refresh started a new export of a newer
database state, so the comparison was doomed whenever production wrote
anything in between — roughly every other nightly backup errored
terminally with existing-object-source-mismatch and left orphaned
~107 MB immutable objects behind.

Finalization now verifies the stored object against the durable
upload-step digest (size, R2 ETag, full SHA-256 re-read) and never
polls D1 again. It also measures statement lengths while streaming
(quote-aware) and persists <objectKey>.stats.json beside the SQL;
oversized statements log backup-unrestorable-statements with failure
status because such a backup cannot be re-imported through the D1 API.

Also refreshes TRUSTED_RESTORE_BASELINE_SHA256 (stale since #904) for
the current migration set including 0102.

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
The staging exporter's 00:30-02:10 UTC window was never enough at
production scale: every night ended mid-artifacts-phase, so
exporter/summary.json was never written and no day could ever be
sealed. Extend the window to 00:30-06:10 (completed days exit via the
cheap already-complete check) and add a 06:15 watchdog lane that fails
loudly to Sentry when the summary is still missing — window exhaustion
was previously silent.

Documents the new schedule, the restore-safe row size contract, and the
TRUSTED_RESTORE_BASELINE_SHA256 recipe in the disaster-recovery runbook.

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
@coderabbitai

coderabbitai Bot commented Jul 27, 2026 •

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@cursor[bot], you've reached your PR review limit, so we couldn't start this review.

Next review available in: 56 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 609d40e9-83ac-447b-a40b-491381931d05

📥 Commits

Reviewing files that changed from the base of the PR and between 95d50a7 and b8a4398.

📒 Files selected for processing (24)
  • docs/contributing/disaster-recovery.md
  • packages/backup-control-plane/backup-control-plane-test-support.ts
  • packages/backup-control-plane/backup-runtime.node.test.ts
  • packages/backup-control-plane/backup-runtime.ts
  • packages/backup-control-plane/backup-types.ts
  • packages/backup-control-plane/immutable-storage.node.test.ts
  • packages/backup-control-plane/immutable-storage.ts
  • packages/backup-control-plane/readme.md
  • packages/backup-control-plane/wrangler.jsonc
  • packages/shared/src/backup-restore-safety.node.test.ts
  • packages/shared/src/backup-restore-safety.ts
  • packages/worker/migrations/0105-restore-safe-row-sizes.sql
  • packages/worker/src/app/restore-safe-row-sizes-migration.node.test.ts
  • packages/worker/src/dr/exporter.node.test.ts
  • packages/worker/src/dr/exporter.ts
  • packages/worker/src/email/repo-body-limits.workers.test.ts
  • packages/worker/src/email/repo.ts
  • packages/worker/src/index.ts
  • packages/worker/src/index.workers.test.ts
  • packages/worker/src/mcp/values/service.node.test.ts
  • packages/worker/src/mcp/values/service.ts
  • packages/worker/src/package-invocations/repo.ts
  • packages/worker/src/package-invocations/service.node.test.ts
  • tools/migration-ledger.json
📝 Walkthrough

Walkthrough

The change adds restore-safe UTF-8 and database value limits, records SQL statement statistics during backups, revises immutable-manifest finalization, adds a staging export watchdog, updates scheduled execution, and documents the resulting recovery and alerting behavior.

Changes

Disaster recovery safeguards

Layer / File(s) Summary
Restore-safe limits and migration
packages/shared/src/backup-restore-safety.ts, packages/worker/migrations/0105-restore-safe-row-sizes.sql, tools/migration-ledger.json, packages/backup-control-plane/wrangler.jsonc
Adds shared byte limits and UTF-8 truncation helpers, applies restore-safe bounds to oversized database values, records the migration checksum, and updates the trusted restore baseline hash.
Runtime storage guards
packages/worker/src/email/repo.ts, packages/worker/src/package-invocations/repo.ts, packages/worker/src/mcp/values/service.ts, packages/worker/src/*test*
Bounds email bodies, nulls oversized invocation responses, rejects oversized MCP values, and adds coverage for these behaviors.
Backup statement stats and finalization
packages/backup-control-plane/backup-types.ts, packages/backup-control-plane/immutable-storage.ts, packages/backup-control-plane/backup-runtime.ts, packages/backup-control-plane/*test*
Scans streamed SQL for statement sizes, persists per-object statistics, verifies durable stored objects during finalization, and updates replay, tamper, retry, and statistics tests.
Nightly export watchdog
packages/worker/src/dr/exporter.ts, packages/worker/src/index.ts, packages/worker/src/*test*, docs/contributing/disaster-recovery.md
Extends the export window to 06:10 UTC, adds a 06:15 staging-summary check, wires a scheduled watchdog lane, and documents the new schedule and alert condition.

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

Sequence Diagram(s)

sequenceDiagram
  participant BackupRuntime
  participant ImmutableStorage
  participant BackupBucket
  participant Manifest
  BackupRuntime->>ImmutableStorage: stream and digest SQL object
  ImmutableStorage-->>BackupRuntime: integrity data and SQL statement stats
  BackupRuntime->>Manifest: write immutable manifest
  BackupRuntime->>BackupBucket: write object-adjacent stats JSON
Loading
sequenceDiagram
  participant WorkerScheduler
  participant DrExportWatchdog
  participant StagingBucket
  WorkerScheduler->>DrExportWatchdog: invoke watchdog tick
  DrExportWatchdog->>StagingBucket: read exporter/summary.json
  StagingBucket-->>DrExportWatchdog: summary present or missing
  DrExportWatchdog-->>WorkerScheduler: return result or throw progress error
Loading

Possibly related PRs

  • kentcdodds/kody#904: Modifies overlapping immutable-storage digest and finalization code paths.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 29.63% 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 is related to the PR’s main DR restore-safety, reliability, completeness, and alerting changes.
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
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cursor/dr-backup-fixes-933e

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

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

main gained 0102-0104 while this branch was open; the data migration is
content-identical, renumbered to the next free prefix with the ledger
entry and TRUSTED_RESTORE_BASELINE_SHA256 recomputed.

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Comment thread packages/worker/src/email/repo.ts Outdated
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
@github-actions

Copy link
Copy Markdown
Contributor

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

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

Mocks:

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit b8a4398. Configure here.

text_body: input.message.textBody ?? null,
html_body: input.message.htmlBody ?? null,
text_body: boundedEmailBody(input.message.textBody),
html_body: boundedEmailBody(input.message.htmlBody),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dual email bodies break limit

Medium Severity

boundedEmailBody caps text_body and html_body independently at maxRestorableTextColumnBytes, but D1 exports one INSERT per row and the import limit applies to the entire statement. A message with both bodies near the cap can still produce an INSERT well above d1ImportMaxStatementBytes, leaving backups unrestorable despite the new safety checks.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b8a4398. Configure here.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/backup-control-plane/immutable-storage.ts (1)

93-131: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Cancel the abandoned digest reader when digestBody fails.

In storeSignedDownload, the download body is teed for concurrent R2 upload and digesting. If digestBody rejects and only calls writer.abort(), the digestBodyStream reader is left unreleased while bucket.put() continues draining the shared source. Explicitly cancel that branch on error so the source/backpressure can be released cleanly.

🔧 Proposed fix
 	} catch (error) {
 		await writer.abort(error).catch(() => undefined)
+		await reader.cancel(error).catch(() => undefined)
 		throw error
 	}
🤖 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/backup-control-plane/immutable-storage.ts` around lines 93 - 131,
Update the error path in digestBody to cancel the digest body reader before or
alongside aborting the writer. Ensure reader.cancel(error) is awaited or safely
handled, while preserving the existing writer.abort(error) cleanup and rethrow
behavior so the abandoned tee branch releases its source cleanly.
🤖 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/backup-control-plane/backup-runtime.ts`:
- Around line 230-241: Make the advisory record-statement-stats step non-fatal
after the immutable manifest is successfully written: isolate the await of
recordSqlStatementStats from the outer backup failure path, catch exhausted
retries, and log the stats error without rethrowing or emitting backup-failure.
Preserve the existing retries and timeout while allowing the overall backup
operation to remain successful when stats persistence fails.

In `@packages/shared/src/backup-restore-safety.ts`:
- Around line 20-25: Replace the universal raw-column cap in
packages/shared/src/backup-restore-safety.ts:20-25 with a serialized SQL-row
budget that accounts for apostrophe escaping and table-specific co-resident
fields. Apply that shared budget in
packages/worker/migrations/0102-restore-safe-row-sizes.sql:12-31, including
combined text_body/html_body limits; update
packages/worker/src/package-invocations/repo.ts:21-33 and
packages/worker/src/mcp/values/service.ts:69-75 to validate escaped SQL-storage
size. Extend
packages/worker/src/app/restore-safe-row-sizes-migration.node.test.ts:75-109 and
packages/worker/src/mcp/values/service.node.test.ts:377-406 with worst-case
apostrophe-heavy and dual-large-body boundary cases.

In `@packages/worker/src/email/repo.ts`:
- Around line 33-40: Replace the independent per-column truncation in
boundedEmailBody with a shared encoded-row budget allocated across both
text_body and html_body. Ensure the combined UTF-8 byte lengths, truncation
notices, and both stored values remain within the aggregate limit, while
preserving null handling and valid UTF-8. Update the email import coverage to
include both bodies near their individual limits and verify the resulting
combined row stays within budget.

---

Outside diff comments:
In `@packages/backup-control-plane/immutable-storage.ts`:
- Around line 93-131: Update the error path in digestBody to cancel the digest
body reader before or alongside aborting the writer. Ensure reader.cancel(error)
is awaited or safely handled, while preserving the existing writer.abort(error)
cleanup and rethrow behavior so the abandoned tee branch releases its source
cleanly.
🪄 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: a3999774-056b-4540-9f71-6587853acd76

📥 Commits

Reviewing files that changed from the base of the PR and between 95d50a7 and 3d3f6f7.

📒 Files selected for processing (24)
  • docs/contributing/disaster-recovery.md
  • packages/backup-control-plane/backup-control-plane-test-support.ts
  • packages/backup-control-plane/backup-runtime.node.test.ts
  • packages/backup-control-plane/backup-runtime.ts
  • packages/backup-control-plane/backup-types.ts
  • packages/backup-control-plane/immutable-storage.node.test.ts
  • packages/backup-control-plane/immutable-storage.ts
  • packages/backup-control-plane/readme.md
  • packages/backup-control-plane/wrangler.jsonc
  • packages/shared/src/backup-restore-safety.node.test.ts
  • packages/shared/src/backup-restore-safety.ts
  • packages/worker/migrations/0102-restore-safe-row-sizes.sql
  • packages/worker/src/app/restore-safe-row-sizes-migration.node.test.ts
  • packages/worker/src/dr/exporter.node.test.ts
  • packages/worker/src/dr/exporter.ts
  • packages/worker/src/email/repo-body-limits.workers.test.ts
  • packages/worker/src/email/repo.ts
  • packages/worker/src/index.ts
  • packages/worker/src/index.workers.test.ts
  • packages/worker/src/mcp/values/service.node.test.ts
  • packages/worker/src/mcp/values/service.ts
  • packages/worker/src/package-invocations/repo.ts
  • packages/worker/src/package-invocations/service.node.test.ts
  • tools/migration-ledger.json

Comment on lines +230 to +241
await step.do(
'record-statement-stats',
{ retries: { limit: 2, delay: '10 seconds' }, timeout: '2 minutes' },
async () =>
recordSqlStatementStats({
env,
day: checkedPayload.day,
instanceId: event.instanceId,
objectKey: stored.objectKey,
stats: stored.sqlStatementStats,
}),
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Advisory stats step can turn a successful backup into a reported failure.

record-statement-stats runs after the immutable manifest is already durably written, but it's awaited directly inside the same try that wraps the whole function. If it exhausts its retries (e.g. a transient R2 hiccup while writing ${objectKey}.stats.json), the error propagates to the outer catch, which logs backup-failure and rethrows — even though the backup already succeeded and is restorable. The function's own comment calls this data "advisory," which contradicts making it fail-closed for the entire run. Per the README, freshness ticks can restart an "errored" instance, so this could also trigger unnecessary restarts/alerts for a day that already has a valid manifest.

🔧 Proposed fix
-		await step.do(
-			'record-statement-stats',
-			{ retries: { limit: 2, delay: '10 seconds' }, timeout: '2 minutes' },
-			async () =>
-				recordSqlStatementStats({
-					env,
-					day: checkedPayload.day,
-					instanceId: event.instanceId,
-					objectKey: stored.objectKey,
-					stats: stored.sqlStatementStats,
-				}),
-		)
+		await step
+			.do(
+				'record-statement-stats',
+				{ retries: { limit: 2, delay: '10 seconds' }, timeout: '2 minutes' },
+				async () =>
+					recordSqlStatementStats({
+						env,
+						day: checkedPayload.day,
+						instanceId: event.instanceId,
+						objectKey: stored.objectKey,
+						stats: stored.sqlStatementStats,
+					}),
+			)
+			.catch((error) => {
+				// Advisory only: the manifest is already durable, so a failure
+				// here must not turn a successful backup into a reported failure.
+				safeLog({
+					event: 'backup-sql-stats',
+					status: 'failure',
+					day: checkedPayload.day,
+					instanceId: event.instanceId,
+					objectKey: stored.objectKey,
+					errorCode: errorCode(error),
+				})
+			})

Happy to also add a regression test exercising a failing record-statement-stats step against an already-successful manifest write, if useful.

📝 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
await step.do(
'record-statement-stats',
{ retries: { limit: 2, delay: '10 seconds' }, timeout: '2 minutes' },
async () =>
recordSqlStatementStats({
env,
day: checkedPayload.day,
instanceId: event.instanceId,
objectKey: stored.objectKey,
stats: stored.sqlStatementStats,
}),
)
await step
.do(
'record-statement-stats',
{ retries: { limit: 2, delay: '10 seconds' }, timeout: '2 minutes' },
async () =>
recordSqlStatementStats({
env,
day: checkedPayload.day,
instanceId: event.instanceId,
objectKey: stored.objectKey,
stats: stored.sqlStatementStats,
}),
)
.catch((error) => {
// Advisory only: the manifest is already durable, so a failure
// here must not turn a successful backup into a reported failure.
safeLog({
event: 'backup-sql-stats',
status: 'failure',
day: checkedPayload.day,
instanceId: event.instanceId,
objectKey: stored.objectKey,
errorCode: errorCode(error),
})
})
🤖 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/backup-control-plane/backup-runtime.ts` around lines 230 - 241, Make
the advisory record-statement-stats step non-fatal after the immutable manifest
is successfully written: isolate the await of recordSqlStatementStats from the
outer backup failure path, catch exhausted retries, and log the stats error
without rethrowing or emitting backup-failure. Preserve the existing retries and
timeout while allowing the overall backup operation to remain successful when
stats persistence fails.

Comment on lines +20 to +25
/**
* Upper bound for any single large text column persisted to D1. Leaves
* headroom below {@link d1ImportMaxStatementBytes} for the other row
* columns, INSERT framing, and quote-escaping expansion in SQL dumps.
*/
export const maxRestorableTextColumnBytes = 65_536

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Raw column limits do not guarantee importable SQL statements.

A 65,536-byte value of ' expands to roughly 131 KiB when exported as a SQL string literal, before INSERT framing. Email rows can additionally contain both large body columns. This still permits SQLITE_TOOBIG, defeating the restore-safety guarantee.

  • packages/shared/src/backup-restore-safety.ts#L20-L25: replace the universal raw-column cap with a serialized-row budget that accounts for SQL escaping and table-specific co-resident fields.
  • packages/worker/migrations/0102-restore-safe-row-sizes.sql#L12-L31: bound existing rows using that same budget, including the combined text_body/html_body size.
  • packages/worker/src/app/restore-safe-row-sizes-migration.node.test.ts#L75-L109: add worst-case apostrophe and dual-large-body migration cases.
  • packages/worker/src/package-invocations/repo.ts#L21-L33: reject/null responses based on their escaped SQL-storage budget, not raw JSON bytes.
  • packages/worker/src/mcp/values/service.ts#L69-L75: apply the safe serialized budget before accepting a value.
  • packages/worker/src/mcp/values/service.node.test.ts#L377-L406: test accepted/rejected boundaries with apostrophe-heavy values.
📍 Affects 6 files
  • packages/shared/src/backup-restore-safety.ts#L20-L25 (this comment)
  • packages/worker/migrations/0102-restore-safe-row-sizes.sql#L12-L31
  • packages/worker/src/app/restore-safe-row-sizes-migration.node.test.ts#L75-L109
  • packages/worker/src/package-invocations/repo.ts#L21-L33
  • packages/worker/src/mcp/values/service.ts#L69-L75
  • packages/worker/src/mcp/values/service.node.test.ts#L377-L406
🤖 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/shared/src/backup-restore-safety.ts` around lines 20 - 25, Replace
the universal raw-column cap in
packages/shared/src/backup-restore-safety.ts:20-25 with a serialized SQL-row
budget that accounts for apostrophe escaping and table-specific co-resident
fields. Apply that shared budget in
packages/worker/migrations/0102-restore-safe-row-sizes.sql:12-31, including
combined text_body/html_body limits; update
packages/worker/src/package-invocations/repo.ts:21-33 and
packages/worker/src/mcp/values/service.ts:69-75 to validate escaped SQL-storage
size. Extend
packages/worker/src/app/restore-safe-row-sizes-migration.node.test.ts:75-109 and
packages/worker/src/mcp/values/service.node.test.ts:377-406 with worst-case
apostrophe-heavy and dual-large-body boundary cases.

Comment on lines +33 to +40
function boundedEmailBody(body: string | null | undefined): string | null {
if (body == null) return null
if (utf8ByteLength(body) <= maxRestorableTextColumnBytes) return body
return (
truncateToUtf8Bytes(
body,
maxRestorableTextColumnBytes - utf8ByteLength(emailBodyTruncationNotice),
) + emailBodyTruncationNotice

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Enforce a row-level budget across both email bodies.

Each column may reach 65,536 bytes, so an email with both bodies populated exceeds 131 KB before the remaining fields and SQL escaping. That can still produce an unimportable D1 INSERT. Allocate one aggregate, encoded-row-safe budget across text_body and html_body, and add coverage where both inputs are near their limits.

🤖 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 33 - 40, Replace the
independent per-column truncation in boundedEmailBody with a shared encoded-row
budget allocated across both text_body and html_body. Ensure the combined UTF-8
byte lengths, truncation notices, and both stored values remain within the
aggregate limit, while preserving null handling and valid UTF-8. Update the
email import coverage to include both bodies near their individual limits and
verify the resulting combined row stays within budget.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/migrations/0105-restore-safe-row-sizes.sql`:
- Around line 21-31: Update the migration’s text_body/html_body truncation logic
to enforce a shared complete-row size budget rather than truncating each column
independently; preserve the raw MIME reference and existing marker behavior
while ensuring other row fields fit within the export limit. Add a regression
case covering an email with both bodies oversized and verify the resulting row
remains within the safe size bound.
🪄 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: 3f8c181a-ef6f-450e-830d-282d19276e9f

📥 Commits

Reviewing files that changed from the base of the PR and between 3d3f6f7 and b8a4398.

📒 Files selected for processing (6)
  • packages/backup-control-plane/wrangler.jsonc
  • packages/worker/migrations/0105-restore-safe-row-sizes.sql
  • packages/worker/src/app/restore-safe-row-sizes-migration.node.test.ts
  • packages/worker/src/email/repo.ts
  • packages/worker/src/mcp/values/service.node.test.ts
  • tools/migration-ledger.json
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/backup-control-plane/wrangler.jsonc
  • packages/worker/src/email/repo.ts
  • packages/worker/src/app/restore-safe-row-sizes-migration.node.test.ts

Comment on lines +21 to +31
UPDATE email_messages
SET text_body = substr(text_body, 1, 16000) || '
[truncated for backup-safe storage; the full message is retained in the raw MIME object]'
WHERE text_body IS NOT NULL
AND LENGTH(CAST(text_body AS BLOB)) > 65536;

UPDATE email_messages
SET html_body = substr(html_body, 1, 16000) || '
[truncated for backup-safe storage; the full message is retained in the raw MIME object]'
WHERE html_body IS NOT NULL
AND LENGTH(CAST(html_body AS BLOB)) > 65536;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Bound the complete email row, not each body column independently.

text_body and html_body are truncated separately to nearly 64 KiB each. If both are populated, the resulting INSERT can still exceed 100 KiB before accounting for the other columns, so the migration does not guarantee restorable exports. Apply a shared per-row budget (or clear one convenience copy when necessary) and add a regression case with both bodies oversized.

🤖 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/0105-restore-safe-row-sizes.sql` around lines 21 -
31, Update the migration’s text_body/html_body truncation logic to enforce a
shared complete-row size budget rather than truncating each column
independently; preserve the raw MIME reference and existing marker behavior
while ensuring other row fields fit within the export limit. Add a regression
case covering an email with both bodies oversized and verify the resulting row
remains within the safe size bound.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants