feat(shard): proactive RSS watchdog pauses writes before kernel OOM (RSS/CPU wave 3)#218
Merged
Merged
Conversation
Review finding #6 from tmp/RSS-CPU-REVIEW.md (wave 3 of the RSS/CPU/OOM train). Moon has a diskfull guard (`MOONERR diskfull` at <5% free) but no memory analogue — when --maxmemory is unset (or too high), RSS can grow until the kernel OOM-killer picks Moon. This adds mem_monitor, mirroring disk_monitor's struct + hysteresis state machine + inject() test backdoor + OnceLock global singleton pattern exactly, inverted for direction (high RSS is bad, not low free space): writes pause once rss% >= mem_full_pct and resume only once rss% <= mem_full_pct - 5 (hysteresis prevents flapping). Fires on ACTUAL RSS (via the existing zero-alloc get_rss_bytes()) vs the DETECTED system/cgroup memory limit (detect_memory_limit_bytes, now pub(crate) so mem_monitor can call it), not the configured --maxmemory (which can be an unconfigured 0). limit_bytes is resolved ONCE at init_global time and held fixed — unlike disk_monitor's per-poll statvfs, the memory-limit probe can shell out on macOS and must never run on the hot poll path. A hidden MOON_MEM_LIMIT_BYTES env override (documented in the module header, same rationale as the MOON_XSHARD_* bench-diagnostic knobs) lets tests engage the guard deterministically without controlling real process RSS. New --mem-full-pct CLI flag (default 95, 0 = disabled) alongside --disk-free-min-pct in config.rs. New INFO fields reclamation_mem_rss_bytes / reclamation_mem_watchdog_active in info_reclamation.rs, OR'd into the existing reclamation_write_stall_active aggregate. Red/green: 15 mem_monitor unit tests (threshold, INVERTED exact-threshold pause, hysteresis resume/flap, disabled via pause_pct=0 or limit_bytes=0, u8-wrap clamp on rss >> limit, initial-optimistic, Send+Sync, real-process smoke) + 1 new info_reclamation wiring test (default false, OR-merge into write_stall_active). author: Tin Dang
Wires the mem_monitor core (previous commit) into the live write path: - segment_stall::is_any_write_stall_active() OR-merges a third source, mem_monitor::is_write_paused(), alongside the existing MA12 disk-pressure and MA1 segment-backlog bits. Hot-path cost: one additional AtomicBool::load(Relaxed) inside the already write-gated branch — no allocation, no lock. - Both dispatch paths' try_enforce_disk_full (handler_monoio and handler_sharded) gain a MOONERR memfull branch in the message-selection chain: diskfull first, then memfull, then the segment-backlog busy fallback — mirrors the existing diskfull/busy distinction verbatim. - event_loop.rs: mem_monitor::poll_global() runs on shard 0's existing 5s disk_monitor timer tick, at BOTH poll sites (tokio select! arm and the monoio tick-counter branch) — no new timer, reuses MA12's cadence. - main.rs: mem_monitor::init_global(config.mem_full_pct) called right after disk_monitor::init_global, the single call site for both guards. Runs one immediate poll inside init_global (see previous commit) so the AOF-replay /segment-load startup recovery peak is visible before the first 5s tick. author: Tin Dang
…xups Adds tests/mem_watchdog.rs, the wire-level counterpart to mem_monitor's unit tests: proves the --mem-full-pct CLI flag, init_global's immediate poll, and the dispatch-level MOONERR memfull message path against a real spawned server (pattern: tests/oom_bypass_closure.rs). Real process RSS and the detected memory limit aren't controllable from an external test process, so the suite drives the guard via the MOON_MEM_LIMIT_BYTES test override documented in mem_monitor's module header: - Case A: MOON_MEM_LIMIT_BYTES=1MB + --mem-full-pct 50 -> the FIRST SET is already refused with MOONERR memfull (init_global's immediate poll, no need to wait for the 5s tick). - Case B: GET is never blocked while the guard is engaged (read-only commands are exempt via the existing is_write gate). - Case C: control — no env override, SET succeeds normally (negative control against spurious engagement). - Case D: --mem-full-pct 0 disables the guard even with a tiny forced limit that would otherwise engage it at any nonzero threshold. RED confirmed by running this suite against a binary built BEFORE the mem_monitor implementation (git stash of all impl commits, rebuild, all 4 cases fail with "server never accepted" because --mem-full-pct doesn't parse yet); GREEN confirmed by unstashing and rebuilding — all 4 pass on both the default (monoio) and runtime-tokio,jemalloc feature sets. Also fixes 3 pre-existing test files that construct ServerConfig via an EXHAUSTIVE struct literal (no ..Default::default() spread), which the new mem_full_pct field broke at compile time: tests/mq_integration.rs, tests/txn_kv_wiring.rs, tests/workspace_integration.rs (2 occurrences). Each gets mem_full_pct: 0 (disabled) next to its existing disk_free_min_pct: 5 line — these tests don't exercise the RSS watchdog and 0 preserves their prior (guard-absent) behavior exactly. author: Tin Dang
Adds the [Unreleased] entry for the mem_monitor RSS watchdog (wave 3 of the RSS/CPU/OOM train) — summarizes the guard's semantics, the new --mem-full-pct flag, MOONERR memfull, and the new INFO fields, matching the format of the existing diskfull/OOM-bypass entries in this section. author: Tin Dang
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? |
📝 WalkthroughWalkthroughAdds a proactive RSS-based write watchdog (mem_monitor) with hysteresis, gated by a new ChangesMemory Watchdog Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Dispatch
participant SegmentStall
participant MemMonitor
participant EventLoop
EventLoop->>MemMonitor: poll_global() every 5s (shard 0)
MemMonitor->>MemMonitor: sample RSS, update hysteresis state
Client->>Dispatch: write command (e.g. SET)
Dispatch->>SegmentStall: is_any_write_stall_active()
SegmentStall->>MemMonitor: is_write_paused()
MemMonitor-->>SegmentStall: true/false
SegmentStall-->>Dispatch: stall active?
alt memory watchdog active
Dispatch-->>Client: MOONERR memfull
else disk pressure active
Dispatch-->>Client: MOONERR diskfull
else other stall
Dispatch-->>Client: MOONERR busy
else no stall
Dispatch-->>Client: command executed
end
Related issues: None found in the provided context. Related PRs: None found in the provided context. Suggested labels: enhancement, reliability, memory-management Suggested reviewers: None found in the provided context. 🐰 A moon-lit watchdog sniffs at RAM, 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config.rs (1)
1-1066: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit
src/config.rsinto smaller modulessrc/config.rsis already 1,902 lines, past the 1,500-line limit.🤖 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 1 - 1066, The config module is too large and needs to be split into smaller pieces to satisfy the size limit. Move coherent groups from ServerConfig and its helpers (such as resolve_dir, apply_memory_guardrail, default_data_dir, parse_size, and detect_memory_limit_bytes) into focused submodules under config, then re-export or wire them back through src/config.rs so public behavior stays unchanged. Keep ServerConfig as the entry point, but reduce src/config.rs by extracting the persistence, memory-guardrail, and parsing logic into separate modules with clear names.Source: Coding guidelines
🤖 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/command/info_reclamation.rs`:
- Around line 436-462: The `info_reclamation_mem_watchdog_wirable` test mutates
`RECL_MEM_WATCHDOG_ACTIVE` without isolation, so it can race with other
atomic-mutating tests like `info_reclamation_write_stall_default_false` and
cause flaky assertions in `write_reclamation_section`. Add shared serialization
around these tests, either by guarding the body with a common `Mutex` or by
marking the relevant `info_reclamation_*` tests as serial, so the store/restore
pattern runs without parallel interference.
---
Outside diff comments:
In `@src/config.rs`:
- Around line 1-1066: The config module is too large and needs to be split into
smaller pieces to satisfy the size limit. Move coherent groups from ServerConfig
and its helpers (such as resolve_dir, apply_memory_guardrail, default_data_dir,
parse_size, and detect_memory_limit_bytes) into focused submodules under config,
then re-export or wire them back through src/config.rs so public behavior stays
unchanged. Keep ServerConfig as the entry point, but reduce src/config.rs by
extracting the persistence, memory-guardrail, and parsing logic into separate
modules with clear names.
🪄 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: ef3d76df-a23c-4b37-b93f-44a4cb35ad81
📒 Files selected for processing (14)
CHANGELOG.mdsrc/command/info_reclamation.rssrc/config.rssrc/main.rssrc/server/conn/handler_monoio/dispatch.rssrc/server/conn/handler_sharded/dispatch.rssrc/shard/event_loop.rssrc/shard/mem_monitor.rssrc/shard/mod.rssrc/shard/segment_stall.rstests/mem_watchdog.rstests/mq_integration.rstests/txn_kv_wiring.rstests/workspace_integration.rs
Comment on lines
+436
to
+462
| /// Default sentinel for the new Wave-3 `mem_watchdog_active` field must | ||
| /// be `false`, and setting `RECL_MEM_WATCHDOG_ACTIVE` must both flip its | ||
| /// own field AND OR into the aggregate `write_stall_active` — mirrors | ||
| /// `info_reclamation_wal_bytes_wirable`'s store/assert/restore shape. | ||
| #[test] | ||
| fn info_reclamation_mem_watchdog_wirable() { | ||
| let mut buf = String::new(); | ||
| write_reclamation_section(&mut buf); | ||
| assert!( | ||
| buf.contains("reclamation_mem_watchdog_active:false\r\n"), | ||
| "default mem_watchdog_active must be false" | ||
| ); | ||
|
|
||
| RECL_MEM_WATCHDOG_ACTIVE.store(1, Ordering::Relaxed); | ||
| let mut buf2 = String::new(); | ||
| write_reclamation_section(&mut buf2); | ||
| RECL_MEM_WATCHDOG_ACTIVE.store(0, Ordering::Relaxed); // restore default | ||
| assert!( | ||
| buf2.contains("reclamation_mem_watchdog_active:true\r\n"), | ||
| "RECL_MEM_WATCHDOG_ACTIVE store must be visible in its own field" | ||
| ); | ||
| assert!( | ||
| buf2.contains("reclamation_write_stall_active:true\r\n"), | ||
| "RECL_MEM_WATCHDOG_ACTIVE must OR into the aggregate write_stall_active" | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files src/command/info_reclamation.rs
wc -l src/command/info_reclamation.rs
sed -n '1,260p' src/command/info_reclamation.rs
printf '\n---\n'
sed -n '260,520p' src/command/info_reclamation.rs
printf '\n--- search shared atomics/tests ---\n'
rg -n "RECL_MEM_WATCHDOG_ACTIVE|RECL_WRITE_STALL_ACTIVE|write_stall_active|mem_watchdog_active|serial_test|Mutex|test" src/command/info_reclamation.rsRepository: pilotspace/moon
Length of output: 23746
Protect these atomic-mutating tests from parallel execution. info_reclamation_mem_watchdog_wirable mutates RECL_MEM_WATCHDOG_ACTIVE without isolation, so it can race with info_reclamation_write_stall_default_false and make the default reclamation_write_stall_active:false assertion flaky. Wrap the test body in a shared Mutex or mark these tests serial.
🤖 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/command/info_reclamation.rs` around lines 436 - 462, The
`info_reclamation_mem_watchdog_wirable` test mutates `RECL_MEM_WATCHDOG_ACTIVE`
without isolation, so it can race with other atomic-mutating tests like
`info_reclamation_write_stall_default_false` and cause flaky assertions in
`write_reclamation_section`. Add shared serialization around these tests, either
by guarding the body with a common `Mutex` or by marking the relevant
`info_reclamation_*` tests as serial, so the store/restore pattern runs without
parallel interference.
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>
TinDang97
added a commit
that referenced
this pull request
Jul 6, 2026
…SS/CPU wave 5, item C8) `tests/sigterm_shutdown.rs`'s `wait_for_ready` was already poll-based (100ms re-poll against a deadline, matching the pattern used elsewhere in this repo, e.g. `crash_recovery_vector_durability`'s `wait_ready`) — but both call sites hardcoded a fixed 15s deadline independently. PR #218 documented "server did not become ready within 15s" as an observed flake, not a real readiness regression: a contended CI runner or the documented OrbStack virtiofs starvation pattern (CLAUDE.md gotchas) can push a cold server spawn + first-bind past 15s even though the process is healthy. Introduce a single `READY_TIMEOUT` constant (60s) shared by both `wait_for_ready` call sites (the main `assert_sigterm_clean_exit_shards` path and the write-storm test), replacing the two independently-hardcoded 15s literals. This does not slow down the healthy-server case at all -- the poll loop still notices readiness within one 100ms tick of it actually happening; only a genuinely-broken server (or a truly wedged CI host) would ever wait out the full deadline. Panic messages now interpolate the constant instead of hardcoding "15s" so they can't drift out of sync with the actual value again. Left the separate post-SIGTERM exit deadline (10s, `assert_sigterm_clean_exit_shards`'s shutdown-wait loop) untouched -- it's a different flake surface not implicated by the #218 report, and out of scope for this item. Tests (verify-and-harden, not a traditional red/green cycle -- this widens a timeout rather than fixing a logic bug): all 7 sigterm tests re-run to confirm the constant swap didn't change pass/fail behavior in the healthy-host case. Verified: `cargo test --release --test sigterm_shutdown -- --test-threads=1` (7/7 passed). fmt-clean. `cargo clippy -- -D warnings` (the exact default-feature CI gate, no `--tests`) is clean; `--tests` surfaces pre-existing, unrelated warnings across other integration-test files not touched here (config.rs default-then-assign, doc-list indentation in xshard_fastpath_api.rs, etc.) -- confirmed pre-existing, not introduced by this change. author: Tin Dang
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 7, 2026
* test(shard): fix two Windows-only main-CI test failures Main's post-merge Check (Windows) job (runs only on main pushes, so PR CI never exercised it) failed with two lib-test failures at 841f8f2: 1. shard::mem_monitor::tests::test_poll_real_process_smoke asserted rss_bytes() > 0 after a real poll(), but get_rss_bytes() only has real implementations on Linux (/proc/self/statm) and macOS (mach task_info); all other platforms get the documented `0` fallback, so the assertion can never hold on Windows. Gate the test with cfg(any(target_os = "linux", target_os = "macos")). 2. shard::scatter_aggregate::tests:: test_scatter_text_aggregate_single_shard_skips_spsc panicked with "ShardSlice not initialized on this thread". The single-shard fast path calls with_shard, which requires init_shard on the current thread — the test never called it. It passed on Linux/macOS purely by libtest thread-scheduling luck: an earlier test on the same harness thread had left an initialized ShardSlice behind. That is a latent order-dependent flake everywhere, deterministic on Windows. Reproduced locally on macOS by running the test in isolation (fresh thread -> same panic). Fix: run the test body on a fresh OS thread, call init_shard there with a minimal fixture, and drive the async fn with an explicit current-thread runtime. The ShardSliceInit fixture moves from slice.rs's private tests module to a shared cfg(test) pub(crate) slice::test_support module so both test modules construct slices the same way. Verified locally on macOS: - red: cargo test --no-default-features --features runtime-tokio,graph,text-index --lib single_shard_skips_spsc reproduced the exact Windows panic before the fix; green after. - touched modules green under all three feature sets (default, runtime-tokio,graph,text-index; runtime-tokio,jemalloc). - cargo fmt --check + clippy -D warnings clean on default and Windows feature sets. author: Tin Dang * test(ci): fix Windows integration-test failures (binary discovery, diskfull guard, CRLF offsets) Round 2 of the Windows main-CI cleanup. After the two lib-test fixes, the dispatched Windows run surfaced the pre-existing integration-tier failures (present in every main run since at least 2026-06-23; the job is main-push only so PR CI never sees them). All 12 failing test binaries reduce to three environmental test-infra bugs: 1. Binary discovery: 8 suites' find_moon_binary() probed target/{release,debug}/moon, which never matches on Windows (missing .exe suffix) -> "No moon binary found". The probing was also the documented stale-binary trap: it ignored CARGO_TARGET_DIR and preferred a possibly-stale release binary over the one cargo just built. All helpers now fall back to env!("CARGO_BIN_EXE_moon") (compile-time path with the right profile, target dir, and platform suffix) after the explicit MOON_BIN override; jepsen_lite gains the MOON_BIN override it was missing. 2. Diskfull guard: windows-latest images run with <5% free on C:, so Moon's disk monitor permanently paused writes in every server-spawning suite (MOONERR diskfull instead of OK, seen in cross_shard_consistency_red, ft_search_yield_red, spsc_wake_floor_red, coordinator_local_leg_durability, durability_tests). New MOON_DISK_FREE_MIN_PCT env override for --disk-free-min-pct (clap "env" feature; explicit CLI flag still wins), exported as "0" by the Windows CI test step and inherited by every spawned moon process. TDD: red asserted the env var was ignored before the clap attribute landed; test also pins CLI>env precedence and the 5 default without the var. 3. CRLF offsets: shardslice_shape.rs::split_off_test_module accumulated line offsets as line.len() + 1, assuming a 1-byte \n terminator. Windows runners check out with core.autocrlf=true, so the split point drifted one byte per line backward and panicked slicing inside a multibyte box-drawing comment char. Now uses split_inclusive('\n') for byte-exact offsets on both line endings. TDD: new split_off_test_module_is_crlf_safe pins the exact split offset for LF and CRLF inputs (red: off by 3 on CRLF). Verified locally on macOS: all six rewritten spawn suites green (spsc_two_db, shardslice_live, mem_watchdog, txn_kv_wiring, vector_del_unindex, oom_bypass_closure), shardslice_shape 6/6, config env-override test red->green, cargo fmt --check clean, CI-parity clippy clean on default and Windows feature sets. author: Tin Dang * test(sigterm): widen readiness poll deadline to tolerate host load (RSS/CPU wave 5, item C8) `tests/sigterm_shutdown.rs`'s `wait_for_ready` was already poll-based (100ms re-poll against a deadline, matching the pattern used elsewhere in this repo, e.g. `crash_recovery_vector_durability`'s `wait_ready`) — but both call sites hardcoded a fixed 15s deadline independently. PR #218 documented "server did not become ready within 15s" as an observed flake, not a real readiness regression: a contended CI runner or the documented OrbStack virtiofs starvation pattern (CLAUDE.md gotchas) can push a cold server spawn + first-bind past 15s even though the process is healthy. Introduce a single `READY_TIMEOUT` constant (60s) shared by both `wait_for_ready` call sites (the main `assert_sigterm_clean_exit_shards` path and the write-storm test), replacing the two independently-hardcoded 15s literals. This does not slow down the healthy-server case at all -- the poll loop still notices readiness within one 100ms tick of it actually happening; only a genuinely-broken server (or a truly wedged CI host) would ever wait out the full deadline. Panic messages now interpolate the constant instead of hardcoding "15s" so they can't drift out of sync with the actual value again. Left the separate post-SIGTERM exit deadline (10s, `assert_sigterm_clean_exit_shards`'s shutdown-wait loop) untouched -- it's a different flake surface not implicated by the #218 report, and out of scope for this item. Tests (verify-and-harden, not a traditional red/green cycle -- this widens a timeout rather than fixing a logic bug): all 7 sigterm tests re-run to confirm the constant swap didn't change pass/fail behavior in the healthy-host case. Verified: `cargo test --release --test sigterm_shutdown -- --test-threads=1` (7/7 passed). fmt-clean. `cargo clippy -- -D warnings` (the exact default-feature CI gate, no `--tests`) is clean; `--tests` surfaces pre-existing, unrelated warnings across other integration-test files not touched here (config.rs default-then-assign, doc-list indentation in xshard_fastpath_api.rs, etc.) -- confirmed pre-existing, not introduced by this change. author: Tin Dang * test(shard): gate RSS-dependent mem_watchdog cases to Linux/macOS Round 3 of the Windows main-CI cleanup. After round 2, the Windows job is down from 12 failing test binaries to one: mem_watchdog cases A and B assert the memfull guard ENGAGES with a tiny fake limit — but the guard reads real process RSS via get_rss_bytes(), which returns the documented 0 fallback on Windows. RSS 0 means 0% used, so the guard is structurally inert there and the engage-assertions can never pass: the exact reasoning that already gated the lib smoke test (test_poll_real_process_smoke) in round 1. Cases C and D assert the guard-disabled directions, which hold trivially at RSS 0 — they keep running on Windows. The macOS leg's sigterm_clean_exit_default 15s-readiness flake is fixed by the cherry-picked wave-5 item C8 commit (shared READY_TIMEOUT 60s), which lands just before this one. Verified locally on macOS: mem_watchdog + sigterm_shutdown 11/11, cargo fmt --check clean. author: Tin Dang --------- Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 7, 2026
…F writer, hygiene batch (#232) * perf(vector): mmap exact-rerank f16 sidecar on segment reload (RSS/CPU wave 5, item A) A segment reloaded from disk (`read_immutable_segment`) used to `fs::read` the entire `raw_f16.bin` exact-rerank sidecar (HQ-1) into a fresh `Vec<u16>`, on top of the identical bytes the segment already wrote when it was compacted/merged. That doubles resident vector memory for any reload-heavy deployment (warm restarts, segment promotion) — the biggest single RSS win identified in the RSS/CPU remediation review. Introduce `RawF16Store` (`src/vector/segment/raw_f16_store.rs`): an enum over `Owned(Vec<u16>)` (freshly-built segments keep their existing owned buffer, zero behavior change) and `Mapped { mmap, len }` (reloaded segments memory-map `raw_f16.bin` instead and hand out a zero-copy `&[u16]` view backed by the kernel page cache — RSS only grows for pages the rerank path actually touches). `ImmutableSegment::raw_f16()` keeps its exact public signature (`Option<&[u16]>`), so every existing call site (rerank_exact, the AE-1 adaptive-ef estimator, GraphUnion merge, FT.INFO) is unchanged. The mmap soundness contract mirrors the existing warm-segment `sealed_mmap` module and the CSR mmap loader (`graph::csr::mmap`) already in this codebase: `raw_f16.bin` is written once via `write_immutable_segment_staged`'s staged-directory -> final-directory atomic rename, and a concurrent GC removal of a superseded segment directory is safe even while a reader still holds an open mmap (POSIX unlink semantics keep the inode's pages alive until the last mmap drops). Two new `unsafe` blocks (both isolated in `raw_f16_store.rs`, each with a SAFETY comment): `Mmap::map` (read-only file mapping) and a `slice::from_raw_parts` reinterpret of the mapped bytes as `&[u16]` (sound because the mmap base is always page-aligned, and the file is written as little-endian halves on moon's only little-endian target architectures). Flagged for user approval in tmp/wave5/SUMMARY.md. Also lands the item-A hygiene follow-up flagged during the mmap investigation: `src/text/posting.rs`'s `PostingList::term_freqs`/ `positions` grow to the peak document count ever seen for a term and are never released (the `postings` HashMap entry is intentionally kept forever, even for a term with zero live docs, per the existing `remove_doc` contract). `remove_doc` now `shrink_to_fit()`s a posting's buffers once its last live document is removed, reclaiming peak capacity for terms that go idle without changing the survive-forever entry contract or any observable tf/doc_freq/search output. Tests (red/green TDD): - `test_reload_raw_f16_sidecar_uses_mmap_and_matches_owned_rerank` (segment_io.rs): red without the mmap wiring (`raw_f16_is_mapped()` was always false); green after — also pins byte-identical sidecar content and identical `search()` output between the Owned and Mapped backing. - `map_file_*` unit tests (raw_f16_store.rs): roundtrip, size-mismatch rejection, missing-file error, empty-file handling. - `remove_doc_shrinks_now_empty_posting_capacity`, `remove_doc_shrinks_now_empty_posting_positions_capacity`, `remove_doc_does_not_shrink_still_live_posting` (posting.rs): red without the shrink_to_fit() calls (verified by temporarily reverting them). Verified: full `vector::` lib test module (626 passed), text::posting tests, and `crash_recovery_vector_durability -- --ignored s1` all green with MOON_BIN pinned to a fresh `cargo build` debug binary. fmt-clean, clippy-clean on both default and `runtime-tokio,jemalloc` feature sets. author: Tin Dang * perf(persistence): idle-adaptive AOF writer wake cadence (RSS/CPU wave 5, item B) The 3 steady-state AOF writer loops that need a bounded channel poll to service the EverySec proactive-fsync deadline check (TopLevel tokio, PerShard tokio, PerShard monoio) polled at a FIXED cadence forever — 50ms for the monoio per-shard writer, 200ms for both tokio writers — even when the server sits completely idle. That is 5-20 wakeups per second per shard doing nothing but re-checking "is there a message yet" and "has 1s passed since the last fsync". TopLevel monoio needed no change: it already blocks on an untimed `rx.recv()`, correct because idle there genuinely means nothing buffered to protect a deadline for. Introduce `IdleWait` (`src/persistence/aof/writer_task.rs`): a tiny state machine tracking an escalation step (50ms -> 250ms -> 1s) and a "pending deadline" flag. `current()` gives the wait duration for the next poll; `on_message()` resets to the fast floor; `on_timeout()` escalates UNLESS a deadline is pending, in which case it stays pinned at the floor. This is safe with zero latency cost on real traffic: `recv_timeout`/ `tokio::time::timeout(..., recv_async())` race a message against the deadline, so an incoming write always wakes the loop immediately no matter how long the current timeout is set — only the "still idle, nothing to do" re-poll cadence relaxes. The one invariant that must never regress is the EverySec bound: the oldest unflushed byte must reach disk within ~1s + one wake, exactly as under the old fixed cadence. `mark_pending()`/`clear_pending()` enforce this literally: escalation is refused for as long as (a) a batch was written under `FsyncPolicy::EverySec` without an immediate fsync (relying on the proactive deadline check), or (b) `last_fsync` was manually back-dated by the F6 post-fold drain trick (both TopLevel-tokio Rewrite/ RewriteSharded paths and the PerShard-monoio RewritePerShard path) — the back-dating comments explicitly promise a "~150ms total" post-fold window assuming a floor-speed next wake, so escalating away from the floor before that fires would have silently widened the window. `FsyncPolicy::Always` never buffers past its own same-iteration fsync and `FsyncPolicy::No` has no deadline at all, so neither ever calls `mark_pending` — both escalate freely once idle, which is correct: nothing time-sensitive to protect. Tests (red/green TDD): `idle_wait_tests` (5 unit tests) pin the state machine directly — start-at-floor, escalate-and-cap, message-resets, pending-blocks-escalation, clearing-resumes-escalation. Verified (fresh `cargo build --release`, since 3 of the integration suites below hardcode `./target/release/moon` rather than honoring `MOON_BIN`/`find_moon_binary` — a pre-existing test-harness quirk, not touched here): - `crash_matrix_per_shard_aof -- --ignored` (3/3) - `crash_matrix_per_shard_bgrewriteaof -- --ignored` (2/2) - `wal_group_commit --include-ignored` (13/13, incl. both sigkill integration tests) - `coordinator_local_leg_durability --include-ignored` (7/7) - `crash_recovery_vector_durability -- --ignored s1` (MOON_BIN pinned) - `cargo test --lib -- persistence::aof::` under both default and `runtime-tokio,jemalloc` feature sets (50 / 56 passed) Also discovered (not caused by this change — confirmed by a 3x baseline run on the item-A-only commit before this one, which reproduced the same ~2/3 failure rate): `aof_fsync_err_subscribe_ordering`'s `_multi_shard` case is flaky independent of these edits. Left untouched; noted as a follow-up in tmp/wave5/SUMMARY.md. fmt-clean, clippy-clean (default and runtime-tokio,jemalloc feature sets). author: Tin Dang * perf(persistence): shrink WAL v3 write buffer after an oversized flush (RSS/CPU wave 5, item C1) `WalWriterV3::buf` (`src/persistence/wal_v3/segment.rs`) starts at 8KB but is a plain `Vec<u8>` — a single oversized record (e.g. a large FullPageImage payload written through `append`) grows it via the normal `Vec` growth policy, and `flush_write`/`rotate_segment` only ever `.clear()` the buffer afterward. `clear()` does not release capacity, so one big write pins that peak allocation for the writer's entire lifetime — `resident_bytes()` (the P10 INFO memory accounting hook) would report the high-water mark forever, not the steady-state working set. Add a `WAL_BUF_SHRINK_THRESHOLD` (4x the 8KB default): both places that drain the buffer (`flush_write`'s normal path and `rotate_segment`'s end-of-segment flush) now `shrink_to(DEFAULT_WAL_BUF_CAPACITY)` once capacity exceeds the threshold. `shrink_to` is a no-op when already at or below the target, so steady small-record traffic (the common case) never pays a reallocation. Tests (red/green TDD): `test_buffer_shrinks_after_flush_following_large_record` appends a 100KB record, confirms `resident_bytes()` exceeds the threshold, flushes, and asserts the buffer drops back to the default capacity — failed to compile without the new constants/shrink call (red), passes after (green). `test_buffer_does_not_shrink_below_threshold` pins the common case: small records never trip the shrink path. Verified: `cargo test --lib persistence::wal_v3::segment` (15/15 passed). fmt-clean, clippy-clean (default features; no runtime-specific code touched so no separate tokio-feature run needed for this file). author: Tin Dang * perf(shard): SmallVec the per-tick elastic-budget shard snapshot (RSS/CPU wave 5, item C3) `ShardDatabases::recompute_elastic_budget` (`src/shard/shared_databases.rs`) is called from every shard's 100ms eviction tick and used to `collect()` a fresh `Vec<usize>` snapshot of all shards' published memory on every call — one heap allocation per shard per tick (num_shards allocations every 100ms cluster-wide), just to hand a `&[usize]` to `compute_elastic_budget`. Switch the snapshot to `SmallVec<[usize; 16]>`: deployments with <=16 shards (the overwhelming common case) now do this entirely on the stack; larger shard counts still spill to one heap allocation per call, same as before — no regression, pure win for the common case. `compute_elastic_budget`'s signature (`&[usize]`) is unchanged; `SmallVec` derefs to a slice so the call site needs no other changes. Tests (red/green TDD, adapted for a container-swap change with no logic delta): added `recompute_elastic_budget_correct_beyond_smallvec_inline_capacity`, a 20-shard scenario that exercises the SmallVec heap-spill boundary (>16 inline capacity) to pin that the swap never truncates or reorders shard readings once it spills. Existing `recompute_elastic_budget_hot_shard_borrows_idle_headroom` and `recompute_elastic_budget_disabled_for_single_shard_or_unlimited` continue to cover the inline (<=16) path unchanged. Verified: `cargo test --lib shard::shared_databases::tests` (7/7 passed). fmt-clean, clippy-clean on both default and `runtime-tokio,jemalloc` feature sets (smallvec is a pre-existing workspace dependency, used elsewhere e.g. HNSW search). author: Tin Dang * feat(scripting,admin): expose Lua script-cache byte estimate via INFO/MEMORY DOCTOR (RSS/CPU wave 5, item C4) `ScriptCache` (`src/scripting/cache.rs`) is an unbounded per-shard `HashMap<String, Bytes>` — by design, matching Redis (`SCRIPT FLUSH` is the only eviction path; no silent eviction was ever appropriate here). But it was also completely invisible to observability: MEMORY DOCTOR and the Prometheus `moon_memory_bytes` gauge accounted for DashTable/HNSW/ CSR/replication-backlog but folded any Lua cache growth silently into "allocator overhead," making a large `EVAL`/`SCRIPT LOAD` workload undiagnosable from the outside. Add `ScriptCache::resident_bytes()` — sum of hex-SHA1 key length + script body length per entry (an estimate; excludes `HashMap`/`String`/`Bytes` allocator bookkeeping, consistent with how the sibling vector/graph estimators in this codebase already work). Wire it through the existing C5/M4 per-shard `ShardStoreMemory` publish pattern: a new `lua: AtomicUsize` field, refreshed on the same 100ms eviction tick that already refreshes vector/text/graph (`run_eviction_tick` now takes the shard's `Rc<RefCell<ScriptCache>>` to read it). `admin/metrics_setup.rs`'s Prometheus emitter and `command/server_admin.rs`'s `MEMORY DOCTOR` text output both gained a `lua`/`Lua scripts:` line, folded into their tracked sum (so "allocator overhead" shrinks by exactly the amount now attributed correctly). Tests (red/green TDD): `test_resident_bytes_empty_cache_is_zero` and `test_resident_bytes_grows_with_entries_and_shrinks_on_flush` pin the new method directly (0 for an empty cache; exact byte count for 1-2 entries; back to 0 after `flush()`) — both failed to compile before `resident_bytes()` existed (red), pass after (green). Verified: `cargo test --lib -- scripting::cache::tests shard::shared_databases::tests shard::slice::tests shard::mq_exec::tests` (29/29) under both default and `runtime-tokio,jemalloc` feature sets. fmt-clean, clippy-clean (`-D warnings`) on both feature sets; `cargo build`/`cargo check` compile clean on both. author: Tin Dang * chore(protocol): remove dead parse_single_frame_zc parser (RSS/CPU wave 5, item C5) `parse_single_frame_zc` (`src/protocol/parse.rs`) was a full RESP2/RESP3 frame parser, superseded by the current two-pass pipeline (`validate_frame` then `parse_frame_zerocopy`, see `parse()`) but never deleted. It had zero external callers -- the only references to it were its own recursive calls for Array/Map/Set/Push nesting. Its private helper `read_decimal_zc` was in the same boat: used exclusively inside the dead function. The file-level `#![allow(dead_code)]` had been silently absorbing the warning this whole time, which is exactly why the spec flagged it for a verify-and-remove pass rather than a design change. Verified dead (not just the originally-suspected Map arm, but the entire 150-line function): grepped the whole worktree (`src/`, `tests/`, `fuzz/fuzz_targets/`) for `parse_single_frame_zc` and `read_decimal_zc` -- every hit was inside `parse.rs` itself, all self-recursive. The RESP fuzz targets (`resp_parse.rs`, `resp_parse_differential.rs`) only call the public `parse::parse()` entry point, which has routed through `parse_frame_zerocopy` since the "defensive Frame::Null on any failure" rewrite documented at that function's doc comment -- confirming `parse_single_frame_zc` was leftover from before that rewrite. Removed both functions (~160 lines). `find_crlf`/`strict_atoi` (its two other private helpers) stay -- both are still used by the live `parse_frame_zerocopy` and `validate_frame` paths. Tests (verify-dead-code TDD per the wave-5 spec, in place of a traditional red/green cycle for a pure deletion): full `protocol::parse::tests` module re-run post-removal as the regression pin -- 50/50 passed, unchanged behavior. Verified: `cargo test --lib -- protocol::parse::tests` (50/50). fmt-clean, clippy-clean (`-D warnings`) on both default and `runtime-tokio,jemalloc` feature sets. Fuzz targets compile-check clean (grep-verified no reference to the removed functions; `cargo-fuzz` itself needs nightly and wasn't re-run, per repo convention). author: Tin Dang * docs: document jemalloc decay/arena tuning policy (RSS/CPU wave 5, item C6) Audited `src/main.rs` for jemalloc decay/arena drift: the baked-in static `_rjem_malloc_conf` export (used at process init) and the `--memory-arenas-cap` re-spawn override (`maybe_respawn_with_arena_override`) already carry the byte-identical tuning string -- `background_thread:true,metadata_thp:auto,dirty_decay_ms:1000,muzzy_decay_ms:5000,abort_conf:true` -- with only `narenas` substituted for the operator's requested cap. No drift, no duplicated-with-different-values config to reconcile; grepped `config.rs`, `runtime/mod.rs`, `admin/metrics_setup.rs`, and `command/server_admin.rs` for any second/conflicting decay definition -- none exists. What WAS missing: none of this was documented for operators. CLAUDE.md's Environment Variables section lists `_RJEM_MALLOC_CONF`-adjacent knobs (`--memory-arenas-cap` is only documented in `config.rs`'s CLI help string) but never explains the actual decay policy or the env var an operator would need to override it. Added a `_RJEM_MALLOC_CONF` entry: what the baked-in defaults are, why 1s dirty-page decay + a background reclaim thread matter for RSS (freed-but-idle pages return to the OS promptly instead of sitting in jemalloc's caches), the exec-before-init timing constraint `--memory-arenas-cap` depends on, and the existing "operator env wins" guard. Docs-only change per the wave-5 spec's guidance for this item ("if no tuning-code drift exists, add MALLOC_CONF guidance to docs only -- config hygiene only, measure nothing"). No code touched; no tests to run. author: Tin Dang * docs: audit tokio 1ms shard tick idle cost, no change needed (RSS/CPU wave 5, item C7) Audited the tokio-runtime shard event loop's 1ms `periodic_interval` tick (`src/shard/event_loop.rs` ~1292, handler in `spsc_handler::drain_spsc_shared`) for idle-cost waste, the same class of problem item B fixed for the AOF writer's poll cadence. Found the tick is already cheap when idle: - `drain_spsc_shared`'s per-consumer drain loop is a non-blocking `try_pop()` that returns `None` immediately when a consumer's queue is empty -- O(num_consumers) pointer checks, zero allocation (scratch `Vec`s are thread-local and reused via `mem::take`). - Every side effect downstream of the drain is already gated behind a cheap conditional: WAL re-notify only `if hit_cap`, autovacuum schedule persist only `if is_dirty()`, CDC registration only `if !pending_cdc_subscribes.is_empty()`, migrations only when `pending_migrations` is non-empty. - The one unconditional per-tick cost, `cached_clock.update()`, is a single `clock_gettime` call by design -- its own doc comment states it's "the ONE place per shard that actually calls clock_gettime," exactly the "Timestamp caching" design decision CLAUDE.md documents. Unlike item B's AOF writer (which polled a channel purely to catch an EverySec deadline with no inherent latency contract on the poll interval itself), this 1ms cadence IS the latency contract: CLAUDE.md's Key Design Decisions calls it out explicitly ("Low-latency append via in-memory buffer flushed on 1ms tick"). Escalating it while idle, the way item B escalates the AOF writer's poll, would directly widen the WAL-flush latency bound it exists to hold -- there is no "idle" for a write-latency SLA in the way there is for an EverySec fsync deadline. Disposition: SKIP code change. No unsafe cheap-skip exists beyond what's already there; a real fix (event-driven WAL flush trigger + a much longer backstop timer, replacing the periodic tick's role entirely) is an architectural change, not a hygiene-item-sized fix -- documented here as a follow-up for a future wave if idle CPU from this specific tick is ever measured as material (not attempted in this audit; no regression possible since nothing was changed). author: Tin Dang * test(sigterm): widen readiness poll deadline to tolerate host load (RSS/CPU wave 5, item C8) `tests/sigterm_shutdown.rs`'s `wait_for_ready` was already poll-based (100ms re-poll against a deadline, matching the pattern used elsewhere in this repo, e.g. `crash_recovery_vector_durability`'s `wait_ready`) — but both call sites hardcoded a fixed 15s deadline independently. PR #218 documented "server did not become ready within 15s" as an observed flake, not a real readiness regression: a contended CI runner or the documented OrbStack virtiofs starvation pattern (CLAUDE.md gotchas) can push a cold server spawn + first-bind past 15s even though the process is healthy. Introduce a single `READY_TIMEOUT` constant (60s) shared by both `wait_for_ready` call sites (the main `assert_sigterm_clean_exit_shards` path and the write-storm test), replacing the two independently-hardcoded 15s literals. This does not slow down the healthy-server case at all -- the poll loop still notices readiness within one 100ms tick of it actually happening; only a genuinely-broken server (or a truly wedged CI host) would ever wait out the full deadline. Panic messages now interpolate the constant instead of hardcoding "15s" so they can't drift out of sync with the actual value again. Left the separate post-SIGTERM exit deadline (10s, `assert_sigterm_clean_exit_shards`'s shutdown-wait loop) untouched -- it's a different flake surface not implicated by the #218 report, and out of scope for this item. Tests (verify-and-harden, not a traditional red/green cycle -- this widens a timeout rather than fixing a logic bug): all 7 sigterm tests re-run to confirm the constant swap didn't change pass/fail behavior in the healthy-host case. Verified: `cargo test --release --test sigterm_shutdown -- --test-threads=1` (7/7 passed). fmt-clean. `cargo clippy -- -D warnings` (the exact default-feature CI gate, no `--tests`) is clean; `--tests` surfaces pre-existing, unrelated warnings across other integration-test files not touched here (config.rs default-then-assign, doc-list indentation in xshard_fastpath_api.rs, etc.) -- confirmed pre-existing, not introduced by this change. author: Tin Dang * fix(vector): mapped raw_f16 sidecar must not count as resident memory Deep-review follow-up to item A (mmap exact-rerank sidecar): ImmutableSegment::resident_bytes() still charged a Mapped sidecar at its full len*2 bytes. Mapped pages are kernel page cache — reclaimable under memory pressure, not pinned heap — so counting them as resident made the elastic memory budget / eviction pipeline behave as if the mmap RSS win never happened for reloaded segments (over-reporting is the safe direction, but it silently negates the feature's benefit in Moon's own accounting). New RawF16Store::resident_bytes() (Owned = full buffer, Mapped = 0), used by resident_bytes(). Pinned by a unit test (Owned=200/Mapped=0 for 100 halves) and an integration assert in the reload-parity test (owned segment reports at least sidecar-bytes more than the reloaded mapped one). Also corrects three stale comments in writer_task.rs claiming the idle ladder "starts at 200ms" — AOF_IDLE_WAIT_STEPS' floor is 50ms (tighter than the old fixed 200ms tokio cadence right after activity, escalating to 1s once idle). author: Tin Dang * docs(vector): move SAFETY markers within audit range of unsafe blocks CI's Lint job runs scripts/audit-unsafe.sh, which requires the literal "SAFETY:" marker within the 3 lines immediately above each unsafe block. Both raw_f16_store.rs comments had the marker at the TOP of a longer explanatory block (5 and 13 lines above the unsafe), so the audit flagged them as missing despite full SAFETY documentation. Restructured both: the detailed invariant explanation stays, and a one-line "// SAFETY: ..." summary now sits directly above each unsafe expression. audit-unsafe.sh passes locally (257/257); no code change. author: Tin Dang --------- Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proactive RSS watchdog ("mem-full guard") — RSS/CPU wave 3
Review item 6 from the RSS/CPU/OOM deep review (wave 1 = #216, wave 2 = #217). Moon had a diskfull guard but no memory analogue: with
--maxmemoryunset (or set too high), RSS grows until the kernel OOM-killer picks Moon.What ships
src/shard/mem_monitor.rs— mirrorsdisk_monitor.rsexactly: one atomic-backed monitor, shard-0 poll on the existing 5s tick, hot path = a singleAtomicBool::load(Relaxed), hysteresis state machine (inverted vs disk: pause atrss% >= --mem-full-pct, resume atpause-5),u8-wrap clamp for RSS > detected limit (a stale/under-detected cgroup limit must read as pressure, not wrap to "healthy").detect_memory_limit_bytes: Linux cgroup v2/v1; macOShw.memsizesince fix(shard): OOM shield — macOS maxmemory guardrail + eviction bypass closure (RSS/CPU wave 2) #217) — not on configuredmaxmemory, which can be an unconfigured 0. Limit resolved once at startup; one immediate poll makes the AOF-replay/segment-load startup peak visible before the first tick.is_any_write_stall_active();MOONERR memfull: writes paused until memory pressure recoversin both dispatch paths (one extra Relaxed load, only evaluated when a stall is already active);--mem-full-pctCLI (default 95, 0 = disabled); INFOreclamation_mem_rss_bytes/reclamation_mem_watchdog_active(ORed intowrite_stall_active).MOON_MEM_LIMIT_BYTESenv override (test/diagnostic knob, same family asMOON_XSHARD_*) so integration tests can engage the guard deterministically.Tests
RED-proven (all 4 integration cases fail on a pre-feature binary): engage-on-first-write, disabled-by-pct-0, no-limit-no-pause, reads-never-blocked. 15 unit tests mirror the disk monitor's coverage plus the inverted-hysteresis and wrap-clamp cases. Green on monoio + tokio matrices.
Deep-reviewed per process (manual diff review + independent test reruns by the orchestrator).
Summary by CodeRabbit
New Features
--mem-full-pctsetting to control when the safeguard activates.Bug Fixes
Tests