Conversation
Co-authored-by: ascorbic <ascorbic@users.noreply.github.com>
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-perf-coordinator | 6142678 | Apr 30 2026, 04:44 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-i18n | 6142678 | Apr 30 2026, 04:44 PM |
🦋 Changeset detectedLatest commit: 6142678 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | 6142678 | Apr 30 2026, 04:44 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | 6142678 | Apr 30 2026, 04:45 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 6142678 | Apr 30 2026, 04:45 PM |
|
@ask-bonk fix this |
PR template validation failedPlease fix the following issues by editing your PR description:
See CONTRIBUTING.md for the full contribution policy. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
Co-authored-by: ascorbic <ascorbic@users.noreply.github.com>
|
All green. Summary of the fix: SummaryRoot cause: Kysely's Fix in
Tests in
The fix is conservative — it only suppresses the exact symptom of the race, not all migration errors — and it's a no-op for sequential callers (only the error path engages the new logic). |
There was a problem hiding this comment.
Pull request overview
Addresses #762 by making runMigrations resilient to concurrent Workers isolate startup on D1/SQLite, preventing a benign migration bookkeeping race from aborting initialization and leaving the admin manifest cache empty.
Changes:
- Add a regression test that reproduces the concurrent
runMigrationsrace and asserts the DB ends fully migrated. - Update
runMigrationsto detect the specific_emdash_migrations.nameUNIQUE-constraint race, wait for the other migrator to finish, and then succeed. - Add a patch changeset describing the fix and user-visible symptoms.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/core/tests/integration/database/migration-race.test.ts | New integration test reproducing and guarding against the migration race condition. |
| packages/core/src/database/migrations/runner.ts | Implements race detection + bounded wait to make concurrent migration attempts safe on D1/SQLite. |
| .changeset/fix-migration-race-d1.md | Patch release note describing the fix and the observed admin/UI symptom chain. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async function waitForConcurrentMigrator(db: Kysely<Database>): Promise<boolean> { | ||
| const deadline = Date.now() + MIGRATION_RACE_WAIT_MS; | ||
| while (Date.now() < deadline) { | ||
| const count = await getAppliedMigrationCount(db); | ||
| if (count !== null && count === MIGRATION_COUNT) { | ||
| return true; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, MIGRATION_RACE_POLL_MS)); | ||
| } |
There was a problem hiding this comment.
waitForConcurrentMigrator polls SELECT COUNT(*) every 100ms for up to 10s. In a real “many isolates on deploy” scenario, multiple losing isolates could generate a burst of D1 queries while waiting. Consider using exponential backoff (+ jitter) and/or a larger initial poll interval to reduce query pressure while still converging quickly.
| async function getAppliedMigrationCount(db: Kysely<Database>): Promise<number | null> { | ||
| try { | ||
| const result = await sql<{ count: number }>` | ||
| SELECT COUNT(*) as count FROM ${sql.ref(MIGRATION_TABLE)} | ||
| `.execute(db); | ||
| return Number(result.rows[0]?.count ?? 0); | ||
| } catch { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
getAppliedMigrationCount currently catches all errors and returns null. That’s fine for the “table missing” fast-path, but when used by waitForConcurrentMigrator it can also mask unexpected failures (e.g. permission/connection errors) and turn them into a 10s wait + fallback behavior. Consider only swallowing the specific “no such table” case (by matching the driver error) and rethrowing otherwise.
| const MIGRATION_RACE_PATTERN = /UNIQUE constraint failed: _emdash_migrations\.name/i; | ||
|
|
There was a problem hiding this comment.
MIGRATION_RACE_PATTERN hardcodes the migration table name (_emdash_migrations). Since the table name is already defined as MIGRATION_TABLE, consider building the regex from that constant (with proper escaping) to avoid the detector silently drifting if the table name ever changes.
| const MIGRATION_RACE_PATTERN = /UNIQUE constraint failed: _emdash_migrations\.name/i; | |
| function escapeRegExp(value: string): string { | |
| return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | |
| } | |
| const MIGRATION_RACE_PATTERN = new RegExp( | |
| `UNIQUE constraint failed: ${escapeRegExp(MIGRATION_TABLE)}\\.name`, | |
| "i", | |
| ); |
|
Good call. Tightened That was the right concern: the old Change in
Existing tests still pass:
I considered the other two Copilot suggestions on this review while I was in here:
|
Co-authored-by: ascorbic <ascorbic@users.noreply.github.com>
|
Comment posted. Summary of what I did: Changed
Why it matters: The old The existing two integration tests still pass (verified locally). Commit + push are handled by the automation. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const MIGRATION_TABLE_MISSING_PATTERN = new RegExp( | ||
| `no such table:\\s*${MIGRATION_TABLE.replace(/[.*+?^${}()|[\\]\\\\]/g, "\\$&")}\\b`, | ||
| "i", |
There was a problem hiding this comment.
MIGRATION_TABLE.replace(...) is using a regex escape character class that differs from the repo’s standard escape pattern (/[.*+?^${}()|[\]\\]/g is used elsewhere, e.g. packages/core/src/database/repositories/content.ts:86). Please reuse the shared escape pattern (or a small escapeRegExp helper) here so MIGRATION_TABLE_MISSING_PATTERN is reliably constructed and consistent across the codebase.
| * Resolves to `true` if the migration table reached `MIGRATION_COUNT` rows | ||
| * within the timeout, `false` otherwise. | ||
| */ | ||
| async function waitForConcurrentMigrator(db: Kysely<Database>): Promise<boolean> { | ||
| const deadline = Date.now() + MIGRATION_RACE_WAIT_MS; | ||
| while (Date.now() < deadline) { | ||
| const count = await getAppliedMigrationCount(db); | ||
| if (count !== null && count === MIGRATION_COUNT) { | ||
| return true; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, MIGRATION_RACE_POLL_MS)); | ||
| } | ||
| const finalCount = await getAppliedMigrationCount(db); | ||
| return finalCount === MIGRATION_COUNT; |
There was a problem hiding this comment.
waitForConcurrentMigrator treats the migration as “done” only when count === MIGRATION_COUNT. If the database ever contains more rows than the current MIGRATION_COUNT (e.g. a brief old/new isolate overlap during deploys, or a rollback to an older build after a newer one added migrations), this will never settle and will incorrectly time out. Consider using count >= MIGRATION_COUNT (and similarly for the final check) so “at least all known migrations are applied” is treated as success.
| * Resolves to `true` if the migration table reached `MIGRATION_COUNT` rows | |
| * within the timeout, `false` otherwise. | |
| */ | |
| async function waitForConcurrentMigrator(db: Kysely<Database>): Promise<boolean> { | |
| const deadline = Date.now() + MIGRATION_RACE_WAIT_MS; | |
| while (Date.now() < deadline) { | |
| const count = await getAppliedMigrationCount(db); | |
| if (count !== null && count === MIGRATION_COUNT) { | |
| return true; | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, MIGRATION_RACE_POLL_MS)); | |
| } | |
| const finalCount = await getAppliedMigrationCount(db); | |
| return finalCount === MIGRATION_COUNT; | |
| * Resolves to `true` if the migration table reached at least | |
| * `MIGRATION_COUNT` rows within the timeout, `false` otherwise. | |
| */ | |
| async function waitForConcurrentMigrator(db: Kysely<Database>): Promise<boolean> { | |
| const deadline = Date.now() + MIGRATION_RACE_WAIT_MS; | |
| while (Date.now() < deadline) { | |
| const count = await getAppliedMigrationCount(db); | |
| if (count !== null && count >= MIGRATION_COUNT) { | |
| return true; | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, MIGRATION_RACE_POLL_MS)); | |
| } | |
| const finalCount = await getAppliedMigrationCount(db); | |
| return finalCount !== null && finalCount >= MIGRATION_COUNT; |
| // Table doesn't exist yet (first run). Fall through to the Migrator | ||
| // which will create it. | ||
| const initialCount = await getAppliedMigrationCount(db); | ||
| if (initialCount === MIGRATION_COUNT) { |
There was a problem hiding this comment.
The fast-path check uses initialCount === MIGRATION_COUNT. If the DB has more applied migrations than this build knows about (e.g. after downgrading/rolling back, or overlapping isolates running different versions), this will fall through to the migrator unnecessarily and can interact badly with the race-wait logic. Using >= MIGRATION_COUNT here would make the fast-path robust to “extra” migration rows.
| if (initialCount === MIGRATION_COUNT) { | |
| if (initialCount >= MIGRATION_COUNT) { |
| const MIGRATION_RACE_PATTERN = /UNIQUE constraint failed: _emdash_migrations\.name/i; | ||
|
|
There was a problem hiding this comment.
MIGRATION_RACE_PATTERN hard-codes _emdash_migrations instead of being derived from MIGRATION_TABLE. If either the migration table name or column name ever changes, the race-detection logic will silently stop working. Consider building this regex from MIGRATION_TABLE (and escaping it) so it stays in sync with the migrator configuration.
|
@ask-bonk see the new reviews |
… for migration count
|
Addressed all four points from the new Copilot review. Changes in
New test in All three migration-race tests pass locally:
Typecheck and lint are clean for the touched files. Commit + push handled by the automation. |
|
Done. Summary of what I addressed from the new Copilot review on PR #857: Fixes applied in
New test Comment posted: #857 (comment) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Pattern used to detect "table does not exist" errors from SQLite-family | ||
| * drivers. better-sqlite3 reports `no such table: ...`; D1 wraps the same | ||
| * message as `D1_ERROR: no such table: ...`. We match on the table name to | ||
| * avoid swallowing unrelated "no such table" errors from queries that touch | ||
| * other tables (defensive — `getAppliedMigrationCount` only references | ||
| * `MIGRATION_TABLE`, but downstream callers may grow). | ||
| */ | ||
| const MIGRATION_TABLE_MISSING_PATTERN = new RegExp( | ||
| `no such table:\\s*${escapeRegExp(MIGRATION_TABLE)}\\b`, |
There was a problem hiding this comment.
MIGRATION_TABLE_MISSING_PATTERN only matches SQLite/D1 "no such table" messages. Since runMigrations is also used with Postgres, a fresh Postgres database will throw on the COUNT query (e.g. "relation ... does not exist") and never reach the Migrator. Use the shared isMissingTableError() helper (optionally also checking for MIGRATION_TABLE in the message), or expand the pattern to include Postgres phrasing.
| * Pattern used to detect "table does not exist" errors from SQLite-family | |
| * drivers. better-sqlite3 reports `no such table: ...`; D1 wraps the same | |
| * message as `D1_ERROR: no such table: ...`. We match on the table name to | |
| * avoid swallowing unrelated "no such table" errors from queries that touch | |
| * other tables (defensive — `getAppliedMigrationCount` only references | |
| * `MIGRATION_TABLE`, but downstream callers may grow). | |
| */ | |
| const MIGRATION_TABLE_MISSING_PATTERN = new RegExp( | |
| `no such table:\\s*${escapeRegExp(MIGRATION_TABLE)}\\b`, | |
| * Pattern used to detect "table does not exist" errors from supported | |
| * drivers. SQLite-family drivers report `no such table: ...`; D1 wraps the | |
| * same message as `D1_ERROR: no such table: ...`. Postgres reports | |
| * `relation "..."" does not exist` (typically with the relation name quoted). | |
| * | |
| * We match on the migration table name to avoid swallowing unrelated missing | |
| * table errors from queries that touch other tables (defensive — | |
| * `getAppliedMigrationCount` only references `MIGRATION_TABLE`, but | |
| * downstream callers may grow). | |
| */ | |
| const MIGRATION_TABLE_MISSING_PATTERN = new RegExp( | |
| `(?:no such table:\\s*${escapeRegExp(MIGRATION_TABLE)}\\b|relation\\s+"?${escapeRegExp(MIGRATION_TABLE)}"?\\s+does not exist\\b)`, |
Postgres reports a missing relation as "relation "foo" does not exist" (or occasionally "table "foo" does not exist"), not the SQLite-family "no such table: foo". Without this fix, a fresh Postgres database would throw out of `getAppliedMigrationCount` on the fast-path probe before the Kysely Migrator ever got a chance to create the migration table, breaking initialization on Postgres entirely. The pattern is still anchored to `MIGRATION_TABLE` to avoid swallowing unrelated missing-table errors, and built via `escapeRegExp` so a table rename cannot silently disable detection. Race detection is unchanged: Kysely's PostgresAdapter uses a real advisory lock (`pg_advisory_xact_lock`), so the concurrent-migration race the rest of this PR addresses can only occur on D1/SQLite.
|
Addressed the Postgres missing-table point from the latest Copilot review. Problem: Fix in
Race detection unchanged. Tests. All 3 race-related tests still pass locally ( Typecheck and lint are clean for the touched file (15 pre-existing diagnostics elsewhere, unchanged from baseline). Commit + push handled by the automation. |
|
Comment posted. Summary of what I addressed from the latest Copilot review on PR #857: Fixed in
Why it matters: Without this, the fast-path probe in Did NOT change Verified the regex covers all expected phrasings (better-sqlite3, D1, Postgres) without matching unrelated tables. All 3 race tests + 14 broader migration tests pass locally. Comment: #857 (comment) |
Reproduced the issue and posted findings: #762 (comment)
Summary:
Root cause: Concurrent Workers isolates race in
runMigrations(packages/core/src/database/migrations/runner.ts:136). The KyselySqliteAdapter.acquireMigrationLockis a no-op, inherited bykysely-d1'sD1Dialectand EmDash'sEmDashD1Dialect. The losing isolate fails onINSERT INTO _emdash_migrationswithUNIQUE constraint failed, throws before reaching the auto-seed atemdash-runtime.ts:1031, and never seeds collections.Why the symptom is "API works, admin UI says not found":
_buildManifestswallows errors (emdash-runtime.ts:1405-1407) and returnscollections: {}if listing fails, then_loadManifestpersists that empty object tooptions['emdash:manifest_cache']. The admin sidebar (Sidebar.tsx:177) and content route guards (router.tsx:393, 504, 878) read exclusively from this cached manifest./api/schema/collectionsreads the DB live with no cache, so it works.Reproduction: Added a failing test at
packages/core/tests/integration/database/migration-race.test.tsthat races tworunMigrationscalls on the same SQLite file and reproduces the exact error string from the report:Migration failed: UNIQUE constraint failed: _emdash_migrations.name (migration: 001_initial).The repro test is committed in this branch. The fix is correct when the test passes.
Closes #762
github run