Enable disaster-recovery backups: multi-store exporter, Access-gated DR UI, one-button restore - #904
Conversation
…endpoint, secret escrow
…d shared contracts
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR adds production disaster-recovery export and restore infrastructure, including staged backups, signed immutable manifests, authenticated control-plane workflows, isolated restore drills, chunked production restores, secret escrow, trust registries, deployment configuration, and operator documentation. ChangesProduction disaster recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ProductionWorker
participant DrBackupS3
participant BackupControlPlane
participant RestoreWorker
ProductionWorker->>DrBackupS3: Write resumable staged exports
BackupControlPlane->>DrBackupS3: Read and seal staged artifacts
BackupControlPlane->>RestoreWorker: Start restore maintenance workflow
RestoreWorker->>DrBackupS3: Read sealed indexes and verified blobs
RestoreWorker-->>BackupControlPlane: Return cursor-based restore progress
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
…iring results, bounded restarts)
…ing failure causes
|
🔎 Preview deployed: https://kody-pr-904.kody-a99.workers.dev Worker: Mocks:
|
|
Bugbot review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| export const drExportRunTimeBudgetMs = 20_000 | ||
| /** Skip individual R2 objects larger than this (full-buffer ceiling). */ | ||
| export const drExportMaxObjectBytes = 25 * 1024 * 1024 |
There was a problem hiding this comment.
R2 skip limit contradicts runbook
Medium Severity
The nightly R2 exporter skips objects larger than drExportMaxObjectBytes (25 MiB), but the disaster-recovery runbook states objects above 100 MiB are skipped. Large MIME or assets between 25 MiB and 100 MiB are omitted from backup with only a warning, while operators may believe they are still in scope.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1c0d565. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
packages/worker/src/storage-runner.import.node.test.ts (1)
17-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for empty-entries "first" page (used by DR restore to clear empty dumps).
dr-restore.ts'srestoreStoragePhasecallsapplyImportStoragePage({mode: 'replace', replacePage: 'first', entries: []})when a sealed dump is empty, but no test here exercises that combination.+test('importStorage first page with empty entries still clears', async () => { + const storage = createMemoryStorage({ stale: true }) + const result = await applyImportStoragePage(storage, { + mode: 'replace', + replacePage: 'first', + entries: [], + }) + expect(result).toEqual({ ok: true, written: 0, cleared: true }) + expect(storage.map.size).toBe(0) +})🤖 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/storage-runner.import.node.test.ts` around lines 17 - 56, Add a test covering applyImportStoragePage with mode 'replace', replacePage 'first', and an empty entries array, verifying the existing storage contents are cleared and the result reports ok, written: 0, and cleared: true. Use the same createMemoryStorage fixture and assertions as the neighboring replace-page tests.packages/shared/src/backup-full-manifest.ts (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
knownR2Labelsfrom the sharedbackupR2BucketLabels.This hardcoded set duplicates
backupR2BucketLabelsfrombackup-staging.ts(already the source ofBackupR2BucketLabelimported on Line 1). If a new bucket label is added there, this validator will silently reject it, diverging the two contracts.♻️ Proposed change
-import { type BackupR2BucketLabel } from './backup-staging.ts' +import { + type BackupR2BucketLabel, + backupR2BucketLabels, +} from './backup-staging.ts'-const knownR2Labels = new Set<string>(['email-blobs', 'community-assets']) +const knownR2Labels = new Set<string>(backupR2BucketLabels)🤖 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-full-manifest.ts` at line 42, Update knownR2Labels to derive its values from the shared backupR2BucketLabels symbol imported from backup-staging.ts, rather than maintaining a hardcoded label list. Preserve Set<string> behavior so validation automatically accepts any labels added to the shared source.
🤖 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 @.github/workflows/dr-escrow.yml:
- Around line 12-13: Update the Checkout step using actions/checkout@v4 to
disable credential persistence by setting persist-credentials to false, while
leaving the rest of the workflow unchanged.
In `@packages/backup-control-plane/access-auth.ts`:
- Around line 48-85: Update loadJwks to enforce an explicit timeout on the
Access JWKS fetch by creating an AbortController, scheduling cancellation after
the configured timeout, and passing its signal to fetcher. Ensure the timer is
cleared after the fetch completes or fails, and preserve existing response
validation and BackupError handling.
In `@packages/backup-control-plane/restore-workflow.ts`:
- Around line 24-36: The restore workflow currently wraps all phases in the
single runProductionRestore step, causing retries to repeat
capturePreRestoreSafetyExport. Split the flow into separate step.do calls for
capture-pre-restore-safety-export, import-d1, and restore-stores, wiring each
phase’s outputs into the next while preserving the existing warning and failure
handling. Ensure retries of import or store restoration do not re-execute the
safety export.
In `@packages/backup-control-plane/wrangler.jsonc`:
- Around line 53-58: Remove the plaintext ACCESS_ALLOWED_EMAIL entry from the
tracked vars configuration in wrangler.jsonc. Configure ACCESS_ALLOWED_EMAIL as
a Worker secret via the deployment process, preserving the existing
env.ACCESS_ALLOWED_EMAIL lookup and BackupEnvironment typing without other code
changes.
In `@packages/shared/src/backup-staging.ts`:
- Around line 181-182: Update the r2Indexes validation in the relevant
staging-manifest guard to require every key to be included in
backupR2BucketLabels, while retaining the existing isFileSummary validation for
values. Match the key-validation behavior of isR2Indexes in
backup-full-manifest.ts so unexpected bucket labels are rejected.
In `@packages/worker/src/dr/dr-restore.ts`:
- Around line 236-321: Update restoreStoragePhase to verify each loaded storage
dump’s SHA-256 checksum against entry.sha256 before parsing or calling
runner.importStorage. Reuse the existing blob-verification helper used by
restoreR2Phase and restoreArtifactsPhase where applicable, and fail with the
established integrity error behavior before any destructive replace import
occurs.
In `@packages/worker/src/dr/exporter.ts`:
- Around line 466-518: In the R2 export loop, snapshot progress.r2PartialNdjson
before processing each listed page. When the inner time-budget check exits
mid-page, restore that snapshot before persistProgress so the page is
reprocessed without duplicating index entries; keep blob deduplication through
putBlobIfAbsent and preserve normal page-boundary cursor advancement.
In `@tools/disaster-recovery/seal-escrow.ts`:
- Around line 126-212: Update the write-once error created by
isWriteOnceRejection inside putSealedEscrowBlob so it no longer claims the
rejection is enforced by an escrow/ bucket lock rule. Use wording that
accurately describes the remote object as write-once and retain guidance to
rotate ESCROW_KEY_VERSION for a new key.
---
Nitpick comments:
In `@packages/shared/src/backup-full-manifest.ts`:
- Line 42: Update knownR2Labels to derive its values from the shared
backupR2BucketLabels symbol imported from backup-staging.ts, rather than
maintaining a hardcoded label list. Preserve Set<string> behavior so validation
automatically accepts any labels added to the shared source.
In `@packages/worker/src/storage-runner.import.node.test.ts`:
- Around line 17-56: Add a test covering applyImportStoragePage with mode
'replace', replacePage 'first', and an empty entries array, verifying the
existing storage contents are cleared and the result reports ok, written: 0, and
cleared: true. Use the same createMemoryStorage fixture and assertions as the
neighboring replace-page tests.
🪄 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: 076f85de-7ca7-4e6d-acb3-b8f8a4f2958d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (60)
.github/workflows/dr-escrow.ymldocs/contributing/architecture/primitives.yamldocs/contributing/disaster-recovery.mddocs/contributing/environment-variables.mddocs/contributing/secret-rotation.mddocs/contributing/security.mddocs/contributing/setup-manifest.mdpackages/backup-control-plane/access-auth.node.test.tspackages/backup-control-plane/access-auth.tspackages/backup-control-plane/backup-control-plane-test-support.tspackages/backup-control-plane/backup-runtime.tspackages/backup-control-plane/backup-types.tspackages/backup-control-plane/control-plane-fetch.node.test.tspackages/backup-control-plane/control-plane-fetch.tspackages/backup-control-plane/control-plane-ui.tspackages/backup-control-plane/d1-export-api.node.test.tspackages/backup-control-plane/d1-export-api.tspackages/backup-control-plane/d1-import-api.tspackages/backup-control-plane/durable-export.node.test.tspackages/backup-control-plane/durable-export.tspackages/backup-control-plane/full-manifest-signing.node.test.tspackages/backup-control-plane/full-manifest-signing.tspackages/backup-control-plane/immutable-storage.tspackages/backup-control-plane/production-restore.node.test.tspackages/backup-control-plane/production-restore.tspackages/backup-control-plane/restore-confirm-token.node.test.tspackages/backup-control-plane/restore-confirm-token.tspackages/backup-control-plane/restore-drill.node.test.tspackages/backup-control-plane/restore-drill.tspackages/backup-control-plane/restore-workflow.tspackages/backup-control-plane/seal-full-backup.node.test.tspackages/backup-control-plane/seal-full-backup.tspackages/backup-control-plane/worker.tspackages/backup-control-plane/wrangler.jsoncpackages/shared/src/backup-full-manifest.node.test.tspackages/shared/src/backup-full-manifest.tspackages/shared/src/backup-staging.tspackages/worker/package.jsonpackages/worker/src/dr/backup-s3.tspackages/worker/src/dr/dr-restore.node.test.tspackages/worker/src/dr/dr-restore.tspackages/worker/src/dr/exporter.node.test.tspackages/worker/src/dr/exporter.tspackages/worker/src/dr/sha256.tspackages/worker/src/dr/storage-identity.tspackages/worker/src/env-schema.tspackages/worker/src/index.tspackages/worker/src/index.workers.test.tspackages/worker/src/maintenance-handler.node.test.tspackages/worker/src/maintenance-handler.tspackages/worker/src/security/public-route-hardening.workers.test.tspackages/worker/src/storage-runner.import.node.test.tspackages/worker/src/storage-runner.tspackages/worker/wrangler.jsonctools/disaster-recovery/readme.mdtools/disaster-recovery/restore-trust-and-verification.node.test.tstools/disaster-recovery/seal-escrow.node.test.tstools/disaster-recovery/seal-escrow.tstools/disaster-recovery/trusted-backup-manifest-public-keys.jsontools/disaster-recovery/trusted-d1-restore-identities.json
| try { | ||
| const progress = await step.do('run-production-restore', async () => | ||
| runProductionRestore(this.env, event.payload), | ||
| ) | ||
| // Persist progress (including warnings) as step output, then fail the | ||
| // workflow instance when restore finished with any dr-restore warnings. | ||
| if (progress.phase === 'failed' || progress.warnings.length > 0) { | ||
| throw new NonRetryableError( | ||
| progress.errorMessage ?? 'production restore completed with warnings', | ||
| progress.errorCode ?? 'dr-restore-warnings', | ||
| ) | ||
| } | ||
| return progress |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Cloudflare Workflows step.do default retry behavior when callback throws
💡 Result:
In Cloudflare Workflows, when the callback provided to step.do throws an error, the default behavior depends on the type of error thrown [1][2]. If a standard Error (or any error other than a NonRetryableError) is thrown, the step will automatically retry according to the configuration defined in the step's retries policy [2][3]. The step will continue to retry until it either succeeds or reaches the configured retry limit [2][3]. If the retry limit is exceeded, the error propagates to the top level, potentially failing the Workflow instance [1][2]. If you wish to stop retries immediately and prevent the Workflow from attempting further retries for that specific step, you must throw a NonRetryableError [1][2]. Throwing a NonRetryableError causes the step to fail immediately, bypassing any remaining retry attempts and propagating the error to the top level [1][2]. You can configure the retry behavior (such as limit, delay, and backoff strategy) by passing a WorkflowStepConfig object to the step.do method [1][2]. Top results: [1], [2], [3]
Citations:
- 1: https://developers.cloudflare.com/workflows/build/workers-api/
- 2: https://developers.cloudflare.com/workflows/build/sleeping-and-retrying/
- 3: Weird way to handle errors in workflows cloudflare/cloudflare-docs#28638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file =="
fd -a 'restore-workflow\.ts$' . | sed 's#^\./##'
echo "== outline =="
ast-grep outline packages/backup-control-plane/restore-workflow.ts || true
echo "== relevant lines =="
cat -n packages/backup-control-plane/restore-workflow.ts | sed -n '1,220p'
echo "== BackupError definitions/usages =="
rg -n "class BackupError|interface BackupError|enum BackupError|BackupError|step\.do|capturePreRestoreSafetyExport|runProductionRestore|NonRetryableError" packages/backup-control-plane packages --glob '*.ts' --glob '*.tsx' | head -n 300Repository: kentcdodds/kody
Length of output: 30091
🌐 Web query:
site:developers.cloudflare.com/workflows build Workers API step.do retries error type throws documentation
💡 Result:
In Cloudflare Workflows, when using the step.do API, you can control retry behavior and error propagation using the following mechanisms: Retry Configuration You can configure retry behavior for a step by passing an optional WorkflowStepConfig object as the second argument to step.do [1][2][3]. This configuration allows you to define: - retries.limit: The maximum number of retry attempts (up to 10,000) [4][5]. - retries.delay: A fixed duration (as a number in milliseconds or a string) or a dynamic delay function [4][1][5]. - retries.backoff: The algorithm to use (constant, linear, or exponential) [4][5]. - timeout: A duration after which a step attempt is considered failed [4][5][2]. Non-Retryable Errors To force a Workflow step to stop retrying immediately and propagate an error to the top level (the run function), you should throw an instance of NonRetryableError [4][1][3]. Throwing this specific error prevents further retry attempts for that step [1][2]. Error Handling and Propagation - Standard Errors: If an uncaught exception (other than NonRetryableError) is thrown within a step, the Workflow runtime will follow the retry policy defined in the step's configuration [4][5]. If the retry limit is reached, the Workflow instance will transition to an Errored state [4][5]. - Catching Errors: You can wrap step.do calls in a try...catch block to handle errors manually, perform cleanup tasks, or trigger conditional logic [4][5]. - Python Specifics: In the Python SDK, while you can catch standard built-in errors, some custom or specific errors may not be re-instantiated correctly through the RPC layer. It is recommended to catch using the general Exception class when working within Workflow steps [6].
Citations:
- 1: https://developers.cloudflare.com/workflows/build/workers-api/
- 2: https://pedro-2025-04-30-docs-add-quotes.preview.developers.cloudflare.com/workflows/build/workers-api/
- 3: https://developers.cloudflare.com/workflows/build/workers-api/index.md
- 4: https://developers.cloudflare.com/workflows/build/sleeping-and-retrying/
- 5: https://developers.cloudflare.com/workflows/build/sleeping-and-retrying/index.md
- 6: https://developers.cloudflare.com/workflows/python/python-workers-api/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== BackupError definition =="
cat -n packages/backup-control-plane/backup-policy.ts | sed -n '1,160p'
echo "== production-restore relevant definitions == "
cat -n packages/backup-control-plane/production-restore.ts | sed -n '230,415p'
echo "== production restore retryable examples =="
cat -n packages/backup-control-plane/production-restore.ts | sed -n '415,485p'
echo "== durable export step retry pattern =="
cat -n packages/backup-control-plane/durable-export.ts | sed -n '80,150p'
cat -n packages/backup-control-plane/backup-runtime.ts | sed -n '70,135p'Repository: kentcdodds/kody
Length of output: 19285
Split restore phases into separate steps before making any phase retryable.
step.do retries non-NonRetryableError failures, and the current workflow rethrows retryable BackupErrors. A failure during the D1 import or store restore retry loop then re-executes the whole runProductionRestore callback on the next attempt, including capturePreRestoreSafetyExport; this can record the already-restored database as the pre-restore rollback snapshot instead of capturing just what failed. Separate step.do('capture-pre-restore-safety-export'), step.do('import-d1'), and step.do('restore-stores') steps would make the already-completed safety export memoized and prevent retrying it.
🤖 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/restore-workflow.ts` around lines 24 - 36, The
restore workflow currently wraps all phases in the single runProductionRestore
step, causing retries to repeat capturePreRestoreSafetyExport. Split the flow
into separate step.do calls for capture-pre-restore-safety-export, import-d1,
and restore-stores, wiring each phase’s outputs into the next while preserving
the existing warning and failure handling. Ensure retries of import or store
restoration do not re-execute the safety export.
| "ACCESS_TEAM_DOMAIN": "kentcdodds.cloudflareaccess.com", | ||
| "ACCESS_APP_AUD": "769c70e393e2652f5878af99333322184a35cc3aef53376344f8753805e42a76", | ||
| "ACCESS_ALLOWED_EMAIL": "me@kentcdodds.com", | ||
| // DR (KCD) account: isolated restore drills only, never production. | ||
| "DRILL_ACCOUNT_ID": "a41d50ecaf0ae0f86dd1824ef6729cb2", | ||
| "PRIMARY_WORKER_ORIGIN": "https://heykody.dev", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Personal email committed in plaintext config.
ACCESS_ALLOWED_EMAIL embeds a real individual's email address directly in a git-tracked vars block, where it persists in history indefinitely. Since this value is only compared against the JWT email claim at runtime, it doesn't need to be build-time visible — moving it to a Worker secret avoids the PII exposure with no behavior change.
🔒 Suggested approach
"vars": {
...
- "ACCESS_ALLOWED_EMAIL": "me@kentcdodds.com",
"DRILL_ACCOUNT_ID": "a41d50ecaf0ae0f86dd1824ef6729cb2",
"PRIMARY_WORKER_ORIGIN": "https://heykody.dev",
},Set ACCESS_ALLOWED_EMAIL via wrangler secret put ACCESS_ALLOWED_EMAIL instead, and read it from env.ACCESS_ALLOWED_EMAIL as before (it's already typed as a string on BackupEnvironment, so no code changes are needed beyond the deploy step).
📝 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.
| "ACCESS_TEAM_DOMAIN": "kentcdodds.cloudflareaccess.com", | |
| "ACCESS_APP_AUD": "769c70e393e2652f5878af99333322184a35cc3aef53376344f8753805e42a76", | |
| "ACCESS_ALLOWED_EMAIL": "me@kentcdodds.com", | |
| // DR (KCD) account: isolated restore drills only, never production. | |
| "DRILL_ACCOUNT_ID": "a41d50ecaf0ae0f86dd1824ef6729cb2", | |
| "PRIMARY_WORKER_ORIGIN": "https://heykody.dev", | |
| "ACCESS_TEAM_DOMAIN": "kentcdodds.cloudflareaccess.com", | |
| "ACCESS_APP_AUD": "769c70e393e2652f5878af99333322184a35cc3aef53376344f8753805e42a76", | |
| // DR (KCD) account: isolated restore drills only, never production. | |
| "DRILL_ACCOUNT_ID": "a41d50ecaf0ae0f86dd1824ef6729cb2", | |
| "PRIMARY_WORKER_ORIGIN": "https://heykody.dev", |
🧰 Tools
🪛 Betterleaks (1.6.1)
[high] 54-54: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 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/wrangler.jsonc` around lines 53 - 58, Remove
the plaintext ACCESS_ALLOWED_EMAIL entry from the tracked vars configuration in
wrangler.jsonc. Configure ACCESS_ALLOWED_EMAIL as a Worker secret via the
deployment process, preserving the existing env.ACCESS_ALLOWED_EMAIL lookup and
BackupEnvironment typing without other code changes.
| while (progress.r2LabelIndex < backupR2BucketLabels.length) { | ||
| if (Date.now() - input.startedAtMs >= input.timeBudgetMs) return true | ||
| const label = backupR2BucketLabels[progress.r2LabelIndex]! | ||
| const bucket = r2BindingForLabel(env, label) | ||
| const listed = await bucket.list({ | ||
| cursor: progress.r2ListCursor ?? undefined, | ||
| limit: 100, | ||
| }) | ||
| for (const object of listed.objects) { | ||
| if (Date.now() - input.startedAtMs >= input.timeBudgetMs) { | ||
| await persistProgress(s3, session) | ||
| return true | ||
| } | ||
| if (object.size > drExportMaxObjectBytes) { | ||
| progress.warnings.push( | ||
| `Skipped ${label} object ${object.key}: size ${object.size} exceeds ${drExportMaxObjectBytes} bytes`, | ||
| ) | ||
| input.counts.r2ObjectsProcessed += 1 | ||
| continue | ||
| } | ||
| const body = await bucket.get(object.key) | ||
| if (!body) { | ||
| progress.warnings.push( | ||
| `Missing ${label} object during export: ${object.key}`, | ||
| ) | ||
| input.counts.r2ObjectsProcessed += 1 | ||
| continue | ||
| } | ||
| const bytes = new Uint8Array(await body.arrayBuffer()) | ||
| const digest = await sha256Hex(bytes) | ||
| await putBlobIfAbsent({ s3, sha256: digest, bytes, progress }) | ||
| const indexEntry = { | ||
| key: object.key, | ||
| size: bytes.byteLength, | ||
| sha256: digest, | ||
| } satisfies R2IndexEntry | ||
| progress.r2PartialNdjson += ndjsonLine(indexEntry) | ||
| input.counts.r2ObjectsProcessed += 1 | ||
| } | ||
| if (listed.truncated) { | ||
| progress.r2ListCursor = listed.cursor | ||
| await persistProgress(s3, session) | ||
| continue | ||
| } | ||
| const objectKey = stagingR2IndexKey(progress.day, label) | ||
| const body = progress.r2PartialNdjson | ||
| await s3.put(objectKey, body, { contentType: 'application/x-ndjson' }) | ||
| progress.r2Completed[label] = await fileSummary(objectKey, body) | ||
| progress.r2LabelIndex += 1 | ||
| progress.r2ListCursor = null | ||
| progress.r2PartialNdjson = '' | ||
| await persistProgress(s3, session) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Mid-page budget exhaustion duplicates R2 index entries on resume.
The inner budget check persists progress and returns without advancing r2ListCursor. Because R2's list cursor only advances at page boundaries, the next tick re-lists the same page and re-appends the already-processed objects into progress.r2PartialNdjson, producing duplicate rows in the sealed R2 index. In the worst case (one page never fits the budget) the phase never makes progress. Snapshot the partial NDJSON at page start and restore it on mid-page exit so resume re-processes the page cleanly (already-written blobs are deduped by the HEAD in putBlobIfAbsent).
Note the test harness's createR2 mock never returns truncated: true and no test exhausts the budget mid-R2-page, so this path is currently uncovered.
🐛 Proposed fix
const listed = await bucket.list({
cursor: progress.r2ListCursor ?? undefined,
limit: 100,
})
+ // R2 list cursors only advance at page boundaries, so a mid-page
+ // stop cannot resume where it left off. Remember the buffer at page
+ // start and roll back on exit so the next tick re-lists this page
+ // without duplicating index rows (blobs are deduped via HEAD).
+ const r2PartialAtPageStart = progress.r2PartialNdjson
for (const object of listed.objects) {
if (Date.now() - input.startedAtMs >= input.timeBudgetMs) {
+ progress.r2PartialNdjson = r2PartialAtPageStart
await persistProgress(s3, session)
return true
}📝 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.
| while (progress.r2LabelIndex < backupR2BucketLabels.length) { | |
| if (Date.now() - input.startedAtMs >= input.timeBudgetMs) return true | |
| const label = backupR2BucketLabels[progress.r2LabelIndex]! | |
| const bucket = r2BindingForLabel(env, label) | |
| const listed = await bucket.list({ | |
| cursor: progress.r2ListCursor ?? undefined, | |
| limit: 100, | |
| }) | |
| for (const object of listed.objects) { | |
| if (Date.now() - input.startedAtMs >= input.timeBudgetMs) { | |
| await persistProgress(s3, session) | |
| return true | |
| } | |
| if (object.size > drExportMaxObjectBytes) { | |
| progress.warnings.push( | |
| `Skipped ${label} object ${object.key}: size ${object.size} exceeds ${drExportMaxObjectBytes} bytes`, | |
| ) | |
| input.counts.r2ObjectsProcessed += 1 | |
| continue | |
| } | |
| const body = await bucket.get(object.key) | |
| if (!body) { | |
| progress.warnings.push( | |
| `Missing ${label} object during export: ${object.key}`, | |
| ) | |
| input.counts.r2ObjectsProcessed += 1 | |
| continue | |
| } | |
| const bytes = new Uint8Array(await body.arrayBuffer()) | |
| const digest = await sha256Hex(bytes) | |
| await putBlobIfAbsent({ s3, sha256: digest, bytes, progress }) | |
| const indexEntry = { | |
| key: object.key, | |
| size: bytes.byteLength, | |
| sha256: digest, | |
| } satisfies R2IndexEntry | |
| progress.r2PartialNdjson += ndjsonLine(indexEntry) | |
| input.counts.r2ObjectsProcessed += 1 | |
| } | |
| if (listed.truncated) { | |
| progress.r2ListCursor = listed.cursor | |
| await persistProgress(s3, session) | |
| continue | |
| } | |
| const objectKey = stagingR2IndexKey(progress.day, label) | |
| const body = progress.r2PartialNdjson | |
| await s3.put(objectKey, body, { contentType: 'application/x-ndjson' }) | |
| progress.r2Completed[label] = await fileSummary(objectKey, body) | |
| progress.r2LabelIndex += 1 | |
| progress.r2ListCursor = null | |
| progress.r2PartialNdjson = '' | |
| await persistProgress(s3, session) | |
| } | |
| while (progress.r2LabelIndex < backupR2BucketLabels.length) { | |
| if (Date.now() - input.startedAtMs >= input.timeBudgetMs) return true | |
| const label = backupR2BucketLabels[progress.r2LabelIndex]! | |
| const bucket = r2BindingForLabel(env, label) | |
| const listed = await bucket.list({ | |
| cursor: progress.r2ListCursor ?? undefined, | |
| limit: 100, | |
| }) | |
| // R2 list cursors only advance at page boundaries, so a mid-page | |
| // stop cannot resume where it left off. Remember the buffer at page | |
| // start and roll back on exit so the next tick re-lists this page | |
| // without duplicating index rows (blobs are deduped via HEAD). | |
| const r2PartialAtPageStart = progress.r2PartialNdjson | |
| for (const object of listed.objects) { | |
| if (Date.now() - input.startedAtMs >= input.timeBudgetMs) { | |
| progress.r2PartialNdjson = r2PartialAtPageStart | |
| await persistProgress(s3, session) | |
| return true | |
| } | |
| if (object.size > drExportMaxObjectBytes) { | |
| progress.warnings.push( | |
| `Skipped ${label} object ${object.key}: size ${object.size} exceeds ${drExportMaxObjectBytes} bytes`, | |
| ) | |
| input.counts.r2ObjectsProcessed += 1 | |
| continue | |
| } | |
| const body = await bucket.get(object.key) | |
| if (!body) { | |
| progress.warnings.push( | |
| `Missing ${label} object during export: ${object.key}`, | |
| ) | |
| input.counts.r2ObjectsProcessed += 1 | |
| continue | |
| } | |
| const bytes = new Uint8Array(await body.arrayBuffer()) | |
| const digest = await sha256Hex(bytes) | |
| await putBlobIfAbsent({ s3, sha256: digest, bytes, progress }) | |
| const indexEntry = { | |
| key: object.key, | |
| size: bytes.byteLength, | |
| sha256: digest, | |
| } satisfies R2IndexEntry | |
| progress.r2PartialNdjson += ndjsonLine(indexEntry) | |
| input.counts.r2ObjectsProcessed += 1 | |
| } | |
| if (listed.truncated) { | |
| progress.r2ListCursor = listed.cursor | |
| await persistProgress(s3, session) | |
| continue | |
| } | |
| const objectKey = stagingR2IndexKey(progress.day, label) | |
| const body = progress.r2PartialNdjson | |
| await s3.put(objectKey, body, { contentType: 'application/x-ndjson' }) | |
| progress.r2Completed[label] = await fileSummary(objectKey, body) | |
| progress.r2LabelIndex += 1 | |
| progress.r2ListCursor = null | |
| progress.r2PartialNdjson = '' | |
| await persistProgress(s3, session) | |
| } |
🤖 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/dr/exporter.ts` around lines 466 - 518, In the R2 export
loop, snapshot progress.r2PartialNdjson before processing each listed page. When
the inner time-budget check exits mid-page, restore that snapshot before
persistProgress so the page is reprocessed without duplicating index entries;
keep blob deduplication through putBlobIfAbsent and preserve normal
page-boundary cursor advancement.
| if (error instanceof BackupError && !error.retryable) { | ||
| throw new NonRetryableError(error.message, error.code) | ||
| } | ||
| throw error |
There was a problem hiding this comment.
Restore workflow retries whole restore
Medium Severity
The production restore Workflow runs all of runProductionRestore inside a single step.do with default step retries. When a retryable BackupError escapes (for example transient dr-restore HTTP or network failures after D1 import), the platform can re-execute the entire step, triggering another safety export, another production D1 import, and a fresh chunked store restore with no persisted cursor.
Reviewed by Cursor Bugbot for commit 1065d07. Configure here.
There was a problem hiding this comment.
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/backup-control-plane/d1-import-api.node.test.ts`:
- Around line 12-15: Update importPollSequence and both terminal-success tests
to track poll consumption and assert that exactly one expected poll response was
used. Expose the poll count or assertion from importPollSequence, then have each
test verify it after awaiting the import operation, ensuring the implementation
reaches the polling phase and observes the terminal response.
🪄 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: 974b4092-6562-4598-abda-8fef6748b61e
📒 Files selected for processing (6)
docs/contributing/disaster-recovery.mdpackages/backup-control-plane/backup-types.tspackages/backup-control-plane/d1-import-api.node.test.tspackages/backup-control-plane/d1-import-api.tspackages/backup-control-plane/seal-full-backup.node.test.tspackages/backup-control-plane/seal-full-backup.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/backup-control-plane/seal-full-backup.node.test.ts
- packages/backup-control-plane/backup-types.ts
- docs/contributing/disaster-recovery.md
- packages/backup-control-plane/d1-import-api.ts
- packages/backup-control-plane/seal-full-backup.ts
…me dedupe, provisioner parity
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5fd03b4. Configure here.
| const existingSummary = await s3.getText(stagingSummaryKey(day)) | ||
| if (existingSummary) { | ||
| return empty('already-complete') | ||
| } |
There was a problem hiding this comment.
Prior staging days never resume
Medium Severity
Each runDrExportTick always targets formatUtcDay(now) and returns immediately outside the 00:30–02:10 UTC window. If staging/{day}/exporter/progress.json for an earlier UTC day never reaches summary.json before the window ends, later cron ticks work on the new day and never resume the stuck day’s exporter.
Reviewed by Cursor Bugbot for commit 5fd03b4. Configure here.
) * fix(dr): bound D1 row sizes so exports stay importable 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> * fix(dr): stop re-downloading from D1 at backup finalization 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> * feat(dr): finish nightly staging and alert when backups go missing 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> * fix(email): resolve import conflict from main merge Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
3 participants
Enables the previously inert disaster-recovery system end to end and gives it an operator UI. The DR control plane is already deployed, enabled, and verified live (see "Live verification" below).
What this does
Production side (
packages/worker)dr_exportscheduled lane (00:30–02:10 UTC window, resumable ~20s ticks) stages the canonical non-D1 stores into the DR-account R2 bucket over the S3 API:StorageRunnerdurable-object dumps (NDJSON per storage id),EMAIL_BLOBS+COMMUNITY_ASSETSobjects deduplicated into a content-addressedblobs/sha256/{hash}store, and published package/job source snapshots fromBUNDLE_ARTIFACTS_KV. Gated byDR_EXPORT_ENABLED+ credentials; progress writes are etag-conditional to prevent overlapping-cron races.StorageRunner.importStoragepaged replace RPC and a fail-closedPOST /__maintenance/dr-restorebearer-secret endpoint (constant-time compare) that restores DO/R2/KV state from a sealed day in bounded chunks; missing dumps/blobs or digest mismatches hard-fail..github/workflows/dr-escrow.yml+tools/disaster-recovery/seal-escrow.ts: one-click workflow that sealsSECRET_STORE_KEYunder an operator passphrase (PBKDF2 600k + AES-256-GCM) and uploads the sealed blob to the DR bucket. Plaintext never leaves the job.DR control plane (
packages/backup-control-plane)fetchhandler protected by Cloudflare Access plus in-worker JWT verification (Cf-Access-Jwt-Assertion: RS256 against the team JWKS with refresh cooldown,iss/audpins, email allowlistme@kentcdodds.com),Sec-Fetch-Site: same-originon all POSTs.dr-restorecalls; any restore warnings error the workflow).daily/full/{day}/with a new signed full-backup manifest (packages/shared/src/backup-full-manifest.ts).Live-API fixes the first real runs surfaced (the pre-existing export client had never run against production):
status: "active"while processing — the old parser treated that as malformed and would have failed every night.DigestStreamiscrypto.DigestStreamin the Workers runtime, not a bare global; the test harness had masked this by patchingglobalThis.Live verification (all against production data)
kody-production-d1-backupsatkody-dr.kentcdodds.com(workers.dev disabled), behind a new Cloudflare Access app pinned to me@kentcdodds.com; unauthenticated requests 302 to the Access login and the worker independently verifies the JWT.true.sha256 cadc1498…), signature verified against the pinned public key.PRAGMA quick_checkok,foreign_key_checkclean, 63 tables, 30 users / 96 jobs / 74 packages / 176 emails present.kody-dr-2026-07); guardrail tests pin the exact registry contents.Remaining manual steps (documented in the runbook)
SECRET_ESCROW_PASSPHRASE+DR_BACKUP_*(R2 token) and run thedr-escrowworkflow once.Testing
npm run validategreen locally (format, lint, typecheck, 1270+ unit tests, Playwright E2E, MCP E2E, structure checks).System recap — extends the backup control plane (medium risk)
Mode: recap · Base:
main@d073014· Head:26309abClassification: extends — no new primitive ids;
backup-control-planeis materially extended (multi-store capture, operator UI, restore flows) and its ownership roots now include the production-side exporter and shared contracts.Primitives touched
backup-control-planedurable-storageimportStoragepaged replace RPC (restore counterpart ofexportStorage)scheduled-crondr_exportlane; control-plane hourly cron also auto-sealssecretsSECRET_STORE_KEYsealed-escrow workflow; store code itself unchangedSystem map
The nightly exporter stages DO/R2/KV state into the DR bucket where the control plane seals and signs it; the restore path runs the same boundary in reverse under Access-gated confirmation.
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 handler"]:::extended durableStorage["durable-storage<br/>Durable storage buckets"]:::extended r2Stores["r2-object-stores<br/>EMAIL_BLOBS / COMMUNITY_ASSETS"]:::untouched backupCP["backup-control-plane<br/>Production backup control plane"]:::extended secretsPrim["secrets<br/>Secret store"]:::touched scheduledCron -->|"dr_export lane: exportStorage pages → staging NDJSON"| durableStorage scheduledCron -->|"list + sha256 → blobs/sha256/{hash} (S3 SigV4)"| r2Stores scheduledCron -->|"staging/{day}/… + summary.json"| backupCP backupCP -->|"seal + Ed25519 full manifest; POST /__maintenance/dr-restore (bearer, chunked)"| durableStorage secretsPrim -->|"dr-escrow.yml seals SECRET_STORE_KEY → escrow/ (PBKDF2+AES-GCM)"| backupCP classDef touched fill:#1a7f37,color:#fff classDef extended fill:#9a6700,color:#fff classDef added fill:#cf222e,color:#fff classDef untouched fill:#57606a,color:#fffChange flow
sequenceDiagram participant Op as Operator (Access-authenticated) participant CP as DR control plane worker participant CF as Cloudflare API participant PW as kody-production Op->>CP: POST /actions/restore/prepare (day) CP-->>Op: confirm page (typed DB name + HMAC token, 10 min) Op->>CP: POST /actions/restore/execute CP->>CP: start RESTORE_WORKFLOW (id derived from token → replay collides) CP->>CF: pre-restore safety export → pre-restore/{day}/… CP->>CF: D1 import (sealed SQL, sha256-verified) loop until done CP->>PW: POST /__maintenance/dr-restore {day, cursor} PW->>PW: importStorage / R2 put / KV put (sha256-verified blobs) end CP-->>Op: /restore-status report (warnings ⇒ errored)Invariants
Per-user isolation: the exporter iterates all users by design (operator-level whole-platform backup); this is a documented, code-commented exception. Restore writes back the same per-user-scoped keys/ids it captured. Fail-closed posture preserved: enable gates, empty-secret 503s, identity allowlists, signature + digest verification before any restore write.
Summary by CodeRabbit