perf: RSS/CPU remediation wave 5 — mmap f16 sidecar, idle-adaptive AOF writer, hygiene batch#232
Merged
Merged
Conversation
…U 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
…e 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
…h (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
…/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
…/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
…ve 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
…em 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
… 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
…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
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
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? |
📝 WalkthroughWalkthroughThis PR bundles multiple independent fixes: AOF writer adaptive idle polling (IdleWait), WAL v3 buffer shrink-back logic, posting-list capacity reclaim, Lua script-cache resident-byte tracking surfaced through metrics/MEMORY DOCTOR, mmap-backed exact-rerank sidecar storage (RawF16Store), a rewritten RESP zero-copy parser, and documentation updates. ChangesAOF Writer Adaptive Idle Wait
Estimated code review effort: 4 (Complex) | ~60 minutes WAL v3 Write-Buffer Shrinking
Posting-List Capacity Reclaim
Lua Script-Cache Memory Accounting
Vector Exact-Rerank Sidecar Mmap Loading
RESP Zero-Copy Parser Rewrite
Documentation Updates
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant WriterLoop
participant IdleWait
participant Fsync
WriterLoop->>IdleWait: current()
IdleWait-->>WriterLoop: timeout duration
WriterLoop->>IdleWait: on_timeout() / on_message()
WriterLoop->>IdleWait: mark_pending() (EverySec buffered)
WriterLoop->>Fsync: flush + sync_data
Fsync-->>WriterLoop: success
WriterLoop->>IdleWait: clear_pending()
sequenceDiagram
participant EventLoop
participant RunEvictionTick
participant ScriptCache
participant ShardStoreMemory
EventLoop->>RunEvictionTick: run_eviction_tick(script_cache_rc)
RunEvictionTick->>ScriptCache: resident_bytes()
ScriptCache-->>RunEvictionTick: byte count
RunEvictionTick->>ShardStoreMemory: store lua atomic
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/persistence/wal_v3/segment.rs (2)
793-807: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeak assertion for the "no shrink below threshold" test.
The 10 small records never grow the buffer anywhere near
WAL_BUF_SHRINK_THRESHOLD(32768), soresident_bytes() <= WAL_BUF_SHRINK_THRESHOLDpasses trivially even if the shrink condition were wrong (e.g. inverted). Consider growing the buffer moderately (aboveDEFAULT_WAL_BUF_CAPACITYbut below the threshold) and asserting capacity is unchanged after flush, to actually exercise the no-shrink branch.🤖 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/persistence/wal_v3/segment.rs` around lines 793 - 807, The test test_buffer_does_not_shrink_below_threshold is too weak because the appended records never bring WalWriterV3’s buffer near WAL_BUF_SHRINK_THRESHOLD, so the assertion can pass without exercising the no-shrink path. Update the test to grow the buffer moderately above DEFAULT_WAL_BUF_CAPACITY but still below the shrink threshold, then call flush_sync and assert the buffer capacity (or resident_bytes) remains unchanged to verify the no-shrink branch in WalWriterV3.
195-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated shrink logic between
flush_writeandrotate_segment.The exact same "clear buffer, then shrink if capacity exceeds threshold" block is repeated verbatim in both methods. Extracting it into a small helper (e.g.
fn shrink_buf_if_oversized(&mut self)) would avoid future drift if the threshold logic changes.♻️ Proposed refactor
+ /// Release peak buffer capacity back to the default once drained, if it + /// grew past the shrink threshold (e.g. from an oversized FullPageImage). + #[inline] + fn shrink_buf_if_oversized(&mut self) { + if self.buf.capacity() > WAL_BUF_SHRINK_THRESHOLD { + self.buf.shrink_to(DEFAULT_WAL_BUF_CAPACITY); + } + } + fn flush_write(&mut self) -> std::io::Result<()> { ... self.buf.clear(); - // Release peak capacity from an oversized record (e.g. a large - // FullPageImage) rather than pinning it for the writer's - // lifetime; `shrink_to` is a no-op below the target capacity. - if self.buf.capacity() > WAL_BUF_SHRINK_THRESHOLD { - self.buf.shrink_to(DEFAULT_WAL_BUF_CAPACITY); - } + self.shrink_buf_if_oversized(); } ... fn rotate_segment(&mut self) -> std::io::Result<()> { ... self.buf.clear(); - if self.buf.capacity() > WAL_BUF_SHRINK_THRESHOLD { - self.buf.shrink_to(DEFAULT_WAL_BUF_CAPACITY); - } + self.shrink_buf_if_oversized();Also applies to: 442-444
🤖 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/persistence/wal_v3/segment.rs` around lines 195 - 200, The buffer cleanup and shrink block is duplicated in both `flush_write` and `rotate_segment`, which risks the two paths drifting over time. Extract the shared “clear buffer, then shrink if capacity exceeds WAL_BUF_SHRINK_THRESHOLD” logic into a small helper such as `shrink_buf_if_oversized` on the segment type, and call that helper from both `flush_write` and `rotate_segment` so the threshold and capacity behavior stay centralized.src/shard/shared_databases.rs (1)
20-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
#[derive(Default)]forShardStoreMemoryto eliminate repeated zero-init literals.All four fields are
AtomicUsize, which implementsDefault. The samevector/text/graph/lua: AtomicUsize::new(0)struct literal is now duplicated in three places (this file'sShardDatabases::new,src/shard/slice.rstest_support, andsrc/shard/mq_exec.rstests). DerivingDefaultand usingShardStoreMemory::default()removes the duplication and reduces the chance of a stale literal when a new field is added later.♻️ Proposed refactor
+#[derive(Default)] pub struct ShardStoreMemory { pub vector: AtomicUsize, pub text: AtomicUsize, pub graph: AtomicUsize, pub lua: AtomicUsize, }- Arc::new(ShardStoreMemory { - vector: AtomicUsize::new(0), - text: AtomicUsize::new(0), - graph: AtomicUsize::new(0), - lua: AtomicUsize::new(0), - }) + Arc::new(ShardStoreMemory::default())Also applies to: 88-114
🤖 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/shared_databases.rs` around lines 20 - 31, `ShardStoreMemory` repeats the same zero-initialized `AtomicUsize` literals in multiple places, so derive `Default` on the `ShardStoreMemory` struct and switch `ShardDatabases::new` plus the `test_support`/`mq_exec` setup sites to use `ShardStoreMemory::default()` instead of manual field-by-field initialization. This keeps the constructor and tests aligned and avoids duplicating the `vector/text/graph/lua` defaults if the struct changes later.
🤖 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/persistence/aof/writer_task.rs`:
- Around line 663-668: Gate the rewrite deadline pinning so `mark_pending()` is
only applied for the `EverySec` proactive fsync path, since `Always` and `No`
never run the matching `clear_pending()` logic. Update the
post-rewrite/back-dating logic in `WriterTask` and the related TopLevel and
monoio per-shard post-fold paths to use the same `EverySec` guard, keeping the
symbols `mark_pending()`, `clear_pending()`, and `last_fsync` aligned across
those branches.
- Around line 747-754: The fsync path in writer_task::AofWriterTask should only
clear the pending gate after a successful durability sync, because ignoring
failures lets idle polling advance while data is still not durable. Update the
`fsync == FsyncPolicy::EverySec` branch to check the results of
`writer.flush().await` and `writer.get_ref().sync_data().await`, and only then
update `last_fsync` and call `idle_wait.clear_pending()`. Keep `last_fsync` and
the pending state unchanged on any failure so the next loop retries at the
floor, and apply the same fix in the other pending-gate fsync block referenced
by the same logic.
In `@src/vector/persistence/segment_io.rs`:
- Around line 559-564: The persisted segment size calculation in segment_io can
overflow before the sidecar mmap check runs, so update the `mvcc.len() * dim`
and related `expected_halves * 2` arithmetic to use checked/saturating size
computation. In the `RawF16Store::map_file` path and the sidecar length
validation logic, guard against overflow by failing closed or returning None
when the computed element or byte count cannot be represented safely, rather
than comparing against a wrapped value.
- Around line 563-570: The raw_f16 handling in segment_io::map_file currently
treats all failures from RawF16Store::map_file as a silent missing sidecar
because of unwrap_or_default, which hides permission, mmap, and transient I/O
problems. Update the logic around raw_f16 and RawF16Store::map_file so that an
existing sidecar which is present and the expected size but fails to open/mmap
emits a warning (or equivalent logged diagnostic) instead of disappearing
silently, while still keeping the expected missing-file case quiet. Use the
existing raw_f16_path metadata check and the raw_f16/is_none branch to
distinguish the cases without adding a second map_file call.
In `@src/vector/segment/raw_f16_store.rs`:
- Around line 62-65: The expected sidecar byte-length calculation in
raw_f16_store::map_file uses unchecked multiplication on expected_halves, which
can wrap in release builds and let an invalid mapping through. Replace the
len_bytes comparison with checked arithmetic for the expected size, and return
the same error path if the computed byte length cannot be represented safely;
keep the fix localized to map_file and preserve the existing Mapped/as_slice
length invariants.
- Around line 40-46: RawF16Store currently exposes the Mapped variant publicly,
allowing callers to create an invalid len and break as_slice safety. Make
RawF16Store non-constructible from unsafe/public fields by hiding the variant
behind a private/internal representation or by making the type crate-private,
then expose only validated constructors such as RawF16Store::owned and the
existing map_file path. Ensure as_slice and any mapped accessors rely only on
the validated len derived from mmap so external code cannot bypass the length
check.
---
Nitpick comments:
In `@src/persistence/wal_v3/segment.rs`:
- Around line 793-807: The test test_buffer_does_not_shrink_below_threshold is
too weak because the appended records never bring WalWriterV3’s buffer near
WAL_BUF_SHRINK_THRESHOLD, so the assertion can pass without exercising the
no-shrink path. Update the test to grow the buffer moderately above
DEFAULT_WAL_BUF_CAPACITY but still below the shrink threshold, then call
flush_sync and assert the buffer capacity (or resident_bytes) remains unchanged
to verify the no-shrink branch in WalWriterV3.
- Around line 195-200: The buffer cleanup and shrink block is duplicated in both
`flush_write` and `rotate_segment`, which risks the two paths drifting over
time. Extract the shared “clear buffer, then shrink if capacity exceeds
WAL_BUF_SHRINK_THRESHOLD” logic into a small helper such as
`shrink_buf_if_oversized` on the segment type, and call that helper from both
`flush_write` and `rotate_segment` so the threshold and capacity behavior stay
centralized.
In `@src/shard/shared_databases.rs`:
- Around line 20-31: `ShardStoreMemory` repeats the same zero-initialized
`AtomicUsize` literals in multiple places, so derive `Default` on the
`ShardStoreMemory` struct and switch `ShardDatabases::new` plus the
`test_support`/`mq_exec` setup sites to use `ShardStoreMemory::default()`
instead of manual field-by-field initialization. This keeps the constructor and
tests aligned and avoids duplicating the `vector/text/graph/lua` defaults if the
struct changes later.
🪄 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: c18773b5-4655-41f4-a3da-4cfa4d8feba9
📒 Files selected for processing (18)
CHANGELOG.mdCLAUDE.mdsrc/admin/metrics_setup.rssrc/command/server_admin.rssrc/persistence/aof/writer_task.rssrc/persistence/wal_v3/segment.rssrc/protocol/parse.rssrc/scripting/cache.rssrc/shard/event_loop.rssrc/shard/mq_exec.rssrc/shard/persistence_tick.rssrc/shard/shared_databases.rssrc/shard/slice.rssrc/text/posting.rssrc/vector/persistence/segment_io.rssrc/vector/segment/immutable.rssrc/vector/segment/mod.rssrc/vector/segment/raw_f16_store.rs
💤 Files with no reviewable changes (1)
- src/protocol/parse.rs
Comment on lines
+663
to
+668
| // writer's post-rewrite back-dating). This is itself | ||
| // a pending deadline — pin the idle wait at its | ||
| // floor so escalation cannot push the next check | ||
| // past the intended window. | ||
| last_fsync = Instant::now() - std::time::Duration::from_millis(900); | ||
| idle_wait.mark_pending(); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Gate rewrite deadline pinning to EverySec.
These mark_pending() calls run for Always and No too, but only the EverySec proactive path calls clear_pending(). After a rewrite under Always/No, the writer can stay pinned at the 50ms floor forever.
Proposed fix
- last_fsync = Instant::now() - std::time::Duration::from_millis(900);
- idle_wait.mark_pending();
+ if fsync == FsyncPolicy::EverySec {
+ last_fsync =
+ Instant::now() - std::time::Duration::from_millis(900);
+ idle_wait.mark_pending();
+ }Apply the same guard to the sharded TopLevel and monoio per-shard post-fold paths.
Also applies to: 711-714, 1482-1490
🤖 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/persistence/aof/writer_task.rs` around lines 663 - 668, Gate the rewrite
deadline pinning so `mark_pending()` is only applied for the `EverySec`
proactive fsync path, since `Always` and `No` never run the matching
`clear_pending()` logic. Update the post-rewrite/back-dating logic in
`WriterTask` and the related TopLevel and monoio per-shard post-fold paths to
use the same `EverySec` guard, keeping the symbols `mark_pending()`,
`clear_pending()`, and `last_fsync` aligned across those branches.
Comment on lines
747
to
+754
| if fsync == FsyncPolicy::EverySec | ||
| && !write_error | ||
| && last_fsync.elapsed() >= std::time::Duration::from_secs(1) | ||
| { | ||
| let _ = writer.flush().await; | ||
| let _ = writer.get_ref().sync_data().await; | ||
| last_fsync = Instant::now(); | ||
| idle_wait.clear_pending(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Only clear pending after a successful fsync.
With the new pending gate, clearing idle_wait after ignored flush() / sync_data() failures lets idle polling escalate while bytes are still not durable. Keep last_fsync and pending unchanged on failure so the next loop retries at the floor.
Proposed fix
- let _ = writer.flush().await;
- let _ = writer.get_ref().sync_data().await;
- last_fsync = Instant::now();
- idle_wait.clear_pending();
+ if let Err(e) = writer.flush().await {
+ error!("AOF EverySec proactive flush failed: {}", e);
+ } else if let Err(e) = writer.get_ref().sync_data().await {
+ error!("AOF EverySec proactive sync_data failed: {}", e);
+ } else {
+ last_fsync = Instant::now();
+ idle_wait.clear_pending();
+ }Also applies to: 1165-1171
🤖 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/persistence/aof/writer_task.rs` around lines 747 - 754, The fsync path in
writer_task::AofWriterTask should only clear the pending gate after a successful
durability sync, because ignoring failures lets idle polling advance while data
is still not durable. Update the `fsync == FsyncPolicy::EverySec` branch to
check the results of `writer.flush().await` and
`writer.get_ref().sync_data().await`, and only then update `last_fsync` and call
`idle_wait.clear_pending()`. Keep `last_fsync` and the pending state unchanged
on any failure so the next loop retries at the floor, and apply the same fix in
the other pending-gate fsync block referenced by the same logic.
Comment on lines
+559
to
+564
| let expected_halves = mvcc.len() * dim; | ||
| let raw_f16_path = seg_dir.join("raw_f16.bin"); | ||
| // missing file (pre-sidecar segment) or open/mmap error -> None, same as | ||
| // a size mismatch. | ||
| let raw_f16: Option<RawF16Store> = | ||
| RawF16Store::map_file(&raw_f16_path, expected_halves).unwrap_or_default(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Check the mvcc.len() * dim and byte-size arithmetic.
A corrupt or extreme persisted segment can overflow expected_halves or expected_halves * 2, making the sidecar length check compare against a wrapped value before attaching the mmap-backed store.
Proposed fix
- let expected_halves = mvcc.len() * dim;
+ let expected_halves = mvcc.len().checked_mul(dim).ok_or_else(|| {
+ SegmentIoError::InvalidMetadata("raw_f16 expected halves overflow".to_owned())
+ })?;
+ let expected_bytes = expected_halves
+ .checked_mul(std::mem::size_of::<u16>())
+ .ok_or_else(|| {
+ SegmentIoError::InvalidMetadata("raw_f16 expected bytes overflow".to_owned())
+ })?;
let raw_f16_path = seg_dir.join("raw_f16.bin");
@@
- if actual_len as usize != expected_halves * 2 {
+ if actual_len != expected_bytes as u64 {
tracing::warn!(
"segment-{segment_id}: raw_f16.bin has {actual_len} bytes, expected {} — \
ignoring sidecar (search degrades to quantized distances)",
- expected_halves * 2
+ expected_bytes
);
}Also applies to: 570-574
🤖 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/vector/persistence/segment_io.rs` around lines 559 - 564, The persisted
segment size calculation in segment_io can overflow before the sidecar mmap
check runs, so update the `mvcc.len() * dim` and related `expected_halves * 2`
arithmetic to use checked/saturating size computation. In the
`RawF16Store::map_file` path and the sidecar length validation logic, guard
against overflow by failing closed or returning None when the computed element
or byte count cannot be represented safely, rather than comparing against a
wrapped value.
Comment on lines
+563
to
+570
| let raw_f16: Option<RawF16Store> = | ||
| RawF16Store::map_file(&raw_f16_path, expected_halves).unwrap_or_default(); | ||
| if raw_f16.is_none() { | ||
| // Distinguish "missing file" (expected, silent) from "present but | ||
| // wrong size" (corruption — worth a loud warning) without doing a | ||
| // second `map_file` call: a cheap metadata probe is enough. | ||
| if let Ok(actual_len) = fs::metadata(&raw_f16_path).map(|m| m.len()) { | ||
| if actual_len as usize != expected_halves * 2 { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Warn on present sidecars that fail to open or mmap.
unwrap_or_default() makes permission, mmap-limit, and transient I/O failures indistinguishable from an expected missing pre-sidecar file. If the file exists and is the right size but mapping fails, exact rerank silently disappears.
Proposed fix
- let raw_f16: Option<RawF16Store> =
- RawF16Store::map_file(&raw_f16_path, expected_halves).unwrap_or_default();
+ let raw_f16: Option<RawF16Store> = match RawF16Store::map_file(&raw_f16_path, expected_halves) {
+ Ok(store) => store,
+ Err(e) if e.kind() == std::io::ErrorKind::NotFound => None,
+ Err(e) => {
+ tracing::warn!(
+ "segment-{segment_id}: failed to map raw_f16.bin: {e}; \
+ ignoring sidecar (search degrades to quantized distances)"
+ );
+ None
+ }
+ };As per coding guidelines, vector-search code must maintain exact-rerank sidecar handling.
📝 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
| let raw_f16: Option<RawF16Store> = | |
| RawF16Store::map_file(&raw_f16_path, expected_halves).unwrap_or_default(); | |
| if raw_f16.is_none() { | |
| // Distinguish "missing file" (expected, silent) from "present but | |
| // wrong size" (corruption — worth a loud warning) without doing a | |
| // second `map_file` call: a cheap metadata probe is enough. | |
| if let Ok(actual_len) = fs::metadata(&raw_f16_path).map(|m| m.len()) { | |
| if actual_len as usize != expected_halves * 2 { | |
| let raw_f16: Option<RawF16Store> = match RawF16Store::map_file(&raw_f16_path, expected_halves) { | |
| Ok(store) => store, | |
| Err(e) if e.kind() == std::io::ErrorKind::NotFound => None, | |
| Err(e) => { | |
| tracing::warn!( | |
| "segment-{segment_id}: failed to map raw_f16.bin: {e}; \ | |
| ignoring sidecar (search degrades to quantized distances)" | |
| ); | |
| None | |
| } | |
| }; | |
| if raw_f16.is_none() { | |
| // Distinguish "missing file" (expected, silent) from "present but | |
| // wrong size" (corruption — worth a loud warning) without doing a | |
| // second `map_file` call: a cheap metadata probe is enough. | |
| if let Ok(actual_len) = fs::metadata(&raw_f16_path).map(|m| m.len()) { | |
| if actual_len as usize != expected_halves * 2 { |
🤖 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/vector/persistence/segment_io.rs` around lines 563 - 570, The raw_f16
handling in segment_io::map_file currently treats all failures from
RawF16Store::map_file as a silent missing sidecar because of unwrap_or_default,
which hides permission, mmap, and transient I/O problems. Update the logic
around raw_f16 and RawF16Store::map_file so that an existing sidecar which is
present and the expected size but fails to open/mmap emits a warning (or
equivalent logged diagnostic) instead of disappearing silently, while still
keeping the expected missing-file case quiet. Use the existing raw_f16_path
metadata check and the raw_f16/is_none branch to distinguish the cases without
adding a second map_file call.
Source: Coding guidelines
Comment on lines
+40
to
+46
| pub enum RawF16Store { | ||
| /// Freshly-built segment (compaction/merge): the sidecar buffer it | ||
| /// already owns in memory. | ||
| Owned(Vec<u16>), | ||
| /// Segment reloaded from disk: zero-copy view into `raw_f16.bin`'s page | ||
| /// cache. `len` is the number of `u16` halves (== `mmap.len() / 2`). | ||
| Mapped { mmap: Mmap, len: usize }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Hide the Mapped variant behind a validated constructor.
RawF16Store::Mapped { mmap, len } is public, so safe code can construct it with a len that exceeds mmap.len() / 2; then as_slice() invokes from_raw_parts with an invalid length. Wrap the enum in a public struct with private variants/fields, or make this type crate-private and expose only validated constructors.
Proposed direction
-pub enum RawF16Store {
+pub struct RawF16Store(RawF16StoreInner);
+
+enum RawF16StoreInner {
/// Freshly-built segment (compaction/merge): the sidecar buffer it
/// already owns in memory.
Owned(Vec<u16>),
/// Segment reloaded from disk: zero-copy view into `raw_f16.bin`'s page
/// cache. `len` is the number of `u16` halves (== `mmap.len() / 2`).
Mapped { mmap: Mmap, len: usize },
}Then add RawF16Store::owned(Vec<u16>) and keep map_file() as the only mapped constructor.
As per coding guidelines, vector-search code must maintain exact-rerank sidecar handling.
🤖 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/vector/segment/raw_f16_store.rs` around lines 40 - 46, RawF16Store
currently exposes the Mapped variant publicly, allowing callers to create an
invalid len and break as_slice safety. Make RawF16Store non-constructible from
unsafe/public fields by hiding the variant behind a private/internal
representation or by making the type crate-private, then expose only validated
constructors such as RawF16Store::owned and the existing map_file path. Ensure
as_slice and any mapped accessors rely only on the validated len derived from
mmap so external code cannot bypass the length check.
Source: Coding guidelines
Comment on lines
+62
to
+65
| pub fn map_file(path: &Path, expected_halves: usize) -> io::Result<Option<Self>> { | ||
| let file = File::open(path)?; | ||
| let len_bytes = file.metadata()?.len() as usize; | ||
| if len_bytes != expected_halves * 2 { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Use checked arithmetic for the expected sidecar byte length.
expected_halves * 2 can overflow in release builds. If it wraps to a matching small file size, map_file() can create a Mapped store whose len no longer matches the mapping, feeding an invalid length into as_slice().
Proposed fix
pub fn map_file(path: &Path, expected_halves: usize) -> io::Result<Option<Self>> {
let file = File::open(path)?;
let len_bytes = file.metadata()?.len() as usize;
- if len_bytes != expected_halves * 2 {
+ let expected_bytes = expected_halves
+ .checked_mul(std::mem::size_of::<u16>())
+ .ok_or_else(|| {
+ io::Error::new(io::ErrorKind::InvalidData, "raw_f16 expected size overflow")
+ })?;
+ if len_bytes != expected_bytes {
return Ok(None);
}📝 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
| pub fn map_file(path: &Path, expected_halves: usize) -> io::Result<Option<Self>> { | |
| let file = File::open(path)?; | |
| let len_bytes = file.metadata()?.len() as usize; | |
| if len_bytes != expected_halves * 2 { | |
| pub fn map_file(path: &Path, expected_halves: usize) -> io::Result<Option<Self>> { | |
| let file = File::open(path)?; | |
| let len_bytes = file.metadata()?.len() as usize; | |
| let expected_bytes = expected_halves | |
| .checked_mul(std::mem::size_of::<u16>()) | |
| .ok_or_else(|| { | |
| io::Error::new(io::ErrorKind::InvalidData, "raw_f16 expected size overflow") | |
| })?; | |
| if len_bytes != expected_bytes { |
🤖 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/vector/segment/raw_f16_store.rs` around lines 62 - 65, The expected
sidecar byte-length calculation in raw_f16_store::map_file uses unchecked
multiplication on expected_halves, which can wrap in release builds and let an
invalid mapping through. Replace the len_bytes comparison with checked
arithmetic for the expected size, and return the same error path if the computed
byte length cannot be represented safely; keep the fix localized to map_file and
preserve the existing Mapped/as_slice length invariants.
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 7, 2026
…en idle (#233) * fix(persistence): TopLevel-monoio AOF writer honors EverySec bound when idle The TopLevel monoio AOF writer blocked on an UNTIMED rx.recv(), with its EverySec deadline check living only inside the batch-commit path. A batch written under appendfsync everysec gets no per-batch fsync, so if the client stopped writing right after a burst, the buffered bytes only became durable when the NEXT message happened to arrive — the 1s fsync bound was deferred indefinitely while idle. Exposure is host-crash-only: the per-batch flush() already reaches the kernel page cache, so a plain process kill (and therefore any kill-9 crash test) loses nothing — which is exactly why this can't be red/green tested from userspace and is instead validated by mirroring the audited PerShard pattern plus full AOF regression reruns. The loop now matches the PerShard writers line-for-line: - bounded rx.recv_timeout() on the wave-5 IdleWait ladder (50ms -> 250ms -> 1s; reset on message, escalate on empty timeout) - mark_pending() after an everysec batch is buffered without its fsync, pinning the poll at the 50ms floor until the fsync lands - the EverySec deadline check moves from inside the batch path to the END of the loop, so it runs on message AND timeout iterations — timeout wake-ups are what restore the ~1s bound while idle - clear_pending() when the proactive fsync succeeds, letting the idle cadence escalate again Also updates the IdleWait struct docs, which claimed "TopLevel monoio blocks on an untimed rx.recv() and needs none of this" — that was the documented follow-up from the wave-5 review, now closed. Validation: clippy clean on both feature sets; AOF regression suites rerun green under default (monoio) features — persistence lib tests, wal_group_commit, coordinator_local_leg_durability (incl. ignored), crash_matrix_per_shard_aof, aof_toplevel_multishard_refusal, aof_fsync_err_subscribe_ordering. Follow-up from the RSS/CPU wave-5 review (PR #232). author: Tin Dang * test(persistence): crash-matrix harness — pin SET reply, serialize server tests Two pre-existing harness defects in crash_matrix_per_shard_aof.rs surfaced while validating the TopLevel-monoio idle-fsync fix; both convert environmental conditions into bogus total-data-loss failures: 1. redis_set asserted only redis-cli's EXIT STATUS, which is 0 even for server ERROR replies. With the host disk under Moon's 5% diskfull guard, every SET replied "MOONERR diskfull: writes paused" yet the test sailed on and later reported "200 missing, 0 mismatched" — pointing at the recovery path instead of the guard. The helper now pins the reply to +OK and names the likely culprit (and the MOON_DISK_FREE_MIN_PCT=0 escape hatch) in the assert message. 2. The three server-spawning tests SPLIT-BRAIN when run in parallel (the libtest default): unique_port() hands out OS-sequential ephemeral ports, the other two tests offset by +1/+2, and moon's per-shard SO_REUSEPORT listeners bind an already-taken port WITHOUT an error — so one test's redis-cli traffic silently lands on another test's server, observed as rotating "200 missing" / "LLEN 0 expected 20" false alarms (which test fails depends on OS port allocation order). A shared static Mutex now serializes the three tests; costs ~seconds, kills the whole interference class. Validated: suite green under default parallel threads AND --test-threads=1, with MOON_DISK_FREE_MIN_PCT=0 exported (host disk currently 96% full — below the guard). author: Tin Dang --------- Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
TinDang97
added a commit
that referenced
this pull request
Jul 7, 2026
…ytes `CsrStorage::resident_bytes()` fed the shard's elastic memory budget but omitted three real allocations: the v5 `node_props`/`edge_props` blobs, the lazily-built `SegmentPropertyIndexes`, and the lazily-built `GraphHnsw` bridge -- all invisible to eviction accounting. It also double-counted the mmap-backed `row_offsets`/`col_indices`/`edge_meta`/ `node_meta` arrays for `CsrStorage::Mmap` segments as if they were heap allocations, even though the doc comment already claimed otherwise. Rewritten to follow the `RawF16Store` precedent (PR #232): mmap-backed sections are kernel page cache (reclaimable under memory pressure) and count as 0 for the `Mmap` variant; the same sections count in full for the `Heap` variant. `props_index`/`hnsw_bridge` are ALWAYS heap-owned once built (derived structures over the source data, never themselves mapped) and count in full for both variants -- read via `OnceLock::get` only, so `resident_bytes()` stays a cheap read that never triggers a lazy build. New tests in src/graph/csr/storage.rs: resident_bytes grows once after `property_index()` is built and again after the HNSW bridge is built (fixture carries real per-node properties and embeddings so both indexes have non-trivial content, not an accidental 0 -> 0 pass); a second test pins the Heap-variant invariant that the core CSR arrays count in full. author: Tin Dang
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 7, 2026
…timization waves (#231) * test(graph): red freeze-boundary suite — post-compaction data loss (Phase 0) Adds tests/graph_freeze_boundary.rs driving the real GRAPH.* command handlers with edge_threshold=8 so MemGraph::freeze_and_compact fires after 8 inserts. Documents the P0 found in the 2026-07 graph deep review (tmp/GRAPH-DEEP-REVIEW-2026-07.md): freeze() DRAINS the write buffer and CSR segments carry no properties, so compacted data is lost to queries. Red (9 failing / 1 passing mechanism-sanity): - MATCH label scan / property filter / RETURN prop lose frozen nodes - 1-hop expand from a frozen point query returns 0 rows - mixed-tier scan sees only the mutable tail - GRAPH.NEIGHBORS rejects frozen nodes ("node not found") - GRAPH.ADDEDGE between frozen endpoints fails (append-once flaw) - GRAPH.PROFILE returns 0 rows post-freeze - GRAPH.HYBRID loses compacted candidates These go green across Phase 0 (property blob v5, MergedNodeView, NEIGHBORS/PROFILE/HYBRID rewiring). author: Tin Dang * feat(graph): CSR v5 — persist properties, embeddings, edge weights across freeze Fixes the freeze-boundary data-loss P0 (storage half): CsrSegment::from_frozen wrote property_offset: 0 unconditionally, silently DISCARDING node properties, embeddings, edge weights, and edge properties when a graph crossed edge_threshold (64K edges). - src/graph/csr/props.rs: self-contained record codec (tagged prop entries + embedding dim + edge weight), offset+1 scheme so 0 keeps meaning "absent". Bounds-checked decoders never panic; fuzz target graph_props_record added to fuzz/ and both CI fuzz matrices. - Format v5: two trailing [len:u64][bytes] blob sections after label_overflow; header layout unchanged (positional parsing like v4). v1-v4 files load with empty blobs (graceful degradation). New unsafe: two mmap pointer-slice accessors following the module's existing validated-pattern (SAFETY comments; flagged for unsafe-policy review). - from_frozen + compact_segments now encode/carry records; the merge copies each winner's record verbatim from its winning segment. - Bonus fix: compact_segments left node_id_to_row + MPH EMPTY, so a freshly merged in-memory segment could not resolve any NodeKey (lookup_node → None) until persisted and reloaded. Rebuilt from external_id like from_bytes does. - parse_label_overflow now returns its end offset (v5 sections follow it). Tests: props codec unit suite (incl. truncation sweep), v5 roundtrip (from_frozen / bytes / mmap), v4-compat parse, merged-segment carry-over + NodeKey lookup, downgrade helpers fixed for v5 trailer. graph lib 424/424. Read-side wiring (NodeScan/eval/NEIGHBORS/HYBRID) lands next — the freeze-boundary integration suite stays red until then by design. author: Tin Dang * fix(graph): MergedNodeView — Cypher reads see frozen CSR tier; NodeKey collision fix Phase 0 task 3 of the graph-engine overhaul: post-freeze read correctness for Cypher MATCH / property eval, plus a newly discovered NodeKey aliasing bug in the freeze path. MergedNodeView (src/graph/view.rs): one seam over both tiers (mutable MemGraph write buffer + immutable CSR segments) — segment_row / contains / is_visible / property / properties / embedding / labels / for_each_visible_node. Frozen rows resolve properties through the v5 property blob, labels through label_bitmap + overflow, visibility through the new visibility::is_meta_visible (NodeMeta twin of is_node_visible, txn_id = 0 since frozen segments hold only committed data). Wired through: - Cypher NodeScan (execute + execute_profile): scans both tiers via for_each_visible_node — frozen nodes were invisible to MATCH before (freeze DRAINS the memgraph; the read side assumed a copy). - Expand target visibility: frozen targets now get the CSR NodeMeta MVCC/valid-time check instead of a silent free pass. - eval_expr PropertyAccess + labels(): merged-tier lookup, so RETURN n.prop / WHERE n.prop = x / labels(n) work on compacted nodes. NodeKey collision fix (P0, pre-existing): freeze_and_compact replaced write_buf with a FRESH MemGraph. A fresh SlotMap restarts key allocation at the same (index, generation) pairs, so the first node added after a freeze received the same NodeKey ffi as the first frozen node — aliasing new nodes onto frozen CSR rows in every merged lookup (SegmentMergeReader included). Fixed with MemGraph::thaw(): re-arm the SAME drained slot maps (drain bumps each slot's generation, guaranteeing fresh keys) and reset live counters; freeze_and_compact now thaws in place on both the success and CSR-build-failure paths. Freeze-boundary integration suite: 6/10 green (was 1/10) — label scan, property filter, property return, one-hop expand, mixed-tier scan. Remaining 4 red are the next tasks: NEIGHBORS/ADDEDGE existence + PROFILE Expand (task 4), GRAPH.HYBRID (task 5). Tests: graph lib suite 429/429; new red/green coverage: - store: test_freeze_and_compact_preserves_key_uniqueness (red on the fresh-MemGraph design, green with thaw) - view: two-tier fixture tests (contains / property+embedding / labels / merged scan with label filter) Also: /target-fast/ gitignored (release-fast iteration dir, matches the target-* convention). author: Tin Dang * fix(graph): cross-tier delta edges + NEIGHBORS/ADDEDGE/PROFILE/replay see frozen tier Phase 0 task 4: the native command surface and WAL replay now work across the freeze boundary, built on a new delta-edge mechanism in MemGraph. Delta edges (memgraph.rs): an edge whose endpoint was frozen into a CSR segment has no MutableNode to carry adjacency, and CsrSegment::from_frozen silently drops edges without in-segment endpoint rows. New design: - ghost_out / ghost_in maps hold adjacency keyed by frozen NodeKey; neighbors() serves them for non-resident nodes, so cross-tier edges are traversable from BOTH ends (SegmentMergeReader picks them up for free). - add_edge_across_tiers(): validates resident endpoints alive; trusts the caller (MergedNodeView::is_visible) for frozen ones. Plain add_edge keeps rejecting unknown endpoints. - freeze() now PARTITIONS edges: live + both endpoints resident → CSR; live + any earlier-frozen endpoint → retained in the mutable tier with its EdgeKey intact and ghost adjacency rebuilt; dead → dropped. freeze_and_compact skips empty freezes (all-delta buffer would otherwise churn empty segments) and thaw() recounts retained edges. Fixed call sites (each silently lost or rejected data post-freeze): - GRAPH.ADDEDGE: verifies frozen endpoints via MergedNodeView (alive in a segment), then inserts a delta edge — previously "ERR source or destination node not found". - GRAPH.NEIGHBORS: existence check via the merged view (comment claimed "freeze copies, doesn't move" — it drains); new DIRECTION IN|OUT|BOTH argument is parsed and honored (was hardcoded Both). - GRAPH.PROFILE Expand: now uses SegmentMergeReader + merged-view target visibility (parity with GRAPH.QUERY) — profiled expansions returned 0 rows on frozen graphs. - WAL replay AddEdge: node_map already seeded CSR-resident endpoints, but add_edge refused them, silently dropping every cross-segment edge during recovery; now inserts delta edges (node_map membership is the existence proof). Freeze-boundary suite: 10/11 green (was 6/10; +new DIRECTION test). Last red is GRAPH.HYBRID (next task). Graph lib 431/431, command::graph 25/25. author: Tin Dang * fix(graph): GRAPH.HYBRID/VSEARCH operate across both tiers — Phase 0 complete Phase 0 task 5 (last freeze-boundary correctness gap): the hybrid graph+vector executors (HYB-01 FILTER, HYB-02 EXPAND, HYB-03 WALK, HYB-04 RERANK) read only the mutable write buffer — post-freeze they rejected the start node (NodeNotFound) or silently scored an empty candidate set. - All four executors now take the immutable CSR segments alongside the MemGraph: traversal goes through SegmentMergeReader (frozen + delta edges), node existence and embeddings resolve through MergedNodeView (embeddings decode from the CSR v5 blob for frozen rows). bfs_collect / collect_context are segment-aware; RERANK's all-nodes scan enumerates both tiers with MVCC visibility. Pass `&[]` for a bare MemGraph. - GRAPH.VSEARCH and GRAPH.HYBRID handlers load the segment snapshot and thread it through. - extract_f32_vector: binary little-endian f32 fallback after the text form — the SAME blob format GRAPH.ADDNODE's VECTOR argument accepts, so vectors round-trip between ADDNODE and VSEARCH/HYBRID unchanged (previously HYBRID only accepted whitespace-separated text and answered "ERR invalid vector" to its own storage format). Freeze-boundary suite: 11/11 GREEN (was 1/10 at review time) — Phase 0 exit criterion met. Graph lib 431/431. author: Tin Dang * perf(graph): per-segment property indexes (lazy, freeze-time data); delete dead insert-time index Phase 1 task 6 — the storage half of the property-index lever identified by the 2026-07 deep review (point query = 99% unindexed label scan; FalkorDB wins reads on exactly this). SegmentPropertyIndexes (index.rs): per-CSR-segment indexes over row ids, built lazily on first use from the v5 node-property blob (OnceLock, same pattern as the incoming-edge index — one code path covers from_frozen, compact, from_bytes, and mmap reload). Numeric values (Int/Float/Bool) share a per-property B-tree (equality + range via the existing PropertyIndex); String/Bytes index by xxh64 hash (equality). Hash collisions and Bool/Int aliasing only ever produce SUPERSET candidate sets — the planner keeps its residual Filter, so collisions cost a re-check, never a wrong row. The build is exhaustive over segment rows, so lookups return empty bitmaps for absent values (no fall-back-to-scan signal needed; pre-v5 segments genuinely hold no properties). Accessor: CsrStorage::property_index() (heap + mmap variants). Deleted the dead insert-time maintenance: NamedGraph.property_indexes was populated on every ADDNODE with `ext_id as u32` — TRUNCATED external ids, a row space nothing could resolve — and no query path ever read it. Field, initializer, and the per-ADDNODE update loop are gone (the _key registration that lived in the same loop is preserved). Next (task 7): PhysicalOp::IndexScan consumes these via label-bitmap ∧ property-bitmap per segment. Tests: graph lib 432/432; new test_segment_property_index_eq_and_range covers Int/Float equality on one B-tree, string hash equality, range, and absent-property emptiness. author: Tin Dang * perf(graph): PhysicalOp::IndexScan — point queries via per-segment property indexes Phase 1 task 7, the read-side half of the property-index lever (probe showed Cypher point queries spend ~99% of their time in the unindexed label scan; this is FalkorDB's entire read advantage). Planner: a pattern node whose inline properties are all literals or parameters (`(n:L {k:3})`, `(n:L {k:$v})`) now plans as PhysicalOp::IndexScan { variable, label, prop_eq } instead of NodeScan — emitted for the scanned first node and shortestPath endpoints (shared push_node_scan helper). The full residual Filter is ALWAYS kept immediately downstream: index lookups have superset semantics (string hashes can collide, Bool/Int alias numerically), so the index only prunes, never decides. Executor (index_scan_keys, both execute and execute_profile): - prop_eq values resolve per run via eval_expr (parameters index-hit under the plan cache); an unresolvable value degrades to the merged label scan. - Frozen tier: per segment, property bitmaps intersect (short-circuit on empty) ∧ label bitmap, then MVCC/valid-time visibility per surviving row. - Mutable tail: linear scan with a superset-consistent loose-equality check (numeric coercion mirrors the index) — bounded by edge_threshold. - Write-path executor treats IndexScan as its NodeScan (mutable-tier MATCH semantics unchanged; residual Filter keeps it exact). - GRAPH.PROFILE reports the op as "IndexScan". Planner contract tests updated: literal inline props now pin IndexScan+Filter (the v3-2 "Filter immediately follows the binding op" contract is preserved, the binding op changed); new emission tests cover literal / parameter / multi-prop / no-prop cases. Known scope cut: WHERE-clause equality (`WHERE n.k = 3`) still plans as NodeScan+Filter — extracting index-eligible conjuncts from WHERE is a follow-up. Tests: graph lib 433/433; freeze-boundary 11/11 (point-query tests now exercise IndexScan end-to-end); tokio cypher suites green (inline-filter, shortest-path, txn-write-rollback). author: Tin Dang * perf(graph): plan-cache auto-parameterization + LRU + single-parse dispatch Phase 2 task 8. The plan cache was keyed on the raw query bytes, so the dominant graph workload — point queries differing only in literal values — NEVER hit it (every GRAPH.QUERY re-parsed and re-compiled). Eviction was also arbitrary (HashMap keys().next()), and the server routing path parsed every Cypher query TWICE (is_cypher_write_query full parse + handler parse). Auto-parameterization (new src/graph/cypher/parameterize.rs): value literals (int/float/string) are rewritten into synthetic parameters ($__p0, $__p1, …) at the TOKEN level, with values extracted into the params map — so `{id: 3}` and `{id: 4}` share one cached plan and still index-hit (PhysicalOp::IndexScan resolves Expr::Parameter per run). Structural literals stay literal because they are baked into the plan, not evaluated: variable-length hop bounds (prev token `*`/`..` or next `..`), LIMIT/SKIP counts (conservative pin), and leading-minus literals (logos maximal munch lexes `x-1` as Ident+Integer(-1); rewriting would swallow a binary minus). Fail-open by construction: lex error, non-UTF-8 string, unparsable number, or a pre-existing `$__p` name returns None and the caller falls back to raw-text raw-hash caching; if normalized text ever failed to parse, parse_effective re-parses the raw text so user errors reference their own query. Wrong results are impossible — only a missed cache share. Plan cache: proper LRU (monotonic access tick, evict min on overflow; O(capacity) scan only when a full cache takes a new query text) and a new invariant — only READ-ONLY plans are inserted (graph_query previously cached write plans compiled for its error path). Single-parse dispatch, enabled by that invariant: - Cache HIT now runs with ZERO parse/compile work (previously: parse + hash + lookup). The hit path routes read-only directly since only read plans are cached. - is_cypher_write_query: full parse → token scan for CREATE/DELETE/SET/ MERGE. May false-positive (property named `set`) which only routes via the write-capable handler; graph_query_or_write AST-classifies and runs the read path correctly. Never false-negatives for a parsable write. - GRAPH.RO_QUERY: parsed twice before (own classify parse + delegated graph_query parse); now shares graph_query_readonly (single parse, and zero on cache hit — a hit is read-only by the invariant). - Write path merges auto-params before execute_mut (CREATE literals are normalized too; without the merge they would store Nulls). Behavior notes: RO_QUERY of a write query against a MISSING graph now reports "graph not found" instead of the write-clause error (graph lookup precedes parse in the shared core); GRAPH.EXPLAIN/PROFILE stay raw-parse (stable output, no cache). Tests (red→green): plan-cache sharing across int and string literal variants with per-query correct results, write-plan non-caching, LRU eviction order, var-length hop bounds pinned to distinct plans (wrongly parameterizing hops would return wrong row counts — pinned 5 vs 6 rows), 11 parameterize unit tests incl. normalized-text-reparses. Suites: lib 3800 green, graph_freeze_boundary 11/11, tokio cypher suites (inline-filter, shortest-path, txn-write-rollback) green, clippy both feature sets, fmt. author: Tin Dang * perf(graph): slot-indexed executor rows — kill per-row HashMap allocation Phase 2 task 9. Every executor row was a HashMap<String, Value>: each scan/expand/unwind emission allocated a fresh hasher table AND cloned the variable name String per binding; row clones re-hashed everything. For multi-row pipelines (the normal case) this dominated per-row cost. Row is now a slot-indexed SmallVec<[Value; 4]> plus a reference to a per-execution SlotTable (all variables the plan can bind, collected once by walking the binding operators: scans, expands, unwind, shortestPath, create/merge patterns). Name→slot resolution is a linear scan over a handful of short names — cheaper than hashing at this cardinality. The public row API is shape-compatible (get/insert/iter), so eval_expr's signature is unchanged; insert now takes &str, deleting the per-binding String clone. Semantics: unbound slots hold Value::Null, which every consumer already treats identically to the old HashMap miss — expression eval defaulted a missing variable to Null, and operator sources pattern-match a concrete variant (Some(Value::Node(_))), which Null fails exactly like None did. Binding is uniform per operator (each binding op binds its variable on every row it emits), so the no-Project column set (SlotTable names, sorted) and RETURN * maps match the old bound-keys view. Referenced-but-never-bound names resolve to no slot (None), matching a HashMap miss; a bound name missing from the table would be a plan/table desync and is debug_assert'ed. Tests: SlotTable collection (match + create/merge pattern vars), Row get/insert/iter/clone semantics incl. unbound-vs-unknown distinction. Suites: lib 3802 green, graph_freeze_boundary 11/11, tokio cypher suites green, clippy both feature sets. author: Tin Dang * perf(graph): write-path + traversal allocation cleanups Phase 2 task 10 — the deep review's allocation-discipline findings (NEIGHBORS profile: ~25% of time dropping Frames, ~20% in the ALLOCATING SegmentMergeReader::neighbors; ADDNODE 2× WAL-encode). - ADDNODE double WAL serialize deleted: a full record (incl. embedding bytes) was built with a placeholder id, dropped unread, and rebuilt from the stored node. Now a single serialization from the stored node's refs. - wal.rs: 15 `itoa::Buffer::format().to_owned()` sites drop the String — the formatted &str is passed straight to write_bulk (statement-temporary pattern already used by write_bulk itself). format_f64's format!() is replaced by ryu (already a dep) for the edge weight and Float property sites; replay parses f64 so "1.0"-vs-"1" style differences are inert. - New src/graph/fasthash.rs: FxHash multiply-fold (rustc's internal hasher, inlined — no new dependency). NodeKey/EdgeKey are slotmap PODs in maps we control (visited sets, dist maps), where SipHash's DoS resistance buys nothing. Module docs pin the provenance contract: never key by attacker-controlled bytes. - traversal.rs: all non-test NodeKey sets/maps → FxHashSet/FxHashMap; Dijkstra's dist/prev/depth map TRIPLE merged into one FxHashMap<NodeKey, DijkstraNodeState> — one hash lookup per relaxation where three (×SipHash) used to run. - Cypher Expand (both executors): the allocating `neighbors()` (fresh HashSet+Vec per source node — worst in variable-length BFS, per frontier node) replaced with `neighbors_into` on scratch hoisted per operator; BFS visited sets → FxHashSet. Deferred (follow-up, noted in review doc): Value::String(String)→Bytes to stop value_to_frame cloning string cells — ripples through all of eval's string ops, separate change. Tests: fasthash unit tests (map/set behavior, 10k-distinct-hash sanity); suites: lib 3804 green, graph_freeze_boundary 11/11, tokio cypher suites green, clippy both feature sets. author: Tin Dang * perf(graph): CSR row-space BFS fast path — parallel levels + direction-optimizing pull Phase 3 task 11. BFS on a fully-frozen graph paid NodeKey hashing on every edge probe (visited set), MPH lookups per hop, and — in the old "ParallelBfs" — fake parallelism: the reader borrows !Send MemGraph, so neighbor expansion ran sequentially (with a per-node nb_buf.clone()) and worker threads only did DashSet merging. New src/graph/row_bfs.rs, gated per query (mutable tier absent-or-empty ∧ exactly one CSR segment ∧ segment visible at the snapshot — every case the reader path handles generically is a reason to fall back, never a behavior fork; BoundedBfs/ParallelBfs::execute try the gate first): - Row-space expansion: rows are dense u32s — visited set is a bitmap, adjacency is row_offsets/col_indices slice walks, NodeKey materialization happens once per RESULT row instead of per edge probe. - TRUE parallel levels: CsrStorage is Send+Sync, so frontier chunks expand on worker threads (thread::scope) against a shared AtomicU64 visited bitmap (fetch_or test-and-set); level triggers at frontier ≥ 256, ≤ 8 workers. - Direction-optimizing BFS (Beamer α=14/β=24): when the frontier's out-edge count dwarfs the unexplored remainder, the level switches to bottom-up pull over the v3-2 IncomingIndex (each unvisited row scans its in-edges for a frontier parent; rows partitioned per worker). Outgoing direction only; the lazy incoming index is warmed outside the worker scope. Semantics parity with the reader path: same edge-validity bitmap, same edge-type filter, same deleted_lsn node visibility, same MergedNeighbor materialization for CSR edges (placeholder edge key, weight 1.0, segment-LSN timestamp). Within a parallel level, discovery ORDER is unspecified (visited set + depths deterministic). Tests (7): push parity vs a hand-rolled reader-path oracle across all 3 directions × filters; pull-vs-push equality (incl. edge-type filter); parallel-level parity on a 2000-node star; gate fallback (mutable tier non-empty / two segments / future segment); deleted-node exclusion parity; depth limit + frontier cap; synthetic-key NodeNotFound (a key from a fresh MemGraph would ALIAS frozen rows — the task-3 collision class — so the test uses an out-of-range synthetic key). Suites: lib 3811 green, graph_freeze_boundary 11/11, tokio cypher suites green, clippy both feature sets. author: Tin Dang * feat(graph): TraversalGuard in Cypher multi-hop ops + real hybrid HnswPreFilter Phase 3 task 12 — the deep review's last two items. TraversalGuard threading (bounded epoch hold, wall-clock): - ExecutionContext gains `guard: Option<TraversalGuard>` (None default — Default/unit-test behavior unchanged) and ExecErrorKind gains Timeout(TraversalTimeout). - Both read executors (execute + execute_profile) check the guard once per hop in variable-length Expand BFS and once per row before each ShortestPath run; GRAPH.QUERY/RO_QUERY/PROFILE create the guard with the default 30s budget. The write executor is untouched: it has no ExecutionContext and walks the mutable tier only (hop-capped), so the epoch-hold risk the guard bounds — long traversals pinning old CSR snapshots — doesn't apply. Hybrid HnswPreFilter (was a silent stub — select_strategy picked it at >= 10K candidates, execute brute-forced anyway): - New src/graph/hnsw_bridge.rs: GraphHnsw builds the vector engine's HnswGraph (HnswBuilder, m=16/efC=128, deterministic seed) over a CSR segment's v5 embeddings with raw-f32 cosine distances — no TurboQuant. Embeddings copied once, unit-normalized (scores identical to simd::cosine_similarity). Search = greedy upper descent + L0 ef-beam (ef = max(64, 4k)), allow-filter applied to the collected beam. - Lazy per-segment build via CsrStorage::hnsw_bridge (OnceLock, same pattern as incoming/property indexes), gated at >= 4096 embeddings; the negative answer is cached. In-memory only, never persisted. - GraphFilteredSearch::execute now honors the strategy: CSR-resident candidates route through their segment's bridge (group top-k with exact cosines), everything else — mutable tier, bridge-less rows, dim mismatch, and any group whose approximate beam under-fills k — falls back to exact scoring. Never silently truncates. Tests: guard timeout red/green for var-length Expand + shortestPath (both executors) with generous-guard parity; bridge build/recall (>= 8/10 vs brute force, exact-score parity), allow-filter, min-vectors gate, degenerate queries, unembedded-row skip; hybrid HnswPreFilter vs brute-force overlap + bit-exact fallback without a bridge + tier partition contract. Suites: lib 3821 green, graph_freeze_boundary 11/11, tokio cypher suites green, clippy both feature sets, fmt. author: Tin Dang * docs(changelog): graph engine overhaul entry under [Unreleased] CHANGELOG entry covering the P0-P3 graph deep-review implementation (freeze-boundary correctness, point-query indexes, plan-cache + executor cost, row-space BFS + TraversalGuard + HNSW bridge), per the CI Lint changelog gate. author: Tin Dang * fix(graph): prevent NodeKey aliasing across restart (P0) `slotmap::SlotMap` key allocation is deterministic: a fresh `MemGraph` always mints SlotMap index 0 / generation 1 first. CSR segments persist `NodeMeta::external_id` as the pre-crash process's raw NodeKey FFI bits. `recover_graph_store` previously re-seeded the post-recovery mutable tier with another fresh, unoffset `MemGraph`, and `replay.rs`'s AddNode replay loop called `mg.add_node()` unconditionally for every replayed record. The first brand-new node replayed after a restart could therefore mint the exact same key as a loaded CSR segment's first frozen node -- `MergedNodeView` checks the mutable tier first, so the frozen node's properties would be silently and permanently shadowed by the replayed node's properties. Fix is two-part: - `MemGraph::with_id_offset` translates every public NodeKey by a fixed offset at the mutable/internal boundary. `recover_graph_store` seeds the post-recovery mutable tier with a watermark one past the largest persisted `external_id` across all loaded segments, so freshly-minted keys can never numerically alias a loaded segment's rows. Zero extra memory (no dummy insert/remove cycle, which would both leak an ever-growing SlotMap backing Vec and remain unsound against arbitrarily bumped pre-crash generations). - `replay.rs`'s AddNode loop now seeds its `node_map` from every loaded CSR segment's node_meta before replaying, and skips `mg.add_node()` for any `node_id` already resident there -- handling full-history replay (AOF fold / WAL truncation left a pre-freeze record in place) without double-enumerating the same logical node. Also updates `MergedNodeView`'s doc comment (src/graph/view.rs) to describe how the "NodeKey lives in exactly one tier" invariant is actively enforced across restart, not merely automatic the way it is for in-process freeze/thaw. Added tests/graph_restart_id_aliasing.rs (red/green, confirmed failing against the pre-fix code via git stash before this commit): asserts no replayed key aliases a loaded segment's external_id, that a frozen node's properties are never shadowed by a replayed node, and that replaying an already-frozen node's record doesn't double-enumerate it. Known follow-up (not in scope here, documented for a separate fix): `NamedGraph.write_buf` -- not `NamedGraph.segments.mutable` -- is the tier every live command dispatch/read path actually consults. Nothing currently reconciles `segments.mutable` (which recovery/replay populate) back into `write_buf`, so today a restart does not expose replayed post-freeze WAL mutations to live queries at all. This fix protects the `segments`-based tier exactly as specified and matches replay.rs's own pre-existing test contract; the write_buf/segments reconciliation gap is a distinct, likely more severe issue requiring its own design pass. author: Tin Dang * perf(graph): raw-hash plan-cache fast path, cache SlotTable `normalize_cypher()` -> `parameterize()` ran unconditionally before the plan-cache lookup on every GRAPH.QUERY -- a full lexer pass plus allocations even for an exact-repeat query -- and `SlotTable::from_plan` was rebuilt from scratch on every execution regardless of cache hit. `PlanCache` now supports dual-keyed inserts: a raw query-bytes hash (exact-repeat fast path, checked first, skipping `parameterize()` entirely on hit and replaying that text's own auto-extracted params) and the pre-existing literal-normalized hash (shared across every query that differs only in literal values), both pointing at one shared `Arc<PhysicalPlan>` + `Arc<SlotTable>` so a hit never rebuilds the slot table. `graph_query_readonly`/`graph_query_or_write` check the raw hash first, fall back to the normalized hash on raw miss, and only compile + `insert_both()` on a full miss. Cache storage switches from `std::HashMap` to the in-repo `FxHashMap` (keyed by an already-xxhashed u64 digest -- never raw attacker bytes -- and bounded by `max_entries` LRU eviction, so the documented "not DoS-resistant" caveat on `FxHashMap` does not apply here). `PlanCache::len()` now reports raw hash-key-slot count (what `max_entries`/LRU actually bounds); added `distinct_plan_count()` for "how many distinct compiled plans are cached" -- the metric the existing literal-sharing correctness tests in src/command/graph/mod.rs actually want, now that a single distinct plan can occupy two hash-key slots. New unit tests: exact-repeat raw-hash hit, string/int literal-variant sharing via the normalized hash, same-hash dedup when a query has no literals to extract, and LRU eviction correctness under dual keys. author: Tin Dang * perf(graph): count v5 property blobs + lazy indexes in CSR resident_bytes `CsrStorage::resident_bytes()` fed the shard's elastic memory budget but omitted three real allocations: the v5 `node_props`/`edge_props` blobs, the lazily-built `SegmentPropertyIndexes`, and the lazily-built `GraphHnsw` bridge -- all invisible to eviction accounting. It also double-counted the mmap-backed `row_offsets`/`col_indices`/`edge_meta`/ `node_meta` arrays for `CsrStorage::Mmap` segments as if they were heap allocations, even though the doc comment already claimed otherwise. Rewritten to follow the `RawF16Store` precedent (PR #232): mmap-backed sections are kernel page cache (reclaimable under memory pressure) and count as 0 for the `Mmap` variant; the same sections count in full for the `Heap` variant. `props_index`/`hnsw_bridge` are ALWAYS heap-owned once built (derived structures over the source data, never themselves mapped) and count in full for both variants -- read via `OnceLock::get` only, so `resident_bytes()` stays a cheap read that never triggers a lazy build. New tests in src/graph/csr/storage.rs: resident_bytes grows once after `property_index()` is built and again after the HNSW bridge is built (fixture carries real per-node properties and embeddings so both indexes have non-trivial content, not an accidental 0 -> 0 pass); a second test pins the Heap-variant invariant that the core CSR arrays count in full. author: Tin Dang * docs(changelog): record PR #231 pre-merge review fixes Adds the three review-gated fixes (restart NodeKey aliasing P0, plan- cache raw-hash fast path, CSR resident_bytes accounting) to the graph engine overhaul entry under [Unreleased]. author: Tin Dang --------- Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wave 5 of the RSS/CPU remediation line (task #20). Implemented by a Sonnet senior-rust-engineer agent, then deep-reviewed commit-by-commit with independent test reruns and red-toggle verification; one accounting bug found in review and fixed here (last commit).
Item A — mmap the exact-rerank f16 sidecar on segment reload (
RawF16Store)Reloaded segments used to
fs::readthe wholeraw_f16.bininto a second heapVec<u16>— doubling vector sidecar RSS on warm starts. Reload now memory-maps the sealed sidecar (zero-copy&[u16], kernel page cache); freshly-built segments keep their owned buffer. Rerank parity Owned-vs-Mapped pinned by test (red-toggle verified). Two newunsafeblocks (Mmap::map+from_raw_partsreinterpret), both SAFETY-commented, mirroring the auditedgraph::csr::mmappattern — user-approved pre-PR. Bundled: text posting-listshrink_to_fit()once a term's last live doc leaves.Item B — idle-adaptive AOF writer wake cadence (
IdleWait)The writer loops woke 5–20×/s forever even when idle. New escalation ladder 50ms→250ms→1s, reset on any message; escalation is refused (
mark_pending) whenever unflushed EverySec bytes or a back-dated fsync deadline exist, so the everysec durability bound is untouched. Wired into all three timeout-bounded loops (TopLevel-tokio, PerShard-tokio, PerShard-monoio). Verified: crash_matrix suites, wal_group_commit 13/13, coordinator_local_leg_durability 7/7 (agent) + wal_group_commit/coordinator/crash-s1 re-run independently (reviewer).Hygiene batch
shrink_to(8KB)after an oversized flush (one giant record no longer pins peak capacity).SmallVec<[usize;16]>for the per-100ms-tick elastic-budget snapshot (stack-only ≤16 shards); >16-shard spill correctness pinned by test.moon_memory_bytes{kind="lua_scripts"}+ MEMORY DOCTOR (cache stays unbounded, Redis parity).parse_single_frame_zcparser (~150 lines; zero external callers, superseded byparse_frame_zerocopy— verified viagit grepon main)._RJEM_MALLOC_CONFoperator docs added to CLAUDE.md.READY_TIMEOUT60s).Review fix (reviewer commit)
ImmutableSegment::resident_bytes()charged a Mapped sidecar at full size — mapped pages are reclaimable page cache, so this silently negated the RSS win inside the elastic-budget/eviction accounting.RawF16Store::resident_bytes()(Owned = full, Mapped = 0), pinned by unit + integration asserts. Plus three stale cadence comments corrected.Test plan
cargo test --lib: 3782 passed (default), 3125 passed (runtime-tokio,jemalloc)-D warningsclean on both feature sets;cargo fmt --checkcleanFollow-ups (documented, not in scope)
recv()— everysec fsync deferral while idle is a host-crash-only exposure (batches flush to page cache); pre-existing.aof_fsync_err_subscribe_ordering::_multi_shardflake confirmed pre-existing via stash A/B.Summary by CodeRabbit
New Features
Bug Fixes
Documentation