Skip to content

Snapshot fabro.sqlite3 before applying new migrations#585

Merged
swerner merged 2 commits into
mainfrom
db-premigration-snapshot
Jul 22, 2026
Merged

Snapshot fabro.sqlite3 before applying new migrations#585
swerner merged 2 commits into
mainfrom
db-premigration-snapshot

Conversation

@swerner

@swerner swerner commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Database::migrate now writes a rollback snapshot to <db>.pre-migration.bak before applying any migration the database has not seen yet.

Why

A binary downgrade after new SQLite migrations have been applied fails sqlx's startup validation outright — migration N was previously applied but is missing in the resolved migrations — before the server reads anything else. Until now there was no rollback artifact for the shared database: the file-based legacy imports rename their sources to .bak, but the database itself had no equivalent, so recovering from a bad upgrade meant hand-deleting _sqlx_migrations rows and dropping tables.

This matters right now because #570/#571/#572/#573 each add new migrations to the shared fabro.sqlite3 (which already holds variables and environments on existing deployments). With this change merged first, the release that ships those migrations automatically leaves every upgraded deployment a one-file rollback path.

Behavior

  • Snapshot is taken only when the database has previously applied migrations and at least one bundled migration is pending — fresh databases and no-op boots skip it, so the file always holds the state from immediately before the most recent schema change.
  • Written with VACUUM INTO from the live pool: a consistent single-file copy, no -wal/-shm siblings needed to restore. Mode 0600.
  • Snapshot failure fails the migration (no rollback artifact → no schema change).
  • Rollback procedure: stop the server, replace fabro.sqlite3 with the snapshot, delete any -wal/-shm files, start the previous binary. Writes made after the upgrade are lost, as with any point-in-time restore.

Testing

  • New: fresh-database migrate takes no snapshot; a simulated upgrade (bundled migration pending on an already-migrated database) snapshots first, preserves pre-migration schema + row data in the snapshot, sets 0600, and leaves the snapshot untouched on subsequent no-op migrates.
  • cargo nextest run -p fabro-db -p fabro-environment -p fabro-variable -p fabro-server: 771/774 pass locally; the 3 failures are the graphviz render tests (no dot on this machine) and fail identically on main.
  • clippy -D warnings and fmt --check clean.

🤖 Generated with Claude Code

A binary downgrade after new SQLite migrations have been applied fails
sqlx's startup validation ("migration was previously applied but is
missing in the resolved migrations") and previously left the operator
with no rollback artifact: the shared database had no backup, so
recovering meant hand-editing _sqlx_migrations and dropping tables.

Database::migrate now writes a consistent single-file snapshot to
<db>.pre-migration.bak (via VACUUM INTO, mode 0600) before applying any
migration the database has not seen. Rollback is: stop the server,
replace the database file with the snapshot, delete -wal/-shm siblings,
start the previous binary. Fresh databases and no-op migrates skip the
snapshot, so the file always preserves the state from immediately before
the most recent schema change. A snapshot failure fails the migration:
no rollback artifact, no schema change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.

Tip: disable this comment in your organization's Code Review settings.

- Detect applied migrations via sqlx's Migrate trait
  (ensure_migrations_table + list_applied_migrations) instead of
  hand-querying the _sqlx_migrations bookkeeping table, so the check
  cannot drift from what Migrator::run actually applies.
- Write the snapshot to a staging file and rename it into place, so a
  failure mid-copy never leaves a partial file at the snapshot path.
- Derive the database path from the pool's connect options instead of
  storing a duplicate copy on Database.
- Drop the invented "fabro.sqlite3" fallback filename from
  pre_migration_snapshot_path; append the suffix to the path directly.
- Deduplicate the snapshot-inspection blocks in the test behind small
  connect_read_only/table_exists helpers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@swerner

swerner commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author

Ran a four-angle cleanup review (reuse, simplification, efficiency, altitude) over this PR and applied the surviving findings in f97ac8d. cargo nextest run -p fabro-db, nightly clippy with -D warnings, and fmt --check are all green.

Applied

  • Use sqlx's own migration bookkeeping (flagged independently by three of the four review angles): applied_migration_versions hand-queried the _sqlx_migrations table, including a sqlite_master existence probe, hardcoding a table name sqlx 0.9 treats as semver-exempt and configurable (Migrator::dangerous_set_table_name). It now calls the public sqlx::migrate::Migrate trait (ensure_migrations_table + list_applied_migrations with MIGRATOR.table_name) — the same first steps Migrator::run performs, so pending-detection can't drift from what actually gets applied.
  • Atomic snapshot publish: the delete-then-VACUUM INTO sequence could leave a partial file at the snapshot path — the one file an operator restores during a failed upgrade. The copy now goes to a .tmp staging file, gets its 0o600 permissions, and is renamed into place, so the final path only ever holds a complete snapshot (and never briefly exists world-readable). This also matches the atomic-backup-write guidance in docs/internal/migrations-strategy.md.
  • Dropped derivable state: the new path: PathBuf field on Database duplicated what the pool already knows; the snapshot code now reads pool.connect_options().get_filename().
  • Removed the invented fallback filename: pre_migration_snapshot_path no longer defaults to a hardcoded "fabro.sqlite3" for pathless inputs (dead policy leaking a product name owned by fabro-config); it just appends the suffix to the path.
  • Smaller cleanups: merged the two early-return guards into one condition, replaced the three-arm remove_file match with a remove_file_if_exists helper, fixed the error context that mislabeled the snapshot path as "database path", and deduplicated the copy-pasted "open snapshot read-only and count tables" blocks in the test behind connect_read_only/table_exists helpers.

Noted but not changed

  • set_private_permissions is now the workspace's fifth identical private copy of the 0o600 helper, but no importable original exists — consolidating into fabro-util would drag its heavy deps into the lean fabro-db crate, so that's a separate change if we want it.
  • The UTF-8-path requirement for VACUUM INTO stands: SQLite takes the target as a text value, so there's no clean way to support non-UTF-8 paths; the staging file inherits the database's directory either way.
  • Efficiency checked out as-is: the expensive VACUUM INTO only runs when a migration is genuinely pending, and steady-state startup cost is two trivial local queries.
  • This PR quietly establishes a backup policy class that docs/internal/migrations-strategy.md doesn't cover (it scopes itself to non-SQL file migrations). The doc comment on snapshot_before_new_migrations documents the policy; a paragraph in the strategy doc could follow separately.

🤖 Generated with Claude Code

@swerner
swerner merged commit 70370fd into main Jul 22, 2026
14 checks passed
@swerner
swerner deleted the db-premigration-snapshot branch July 22, 2026 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant