From c50455d5f6dbc52cca46671bb7bafd89daa98e5f Mon Sep 17 00:00:00 2001
From: Padreug <padreug@aiolabs.dev>
Date: Sun, 12 Jul 2026 12:23:04 +0200
Subject: [PATCH] fix(migrations): make all migrations idempotent
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The migration version bump lands in the core dbversions table while
the DDL lands in ext_libra — the two writes are not atomic. A failed
bump re-runs the whole migration on next boot; bare
CREATE/ALTER/INSERT then crashes the extension until manual
dbversions surgery.

- CREATE TABLE / CREATE INDEX -> IF NOT EXISTS
- ALTER TABLE ADD COLUMN -> _alter_add_column_safe (same swallow
  pattern as the events/withdraw fork migrations)
- seed INSERTs (default accounts, virtual parents, default roles) ->
  ON CONFLICT (name) DO NOTHING

Tests run the chain twice against a fresh SQLite DB (full-chain rerun
and per-migration rerun); both fail against the previous migrations.

Addresses CODE-REVIEW-2026-06 findings #3 and #12.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 migrations.py            | 105 +++++++++++++++++++++++--------------
 tests/test_migrations.py | 109 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 176 insertions(+), 38 deletions(-)
 create mode 100644 tests/test_migrations.py

diff --git a/migrations.py b/migrations.py
index 9c38c55..2c39507 100644
--- a/migrations.py
+++ b/migrations.py
@@ -34,9 +34,33 @@ Original migration sequence (Nov 2025):
 - m014: Removed legacy equity accounts (MemberEquity, RetainedEarnings)
 - m015: Converted entry_lines to single amount field
 - m016: Dropped journal_entries and entry_lines tables (Fava integration)
+
+IDEMPOTENCY CONTRACT:
+Every statement here must be a silent no-op on re-run. The migration
+version bump lands in the core LNbits DB (`dbversions`) while the DDL
+lands in `ext_libra` — the two writes are not atomic. If the version
+bump fails after the DDL commits, the whole migration re-runs on next
+boot; a bare CREATE/ALTER/INSERT then crashes the extension until
+manual dbversions surgery.
 """
 
 
+async def _alter_add_column_safe(db, sql: str) -> None:
+    """ALTER TABLE ADD COLUMN that swallows duplicate-column errors.
+
+    Neither SQLite nor Postgres supports ADD COLUMN IF NOT EXISTS
+    portably, so re-runs are made no-ops by swallowing the error both
+    backends raise for an existing column.
+    """
+    try:
+        await db.execute(sql)
+    except Exception as exc:
+        msg = str(exc).lower()
+        if "duplicate column" in msg or "already exists" in msg:
+            return
+        raise
+
+
 async def m001_initial(db):
     """
     Initial Libra database schema (squashed from m001-m016).
@@ -63,7 +87,7 @@ async def m001_initial(db):
 
     await db.execute(
         f"""
-        CREATE TABLE accounts (
+        CREATE TABLE IF NOT EXISTS accounts (
             id TEXT PRIMARY KEY,
             name TEXT NOT NULL UNIQUE,
             account_type TEXT NOT NULL,
@@ -76,13 +100,13 @@ async def m001_initial(db):
 
     await db.execute(
         """
-        CREATE INDEX idx_accounts_user_id ON accounts (user_id);
+        CREATE INDEX IF NOT EXISTS idx_accounts_user_id ON accounts (user_id);
         """
     )
 
     await db.execute(
         """
-        CREATE INDEX idx_accounts_type ON accounts (account_type);
+        CREATE INDEX IF NOT EXISTS idx_accounts_type ON accounts (account_type);
         """
     )
 
@@ -93,7 +117,7 @@ async def m001_initial(db):
 
     await db.execute(
         f"""
-        CREATE TABLE extension_settings (
+        CREATE TABLE IF NOT EXISTS extension_settings (
             id TEXT NOT NULL PRIMARY KEY,
             libra_wallet_id TEXT,
             fava_url TEXT NOT NULL DEFAULT 'http://localhost:3333',
@@ -111,7 +135,7 @@ async def m001_initial(db):
 
     await db.execute(
         f"""
-        CREATE TABLE user_wallet_settings (
+        CREATE TABLE IF NOT EXISTS user_wallet_settings (
             id TEXT NOT NULL PRIMARY KEY,
             user_wallet_id TEXT,
             updated_at TIMESTAMP NOT NULL DEFAULT {db.timestamp_now}
@@ -126,7 +150,7 @@ async def m001_initial(db):
 
     await db.execute(
         f"""
-        CREATE TABLE manual_payment_requests (
+        CREATE TABLE IF NOT EXISTS manual_payment_requests (
             id TEXT PRIMARY KEY,
             user_id TEXT NOT NULL,
             amount INTEGER NOT NULL,
@@ -143,14 +167,14 @@ async def m001_initial(db):
 
     await db.execute(
         """
-        CREATE INDEX idx_manual_payment_requests_user_id
+        CREATE INDEX IF NOT EXISTS idx_manual_payment_requests_user_id
         ON manual_payment_requests (user_id);
         """
     )
 
     await db.execute(
         """
-        CREATE INDEX idx_manual_payment_requests_status
+        CREATE INDEX IF NOT EXISTS idx_manual_payment_requests_status
         ON manual_payment_requests (status);
         """
     )
@@ -163,7 +187,7 @@ async def m001_initial(db):
 
     await db.execute(
         f"""
-        CREATE TABLE balance_assertions (
+        CREATE TABLE IF NOT EXISTS balance_assertions (
             id TEXT PRIMARY KEY,
             date TIMESTAMP NOT NULL,
             account_id TEXT NOT NULL,
@@ -188,21 +212,21 @@ async def m001_initial(db):
 
     await db.execute(
         """
-        CREATE INDEX idx_balance_assertions_account_id
+        CREATE INDEX IF NOT EXISTS idx_balance_assertions_account_id
         ON balance_assertions (account_id);
         """
     )
 
     await db.execute(
         """
-        CREATE INDEX idx_balance_assertions_status
+        CREATE INDEX IF NOT EXISTS idx_balance_assertions_status
         ON balance_assertions (status);
         """
     )
 
     await db.execute(
         """
-        CREATE INDEX idx_balance_assertions_date
+        CREATE INDEX IF NOT EXISTS idx_balance_assertions_date
         ON balance_assertions (date);
         """
     )
@@ -216,7 +240,7 @@ async def m001_initial(db):
 
     await db.execute(
         f"""
-        CREATE TABLE user_equity_status (
+        CREATE TABLE IF NOT EXISTS user_equity_status (
             user_id TEXT PRIMARY KEY,
             is_equity_eligible BOOLEAN NOT NULL DEFAULT FALSE,
             equity_account_name TEXT,
@@ -230,7 +254,7 @@ async def m001_initial(db):
 
     await db.execute(
         """
-        CREATE INDEX idx_user_equity_status_eligible
+        CREATE INDEX IF NOT EXISTS idx_user_equity_status_eligible
         ON user_equity_status (is_equity_eligible)
         WHERE is_equity_eligible = TRUE;
         """
@@ -245,7 +269,7 @@ async def m001_initial(db):
 
     await db.execute(
         f"""
-        CREATE TABLE account_permissions (
+        CREATE TABLE IF NOT EXISTS account_permissions (
             id TEXT PRIMARY KEY,
             user_id TEXT NOT NULL,
             account_id TEXT NOT NULL,
@@ -262,7 +286,7 @@ async def m001_initial(db):
     # Index for looking up permissions by user
     await db.execute(
         """
-        CREATE INDEX idx_account_permissions_user_id
+        CREATE INDEX IF NOT EXISTS idx_account_permissions_user_id
         ON account_permissions (user_id);
         """
     )
@@ -270,7 +294,7 @@ async def m001_initial(db):
     # Index for looking up permissions by account
     await db.execute(
         """
-        CREATE INDEX idx_account_permissions_account_id
+        CREATE INDEX IF NOT EXISTS idx_account_permissions_account_id
         ON account_permissions (account_id);
         """
     )
@@ -278,7 +302,7 @@ async def m001_initial(db):
     # Composite index for checking specific user+account permissions
     await db.execute(
         """
-        CREATE INDEX idx_account_permissions_user_account
+        CREATE INDEX IF NOT EXISTS idx_account_permissions_user_account
         ON account_permissions (user_id, account_id);
         """
     )
@@ -286,7 +310,7 @@ async def m001_initial(db):
     # Index for finding permissions by type
     await db.execute(
         """
-        CREATE INDEX idx_account_permissions_type
+        CREATE INDEX IF NOT EXISTS idx_account_permissions_type
         ON account_permissions (permission_type);
         """
     )
@@ -294,7 +318,7 @@ async def m001_initial(db):
     # Index for finding expired permissions
     await db.execute(
         """
-        CREATE INDEX idx_account_permissions_expires
+        CREATE INDEX IF NOT EXISTS idx_account_permissions_expires
         ON account_permissions (expires_at)
         WHERE expires_at IS NOT NULL;
         """
@@ -320,6 +344,7 @@ async def m001_initial(db):
             f"""
             INSERT INTO accounts (id, name, account_type, description, created_at)
             VALUES (:id, :name, :type, :description, {db.timestamp_now})
+            ON CONFLICT (name) DO NOTHING
             """,
             {
                 "id": str(uuid.uuid4()),
@@ -342,17 +367,18 @@ async def m002_add_account_is_active(db):
 
     Default: All existing accounts are marked as active (TRUE).
     """
-    await db.execute(
+    await _alter_add_column_safe(
+        db,
         """
         ALTER TABLE accounts
         ADD COLUMN is_active BOOLEAN NOT NULL DEFAULT TRUE
-        """
+        """,
     )
 
     # Create index for faster queries filtering by is_active
     await db.execute(
         """
-        CREATE INDEX idx_accounts_is_active ON accounts (is_active)
+        CREATE INDEX IF NOT EXISTS idx_accounts_is_active ON accounts (is_active)
         """
     )
 
@@ -372,17 +398,18 @@ async def m003_add_account_is_virtual(db):
 
     Default: All existing accounts are real (is_virtual = FALSE).
     """
-    await db.execute(
+    await _alter_add_column_safe(
+        db,
         """
         ALTER TABLE accounts
         ADD COLUMN is_virtual BOOLEAN NOT NULL DEFAULT FALSE
-        """
+        """,
     )
 
     # Create index for faster queries filtering by is_virtual
     await db.execute(
         """
-        CREATE INDEX idx_accounts_is_virtual ON accounts (is_virtual)
+        CREATE INDEX IF NOT EXISTS idx_accounts_is_virtual ON accounts (is_virtual)
         """
     )
 
@@ -402,6 +429,7 @@ async def m003_add_account_is_virtual(db):
             f"""
             INSERT INTO accounts (id, name, account_type, description, is_active, is_virtual, created_at)
             VALUES (:id, :name, :type, :description, TRUE, TRUE, {db.timestamp_now})
+            ON CONFLICT (name) DO NOTHING
             """,
             {
                 "id": str(uuid.uuid4()),
@@ -438,7 +466,7 @@ async def m004_add_rbac_tables(db):
 
     await db.execute(
         f"""
-        CREATE TABLE roles (
+        CREATE TABLE IF NOT EXISTS roles (
             id TEXT PRIMARY KEY,
             name TEXT NOT NULL UNIQUE,
             description TEXT,
@@ -451,13 +479,13 @@ async def m004_add_rbac_tables(db):
 
     await db.execute(
         """
-        CREATE INDEX idx_roles_name ON roles (name);
+        CREATE INDEX IF NOT EXISTS idx_roles_name ON roles (name);
         """
     )
 
     await db.execute(
         """
-        CREATE INDEX idx_roles_is_default ON roles (is_default)
+        CREATE INDEX IF NOT EXISTS idx_roles_is_default ON roles (is_default)
         WHERE is_default = TRUE;
         """
     )
@@ -469,7 +497,7 @@ async def m004_add_rbac_tables(db):
 
     await db.execute(
         f"""
-        CREATE TABLE role_permissions (
+        CREATE TABLE IF NOT EXISTS role_permissions (
             id TEXT PRIMARY KEY,
             role_id TEXT NOT NULL,
             account_id TEXT NOT NULL,
@@ -484,19 +512,19 @@ async def m004_add_rbac_tables(db):
 
     await db.execute(
         """
-        CREATE INDEX idx_role_permissions_role_id ON role_permissions (role_id);
+        CREATE INDEX IF NOT EXISTS idx_role_permissions_role_id ON role_permissions (role_id);
         """
     )
 
     await db.execute(
         """
-        CREATE INDEX idx_role_permissions_account_id ON role_permissions (account_id);
+        CREATE INDEX IF NOT EXISTS idx_role_permissions_account_id ON role_permissions (account_id);
         """
     )
 
     await db.execute(
         """
-        CREATE INDEX idx_role_permissions_type ON role_permissions (permission_type);
+        CREATE INDEX IF NOT EXISTS idx_role_permissions_type ON role_permissions (permission_type);
         """
     )
 
@@ -507,7 +535,7 @@ async def m004_add_rbac_tables(db):
 
     await db.execute(
         f"""
-        CREATE TABLE user_roles (
+        CREATE TABLE IF NOT EXISTS user_roles (
             id TEXT PRIMARY KEY,
             user_id TEXT NOT NULL,
             role_id TEXT NOT NULL,
@@ -522,19 +550,19 @@ async def m004_add_rbac_tables(db):
 
     await db.execute(
         """
-        CREATE INDEX idx_user_roles_user_id ON user_roles (user_id);
+        CREATE INDEX IF NOT EXISTS idx_user_roles_user_id ON user_roles (user_id);
         """
     )
 
     await db.execute(
         """
-        CREATE INDEX idx_user_roles_role_id ON user_roles (role_id);
+        CREATE INDEX IF NOT EXISTS idx_user_roles_role_id ON user_roles (role_id);
         """
     )
 
     await db.execute(
         """
-        CREATE INDEX idx_user_roles_expires ON user_roles (expires_at)
+        CREATE INDEX IF NOT EXISTS idx_user_roles_expires ON user_roles (expires_at)
         WHERE expires_at IS NOT NULL;
         """
     )
@@ -542,7 +570,7 @@ async def m004_add_rbac_tables(db):
     # Composite index for checking specific user+role assignments
     await db.execute(
         """
-        CREATE INDEX idx_user_roles_user_role ON user_roles (user_id, role_id);
+        CREATE INDEX IF NOT EXISTS idx_user_roles_user_role ON user_roles (user_id, role_id);
         """
     )
 
@@ -586,6 +614,7 @@ async def m004_add_rbac_tables(db):
             f"""
             INSERT INTO roles (id, name, description, is_default, created_by, created_at)
             VALUES (:id, :name, :description, :is_default, :created_by, {db.timestamp_now})
+            ON CONFLICT (name) DO NOTHING
             """,
             {
                 "id": str(uuid.uuid4()),
diff --git a/tests/test_migrations.py b/tests/test_migrations.py
new file mode 100644
index 0000000..b66b595
--- /dev/null
+++ b/tests/test_migrations.py
@@ -0,0 +1,109 @@
+"""Migration idempotency tests.
+
+The migration version bump lands in the core LNbits DB (`dbversions`)
+while the DDL lands in `ext_libra` — the two writes are not atomic. If
+the bump fails after the DDL commits, the whole migration re-runs on
+next boot, so every statement must be a silent no-op on re-run instead
+of crashing with `duplicate column` / `table exists` / UNIQUE
+violations (which bricks the extension until manual dbversions
+surgery).
+
+These tests run the full migration chain twice against a fresh SQLite
+database — the second pass simulates the lost-version-bump re-run.
+"""
+import importlib
+import re
+from uuid import uuid4
+
+import pytest
+
+from lnbits.db import Database
+
+pytestmark = pytest.mark.anyio
+
+
+def _module(name: str):
+    """Import a libra submodule under whichever path the active LNbits layout
+    uses (default `lnbits.extensions.libra` or bare `libra`)."""
+    for prefix in ("lnbits.extensions.libra", "libra"):
+        try:
+            return importlib.import_module(f"{prefix}.{name}")
+        except ModuleNotFoundError:
+            continue
+    raise ModuleNotFoundError(f"libra.{name}: tried both import paths")
+
+
+migrations = _module("migrations")
+
+# Same discovery as lnbits.core.helpers.run_migration: m### prefix,
+# module definition order.
+_MIGRATION_RE = re.compile(r"^m(\d\d\d)_")
+MIGRATION_FUNCTIONS = [
+    fn for name, fn in vars(migrations).items() if _MIGRATION_RE.match(name)
+]
+
+# Tables the chain must leave behind — one probe row read per table
+# proves both existence and queryability after a double run.
+EXPECTED_TABLES = [
+    "accounts",
+    "extension_settings",
+    "user_wallet_settings",
+    "manual_payment_requests",
+    "balance_assertions",
+    "user_equity_status",
+    "account_permissions",
+    "roles",
+    "role_permissions",
+    "user_roles",
+]
+
+
+async def _run_all_migrations(db: Database) -> None:
+    async with db.connect() as conn:
+        for migrate in MIGRATION_FUNCTIONS:
+            await migrate(conn)
+
+
+async def _seed_counts(db: Database) -> dict:
+    async with db.connect() as conn:
+        accounts = await conn.fetchall("SELECT id, name FROM accounts")
+        roles = await conn.fetchall("SELECT id, name FROM roles")
+    return {
+        "account_names": sorted(r["name"] for r in accounts),
+        "account_ids": sorted(r["id"] for r in accounts),
+        "role_names": sorted(r["name"] for r in roles),
+        "role_ids": sorted(r["id"] for r in roles),
+    }
+
+
+async def test_migrations_rerun_is_noop():
+    """Full chain twice: second run must not raise and not re-seed."""
+    db = Database(f"ext_libra_migtest_{uuid4().hex[:8]}")
+
+    await _run_all_migrations(db)
+    first = await _seed_counts(db)
+
+    # Simulate the lost dbversions bump: everything runs again.
+    await _run_all_migrations(db)
+    second = await _seed_counts(db)
+
+    # Seeds must not duplicate (names) and must not be replaced (ids).
+    assert second == first
+    assert first["account_names"], "seed accounts missing after migration"
+    assert "Employee" in first["role_names"]
+
+    # Every table exists and is queryable after the double run.
+    async with db.connect() as conn:
+        for table in EXPECTED_TABLES:
+            await conn.fetchall(f"SELECT * FROM {table} LIMIT 1")  # noqa: S608
+
+
+async def test_single_migration_rerun_is_noop():
+    """Each migration individually survives an immediate re-run (the
+    version bump fails right after that one migration committed)."""
+    db = Database(f"ext_libra_migtest_{uuid4().hex[:8]}")
+
+    async with db.connect() as conn:
+        for migrate in MIGRATION_FUNCTIONS:
+            await migrate(conn)
+            await migrate(conn)  # re-run before "bumping" to the next