diff --git a/CHANGELOG.md b/CHANGELOG.md index 27bd0167..d1d24a98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,55 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — `--disk-offload` without a durability backstop broke the default LRU cache recipe + +- **GCP benchmark finding (2026-07-10)**: with `--disk-offload enable` (the + default) but `--appendonly no` and no `--save`, the durable-spill eviction + path added to fix the crash window above (`evict_batch_durable_no_aof`) + never runs, because it needs a `ShardManifest` that is only threaded + through the tick-driven memory-pressure cascade + (`shard::persistence_tick::handle_memory_pressure`), itself gated on + `persistence_dir`, which `main.rs` only constructs when + `appendonly == "yes" || save.is_some()`. The inline write-path eviction + gate has no manifest access, so durable spill is impossible from that call + site — but the pre-fix `manifest is None` branch OOM'd **unconditionally** + regardless of `--maxmemory-policy`, silently breaking the documented + "Pure cache (no durability)" recipe + (`--appendonly no --maxmemory --maxmemory-policy allkeys-lru`): a + classic LRU cache rejected writes at the cap instead of evicting. +- **Behavior fix** (`src/storage/eviction.rs`, + `try_evict_if_needed_async_spill_with_total_budget`'s `manifest is None` + branch): made the fail-close policy-aware, matching Redis semantics. When + `total_memory <= budget` nothing happens; when `policy == NoEviction` it + still OOMs (correct — noeviction always rejects); otherwise (any evicting + policy — `allkeys-*`/`volatile-*`) it now reclaims the budget by + **dropping** victims via `evict_one_with_spill(.., spill: None)` (the same + plain-drop helper the disk-offload-disabled write path already uses — no + new victim-selection logic), looping until the budget is satisfied or no + eligible victim remains (then OOM). No durable spill is attempted here — + that still only happens via the tick's `evict_batch_durable_no_aof` when a + manifest is reachable. Three new tests in `src/storage/eviction.rs`: + `async_spill_no_aof_backstop_no_manifest_allkeys_lru_evicts_pure_cache` + (the regression test — fails with OOM on pre-fix code, passes after), + `async_spill_no_aof_backstop_no_manifest_noeviction_still_rejects` + (unchanged noeviction OOM behavior), and + `async_spill_no_aof_backstop_no_manifest_volatile_lru_scoped_to_ttl_keys` + (`volatile-*` only evicts keys with a TTL, never a persistent key). The + `--appendonly yes` async-spill path and the manifest-reachable durable + batch-spill path are untouched. +- **Startup warning**: `ServerConfig::disk_offload_spill_inert` (new + predicate, `src/config.rs`) plus + `ServerConfig::warn_disk_offload_without_durability` (mirrors + `warn_deprecated_cold_tier_flags`) emit a single `tracing::warn!` at + startup when `--disk-offload enable` + no durability backstop is + detected, called from `main.rs` right after the existing cold-tier-flags + warning: disk-offload's cold-spill tiering is still inert in this + combination (evicting policies drop eligible victims instead of tiering; + `noeviction` — and any evicting policy with no eligible victim left, e.g. + `volatile-*` once its TTL-bearing keys are gone — returns OOM). Documented + in `docs/guides/tuning.md`'s "Tiered + memory offload" section. + ### Fixed — test flakiness: oom_bypass_closure readiness on loaded CI runners - `tests/oom_bypass_closure.rs`: the readiness path used a fixed 30s connect diff --git a/docs/guides/tuning.md b/docs/guides/tuning.md index ee5789fd..0a8ffa0d 100644 --- a/docs/guides/tuning.md +++ b/docs/guides/tuning.md @@ -210,6 +210,29 @@ resident forever: backing both the KV cold-read cache and vector/graph page I/O; it starts empty and grows lazily, so setting it high does not pre-commit RAM. +> **Requires a durability backstop.** The KV cold-spill path above needs a +> `ShardManifest`, which is only threaded through the tick-driven +> memory-pressure cascade — itself gated on `--appendonly yes` or `--save` +> being configured. With `--disk-offload enable` (the default) but +> `--appendonly no` and no `--save`, the inline write-path eviction gate has +> no manifest access and cannot durably spill: cold data is **never spilled +> to disk** in this combination, regardless of `--maxmemory-policy`. The +> **"Pure cache (no durability)" recipe above still works correctly** — +> `allkeys-lru` (and the other evicting policies) fall back to Redis-style +> cache eviction: victims are DROPPED outright (no tiering, no durability +> claim needed since nothing is meant to survive a restart) to keep +> `--maxmemory` honored. `noeviction` rejects writes with OOM once the budget +> is hit, and an evicting policy also returns OOM when no eligible victim +> remains (e.g. no TTL-bearing key is left under a `volatile-*` policy) — same +> as with disk-offload off. This +> spill-inertness is intentional (correctness over availability for the +> *tiering* feature specifically) — Moon warns about it once at startup +> (`ServerConfig::warn_disk_offload_without_durability`). Enable +> `--appendonly yes` or configure `--save` to activate disk-offload spill +> (durable tiering instead of dropping), or pass `--disk-offload disable` if +> you only want in-memory `--maxmemory-policy` eviction with no spill code +> path involved at all. + ## Vector/FTS/graph idle-unload Immutable vector segments (`ImmutableSegment`: full in-memory HNSW graph + diff --git a/src/config.rs b/src/config.rs index fa07752e..1db28a82 100644 --- a/src/config.rs +++ b/src/config.rs @@ -931,6 +931,54 @@ impl ServerConfig { } } + /// True when `--disk-offload enable` is configured but neither AOF + /// (`--appendonly yes`) nor RDB (`--save`) durability is on (GCP + /// benchmark finding, 2026-07-10). + /// + /// The durable-spill eviction path (`evict_batch_durable_no_aof`) needs + /// a `ShardManifest`, which today is only threaded through the + /// tick-driven memory-pressure cascade + /// (`shard::persistence_tick::handle_memory_pressure`) — itself gated on + /// `persistence_dir`, which `main.rs` only constructs when + /// `appendonly == "yes" || save.is_some()` (see `main.rs`'s + /// `persistence_dir` binding). The inline per-connection write-path + /// eviction gate has no manifest access and, under this combination, + /// cannot durably spill — cold data is NOT tiered to disk regardless of + /// policy. What happens to the *write* itself is now policy-aware + /// (`src/storage/eviction.rs`, the `manifest is None` branch of + /// `try_evict_if_needed_async_spill_with_total_budget`): an evicting + /// policy (`allkeys-*`/`volatile-*`) still honors `--maxmemory` by + /// DROPPING victims outright (Redis cache semantics, no tiering, no + /// crash-durability claim needed since nothing needs to survive a + /// restart); `noeviction` — and any evicting policy once no eligible + /// victim remains (e.g. `volatile-*` with no TTL keys left) — rejects + /// writes with OOM at the cap. This + /// predicate stays orthogonal to `maxmemory_policy` — spill IS still + /// inert either way — and exists only to make that degradation loud. + pub fn disk_offload_spill_inert(&self) -> bool { + self.disk_offload_enabled() && self.appendonly != "yes" && self.save.is_none() + } + + /// Warn once at startup when [`disk_offload_spill_inert`] holds — see + /// that method's docs for the full mechanism. Behavior is unchanged; + /// this only surfaces the pre-existing silent degradation. + /// + /// [`disk_offload_spill_inert`]: Self::disk_offload_spill_inert + pub fn warn_disk_offload_without_durability(&self) { + if self.disk_offload_spill_inert() { + tracing::warn!( + "--disk-offload is enabled but persistence is off (appendonly=no and no \ + --save). The disk-offload cold-spill tier requires a durability backstop \ + to function: without one, cold data is NOT spilled to disk. Evicting \ + policies (allkeys-*/volatile-*) fall back to DROPPING eligible \ + victims with no tiering; noeviction — and any evicting policy once \ + no eligible victim remains (e.g. volatile-* with no TTL keys) — \ + rejects writes with OOM at the cap. Enable \ + --appendonly yes or --save to activate durable spill." + ); + } + } + /// Validate `--databases` fits the `u8` db_index tag used by vector/text /// index scoping (WS5a round 2). Returns an error message (not a /// `Frame`/`anyhow::Error` — kept plain so both `main.rs`'s early-boot @@ -2369,6 +2417,33 @@ mod tests { assert_eq!(config.pagecache_size, None); } + #[test] + fn disk_offload_spill_inert_true_without_durability_backstop() { + // disk-offload defaults to enabled; explicitly turning off both AOF + // and RDB durability with no backstop leaves cold-spill inert. + let config = ServerConfig::parse_from(["moon", "--appendonly", "no"]); + assert!(config.disk_offload_spill_inert()); + } + + #[test] + fn disk_offload_spill_inert_false_when_appendonly_yes() { + let config = ServerConfig::parse_from(["moon", "--appendonly", "yes"]); + assert!(!config.disk_offload_spill_inert()); + } + + #[test] + fn disk_offload_spill_inert_false_when_save_configured() { + let config = ServerConfig::parse_from(["moon", "--appendonly", "no", "--save", "3600 1"]); + assert!(!config.disk_offload_spill_inert()); + } + + #[test] + fn disk_offload_spill_inert_false_when_disk_offload_disabled() { + let config = + ServerConfig::parse_from(["moon", "--appendonly", "no", "--disk-offload", "disable"]); + assert!(!config.disk_offload_spill_inert()); + } + #[test] fn test_parse_size() { assert_eq!(ServerConfig::parse_size("256mb"), Some(268_435_456)); diff --git a/src/main.rs b/src/main.rs index 31320940..07bf945d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -140,6 +140,12 @@ fn main() -> anyhow::Result<()> { // once if an operator still sets any of them away from their default. config.warn_deprecated_cold_tier_flags(); + // GCP benchmark finding (2026-07-10): --disk-offload enable without a + // durability backstop (--appendonly yes or --save) leaves the cold-spill + // tier silently inert — --maxmemory becomes a hard reject-at-cap instead + // of spilling. Warn once at startup; behavior is intentionally unchanged. + config.warn_disk_offload_without_durability(); + // ── --profile tuning presets (WS4, v0.6.0) ────────────────────────────── // Fill-only: a preset only sets flags the operator left at their default // (see `ServerConfig::apply_profile`); anything explicitly passed on the diff --git a/src/storage/eviction.rs b/src/storage/eviction.rs index 0c9f1556..48d5ae55 100644 --- a/src/storage/eviction.rs +++ b/src/storage/eviction.rs @@ -615,12 +615,29 @@ pub fn try_evict_if_needed_async_spill_with_total_budget( if config.appendonly != "yes" { let Some(manifest) = manifest else { // No manifest reachable from this call site and no AOF backstop: - // cannot guarantee durability, so do not evict anything here. - return if total_memory > budget { - Err(oom_error()) - } else { - Ok(()) - }; + // durable spill (evict_batch_durable_no_aof, below) is impossible + // here. That does NOT mean the cap goes unenforced, though: an + // evicting policy (AllKeys*/Volatile*) must still honor Redis + // semantics and reclaim the budget by DROPPING victims with no + // tiering (`evict_one_with_spill` with `spill: None`, the exact + // plain-drop helper the disk-offload-disabled write path already + // uses) -- only `noeviction` rejects with OOM. A durable spill + // still happens whenever a manifest IS reachable (the + // tick-driven memory-pressure cascade, handled below) or once + // `--appendonly yes` / `--save` gives this call site a backstop. + let mut current_total = total_memory; + while current_total > budget { + if policy == EvictionPolicy::NoEviction { + return Err(oom_error()); + } + let before = db.estimated_memory(); + if !evict_one_with_spill(db, config, &policy, None) { + return Err(oom_error()); + } + let after = db.estimated_memory(); + current_total = current_total.saturating_sub(before.saturating_sub(after)); + } + return Ok(()); }; let mut current_total = total_memory; while current_total > budget { @@ -1801,34 +1818,95 @@ mod tests { ); } - /// 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. + /// RED→GREEN (GCP benchmark finding, 2026-07-10 -- policy-aware + /// fail-close): when no `ShardManifest` is reachable (e.g. the inline + /// per-connection write-path eviction gate) AND there is no AOF + /// backstop, durable spill (`evict_batch_durable_no_aof`) is impossible. + /// That must NOT mean the cap goes unenforced for an evicting policy, + /// though -- Redis semantics require `allkeys-lru` (and friends) to + /// honor `--maxmemory` by DROPPING victims (no tiering) when no durable + /// spill path exists; only `noeviction` may reject. Before this fix, the + /// `manifest is None` branch OOM'd unconditionally regardless of policy, + /// which silently broke the documented "Pure cache (no durability)" + /// recipe (`--appendonly no --maxmemory ... --maxmemory-policy + /// allkeys-lru`) -- a classic LRU cache rejected writes at the cap + /// instead of evicting. This test reproduces exactly that regression: + /// it FAILS (OOM, value retained) on the pre-fix code and PASSES + /// (evicted, `Ok`) after. #[test] - fn async_spill_no_aof_backstop_no_manifest_retains_hot_value() { + fn async_spill_no_aof_backstop_no_manifest_allkeys_lru_evicts_pure_cache() { 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 total = db.estimated_memory(); + // budget of 1 byte: always over, for any non-empty value. let mut config = make_config(1, "allkeys-lru"); config.appendonly = "no".to_string(); let (tx, _rx) = flume::bounded::(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, + None, + ); + + assert!( + result.is_ok(), + "an evicting policy must reclaim budget by dropping, not OOM: {result:?}" + ); + assert_eq!( + db.len(), + 0, + "victim must be dropped (no spill, no manifest) to honor the cap" + ); + assert!( + !shard_dir.join("data").exists(), + "this is a plain drop, not a spill -- no file should be written" + ); + assert!( + _rx.try_recv().is_err(), + "the plain-drop path must bypass the SpillThread channel entirely" + ); + } + + /// GREEN (unchanged): `noeviction` rejects writes with OOM regardless of + /// manifest/AOF availability -- there is no policy under which + /// `noeviction` may drop a key. The periodic memory-pressure tick (which + /// does have a manifest, when a durability backstop exists) is + /// irrelevant here since `noeviction` never evicts anywhere. + #[test] + fn async_spill_no_aof_backstop_no_manifest_noeviction_still_rejects() { + 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, "noeviction"); + config.appendonly = "no".to_string(); + + let (tx, _rx) = flume::bounded::(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" + "noeviction must surface OOM, never silently evict" ); - assert_eq!(db.len(), 1, "hot value must be retained, not dropped"); + assert_eq!(db.len(), 1, "hot value must be retained under noeviction"); assert!(db.data().get(&b"k1"[..]).is_some()); // Nothing was queued either -- no half-done spill left dangling. assert!(_rx.try_recv().is_err()); @@ -1838,6 +1916,89 @@ mod tests { ); } + /// GREEN: `volatile-lru` must only evict keys carrying a TTL -- a + /// persistent key must never be dropped to satisfy this policy's cap, + /// matching Redis's documented "volatile-*" scope. When no volatile + /// victim exists (only a persistent key remains) the cap cannot be + /// honored at all, so this must OOM rather than drop the persistent key. + #[test] + fn async_spill_no_aof_backstop_no_manifest_volatile_lru_scoped_to_ttl_keys() { + 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"persistent"), Bytes::from_static(b"v")); + // Budget the persistent key alone comfortably fits under, but the + // persistent+volatile total exceeds -- so eviction must stop exactly + // once the volatile key is gone, without needing a "drop everything" + // budget that would otherwise force a second (impossible) eviction + // attempt against the persistent-only remainder within the same + // internal `while current_total > budget` loop. + let persistent_only_memory = db.estimated_memory(); + + let future_ms = current_time_ms() + 3_600_000; + db.set_string_with_expiry( + Bytes::from_static(b"volatile"), + Bytes::from_static(b"v"), + future_ms, + ); + let total_with_both = db.estimated_memory(); + assert!( + total_with_both > persistent_only_memory, + "adding the volatile key must increase estimated memory" + ); + + let mut config = make_config(persistent_only_memory, "volatile-lru"); + config.appendonly = "no".to_string(); + + let (tx, _rx) = flume::bounded::(4096); + + let result = try_evict_if_needed_async_spill_with_total_budget( + &mut db, + &config, + &tx, + shard_dir, + &mut next_file_id, + total_with_both, + 0, + 0, + None, + ); + + assert!(result.is_ok(), "{result:?}"); + assert_eq!(db.len(), 1, "only the volatile key may be dropped"); + assert!( + db.data().contains_key(b"persistent" as &[u8]), + "persistent key must survive volatile-lru eviction" + ); + assert!(!db.data().contains_key(b"volatile" as &[u8])); + + // Only the persistent key remains now: tighten the budget so it's + // over-budget with no volatile candidate left to satisfy it -- the + // cap cannot be honored, so this must OOM, never drop a persistent + // key under a `volatile-*` policy. + let mut config2 = make_config(1, "volatile-lru"); + config2.appendonly = "no".to_string(); + let current = db.estimated_memory(); + let result2 = try_evict_if_needed_async_spill_with_total_budget( + &mut db, + &config2, + &tx, + shard_dir, + &mut next_file_id, + current, + 0, + 0, + None, + ); + assert!( + result2.is_err(), + "volatile-lru with no volatile candidate must OOM, not drop a persistent key" + ); + assert_eq!(db.len(), 1); + } + /// 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`