fix(dr): fail closed on unrestorable D1 backups - #1225
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThe change adds versioned SQL statistics, validates D1 backup restorability across backup and restore workflows, and reports restorability in sealing, freshness checks, and the dashboard. Oversized, missing, conflicting, and corrupt statistics now produce explicit outcomes. ChangesD1 SQL restorability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BackupWorkflow
participant SqlStatsStorage
participant ManifestStorage
participant RestoreWorkflow
BackupWorkflow->>SqlStatsStorage: persist validated SQL statistics
SqlStatsStorage-->>BackupWorkflow: statistics accepted or error
BackupWorkflow->>ManifestStorage: publish canonical manifest
RestoreWorkflow->>SqlStatsStorage: validate referenced SQL statistics
SqlStatsStorage-->>RestoreWorkflow: restorable or BackupError
RestoreWorkflow->>ManifestStorage: continue restore only when valid
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
|
🔎 Preview deployed: https://kody-pr-1225.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/control-plane-ui.ts (1)
183-223: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSeparate the stats read failure from the manifest read failure.
readSqlRestorabilitynow runs inside the sametryasreadManifest. If the stats read or the JSON body read fails, thecatchat line 220 resetsd1Verifiedtonulland reports'D1 manifest unreadable'. The manifest was already read and verified at that point, so the dashboard shows a verified day as unverified and names the wrong cause. Wrap the restorability read in its owntryso the failure maps tod1Restorableand to a stats-specific warning.🐛 Suggested scope split
else { - const restorability = await readSqlRestorability( - env.BACKUP_BUCKET, - day, - manifest.payload.sql.objectKey, - ) - switch (restorability.kind) { + let restorability + try { + restorability = await readSqlRestorability( + env.BACKUP_BUCKET, + day, + manifest.payload.sql.objectKey, + ) + } catch { + warnings.push('D1 SQL stats unreadable') + } + switch (restorability?.kind) { + case undefined: + break case 'restorable':🤖 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/control-plane-ui.ts` around lines 183 - 223, Separate the readSqlRestorability call and its handling from the manifest try/catch in the surrounding control flow. Preserve d1Verified after a successful readManifest, and handle restorability or JSON-body failures by setting d1Restorable appropriately and adding a stats-specific warning instead of resetting d1Verified or reporting “D1 manifest unreadable.”
🧹 Nitpick comments (3)
packages/backup-control-plane/seal-full-backup.ts (1)
385-394: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the new seal-skip reasons.
These three branches return
incompletewithout a log record. The blob-missing branch at lines 517-524 emitsfull-backup-seal-skipped, so alerting depends on that event. Emit the same event here so an unrestorable, missing, or corrupt stats sibling is visible to operators instead of only through the HTTP response.♻️ Suggested change
case 'unrestorable': + safeLog({ + event: 'full-backup-seal-skipped', + status: 'failure', + day, + errorCode: 'backup-unrestorable-statements', + }) return { kind: 'incomplete', day, reason: 'backup-unrestorable-statements', } case 'missing': + safeLog({ + event: 'full-backup-seal-skipped', + status: 'failure', + day, + errorCode: 'backup-sql-stats-missing', + }) return { kind: 'incomplete', day, reason: 'backup-sql-stats-missing' } case 'corrupt': + safeLog({ + event: 'full-backup-seal-skipped', + status: 'failure', + day, + errorCode: 'backup-sql-stats-corrupt', + }) return { kind: 'incomplete', day, reason: 'backup-sql-stats-corrupt' }🤖 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/seal-full-backup.ts` around lines 385 - 394, Update the switch branches for unrestorable, missing, and corrupt stats to emit the same full-backup-seal-skipped event used by the blob-missing branch before returning incomplete. Preserve each branch’s existing day and reason values, and reuse the established logging mechanism and event payload shape.packages/shared/src/backup-sql-stats.node.test.ts (1)
15-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the limit from the shared constant and cover the valid oversized case.
The fixtures hardcode
100_000and100_001. Ifd1ImportMaxStatementByteschanges, thevalidfixture starts to throw and the failure reason is unclear. Import the constant instead.The suite also does not assert that a legitimately oversized record parses. That branch gates
unrestorableinreadSqlRestorability, so add it. A non-record input and a wrongschemaVersionare also uncovered.💚 Suggested test additions
+import { d1ImportMaxStatementBytes } from './backup-restore-safety.ts' + const valid = { schemaVersion: backupSqlStatsSchemaVersion, day: '2026-07-31', objectKey: 'daily/d1/database/2026-07-31/backup-bookmark.sql', maxStatementBytes: 50_000, oversizedStatementCount: 0, - importStatementLimitBytes: 100_000, + importStatementLimitBytes: d1ImportMaxStatementBytes, } assert.deepEqual(parseBackupSqlStats(valid), valid) + + const oversized = { + ...valid, + maxStatementBytes: d1ImportMaxStatementBytes + 1, + oversizedStatementCount: 1, + } + assert.deepEqual(parseBackupSqlStats(oversized), oversized) for (const corrupt of [ - { ...valid, importStatementLimitBytes: 100_001 }, - { ...valid, maxStatementBytes: 100_001 }, + null, + { ...valid, schemaVersion: backupSqlStatsSchemaVersion + 1 }, + { ...valid, importStatementLimitBytes: d1ImportMaxStatementBytes + 1 }, + { ...valid, maxStatementBytes: d1ImportMaxStatementBytes + 1 }, { ...valid, maxStatementBytes: 50_000, oversizedStatementCount: 1 }, ]) {🤖 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-sql-stats.node.test.ts` around lines 15 - 31, Update the backup SQL stats tests around parseBackupSqlStats to import and use the shared d1ImportMaxStatementBytes constant instead of hardcoded 100_000 values. Add a valid fixture with oversizedStatementCount greater than zero and the corresponding limit so it parses successfully, and add rejection cases for non-record input and an incorrect schemaVersion.packages/shared/src/backup-sql-stats.ts (1)
45-47: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConfirm the oversize boundary and avoid hard-pinning
importStatementLimitBytes.
maxStatementBytes === d1ImportMaxStatementBytesis treated as restorable, so the producer must only mark a statement oversized when its length is greater than the limit. If that comparison uses>=, exactly the limit can setoversizedStatementCount = 1, making a restorable row reportcorrupt.Also avoid requiring
importStatementLimitBytesto equal the current constant exactly. Ifd1ImportMaxStatementByteschanges, all already-persisted stats siblings fail the validation at once. Use the recordedimportStatementLimitBytesfor the restorability check, or bumpschemaVersionwith a documented migration.Also add parentheses around the second operand. Relational operators bind tighter than
!==, so the expression is correct, but the intent is not obvious.🤖 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-sql-stats.ts` around lines 45 - 47, Update the producer’s oversized-statement comparison to use a strict greater-than limit so statements exactly equal to d1ImportMaxStatementBytes remain restorable. In the validation expression around oversizedStatementCount and importStatementLimitBytes, compare against the recorded value.importStatementLimitBytes instead of hard-pinning d1ImportMaxStatementBytes, and parenthesize the relational operand for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/contributing/disaster-recovery.md`:
- Around line 373-378: Update the disaster-recovery documentation near the
“unrestorable export” statement to limit the “never receives a canonical day
manifest” guarantee to post-cutover exports. Explicitly note that historical bad
sealed days may already have canonical manifests and are intentionally left
unchanged, while preserving the existing catch-up retry guidance.
---
Outside diff comments:
In `@packages/backup-control-plane/control-plane-ui.ts`:
- Around line 183-223: Separate the readSqlRestorability call and its handling
from the manifest try/catch in the surrounding control flow. Preserve d1Verified
after a successful readManifest, and handle restorability or JSON-body failures
by setting d1Restorable appropriately and adding a stats-specific warning
instead of resetting d1Verified or reporting “D1 manifest unreadable.”
---
Nitpick comments:
In `@packages/backup-control-plane/seal-full-backup.ts`:
- Around line 385-394: Update the switch branches for unrestorable, missing, and
corrupt stats to emit the same full-backup-seal-skipped event used by the
blob-missing branch before returning incomplete. Preserve each branch’s existing
day and reason values, and reuse the established logging mechanism and event
payload shape.
In `@packages/shared/src/backup-sql-stats.node.test.ts`:
- Around line 15-31: Update the backup SQL stats tests around
parseBackupSqlStats to import and use the shared d1ImportMaxStatementBytes
constant instead of hardcoded 100_000 values. Add a valid fixture with
oversizedStatementCount greater than zero and the corresponding limit so it
parses successfully, and add rejection cases for non-record input and an
incorrect schemaVersion.
In `@packages/shared/src/backup-sql-stats.ts`:
- Around line 45-47: Update the producer’s oversized-statement comparison to use
a strict greater-than limit so statements exactly equal to
d1ImportMaxStatementBytes remain restorable. In the validation expression around
oversizedStatementCount and importStatementLimitBytes, compare against the
recorded value.importStatementLimitBytes instead of hard-pinning
d1ImportMaxStatementBytes, and parenthesize the relational operand for clarity.
🪄 Autofix
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: 2f4b9a42-1d01-436d-8ada-f5397398a7b7
📒 Files selected for processing (19)
docs/contributing/disaster-recovery.mdpackages/backup-control-plane/backup-control-plane-test-support.tspackages/backup-control-plane/backup-runtime.node.test.tspackages/backup-control-plane/backup-runtime.tspackages/backup-control-plane/backup-types.tspackages/backup-control-plane/control-plane-ui.node.test.tspackages/backup-control-plane/control-plane-ui.tspackages/backup-control-plane/freshness-check.node.test.tspackages/backup-control-plane/freshness-check.tspackages/backup-control-plane/production-restore.node.test.tspackages/backup-control-plane/production-restore.tspackages/backup-control-plane/readme.mdpackages/backup-control-plane/restore-drill.node.test.tspackages/backup-control-plane/restore-drill.tspackages/backup-control-plane/seal-full-backup.node.test.tspackages/backup-control-plane/seal-full-backup.tspackages/backup-control-plane/sql-statement-stats.tspackages/shared/src/backup-sql-stats.node.test.tspackages/shared/src/backup-sql-stats.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Prevent D1 exports containing statements above Cloudflare's import limit from being published, sealed, or restored as healthy disaster-recovery media. Fixes threat T4 in #1223.
Summary
Testing
npm run typecheck: passednpm run backup:build: passednpm run validate: passed (555 files, 1,866 tests)workerdE2E crashConductor report
Status: done
Squash-merged as
1256e8732626be3ad9f35d35041aed6f9c2ca0b0. All Track 4 gates are live, merged-main validation is green, and the DR backup control-plane deployment succeeded. Nothing remains for this track.System changes
System recap — extends an existing primitive (medium risk)
Mode: recap · Base:
main@63616db6· Head:d29a8b2eClassification: extends — changes the backup control plane's publication, health, sealing, and restore-safety contracts without adding a new system primitive.
Primitives touched
backup-control-planeSystem map
D1 export statistics now form a fail-closed gate from immutable SQL capture through every path that can label or consume backup media.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Invariants
Summary by CodeRabbit
New Features
Documentation