Merge pull request #1826 from ViktorT-11/2025-09-check-dirty-database-version

tapdb: error on dirty database versions

Author: Roasbeef

docs/release-notes/release-notes-0.7.0.md

@@ -53,6 +53,16 @@
   requiring a fully initialized AuxSweeper, allowing sweep operations to proceed
   during server startup.
 
+- [Ensure that tapd won't start if the latest db migration errors and sets the
+  db to a dirty state for sqlite database
+  backends](https://github.com/lightninglabs/taproot-assets/pull/1826).
+  If the latest migration errors and sets the db to a dirty state, startup of
+  tapd will now be prevented for all database backend types. Previously, this
+  safeguard did not work correctly for SQLite backends if the most recent
+  migration failed. Tapd could then still start even though the database was
+  dirty. This issue has been resolved, and the behavior is now consistent across
+  all database backend types.
+
 # New Features
 
 ## Functional Enhancements

tapdb/migrations.go

@@ -166,7 +166,19 @@ func applyMigrations(fs fs.FS, driver database.Driver, path, dbName string,
 		return err
 	}
 
-	migrationVersion, _, _ := sqlMigrate.Version()
+	migrationVersion, dirty, err := sqlMigrate.Version()
+	if err != nil && !errors.Is(err, migrate.ErrNilVersion) {
+		return fmt.Errorf("unable to determine current migration "+
+			"version: %w", err)
+	}
+
+	// If the migration version is dirty, we should not proceed with further
+	// migrations, as this indicates that a previous migration did not
+	// complete successfully and requires manual intervention.
+	if dirty {
+		return fmt.Errorf("database is in a dirty state at version "+
+			"%v, manual intervention required", migrationVersion)
+	}
 
 	// As the down migrations may end up *dropping* data, we want to
 	// prevent that without explicit accounting.

tapdb/migrations_test.go

@@ -3,6 +3,7 @@ package tapdb
 import (
 	"context"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -694,3 +695,80 @@ func TestMigration37(t *testing.T) {
 
 	require.Len(t, burns, 5)
 }
+
+// TestDirtySqliteVersion tests that if a migration fails and leaves an Sqlite
+// database backend in a dirty state, any attempts of re-executing migrations on
+// the db (i.e. restart tapd), will fail with an error indicating that the
+// database is in a dirty state. This is regardless of whether the failing
+// migration is the latest migration or an intermediate migration.
+func TestDirtySqliteVersion(t *testing.T) {
+	var (
+		err       error
+		testError = errors.New("test error")
+
+		// testPostMigrationChecks1 is a map that will trigger a
+		// migration callback for migration 2 which always returns an
+		// error. This is used to simulate an intermediate migration
+		// that fails and leaves the db in a dirty state.
+		testPostMigrationChecks1 = map[uint]postMigrationCheck{
+			2: func(ctx context.Context, q sqlc.Querier) error {
+				return testError
+			},
+		}
+
+		// testPostMigrationChecks2 is a map that will trigger a
+		// migration callback for the latest migration which always
+		// returns an error. This is used to simulate that the latest
+		// migration fails and leaves the db in a dirty state.
+		testPostMigrationChecks2 = map[uint]postMigrationCheck{
+			LatestMigrationVersion: func(ctx context.Context,
+				q sqlc.Querier) error {
+
+				return testError
+			},
+		}
+	)
+
+	// First, we'll test that intermediate migration that fails and leaves
+	// the db in a dirty state is detected on subsequent migration attempts.
+	// Note that this test only targets Sqlite database backends, and
+	// therefore we create a new Sqlite test db for this.
+	db1 := NewTestSqliteDBWithVersion(t, 1)
+
+	// As intend that the failing migration version should be an
+	// intermediate migration, we use the testPostMigrationChecks1 when
+	// executing the migration. Note that we use the `backupAndMigrate` func
+	// as the MigrationTarget, to simulate what is used in production for
+	// Sqlite database backends.
+	err = db1.ExecuteMigrations(db1.backupAndMigrate, WithPostStepCallbacks(
+		makePostStepCallbacks(db1, testPostMigrationChecks1),
+	))
+	require.ErrorIs(t, err, testError)
+
+	// If we now attempt to execute migrations again, it should fail with an
+	// error indicating that the db is in a dirty state.
+	err = db1.ExecuteMigrations(db1.backupAndMigrate, WithPostStepCallbacks(
+		makePostStepCallbacks(db1, testPostMigrationChecks1),
+	))
+	require.ErrorContains(t, err, "database is in a dirty state")
+
+	// Next, we'll test that if the **latest** migration fails and leaves
+	// the db in a dirty state, that this is also detected on subsequent
+	// migration attempts.
+	db2 := NewTestSqliteDBWithVersion(t, 1)
+
+	// As we intend that the failing migration version should be the latest
+	// migration, we now use the testPostMigrationChecks2 when executing
+	// the migration.
+	err = db2.ExecuteMigrations(db2.backupAndMigrate, WithPostStepCallbacks(
+		makePostStepCallbacks(db2, testPostMigrationChecks2),
+	))
+	require.ErrorIs(t, err, testError)
+
+	// If we now attempt to execute migrations again, it should fail with an
+	// error indicating that the db is in a dirty state.
+	err = db2.ExecuteMigrations(db2.backupAndMigrate, WithPostStepCallbacks(
+		makePostStepCallbacks(db2, testPostMigrationChecks2),
+	))
+	require.ErrorContains(t, err, "database is in a dirty state")
+}