From 11d0393704e5d6c5eddb6e6b21ca61dfad40464b Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 18:30:17 +0000 Subject: [PATCH 1/2] Test that concurrent SQLite writes succeed and the default db_url uses WAL Adds two tests that fail before the fix. One opens the database the way pixad does and makes writes like one request's and the eviction pass's from several goroutines at once; on separate connections with no busy timeout, some fail with "database is locked". The other opens the db_url derived from state_dir and checks the database is in WAL mode, which the current default's _journal_mode=WAL does not do because the driver ignores that parameter. The expected default db_url in TestOmittedValuesUseDefaults changes to the new one. Model: opus-5-5 --- .../config/config_validation_internal_test.go | 36 ++++- .../concurrent_writes_internal_test.go | 153 ++++++++++++++++++ 2 files changed, 188 insertions(+), 1 deletion(-) create mode 100644 internal/database/concurrent_writes_internal_test.go 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 +} -- 2.54.0 From c33713c85f1093dab05b70af8fef30d5860cbd5a Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 18:32:44 +0000 Subject: [PATCH 2/2] 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 ++- internal/database/database.go | 12 +++++++++++- 5 files changed, 29 insertions(+), 5 deletions(-) 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/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) -- 2.54.0