Merge pull request #345 from docker/fix/database-migration-race-condition

Database migration race condition

Author: bobbyhouse

docs/database/README.md

@@ -0,0 +1,92 @@
+# Docker MCP Gateway Database
+
+## Overview
+
+The Docker MCP Gateway uses an embedded SQLite database to store configuration data, profiles (working sets), server catalogs, and migration status. The database is located at `~/.docker/mcp/mcp-toolkit.db` and is automatically created and migrated when the CLI first runs.
+
+This document covers the database architecture, migration system, concurrent access handling, and troubleshooting.
+
+## Architecture
+
+### Database Location
+
+```
+~/.docker/mcp/
+├── mcp-toolkit.db           # Main SQLite database
+├── .mcp-toolkit-migration.lock  # Migration lock file (persists after creation)
+```
+
+### Database Schema
+
+The database schema is managed through versioned migrations in `pkg/db/migrations/`
+
+### Key Components
+
+**Database Package** (`pkg/db/`)
+   - DAO interface for all database operations
+   - Migration management with golang-migrate
+   - Connection pooling and configuration
+   - File locking for concurrent access safety
+
+
+## Migration System
+
+### How Migrations Work
+
+The Docker MCP Gateway uses [golang-migrate](https://github.com/golang-migrate/migrate) for schema versioning:
+
+```
+Application Startup
+    ↓
+pkg/db/New() called
+    ↓
+Check for pending migrations
+    ↓
+[Acquire File Lock] ← Prevents concurrent migrations
+    ↓
+Run migrations sequentially
+    ↓
+[Release File Lock]
+    ↓
+Database ready for use
+```
+
+### Migration Lock File
+
+To prevent race conditions when multiple processes start simultaneously, migrations use an OS-level file lock:
+
+- **Lock File**: `~/.docker/mcp/.mcp-toolkit-migration.lock`
+- **Library**: `github.com/gofrs/flock` (cross-platform)
+- **Persistence**: The lock file persists on disk after migrations complete (this is intentional and safe)
+
+### Migration Operations
+
+Migrations use both SQLite locks AND file locks:
+- File lock prevents multiple processes from attempting migrations
+- SQLite locks protect individual migration transactions
+- This two-level locking ensures safety during initialization
+
+### Why File Locking Is Necessary
+
+Migrations with [golang-migrate](https://github.com/golang-migrate/migrate) are not run within a single transaction. There is the possibility
+for a race condition if two process concurrently attempt to run the migrations.
+
+**The Problem**: When multiple processes start simultaneously:
+1. All check the database state (clean)
+2. All try to run migrations at the same time
+3. Race condition occurs → "Dirty database version" error
+
+**The Solution**: OS-level file locking ensures only one process runs migrations at a time. Other processes wait, then see migrations already complete.
+
+
+## Testing
+
+### Concurrent Migration Test
+
+Test that file locking prevents migration race conditions:
+
+```bash
+go run test_migration_race.go
+```
+
+This test launches 100 concurrent processes that all try to initialize the database simultaneously. With file locking, all 100 should succeed (100% success rate). Without file locking, most would fail with "Dirty database version" errors.

docs/database/testing/test_migration_race.go

@@ -0,0 +1,147 @@
+package main
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"os/exec"
+	"strings"
+	"sync"
+	"time"
+)
+
+const (
+	dbPath     = "/Users/bobby/.docker/mcp/mcp-toolkit.db"
+	iterations = 100
+)
+
+type result struct {
+	iteration int
+	success   bool
+	output    string
+	err       error
+	duration  time.Duration
+}
+
+func main() {
+	fmt.Printf("Starting migration race test with %d concurrent iterations...\n", iterations)
+
+	// Delete the database once at the beginning
+	fmt.Printf("Deleting database at %s...\n", dbPath)
+	if err := os.Remove(dbPath); err != nil && !os.IsNotExist(err) {
+		fmt.Printf("Warning: failed to remove db: %v\n", err)
+	} else {
+		fmt.Println("Database deleted successfully")
+	}
+
+	fmt.Println("\nLaunching all processes simultaneously...")
+
+	var wg sync.WaitGroup
+	results := make(chan result, iterations)
+
+	startTime := time.Now()
+
+	// Launch all iterations concurrently - they will all race to initialize the database
+	for i := range iterations {
+		wg.Add(1)
+		go func(iterNum int) {
+			defer wg.Done()
+
+			iterStart := time.Now()
+
+			// Run docker mcp profile list - all will try to initialize/migrate at once
+			ctx := context.Background()
+			cmd := exec.CommandContext(ctx, "docker", "mcp", "profile", "list")
+			output, err := cmd.CombinedOutput()
+
+			results <- result{
+				iteration: iterNum,
+				success:   err == nil,
+				output:    string(output),
+				err:       err,
+				duration:  time.Since(iterStart),
+			}
+		}(i)
+	}
+
+	// Wait for all goroutines to complete
+	go func() {
+		wg.Wait()
+		close(results)
+	}()
+
+	// Collect and analyze results
+	var (
+		successCount    int
+		failureCount    int
+		migrationErrors int
+		dirtyDBErrors   int
+	)
+
+	fmt.Println("Results:")
+	fmt.Println("--------")
+
+	for res := range results {
+		if res.success {
+			successCount++
+			fmt.Printf("[%3d] ✓ Success (took %v)\n", res.iteration, res.duration)
+		} else {
+			failureCount++
+			fmt.Printf("[%3d] ✗ FAILED (took %v)\n", res.iteration, res.duration)
+			fmt.Printf("      Error: %v\n", res.err)
+
+			if res.output != "" {
+				fmt.Printf("      Output: %s\n", res.output)
+			}
+
+			// Check for specific error patterns
+			outputStr := res.output
+			if res.err != nil {
+				outputStr += res.err.Error()
+			}
+
+			if containsStr(outputStr, "failed to run migrations") {
+				migrationErrors++
+			}
+			if containsStr(outputStr, "Dirty database version") {
+				dirtyDBErrors++
+			}
+		}
+	}
+
+	totalDuration := time.Since(startTime)
+
+	fmt.Println("\n" + strings.Repeat("=", 60))
+	fmt.Println("Summary:")
+	fmt.Println(strings.Repeat("=", 60))
+	fmt.Printf("Total iterations:      %d\n", iterations)
+	fmt.Printf("Successful:            %d (%.1f%%)\n", successCount, float64(successCount)/float64(iterations)*100)
+	fmt.Printf("Failed:                %d (%.1f%%)\n", failureCount, float64(failureCount)/float64(iterations)*100)
+	fmt.Printf("Migration errors:      %d\n", migrationErrors)
+	fmt.Printf("Dirty DB errors:       %d\n", dirtyDBErrors)
+	fmt.Printf("Total time:            %v\n", totalDuration)
+	fmt.Printf("Avg time per iter:     %v\n", totalDuration/time.Duration(iterations))
+
+	if dirtyDBErrors > 0 {
+		fmt.Println("\n🎯 SUCCESS! Reproduced the 'Dirty database version' error!")
+	} else if migrationErrors > 0 {
+		fmt.Println("\n⚠️  Found migration errors but not the specific 'Dirty database' error")
+	} else if failureCount > 0 {
+		fmt.Println("\n⚠️  Found failures but not migration-related")
+	} else {
+		fmt.Println("\n✓ All iterations succeeded (could not reproduce the error)")
+	}
+}
+
+func containsStr(s, substr string) bool {
+	return len(s) >= len(substr) && (s == substr || len(s) > len(substr) && contains(s, substr))
+}
+
+func contains(s, substr string) bool {
+	for i := 0; i <= len(s)-len(substr); i++ {
+		if s[i:i+len(substr)] == substr {
+			return true
+		}
+	}
+	return false
+}

go.mod

@@ -15,6 +15,7 @@ require (
 	github.com/dop251/goja v0.0.0-20251008123653-cf18d89f3cf6
 	github.com/fsnotify/fsnotify v1.9.0
 	github.com/go-playground/validator/v10 v10.28.0
+	github.com/gofrs/flock v0.13.0
 	github.com/golang-migrate/migrate/v4 v4.19.0
 	github.com/google/go-containerregistry v0.20.6
 	github.com/google/jsonschema-go v0.3.0

go.sum

@@ -366,6 +366,8 @@ github.com/goccy/go-yaml v1.18.0/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7Lk
 github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2 h1:ZpnhV/YsD2/4cESfV5+Hoeu/iUR3ruzNvZ+yQfO03a0=
 github.com/godbus/dbus/v5 v5.1.0 h1:4KLkAxT3aOY8Li4FRJe/KvhoNFFxo0m6fNuFUO8QJUk=
 github.com/godbus/dbus/v5 v5.1.0/go.mod h1:xhWf0FNVPg57R7Z0UbKHbJfkEywrmjJnf7w5xrFpKfA=
+github.com/gofrs/flock v0.13.0 h1:95JolYOvGMqeH31+FC7D2+uULf6mG61mEZ/A8dRYMzw=
+github.com/gofrs/flock v0.13.0/go.mod h1:jxeyy9R1auM5S6JYDBhDt+E2TCo7DkratH4Pgi8P+Z0=
 github.com/gogo/protobuf v1.0.0/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
 github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
 github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=

pkg/db/db.go

@@ -1,6 +1,7 @@
 package db
 
 import (
+	"context"
 	"database/sql"
 	"embed"
 	"errors"
@@ -9,13 +10,13 @@ import (
 	"path/filepath"
 	"time"
 
+	"github.com/gofrs/flock"
 	"github.com/golang-migrate/migrate/v4"
 	msqlite "github.com/golang-migrate/migrate/v4/database/sqlite"
 	"github.com/golang-migrate/migrate/v4/source/iofs"
 	"github.com/jmoiron/sqlx"
 
 	"github.com/docker/mcp-gateway/pkg/log"
-	"github.com/docker/mcp-gateway/pkg/retry"
 	"github.com/docker/mcp-gateway/pkg/user"
 
 	// This enables to sqlite driver
@@ -94,18 +95,36 @@ func New(opts ...Option) (DAO, error) {
 		return nil, err
 	}
 
-	// Migrations are transactional for individual migrations, not for the entire migration process
-	// A race condition can happen if two instances of the CLI try to run migrations at the same time
-	// This doesn't harm the state of the database, but the second process will fail with an error
-	// We work around this by retrying the migration up to 5 times.
-	err = retry.Retry(5, 300*time.Millisecond, func() error {
-		err := mig.Up()
-		if err != nil && !errors.Is(err, migrate.ErrNoChange) {
-			return err
-		}
-		return nil
-	})
+	// Use file locking to prevent concurrent migrations across processes
+	// This ensures only one process can run migrations at a time, preventing
+	// "Dirty database version" errors when multiple processes start simultaneously.
+	// See docs/database/README.md
+	//
+	// 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")
+	fileLock := flock.New(lockFile)
+
+	// Try to acquire the lock with a 5 second timeout
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+
+	locked, err := fileLock.TryLockContext(ctx, 100*time.Millisecond)
 	if err != nil {
+		return nil, fmt.Errorf("failed to acquire migration lock: %w", err)
+	}
+	if !locked {
+		return nil, 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)
 	}