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
@@ -90,6 +96,33 @@ func (database *Database) connect(ctx context.Context) error {
 		dbURL = config.DefaultDBURL
 	}
 
+	// SQLite keeps the foreign keys setting per connection. The driver
+	// runs each _pragma parameter on every connection it opens. So the
+	// one added below also covers the connection the pool opens after
+	// 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 hasQuery {
+		separator = "&"
+	}
+
+	dbURL += separator + "_pragma=foreign_keys(1)"
+
 	database.log.Info(
 		"connecting to database", "url", dbURL,
 	)
@@ -109,13 +142,6 @@ func (database *Database) connect(ctx context.Context) error {
 	database.conn = conn
 	database.log.Info("database connected")
 
-	_, err = database.conn.ExecContext(
-		ctx, "PRAGMA foreign_keys = ON",
-	)
-	if err != nil {
-		return fmt.Errorf("enable foreign keys: %w", err)
-	}
-
 	return database.runMigrations(ctx)
 }
 

internal/db/db_test.go

@@ -1,9 +1,13 @@
 package db_test
 
 import (
+	"context"
+	"errors"
+	"net/url"
 	"path/filepath"
 	"strings"
 	"testing"
+	"time"
 
 	"go.uber.org/fx/fxtest"
 	"sneak.berlin/go/neoirc/internal/config"
@@ -14,6 +18,16 @@ import (
 
 const wantBusyTimeoutMS = 5000
 
+const (
+	// endlessQuery counts the rows of a recursive query that never
+	// stops, so it runs until its context ends.
+	endlessQuery = `WITH RECURSIVE n(x) AS
+		(SELECT 1 UNION ALL SELECT x + 1 FROM n)
+		SELECT COUNT(*) FROM n`
+
+	endlessQueryTimeout = 100 * time.Millisecond
+)
+
 // TestDefaultDBURLSetsWALAndBusyTimeout opens a file database
 // with the default URL's parameters the way the server does,
 // and checks the journal mode and busy timeout it ends up with.
@@ -73,3 +87,145 @@ 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. A TEMP table lasts only as long as its connection, so
+// 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()
+
+	_, defaultParams, _ := strings.Cut(config.DefaultDBURL, "?")
+
+	urlParams := map[string]string{
+		"default parameters": "?" + defaultParams,
+		"no parameters":      "",
+	}
+
+	for name, params := range urlParams {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			dbURL := "file://" +
+				filepath.Join(t.TempDir(), "state.db") + params
+
+			database, err := startDatabase(t, dbURL)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			_, err = database.GetDB().ExecContext(
+				t.Context(), "CREATE TEMP TABLE first_connection (x)",
+			)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			ctx, cancel := context.WithTimeout(
+				t.Context(), endlessQueryTimeout,
+			)
+			defer cancel()
+
+			_, err = database.GetDB().ExecContext(ctx, endlessQuery)
+			if !errors.Is(err, context.DeadlineExceeded) {
+				t.Fatalf("endless query: err = %v", err)
+			}
+
+			tempTables := queryInt(
+				t, database, "SELECT COUNT(*) FROM sqlite_temp_schema",
+			)
+			if tempTables != 0 {
+				t.Fatal("the pool kept its first connection")
+			}
+
+			migrations := queryInt(
+				t, database, "SELECT COUNT(*) FROM schema_migrations",
+			)
+			if migrations == 0 {
+				t.Error("schema_migrations is empty")
+			}
+
+			foreignKeys := queryInt(t, database, "PRAGMA foreign_keys")
+			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
+}