fix(db): numbered migrations that run once and fail loudly (FLAW-13)

schema_migrations records every applied step; the former migrate() is step 3, the baseline that builds the v0.4.1 schema and repairs older databases. Columns are only added when missing, so every other error now stops the start with a clear message instead of being swallowed, and the cleanup statements no longer run on each start. migrations/001_init.sql was never read and is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-29 14:49:35 +01:00
1 parent f57a9884d1
commit dc44f8250c
@@ -72,7 +72,7 @@ type docQueryer interface {
 }

 // migrateDocSync adds the columns and tables of the collaboration model and converts old flat block lists.
-func (s *Store) migrateDocSync() {
+func (s *Store) migrateDocSync() error {
 	stmts := []string{
 		"ALTER TABLE blocks ADD COLUMN parent_id TEXT NOT NULL DEFAULT '';",
 		"ALTER TABLE blocks ADD COLUMN order_key TEXT NOT NULL DEFAULT '';",
@@ -110,15 +110,16 @@ func (s *Store) migrateDocSync() {
 		"CREATE INDEX IF NOT EXISTS idx_doc_op_results_created ON doc_op_results(created_at);",
 	}
 	for _, q := range stmts {
-		if _, err := s.db.Exec(q); err != nil && !strings.Contains(err.Error(), "duplicate column") {
-			slog.Error("Migration der Notiz-Synchronisation fehlgeschlagen", "error", err, "statement", q)
+		if err := s.execSchemaStatement(q); err != nil {
+			return err
 		}
 	}
 	// Tombstones from before deleted_at existed start their retention now
 	if _, err := s.db.Exec(`UPDATE blocks SET deleted_at = ? WHERE deleted = 1 AND deleted_at = ''`, domain.NowISO()); err != nil {
-		slog.Error("Migration der Notiz-Synchronisation fehlgeschlagen", "error", err)
+		return err
 	}
 	s.backfillBlockTree()
+	return nil
 }

 // backfillBlockTree converts blocks stored as a flat list (level + position) into parent_id + order_key.
@@ -0,0 +1,112 @@
+package storage
+
+import (
+	"database/sql"
+	"fmt"
+	"log/slog"
+	"regexp"
+	"strings"
+
+	"github.com/sandrodach/mindsift/internal/domain"
+)
+
+// Schema migrations (FLAW-13). Every step runs exactly once per database and is recorded in
+// schema_migrations; a failing step stops the start with its error instead of being ignored.
+// New schema changes are appended as new steps (next free id), never edited into old ones.
+// PRAGMA user_version mirrors the highest applied id. Ids 1 and 2 were used by earlier releases
+// (database wipes on start), so the first step here is 3.
+
+type migration struct {
+	id   int
+	name string
+	run  func(s *Store) error
+}
+
+var migrations = []migration{
+	{3, "Basis-Schema v0.4.1", (*Store).migrateBaseline},
+}
+
+func (s *Store) migrate() error {
+	if _, err := s.db.Exec(`CREATE TABLE IF NOT EXISTS schema_migrations (
+		id INTEGER PRIMARY KEY,
+		name TEXT NOT NULL,
+		applied_at TEXT NOT NULL
+	)`); err != nil {
+		return err
+	}
+	applied := map[int]bool{}
+	rows, err := s.db.Query(`SELECT id FROM schema_migrations`)
+	if err != nil {
+		return err
+	}
+	for rows.Next() {
+		var id int
+		if rows.Scan(&id) == nil {
+			applied[id] = true
+		}
+	}
+	_ = rows.Close()
+
+	last := 0
+	for _, m := range migrations {
+		last = m.id
+		if applied[m.id] {
+			continue
+		}
+		slog.Info("Datenbank-Migration", "id", m.id, "name", m.name)
+		if err := m.run(s); err != nil {
+			return fmt.Errorf("Migration %d (%s): %w", m.id, m.name, err)
+		}
+		if _, err := s.db.Exec(`INSERT INTO schema_migrations (id, name, applied_at) VALUES (?, ?, ?)`,
+			m.id, m.name, domain.NowISO()); err != nil {
+			return err
+		}
+	}
+	_, err = s.db.Exec(fmt.Sprintf(`PRAGMA user_version = %d`, last))
+	return err
+}
+
+var addColumnRe = regexp.MustCompile(`(?is)^\s*ALTER\s+TABLE\s+(\w+)\s+ADD\s+COLUMN\s+(\w+)\s+(.+?);?\s*$`)
+
+// execSchemaStatement runs one schema statement; "ALTER TABLE … ADD COLUMN" only when the column is
+// missing, so that a real error is never mistaken for "already there".
+func (s *Store) execSchemaStatement(q string) error {
+	if m := addColumnRe.FindStringSubmatch(q); m != nil {
+		return s.addColumnIfMissing(m[1], m[2], m[3])
+	}
+	if _, err := s.db.Exec(q); err != nil {
+		return fmt.Errorf("%w (%s)", err, strings.TrimSpace(q))
+	}
+	return nil
+}
+
+func (s *Store) addColumnIfMissing(table, column, definition string) error {
+	exists, err := s.columnExists(table, column)
+	if err != nil || exists {
+		return err
+	}
+	if _, err := s.db.Exec(fmt.Sprintf(`ALTER TABLE %s ADD COLUMN %s %s`, table, column, definition)); err != nil {
+		return fmt.Errorf("Spalte %s.%s anlegen: %w", table, column, err)
+	}
+	return nil
+}
+
+func (s *Store) columnExists(table, column string) (bool, error) {
+	rows, err := s.db.Query(fmt.Sprintf(`PRAGMA table_info(%s)`, table))
+	if err != nil {
+		return false, err
+	}
+	defer rows.Close()
+	for rows.Next() {
+		var cid, notNull, pk int
+		var name, typ string
+		var dflt sql.NullString
+		if err := rows.Scan(&cid, &name, &typ, &notNull, &dflt, &pk); err != nil {
+			return false, err
+		}
+		if strings.EqualFold(name, column) {
+			return true, nil
+		}
+	}
+	return false, rows.Err()
+}
@@ -0,0 +1,45 @@
+package storage
+
+import (
+	"errors"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+func TestMigrationsRunOnceAndRepairOldDatabases(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "migrate.db")
+	store, err := NewStore(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var n int
+	_ = store.db.QueryRow(`SELECT COUNT(*) FROM schema_migrations`).Scan(&n)
+	if n != len(migrations) {
+		t.Fatalf("expected %d recorded migrations, got %d", len(migrations), n)
+	}
+
+	// A database from before the migration table, missing a later column
+	if _, err := store.db.Exec(`ALTER TABLE thoughts DROP COLUMN alarm_failures`); err != nil {
+		t.Fatal(err)
+	}
+	_, _ = store.db.Exec(`DROP TABLE schema_migrations`)
+	_ = store.Close()
+
+	store, err = NewStore(path)
+	if err != nil {
+		t.Fatalf("reopen: %v", err)
+	}
+	if ok, _ := store.columnExists("thoughts", "alarm_failures"); !ok {
+		t.Fatal("baseline must add missing columns to older databases")
+	}
+
+	// A failing step stops the start with its error and is not recorded
+	saved := migrations
+	defer func() { migrations = saved }()
+	migrations = append(append([]migration{}, saved...), migration{999, "kaputt", func(*Store) error { return errors.New("boom") }})
+	_ = store.Close()
+	if _, err := NewStore(path); err == nil || !strings.Contains(err.Error(), "Migration 999") {
+		t.Fatalf("failing migration must abort the start, got %v", err)
+	}
+}
@@ -57,10 +57,6 @@ func (s *Store) Close() error {
 	return s.db.Close()
 }

-// schemaVersion is stored in PRAGMA user_version so later releases can tell which migrations a database
-// already has. (Version 2 once wiped older databases on start; all installations are past that now.)
-const schemaVersion = 2
-
 // dropAllTables removes every table; used by the demo mode reset.
 func (s *Store) dropAllTables() error {
 	conn, err := s.db.Conn(context.Background())
@@ -94,7 +90,9 @@ func (s *Store) dropAllTables() error {
 	return err
 }

-func (s *Store) migrate() error {
+// migrateBaseline is migration 3: the complete schema as of v0.4.1, built so that it also brings any
+// older database up to that state. It runs once per database (see migrations.go).
+func (s *Store) migrateBaseline() error {
 	schema := `
 	PRAGMA journal_mode = WAL;
 	PRAGMA synchronous = NORMAL;
@@ -463,23 +461,41 @@ func (s *Store) migrate() error {
 		"ALTER TABLE thoughts ADD COLUMN recurrence_spawned INTEGER NOT NULL DEFAULT 0;",
 	}
 	for _, q := range columnMigrations {
-		_, _ = s.db.Exec(q)
+		if err := s.execSchemaStatement(q); err != nil {
+			return err
+		}
 	}

 	// Clean up legacy space and old document block/crdt tables
-	_, _ = s.db.Exec(`DROP TABLE IF EXISTS spaces`)
-	_, _ = s.db.Exec(`DROP TABLE IF EXISTS space_members`)
-	_, _ = s.db.Exec(`DROP TABLE IF EXISTS space_team_shares`)
-	_, _ = s.db.Exec(`DROP TABLE IF EXISTS document_crdt_state`)
-	_, _ = s.db.Exec(`DROP TABLE IF EXISTS document_crdt_updates`)
-	_, _ = s.db.Exec(`DROP TABLE IF EXISTS document_blocks`)
-	_, _ = s.db.Exec(`DROP TABLE IF EXISTS doc_block_tags`)
+	if _, err := s.db.Exec(`DROP TABLE IF EXISTS spaces`); err != nil {
+		return err
+	}
+	if _, err := s.db.Exec(`DROP TABLE IF EXISTS space_members`); err != nil {
+		return err
+	}
+	if _, err := s.db.Exec(`DROP TABLE IF EXISTS space_team_shares`); err != nil {
+		return err
+	}
+	if _, err := s.db.Exec(`DROP TABLE IF EXISTS document_crdt_state`); err != nil {
+		return err
+	}
+	if _, err := s.db.Exec(`DROP TABLE IF EXISTS document_crdt_updates`); err != nil {
+		return err
+	}
+	if _, err := s.db.Exec(`DROP TABLE IF EXISTS document_blocks`); err != nil {
+		return err
+	}
+	if _, err := s.db.Exec(`DROP TABLE IF EXISTS doc_block_tags`); err != nil {
+		return err
+	}

 	// Backfill username for existing accounts where username is empty
-	_, _ = s.db.Exec(`UPDATE users SET username = LOWER(SUBSTR(email, 1, INSTR(email, '@') - 1)) WHERE (username IS NULL OR username = '') AND INSTR(email, '@') > 0`)
+	if _, err := s.db.Exec(`UPDATE users SET username = LOWER(SUBSTR(email, 1, INSTR(email, '@') - 1)) WHERE (username IS NULL OR username = '') AND INSTR(email, '@') > 0`); err != nil {
+		return err
+	}

 	// Default system settings (public registration disabled by default; invite or admin creation required)
-	_, _ = s.db.Exec(`INSERT OR IGNORE INTO system_settings (key, value) VALUES 
+	if _, err := s.db.Exec(`INSERT OR IGNORE INTO system_settings (key, value) VALUES 
 		('sharing_mode', 'organizations'),
 		('allow_registration', 'false'),
 		('instance_name', 'MindSift'),
@@ -490,18 +506,30 @@ func (s *Store) migrate() error {
 		('smtp_from', ''),
 		('smtp_tls', 'true'),
 		('smtp_skip_verify', 'false'),
-		('allow_private_webhooks', 'false')`)
+		('allow_private_webhooks', 'false')`); err != nil {
+		return err
+	}

 	// Clean up legacy roles in team_members and users
-	_, _ = s.db.Exec(`UPDATE team_members SET role = 'admin' WHERE role IN ('team_admin', 'owner')`)
-	_, _ = s.db.Exec(`UPDATE team_members SET role = 'member' WHERE role NOT IN ('admin', 'member')`)
-	_, _ = s.db.Exec(`UPDATE users SET role = 'member' WHERE role = 'team_admin'`)
+	if _, err := s.db.Exec(`UPDATE team_members SET role = 'admin' WHERE role IN ('team_admin', 'owner')`); err != nil {
+		return err
+	}
+	if _, err := s.db.Exec(`UPDATE team_members SET role = 'member' WHERE role NOT IN ('admin', 'member')`); err != nil {
+		return err
+	}
+	if _, err := s.db.Exec(`UPDATE users SET role = 'member' WHERE role = 'team_admin'`); err != nil {
+		return err
+	}

 	// Keys from before scopes existed ("ingest,read") may deliver into the inbox, as before
-	_, _ = s.db.Exec(`UPDATE api_keys SET scopes = 'webhook:triage' WHERE scopes NOT LIKE '%webhook:%'`)
+	if _, err := s.db.Exec(`UPDATE api_keys SET scopes = 'webhook:triage' WHERE scopes NOT LIKE '%webhook:%'`); err != nil {
+		return err
+	}

 	// Block tree, operation log and idempotency tables for collaborative editing
-	s.migrateDocSync()
+	if err := s.migrateDocSync(); err != nil {
+		return err
+	}

 	// Migrate workspace documents, blocks, folders, and strip legacy folder:* tags into pure DB tables
 	s.migrateWorkspaceToPureDB()
@@ -518,8 +546,7 @@ func (s *Store) migrate() error {
 		return err
 	}

-	_, err := s.db.Exec(fmt.Sprintf(`PRAGMA user_version = %d`, schemaVersion))
-	return err
+	return nil
 }

 // EnsureDefaultUser is retained strictly for unit test convenience
@@ -408,10 +408,10 @@ func TestFolderManagementAndDeletion(t *testing.T) {
 		t.Fatalf("GetFolders failed: %v", err)
 	}
 	expected := map[string]bool{
-		"Archiv":                 true,
-		"Projekte":               true,
-		"Projekte/MindSift":      true,
-		"Projekte/MindSift/Sub":  true,
+		"Archiv":                true,
+		"Projekte":              true,
+		"Projekte/MindSift":     true,
+		"Projekte/MindSift/Sub": true,
 	}
 	for _, f := range folders {
 		delete(expected, f)
@@ -165,8 +165,8 @@ func TestDataSurvivesRestart(t *testing.T) {
 	}
 	var v int
 	_ = store.db.QueryRow(`PRAGMA user_version`).Scan(&v)
-	if v != schemaVersion {
-		t.Fatalf("user_version = %d, want %d", v, schemaVersion)
+	if want := migrations[len(migrations)-1].id; v != want {
+		t.Fatalf("user_version = %d, want %d", v, want)
 	}
 }