diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b68abba3..bdcf9b4f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -205,6 +205,44 @@ conversion is paired with a "no leftover temp file after a successful write" regression test. AOF appends, WAL segments, and the legacy `#[allow(dead_code)]` `rewrite_aof_sync` RDB-preamble path are out of scope (different framing/fsync mechanisms, per task brief). +### Fixed — task #45: tick-path eviction now spills collections instead of plain-dropping them + +Root cause of the intermittent `tests/cold_collection_visibility.rs` CI flake +(stable GHOST: `EXISTS == 1`, `LRANGE`/`ZRANGE`/etc. permanently empty — only +observed on shared/slow Linux runners, never test-induced). Two compounding +defects in `src/storage/eviction.rs`'s synchronous spill path +(`evict_one_with_spill`): + +1. Collection victims (Hash/List/Set/ZSet/Stream) were gated behind a + stale `is_string` check that predates `kv_spill::spill_to_datafile` + gaining full collection support (it already serializes any + `RedisValueRef` via `kv_serde`) — so a collection picked as a victim + while a `SpillContext` WAS present still fell through to a silent + plain-drop, indistinguishable from `spill: None`. +2. Even for strings, a successful sync spill never registered a + `ColdIndex` entry (`spill_to_datafile` was always called with + `cold_index: None`) — the durable `.mpf` file existed and was + manifest-registered, but nothing could ever read it back via + `promote_cold_if_present`. + +Separately, `src/shard/timers::run_eviction` (the periodic 100ms tick, +independent of the memory-pressure cascade) *never* received a +`SpillContext` at all — every victim it picked was plain-dropped even with +`--disk-offload enable` and a durability backstop (`--appendonly yes`) +configured. `src/shard/persistence_tick.rs`'s `run_eviction_tick` now +builds a real `SpillContext` (from the shard's `ShardManifest` + shard +data dir + `next_file_id`) whenever disk-offload is enabled and a manifest +is present, and threads it through — falling back to the pre-existing +fail-close plain-drop (PR #273 policy-aware discipline: `noeviction` still +OOMs, an evicting policy still frees RAM) when no durability backstop +exists, matching the already-documented "spill is inert without one" rule. + +New deterministic regression tests (no server process, no timing race): +`storage::eviction::tests::sync_spill_non_string_victim_is_durably_spilled_not_plain_dropped` +and `shard::timers::tests::test_run_eviction_spills_collection_victim_when_spill_context_given`. +`tests/cold_collection_visibility.rs` (10x local reruns, both runtimes) +remains green and unmodified — its GHOST assertions still fail the suite on +any regression. ### Added — unified per-shard floor register + min-across-planes WAL recycle (kernel M3 stage 2 / K2) diff --git a/src/shard/persistence_tick.rs b/src/shard/persistence_tick.rs index 3366d6d14..b5bd031c4 100644 --- a/src/shard/persistence_tick.rs +++ b/src/shard/persistence_tick.rs @@ -409,6 +409,30 @@ pub(crate) fn run_eviction_tick( wal_kv_log, ); } else { + // task #45: give the tick eviction path the same write-then-durable + // -then-drop discipline the interactive write-path gate and the + // cascade's sync-spill fallback (step 3, above) already have. Only + // built when there is BOTH a live disk-offload config AND a + // `ShardManifest` (the durability backstop -- `--appendonly yes` or + // `--save`; "spill is inert without one" is an existing, documented + // rule, see `tests/cold_collection_visibility.rs`'s module doc) -- + // otherwise `spill_ctx` stays `None` and `run_eviction` falls back to + // its pre-existing fail-close plain-drop (policy-aware: `noeviction` + // still OOMs, an evicting policy still frees RAM, just with no cold + // copy -- matches PR #273's fail-close discipline). + let eviction_shard_dir = server_config + .effective_disk_offload_dir() + .join(format!("shard-{}", shard_id)); + let mut spill_ctx: Option> = None; + if server_config.disk_offload_enabled() + && let Some(ref mut manifest) = *shard_manifest + { + spill_ctx = Some(crate::storage::eviction::SpillContext { + shard_dir: &eviction_shard_dir, + manifest, + next_file_id, + }); + } super::timers::run_eviction( shard_databases, shard_id, @@ -419,6 +443,7 @@ pub(crate) fn run_eviction_tick( repl_state, aof_pool, wal_kv_log, + spill_ctx.as_mut(), ); } diff --git a/src/shard/timers.rs b/src/shard/timers.rs index 5a578ada6..37c386b1d 100644 --- a/src/shard/timers.rs +++ b/src/shard/timers.rs @@ -68,10 +68,28 @@ pub(crate) fn run_active_expiry( /// Run background eviction if maxmemory (or a per-db quota) is configured. /// -/// task #34 (Wave A): plain-dropped victims (no disk-offload spill in this -/// path — `--disk-offload disable`, or disk-offload's own budget cascade -/// handles the spill-capable case elsewhere) get a dual-plane `DEL` record -/// via the same `record_reason_del` handles `run_active_expiry` uses. +/// task #45: `spill` carries the same write-then-durable-then-drop +/// `SpillContext` the interactive write-path eviction gate and the memory- +/// pressure cascade's sync-spill fallback use. Before this fix the tick +/// NEVER received one — every collection (and, separately, every string — +/// see `evict_one_with_spill`'s task #45 fix) victim it picked was plain- +/// dropped even with `--disk-offload enable` and a durability backstop +/// (`--appendonly yes`/`--save`) present, producing the +/// `cold_collection_visibility` stable-ghost flake: a key whose hot copy this +/// path dropped without a cold copy stayed `EXISTS == 1` if some OTHER +/// eviction path's earlier (or later, via the async spill-completion race) +/// cold-index entry for the same key was still live, while every content +/// read on it came back empty forever. Threading a real `SpillContext` +/// through (built by the caller from `shard_manifest`/`shard_dir`/ +/// `next_file_id` — `None` when no durability backstop exists, matching the +/// documented "spill is inert without one" rule) makes this path spill +/// exactly like every other eviction path, closing the race at its root +/// instead of papering over one instance of it. +/// +/// task #34 (Wave A): plain-dropped victims (no `spill` context passed in — +/// `--disk-offload disable`, or no durability backstop) get a dual-plane +/// `DEL` record via the same `record_reason_del` handles `run_active_expiry` +/// uses. #[allow(clippy::too_many_arguments)] pub(crate) fn run_eviction( shard_databases: &Arc, @@ -83,6 +101,7 @@ pub(crate) fn run_eviction( repl_state: &Option, aof_pool: Option<&Arc>, wal_kv_log: bool, + mut spill: Option<&mut crate::storage::eviction::SpillContext<'_>>, ) { let rt = runtime_config.read(); if rt.maxmemory > 0 { @@ -129,7 +148,7 @@ pub(crate) fn run_eviction( crate::shard::slice::with_shard_db(i, |db| { let before = db.estimated_memory(); let _ = crate::storage::eviction::try_evict_if_needed_with_spill_and_total_budget_reporting( - db, &rt, None, remaining, budget, + db, &rt, spill.as_deref_mut(), remaining, budget, &mut |key| { crate::replication::reason_del::record_reason_del( key, @@ -606,6 +625,7 @@ mod tests { &h.repl_state, None, false, + None, ); let len0 = crate::shard::slice::with_shard_db(0, |db| db.len()); @@ -668,6 +688,7 @@ mod tests { &h.repl_state, None, false, + None, ); let before = crate::shard::slice::with_shard_db(0, |db| db.len()); assert_eq!(before, 100, "KV under budget must not evict"); @@ -687,6 +708,7 @@ mod tests { &h.repl_state, None, false, + None, ); let after = crate::shard::slice::with_shard_db(0, |db| db.len()); assert!( @@ -694,4 +716,99 @@ mod tests { "vector bytes over budget must trigger KV eviction ({after} vs {before})" ); } + + /// RED before the task #45 fix (`run_eviction` always passed `None` for + /// `spill`, no matter what the caller had available): the tick eviction + /// path plain-dropped a Hash victim even though a `SpillContext` (a real + /// `ShardManifest` + `shard_dir` + `next_file_id`, exactly what + /// `persistence_tick::run_eviction_tick` builds when disk-offload is + /// enabled and a durability backstop is configured) was available. This + /// is the deterministic, non-racy reproduction of the + /// `cold_collection_visibility` stable-ghost flake's root cause: a + /// single call, single victim, no server process, no timing race between + /// two eviction paths -- just "does `run_eviction` use the `SpillContext` + /// it was handed." + /// + /// GREEN after the fix: the Hash victim is durably spilled (a `.mpf` file + /// exists under `shard_dir/data/` and `db.cold_index` has a live entry + /// for the key), NOT plain-dropped -- so a subsequent `EXISTS`/`HGETALL` + /// can find it via `promote_cold_if_present` instead of returning a + /// stable ghost (`EXISTS == 1`, empty read). + #[test] + fn test_run_eviction_spills_collection_victim_when_spill_context_given() { + use crate::storage::eviction::SpillContext; + use crate::storage::tiered::cold_index::ColdIndex; + + let dbs = vec![vec![Database::new()]]; + let (shared, mut inits) = ShardDatabases::new(dbs); + crate::shard::slice::reset_test_shard(crate::shard::slice::ShardSlice::new( + inits.remove(0), + )); + + // A real Hash key -- the exact shape `tests/cold_collection_visibility.rs` + // probes (HSET f1/v1 f2/v2) -- plus enough filler so the shard is + // over its 1-byte-effective budget and `allkeys-lru` is forced to + // pick it as the (only) victim. + crate::shard::slice::with_shard_db(0, |db| { + db.cold_index = Some(ColdIndex::new()); + let h = db.get_or_create_hash(b"probehash:0").unwrap(); + h.insert(Bytes::from_static(b"f1"), Bytes::from_static(b"v1")); + h.insert(Bytes::from_static(b"f2"), Bytes::from_static(b"v2")); + }); + + let mut rt = RuntimeConfig::default(); + rt.maxmemory = 1; // force eviction of the one key present + rt.num_shards = 1; + rt.maxmemory_policy = "allkeys-lru".to_string(); + let runtime_config = Arc::new(parking_lot::RwLock::new(rt)); + + let tmp = tempfile::tempdir().unwrap(); + let shard_dir = tmp.path().join("shard-0"); + std::fs::create_dir_all(&shard_dir).unwrap(); + let manifest_path = shard_dir.join("shard.manifest"); + let mut manifest = + crate::persistence::manifest::ShardManifest::create(&manifest_path).unwrap(); + let mut next_file_id = 1u64; + let mut ctx = SpillContext { + shard_dir: &shard_dir, + manifest: &mut manifest, + next_file_id: &mut next_file_id, + }; + + let mut h = NoOpReasonDelHandles::new(); + run_eviction( + &shared, + 0, + &runtime_config, + &mut h.wal_writer, + &h.repl_backlog, + &mut h.replica_txs, + &h.repl_state, + None, + false, + Some(&mut ctx), + ); + + let (len, has_cold_entry) = crate::shard::slice::with_shard_db(0, |db| { + ( + db.len(), + db.cold_index + .as_ref() + .is_some_and(|ci| ci.lookup(b"probehash:0").is_some()), + ) + }); + assert_eq!(len, 0, "the hash victim must be evicted from hot RAM"); + assert!( + shard_dir.join("data").exists(), + "P0 (task #45): the tick eviction path must durably spill a \ + collection victim to disk when a SpillContext is available, not \ + plain-drop it" + ); + assert!( + has_cold_entry, + "P0 (task #45): the spilled hash must be registered in \ + ColdIndex, or EXISTS/HGETALL can never find it again -- a \ + stable ghost" + ); + } } diff --git a/src/storage/eviction.rs b/src/storage/eviction.rs index 9a0c20898..60874d749 100644 --- a/src/storage/eviction.rs +++ b/src/storage/eviction.rs @@ -1151,40 +1151,89 @@ pub(crate) fn evict_one_with_spill( // record. Tying the flag to the branch that actually performs the write // makes the two cases (no `SpillContext` vs. a `SpillContext` that // couldn't spill this value type) equivalent by construction. + // task #45: `kv_spill::spill_to_datafile` has fully supported collection + // types (Hash/List/Set/ZSet/Stream, serialized via `kv_serde`) since the + // async write-path eviction gate started using it -- but this synchronous + // path (background tick `timers::run_eviction`, the memory-pressure + // cascade's sync-spill fallback, and `db_quota`) still gated the call + // behind an `is_string` check left over from before that support existed. + // A Hash/List/Set/ZSet/Stream victim picked here while a `SpillContext` + // WAS present therefore never got a byte written to `shard_dir`, yet fell + // through to the same unconditional `db.remove` a genuine plain-drop + // takes -- but WITHOUT the `on_plain_drop` accounting (the old + // `is_plain_drop = spill.is_none()` snapshot read `false` even though + // nothing durable exists anywhere for that key). Spilling every type here + // closes that gap; `spill_to_datafile`'s own type match is exhaustive + // (compile error if a new `RedisValueRef` variant is ever added without a + // `ValueType` mapping). + // + // `file_id`/`ttl_for_cold` are captured from the immutable `entry` + // borrow BEFORE `db.remove` (which also clears any pre-existing cold + // copy of this key, D1: DEL-before-spill ordering) and the index insert + // into `db.cold_index` (a different field of `db`, taken as `&mut` only + // AFTER the `entry` borrow has ended) -- same split-borrow shape as + // `evict_batch_durable_no_aof`'s `db.remove` -> `ci.insert` ordering + // above. let mut spilled = false; + let mut spilled_file_id = 0u64; + let mut spilled_ttl_ms: Option = None; if let Some(ctx) = spill { if let Some(entry) = db.data().get(key.as_bytes()) { - // Only spill string entries (collection types not yet supported) - let is_string = matches!(entry.as_redis_value(), RedisValueRef::String(_)); - if is_string { - if let Err(e) = kv_spill::spill_to_datafile( - ctx.shard_dir, - *ctx.next_file_id, - key.as_bytes(), - entry, - ctx.manifest, - None, - ) { - warn!( - key = %String::from_utf8_lossy(key.as_bytes()), - error = %e, - "kv_spill: I/O error during spill; retaining hot value, \ - aborting this eviction attempt (no silent drop)" - ); - // Spill failed: the value has no durable copy anywhere. - // Do NOT evict -- keep the hot value resident and let the - // caller retry (or surface OOM) instead of losing data. - return false; - } - *ctx.next_file_id += 1; - spilled = true; + let file_id = *ctx.next_file_id; + let ttl_ms = if entry.has_expiry() { + Some(entry.expires_at_ms(0)) + } else { + None + }; + if let Err(e) = kv_spill::spill_to_datafile( + ctx.shard_dir, + file_id, + key.as_bytes(), + entry, + ctx.manifest, + None, + ) { + warn!( + key = %String::from_utf8_lossy(key.as_bytes()), + error = %e, + "kv_spill: I/O error during spill; retaining hot value, \ + aborting this eviction attempt (no silent drop)" + ); + // Spill failed: the value has no durable copy anywhere. + // Do NOT evict -- keep the hot value resident and let the + // caller retry (or surface OOM) instead of losing data. + return false; } + *ctx.next_file_id += 1; + spilled = true; + spilled_file_id = file_id; + spilled_ttl_ms = ttl_ms; } } db.remove(key.as_bytes()); crate::admin::metrics_setup::record_eviction(); - if !spilled { + if spilled { + // Register the cold location so `Database::exists`/`get`/the + // type-specific accessors' `promote_cold_if_present` can find this + // key again -- `spill_to_datafile` itself only inserts into a + // `ColdIndex` it is explicitly handed (`None` here, split-borrow + // constraint above), so the caller owns the insert. Single-page spill + // only (no overflow-chain support in this path, same limitation + // `spill_to_datafile`'s single-file layout has), so `page_idx`/ + // `slot_idx` are always the entry's sole slot: 0/0. + if let Some(ref mut ci) = db.cold_index { + ci.insert( + Bytes::copy_from_slice(key.as_bytes()), + crate::storage::tiered::cold_index::ColdLocation { + file_id: spilled_file_id, + page_idx: 0, + slot_idx: 0, + ttl_ms: spilled_ttl_ms, + }, + ); + } + } else { on_plain_drop(key.as_bytes()); } true @@ -2218,20 +2267,23 @@ mod tests { assert!(db.data().get(&b"spill_key"[..]).is_some()); } - /// RED (defect 1, task #34 review): `evict_one_with_spill`'s spill body - /// only durably spills `RedisValueRef::String` entries (see the - /// `is_string` gate). A Hash (or List/Set/ZSet) victim picked while a - /// `SpillContext` IS present therefore takes neither branch of the - /// `if is_string` check — no bytes are ever written to `shard_dir` — yet - /// falls through to the unconditional `db.remove` a few lines later. The - /// bug: `is_plain_drop` was snapshotted from `spill.is_none()` BEFORE - /// this happened, so it reads `false` (a `SpillContext` was passed) and - /// `on_plain_drop` never fires even though nothing durable exists for - /// this key anywhere. This must be a reported plain-drop, indistinguishable - /// from `spill: None`, because the outcome (no cold copy, no AOF/repl - /// record) is identical. + /// GREEN (task #45 fix, was RED as `sync_spill_non_string_victim_reports_plain_drop` + /// under the old `is_string` gate): `evict_one_with_spill`'s spill body + /// now durably spills every `RedisValueRef` type via + /// `kv_spill::spill_to_datafile` (which has fully supported collections + /// since the async write-path eviction gate started using it). A Hash + /// victim picked here while a `SpillContext` is present must therefore + /// (a) write real bytes to `shard_dir`, (b) register a `ColdIndex` entry + /// so `EXISTS`/`HGETALL`/etc. can find it again via + /// `promote_cold_if_present`, and (c) NOT be reported to `on_plain_drop` + /// (a spilled entry stays cold-readable, not deleted). Before this fix a + /// Hash/List/Set/ZSet victim under a live `SpillContext` was silently + /// plain-dropped with no cold copy anywhere -- the tick/cascade root + /// cause of the `cold_collection_visibility` stable-ghost flake (task + /// #45): `EXISTS` on a key with a stale cold-index reference to an OLDER + /// spill could read `1` while every content read came back empty forever. #[test] - fn sync_spill_non_string_victim_reports_plain_drop() { + fn sync_spill_non_string_victim_is_durably_spilled_not_plain_dropped() { let tmp = tempfile::tempdir().unwrap(); let shard_dir = tmp.path(); let manifest_path = tmp.path().join("shard.manifest"); @@ -2239,6 +2291,7 @@ mod tests { let mut next_file_id = 1u64; let mut db = Database::new(); + db.cold_index = Some(crate::storage::tiered::cold_index::ColdIndex::new()); // Hash victim: get_or_create_hash + a field write, matching how HSET // populates a real hash entry (mirrors src/storage/db.rs's own test // fixture pattern). @@ -2267,16 +2320,23 @@ mod tests { assert!(evicted, "the hash victim must still be evicted from RAM"); assert_eq!(db.len(), 0); assert!( - !shard_dir.join("data").exists(), - "setup invariant: a Hash entry must never actually be spilled \ - to disk today (spill body is string-only)" + shard_dir.join("data").exists(), + "P0 fix: a Hash victim under a live SpillContext must be durably \ + spilled to disk, not plain-dropped" ); - assert_eq!( - reported, - vec![b"hash_key".to_vec()], - "a Hash victim that was NOT actually spilled must be reported to \ - on_plain_drop (no cold copy exists anywhere for it) -- silent \ - data loss otherwise, with no AOF/replication DEL record" + assert!( + reported.is_empty(), + "a durably-spilled Hash victim must NOT be reported to \ + on_plain_drop -- it stays cold-readable, not deleted: {reported:?}" + ); + assert!( + db.cold_index + .as_ref() + .unwrap() + .lookup(b"hash_key") + .is_some(), + "the spilled Hash key must be registered in ColdIndex so EXISTS/ \ + promote_cold_if_present can find it again" ); }