fix(store): write transactions take the lock at BEGIN, not partway through
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 <noreply@anthropic.com>
This commit is contained in:
committed by
Kristian Wind
co-authored by
Claude Opus 5
parent
2a3367bfe0
commit
40a679edb6
@@ -38,6 +38,20 @@ func Open(path string) (*DB, error) {
|
|||||||
"_pragma=busy_timeout(5000)",
|
"_pragma=busy_timeout(5000)",
|
||||||
"_pragma=foreign_keys(ON)",
|
"_pragma=foreign_keys(ON)",
|
||||||
"_pragma=synchronous(NORMAL)",
|
"_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
|
// Keep the temp b-trees FTS5 and ORDER BY build in memory rather than
|
||||||
// spilling them into the data volume.
|
// spilling them into the data volume.
|
||||||
"_pragma=temp_store(MEMORY)",
|
"_pragma=temp_store(MEMORY)",
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ package store
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
|
"fmt"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"testing"
|
"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
|
// 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
|
// code, so a task written by any path at all is searchable. That contract is worth
|
||||||
// a test of its own.
|
// a test of its own.
|
||||||
|
|||||||
Reference in New Issue
Block a user