Harden DB migrations with explicit guards + pre-destructive backups#129
Merged
Harden DB migrations with explicit guards + pre-destructive backups#129
Conversation
…ive backups
- Replace try/catch{} silent error swallowing in v1-v3 with explicit
tableExists() guards. Fresh installs (post-v5 schema) skip these
migrations without ever touching SQL; populated old DBs still run them.
Genuine failures now surface instead of being silenced.
- Add backupBeforeDestructive(): VACUUM INTO snapshot to
<dbDir>/backups/spool-pre-v{N+1}-<ts>.db before v5 (drops captures +
connector tables) and v7 (drops stars). Skips in-memory DBs and
empty/no-data DBs (fresh installs need no backup).
- Add comprehensive migration tests covering every starting version
(v0/v3/v4/v6/fresh), the backup helper itself, and an end-to-end
smoke that walks getDB() through each path and exercises the
resulting DB. Test coverage: 79 -> 95.
Why: with v6 + v7 just landed and v7 dropping the stars table, the
inline migration block has accumulated enough surface area that silent
catches and unrecoverable destructive steps are real risks.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Hardens
packages/core/src/db/db.tsmigration runner along three axes:Replace
try { ... } catch {}in v1–v3 with explicittableExists()guards. v1–v3 all operate on tables (connector_sync_state,captures,capture_connectors,captures_fts*) that v5 dropped. The historical try/catch was there so fresh installs on the post-v5 schema wouldn't crash when those tables don't exist. The new guards express that intent explicitly and stop swallowing genuine errors.Add
backupBeforeDestructive(db, fromVersion)and wire it into v5 and v7. Both migrations drop tables irreversibly (v5 dropscaptures+ connector tables; v7 dropsstars). The helper takes aVACUUM INTOsnapshot to<dbDir>/backups/spool-pre-v{N+1}-<ts>.dbbefore each destructive step. Skips in-memory DBs and empty / no-data DBs so fresh-install paths don't write spurious backups.Comprehensive migration tests. Coverage went from 79 → 95 tests. New files:
migration-v1-v3.test.ts— characterizes the v0→head historical path on a populated old schema, plus fresh-install no-op verification.migration-v4.test.ts— exercises the v3→head path, including legacysession_starscleanup.migration-backup.test.ts— unit tests for the helper (file-backed, in-memory, empty DB, version encoding) and integration assertions that v5 actually writes a backup file.migration-smoke.test.ts— end-to-end smoke throughgetDB()simulating fresh install, v0/v4/v6 starting users, and idempotent re-open. Each test inserts a session + pin + queries pins back to prove the migrated DB is functionally usable, not just structurally correct.Why
The migration runner has accumulated 7 versions worth of surface area, with v6 and v7 landing in the past week (project identity, stars→pins). Two specific risks were worth addressing now rather than waiting for a real incident:
user_versionand move on. The guard pattern is both clearer in intent and lets real errors propagate.VACUUM INTOis cheap, atomic at the SQLite level, and skipped on no-data DBs so the cost is zero for fresh installs.A heavier framework migration (Drizzle migrations, Umzug, etc.) was considered and rejected: it would split schema between the fresh-install
CREATE TABLEblock and per-version migration files (drift risk), and add packaging complexity for a single-user local SQLite store. The targeted fixes here address the actual failure modes without buying a framework.How it connects
Touches only
packages/core/src/db/db.tsplus new test files in the same directory. No public API changes beyond exportingbackupBeforeDestructivefor testing. The behavior change for end users is: abackups/directory may appear next tospool.dbafter a major version upgrade, and previously-silent v1–v3 errors would now surface (they have not surfaced in any reported issue, so this is purely a defensive change).Test plan
pnpm -C packages/core test→ 95/95 pass (was 79)pnpm -C packages/core build→ clean typecheckpnpm -C packages/app build→ clean typecheck