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>
TinDang97
added a commit
that referenced
this pull request
Jul 14, 2026
…ound-truth mismatch (task #44) crash_recovery_disk_offload_no_aof was the last red suite cited by PRODUCTION-CONTRACT.md row CRASH-01. Investigated whether it was harness rot (like task #31's WAL v2 probe rot) or a real durability defect (like task #60's replay no-op) with on-disk evidence before touching anything. Root cause: harness rot, not a data-loss regression. The test forced LRU eviction with a 16,000-key pipelined SET burst and asserted post-crash recovery >= 65% of PROBE_COUNT, assuming most evicted probes land durably in a heap-*.mpf file. That assumption predates disk_offload_spill_inert() / PR #273 (policy-aware eviction fail-close): under --appendonly no, the per-connection write-path eviction gate (run_write_eviction_gate, src/server/conn/handler_monoio/mod.rs) -- the path every ordinary SET/HSET past maxmemory actually takes -- has no ShardManifest handle and PLAIN-DROPS victims. This is documented Redis "pure cache, no durability" semantics (see docs/PRODUCTION-CONTRACT.md's CRASH-02 note on task #57, which made this drop-not-OOM behavior the intended fix, not a bug). Only the periodic memory-pressure tick (shard/persistence_tick.rs) durably spills under --appendonly no, and a busy connection's synchronous per-write eviction always wins the race before that tick ever observes a sustained over-budget shard. Confirmed via direct manifest/heap-*.mpf inspection: the old pipelined-SET burst durably spilled 0-1/200 probes, making the 65% floor permanently unreachable regardless of settle time or contention tuning. Fix (test-only, no src/ change): 1. write_filler now sends the filler as ONE MSET instead of 16,000 pipelined SETs. MSET's handler applies all pairs with no per-key eviction check, and the write-path gate that does run checks memory once, pre-MSET -- so a shard can end up far over budget with no further write pending to re-trigger the plain-drop path. The periodic tick is then the only thing left to reclaim it, durably spilling the LRU-oldest keys (the probes) via the manifest. 2. The fixed RECOVERY_FLOOR is replaced by count_durable_probe_entries, which reads each shard's manifest + Active KvLeaf DataFiles directly (the same way recover_shard_v3_pitr does at boot) right before the kill to establish ground truth, and asserts recovery returns AT LEAST that many probes -- the actual #22 regression signal, decoupled from eviction throughput. Verified the rewritten test still red-flags a reintroduced #22 regression: manually reverted the `persistence_dir.is_some() || disk_offload_base.is_some()` gate in main.rs, confirmed RED (ground truth 20, post 0), reverted back. Green 12x+ consecutive on macOS (monoio default + tokio runtime); no regression in crash_recovery_cold_del_resurrection or the crash_matrix_cross_plane no-durability-contract spot check (cross_plane_spot_offload_no_s1_no_durability_contract_liveness), which already independently documents the same drop-not-spill contract. fmt clean, clippy clean (default + tokio+jemalloc matrix). Updates CRASH-01's audit caveat in docs/PRODUCTION-CONTRACT.md and adds a CHANGELOG [Unreleased] entry. author: Tin Dang
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 14, 2026
…ound-truth mismatch (task #44) (#328) crash_recovery_disk_offload_no_aof was the last red suite cited by PRODUCTION-CONTRACT.md row CRASH-01. Investigated whether it was harness rot (like task #31's WAL v2 probe rot) or a real durability defect (like task #60's replay no-op) with on-disk evidence before touching anything. Root cause: harness rot, not a data-loss regression. The test forced LRU eviction with a 16,000-key pipelined SET burst and asserted post-crash recovery >= 65% of PROBE_COUNT, assuming most evicted probes land durably in a heap-*.mpf file. That assumption predates disk_offload_spill_inert() / PR #273 (policy-aware eviction fail-close): under --appendonly no, the per-connection write-path eviction gate (run_write_eviction_gate, src/server/conn/handler_monoio/mod.rs) -- the path every ordinary SET/HSET past maxmemory actually takes -- has no ShardManifest handle and PLAIN-DROPS victims. This is documented Redis "pure cache, no durability" semantics (see docs/PRODUCTION-CONTRACT.md's CRASH-02 note on task #57, which made this drop-not-OOM behavior the intended fix, not a bug). Only the periodic memory-pressure tick (shard/persistence_tick.rs) durably spills under --appendonly no, and a busy connection's synchronous per-write eviction always wins the race before that tick ever observes a sustained over-budget shard. Confirmed via direct manifest/heap-*.mpf inspection: the old pipelined-SET burst durably spilled 0-1/200 probes, making the 65% floor permanently unreachable regardless of settle time or contention tuning. Fix (test-only, no src/ change): 1. write_filler now sends the filler as ONE MSET instead of 16,000 pipelined SETs. MSET's handler applies all pairs with no per-key eviction check, and the write-path gate that does run checks memory once, pre-MSET -- so a shard can end up far over budget with no further write pending to re-trigger the plain-drop path. The periodic tick is then the only thing left to reclaim it, durably spilling the LRU-oldest keys (the probes) via the manifest. 2. The fixed RECOVERY_FLOOR is replaced by count_durable_probe_entries, which reads each shard's manifest + Active KvLeaf DataFiles directly (the same way recover_shard_v3_pitr does at boot) right before the kill to establish ground truth, and asserts recovery returns AT LEAST that many probes -- the actual #22 regression signal, decoupled from eviction throughput. Verified the rewritten test still red-flags a reintroduced #22 regression: manually reverted the `persistence_dir.is_some() || disk_offload_base.is_some()` gate in main.rs, confirmed RED (ground truth 20, post 0), reverted back. Green 12x+ consecutive on macOS (monoio default + tokio runtime); no regression in crash_recovery_cold_del_resurrection or the crash_matrix_cross_plane no-durability-contract spot check (cross_plane_spot_offload_no_s1_no_durability_contract_liveness), which already independently documents the same drop-not-spill contract. fmt clean, clippy clean (default + tokio+jemalloc matrix). Updates CRASH-01's audit caveat in docs/PRODUCTION-CONTRACT.md and adds a CHANGELOG [Unreleased] entry. author: Tin Dang Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 16, 2026
…les, ~129× fewer heap files (v0.8 item 3) (#350) * perf(storage): coalesce oversized entries into shared spill batch files v0.8 roadmap item 3 / task #57 follow-up: disk-offload spill files must batch effectively so file count scales as ~keys/batch, not ~keys. The G2 acceptance run (260K x 10KB keys, 256MB cap) produced ~236K heap-*.mpf files even though the format nominally supports batching (<=256 keys per file). Root cause: SpillThread::flush_buffer already coalesced up to 256 requests per flush, but partitioned them by value size before writing — every entry over INLINE_MAX_VALUE_BYTES (3500B, e.g. all of G2's 10KB values) was routed to its own dedicated single-entry file via spill_single_entry, defeating the batch entirely. build_kv_spill_batch / write_kv_spill_batch (src/storage/tiered/kv_spill.rs) are rewritten to pack inline AND oversized entries into ONE shared batch file per flush: each oversized entry gets a dedicated leaf page (holding an overflow pointer) plus its overflow-page chain placed inline in the same page stream, via a new BatchSlot enum (Leaf | Overflow) replacing the old two-Vec (leaves, overflow) BatchPages layout. The atomic temp-file+sync_all+rename+fsync_directory write sequence is unchanged, so the payload-before-reference durability ordering (a key is only marked cold after its containing segment is durable) is preserved. flush_buffer (src/storage/tiered/spill_thread.rs) no longer partitions the buffer by size — it builds one SpillEntry list and calls build_kv_spill_batch once per flush, producing one SpillCompletion (or falling back to per-entry salvage only on a build/write error). Fixing this uncovered a latent bug in build_overflow_chain (src/persistence/kv_page.rs): its prev/next link computation silently assumed every chain's start_page_id == 1 (true of every caller before this change, since a single-entry file always put its one leaf at page 0) — a chain-local i+1/i+2 formula that produced dangling/wrong links the moment a caller (this new batching code) passed a variable start_page_id > 1 for the second, third, etc. oversized entry sharing a file. Links are now computed as file-absolute (start_page_id + i), matching what read_overflow_chain already expected. Caught by the new test_rebuild_from_manifest_oversized_batch_roundtrip test before it could ship. Empirical measurement (macOS, matching G2's shape): RED (pre-fix) 3742 files / ~34,908 evicted 10KB keys; GREEN (post-fix) 29 files for the same eviction load — a ~129x reduction, spill_batches_flushed now matches file count 1:1 as designed. New tests: - kv_spill.rs::tests: test_build_kv_spill_batch_oversized_entries_share_one_file, test_build_kv_spill_batch_mixed_inline_and_oversized, test_rebuild_from_manifest_oversized_batch_roundtrip - tests/crash_recovery_spill_batch_kill9.rs: end-to-end kill-9-mid-spill regression guard. Drives a real server through a sustained eviction+spill burst of 4000 x 8000-byte (overflow-chain) values under --appendonly yes --appendfsync always, SIGKILLs it with no settle window (maximizing the chance of a torn trailing batch — either an unrenamed heap-*.tmp or a just-renamed file whose manifest commit raced the kill), then restarts and asserts zero acknowledged-write loss, byte- exact content, a batched (<= keys/64) heap-file count before the kill, and no lingering .tmp files after the post-restart orphan sweep settles. Verified green on both the monoio (default) and tokio (--features runtime-tokio,jemalloc) runtimes. Note: an earlier version of this test restarted round 2 with the same small --maxmemory as round 1 and saw non-deterministic scattered loss (68-533 keys across repeated runs) on BOTH this fix and the pre-fix binary (A/B'd by hand via git stash) — root-caused to AOF replay re-inserting the full dataset hot with no eviction gate, then racing the verification reads' cold-promotions against the periodic post-restart eviction tick. That is a distinct, pre-existing gap orthogonal to spill batching (adjacent to the in-flight used_memory-accounting fix in a sibling worktree) and out of scope here; round 2 now restarts with a generous --maxmemory to remove that confound and isolate the contract actually in scope. Preserved invariants (verified via the full existing kv_spill/spill_thread test suite plus the crash test above): next_spill_file_id_seed uniqueness across orphans, crash-orphan sweep classification, FLUSH/DEL tombstone + cold-plane replay (PR #257), and eviction fail-close semantics (#273) are all unaffected — none of that logic was touched. Gates: cargo fmt --check, cargo clippy -- -D warnings (default features and --no-default-features --features runtime-tokio,jemalloc, both including --tests), full cargo test --lib (4333 passed), targeted kv_spill/spill_thread/kv_page unit tests, the new crash_recovery_spill_batch_kill9 integration test (3x stable on monoio, 1x green on tokio), and the pre-existing crash_recovery_disk_offload_no_aof regression test — all green. Not in scope (deferred, flagged for follow-up): the synchronous per-key SpillContext eviction path (timers::run_eviction, used when the async memory-pressure cascade is not active) still spills one file per victim. The empirical repro matching G2's shape (--appendonly yes) confirmed the async batching path above is dominant and fully fixes the reported regression; the sync path is lower-traffic and was left alone given the scoped time budget. author: Tin Dang <tindang.ht97@gmail.com> * fix(storage): close adversarial-review gaps on spill-segment-batching Adversarial review of perf/spill-segment-batching (SHIP-WITH-FIXES) flagged three gaps in the spill-file-batching rewrite before merge: 1. Test gap: the existing kill-9 test only proves AOF-replay-derived recovery (every acked key ends up hot via DispatchReplayEngine, never touching cold_read_through), so it could not tell "the leaf-offset + overflow-chain-with-start_page_id>1 read path works" apart from "AOF replay papered over a broken cold layout". Added spill_batch_shared_file_survives_cold_read_after_kill9 (tests/crash_recovery_spill_batch_kill9.rs): pre-seeds a 3-entry shared batch file (1 inline + 2 oversized, ordered so the second oversized entry's overflow chain starts at file-absolute page_idx > 1) directly via build_kv_spill_batch/write_kv_spill_batch + ShardManifest, boots a real --appendonly no server on top of it, kills it without ever touching the keys, restarts, and GETs all three -- the only way any of these bytes reach the client is a genuine ColdIndex::rebuild_from_manifest + cold_read_through read. Getting this test to actually exercise that path (rather than a hot-RAM hit) required pulling in src/persistence/recovery.rs's fix from the concurrent task #56 effort (fix/t56-used-memory-offload, commit 689c52e): v3 recovery Phase 3 used to re-insert every previously-spilled String entry directly into the hot DashTable in ADDITION to rebuilding the ColdIndex stub for the same key -- for an entry_flags::OVERFLOW entry this hot-preload loop ignored the flag and inserted the raw 4-byte page-pointer as if it were the literal value, silently corrupting every oversized cold key on every restart (a pre-existing bug, independent of this batching fix -- it affected the old single-entry-per-file layout too). That recovery.rs hunk is a clean, self-contained cherry-pick (verified: 689c52e's parent has an IDENTICAL recovery.rs to this branch's pre-fix state, and the hunk does not touch any of task #56's other, out-of-scope-here, used_memory ledger/RSS/AOF-replay-demotion changes) and is a hard prerequisite for this test's premise -- there is no way to route around it from this side. A companion library-level unit test, test_rebuild_from_manifest_mixed_inline_and_oversized_roundtrip (kv_spill.rs), independently proves the exact same read path correct via direct calls to the production functions, without depending on server boot sequencing. 2. Memory-pressure characteristic: build_kv_spill_batch materialized an entire flush's pages in RAM before write_kv_spill_batch touched disk -- fine for the old ~50KB/256-small-entries case, but this fix lets oversized entries share that same unconditional batch, so 256 consistently-large (e.g. near-maxmemory-sized) values could resident hundreds of MB at once, exactly when spills fire under memory pressure. flush_buffer (spill_thread.rs) now splits each FLUSH_ENTRY_CAP-sized buffer into sub-batches capped at 4 MiB of cumulative value_bytes (chosen against the OLD per-entry path's peak -- one value's pages at a time, no cross-entry amplification -- while leaving the G2 acceptance shape, 256 x ~10KB = ~2.5 MiB, as a single file, unchanged), each becoming its own file via that sub-batch's own already-pre-assigned file_id. New unit test: oversized_flush_splits_into_byte_capped_sub_batches. 3. Hygiene: fixed the stale FLUSH_ENTRY_CAP comment (no longer claims to bound in-RAM size by itself -- superseded by BATCH_BYTES_CAP) and the stale entry_flags::OVERFLOW doc (corrected from a fictitious 12-byte file_id:u64+page_id:u32 layout to the actual 4-byte file-absolute start_page_idx -- no file_id is stored, the chain always lives in the same physical file as its leaf stub). Added a comment at write_kv_spill_batch explaining why it hand-rolls temp+fsync+rename instead of calling persistence::atomic::atomic_write_durable: that helper takes one pre-materialized &[u8], and concatenating batch.pages into a single buffer first would undo BATCH_BYTES_CAP's whole point. Gates (macOS, this worktree): - cargo fmt --check: clean - cargo clippy --tests -- -D warnings (default features): clean - cargo clippy --no-default-features --features runtime-tokio,jemalloc --tests -- -D warnings: clean - cargo test --release --lib (default features): 4335 passed, 0 failed, 1 ignored - cargo test --no-default-features --features runtime-tokio,jemalloc --release --lib: 3518 passed, 0 failed, 1 ignored - cargo test --release --lib storage::tiered::kv_spill::: 16 passed - cargo test --release --lib persistence::kv_page::: 23 passed - cargo test --release --lib persistence::recovery::: 15 passed - cargo test --release --lib storage::tiered::spill_thread::: 13 passed (incl. new byte-cap-split test) - cargo test --release --test crash_recovery_spill_batch_kill9 -- --ignored (monoio, default features): 2 passed (spill_batch_shared_file_survives_cold_read_after_kill9 re-run 3x clean before this final pass) - cargo test --no-default-features --features runtime-tokio,jemalloc --release --test crash_recovery_spill_batch_kill9 -- --ignored (tokio, MOON_NO_URING=1): 2 passed author: Tin Dang <tindang.ht97@gmail.com> --------- 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.