fix(persistence): K3 — shared atomic_write_durable + graph/text/vector crash-durability sweep (kernel M2 stage 1)#296
Merged
Conversation
Kernel M2 stage 1 (K3): every durable on-disk artifact in this codebase hand-rolled its own copy of temp-write -> fsync -> rename -> dir-fsync, and several copies silently dropped a step (usually the directory fsync). Extract the sequence the vector engine's Stack-B sidecar writers (write_manifest_atomic / write_keymap_atomic) got right into one shared `atomic_write_durable(path, bytes)` in src/persistence/atomic.rs, so every adoption site in this sweep calls the same audited code path instead of re-deriving it. Adds a typed AtomicWriteError (thiserror) tagging which stage of the sequence failed (WriteTemp / SyncTemp / Rename / SyncDir) so a caller gating durability-sensitive progress (a WAL checkpoint, a manifest write) can tell "never became durable" apart from "is live but the last fsync could not be proven" -- both must be treated as failures, but the distinction matters for logs and retry policy. Converts to io::Error for drop-in use at existing io::Result<()> call sites. Unit tests cover the documented contract: roundtrip correctness, overwrite semantics, no leftover temp file on success, fail-loud (not panic) on a missing parent directory and on a forced WriteTemp failure with the original file proven untouched, and a concurrent reader/writer stress test asserting a reader NEVER observes a torn write -- the property a regression to a bare fs::write would break. This is the shared primitive only; adoption at the graph CSR segment, text sidecar, and other flagged call sites lands in follow-up commits on this branch. Ref: .planning/reviews/kernel-m2-brief-2026-07-12.md (K3, stage 1) Ref: .planning/reviews/storage-architecture-synthesis-2026-07-12.md §2 author: Tin Dang
K3 site 1 (kernel-m2-brief-2026-07-12.md). CsrSegment::write_to_file
was a bare std::fs::write: no fsync, and the final path was
overwritten in place rather than replaced via rename. A crash
mid-write left a torn segment directly at the path GraphManifest
already references (audit: storage-audit-2026-07-12-graph-fts.md --
"manifest referencing a segment is more durable than the segment
itself"), and even a clean shutdown gave the OS no durability
guarantee for the bytes.
RED-first: added
test_write_to_file_concurrent_rewrite_never_exposes_torn_segment,
which alternates two differently-sized/differently-LSN'd segments at
the same path from a writer thread while a reader thread repeatedly
reads back through CsrSegment::from_file. Confirmed it fails against
the pre-fix bare fs::write (InvalidData("data shorter than header") --
a genuine torn read caught within the first iteration). Fix: route
write_to_file through the new atomic_write_durable primitive (temp +
fsync + rename + dir-fsync). Test now green, 45/45 graph::csr tests
and 544/544 graph:: tests pass.
save_graph_store (src/graph/recovery.rs) already writes segments
before GraphManifest::save runs, so payload-before-reference ordering
was already correct at the call-site level -- this fix closes the
per-file atomicity gap underneath it.
author: Tin Dang
K3 site 2 (kernel-m2-brief-2026-07-12.md). save_text_index_metadata
and save_fst_sidecar (src/text/index_persist.rs) already did
write-temp + fsync-temp + rename, but never fsync'd the parent
directory afterward -- on data=ordered-journaled filesystems a crash
between rename and dir-fsync can revert the directory entry to the
old name even though rename() returned success, so the "atomic"
replace was not actually durable. Audit:
storage-audit-2026-07-12-graph-fts.md ("text sidecar rename lacks
dir-fsync; graph manifest has it; vector has it").
Route both writers through atomic_write_durable, which folds in the
missing dir-fsync. Added a regression guard per site
(test_save_leaves_no_leftover_temp_file /
test_save_fst_sidecar_leaves_no_leftover_temp_file) pinning that the
shared primitive's temp-file convention is what's actually in use.
Dropped the now-unused std::io::Write import. 162/162 text:: tests
pass.
author: Tin Dang
K3 site 3 (kernel-m2-brief-2026-07-12.md). TextStore::load_fst_sidecars had zero callers -- save_fst_sidecar_for_index wrote a durable FST term-dict sidecar at every FT.COMPACT, but nothing ever read it back, so fuzzy/prefix queries silently brute-forced the HashMap on every restart until the next compact (audit: storage-audit-2026-07-12-graph-fts.md -- "FST sidecar durably written, never loaded (dead API)"; the doc comment on load_fst_sidecars already claimed it ran during recovery -- false until now). Decision: wire the load (not delete the writes). The call site was low-risk to add: src/shard/event_loop.rs already restores text index shells from text-indexes.meta and sets persist_dir on text_store before that point, so `s.text_store.load_fst_sidecars()` after the restore loop is a single call using state that already exists. Ordering is safe because load_fst_sidecars only ever touches `fst_maps` (a lookup-acceleration structure); it never overlaps with the auto-reindex keyspace rescan that follows and rebuilds postings, so which one runs first doesn't matter. A missing sidecar (pre-K3 data, or an index never FT.COMPACT'd) leaves fst_maps at None per field -- the existing, already-tested fallback path (test_fst_sidecar_missing_returns_empty), never an error. No new integration test added: the wiring is a single call reusing plumbing already covered by store.rs's fst sidecar roundtrip unit tests (save/load byte-for-byte, missing-sidecar fallback); a restart-driven FT.SEARCH FUZZY integration test would be the higher-fidelity check but is out of proportion to a one-line call-site change under the current effort budget -- flagged here rather than silently skipped. author: Tin Dang
K3 grep sweep (kernel-m2-brief-2026-07-12.md, item 4). Found by
grepping for remaining bare fs::write / File::create+write_all on
durable artifacts: save_index_metadata_v3
(src/vector/index_persist.rs) had the same gap just fixed in the text
sidecars -- write-temp + fsync-temp + rename, but no directory fsync,
so the rename was not provably durable across a crash on
data=ordered-journaled filesystems.
Routes through the shared atomic_write_durable primitive. Added
test_save_leaves_no_leftover_temp_file as a regression guard (mirrors
the text sidecar commit). Dropped the now-unused std::io::Write
import. 22/22 vector::index_persist:: tests pass.
Not converted in this sweep (documented as judgment calls, not bugs
in the sense of missing durability -- see the branch summary for the
full list and rationale): several other bare-write/tmp+rename sites
already implement the full four-step sequence correctly today
(src/persistence/{manifest,snapshot,control}.rs,
src/vector/persistence/{manifest,segment_io}.rs,
src/graph/manifest.rs, src/storage/tiered/{kv_spill,warm_tier}.rs) --
converting these to the shared primitive is pure de-duplication with
no correctness change, deferred to avoid unnecessary diff/regression
surface on working hot paths. A second group has NO fsync/atomicity
at all (ACL save, CONFIG REWRITE, cluster nodes.conf, replication
state, Redis-format RDB save) -- real gaps, but each sits on a
different plane's admin/hot path and deserves its own dedicated PR
with a targeted crash test rather than being folded into this
mechanical K3 sweep; flagged as K3-sweep-2 follow-up candidates.
author: Tin Dang
Records the graph CSR / text sidecar / vector sidecar durability fixes and the FST sidecar load wiring landed on this branch, per repo convention (CI Lint gate requires a CHANGELOG [Unreleased] entry per PR). author: Tin Dang
…e_durable Adversarial review (P1, CONFIRMED): src/persistence/atomic.rs:159-161 had a bare .expect() in audited library code, failing scripts/audit-unwrap.sh (1 unannotated over baseline 0). The invariant was genuinely safe -- temp_path_for always builds tmp_path via parent.join(...), so tmp_path.parent() cannot return None in practice -- but repo policy (CLAUDE.md: "No unwrap()/expect() in library code outside tests") prefers let-else over an #[allow] annotation when a typed error is already available at the call site. Restructured to a let-else that returns AtomicWriteError::NoParentDir (the same variant used a few lines above for the equivalent case), so a hypothetical future change to temp_path_for's contract fails loud with a typed error instead of panicking. scripts/audit-unwrap.sh now reports 0. 9/9 persistence::atomic:: tests pass. author: Tin Dang
…start
Reverts commit 06c8dc20 ("feat(text): wire load_fst_sidecars into
shard startup recovery").
Adversarial review (P0, CONFIRMED real correctness bug): loading the
FST sidecar after restart corrupts FUZZY/PREFIX search results.
Root cause chain --
1. TermDictionary::get_or_insert assigns term_ids sequentially by
first-encounter order.
2. The post-restore auto-reindex rescan rebuilds every text index's
term dict from DashTable hash iteration order, which is NOT
reproducible across restarts.
3. The .fst sidecar bakes in the OLD generation's term_ids with no
corpus fingerprint tying it to the rescan that follows.
4. fst_high_water_mark stays 0, so expand_terms happily merges
stale-FST ids that now collide with unrelated freshly-assigned
ids from the new generation -- silently wrong search results,
not even a detectable error.
The load cannot be made correct without persisting the term
dictionary itself (so ids survive restart, or the sidecar can be
fingerprint-checked against the corpus and discarded on mismatch).
That is kernel M4 scope (FTS content persistence) -- filed as task
#50. Reverting keeps history honest rather than force-pushing it away.
Kept from the original commit's sibling work (not reverted): the
sidecar WRITE path and its dir-fsync fix (commit 3b770358) -- the
on-disk format is fine, it's the load-without-term-dict-persistence
that's unsafe. TextStore::load_fst_sidecars itself (the function) is
untouched by this revert; a follow-up commit documents on it directly
why it must stay uncalled until task #50 lands.
author: Tin Dang
Follow-up to the revert of commit 06c8dc20 (see the revert commit message for the full root-cause chain: restart-rescan term_ids are not reproducible across restarts, the .fst sidecar bakes in the old generation's ids with no corpus fingerprint, fst_high_water_mark stays 0, and stale-FST ids silently collide with fresh ones -> wrong FUZZY/ PREFIX results). The function's old doc comment claimed it was "called during startup/ recovery" -- already false before the revert (audit finding) and would have stayed misleading after it. Replaced with an explicit "do not wire this in" section spelling out the failure chain and pointing at task #50 (kernel M4, FTS content/term-dict persistence) as the prerequisite, so the next person doesn't rediscover the bug by re-wiring it. Behavior unchanged -- doc-only. author: Tin Dang
…ollision
Adversarial review P2a + P2b.
P2a: temp_path_for used a fixed `.{filename}.tmp` name regardless of
caller -- undocumented trap for any future caller that doesn't hold
the single-writer-per-path invariant every current call site (graph
segments write-once per id, sidecars written by one shard thread at a
time) happens to hold today. Hardened rather than only documented
(reviewer's preferred option, confirmed it doesn't disturb the
existing regression tests): temp_path_for now mixes in a
caller_fingerprint deterministic per (process, calling thread) --
same thread predicting its own temp path twice gets the same answer
(needed by test_original_untouched_when_temp_write_fails, which
precomputes the path atomic_write_durable will use internally, and
now has a dedicated determinism test), while two different threads or
processes racing the same final path get distinct temp names and can
no longer stomp each other's unfsynced bytes mid-write. Documented in
the module docs what this does and does not solve: no more temp-file
corruption, but two racing writers to the same final path still have
a last-writer-wins race at the rename step -- inherent to any
rename-based scheme, not something this module can resolve; callers
still own serializing writers to a given artifact.
Added test_temp_path_is_deterministic_for_same_thread and
test_temp_path_differs_across_threads_for_same_final_path; updated
test_temp_path_is_hidden_and_same_directory to check the naming
pattern (dir/hidden-prefix/.tmp-suffix) instead of an exact string
match now that a fingerprint sits in the middle. 11/11
persistence::atomic:: tests pass.
P2b: added a Windows notes section to the module docs --
rename-over-open-file semantics differ from POSIX
(ERROR_SHARING_VIOLATION without FILE_SHARE_DELETE on the open
handle), and a pointer to fsync_directory's existing no-op-on-Windows
doc for the directory-fsync step. Doc-only, no behavior change.
author: Tin Dang
The Unreleased K3 entry still advertised load_fst_sidecars as wired; adversarial review proved the load corrupts FUZZY/PREFIX results (stale id-space vs the restart rescan) and the wiring was reverted. Entry now records the revert, the why, and the task #50 pointer. author: Tin Dang
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughA shared durable atomic-write primitive was added and applied to graph CSR segments, text metadata and FST sidecars, and vector index metadata. Tests cover concurrent rewrites and temporary-file cleanup, while documentation records the FST sidecar loading constraint. ChangesDurable persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
CHANGELOG.md (1)
38-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider referencing task
#49(remaining non-atomic sites) alongside task#50.This entry documents the FST-sidecar gap (task
#50) but omits that other non-atomic write sites are still tracked separately (task#49, per the PR description). A brief mention would prevent readers from assuming this PR closes all durability gaps in the codebase.As per PR objectives, "Additional non-atomic sites are tracked as task
#49."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 38 - 49, Add a brief reference to task `#49` in the FST term-dict sidecar changelog entry, noting that remaining non-atomic write sites are tracked separately and this PR does not address all durability gaps; retain the existing task `#50` persistence reference.src/graph/csr/mod.rs (1)
1009-2192: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftFile exceeds the 1500-line guideline; this PR adds further to an already-oversized file.
src/graph/csr/mod.rsruns well past 1500 lines (themod testsblock alone spans roughly 1009–2192+). This PR's new test adds ~65 more lines without splitting. Per repo guidelines, files should be split (e.g. read/write implementations, or command-group into directory modules) once approaching this limit.As per coding guidelines, "No single Rust file should exceed 1500 lines. Split command-group files into directory modules when approaching the limit; split read and write implementations above 1000 lines."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graph/csr/mod.rs` around lines 1009 - 2192, Split the oversized `src/graph/csr/mod.rs` into focused modules, moving the large `mod tests` block—especially the persistence, mmap, migration, label-overflow, and property-blob tests—into dedicated test modules while keeping them able to access the CSR implementation. Also separate read/write implementation code when appropriate so no Rust file exceeds the 1500-line guideline and read/write sections remain below 1000 lines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 38-49: Add a brief reference to task `#49` in the FST term-dict
sidecar changelog entry, noting that remaining non-atomic write sites are
tracked separately and this PR does not address all durability gaps; retain the
existing task `#50` persistence reference.
In `@src/graph/csr/mod.rs`:
- Around line 1009-2192: Split the oversized `src/graph/csr/mod.rs` into focused
modules, moving the large `mod tests` block—especially the persistence, mmap,
migration, label-overflow, and property-blob tests—into dedicated test modules
while keeping them able to access the CSR implementation. Also separate
read/write implementation code when appropriate so no Rust file exceeds the
1500-line guideline and read/write sections remain below 1000 lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7eb15be7-74f6-440d-835b-99e8ee4a9da9
📒 Files selected for processing (7)
CHANGELOG.mdsrc/graph/csr/mod.rssrc/persistence/atomic.rssrc/persistence/mod.rssrc/text/index_persist.rssrc/text/store.rssrc/vector/index_persist.rs
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 13, 2026
…-write sites (#304) Task #49 (kernel M4 prep): eight durable-state writers still hand-rolled their own tmp-write/rename sequence instead of the shared K3 primitive (src/persistence/atomic.rs, PR #296). ACL SAVE (both acl::io::acl_save and the ACL SAVE command handler) had zero atomicity at all -- a bare fs::write + rename with no fsync whatsoever. CONFIG REWRITE, cluster nodes.conf (save_nodes_conf), and replication.state (save_replication_state) all did write+rename but skipped both the temp-file fsync and the parent-directory fsync, so a kill-9 immediately after rename() returns can still revert the directory entry to stale or empty contents on data=ordered ext4/xfs. persistence::clog::write_clog_page and persistence::kv_page::write_datafile / write_datafile_mixed wrote directly to the FINAL path with no temp file or rename at all (only a trailing sync_all/fsync_file), so a crash mid-write could leave a torn CLOG page or KV heap data file visible to a concurrent reader. The native RDB save paths used by BGSAVE (persistence::rdb::save / save_from_snapshot, persistence::redis_rdb::save) also lacked the parent-directory fsync step. All eight sites now route through atomic_write_durable (temp file -> sync_all -> rename -> parent-dir fsync), matching the pattern already proven at the graph CSR / text sidecar / vector Stack-B call sites from PR #296. Each conversion is paired with a "no leftover temp file after a successful write" regression test (8 new tests total). Out of scope per task brief: AOF appends, WAL segments (own framing/fsync mechanisms), and the legacy #[allow(dead_code)] rewrite_aof_sync RDB-preamble path (dead code, not a live BGSAVE path). Verified: cargo test --lib (default features + runtime-tokio,jemalloc), cargo fmt --check, cargo clippy -D warnings (both feature sets). author: Tin Dang Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Kernel M2 stage 1 (K3 from the storage-architecture review): one shared crash-consistent write primitive, adopted at every durable-artifact site the K3 audit flagged. Closes real kill-9 torn-write windows on the graph plane and dir-entry-revert windows on text/vector sidecars. Brief:
.planning/reviews/kernel-m2-brief-2026-07-12.md.What's in here
src/persistence/atomic.rs(new):atomic_write_durable(path, bytes)— temp-in-same-dir →sync_all→rename→ dir-fsync — extracted from the vector Stack-B reference. TypedAtomicWriteError(thiserror, stage-tagged,From → io::Error), collision-hardened temp naming (per-process+thread fingerprint), documented payload-before-reference rule + single-writer-per-path residual + Windows notes. 11 unit tests incl. a concurrent reader/writer rename-atomicity stress test.CsrSegment::write_to_file): was a bare in-placefs::write— a crash mid-write left a torn segment at the exact pathGraphManifestreferences. Proven RED-first with a torn-read regression test, then converted. Graph kill-9 crash tests G4/G5 green on Linux.load_fst_sidecarswas reverted after adversarial review proved a loaded FST's term-ids are stale id-space garbage against the restart rescan's rebuilt dictionary, silently corrupting FUZZY/PREFIX results. Now documented on the function; the real fix (persist the term dictionary) is task P2-1: Cargo feature gate for disk-offload (~50 KLOC always compiles in) #50 / kernel M4.Out of scope, filed
Task #49: seven more genuinely non-atomic sites found by the sweep's grep audit (ACL SAVE — no atomicity at all, cluster nodes.conf, CONFIG REWRITE, replication state, native BGSAVE path, clog/kv_page) — each is a different plane's admin/hot path deserving its own PR + crash test.
Gates
audit-unwrap0/0,audit-unsafe256/256,persistence::atomic11/11,text::162/162,graph::csr60/60.replication_graph3/3, lib both runtimes — see CI. (G1–G3 legacy-v2 failures verified pre-existing on base1180f485via isolated-worktree A/B; nothing in this diff touches legacy paths.)Kernel M2 stage 1 of 2; K4 accounting follows.
Summary by CodeRabbit
Bug Fixes
Documentation