Merge pull request #374 from docker/migration-broken-fix

fix: error looking for database down migrations

Author: bobbyhouse

docs/database/README.md

@@ -40,11 +40,13 @@ Application Startup
     ↓
 pkg/db/New() called
     ↓
-Check for pending migrations
-    ↓
 [Acquire File Lock] ← Prevents concurrent migrations
     ↓
-Run migrations sequentially
+Check current database version
+    ↓
+Database version > available migrations?
+    ├─ Yes → Return error (database from newer version)
+    └─ No  → Run pending migrations sequentially
     ↓
 [Release File Lock]
     ↓
@@ -78,6 +80,17 @@ for a race condition if two process concurrently attempt to run the migrations.
 
 **The Solution**: OS-level file locking ensures only one process runs migrations at a time. Other processes wait, then see migrations already complete.
 
+### Database Version Compatibility
+
+#### Running Older Version After Newer Version
+
+If you run an older version of the tool after having used a newer version, the database may be at a higher schema version than the older tool expects. In this case:
+
+- The tool detects the database is ahead of available migrations
+- An error is returned to prevent potential data corruption:
+  ```
+  database version 8 (~/.docker/mcp/mcp-toolkit.db) is ahead of the current application version. Please upgrade to the latest version
+  ```
 
 ## Testing
 

pkg/db/db.go

@@ -6,6 +6,7 @@ import (
 	"embed"
 	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"time"
@@ -41,7 +42,9 @@ type dao struct {
 var migrations embed.FS
 
 type options struct {
-	dbFile string
+	dbFile         string
+	migrationsFS   fs.FS
+	migrationsPath string
 }
 
 type Option func(o *options) error
@@ -53,6 +56,14 @@ func WithDatabaseFile(dbFile string) Option {
 	}
 }
 
+func WithMigrations(filesystem fs.FS, path string) Option {
+	return func(o *options) error {
+		o.migrationsFS = filesystem
+		o.migrationsPath = path
+		return nil
+	}
+}
+
 func New(opts ...Option) (DAO, error) {
 	var o options
 	for _, opt := range opts {
@@ -80,19 +91,70 @@ func New(opts ...Option) (DAO, error) {
 	db.SetMaxIdleConns(1)
 	db.SetConnMaxLifetime(0)
 
-	migDriver, err := iofs.New(migrations, "migrations")
+	migrationsFS := o.migrationsFS
+	if migrationsFS == nil {
+		migrationsFS = &migrations
+	}
+
+	migrationsPath := o.migrationsPath
+	if migrationsPath == "" {
+		migrationsPath = "migrations"
+	}
+
+	err = runMigrations(o.dbFile, db, migrationsFS, migrationsPath)
 	if err != nil {
 		return nil, err
 	}
 
+	sqlxDb := sqlx.NewDb(db, "sqlite")
+
+	return &dao{db: sqlxDb}, nil
+}
+
+func (d *dao) Close() error {
+	return d.db.Close()
+}
+
+func DefaultDatabaseFilename() (string, error) {
+	homeDir, err := user.HomeDir()
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(homeDir, ".docker", "mcp", "mcp-toolkit.db"), nil
+}
+
+func ensureDirectoryExists(path string) {
+	dir := filepath.Dir(path)
+	if _, err := os.Stat(dir); os.IsNotExist(err) {
+		_ = os.MkdirAll(dir, 0o755)
+	}
+}
+
+func txClose(tx *sqlx.Tx, err *error) {
+	if err == nil || *err == nil {
+		return
+	}
+
+	if txerr := tx.Rollback(); txerr != nil {
+		log.Logf("failed to rollback transaction: %v", txerr)
+	}
+}
+
+func runMigrations(dbFile string, db *sql.DB, migrationsFS fs.FS, migrationsPath string) error {
+	migDriver, err := iofs.New(migrationsFS, migrationsPath)
+	if err != nil {
+		return err
+	}
+	defer migDriver.Close()
+
 	driver, err := msqlite.WithInstance(db, &msqlite.Config{})
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	mig, err := migrate.NewWithInstance("iofs", migDriver, "sqlite", driver)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	// Use file locking to prevent concurrent migrations across processes
@@ -102,7 +164,7 @@ func New(opts ...Option) (DAO, error) {
 	//
 	// Note: The lock file persists on disk after Unlock() - this is intentional.
 	// flock.Unlock() only releases the lock and closes the file descriptor
-	lockFile := filepath.Join(filepath.Dir(o.dbFile), ".mcp-toolkit-migration.lock")
+	lockFile := filepath.Join(filepath.Dir(dbFile), ".mcp-toolkit-migration.lock")
 	fileLock := flock.New(lockFile)
 
 	// Try to acquire the lock with a 5 second timeout
@@ -111,53 +173,51 @@ func New(opts ...Option) (DAO, error) {
 
 	locked, err := fileLock.TryLockContext(ctx, 100*time.Millisecond)
 	if err != nil {
-		return nil, fmt.Errorf("failed to acquire migration lock: %w", err)
+		return fmt.Errorf("failed to acquire migration lock: %w", err)
 	}
 	if !locked {
-		return nil, fmt.Errorf("timeout waiting for migration lock")
+		return fmt.Errorf("timeout waiting for migration lock")
 	}
 	defer func() {
 		if err := fileLock.Unlock(); err != nil {
 			log.Logf("failed to unlock migration lock: %v", err)
 		}
 	}()
 
-	// Now safely run migrations with the lock held
-	err = mig.Up()
-	if err != nil && !errors.Is(err, migrate.ErrNoChange) {
-		return nil, fmt.Errorf("failed to run migrations: %w", err)
-	}
+	// Now that we have the lock, check the current migration state
+	version, dirty, err := mig.Version()
 
-	sqlxDb := sqlx.NewDb(db, "sqlite")
+	// If ErrNilVersion, the database is fresh (no migrations run yet)
+	// In this case, proceed with running migrations
+	isFreshDatabase := errors.Is(err, migrate.ErrNilVersion)
 
-	return &dao{db: sqlxDb}, nil
-}
-
-func (d *dao) Close() error {
-	return d.db.Close()
-}
-
-func DefaultDatabaseFilename() (string, error) {
-	homeDir, err := user.HomeDir()
-	if err != nil {
-		return "", err
+	if err != nil && !isFreshDatabase {
+		return fmt.Errorf("failed to get migration version: %w", err)
 	}
-	return filepath.Join(homeDir, ".docker", "mcp", "mcp-toolkit.db"), nil
-}
 
-func ensureDirectoryExists(path string) {
-	dir := filepath.Dir(path)
-	if _, err := os.Stat(dir); os.IsNotExist(err) {
-		_ = os.MkdirAll(dir, 0o755)
+	// Check if database is in dirty state (migration was interrupted)
+	if dirty {
+		return fmt.Errorf("database is in dirty state at version %d, manual intervention required", version)
 	}
-}
 
-func txClose(tx *sqlx.Tx, err *error) {
-	if err == nil || *err == nil {
-		return
+	// For fresh databases, always run migrations
+	if !isFreshDatabase {
+		// Check if database version is ahead of available migrations
+		// This happens when running older code against a database that was upgraded by newer code
+		_, _, err = migDriver.ReadUp(version)
+		if errors.Is(err, os.ErrNotExist) {
+			return fmt.Errorf("database version %d (%s) is ahead of the current application version. Please upgrade to the latest version", version, dbFile)
+		}
+		if err != nil {
+			return fmt.Errorf("failed to read migration file for version %d: %w", version, err)
+		}
 	}
 
-	if txerr := tx.Rollback(); txerr != nil {
-		log.Logf("failed to rollback transaction: %v", txerr)
+	// Now safely run migrations with the lock held
+	err = mig.Up()
+	if err != nil && !errors.Is(err, migrate.ErrNoChange) {
+		return fmt.Errorf("failed to run migrations: %w", err)
 	}
+
+	return nil
 }