fix(shard): OOM shield — macOS maxmemory guardrail + eviction bypass closure (RSS/CPU wave 2)#217
Merged
Merged
Conversation
RSS/CPU/OOM review item 4: macOS (a first-class target) booted "UNLIMITED + noeviction" whenever --maxmemory was omitted because detect_memory_limit_bytes() was Linux-only — no OOM defense at all. The probe now reads `sysctl -n hw.memsize` (spawned command, not sysctlbyname FFI, so no new unsafe block; one fork at startup) and feeds the same 80% auto-cap + allkeys-lru guardrail as Linux. Red/green: config::tests::macos_detects_memory_limit. author: Tin Dang
…passes
--maxmemory + eviction is enforced only in the four connection handlers
(run_write_eviction_gate in handler_monoio/mod.rs and its equivalents in
handler_sharded, handler_single, blocking.rs). Three write paths never ran
that check, so memory could be driven past maxmemory without limit:
1. Cross-shard SPSC write legs (src/shard/spsc_handler.rs): Execute,
MultiExecute, PipelineBatch and their *Slotted variants executed write
commands against the TARGET shard's Database directly, bypassing the
connection handler's eviction gate entirely. A pipeline of
individually-routed SET commands hashing to a remote shard kept
returning +OK far past the point where a same-shard write would already
OOM.
2. Lua redis.call/redis.pcall writes (src/scripting/bridge.rs): EVAL and
EVALSHA carry no WRITE command flag (src/command/metadata.rs), so the
dispatch-level OOM check never saw them, and the bridge itself never ran
the eviction check before executing a write inside a script. A tight
redis.call('SET', ...) loop could write arbitrarily far past the cap.
3. Cross-db COPY ... DB n (src/shard/spsc_handler.rs): COPY is
special-cased ahead of the generic write path (same as MOVE — both need
two &mut Database borrows at once), so the generic gate never saw it
either. COPY duplicates the value into the destination db, unlike
same-shard MOVE which is net-zero (key leaves the source db as it lands
in the destination, same rationale as DEL) and stays ungated by design.
Found while closing (1) above, same file, same intercept family.
Fix mirrors the existing handler-level gate exactly, in all three cases:
- New spsc_eviction_gate() helper in spsc_handler.rs duplicates
run_write_eviction_gate's elastic-budget lookup (GAP-1) and
spill-vs-plain branching, since spsc_handler.rs is runtime-agnostic
(shared by runtime-monoio and runtime-tokio) and cannot call the
monoio-cfg-gated handler function directly. Threaded through
drain_spsc_shared/handle_shard_message_shared and gated on a
once-per-drain-cycle `evict_active` snapshot (perf parity with
handler_monoio's batch_eviction_active: zero extra RwLock read when
maxmemory is unset, the common case). The generic write-path call site
and the COPY special-case call site both go through this same helper,
the latter against the destination db (ca.dst_db) before copy_core.
- New LuaEvictionCtx in scripting/bridge.rs is captured by value into the
redis.call/redis.pcall closures at VM-setup time (setup_lua_vm, called
once per shard from the 4 conn_accept.rs spawn sites) — zero new unsafe
code (closure capture, not a new thread-local raw pointer) and zero
per-call allocation (an Option check short-circuits when maxmemory is
unset). redis.call raises the OOM error as a Lua runtime error (script
aborts); redis.pcall returns it as an {err = ...} table. Read-only
scripts are unaffected. FCALL-internal writes keep a documented,
pre-existing gap (no shard context threaded through the function-library
loader) — explicitly out of scope for this fix.
New wire-level suite tests/oom_bypass_closure.rs (4 cases, both runtimes):
direct-SET control (A, sanity — already enforced), cross-shard pipeline
(B), Lua EVAL loop (C), read-only EVAL under memory pressure not blocked
(D, locks the write-only scope of the Lua gate). B and C reproduced RED
against the pre-fix binary (268/3000 OOM errors instead of the expected
majority for B; EVAL completed as 'DONE' instead of erroring for C) and
all 4 cases are GREEN after the fix on both runtime-monoio (default) and
runtime-tokio.
Case B deliberately avoids MSET: coordinate_mset's cross-shard scatter
path (src/shard/coordinator.rs) discards every remote leg's reply and
hardcodes +OK regardless of outcome — a separate, pre-existing coordinator
bug independent of the SPSC eviction gate this fix targets, found while
tracing the reply path for this test. Left unfixed (out of scope) and
flagged for a follow-up.
author: Tin Dang
…ction coverage Follow-up to 5cea6912. Review flagged that the COPY-dest fix in that commit (spsc_handler.rs's plain Execute-arm two-database intercept) was untested end-to-end: no wire-level case exercised it, so "COPY dest is gated" was asserted, not proven. Verified COPY's actual eviction coverage first (metadata.rs confirms `COPY` and `BITOP` both carry the generic `W` write flag): on the cross-shard dispatch arms handler_monoio actually uses for remote writes (ExecuteSlotted, PipelineBatchSlotted), COPY already hits the same generic `is_write` eviction gate as any other write command, added in 5cea6912 for cases B/C — there is no COPY-specific bypass on that path. The Execute-arm dest-gate from 5cea6912 remains correct and is kept (it closes the narrower gap on handler_sharded's single-key remote-dispatch arm, which does bypass the generic gate via an early return), but it is not what the new test below exercises. New case E in tests/oom_bypass_closure.rs: SET N keys under budget, then COPY each to db 1 with a DISTINCT dst key (same src/dst key name is rejected by Redis semantics regardless of db, which was the bug in an earlier draft of this test — it produced a false 0-OOM reading because every same-key copy failed before any memory grew). Confirmed genuine RED by reverting the 8 src/ files from 5cea6912 to their parent-commit versions, rebuilding, and rerunning case E: 0/300 OOM (git-stash-style verification, not a mechanical `git stash` since the fix was already committed). Restored the fix via `git checkout --`, rebuilt, reran: 104/300 OOM. Threshold (N/10 = 30) sits safely between both observed counts. GREEN on both runtime-monoio (default) and runtime-tokio. Also documents, but does not fix, a separate pre-existing bug found while building this test: cross-shard remote COPY ... DB n silently ignores the DB clause and performs a same-db copy instead, because the two-database intercept exists only in the plain Execute arm, not ExecuteSlotted/PipelineBatchSlotted. Confirmed via manual probe (4/20 remote COPYs landed in the wrong db). Data-correctness bug, independent of --maxmemory (the wrong same-db copy is still eviction-gated by the generic path), out of scope for this fix — flagged as a follow-up requiring the intercept to be replicated across every ShardMessage arm. CHANGELOG.md updated to state precisely what is and isn't closed: COPY eviction coverage is confirmed/hardened; the COPY DB-clause functional bug is a new, separate, open finding. author: Tin Dang
Post-commit self-review (advisor pass) of 5cea6912 + 7365db09 flagged two places where the CHANGELOG's phrasing overstated what was actually tested: 1. The Execute-arm COPY-dest gate (5cea6912) is exercised by zero wire-level test cases, including the new case E — E pipelines COPY, which routes through ExecuteSlotted/PipelineBatchSlotted on both runtimes, not the plain Execute arm (only reachable via handler_sharded's single-key remote dispatch, which no test in this suite drives). The gate is correct by inspection (verbatim mirror of the tested generic gate) but that is a weaker claim than "gated and tested" — the CHANGELOG now says so explicitly instead of implying test coverage it doesn't have. 2. Case E's RED (0/300 OOM) is produced by reverting the same fix commit that also REDs case B — it demonstrates COPY drives the generic `is_write` eviction gate to OOM once the underlying gate exists, but it is not an independently-discovered COPY-specific RED distinct from B's. Said so explicitly rather than implying E caught its own bypass. 3. The "same-shard MOVE is left ungated (net-zero)" line read as if MOVE's cross-shard DB-clause handling were otherwise fine. It shares COPY's structural gap (the two-database intercept lives only in the plain Execute arm), so it plausibly has the same DB-clause functional defect found for COPY — unverified, not investigated further (out of scope), but now flagged in the same found-not-fixed bullet instead of left unmentioned. No code changes; no test changes. Documentation precision only, so future readers (including future-me) don't over-read what was proven versus what was inspected/inferred. author: Tin Dang
…OM test threshold
Deep-review fixes on the eviction-bypass-closure work:
1. LuaEvictionCtx::gate took a RuntimeConfig RwLock read on EVERY
write redis.call/pcall, even with maxmemory unset — the doc comment
claimed a lock-free short-circuit that did not exist. The gate now
decides "is eviction active?" from a per-script-execution snapshot:
set_script_db bumps a thread-local SCRIPT_GENERATION counter and the
ctx caches (generation, active) in a Cell, so a tight
redis.call('SET', ...) loop pays one Cell read per write call and
reads the lock at most once per script run — mirroring the
batch_eviction_active per-batch snapshot the connection handlers
use. Staleness is bounded to one script execution, same as the
handlers' per-batch bound.
2. tests/oom_bypass_closure.rs case B threshold was flaky: GAP-1
elastic budget redistributes per-shard caps on a 100ms snapshot
tick, so the OK-count before OOM kicks in is timing-sensitive
(observed 500-1100 OKs across runs). Threshold widened from
2N/3 to N/2, which still cleanly discriminates the pre-fix
ceiling (~N/4 = 750 OOMs, bypass present) from the post-fix floor
(~1900+ OOMs observed).
Perf-reviewer finding on the SPSC drain (one lock read per drain
cycle) reviewed and accepted as-is: matches the established
batch_eviction_active precedent, amortized over up to 256 messages;
an atomic maxmemory snapshot is noted as a future refinement.
author: Tin Dang
📝 WalkthroughWalkthroughThis PR adds ChangesEviction bypass closure
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Lua
participant redis_api
participant LuaEvictionCtx
participant write_command
Lua->>redis_api: redis.call / redis.pcall
redis_api->>LuaEvictionCtx: gate(db, db_idx)
LuaEvictionCtx-->>redis_api: pass or OOM Frame
alt gate passes
redis_api->>write_command: execute write
else gate fails
redis_api-->>Lua: return OOM result
end
sequenceDiagram
participant event_loop as event loop
participant drain as drain_spsc_shared
participant handler as handle_shard_message_shared
participant gate as spsc_eviction_gate
event_loop->>drain: drain(runtime_config, spill_sender, spill_file_id, disk_offload_dir)
drain->>drain: compute evict_active
drain->>handler: dispatch(evict_active, eviction context)
handler->>gate: gate write command
gate-->>handler: pass or OOM Frame
sequenceDiagram
participant shard as SPSC shard handler
participant two_db as try_two_db_intercept
participant gate as spsc_eviction_gate
participant db as destination database
shard->>two_db: MOVE or COPY ... DB n
two_db->>gate: gate destination write
alt gate passes
two_db->>db: move_core / copy_core
two_db-->>shard: Frame response
else gate fails
two_db-->>shard: OOM Frame
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoClose maxmemory eviction bypasses in SPSC + Lua, enable guardrail on macOS
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
50 rules 1. setup_lua_vm() unwrap unannotated
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| #[test] | ||
| fn test_setup_lua_vm() { | ||
| let lua = setup_lua_vm().unwrap(); | ||
| let lua = setup_lua_vm(bridge::LuaEvictionCtx::disabled()).unwrap(); |
There was a problem hiding this comment.
2. setup_lua_vm() unwrap unannotated 📘 Rule violation ✧ Quality
The modified test line calls .unwrap() without the required #[allow(clippy::unwrap_used)] plus a preceding single-line justification comment. This violates the unwrap-annotation requirement for Rust diffs (including tests).
Agent Prompt
## Issue description
A changed test line uses `.unwrap()` without the required `#[allow(clippy::unwrap_used)]` attribute and an immediately preceding `// ...` justification comment.
## Issue Context
The policy requires each unwrap in diffs (including tests) to be explicitly acknowledged and justified to avoid accidental panics.
## Fix Focus Areas
- src/scripting/mod.rs[318-380]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+73
to
+75
| /// Real gate, built from the shard's own handles at VM-setup time. | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub fn new( |
There was a problem hiding this comment.
3. luaevictionctx::new allow lacks comment 📘 Rule violation ✧ Quality
LuaEvictionCtx::new adds #[allow(clippy::too_many_arguments)] without an explicit justification comment. This violates the requirement that new clippy suppressions be justified in-code.
Agent Prompt
## Issue description
A new `#[allow(clippy::too_many_arguments)]` suppression was added without a justification comment.
## Issue Context
The compliance rule requires new lint suppressions to be narrowly scoped and justified to avoid masking real issues.
## Fix Focus Areas
- src/scripting/bridge.rs[73-82]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
160
to
+177
| let lua = { | ||
| let mut lua_opt = lua_rc.borrow_mut(); | ||
| if lua_opt.is_none() { | ||
| *lua_opt = | ||
| Some(crate::scripting::setup_lua_vm().expect("Lua VM initialization failed")); | ||
| // M3 fix: bake this shard's OOM eviction context into the | ||
| // redis.call/pcall closures at VM-setup time (once per shard, | ||
| // shared by every connection landing here — shard_id/spill | ||
| // handles/runtime_config are identical across connections). | ||
| let eviction_ctx = crate::scripting::bridge::LuaEvictionCtx::new( | ||
| sdbs.clone(), | ||
| runtime_config.clone(), | ||
| shard_id, | ||
| spill_sender.clone(), | ||
| spill_file_id.clone(), | ||
| disk_offload_dir.clone(), | ||
| ); | ||
| *lua_opt = Some( | ||
| crate::scripting::setup_lua_vm(eviction_ctx).expect("Lua VM initialization failed"), | ||
| ); |
There was a problem hiding this comment.
4. setup_lua_vm runs before eval 📘 Rule violation ➹ Performance
The Lua VM is initialized during connection spawn (when lua_opt.is_none()), not lazily on the first EVAL handling path. This means the Lua sandbox can be created even if no client ever issues EVAL/EVALSHA.
Agent Prompt
## Issue description
Lua sandbox/VM initialization occurs during connection spawn rather than being lazily initialized on the first EVAL command.
## Issue Context
The compliance requirement is to avoid constructing the Lua sandbox unless scripting is actually used, and to make initialization happen only on the EVAL path.
## Fix Focus Areas
- src/shard/conn_accept.rs[160-180]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1032
to
+1041
| fn detect_memory_limit_bytes() -> Option<usize> { | ||
| let out = std::process::Command::new("sysctl") | ||
| .args(["-n", "hw.memsize"]) | ||
| .output() | ||
| .ok()?; | ||
| if !out.status.success() { | ||
| return None; | ||
| } | ||
| std::str::from_utf8(&out.stdout).ok()?.trim().parse().ok() | ||
| } |
There was a problem hiding this comment.
5. Path-dependent sysctl probe 🐞 Bug ☼ Reliability
On macOS, detect_memory_limit_bytes() invokes Command::new("sysctl") and converts any spawn/exec
failure into None, which makes the memory guardrail resolve to Skipped and boot UNLIMITED. This
can happen in environments where sysctl is not resolvable via PATH (or is shadowed), undermining
the PR’s goal of making the macOS guardrail reliably engage.
Agent Prompt
## Issue description
On macOS, `detect_memory_limit_bytes()` runs `sysctl` via PATH lookup and drops errors via `.output().ok()?`, so failure to resolve/execute `sysctl` makes the guardrail act as if no memory limit is detectable and the server boots UNLIMITED.
## Issue Context
The guardrail outcome is logged as `Skipped` (UNLIMITED) when detection returns `None`, so a PATH-dependent failure defeats the macOS-first-class goal.
## Fix Focus Areas
- Prefer an absolute path for sysctl on macOS (e.g. `/usr/sbin/sysctl`), optionally falling back to `sysctl` if the absolute path is missing.
- When the probe fails (spawn error or non-zero exit), emit a `tracing::warn!` with the error/exit status to distinguish “probe failed” from “platform unsupported”.
- src/config.rs[1027-1041]
- src/config.rs[1078-1103]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+41
to
+56
| fn find_moon_binary() -> std::path::PathBuf { | ||
| if let Ok(bin) = std::env::var("MOON_BIN") { | ||
| let p = std::path::PathBuf::from(bin); | ||
| if p.exists() { | ||
| return p; | ||
| } | ||
| } | ||
| let manifest = env!("CARGO_MANIFEST_DIR"); | ||
| let release = std::path::PathBuf::from(format!("{manifest}/target/release/moon")); | ||
| if release.exists() { | ||
| return release; | ||
| } | ||
| let debug = std::path::PathBuf::from(format!("{manifest}/target/debug/moon")); | ||
| if debug.exists() { | ||
| return debug; | ||
| } |
There was a problem hiding this comment.
6. Test hardcodes non-windows binary 🐞 Bug ⚙ Maintainability
The new tests/oom_bypass_closure.rs locates the server binary as target/{release,debug}/moon
without appending the platform executable suffix, so it will fail to find the built binary on
Windows unless MOON_BIN is manually set. This reduces cross-platform CI/dev portability for the
new regression suite.
Agent Prompt
## Issue description
`find_moon_binary()` hardcodes `target/release/moon` and `target/debug/moon`, which won’t match the produced artifact name on Windows (`moon.exe`).
## Issue Context
This is a wire-level integration test intended to run in CI across runtimes/platforms.
## Fix Focus Areas
- Use `std::env::consts::EXE_SUFFIX` (or equivalent) when constructing candidate paths.
- Alternatively, gate the test with `#[cfg(not(windows))]` if Windows support is intentionally out of scope.
- tests/oom_bypass_closure.rs[41-56]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/oom_bypass_closure.rs (1)
300-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated "drive to OOM" loop into a shared helper.
Case A and Case D both hand-roll the same SET-until-OOM loop; Case D's comment even calls out reusing Case A's setup. Consolidating avoids the two copies drifting apart.
♻️ Suggested helper
/// Writes SET commands with `key_prefix:{i}` keys until an OOM error is /// observed (or `max_iters` is exhausted). Returns whether OOM was hit. fn drive_to_oom(c: &mut Client, key_prefix: &str, value: &[u8], max_iters: usize) -> bool { for i in 0..max_iters { let key = format!("{key_prefix}:{i}"); if c.cmd(&[b"SET", key.as_bytes(), value]).is_oom_error() { return true; } } false }- let mut oomed = false; - for i in 0..MAX_ITERS { - let key = format!("a:{i}"); - let r = c.cmd(&[b"SET", key.as_bytes(), &value]); - if r.is_oom_error() { - oomed = true; - break; - } - assert_eq!( - r, - V::Simple("OK".into()), - "SET a:{i} unexpected reply {r:?}" - ); - } + let oomed = drive_to_oom(&mut c, "a", &value, MAX_ITERS);- let mut set_up_oom = false; - for i in 0..200 { - let key = format!("d:{i}"); - let r = c.cmd(&[b"SET", key.as_bytes(), &value]); - if r.is_oom_error() { - set_up_oom = true; - break; - } - } + let set_up_oom = drive_to_oom(&mut c, "d", &value, 200);Also applies to: 444-456
🤖 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 `@tests/oom_bypass_closure.rs` around lines 300 - 313, Extract the repeated SET-until-OOM loop used in the Case A and Case D logic into a shared helper, such as a small function near the existing test helpers in oom_bypass_closure.rs. Keep the helper behavior aligned with the current loop in the test body by taking the client, key prefix, value, and max iteration count, then returning whether OOM was reached; update both call sites to use this helper so the Case D path reuses Case A’s setup and the duplicate loop cannot drift.
🤖 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 `@src/config.rs`:
- Around line 1032-1036: The detect_memory_limit_bytes() helper currently
invokes sysctl via Command::new("sysctl"), which relies on $PATH lookup. Update
this call to use the fixed absolute macOS path for sysctl instead, keeping the
rest of the command construction and parsing logic unchanged so the binary
location is pinned and not environment-dependent.
In `@src/shard/spsc_handler.rs`:
- Around line 689-705: The OOM gating in spsc_handler is treating all write
paths the same, so delete-style commands like DEL and UNLINK can be rejected
before they have a chance to free memory. Update run_write_eviction_gate and
spsc_eviction_gate to carry and inspect command metadata instead of only relying
on metadata::is_write, and make try_evict_if_needed_budget DENYOOM-aware so
memory-freeing writes are allowed through the gate when maxmemory-policy is
noeviction.
---
Nitpick comments:
In `@tests/oom_bypass_closure.rs`:
- Around line 300-313: Extract the repeated SET-until-OOM loop used in the Case
A and Case D logic into a shared helper, such as a small function near the
existing test helpers in oom_bypass_closure.rs. Keep the helper behavior aligned
with the current loop in the test body by taking the client, key prefix, value,
and max iteration count, then returning whether OOM was reached; update both
call sites to use this helper so the Case D path reuses Case A’s setup and the
duplicate loop cannot drift.
🪄 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: e367de96-6108-4f1b-adfb-1f7c75e8cf8e
📒 Files selected for processing (11)
CHANGELOG.mdsrc/config.rssrc/scripting/bridge.rssrc/scripting/functions.rssrc/scripting/mod.rssrc/scripting/sandbox.rssrc/shard/conn_accept.rssrc/shard/event_loop.rssrc/shard/mod.rssrc/shard/spsc_handler.rstests/oom_bypass_closure.rs
Comment on lines
+1032
to
+1036
| fn detect_memory_limit_bytes() -> Option<usize> { | ||
| let out = std::process::Command::new("sysctl") | ||
| .args(["-n", "hw.memsize"]) | ||
| .output() | ||
| .ok()?; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Use an absolute path for the sysctl binary.
Command::new("sysctl") resolves the binary via $PATH, which is attacker-influenceable in some deployment scenarios (e.g., inherited/misconfigured environment). Since sysctl lives at a fixed, well-known location on macOS, pinning the path removes this class of risk for essentially no cost.
🔒 Proposed fix
- let out = std::process::Command::new("sysctl")
+ let out = std::process::Command::new("/usr/sbin/sysctl")
.args(["-n", "hw.memsize"])
.output()
.ok()?;📝 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
| fn detect_memory_limit_bytes() -> Option<usize> { | |
| let out = std::process::Command::new("sysctl") | |
| .args(["-n", "hw.memsize"]) | |
| .output() | |
| .ok()?; | |
| let out = std::process::Command::new("/usr/sbin/sysctl") | |
| .args(["-n", "hw.memsize"]) | |
| .output() | |
| .ok()?; |
🤖 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 1032 - 1036, The detect_memory_limit_bytes()
helper currently invokes sysctl via Command::new("sysctl"), which relies on
$PATH lookup. Update this call to use the fixed absolute macOS path for sysctl
instead, keeping the rest of the command construction and parsing logic
unchanged so the binary location is pinned and not environment-dependent.
Comment on lines
689
to
+705
| if is_write { | ||
| crate::shard::slice::with_shard_db(db_idx, |db| { | ||
| if evict_active { | ||
| if let Err(oom) = spsc_eviction_gate( | ||
| db, | ||
| db_idx, | ||
| shard_databases, | ||
| shard_id, | ||
| runtime_config, | ||
| spill_sender, | ||
| spill_file_id, | ||
| disk_offload_dir, | ||
| ) { | ||
| oom_frame = Some(oom); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare gating predicate on the local write path vs is_write here.
rg -nP -C4 'run_write_eviction_gate' src/server/conn/handler_monoio/ || true
# Does the eviction budget check distinguish denyoom / memory-reducing commands?
rg -nP -C3 'denyoom|DENYOOM|is_write|deny_oom' src/storage/eviction.rs || trueRepository: pilotspace/moon
Length of output: 1736
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== handler_monoio::run_write_eviction_gate =='
sed -n '1,170p' src/server/conn/handler_monoio/mod.rs
echo
echo '== eviction.rs outline =='
ast-grep outline src/storage/eviction.rs --view expanded || true
echo
echo '== eviction.rs relevant slices =='
rg -n -C4 'try_evict_if_needed_budget|DENYOOM|denyoom|is_write|memory-reducing|DEL|UNLINK' src/storage/eviction.rs src/storage -g '!**/target/**' || trueRepository: pilotspace/moon
Length of output: 28945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== eviction.rs: budget path and OOM handling =='
sed -n '287,360p' src/storage/eviction.rs
echo
echo '== command metadata flags for DENYOOM =='
rg -n -C3 'DENYOOM|denyoom|deny_oom|is_write' src/command src/storage src/server -g '!**/target/**' || true
echo
echo '== cross-shard slice path around eviction gate =='
sed -n '660,730p' src/shard/spsc_handler.rsRepository: pilotspace/moon
Length of output: 33983
Allow memory-freeing writes through the OOM gate. run_write_eviction_gate and spsc_eviction_gate only key off metadata::is_write, and try_evict_if_needed_budget takes no command metadata, so under maxmemory-policy noeviction an over-budget DEL/UNLINK can be rejected before dispatch. That blocks the writes that could free memory; the gate needs DENYOOM-aware handling so delete-style commands can still recover.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shard/spsc_handler.rs` around lines 689 - 705, The OOM gating in
spsc_handler is treating all write paths the same, so delete-style commands like
DEL and UNLINK can be rejected before they have a chance to free memory. Update
run_write_eviction_gate and spsc_eviction_gate to carry and inspect command
metadata instead of only relying on metadata::is_write, and make
try_evict_if_needed_budget DENYOOM-aware so memory-freeing writes are allowed
through the gate when maxmemory-policy is noeviction.
Two call sites answered the same question — "is --maxmemory nonzero?" — with different, more expensive mechanisms: - `drain_spsc_shared` (src/shard/spsc_handler.rs) took a `runtime_config.read()` lock once per SPSC drain cycle. - `LuaEvictionCtx::gate` (src/scripting/bridge.rs) used a generation-counter/`Cell` snapshot (`SCRIPT_GENERATION`, bumped by `set_script_db`) to avoid a per-`redis.call` lock read. Publish that boolean as a single process-global atomic instead: `storage::eviction::MAXMEMORY_GLOBAL` (`AtomicU64`, 0 = unset), with `publish_maxmemory(bytes)` (Relaxed store) and `maxmemory_is_set()` (Relaxed load != 0). This is a distinct, simpler atomic from the pre-existing `MAXMEMORY_HINT`/`MAXMEMORY_PER_SHARD_HINT` pair, which additionally encodes the exact budget for the inline write path's skip-eviction arithmetic — those are untouched. Published at every production write site of `RuntimeConfig.maxmemory`: `CONFIG SET maxmemory` (src/command/config.rs) and server startup (src/main.rs, after the guardrail-capped value resolves into `RuntimeConfig`). `src/shard/shared_databases.rs`'s `rt_config()` helper was checked and confirmed test-only (used only by `#[cfg(test)]` elastic-budget tests) — no publish needed there. Consumers replaced: - spsc_handler.rs:141's per-drain-cycle `evict_active` snapshot now reads `maxmemory_is_set()` instead of taking the `RuntimeConfig` lock. `runtime_config` is still threaded through unchanged — the actual eviction pass (`spsc_eviction_gate`) still needs the live budget. - bridge.rs's `LuaEvictionCtx::gate` drops the `SCRIPT_GENERATION` thread-local, the `evict_active: Cell<(u64, bool)>` field, and the generation-comparison branch entirely; replaced with a single `maxmemory_is_set()` load ORed with the existing `spill_sender.is_some()` check. `set_script_db`'s signature is unchanged (only the generation bump inside it is removed). Tests (red/green TDD): - Unit test `maxmemory_publish_and_is_set_roundtrip` (src/storage/eviction.rs) covers publish/un-publish round-tripping. - New case F in tests/oom_bypass_closure.rs: server starts with `--maxmemory 0 --disk-offload disable` (both required — omitting `--maxmemory` trips the config auto-guardrail's nonzero default, and disk-offload's spill-sender presence ORs into the same `evict_active` gate, either of which would mask whether `CONFIG SET maxmemory` specifically published the atomic). Drives a 3000-command cross-shard pipeline before CONFIG SET (all OK), after `CONFIG SET maxmemory 2097152` (must mostly OOM), and after `CONFIG SET maxmemory 0` (all OK again). Confirmed genuinely RED before the fix: manually reverting the CONFIG SET call site's publish call (keeping the startup publish and `runtime_config.maxmemory` assignment intact) reproduced 775/3000 OOM — exactly the local-only share a connection's own shard would still gate via the independent `batch_eviction_active` check in handler_monoio, with the cross-shard SPSC legs bypassing entirely. GREEN after restoring the call: 3000/3000. Verification: cargo fmt (no changes needed), cargo clippy -- -D warnings (default features, clean), CARGO_TARGET_DIR=target-tokio cargo clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings (clean), cargo test --lib (3732 passed, 1 ignored), CARGO_TARGET_DIR=target-tokio cargo test --no-default-features --features runtime-tokio,jemalloc --lib (3075 passed, 1 ignored), cargo test --test oom_bypass_closure on both a monoio release-fast binary and a tokio release-fast binary (6/6 passed on each). author: Tin Dang
`FUNCTION LOAD` (src/scripting/functions.rs) creates its own per-library
sandboxed Lua VM, separate from the shard's shared EVAL/EVALSHA VM. That
per-library VM registered redis.call/redis.pcall with
`LuaEvictionCtx::disabled()` unconditionally — a Lua FUNCTION doing
`redis.call('SET', ...)` in a loop bypassed `--maxmemory` entirely,
independent of the EVAL/EVALSHA fix that already covers the shared VM.
`FunctionRegistry::new` now takes a `LuaEvictionCtx`, stores it, and
`create_library` clones it into every library's freshly-created VM (each
library gets its own VM, so the ctx must be cloned per VM, not wired once).
Both production construction sites now build a real ctx from the same
shard handles `setup_lua_vm` already uses for the shared VM
(src/shard/conn_accept.rs's recipe):
- src/server/conn/handler_monoio/mod.rs:172
- src/server/conn/handler_sharded/mod.rs:281 (tokio runtime)
Both read `ctx.{shard_databases, runtime_config, shard_id, spill_sender,
spill_file_id, disk_offload_dir}` off the shard's `ConnectionContext` —
already present, no new handle needed. `core.rs`'s `#[allow(dead_code)]` on
those three fields is removed since both handlers now read them (previously
only handler_monoio's tiered-storage eviction path did under some builds).
The gate itself was already correct once wired (FCALL brackets execution
with set_script_db/clear_script_db, gates FCALL_RO via SCRIPT_READ_ONLY);
after the Gap C atomic-snapshot commit the common case (`maxmemory` unset)
is one Relaxed load, so this adds no measurable per-call cost to the FCALL
hot path.
Test-only call sites (functions.rs's own unit tests) pass
`LuaEvictionCtx::disabled()` — the module doc-comment on `disabled()`
already scopes it to unit tests plus real production ctx builders, updated
in the prior Gap C commit.
Red/green TDD:
- New cases in tests/oom_bypass_closure.rs:
- Case G: FUNCTION LOAD a library whose function loops
`redis.call('SET', ...)` 2000 times (~16MB attempted), FCALL it against
a 2MB-capped noeviction server. RED before the fix: FCALL completed
with `'DONE'` instead of an OOM error (confirmed by temporarily
re-running against the pre-fix registry — the write loop never trips
the gate). GREEN after: FCALL surfaces the OOM error (wrapped as `ERR
Error running script: runtime error: OOM ...` since redis.call
propagates it as a Lua runtime error, uncaught by the function body).
- Case H: control — FCALL with no maxmemory configured must still
succeed (mirrors case D for EVAL), locking the write-only/OOM-only
scope of the gate.
- Lua FUNCTION bodies are called with zero Lua arguments (`call_function`
invokes `registered.call(())`) — they read `KEYS`/`ARGV` as globals, same
as an EVAL script body, not as function parameters. (First draft of these
tests used `function(keys, args)` parameters and failed with "attempt to
index a nil value" — fixed before the gap-specific RED assertion was ever
meaningfully exercised.)
Verification: cargo fmt, cargo clippy -- -D warnings (default features,
clean), CARGO_TARGET_DIR=target-tokio cargo clippy --no-default-features
--features runtime-tokio,jemalloc -- -D warnings (clean), cargo test --lib
(3732 passed, 1 ignored), CARGO_TARGET_DIR=target-tokio cargo test
--no-default-features --features runtime-tokio,jemalloc --lib (3075 passed,
1 ignored), cargo test --test oom_bypass_closure on both a monoio
release-fast binary and a tokio release-fast binary (8/8 passed on each).
author: Tin Dang
`MOVE` and `COPY ... DB n` each need two `&mut Database` borrows at once
(source + destination) — something the generic per-db write path
(`cmd_dispatch` / `key_extra::copy`) cannot provide, since it only ever sees
one `&mut Database`. The plain `Execute` arm in `src/shard/spsc_handler.rs`
special-cased both commands ahead of the generic path, but the other five
`ShardMessage` arms (`MultiExecute`, `PipelineBatch`, `ExecuteSlotted`,
`MultiExecuteSlotted`, `PipelineBatchSlotted`) had no such intercept —
including `PipelineBatchSlotted`, the one arm real client traffic
(`handler_monoio`/`handler_sharded`'s pipelined remote-write path) actually
uses. A remote `COPY src dst DB n` fell through to the generic single-db
`key_extra::copy`, which parses and silently ignores the `DB` clause: the
copy landed in the SAME db as the source (data corruption — confirmed
pre-fix via manual probe, 4/20 remote COPYs landed in the wrong db). A
remote `MOVE` returned a loud-but-wrong "MOVE requires handler-level
dispatch (cross-db operation)" error instead of moving the key.
The intercept is extracted verbatim out of the `Execute` arm into a new
shared helper, `src/shard/spsc_two_db::try_two_db_intercept`, and every arm
now calls it before falling through to the generic write path:
- `Some(response)` means MOVE, or COPY with a DB clause targeting a
different db — the caller treats it as the command's final response (no
generic dispatch, no COW intercept, no auto-index hooks — mirrors the
Execute arm's pre-existing behavior).
- `None` means anything else (not MOVE/COPY, or a same-db COPY with no DB
clause) — falls through to the generic single-db path as before.
Persistence (WAL/AOF) stays in each arm's own per-command block, gated on
`matches!(response, Frame::Integer(1))` — stricter than the batch arms'
generic `!Error` condition, since a same-db MOVE or a COPY of a missing key
returns `Integer(0)` and must not persist.
Borrow discipline: the batch arms (`MultiExecute`, `PipelineBatch`, and
their `*Slotted` variants) previously took one `&mut Database` borrow for
the whole loop. The intercept needs `&mut [Database]` (both src and dst) for
just the MOVE/COPY command, so the per-db borrow is now taken per-iteration,
inside the loop, after the intercept check — the one-time
`refresh_now_from_cache` call and any other per-batch setup that doesn't
need the borrow stays outside.
Cross-db COPY duplicates the value (unlike net-zero MOVE), so the intercept
still runs `spsc_eviction_gate` against the destination db before
`copy_core`, using the same per-shard elastic budget as any other write.
Two things found while wiring this that are explicitly OUT of scope, with
CHANGELOG "found, not fixed" notes:
1. Cross-shard COPY's destination key is only reliably `GET`-able when it
hash-tag-colocates with the source key. Moon shards purely by key hash
with no db-awareness; COPY executes on whichever shard owns the SOURCE
key (the routing key for the whole command) and writes the destination
into that SAME shard's db space — it does not relocate the write to
wherever the destination key's own hash would pick. A later plain `GET
dst` routes by `hash(dst)`, so it only reliably finds the value when src
and dst share a hash tag (or `--shards 1`). MOVE is exempt (same key
name, so `hash(key)` is unchanged). This is a pre-existing property of
the per-key-hash sharding model applied to a renamed-destination COPY —
the Execute arm this fix mirrors has the identical property, not a
regression introduced here. Fixing it needs a real cross-shard two-phase
protocol, well beyond "apply the intercept to more arms."
2. Moon's eviction gate is per-DATABASE, not per-shard-aggregate-across-dbs:
`try_evict_if_needed_budget` measures the ONE db being written to, not
the sum across all 16 logical dbs a shard holds. This is uniform across
every SPSC write arm (see `spsc_handler.rs:619`'s pre-existing single-db
gate call for a normal write) — not something this fix introduces — but
it did surface a stale test premise (see below).
Red/green TDD:
- New suite `tests/spsc_two_db.rs` (3 cases, both runtimes): pipelined
cross-shard `COPY ... DB n`, pipelined cross-shard `MOVE`, and a same-db
`COPY` control (routes through the coordinator's multi-key scatter path,
unaffected by this fix — kept as a negative control). RED confirmed via a
git-stash of just the tracked source changes (spsc_two_db.rs is new/
untracked, so stashing spsc_handler.rs + mod.rs alone reverts to the
pre-fix state): MOVE hit the loud wrong error, COPY silently same-db
copied. GREEN after restoring the stash.
The COPY case uses `{i}`-hash-tagged src/dst key pairs (see out-of-scope
finding 1) — an earlier untagged draft of this test only passed for the
~1-in-4 keys where `hash(dst)` happened to collide with `hash(src)`.
- Existing `tests/oom_bypass_closure.rs` case E (cross-db COPY under memory
pressure) initially regressed to 0/300 OOM against this fix — traced to
out-of-scope finding 2 above: case E's pre-fix pass relied on the
same-db-copy bug landing writes in db 0 (already loaded from phase 1),
tripping the per-db gate as a side effect of the routing bug, not because
the destination db was under its own pressure. Fixed by adding a ballast
phase that pre-loads db 1 with the same keys/sizes as db 0 before the OOM
phase, so the destination-db gate has something real to trip on
(76/300 OOM fully fixed). Verified this isolates the destination-db gate
specifically: temporarily disabling only the `spsc_eviction_gate` call
inside `try_two_db_intercept` (keeping the destination-db routing intact)
reproduces 0/300 OOM — confirmed RED for the right reason, then restored.
Verification: cargo fmt, cargo clippy -- -D warnings (default features,
clean), CARGO_TARGET_DIR=target-tokio cargo clippy --no-default-features
--features runtime-tokio,jemalloc -- -D warnings (clean), cargo test --lib
(3732 passed, 1 ignored), CARGO_TARGET_DIR=target-tokio cargo test
--no-default-features --features runtime-tokio,jemalloc --lib (3075 passed,
1 ignored), cargo test --test oom_bypass_closure (8/8) and cargo test
--test spsc_two_db (3/3) on both a monoio release-fast binary and a tokio
release-fast binary.
author: Tin Dang
Self-review after the Gap A commit (5278adb) caught a false statement in `tests/spsc_two_db.rs`'s module doc: it claimed the dead/admin-only SPSC arms (`MultiExecute`, `ExecuteSlotted`, `MultiExecuteSlotted`) "are covered by direct unit tests of the shared `try_two_db_intercept` helper" — no such unit tests exist; `src/shard/spsc_two_db.rs` has no `#[cfg(test)]` module. The wire-level cases in this suite do exercise `PipelineBatchSlotted`, which calls `try_two_db_intercept` verbatim (the same helper every other arm calls), so that coverage transitively validates the shared logic — but it does not independently exercise the dead arms' own call sites, and no direct unit test does either. Rewrote the doc comment to state only what is actually true, and to flag the missing direct unit test as a documented gap rather than implying it's covered. No production code change; test-only doc correction. author: Tin Dang
Contributor
Author
|
Pushed 4 more commits closing all three items previously listed under Known gaps (now fixed, not gaps):
Two pre-existing properties found during this work are documented in CHANGELOG as known follow-ups (not regressions): cross-shard Independently re-verified before push: both integration suites green on monoio AND tokio feature sets, lib suite 3732 green, fmt/clippy clean on both matrices, unsafe/unwrap audits pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/spsc_two_db.rs`:
- Around line 493-519: The same-db COPY control test uses untagged keys, so the
destination GET can route to a different shard than the COPY wrote to. Update
the copy/retrieval keys in the spsc_two_db test to use the same hash-tagged
pattern as Case 1, and keep the COPY and GET assertions aligned around the
tagged key pair so `key_extra::copy` is exercised consistently.
🪄 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: 0db3072b-e499-4813-9694-fdb4111116d0
📒 Files selected for processing (14)
CHANGELOG.mdsrc/command/config.rssrc/main.rssrc/scripting/bridge.rssrc/scripting/functions.rssrc/server/conn/core.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_sharded/mod.rssrc/shard/mod.rssrc/shard/spsc_handler.rssrc/shard/spsc_two_db.rssrc/storage/eviction.rstests/oom_bypass_closure.rstests/spsc_two_db.rs
✅ Files skipped from review due to trivial changes (2)
- src/server/conn/core.rs
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shard/spsc_handler.rs
Comment on lines
+493
to
+519
| let copy_cmds: Vec<Vec<Vec<u8>>> = (0..N) | ||
| .map(|i| { | ||
| vec![ | ||
| b"COPY".to_vec(), | ||
| format!("sd:{i}").into_bytes(), | ||
| format!("sd:{i}:copy").into_bytes(), | ||
| ] | ||
| }) | ||
| .collect(); | ||
| let copy_replies = c.pipeline(©_cmds); | ||
| for (i, r) in copy_replies.iter().enumerate() { | ||
| assert_eq!( | ||
| *r, | ||
| V::Int(1), | ||
| "same-db COPY sd:{i} -> sd:{i}:copy did not report success: {r:?}" | ||
| ); | ||
| } | ||
|
|
||
| for i in 0..N { | ||
| let dst = format!("sd:{i}:copy"); | ||
| let got = c.cmd(&[b"GET", dst.as_bytes()]); | ||
| assert_eq!( | ||
| got, | ||
| V::Bulk(values[i].clone()), | ||
| "same-db COPY control regressed: sd:{i}:copy missing/wrong value" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how same-db COPY (no DB clause) resolves destination shard on the
# PipelineBatchSlotted arm vs the cross-db intercept, to confirm whether
# non-colocated dst is written on the source shard (Case 3 flakiness).
fd -t f 'spsc_two_db.rs' src
rg -nP -C4 'fn try_two_db_intercept' src/shard/spsc_two_db.rs
rg -nP -C3 'key_extra::copy|fn copy\b' src/command
rg -nP -C4 'PipelineBatchSlotted' src/shard/spsc_handler.rsRepository: pilotspace/moon
Length of output: 5274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the same-db COPY test and the COPY implementation/dispatch paths.
sed -n '440,530p' tests/spsc_two_db.rs
printf '\n--- key_extra::copy ---\n'
sed -n '1,220p' src/command/key_extra.rs
printf '\n--- move_cmd COPY DB parsing ---\n'
sed -n '140,240p' src/command/keyspace/move_cmd.rs
printf '\n--- shard handler PipelineBatchSlotted branch ---\n'
sed -n '1540,1605p' src/shard/spsc_handler.rsRepository: pilotspace/moon
Length of output: 17235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' src/shard/spsc_two_db.rs
printf '\n---\n'
sed -n '240,420p' src/shard/spsc_two_db.rsRepository: pilotspace/moon
Length of output: 5853
Hash-tag the same-db COPY control keys
COPY without DB falls through to key_extra::copy, which writes dst into the source shard’s DB; the later GET sd:{i}:copy still routes by the destination key and can miss when the pair isn’t colocated. Use the same tagged-key pattern as Case 1.
🤖 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 `@tests/spsc_two_db.rs` around lines 493 - 519, The same-db COPY control test
uses untagged keys, so the destination GET can route to a different shard than
the COPY wrote to. Update the copy/retrieval keys in the spsc_two_db test to use
the same hash-tagged pattern as Case 1, and keep the COPY and GET assertions
aligned around the tagged key pair so `key_extra::copy` is exercised
consistently.
TinDang97
added a commit
that referenced
this pull request
Jul 6, 2026
GAP-1's elastic budget redistributes per-shard caps on a 100ms tick, so case E's cross-shard COPY OOM share is timing-sensitive: 76/300 locally but 28/300 on a slow GitHub Actions runner, which flaked the N/10 (30) threshold. The gate-disabled RED run is exactly 0/300, so any clearly nonzero threshold discriminates; N/30 (10) keeps margin both ways. Rides with the deps PR because the flake blocks the merge train; the test landed on main in PR #217. author: Tin Dang
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 6, 2026
…PRs) (#219) * chore(deps): consolidated dependency bumps superseding 12 dependabot PRs Cargo: ringbuf 0.4.8->0.5.0, metrics-exporter-prometheus 0.16.2->0.18.3 (both semver-major, compile+test green with zero code changes), bytes 1.11.1->1.12.0, rand 0.10.1->0.10.2, lz4_flex 0.13.0->0.13.1, cmov 0.5.3->0.5.4, cudarc 0.19.4->0.19.8, openssl 0.10.78->0.10.81 (sdk/rust, security). Actions: checkout v4->v7 (incl. docker-publish.yml straggler dependabot missed), deploy-pages v4->v5. Console: vite 7.3.2->7.3.5. Every dependabot PR failed only the Lint CHANGELOG gate (dependabot can't satisfy it); this consolidated PR carries one entry for all of them. Supersedes #191 #194 #196 #197 #198 #199 #200 #201 #208 #209 #210. dependabot.yml now ignores dtolnay/rust-toolchain: that action pin IS the MSRV (1.94) CI gate, and dependabot bumping it to latest stable (#195, closed) silently defeats the MSRV check. author: Tin Dang * test(oom): deflake case E threshold N/10 -> N/30 for CI timing variance GAP-1's elastic budget redistributes per-shard caps on a 100ms tick, so case E's cross-shard COPY OOM share is timing-sensitive: 76/300 locally but 28/300 on a slow GitHub Actions runner, which flaked the N/10 (30) threshold. The gate-disabled RED run is exactly 0/300, so any clearly nonzero threshold discriminates; N/30 (10) keeps margin both ways. Rides with the deps PR because the flake blocks the merge train; the test landed on main in PR #217. author: Tin Dang --------- Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
TinDang97
added a commit
that referenced
this pull request
Jul 6, 2026
…for quickwins_red_api Main's post-merge CI runs for PR #217/#218/#219 are all red on two issues this branch's PR (#228) also inherits; both are test-side. 1. test_case_e_cross_db_copy_oom: 0/300 OOM on BOTH CI platforms while passing locally (76/300; earlier deflake N/10 -> N/30 saw 28/300). The single-shot statistical assert races GAP-1's elastic budget: one round of 300x4KB COPYs (~300KB/shard) fits inside the slack the elastic budget plus its 100ms-stale usage snapshots can grant, so the OOM share is runner-speed-dependent all the way down to zero. Rewritten as bounded escalation: up to 8 rounds of COPYs into fresh destination keys, stopping early once OOMs appear. The destination db's per-shard usage (~2.4MB after all rounds) provably exceeds the ABSOLUTE per-shard budget ceiling (compute_elastic_budget returns at most base + surplus <= maxmemory = 2MB, and the gate compares db.estimated_memory() against it under noeviction), so gate-checked COPYs MUST OOM on any runner speed. The RED floor stays exact and was re-verified: with the Gap A gate block neutered, 0 OOM through all 8 rounds (2400 COPYs) — the assert still discriminates. 2. tests/quickwins_red_api.rs broke the Windows CI leg at COMPILE time: qw1_accepted_socket_has_nodelay calls moon::server::socket_opts::apply_client_socket_opts, which is #[cfg(unix)] (takes AsFd). The test and its TcpListener/TcpStream imports are now unix-gated to match the helper. Validation: oom_bypass_closure 8/8 green on monoio AND tokio+jemalloc (MOON_NO_URING=1) locally; case E red/green toggled via the gate block. author: Tin Dang
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 6, 2026
…fix pre-existing kill-9 doc loss (#228) * feat(vector): add BucketedKeyMap — bucketed CoW keymap (256 buckets) VectorIndex's key_hash -> V maps were single Arc<HashMap<u64, V>> instances. Under ft-search-off-eventloop, a live FT.SEARCH snapshot holds an extra Arc clone of the whole map while the event loop keeps processing writes; the next write's Arc::make_mut then sees refcount > 1 and full-clones the entire map synchronously on the shard event loop (~47MB @1m vectors). BucketedKeyMap<V> shards the map into 256 fixed buckets, each an independently-cloneable Arc<HashMap<u64, V>>, with bucket selected by (key_hash >> 56). A concurrent snapshot now pins at most 1/256th of the map per write instead of the whole thing. Manual Clone impl avoids a spurious V: Clone bound that #[derive(Clone)] would add. 8 unit tests: get/insert/remove/len round-trip against a reference HashMap under thousands of random ops, snapshot isolation, and the key property test — after snap = map.snapshot(), one insert leaves >= 255 buckets Arc::ptr_eq with the snapshot. Not yet wired into VectorIndex; that follows in the next commit. author: Tin Dang * fix(vector): bucket-scoped CoW for keymap fields Closes the Arc<HashMap> full-clone amplification under live search snapshots (see previous commit for BucketedKeyMap rationale). Converts VectorIndex's three key_hash -> V maps from Arc<HashMap<u64, V>> to BucketedKeyMap<V>: key_hash_to_key: Arc<HashMap<u64, Bytes>> -> BucketedKeyMap<Bytes> key_hash_to_global_id: Arc<HashMap<u64, u32>> -> BucketedKeyMap<u32> key_hash_to_vec_checksum: Arc<HashMap<u64, u64>> -> BucketedKeyMap<u64> and SearchSnapshot.key_hash_to_key to match. All call sites that used Arc::make_mut(&mut idx.key_hash_to_*).insert/remove(...) now call .insert/.remove(...) directly on the BucketedKeyMap (the bucket-scoped CoW happens inside insert()); two SPSC insert paths use the new get_or_insert_with() helper in place of the former .entry(...).or_insert_with(...) pattern. Read paths (FT.SEARCH response building, session filtering, hybrid search, recovery) take &BucketedKeyMap<Bytes> instead of &HashMap; these are read-only signature changes with no behavior change since BucketedKeyMap mirrors HashMap's get/iter/len/is_empty API. On-disk keymap.bin format is unchanged: run_snapshot_job's persistence loop only calls .iter()/.get(), and recover_v2.rs rebuilds the map from the same on-disk entries via a plain insert loop. Touches: src/vector/store.rs, src/vector/segment/holder.rs, src/command/vector_search/{ft_search/execute,ft_search/response, session,hybrid,hybrid_multi,tests}.rs, src/shard/spsc_handler.rs, src/shard/scatter_hybrid.rs, src/vector/persistence/recover_v2.rs. author: Tin Dang * perf(vector): coalescing SnapshotPool — bound the durability write queue SnapshotPool used an unbounded flume::unbounded::<SnapshotJob>() queue drained by a single worker. If compact/merge cadence outpaced the worker, queued jobs pinned every submitter's Arcs (now BucketedKeyMap buckets) indefinitely, growing unboundedly under sustained load. Replaced the channel with a coalescing slot: Mutex<HashMap<PathBuf, SnapshotJob>> + Condvar, keyed by the job's idx_dir (already unique per index and present on every SnapshotJob, so no new key type is needed). submit() inserts/overwrites the pending entry for that index and notifies one worker; a resubmit for an index whose prior job is still pending (not yet dequeued) replaces it in place, dropping the stale job's data immediately, so only the newest state per index is ever persisted. A job already dequeued and running is unaffected by a concurrent resubmit — the worker loop only removes an entry from the map at the moment it starts running that job. submit() never blocks the caller (map insert + notify, no waiting on capacity). SnapshotJob's three fields are now BucketedKeyMap<Bytes/u32/u64> (no Arc wrapper) to match VectorIndex's field types from the previous commit. run_snapshot_job's body is unchanged since it only calls .iter()/.get() on the map. 3 new tests: same-index submits under a slow worker execute far fewer than the number submitted (final persisted state is the newest job); jobs for distinct indexes all execute; an in-flight job survives a resubmit for the same index (the running job completes normally, the resubmit becomes the new pending entry). Also fixes 2 doc_lazy_continuation clippy lints in the new SnapshotPool doc comment. author: Tin Dang * docs(changelog): vector keymap CoW amplification + bounded snapshot queue fix Records the RSS/CPU wave 4 defects fixed in the previous three commits under [Unreleased]: BucketedKeyMap replacing the single Arc<HashMap> keymap fields, and the coalescing SnapshotPool replacing the unbounded flume queue. author: Tin Dang * fix(vector): kill-9 doc loss — FT.COMPACT residue drain + B3 segment-membership gate Two pre-existing, compounding bugs (both reproduce on main; surfaced while validating this branch against crash_recovery_vector_durability — the "wave-4 regression" was actually machine-load timing): 1. force_compact early-return left mutable residue unfrozen. When an explicit FT.COMPACT found a background auto-compaction in flight, it drained that build, installed it, persisted, and RETURNED — without compacting the docs inserted while the background build ran (the clone_suffix(frozen_len) mutable tail). Those docs stayed mutable-only with no durable segment until some future compact, breaking force_compact's full-drain contract (frozen == mutable on reply). Observed stuck durable state: segments live=1961, keymap entries=2000, converging never. force_compact now falls through to the inline drain loop after installing the in-flight build (a no-op when the tail is empty). 2. B3 recovery "verified" keymap entries with no backing segment doc. The durable keymap-<epoch>.bin covers EVERY indexed key (mutable + immutable) at snapshot-submit time, while the paired manifest.json lists only installed immutable segments — so after a kill -9 the on-disk keymap can be a strict SUPERSET of the on-disk segments (bug 1 makes that state persistent; even with bug 1 fixed, a crash between a segment install and its async snapshot commit still produces it transiently). The B3 dedup rescan treated a keymap checksum match as "verified unchanged" and never re-indexed those keys: their documents silently vanished from search (post-crash num_docs 1950/2000, "2000 key(s) verified unchanged, 0 re-indexed", wrong KNN top-1 — decoded from a preserved failing run's artifacts). load_segments_and_keymap now builds the union of live key_hashes across loaded segments (new ImmutableSegment::live_key_hashes, honoring install-time delete_lsn and steady-state tombstones) and drops any keymap entry not in it, so the rescan re-indexes those keys from the AOF — never wrong, only occasionally slower. A loud info line reports the dropped-phantom count. Tests (each red/green verified by toggling its fix): - unit: test_force_compact_drains_tail_inserted_during_inflight_bg_build stages bug 1 deterministically (in-flight bg build + 30 tail inserts, then force_compact; red: 30 docs left mutable, green: 0 and all 130 segment-resident). - unit: recover_reindexes_keymap_entries_not_backed_by_any_segment stages the window deterministically (rewrites the durable keymap with a checksum-matching phantom entry) and asserts the phantom is dropped at load, re-indexed on rescan, and searchable end-to-end. - integration S6 (crash_recovery_vector_durability): kills inside the async-snapshot window (manifest >= 2 segments gate, no full-durability wait) and asserts only the crash contract: num_docs == N and every key answers its own exact-match query. - S1/S2 flake fix: their re_indexed == 0 assertion silently assumed full durability at kill time; a new wait_for_durable_docs pre-kill gate (manifest'd segment live-counts AND keymap entries both == N) makes the precondition explicit — it is what exposed bug 1's never-converging state. Server stdout/stderr logs now open in append mode so the pre-crash generation's log survives restart for post-mortem diagnosis. author: Tin Dang * test(ci): deterministic OOM case E escalation + Windows compile gate for quickwins_red_api Main's post-merge CI runs for PR #217/#218/#219 are all red on two issues this branch's PR (#228) also inherits; both are test-side. 1. test_case_e_cross_db_copy_oom: 0/300 OOM on BOTH CI platforms while passing locally (76/300; earlier deflake N/10 -> N/30 saw 28/300). The single-shot statistical assert races GAP-1's elastic budget: one round of 300x4KB COPYs (~300KB/shard) fits inside the slack the elastic budget plus its 100ms-stale usage snapshots can grant, so the OOM share is runner-speed-dependent all the way down to zero. Rewritten as bounded escalation: up to 8 rounds of COPYs into fresh destination keys, stopping early once OOMs appear. The destination db's per-shard usage (~2.4MB after all rounds) provably exceeds the ABSOLUTE per-shard budget ceiling (compute_elastic_budget returns at most base + surplus <= maxmemory = 2MB, and the gate compares db.estimated_memory() against it under noeviction), so gate-checked COPYs MUST OOM on any runner speed. The RED floor stays exact and was re-verified: with the Gap A gate block neutered, 0 OOM through all 8 rounds (2400 COPYs) — the assert still discriminates. 2. tests/quickwins_red_api.rs broke the Windows CI leg at COMPILE time: qw1_accepted_socket_has_nodelay calls moon::server::socket_opts::apply_client_socket_opts, which is #[cfg(unix)] (takes AsFd). The test and its TcpListener/TcpStream imports are now unix-gated to match the helper. Validation: oom_bypass_closure 8/8 green on monoio AND tokio+jemalloc (MOON_NO_URING=1) locally; case E red/green toggled via the gate block. author: Tin Dang * test(ci): merge racing RECL_SEGMENT_STALL_ACTIVE tests into one test_recl_segment_stall_active_atomic_exists and test_is_segment_stall_active_helper both store/load the PROCESS-GLOBAL RECL_SEGMENT_STALL_ACTIVE atomic while the test harness runs them on parallel threads within the same binary — the atomic-exists test's `store(1); load()` observed the helper test's final `store(0)` interleaving (CI macOS: `left: 0, right: 1` at the read-back assert; PR #228 round-2 Check (macOS) leg). Pre-existing since MA1 (wave 1); purely a test-parallelism race, not a stall-gate defect. All mutation of the global now lives in ONE merged test (test_recl_segment_stall_active_atomic_and_helper) so there is nothing left to race with; the default-0 assert is also only valid under that single-writer arrangement. Ran 5x locally green. author: Tin Dang * docs(changelog): note the stall-atomic test-race merge in the CI-fix entry author: Tin Dang --------- Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
This was referenced Jul 8, 2026
Merged
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.
OOM shield — wave 2 of the RSS/CPU/OOM hardening review
Second PR from the
tmp/RSS-CPU-REVIEW.mdranked findings (wave 1 = #216). Closes the two remaining paths where writes could grow memory past--maxmemorywithout any eviction/OOM check, and makes the guardrail work on macOS at all.1. macOS memory guardrail (
src/config.rs)detect_memory_limit_bytes()previously returnedNoneon macOS, so the 80% auto-cap + allkeys-lru guardrail never engaged. Now probessysctl -n hw.memsizevia a spawnedCommand(no newunsafe). RED/GREEN testmacos_detects_memory_limit.2. Eviction bypass closure (
src/shard/spsc_handler.rs,src/scripting/)Two write paths ran zero eviction gating:
spsc_eviction_gatenow mirrorsrun_write_eviction_gate(elastic budget + spill-vs-plain branching) on all six arms. Cost whenmaxmemoryis unset: oneevict_activesnapshot per drain cycle (amortized over up to 256 messages, same granularity as the handlers'batch_eviction_active), zero per-message.redis.call/redis.pcallwrites: EVAL/EVALSHA carry no WRITE command flag, so a script couldSETin a loop unbounded.LuaEvictionCtxis captured into the redis.call/pcall closures at VM setup (per-shard, zero per-call alloc); the gate decides "is eviction active?" from a per-script-execution snapshot (thread-local generation counter +Cellcache), so the common maxmemory-unset case pays oneCellread per write call and reads theRuntimeConfiglock at most once per script run.Per-command OOM semantics match Redis: in pipelines/MULTI the failed command gets the OOM error, the batch continues, replies stay 1:1 aligned; a gated-out write never touches the database (no partial state).
Tests
tests/oom_bypass_closure.rs— 5 cases (direct writes, cross-shard pipeline flood, MULTI, EVAL loop, cross-shard COPY regression), green on monoio and tokio feature sets. Case B threshold is set to N/2: GAP-1's elastic budget redistributes on a 100ms tick so OK-counts are timing-sensitive; N/2 still discriminates the pre-fix ceiling (~N/4 OOMs) from the post-fix floor (~1900+/3000 observed).Review
Implementation deep-reviewed per process: manual arm-by-arm review + independent test reruns (caught the case-B flake) + a perf-reviewer pass. Reviewer verified: lock discipline (parking_lot only, nothing held across
.await), reply-always-sent on all six gated arms, no partial MULTI mutation, migration rebuilds the ctx from the destination shard's handles,spill_file_idshared correctly across all three write paths. Its MEDIUM finding (Lua gate took the lock per write call, contradicting the doc comment) is fixed in the final commit.Follow-up round: all former "Known gaps" are now FIXED in this PR
FunctionRegistrycarries a realLuaEvictionCtxat both handler construction sites (disabled()survives only in unit tests).COPY src dst DB n/MOVEnow work on every SPSC arm via a shared two-database intercept (src/shard/spsc_two_db.rs); the silent wrong-db COPY data bug is gone. Persistence strictly gated onInteger(1). New RED-verified suitetests/spsc_two_db.rs.storage::eviction::{publish_maxmemory, maxmemory_is_set}), published at startup andCONFIG SET maxmemory(test case F proves the CONFIG SET path).Pre-existing properties found during this work (documented in CHANGELOG, not regressions)
COPY ... DB nmaterializes the destination on the source key's shard — the copy is only readable viaGET dstwhen src/dst share a hash tag. Affects the originalExecutearm equally.db.estimated_memory()), not shard-aggregate across all 16 dbs.RSS watchdog (review item 6) lands in the next PR of this train (wave 3, already implemented and review-approved, pending this merge).
Summary by CodeRabbit
Bug Fixes
--maxmemoryenforcement across remote/cross-shard write paths, including Luaredis.call/redis.pcallandFUNCTION-internal writes.MOVEandCOPY ... DB noperations.maxmemory-enabled servers don’t bypass enforcement.Tests
MOVE/COPYbehavior.