perf(vector): SIMD SQ8 ADC + exact rerank + churn reliability fixes (deep-review bundles 1-5)#214
Merged
Merged
Conversation
Seven verified findings from the 2026-07-05 vector deep review (tmp/VECTOR-DEEP-REVIEW.md), all mechanical and behavior-preserving: - QP-1: Arc-wrap VectorIndex.key_hash_to_key. The previous owned HashMap (one entry per indexed vector) was deep-cloned on EVERY dense-KNN query snapshot and SESSION/RANGE/hybrid path just to resolve k result keys. Capture is now an O(1) Arc bump; writers copy-on-write via Arc::make_mut (clones at most once per in-flight snapshot, only under concurrent read+write). - VEC-3: delete dead per-insert work. handle_vector_insert(_field) and VectorStore::insert_vector heap-allocated an SQ8 quantization (vec![0i8; dim]) and computed a norm that append()/append_transactional() declared as unused `_vector_sq`/`_norm` params and re-derived internally. Params removed from the signatures; all callers swept. - QP-4: hoist brute-force query prep out of the yield-chunk loop. The chunked mutable-segment scan re-allocated its top-k heap and re-derived the FWHT query rotation (or SQ8 normalize) every 1024-entry chunk. New BruteForceQuery ctx (prepare_brute_force_query + brute_force_scan_mvcc_chunk) prepares once per query and shares one heap across chunks; the sync full-scan API delegates to the same code, so results stay identical. - XC-5: stripe VECTOR_SEARCH_TOTAL / VECTOR_TOTAL_VECTORS. Global fetch_add counters on every FT.SEARCH/insert ping-pong one cache line across shard threads; now 16 padded stripes with a thread-local slot, summed only on INFO reads. - HQ-4: implement aarch64 prefetch_node. The 2-hop dual prefetch was a documented no-op on ARM (Moon's primary target); now stable inline-asm PRFM PLDL1KEEP mirroring the x86_64 hint pattern (2 neighbor lines + 3 code lines). SAFETY: PRFM never faults; addresses built with wrapping_add so pointer::add's in-bounds contract is not invoked. - VEC-5: fix latent code_len for SQ8 at compaction.rs:554 (missing is_sq8 branch present at the other two sites; was dim+4 instead of dim, masked only by a codebook-None early-continue). - HQ-6: skip the per-row normalize+FWHT sub-centroid sign prep for SQ8 during compaction — both inner branches unconditionally `continue` for SQ8, so the O(n·d log d) rotation work was computed and discarded. Validation: cargo fmt; clippy clean (default + runtime-tokio,jemalloc); 854 vector lib tests + 281 vector_search command tests + 41 integration tests (edge cases, stress, segment merge, warm e2e, memory audit, insert bench) all pass in release. Refs: tmp/VECTOR-DEEP-REVIEW.md (Bundle 1) author: Tin Dang
Five verified defects from tmp/VECTOR-DEEP-REVIEW.md, each TDD'd red -> green: - VEC-1 update duplication: HSET on an already-indexed key appended a second copy instead of replacing. Insert path now tombstones the old copy first: O(1) fast path via key_hash_to_global_id + mark_deleted_if_key when the old copy lives in the mutable segment, scan fallback (mark_deleted_by_key_hash across mutable + immutable segments) otherwise. Field-segment inserts use the scan path (no gid map there). Red tests reproduced 2-vs-1 and 6-vs-5 duplicate counts; green after fix (tests/vector_update_tombstone.rs). - XC-SHARD-1 FT.INFO under-report: at shards=N, FT.INFO answered from the local shard only (~1/N of num_docs). Added scatter_ft_info (coordinator SPSC scatter modeled on scatter_invalidate_range) + merge_ft_info_responses (additive top-level counters, per-field merge keyed by field_name, error propagation), wired in both the sharded and monoio handlers. Unit tests in ft_info.rs. - XC-3 filtered-search strategy on yielding path: the cooperative-yield search ignored FilterStrategy and always graph-filtered. Snapshot now carries the selectivity-picked strategy; HnswPostFilter searches unfiltered with fetch_k = 3k / ef >= 3k and post-filters by bitmap, matching the non-yielding path. - VEC-4 merge recall gate: background GraphUnion tolerance (0.70) is now a per-index knob, FT.CONFIG SET/GET MERGE_RECALL_TOLERANCE (validated 0.0..=1.0, default unchanged). Raising the default was rejected: merge-abort has no backoff, so a tighter default would spin futile merge retries on small indexes. - VEC-7 fail-loud stub: FT.CREATE ... MERGE_MODE KEEP_RAW silently fell back to graph-union semantics; now rejected with an explicit error until the raw-vector sidecar exists. The separate KEEP_RAW ON boolean flag is untouched (round-trip test still passes). Docs: CLAUDE.md vector section corrected — immutable segments DO merge (GraphUnion default, threshold 16 / 20% dead, recall-gated); the forbidden thing is decode+re-encode. FT.INFO cross-shard invariant and the new MERGE_RECALL_TOLERANCE knob documented. Refs: tmp/VECTOR-DEEP-REVIEW.md (VEC-1, XC-SHARD-1, XC-3, VEC-4, VEC-7) author: Tin Dang
MEMORY DOCTOR's "DashTable + entries" line (and the Prometheus KV memory gauge) read ShardDatabases::memory_per_shard, but the publish on the 100ms eviction tick was gated on `rt.maxmemory > 0` since the GAP-1 elastic-budget wiring (7329b18, 2026-06-10). Under the default unlimited maxmemory the atomics stayed at 0 forever, so MEMORY DOCTOR attributed 100% of RSS to allocator overhead regardless of dataset size. The publish is an O(1) accumulator read per database (Database:: estimated_memory returns the used_memory field) plus one Relaxed store, so it now runs unconditionally on the tick. Elastic-budget recompute still requires a finite cap and stays gated. Caught by tests/memory_doctor_response.rs, which asserts DashTable > 0 after loading 1000 keys. The test predates the regression (2026-04-28) but self-skips in CI (needs a prebuilt release binary + redis-cli), so the breakage was invisible for three weeks; red -> green locally with this fix. author: Tin Dang
SQ8 asymmetric distance previously ran a scalar per-element loop
(query[i] - (min + code[i]*scale))^2 for every beam-search candidate.
Decompose it algebraically so the per-candidate pass is pure
widen+FMA reductions that vectorize cleanly:
d = SUM(q_i - min - s*c_i)^2
= (SUMq^2 - 2*min*SUMq + n*min^2) <- per-QUERY, once
- 2s*(SUM(q*c) - min*SUMc) + s^2*SUMc^2 <- per-candidate stats
Per-candidate work is exactly three running sums (SUM(q*c), SUMc,
SUMc^2) computed together in one pass: sq8_candidate_stats_scalar is
the portable reference; NEON / AVX2+FMA / AVX-512F kernels widen 16-32
u8 codes per iteration (u8->f32 exact for 0..=255) and are dispatched
through a new DistanceTable::sq8_stats fn pointer resolved ONCE per
query. sq8_l2_from_stats / sq8_ip_from_stats do the O(1) combine.
Wired into: HNSW beam search (both dist closures in
hnsw_search_filtered) and both mutable-segment brute-force paths; the
chunked MVCC scan carries (SUMq, SUMq^2) in BruteForceQuery so they
are computed once per query across all yield chunks, over the PREPARED
(normalized) query buffer the kernel actually sees.
Criterion (aarch64 NEON, dev host; per-candidate distance):
128d: 39.8ns -> 14.5ns (2.74x)
384d: 100.3ns -> 54.4ns (1.84x)
768d: 190.7ns -> 120.5ns (1.58x)
Known tradeoff (measured): the decomposition WITHOUT SIMD is ~1.65x
slower than the naive loop (three sums vs one), so the scalar-fallback
tier (pre-AVX2 x86_64, ~2012-era) regresses; all supported targets
(aarch64 NEON baseline, x86_64 AVX2/AVX-512) are wins. An SSE2
sq8_stats kernel is a possible follow-up if pre-AVX2 ever matters.
Verification (math re-derived and reviewed line-by-line, kernels
audited for lane alignment/bounds/tail handling): parity tests pin
scalar-vs-SIMD and stats-vs-naive at dims 1..768 including SIMD-width
tails and negative-min / extreme-scale quantization params; dispatched
end-to-end test reproduces naive ADC to 1e-3 relative. Full release
suite + tokio feature set + clippy both feature sets green.
Implemented with a senior-rust-engineer subagent (worktree); math,
SIMD lane layout, and integration independently verified and the
mutable-segment wiring ported onto the BruteForceQuery chunked-scan
structure by hand.
Refs: tmp/VECTOR-DEEP-REVIEW.md (HQ-2)
author: Tin Dang
Compacted segments ranked candidates purely by quantized ADC distance: SQ8 got ZERO refinement and TQ4 only estimator-level refinement — the main recall gap vs engines that keep full-precision vectors (deep-review HQ-1; RediSearch comparison 0.86 vs 0.96 R@10 at MiniLM 384d). Design: - New src/vector/f16.rs: hand-rolled IEEE 754 binary16 conversion (round-to-nearest-even, subnormals, overflow->inf, NaN — each pinned by unit tests) — no new dependency. - MutableSegment now retains an f16 copy of every inserted vector in BOTH build modes (unlike raw_f32, which is Exact-only) at 2*dim bytes/vector; freeze() passes it through FrozenSegment. - compact() hands the immutable segment its sidecar by BFS permutation alone (no re-encode); GraphUnion merge propagates sidecars all-or-nothing (a partial sidecar would silently mix exact and ADC distances within one segment). - ImmutableSegment::search/search_filtered re-score the top 4*k ADC-ranked beam candidates from the sidecar before top-k truncation (beam results arrive nearest-first, so the prefix is the oversample set). Conventions match the quantized paths for merge consistency: L2 -> true squared L2; Cosine/InnerProduct -> normalized-pair squared L2 (2 - 2*<q,x>/|x|). The TQ_prod estimator rerank is skipped when a sidecar is present. - Persistence: raw_f16.bin (u16 LE) per segment directory; a missing or size-mismatched file means "no sidecar" — pre-sidecar segment dirs load unchanged and degrade to ADC distances (fail-soft), old readers ignore the extra file. - resident_bytes()/MEMORY DOCTOR account the sidecar. Memory cost: +2*dim B/vector in mutable and compacted segments (384d = +768 B/vector). Opt-out knob deferred as follow-up. TDD: tests/vector_exact_rerank.rs written red-first — L2/Cosine/TQ4 distance-exactness (SQ8 ADC error measured ~1.7-2.7e-3 rel, TQ4 up to 18.6%; post-fix within f16 tolerance), recall@10 guard, persistence roundtrip, legacy-dir fallback, merge preservation. 7/7 green; full release suite + tokio feature set + clippy both feature sets green. Refs: tmp/VECTOR-DEEP-REVIEW.md (HQ-1) author: Tin Dang
The Bundle-5 soak diagnostic (scripts/vector-validate.py) caught deleted keys resurfacing in FT.SEARCH: after one minute of mixed churn at --shards 1, 40/200 KNN results were DELETED keys and live-set recall collapsed 0.985 -> 0.735. Root cause is the three-dispatch-paths gap: the vector auto-delete hook (VectorStore::mark_deleted_for_key) existed ONLY on the cross-shard SPSC Execute arm and the tokio sharded handler. Missing on: - handler_monoio conn-local writes (the default runtime's ONLY path at shards=1 — every DEL was invisible to the index) - handler_single direct + MULTI/EXEC paths - shared.rs MULTI batch path - SPSC PipelineBatch + PipelineBatchSlotted arms All six sites now call a single parity helper, auto_delete_vectors (spsc_handler.rs, next to auto_index_hset_public): every path that runs HSET auto-index runs the DEL/UNLINK tombstone. Arms with neither hook (ExecuteSlotted, MultiExecute*) can't resurrect — keys they touch were never auto-indexed — and are left as-is. TDD: tests/vector_del_unindex.rs written red-first, wire-level (spawns real servers; store-level tests cannot catch dispatch wiring): DEL, UNLINK, MULTI/EXEC DEL at shards=1, pipelined DEL at shards=4 — all 4 resurrected before the fix, green after. Soak re-run: resurrections 40 -> 0, live-set recall recovered. Refs: Bundle-5 reliability validation (vector deep review follow-on) author: Tin Dang
The Bundle-5 churn soak (60% search / 25% update / 10% insert / 5% delete over the wire) lost 32% of live keys from the vector index in one minute: self-recall probes missed 1054/3262 keys whose hashes still existed in KV, and live-set recall collapsed 0.985 -> 0.685. A/B on the identical op sequence against v0.5.1 (LOST=0) proved a branch regression; commit-bisect pinned first-bad to the VEC-1 update path (846910f). Two install-time reconcile sites treated "this ENTRY is dead" as "this KEY is deleted": 1. Compact install (snap_and_reconcile): any dead entry in the frozen window triggered a key_hash-WIDE tombstone on the new immutable. VEC-1 tombstones the old copy in place on update, so a key updated before the freeze had dead-old + live-new in the SAME window — and the install deleted the new copy out of the immutable it had just been compacted into. Fix: a dead window entry only proves deletion when the key has no live window sibling (DEL/UNLINK kills all copies, so real deletes still fire; post-freeze updates land in the tail and stay covered by the tail_keys arm). 2. Merge install (poll_install_merge): replayed each source's LIFETIME interior tombstone set key_hash-wide onto the merged output. An update interior-tombstones the old copy's home segment (and, via the VEC-1 fallback, every immutable) while the new copy lives elsewhere — the replay killed the current copy merged in from a sibling segment. Fix: origin-gate the replay (new mark_deleted_by_key_hash_install_from) — a source's tombstone only kills merged entries whose global_id came from that source; real DEL/UNLINK tombstones are in every source's set and still apply. 3. VectorStore::insert_vector (test/convenience API) allocated insert_lsn = mutable.len()+1, which restarts after every compaction; merge dedup (keep highest insert_lsn) then kept a STALE copy over the current one. Now allocates from the same monotonic txn_manager LSN allocator as the wire path. TDD red-first: test_bg_compact_update_before_freeze_survives_install (count=0 red -> 1 green) and test_bg_merge_update_across_segments_ survives (0 red -> 1 green). End-to-end churn repro (24k wire ops, default GraphUnion merge): LOST 1054 -> 0, live-set recall 0.685 -> 0.98, and with MERGE_MODE NONE: LOST 684 -> 0. All 562 vector lib tests green. Refs: Bundle-5 reliability validation (scripts/vector-validate.py) author: Tin Dang
Bundle-5 deliverable: validate reliability/stability/durability of the
vector engine under long-run churn, on real cloud hardware, comparing
this branch against the v0.5.1 baseline.
- scripts/vector-validate.py — on-target driver (raw RESP client, no
redis-cli: FT.SEARCH PARAMS blobs are binary). Three phases:
recall per binary x SQ8/TQ4: 20k MiniLM-like 384d unit
vectors, R@10 vs numpy brute force, QPS + p50/p99.
soak appendonly yes, 60/25/10/5% search/update/insert/delete
churn; samples live-set recall, RSS, num_docs, and
deleted-key resurrection every 60s; hard-fails on
resurrection, recall < 0.85, or RSS runaway.
durability settled-write survival across kill -9 (everysec + 5s
margin), post-recovery recall, double-crash restart.
Servers are spawned/killed by the driver (SIGKILL only — SIGTERM +
SO_REUSEPORT hang gotcha). O(1) id-pool sampling in the churn loop.
- scripts/gcloud-vector-soak.sh — GCE orchestration (c4a-standard-8 ARM
+ c3-standard-8 x86): ships the unpushed branch via git bundle,
builds moon-base (v0.5.1) + moon-branch with ELF asserts, installs
python3-numpy, runs the driver, fetches results JSON, teardown trap.
--self-test gate runs with no GCloud cost.
This harness caught all three defects fixed in the two preceding
commits before any cloud spend (DEL/UNLINK unindex gap, compact-install
update loss, merge-install tombstone replay loss).
Refs: Bundle-5 (vector deep review follow-on)
author: Tin Dang
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR adds SQ8 stats-based distance kernels, an f16 exact-rerank sidecar, cluster-wide FT.INFO aggregation, DEL/UNLINK delete parity, striped vector metrics, bounded compaction, and standalone validation/benchmark scripts. ChangesVector search correctness and performance
Long-run vector reliability test harness
Estimated code review effort: 5 (Critical) | ~150 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoSIMD SQ8 ADC, exact rerank, and vector churn reliability fixes
AI Description
Diagram
High-Level Assessment
Files changed (49)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
40 rules 1. Exact rerank tests bare unwrap
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+234
to
+239
| let mut truth: Vec<(f32, u32)> = vecs | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(i, v)| (l2_sq(&q, v), i as u32)) | ||
| .collect(); | ||
| truth.sort_by(|a, b| a.0.partial_cmp(&b.0).unwrap()); |
There was a problem hiding this comment.
2. Exact rerank tests bare unwrap 📘 Rule violation ✧ Quality
New .unwrap() usages were introduced across multiple test areas (including tests/vector_exact_rerank.rs, src/vector/store.rs, SQ8 stats tests, and merge_tests) without the required adjacent allow+justification comment pair, and a new crate-level #![allow(clippy::unwrap_used)] was added without an immediate justification comment. This violates the unwrap annotation/audit requirements and includes a partial_cmp(...).unwrap() panic risk if NaNs are encountered.
Agent Prompt
## Issue description
Several new `.unwrap()` calls were added in test code without the required adjacent `allow(clippy::unwrap_used)` plus directly-preceding justification comment, and a new crate-level `#![allow(clippy::unwrap_used)]` was introduced without an immediate justification and is overly broad. Additionally, the `partial_cmp(...).unwrap()` used in a sort comparator can panic if NaNs appear.
## Issue Context
- Compliance Rule 302083 requires that each unwrap be either avoided or explicitly annotated with an attached `#[allow(clippy::unwrap_used)]` and a justification comment immediately above that allow.
- The requirement applies in both application and test code.
- Where possible, prefer avoiding unwraps via `expect` (with a message), returning `Result` from tests and using `?`, or using safer comparisons (e.g., `f32::total_cmp`) rather than `partial_cmp(...).unwrap()`.
- Rule 209935 additionally requires new lint suppressions to be justified and narrowly scoped; avoid crate-level suppressions unless absolutely necessary and explicitly justified.
## Fix Focus Areas
- tests/vector_exact_rerank.rs[234-239]
- tests/vector_exact_rerank.rs[271-273]
- tests/vector_exact_rerank.rs[319-321]
- src/vector/store.rs[2894-2911]
- src/vector/store.rs[3151-3184]
- src/vector/turbo_quant/sq8.rs[581-600]
- src/command/vector_search/ft_info.rs[487-490]
- tests/vector_del_unindex.rs[18-19]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vector/store.rs (1)
177-240: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
store.rsfar exceeds the 1500-line file limit.This file spans well over 3400 lines (test module alone runs to ~3438), and this PR adds further fields/logic/tests to it rather than splitting it into submodules.
As per coding guidelines, "No single
.rsfile should exceed 1500 lines; split into submodules when approaching the 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/vector/store.rs` around lines 177 - 240, The VectorIndex/store.rs file is already far beyond the allowed single-file size, so this change should be moved out of the monolithic store.rs module. Split the new VectorIndex fields and related logic into focused submodules (for example around VectorIndex, compaction, or key-hash tracking), and update the main store.rs to just re-export or wire them together. Use the VectorIndex struct and its associated methods as the anchor points for extracting the code.Source: Coding guidelines
🧹 Nitpick comments (5)
src/command/vector_search/ft_config.rs (1)
134-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a unit test for the new FT.CONFIG parameter.
No test exercising
FT.CONFIG SET/GET <idx> MERGE_RECALL_TOLERANCEis included in this bundle (unlike theMERGE_MODE KEEP_RAWrejection, which hastest_ft_create_merge_mode_keep_raw_rejected).As per coding guidelines, "Every new command needs at least one unit test and one consistency test entry."
Also applies to: 199-203
🤖 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/vector_search/ft_config.rs` around lines 134 - 158, Add coverage for the new FT.CONFIG MERGE_RECALL_TOLERANCE parameter by creating a unit test that exercises FT.CONFIG SET and GET on an index and verifies the value is accepted and persisted through the existing FT.CONFIG handling in ft_config.rs. Follow the pattern of test_ft_create_merge_mode_keep_raw_rejected for locating the right test module, and add a corresponding consistency test entry as required by the coding guidelines so this new command behavior is covered end-to-end.Source: Coding guidelines
src/command/vector_search/ft_info.rs (2)
263-267: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNon-
Arrayremote responses are silently dropped from the aggregate instead of failing loud.The doc comment states errors are propagated fail-loud, but a remote response that is neither
Frame::ErrornorFrame::Arrayis silently skipped (continue) rather than surfaced. This would silently under-countnum_docs/other additive stats for that shard with no indication to the caller. Compare withscatter_invalidate_rangeinsrc/shard/coordinator.rs, which explicitly returnsFrame::Error(...)forOk(other)unexpected response shapes.In practice this is unreachable today since
ft_info/ft_info_text_onlyonly ever returnFrame::ArrayorFrame::Error, so this is a defensive gap rather than an active bug.🛡️ Optional defensive fix
for remote in remotes { let r_items: &[Frame] = match remote { Frame::Array(a) => a, - _ => continue, + other => { + let _ = other; + return Frame::Error(Bytes::from_static( + b"ERR FT.INFO: unexpected response from remote shard", + )); + } };🤖 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/vector_search/ft_info.rs` around lines 263 - 267, Non-`Array` remote responses in the ft_info aggregate are being skipped instead of surfaced, which breaks the fail-loud behavior described by the doc comment. Update the aggregation logic in the ft_info path so the match on each remote response does not `continue` on unexpected shapes; instead, return a `Frame::Error` for any response that is neither `Frame::Array` nor the expected error form. Use the existing ft_info aggregation code and mirror the defensive pattern used by `scatter_invalidate_range` in `coordinator` to keep unexpected remote replies visible to the caller.
209-316: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDerived stats (
bytes_per_posting) go stale after additive merge.
bytes_per_postingis precomputed locally astotal_postings / num_docs_val(seeft_info_text_only,src/command/vector_search/ft_info.rs:370-378). Aftermerge_ft_info_responsessumsnum_docsandtotal_inverted_index_sizeacross shards,bytes_per_postingis left at the local shard's pre-merge ratio, producing an internally inconsistent FT.INFO report (e.g. summed totals imply one ratio,bytes_per_postingreports another).avg_doc_lenhas the same staleness issue but can't be trivially recomputed from the currently-additive fields alone (would need an additional additive "total doc length" field to properly weight the average).Consider recomputing
bytes_per_postingfrom the merged totals after the loop.♻️ Suggested recompute after the merge loop
} + // Recompute derived stat now that num_docs/total_inverted_index_size are merged. + if let (Some(ti), Some(ndi), Some(bpi)) = ( + value_idx(&items, b"total_inverted_index_size"), + value_idx(&items, b"num_docs"), + value_idx(&items, b"bytes_per_posting"), + ) { + if let (Some(total), Some(docs)) = (int_at(&items, ti), int_at(&items, ndi)) { + items[bpi] = Frame::Integer(if docs > 0 { total / docs } else { 0 }); + } + } + Frame::Array(items.into()) }🤖 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/vector_search/ft_info.rs` around lines 209 - 316, `merge_ft_info_responses` leaves the derived `bytes_per_posting` field stale after it adds shard totals, so the merged FT.INFO output becomes internally inconsistent. After the additive merge loop in `merge_ft_info_responses`, recompute `bytes_per_posting` from the merged `total_inverted_index_size` and `num_docs`/posting totals for the top-level FT info frame, using the same logic as `ft_info_text_only` to keep the report consistent. Keep `avg_doc_len` unchanged unless you introduce an additional additive source field for it.tests/vector_update_tombstone.rs (1)
1-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood regression coverage for the default field; additional-field path is untested.
Both tests exercise
handle_vector_insert(default field,field_idx == 0) viaft_create_args/hset, which only ever indexes a single "vec" field. The parallel VEC-1 fix inhandle_vector_insert_field(src/shard/spsc_handler.rs, additional-field tombstoning) has no equivalent test here — an update to a non-default vector field after compaction would not be caught by this suite.Consider adding a multi-field variant (schema with two VECTOR fields) exercising the
hset_update_after_compaction_...scenario against the additional field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/vector_update_tombstone.rs` around lines 1 - 131, The current regression only covers the default vector field path through handle_vector_insert via ft_create_args and hset, so it misses the additional-field tombstoning logic in handle_vector_insert_field. Add a new multi-field test variant (schema with two VECTOR fields) that updates a non-default vector field after compaction, mirroring hset_update_after_compaction_tombstones_immutable_copy, and assert the updated key appears exactly once so stale copies in the secondary field are caught.src/command/vector_search/tests.rs (1)
684-686: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLeftover
sqquantization is now dead work. After theappendsignature change these blocks still buildsqviaquantize_f32_to_sq(v, &mut sq)but never pass it anywhere, so the computation is discarded. Consider dropping thesq/quantize_f32_to_sqlines at these call sites to avoid confusion during future edits.♻️ Example cleanup (repeat at each listed site)
- let mut sq = vec![0i8; dim]; - quantize_f32_to_sq(v, &mut sq); snap.mutable.append(i as u64, v, i as u64);Also applies to: 2420-2422, 2434-2436, 2524-2526, 2810-2812, 3152-3154, 3350-3352
🤖 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/vector_search/tests.rs` around lines 684 - 686, Remove the leftover SQ quantization work at each vector_search test call site: in the blocks around append on snap.mutable, the local sq buffer and quantize_f32_to_sq(v, &mut sq) are no longer used after the append signature change. Update the affected test cases to call append directly and drop the dead sq-related lines in vector_search/tests.rs wherever this pattern appears, including the other listed append sites.
🤖 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 `@scripts/vector-validate.py`:
- Around line 225-238: The rss_mb() helper currently mis-scales RSS values
because its fallback assumes every non-MB/GB unit is KB. Update the unit
handling in rss_mb() so the RSS parsing branch explicitly supports B, KB, MB,
GB, and TB using the correct conversion to megabytes, while keeping the existing
MEMORY DOCTOR parsing logic and rss_mb() return behavior intact.
In `@src/command/vector_search/ft_config.rs`:
- Around line 134-158: The MERGE_RECALL_TOLERANCE branch in ft_config.rs only
updates idx.merge_recall_tolerance in memory, so the value is lost on restart.
Persist this setting alongside the existing index metadata used by the vector
sidecar, and update the save/load path so the field is serialized and restored
with compaction_weight. Make the changes in the metadata handling and the code
paths that read/write the index state so MERGE_RECALL_TOLERANCE survives
restarts.
In `@src/shard/spsc_handler.rs`:
- Around line 2150-2162: DEL/UNLINK vector tombstones are applied without
rollback tracking, so they bypass cross-store transaction undo. Update the
DEL/UNLINK path that calls auto_delete_vectors in spsc_handler to also record a
matching vector intent in the active txn state, consistent with how
abort_cross_store_txn replays txn.vector_intents. Use the same transaction
plumbing already used by the HSET auto-index flow so TXN.ABORT can restore
vector state when kv_undo restores the key.
In `@src/vector/f16.rs`:
- Around line 76-82: The subnormal path in f16_to_f32 uses an off-by-one shift
constant, causing every nonzero subnormal to decode at half the correct
magnitude. Update the leading-zero adjustment in the subnormal branch of
f16_to_f32 (the lead calculation) so the exponent is normalized correctly, then
expand the roundtrip tests in the f16 module to cover a few subnormal inputs
like the smallest and largest subnormals.
---
Outside diff comments:
In `@src/vector/store.rs`:
- Around line 177-240: The VectorIndex/store.rs file is already far beyond the
allowed single-file size, so this change should be moved out of the monolithic
store.rs module. Split the new VectorIndex fields and related logic into focused
submodules (for example around VectorIndex, compaction, or key-hash tracking),
and update the main store.rs to just re-export or wire them together. Use the
VectorIndex struct and its associated methods as the anchor points for
extracting the code.
---
Nitpick comments:
In `@src/command/vector_search/ft_config.rs`:
- Around line 134-158: Add coverage for the new FT.CONFIG MERGE_RECALL_TOLERANCE
parameter by creating a unit test that exercises FT.CONFIG SET and GET on an
index and verifies the value is accepted and persisted through the existing
FT.CONFIG handling in ft_config.rs. Follow the pattern of
test_ft_create_merge_mode_keep_raw_rejected for locating the right test module,
and add a corresponding consistency test entry as required by the coding
guidelines so this new command behavior is covered end-to-end.
In `@src/command/vector_search/ft_info.rs`:
- Around line 263-267: Non-`Array` remote responses in the ft_info aggregate are
being skipped instead of surfaced, which breaks the fail-loud behavior described
by the doc comment. Update the aggregation logic in the ft_info path so the
match on each remote response does not `continue` on unexpected shapes; instead,
return a `Frame::Error` for any response that is neither `Frame::Array` nor the
expected error form. Use the existing ft_info aggregation code and mirror the
defensive pattern used by `scatter_invalidate_range` in `coordinator` to keep
unexpected remote replies visible to the caller.
- Around line 209-316: `merge_ft_info_responses` leaves the derived
`bytes_per_posting` field stale after it adds shard totals, so the merged
FT.INFO output becomes internally inconsistent. After the additive merge loop in
`merge_ft_info_responses`, recompute `bytes_per_posting` from the merged
`total_inverted_index_size` and `num_docs`/posting totals for the top-level FT
info frame, using the same logic as `ft_info_text_only` to keep the report
consistent. Keep `avg_doc_len` unchanged unless you introduce an additional
additive source field for it.
In `@src/command/vector_search/tests.rs`:
- Around line 684-686: Remove the leftover SQ quantization work at each
vector_search test call site: in the blocks around append on snap.mutable, the
local sq buffer and quantize_f32_to_sq(v, &mut sq) are no longer used after the
append signature change. Update the affected test cases to call append directly
and drop the dead sq-related lines in vector_search/tests.rs wherever this
pattern appears, including the other listed append sites.
In `@tests/vector_update_tombstone.rs`:
- Around line 1-131: The current regression only covers the default vector field
path through handle_vector_insert via ft_create_args and hset, so it misses the
additional-field tombstoning logic in handle_vector_insert_field. Add a new
multi-field test variant (schema with two VECTOR fields) that updates a
non-default vector field after compaction, mirroring
hset_update_after_compaction_tombstones_immutable_copy, and assert the updated
key appears exactly once so stale copies in the secondary field are caught.
🪄 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: 8f07e894-3037-4ef4-877a-14d5207e2462
📒 Files selected for processing (49)
CHANGELOG.mdCLAUDE.mdCargo.tomlbenches/sq8_adc_bench.rsscripts/gcloud-vector-soak.shscripts/vector-validate.pysrc/command/connection.rssrc/command/vector_search/ft_config.rssrc/command/vector_search/ft_create.rssrc/command/vector_search/ft_info.rssrc/command/vector_search/ft_search/dispatch.rssrc/command/vector_search/ft_search/execute.rssrc/command/vector_search/hybrid.rssrc/command/vector_search/mod.rssrc/command/vector_search/tests.rssrc/server/conn/handler_monoio/ft.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_sharded/ft.rssrc/server/conn/handler_single.rssrc/server/conn/shared.rssrc/shard/coordinator.rssrc/shard/persistence_tick.rssrc/shard/spsc_handler.rssrc/vector/distance/avx2.rssrc/vector/distance/avx512.rssrc/vector/distance/mod.rssrc/vector/distance/neon.rssrc/vector/f16.rssrc/vector/hnsw/graph.rssrc/vector/hnsw/search.rssrc/vector/metrics.rssrc/vector/mod.rssrc/vector/persistence/recovery.rssrc/vector/persistence/segment_io.rssrc/vector/segment/compaction.rssrc/vector/segment/holder.rssrc/vector/segment/immutable.rssrc/vector/segment/mutable.rssrc/vector/store.rssrc/vector/turbo_quant/sq8.rstests/moonstore_warm_e2e.rstests/vector_del_unindex.rstests/vector_edge_cases.rstests/vector_exact_rerank.rstests/vector_insert_bench.rstests/vector_memory_audit.rstests/vector_segment_merge.rstests/vector_stress.rstests/vector_update_tombstone.rs
Comment on lines
+225
to
+238
| def rss_mb(c): | ||
| r = c.cmd("MEMORY", "DOCTOR") | ||
| if isinstance(r, (bytes, str)): | ||
| text = r.decode() if isinstance(r, bytes) else r | ||
| for line in text.splitlines(): | ||
| if "RSS:" in line: | ||
| parts = line.split() | ||
| try: | ||
| val = float(parts[-2]) | ||
| unit = parts[-1] | ||
| return val * (1024 if unit == "GB" else 1) if unit in ("MB", "GB") else val / 1024 | ||
| except (ValueError, IndexError): | ||
| pass | ||
| return -1.0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the string/format rss_mb() depends on actually exists in the server output path.
rg -nP -C3 '\bRSS\b' --type=rust src/command src/ | rg -i 'memory|doctor|rss'
rg -nP -C3 'MEMORY|DOCTOR' --type=rust src/command | head -50Repository: pilotspace/moon
Length of output: 11939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the MEMORY DOCTOR formatter and the rss_mb parser.
rg -n "humanize_bytes|rss_mb|MEMORY DOCTOR|RSS:" src scripts/vector-validate.py
# Show the relevant implementation slices with line numbers.
sed -n '1,220p' src/command/server_admin.rs | cat -n
printf '\n---\n'
sed -n '220,280p' scripts/vector-validate.py | cat -nRepository: pilotspace/moon
Length of output: 15979
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact unit formatting used for RSS output.
sed -n '520,560p' src/command/server_admin.rs | cat -nRepository: pilotspace/moon
Length of output: 1733
Handle B and TB RSS outputs rss_mb() treats every non-MB/GB unit as val / 1024, which is only correct for KB; B and TB are mis-scaled and can skew the soak’s RSS growth check.
🤖 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 `@scripts/vector-validate.py` around lines 225 - 238, The rss_mb() helper
currently mis-scales RSS values because its fallback assumes every non-MB/GB
unit is KB. Update the unit handling in rss_mb() so the RSS parsing branch
explicitly supports B, KB, MB, GB, and TB using the correct conversion to
megabytes, while keeping the existing MEMORY DOCTOR parsing logic and rss_mb()
return behavior intact.
Comment on lines
+134
to
+158
| } else if param.eq_ignore_ascii_case(b"MERGE_RECALL_TOLERANCE") { | ||
| // VEC-4: recall gate for UNATTENDED (background/vacuum) GraphUnion | ||
| // merges. Default 0.70 catches only catastrophic collapse; operators | ||
| // running recall-sensitive workloads can tighten toward the manual | ||
| // FT.COMPACT gate (0.90). Raising it can make auto-merge abort | ||
| // repeatedly on small indexes (segments stay > threshold) — informed | ||
| // trade-off, hence a knob rather than a new default. | ||
| let parsed: f32 = match std::str::from_utf8(value) | ||
| .ok() | ||
| .and_then(|s| s.parse::<f32>().ok()) | ||
| { | ||
| Some(v) => v, | ||
| None => { | ||
| return Frame::Error(Bytes::from_static( | ||
| b"ERR MERGE_RECALL_TOLERANCE must be a number", | ||
| )); | ||
| } | ||
| }; | ||
| if !(0.0..=1.0).contains(&parsed) { | ||
| return Frame::Error(Bytes::from_static( | ||
| b"ERR MERGE_RECALL_TOLERANCE must be between 0.0 and 1.0", | ||
| )); | ||
| } | ||
| idx.merge_recall_tolerance = parsed; | ||
| Frame::SimpleString(Bytes::from_static(b"OK")) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether merge_recall_tolerance is persisted/restored anywhere else.
rg -n "merge_recall_tolerance" -C3
rg -n "collect_index_metas_with_weights|save_index_metadata_v3" -C5Repository: pilotspace/moon
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant config handler and persistence code.
git ls-files | rg '^src/command/vector_search/ft_config\.rs$|^src/vector/store\.rs$|^src/command/vector_search/' || true
echo '--- ft_config.rs outline ---'
ast-grep outline src/command/vector_search/ft_config.rs --view expanded || true
echo '--- store.rs outline ---'
ast-grep outline src/vector/store.rs --view expanded || true
echo '--- search for sidecar persistence and merge recall tolerance ---'
rg -n "merge_recall_tolerance|compaction_weight|save_index_meta_sidecar|collect_index_metas_with_weights|save_index_metadata_v3" src -C 3 || trueRepository: pilotspace/moon
Length of output: 28849
MERGE_RECALL_TOLERANCE needs persistence across restarts. This branch updates idx.merge_recall_tolerance, but the vector sidecar currently only stores compaction_weight, so the value resets to 0.70 after a restart. Persist it with the rest of the index metadata and extend the load/save format accordingly.
🤖 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/vector_search/ft_config.rs` around lines 134 - 158, The
MERGE_RECALL_TOLERANCE branch in ft_config.rs only updates
idx.merge_recall_tolerance in memory, so the value is lost on restart. Persist
this setting alongside the existing index metadata used by the vector sidecar,
and update the save/load path so the field is serialized and restored with
compaction_weight. Make the changes in the metadata handling and the code paths
that read/write the index state so MERGE_RECALL_TOLERANCE survives restarts.
Comment on lines
+2150
to
+2162
| /// Tombstone auto-indexed vectors for every key argument of a successful | ||
| /// DEL/UNLINK. Wire-parity requirement: every dispatch path that runs | ||
| /// `auto_index_hset*` on HSET must run this on DEL/UNLINK, or deleted keys | ||
| /// keep matching FT.SEARCH forever (resurrection + live-set recall collapse; | ||
| /// found by the Bundle-5 soak diagnostic at shards=1). | ||
| pub fn auto_delete_vectors(vector_store: &mut VectorStore, args: &[crate::protocol::Frame]) { | ||
| for arg in args { | ||
| if let Some(key) = crate::server::connection::extract_bytes(arg) { | ||
| vector_store.mark_deleted_for_key(key.as_ref()); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether TXN.ABORT has any compensation path for vector deletes,
# analogous to record_vector's undo path for HSET inserts.
rg -n 'mark_deleted_for_key|record_vector|VectorIntent' src/transaction/abort.rs -A3 -B3
rg -n 'fn mark_deleted_for_key' -A 25 src/vector/store.rsRepository: pilotspace/moon
Length of output: 1653
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect transaction abort logic for any vector-delete compensation or undo hooks.
rg -n 'auto_delete_vectors|mark_deleted_for_key|VectorIntent|kv_undo|undo' src/transaction/abort.rs src -A4 -B4
# Inspect the helper and its nearby call sites / txn gating.
rg -n 'fn auto_delete_vectors|auto_delete_vectors\(' src/shard/spsc_handler.rs src -A4 -B4
# Find all direct references to delete-vector helper across handlers.
rg -n 'auto_delete_vectors' src -g '*.rs'Repository: pilotspace/moon
Length of output: 50374
🏁
Length of output: 25
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show any vector-related undo/abort logic and the helper's wiring points.
sed -n '1,260p' src/transaction/abort.rs | nl -ba | sed -n '1,260p' | rg -n 'vector|Vector|kv_undo|undo|abort|tombstone|delete|record_vector'
printf '\n--- auto_delete_vectors call sites ---\n'
rg -n 'auto_delete_vectors' src -g '*.rs' -A2 -B2
printf '\n--- txn_id gating around vector insert/delete helpers ---\n'
rg -n 'txn_id\s*==\s*0|txn_id' src/shard/spsc_handler.rs -A2 -B2 | rg -n 'handle_vector_insert|handle_vector_insert_field|auto_delete_vectors|mark_deleted_for_key|txn_id'Repository: pilotspace/moon
Length of output: 189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect txn abort handling and all current uses of the delete helper.
if [ -f src/transaction/abort.rs ]; then
printf '%s\n' '--- src/transaction/abort.rs ---'
rg -n 'vector|Vector|kv_undo|undo|abort|tombstone|delete|record_vector' src/transaction/abort.rs -A3 -B3 || true
fi
printf '\n%s\n' '--- auto_delete_vectors call sites ---'
rg -n 'auto_delete_vectors' src -g '*.rs' -A2 -B2 || true
printf '\n%s\n' '--- txn_id gating around vector insert/delete helpers ---'
rg -n 'txn_id\s*==\s*0|handle_vector_insert|handle_vector_insert_field|auto_delete_vectors|mark_deleted_for_key' src/shard/spsc_handler.rs -A2 -B2 || trueRepository: pilotspace/moon
Length of output: 22094
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the active cross-txn DEL/UNLINK and HSET branches in the sharded handler.
sed -n '1260,1420p' src/server/conn/handler_sharded/mod.rs | cat -n
printf '\n--- monoio delete wiring ---\n'
sed -n '1288,1325p' src/server/conn/handler_monoio/mod.rs | cat -n
printf '\n--- shared post-dispatch auto-delete path ---\n'
sed -n '230,255p' src/server/conn/shared.rs | cat -n
printf '\n--- vector intent field definitions ---\n'
rg -n 'struct CrossStoreTxn|vector_intents|VectorIntent' src/transaction src/server/conn -A4 -B4Repository: pilotspace/moon
Length of output: 22356
🏁
Length of output: 25
DEL/UNLINK need vector rollback plumbing.
abort_cross_store_txn only replays txn.vector_intents, but the new auto_delete_vectors(...) calls never record a matching intent. If DEL/UNLINK runs inside an active cross-store TXN, the vector tombstone becomes visible immediately and won’t be undone by TXN.ABORT even if kv_undo restores the key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shard/spsc_handler.rs` around lines 2150 - 2162, DEL/UNLINK vector
tombstones are applied without rollback tracking, so they bypass cross-store
transaction undo. Update the DEL/UNLINK path that calls auto_delete_vectors in
spsc_handler to also record a matching vector intent in the active txn state,
consistent with how abort_cross_store_txn replays txn.vector_intents. Use the
same transaction plumbing already used by the HSET auto-index flow so TXN.ABORT
can restore vector state when kv_undo restores the key.
Comment on lines
+76
to
+82
| } else { | ||
| // Subnormal: normalize by shifting the mantissa up. | ||
| let lead = mant.leading_zeros() - 21; // Zeros above bit 9. | ||
| let exp32 = 127 - 15 - lead; | ||
| let mant32 = (mant << (lead + 1)) & 0x03FF; | ||
| sign | (exp32 << 23) | (mant32 << 13) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby tests.
git ls-files src/vector/f16.rs
wc -l src/vector/f16.rs
sed -n '1,220p' src/vector/f16.rs
# Find tests or references for subnormal handling.
rg -n "known_values_roundtrip_exact|relative_error_bounded|f16_to_f32|subnormal|0x0001|0x0200|0x03FF" src -SRepository: pilotspace/moon
Length of output: 7864
🏁 Script executed:
python3 - <<'PY'
import struct, math
def current(bits):
sign = ((bits & 0x8000) << 16) & 0xffffffff
exp = (bits >> 10) & 0x1f
mant = bits & 0x3ff
if exp == 0:
if mant == 0:
out = sign
else:
lead = mant.bit_length() # 1..10
lz = 32 - lead
lead = lz - 21
exp32 = 127 - 15 - lead
mant32 = (mant << (lead + 1)) & 0x03FF
out = sign | (exp32 << 23) | (mant32 << 13)
elif exp == 0x1f:
out = sign | 0x7f800000 | (mant << 13)
else:
out = sign | ((exp + 127 - 15) << 23) | (mant << 13)
return struct.unpack('<f', struct.pack('<I', out & 0xffffffff))[0]
def exact(bits):
sign = -1.0 if (bits & 0x8000) else 1.0
exp = (bits >> 10) & 0x1f
mant = bits & 0x3ff
if exp == 0:
if mant == 0:
return math.copysign(0.0, sign)
return sign * mant * 2.0**-24
if exp == 0x1f:
return math.copysign(math.inf if mant == 0 else math.nan, sign)
return sign * (1.0 + mant / 1024.0) * 2.0**(exp - 15)
bad = []
for bits in range(0x0001, 0x0400):
c = current(bits)
e = exact(bits)
if c != e:
bad.append((bits, c, e))
if len(bad) >= 10:
break
print("mismatches:", len(bad))
for b, c, e in bad:
print(hex(b), c, e)
for b in [0x0001, 0x0200, 0x03FF]:
print(hex(b), current(b), exact(b))
PYRepository: pilotspace/moon
Length of output: 796
Subnormal f16_to_f32 is off by one exponent (all subnormals decode at half magnitude).
lead = mant.leading_zeros() - 21 is one too small in the subnormal branch, so every nonzero subnormal comes back 2× too small. For example, 0x0001 decodes to 2^-25 instead of 2^-24, and 0x0200 decodes to 2^-16 instead of 2^-15. Change the constant to 22, and add a few subnormal cases to the roundtrip test.
🤖 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/f16.rs` around lines 76 - 82, The subnormal path in f16_to_f32
uses an off-by-one shift constant, causing every nonzero subnormal to decode at
half the correct magnitude. Update the leading-zero adjustment in the subnormal
branch of f16_to_f32 (the lead calculation) so the exponent is normalized
correctly, then expand the roundtrip tests in the f16 module to cover a few
subnormal inputs like the smallest and largest subnormals.
Source: Coding guidelines
Adds scripts/bench-vector-vs-redisearch.py: because Moon speaks the
RediSearch FT.* wire dialect, one driver benches both engines with
identical protocol, data, and ground truth (numpy brute-force cosine):
- insert throughput (pipelined HSET, vectors auto-indexed)
- FT.SEARCH KNN-10 QPS + p50/p99 (single connection, 10s tight loop)
- R@10 on two 384d unit-vector datasets:
* gaussian — iid random (concentration-of-distances hard case)
* clustered — 200-center mixture, sigma 0.04 => members at ~0.8
cosine to their center, in-distribution queries (real-embedding-
like neighborhoods; larger sigmas degenerate to the random regime)
- RediSearch EF_RUNTIME sweep (default/64/128/256/512) so search QPS
is compared at MATCHED recall, not mismatched engine defaults
RediSearch runs FLOAT32 HNSW via --loadmodule redisearch.so
(REDISEARCH_SO env); Moon runs SQ8 + TQ4 pre- and post-FT.COMPACT.
--only moon|redisearch supports single-engine smoke runs.
Measured on GCE c3-standard-8 (results posted to PR #214): Moon wins
insert 24-38x (86k vs 2.3-3.5k vec/s) and hard-dataset recall at
matched latency; RediSearch keeps ~3.8x search QPS at matched
recall=1.0 on well-clustered data.
refs: PR #214 follow-up bench promised in the PR body
author: Tin Dang
Contributor
Author
Head-to-head: Moon (this branch) vs RediSearch 7.4 — resultsAs promised in the PR body. Setup: GCE Two datasets: clustered (200-center mixture, members at ~0.8 cosine to center, in-distribution queries — real-embedding-like) and gaussian (iid random — the concentration-of-distances hard case). RediSearch gets an Insert / indexing throughput
Moon 24–38× faster. (Moon ingests via mutable segment + background HNSW build; RediSearch builds the graph synchronously on write.) Search — clustered (realistic embeddings-like)
At matched recall (1.0): RediSearch wins search QPS ~3.8× (6,899 vs 1,794). Note the June-2026 baseline had this gap at 16× with a recall deficit (0.86 vs 0.96) — HQ-1/HQ-2 closed it to 3.8× at recall parity, and TQ4's exact-rerank now reaches 1.0 recall on realistic data. Search — gaussian (hard / high-entropy)
Interpolating RediSearch to Moon's 0.772 recall (~ef-384) gives ≈1,100–1,200 QPS: parity, slight Moon edge, with lower p50. Where the data is hard, quantized-beam + exact rerank holds recall that a pure fp32 HNSW beam only reaches at large EF. Verdict ("does this beat Redis?")
Raw JSONs: 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/bench-vector-vs-redisearch.py`:
- Around line 10-12: The module docstring in bench-vector-vs-redisearch.py is
out of sync with the actual clustered dataset default. Update the header
description for the clustered dataset to match gen_clustered and its default
sigma value, and make sure the rationale in the docstring stays consistent with
the implementation details in gen_clustered. Keep the wording aligned with the
symbols gen_clustered and the module docstring so future readers see the correct
dataset construction.
- Around line 171-198: The fallback in knn_ids currently retries FT.SEARCH
without both NOCONTENT and EF_RUNTIME, which can silently change ef-* benchmark
behavior. Update the retry path in knn_ids so it only removes NOCONTENT while
preserving EF_RUNTIME when present, or otherwise explicitly report that
EF_RUNTIME is unsupported. Keep the logic that parses the FT.SEARCH response and
only adjust the query construction used in the retry branch.
🪄 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: ade2f1f1-3adb-421a-8646-63a96d31a1b8
📒 Files selected for processing (2)
CHANGELOG.mdscripts/bench-vector-vs-redisearch.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Comment on lines
+10
to
+12
| Datasets (384d unit vectors): | ||
| gaussian — iid random Gaussian, normalized (harsh: concentration of distances) | ||
| clustered — 200-center Gaussian mixture, sigma 0.25 (closer to real embeddings) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Docstring sigma value is stale.
The module docstring says clustered data uses sigma 0.25, but gen_clustered's actual default is sigma=0.04 (matching the inline rationale at Lines 152-155). Future readers relying on the header docs would misunderstand the actual dataset construction.
📝 Proposed fix
- clustered — 200-center Gaussian mixture, sigma 0.25 (closer to real embeddings)
+ clustered — 200-center Gaussian mixture, sigma 0.04 (closer to real embeddings)Also applies to: 149-163
🤖 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 `@scripts/bench-vector-vs-redisearch.py` around lines 10 - 12, The module
docstring in bench-vector-vs-redisearch.py is out of sync with the actual
clustered dataset default. Update the header description for the clustered
dataset to match gen_clustered and its default sigma value, and make sure the
rationale in the docstring stays consistent with the implementation details in
gen_clustered. Keep the wording aligned with the symbols gen_clustered and the
module docstring so future readers see the correct dataset construction.
Comment on lines
+171
to
+198
| def knn_ids(c, idx, q, k=K, ef_runtime=None): | ||
| query = f"*=>[KNN {k} @vec $B" + (f" EF_RUNTIME {ef_runtime}" if ef_runtime else "") + "]" | ||
| r = c.cmd( | ||
| "FT.SEARCH", idx, query, "PARAMS", "2", "B", q.tobytes(), | ||
| "NOCONTENT", "DIALECT", "2", | ||
| ) | ||
| if isinstance(r, Exception): | ||
| # Engines differ on NOCONTENT / EF_RUNTIME support — retry plain. | ||
| r = c.cmd( | ||
| "FT.SEARCH", idx, f"*=>[KNN {k} @vec $B]", | ||
| "PARAMS", "2", "B", q.tobytes(), "DIALECT", "2", | ||
| ) | ||
| if isinstance(r, Exception): | ||
| raise RuntimeError(f"FT.SEARCH failed: {r}") | ||
| keys = [r[i] for i in range(1, len(r), 2)] | ||
| else: | ||
| # NOCONTENT reply: [total, key1, key2, ...]; with-content: keys at odd idx. | ||
| body = r[1:] | ||
| if body and isinstance(body[0], bytes) and len(body) >= 2 and isinstance(body[1], list): | ||
| keys = body[0::2] | ||
| else: | ||
| keys = body | ||
| out = [] | ||
| for kk in keys: | ||
| s = kk.decode() if isinstance(kk, bytes) else str(kk) | ||
| if ":" in s: | ||
| out.append(int(s.split(":", 1)[1])) | ||
| return out |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant section with line numbers.
wc -l scripts/bench-vector-vs-redisearch.py
sed -n '140,230p' scripts/bench-vector-vs-redisearch.py
# Search for related FT.SEARCH usage and any notes about EF_RUNTIME/NOCONTENT.
rg -n "FT\.SEARCH|EF_RUNTIME|NOCONTENT|DIALECT" scripts/bench-vector-vs-redisearch.py README* docs* . 2>/dev/null || trueRepository: pilotspace/moon
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Redis/RediSearch docs in the repo if present.
fd -a -t f -g '*redisearch*' -g '*search*' .Repository: pilotspace/moon
Length of output: 265
🌐 Web query:
Redis FT.SEARCH syntax NOCONTENT PARAMS DIALECT order EF_RUNTIME RediSearch command argument validation
💡 Result:
The FT.SEARCH command in Redis Search (RediSearch) follows a specific syntax structure [1][2]. Parameters such as NOCONTENT, PARAMS, and DIALECT modify the command behavior, with DIALECT being particularly important for query parsing and feature availability [1][2][3]. Command Syntax Structure: FT.SEARCH index query [NOCONTENT] [TIMEOUT timeout] [PARAMS nargs name value...] [RETURN num field [AS alias]...] [LIMIT offset num] [DIALECT version] Key Arguments and Their Roles: 1. NOCONTENT: This optional flag instructs the command to return only the document IDs, excluding the actual field content [1][2]. It is useful for reducing network traffic [1][4]. 2. PARAMS: Allows passing external parameters into the query string, which is essential for vector search and security (to avoid injection) [1][4]. The argument 'nargs' must specify the count of key-value pairs multiplied by two [1][2]. 3. DIALECT: Specifies the query syntax version [3]. RediSearch supports multiple dialects (e.g., 1, 2, 3, 4), and newer dialects are often required for advanced query features, specific parsing improvements, and performance optimizations [3][5][6]. Dialect 2 or higher is typically recommended for modern search functionality [3]. 4. EF_RUNTIME: While often discussed in the context of vector search, this parameter can be passed via the PARAMS argument to tune the search runtime for K-Nearest Neighbors (KNN) operations, controlling the size of the dynamic candidate list [7]. Validation and Parsing: RediSearch has evolved its query parsing and validation logic in recent versions [8][5]. Recent updates, including changes in Redis 8, have focused on stricter validation, such as ensuring numeric range boundaries (min <= max) are correctly formed and dropping support for legacy features like the old FILTER and GEOFILTER options in favor of modern query syntax [8][5]. When using higher dialect versions, the engine applies more rigorous validation on field types and query predicates [5]. For optimal performance and to utilize the latest features, it is recommended to explicitly set the DIALECT and ensure queries adhere to current syntax standards as defined in the official Redis Search documentation [3][6].
Citations:
- 1: https://cloud.google.com/memorystore/docs/cluster/ftsearch
- 2: https://cloud.google.com/memorystore/docs/redis/ftsearch
- 3: https://redis.io/docs/latest/develop/ai/search-and-query/advanced-concepts/dialects/
- 4: https://docs.aws.amazon.com/memorydb/latest/devguide/vector-search-commands-ft.search.html
- 5: Improve query parsing - [MOD-7794, MOD-7764] RediSearch/RediSearch#5216
- 6: https://redis.io/docs/latest/develop/ai/search-and-query/indexing/field-and-type-options/
- 7: https://redis.io/docs/latest/develop/ai/search-and-query/vectors/
- 8: https://support.redislabs.com/hc/en-us/articles/31135349231122-Redis-Software-Search-Command-Deprecations-and-Query-Parsing-Changes-in-Redis-8
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the call sites and how benchmark output uses the returned IDs.
rg -n "knn_ids\(|ef_runtime|recall|sweep|json|print" scripts/bench-vector-vs-redisearch.py
# Read the main loop / reporting region.
sed -n '1,260p' scripts/bench-vector-vs-redisearch.pyRepository: pilotspace/moon
Length of output: 10059
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '109,120p' docs/vector-search-guide.md
sed -n '300,320p' scripts/bench-vector-vs-redisearch.pyRepository: pilotspace/moon
Length of output: 1642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any documented vector-search examples that include NOCONTENT.
rg -n "FT\.SEARCH.*NOCONTENT|NOCONTENT.*PARAMS|NOCONTENT.*DIALECT|RETURN 0" docs scripts README.md
# Show the vector-search guide section around FT.SEARCH examples if it mentions NOCONTENT.
sed -n '1,140p' docs/vector-search-guide.mdRepository: pilotspace/moon
Length of output: 7920
Preserve EF_RUNTIME on retry
The fallback drops EF_RUNTIME, so any NOCONTENT-related error can make every ef-* run report default-runtime results with no warning. Retry without NOCONTENT only, or surface the EF_RUNTIME incompatibility.
🤖 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 `@scripts/bench-vector-vs-redisearch.py` around lines 171 - 198, The fallback
in knn_ids currently retries FT.SEARCH without both NOCONTENT and EF_RUNTIME,
which can silently change ef-* benchmark behavior. Update the retry path in
knn_ids so it only removes NOCONTENT while preserving EF_RUNTIME when present,
or otherwise explicitly report that EF_RUNTIME is unsupported. Keep the logic
that parses the FT.SEARCH response and only adjust the query construction used
in the retry branch.
|
Please don't ping me with your AI slop thanks 👍 |
…tion Closes the single-connection search-QPS gap vs RediSearch by attacking per-query LATENCY (single-conn QPS = 1/latency): the per-segment HNSW searches of ONE KNN query now fan out across a worker pool instead of running serially on the shard task. 1. src/vector/search_pool.rs — SearchWorkerPool: std::thread workers + flume channels (mirrors BackgroundCompactor). Jobs carry Arc<ImmutableSegment>/Arc<WarmSearchSegment> references (O(1) refcount bumps); each worker owns a cached SearchScratch (amortized zero alloc per job); per-job catch_unwind containment (empty result + warn on a panicking segment search — never a hang; a dropped reply surfaces as RecvError on the caller). Global pool via OnceLock; uninitialized = disabled so embedded/unit-test uses see the exact serial path. 2. SegmentHolder::search_mvcc_yielding_with_pool — submits all immutable/warm segment searches BEFORE the mutable MVCC chunk scan (workers overlap with it), then awaits replies via recv_async (parks the task, never the shard event loop). ACORN-filtered traversal ships the bitmap Arc to workers; HnswPostFilter mode applies the bitmap at collection, mirroring the serial branch. Cold/IVF tiers stay serial (rare). Serial path preserved verbatim for pool=None / <2 segments. Identity guaranteed: SearchResult's total order (distance, then id) makes accumulation order immaterial — enforced by pooled-vs-serial identity tests + an 8-thread concurrency stress test. 3. Bounded bulk compaction (the enabler): a bulk-loaded mutable used to compact into ONE giant graph, leaving the pool nothing to fan out over. With a pool active and COMPACT_THRESHOLD > 0, each build now freezes at most max(threshold, len/8) entries (MutableSegment::freeze_prefix — prefix window, absolute vector_offsets stay valid, tail survives via clone_suffix exactly like a mid-build append; applies to both the force_compact drain loop and begin_background_compact). Pool-less deployments keep single-segment builds — multi-segment SERIAL search is strictly slower, so no configuration regresses. Red/green: test_force_compact_bulk_bounded_segments, test_bg_compact_bulk_bounded_segments. 4. --ft-search-workers N config: default auto = vCPUs − shards capped at 8 (0 on shards==cores KV deployments — zero interference); 0 = explicit off. Parse test added. Measured (macOS dev box, 20k×384d clustered SQ8, single conn, post-FT.COMPACT, R@10 = 1.0 in all rows): - status quo (1 segment, serial): p50 2.06 ms, 473 QPS - bounded 5 segments, workers=0: p50 9.58 ms, 105 QPS - bounded 5 segments, workers=auto: p50 0.46 ms, 2321 QPS (4.9×) GCE vs-RediSearch re-run posted to PR #214. Trade-off note: each segment is searched at the full resolved ef, so a pooled multi-segment query does ~Nseg× the CPU work of a single-graph query for the latency win — the same per-query cost profile as RediSearch's MT query workers. Per-segment ef reduction is a possible follow-up (recall-gated). CI parity: fmt, clippy (default + tokio,jemalloc), full release + tokio suites green. author: Tin Dang
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/vector/search_pool.rs (1)
92-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
.expectin library code should be annotated per guidelines.
SearchWorkerPool::newlives in library code (src/vector/...), where the guideline forbids bareunwrap()/expect(). A thread-spawn failure here is a reasonable panic, but it must carry an explicit allow + justification (asmain.rsdoes for its own spawns).♻️ Suggested annotation
let rx = job_rx.clone(); + // ALLOW: spawning the fixed startup worker set; failure here is + // an unrecoverable process-init error. + #[allow(clippy::expect_used)] std::thread::Builder::new() .name(format!("moon-vec-search-{i}")) .spawn(move || worker_loop(&rx)) .expect("failed to spawn vector search worker thread")As per coding guidelines: "Do not use
unwrap()orexpect()in library code outside tests ... If anunwrap()is truly safe, annotate it with#[allow(clippy::unwrap_used)]and add a one-line justification comment."🤖 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/search_pool.rs` around lines 92 - 112, SearchWorkerPool::new currently uses a bare expect() when spawning worker threads, which violates the library-code guideline. Update the worker spawn in SearchWorkerPool::new to add the required #[allow(clippy::unwrap_used)] annotation with a one-line justification comment explaining why the panic is acceptable here, matching the pattern used in main.rs. Keep the change localized around the thread::Builder::spawn call and worker_loop path so the intent is clear to future readers.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.
Nitpick comments:
In `@src/vector/search_pool.rs`:
- Around line 92-112: SearchWorkerPool::new currently uses a bare expect() when
spawning worker threads, which violates the library-code guideline. Update the
worker spawn in SearchWorkerPool::new to add the required
#[allow(clippy::unwrap_used)] annotation with a one-line justification comment
explaining why the panic is acceptable here, matching the pattern used in
main.rs. Keep the change localized around the thread::Builder::spawn call and
worker_loop path so the intent is clear to future readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fbc3fe5-508c-4fcf-aaee-170ae2ffae2c
📒 Files selected for processing (12)
CHANGELOG.mdscripts/bench-vector-vs-redisearch.pysrc/config.rssrc/main.rssrc/vector/mod.rssrc/vector/search_pool.rssrc/vector/segment/holder.rssrc/vector/segment/mutable.rssrc/vector/store.rstests/mq_integration.rstests/txn_kv_wiring.rstests/workspace_integration.rs
✅ Files skipped from review due to trivial changes (4)
- tests/txn_kv_wiring.rs
- tests/mq_integration.rs
- src/vector/mod.rs
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/bench-vector-vs-redisearch.py
- src/vector/segment/mutable.rs
GCE c3-standard-8 head-to-head (run 5) showed the auto default (vCPUs − shards) REGRESSING clustered SQ8 at the default resolved ef: 1,732 QPS (pool-off control, byte-identical to pre-change) → 1,165 QPS pooled. Probe matrix on the same instance isolated the cause: it is the ef work multiplier (each of the 5 segments pays the full ef=300 — workers=3 vs workers=7 within 3%, so NOT SMT oversubscription), which on a 4-physical- core box exceeds the parallel win. On a 10-physical-core macOS box the same default is a 4.9x QPS win (473 -> 2,321, R@10=1.0). A knob that regresses at defaults on common cloud shapes must not be a silent tax — same posture as --io-busy-poll-us: measured opt-in with documented per-platform guidance (good size: physical cores − shards, cap 8; auto_workers() stays as the reference sizing). Consequence: with the pool off by default, bounded bulk compaction (gated on pool presence) is also off by default — FT.COMPACT default behavior is byte-identical to v0.5.1, so this feature is zero-risk at defaults. Probe bonus finding (documented in CHANGELOG): the existing per-index EF_RUNTIME knob measured fairly closes most of the vs-RediSearch clustered gap serially — ef 64: 4,493 QPS at R@10=1.0 vs RediSearch 5,864 (was 3.8x at ef-default-vs-ef300, now ~1.3x). Full tables in PR #214. author: Tin Dang
Contributor
Author
GCE re-run: intra-query worker pool + fair EF-swept head-to-head (run 5 + probe matrix)TL;DR — the 3.8× clustered search-QPS gap is now ~1.3× at matched recall 1.0, and most of it came from measuring fairly (sweeping Moon's existing per-index Setup: GCE What shipped
RediSearch EF_RUNTIME sweep (clustered, the hard dataset)
Matched-recall anchor: RediSearch 1.0 @ 5,864 QPS (run-to-run variance vs run 4's 6,899 is ±15%). Moon clustered SQ8: pooled defaults vs control vs probe matrixRun 5 at Moon's default resolved ef (300 for k=10/384d):
The pooled regression prompted a 4-config probe on the same instance (all R@10 = 1.000):
Readings:
Gaussian (iid) — Moon dominates the recall-QPS frontierRediSearch never exceeds R@10 0.838 even at ef 512 (927 QPS; 0.062 at default ef). Moon SQ8: serial 0.772 @ 1,218 QPS; pooled 5-segment 0.9855 @ 730 QPS — a recall level RediSearch doesn't reach at any swept setting. (The +0.21 recall from pooling is the multi-segment oversampling effect: 5 graphs × full ef ≫ one graph's candidate set.) Inserts (unchanged headline)Moon 64.5–80.6k vec/s vs RediSearch 2.2–3.5k → 24–38× faster ingest, both datasets. Why default-offOn this 4-physical-core box the auto default regressed clustered SQ8 1,732 → 1,165 and TQ4 561 → 277; on a 10-physical-core macOS box the same default is 473 → 2,321 QPS (4.9×) at R@10 = 1.0. A knob that regresses at defaults on common cloud shapes must be a measured opt-in, not a silent tax (the Follow-ups
Bench driver: |
…lock
scripts/audit-unsafe.sh scans only the 3 lines above `unsafe {` for the
SAFETY: tag; the PRFM prefetch block's 5-line SAFETY comment pushed the
tag to offset 5, failing the CI safety audit (Lint job) despite the
block being fully documented. Reorder the comment so the SAFETY:
sentence is adjacent to the block; no code change.
author: Tin Dang
…timization # Conflicts: # CHANGELOG.md
…r, cosine clamp, safe SIMD tails Addresses the three real defects from the Qodo review of PR #214 (comment 4885258727): - f16_to_f32 subnormal decode was off by one in BOTH the exponent and the normalization shift: power-of-two mantissas decoded to half their value, multi-bit mantissas also lost their top fraction bit. Fixed (exp32 = 113 - lead, shift = lead) and pinned by two exhaustive tests: all 1023 subnormals decode to exactly mant * 2^-24, and every finite f16 bit pattern roundtrips f16 -> f32 -> f16 bit-identically. - rerank_exact ran BEFORE the tombstone liveness filter at both search call sites, so dead candidates consumed the 4*k exact-rerank budget and surviving live results could keep quantized ADC estimates mixed into the post-rerank ordering. The filter now runs first; regression test deletes the 30 nearest of 64 vectors (k=5, budget 20) and asserts exact distances + true live top-k (RED-verified against the old ordering). - Cosine rerank now clamps cos to [-1, 1] before computing 2 - 2*cos: f16 rounding could push the ratio slightly outside the legal range, producing distances below 0 / above 4. - SQ8 stats kernels (avx2/avx512/neon) scalar tails use safe zip iteration instead of get_unchecked, per UNSAFE_POLICY: the tail is at most one sub-vector-width pass, so bounds checks cost nothing and the unsafe surface stays confined to the intrinsics. Refs: PR #214 review author: Tin Dang
…timization # Conflicts: # CHANGELOG.md
TinDang97
added a commit
that referenced
this pull request
Jul 6, 2026
A G-segment index searched EVERY graph segment at the full resolved ef, doing ~G× the CPU work of one ef-wide beam over a single graph. This was measured as the root cause of the worker pool's regression on SMT-constrained boxes (GCE c3 probe matrix, PR #214: workers 3 vs 7 within 3% => the ef work multiplier, not SMT oversubscription). per_segment_ef(ef, G, quota) = clamp: max(ceil(ef/sqrt(G)), quota, 24), capped at ef. Rationale: segments partition the corpus, so each returns its LOCAL nearest — a per-segment beam of ef/sqrt(G) keeps the merged candidate mass at ~ef*sqrt(G), still a strict oversample of the single-graph baseline. Floors: the merge's per-segment quota (fetch_k / oversample_k — HNSW must be able to return that many) and 24 (beam degenerates below). Applied at ALL multi-segment graph search sites — sync search_filtered (all four strategy branches), sync search_mvcc, and the yielding path's shared graph_ef (worker-pool jobs, submit-failure inline fallback, and serial loops all read it) — so pooled == serial byte-identity holds (identity + stress tests green, unchanged). Recall gate (macOS, same data/seeds as the sidecar A/B earlier today, 20k×384d SQ8 post-compact, 5 segments, resolved ef 300 → 135/segment): clustered 5seg: R@10 1.0000 -> 1.0000 (p50 0.699 -> 0.687 ms) gaussian 5seg: R@10 0.9915 -> 0.9915 (p50 0.808 -> 0.789 ms) 1seg controls byte-unchanged (1.0000 / 0.7710). On a 10-core dev box the wall-clock delta is small (workers already absorb the excess in parallel); the win is the ~2× CPU-work cut, which is exactly what the 4-physical-core cloud shapes lacked. GCE before/ after on the recorded probe configs (1,198 / 2,919 / 2,396 QPS) lands with the branch validation run. Unit tests pin the helper's split/floor/cap/monotonicity behavior. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Jul 6, 2026
Apply the same UNSAFE_POLICY cleanup that PR #214 review applied to the f32 SQ8 stats kernels: the int8 symmetric ADC tails (avx2/avx512/neon) now iterate `qi8[i..n].iter().zip(codes[i..n])` instead of `get_unchecked` — the tail is at most one sub-vector-width pass, so bounds checks cost nothing and the unsafe surface stays confined to the SIMD intrinsics. Refs: PR #214 review follow-through author: Tin Dang
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 6, 2026
…ef, int8 ADC, keyspace parity, full index durability (#215) * perf(vector): SIMD f16 exact-rerank kernels (NEON integer-rescale + F16C) Profile evidence (perf, ef64 matched-recall config): rerank_exact was 17.2% of every FT.SEARCH query — 4k candidates × dim scalar software f16_to_f32 decodes (branchy bit-twiddling) + scalar FMAs per query per segment. This was the single largest addressable slice of the 55µs/query gap vs RediSearch at matched recall. Two new fused decode+distance kernels in the DistanceTable (dispatch via the existing OnceLock table, scalar fallback always available): - f16_l2 / f16_dot_normsq, aarch64 NEON: baseline-NEON integer-rescale decode — (bits&0x7FFF)<<13 reinterpreted as f32 then multiplied by 2^112 re-biases the exponent exactly (power of two; every finite f16 is exact in f32; subnormals normalize through the same multiply); exp==0x1F lanes (Inf/NaN) rebuilt via bit-select. No ARMv8.2 FP16 intrinsics required — runs on every AArch64 CPU. - x86_64: F16C vcvtph2ps + FMA, 16 halves/iteration. Runtime-detected as f16c && fma && avx2 (AVX2 is NOT implied by F16C+FMA — AMD Piledriver has both without AVX2; the hsum helper needs AVX2). - rerank_exact (immutable.rs) now calls the dispatched kernels for both the L2 and the fused cosine/IP path (the cosine loop was previously inline scalar). Semantics pinned by tests: SIMD == scalar on normals, f16 subnormals, zeros, odd tails (13 lengths), and Inf/NaN propagation — per-arch direct tests plus dispatched-table tests. Measured (OrbStack VM aarch64, 20k×384d clustered SQ8, 1 segment, single conn): ef64 7,795 → 8,468 QPS (+8.6%), rerank self-time 17.2% → ~9%; ef300 2,280 → 2,353. x86 F16C expected better (1-instr decode); GCE numbers with the full stack in the PR. Negative result (recorded in CHANGELOG): skipping rerank entirely for SQ8 was hypothesized (SQ8 ADC error is small) and A/B-REFUTED — R@10 drops 0.010 clustered / 0.017 gaussian-5seg. Recall is Moon's edge over RediSearch, so the pass stays unconditional and the kernels make it cheap instead. The A/B gate did its job. New unsafe: SIMD intrinsics blocks in distance/neon.rs + distance/avx2.rs following the module's existing target_feature + SAFETY-comment pattern (user-approved SIMD kernel work). author: Tin Dang * perf(vector): per-query fixed-cost cleanup — scratch TLS, Arc treemap, ryu Three per-query fixed costs on the FT.SEARCH wire path, found in the same profile as the f16 rerank work: 1. QP-3 thread-cached SearchScratch: capture_dense_knn_snapshot built a fresh SearchScratch EVERY query — BinaryHeaps, visited BitVec, and the 32-65KB ADC LUT buffer reallocated (then re-grown mid-search) per query. New take_thread_scratch/recycle_thread_scratch TLS pair in hnsw::search (one slot; exact padded_dim match — query_rotated .len() ACTS as the search's padded dim, so larger-is-fine is wrong); the yielding search recycles at its single exit. Concurrent same-thread searches just miss the cache and allocate as before. 2. QP-2 amortized committed-treemap capture: all 7 search-capture sites cloned the MVCC committed RoaringTreemap per query. TransactionManager::committed_snapshot() now returns a cached Arc<RoaringTreemap>, refreshed lazily on first capture after a commit/prune (dirty flag set at the ONLY two mutation points: commit() insert + prune remove_range). Read-heavy: one Arc bump per search. Write-heavy: at most one clone per search — never worse than before. SearchSnapshot.committed is now the Arc; MvccContext still borrows &RoaringTreemap via deref. Unit test pins snapshot equivalence + Arc-identity amortization + owned-view isolation. 3. RESP score formatting: __vec_score built f32 strings via Display (grisu::format_shortest_opt, 1.4% self-time in the ef64 profile). ryu (itoa's float sibling, new dep) replaces it in the three response.rs sites; {:.6} fixed-precision sites elsewhere unchanged. Measured (OrbStack VM aarch64, 20k×384d clustered SQ8, 1 segment, single conn, ef64): 8,468 -> 8,810 QPS (+4.0%); cumulative with the SIMD f16 kernels +13% from the 7,795 baseline. ef300 unchanged (fixed costs amortize into the longer search). author: Tin Dang * perf(vector): EF-SPLIT — divide the beam across graph segments A G-segment index searched EVERY graph segment at the full resolved ef, doing ~G× the CPU work of one ef-wide beam over a single graph. This was measured as the root cause of the worker pool's regression on SMT-constrained boxes (GCE c3 probe matrix, PR #214: workers 3 vs 7 within 3% => the ef work multiplier, not SMT oversubscription). per_segment_ef(ef, G, quota) = clamp: max(ceil(ef/sqrt(G)), quota, 24), capped at ef. Rationale: segments partition the corpus, so each returns its LOCAL nearest — a per-segment beam of ef/sqrt(G) keeps the merged candidate mass at ~ef*sqrt(G), still a strict oversample of the single-graph baseline. Floors: the merge's per-segment quota (fetch_k / oversample_k — HNSW must be able to return that many) and 24 (beam degenerates below). Applied at ALL multi-segment graph search sites — sync search_filtered (all four strategy branches), sync search_mvcc, and the yielding path's shared graph_ef (worker-pool jobs, submit-failure inline fallback, and serial loops all read it) — so pooled == serial byte-identity holds (identity + stress tests green, unchanged). Recall gate (macOS, same data/seeds as the sidecar A/B earlier today, 20k×384d SQ8 post-compact, 5 segments, resolved ef 300 → 135/segment): clustered 5seg: R@10 1.0000 -> 1.0000 (p50 0.699 -> 0.687 ms) gaussian 5seg: R@10 0.9915 -> 0.9915 (p50 0.808 -> 0.789 ms) 1seg controls byte-unchanged (1.0000 / 0.7710). On a 10-core dev box the wall-clock delta is small (workers already absorb the excess in parallel); the win is the ~2× CPU-work cut, which is exactly what the 4-physical-core cloud shapes lacked. GCE before/ after on the recorded probe configs (1,198 / 2,919 / 2,396 QPS) lands with the branch validation run. Unit tests pin the helper's split/floor/cap/monotonicity behavior. author: Tin Dang * perf(vector): AE-1 saturation-gated per-segment adaptive ef, retire blanket EF-SPLIT Compact/GraphUnion builds now run a one-time self-probe against the exact f16-sidecar ground truth (estimate_suggested_ef): 16 leave-self-out sample queries, brute-force GT via the SIMD f16 kernels, R@10 measured across an ef ladder [24..256]. A segment whose curve is FULLY SATURATED from the minimum rung (first rung >= 0.995 and within epsilon 0.005 of the ladder top) is certified trivially easy and searches at min-ef (24); every other segment keeps the FULL resolved ef. The suggestion only applies when ef was heuristic-defaulted (never over a user-pinned EF_RUNTIME), is floored by the merge quota, and is in-memory only — segments reloaded from disk carry None and use the full beam. This SUPERSEDES the blanket ef/sqrt(G) split (EF-SPLIT, previous commit): re-validation on a confirmed-fresh binary showed the split silently cost gaussian 5-seg R@10 0.9915 -> 0.9295 (the original "recall preserved" A/B had exercised a stale binary — identical recall AND latency to the pre-split build was the tell). A second design letting the probe pick a mid-ladder knee was also rejected (0.9915 -> 0.939): self-sampled queries are optimistically biased on unstructured data — the whole measured curve shifts up — so the probe's only trustworthy verdict is "saturated at min-ef". per_segment_ef and EF_SPLIT_MIN are removed; a policy note in holder.rs records the negative result. A compact-time tracing::debug logs each segment's ladder and decision for field diagnosis. Recall/latency A/B (20k x 384d SQ8 cosine, COMPACT_THRESHOLD 4096, same seeds/harness end-to-end, OrbStack-host macOS dev box): clustered 5seg R@10 1.0000 p50 0.79 -> 0.26 ms (~3x) clustered 1seg R@10 1.0000 -> 0.9960 (within probe epsilon 0.005) p50 1.78 -> 1.46 ms gaussian 5seg R@10 0.9915 p50 ~0.79 ms (probe self-rejects on all 5 segments -> full beam; byte-identical to pre-split) gaussian 1seg R@10 0.7710 (unchanged control) Tests: vector lib suite 887 passed; test_compact_measures_suggested_ef covers the probe attach path; clippy clean on default and runtime-tokio,jemalloc feature sets; fmt clean. CHANGELOG rewrites the now-inaccurate EF-SPLIT entry into this combined AE-1 entry. author: Tin Dang * fix(vector): FLUSHALL/FLUSHDB ghost vectors + HDEL stale-vector tombstone (R3/R4) Persistence-review findings R3 + R4 (compaction/AOF/WAL data-flow audit): R3 - FLUSHALL/FLUSHDB never touched the FT indexes: flushed hashes stayed searchable as ghost vectors/documents until the next restart. Both commands now clear every vector + text index's CONTENTS while KEEPING the FT.CREATE definitions, matching restart semantics (definitions come from the sidecar; contents re-derive from the now-empty keyspace): - VectorStore::clear_all_contents(): per index, tombstone warm segments (as drop_index) and rebuild a fresh empty index from the same IndexMeta; pending_segments discarded so a post-flush FT.CREATE cannot resurrect pre-flush contents via the recovery-claim path in create_index. - TextStore::clear_all_contents(): rebuild each TextIndex from its schema via new_with_schema (postings, term dicts, doc maps, TAG/NUMERIC reset). R4 - HDEL of an indexed vector field left the vector searchable (only whole-key DEL/UNLINK tombstoned; the sharded handler even documented the gap as intended). auto_hdel_vectors tombstones the key in exactly the indexes whose vector field was removed, via the new per-index VectorStore::mark_deleted_for_key_in_index (sibling indexes keyed on other fields keep their entries; mark_deleted_for_key refactored onto the shared tombstone_key_in_index body). After a successful HDEL the named fields are definitively absent, so the tombstone always agrees with the hash state. Documented follow-ups: multi-vector-field indexes tombstone the whole doc; TEXT/TAG/NUMERIC field removal not re-indexed. Wire-parity: both hooks added on ALL dispatch paths - handler_single (inline + MULTI/EXEC arm), handler_monoio conn-local, handler_sharded local write path, SPSC execute (both arms), shared batch path. TDD: tests/vector_flush_hdel_tombstone.rs (3 tests, red -> green; tokio+graph harness mirroring ft_search_as_of_filter.rs). Vector lib suite 887 passed, text 229 passed; fmt + clippy clean on default and runtime-tokio,jemalloc feature sets. author: Tin Dang * fix(vector): make exact-rerank sidecar drop loud + FT.INFO coverage (R5) Persistence-review finding R5: GraphUnion merge propagates the HQ-1 exact-rerank f16 sidecar all-or-nothing (compaction.rs all_have_raw) -- one sidecar-less source segment silently drops exact rerank for the ENTIRE merged segment, degrading recall with no signal. Rebuilding the sidecar from SQ8 decode was rejected (re-quantization error would contaminate "exact" rerank distances). - merge_immutable now emits tracing::warn with sources / sources_with_sidecar counts when the drop discards at least one real sidecar (all-sources-bare merges stay quiet: nothing is lost). - FT.INFO gains additive top-level counters graph_segments and segments_with_exact_rerank (summed across shards by merge_ft_info_responses), so coverage < total flags ADC-only segments in steady state. Unit test sums_exact_rerank_coverage_counters. author: Tin Dang * docs(vector): document AE-1 adaptive ef, FLUSHALL/HDEL parity, FT.INFO coverage counters Bring the doc surface in line with the recent vector-search commits: - CLAUDE.md (Vector Search section): HQ-1 rerank bullet now names the runtime-detected SIMD f16 kernels (NEON integer-rescale / F16C+FMA, +8.6% QPS alone, ~+13% with the per-query fixed-cost cleanup) and the now-loud sidecar-drop warn + FT.INFO coverage counters; new bullets for AE-1 saturation-gated per-segment ef (incl. the REJECTED blanket EF-SPLIT with its measured 0.9915 -> 0.9295 gaussian regression) and FLUSHALL/FLUSHDB/HDEL keyspace parity with documented follow-ups. - docs/vector-search-guide.md: FT.INFO graph_segments / segments_with_exact_rerank; new FLUSHALL/FLUSHDB/HDEL section (contents cleared, definitions survive, keyspace-global FLUSHDB note, restart-rebuild durability model as it stands today); AE-1 self-probe as compaction step 7; Search Path corrected (stale "no separate rerank needed" replaced with exact-rerank + AE-1 ef steps). - docs/commands.md: FLUSHALL/HDEL one-liner in the vector tip + known HDEL limitations bullet linking the new guide anchor. Covers commits 213b7b75, eace11d3, 5e8d3f47, e71bea58, f332f481. No source changes; in-progress durability design intentionally NOT documented as shipped. author: Tin Dang * feat(vector): durability write path - persist segments + manifest/keymap snapshots (B1+B2) Phases B1+B2 of tmp/VECTOR-DURABILITY-DESIGN.md (persistence-audit R1/R2 remediation; R6 folded in). Recovery behavior unchanged in this commit - the loader/dedup-rescan lands next (B3). B1 - formats: - SegmentMeta.suggested_ef: Option<u32> (serde default; old dirs load) so the AE-1 compact-time estimate survives disk round-trips (R6). - write_immutable_segment_staged: staging-<id> dir -> per-file fsync + dir fsync -> atomic rename -> root-dir fsync. A crash mid-write leaves only an orphan staging dir, never a half segment under a live name. - New src/vector/persistence/manifest.rs: IndexManifest JSON (atomic tmp+fsync+rename+dir-fsync write, tolerant load) carrying collection_id (the QJL rotation seed - REQUIRED to search reloaded codes), next_segment_id / next_global_id floors, live segment_ids and keymap_epoch; binary keymap (MKM1, xxh64 integrity, corrupt -> None + warn) mapping key_hash -> (global_id, vec_checksum, key); orphan-sweep helpers. 19 unit tests (round-trip, corruption tolerance, atomicity, sweep correctness). B2 - write path: - VectorIndex.key_hash_to_vec_checksum: Arc<HashMap<u64,u64>> (xxh64 of the HSET vector blob; the B3 rescan uses it to skip re-encoding unchanged keys). key_hash_to_global_id Arc-wrapped for O(1) capture. Maintained at auto-index insert, pruned in the shared tombstone body. - Compaction/merge persist the built segment (staged writer) when the store has a persist_dir - background worker writes off the shard thread; segment ids allocated ON the shard thread at submit. ImmutableSegment.disk_segment_id links installed segments to their on-disk ids for manifest assembly. - persist_hook_after_install (poll_install compact/merge + force paths): O(1) Arc capture on the shard thread; a dedicated single-worker SnapshotPool writes keymap-<epoch> then manifest (in that order - a manifest never references an unwritten keymap), then GCs superseded segment dirs/keymap epochs. Per-index seq + watermark Mutex held across the whole write+GC section: stale jobs drop, never reorder (BackgroundCompactor is multi-worker with NO per-index ordering - the guard is load-bearing). - drop_index / clear_all_contents delete the idx-<hex> persist dir (async best-effort); create_index floor-reads any surviving manifest so a drop/recreate generation never collides with old files. Crash contract: the manifest may UNDERSTATE what is on disk (crash between segment write and manifest commit -> orphan, swept at B3 recovery) but never overstates - understating costs rescan work, never correctness. Known accepted gaps (B3/B4 scope, documented in code): multi-field indexes checksum only the default field (missing checksum -> B3 treats as changed -> safe re-encode); drop+recreate same-name gets a fresh watermark Arc (old-generation in-flight jobs tolerated via id floors). Verification: vector lib 914 passed (+27), persistence 467 (+19), text 229; fmt + clippy -D warnings clean on default and runtime-tokio,jemalloc feature sets. author: Tin Dang * fix(text): remove clone_on_copy in TextStore::clear_all_contents BM25Config is Copy; the .clone() in the R3 flush-parity rebuild (eace11d3) trips clippy::clone_on_copy under -D warnings. Masked at commit time by clippy's crate-level cache; surfaced by the B3 agent's clean-tree run. author: Tin Dang * feat(vector): startup recovery from persisted segments + dedup rescan (B3) Phase B3 of tmp/VECTOR-DURABILITY-DESIGN.md - closes the persistence audit's R1 (discarded-recovery landmine) and R2 (restart cost scales with total vectors). With B1+B2 (2918f3c9) this completes the vector durability line: restart now loads compacted segments from disk and re-encodes only keys that changed. New src/vector/persistence/recover_v2.rs (RecoveryState; wired through the event_loop's existing index-restore loop, rescan loop, and a new finalize call - event_loop diff is wiring only): - create_index: reads the index's manifest; when present, recreates the index PINNING the recovered collection_id (QJL rotation seed - loaded codes are unsearchable under any other seed) via new VectorStore::create_index_with_collection_id, seeds next_collection_id / next_segment_id / global-id allocator from the manifest floors, loads each segment dir (disk_segment_id + suggested_ef restored, AE-1 estimator NOT re-run), installs the SegmentList, and loads the keymap into the three key-hash maps. - 4-level degradation ladder on segment-load failure: cid-matched load; cid mismatch -> drop that segment's keys via its own MVCC headers (new segment_io::read_mvcc_headers_only); headers-only parse -> same precise drop; nothing readable -> abandon ALL recovered state for the index (partial keymap + partial segments must never mix into stale search results). Crash contract: degrade to rebuild, never to wrong. - reconcile_key (dedup rescan): per matching key, xxh64 of the live vector blob vs the recovered checksum; unchanged across EVERY matching recovered index -> re-run auto_index_hset with the default vector field STRIPPED from args (existing metadata-only branch rebuilds PayloadIndex; TEXT/TAG/NUMERIC fields remain -> text re-derived; zero HNSW/TQ work). Any disagreement, unknown key, checksum 0 (documented multi-field gap) or fresh index -> tombstone stale copies then full re-encode. Conservative by design. - finish: deletion probe (keymap keys never observed by the rescan -> tombstoned), orphan sweep (staging/segment/keymap files unreferenced by the manifest; idx-* dirs with no sidecar index), and a per-index acceptance log: loaded segments / verified-unchanged / re-indexed / tombstoned. R1 removal: recovery.rs (624-line WAL-replay path) DELETED; Shard::recover_vectors + both call sites, pending_segments / attach_recovered / register_warm_segments / register_cold_segments removed (serena-verified unreferenced); the _discarded_vector_store comment now states the real contract; VectorWalRecord doc-headered as test-only. tests/crash_recovery_vacuum.rs trimmed of the two tests that exercised the deleted path. Known documented gap: the collection_id pin's merge-compatibility invariant is not exercisable by a search-only test (segments rotate by their own disk-reconstructed metadata); a full compact+merge cycle test lands with B4. Tests: recover_v2 unit tests incl. round-trip asserting verified_unchanged == n (proves the metadata-only branch fired, not just result equality) + corrupt-segment fallback + finish-tombstone probe. Full lib suite 3707 passed (default) / 3050 (tokio); fmt + clippy -D warnings clean both feature sets; integration smokes green (vector_flush_hdel_tombstone, ft_search_as_of_filter, crash_recovery_vacuum). author: Tin Dang * test(vector): B4 kill-9 crash-recovery suite + merge recall gate self-exclusion fix B4 closes the durability line (B1-B3, tmp/VECTOR-DURABILITY-DESIGN.md) with five end-to-end kill-9 crash-recovery scenarios against real server processes in tests/crash_recovery_vector_durability.rs: S1 unchanged-keys dedup fast path (rescan skips re-encoding) S2 updates+deletes reconcile across a crash (checksum + deletion probe) S3 orphan staging/segment/keymap files swept on boot S4 collection_id pin survives a post-recovery compact + GraphUnion merge S5 no-persist-dir regression guard (--appendonly no => no idx-* dirs) All waits are bounded condition polls (port accept / manifest / log line), never fixed sleeps. Servers spawn with --disk-free-min-pct 0 so the suite is immune to the dev volume hovering at Moon's 5% diskfull write-pause. S4 exposed a REAL pre-existing bug, fixed here: verify_merge_recall (src/vector/segment/compaction.rs) compared merged-graph HNSW results INCLUDING the query point (every sampled query is a database point, distance 0 -> always rank 1) against a brute-force ground truth that EXCLUDED it, structurally capping measurable recall at (k-1)/k = 0.90 -- exactly the manual FT.COMPACT/VACUUM VECTOR gate, so ANY manual merge of an index with >= MIN_RECALL_SAMPLE_N (50) vectors was rejected with "merge recall 0.9000 < tolerance 0.9000" regardless of graph quality. (Smoking gun: the reported recall was byte-identical, 0.8999959826..., across completely different datasets -- the f32 accumulation of Q x 0.9/Q.) The background 0.70 gate masked this. Fix: request k+1 from the merged graph and drop the self-point before comparison. Fallout fixes in existing tests: - tests/vector_segment_merge.rs: test_recall_gate_rejects_impossible_tolerance previously "passed" only because of the capped-at-0.9 bug (tolerance 1.0 could never be met). Honest recall on a clean merge is exactly 1.0 (the graph is verified in the decoded-centroid space it was built in), so the rejection/rollback path is now pinned with an unattainable tolerance > 1.0. - tests/vector_segment_merge.rs: merge_immutable call-site updated for the B1 persist parameter (missed in 2918f3c9; the suite had not been compiled since). Verified: crash_recovery_vector_durability 3x consecutive green (5/5), vector_segment_merge 13/13, vector_exact_rerank 7/7, crash matrix suites green serially, lib compact/merge unit tests green, fmt + clippy clean on default and runtime-tokio,jemalloc feature sets. author: Tin Dang * perf(vector): int8 symmetric ADC kernels for SQ8 search Replace the f32-widening per-candidate distance computation in SQ8's ADC (52-61% of FT.SEARCH query time per the prior vector deep review) with integer dot-product SIMD kernels. The query is quantized once per search to int8 symmetric (no zero point, qmax=127); per-candidate dot/sum/sumsq statistics against the persisted u8 SQ8 codes are then computed with exact-integer SIMD, reconstructed to f32, and fed into the existing (unchanged) sq8_l2_from_stats combine step. Kernel tiers (all gated behind sq8_i8_stats: Option<Sq8I8StatsFn> in DistanceTable, resolved once per query/chunk, None falls back to the pre-existing f32 sq8_stats path): - NEON (aarch64, baseline): vmull_s8 offset-trick widening multiply (c' = c ^ 0x80) + vpadalq accumulate. vdotq_s32/SDOT is gated behind the unstable stdarch_neon_dotprod feature on rustc 1.94 and was avoided entirely in favor of this portable shape. - AVX2 (x86_64): cvtepi8_epi16/cvtepu8_epi16 widen to i16 (exact) + mullo_epi16 (exact, |q*c| <= 127*255 < i16::MAX) + madd_epi16 accumulate. Deliberately NOT VPMADDUBSW (_mm256_maddubs_epi16) as originally designed -- that path requires qmax=63 to avoid signed saturation, and empirical recall A/B testing showed qmax=63 breaches the 0.01 R@10 gate (0.0165-0.0267 measured) while qmax=127 stays within bounds (0.003-0.0045). qmax=127 is used uniformly across all three tiers instead of the design doc's suggested 127/63 split. - AVX512-VNNI (x86_64, feature-gated): true _mm512_dpbusd_epi32 (VPDPBUSD) for dot_qc and sum_c (codes as the unsigned operand, quantized query/ones as the signed operand); sumsq_c can't use VPDPBUSD directly (codes can't be the signed operand) so it reuses the AVX2-style i16-widen + madd_epi16 shape. Recall safety: the existing exact-rerank sidecar (HQ-1) is untouched. Added an explicit recall A/B test (top-10 overlap >= 0.98 AND |R@10(f32) - R@10(int8)| <= 0.01) across L2 (dim 128, dim 384) and cosine-normalized (dim 128), N=5000 vectors / 200 queries, seeded LCG data, pure brute-force (no ANN gate). Extreme-value + tail-handling tests pin each SIMD kernel against an exact i64 scalar oracle. MOON_SQ8_INT8_ADC=0 forces the old f32 kernels (OnceLock-cached env read); documented in CHANGELOG, intentionally not added to CLAUDE.md's env var list per design-doc scope. Local measurement (NEON, this dev machine only -- not a production number): ~1.5x/2.0x/2.15x speedup at dim 128/384/768 via the new criterion bench variant (benches/sq8_adc_bench.rs). AVX2 and AVX512- VNNI kernels are compile-verified cross-target from aarch64 (x86_64-apple-darwin, including --features simd-avx512) but not runtime-executed this session -- they run under x86 CI/GCE. No changes to the persisted SQ8 on-disk format, tq_adc.rs/TQ4, or any existing f32 kernel path. Files: src/vector/turbo_quant/sq8.rs, src/vector/distance/{mod,neon, avx2,avx512}.rs, src/vector/hnsw/search.rs, src/vector/segment/ mutable.rs, benches/sq8_adc_bench.rs, CHANGELOG.md. author: Tin Dang * refactor(vector): safe indexing in int8 ADC scalar tails Apply the same UNSAFE_POLICY cleanup that PR #214 review applied to the f32 SQ8 stats kernels: the int8 symmetric ADC tails (avx2/avx512/neon) now iterate `qi8[i..n].iter().zip(codes[i..n])` instead of `get_unchecked` — the tail is at most one sub-vector-width pass, so bounds checks cost nothing and the unsafe surface stays confined to the SIMD intrinsics. Refs: PR #214 review follow-through author: Tin Dang --------- Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
This was referenced Jul 6, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
Vector-engine optimization + reliability branch from the 2026-07-05 deep review (
tmp/VECTOR-DEEP-REVIEW.md), validated end-to-end on GCE (both arches, vs v0.5.1 baseline). 8 commits, each independently reviewable:2b9b3792846910f6eb6c1ee830ce53e77df3346f3e27f0231261ab83098cf3e2scripts/vector-validate.py+scripts/gcloud-vector-soak.sh)Reliability: 3 defects found by the new churn soak, fixed red→green
mark_deleted_for_key— 20% of KNN results were deleted keys after 1 min of churn. Wire-level tests intests/vector_del_unindex.rs.test_bg_compact_update_before_freeze_survives_install.test_bg_merge_update_across_segments_survives.Invariant established: an entry tombstone is not a key deletion — reconcile kills must be scoped (live-sibling check / origin gid), never key_hash-wide.
GCE validation (branch vs v0.5.1, c4a ARM Axion + c3 x86 Sapphire Rapids)
Recall/QPS @20k random-Gaussian 384d unit vectors (single index,
COMPACT_THRESHOLD 4096):(Absolute R@10≈0.77 is the documented random-Gaussian-384d concentration artifact — real MiniLM embeddings score 0.96+; identical for both binaries.)
Soak (20 min,
appendonly yes, 60/25/10/5 search/update/insert/delete wire churn): 311k ops (ARM) / 377k ops (x86), 0 resurrections across all samples, 0/1000 self-recall lost-probe on both arches, RSS proportional to live growth (20k→36–39k live).Durability (kill -9 mid-churn, everysec + 5 s margin): 0/5000 settled keys missing, post-recovery recall 0.986, double-crash restart clean — both arches.
Full report:
tmp/VECTOR-SOAK-VALIDATION.md; raw verdictstmp/vec-soak-{c4a,c3}-standard-8.json(both"pass": true).Local CI parity
Release suite 3996 passed; tokio feature suite 3462 passed; clippy clean on default AND
runtime-tokio,jemalloc;cargo fmt --checkclean. Memory cost of the rerank sidecar: +2·dim B/vector (opt-out knob is a noted follow-up).vs RediSearch
The 2026-06 four-feature baseline had Moon winning vector insert 6–20× but trailing RediSearch on search QPS/recall — HQ-1/HQ-2 are the two levers from that review. A fresh head-to-head bench on this branch is running; results will be posted as a comment.
🤖 Generated with Claude Code
Summary by CodeRabbit
MERGE_RECALL_TOLERANCE.--ft-search-workers(including0to disable).FT.COMPACTquery latency/QPS impact.DEL/UNLINKunindexing consistency across dispatch paths (including transactions/pipelines).FT.INFOcluster-wide aggregation; preserved exact-rerank sidecar behavior and rejected unsupportedFT.CREATE ... MERGE_MODE KEEP_RAW.maxmemory.