Skip to content

Fix SQLite writer atomic cache publish#940

Open
WarGloom wants to merge 1 commit into
DeusData:mainfrom
WarGloom:codex/fix-sqlite-writer-atomic-publish
Open

Fix SQLite writer atomic cache publish#940
WarGloom wants to merge 1 commit into
DeusData:mainfrom
WarGloom:codex/fix-sqlite-writer-atomic-publish

Conversation

@WarGloom

@WarGloom WarGloom commented Jul 7, 2026

Copy link
Copy Markdown

Summary

  • write direct SQLite dumps to a sibling temp file and atomically publish them only after finalization succeeds
  • discard temp DBs on writer errors instead of replacing the live cache with partial output
  • remove destination -wal and -shm sidecars when installing the freshly rebuilt DB

Root Cause

The custom SQLite page writer opened the final cache DB with fopen(path, "wb"). In shared MCP sessions, that can truncate a live DB while SQLite readers still have the old file mapped, and it can leave old-generation WAL/SHM sidecars beside the rebuilt DB. The observed crash reached SQLite btreeInitPage during cbm_store_delete_file_hashes after the dump/reopen step.

Validation

  • git diff --check
  • ASAN_OPTIONS=detect_leaks=0 build/c/test-runner sqlite_writer
  • ASAN_OPTIONS=detect_leaks=0 build/c/test-runner pipeline
    • passes 217 tests
    • note: existing unrelated UBSAN warning remains in src/pipeline/pass_similarity.c:152
  • make -f Makefile.cbm build/c/codebase-memory-mcp
  • CBM_CACHE_DIR=/tmp/cbm-wiki-mcp-pr-full-20260707-1 build/c/codebase-memory-mcp cli --index-worker index_repository '{"repo_path":"/home/nikita/work/Projects/ai/wiki-mcp"}' --response-out /tmp/cbm-wiki-mcp-pr-full-20260707-1.response
    • completed with status:"indexed", nodes:2256, edges:12934

Fixes #897.

@DeusData DeusData added bug Something isn't working stability/performance Server crashes, OOM, hangs, high CPU/memory priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jul 7, 2026
@DeusData

DeusData commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Thanks for the SQLite writer publish fix. Triage: draft high-priority cache/store integrity bug.

Review will be cautious because this touches direct DB publication: temp-file atomicity, cleanup on failure, destination WAL/SHM sidecar removal, and live-reader behavior all need explicit tests. Please also rename the PR title before it is ready for review so it describes the change directly.

@WarGloom WarGloom force-pushed the codex/fix-sqlite-writer-atomic-publish branch from 123f501 to 82df8c2 Compare July 8, 2026 09:24
@WarGloom WarGloom changed the title [codex] fix sqlite writer db publish Fix SQLite writer atomic cache publish Jul 8, 2026
@WarGloom

WarGloom commented Jul 8, 2026

Copy link
Copy Markdown
Author

Addressed.

  • Renamed the PR to describe the change directly: "Fix SQLite writer atomic cache publish".
  • Added explicit regression coverage for the remaining publish-integrity cases:
    • sw_publish_failure_removes_temp_output verifies a failed destination replacement discards the temporary DB output.
    • sw_publish_preserves_live_reader verifies a POSIX reader with an open transaction keeps seeing the old DB while a new connection sees the atomically published DB after replacement.
  • Kept the existing coverage for pre-finalize non-truncation and destination -wal/-shm sidecar cleanup.

Local validation after rebasing onto current main:

  • ASAN_OPTIONS=detect_leaks=0 build/c/test-runner sqlite_writer
  • bash scripts/check-no-test-skips.sh
  • git diff --check

Signed-off-by: Nikita Bige <wargloom@gmail.com>
@WarGloom WarGloom force-pushed the codex/fix-sqlite-writer-atomic-publish branch from 82df8c2 to 076b9a9 Compare July 8, 2026 09:39
@WarGloom WarGloom marked this pull request as ready for review July 8, 2026 12:11
@WarGloom WarGloom requested a review from DeusData as a code owner July 8, 2026 12:11
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 8, 2026
@DeusData

DeusData commented Jul 9, 2026

Copy link
Copy Markdown
Owner

This fixes a real and nasty class — truncating the live DB under concurrent readers (#897's btreeInitPage crash) — and temp-write + atomic publish + discard-on-error is the right design. The tests and the WAL/SHM sidecar cleanup are welcome. Three changes requested:

  1. Windows non-ASCII paths: the new code opens the temp file with raw fopen and publishes via MoveFileExA (ANSI). On Windows, both break for non-ASCII cache paths (e.g. a user profile with umlauts — the cache DB name derives from the repo path). Repo policy is cbm_fopen (UTF-8 → _wfopen) for any user/cache path; please use it for the temp + sidecar writes here, and use MoveFileExW with a UTF-8→wide conversion for the publish (yes, artifact.c has the same ANSI call from Fix Windows artifact refresh in write_file_atomic #492 — that's a latent bug we'd rather not duplicate; feel free to extract a shared cbm_rename_replace() helper and we'll fix artifact.c in a follow-up).

  2. Durability of the publish: there's no fflush + fsync before the rename on the POSIX side (Windows at least passes MOVEFILE_WRITE_THROUGH). Without it, a power loss can atomically publish an incompletely-written file — the exact corruption the temp+rename dance is meant to prevent. fflush(fp); fsync(fileno(fp)); before fclose in the finalize path closes it.

  3. Sidecar removal ordering: please confirm (and comment) that the destination -wal/-shm removal happens only on the success path relative to the rename — removing the WAL of a DB an old reader still has open is fine on POSIX (unlinked-but-open) but fails or misbehaves on Windows if a handle is held; a note on the expected ordering/failure mode would help the next reader.

With those, this is exactly the writer we want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. stability/performance Server crashes, OOM, hangs, high CPU/memory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Installing a fresh DB file leaves the old -wal/-shm behind — SQLite replays the stale WAL over the new file

2 participants