Keep foreign keys on for every database connection (closes #207)

SQLite keeps the foreign keys setting per connection, and the server
turned it on once with a PRAGMA. A query cancelled while it runs makes
the pool replace its only connection, and the new one ran without
foreign keys until a restart.

connect now adds `_pragma=foreign_keys(1)` to the database URL. The
driver runs it on every connection it opens. The server refuses to
start when DBURL sets foreign_keys itself, because the driver sorts
the pragmas and that one could win.

One test cancels a query and checks, with a TEMP table, that the
connection was replaced and that foreign keys are on. Another checks
the startup error for several spellings.

Model: opus-5-5

README.md

@@ -2611,7 +2611,7 @@ listener.
 | Variable               | Type   | Default                                                                                | Description                                                                                                                                                                                                                                                                             |
 | ---------------------- | ------ | -------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
 | `PORT`                 | int    | `8080`                                                                                 | HTTP listen port                                                                                                                                                                                                                                                                        |
-| `DBURL`                | string | `file:///var/lib/neoirc/state.db?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)` | SQLite connection string. For file-based: `file:///path/to/db.db?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)`. For in-memory (testing): `file::memory:?cache=shared`. See [SQLite Considerations](#sqlite-considerations).                                                     |
+| `DBURL`                | string | `file:///var/lib/neoirc/state.db?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)` | SQLite connection string. For file-based: `file:///path/to/db.db?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)`. For in-memory (testing): `file::memory:?cache=shared`. It must not set `foreign_keys`. See [SQLite Considerations](#sqlite-considerations).                     |
 | `DEBUG`                | bool   | `false`                                                                                | Switch logging to debug level                                                                                                                                                                                                                                                           |
 | `MESSAGE_MAX_AGE`      | string | `720h`                                                                                 | Maximum age of messages as a Go duration string (e.g. `720h`, `24h`). Messages older than this are pruned. Default is 30 days.                                                                                                                                                          |
 | `SESSION_IDLE_TIMEOUT` | string | `720h`                                                                                 | Idle timeout as a Go duration string (e.g. `720h`, `24h`). Clients with no authenticated HTTP API request or IRC listener command for this long are removed, and a session left with none is expired and its nick released (see [Data Lifecycle](#data-lifecycle)). Default is 30 days. |

internal/db/db.go

@@ -5,9 +5,11 @@ import (
 	"context"
 	"database/sql"
 	"embed"
+	"errors"
 	"fmt"
 	"io/fs"
 	"log/slog"
+	"net/url"
 	"sort"
 	"strconv"
 	"strings"
@@ -22,6 +24,10 @@ import (
 
 const minMigrationParts = 2
 
+var errForeignKeysInDBURL = errors.New(
+	"DBURL must not set foreign_keys, which the server always turns on",
+)
+
 // SchemaFiles contains embedded SQL migration files.
 //
 //go:embed schema/*.sql
@@ -91,11 +97,27 @@ func (database *Database) connect(ctx context.Context) error {
 	}
 
 	// SQLite keeps the foreign keys setting per connection. The driver
-	// runs each _pragma parameter on every connection it opens. A
+	// runs each _pragma parameter on every connection it opens. So the
-	// PRAGMA run once would miss the connection the pool opens after a
+	// one added below also covers the connection the pool opens after
-	// cancelled query breaks the old one.
+	// a cancelled query breaks the old one. A foreign_keys pragma in
+	// DBURL could undo it, because the driver sorts the pragmas.
+	_, query, hasQuery := strings.Cut(dbURL, "?")
+
+	urlParams, err := url.ParseQuery(query)
+	if err != nil {
+		return fmt.Errorf("parse DBURL: %w", err)
+	}
+
+	for _, pragma := range urlParams["_pragma"] {
+		if strings.Contains(strings.ToLower(pragma), "foreign_keys") {
+			return fmt.Errorf(
+				"%w: _pragma=%s", errForeignKeysInDBURL, pragma,
+			)
+		}
+	}
+
 	separator := "?"
-	if strings.Contains(dbURL, "?") {
+	if hasQuery {
 		separator = "&"
 	}
 

internal/db/db_test.go

@@ -3,6 +3,7 @@ package db_test
 import (
 	"context"
 	"errors"
+	"net/url"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -89,8 +90,10 @@ func TestDefaultDBURLSetsWALAndBusyTimeout(t *testing.T) {
 
 // TestForeignKeysOnAfterCancelledQuery cancels a query while it
 // runs, which makes the pool close its only connection and open a
-// new one. It checks that the file database keeps its tables and
+// new one. A TEMP table lasts only as long as its connection, so
-// that the new connection has foreign keys on.
+// its absence shows the connection was replaced. The test then
+// checks that the file database keeps its tables and that the new
+// connection has foreign keys on.
 func TestForeignKeysOnAfterCancelledQuery(t *testing.T) {
 	t.Parallel()
 
@@ -108,26 +111,18 @@ func TestForeignKeysOnAfterCancelledQuery(t *testing.T) {
 			dbURL := "file://" +
 				filepath.Join(t.TempDir(), "state.db") + params
 
-			lifecycle := fxtest.NewLifecycle(t)
+			database, err := startDatabase(t, dbURL)
-
-			log, err := logger.New(lifecycle, logger.Params{
-				Globals: &globals.Globals{},
-			})
 			if err != nil {
 				t.Fatal(err)
 			}
 
-			database, err := db.New(lifecycle, db.Params{
+			_, err = database.GetDB().ExecContext(
-				Logger: log,
+				t.Context(), "CREATE TEMP TABLE first_connection (x)",
-				Config: &config.Config{DBURL: dbURL},
+			)
-			})
 			if err != nil {
 				t.Fatal(err)
 			}
 
-			lifecycle.RequireStart()
-			t.Cleanup(lifecycle.RequireStop)
-
 			ctx, cancel := context.WithTimeout(
 				t.Context(), endlessQueryTimeout,
 			)
@@ -138,32 +133,99 @@ func TestForeignKeysOnAfterCancelledQuery(t *testing.T) {
 				t.Fatalf("endless query: err = %v", err)
 			}
 
-			var migrations int
+			tempTables := queryInt(
-
+				t, database, "SELECT COUNT(*) FROM sqlite_temp_schema",
-			err = database.GetDB().QueryRowContext(
+			)
-				t.Context(),
+			if tempTables != 0 {
-				"SELECT COUNT(*) FROM schema_migrations",
+				t.Fatal("the pool kept its first connection")
-			).Scan(&migrations)
-			if err != nil {
-				t.Fatal(err)
 			}
 
+			migrations := queryInt(
+				t, database, "SELECT COUNT(*) FROM schema_migrations",
+			)
 			if migrations == 0 {
 				t.Error("schema_migrations is empty")
 			}
 
-			var foreignKeys int
+			foreignKeys := queryInt(t, database, "PRAGMA foreign_keys")
-
-			err = database.GetDB().QueryRowContext(
-				t.Context(), "PRAGMA foreign_keys",
-			).Scan(&foreignKeys)
-			if err != nil {
-				t.Fatal(err)
-			}
-
 			if foreignKeys != 1 {
 				t.Errorf("foreign_keys = %d, want 1", foreignKeys)
 			}
 		})
 	}
 }
+
+// TestDBURLSettingForeignKeysFailsStartup checks that the database
+// refuses to start when DBURL sets foreign_keys itself, however it
+// is spelled.
+func TestDBURLSettingForeignKeysFailsStartup(t *testing.T) {
+	t.Parallel()
+
+	pragmas := []string{
+		"foreign_keys(0)",
+		"foreign_keys(off)",
+		"foreign_keys=0",
+		"FOREIGN_KEYS(FALSE)",
+		" Foreign_Keys = 1",
+	}
+
+	for _, pragma := range pragmas {
+		t.Run(pragma, func(t *testing.T) {
+			t.Parallel()
+
+			dbURL := "file://" +
+				filepath.Join(t.TempDir(), "state.db") +
+				"?_pragma=" + url.QueryEscape(pragma)
+
+			_, err := startDatabase(t, dbURL)
+			if err == nil {
+				t.Fatal("the database started")
+			}
+
+			if !strings.Contains(err.Error(), "DBURL") {
+				t.Errorf("error %q does not name DBURL", err)
+			}
+		})
+	}
+}
+
+// startDatabase starts a Database for dbURL the way the server does.
+// It returns the error that stopped it from starting, if any.
+func startDatabase(t *testing.T, dbURL string) (*db.Database, error) {
+	t.Helper()
+
+	lifecycle := fxtest.NewLifecycle(t)
+
+	log, err := logger.New(lifecycle, logger.Params{
+		Globals: &globals.Globals{},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	database, err := db.New(lifecycle, db.Params{
+		Logger: log,
+		Config: &config.Config{DBURL: dbURL},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	t.Cleanup(lifecycle.RequireStop)
+
+	return database, lifecycle.Start(t.Context())
+}
+
+// queryInt runs a query that returns a single number.
+func queryInt(t *testing.T, database *db.Database, query string) int {
+	t.Helper()
+
+	var result int
+
+	err := database.GetDB().QueryRowContext(t.Context(), query).Scan(&result)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	return result
+}