Use immediate rather than deferred transaction in sqlitemigration ensureAppID (#98)

Co-authored-by: Ross Light <ross@zombiezen.com>

Author: bradenrayhorn

CHANGELOG.md

@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Address low-frequency errors with concurrent use of `sqlitemigration`
+  ([#99](https://github.com/zombiezen/go-sqlite/issues/99)).
 - The error returned from `sqlitex.NewPool`
   when trying to open an in-memory database
   now gives correct advice

sqlitemigration/sqlitemigration.go

@@ -353,7 +353,19 @@ func rollback(conn *sqlite.Conn) {
 }
 
 func ensureAppID(conn *sqlite.Conn, wantAppID int32) (schemaVersion int32, err error) {
-	defer sqlitex.Save(conn)(&err)
+	// This transaction will later be upgraded to a write transaction. If at the point of upgrading
+	// to a write transaction, the database is locked, SQLite will fail immediately with
+	// SQLITE_BUSY and the busy timeout will have no effect, causing the pool to fail.
+	//
+	// If we use an immediate transaction, telling SQLite this is a write transaction, SQLite
+	// will attempt to lock the database immediately. If a lock cannot be acquired, the busy
+	// timeout is used allowing the transaction to wait until it can get a lock, thus allowing the
+	// pool to start successfully.
+	end, err := sqlitex.ImmediateTransaction(conn)
+	if err != nil {
+		return 0, err
+	}
+	defer end(&err)
 
 	var hasSchema bool
 	err = sqlitex.ExecuteTransient(conn, "VALUES ((SELECT COUNT(*) FROM sqlite_master) > 0);", &sqlitex.ExecOptions{

sqlitemigration/sqlitemigration_test.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"fmt"
 	"path/filepath"
+	"sync"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -917,6 +918,63 @@ func TestMigrate(t *testing.T) {
 			t.Error("Foreign keys were disabled after migration")
 		}
 	})
+
+	t.Run("Concurrent", func(t *testing.T) {
+		ctx := context.Background()
+		dbPath := filepath.Join(t.TempDir(), "concurrent.db")
+		schema := Schema{
+			AppID: 0xedbeef,
+			Migrations: []string{
+				`create table foo ( id integer primary key not null );`,
+			},
+		}
+
+		// Attempt to perform migrations while writing.
+		var wg sync.WaitGroup
+		defer wg.Wait()
+		const numConcurrent = 5
+		wg.Add(numConcurrent)
+		for i := 0; i < numConcurrent; i++ {
+			go func(i int) {
+				defer wg.Done()
+
+				conn, err := sqlite.OpenConn(dbPath, sqlite.OpenReadWrite, sqlite.OpenCreate)
+				if err != nil {
+					t.Error(err)
+					return
+				}
+				defer func() {
+					if err := conn.Close(); err != nil {
+						t.Error(err)
+					}
+				}()
+				if err := Migrate(ctx, conn, schema); err != nil {
+					t.Error("Migrate:", err)
+				}
+			}(i)
+		}
+
+		// Migrate and issue writes on one connection.
+		conn, err := sqlite.OpenConn(dbPath, sqlite.OpenReadWrite, sqlite.OpenCreate)
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer func() {
+			if err := conn.Close(); err != nil {
+				t.Error(err)
+			}
+		}()
+		if err := Migrate(ctx, conn, schema); err != nil {
+			t.Fatal("Migrate:", err)
+		}
+		for i := 0; i < 150; i++ {
+			if err := sqlitex.Execute(conn, "insert into foo values (?)", &sqlitex.ExecOptions{
+				Args: []any{i},
+			}); err != nil {
+				t.Error("insert query:", err)
+			}
+		}
+	})
 }
 
 // withTestConn makes an independent connection to the given database.