fix(storage): policy-aware eviction fail-close for disk-offload without a durability backstop#273
Merged
Merged
Conversation
…ackstop GCP benchmark (2026-07-10) found that `--disk-offload enable` (the default) combined with `--appendonly no` and no `--save` silently degrades: the durable-spill eviction path added by PR #270 (`evict_batch_durable_no_aof`) only runs from the tick-driven memory-pressure cascade (`shard::persistence_tick::handle_memory_pressure`), which requires a `ShardManifest` threaded through `persistence_dir` — and `main.rs` only constructs `persistence_dir` when `appendonly == "yes" || save.is_some()`. The inline per-connection write-path eviction gate has no manifest access and, correctly, bails rather than risk an unrecoverable crash-loss window. The net effect: `--maxmemory` silently degrades from "spill cold data to disk" to a hard reject-at-cap (writes OOM-rejected), and cold data is never spilled. This trade-off (correctness over availability) is intentional and unchanged by this commit — the eviction/spill logic is untouched. What was missing was visibility: an operator following the documented `--disk-offload enable` default plus `--appendonly no` would hit OOM rejections with no indication why disk-offload wasn't relieving pressure. Adds `ServerConfig::disk_offload_spill_inert` (testable predicate) and `ServerConfig::warn_disk_offload_without_durability` (single `tracing::warn!` at startup), mirroring the existing `warn_deprecated_cold_tier_flags` pattern exactly. Wired into `main.rs` right after that call site. Four unit tests cover the true case and the three ways to make it false (`--appendonly yes`, `--save` set, `--disk-offload disable`). Documents the requirement in `docs/guides/tuning.md`'s "Tiered memory offload" section and adds a CHANGELOG entry. author: Tin Dang <tindang.ht97@gmail.com>
…licies without a spill backstop, OOM only for noeviction
The previous commit added a startup warning for `--disk-offload enable`
without a durability backstop (`--appendonly no` + no `--save`), but left
the underlying behavior as an unconditional OOM reject whenever the
`manifest is None` branch of
`try_evict_if_needed_async_spill_with_total_budget` was reached — the inline
per-connection write-path eviction gate under that config combination. That
meant `--maxmemory-policy allkeys-lru --appendonly no` (the documented "Pure
cache (no durability)" recipe, and disk-offload's own default of `enable`)
OOM'd at the cap instead of evicting: a classic LRU cache broken out of the
box by a config default.
Durable spill (`evict_batch_durable_no_aof`) is genuinely impossible from
this call site without a `ShardManifest` — that part is correct and
unchanged. What was wrong is treating "can't spill durably" as "can't evict
at all," for every policy. Redis semantics only require `noeviction` to
reject; every other policy (`allkeys-*`/`volatile-*`) is expected to make
room by dropping keys.
Fix (`src/storage/eviction.rs`, the `manifest is None` branch): now
policy-aware, mirroring the loop shape of the `manifest is Some` branch
right below it but calling the existing plain-drop helper
(`evict_one_with_spill(db, config, &policy, None)` — the exact function the
disk-offload-disabled write path already uses, no new victim-selection
logic) instead of `evict_batch_durable_no_aof`:
1. `total_memory <= budget` → `Ok(())`, nothing to do.
2. `policy == NoEviction` → `Err(oom_error())`, unchanged.
3. Any evicting policy → loop dropping victims (no spill, no tiering)
until the budget is satisfied or no eligible victim remains (then
OOM, e.g. `volatile-*` with no TTL'd keys left).
Also reworded the startup warning added in the previous commit: it
previously implied `--maxmemory` unconditionally becomes a hard reject
cap, which is now only true for `noeviction`. It now says evicting
policies fall back to dropping keys with no tiering, and only
`noeviction` rejects with OOM.
Three new tests in `src/storage/eviction.rs::tests` (RED confirmed against
the pre-fix code before reapplying the fix):
- `async_spill_no_aof_backstop_no_manifest_allkeys_lru_evicts_pure_cache`
— the pure-cache regression test itself: allkeys-lru over budget now
evicts (Ok, memory reclaimed) instead of OOM.
- `async_spill_no_aof_backstop_no_manifest_noeviction_still_rejects` —
confirms noeviction is unchanged (OOM, value retained).
- `async_spill_no_aof_backstop_no_manifest_volatile_lru_scoped_to_ttl_keys`
— volatile-lru only drops keys carrying a TTL; a persistent key survives,
and once no volatile candidate remains the cap correctly OOMs rather than
dropping the persistent key.
The `--appendonly yes` async-spill path and the `manifest is Some` durable
batch-spill path are untouched; their existing tests
(`async_spill_no_aof_backstop_durably_spills_before_drop`,
`async_spill_no_aof_backstop_spill_failure_retains_hot_value`, and the
elastic-budget/per-shard-budget suite) still pass unmodified.
Updated `docs/guides/tuning.md`'s disk-offload durability-backstop note to
reflect that the "Pure cache" recipe works correctly, and expanded the
CHANGELOG [Unreleased] entry to cover the behavior fix.
author: Tin Dang <tindang.ht97@gmail.com>
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 (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDisk-offload now detects configurations without AOF or RDB durability, warns at startup, documents inert cold-spill behavior, and falls back to policy-based in-memory eviction when enforcing memory limits without a spill manifest. ChangesDisk-offload durability fallback
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant main
participant ServerConfig
participant try_evict_if_needed_async_spill_with_total_budget
participant evict_one_with_spill
main->>ServerConfig: warn_disk_offload_without_durability()
ServerConfig-->>main: Emit warning when spill is inert
main->>try_evict_if_needed_async_spill_with_total_budget: Enforce total memory budget
try_evict_if_needed_async_spill_with_total_budget->>evict_one_with_spill: Drop eligible victim with spill=None
evict_one_with_spill-->>try_evict_if_needed_async_spill_with_total_budget: Reclaim memory
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: 8
🤖 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 45-53: Update the changelog entry describing disk-offload behavior
to replace “only noeviction still OOMs” with wording that also includes volatile
eviction policies once their TTL-bearing victims are exhausted, matching the
regression test and tuning guide.
In `@docs/guides/tuning.md`:
- Around line 220-225: Update the “Pure cache (no durability)” section to
qualify the statement that only noeviction rejects writes: volatile-* policies
can also return OOM when no TTL-bearing keys remain. Reference the eviction
behavior verified by the regression test in eviction.rs, and revise the wording
to state that volatile policies evict only TTL-bearing victims while retaining
persistent keys.
In `@src/config.rs`:
- Around line 934-958: Split the oversized Config implementation and its related
tests from src/config.rs into focused Rust modules before adding further
configuration APIs. Preserve public interfaces and behavior, including
disk_offload_spill_inert and its associated tests, and update module
declarations/imports so all existing callers and tests continue to compile.
Ensure no resulting Rust file exceeds 1500 lines.
- Around line 947-955: Update the documentation near the spill/maxmemory policy
explanation to state that writes can also return OOM under volatile-* policies
when no TTL-bearing eviction victim remains, as implemented in
try_evict_if_needed_async_spill_with_total_budget. Replace the overly broad
claim that only noeviction rejects writes with OOM while preserving the
distinction that spill remains inert.
- Around line 956-958: The disk offload warning logic in
disk_offload_spill_inert must use parsed active save rules rather than
save.is_none(), so an empty --save value is treated as having no RDB schedule.
Update the condition using the existing save-rule parsing representation and add
a test covering --save "".
In `@src/main.rs`:
- Around line 143-147: Split the oversized startup implementation in src/main.rs
into focused Rust modules before adding further logic, keeping main startup
behavior intact. Move related functions, types, and helpers together and update
module declarations/imports so callers still resolve correctly; ensure no Rust
source file exceeds the 1500-line limit.
- Around line 143-147: Update the startup comment above
warn_disk_offload_without_durability to describe the current behavior: eviction
policies drop eligible victims, while OOM occurs only with noeviction or when no
eligible victim exists; retain the note that disk offload remains inert without
a durability backstop and behavior is unchanged.
In `@src/storage/eviction.rs`:
- Around line 618-640: Split eviction.rs so every Rust file remains under the
1500-line limit: move the large test module into a separate test file/module,
and separate read/write eviction implementations if necessary. Preserve existing
module visibility, imports, and test coverage, with production symbols such as
evict_one_with_spill and related eviction APIs remaining correctly accessible.
🪄 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: 1d67839e-99a7-4aba-a97d-f6c25ba38cc9
📒 Files selected for processing (5)
CHANGELOG.mddocs/guides/tuning.mdsrc/config.rssrc/main.rssrc/storage/eviction.rs
Comment on lines
+934
to
+958
| /// 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); only `noeviction` 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() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split src/config.rs before extending it further.
The final file reaches Line 2808, so this change adds more configuration surface to a Rust file already well beyond the 1500-line limit. Move related implementation/tests into modules before continuing to grow this file.
As per coding guidelines, “No single Rust 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/config.rs` around lines 934 - 958, Split the oversized Config
implementation and its related tests from src/config.rs into focused Rust
modules before adding further configuration APIs. Preserve public interfaces and
behavior, including disk_offload_spill_inert and its associated tests, and
update module declarations/imports so all existing callers and tests continue to
compile. Ensure no resulting Rust file exceeds 1500 lines.
Source: Coding guidelines
Comment on lines
+956
to
+958
| pub fn disk_offload_spill_inert(&self) -> bool { | ||
| self.disk_offload_enabled() && self.appendonly != "yes" && self.save.is_none() | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
ast-grep outline src/config.rs --items all --type function
rg -n -C6 -- '--save|parse_save_rules|save\.is_some|save\.is_none' \
src/config.rs src/main.rsRepository: pilotspace/moon
Length of output: 15227
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect the save field definition and relevant parsing logic.
ast-grep outline src/config.rs --match 'struct ServerConfig' --view expanded
rg -n -C4 'save:|--save|parse_save_rules|save\.is_some|save\.is_none|disable form|empty.*save|snapshot rule' src/config.rs src/main.rs src/persistence -g '!target'
# Inspect auto-save rule parsing implementation.
ast-grep outline src/persistence/auto_save.rs --view expanded
sed -n '1,260p' src/persistence/auto_save.rsRepository: pilotspace/moon
Length of output: 28129
🏁 Script executed:
#!/bin/bash
set -eu
# Show the CLI field annotations and any normalization of save.
sed -n '130,170p' src/config.rs
sed -n '1230,1335p' src/config.rs
# Search for explicit tests or docs about disabling save snapshots.
rg -n -C3 'save.*""|--save ""|empty string|disable.*save|save is none|save rules empty|parse_save_rules\(&input\)' src tests .Repository: pilotspace/moon
Length of output: 46382
Use active save rules here
--save "" is still Some(""), so save.is_none() misses the disable form and suppresses this warning even when no RDB schedule is active. Check parsed save rules instead, and add coverage for --save "".
🤖 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/config.rs` around lines 956 - 958, The disk offload warning logic in
disk_offload_spill_inert must use parsed active save rules rather than
save.is_none(), so an empty --save value is treated as having no RDB schedule.
Update the condition using the existing save-rule parsing representation and add
a test covering --save "".
Comment on lines
+143
to
+147
| // 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(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split src/main.rs before adding more startup logic.
The final file reaches Line 1862, exceeding the repository’s 1500-line Rust-file limit.
As per coding guidelines, “No single Rust 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/main.rs` around lines 143 - 147, Split the oversized startup
implementation in src/main.rs into focused Rust modules before adding further
logic, keeping main startup behavior intact. Move related functions, types, and
helpers together and update module declarations/imports so callers still resolve
correctly; ensure no Rust source file exceeds the 1500-line limit.
Source: Coding guidelines
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the startup comment to match the new eviction behavior.
This comment still says --maxmemory becomes a hard reject, but evicting policies now drop eligible victims; OOM remains for noeviction and policies with no eligible victim.
Suggested wording
- // tier silently inert — --maxmemory becomes a hard reject-at-cap instead
- // of spilling. Warn once at startup; behavior is intentionally unchanged.
+ // tier silently inert — evicting policies drop eligible victims instead
+ // of spilling, while noeviction rejects writes at the cap. Warn once.📝 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
| // 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(); | |
| // GCP benchmark finding (2026-07-10): --disk-offload enable without a | |
| // durability backstop (--appendonly yes or --save) leaves the cold-spill | |
| // tier silently inert — evicting policies drop eligible victims instead | |
| // of spilling, while noeviction rejects writes at the cap. Warn once. | |
| config.warn_disk_offload_without_durability(); |
🤖 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/main.rs` around lines 143 - 147, Update the startup comment above
warn_disk_offload_without_durability to describe the current behavior: eviction
policies drop eligible victims, while OOM occurs only with noeviction or when no
eligible victim exists; retain the note that disk offload remains inert without
a durability backstop and behavior is unchanged.
Comment on lines
+618
to
+640
| // 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(()); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split src/storage/eviction.rs before adding more eviction logic.
The final file reaches Line 2187, exceeding the repository’s 1500-line Rust-file limit. Separate production eviction logic from the large test module and split read/write implementations as needed.
As per coding guidelines, “No single Rust 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 618 - 640, Split eviction.rs so every
Rust file remains under the 1500-line limit: move the large test module into a
separate test file/module, and separate read/write eviction implementations if
necessary. Preserve existing module visibility, imports, and test coverage, with
production symbols such as evict_one_with_spill and related eviction APIs
remaining correctly accessible.
Source: Coding guidelines
…remains Address CodeRabbit review on #273: the "only noeviction OOMs" phrasing understated the fail-close. An evicting policy also returns OOM when it has no eligible victim to drop — the canonical case is a volatile-* policy once its TTL-bearing keys are exhausted (persistent keys are retained, the write is rejected), exactly as the volatile-lru regression test verifies. Corrected in the three user-facing spots: CHANGELOG [Unreleased], the disk_offload_spill_inert doc comment + startup warn string (src/config.rs), and the "Pure cache" recipe note in docs/guides/tuning.md. No behavior change — wording only. author: Tin Dang <tindang.ht97@gmail.com>
Contributor
Author
|
Addressed CodeRabbit review in c89bac4:
|
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 13, 2026
… of plain-dropping them (#312) Root-causes the intermittent tests/cold_collection_visibility.rs CI flake (stable GHOST: EXISTS == 1 but LRANGE/ZRANGE/HGETALL/SMEMBERS permanently empty; reproduced on unmodified main, only on shared/slow Linux runners). Two compounding defects in storage::eviction::evict_one_with_spill (the synchronous spill path shared by the tick, the memory-pressure cascade's sync-spill fallback, and db_quota): 1. Collection victims (Hash/List/Set/ZSet/Stream) were gated behind a stale `is_string` check predating kv_spill::spill_to_datafile's full collection support (it already serializes any RedisValueRef via kv_serde) — a collection victim under a live SpillContext silently plain-dropped, indistinguishable from spill: None. 2. Even string victims never registered a ColdIndex entry after a successful sync spill (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. Separately, 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. shard::persistence_tick::run_eviction_tick now builds a real SpillContext (ShardManifest + shard data dir + next_file_id) whenever disk-offload is enabled and a manifest is present, threading it through; falls 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 documented "spill is inert without one" rule. Deterministic regression tests (no server process, no timing race): - storage::eviction::tests::sync_spill_non_string_victim_is_durably_spilled_not_plain_dropped - shard::timers::tests::test_run_eviction_spills_collection_victim_when_spill_context_given tests/cold_collection_visibility.rs is unmodified (its GHOST assertions are the correctness gate this fix satisfies) and green 10/10 locally on both runtimes (monoio release-fast + tokio jemalloc release-fast). Gates: cargo fmt --check clean; cargo clippy -- -D warnings clean (default + --no-default-features --features runtime-tokio,jemalloc); storage::eviction, shard::timers, shard::persistence_tick, storage::db_quota, storage::tiered lib test modules green. footer: closes task #45 author: Tin Dang Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
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
Fixes a default-config regression surfaced by the 2026-07-10 GCP disk-offload A/B: with
--disk-offload enable(the default) +--appendonly no+ no--save, an evictingmaxmemory-policy(allkeys-lru/lfu/random, volatile-*) would OOM-reject writes at the cap instead of evicting — i.e. the classic "Redis as an LRU cache" recipe was silently broken by default.Root cause: the inline write-path eviction gate (
try_evict_if_needed_async_spill_*) has noShardManifestand no AOF backstop underappendonly=no, so it took a policy-blind fail-close branch (added in #270 for crash-safety) that OOMs regardless of policy. #270's durable-spill replacement (evict_batch_durable_no_aof) only runs from the persistence tick, which needspersistence_dir(appendonly=yes || save), soappendonly=nofell through to the OOM.Fix (2 commits)
fix(config)— startuptracing::warn!when--disk-offloadis enabled without a durability backstop (disk_offload_spill_inert()predicate), documenting that evicting policies drop and noeviction OOMs at the cap.fix(storage)— the fail-close is now policy-aware, matching Redis semantics:total_memory <= budget→ no-opNoEviction→ still OOMs (correct; noeviction always rejects)evict_one_with_spill(..., spill: None), the same plain-drop helper the disk-offload-disabled path already uses).volatile-*stays scoped to TTL-bearing keys and OOMs rather than drop a persistent key.No behavior change to the
appendonly=yesdurable-spill path or the disk-offload-disabled path — the new branch only replaces the old blanket-OOM in the no-manifest/no-AOF case.Verification
Unit (red/green, both runtimes):
async_spill_no_aof_backstop_no_manifest_allkeys_lru_evicts_pure_cache(RED pre-fix: OOM panic → GREEN)async_spill_no_aof_backstop_no_manifest_noeviction_still_rejectsasync_spill_no_aof_backstop_no_manifest_volatile_lru_scoped_to_ttl_keysstorage::eviction32 passed;config125 passed; clippy-D warnings+ fmt clean on default andruntime-tokio,jemalloc.GCP e2e (x86 c3,
--disk-offload enable, 256 MiB cap, 500k unique 512B SETs, OOM-counting client — no abort-on-error):ok=489282 oom=10718(cache broken)ok=500000 oom=0(evicts, bounded)oom=107388rejects at cap ✓oom=0oom=0(unchanged) ✓Binaries verified distinct (md5 + fix-string presence) before the A/B.
Follow-up (not this PR)
A two-threshold functional offload-without-AOF (spill under appendonly=no with a crash-safe backstop) is deferred to v0.7 if it becomes a real requirement; today the correct default-config behavior is evict-or-OOM per policy, which this PR delivers.
Summary by CodeRabbit
noevictionnow reliably returns an OOM error at the limit, while other eviction policies reclaim memory by dropping keys as appropriate.