fix(dr): make D1 restore imports FK-safe with foreign_keys=OFF prelude - #943
Merged
Merged
Conversation
D1 remote import enforces FKs during CREATE TABLE, but Cloudflare exports are not topologically ordered, so drills failed with no such table: main.users. Verify the unmodified SQL MD5 against the signed manifest etag, then upload a prefixed body and use that prepared MD5 for import init/ingest.
📝 WalkthroughWalkthroughThe D1 import API now validates the original SQL digest, prepends ChangesD1 import flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RestoreFlow
participant R2Bucket
participant D1ImportAPI
participant D1
RestoreFlow->>R2Bucket: head SQL object
R2Bucket-->>RestoreFlow: object metadata or missing
RestoreFlow->>D1ImportAPI: sourceMd5Etag and loadSqlBody
D1ImportAPI->>R2Bucket: get SQL body
R2Bucket-->>D1ImportAPI: SQL stream or body
D1ImportAPI->>D1ImportAPI: prepend foreign-key pragma and compute upload MD5
D1ImportAPI->>D1: initialize and upload prepared SQL
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
kody-bot
marked this pull request as ready for review
July 25, 2026 20:26
Contributor
|
🔎 Preview deployed: https://kody-pr-943.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.
🧹 Nitpick comments (1)
packages/backup-control-plane/d1-import-api.ts (1)
340-379: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant second
loadSqlBody()call for non-stream bodies.For the
string/Uint8Arraybranch,firstalready holds the fully materialized bytes (not consumed byconsumeSqlBodyForHashes, unlike a stream reader). CallingloadSqlBody()again to getsecondjust to rebuild the same bytes is unnecessary work;firstcan be reused directly.♻️ Proposed refactor to avoid the redundant reload
const expectedSourceMd5 = hexMd5FromR2Etag(input.sourceMd5Etag) const prefix = new TextEncoder().encode(d1ImportForeignKeysOffPrefix) const first = await input.loadSqlBody() const hashes = await consumeSqlBodyForHashes(first, prefix) if (hashes.sourceMd5Hex !== expectedSourceMd5) { throw new BackupError( 'import-source-etag-mismatch', 'Backup SQL MD5 did not match the signed manifest R2 ETag', ) } - const second = await input.loadSqlBody() - if (typeof second === 'string' || second instanceof Uint8Array) { - const sourceBytes = toUint8Array(second) + if (typeof first === 'string' || first instanceof Uint8Array) { + const sourceBytes = toUint8Array(first) const uploadBytes = new Uint8Array( prefix.byteLength + sourceBytes.byteLength, ) uploadBytes.set(prefix, 0) uploadBytes.set(sourceBytes, prefix.byteLength) return { uploadBody: uploadBytes, uploadMd5Hex: hashes.uploadMd5Hex, sourceBytes: hashes.sourceBytes, } } + const second = await input.loadSqlBody() + if (typeof second === 'string' || second instanceof Uint8Array) { + throw new BackupError( + 'import-sql-body-inconsistent', + 'loadSqlBody() returned different body types on repeated invocation', + ) + } return { uploadBody: prependForeignKeysOffStream(prefix, second), uploadMd5Hex: hashes.uploadMd5Hex, sourceBytes: hashes.sourceBytes, }Note: this branch is currently only exercised by the buffer/string test cases, since both real callers (
production-restore.ts,restore-drill.ts) supplyloadSqlBodyfunctions returningR2Object.body, which is always aReadableStreamin the Workers runtime, so real-world impact is limited.🤖 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/d1-import-api.ts` around lines 340 - 379, Reuse the already loaded first body in prepareD1ImportUpload for non-stream inputs instead of calling input.loadSqlBody() a second time. Branch on first being a string or Uint8Array, preserve the existing prefixing and returned metadata, and only load the body again for stream inputs that are consumed by consumeSqlBodyForHashes.
🤖 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.
Nitpick comments:
In `@packages/backup-control-plane/d1-import-api.ts`:
- Around line 340-379: Reuse the already loaded first body in
prepareD1ImportUpload for non-stream inputs instead of calling
input.loadSqlBody() a second time. Branch on first being a string or Uint8Array,
preserve the existing prefixing and returned metadata, and only load the body
again for stream inputs that are consumed by consumeSqlBodyForHashes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a756fbb3-601f-45c0-abac-14ec27f66997
📒 Files selected for processing (7)
docs/contributing/disaster-recovery.mdpackages/backup-control-plane/d1-import-api.node.test.tspackages/backup-control-plane/d1-import-api.tspackages/backup-control-plane/production-restore.node.test.tspackages/backup-control-plane/production-restore.tspackages/backup-control-plane/restore-drill.tspackages/backup-control-plane/wrangler.jsonc
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.
Summary
Isolated restore drills were failing with
import-failed/no such table: main.usersbecause D1 remote import enforces foreign keys duringCREATE TABLE, while Cloudflare D1 exports are not topologically ordered.This change verifies the unmodified backup SQL MD5 against the signed manifest R2 ETag, then prefixes
PRAGMA foreign_keys=OFF;and uses the prepared body's MD5 for D1 import init/ingest. Stream bodies are loaded twice vialoadSqlBodyso large backups are not fully buffered in Worker memory.Operator follow-up after merge
packages/backup-control-plane) to the KCD account (not app CI).2026-07-24(or2026-07-23) at https://kody-dr.kentcdodds.comPRAGMA quick_checkok and cleanup of the drill DB.Test plan
packages/backup-control-planeunit tests (64) + typecheckSystem recap — extends the backup control plane (medium risk)
Mode: recap · Base:
main@0a57498b· Head:15818d8eClassification: extends — changes restore/drill D1 import behavior (FK-safe prepare + etag contract) inside the existing
backup-control-planeprimitive.Primitives touched
backup-control-planePRAGMA foreign_keys=OFF;, uploads prepared MD5; drill + production restore call sites updated;nodejs_compatenabled for streaming MD5System map
Change flow
Invariants
payload.sql.r2Etagbefore any transform.loadSqlBodytwice; no full-buffer requirement for streams).DRILL_ACCOUNT_ID; production restore path shares the same import prepare logic.Summary by CodeRabbit
Improvements
Documentation