docs(roadmap): task #49 atomic-write sweep — audit closes it, no code change needed#347
Merged
Merged
Conversation
… change needed 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>
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? |
📝 WalkthroughWalkthroughTask ChangesTask
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@docs/roadmap/ROADMAP.md`:
- Around line 38-44: Update the roadmap correction note to accurately reference
the CHANGELOG section containing Task `#49`’s audit entry, aligning it with the
actual section used in CHANGELOG.md. Preserve the surrounding correction details
and removal/striking context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d653fb8d-ae34-48a2-9389-b416616d237d
📒 Files selected for processing (2)
CHANGELOG.mddocs/roadmap/ROADMAP.md
Comment on lines
+38
to
+44
| *(2026-07-16 correction: this Rev 2 pass had re-listed "atomic-write stragglers (task #49)" as | ||
| an open v0.8 gap below and as v0.8 item 1 in §4. Verified against the code during v0.8 close-out | ||
| work: all 7 sites (ACL SAVE, nodes.conf, CONFIG REWRITE, replication state, native BGSAVE, | ||
| clog, kv_page) were already converted to `atomic_write_durable` in PR #304 — merged 2026-07-13, | ||
| *before* this Rev 2 pass, and its CHANGELOG entry landed under the v0.7.0 release section, not | ||
| Unreleased. Task #49 was never actually an open v0.8 item; the row below and the item in §4 have | ||
| been removed/struck accordingly.)* |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Correct the CHANGELOG section referenced here.
CHANGELOG.md places the new Task #49 audit under [Unreleased], but this text says it landed under the v0.7.0 release section. Update one side so the release history is consistent.
Proposed fix
- its CHANGELOG entry landed under the v0.7.0 release section, not
- Unreleased.
+ its CHANGELOG entry is recorded under the current Unreleased section.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| *(2026-07-16 correction: this Rev 2 pass had re-listed "atomic-write stragglers (task #49)" as | |
| an open v0.8 gap below and as v0.8 item 1 in §4. Verified against the code during v0.8 close-out | |
| work: all 7 sites (ACL SAVE, nodes.conf, CONFIG REWRITE, replication state, native BGSAVE, | |
| clog, kv_page) were already converted to `atomic_write_durable` in PR #304 — merged 2026-07-13, | |
| *before* this Rev 2 pass, and its CHANGELOG entry landed under the v0.7.0 release section, not | |
| Unreleased. Task #49 was never actually an open v0.8 item; the row below and the item in §4 have | |
| been removed/struck accordingly.)* | |
| *(2026-07-16 correction: this Rev 2 pass had re-listed "atomic-write stragglers (task `#49`)" as | |
| an open v0.8 gap below and as v0.8 item 1 in §4. Verified against the code during v0.8 close-out | |
| work: all 7 sites (ACL SAVE, nodes.conf, CONFIG REWRITE, replication state, native BGSAVE, | |
| clog, kv_page) were already converted to `atomic_write_durable` in PR `#304` — merged 2026-07-13, | |
| *before* this Rev 2 pass, and its CHANGELOG entry is recorded under the current Unreleased section. | |
| Task `#49` was never actually an open v0.8 item; the row below and the item in §4 have | |
| been removed/struck accordingly.)* |
🤖 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 `@docs/roadmap/ROADMAP.md` around lines 38 - 44, Update the roadmap correction
note to accurately reference the CHANGELOG section containing Task `#49`’s audit
entry, aligning it with the actual section used in CHANGELOG.md. Preserve the
surrounding correction details and removal/striking context.
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 16, 2026
…evidence (#356) Re-ran the G2 acceptance benchmark (first run 2026-07-13 @ 983952a) on main @ 4dcfd53 with the complete v0.8 storage batch merged (#347 t49 audit, #350 spill batching, #352 crash-matrix CI, #349 used_memory truth, #353 vec-merge backoff). moon-dev VM, real-disk btrfs --dir, appendonly=yes, 4 shards, 256MB cap, 260K x 10KB (2.6GB = 10x), runner verified idle for all timed phases. Results vs baseline: - Spill files for the full 2.6GB: ~236,000 -> 840 (~280x, PR #350). - used_memory: 406-762MB (RSS masquerading as the ledger) -> 1.00x cap at steady state; post-restart transient (AOF replay re-heats keys) drains to 255MB (< cap) within 5s and holds — demote pass logged "demoted 83,788 AOF-replay hot shadow(s)" (task #56, PR #349). - Cold GET during active spill flood: worst 1,910ms -> 205ms (9.3x). Task #59 (per-shard read-vs-spill-writer fairness) remains open. - kill-9 restart-to-PONG: 16.9s, decomposed as ~2s/shard cold-index rebuild (179K entries) + ~9.4s AOF incr replay (340K cmds / 3.3GB, no rewrite had fired). Criterion "readiness not O(spilled-keys)" holds — the cost is AOF-sized, the standard Redis-style bound; file count is out of the boot path. - Integrity: 497/500 at first read (3 read-side artifacts during the post-replay drain), 500/500 on rediff after settle. Zero acked loss. New finding filed as #355: DBSIZE counts only resident keys under disk-offload (24,275 reported vs ~164K logical) — pre-existing semantics gap, made visible by this investigation, non-blocking. Also updates PRODUCTION-CONTRACT.md: CRASH-02 row (matrix 37->46 cells, all green ungated, scheduled CI via PR #352), MEM-10X-01 row (this re-run's evidence), and a 2026-07-16 audit-trail entry. author: Tin Dang <tindang.ht97@gmail.com> Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 16, 2026
…ne + 10× RAM datasets (#360) Version 0.7.1 -> 0.8.0 (Cargo.toml + Cargo.lock), CHANGELOG [0.8.0] roll-up, RELEASES.md ledger entry, ROADMAP v0.8.0 section flipped to SHIPPED (all 6 items ticked; exit-criterion wording corrected 42 -> 46 cells). Release converts the storage kernel built during the v0.7.0 cycle into a verifiable public claim: - CRASH-02: 46-cell cross-plane kill-9 matrix green ungated, in scheduled CI (nightly full + Saturday ITERS=20 soak, PR #352) - MEM-10X-01: G2 re-run on real disk (docs/perf/2026-07-16-g2-10x-ram-rerun.md, PR #356) — spill 236K -> 840 files (#350), used_memory 1.00x cap with <=5s post-restart drain (#349), cold-GET tail 1910 -> 205ms - Plus: vector auto-merge CPU-livelock fix (#353), CI recall-canary offload (#354), task #49 audit-close (#347) Disclosed follow-ups: task #59 read-vs-spill fairness (205ms tail, goal <10ms), issue #355 DBSIZE resident-only under offload, restart time bounded by AOF-rewrite cadence. Tag gate: crash-matrix nightly full matrix + soak ITERS=20 both dispatched on RC 82752af. author: Tin Dang <tindang.ht97@gmail.com> 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
Task #49 (v0.8 item 1) close-out: the audit found no code change needed — all 7 bare-write sites named by the storage-kernel review (ACL SAVE, cluster
nodes.conf, CONFIG REWRITE, replication state, native BGSAVE ×3 writers,clog/kv_page) already route throughatomic_write_durable(temp →sync_all→ rename → dir-fsync), shipped in PR #304 and released with v0.7.0.git logon every touched file confirms none of the 9 converted call sites were reverted or bypassed since.The Rev 2 roadmap pass (task #68, PR #337) had re-listed #49 as an open v0.8 gap without re-verifying against HEAD; this PR corrects
docs/roadmap/ROADMAP.md(§1 gap table, §4 v0.8 item 1, §5 debt register) and records the audit in the CHANGELOG. Adjacent hand-rolled writers (warm_tier.rsstaging-dir rename protocol,kv_spill.rsbatch writer) were audited and confirmed correctly out of scope.Verification
Docs-only. Audit evidence in the CHANGELOG entry; independently spot-verified (atomic_write_durable usages present in acl/io.rs, command/config.rs, replication/state.rs, persistence/{rdb,redis_rdb,clog,kv_page}.rs, cluster/migration.rs).
Summary by CodeRabbit