From f9ce38658b083387350d8e1cad3977d65044f6a5 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 7 Oct 2026 19:20:08 +0200 Subject: [PATCH] fix(store): one process-wide section for a query connection's first access On an idle WAL database (no connection open, -shm reset) the first read of a new connection runs WAL recovery: walIndexRecover rebuilds the shared wal-index header under SQLite's write lock. A second connection of the same process opening at that moment reads that header lock-free at the start of walTryBeginRead, before it takes its read lock. Across processes SQLite's file locks order the two; between the daemon's request threads nothing did. TSan reported exactly that pair three times in CI (2026-10-06 on #2458's tsan-arm leg, 2026-10-07 twice on #2348's macOS leg), from concurrent index_repository requests reading the project record through cbm_store_open_path_query. cbm_store_open_path_query now opens and probes the database (the plain read-only open, its first read, and the immutable fallback) inside SQLITE_MUTEX_STATIC_APP1, the process-wide mutex SQLite reserves for the application. The probe is one sqlite_master read, so the section is short; the queries that follow run outside it, as before. Regression test store_query_first_access_is_one_process_section: a test seam holds one opener inside the section, and sqlite3_mutex_try on the section's mutex must then report SQLITE_BUSY. It fails without the lock (try returns SQLITE_OK) and passes with it. SQLite's Windows build reports BUSY from sqlite3_mutex_try unconditionally, so the failing case is shown on POSIX; the locking code is the same on every platform. Signed-off-by: Martin Vogel --- src/store/store.c | 99 ++++++++++++++++++++++++----------- src/store/store.h | 7 +++ tests/test_store_checkpoint.c | 65 +++++++++++++++++++++++ 3 files changed, 140 insertions(+), 31 deletions(-) diff --git a/src/store/store.c b/src/store/store.c index 375834a25a..31737ea4f3 100644 --- a/src/store/store.c +++ b/src/store/store.c @@ -990,33 +990,24 @@ static bool build_immutable_uri(const char *path, char *out, size_t out_sz) { return true; } -cbm_store_t *cbm_store_open_path_query(const char *db_path) { - if (!db_path) { - return NULL; - } - - cbm_store_t *s = calloc(CBM_ALLOC_ONE, sizeof(cbm_store_t)); - if (!s) { - return NULL; - } - - /* Query tools open the project DB READ-ONLY: a read query must never - * mutate the DB (the previous READWRITE open + WAL write-pragmas did), - * and must work on a read-only DB file / filesystem. - * - * Try a plain READONLY open first — on a normal writable filesystem this - * reads WAL frames correctly via the -shm wal-index. SQLite opens lazily, - * so a read-only-filesystem failure (cannot create -shm for a WAL-mode - * DB) surfaces on first access, not at open time; we probe with a trivial - * read to force it. If the probe fails, retry once with an immutable URI - * that bypasses WAL and reads the main DB file directly. - * - * No SQLITE_OPEN_CREATE on either path — a missing DB must return NULL - * (no ghost .db for unknown/unindexed projects). */ +/* Query tools open the project DB READ-ONLY: a read query must never + * mutate the DB (the previous READWRITE open + WAL write-pragmas did), + * and must work on a read-only DB file / filesystem. + * + * Try a plain READONLY open first — on a normal writable filesystem this + * reads WAL frames correctly via the -shm wal-index. SQLite opens lazily, + * so a read-only-filesystem failure (cannot create -shm for a WAL-mode + * DB) surfaces on first access, not at open time; we probe with a trivial + * read to force it. If the probe fails, retry once with an immutable URI + * that bypasses WAL and reads the main DB file directly. + * + * No SQLITE_OPEN_CREATE on either path — a missing DB must return NULL + * (no ghost .db for unknown/unindexed projects). Returns false with s->db + * closed when the DB cannot be opened. */ +static bool query_open_first_access(cbm_store_t *s, const char *db_path) { char open_path[4096]; if (!cbm_path_for_file_api(db_path, open_path, sizeof(open_path))) { - free(s); - return NULL; + return false; } int rc = sqlite3_open_v2(open_path, &s->db, SQLITE_OPEN_READONLY, NULL); if (rc == SQLITE_OK) { @@ -1036,22 +1027,68 @@ cbm_store_t *cbm_store_open_path_query(const char *db_path) { * be opened (the read-only-filesystem case). This also keeps the * common "project not found" path to a single open attempt. */ if (!cbm_file_exists(db_path)) { - free(s); - return NULL; + return false; } char uri[ST_QUERY_URI_MAX]; if (!build_immutable_uri(db_path, uri, sizeof(uri))) { - free(s); - return NULL; + return false; } rc = sqlite3_open_v2(uri, &s->db, SQLITE_OPEN_READONLY | SQLITE_OPEN_URI, NULL); if (rc != SQLITE_OK) { /* sqlite3_open_v2 allocates a handle even on failure — must close it. */ sqlite3_close(s->db); - free(s); - return NULL; + s->db = NULL; + return false; } } + return true; +} + +#ifdef CBM_ENABLE_TEST_SEAMS +static void (*g_first_access_hook)(void *ctx) = NULL; +static void *g_first_access_hook_ctx = NULL; +void cbm_store_query_first_access_hook_for_testing(void (*hook)(void *ctx), void *ctx) { + g_first_access_hook = hook; + g_first_access_hook_ctx = ctx; +} +static void query_first_access_hook(void) { + if (g_first_access_hook) { + g_first_access_hook(g_first_access_hook_ctx); + } +} +#else +static void query_first_access_hook(void) {} +#endif + +cbm_store_t *cbm_store_open_path_query(const char *db_path) { + if (!db_path) { + return NULL; + } + + cbm_store_t *s = calloc(CBM_ALLOC_ONE, sizeof(cbm_store_t)); + if (!s) { + return NULL; + } + + /* One query connection at a time takes its first look at the WAL. On an + * idle DB (no connection open, -shm reset) that first read runs WAL + * recovery, rebuilding the shared wal-index header under SQLite's write + * lock, while another opener reads the same header lock-free before it + * takes its read lock (walTryBeginRead). Across processes SQLite's file + * locks order the two; between this process's request threads nothing + * did, and TSan reported walTryBeginRead against walIndexRecover from + * concurrent index_repository requests. Holding one process-wide mutex + * across open + first access puts a recovery before the next connection's + * first read. The probe is one sqlite_master read, so the hold is short. */ + sqlite3_mutex *first_access = sqlite3_mutex_alloc(SQLITE_MUTEX_STATIC_APP1); + sqlite3_mutex_enter(first_access); + bool opened = query_open_first_access(s, db_path); + query_first_access_hook(); + sqlite3_mutex_leave(first_access); + if (!opened) { + free(s); + return NULL; + } s->db_path = heap_strdup(db_path); diff --git a/src/store/store.h b/src/store/store.h index fd690b5daf..058e1a5877 100644 --- a/src/store/store.h +++ b/src/store/store.h @@ -385,6 +385,13 @@ cbm_store_t *cbm_store_open_path_existing(const char *db_path); * exist — never creates a new .db file. */ cbm_store_t *cbm_store_open_path_query(const char *db_path); +#ifdef CBM_ENABLE_TEST_SEAMS +/* Every following query open calls `hook(ctx)` inside its first-access + * section (after the first read, the process-wide lock still held) until the + * hook is cleared with NULL. Test builds only. */ +void cbm_store_query_first_access_hook_for_testing(void (*hook)(void *ctx), void *ctx); +#endif + /* Validate and seal an existing DB for atomic replacement without creating or * migrating its schema. Returns OK when sealed, NOT_FOUND when the bytes are * definitely corrupt/incompatible and should be quarantined, or ERR when the diff --git a/tests/test_store_checkpoint.c b/tests/test_store_checkpoint.c index 8ab6d76aea..295ccd0011 100644 --- a/tests/test_store_checkpoint.c +++ b/tests/test_store_checkpoint.c @@ -20,6 +20,8 @@ #include #include #include +#include "../src/foundation/compat_thread.h" +#include static void tsc_cleanup_db(const char *db_path) { char sidecar[512]; @@ -426,7 +428,70 @@ TEST(remove_db_sidecars_rejects_truncated_suffix_path) { PASS(); } +/* The first read of a query connection on an idle WAL DB runs WAL recovery, + * rebuilding the shared wal-index header; another connection of this process + * opening at the same moment read that header lock-free (TSan, three CI + * sightings: walTryBeginRead vs walIndexRecover from concurrent + * index_repository requests). A query open's first access is therefore one + * process-wide section, SQLite's SQLITE_MUTEX_STATIC_APP1. With opener A held + * inside it, the section must not be free. + * Windows: SQLite's sqlite3_mutex_try reports BUSY unconditionally without + * SQLITE_WIN32_MUTEX_TRYENTER, so there the assertion cannot go RED; the + * mutex is the same code on every platform and the RED proof is POSIX. */ +typedef struct { + const char *path; + atomic_int inside; + atomic_int release; +} tsc_first_access_t; + +static void tsc_first_access_hold(void *ctx) { + tsc_first_access_t *p = ctx; + atomic_store_explicit(&p->inside, 1, memory_order_release); + while (!atomic_load_explicit(&p->release, memory_order_acquire)) {} +} + +static void *tsc_first_access_open(void *arg) { + tsc_first_access_t *p = arg; + cbm_store_t *s = cbm_store_open_path_query(p->path); + if (s) { + cbm_store_close(s); + } + return NULL; +} + +TEST(store_query_first_access_is_one_process_section) { + char *dir = th_mktempdir("cbm_first_access"); + ASSERT_NOT_NULL(dir); + char db[512]; + snprintf(db, sizeof(db), "%s/g.db", dir); + cbm_store_t *w = cbm_store_open_path(db); + ASSERT_NOT_NULL(w); + cbm_store_close(w); + + tsc_first_access_t p = {.path = db}; + atomic_init(&p.inside, 0); + atomic_init(&p.release, 0); + cbm_store_query_first_access_hook_for_testing(tsc_first_access_hold, &p); + cbm_thread_t opener; + ASSERT_EQ(cbm_thread_create(&opener, 0, tsc_first_access_open, &p), 0); + while (!atomic_load_explicit(&p.inside, memory_order_acquire)) {} + sqlite3_mutex *section = sqlite3_mutex_alloc(SQLITE_MUTEX_STATIC_APP1); + int try_rc = sqlite3_mutex_try(section); + if (try_rc == SQLITE_OK) { + sqlite3_mutex_leave(section); + } + atomic_store_explicit(&p.release, 1, memory_order_release); + (void)cbm_thread_join(&opener); + cbm_store_query_first_access_hook_for_testing(NULL, NULL); + tsc_cleanup_db(db); + th_rmtree(dir); + + ASSERT_EQ(try_rc, SQLITE_BUSY); + PASS(); +} + SUITE(store_checkpoint) { + RUN_TEST(store_query_first_access_is_one_process_section); RUN_TEST(checkpoint_does_not_truncate_wal); RUN_TEST(seal_for_atomic_publish_makes_main_file_self_contained); RUN_TEST(seal_for_atomic_publish_fails_closed_while_reader_pins_wal);