From 45b9a5c5e125fe56e37acb80e89e38a06ae67ac5 Mon Sep 17 00:00:00 2001 From: Nikita Bige Date: Tue, 7 Jul 2026 22:28:28 +0300 Subject: [PATCH] fix(sqlite_writer): publish rebuilt db atomically Keep the live database visible until a synced temporary generation can replace it. Preserve destination sidecars on replacement failure and use UTF-8-safe Windows file operations. Signed-off-by: Nikita Bige --- internal/cbm/sqlite_writer.c | 115 +++++++++++---- src/foundation/compat_fs.c | 21 +-- src/foundation/compat_fs.h | 8 +- tests/test_sqlite_writer.c | 275 +++++++++++++++++++++++++++++++++++ 4 files changed, 376 insertions(+), 43 deletions(-) diff --git a/internal/cbm/sqlite_writer.c b/internal/cbm/sqlite_writer.c index 2633a0413..9c18ba67d 100644 --- a/internal/cbm/sqlite_writer.c +++ b/internal/cbm/sqlite_writer.c @@ -27,6 +27,16 @@ #include #include +#ifdef _WIN32 +#include +#include +#include +#define cbm_writer_getpid _getpid +#else +#include +#define cbm_writer_getpid getpid +#endif + #define CBM_PAGE_SIZE 65536 /* SQLite reserves the page containing the 1 GiB file offset (the "pending byte" @@ -1707,6 +1717,8 @@ static uint32_t build_node_index_sorted(FILE *fp, uint32_t *next_page, CBMDumpNo typedef struct { FILE *fp; uint32_t next_page; + char final_path[CBM_SZ_4K]; + char temp_path[CBM_SZ_4K]; const char *project; const char *root_path; const char *indexed_at; @@ -1720,6 +1732,60 @@ typedef struct { int token_vec_count; } write_db_ctx_t; +static int make_writer_temp_path(const char *path, const void *token, char *out, size_t out_size) { + int n = snprintf(out, out_size, "%s.tmp.%ld.%p", path, (long)cbm_writer_getpid(), token); + return (n >= 0 && (size_t)n < out_size) ? 0 : ERR_WRITE_FAILED; +} + +static int sync_writer_output(FILE *fp) { + if (fflush(fp) != 0) { + return ERR_WRITE_FAILED; + } +#ifdef _WIN32 + return _commit(_fileno(fp)) == 0 ? 0 : ERR_WRITE_FAILED; +#else + return fsync(fileno(fp)) == 0 ? 0 : ERR_WRITE_FAILED; +#endif +} + +static int discard_writer_output(write_db_ctx_t *w, int rc) { + if (w->fp) { + (void)fclose(w->fp); + w->fp = NULL; + } + if (w->temp_path[0]) { + (void)cbm_unlink(w->temp_path); + } + return rc; +} + +static int publish_writer_output(write_db_ctx_t *w) { + if (sync_writer_output(w->fp) != 0) { + return discard_writer_output(w, ERR_WRITE_FAILED); + } + if (fclose(w->fp) != 0) { + w->fp = NULL; + if (w->temp_path[0]) { + (void)cbm_unlink(w->temp_path); + } + return ERR_WRITE_FAILED; + } + w->fp = NULL; + if (!w->temp_path[0] || !w->final_path[0]) { + return 0; + } + if (cbm_rename_replace(w->temp_path, w->final_path) != 0) { + (void)cbm_unlink(w->temp_path); + return ERR_WRITE_FAILED; + } + /* Sidecars are removed only after the replacement succeeds. On POSIX, + * readers of the old generation retain their unlinked handles. On + * Windows, an incompatible open handle makes MoveFileExW fail before + * this cleanup, preserving the old DB and its sidecars. */ + cbm_remove_db_sidecars(w->final_path); + return 0; +} + /* Callback type for building a record from an item at index i. */ typedef uint8_t *(*build_record_fn)(const void *items, int i, int *out_len); typedef int64_t (*get_rowid_fn)(const void *items, int i); @@ -1992,20 +2058,17 @@ static int write_db_after_nodes(write_db_ctx_t *w, uint32_t nodes_root) { int rc = write_one_table(w, &edges_root, w->edges, w->edge_count, adapt_build_edge, adapt_edge_id); if (rc != 0) { - (void)fclose(fp); - return rc; + return discard_writer_output(w, rc); } rc = write_one_table(w, &vectors_root, w->vectors, w->vector_count, adapt_build_vector, adapt_vector_id); if (rc != 0) { - (void)fclose(fp); - return rc; + return discard_writer_output(w, rc); } rc = write_one_table(w, &token_vecs_root, w->token_vecs, w->token_vec_count, adapt_build_token_vec, adapt_token_vec_id); if (rc != 0) { - (void)fclose(fp); - return rc; + return discard_writer_output(w, rc); } CBM_PROF_END_N("write_db", "1_data_tables", t_data, node_count + edge_count); @@ -2058,8 +2121,7 @@ static int write_db_after_nodes(write_db_ctx_t *w, uint32_t nodes_root) { &idx_nodes_name_root, &idx_nodes_file_root, &autoindex_nodes_root); CBM_PROF_END_N("write_db", "4_node_indexes_seq", t_node_idx, node_count * NODE_SORT_THREADS); if (nrc != 0) { - (void)fclose(fp); - return nrc; + return discard_writer_output(w, nrc); } CBM_PROF_START(t_edge_idx); @@ -2076,8 +2138,7 @@ static int write_db_after_nodes(write_db_ctx_t *w, uint32_t nodes_root) { &idx_edges_url_path_root, &autoindex_edges_root); CBM_PROF_END_N("write_db", "5_edge_indexes_seq", t_edge_idx, edge_count * EDGE_SORT_THREADS); if (erc != 0) { - (void)fclose(fp); - return erc; + return discard_writer_output(w, erc); } // Autoindex for projects(name TEXT PK) — single text column @@ -2188,12 +2249,10 @@ static int write_db_after_nodes(write_db_ctx_t *w, uint32_t nodes_root) { int master_count = sizeof(master) / sizeof(master[0]); int rc2 = write_master_page1(fp, master, master_count, next_page); if (rc2 != 0) { - (void)fclose(fp); - return rc2; + return discard_writer_output(w, rc2); } pad_file_to_page_boundary(fp, next_page); - (void)fclose(fp); - return 0; + return publish_writer_output(w); } // --- Streaming writer (incremental bulk node-table append) --- @@ -2207,21 +2266,20 @@ struct cbm_db_writer { }; cbm_db_writer_t *cbm_writer_open(const char *path) { - /* Installing a fresh DB generation: drop the destination's leftover - * -wal/-shm or a crashed session's WAL gets replayed on top of the - * new file at the next open (#897). */ - cbm_remove_db_sidecars(path); - /* cbm_fopen, not raw fopen: the cache dir lives under the user profile, - * and an ANSI-CP fopen fails to create the DB on non-ASCII Windows - * profiles — the reported phase=dump failure (#996). Everything around - * this call (cbm_mkdir_p, sqlite3_open_v2 reopen) is already wide-safe. */ - FILE *fp = cbm_fopen(path, "wb"); - if (!fp) { - return NULL; - } cbm_db_writer_t *w = (cbm_db_writer_t *)calloc(CBM_ALLOC_ONE, sizeof(*w)); if (!w) { - (void)fclose(fp); + return NULL; + } + int n = snprintf(w->wc.final_path, sizeof(w->wc.final_path), "%s", path); + if (n < 0 || (size_t)n >= sizeof(w->wc.final_path) || + make_writer_temp_path(path, w, w->wc.temp_path, sizeof(w->wc.temp_path)) != 0) { + free(w); + return NULL; + } + FILE *fp = cbm_fopen(w->wc.temp_path, "wb"); + if (!fp) { + (void)cbm_unlink(w->wc.temp_path); + free(w); return NULL; } w->wc.fp = fp; @@ -2287,8 +2345,7 @@ int cbm_writer_finalize(cbm_db_writer_t *w, const char *project, const char *roo write_db_ctx_t wc = w->wc; /* value copy survives free(w) */ free(w); if (err != 0) { - (void)fclose(wc.fp); /* wc is a value copy, valid after free(w) */ - return err; + return discard_writer_output(&wc, err); } return write_db_after_nodes(&wc, nodes_root); } diff --git a/src/foundation/compat_fs.c b/src/foundation/compat_fs.c index 428f83b89..7c5ed775e 100644 --- a/src/foundation/compat_fs.c +++ b/src/foundation/compat_fs.c @@ -761,17 +761,19 @@ int cbm_canonical_path(const char *path, char *out, size_t out_sz) { /* rename() with overwrite semantics on every platform: POSIX rename already * replaces atomically; Windows rename fails with EEXIST when the target - * exists, so use MoveFileExW(MOVEFILE_REPLACE_EXISTING) there (wide paths — - * raw MoveFileExA would re-mangle non-ASCII cache paths). */ + * exists, so use write-through MoveFileExW(MOVEFILE_REPLACE_EXISTING) there + * (wide paths — raw MoveFileExA would re-mangle non-ASCII cache paths). */ int cbm_rename_replace(const char *src, const char *dst) { #ifdef _WIN32 wchar_t *wsrc = cbm_utf8_to_wide(src); wchar_t *wdst = cbm_utf8_to_wide(dst); int ret = CBM_NOT_FOUND; if (wsrc && wdst) { - ret = MoveFileExW(wsrc, wdst, MOVEFILE_REPLACE_EXISTING | MOVEFILE_COPY_ALLOWED) - ? 0 - : CBM_NOT_FOUND; + ret = + MoveFileExW(wsrc, wdst, + MOVEFILE_REPLACE_EXISTING | MOVEFILE_COPY_ALLOWED | MOVEFILE_WRITE_THROUGH) + ? 0 + : CBM_NOT_FOUND; } free(wsrc); free(wdst); @@ -782,11 +784,10 @@ int cbm_rename_replace(const char *src, const char *dst) { } /* Remove a SQLite database's -wal/-shm sidecars (both platforms). Any code - * path that installs a FRESH database file at a path where a previous - * generation lived must call this first: SQLite decides whether to replay a - * WAL purely from the sidecar's own header/checksums, so a leftover WAL - * from a crashed session is recovered ON TOP of the freshly installed file - * at the next open, splicing old-generation pages into it (#897). */ + * path that installs a FRESH database file where a previous generation lived + * must remove them before the new generation can be opened: SQLite decides + * whether to replay a WAL purely from the sidecar's own header/checksums, so + * a leftover WAL can splice old-generation pages into the new file (#897). */ void cbm_remove_db_sidecars(const char *db_path) { if (!db_path || !db_path[0]) { return; diff --git a/src/foundation/compat_fs.h b/src/foundation/compat_fs.h index 9f2929c37..af89c795d 100644 --- a/src/foundation/compat_fs.h +++ b/src/foundation/compat_fs.h @@ -46,12 +46,12 @@ bool cbm_mkdir_p(const char *path, int mode); /* Delete a file. Returns 0 on success. */ int cbm_unlink(const char *path); -/* Remove -wal/-shm. MUST be called by any path installing a fresh - * DB file where a previous generation lived — a leftover WAL is otherwise - * replayed on top of the new file at the next open (#897). */ +/* Remove -wal/-shm. Any path installing a fresh DB generation must + * do this before the new generation can be opened; a leftover WAL is + * otherwise replayed on top of the new file (#897). */ void cbm_remove_db_sidecars(const char *db_path); /* rename() that replaces an existing destination on every platform - * (Windows rename fails with EEXIST; this uses MoveFileExW there). */ + * (Windows rename fails with EEXIST; this uses write-through MoveFileExW). */ int cbm_rename_replace(const char *src, const char *dst); /* Canonicalize an EXISTING path (realpath / wide GetFullPathNameW). Locale- * independent on Windows — never routes UTF-8 through the ANSI CRT (#973). diff --git a/tests/test_sqlite_writer.c b/tests/test_sqlite_writer.c index 8898eb046..1084d0529 100644 --- a/tests/test_sqlite_writer.c +++ b/tests/test_sqlite_writer.c @@ -8,6 +8,7 @@ * bypassing the SQL parser entirely. These tests verify integrity. */ #include "../src/foundation/compat.h" +#include "foundation/compat_fs.h" #include "test_framework.h" /* sqlite_writer.h is at internal/cbm/ — Makefile adds -Iinternal/cbm */ #include "sqlite_writer.h" /* CBMDumpNode, CBMDumpEdge, cbm_write_db */ @@ -25,6 +26,81 @@ static int make_temp_db(char *path, size_t pathsz) { return 0; } +static int assert_node_name(sqlite3 *db, const char *expected) { + sqlite3_stmt *stmt = NULL; + int rc = sqlite3_prepare_v2(db, "SELECT name FROM nodes WHERE id=1", -1, &stmt, NULL); + ASSERT_EQ(rc, SQLITE_OK); + rc = sqlite3_step(stmt); + ASSERT_EQ(rc, SQLITE_ROW); + ASSERT_STR_EQ((const char *)sqlite3_column_text(stmt, 0), expected); + sqlite3_finalize(stmt); + return 0; +} + +static int write_fixture_file(const char *path, const char *contents) { + FILE *fp = cbm_fopen(path, "wb"); + if (!fp) { + return -1; + } + size_t len = strlen(contents); + size_t written = fwrite(contents, 1, len, fp); + int close_rc = fclose(fp); + return written == len && close_rc == 0 ? 0 : -1; +} + +static int fixture_file_equals(const char *path, const char *expected) { + FILE *fp = cbm_fopen(path, "rb"); + if (!fp) { + return 0; + } + char buf[64] = {0}; + size_t n = fread(buf, 1, sizeof(buf), fp); + int close_rc = fclose(fp); + size_t expected_len = strlen(expected); + return close_rc == 0 && n == expected_len && memcmp(buf, expected, expected_len) == 0; +} + +static int count_temp_outputs_for(const char *path) { + char dir[256]; + char base[256]; + const char *slash = strrchr(path, '/'); +#ifdef _WIN32 + const char *backslash = strrchr(path, '\\'); + if (!slash || (backslash && backslash > slash)) { + slash = backslash; + } +#endif + if (slash) { + size_t dir_len = (size_t)(slash - path); + if (dir_len == 0 || dir_len >= sizeof(dir)) { + return -1; + } + memcpy(dir, path, dir_len); + dir[dir_len] = '\0'; + snprintf(base, sizeof(base), "%s", slash + 1); + } else { + snprintf(dir, sizeof(dir), "."); + snprintf(base, sizeof(base), "%s", path); + } + + cbm_dir_t *d = cbm_opendir(dir); + if (!d) { + return -1; + } + size_t base_len = strlen(base); + int count = 0; + cbm_dirent_t *ent; + while ((ent = cbm_readdir(d)) != NULL) { + size_t name_len = strlen(ent->name); + if (name_len > base_len + 5 && strncmp(ent->name, base, base_len) == 0 && + strncmp(ent->name + base_len, ".tmp.", 5) == 0) { + count++; + } + } + cbm_closedir(d); + return count; +} + /* ── Tests ─────────────────────────────────────────────────────── */ TEST(sw_minimal_data) { @@ -605,6 +681,200 @@ TEST(sw_oversized_node) { PASS(); } +TEST(sw_stream_open_does_not_truncate_destination) { + char path[256]; + ASSERT_EQ(make_temp_db(path, sizeof(path)), 0); + + const char sentinel[] = "old-db-visible-until-finalize"; + ASSERT_EQ(write_fixture_file(path, sentinel), 0); + + cbm_db_writer_t *w = cbm_writer_open(path); + ASSERT(w != NULL); + + char buf[64] = {0}; + FILE *fp = cbm_fopen(path, "rb"); + ASSERT(fp != NULL); + size_t n = fread(buf, 1, sizeof(sentinel) - 1, fp); + ASSERT_EQ(fclose(fp), 0); + ASSERT_EQ(n, sizeof(sentinel) - 1); + ASSERT_EQ(memcmp(buf, sentinel, sizeof(sentinel) - 1), 0); + + int rc = cbm_writer_finalize(w, "test", "/tmp/test", "2026-07-07T00:00:00Z", NULL, 0, NULL, 0, + NULL, 0, NULL, 0); + ASSERT_EQ(rc, 0); + + sqlite3 *db = NULL; + rc = sqlite3_open(path, &db); + ASSERT_EQ(rc, SQLITE_OK); + + sqlite3_stmt *stmt = NULL; + sqlite3_prepare_v2(db, "PRAGMA integrity_check", -1, &stmt, NULL); + rc = sqlite3_step(stmt); + ASSERT_EQ(rc, SQLITE_ROW); + ASSERT_STR_EQ((const char *)sqlite3_column_text(stmt, 0), "ok"); + sqlite3_finalize(stmt); + + sqlite3_close(db); + cbm_unlink(path); + PASS(); +} + +TEST(sw_publish_removes_destination_sidecars) { + char path[256]; + ASSERT_EQ(make_temp_db(path, sizeof(path)), 0); + + char wal[320]; + char shm[320]; + snprintf(wal, sizeof(wal), "%s-wal", path); + snprintf(shm, sizeof(shm), "%s-shm", path); + + ASSERT_EQ(write_fixture_file(wal, "stale-wal"), 0); + ASSERT_EQ(write_fixture_file(shm, "stale-shm"), 0); + + int rc = cbm_write_db(path, "test", "/tmp/test", "2026-07-07T00:00:00Z", NULL, 0, NULL, 0, NULL, + 0, NULL, 0); + ASSERT_EQ(rc, 0); + ASSERT(!fixture_file_equals(wal, "stale-wal")); + ASSERT(!fixture_file_equals(shm, "stale-shm")); + + sqlite3 *db = NULL; + rc = sqlite3_open(path, &db); + ASSERT_EQ(rc, SQLITE_OK); + + sqlite3_stmt *stmt = NULL; + sqlite3_prepare_v2(db, "PRAGMA integrity_check", -1, &stmt, NULL); + rc = sqlite3_step(stmt); + ASSERT_EQ(rc, SQLITE_ROW); + ASSERT_STR_EQ((const char *)sqlite3_column_text(stmt, 0), "ok"); + sqlite3_finalize(stmt); + + sqlite3_close(db); + cbm_unlink(path); + PASS(); +} + +TEST(sw_publish_failure_preserves_destination_sidecars) { + char path[256]; + snprintf(path, sizeof(path), "/tmp/cbm_sw_dir_XXXXXX"); + ASSERT(cbm_mkdtemp(path) != NULL); + + char wal[320]; + char shm[320]; + snprintf(wal, sizeof(wal), "%s-wal", path); + snprintf(shm, sizeof(shm), "%s-shm", path); + ASSERT_EQ(write_fixture_file(wal, "live-wal"), 0); + ASSERT_EQ(write_fixture_file(shm, "live-shm"), 0); + + int rc = cbm_write_db(path, "test", "/tmp/test", "2026-07-07T00:00:00Z", NULL, 0, NULL, 0, NULL, + 0, NULL, 0); + ASSERT(rc != 0); + + ASSERT_EQ(count_temp_outputs_for(path), 0); + ASSERT(fixture_file_equals(wal, "live-wal")); + ASSERT(fixture_file_equals(shm, "live-shm")); + + cbm_unlink(wal); + cbm_unlink(shm); + cbm_rmdir(path); + PASS(); +} + +TEST(sw_publish_supports_non_ascii_path) { + char dir[256]; + snprintf(dir, sizeof(dir), "/tmp/cbm_sw_utf8_XXXXXX"); + ASSERT(cbm_mkdtemp(dir) != NULL); + + char path[320]; + char wal[384]; + char shm[384]; + snprintf(path, sizeof(path), "%s/db-\xC3\xBC.sqlite", dir); + snprintf(wal, sizeof(wal), "%s-wal", path); + snprintf(shm, sizeof(shm), "%s-shm", path); + ASSERT_EQ(write_fixture_file(wal, "stale-wal"), 0); + ASSERT_EQ(write_fixture_file(shm, "stale-shm"), 0); + + int rc = cbm_write_db(path, "test", "/tmp/test", "2026-07-07T00:00:00Z", NULL, 0, NULL, 0, NULL, + 0, NULL, 0); + ASSERT_EQ(rc, 0); + ASSERT(!fixture_file_equals(wal, "stale-wal")); + ASSERT(!fixture_file_equals(shm, "stale-shm")); + + sqlite3 *db = NULL; + rc = sqlite3_open(path, &db); + ASSERT_EQ(rc, SQLITE_OK); + sqlite3_stmt *stmt = NULL; + sqlite3_prepare_v2(db, "PRAGMA integrity_check", -1, &stmt, NULL); + ASSERT_EQ(sqlite3_step(stmt), SQLITE_ROW); + ASSERT_STR_EQ((const char *)sqlite3_column_text(stmt, 0), "ok"); + sqlite3_finalize(stmt); + sqlite3_close(db); + + cbm_unlink(path); + cbm_rmdir(dir); + PASS(); +} + +TEST(sw_publish_preserves_live_reader) { +#ifdef _WIN32 + SKIP_PLATFORM("POSIX open-reader rename semantics"); +#endif + char path[256]; + ASSERT_EQ(make_temp_db(path, sizeof(path)), 0); + + CBMDumpNode old_nodes[1] = {{ + .id = 1, + .project = "test", + .label = "Function", + .name = "old_fn", + .qualified_name = "test.old_fn", + .file_path = "old.go", + .start_line = 1, + .end_line = 2, + .properties = "{}", + }}; + int rc = cbm_write_db(path, "test", "/tmp/test", "2026-07-07T00:00:00Z", old_nodes, 1, NULL, 0, + NULL, 0, NULL, 0); + ASSERT_EQ(rc, 0); + + sqlite3 *reader = NULL; + rc = sqlite3_open(path, &reader); + ASSERT_EQ(rc, SQLITE_OK); + rc = sqlite3_exec(reader, "PRAGMA mmap_size=268435456", NULL, NULL, NULL); + ASSERT_EQ(rc, SQLITE_OK); + rc = sqlite3_exec(reader, "BEGIN", NULL, NULL, NULL); + ASSERT_EQ(rc, SQLITE_OK); + ASSERT_EQ(assert_node_name(reader, "old_fn"), 0); + + CBMDumpNode new_nodes[1] = {{ + .id = 1, + .project = "test", + .label = "Function", + .name = "new_fn", + .qualified_name = "test.new_fn", + .file_path = "new.go", + .start_line = 1, + .end_line = 2, + .properties = "{}", + }}; + rc = cbm_write_db(path, "test", "/tmp/test", "2026-07-07T00:00:01Z", new_nodes, 1, NULL, 0, + NULL, 0, NULL, 0); + ASSERT_EQ(rc, 0); + + ASSERT_EQ(assert_node_name(reader, "old_fn"), 0); + rc = sqlite3_exec(reader, "COMMIT", NULL, NULL, NULL); + ASSERT_EQ(rc, SQLITE_OK); + sqlite3_close(reader); + + sqlite3 *db = NULL; + rc = sqlite3_open(path, &db); + ASSERT_EQ(rc, SQLITE_OK); + ASSERT_EQ(assert_node_name(db, "new_fn"), 0); + sqlite3_close(db); + + cbm_unlink(path); + PASS(); +} + /* ── Suite ─────────────────────────────────────────────────────── */ SUITE(sqlite_writer) { @@ -615,4 +885,9 @@ SUITE(sqlite_writer) { RUN_TEST(sw_empty); RUN_TEST(sw_multi_page); RUN_TEST(sw_oversized_node); + RUN_TEST(sw_stream_open_does_not_truncate_destination); + RUN_TEST(sw_publish_removes_destination_sidecars); + RUN_TEST(sw_publish_failure_preserves_destination_sidecars); + RUN_TEST(sw_publish_supports_non_ascii_path); + RUN_TEST(sw_publish_preserves_live_reader); }