From 6445fc3136ca441120c0a31fecc5bc696681c1a3 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Sat, 8 Aug 2026 15:58:58 +0100 Subject: [PATCH 1/3] Order migration 0021 so it never drops a table with children Applying 0021 to the remote database failed with "FOREIGN KEY constraint failed" while local succeeded. The migration opened with PRAGMA foreign_keys=OFF, which is a documented no-op inside a transaction - wrangler wraps each migration file in one, so it silently did nothing against D1. The tests ran statements outside a transaction, where the pragma works, which is precisely why they could not see the difference. PRAGMA defer_foreign_keys is not a substitute, which I verified before relying on it: every statement succeeds and PRAGMA foreign_key_check comes back clean, but COMMIT still fails, because DROP TABLE on a parent increments SQLite's deferred-violation counter once per child row and renaming the replacement into place never decrements it. SQLite's own 12-step ALTER procedure sidesteps this by demanding foreign_keys=OFF, the one thing unavailable here. The fix is ordering, not a pragma, so the pragma is removed rather than left in as decoration. extension_submissions is copied into a holding table and dropped before extensions is rebuilt, leaving extensions childless at the moment it is replaced; extension_revisions is created afterwards. 0021 now applies with foreign keys fully enforced. The developers rebuild is dropped for the same reason: developer_claims, developer_transfers and extensions all reference it, so it can never be dropped this way, and rebuilding all three children to replace a default nothing reads is not a trade worth making. created_at/updated_at keep the 1970 placeholder, with a note on why it stays. migrations.test.ts gains applyAllAsD1(), which runs the chain with foreign keys enforced and 0021 inside a transaction. Against 0021 as merged it reproduces the production failure exactly; against this one it passes. No data was changed by the failed run: it rolled back whole, 0021 is not recorded in d1_migrations, and the remote schema is still at 0020. --- src/services/extensions/v2/README.md | 25 ++-- .../0021_restructure_extensions_revisions.sql | 114 +++++++----------- .../v2/db/migrations/meta/0021_snapshot.json | 4 +- src/services/extensions/v2/db/schema.ts | 26 ++-- .../services/extensions/v2/migrations.test.ts | 76 +++++++++--- 5 files changed, 134 insertions(+), 111 deletions(-) diff --git a/src/services/extensions/v2/README.md b/src/services/extensions/v2/README.md index c5f9e2a..8cc1f4f 100644 --- a/src/services/extensions/v2/README.md +++ b/src/services/extensions/v2/README.md @@ -104,15 +104,22 @@ guaranteeing a published row is never half-written. `extension_revisions` (renamed from `extension_submissions` in migration 0021) holds proposed content, always attached to a real extension row and cascading with it. -Migration 0021 rebuilds `extensions`, `extension_revisions` and `developers` -in one step, because SQLite cannot relax `NOT NULL`, add a `CHECK`, or add a -foreign key in place. It also renames `extensions.author_id` to `developer_id` -(nothing public depended on the old name — v1's response field is `author` -either way) and replaces `developers.created_at`/`updated_at`'s placeholder -1970 default with `CURRENT_TIMESTAMP`, which is what every writer already -uses. Existing 1970 values are left alone: they are the only record those rows -have, and a timestamp invented at migration time would look real without being -so. +Migration 0021 rebuilds `extensions` and replaces `extension_submissions` with +`extension_revisions`, because SQLite cannot relax `NOT NULL`, add a `CHECK`, or +add a foreign key in place. It also renames `extensions.author_id` to +`developer_id` — nothing public depended on the old name, since v1's response +field is `author` either way. + +**Ordering in 0021 is load-bearing.** It never drops a table that still has +children, so `extension_submissions` is copied aside and dropped before +`extensions` is rebuilt. Foreign keys cannot be relaxed to avoid this: +`PRAGMA foreign_keys` is a no-op inside a transaction and wrangler wraps each +migration file in one, while `PRAGMA defer_foreign_keys` does not help either — +dropping a parent increments SQLite's deferred-violation counter per child row +and nothing decrements it, so the commit fails even when the data is sound. The +same constraint is why `developers` is not rebuilt: three tables reference it. +`migrations.test.ts` applies the chain under those conditions so this cannot +regress. Apply migrations **only from this repository**, from `db/migrations`, with `npm run db:migrate:extensions-v2:local` / `:remote`. The Extensions site has no D1 migration source. diff --git a/src/services/extensions/v2/db/migrations/0021_restructure_extensions_revisions.sql b/src/services/extensions/v2/db/migrations/0021_restructure_extensions_revisions.sql index d47d736..e6d8570 100644 --- a/src/services/extensions/v2/db/migrations/0021_restructure_extensions_revisions.sql +++ b/src/services/extensions/v2/db/migrations/0021_restructure_extensions_revisions.sql @@ -7,17 +7,40 @@ -- the first approval), and extension_submissions becomes extension_revisions: -- one proposed content version, always attached to a real extension row. -- --- The tables are rebuilt rather than ALTERed because SQLite cannot relax NOT --- NULL, add a CHECK, or add a foreign key in place. Two renames ride along, --- since the rebuild is already paid for: extensions.author_id becomes --- developer_id, and developers' created_at/updated_at lose the placeholder 1970 --- default that migration 0002 was forced to use and no writer ever produced. +-- extensions is rebuilt rather than ALTERed because SQLite cannot relax NOT +-- NULL, add a CHECK, or add a foreign key in place. author_id becomes +-- developer_id on the way through, since the rebuild is already paid for. -- -- Hand-written, not drizzle-kit-generated: the generated diff cannot infer the -- table rename or the backfills below non-interactively, so only the snapshot -- in meta/0021_snapshot.json comes from drizzle-kit. The end state is verified -- against schema.ts by test/services/extensions/v2/migrations.test.ts. -PRAGMA foreign_keys=OFF;--> statement-breakpoint +-- +-- This migration relaxes foreign keys nowhere, and cannot. An earlier version +-- opened with PRAGMA foreign_keys=OFF, passed locally and failed on the first +-- remote apply with a bare "FOREIGN KEY constraint failed". Two reasons, and +-- the first is the one that matters: +-- +-- * PRAGMA foreign_keys is a documented no-op inside a transaction, and +-- wrangler wraps each migration file in one. The pragma silently did +-- nothing remotely. The tests ran statements outside a transaction, where +-- it works, which is exactly why they could not see the difference. +-- * PRAGMA defer_foreign_keys is not a substitute. DROP TABLE on a parent +-- performs an implicit DELETE FROM that increments SQLite's deferred +-- violation counter once per child row, and renaming a replacement into +-- place never decrements it - so COMMIT fails even though +-- PRAGMA foreign_key_check reports nothing wrong. +-- +-- So the ordering below is load-bearing: nothing here ever drops a table that +-- still has children. extension_submissions is copied aside and dropped first, +-- which leaves extensions childless at the moment it is replaced, and +-- extension_revisions is created only afterwards. Verified by +-- "applies on D1's terms" in migrations.test.ts, which runs the whole chain +-- with foreign keys enforced and 0021 inside a transaction. +-- +-- The same constraint is why developers is not rebuilt here: three tables +-- reference it, so it can never be dropped this way. See the note on its +-- created_at default in schema.ts. -- idx_extensions_id_nocase, created further down, is the constraint that stops -- a new lowercase id colliding with an adopted mixed-case one. A catalogue @@ -104,59 +127,13 @@ WHERE LOWER(COALESCE(extension_id, json_extract(payload, '$.extension.id'))) DROP TABLE _reserved_submission_targets;--> statement-breakpoint --- developers first, while every table that references it is still the old one: --- the drop-and-rename re-parses every schema, and doing it with a referrer --- pointing at a dropped table is the case that errors. -CREATE TABLE `__new_developers` ( - `id` text PRIMARY KEY NOT NULL, - `type` text NOT NULL, - `name` text NOT NULL, - `url` text, - `owner_user_id` text, - `approved_at` text, - `created_at` text DEFAULT CURRENT_TIMESTAMP NOT NULL, - `updated_at` text DEFAULT CURRENT_TIMESTAMP NOT NULL, - `avatar_url` text, - `contact_email` text, - `ownership_epoch` integer DEFAULT 1 NOT NULL, - `content_revision` integer DEFAULT 1 NOT NULL, - `approved_revision` integer, - `approved_by` text, - `github_org_verified` integer, - `github_verification_note` text, - `github_verified_at` text, - `github_url_verified` integer, - `url_check_cooldown_until` text, - FOREIGN KEY (`owner_user_id`) REFERENCES `users`(`id`) ON UPDATE no action ON DELETE no action, - CONSTRAINT "developers_ownership_epoch_check" CHECK("__new_developers"."ownership_epoch" >= 1), - CONSTRAINT "developers_content_revision_check" CHECK("__new_developers"."content_revision" >= 1), - CONSTRAINT "developers_github_org_verified_check" CHECK("__new_developers"."github_org_verified" IN (0, 1)), - CONSTRAINT "developers_github_url_verified_check" CHECK("__new_developers"."github_url_verified" = 1) -);--> statement-breakpoint +-- extension_submissions is the only table referencing extensions, so it goes +-- first. CREATE TABLE ... AS SELECT copies the rows without carrying any +-- constraints across, which is what makes this holding table safe to keep +-- across the rebuild. +CREATE TABLE `_submissions_backup` AS SELECT * FROM `extension_submissions`;--> statement-breakpoint --- Existing 1970 values stay. They are wrong, but they are the only record --- those rows have, and a timestamp invented here would look real without --- being so. -INSERT INTO `__new_developers` ( - id, type, name, url, owner_user_id, approved_at, created_at, updated_at, - avatar_url, contact_email, ownership_epoch, content_revision, - approved_revision, approved_by, github_org_verified, - github_verification_note, github_verified_at, github_url_verified, - url_check_cooldown_until -) -SELECT - id, type, name, url, owner_user_id, approved_at, created_at, updated_at, - avatar_url, contact_email, ownership_epoch, content_revision, - approved_revision, approved_by, github_org_verified, - github_verification_note, github_verified_at, github_url_verified, - url_check_cooldown_until -FROM `developers`;--> statement-breakpoint - -DROP TABLE `developers`;--> statement-breakpoint -ALTER TABLE `__new_developers` RENAME TO `developers`;--> statement-breakpoint - -CREATE UNIQUE INDEX `idx_developers_owner_unique` ON `developers` (`owner_user_id`);--> statement-breakpoint -CREATE INDEX `idx_developers_approved` ON `developers` (`approved_at`);--> statement-breakpoint +DROP TABLE `extension_submissions`;--> statement-breakpoint CREATE TABLE `__new_extensions` ( `id` text PRIMARY KEY NOT NULL, @@ -216,7 +193,7 @@ SELECT target.target_id, ( SELECT s.developer_id - FROM extension_submissions s + FROM _submissions_backup s WHERE LOWER(COALESCE(s.extension_id, json_extract(s.payload, '$.extension.id'))) = target.target_id ORDER BY s.created_at DESC, s.id DESC LIMIT 1 @@ -227,7 +204,7 @@ SELECT FROM ( SELECT DISTINCT LOWER(COALESCE(extension_id, json_extract(payload, '$.extension.id'))) AS target_id - FROM extension_submissions + FROM _submissions_backup ) AS target WHERE target.target_id IS NOT NULL AND NOT EXISTS ( @@ -288,7 +265,7 @@ SELECT s.created_at, s.reviewed_at, s.ownership_epoch -FROM extension_submissions s +FROM _submissions_backup s JOIN extensions e ON LOWER(e.id) = LOWER(COALESCE(s.extension_id, json_extract(s.payload, '$.extension.id'))) -- A payload without an extension object cannot become a revision. This has @@ -327,15 +304,12 @@ CREATE INDEX `idx_extension_revisions_extension_page` ON `extension_revisions` ( CREATE INDEX `idx_extension_revisions_submitter_page` ON `extension_revisions` (`submitted_by`,"created_at" desc,"id" desc);--> statement-breakpoint CREATE INDEX `idx_extension_revisions_queue_page` ON `extension_revisions` (`status`,`created_at`,`id`);--> statement-breakpoint -DROP TABLE `extension_submissions`;--> statement-breakpoint +DROP TABLE `_submissions_backup`;--> statement-breakpoint --- The rebuilds above run with foreign_keys=OFF, which means SQLite does not --- re-validate the copied rows against the new declarations - a pre-existing --- extension pointing at a developer that no longer exists would be carried --- through silently, and every read would then have to defend against it --- forever. Fail the deploy instead, and let the reads assume the join always --- matches. Same CHECK-on-a-scratch-table trick as migration 0020, for the --- same reason: SQLite has no RAISE() outside a trigger. +-- Deferred foreign keys are checked when the transaction commits, which will +-- catch a dangling reference - but as an unattributed constraint failure at +-- the very end. Check explicitly first so the failure names what is wrong, +-- and so the reads below can assume the join always matches. CREATE TABLE _unresolved_references ( kind TEXT NOT NULL, row_id TEXT NOT NULL, @@ -350,5 +324,3 @@ SELECT 'revision.extension_id', r.id FROM extension_revisions r WHERE NOT EXISTS (SELECT 1 FROM extensions e WHERE e.id = r.extension_id);--> statement-breakpoint DROP TABLE _unresolved_references;--> statement-breakpoint - -PRAGMA foreign_keys=ON; diff --git a/src/services/extensions/v2/db/migrations/meta/0021_snapshot.json b/src/services/extensions/v2/db/migrations/meta/0021_snapshot.json index 809b481..cf8531d 100644 --- a/src/services/extensions/v2/db/migrations/meta/0021_snapshot.json +++ b/src/services/extensions/v2/db/migrations/meta/0021_snapshot.json @@ -394,7 +394,7 @@ "primaryKey": false, "notNull": true, "autoincrement": false, - "default": "CURRENT_TIMESTAMP" + "default": "'1970-01-01T00:00:00.000Z'" }, "updated_at": { "name": "updated_at", @@ -402,7 +402,7 @@ "primaryKey": false, "notNull": true, "autoincrement": false, - "default": "CURRENT_TIMESTAMP" + "default": "'1970-01-01T00:00:00.000Z'" }, "avatar_url": { "name": "avatar_url", diff --git a/src/services/extensions/v2/db/schema.ts b/src/services/extensions/v2/db/schema.ts index 8a7aff0..6db78f5 100644 --- a/src/services/extensions/v2/db/schema.ts +++ b/src/services/extensions/v2/db/schema.ts @@ -125,16 +125,22 @@ export const developers = sqliteTable( url: text("url"), ownerUserId: text("owner_user_id").references(() => users.id), approvedAt: text("approved_at"), - // Migration 0002 could only give these a constant default (SQLite rejects - // non-constant ALTER TABLE ADD COLUMN defaults), so they carried a - // placeholder 1970 epoch that no write ever produced. 0021 rebuilds the - // table and replaces it with the value every writer already uses. - createdAt: text("created_at") - .notNull() - .default(sql`CURRENT_TIMESTAMP`), - updatedAt: text("updated_at") - .notNull() - .default(sql`CURRENT_TIMESTAMP`), + // Placeholder default from migration 0002 (SQLite rejects non-constant + // ALTER TABLE ADD COLUMN defaults). Every write sets this explicitly (see + // db/developer-profiles.ts) - the literal default is never actually read, + // but it is part of the real column definition, so it is kept here for + // baseline-diff fidelity against the existing database. + // + // Replacing it needs a table rebuild, and this table cannot be rebuilt on + // D1: developer_claims, developer_transfers and extensions all reference + // it, D1 does not allow foreign keys to be switched off, and deferring + // them is not equivalent - DROP TABLE on a parent increments SQLite's + // deferred-violation counter for every child row, renaming the replacement + // into place never decrements it, and COMMIT then fails even though the + // data is consistent. Doing it anyway would mean rebuilding all three + // children too, which is a lot of risk for a default nothing reads. + createdAt: text("created_at").notNull().default("1970-01-01T00:00:00.000Z"), + updatedAt: text("updated_at").notNull().default("1970-01-01T00:00:00.000Z"), avatarUrl: text("avatar_url"), contactEmail: text("contact_email"), ownershipEpoch: integer("ownership_epoch").notNull().default(1), diff --git a/test/services/extensions/v2/migrations.test.ts b/test/services/extensions/v2/migrations.test.ts index 6089982..cc586d7 100644 --- a/test/services/extensions/v2/migrations.test.ts +++ b/test/services/extensions/v2/migrations.test.ts @@ -76,6 +76,35 @@ function seedSubmissionFixture(db: DatabaseSync): void { ); } +// Applies the whole chain the way D1 does, which is not how the other tests +// here run it. Two differences matter and the second one cost a failed +// production deploy: D1 keeps foreign keys enabled and silently ignores an +// attempt to turn them off, and wrangler wraps each migration file in a single +// transaction. Under those conditions a migration can pass every statement, +// leave PRAGMA foreign_key_check clean, and still fail at COMMIT - DROP TABLE +// on a parent increments SQLite's deferred-violation counter once per child +// row and nothing ever decrements it. +function applyAllAsD1( + db: DatabaseSync, + seed?: (db: DatabaseSync) => void +): void { + db.exec("PRAGMA foreign_keys = ON;"); + for (const name of migrationNames.filter( + (candidate) => !candidate.startsWith("0021") + )) { + db.exec(migration(name)); + } + seed?.(db); + db.exec("BEGIN"); + try { + db.exec(migration("0021_restructure_extensions_revisions.sql")); + db.exec("COMMIT"); + } catch (error) { + db.exec("ROLLBACK"); + throw error; + } +} + describe("Extensions D1 migrations", () => { it("upgrades the split-owned schema without losing users or domain references", () => { const db = new DatabaseSync(":memory:"); @@ -226,25 +255,6 @@ describe("Extensions D1 migrations", () => { .get("legacy-history") ).toEqual({ changed_by: "legacy-user" }); expect(db.prepare("PRAGMA foreign_key_check").all()).toEqual([]); - - // 0021 rebuilds developers only to replace the placeholder 1970 default - // migration 0002 was forced to use. Rows keep whatever they had - a - // wrong-but-real timestamp beats one invented here - while a new insert - // that omits the column now gets the value every writer already uses. - expect( - db - .prepare("SELECT created_at FROM developers WHERE id = ?") - .get("legacy-developer") - ).toEqual({ created_at: "1970-01-01T00:00:00.000Z" }); - - db.prepare( - "INSERT INTO developers (id, type, name, owner_user_id) VALUES (?,?,?,?)" - ).run("post-migration", "user", "After", null); - const fresh = db - .prepare("SELECT created_at, updated_at FROM developers WHERE id = ?") - .get("post-migration") as { created_at: string; updated_at: string }; - expect(fresh.created_at).toMatch(/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/); - expect(fresh.updated_at).toBe(fresh.created_at); } finally { db.close(); } @@ -300,6 +310,34 @@ describe("Extensions D1 migrations", () => { } }); + // Regression guard for the failed remote apply of 0021: it worked locally, + // where PRAGMA foreign_keys=OFF is honoured, and failed on D1, where it is + // not. Everything else in this file runs statements outside a transaction + // with foreign keys off, which cannot see the difference. + it("applies on D1's terms: foreign keys on, one transaction", () => { + const db = new DatabaseSync(":memory:"); + + try { + applyAllAsD1(db, seedSubmissionFixture); + + // Committed, so the deferred counter reached zero and the data is sound. + expect(db.prepare("PRAGMA foreign_key_check").all()).toEqual([]); + expect(db.prepare("SELECT COUNT(*) AS n FROM extensions").get()).toEqual({ + n: 1 + }); + // The holding table used to carry submissions across the rebuild is gone. + expect( + db + .prepare( + "SELECT COUNT(*) AS n FROM sqlite_master WHERE name LIKE '\\_%' ESCAPE '\\'" + ) + .get() + ).toEqual({ n: 0 }); + } finally { + db.close(); + } + }); + // The pre-0021 flow could leave a submission naming a developer that does // not exist. Such a row is already unapprovable - the old approve() only // ever UPDATEd a developer - but it must not vanish without saying so. From b3ca74ebc43335ed50cf45b10cba1018f32646c5 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Sat, 8 Aug 2026 16:03:57 +0100 Subject: [PATCH 2/3] Check references before the copy, not after Follow-up to the ordering fix. The reference check ran at the end of the file, where it could never fire: with foreign keys enforced the rebuilt table's own constraint rejects a dangling row during the copy, so the failure arrived as a bare "FOREIGN KEY constraint failed" and the named check was dead code. It only ever fired in tests, which ran with foreign keys off. Moved to the front with the other pre-flight checks, so it fires before anything is copied and names what is wrong. Its second half is dropped: extension_revisions.extension_id comes from a join against extensions and cannot dangle by construction. The test now runs this case through applyAllAsD1() rather than with foreign keys off, so it exercises the path that actually runs. Also trimmed the comments this change added - the pragma explanation, the holding-table note, the developers-default note and applyAllAsD1's header were all saying the same thing more than once. --- .../0021_restructure_extensions_revisions.sql | 76 +++++++------------ src/services/extensions/v2/db/schema.ts | 12 +-- .../services/extensions/v2/migrations.test.ts | 74 +++++++++--------- 3 files changed, 68 insertions(+), 94 deletions(-) diff --git a/src/services/extensions/v2/db/migrations/0021_restructure_extensions_revisions.sql b/src/services/extensions/v2/db/migrations/0021_restructure_extensions_revisions.sql index e6d8570..301d2f9 100644 --- a/src/services/extensions/v2/db/migrations/0021_restructure_extensions_revisions.sql +++ b/src/services/extensions/v2/db/migrations/0021_restructure_extensions_revisions.sql @@ -16,31 +16,17 @@ -- in meta/0021_snapshot.json comes from drizzle-kit. The end state is verified -- against schema.ts by test/services/extensions/v2/migrations.test.ts. -- --- This migration relaxes foreign keys nowhere, and cannot. An earlier version --- opened with PRAGMA foreign_keys=OFF, passed locally and failed on the first --- remote apply with a bare "FOREIGN KEY constraint failed". Two reasons, and --- the first is the one that matters: --- --- * PRAGMA foreign_keys is a documented no-op inside a transaction, and --- wrangler wraps each migration file in one. The pragma silently did --- nothing remotely. The tests ran statements outside a transaction, where --- it works, which is exactly why they could not see the difference. --- * PRAGMA defer_foreign_keys is not a substitute. DROP TABLE on a parent --- performs an implicit DELETE FROM that increments SQLite's deferred --- violation counter once per child row, and renaming a replacement into --- place never decrements it - so COMMIT fails even though --- PRAGMA foreign_key_check reports nothing wrong. --- --- So the ordering below is load-bearing: nothing here ever drops a table that --- still has children. extension_submissions is copied aside and dropped first, --- which leaves extensions childless at the moment it is replaced, and --- extension_revisions is created only afterwards. Verified by --- "applies on D1's terms" in migrations.test.ts, which runs the whole chain --- with foreign keys enforced and 0021 inside a transaction. --- --- The same constraint is why developers is not rebuilt here: three tables --- reference it, so it can never be dropped this way. See the note on its --- created_at default in schema.ts. +-- The ordering below is load-bearing: nothing here drops a table that still +-- has children, which is why extension_submissions is copied aside and dropped +-- before extensions is rebuilt. Neither pragma can buy you out of this. +-- foreign_keys=OFF is a no-op inside a transaction, and wrangler wraps each +-- migration file in one - an earlier version of this file opened with it, +-- passed locally where statements run outside a transaction, and failed the +-- first remote apply. defer_foreign_keys is not a substitute either: DROP +-- TABLE on a parent increments SQLite's deferred-violation counter once per +-- child row and nothing ever decrements it, so COMMIT fails even when +-- foreign_key_check is clean. It is also why developers is not rebuilt here - +-- three tables reference it. See migrations.test.ts's applyAllAsD1(). -- idx_extensions_id_nocase, created further down, is the constraint that stops -- a new lowercase id colliding with an adopted mixed-case one. A catalogue @@ -71,6 +57,22 @@ GROUP BY LOWER(id) HAVING COUNT(*) > 1;--> statement-breakpoint DROP TABLE _extension_id_case_conflicts;--> statement-breakpoint +-- A dangling developer reference would be caught by the real foreign key on +-- the rebuilt table, but as a bare "FOREIGN KEY constraint failed" from the +-- middle of the copy. Check it here, before anything is copied, so the failure +-- names what is wrong and points at the rows. +CREATE TABLE _unresolved_references ( + kind TEXT NOT NULL, + row_id TEXT NOT NULL, + CONSTRAINT extension_references_must_resolve CHECK (1 = 0) +);--> statement-breakpoint + +INSERT INTO _unresolved_references (kind, row_id) +SELECT 'extension.developer_id', e.id FROM extensions e +WHERE NOT EXISTS (SELECT 1 FROM developers d WHERE d.id = e.author_id);--> statement-breakpoint + +DROP TABLE _unresolved_references;--> statement-breakpoint + -- A submission naming a developer that does not exist cannot become an -- extension row: developer_id is NOT NULL with a foreign key. Such a -- submission is already unapprovable today - the pre-0021 approve() only ever @@ -128,9 +130,8 @@ WHERE LOWER(COALESCE(extension_id, json_extract(payload, '$.extension.id'))) DROP TABLE _reserved_submission_targets;--> statement-breakpoint -- extension_submissions is the only table referencing extensions, so it goes --- first. CREATE TABLE ... AS SELECT copies the rows without carrying any --- constraints across, which is what makes this holding table safe to keep --- across the rebuild. +-- first. AS SELECT rather than a declared table: it carries no constraints +-- across, so the holding table survives the rebuild it spans. CREATE TABLE `_submissions_backup` AS SELECT * FROM `extension_submissions`;--> statement-breakpoint DROP TABLE `extension_submissions`;--> statement-breakpoint @@ -305,22 +306,3 @@ CREATE INDEX `idx_extension_revisions_submitter_page` ON `extension_revisions` ( CREATE INDEX `idx_extension_revisions_queue_page` ON `extension_revisions` (`status`,`created_at`,`id`);--> statement-breakpoint DROP TABLE `_submissions_backup`;--> statement-breakpoint - --- Deferred foreign keys are checked when the transaction commits, which will --- catch a dangling reference - but as an unattributed constraint failure at --- the very end. Check explicitly first so the failure names what is wrong, --- and so the reads below can assume the join always matches. -CREATE TABLE _unresolved_references ( - kind TEXT NOT NULL, - row_id TEXT NOT NULL, - CONSTRAINT extension_references_must_resolve CHECK (1 = 0) -);--> statement-breakpoint - -INSERT INTO _unresolved_references (kind, row_id) -SELECT 'extension.developer_id', e.id FROM extensions e -WHERE NOT EXISTS (SELECT 1 FROM developers d WHERE d.id = e.developer_id) -UNION ALL -SELECT 'revision.extension_id', r.id FROM extension_revisions r -WHERE NOT EXISTS (SELECT 1 FROM extensions e WHERE e.id = r.extension_id);--> statement-breakpoint - -DROP TABLE _unresolved_references;--> statement-breakpoint diff --git a/src/services/extensions/v2/db/schema.ts b/src/services/extensions/v2/db/schema.ts index 6db78f5..27dd1f1 100644 --- a/src/services/extensions/v2/db/schema.ts +++ b/src/services/extensions/v2/db/schema.ts @@ -131,14 +131,10 @@ export const developers = sqliteTable( // but it is part of the real column definition, so it is kept here for // baseline-diff fidelity against the existing database. // - // Replacing it needs a table rebuild, and this table cannot be rebuilt on - // D1: developer_claims, developer_transfers and extensions all reference - // it, D1 does not allow foreign keys to be switched off, and deferring - // them is not equivalent - DROP TABLE on a parent increments SQLite's - // deferred-violation counter for every child row, renaming the replacement - // into place never decrements it, and COMMIT then fails even though the - // data is consistent. Doing it anyway would mean rebuilding all three - // children too, which is a lot of risk for a default nothing reads. + // Replacing it needs a table rebuild, which this table cannot have: three + // tables reference it and a parent cannot be dropped with foreign keys + // enforced (see migration 0021's header). Rebuilding all three children + // too is a lot of risk for a default nothing reads. createdAt: text("created_at").notNull().default("1970-01-01T00:00:00.000Z"), updatedAt: text("updated_at").notNull().default("1970-01-01T00:00:00.000Z"), avatarUrl: text("avatar_url"), diff --git a/test/services/extensions/v2/migrations.test.ts b/test/services/extensions/v2/migrations.test.ts index cc586d7..ceef000 100644 --- a/test/services/extensions/v2/migrations.test.ts +++ b/test/services/extensions/v2/migrations.test.ts @@ -76,14 +76,10 @@ function seedSubmissionFixture(db: DatabaseSync): void { ); } -// Applies the whole chain the way D1 does, which is not how the other tests -// here run it. Two differences matter and the second one cost a failed -// production deploy: D1 keeps foreign keys enabled and silently ignores an -// attempt to turn them off, and wrangler wraps each migration file in a single -// transaction. Under those conditions a migration can pass every statement, -// leave PRAGMA foreign_key_check clean, and still fail at COMMIT - DROP TABLE -// on a parent increments SQLite's deferred-violation counter once per child -// row and nothing ever decrements it. +// Applies the chain the way D1 does, which is not how the other tests here run +// it: foreign keys enforced, and 0021 inside the transaction wrangler wraps +// each migration file in. That combination is what a local apply cannot see, +// and it is what let a broken 0021 reach production. function applyAllAsD1( db: DatabaseSync, seed?: (db: DatabaseSync) => void @@ -548,38 +544,38 @@ describe("Extensions D1 migrations", () => { const db = new DatabaseSync(":memory:"); try { - for (const name of migrationNames.filter( - (candidate) => !candidate.startsWith("0021") - )) { - db.exec(migration(name)); - } - - // Enforcement off, which is exactly how such a row could have come to - // exist before the constraint was there to stop it. - db.exec("PRAGMA foreign_keys = OFF;"); - db.prepare( - `INSERT INTO extensions ( - id, type, author_id, name, description, releases, website, license, - icon_url, readme, source, version, download_url - ) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?)` - ).run( - "dangling", - "mod", - "developer-that-never-existed", - "Dangling", - "d", - "[]", - "https://example.com", - '{"name":"MIT"}', - null, - "# d", - '{"type":"github","repo":"example/d"}', - "1.0.0", - "https://example.com/d.zip" - ); - + // Run on D1's terms: the pre-flight check has to fire before the copy, + // because the rebuilt table's real foreign key would otherwise reject + // the row first with a bare, unattributed error. expect(() => - db.exec(migration("0021_restructure_extensions_revisions.sql")) + applyAllAsD1(db, (seeded) => { + // Enforcement off only while seeding, which is how such a row could + // have come to exist before the constraint was there to stop it. + seeded.exec("PRAGMA foreign_keys = OFF;"); + seeded + .prepare( + `INSERT INTO extensions ( + id, type, author_id, name, description, releases, website, + license, icon_url, readme, source, version, download_url + ) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?)` + ) + .run( + "dangling", + "mod", + "developer-that-never-existed", + "Dangling", + "d", + "[]", + "https://example.com", + '{"name":"MIT"}', + null, + "# d", + '{"type":"github","repo":"example/d"}', + "1.0.0", + "https://example.com/d.zip" + ); + seeded.exec("PRAGMA foreign_keys = ON;"); + }) ).toThrow(/CHECK constraint failed: extension_references_must_resolve/); } finally { db.close(); From f772bc4909949eca729565b65b7fe38e13ddb400 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Sat, 8 Aug 2026 16:11:47 +0100 Subject: [PATCH 3/3] Give the D1 regression test a child row to trip over The test named for the drop-parent-with-children ordering did not actually exercise it. seedSubmissionFixture creates no submissions, so extension_submissions was empty and extensions had no children - and SQLite only raises on DROP TABLE when a child row actually references a deleted one. Reintroducing the old ordering left this test green. The evidence was already in front of me: when I mutation-checked the ordering earlier, three tests failed and this one was not among them. They fail because the SQL references a dropped table, not because of foreign key enforcement, so they would not have caught the production failure either. It now seeds a submission with extension_id set. Null is not enough - extension_id is what makes extensions a parent, and every submission in production happened to be null, which is why the remote apply failed on the developers drop rather than this one. Restoring the old ordering now fails this test. --- .../services/extensions/v2/migrations.test.ts | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/test/services/extensions/v2/migrations.test.ts b/test/services/extensions/v2/migrations.test.ts index ceef000..8884302 100644 --- a/test/services/extensions/v2/migrations.test.ts +++ b/test/services/extensions/v2/migrations.test.ts @@ -314,10 +314,36 @@ describe("Extensions D1 migrations", () => { const db = new DatabaseSync(":memory:"); try { - applyAllAsD1(db, seedSubmissionFixture); + applyAllAsD1(db, (seeded) => { + seedSubmissionFixture(seeded); + // extension_id must be set, not null. It is what makes extensions a + // parent with a child row, and so the only thing that makes dropping + // it a foreign key violation - without it this test passes on the very + // ordering it exists to reject. + seeded + .prepare( + `INSERT INTO extension_submissions + (id, extension_id, developer_id, submitted_by, status, payload, target_key) + VALUES (?,?,?,?,?,?,?)` + ) + .run( + "edit-of-live", + "live-ext", + "acme", + "submitter", + "pending", + '{"developer":{"id":"acme"},"extension":{"id":"live-ext","name":"E"}}', + "live-ext" + ); + }); // Committed, so the deferred counter reached zero and the data is sound. expect(db.prepare("PRAGMA foreign_key_check").all()).toEqual([]); + expect( + db + .prepare("SELECT extension_id FROM extension_revisions WHERE id = ?") + .get("edit-of-live") + ).toEqual({ extension_id: "live-ext" }); expect(db.prepare("SELECT COUNT(*) AS n FROM extensions").get()).toEqual({ n: 1 });