diff --git a/internal/cbm/sqlite_writer.c b/internal/cbm/sqlite_writer.c index 3b102a079..2f70c5fc4 100644 --- a/internal/cbm/sqlite_writer.c +++ b/internal/cbm/sqlite_writer.c @@ -16,6 +16,7 @@ #include "sqlite_writer.h" #include "foundation/constants.h" +#include "foundation/compat_fs.h" #include "foundation/compat_thread.h" #include "foundation/profile.h" @@ -2206,6 +2207,10 @@ 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); FILE *fp = fopen(path, "wb"); if (!fp) { return NULL; diff --git a/src/foundation/compat_fs.c b/src/foundation/compat_fs.c index f1b94c0d9..838c1cf96 100644 --- a/src/foundation/compat_fs.c +++ b/src/foundation/compat_fs.c @@ -713,3 +713,47 @@ int cbm_exec_no_shell(const char *const *argv) { } #endif /* _WIN32 */ + +/* 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). */ +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; + } + free(wsrc); + free(wdst); + return ret; +#else + return rename(src, dst); +#endif +} + +/* 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). */ +void cbm_remove_db_sidecars(const char *db_path) { + if (!db_path || !db_path[0]) { + return; + } + enum { SIDECAR_PATH_MAX = 4096 }; + char side[SIDECAR_PATH_MAX]; + int n = snprintf(side, sizeof(side), "%s-wal", db_path); + if (n > 0 && (size_t)n < sizeof(side)) { + (void)cbm_unlink(side); + } + n = snprintf(side, sizeof(side), "%s-shm", db_path); + if (n > 0 && (size_t)n < sizeof(side)) { + (void)cbm_unlink(side); + } +} diff --git a/src/foundation/compat_fs.h b/src/foundation/compat_fs.h index 224e2b9a4..05cae3dda 100644 --- a/src/foundation/compat_fs.h +++ b/src/foundation/compat_fs.h @@ -46,6 +46,13 @@ 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). */ +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). */ +int cbm_rename_replace(const char *src, const char *dst); /* Delete an empty directory. Returns 0 on success. */ int cbm_rmdir(const char *path); diff --git a/src/pipeline/artifact.c b/src/pipeline/artifact.c index d01fff2e0..4c525a71e 100644 --- a/src/pipeline/artifact.c +++ b/src/pipeline/artifact.c @@ -699,7 +699,11 @@ int cbm_artifact_import(const char *repo_path, const char *cache_db_path) { cbm_store_close(store); - /* Atomic rename to final path */ + /* Atomic rename to final path. Drop the DESTINATION's leftover + * -wal/-shm first: the import cleans the tmp file's sidecars, but a + * stale WAL next to the cache path would be replayed on top of the + * imported file at the next open (#897). */ + cbm_remove_db_sidecars(cache_db_path); if (rename(tmp_path, cache_db_path) != 0) { cbm_log_error("artifact.import", "err", "rename_to_cache"); cbm_unlink(tmp_path); diff --git a/src/store/store.c b/src/store/store.c index 67e46e970..4d506f58c 100644 --- a/src/store/store.c +++ b/src/store/store.c @@ -61,6 +61,7 @@ enum { #define SLEN(s) (sizeof(s) - 1) #include "store/store.h" +#include "foundation/compat_fs.h" #include "foundation/platform.h" #include "foundation/compat.h" #include "foundation/log.h" @@ -1095,9 +1096,12 @@ int cbm_store_dump_to_file(cbm_store_t *s, const char *dest_path) { sqlite3_exec(dest_db, "PRAGMA journal_mode = WAL;", NULL, NULL, NULL); sqlite3_close(dest_db); - /* Atomic rename: old WAL/SHM become stale and get recreated by - * the next reader's configure_pragmas call. */ - if (rename(tmp_path, dest_path) != 0) { + /* Remove the DESTINATION's leftover sidecars before installing: a + * stale WAL from a crashed session would be replayed on top of the + * fresh file at the next open — SQLite validates the WAL against its + * own header/checksums, not against the main file (#897). */ + cbm_remove_db_sidecars(dest_path); + if (cbm_rename_replace(tmp_path, dest_path) != 0) { store_set_error(s, "dump: rename failed"); (void)unlink(tmp_path); return CBM_STORE_ERR; diff --git a/tests/test_store_checkpoint.c b/tests/test_store_checkpoint.c index 11542969c..9097befd7 100644 --- a/tests/test_store_checkpoint.c +++ b/tests/test_store_checkpoint.c @@ -74,6 +74,116 @@ TEST(checkpoint_does_not_truncate_wal) { PASS(); } + +/* #897: any code path installing a fresh DB file must delete the + * destination's -wal/-shm first. SQLite decides whether to replay a WAL + * purely from the sidecar's own header/checksums — a leftover WAL from a + * crashed previous session is recovered ON TOP of the freshly installed + * file at the next open, splicing old-generation pages into it (short + * indexes, btreeInitPage failures, or resurrected stale rows). + * + * Repro (per the issue): hot-copy a live WAL aside, close cleanly, restore + * the copy as the crashed-session leftover, install a fresh generation via + * cbm_store_dump_to_file, reopen — the stale generation's row must NOT be + * visible and the fresh row must be. */ +static int tsc_copy_file(const char *src, const char *dst) { + FILE *in = fopen(src, "rb"); + if (!in) { + return -1; + } + FILE *out = fopen(dst, "wb"); + if (!out) { + (void)fclose(in); + return -1; + } + char buf[4096]; + size_t n; + while ((n = fread(buf, 1, sizeof(buf), in)) > 0) { + if (fwrite(buf, 1, n, out) != n) { + (void)fclose(in); + (void)fclose(out); + return -1; + } + } + (void)fclose(in); + (void)fclose(out); + return 0; +} + +TEST(dump_install_ignores_stale_wal_sidecar) { + char *td = th_mktempdir("cbm_stalewal"); + char db_path[512]; + char wal_path[512]; + char stale_copy[512]; + snprintf(db_path, sizeof(db_path), "%s/gen.db", td); + snprintf(wal_path, sizeof(wal_path), "%s-wal", db_path); + snprintf(stale_copy, sizeof(stale_copy), "%s/stale.wal", td); + + /* Generation 1: file-backed store with a marker row living in the WAL. */ + cbm_store_t *s1 = cbm_store_open_path(db_path); + ASSERT_NOT_NULL(s1); + cbm_store_upsert_project(s1, "walgen", "/tmp/walgen"); + cbm_node_t stale = {.project = "walgen", + .label = "Function", + .name = "stale_gen_node", + .qualified_name = "walgen.mod.stale_gen_node", + .file_path = "mod.py", + .start_line = 1, + .end_line = 2}; + ASSERT_TRUE(cbm_store_upsert_node(s1, &stale) > 0); + + /* Hot-copy the live WAL (must be non-empty or the repro is vacuous). */ + struct stat st_wal = {0}; + ASSERT_EQ(stat(wal_path, &st_wal), 0); + ASSERT_TRUE(st_wal.st_size > 0); + ASSERT_EQ(tsc_copy_file(wal_path, stale_copy), 0); + cbm_store_close(s1); /* clean close checkpoints + removes the WAL */ + + /* Simulate the crashed previous session's leftover sidecar. */ + ASSERT_EQ(tsc_copy_file(stale_copy, wal_path), 0); + + /* Generation 2: fresh store installed over db_path. */ + cbm_store_t *s2 = cbm_store_open_memory(); + ASSERT_NOT_NULL(s2); + cbm_store_upsert_project(s2, "walgen", "/tmp/walgen"); + cbm_node_t fresh = {.project = "walgen", + .label = "Function", + .name = "fresh_gen_node", + .qualified_name = "walgen.mod.fresh_gen_node", + .file_path = "mod.py", + .start_line = 1, + .end_line = 2}; + ASSERT_TRUE(cbm_store_upsert_node(s2, &fresh) > 0); + ASSERT_EQ(cbm_store_dump_to_file(s2, db_path), CBM_STORE_OK); + cbm_store_close(s2); + + /* Reader: the stale WAL must not have been replayed onto gen 2. */ + cbm_store_t *s3 = cbm_store_open_path(db_path); + ASSERT_NOT_NULL(s3); + cbm_node_t *hits = NULL; + int hit_count = 0; + ASSERT_EQ(cbm_store_find_nodes_by_name(s3, "walgen", "fresh_gen_node", &hits, &hit_count), + CBM_STORE_OK); + ASSERT_TRUE(hit_count >= 1); + cbm_store_free_nodes(hits, hit_count); + hits = NULL; + hit_count = 0; + ASSERT_EQ(cbm_store_find_nodes_by_name(s3, "walgen", "stale_gen_node", &hits, &hit_count), + CBM_STORE_OK); + ASSERT_EQ(hit_count, 0); + cbm_store_free_nodes(hits, hit_count); + cbm_store_close(s3); + + unlink(wal_path); + char shm_path[512]; + snprintf(shm_path, sizeof(shm_path), "%s-shm", db_path); + unlink(shm_path); + unlink(db_path); + unlink(stale_copy); + PASS(); +} + SUITE(store_checkpoint) { RUN_TEST(checkpoint_does_not_truncate_wal); + RUN_TEST(dump_install_ignores_stale_wal_sidecar); }