refactor(storage): unify RDB and spill value codecs into one module (W1)#396
Merged
Conversation
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? |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughChangesShared value serialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RDBWriter
participant ValueCodec
participant SpillSerializer
participant SpillDecoder
RDBWriter->>ValueCodec: encode_value_body
ValueCodec-->>RDBWriter: encoded value body
SpillSerializer->>ValueCodec: encode_value_body
ValueCodec-->>SpillSerializer: encoded spill body
SpillDecoder->>ValueCodec: decode_value_body with lenient trailer mode
ValueCodec-->>SpillDecoder: decoded RedisValue
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 2
🤖 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 `@src/persistence/rdb.rs`:
- Around line 1092-1118: The boot-time RDB loading flow still uses
read_entry_zero_copy() instead of the shared value_codec decoder, leaving the
hand-written path inconsistent with read_entry(). Update read_entry_zero_copy()
and its load() call path to use value_codec::decode_value_body() with the
appropriate ValueType and HashTtlTrailer handling, preserving string decoding
and existing error conversion.
In `@src/storage/eviction.rs`:
- Around line 892-898: The eviction and spill paths must fail closed when
serialize_collection returns None. In src/storage/eviction.rs:892-898, update
evict_batch_durable_no_aof to skip and retain the victim while logging the
serialization failure; in src/storage/eviction.rs:1022-1028, make
evict_one_async_spill return without db.remove, matching the try_send failure
path; in src/storage/tiered/kv_spill.rs:146-152, propagate or skip None in
spill_to_datafile instead of writing an empty body.
🪄 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: 2bcaf6de-3790-42bc-acb9-7c76bf68845e
📒 Files selected for processing (7)
CHANGELOG.mdsrc/persistence/rdb.rssrc/storage/eviction.rssrc/storage/mod.rssrc/storage/tiered/kv_serde.rssrc/storage/tiered/kv_spill.rssrc/storage/value_codec.rs
Comment on lines
892
to
898
| let (value_type, value_bytes): (ValueType, &[u8]) = match val_ref { | ||
| RedisValueRef::String(s) => (ValueType::String, s), | ||
| ref other => { | ||
| let vt = match other { | ||
| RedisValueRef::Hash(_) | ||
| | RedisValueRef::HashListpack(_) | ||
| | RedisValueRef::HashWithTtl { .. } => ValueType::Hash, | ||
| RedisValueRef::List(_) | RedisValueRef::ListListpack(_) => ValueType::List, | ||
| RedisValueRef::Set(_) | ||
| | RedisValueRef::SetListpack(_) | ||
| | RedisValueRef::SetIntset(_) => ValueType::Set, | ||
| RedisValueRef::SortedSet { .. } | ||
| | RedisValueRef::SortedSetBPTree { .. } | ||
| | RedisValueRef::SortedSetListpack(_) => ValueType::ZSet, | ||
| RedisValueRef::Stream(_) => ValueType::Stream, | ||
| RedisValueRef::String(_) => unreachable!(), | ||
| }; | ||
| let vt = kv_spill::value_type_of(other); | ||
| collection_buf = kv_serde::serialize_collection(other).unwrap_or_default(); | ||
| (vt, collection_buf.as_slice()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
serialize_collection(...).unwrap_or_default() defeats the new fail-closed codec and can silently drop data. With the codec change, serialize_collection now returns None for an unencodable value (e.g. ValueCodecError::CorruptScore) instead of writing a 0.0 body. unwrap_or_default() converts that None into an empty Vec<u8>, so the corrupt value is spilled as an empty body. In the two eviction paths the hot entry is removed after a "successful" spill, turning a rare in-memory corruption into total data loss on read-back — strictly worse than the prior 0.0-score behavior this PR sought to fix. (PR objectives note these are slated for W2; flagging as the shared blocker.)
src/storage/eviction.rs#L892-L898: inevict_batch_durable_no_aof, do not stage/evict the victim whenserialize_collectionreturnsNone; skip the key (retain hot) and log, mirroring the existing durable-failure retention comments.src/storage/eviction.rs#L1022-L1028: inevict_one_async_spill, bail out (return withoutdb.remove) when serialization returnsNone, same as thetry_sendfailure path.src/storage/tiered/kv_spill.rs#L146-L152: inspill_to_datafile, propagate/skip onNoneinstead of spilling an empty body into the cold index.
📍 Affects 2 files
src/storage/eviction.rs#L892-L898(this comment)src/storage/eviction.rs#L1022-L1028src/storage/tiered/kv_spill.rs#L146-L152
🤖 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/storage/eviction.rs` around lines 892 - 898, The eviction and spill paths
must fail closed when serialize_collection returns None. In
src/storage/eviction.rs:892-898, update evict_batch_durable_no_aof to skip and
retain the victim while logging the serialization failure; in
src/storage/eviction.rs:1022-1028, make evict_one_async_spill return without
db.remove, matching the try_send failure path; in
src/storage/tiered/kv_spill.rs:146-152, propagate or skip None in
spill_to_datafile instead of writing an empty body.
This was referenced Jul 20, 2026
Merged
…umented module (W1)
The value-body wire format existed as two hand-maintained copies:
rdb::write_entry's value section and kv_serde::serialize_collection
(whose doc comment admitted "format identical to rdb.rs" — a promise
kept by discipline, not by the compiler). The decode side was likewise
duplicated. Every format evolution (e.g. the v2 hash-TTL trailer) had
to be applied twice; a missed edit silently corrupts the other plane.
This PR makes src/storage/value_codec.rs the single implementation:
- encode_value_body/decode_value_body cover all collection types;
strings remain container-framed (StringHasNoBody guard so a caller
can never persist an empty body for a string).
- HashTtlTrailer::{Absent,Required,Lenient} models the three container
semantics (RDB v1, RDB v2, unversioned spill blobs with legacy
pre-trailer forgiveness) that previously lived as divergent ad-hoc
logic in the two decoders.
- value_type_of (RedisValueRef -> ValueType) exists once; kv_spill
re-exports it and the two inline copies in eviction.rs delegate.
- Decode hardening: every count field is validated against remaining
input BEFORE Vec::with_capacity (the RDB-DoS fix had missed the
spill decoder — corrupt .mpf pages could drive multi-GB allocations).
- Fail-closed scores: an unparseable sorted-set listpack score now
refuses to encode (ValueCodecError::CorruptScore) instead of silently
writing 0.0 on both planes (red-first test).
Format compatibility is pinned by golden byte-spec tests that rebuild
the expected bytes by hand per type (so the tests pin the FORMAT, not
"whatever the encoder does"), plus legacy pre-trailer lenient-decode
and oversized-count rejection tests. Existing RDB files and spill
pages load unchanged; full lib suite 4419 green.
Groundwork for W2 (single spill pipeline): the three spill call sites
still map encode errors to unwrap_or_default(); W2 turns that into
abort-eviction fail-closed.
author: Tin Dang
TinDang97
force-pushed
the
refactor/w1-value-codec
branch
from
July 20, 2026 05:33
81599ed to
3f4fe04
Compare
TinDang97
added a commit
that referenced
this pull request
Jul 20, 2026
…the durable batch writer (W2) Moon had two spill families: the async/batch path (SpillThread flush_buffer: shared multi-entry files, one manifest commit per file) and a sync per-victim path (kv_spill::spill_to_datafile: ONE single-entry file + ONE shard-thread-blocking durable manifest commit PER KEY) used by the background eviction tick, the memory-pressure cascade's sync fallback, and db_quota. The two evolved independently and that seam already bit three times: the task #34 plain-drop misclassification, the task #45 is_string gate, and the #139 spill-test blindness were all divergence bugs between these copies. This PR makes the batch machinery the ONE spill implementation: - select_victim (policy dispatch) and build_spill_payload (victim serialization) exist once — previously copy-pasted at all three entry points. - evict_batch_durable_no_aof is renamed evict_batch_durable and now backs every SpillContext sync path: the sync wrapper reclaims its whole deficit as batches; evict_one_with_spill's spill arm is a deficit-1 delegation. Write-then-durable-then-drop ordering, D1 remove-before-cold-index, and fail-closed retention on I/O error are inherited, not re-implemented. - Red-first: sync-evicting 40 keys now produces 1 shared .mpf file (was 40 files + 40 blocking manifest fsync round-trips). - Fail-closed serialization (red-first): all three sites previously did serialize_collection(..).unwrap_or_default() — a corrupt value was "spilled" as an EMPTY body and evicted (silent durable data loss on reload). Unserializable victims are now retained hot, skipped, and loudly logged, on the sync, batch, and async paths. - evict_batch_durable now calls record_eviction() per removed key (the batch path previously under-counted the eviction metric). - kv_spill::spill_to_datafile is no longer a production eviction path; it remains the single-entry primitive test fixtures use (same page layout the batch salvage path emits). Gates: lib suite 4421 green (macOS + Linux VM release-fast); eviction pin tests (spill-I/O-failure OOM retention, non-string durable spill, db_index restamp) all green; offload/cold integration suites + crash matrix on Linux VM; clippy -D warnings on default and tokio,jemalloc feature sets. Stacked on refactor/w1-value-codec (PR #396). author: Tin Dang
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 20, 2026
…the durable batch writer (W2) (#397) Moon had two spill families: the async/batch path (SpillThread flush_buffer: shared multi-entry files, one manifest commit per file) and a sync per-victim path (kv_spill::spill_to_datafile: ONE single-entry file + ONE shard-thread-blocking durable manifest commit PER KEY) used by the background eviction tick, the memory-pressure cascade's sync fallback, and db_quota. The two evolved independently and that seam already bit three times: the task #34 plain-drop misclassification, the task #45 is_string gate, and the #139 spill-test blindness were all divergence bugs between these copies. This PR makes the batch machinery the ONE spill implementation: - select_victim (policy dispatch) and build_spill_payload (victim serialization) exist once — previously copy-pasted at all three entry points. - evict_batch_durable_no_aof is renamed evict_batch_durable and now backs every SpillContext sync path: the sync wrapper reclaims its whole deficit as batches; evict_one_with_spill's spill arm is a deficit-1 delegation. Write-then-durable-then-drop ordering, D1 remove-before-cold-index, and fail-closed retention on I/O error are inherited, not re-implemented. - Red-first: sync-evicting 40 keys now produces 1 shared .mpf file (was 40 files + 40 blocking manifest fsync round-trips). - Fail-closed serialization (red-first): all three sites previously did serialize_collection(..).unwrap_or_default() — a corrupt value was "spilled" as an EMPTY body and evicted (silent durable data loss on reload). Unserializable victims are now retained hot, skipped, and loudly logged, on the sync, batch, and async paths. - evict_batch_durable now calls record_eviction() per removed key (the batch path previously under-counted the eviction metric). - kv_spill::spill_to_datafile is no longer a production eviction path; it remains the single-entry primitive test fixtures use (same page layout the batch salvage path emits). Gates: lib suite 4421 green (macOS + Linux VM release-fast); eviction pin tests (spill-I/O-failure OOM retention, non-string durable spill, db_index restamp) all green; offload/cold integration suites + crash matrix on Linux VM; clippy -D warnings on default and tokio,jemalloc feature sets. Stacked on refactor/w1-value-codec (PR #396). author: Tin Dang Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
This was referenced Jul 20, 2026
TinDang97
added a commit
that referenced
this pull request
Jul 20, 2026
…L fidelity (#404) Patch release rolling up the storage-unification stacked train (#396–#401) and the follow-up db.rs directory split (#403). Correctness: millisecond-TTL keys no longer expire up to 999 ms early (CompactEntry stored expiry as ms/1000-floored seconds; now absolute Unix ms end-to-end, exact PTTL readback, exact-ms RDB round-trips). Unification (behavior-preserving, pinned by golden byte-spec tests): one value codec behind RDB/spill/kv_serde with fail-closed corrupt decode + DoS count guards on every path; one spill pipeline — sync eviction batches victims into shared files (~N/256 manifest fsyncs instead of N per-key) and retains unserializable victims fail-closed; one eviction entry point (evict_to_budget + EvictionRun/EvictionSink, was 13 variants / 5 reclaim-loop copies); one typed-accessor skeleton (storage::db_kind static-dispatch markers); AofShardManifest rename; db.rs split into db/{mod,hash_ttl,kv_ops,accessors}.rs. Validation: same-VM A/B v0.8.1-vs-main — p=1 parity-or-better, eviction-plain faster every paired round, spill +3%, p=16 within noise. Release gate: crash-matrix nightly full matrix + ITERS=20 soak dispatched on the RC, green before tag (soak-first-then-tag). Rolls CHANGELOG [Unreleased] into [0.8.2], bumps Cargo.toml/lock, adds the RELEASES.md row, updates the README milestone table. author: Tin Dang
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
First PR of the storage-layer unification wave (W1 of W1–W6, plan reviewed 2026-07-20).
The value-body wire format existed as two hand-maintained copies —
rdb::write_entry's value section andkv_serde::serialize_collection(~150 lines each, "format identical to rdb.rs" kept true by discipline only). This PR makessrc/storage/value_codec.rsthe single implementation; both call sites delegate.encode_value_body/decode_value_body+HashTtlTrailer::{Absent,Required,Lenient}modeling the three container semantics (RDB v1 / RDB v2 / unversioned spill blobs with legacy pre-trailer forgiveness).value_type_ofexists once —kv_spillre-exports it; the two inline copies ineviction.rsnow delegate..mpflength fields went straight intoVec::with_capacity. Every count is now validated before allocation on both planes.ValueCodecError::CorruptScore) instead of silently persisting0.0on both planes.Format compatibility
Byte-identical by construction, pinned by golden byte-spec tests that rebuild expected bytes by hand per type (tests pin the FORMAT, not "whatever the encoder does"), plus legacy pre-trailer lenient-decode and oversized-count rejection tests. Existing RDB files and spill pages load unchanged.
Testing
-D warningson default ANDruntime-tokio,jemallocfeature sets; fmt cleanFollow-up (W2): the three spill call sites still
unwrap_or_default()on encode failure — W2 turns that into abort-eviction fail-closed as part of the single-spill-pipeline unification.Summary by CodeRabbit
Changed
Bug Fixes
0.0), preventing silent data corruption.