Skip to content

feat(migrate): shared migrator package + consolidate server schema into migrations#26

Merged
khaliqgant merged 2 commits intomainfrom
feat/relayauth-migrate-package
Apr 23, 2026
Merged

feat(migrate): shared migrator package + consolidate server schema into migrations#26
khaliqgant merged 2 commits intomainfrom
feat/relayauth-migrate-package

Conversation

@kjgbot
Copy link
Copy Markdown
Contributor

@kjgbot kjgbot commented Apr 23, 2026

Summary

  • New workspace package @relayauth/migrate — an adapter-agnostic migration runner with a _migrations journal and sha256 checksum drift detection. Ships a createNodeSqliteRunner (node:sqlite / better-sqlite3) and createFsMigrationSource today. The MigrationRunner interface is fully async so the cloud D1 adapter can plug in a custom runner in a follow-up PR.
  • @relayauth/server now applies its SQLite schema via runMigrations at adapter boot. The 13 inline CREATE TABLE IF NOT EXISTS blocks that used to live in packages/server/src/storage/sqlite.ts are consolidated into packages/server/src/db/migrations/0001_local_bootstrap.sql, which becomes the source of truth.
  • Legacy ensureTokensSchema / ensureRevokedTokensSchema helpers remain as runtime column-shape guards (ALTER-only fallbacks and a clear error on an ancient column shape) but no longer own any DDL.

Rationale

Having schema live inline in an adapter meant it drifted silently vs the 0001_local_bootstrap.sql file it was supposed to mirror, and there was no journal for evolving beyond CREATE TABLE IF NOT EXISTS. Centralising the migrator into a standalone package lets cloud's D1 adapter reuse the same journal, checksum, and runMigrations contract without importing anything Node-specific.

Follow-up

Cloud-side (D1) adoption — a new runner implementing MigrationRunner backed by the async D1 binding — will ship in a separate PR after this one merges. Nothing in the cloud repo changes in this PR.

Test plan

  • node --test --import tsx packages/migrate/src/__tests__/*.test.ts — 8 tests pass (fresh DB, rerun, new migration added, checksum mismatch, empty directory, nonexistent directory, malformed SQL rollback, non-sql files ignored).
  • node --test --import tsx packages/server/src/__tests__/*.test.ts — 324 tests pass.
  • npx turbo typecheck — 11 tasks succeed.
  • npx turbo build — 9 tasks succeed; dist/db/migrations/0001_local_bootstrap.sql is copied into the built server bundle.
  • grep "CREATE TABLE" packages/server/src/storage/sqlite.ts — no inline DDL remaining.

Open in Devin Review

…to migrations

Introduces @relayauth/migrate, an adapter-agnostic migration orchestrator
backed by a _migrations journal table with sha256 checksum drift detection.
Ships a Node SQLite runner and fs-based migration source today; the
MigrationRunner interface is async/generic so a follow-up PR can plug in a
Cloudflare D1 runner without touching this core.

@relayauth/server now applies its schema via runMigrations at adapter boot.
The 13 inline CREATE TABLE blocks that used to live in storage/sqlite.ts
are consolidated into packages/server/src/db/migrations/0001_local_bootstrap.sql,
which is the new source of truth. ensureTokensSchema / ensureRevokedTokensSchema
remain as runtime column-shape guards, but no longer own any DDL.

Follow-up: cloud's D1 adapter will build on top of this package.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@khaliqgant khaliqgant merged commit 2f7c1f8 into main Apr 23, 2026
2 checks passed
@khaliqgant khaliqgant deleted the feat/relayauth-migrate-package branch April 23, 2026 18:47
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 5 additional findings in Devin Review.

Open in Devin Review

Comment on lines +33 to +34
* rolls back the partial DDL. The `_migrations` insert also participates in
* the transaction, so crashes between exec and recordApplied are impossible.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 recordApplied runs outside the transaction committed by exec, contradicting documented atomicity guarantee

The JSDoc for createNodeSqliteRunner claims "The _migrations insert also participates in the transaction, so crashes between exec and recordApplied are impossible." This is false. The exec method wraps the migration SQL in BEGIN/COMMIT (lines 43-46), and then runMigrations calls recordApplied as a separate step (packages/migrate/src/runner.ts:72) after exec has already committed. The recordApplied method (packages/migrate/src/runners/node-sqlite.ts:73-76) runs a standalone INSERT OR IGNORE with no transaction.

A process crash in the window between exec committing and recordApplied inserting the journal row would leave the migration applied to the database but not recorded in _migrations. On restart, runMigrations would re-execute that migration's SQL. For the current idempotent CREATE TABLE IF NOT EXISTS usage this is benign, but for non-idempotent migrations (which are the common case for a general-purpose framework like @relayauth/migrate), re-execution could fail or cause data corruption. Notably, the MigrationRunner interface doc in packages/migrate/src/types.ts:59-67 correctly acknowledges this crash window exists, directly contradicting this JSDoc.

Suggested change
* rolls back the partial DDL. The `_migrations` insert also participates in
* the transaction, so crashes between exec and recordApplied are impossible.
* Each migration runs inside a `BEGIN`/`COMMIT` transaction so a failure
* rolls back the partial DDL. The journal insert (`recordApplied`) runs
* after the transaction commits; `INSERT OR IGNORE` keeps this idempotent
* so a crash between exec and recordApplied is recoverable by re-running.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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.

2 participants