From 40a679edb6ba3e10c6258941f0843cc88b42db08 Mon Sep 17 00:00:00 2001 From: Kristian Wind Date: Mon, 17 Aug 2026 10:38:55 +0200 Subject: [PATCH] fix(store): write transactions take the lock at BEGIN, not partway through MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two people saving at the same moment got a 500 after two milliseconds. The DSN has set busy_timeout since the first commit, and it covered none of the contention it looks like it covers. With a plain BEGIN a write transaction starts as a reader and asks for the write lock at its first UPDATE — and SQLite refuses that upgrade with SQLITE_BUSY immediately, without applying busy_timeout to it. Backing off cannot help: the transaction is already holding a read snapshot the other writer is invalidating. _txlock=immediate takes the lock at BEGIN instead, where the timeout does apply and the second writer waits its turn. The driver leaves read-only transactions deferred, so readers still run concurrently, and every one of the twenty callers of db.Tx is a write. Found by opening a task and saving a description, a sub-task and a comment at once — three writes, one of which came back a 500. The test fails without the flag. Co-Authored-By: Claude Opus 5 --- internal/store/store.go | 14 +++++++++++ internal/store/store_test.go | 46 ++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/internal/store/store.go b/internal/store/store.go index 7ef6e78..1f42b19 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -38,6 +38,20 @@ func Open(path string) (*DB, error) { "_pragma=busy_timeout(5000)", "_pragma=foreign_keys(ON)", "_pragma=synchronous(NORMAL)", + // Every write transaction takes the write lock up front. + // + // Without this, `BEGIN` is deferred: a transaction starts as a reader and + // asks for the write lock at its first UPDATE. SQLite refuses that upgrade + // with SQLITE_BUSY *immediately* and does not apply busy_timeout to it — + // backing off cannot help, because the transaction is already holding a + // read snapshot that the other writer is invalidating. So the timeout above + // looks like it covers writer contention and covers none of it: two people + // saving at the same moment get a 500 in two milliseconds. + // + // `immediate` takes the lock at BEGIN instead, where busy_timeout does + // apply and the second writer waits its turn. The driver leaves read-only + // transactions deferred, so readers still run concurrently. + "_txlock=immediate", // Keep the temp b-trees FTS5 and ORDER BY build in memory rather than // spilling them into the data volume. "_pragma=temp_store(MEMORY)", diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 29450df..0013413 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -2,6 +2,7 @@ package store import ( "context" + "fmt" "path/filepath" "testing" ) @@ -95,6 +96,51 @@ func TestPragmasAreSet(t *testing.T) { } } +// Two people saving at the same moment must both succeed. +// +// This is a regression test for a specific and badly-behaved failure: with the +// default deferred BEGIN, a write transaction starts as a reader and asks for the +// write lock partway through. SQLite refuses that upgrade with SQLITE_BUSY at once +// and does *not* apply busy_timeout to it — so the contention surfaced as a 500 +// two milliseconds after the request arrived, while every single-threaded test +// passed. `_txlock=immediate` is what makes the timeout actually cover writers. +func TestConcurrentWritersDoNotHitSQLiteBusy(t *testing.T) { + db := openTest(t) + ctx := context.Background() + seedTaskFixtures(t, db) + + const writers = 8 + errs := make(chan error, writers) + + start := make(chan struct{}) + for i := range writers { + go func(i int) { + <-start // all of them ask for the write lock at once + errs <- db.CreateTask(ctx, &Task{ + ProjectID: "p1", + Content: fmt.Sprintf("opgave %d", i), + CreatedBy: "u1", + }, nil) + }(i) + } + close(start) + + for range writers { + if err := <-errs; err != nil { + t.Fatalf("concurrent write failed: %v", err) + } + } + + var n int + if err := db.QueryRowContext(ctx, + `SELECT count(*) FROM tasks WHERE project_id = 'p1'`).Scan(&n); err != nil { + t.Fatal(err) + } + if n != writers { + t.Errorf("wrote %d tasks, want %d", n, writers) + } +} + // The FTS index is kept in step with `tasks` by triggers rather than by application // code, so a task written by any path at all is searchable. That contract is worth // a test of its own.