From c5ee817ebb3158170e11fd0418afbc4d6050b118 Mon Sep 17 00:00:00 2001 From: kaivalya Date: Thu, 9 Jul 2026 15:14:08 +0200 Subject: [PATCH] F3: DB restore keeps the live schema_version stamp Restoring a backup copied the backup's schema_version into the live DB. Once SCHEMA_VERSION moves past 1, restoring an older backup (already adapted to the current schema by the migration scripts) would leave an old stamp, so the next launch re-runs migrations against an already-adapted schema and blocks the app. schema_version is now excluded from the restore copy; the live stamp is correct by construction. Regression test tampers a backup's stamp and asserts the live one survives. Co-Authored-By: Claude Fable 5 --- docs/decisions-log.md | 6 +++++ src/db/releases.js | 9 +++++++ src/db/worker.js | 11 ++++++-- tests/test_step2_settings.py | 52 ++++++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+), 2 deletions(-) diff --git a/docs/decisions-log.md b/docs/decisions-log.md index 5a51a5d..3a8d195 100644 --- a/docs/decisions-log.md +++ b/docs/decisions-log.md @@ -2,6 +2,12 @@ Record of architectural and technical decisions made during development. +## 2026-07-09 — DB restore no longer copies the backup's schema_version stamp (code review F3) + +**Decision:** `handleImportDb` in `src/db/worker.js` now excludes `schema_version` from the tables copied out of a restored backup. The live DB keeps its own version stamp, which is correct by construction: any adapt/migration scripts run against the temp DB *before* the copy, so the restored data always matches the current schema. + +**Rationale:** This was a latent time bomb, dormant only because `SCHEMA_VERSION` is still 1. Once a v2 migration ships, restoring a v1 backup would have copied the old stamp into the live DB; the next launch would read migration-pending and re-run the migration against an already-adapted schema (e.g. a duplicate `ALTER TABLE ADD COLUMN` fails and strands the user on the migration-failed screen). Excluding the table was chosen over re-stamping after restore because it keeps the restore loop generic and adds no write. A regression test simulates an older stamp (version 0) in a backup and asserts the live stamp survives the restore; a comment in `src/db/releases.js` requires the first real v2 migration to ship a full end-to-end older-version restore test. + ## 2026-07-09 — Same-user re-import skips existing execution logs (code review F1) **Decision:** In `useJsonImport.js`, execution logs whose id already exists locally (`sameUserCollisions`) are now counted as skipped instead of being re-inserted with their original id. Previously each one hit a PRIMARY KEY violation whose message ended up in the import report's error list, making a routine "re-import my own full export" look like a partially failed import. diff --git a/src/db/releases.js b/src/db/releases.js index f5f01c8..40abbd8 100644 --- a/src/db/releases.js +++ b/src/db/releases.js @@ -1,3 +1,12 @@ +// Interaction with DB restore (src/db/worker.js, handleImportDb): when a backup +// is restored, migration scripts in range run against the *temp* DB before its +// tables are copied into the live DB, and the schema_version table is NOT +// copied — the live DB keeps its current-version stamp. If it were copied, the +// next launch would see the backup's old stamp and re-run migrations against an +// already-adapted schema (e.g. a duplicate ALTER TABLE ADD COLUMN would fail +// and block the app behind the migration-failed screen). The first release that +// bumps schemaVersion past 1 must ship an end-to-end test restoring an +// older-version backup. export const RELEASES = [ { schemaVersion: 1, diff --git a/src/db/worker.js b/src/db/worker.js index f62b298..837e996 100644 --- a/src/db/worker.js +++ b/src/db/worker.js @@ -246,8 +246,15 @@ function handleImportDb(bytes) { ) const currentTableNames = currentTableRows.map((r) => r[0]) - // Only restore tables that exist in both databases - const tablesToRestore = importedTableNames.filter((t) => currentTableNames.includes(t)) + // Only restore tables that exist in both databases. + // schema_version is deliberately excluded: after the adapt scripts ran on the + // temp DB (step 5), its data matches the *current* schema, so the live DB's + // own version stamp is already correct. Copying the backup's (older) stamp + // would make the next launch re-run migrations against an already-adapted + // schema (see src/db/releases.js). + const tablesToRestore = importedTableNames.filter( + (t) => currentTableNames.includes(t) && t !== 'schema_version', + ) // 7. Table-by-table copy inside a single transaction so a mid-restore // failure cannot leave the live DB partially wiped. diff --git a/tests/test_step2_settings.py b/tests/test_step2_settings.py index 01f5478..b252bf9 100644 --- a/tests/test_step2_settings.py +++ b/tests/test_step2_settings.py @@ -14,6 +14,7 @@ Step 2 — Settings Tests - test_restore_db_button: Restore DB button is visible and enabled - test_restore_db_cancel_dialog: Declining confirm dialog aborts the restore - test_restore_db_flow: Full restore round-trip with spinner and report +- test_restore_db_keeps_current_schema_version: Restore keeps the live DB's schema_version stamp - test_restore_db_invalid_file: Uploading a non-SQLite file shows error report - test_restore_db_incomplete_db: Uploading a DB missing required tables shows error - test_export_import_enabled: Export/Import buttons are present and enabled (wired in Step 3b) @@ -23,6 +24,7 @@ Step 2 — Settings Tests - test_initialize_db_flow: Full initialize flow resets database and reloads page """ import os +import sqlite3 import tempfile from playwright.sync_api import Page, expect @@ -461,6 +463,56 @@ def test_restore_db_flow(page: Page, app_url: str, tmp_path_factory): expect(report).not_to_be_visible() +def test_restore_db_keeps_current_schema_version(page: Page, app_url: str, tmp_path_factory): + """After a restore, the live DB keeps its current schema_version stamp + instead of inheriting the backup's (F3). A backup stamped with an older + version must not leave the live DB looking migration-pending, or the next + launch would re-run migrations against an already-adapted schema. + + Note: while only schema v1 exists this can only simulate an older stamp; + the first real v2 migration must ship a full end-to-end restore test of an + actual older-version backup. + """ + _go_to_settings(page, app_url) + + current_version = page.evaluate( + "async () => (await window.__db.selectOne(" + "'SELECT MAX(version) AS v FROM schema_version')).v" + ) + + tmp_dir = tmp_path_factory.mktemp("restore_version") + db_path = str(tmp_dir / "old-stamp.sqlite3") + download = _download_via_button(page, '[data-testid="settings-download-db"]') + download.save_as(db_path) + + # Stamp the backup with an older schema version, as if it were taken + # before a (future) migration + conn = sqlite3.connect(db_path) + conn.execute("UPDATE schema_version SET version = 0") + conn.commit() + conn.close() + + page.once('dialog', lambda dialog: dialog.accept()) + with page.expect_file_chooser() as fc_info: + page.locator('[data-testid="settings-restore-db"]').click() + file_chooser = fc_info.value + file_chooser.set_files(db_path) + + report = page.locator('[data-testid="restore-report"]') + expect(report).to_be_visible(timeout=15000) + expect(report.locator('.report-success')).to_be_visible() + page.locator('[data-testid="restore-report-dismiss"]').click() + + restored_version = page.evaluate( + "async () => (await window.__db.selectOne(" + "'SELECT MAX(version) AS v FROM schema_version')).v" + ) + assert restored_version == current_version, ( + f"Live DB schema_version must stay {current_version} after restore, " + f"got {restored_version}" + ) + + def test_restore_db_invalid_file(page: Page, app_url: str): """Uploading a non-SQLite file shows an error report.""" _go_to_settings(page, app_url)