chore(db): update schema version in the same transaction as migration (#10321)

Just to be entirely sure that if the migration succeeds the schema
version is always also updated. Currently if a migration succeeds but a
later migration doesn't, the changes of the migration apply but the
version stays - if the migration is breaking/non-idempotent, it will
fail when it tries to rerun it next time (otherwise it's just a
pointless re-execution).

Unfortunately with the current `db.runScripts` it wasn't that easy to
do, so I had to do quite a bit of refactoring. I am also ensuring the
right order of transactions now, though I assume that was already the
case lexicographically - can't hurt to be safe.

internal/db/sqlite/basedb.go

@@ -7,11 +7,13 @@
 package sqlite
 
 import (
+	"cmp"
 	"database/sql"
 	"embed"
 	"io/fs"
 	"net/url"
 	"path/filepath"
+	"slices"
 	"strconv"
 	"strings"
 	"sync"
@@ -42,6 +44,7 @@ type baseDB struct {
 	tplInput      map[string]any
 }
 
+//nolint:noctx
 func openBase(path string, maxConns int, pragmas, schemaScripts, migrationScripts []string) (*baseDB, error) {
 	// Open the database with options to enable foreign keys and recursive
 	// triggers (needed for the delete+insert triggers on row replace).
@@ -89,27 +92,36 @@ func openBase(path string, maxConns int, pragmas, schemaScripts, migrationScript
 
 	ver, _ := db.getAppliedSchemaVersion()
 	if ver.SchemaVersion > 0 {
-		filter := func(scr string) bool {
-			scr = filepath.Base(scr)
-			nstr, _, ok := strings.Cut(scr, "-")
+		type migration struct {
+			script  string
+			version int
+		}
+		migrations := make([]migration, 0, len(migrationScripts))
+		for _, script := range migrationScripts {
+			base := filepath.Base(script)
+			nstr, _, ok := strings.Cut(base, "-")
 			if !ok {
-				return false
+				continue
 			}
 			n, err := strconv.ParseInt(nstr, 10, 32)
 			if err != nil {
-				return false
+				continue
 			}
-			return int(n) > ver.SchemaVersion
+			migrations = append(migrations, migration{
+				script:  script,
+				version: int(n),
+			})
 		}
-		for _, script := range migrationScripts {
-			if err := db.runScripts(script, filter); err != nil {
+		slices.SortFunc(migrations, func(m1, m2 migration) int { return cmp.Compare(m1.version, m2.version) })
+		for _, m := range migrations {
+			if err := db.applyMigration(m.version, m.script); err != nil {
 				return nil, wrap(err)
 			}
 		}
 	}
 
 	// Set the current schema version, if not already set
-	if err := db.setAppliedSchemaVersion(currentSchemaVersion); err != nil {
+	if err := setAppliedSchemaVersion(currentSchemaVersion, db.sql); err != nil {
 		return nil, wrap(err)
 	}
 
@@ -204,6 +216,7 @@ func (f failedStmt) Get(_ any, _ ...any) error           { return f.err }
 func (f failedStmt) Queryx(_ ...any) (*sqlx.Rows, error) { return nil, f.err }
 func (f failedStmt) Select(_ any, _ ...any) error        { return f.err }
 
+//nolint:noctx
 func (s *baseDB) runScripts(glob string, filter ...func(s string) bool) error {
 	scripts, err := fs.Glob(embedded, glob)
 	if err != nil {
@@ -223,24 +236,51 @@ nextScript:
 				continue nextScript
 			}
 		}
-		bs, err := fs.ReadFile(embedded, scr)
-		if err != nil {
-			return wrap(err, scr)
-		}
-		// SQLite requires one statement per exec, so we split the init
-		// files on lines containing only a semicolon and execute them
-		// separately. We require it on a separate line because there are
-		// also statement-internal semicolons in the triggers.
-		for _, stmt := range strings.Split(string(bs), "\n;") {
-			if _, err := tx.Exec(s.expandTemplateVars(stmt)); err != nil {
-				return wrap(err, stmt)
-			}
+		if err := s.execScript(tx, scr); err != nil {
+			return wrap(err)
 		}
 	}
 
 	return wrap(tx.Commit())
 }
 
+//nolint:noctx
+func (s *baseDB) applyMigration(ver int, script string) error {
+	tx, err := s.sql.Begin()
+	if err != nil {
+		return wrap(err)
+	}
+	defer tx.Rollback() //nolint:errcheck
+
+	if err := s.execScript(tx, script); err != nil {
+		return wrap(err)
+	}
+
+	if err := setAppliedSchemaVersion(ver, tx); err != nil {
+		return wrap(err)
+	}
+
+	return wrap(tx.Commit())
+}
+
+//nolint:noctx
+func (s *baseDB) execScript(tx *sql.Tx, scr string) error {
+	bs, err := fs.ReadFile(embedded, scr)
+	if err != nil {
+		return wrap(err, scr)
+	}
+	// SQLite requires one statement per exec, so we split the init
+	// files on lines containing only a semicolon and execute them
+	// separately. We require it on a separate line because there are
+	// also statement-internal semicolons in the triggers.
+	for _, stmt := range strings.Split(string(bs), "\n;") {
+		if _, err := tx.Exec(s.expandTemplateVars(stmt)); err != nil {
+			return wrap(err, stmt)
+		}
+	}
+	return nil
+}
+
 type schemaVersion struct {
 	SchemaVersion    int
 	AppliedAt        int64
@@ -251,11 +291,11 @@ func (s *schemaVersion) AppliedTime() time.Time {
 	return time.Unix(0, s.AppliedAt)
 }
 
-func (s *baseDB) setAppliedSchemaVersion(ver int) error {
-	_, err := s.stmt(`
+func setAppliedSchemaVersion(ver int, execer sqlx.Execer) error {
+	_, err := execer.Exec(`
 		INSERT OR IGNORE INTO schemamigrations (schema_version, applied_at, syncthing_version)
 		VALUES (?, ?, ?)
-	`).Exec(ver, time.Now().UnixNano(), build.LongVersion)
+	`, ver, time.Now().UnixNano(), build.LongVersion)
 	return wrap(err)
 }
 

internal/db/sqlite/sql/README.md

@@ -2,7 +2,8 @@ These SQL scripts are embedded in the binary.
 
 Scripts in `schema/` are run at every startup, in alphanumerical order.
 
-Scripts in `migrations/` are run when a migration is needed; the must begin
+Scripts in `migrations/` are run when a migration is needed; they must begin
 with a number that equals the schema version that results from that
-migration. Migrations are not run on initial database creation, so the
-scripts in `schema/` should create the latest version.
+migration. Only one script per schema version must exist.
+Migrations are not run on initial database creation, so the scripts in `schema/`
+should create the latest version.