From df247fd39fda2545aa934b486de30c6bd7979024 Mon Sep 17 00:00:00 2001 From: Andrey Kumanyaev Date: Sat, 27 Jun 2026 10:46:52 +0200 Subject: [PATCH] fix(persistence): retry sidecar open on the rollback->WAL conversion BUSY race Concurrent first opens of a stale (rollback-journal) sidecar raced and one failed with "sidecar schema: begin: database is locked (5) (SQLITE_BUSY)". The DSN sets journal_mode=WAL, so the first openers convert the file from rollback to WAL, which takes a brief EXCLUSIVE lock. SQLite does not consult the busy handler for a journal-mode change, so busy_timeout is bypassed and the loser gets an immediate SQLITE_BUSY (observed in ~0.02s, far under the 5s timeout). _txlock=immediate + busy_timeout only serialise the post-conversion BEGIN IMMEDIATE write lock, not the conversion itself. Add a bounded application-level BUSY/LOCKED retry (withSidecarBusyRetry, typed detection via the modernc *sqlite.Error code) and wrap runBaseSchema and runMigrations in OpenSidecar with it. Both are idempotent (CREATE ... IF NOT EXISTS / user_version-gated), so retry is safe. Verified load-bearing: TestOpenSidecarConcurrentProcesses fails reliably without the change and passes under -race (and a 180-open stress) with it. --- internal/persistence/sidecar_migrate.go | 53 +++++++++++++++++++++++++ internal/persistence/sidecar_sqlite.go | 17 +++++--- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/internal/persistence/sidecar_migrate.go b/internal/persistence/sidecar_migrate.go index bddf8d00f..69724ad64 100644 --- a/internal/persistence/sidecar_migrate.go +++ b/internal/persistence/sidecar_migrate.go @@ -2,8 +2,12 @@ package persistence import ( "database/sql" + "errors" "fmt" "strings" + "time" + + sqlite "modernc.org/sqlite" ) // Sidecar schema migrations. @@ -58,6 +62,55 @@ func runMigrations(db *sql.DB) error { return nil } +// isSidecarBusyErr reports whether err is a SQLite BUSY/LOCKED result code +// from the modernc driver (matching extended codes by their base value). +func isSidecarBusyErr(err error) bool { + var se *sqlite.Error + if !errors.As(err, &se) { + return false + } + // Mask the high byte so extended codes (SQLITE_BUSY_SNAPSHOT, ...) match + // their base SQLITE_BUSY (5) / SQLITE_LOCKED (6). + switch se.Code() & 0xff { + case 5, 6: + return true + } + return false +} + +// withSidecarBusyRetry runs fn, retrying on a SQLite BUSY/LOCKED error with +// bounded exponential backoff. +// +// busy_timeout (set in the sidecar DSN) covers ordinary write-lock contention +// once the file is in WAL mode, but it does NOT cover the rollback-journal -> +// WAL conversion the very first opener performs: that conversion takes a brief +// EXCLUSIVE lock whose acquisition SQLite answers with an immediate, un-retried +// SQLITE_BUSY (the busy handler is not consulted for a journal-mode change) +// when another process has the file open. The daemon, every per-repo +// `gortex mcp` subprocess, and the CLI can all open a fresh/stale sidecar at +// the same instant, so a bounded application-level retry is required on top of +// busy_timeout. fn must be idempotent — runBaseSchema (CREATE ... IF NOT +// EXISTS) and runMigrations (user_version-gated) both are. +func withSidecarBusyRetry(fn func() error) error { + const ( + maxAttempts = 40 + baseDelay = 5 * time.Millisecond + maxDelay = 250 * time.Millisecond + ) + delay := baseDelay + var err error + for attempt := 0; attempt < maxAttempts; attempt++ { + if err = fn(); err == nil || !isSidecarBusyErr(err) { + return err + } + time.Sleep(delay) + if delay *= 2; delay > maxDelay { + delay = maxDelay + } + } + return err +} + // runBaseSchema applies the idempotent base shape (sidecarSchema) inside one // IMMEDIATE-locked transaction. The sidecar DSN's _txlock=immediate makes // db.Begin() emit BEGIN IMMEDIATE, so the reserved write lock is held from the diff --git a/internal/persistence/sidecar_sqlite.go b/internal/persistence/sidecar_sqlite.go index d87e55aa1..cac68ed3c 100644 --- a/internal/persistence/sidecar_sqlite.go +++ b/internal/persistence/sidecar_sqlite.go @@ -252,9 +252,13 @@ func OpenSidecar(path string) (*SidecarStore, error) { // takes a brief EXCLUSIVE lock to convert the file. Unlike the graph store // (one process), the sidecar is opened concurrently by the daemon, every // per-repo `gortex mcp` subprocess, and the CLI — so that conversion races. - // With busy_timeout set first the loser blocks and retries; set after - // journal_mode it would still be 0 during the conversion and fail - // immediately with SQLITE_BUSY. + // Ordering busy_timeout first installs the busy handler before any later + // pragma or BEGIN, so write-lock contention blocks-and-retries instead of + // failing fast (set after journal_mode the timeout would still be 0 during + // the conversion). The journal-mode CONVERSION itself is NOT covered by the + // busy handler — SQLite returns an immediate SQLITE_BUSY for a mode change + // while another connection has the file open — so withSidecarBusyRetry in + // OpenSidecar is the backstop for that residual race. // // _txlock=immediate makes db.Begin() emit BEGIN IMMEDIATE so a write // transaction takes the reserved lock at BEGIN. Both runBaseSchema (the @@ -276,7 +280,10 @@ func OpenSidecar(path string) (*SidecarStore, error) { if err != nil { return nil, fmt.Errorf("persistence: open sidecar: %w", err) } - if err := runBaseSchema(db); err != nil { + // busy_timeout does not cover the rollback->WAL conversion the first opener + // performs, so wrap the lock-taking schema + migration steps in a bounded + // BUSY/LOCKED retry (see withSidecarBusyRetry). Both steps are idempotent. + if err := withSidecarBusyRetry(func() error { return runBaseSchema(db) }); err != nil { _ = db.Close() return nil, fmt.Errorf("persistence: sidecar schema: %w", err) } @@ -284,7 +291,7 @@ func OpenSidecar(path string) (*SidecarStore, error) { // and column-dependent indexes that CREATE TABLE IF NOT EXISTS cannot add // to a pre-existing table. Forward-only, additive, and safe when several // gortex processes open the same file at once (see runMigrations). - if err := runMigrations(db); err != nil { + if err := withSidecarBusyRetry(func() error { return runMigrations(db) }); err != nil { _ = db.Close() return nil, err }