fix(storage): make KV spill durable before dropping hot value (appendonly=no crash window)#270
Merged
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? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAsync disk-offload eviction now uses synchronous durable batching when AOF is disabled, passing a shard manifest through the memory-pressure path. Hot values remain until spill and manifest commit succeed. Legacy spill failures also retain values, with regression tests and changelog coverage. ChangesDisk-offload durability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant handle_memory_pressure
participant try_evict_if_needed_async_spill_with_total_budget
participant evict_batch_durable_no_aof
participant flush_buffer
participant ShardManifest
participant Database
participant ColdIndex
handle_memory_pressure->>try_evict_if_needed_async_spill_with_total_budget: provide shard manifest
try_evict_if_needed_async_spill_with_total_budget->>evict_batch_durable_no_aof: stage victim spill requests
evict_batch_durable_no_aof->>flush_buffer: flush spill batch
flush_buffer->>ShardManifest: commit durable manifest
evict_batch_durable_no_aof->>Database: remove hot keys after commit
evict_batch_durable_no_aof->>ColdIndex: publish cold entries
Possibly related PRs
🚥 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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shard/persistence_tick.rs (1)
683-696: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftCarry the remaining shard total across databases.
total_memis the aggregate shard usage, but it is passed unchanged to every DB. Each invocation can reclaim the full aggregate deficit: e.g., total 1000/budget 900 with DBs of 400 and 600 can evict from both DBs, ending near 800. Track actual reclaimed bytes in a sharedremaining_totaland stop once it reachesbudget.🤖 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/shard/persistence_tick.rs` around lines 683 - 696, Update the loop in the persistence tick eviction logic to maintain a mutable shared remaining total initialized from aggregate shard usage, passing it into each eviction call and updating it with the actual bytes reclaimed. Stop iterating once the remaining total is at or below budget, and use the eviction result/API associated with try_evict_if_needed_async_spill_with_total_budget to calculate reclaimed bytes.
🤖 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 `@CHANGELOG.md`:
- Around line 28-31: Update the CHANGELOG description to state that spill writes
and fsync occur in spill_thread::flush_buffer, while evict_batch_durable_no_aof
subsequently adds and commits the manifest entries after flush_buffer returns.
Avoid attributing the manifest commit itself to flush_buffer.
In `@src/storage/eviction.rs`:
- Around line 819-830: Handle failed manifest commits in the completion loop by
rolling back the `manifest.add_file` mutation or aborting and reconciling the
batch before processing later completions, so retained-hot files are not
committed by a subsequent success. Update the logic around `manifest.add_file`
and `manifest.commit`, then add a regression test covering a first commit
failure followed by a successful commit and verifying the failed file is not
indexed.
- Around line 706-798: The synchronous evict_batch_durable_no_aof path allocates
per pressure pass; thread reusable workspace state through the eviction call
chain instead. Replace the local HashSet, buffer Vec, and collection
serialization storage with caller-owned reusable buffers, avoid key.clone() by
using borrowed or reusable key storage, and reuse a prebuilt shard_dir PathBuf
rather than PathBuf::from(shard_dir). Update the relevant eviction state and
SpillRequest construction so no per-tick allocations occur.
- Around line 1705-1931: Split the oversized eviction module so no .rs file
exceeds 1,500 lines. Extract the durable spill eviction implementation,
including related helpers such as
try_evict_if_needed_async_spill_with_total_budget and
try_evict_if_needed_with_spill, into a focused submodule, and move the
associated durable-spill regression tests (including
async_spill_no_aof_backstop_* and
sync_spill_failure_retains_hot_value_no_silent_drop) alongside it; update module
declarations, imports, and visibility as needed.
---
Outside diff comments:
In `@src/shard/persistence_tick.rs`:
- Around line 683-696: Update the loop in the persistence tick eviction logic to
maintain a mutable shared remaining total initialized from aggregate shard
usage, passing it into each eviction call and updating it with the actual bytes
reclaimed. Stop iterating once the remaining total is at or below budget, and
use the eviction result/API associated with
try_evict_if_needed_async_spill_with_total_budget to calculate reclaimed bytes.
🪄 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: f94c1739-2b82-4ca5-af5d-ab036b9f9338
📒 Files selected for processing (4)
CHANGELOG.mdsrc/shard/persistence_tick.rssrc/storage/eviction.rssrc/storage/tiered/spill_thread.rs
Comment on lines
+28
to
+31
| writes the whole batch as one durable, synchronous call to | ||
| `spill_thread::flush_buffer` (pwrite + fsync + manifest commit — the same | ||
| inline/oversized page routing the background thread itself uses, now made | ||
| `pub(crate)` for reuse), and only *then* removes from RAM the keys whose |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that manifest commit occurs after flush_buffer.
flush_buffer performs spill writes and returns completions; evict_batch_durable_no_aof subsequently adds and commits manifest entries. The current wording attributes the commit to flush_buffer.
Proposed wording
- writes the whole batch as one durable, synchronous call to
- `spill_thread::flush_buffer` (pwrite + fsync + manifest commit — the same
+ writes the whole batch synchronously through
+ `spill_thread::flush_buffer` (pwrite + fsync), then commits the manifest — the same📝 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
| writes the whole batch as one durable, synchronous call to | |
| `spill_thread::flush_buffer` (pwrite + fsync + manifest commit — the same | |
| inline/oversized page routing the background thread itself uses, now made | |
| `pub(crate)` for reuse), and only *then* removes from RAM the keys whose | |
| writes the whole batch synchronously through | |
| `spill_thread::flush_buffer` (pwrite + fsync), then commits the manifest — the same | |
| inline/oversized page routing the background thread itself uses, now made | |
| `pub(crate)` for reuse), and only *then* removes from RAM the keys whose |
🤖 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 28 - 31, Update the CHANGELOG description to state
that spill writes and fsync occur in spill_thread::flush_buffer, while
evict_batch_durable_no_aof subsequently adds and commits the manifest entries
after flush_buffer returns. Avoid attributing the manifest commit itself to
flush_buffer.
Comment on lines
+706
to
+798
| fn evict_batch_durable_no_aof( | ||
| db: &mut Database, | ||
| config: &RuntimeConfig, | ||
| policy: &EvictionPolicy, | ||
| shard_dir: &Path, | ||
| next_file_id: &mut u64, | ||
| manifest: &mut ShardManifest, | ||
| db_index: usize, | ||
| deficit: usize, | ||
| ) -> usize { | ||
| let mut seen: std::collections::HashSet<CompactKey> = std::collections::HashSet::new(); | ||
| let mut buffer: Vec<SpillRequest> = Vec::new(); | ||
| let mut staged_bytes = 0usize; | ||
| let mut stall = 0usize; | ||
|
|
||
| while buffer.len() < NO_AOF_BATCH_CAP && staged_bytes < deficit { | ||
| let victim = match policy { | ||
| EvictionPolicy::NoEviction => None, | ||
| EvictionPolicy::AllKeysLru => find_victim_lru(db, config.maxmemory_samples, false), | ||
| EvictionPolicy::AllKeysLfu => { | ||
| find_victim_lfu(db, config.maxmemory_samples, config.lfu_decay_time, false) | ||
| } | ||
| EvictionPolicy::AllKeysRandom => find_victim_random(db, false), | ||
| EvictionPolicy::VolatileLru => find_victim_lru(db, config.maxmemory_samples, true), | ||
| EvictionPolicy::VolatileLfu => { | ||
| find_victim_lfu(db, config.maxmemory_samples, config.lfu_decay_time, true) | ||
| } | ||
| EvictionPolicy::VolatileRandom => find_victim_random(db, true), | ||
| EvictionPolicy::VolatileTtl => find_victim_volatile_ttl(db, config.maxmemory_samples), | ||
| }; | ||
| let key = match victim { | ||
| Some(k) => k, | ||
| None => break, | ||
| }; | ||
| if seen.contains(&key) { | ||
| stall += 1; | ||
| if stall >= NO_AOF_BATCH_STALL_LIMIT { | ||
| break; | ||
| } | ||
| continue; | ||
| } | ||
| stall = 0; | ||
| seen.insert(key.clone()); | ||
|
|
||
| let Some(entry) = db.data().get(key.as_bytes()) else { | ||
| // Race with expiry: nothing to spill for this key, but it is | ||
| // already gone, so keep sampling for more victims. | ||
| continue; | ||
| }; | ||
| let val_ref = entry.as_redis_value(); | ||
| let collection_buf: Vec<u8>; | ||
| 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!(), | ||
| }; | ||
| collection_buf = kv_serde::serialize_collection(other).unwrap_or_default(); | ||
| (vt, collection_buf.as_slice()) | ||
| } | ||
| }; | ||
| let mut flags: u8 = 0; | ||
| let ttl_ms = if entry.has_expiry() { | ||
| flags |= entry_flags::HAS_TTL; | ||
| Some(entry.expires_at_ms(0)) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| staged_bytes += key.as_bytes().len() + value_bytes.len(); | ||
| let file_id = *next_file_id; | ||
| *next_file_id += 1; | ||
| buffer.push(SpillRequest { | ||
| key: Bytes::copy_from_slice(key.as_bytes()), | ||
| db_index, | ||
| value_bytes: Bytes::copy_from_slice(value_bytes), | ||
| value_type, | ||
| flags, | ||
| ttl_ms, | ||
| file_id, | ||
| shard_dir: PathBuf::from(shard_dir), | ||
| }); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Remove allocations from the shard event-loop path.
This synchronous tick path introduces HashSet::new(), Vec::new(), clone(), collection serialization allocations, and PathBuf::from(). Thread reusable batch/workspace state through the eviction path instead of allocating per pressure pass.
As per coding guidelines, “Avoid hot-path allocations in … shard event loops … no … Vec::new() … clone() …”.
🤖 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 706 - 798, The synchronous
evict_batch_durable_no_aof path allocates per pressure pass; thread reusable
workspace state through the eviction call chain instead. Replace the local
HashSet, buffer Vec, and collection serialization storage with caller-owned
reusable buffers, avoid key.clone() by using borrowed or reusable key storage,
and reuse a prebuilt shard_dir PathBuf rather than PathBuf::from(shard_dir).
Update the relevant eviction state and SpillRequest construction so no per-tick
allocations occur.
Source: Coding guidelines
Comment on lines
+819
to
+830
| let file_id = completion.file_entry.file_id; | ||
| manifest.add_file(completion.file_entry); | ||
| if let Err(e) = manifest.commit() { | ||
| warn!( | ||
| file_id, | ||
| error = %e, | ||
| "kv_spill: manifest commit failed for durable batch spill under \ | ||
| appendonly=no; retaining hot values (spill file may be orphaned; \ | ||
| the orphan sweep reclaims it)" | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Rollback or reconcile the manifest after a failed commit.
add_file mutates the active manifest before commit(). On failure, this loop continues; a later successful completion commits all active entries, including the earlier retained-hot file. That can leave an unindexed stale cold copy which may resurrect after a later update/delete and restart. Restore the pre-add state or abort/reconcile the batch after commit failure, and add a fail-first/succeed-second regression test.
🤖 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 819 - 830, Handle failed manifest
commits in the completion loop by rolling back the `manifest.add_file` mutation
or aborting and reconciling the batch before processing later completions, so
retained-hot files are not committed by a subsequent success. Update the logic
around `manifest.add_file` and `manifest.commit`, then add a regression test
covering a first commit failure followed by a successful commit and verifying
the failed file is not indexed.
Comment on lines
+1705
to
+1931
| // ── Crash-window fix: write-then-durable-then-drop under appendonly=no ── | ||
| // | ||
| // Without an AOF backstop the async fast path's "queue-then-drop" | ||
| // ordering is a genuine, unconditional data-loss bug: the value would | ||
| // exist nowhere (not in RAM, not yet on disk, no AOF/WAL record) for the | ||
| // whole `SpillThread` batching window. Pre-fix, `evict_one_async_spill` | ||
| // had no `--appendonly` check at all and no way to spill synchronously, | ||
| // so a successful `try_send` was immediately followed by `db.remove` — | ||
| // exactly the crash window these tests lock shut. | ||
|
|
||
| /// GREEN: under `--appendonly no`, eviction must perform a fully durable | ||
| /// synchronous spill (pwrite + fsync + manifest commit) BEFORE dropping | ||
| /// the hot value, and must make the key immediately cold-read-through-able | ||
| /// (there is no later `SpillCompletion` on this path to do it for us). | ||
| #[test] | ||
| fn async_spill_no_aof_backstop_durably_spills_before_drop() { | ||
| let tmp = tempfile::tempdir().unwrap(); | ||
| let shard_dir = tmp.path(); | ||
| let manifest_path = shard_dir.join("shard.manifest"); | ||
| let mut manifest = ShardManifest::create(&manifest_path).unwrap(); | ||
| let mut next_file_id = 1u64; | ||
|
|
||
| let mut db = Database::new(); | ||
| db.cold_index = Some(crate::storage::tiered::cold_index::ColdIndex::new()); | ||
| db.set_string(Bytes::from_static(b"k1"), Bytes::from_static(b"v1")); | ||
| let total = db.estimated_memory(); | ||
|
|
||
| let mut config = make_config(1, "allkeys-lru"); | ||
| config.appendonly = "no".to_string(); // no AOF backstop | ||
|
|
||
| // Channel is never touched by the durable path -- bounded(1) with | ||
| // nothing pre-filled proves that if the fast path ran instead, this | ||
| // assertion would still pass, so we additionally assert `_rx` stayed | ||
| // empty below to prove the queue was never used. | ||
| let (tx, rx) = flume::bounded::<SpillRequest>(4096); | ||
|
|
||
| let result = try_evict_if_needed_async_spill_with_total_budget( | ||
| &mut db, | ||
| &config, | ||
| &tx, | ||
| shard_dir, | ||
| &mut next_file_id, | ||
| total, | ||
| 0, | ||
| 0, | ||
| Some(&mut manifest), | ||
| ); | ||
| assert!( | ||
| result.is_ok(), | ||
| "durable synchronous spill must succeed: {result:?}" | ||
| ); | ||
| assert_eq!(db.len(), 0, "hot value dropped only after durable spill"); | ||
| assert!( | ||
| rx.try_recv().is_err(), | ||
| "the durable fallback must bypass the SpillThread channel entirely" | ||
| ); | ||
|
|
||
| // The .mpf file must already be fsynced on disk -- the durability | ||
| // barrier that has to complete before RAM is freed. | ||
| let file_path = shard_dir.join("data/heap-000001.mpf"); | ||
| assert!(file_path.exists(), "spill file must be durable before drop"); | ||
| let pages = read_datafile(&file_path).unwrap(); | ||
| let entry = pages[0].get(0).unwrap(); | ||
| assert_eq!(entry.key, b"k1"); | ||
| assert_eq!(entry.value, b"v1"); | ||
|
|
||
| // A FRESH manifest load from disk (simulating post-crash recovery) | ||
| // must already see the file -- the commit landed before drop. | ||
| let reloaded = crate::persistence::manifest::ShardManifest::open(&manifest_path) | ||
| .expect("manifest must be openable"); | ||
| assert!( | ||
| reloaded.files().iter().any(|f| f.file_id == 1), | ||
| "manifest commit must be durable before the hot value is dropped" | ||
| ); | ||
|
|
||
| // No SpillCompletion will ever arrive for this eviction (no | ||
| // SpillThread involved) -- ColdIndex must be populated inline or the | ||
| // key is unreadable until the next full restart. | ||
| assert!( | ||
| db.cold_index.as_ref().unwrap().lookup(b"k1").is_some(), | ||
| "key must be immediately cold-read-through-able after a durable sync spill" | ||
| ); | ||
|
|
||
| // Requirement (4) regression guard (#257 class): a DEL of a key that | ||
| // was just durably spilled by this new path must tombstone the cold | ||
| // copy too, never leave it resurrectable via read-through. This | ||
| // pins the fix's `db.remove()`-before-`ci.insert()` ordering (an | ||
| // earlier draft of this fix got that backwards: inserting into | ||
| // ColdIndex before `db.remove` let `Database::remove`'s own cold-tier | ||
| // cleanup wipe the entry right back out; this test would have caught | ||
| // the opposite bug too -- a DEL failing to clear a cold entry that | ||
| // durable-spill inserted). | ||
| db.remove(b"k1"); | ||
| assert!( | ||
| db.cold_index.as_ref().unwrap().lookup(b"k1").is_none(), | ||
| "DEL after a durable sync spill must not leave the key resurrectable from cold" | ||
| ); | ||
| } | ||
|
|
||
| /// GREEN: when no `ShardManifest` is reachable (e.g. the inline | ||
| /// per-connection write-path eviction gate) AND there is no AOF backstop, | ||
| /// eviction MUST NOT drop the hot value -- there is no way to guarantee | ||
| /// durability from this call site, so it must bail (OOM surfaced, value | ||
| /// retained) rather than manufacture a crash-loss window. The periodic | ||
| /// memory-pressure tick (which does have a manifest) picks up the slack. | ||
| #[test] | ||
| fn async_spill_no_aof_backstop_no_manifest_retains_hot_value() { | ||
| let tmp = tempfile::tempdir().unwrap(); | ||
| let shard_dir = tmp.path(); | ||
| let mut next_file_id = 1u64; | ||
|
|
||
| let mut db = Database::new(); | ||
| db.set_string(Bytes::from_static(b"k1"), Bytes::from_static(b"v1")); | ||
|
|
||
| let mut config = make_config(1, "allkeys-lru"); | ||
| config.appendonly = "no".to_string(); | ||
|
|
||
| let (tx, _rx) = flume::bounded::<SpillRequest>(4096); | ||
|
|
||
| let result = | ||
| try_evict_if_needed_async_spill(&mut db, &config, &tx, shard_dir, &mut next_file_id, 0); | ||
|
|
||
| assert!( | ||
| result.is_err(), | ||
| "no manifest + no AOF backstop must surface OOM, never silently evict" | ||
| ); | ||
| assert_eq!(db.len(), 1, "hot value must be retained, not dropped"); | ||
| assert!(db.data().get(&b"k1"[..]).is_some()); | ||
| // Nothing was queued either -- no half-done spill left dangling. | ||
| assert!(_rx.try_recv().is_err()); | ||
| assert!( | ||
| !shard_dir.join("data").exists(), | ||
| "no spill file should have been written" | ||
| ); | ||
| } | ||
|
|
||
| /// GREEN (spill-failure retention, no-AOF path): if the durable | ||
| /// synchronous spill itself fails (I/O error), the hot value must be | ||
| /// retained -- never silently dropped. Simulated by pointing `shard_dir` | ||
| /// at a path whose parent is a regular file, so `create_dir_all("data")` | ||
| /// fails. | ||
| #[test] | ||
| fn async_spill_no_aof_backstop_spill_failure_retains_hot_value() { | ||
| let tmp = tempfile::tempdir().unwrap(); | ||
| let blocker_file = tmp.path().join("not_a_dir"); | ||
| std::fs::write(&blocker_file, b"x").unwrap(); | ||
| // shard_dir/data cannot be created because `shard_dir` itself is a | ||
| // regular file, not a directory. | ||
| let shard_dir = blocker_file.as_path(); | ||
| let manifest_path = tmp.path().join("shard.manifest"); | ||
| let mut manifest = ShardManifest::create(&manifest_path).unwrap(); | ||
| let mut next_file_id = 1u64; | ||
|
|
||
| let mut db = Database::new(); | ||
| db.set_string(Bytes::from_static(b"k1"), Bytes::from_static(b"v1")); | ||
| let total = db.estimated_memory(); | ||
|
|
||
| let mut config = make_config(1, "allkeys-lru"); | ||
| config.appendonly = "no".to_string(); | ||
|
|
||
| let (tx, _rx) = flume::bounded::<SpillRequest>(4096); | ||
|
|
||
| let result = try_evict_if_needed_async_spill_with_total_budget( | ||
| &mut db, | ||
| &config, | ||
| &tx, | ||
| shard_dir, | ||
| &mut next_file_id, | ||
| total, | ||
| 0, | ||
| 0, | ||
| Some(&mut manifest), | ||
| ); | ||
|
|
||
| assert!( | ||
| result.is_err(), | ||
| "spill I/O failure must surface OOM, never a false success" | ||
| ); | ||
| assert_eq!( | ||
| db.len(), | ||
| 1, | ||
| "hot value must be retained when the durable spill fails" | ||
| ); | ||
| assert!(db.data().get(&b"k1"[..]).is_some()); | ||
| } | ||
|
|
||
| /// GREEN (bug #1 regression guard, sync path): `evict_one_with_spill` | ||
| /// previously fell through to `db.remove` unconditionally even when | ||
| /// `spill_to_datafile` failed, discarding the value with no copy | ||
| /// anywhere -- real, unconditional data loss on every spill I/O error, | ||
| /// independent of any crash. Same blocker-file trick as above. | ||
| #[test] | ||
| fn sync_spill_failure_retains_hot_value_no_silent_drop() { | ||
| let tmp = tempfile::tempdir().unwrap(); | ||
| let blocker_file = tmp.path().join("not_a_dir"); | ||
| std::fs::write(&blocker_file, b"x").unwrap(); | ||
| let shard_dir = blocker_file.as_path(); | ||
| let manifest_path = tmp.path().join("shard.manifest"); | ||
| let mut manifest = ShardManifest::create(&manifest_path).unwrap(); | ||
| let mut next_file_id = 1u64; | ||
|
|
||
| let mut db = Database::new(); | ||
| db.set_string( | ||
| Bytes::from_static(b"spill_key"), | ||
| Bytes::from_static(b"spill_val"), | ||
| ); | ||
|
|
||
| let config = make_config(1, "allkeys-lru"); | ||
| let mut ctx = SpillContext { | ||
| shard_dir, | ||
| manifest: &mut manifest, | ||
| next_file_id: &mut next_file_id, | ||
| }; | ||
|
|
||
| let result = try_evict_if_needed_with_spill(&mut db, &config, Some(&mut ctx)); | ||
| assert!( | ||
| result.is_err(), | ||
| "spill I/O failure must surface OOM, never a false success" | ||
| ); | ||
| assert_eq!( | ||
| db.len(), | ||
| 1, | ||
| "hot value must be retained when the sync spill fails (no silent drop)" | ||
| ); | ||
| assert!(db.data().get(&b"spill_key"[..]).is_some()); | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split this oversized Rust module.
src/storage/eviction.rs now exceeds 2,000 lines. Extract durable spill eviction and its tests into submodules before merging.
As per coding guidelines, “No single .rs file should exceed 1500 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/storage/eviction.rs` around lines 1705 - 1931, Split the oversized
eviction module so no .rs file exceeds 1,500 lines. Extract the durable spill
eviction implementation, including related helpers such as
try_evict_if_needed_async_spill_with_total_budget and
try_evict_if_needed_with_spill, into a focused submodule, and move the
associated durable-spill regression tests (including
async_spill_no_aof_backstop_* and
sync_spill_failure_retains_hot_value_no_silent_drop) alongside it; update module
declarations, imports, and visibility as needed.
Source: Coding guidelines
TinDang97
force-pushed
the
fix/kv-spill-evict-ordering
branch
from
347a136 to
3b2e3b3
Compare
Under `--disk-offload enable` + `--appendonly no`, the memory-pressure eviction path queued a key's SpillRequest to the background SpillThread and immediately dropped the hot value from RAM -- before the pwrite, fsync, or manifest commit had happened. Under `--appendonly yes` this ordering is safe (a crash in that window is covered by AOF replay of the original write), but with no AOF backstop the value existed nowhere -- not in RAM, not yet durable on disk, no WAL/AOF record -- for the entire SpillThread batching window. A kill -9 in that window was silent, unrecoverable data loss. `try_evict_if_needed_async_spill_with_total_budget` (src/storage/ eviction.rs) now branches on `--appendonly`. With an AOF backstop the existing fast fire-and-forget path is unchanged. Without one, a new `evict_batch_durable_no_aof` collects up to 256 victims without removing them from RAM, spills the whole batch durably in one synchronous call to `spill_thread::flush_buffer` (pwrite + fsync + manifest commit, now `pub(crate)` for reuse -- batching is required, not optional: an earlier draft did one fsync per key and stalled the event loop under sustained pressure), and only then removes from RAM the keys whose file both pwrite-succeeded and manifest-committed, immediately populating ColdIndex. `db.remove()` is called before the ColdIndex insert, never after -- Database::remove's own cold-tier tombstone cleanup would otherwise silently wipe the insert back out (the #257 DEL/cold-tier resurrection class); this ordering bug was caught by the test suite during development, not by inspection. The manifest is threaded through as `Option<&mut ShardManifest>`; only the tick-driven memory-pressure cascade (handle_memory_pressure) has one to give. The four other call sites (inline per-connection write-path gate, sharded/monoio handlers, spsc_handler, the Lua scripting bridge) have no manifest reachable, so under `--appendonly no` they now bail -- retain the hot value, surface OOM -- instead of risking the same crash window; the next 100ms tick reclaims the memory via the durable path. This does mean those inline sites no longer evict opportunistically under `--appendonly no` (that unconditional eviction regardless of durability was the bug), so a write burst that crosses maxmemory now converges to budget only as fast as the 100ms tick reclaims it. Verified live via the shard's own estimated_memory() accounting (not RSS, which includes allocator/SSO overhead the eviction budget doesn't model) that this converges correctly and stays converged -- not a stall or a leak, just a smaller, safer per-tick eviction volume than the old unconditional fast path. A second, independent bug in the same file: evict_one_with_spill (the legacy fully-synchronous path used when no SpillThread exists) logged a warning on spill I/O failure but still evicted the key -- data loss on every spill failure regardless of appendonly or crash. Now returns without evicting on failure, matching the async path's fail-closed contract. Regression coverage (src/storage/eviction.rs tests): - sync_spill_failure_retains_hot_value_no_silent_drop - async_spill_no_aof_backstop_durably_spills_before_drop (locks the pwrite+fsync+manifest-commit-before-drop ordering, immediate ColdIndex read-through, and a DEL-after-spill check guarding against the #257 resurrection class) - async_spill_no_aof_backstop_no_manifest_retains_hot_value - async_spill_no_aof_backstop_spill_failure_retains_hot_value Verification: cargo test --release --lib storage::eviction:: (30/30 passed), cargo clippy -- -D warnings (clean), cargo clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings (clean), cargo fmt --check (clean), cargo check --no-default-features --features runtime-tokio,jemalloc (clean). author: Tin Dang <tindang.ht97@gmail.com>
TinDang97
force-pushed
the
fix/kv-spill-evict-ordering
branch
from
3b2e3b3 to
330bdee
Compare
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 10, 2026
…ut a durability backstop (#273) Evicting maxmemory-policies now DROP eligible victims (Redis cache semantics) instead of OOM-rejecting at the cap under --disk-offload enable + appendonly=no + no --save; noeviction (and any evicting policy with no eligible victim) still OOMs. Adds a startup warn for the spill-inert combination. Fixes the #270 default-config LRU-cache regression. Verified: unit 5/5 both runtimes + GCP x86 e2e (allkeys-lru+appendonly=no oom 10718→0, aof=yes unchanged).
This was referenced Jul 11, 2026
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.
Crash window (characterized precisely)
Under
--disk-offload enable --appendonly no+ memory pressure,evict_one_async_spillqueued aSpillRequestto the backgroundSpillThreadand immediately dropped the hot value — before pwrite/fsync/manifest-commit. For the SpillThread batching window (≤256 entries or 100ms) the value existed nowhere: not RAM, not disk, no AOF/WAL.kill -9there = silent unrecoverable loss. (Under--appendonly yesAOF replay covers the window — that fast path is untouched.)Fix
appendonly=noeviction now routes throughevict_batch_durable_no_aof: batch-collect victims without removing from RAM → synchronous durable spill via the SpillThread's ownflush_buffer(pwrite+fsync) →manifest.commit()→ only thendb.remove+ ColdIndex insert (in that order — reversing it letsDatabase::remove's cold-copy cleanup wipe the fresh insert, the fix(persistence): replayed DEL/FLUSH tombstone the cold plane; spills no longer suppress AOF recovery #257 resurrection class; caught by test).appendonly=no: retain + surface OOM instead of recreating the window. Tick-drivenhandle_memory_pressurestill evicts durably each tick. Trade-off documented in CHANGELOG: no more opportunistic inline eviction underappendonly=no.evict_one_with_spillevicted unconditionally even on spill I/O error → now retain-on-failure.Tests
4 new regression tests (durable-before-drop, no-manifest retain, spill-failure retain ×2 paths);
storage::eviction::30/30; clippy-D warnings+ fmt clean on both feature sets.Summary by CodeRabbit