fix(db): sqlite migrations check real schema instead of schema_migrations records - #111

Walkthrough

SQLite migrations now use real schema inspection instead of migration records alone. The runner applies missing migrations, including on incomplete legacy databases. Tests cover fresh, legacy, poisoned, and repeated migration runs. Repository guidance documents the centralized validation rule.

Changes

SQLite migration schema validation

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🔵 Low · up to a0f44

The migration behavior is otherwise mergeable, with a bounded follow-up needed to align the three async unit tests with the expected test execution pattern; no current evidence indicates a production data or availability risk.

Suggested reviewers: codeerrorsleep

Sequence Diagram(s)

sequenceDiagram
  participant run_migrations
  participant migration_applied
  participant SQLite_schema
  participant migration_sql
  run_migrations->>migration_applied: Check migration version
  migration_applied->>SQLite_schema: Inspect required schema objects
  SQLite_schema-->>migration_applied: Return schema state
  migration_applied-->>run_migrations: Return applied status
  run_migrations->>migration_sql: Execute missing migration SQL
  migration_sql->>SQLite_schema: Create missing tables, columns, or indexes

❌ Failed checks (1 warning)

• X
• Mastodon
• Reddit
• LinkedIn

• Stop trusting schema_migrations for SQLite; detect applied migrations via real schema checks.
• Heal fresh/legacy/poisoned databases by re-running missing migrations idempotently.
• Add regression tests and document the required migration-check pattern.

graph TD
  A([App startup]) --> B["run_migrations()"] --> C{"applied?"} --> D["migration_applied(version)"] --> E[(SQLite schema
sqlite_master / pragma)] --> C
  C -->|no| F["Execute migration SQL"] --> B
  C -->|yes| B

  subgraph Legend
    direction LR
    _proc([Process]) ~~~ _dec{"Decision"} ~~~ _db[(Database)]
  end

Loading

The following are alternative approaches to this PR:

1. Keep schema_migrations but validate schema on mismatch

• ➕ Preserves explicit version history for debugging/ops
• ➕ Avoids per-version schema probes on every startup in the common case
• ➖ More branching logic (records vs schema) and more edge cases
• ➖ Still requires per-migration schema checks as a fallback, so complexity remains

2. Adopt sqlx::migrate! (or equivalent) as the single migration runner

• ➕ Leverages a battle-tested migration engine and conventions
• ➕ May reduce custom logic and future regressions
• ➖ Potentially larger refactor and operational change (file layout, metadata, runtime behavior)
• ➖ May not easily support the current 'self-heal legacy schema' expectations without customization

3. Add a one-time repair migration for affected legacy DBs

• ➕ Smallest change to the migration runner itself
• ➕ Targets the known regression explicitly
• ➖ Does not generalize to future cases where records drift from schema
• ➖ Requires reliably detecting the bad state, which is exactly what record-only tracking failed at

Recommendation: The PR’s approach (schema-as-source-of-truth) is the most robust for SQLite in the presence of drift/poisoned metadata. Keeping schema_migrations as an optimization could be considered later, but only if schema checks remain authoritative to prevent repeating the 2026-08 regression.

Bug fix (1) +153 / -41

migrations.rsReplace migration tracking with real-schema checks and add regression tests +153/-41

Replace migration tracking with real-schema checks and add regression tests

• Removes schema_migrations-based tracking/legacy auto-marking and introduces migration_applied(version) which queries sqlite_master/pragma_table_info to determine whether each migration is effective. Updates run_migrations() to execute only missing migrations and adds three tests covering fresh, legacy-without-records, and poisoned 'all-marked-but-missing-schema' databases.

src-tauri/src/db/migrations.rs

Documentation (1) +5 / -0

AGENTS.mdDocument schema-based SQLite migration verification requirement +5/-0

Document schema-based SQLite migration verification requirement

• Adds guidance that SQLite migrations must be considered applied only when the real schema (tables/columns) exists. Documents the 2026-08 regression and points contributors to centralize checks in migration_applied().

AGENTS.md

Description

migration_applied treats several multi-column migrations as “applied” when only a single column
exists (e.g., versions 4, 11–14), so a partially-migrated/poisoned DB can skip the migration and
still crash later when code queries missing columns. Worse, if some of the later columns already
exist, the migration may run and then fail mid-way on “duplicate column”, leaving the DB in a broken
state.

Code

src-tauri/src/db/migrations.rs[R77-79]

+        12 => column_exists("connections", "mode"),
+        13 => column_exists("connections", "auth_mode"),
+        14 => column_exists("connections", "service_name"),

Evidence

migration_applied uses a single-column existence check for migrations that add multiple columns,
but app code later issues SQL statements referencing those additional columns. If any of those
columns are missing, SQLite will throw no such column at runtime; if some exist and the migration
runs, ALTER TABLE ... ADD COLUMN will fail on duplicates mid-migration.

src-tauri/src/db/migrations.rs[64-83]
src-tauri/migrations/012_add_redis_connection_options.sql[1-4]
src-tauri/migrations/013_add_elasticsearch_connection_options.sql[1-5]
src-tauri/migrations/014_add_sentinel_fields.sql[1-2]
src-tauri/migrations/004_add_ssh_fields.sql[1-6]
src-tauri/migrations/011_add_ssl_fields.sql[1-2]
src-tauri/src/db/local/connections.rs[30-33]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`migration_applied()` currently checks only one “sentinel” column for migrations that actually add multiple columns (e.g. 004, 011–014). This can incorrectly skip migrations on partially-migrated databases, leaving required columns missing and causing runtime SQL errors (e.g. `INSERT INTO connections (...)` references many of these columns). It can also cause a migration to run when *some* columns already exist and then fail mid-migration due to `duplicate column name`.

### Issue Context
Multi-column migrations in this repo include:
- 004 adds `ssh_enabled` plus 5 other `ssh_*` columns
- 011 adds `ssl_mode` and `ssl_ca_cert`
- 012 adds `mode`, `seed_nodes`, `sentinels`, `connect_timeout_ms`
- 013 adds `auth_mode`, `api_key_id`, `api_key_secret`, `api_key_encoded`, `cloud_id`
- 014 adds `service_name`, `sentinel_password`

Production code uses these columns in queries against `connections`, so missing any of them can crash startup/CRUD.

### Fix Focus Areas
- src-tauri/src/db/migrations.rs[50-89]
- src-tauri/migrations/004_add_ssh_fields.sql[1-6]
- src-tauri/migrations/011_add_ssl_fields.sql[1-2]
- src-tauri/migrations/012_add_redis_connection_options.sql[1-4]
- src-tauri/migrations/013_add_elasticsearch_connection_options.sql[1-5]
- src-tauri/migrations/014_add_sentinel_fields.sql[1-2]

### What to change
- Make each migration’s “applied” check verify **all** schema elements that migration adds.
 - Example: v12 should only be considered applied if **all** of `mode`, `seed_nodes`, `sentinels`, `connect_timeout_ms` exist.
 - v13 should check all five columns.
 - v4 should check all `ssh_*` columns.
 - v11 should check both `ssl_mode` and `ssl_ca_cert`.
 - v14 should check both `service_name` and `sentinel_password`.
- Prefer a robust approach that avoids string-building SQL per column:
 - Fetch column names once via `pragma_table_info('connections')` and check membership in Rust.
 - Similarly, for indexes/triggers, query `sqlite_master` once and check required names.
- Add a regression unit test for a “partially applied” DB (e.g., has `mode` but missing `seed_nodes`) and assert `run_migrations()` heals it.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

For several migrations that create both tables and indexes (e.g., 005/006/007/010/016),
migration_applied only checks table existence, so a DB can be treated as fully migrated while
still missing the indexes created by those migrations. That can silently regress query performance
and, for unique indexes, potentially allow unwanted duplicates until later code hits
inconsistencies.

Code

src-tauri/src/db/migrations.rs[R69-71]

+        5 => table_exists("ai_providers"),
+        6 => table_exists("ai_conversations"),
+        7 => table_exists("ai_messages"),

Evidence

The SQL migration files explicitly create indexes, but the new applied-check logic for these
versions only checks sqlite_master for the table. Therefore an index can be missing while the
migration is still considered applied and never re-executed.

src-tauri/src/db/migrations.rs[64-83]
src-tauri/migrations/005_ai_providers.sql[15-17]
src-tauri/migrations/010_sql_execution_logs.sql[1-13]
src-tauri/migrations/016_redis_command_logs.sql[1-12]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Some migrations create indexes in addition to tables, but `migration_applied()` considers the migration applied if the table exists. This can leave DBs missing intended indexes (and possibly triggers), causing performance regressions and potentially integrity regressions for uniqueness constraints.

### Issue Context
Examples:
- 005 creates `ai_providers` plus `idx_ai_providers_default_true`
- 006 creates `ai_conversations` plus `idx_ai_conversations_updated_at`
- 007 creates `ai_messages` plus `idx_ai_messages_conversation_id`
- 010 creates `sql_execution_logs` plus `idx_sql_execution_logs_executed_at`
- 016 creates `redis_command_logs` plus `idx_redis_command_logs_executed_at`

### Fix Focus Areas
- src-tauri/src/db/migrations.rs[64-83]
- src-tauri/migrations/005_ai_providers.sql[1-17]
- src-tauri/migrations/006_ai_conversations.sql[1-12]
- src-tauri/migrations/007_ai_messages.sql[1-16]
- src-tauri/migrations/010_sql_execution_logs.sql[1-13]
- src-tauri/migrations/016_redis_command_logs.sql[1-12]

### What to change
- Extend `migration_applied()` for these versions to verify the presence of their key indexes (and triggers if applicable), not just the table.
- Implementation suggestion:
 - Query `sqlite_master` for `type in ('table','index','trigger')` once per migration/version and validate required object names.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

migration_applied returns Ok(true) for unknown migration versions, so if a new migration is
added to MIGRATIONS but the match-arm isn’t updated, the migration will be silently skipped. That
failure mode reintroduces “missing schema at runtime” issues without any obvious error.

Code

src-tauri/src/db/migrations.rs[82]

+        _ => return Ok(true),

Evidence

The new implementation explicitly returns Ok(true) for any version not handled in the match, which
means the caller will continue and never execute the corresponding migration SQL.

src-tauri/src/db/migrations.rs[64-83]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The default match arm in `migration_applied()` returns `Ok(true)` (treat as already applied). If `MIGRATIONS` is extended and someone forgets to update the schema-check match arms, the new migration will never run and the system can ship with missing schema.

### Issue Context
This PR explicitly relies on developers keeping `migration_applied()` in sync with `MIGRATIONS`.

### Fix Focus Areas
- src-tauri/src/db/migrations.rs[64-83]

### What to change
- Replace `_ => return Ok(true)` with a fail-fast behavior:
 - Prefer: return `Err(AppError::internal(...))` mentioning the missing check branch, OR
 - At least: `_ => return Ok(false)` so new migrations run by default.
- Add a unit test that asserts all versions in `MIGRATIONS` are covered (e.g., by iterating versions and ensuring `migration_applied` doesn’t hit the default arm).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗