fix(persistence): adopt atomic_write_durable at 8 remaining bare-write sites (task #49)#304
Conversation
…-write sites 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
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? |
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 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 |
… change needed (#347) Task #49 (v0.8 item 1, kernel review) asked to adopt `atomic_write_durable` at the remaining bare-write persistence sites: ACL SAVE, cluster nodes.conf, CONFIG REWRITE, replication state, native BGSAVE (RDB), clog, kv_page. Audited every site against current HEAD (site-by-site, not a blanket grep pass): - src/acl/io.rs::acl_save -- atomic_write_durable - src/command/acl.rs SAVE handler -- routes through acl::io::acl_save (same call site, not a second hand-rolled sequence) - src/cluster/migration.rs::save_nodes_conf -- atomic_write_durable - src/command/config.rs::config_rewrite -- atomic_write_durable - src/replication/state.rs::save_replication_state -- atomic_write_durable - src/persistence/rdb.rs::save / save_from_snapshot -- atomic_write_durable - src/persistence/redis_rdb.rs::save -- atomic_write_durable - src/persistence/clog.rs::write_clog_page -- atomic_write_durable - src/persistence/kv_page.rs::write_datafile / write_datafile_mixed -- atomic_write_durable All nine call sites across the seven named categories were already converted in PR #304 (merged 2026-07-13, commit 4e0688e), which is an ancestor of current HEAD, and its CHANGELOG entry is filed under the v0.7.0 release section (not Unreleased) -- the fix shipped in v0.7.0. `git log 4e0688e..HEAD` on every touched file shows no subsequent commit reverted or bypassed the conversion. Also checked adjacent hand-rolled writers for regression risk: storage/tiered/warm_tier.rs (staging-dir -> final-dir rename is its own documented directory-level atomicity protocol; per-file .mpf writes inside the staging dir are covered by that rename, not a bare-write gap) and storage/tiered/kv_spill.rs::write_kv_spill_batch (already hand-rolled tmp+fsync+rename+dir-fsync correctly; not one of the 7 named sites). Both correctly out of scope -- no change made. The only real gap found was in documentation: docs/roadmap/ROADMAP.md's Rev 2 pass (task #68, commit 82b3bca, dated 2026-07-15 -- one day after PR #304 merged and the same day v0.7.0/v0.7.1 released) had re-listed task #49 as an open v0.8 gap (gap table, v0.8.0 item 1, debt register) without cross-checking it against the already-shipped v0.7.0 CHANGELOG entry. This commit corrects the roadmap: the gap-table row is removed, the v0.8.0 §4 item and the §5 debt-register row are struck through with a "shipped v0.7.0 / PR #304" note (not deleted, to preserve the audit trail), and a dated correction note is added alongside the existing Rev 2 note. CHANGELOG.md [Unreleased] gets a matching Documentation entry recording the audit and its outcome. No src/ changes. No new tests -- the eight regression tests from PR #304 already cover every conversion ("no leftover temp file after a successful write" per site); re-ran the full lib test suite locally to confirm they still pass with no regression since the merge. author: Tin Dang <tindang.ht97@gmail.com> Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
2 participants
Summary
Task #49 (kernel M4 prep): eight durable-state writers still hand-rolled their own write sequence instead of the shared K3 primitive (
atomic_write_durable, PR #296: temp → sync_all → rename → dir-fsync).acl/io.rs,command/acl.rs).mpfdatafiles (write_datafile/_mixed)A rename without temp-file + dir fsync can surface as an EMPTY or stale file post-kill-9 on data=ordered ext4/xfs. All sites now route through the shared primitive; each conversion has a regression test (8 new). Skipped: the dead
rewrite_aof_syncRDB-preamble path (dead code); AOF/WAL out of scope (own mechanisms).Gates
legacy_yes_s1_mq_isolated) was 3/3 green in isolation — known timing-flake class, unrelated.Refs: task #49, K3 (PR #296).