diff --git a/pkg/hold/db/hold_db.go b/pkg/hold/db/hold_db.go index 3959e32..e43153f 100644 --- a/pkg/hold/db/hold_db.go +++ b/pkg/hold/db/hold_db.go @@ -5,7 +5,6 @@ import ( "fmt" "io" "log/slog" - "strings" "time" "github.com/tursodatabase/go-libsql" @@ -64,34 +63,19 @@ func OpenHoldDB(path string, cfg LibsqlConfig) (*HoldDB, error) { connector = conn slog.Info("Hold database opened in embedded replica mode", "path", path, "sync_url", cfg.SyncURL) } else { - // Local-only mode: plain file via libsql driver - dsn := path - if !strings.HasPrefix(path, "file:") && !strings.HasPrefix(path, ":memory:") { - dsn = "file:" + path - } + // Local-only mode: WAL journal mode plus a busy_timeout on *every* + // pooled connection. In embedded replica mode the remote server manages + // these settings and PRAGMA assignments are rejected as "unsupported + // statement" (observed with Bunny Database), so OpenLocalDB is only for + // the local-only branch. var err error - db, err = sql.Open("libsql", dsn) + db, err = OpenLocalDB(path) if err != nil { return nil, fmt.Errorf("failed to open hold database: %w", err) } slog.Info("Hold database opened in local-only mode", "path", path) } - // In local-only mode, configure WAL and busy_timeout locally. - // In embedded replica mode, the remote server manages these settings - // and PRAGMA assignments are rejected as "unsupported statement" - // (observed with Bunny Database). - if cfg.SyncURL == "" { - var journalMode string - if err := db.QueryRow("PRAGMA journal_mode = WAL").Scan(&journalMode); err != nil { - return nil, fmt.Errorf("failed to set journal mode: %w", err) - } - var busyTimeout int - if err := db.QueryRow("PRAGMA busy_timeout = 5000").Scan(&busyTimeout); err != nil { - return nil, fmt.Errorf("failed to set busy_timeout: %w", err) - } - } - // Foreign keys work in both modes if _, err := db.Exec("PRAGMA foreign_keys = ON"); err != nil { return nil, fmt.Errorf("failed to enable foreign keys: %w", err) diff --git a/pkg/hold/db/libsql_open.go b/pkg/hold/db/libsql_open.go new file mode 100644 index 0000000..76901b6 --- /dev/null +++ b/pkg/hold/db/libsql_open.go @@ -0,0 +1,131 @@ +package db + +import ( + "context" + "database/sql" + "database/sql/driver" + "fmt" + "log/slog" + "strings" + + _ "github.com/tursodatabase/go-libsql" +) + +// DefaultBusyTimeoutMs is how long a connection waits for a lock before giving +// up with SQLITE_BUSY. Matches the appview and labeler databases. +const DefaultBusyTimeoutMs = 5000 + +// localDSN normalizes a filesystem path into a libsql DSN. +func localDSN(path string) string { + if strings.HasPrefix(path, ":memory:") || strings.HasPrefix(path, "file:") { + return path + } + return "file:" + path +} + +// OpenLocalDB opens a local (non-replica) libsql database with WAL journal mode +// and PRAGMA busy_timeout applied to every pooled connection. +// +// Every subsystem that opens a hold database file directly must go through +// this. database/sql opens connections lazily and on demand, and SQLite's +// busy_timeout is per-connection, so running the PRAGMA once against the pool +// only configures whichever connection happened to serve it. Every other +// connection commits with busy_timeout=0 and fails immediately the moment +// another writer holds the lock. +// +// ":memory:" databases are returned unwrapped: libsql gives each connection its +// own private in-memory database, so there is no cross-connection lock to wait +// on and WAL is meaningless. +func OpenLocalDB(path string) (*sql.DB, error) { + dsn := localDSN(path) + + if strings.HasPrefix(dsn, ":memory:") { + return sql.Open("libsql", dsn) + } + + base, err := openLibsqlLocalConnector(dsn) + if err != nil { + return nil, err + } + db := sql.OpenDB(&busyTimeoutConnector{base: base, timeoutMs: DefaultBusyTimeoutMs}) + + if err := applyWAL(db, dsn); err != nil { + _ = db.Close() + return nil, err + } + return db, nil +} + +// applyWAL puts the database file into WAL journal mode. Unlike busy_timeout, +// journal mode is a property of the file itself, so one call covers every +// connection and every other process that opens the same file. +func applyWAL(db *sql.DB, dsn string) error { + var journalMode string + // libsql treats PRAGMA assignments as queries that return a row. + if err := db.QueryRow("PRAGMA journal_mode = WAL").Scan(&journalMode); err != nil { + return fmt.Errorf("failed to set journal mode: %w", err) + } + if !strings.EqualFold(journalMode, "wal") { + // Some filesystems (notably network mounts) cannot support WAL. Carry + // on rather than refusing to boot: busy_timeout still applies, and the + // rollback journal is correct, just less concurrent. + slog.Warn("Database did not accept WAL journal mode", "dsn", dsn, "journal_mode", journalMode) + } + return nil +} + +// openLibsqlLocalConnector returns a driver.Connector for a local libsql DSN. +// go-libsql exports NewEmbeddedReplicaConnector for replica mode but no public +// constructor for local files, so we obtain the driver via a probe sql.Open +// (which is lazy and opens no connection) and ask it for a Connector. +func openLibsqlLocalConnector(dsn string) (driver.Connector, error) { + probe, err := sql.Open("libsql", dsn) + if err != nil { + return nil, fmt.Errorf("probe libsql driver: %w", err) + } + drv := probe.Driver() + _ = probe.Close() + + dctx, ok := drv.(driver.DriverContext) + if !ok { + return nil, fmt.Errorf("libsql driver does not implement driver.DriverContext") + } + return dctx.OpenConnector(dsn) +} + +// busyTimeoutConnector wraps a driver.Connector and runs PRAGMA busy_timeout on +// every newly opened connection. SQLite's busy_timeout is per-connection, so +// this is the only way to ensure every conn in the pool waits on lock +// contention instead of returning SQLITE_BUSY immediately. +type busyTimeoutConnector struct { + base driver.Connector + timeoutMs int +} + +func (c *busyTimeoutConnector) Connect(ctx context.Context) (driver.Conn, error) { + conn, err := c.base.Connect(ctx) + if err != nil { + return nil, err + } + + // libsql treats PRAGMA assignments as queries that return a row, so we + // must use QueryerContext rather than ExecerContext. + queryer, ok := conn.(driver.QueryerContext) + if !ok { + _ = conn.Close() + return nil, fmt.Errorf("libsql conn does not support QueryerContext") + } + + rows, err := queryer.QueryContext(ctx, fmt.Sprintf("PRAGMA busy_timeout = %d", c.timeoutMs), nil) + if err != nil { + _ = conn.Close() + return nil, fmt.Errorf("set busy_timeout on new conn: %w", err) + } + _ = rows.Close() + + return conn, nil +} + +func (c *busyTimeoutConnector) Driver() driver.Driver { + return c.base.Driver() +} diff --git a/pkg/hold/db/libsql_open_test.go b/pkg/hold/db/libsql_open_test.go new file mode 100644 index 0000000..48bfa14 --- /dev/null +++ b/pkg/hold/db/libsql_open_test.go @@ -0,0 +1,93 @@ +package db + +import ( + "context" + "database/sql" + "path/filepath" + "testing" +) + +// assertPoolSettings checks journal mode and busy_timeout on several +// simultaneously held connections. Holding them at once forces database/sql to +// open distinct connections: busy_timeout is per-connection, so a regression to +// a one-shot "PRAGMA busy_timeout" on the pool shows up on conn 1 onward. +func assertPoolSettings(t *testing.T, label string, db *sql.DB, wantJournal string, wantBusy int) { + t.Helper() + ctx := context.Background() + + const n = 4 + conns := make([]*sql.Conn, 0, n) + defer func() { + for _, c := range conns { + _ = c.Close() + } + }() + for i := 0; i < n; i++ { + c, err := db.Conn(ctx) + if err != nil { + t.Fatalf("%s: open conn %d: %v", label, i, err) + } + conns = append(conns, c) + } + + for i, c := range conns { + var journal string + if err := c.QueryRowContext(ctx, "PRAGMA journal_mode").Scan(&journal); err != nil { + t.Fatalf("%s conn %d: read journal_mode: %v", label, i, err) + } + if journal != wantJournal { + t.Errorf("%s conn %d: journal_mode = %q, want %q", label, i, journal, wantJournal) + } + + var busy int + if err := c.QueryRowContext(ctx, "PRAGMA busy_timeout").Scan(&busy); err != nil { + t.Fatalf("%s conn %d: read busy_timeout: %v", label, i, err) + } + if busy != wantBusy { + t.Errorf("%s conn %d: busy_timeout = %d, want %d", label, i, busy, wantBusy) + } + } +} + +func TestOpenLocalDBSettingsOnEveryConnection(t *testing.T) { + path := filepath.Join(t.TempDir(), "db.sqlite3") + db, err := OpenLocalDB(path) + if err != nil { + t.Fatalf("OpenLocalDB: %v", err) + } + defer db.Close() + assertPoolSettings(t, "OpenLocalDB", db, "wal", DefaultBusyTimeoutMs) +} + +func TestOpenHoldDBSettingsOnEveryConnection(t *testing.T) { + path := filepath.Join(t.TempDir(), "db.sqlite3") + h, err := OpenHoldDB(path, LibsqlConfig{}) + if err != nil { + t.Fatalf("OpenHoldDB: %v", err) + } + defer h.Close() + assertPoolSettings(t, "OpenHoldDB", h.DB, "wal", DefaultBusyTimeoutMs) +} + +func TestCarstoreSettingsOnEveryConnection(t *testing.T) { + sqs, err := NewSqliteStore(t.TempDir()) + if err != nil { + t.Fatalf("NewSqliteStore: %v", err) + } + defer sqs.Close() + assertPoolSettings(t, "carstore", sqs.DB(), "wal", DefaultBusyTimeoutMs) +} + +// TestOpenLocalDBMemory documents that ":memory:" is left unwrapped: libsql +// gives each connection its own private database, so there is no shared lock to +// wait on and WAL does not apply. +func TestOpenLocalDBMemory(t *testing.T) { + db, err := OpenLocalDB(":memory:") + if err != nil { + t.Fatalf("OpenLocalDB(:memory:): %v", err) + } + defer db.Close() + if err := db.Ping(); err != nil { + t.Fatalf("ping in-memory db: %v", err) + } +} diff --git a/pkg/hold/db/sqlite_store.go b/pkg/hold/db/sqlite_store.go index ae08a76..84984a2 100644 --- a/pkg/hold/db/sqlite_store.go +++ b/pkg/hold/db/sqlite_store.go @@ -23,7 +23,6 @@ import ( "log/slog" "os" "path/filepath" - "strings" "go.opentelemetry.io/otel/attribute" @@ -106,15 +105,12 @@ func (sqs *SQLiteStore) Open(path string) error { } sqs.log.Debug("open db", "path", path) - // Build DSN for go-libsql - dsn := path - if path == ":memory:" { - dsn = ":memory:" - } else if !strings.HasPrefix(path, "file:") { - dsn = "file:" + path - } - - db, err := sql.Open("libsql", dsn) + // OpenLocalDB applies WAL journal mode and a per-connection busy_timeout. + // Without them, two concurrent writers (the carstore pool serves one + // connection per concurrent caller, and the records index, events and scan + // broadcaster may share or shadow this same file) fail their COMMIT with + // "database is locked" instead of waiting for the lock. + db, err := OpenLocalDB(path) if err != nil { return fmt.Errorf("%s: sqlite could not open, %w", path, err) } @@ -466,6 +462,13 @@ func (sqs *SQLiteStore) Close() error { return nil } +// DB returns the underlying database handle so callers that need a second +// logical table on the same file (the records index, for one) can share this +// pool instead of opening the file again. +func (sqs *SQLiteStore) DB() *sql.DB { + return sqs.db +} + func (sqs *SQLiteStore) getBlock(ctx context.Context, user models.Uid, bcid cid.Cid) (blockformat.Block, error) { tx, err := sqs.db.BeginTx(ctx, &txReadOnly) if err != nil { diff --git a/pkg/hold/db/sqlite_store_contention_test.go b/pkg/hold/db/sqlite_store_contention_test.go new file mode 100644 index 0000000..3fc1d9c --- /dev/null +++ b/pkg/hold/db/sqlite_store_contention_test.go @@ -0,0 +1,160 @@ +package db + +import ( + "context" + "fmt" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/bluesky-social/indigo/models" + blockformat "github.com/ipfs/go-block-format" + "github.com/ipfs/go-cid" +) + +// isLockErr reports whether err is a SQLite busy/locked error rather than a +// logic error. Contention tests must fail on lock errors specifically, so a +// typo cannot masquerade as a reproduction. +func isLockErr(err error) bool { + if err == nil { + return false + } + s := strings.ToLower(err.Error()) + return strings.Contains(s, "database is locked") || + strings.Contains(s, "database table is locked") || + strings.Contains(s, "sqlite_busy") || + strings.Contains(s, "busy") +} + +func makeBlocks(t *testing.T, n int, salt string) (cid.Cid, map[cid.Cid]blockformat.Block) { + t.Helper() + blks := make(map[cid.Cid]blockformat.Block, n) + var root cid.Cid + for i := 0; i < n; i++ { + b := blockformat.NewBlock([]byte(fmt.Sprintf("%s-block-%d-%s", salt, i, strings.Repeat("x", 2048)))) + blks[b.Cid()] = b + if i == 0 { + root = b.Cid() + } + } + return root, blks +} + +// writeShards hammers the carstore with concurrent shard writes and reports any +// lock error it hits. This is the exact path every hold record write takes. +func writeShards(t *testing.T, sqs *SQLiteStore, writers, iters int) error { + t.Helper() + ctx := context.Background() + var wg sync.WaitGroup + var mu sync.Mutex + var firstErr error + + for w := 0; w < writers; w++ { + wg.Add(1) + go func(w int) { + defer wg.Done() + for i := 0; i < iters; i++ { + salt := fmt.Sprintf("w%d-i%d", w, i) + root, blks := makeBlocks(t, 12, salt) + rev := fmt.Sprintf("rev-%03d-%03d", w, i) + _, err := sqs.writeNewShard(ctx, root, rev, models.Uid(1), i, blks, nil) + if err != nil { + mu.Lock() + if firstErr == nil { + firstErr = err + } + mu.Unlock() + return + } + } + }(w) + } + wg.Wait() + return firstErr +} + +// TestCarstoreConcurrentWritesSharedPool reproduces the production topology: +// one *sql.DB pool (OpenHoldDB) shared by the carstore, records index, events +// and scan broadcaster. database/sql opens a fresh connection per concurrent +// caller, and busy_timeout is per-connection, so any connection beyond the +// first commits with busy_timeout=0 and fails immediately under contention. +func TestCarstoreConcurrentWritesSharedPool(t *testing.T) { + path := filepath.Join(t.TempDir(), "db.sqlite3") + h, err := OpenHoldDB(path, LibsqlConfig{}) + if err != nil { + t.Fatalf("OpenHoldDB: %v", err) + } + defer h.Close() + + sqs, err := NewSQLiteStoreWithDB(path, h.DB) + if err != nil { + t.Fatalf("NewSQLiteStoreWithDB: %v", err) + } + + if err := writeShards(t, sqs, 8, 25); err != nil { + if isLockErr(err) { + t.Fatalf("shared-pool carstore write hit a lock error: %v", err) + } + t.Fatalf("shared-pool carstore write failed: %v", err) + } +} + +// TestCarstoreConcurrentWritesTwoOpeners covers the topology where a second +// subsystem (the scan broadcaster or the event broadcaster, when they are not +// handed the shared pool) opens