Skip to content

fix(sqlite): prevent silent ON DELETE CASCADE data loss during table-rebuild migrations#5784

Open
Zelys-DFKH wants to merge 1 commit into
drizzle-team:mainfrom
Zelys-DFKH:fix/sqlite-fk-cascade-migrate
Open

fix(sqlite): prevent silent ON DELETE CASCADE data loss during table-rebuild migrations#5784
Zelys-DFKH wants to merge 1 commit into
drizzle-team:mainfrom
Zelys-DFKH:fix/sqlite-fk-cascade-migrate

Conversation

@Zelys-DFKH
Copy link
Copy Markdown

Fixes #5782

Credit to @truffle-dev for the root-cause analysis in the thread.

Every table-rebuild migration from drizzle-kit starts with PRAGMA foreign_keys=OFF — correct, since the rename dance needs cascades disarmed. But SQLite documents that this pragma is a no-op inside a transaction: "foreign key constraint enforcement may only be enabled or disabled when there is no pending BEGIN or SAVEPOINT." The migrator wraps each migration in BEGIN … COMMIT. So the pragma drizzle-kit generates to protect data is neutralized by the very transaction that protects atomicity.

With ON DELETE CASCADE: the DROP TABLE inside the rebuild fires the cascade, child rows are deleted, the migration commits, and __drizzle_migrations records it as applied. No error. No rollback. Users find out when their tables are empty.

This is the silent variant of #1813 and #4089. Those throw SQLITE_CONSTRAINT, roll back, give the user an error message. This one succeeds.

Fix

Before BEGIN, scan pending migrations for PRAGMA foreign_keys=OFF (the drizzle-kit rebuild signature). If found, read the current FK state, issue the pragma at session level where SQLite actually honors it, skip PRAGMA foreign_keys statements inside the transaction loop (they'd be no-ops anyway), and restore in a finally block. The migrator saves FK state before the hoist and restores it conditionally. Always restoring ON would change the caller's connection state if they never had FK enforcement enabled, since SQLite defaults to OFF.

Both SQLiteSyncDialect and SQLiteAsyncDialect paths are covered. All SQLite drivers flow through these two methods.

  • The hoist applies to the full migration batch. If a batch contains a rebuild alongside simple migrations, FK enforcement is off for all of them. drizzle-kit generates one migration per schema change in practice, so this won't come up.
  • For HTTP-backed drivers (D1, sqlite-proxy), db.session.run() and db.session.transaction() can land on different connections, so the session-level pragma may not carry over. Those drivers don't support PRAGMA foreign_keys anyway, so needsFkHoist stays false.

Test

Test in integration-tests/tests/sqlite/better-sqlite.test.ts seeds a parent + child schema with ON DELETE CASCADE, runs a table-rebuild migration in the exact drizzle-kit output shape (PRAGMA foreign_keys=OFF; → create __new_* → INSERT SELECT → DROP → RENAME → PRAGMA foreign_keys=ON;), and asserts the child rows survive.

@truffle-dev
Copy link
Copy Markdown

The hoist+session-pragma shape is the right fix. PRAGMA foreign_keys inside a transaction is documented as a no-op (https://www.sqlite.org/pragma.html#pragma_foreign_keys), and defer_foreign_keys doesn't help because SQLite explicitly excludes cascading actions from deferral (https://www.sqlite.org/foreignkeys.html#fk_deferred). Migrator-side hoist is the only option.

Two design choices worth surfacing for reviewers:

  1. Inner-loop pragma skip is load-bearing, not just an optimization (dialect.ts:988 sync, :1051 async). fkPragmaRe is broader than fkOffRe on purpose: the inner skip swallows both the PRAGMA foreign_keys=OFF at the top of the rebuild (no-op inside the txn but a wasted session.run) and the trailing PRAGMA foreign_keys=ON that drizzle-kit emits. Without the broader skip, the inner ON pragma would attempt to flip FK enforcement mid-transaction; the hoist+finally pattern restores at session level instead, which is the only level SQLite honors.

  2. prevFkEnabled preservation is the conservative choice (dialect.ts:1005 sync, :1064 async). Unconditional restore-to-ON would change session state for callers who never opted into FK enforcement (SQLite's compile-time default is OFF). The if (prevFkEnabled) gate keeps the caller's connection exactly where it was.

One observation, not blocking: fkOffRe and fkPragmaRe implicitly encode the drizzle-kit rebuild output shape. If drizzle-kit ever splits the OFF/ON pragmas across migration files or wraps them in a savepoint, the detector misses. A short comment tying the regexes to the drizzle-kit-generated rebuild signature would help future maintenance.

Verified locally: with the fix, both child rows survive the rebuild; reverted, both are wiped silently. The severity here versus #1813 and #4089 is that there's no error and no rollback. __drizzle_migrations records the migration as applied, so the data loss is invisible until someone notices the empty table.

… CASCADE data loss

SQLite ignores PRAGMA foreign_keys inside a transaction. Drizzle-kit table-rebuild
migrations emit this pragma at the top, but the migrator wraps everything in
BEGIN...COMMIT, making it a no-op. With ON DELETE CASCADE, the DROP TABLE inside
the rebuild silently wipes child rows and the migration records as applied with no error.

Fix: scan pending migrations for the PRAGMA foreign_keys=OFF signature before BEGIN.
If found, hoist it to session level (where SQLite honors it), skip pragma statements
inside the transaction loop, and restore the previous FK state in a finally block.
Covers both SQLiteSyncDialect and SQLiteAsyncDialect.

Fixes drizzle-team#5782

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Zelys-DFKH Zelys-DFKH force-pushed the fix/sqlite-fk-cascade-migrate branch from 25df139 to 606405e Compare May 20, 2026 15:58
@Zelys-DFKH
Copy link
Copy Markdown
Author

Thanks for verifying this locally and spelling out the design choices. Good catch on the regex coupling. I've added a note above the definitions tying them to drizzle-kit's rebuild format. Pushing the amend now.

@truffle-dev
Copy link
Copy Markdown

The hoist+session-pragma shape is the right fix. PRAGMA foreign_keys inside a transaction is documented as a no-op (https://www.sqlite.org/pragma.html#pragma_foreign_keys), and defer_foreign_keys doesn't help because SQLite explicitly excludes cascading actions from deferral (https://www.sqlite.org/foreignkeys.html#fk_deferred). Migrator-side hoist is the only option.

Empirically verified against the dist with the dialect patch applied on top: before the fix the child rows go from 2 to 0 silently, after the fix they survive.

Before rebuild migration: child rows = [{"id":1,"parent_id":1},{"id":2,"parent_id":1}]

# unfixed dist:
After rebuild migration:  child rows = []
BUG CONFIRMED: child rows wiped by CASCADE during table-rebuild migration.

# dist with this PR applied:
After rebuild migration:  child rows = [{"id":1,"parent_id":1},{"id":2,"parent_id":1}]
FIX WORKING: child rows survived the table-rebuild migration.

Two design choices in the patch worth surfacing for reviewers:

  1. The inner-loop fkPragmaRe skip is load-bearing, not just an optimization (dialect.ts:988 sync, :1051 async). The broader regex swallows both the PRAGMA foreign_keys=OFF at the top of the rebuild (a wasted session.run after hoist) and the trailing PRAGMA foreign_keys=ON that drizzle-kit emits. Without the broader skip, the inner ON pragma would attempt to flip FK enforcement mid-transaction; the finally block restores at session level instead, which is the only level SQLite honors.

  2. prevFkEnabled preservation is the conservative choice (dialect.ts:1005 sync, :1064 async). Unconditional restore-to-ON would change session state for callers who never opted into FK enforcement (SQLite's compile-time default is OFF). The if (prevFkEnabled) gate keeps the caller's connection state exactly where it was.

One observation, not blocking: fkOffRe and fkPragmaRe implicitly encode the drizzle-kit rebuild output shape. If drizzle-kit ever splits the OFF/ON pragmas across migration files or wraps them in a savepoint, the detector misses. A short comment tying the regexes to the drizzle-kit-generated rebuild signature would help future maintenance.

One test bug to fix before CI runs: the added test at integration-tests/tests/sqlite/better-sqlite.test.ts:62-93 writes migrations as <migrationDir>/<timestamp>_<name>/migration.sql, but readMigrationFiles (drizzle-orm/src/migrator.ts:23-29) requires <migrationDir>/meta/_journal.json listing entries with tag fields and <migrationDir>/<tag>.sql files alongside. The test throws Error: Can't find meta/_journal.json file before reaching the migrator's migrate() code path, so it never exercises the bug it's supposed to prove. To make it pass, the test needs to write meta/_journal.json with entries: [{idx, when, tag, breakpoints}] plus flat <tag>.sql files at the migrationDir root (the layout drizzle-kit actually generates and the existing migrator test at line 19 uses).

The severity here versus #1813 and #4089 is that there's no error and no rollback. __drizzle_migrations records the migration as applied, so the data loss is invisible until someone notices the empty table.

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.

[BUG]: SQLite table-rebuild migrations silently wipe child tables for any FK with ON DELETE CASCADE

2 participants