fix(storage): cold-spilled collections invisible to their own commands + write shadowing (task #41, P0)#287
Merged
Conversation
Disk-offload spills Hash/List/Set/ZSet/Stream values to the cold tier via the production eviction paths (evict_one_async_spill, evict_batch_durable_no_aof in src/storage/eviction.rs), but the type-specific accessors in src/storage/db.rs never consult the cold index before this change. That means an evicted collection reads back as absent (HGET/HGETALL/LRANGE/SMEMBERS/ZRANGE/EXISTS all report the key missing) and a subsequent write fabricates a brand-new EMPTY container that permanently shadows the cold copy on the next flush — silent, unrecoverable user data loss for any collection key subject to eviction under the default disk-offload configuration. Add the failing (RED) evidence for this P0 bug ahead of the fix: - 13 unit tests in storage::db::tests, using a new db_with_spilled_value() test helper that spills a collection via the same spill_to_datafile() production helper the eviction paths use (mirrors the pattern already used by tiered::cold_read's own tests), then exercises get_or_create_hash/list/set/sorted_set/stream, get_hash_ref_if_alive/get_list_ref_if_alive/get_set_ref_if_alive/ get_sorted_set_ref_if_alive/get_stream_if_alive, and exists/ exists_if_alive against the resulting cold-only key. - tests/cold_collection_visibility.rs: a new black-box suite that spawns a real server with --disk-offload enable, drives real LRU eviction of small Hash/List/Set/ZSet probe keys, and asserts EXISTS, read-after- evict, and write-then-read-merges-with-promoted-data for each type. Wire-level on purpose: dispatch-wiring bugs of this shape are not reliably caught by storage-layer unit tests alone. Both suites currently fail on the feature assertion (confirmed against this commit): the unit tests report fabricated empty containers / false-negative EXISTS, and the black-box suite reports EXISTS == 0 and empty reads after eviction for all four collection types. The next commit makes these green. Ref: .planning/reviews/storage-audit-2026-07-12-kv.md author: Tin Dang
… (task #41, P0) Disk-offload is on by default. Its eviction paths already spilled Hash/List/Set/ZSet/Stream values to the cold tier correctly (kv_serde::serialize_collection), but the type-specific read/write accessors in Database never consulted the cold index — only the generic string-oriented get()/set()/del() did. Result: HGET/HGETALL/ LRANGE/SMEMBERS/ZRANGE/EXISTS reported an evicted collection key as absent, and HSET/LPUSH/SADD/ZADD/XADD silently fabricated a brand-new EMPTY container in its place. Since the fabricated container is what gets spilled on the next eviction, the cold copy was permanently shadowed and unrecoverable — a genuine, silent data-loss bug for any collection subject to eviction. Design: a single promote-if-cold hook, not a rewrite of every accessor. Database::promote_cold_if_present(key, now_ms) -> bool On a hot miss, does exactly one ColdIndex lookup. On a hit it decodes the cold value, installs it hot via the existing `set()` path, and removes the ColdIndex entry (one owning copy, never a dual reference). On an expired cold entry it evicts the ColdIndex entry and reports absent. Hot-hit callers pay nothing extra: the hook is only reached after an existing `contains_key` miss. All 8 mutating accessors that used to fabricate on a miss now call this hook first, so a fabrication only happens on a genuine double-miss (neither hot nor cold): get_or_create_hash[_listpack], get_or_create_list[_listpack], get_or_create_set/get_or_create_intset, get_or_create_sorted_set, get_or_create_stream, get_hash, get_list, get_set, get_sorted_set, get_stream[_mut]. Because kv_serde::deserialize_collection always decodes to the full expanded RedisValue (never the compact listpack/intset in-RAM-only encodings), the compact "try this encoding first" accessors naturally fall through to the full accessor after promotion — no command-layer changes needed in hash/list/set write paths. The four `&self`-typed shared-read accessors (get_hash_ref_if_alive, get_list_ref_if_alive, get_set_ref_if_alive, get_sorted_set_ref_if_alive, get_stream_if_alive) are also called by the `_readonly` dispatch path holding only a shared &Database, so they cannot promote. They instead do a non-promoting cold read-through: HashRef/ListRef/SetRef/SortedSetRef gain `Owned`/`OwnedWithTtl` variants holding a freshly cold-decoded value with no backing hot entry; get_stream_if_alive's return type changes from Result<Option<&StreamData>, Frame> to Result<Option<StreamRef<'_>>, Frame>, where StreamRef derefs transparently to &StreamData over either a Borrowed(&'a StreamData) or an Owned(Box<StreamData>) — existing field/method call sites at every `_ref_if_alive`/`_if_alive` caller are unchanged. Fast path (hot hit) is a single probe, unchanged; a hot miss costs one extra ColdIndex lookup only on the miss branch. EXISTS is fixed with the cheapest possible check rather than a promotion: cold_contains_alive() is a ColdIndex-only presence + TTL check (no disk I/O, no promotion) that exists()/exists_if_alive() fall back to on a hot miss. zrank_readonly/zrevrank_readonly (command/sorted_set/sorted_set_read.rs) pattern-matched SortedSetRef exhaustively with a wildcard catch-all that silently returned Frame::Null for any unmatched variant; without an explicit arm, a cold-promoted Owned zset would report "member not ranked" even though the member exists. Fixed by routing SortedSetRef::Owned through the same O(n) fallback already used for the Listpack variant. Out of scope (per the storage audit and explicit direction): recovery.rs rehydration, replication, and the eviction gates' Wave A reporting sinks are untouched. Verified via red/green TDD (previous commit is the RED evidence): - storage::db::tests: 12/34 new-assertion tests failed pre-fix (e.g. "assertion `left == right` failed: must promote existing fields, not fabricate empty hash"), 34/34 pass post-fix. - tests/cold_collection_visibility.rs (black-box, real server, --disk-offload enable, sampled LRU eviction): all 4 tests (hash/list/set/zset) failed pre-fix (EXISTS == 0, empty reads after eviction), all 4 pass post-fix. Gates run clean: cargo test --release --lib (4134 passed, 0 failed, 1 ignored), cargo test --release --lib storage (377 passed), cargo clippy -- -D warnings (default features) and cargo clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings (both clean), cargo fmt --check (clean), and the named regression suites cold_orphan_sweep / crash_recovery_cold_del_resurrection / oom_bypass_closure (all green, unrelated to this change). One pre-existing failure was found and ruled out of scope: crash_recovery_disk_offload_no_aof fails identically against the unmodified main (35cb315) base — a distinct persistence-dir gating regression this branch does not touch. CHANGELOG.md: added an [Unreleased] entry under a new "cold-collection visibility" section. Ref: .planning/reviews/storage-audit-2026-07-12-kv.md author: Tin Dang
#41) The single-probe-per-type black-box suite added in the previous commit failed 4/4 on the Linux VM (it had only been verified on macOS): every test died at "P0: a cold-spilled X must still count as existing" with EXISTS == 0. Root cause of the false failure, not the fix: two independent eviction paths can claim the sampled-LRU victim, and only one of them spills. The interactive write-path gate (synchronous, runs inline on a SET/etc that crosses maxmemory) spills every type via evict_one_async_spill. The periodic background tick (shard::persistence_tick / src/shard/timers.rs) passes no SpillContext and plain-DROPS its victim instead — which is legal allkeys-lru semantics (Wave A records a real reason-DEL for it), not a bug. Which path claims a given probe is a genuine OS-scheduler-dependent race: the async-spill path usually won on macOS, the 100ms tick usually won on Linux. A single probe therefore made the suite nondeterministic about which path — and thus which bug-relevant behavior — it exercised each run; a plain-dropped probe's EXISTS == 0 is *correct*, and looks identical from the client's point of view to the pre-fix bug's *incorrect* EXISTS == 0 on a still-cold key. Reworked all 4 tests to be path-agnostic and deterministic: - PROBE_COUNT (32) probe collections per type instead of 1, half written up front and half interleaved DURING the filler wave (drive_eviction_interleaved), so across the batch both eviction paths are very likely to each claim at least one probe in a single run. - After the wave, every probe is classified as Complete (present, full original content — the promote-from-cold path ran), Absent (gone entirely, empty content — a legitimate plain-drop), or Ghost (anything else: EXISTS disagreeing with content, or partial/ corrupted content — the actual P0 shape). partition_probes fails on any Ghost and separately fails, with an actionable diagnostic, if every probe came back Absent (the promote path was never exercised by that run, which would make the rest of the assertions vacuous). - The no-fabrication-on-write check now runs against one Complete probe (the write MUST merge with the promoted content) and, when at least one exists, one Absent probe (the write legitimately creates a fresh container — the plain-drop path already deleted any cold copy, there is nothing to merge with). Hardening found during verification: classify_* issues two separate round trips (EXISTS, then a type-specific read-all) that are not atomic with respect to the server. evict_one_async_spill removes the hot entry immediately but hands off to a background spill-thread channel, so a probe caught squarely in that in-flight window can legitimately answer EXISTS == 0 on one round trip and then content-non-empty a moment later on the next — transient, not the permanent P0 shape. Caught exactly this transient shape once on the Linux VM (probezset:0, "EXISTS=0 but ZRANGE non-empty") during hardening; added a bounded settle-and-retry (GHOST_RECHECK_ATTEMPTS/GHOST_RECHECK_DELAY in partition_probes) so a transient window resolves to a stable Complete/Absent without masking a verdict that never stabilizes (a real, persistent ghost). Confirmed non-reproducing across 5 additional back-to-back Linux VM runs and 2 additional macOS runs after the fix, all green. Verified per the coordinator's explicit instruction: orb run -m moon-dev bash -c 'source ~/.cargo/env && cd \ /Volumes/Games/tindang-repo/moon && CARGO_TARGET_DIR=target-linux \ cargo test --release --test cold_collection_visibility' green on both the Linux VM (6 total runs, 1 caught-and-fixed transient above) and macOS (3 total runs). cargo fmt --check and cargo clippy --test cold_collection_visibility -- -D warnings clean on macOS. No production code changed in this commit — test-only. Ref: .planning/reviews/storage-audit-2026-07-12-kv.md 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? |
📝 WalkthroughWalkthroughDisk-offloaded collections can now be detected, read without promotion, or promoted before mutation. Hash, List, Set, ZSet, and Stream accessors support cold values, sorted-set rank reads handle owned data, and integration tests cover eviction visibility and write merging. ChangesCold collection visibility
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant ColdIndex
participant DiskStorage
Client->>Database: read or mutate collection
Database->>ColdIndex: check cold key
ColdIndex->>DiskStorage: decode spilled collection
DiskStorage-->>Database: cold collection value
Database-->>Client: read-through value or promoted mutable collection
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/storage/db.rs (1)
1175-1199: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
exists()should stop at an expired hot entry. After removing the hot copy, falling back tocold_contains_alive()can returntruefor an orphaned cold spill that is still shadowed by the expired hot key; mirrorget()and returnfalsehere.🤖 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/db.rs` around lines 1175 - 1199, Update the expired-entry branch in exists to return false immediately after removing the expired hot entry, rather than calling cold_contains_alive. Keep cold_contains_alive for the None branch so cold-only live keys remain visible, and preserve the existing cleanup logic.
🧹 Nitpick comments (3)
src/command/sorted_set/sorted_set_read.rs (1)
676-683: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Ownedcarries a functionalBPTree— consider O(log n)tree.rank()instead of O(n)entries_sorted().The
Ownedvariant has the sametree: BPTreefield asBPTree, andentries_sorted()already callstree.iter()on it (db_read.rs:354), confirming the tree is fully functional post-deserialization. The comment justifies the O(n) fallback by notingmembers_map/bptreereturnNoneforOwned, but this match arm destructurestreedirectly — those accessor methods are irrelevant here.You could merge
Ownedinto the existingBPTreearm to get O(log n) rank with zero allocation:♻️ Proposed refactor for `zrank_readonly` (lines 670-683)
- (SortedSetRef::BPTree { tree, .. }, Some(score)) => { - match tree.rank(OrderedFloat(score), member) { - Some(rank) => Frame::Integer(rank as i64), - None => Frame::Null, - } - } - // Listpack has no O(log n) rank structure; Owned (P0 - // cold-collection-visibility fix: a value decoded fresh from - // the cold tier, see `SortedSetRef::Owned`) carries its own - // `BPTree` but `members_map`/`bptree` deliberately return - // `None` for it (same as Listpack) — both fall back to the - // same O(n) rank-from-sorted-entries computation. - (SortedSetRef::Listpack(_), Some(score)) - | (SortedSetRef::Owned { .. }, Some(score)) => { - let entries = zref.entries_sorted(); - let target_score = OrderedFloat(score); - let target_member = Bytes::copy_from_slice(member); - match entries - .iter() - .position(|(m, s)| OrderedFloat(*s) == target_score && *m == target_member) - { - Some(rank) => Frame::Integer(rank as i64), - None => Frame::Null, - } - } + (SortedSetRef::BPTree { tree, .. } + | SortedSetRef::Owned { tree, .. }, Some(score)) => { + match tree.rank(OrderedFloat(score), member) { + Some(rank) => Frame::Integer(rank as i64), + None => Frame::Null, + } + } + // Listpack has no O(log n) rank structure; fall back to + // O(n) rank-from-sorted-entries computation. + (SortedSetRef::Listpack(_), Some(score)) => { + let entries = zref.entries_sorted(); + let target_score = OrderedFloat(score); + let target_member = Bytes::copy_from_slice(member); + match entries + .iter() + .position(|(m, s)| OrderedFloat(*s) == target_score && *m == target_member) + { + Some(rank) => Frame::Integer(rank as i64), + None => Frame::Null, + } + }The same merge applies to
zrevrank_readonly(lines 718-728) usingtree.rev_rank(). Verify thatBPTree::rank/rev_rankproduce identical results on a deserialized tree as on a live one, and that the or-pattern binding (BPTree { tree, .. } | Owned { tree, .. }) compiles under edition 2024.Also applies to: 724-728
🤖 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/command/sorted_set/sorted_set_read.rs` around lines 676 - 683, Update zrank_readonly and zrevrank_readonly so SortedSetRef::Owned is handled with the existing BPTree tree.rank() and tree.rev_rank() paths instead of the O(n) entries_sorted() fallback. Merge the match arms using compatible tree bindings, ensure the edition 2024 or-pattern compiles, and preserve identical rank results for deserialized Owned trees.src/storage/db_read.rs (1)
18-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the TTL-filtering logic shared by
WithTtlandOwnedWithTtl.
get_field,len, andentrieseach duplicate the identical fast-path/slow-path TTL filtering between the borrowedWithTtland the new ownedOwnedWithTtlvariant. Since both bind to&HashMap<Bytes, Bytes>/&HashMap<Bytes, u64>inside amatch &selfarm, a small private helper (taking references +now_ms/min_expiry_msby value) could serve both arms and remove the triplicated logic, reducing the risk that a future TTL-semantics fix gets applied to one variant but not the other.♻️ Sketch of the extraction (for `get_field`; same pattern applies to `len`/`entries`)
+ fn get_field_filtered( + fields: &HashMap<Bytes, Bytes>, + ttls: &HashMap<Bytes, u64>, + now_ms: u64, + min_expiry_ms: u64, + field: &[u8], + ) -> Option<Bytes> { + if now_ms < min_expiry_ms { + return fields.get(field).cloned(); + } + let expired = ttls.get(field).is_some_and(|&t| t <= now_ms); + if expired { None } else { fields.get(field).cloned() } + } + pub fn get_field(&self, field: &[u8]) -> Option<Bytes> { match self { ... - HashRef::WithTtl { fields, ttls, now_ms, min_expiry_ms } => { /* ... */ } + HashRef::WithTtl { fields, ttls, now_ms, min_expiry_ms } => + Self::get_field_filtered(fields, ttls, *now_ms, *min_expiry_ms, field), ... - HashRef::OwnedWithTtl { fields, ttls, now_ms, min_expiry_ms } => { /* ... */ } + HashRef::OwnedWithTtl { fields, ttls, now_ms, min_expiry_ms } => + Self::get_field_filtered(fields, ttls, *now_ms, *min_expiry_ms, field), } }🤖 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/db_read.rs` around lines 18 - 190, Extract the duplicated TTL filtering in HashRef’s WithTtl and OwnedWithTtl branches into private shared helper functions for get_field, len, and entries, accepting field/TTL map references plus now_ms and min_expiry_ms by value. Update both variants to delegate to these helpers while preserving the existing fast path, expiration semantics, and return behavior.src/storage/db.rs (1)
2665-2988: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftFile is well past the 1500-line guideline cap; this PR grows it further.
The file is already ~2989 lines. As per coding guidelines, "No single Rust file should exceed 1500 lines. Split command-group files into directory modules when approaching the limit." This PR adds ~150 lines of production code plus this ~320-line test module on top of an already-oversized file. A low-friction first step: move this new
#[cfg(test)] mod testscold-visibility block into its own test submodule file rather than growingdb.rsfurther; longer-term, the accessor groups (hash/list/set/zset/stream) are natural split points.🤖 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/db.rs` around lines 2665 - 2988, Move the cold-collection visibility test block, including helpers such as db_with_spilled_value and its #[test] cases, out of db.rs into a dedicated test submodule file. Wire that module into the existing Database test structure without changing test behavior or production accessor code, and preserve access to the required crate-local symbols and imports.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.
Inline comments:
In `@src/storage/db.rs`:
- Around line 1959-1971: Update exists_if_alive to distinguish a
present-but-expired hot entry from an absent key: return false when
self.data.get(key) finds an expired entry, and call cold_contains_alive only
when the hot lookup returns None. Preserve the existing alive-hot-entry behavior
and the hot-entry shadowing invariant.
- Around line 969-987: Split cold-read handling around cold_read_only and its
dispatch_read call sites: first probe for cold presence while holding the shard
read lock, then release the guard before fetching and decoding the spilled value
from disk. Update get_cold_value usage so disk I/O never occurs under the RwLock
read guard, while preserving the existing Option<RedisValue> behavior for
missing or expired keys.
---
Outside diff comments:
In `@src/storage/db.rs`:
- Around line 1175-1199: Update the expired-entry branch in exists to return
false immediately after removing the expired hot entry, rather than calling
cold_contains_alive. Keep cold_contains_alive for the None branch so cold-only
live keys remain visible, and preserve the existing cleanup logic.
---
Nitpick comments:
In `@src/command/sorted_set/sorted_set_read.rs`:
- Around line 676-683: Update zrank_readonly and zrevrank_readonly so
SortedSetRef::Owned is handled with the existing BPTree tree.rank() and
tree.rev_rank() paths instead of the O(n) entries_sorted() fallback. Merge the
match arms using compatible tree bindings, ensure the edition 2024 or-pattern
compiles, and preserve identical rank results for deserialized Owned trees.
In `@src/storage/db_read.rs`:
- Around line 18-190: Extract the duplicated TTL filtering in HashRef’s WithTtl
and OwnedWithTtl branches into private shared helper functions for get_field,
len, and entries, accepting field/TTL map references plus now_ms and
min_expiry_ms by value. Update both variants to delegate to these helpers while
preserving the existing fast path, expiration semantics, and return behavior.
In `@src/storage/db.rs`:
- Around line 2665-2988: Move the cold-collection visibility test block,
including helpers such as db_with_spilled_value and its #[test] cases, out of
db.rs into a dedicated test submodule file. Wire that module into the existing
Database test structure without changing test behavior or production accessor
code, and preserve access to the required crate-local symbols and imports.
🪄 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: 7ad7b736-5797-4129-ad11-64769cc3e886
📒 Files selected for processing (5)
CHANGELOG.mdsrc/command/sorted_set/sorted_set_read.rssrc/storage/db.rssrc/storage/db_read.rstests/cold_collection_visibility.rs
Comment on lines
+969
to
+987
| /// Non-promoting cold read-through for the `&self` "*_ref_if_alive" | ||
| /// accessors: decodes a spilled value from disk WITHOUT touching hot RAM | ||
| /// or the cold index. Thin wrapper over [`Self::get_cold_value`] using | ||
| /// this `Database`'s own cached clock semantics is left to the caller | ||
| /// (they already have `now_ms` in hand); kept as a private alias so the | ||
| /// call sites below read as "cold read" rather than repeating the | ||
| /// `cold_shard_dir`/`cold_index` plumbing. | ||
| /// | ||
| /// These accessors back BOTH the exclusive-dispatch path (`&mut | ||
| /// Database`, which downgrades to `&self` for the call) AND the | ||
| /// RwLock-shared-read dispatch path (`&Database` only — see | ||
| /// `dispatch_read` / `*_readonly` command handlers). The latter cannot | ||
| /// mutate `self` to promote a cold hit into hot RAM, so the safe fix is | ||
| /// "decode it from disk every time it's cold" rather than "silently | ||
| /// report the key absent" (same P0 as [`Self::promote_cold_if_present`]). | ||
| #[inline] | ||
| fn cold_read_only(&self, key: &[u8], now_ms: u64) -> Option<RedisValue> { | ||
| self.get_cold_value(key, now_ms) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether *_ref_if_alive / get_stream_if_alive are invoked while a
# shared RwLock guard on Database is held (vs. guard already dropped).
rg -n "RwLock<.*Database|RwLock::new\(Database" src -g '*.rs'
rg -nP '\.read\(\)' src/command src/shard -g '*.rs' -B3 -A6 | head -150
rg -n "fn .*_readonly\(" src/command -g '*.rs' -A10 | head -150Repository: pilotspace/moon
Length of output: 20419
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dispatch/read path =="
rg -n "dispatch_read|readonly" src/command src/server src/shard -g '*.rs' -A8 -B8
echo
echo "== cold read helper and ref accessors =="
rg -n "fn cold_read_only|fn get_hash_ref_if_alive|fn get_list_ref_if_alive|fn get_set_ref_if_alive|fn get_sorted_set_ref_if_alive|fn get_stream_if_alive|get_cold_value" src/storage/db.rs -A30 -B20
echo
echo "== shard/database lock usage around command execution =="
rg -n "read\(\)|write\(\)|RwLock<Database>|SharedDatabases|dbs\[" src/command src/server src/persistence -g '*.rs' -A6 -B6Repository: pilotspace/moon
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Database cold-read helpers =="
sed -n '930,1035p' src/storage/db.rs
echo
echo "== dispatch_read and readonly handlers =="
rg -n "dispatch_read|_readonly\(" src/command src/shard -g '*.rs' -A20 -B12
echo
echo "== lock acquisition around readonly dispatch =="
rg -n "read\(\)|write\(\)|let .*guard|drop\(guard\)|RwLockReadGuard|RwLockWriteGuard" src/command src/shard src/server -g '*.rs' -A6 -B6 | head -300Repository: pilotspace/moon
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== call sites of dispatch_read =="
rg -n "dispatch_read\(" src -g '*.rs' -A6 -B6
echo
echo "== command connection / shard read path around dispatch_read =="
sed -n '1,260p' src/command/connection.rs
echo
sed -n '1,260p' src/shard/dispatch.rsRepository: pilotspace/moon
Length of output: 32729
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== with_shard_db implementation =="
rg -n "fn with_shard_db|with_shard_db\(" src/shard/slice.rs src/shard -g '*.rs' -A40 -B20
echo
echo "== cold_value / cold_read_only doc and implementation =="
rg -n "performs synchronous disk I/O|fn get_cold_value|fn cold_read_only|fn get_hash_ref_if_alive|fn get_list_ref_if_alive|fn get_set_ref_if_alive|fn get_sorted_set_ref_if_alive|fn get_stream_if_alive" src/storage/db.rs -A35 -B20
echo
echo "== shard slice docs about lock lifetime =="
sed -n '1,220p' src/shard/slice.rsRepository: pilotspace/moon
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== handler_single dispatch scope =="
sed -n '2108,2145p' src/server/conn/handler_single.rs
echo
echo "== handler_single lock acquisition =="
rg -n "read\(\)|write\(\)|dispatch_read\(&\*guard" src/server/conn/handler_single.rs -A8 -B8Repository: pilotspace/moon
Length of output: 46544
Move cold reads out from under the shard read lock
cold_read_only in src/storage/db.rs:969-987 delegates to get_cold_value, and src/server/conn/handler_single.rs:2137 invokes it through dispatch_read(&*guard, ...) while the RwLock read guard is still live. Split the hot-path probe from the disk fetch so the cold read runs after dropping the guard.
🤖 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/db.rs` around lines 969 - 987, Split cold-read handling around
cold_read_only and its dispatch_read call sites: first probe for cold presence
while holding the shard read lock, then release the guard before fetching and
decoding the spilled value from disk. Update get_cold_value usage so disk I/O
never occurs under the RwLock read guard, while preserving the existing
Option<RedisValue> behavior for missing or expired keys.
Comment on lines
1959
to
1971
| /// Read-only existence check: returns false if expired. | ||
| /// | ||
| /// Also counts a cold-only key (spilled by eviction) as existing — see | ||
| /// [`Self::cold_contains_alive`] (P0 cold-collection-visibility fix). | ||
| /// Used by the RwLock-shared-read `EXISTS` dispatch path, which cannot | ||
| /// mutate `self` to promote. | ||
| pub fn exists_if_alive(&self, key: &[u8], now_ms: u64) -> bool { | ||
| let base_ts = self.base_timestamp; | ||
| self.data | ||
| .get(key) | ||
| .map(|e| !e.is_expired_at(base_ts, now_ms)) | ||
| .unwrap_or(false) | ||
| match self.data.get(key) { | ||
| Some(e) if !e.is_expired_at(base_ts, now_ms) => true, | ||
| _ => self.cold_contains_alive(key, now_ms), | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Same root cause as exists() above.
exists_if_alive's catch-all (_ => self.cold_contains_alive(...)) fires for both a truly-absent key AND a present-but-expired hot key, incorrectly treating the latter the same as the former. Per is_hot's documented invariant just above this method, a hot entry (even expired) shadows the cold copy and should not fall through.
🐛 Proposed fix
pub fn exists_if_alive(&self, key: &[u8], now_ms: u64) -> bool {
let base_ts = self.base_timestamp;
match self.data.get(key) {
- Some(e) if !e.is_expired_at(base_ts, now_ms) => true,
- _ => self.cold_contains_alive(key, now_ms),
+ Some(e) => !e.is_expired_at(base_ts, now_ms),
+ None => self.cold_contains_alive(key, now_ms),
}
}🤖 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/db.rs` around lines 1959 - 1971, Update exists_if_alive to
distinguish a present-but-expired hot entry from an absent key: return false
when self.data.get(key) finds an expired entry, and call cold_contains_alive
only when the hot lookup returns None. Preserve the existing alive-hot-entry
behavior and the hot-entry shadowing invariant.
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
P0 user data loss (task #41, found by the 2026-07-12 storage audit): with disk-offload enabled (the default), evicted collections were spilled to the cold tier but became invisible to their own commands — and the next write silently destroyed them.
The production eviction paths (
evict_one_async_spill,evict_batch_durable_no_aof) serialize and spill ALL value types, but onlyDatabase::get()ever consulted the cold index. Consequences before this PR:HGETALL/HLEN/LRANGE/SMEMBERS/ZRANGE/EXISTSreported a spilled collection as absent, whileTYPE/DELstill saw it — an inconsistent, ghost-key surface.HSET/LPUSH/SADD/ZADDon a spilled key fabricated a brand-new empty container that permanently shadowed the cold copy (user data destroyed; the orphan sweep later reclaimed the shadowed file).Fix
One promote-if-cold hook instead of rewriting every accessor:
Database::promote_cold_if_present(&mut self, key, now_ms)— oneColdIndexprobe on hot miss; on hit decodes, installs hot (TTL preserved), removes the cold entry (single owning copy). Called by all mutatingget_or_create_*accessors and the ownedget_*accessors.&selfref accessors backing the read-only dispatch path gainedOwnedvariants (HashRef/ListRef/SetRef/SortedSetRef, newStreamRef) fed by a non-promoting cold read-through.EXISTSusescold_contains_alive()— presence + TTL check only, no I/O, no promotion.Verification (red/green)
storage::db34/34; full lib 4134/4134; black-box suite 4/4 on macOS ×3 AND Linux VM ×6;cold_orphan_sweep,crash_recovery_cold_del_resurrection,oom_bypass_closuregreen; clippy-D warningsboth matrices; fmt.Option-guarded map probe.Follow-ups filed
run_evictionshould spill (not plain-drop) collections when a spill thread exists.crash_recovery_disk_offload_no_aoffails on unmodified main (pre-existing, isolated during this work).recovery.rs"skip for now") — now strictly additive on this fix.Summary by CodeRabbit
Bug Fixes
EXISTSand read operations to recognize live offloaded data.Tests