From 625fd42ace36691d918910c22871cd2fe4eaab0d Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 20:58:37 +0200 Subject: [PATCH] Wait on a busy SQLite database and turn on WAL mode (closes #198) Requests and the eviction pass write on separate connections, and with no busy timeout a write that met another one failed at once with "database is locked" and was lost. internal/database now adds _pragma=busy_timeout(5000) to every db_url, the default or one the operator sets, so such a write waits up to five seconds. The default db_url's _journal_mode=WAL is not a parameter the driver reads, so it is now _pragma=journal_mode(WAL). README.md and config.example.yml say what pixa adds to db_url. Model: opus-5-5 --- README.md | 6 + TODO.md | 6 + config.example.yml | 7 +- internal/config/config.go | 3 +- .../config/config_validation_internal_test.go | 36 ++++- .../concurrent_writes_internal_test.go | 153 ++++++++++++++++++ internal/database/database.go | 12 +- 7 files changed, 217 insertions(+), 6 deletions(-) create mode 100644 internal/database/concurrent_writes_internal_test.go diff --git a/README.md b/README.md index 75c0d1d..f2ee4e2 100644 --- a/README.md +++ b/README.md @@ -490,6 +490,12 @@ Key settings in more detail: waits for an upstream connection and for a processing slot (up to 10 seconds each), so keep it longer than `upstream_fetch_timeout` plus 20 seconds - `signing_key` — HMAC secret for URL signatures +- `db_url` — the SQLite database to open; omitted, it is + `file:/state.sqlite3?_pragma=journal_mode(WAL)`, which keeps the + database in WAL mode. pixa adds `_pragma=busy_timeout(5000)` to any `db_url`, + so a write that finds another in progress waits up to five seconds for it + instead of failing. WAL mode comes only from the URL: keep + `_pragma=journal_mode(WAL)` in one you set - `cache_max_bytes` — disk cache size limit in bytes; `0` disables the disk cache entirely; omitted defaults to 75% of the sum of the free space on the filesystem containing `/cache/` and the bytes of source and diff --git a/TODO.md b/TODO.md index 8c3c6df..89a78f1 100644 --- a/TODO.md +++ b/TODO.md @@ -31,6 +31,12 @@ P2: security: referer blacklist # Completed Steps +- 2026-10-04 SQLite writes no longer fail with "database is locked" (closes + #198): pixa adds `_pragma=busy_timeout(5000)` to every `db_url`, so a write + that finds another in progress on another connection waits up to five seconds + for it, and the default `db_url` turns on WAL mode with + `_pragma=journal_mode(WAL)`. The old default's `_journal_mode=WAL` is not a + parameter the driver reads, so the database was never in WAL mode. - 2026-10-04 `TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup` only passes through a periodic pass (closes #189): it slept for three eviction intervals before writing its file, and a startup pass still running diff --git a/config.example.yml b/config.example.yml index be42aa4..098d936 100644 --- a/config.example.yml +++ b/config.example.yml @@ -34,9 +34,10 @@ maintenance_mode: false state_dir: ./data # SQLite database URL (default: -# file:/state.sqlite3?_journal_mode=WAL). An empty value aborts -# startup; leave the key out to use the default. -# db_url: "file:./data/state.sqlite3?_journal_mode=WAL" +# file:/state.sqlite3?_pragma=journal_mode(WAL)). pixa adds +# _pragma=busy_timeout(5000) to it. An empty value aborts startup; leave the +# key out to use the default. +# db_url: "file:./data/state.sqlite3?_pragma=journal_mode(WAL)" # Image proxy settings # HMAC signing key for URL signatures (required, at least 32 characters) diff --git a/internal/config/config.go b/internal/config/config.go index a9f7b72..7160ca7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -320,7 +320,8 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { settingName(keyDBURL), errValueEmpty) } - c.DBURL = fmt.Sprintf("file:%s/state.sqlite3?_journal_mode=WAL", c.StateDir) + // The driver sets the journal mode only through a _pragma parameter. + c.DBURL = fmt.Sprintf("file:%s/state.sqlite3?_pragma=journal_mode(WAL)", c.StateDir) } if loader.err != nil { diff --git a/internal/config/config_validation_internal_test.go b/internal/config/config_validation_internal_test.go index 1800ab3..d3a3736 100644 --- a/internal/config/config_validation_internal_test.go +++ b/internal/config/config_validation_internal_test.go @@ -1,6 +1,7 @@ package config import ( + "database/sql" "log/slog" "os" "path/filepath" @@ -9,6 +10,8 @@ import ( "time" "git.eeqj.de/sneak/smartconfig" + + _ "modernc.org/sqlite" // SQLite driver registration ) // validTestSigningKey is a 32-character signing key that satisfies the @@ -94,12 +97,43 @@ func TestOmittedValuesUseDefaults(t *testing.T) { t.Errorf("AllowlistHosts = %v, want empty", c.AllowlistHosts) } - wantDBURL := "file:" + DefaultStateDir + "/state.sqlite3?_journal_mode=WAL" + wantDBURL := "file:" + DefaultStateDir + + "/state.sqlite3?_pragma=journal_mode(WAL)" if c.DBURL != wantDBURL { t.Errorf("DBURL = %q, want derived default %q", c.DBURL, wantDBURL) } } +// TestDefaultDBURLOpensTheDatabaseInWALMode opens the db_url derived from +// state_dir with the SQLite driver pixad uses and checks that the database +// is in WAL mode: the driver ignores any parameter it does not know. +func TestDefaultDBURLOpensTheDatabaseInWALMode(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine+"state_dir: "+t.TempDir()+"\n") + if err != nil { + t.Fatalf("config with only state_dir set should be valid, got: %v", err) + } + + db, err := sql.Open("sqlite", c.DBURL) + if err != nil { + t.Fatalf("failed to open %q: %v", c.DBURL, err) + } + + t.Cleanup(func() { _ = db.Close() }) + + var journalMode string + + err = db.QueryRowContext(t.Context(), "PRAGMA journal_mode").Scan(&journalMode) + if err != nil { + t.Fatalf("failed to read the journal mode of %q: %v", c.DBURL, err) + } + + if journalMode != "wal" { + t.Errorf("journal mode of %q = %q, want wal", c.DBURL, journalMode) + } +} + func TestExplicitValidValuesAreUsed(t *testing.T) { t.Parallel() diff --git a/internal/database/concurrent_writes_internal_test.go b/internal/database/concurrent_writes_internal_test.go new file mode 100644 index 0000000..3db4c4e --- /dev/null +++ b/internal/database/concurrent_writes_internal_test.go @@ -0,0 +1,153 @@ +package database + +import ( + "context" + "database/sql" + "fmt" + "log/slog" + "path/filepath" + "sync" + "testing" + + "sneak.berlin/go/pixa/internal/config" +) + +// TestConcurrentWritesAllSucceed opens a database the way pixad does and +// writes to it from several goroutines at once, so the writes run on +// separate connections, as one request's writes and the background eviction +// pass do. Every write must succeed, none failing with "database is locked", +// whether or not db_url already has parameters, and the parameters it has +// must still apply. +func TestConcurrentWritesAllSucceed(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + query string + wantJournalMode string + }{ + { + name: "db_url without parameters", + query: "", + wantJournalMode: "delete", + }, + { + name: "db_url with the WAL parameter", + query: "?_pragma=journal_mode(WAL)", + wantJournalMode: "wal", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + dbURL := "file:" + filepath.Join(t.TempDir(), "state.sqlite3") + tt.query + + d := &Database{ + log: slog.New(slog.DiscardHandler), + config: &config.Config{DBURL: dbURL}, + } + + err := d.connect(t.Context()) + if err != nil { + t.Fatalf("failed to connect to %q: %v", dbURL, err) + } + + t.Cleanup(func() { _ = d.db.Close() }) + + writeConcurrently(t, d.db) + + var journalMode string + + err = d.db.QueryRowContext(t.Context(), "PRAGMA journal_mode"). + Scan(&journalMode) + if err != nil { + t.Fatalf("failed to read the journal mode: %v", err) + } + + if journalMode != tt.wantJournalMode { + t.Errorf("journal mode = %q, want %q", journalMode, tt.wantJournalMode) + } + }) + } +} + +// writeConcurrently runs writeLikeOneRequest from several goroutines at once +// and checks that every write was made. +func writeConcurrently(t *testing.T, db *sql.DB) { + t.Helper() + + const ( + writers = 4 + requestsEach = 20 + totalRequests = writers * requestsEach + ) + + ctx := t.Context() + + var wg sync.WaitGroup + + for writer := range writers { + wg.Go(func() { + for request := range requestsEach { + key := fmt.Sprintf("%d-%d", writer, request) + + err := writeLikeOneRequest(ctx, db, key) + if err != nil { + t.Errorf("writer %d: %v", writer, err) + + return + } + } + }) + } + + wg.Wait() + + var hits, sources int + + err := db.QueryRowContext(ctx, ` + SELECT hit_count, (SELECT COUNT(*) FROM source_content) + FROM cache_stats WHERE id = 1 + `).Scan(&hits, &sources) + if err != nil { + t.Fatalf("failed to count the writes: %v", err) + } + + if hits != totalRequests || sources != totalRequests { + t.Errorf("hit_count = %d and %d source_content rows, want %d of each", + hits, sources, totalRequests) + } +} + +// writeLikeOneRequest makes the writes one request and the eviction pass +// make: it counts a cache hit, stores a source, records a transformed image +// and deletes that record again. +func writeLikeOneRequest(ctx context.Context, db *sql.DB, key string) error { + _, err := db.ExecContext(ctx, + `UPDATE cache_stats SET hit_count = hit_count + 1 WHERE id = 1`) + if err != nil { + return fmt.Errorf("counting a cache hit: %w", err) + } + + _, err = db.ExecContext(ctx, `INSERT INTO source_content + (content_hash, content_type, size_bytes) VALUES (?, 'image/png', 1)`, key) + if err != nil { + return fmt.Errorf("storing a source: %w", err) + } + + _, err = db.ExecContext(ctx, `INSERT INTO variant_content + (cache_key, size_bytes, content_type) VALUES (?, 1, 'image/png')`, key) + if err != nil { + return fmt.Errorf("recording a transformed image: %w", err) + } + + _, err = db.ExecContext(ctx, + `DELETE FROM variant_content WHERE cache_key = ?`, key) + if err != nil { + return fmt.Errorf("evicting a transformed image: %w", err) + } + + return nil +} diff --git a/internal/database/database.go b/internal/database/database.go index ab59f75..88d9840 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -243,7 +243,17 @@ func (s *Database) DB() *sql.DB { } func (s *Database) connect(ctx context.Context) error { - dbURL := s.config.DBURL + // Requests and the eviction pass write on separate connections. With + // a busy timeout, a write that finds another one in progress waits up + // to five seconds for it instead of failing at once with "database is + // locked". The driver runs each _pragma parameter on every connection + // it opens. + separator := "?" + if strings.Contains(s.config.DBURL, "?") { + separator = "&" + } + + dbURL := s.config.DBURL + separator + "_pragma=busy_timeout(5000)" s.log.Info("connecting to database", "url", dbURL)