perf(vector): beat-RediSearch wave 2 — SIMD rerank kernels, adaptive ef, int8 ADC, keyspace parity, full index durability#215
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds durable persistence and crash recovery for vector indexes (manifest/keymap/segments with B3 dedup rescan), an int8 symmetric ADC distance path for SQ8 with SIMD kernels, adaptive per-segment ef, exact-rerank f16 SIMD kernels, cached MVCC snapshots, FLUSHALL/FLUSHDB/HDEL index parity, and FT.INFO observability counters. ChangesVector Durability and Recovery
Int8 Symmetric ADC and Exact Rerank Kernels
FLUSHALL/FLUSHDB/HDEL Index Parity and Observability
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant EventLoop as Shard Event Loop
participant RecoveryState
participant Manifest as Manifest/Keymap (disk)
participant VectorStore
EventLoop->>RecoveryState: create_index(vector_store, idx_persist_root, meta)
RecoveryState->>Manifest: read_manifest_tolerant / read_keymap_tolerant
Manifest-->>RecoveryState: manifest + keymap entries (or None)
RecoveryState->>VectorStore: load segments + reconstruct key maps
loop for each scanned hash key
EventLoop->>RecoveryState: reconcile_key(key, args)
RecoveryState->>RecoveryState: compare live checksum vs recovered
alt unchanged
RecoveryState->>VectorStore: metadata-only dedup
else changed
RecoveryState->>VectorStore: full re-index
end
end
EventLoop->>RecoveryState: finish(vector_store, idx_persist_root)
RecoveryState->>VectorStore: tombstone missing keys
RecoveryState->>Manifest: sweep_orphans_from_disk
sequenceDiagram
participant Client
participant Handler as Connection Handler
participant VectorStore
participant TextStore
Client->>Handler: HDEL key vec_field
Handler->>Handler: dispatch command
alt success
Handler->>VectorStore: auto_hdel_vectors(key, fields)
VectorStore->>VectorStore: tombstone vector in matching indexes
end
Client->>Handler: FLUSHALL / FLUSHDB
Handler->>Handler: dispatch command
alt success
Handler->>VectorStore: auto_flush_indexes (clear contents)
Handler->>TextStore: clear_all_contents (preserve schema)
end
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…16C) 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
…, 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
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
…lanket 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
…tone (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
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
…O 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
…map 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
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
… (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
…-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
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
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
TinDang97
force-pushed
the
perf/vector-beat-redisearch
branch
from
8781e6e to
4e25920
Compare
Code Review by Qodo
Context used✅ Compliance rules (platform):
44 rules 1. sigkill() unsafe lacks SAFETY
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+61
to
+76
| fn find_moon_binary() -> PathBuf { | ||
| if let Ok(bin) = std::env::var("MOON_BIN") { | ||
| let p = PathBuf::from(bin); | ||
| if p.exists() { | ||
| return p; | ||
| } | ||
| } | ||
| let manifest = env!("CARGO_MANIFEST_DIR"); | ||
| let release = PathBuf::from(format!("{manifest}/target/release/moon")); | ||
| if release.exists() { | ||
| return release; | ||
| } | ||
| let debug = PathBuf::from(format!("{manifest}/target/debug/moon")); | ||
| if debug.exists() { | ||
| return debug; | ||
| } |
There was a problem hiding this comment.
1. find_moon_binary() falls back path 📘 Rule violation ▣ Testability
The new crash-recovery integration test resolves the server binary by falling back to target/release/moon / target/debug/moon when MOON_BIN is unset. This violates the requirement that integration tests must require an explicitly provided MOON_BIN to avoid accidentally running the wrong binary in CI/local environments.
Agent Prompt
## Issue description
`tests/crash_recovery_vector_durability.rs` falls back to `target/release/moon` and `target/debug/moon` when `MOON_BIN` is not set, which violates the integration-test requirement to set `MOON_BIN` explicitly.
## Issue Context
This test spawns real server processes; using a guessed binary path can accidentally run an out-of-date or wrong build artifact, making results non-deterministic across CI runners and developer machines.
## Fix Focus Areas
- tests/crash_recovery_vector_durability.rs[61-87]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+149
to
+155
| #[cfg(unix)] | ||
| fn sigkill(child: &mut Child) { | ||
| let pid = child.id() as i32; | ||
| unsafe { | ||
| libc::kill(pid, libc::SIGKILL); | ||
| } | ||
| let _ = child.wait(); |
There was a problem hiding this comment.
2. sigkill() unsafe lacks safety 📘 Rule violation ≡ Correctness
The new unsafe { libc::kill(...) } block has no adjacent // SAFETY: comment describing required
invariants. This violates the repo unsafe policy requirements enforced by the compliance checklist
item.
Agent Prompt
## Issue description
An `unsafe` block was added without the required adjacent `// SAFETY:` comment documenting invariants and why UB cannot occur.
## Issue Context
`UNSAFE_POLICY.md` requires every `unsafe` block to include a `// SAFETY:` comment describing the preconditions and why violating them would be UB.
## Fix Focus Areas
- tests/crash_recovery_vector_durability.rs[149-155]
- UNSAFE_POLICY.md[14-26]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+542
to
+559
| impl SnapshotPool { | ||
| fn new(num_workers: usize) -> Self { | ||
| let (job_tx, job_rx) = flume::unbounded::<SnapshotJob>(); | ||
| let workers: Vec<_> = (0..num_workers.max(1)) | ||
| .map(|i| { | ||
| let rx = job_rx.clone(); | ||
| // Startup-time OS thread spawn failure is unrecoverable | ||
| // (matches the existing `BackgroundCompactor::new` convention). | ||
| #[allow(clippy::unwrap_used)] | ||
| let handle = std::thread::Builder::new() | ||
| .name(format!("moon-vec-snapshot-{i}")) | ||
| .spawn(move || { | ||
| while let Ok(job) = rx.recv() { | ||
| run_snapshot_job(job); | ||
| } | ||
| }) | ||
| .expect("failed to spawn vector snapshot worker thread"); | ||
| handle |
There was a problem hiding this comment.
3. snapshotpool uses .expect() 📘 Rule violation ☼ Reliability
SnapshotPool::new uses .expect(...) when spawning worker threads, which can panic in production library code. This violates the rule disallowing unwrap()/expect() in non-test Rust library code.
Agent Prompt
## Issue description
Production code in `src/vector/persistence/manifest.rs` calls `.expect(...)` on thread spawn, which may panic and is disallowed in non-test library code.
## Issue Context
If thread creation fails (resource limits, OS error), the code currently panics. The compliance rule requires handling this without `expect()`/`unwrap()` in library code.
## Fix Focus Areas
- src/vector/persistence/manifest.rs[542-566]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+35
to
+36
| #![cfg(any(feature = "runtime-monoio", feature = "runtime-tokio"))] | ||
| #![allow(clippy::unwrap_used)] |
There was a problem hiding this comment.
4. unwrap_used lacks justification comment 📘 Rule violation ✧ Quality
New unwrap suppressions/usages were added without the required #[allow(clippy::unwrap_used)] + immediately preceding justification comment pattern. This violates the unwrap-audit requirement and makes new panics harder to review and ratchet in CI.
Agent Prompt
## Issue description
New `.unwrap()` usage and/or `#[allow(clippy::unwrap_used)]` were added without the required justification comment directly above the allow attribute (and in some cases without the allow at all).
## Issue Context
The compliance rule requires every remaining unwrap to be explicitly justified with a comment immediately above an attached `#[allow(clippy::unwrap_used)]` at the smallest reasonable scope.
## Fix Focus Areas
- tests/crash_recovery_vector_durability.rs[35-36]
- src/vector/store.rs[1819-1820]
- src/vector/persistence/manifest.rs[609-616]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1810
to
+1820
| // B2 (durability): FLUSH discards contents — the whole durability | ||
| // dir (segments/manifest/keymaps) goes with it. Best-effort, | ||
| // background (see `spawn_delete_index_persist_dir`); the | ||
| // immediately-following `create_index` reseeds id allocators | ||
| // above whatever the (possibly still-being-deleted) old manifest | ||
| // recorded, so the recreated index never collides with it. | ||
| self.spawn_delete_index_persist_dir(&name); | ||
| // Recreate from the same definition — a fresh, empty index. | ||
| // Also rewrites the sidecar and bumps the version token. | ||
| #[allow(clippy::unwrap_used)] // name was just removed above; create cannot collide | ||
| self.create_index(meta).unwrap(); |
There was a problem hiding this comment.
6. Persist-dir delete recreation race 🐞 Bug ☼ Reliability
spawn_delete_index_persist_dir() runs remove_dir_all(idx_dir) on a background thread, but clear_all_contents() immediately recreates the same-named index (reusing the same idx-<hex> path) after spawning the delete. The GC thread can therefore delete the newly recreated index’s durability directory/files or cause snapshot writes to fail mid-flight, breaking the “kill-9 proof” durability guarantees.
Agent Prompt
## Issue description
The durability directory deletion is asynchronous and uses `remove_dir_all()` on the live `idx-<hex>` path. When the index is recreated quickly (notably in `clear_all_contents()` and potentially DROP+recreate), the deletion thread can remove the new generation’s persistence state.
## Issue Context
- `clear_all_contents()` removes each index, calls `spawn_delete_index_persist_dir(&name)`, then immediately calls `create_index(meta)`.
- `create_index()` and all persistence writers use `index_persist_dir(..)` which resolves to the same `idx-<hex>` directory.
## Fix Focus Areas
- src/vector/store.rs[1738-1763]
- src/vector/store.rs[1794-1821]
- src/vector/store.rs[1547-1565]
### Suggested implementation direction
- Change `spawn_delete_index_persist_dir` to *first* atomically rename the directory to a unique tombstone path (e.g. `idx-<hex>.gc-<ts>-<rand>`), then `remove_dir_all()` the tombstoned path.
- If `rename` fails with NotFound, treat as already deleted.
- This guarantees new indexes can safely recreate `idx-<hex>` immediately.
- Alternatively (more intrusive): block on deletion completion before recreating, but the rename approach preserves non-blocking behavior.
- Add a regression test that simulates: create index → trigger async delete → immediately recreate → write a manifest/keymap/segment → ensure the recreated index’s persisted files remain present.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
PR Summary by QodoVector search: SIMD rerank kernels, int8 ADC, adaptive ef, keyspace parity, full index durability
AI Description
Diagram
High-Level Assessment
Files changed (46)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/shard/mod.rs (1)
196-196: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc comment.
restore_from_persistence_v2no longer performs vector recovery (Lines 259-264), so the "+ vector recovery" suffix is now inaccurate.✏️ Fix
- /// V2 recovery path: snapshot load + WAL v2 replay + vector recovery. + /// V2 recovery path: snapshot load + WAL v2 replay. Vector-store recovery + /// is handled later against `ShardSlice.vector_store` (B3), not here.🤖 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/mod.rs` at line 196, The doc comment for restore_from_persistence_v2 is stale because it still says “+ vector recovery,” but that step is no longer part of the function. Update the comment on restore_from_persistence_v2 to match the current recovery flow by removing the vector recovery mention and keeping only the snapshot load and WAL v2 replay description.src/shard/spsc_handler.rs (1)
698-710: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd HDEL/FLUSH parity to the remaining SPSC execute arms.
PipelineBatchandPipelineBatchSlottedalready callauto_hdel_vectorsandauto_flush_indexes, butShardMessage::Execute,ExecuteSlotted,MultiExecute, andMultiExecuteSlottedonly handleauto_delete_vectorshere. AnyHDELorFLUSHDB/FLUSHALLrouted through those paths can leave stale vector entries behind.🤖 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 698 - 710, The SPSC auto-delete handling in `spsc_handler` only covers DEL/UNLINK, so HDEL and FLUSH commands can miss vector cleanup in `ShardMessage::Execute`, `ExecuteSlotted`, `MultiExecute`, and `MultiExecuteSlotted`. Extend the existing auto-delete branch alongside `auto_delete_vectors` to also invoke the same HDEL and FLUSH cleanup helpers used by `PipelineBatch` and `PipelineBatchSlotted`, ensuring all remaining execute arms apply parity for key deletion and index flushing.
🧹 Nitpick comments (2)
tests/crash_recovery_vacuum.rs (1)
221-225: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDocumented
BUG:markers for the WAL-less write-intent gap. These describe a real durability gap (write intents lost on crash, replay window for a duplicatepoint_id). Since the surrounding tests only assert the current (lossy) behavior, this gap has no failing guard. Want me to open a tracking issue so it isn't lost in a test 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 `@tests/crash_recovery_vacuum.rs` around lines 221 - 225, The comments in crash_recovery_vacuum test document a real WAL write-intent durability gap but leave it as an untracked note; replace the ad hoc BUG markers with a proper tracking reference or issue link so the gap is explicitly owned. Update the surrounding test commentary in the crash_recovery_vacuum scenario to reference the write-intent persistence path in vector::mvcc::manager and wal_record instead of embedding unresolved BUG notes directly in the test.src/command/vector_search/hybrid.rs (1)
597-619: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
ef_defaulted: falsehere diverges from the heuristic-derivedef_search.
run_dense_knncomputesef_searchfrom the AE-1 heuristic wheneveridx.meta.hnsw_ef_runtime == 0(Line 572), but hardcodesef_defaulted: falsein both branches. Unlikeexecute.rs::search_local_raw/filtered(which setef_defaulted = idx.meta.hnsw_ef_runtime == 0), the hybrid dense stream therefore never benefits from per-segment min-ef selection. This is behavior-preserving (every segment runs at the full resolvedef, favoring recall), so it may be intentional for pre-fusion recall — but please confirm the divergence is deliberate rather than an oversight, and consider a comment noting why hybrid opts out.🤖 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/hybrid.rs` around lines 597 - 619, The `run_dense_knn` path is hardcoding `ef_defaulted: false` even though it already computes `ef_search` from the AE-1 heuristic, so please verify whether the hybrid search in `src/command/vector_search/hybrid.rs` is intentionally opting out of per-segment min-ef behavior used by `execute.rs::search_local_raw/filtered`. If this divergence is deliberate, add a clear comment near the `MvccContext` construction explaining why hybrid keeps the resolved `ef` for all segments; otherwise, change the `ef_defaulted` value to follow the same `idx.meta.hnsw_ef_runtime == 0` logic used elsewhere. Use `run_dense_knn` and the `MvccContext` initialization as the anchor points for the update.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/conn/handler_monoio/mod.rs`:
- Around line 1339-1358: The HDEL auto-tombstone path in the monoio handler can
delete indexed vectors without recording any undo in CrossStoreTxn, so TXN.ABORT
restores the hash but not the vector index. Update the HDEL handling around
auto_hdel_vectors in handler_monoio::mod to either skip this auto-tombstone
behavior while a cross-store transaction is active or record a matching undo
intent in CrossStoreTxn so rollback can restore the vector tombstone state
consistently.
In `@src/server/conn/handler_sharded/mod.rs`:
- Around line 1436-1461: FLUSHALL/FLUSHDB handling in
`handler_sharded::auto_flush_indexes` only updates the current shard’s stores,
but these commands are keyless and must clear every shard in a sharded
deployment. Update the `FLUSHDB`/`FLUSHALL` branch in `handler_sharded::mod` to
broadcast the flush action across all shards instead of using `with_shard`
locally, while keeping `HDEL` behavior unchanged because it is key-routed.
Ensure the existing `response`/`cmd` checks still guard the flush path and that
both `vector_store` and `text_store` are cleared on each shard.
In `@src/server/conn/handler_single.rs`:
- Around line 1152-1172: The FLUSHDB/FLUSHALL transaction handling in
handler_single.rs is incorrectly gated by the vector_store check, so text_store
clearing is skipped when vector_store is None. Update the MULTI/EXEC per-command
loop around the FLUSHDB/FLUSHALL branch to handle text_store independently of
vector_store, mirroring the non-transactional write path, so
ts.lock().clear_all_contents() still runs when only text indexing is enabled.
In `@src/shard/event_loop.rs`:
- Around line 1046-1050: The recovery path in `finish()` currently always runs
`sweep_unknown_index_dirs()` through `recovery_state.finish(...)`, even when
`load_index_metadata_with_weights()` failed and no vector sidecar metadata was
actually loaded. Update `event_loop::finish` and the `recovery_state.finish`
flow so the final sweep only happens when the vector sidecar metadata was
successfully loaded, or otherwise distinguish an unreadable
`vector-indexes.meta` from a genuinely empty index set to avoid deleting valid
`idx-*` directories.
In `@src/text/store.rs`:
- Around line 1515-1542: clear_all_contents currently resets the in-memory
TextIndex state but leaves the persisted FST sidecar intact, so also delete or
invalidate the {index}.fst data during this flush path. Update the
clear_all_contents method in TextStore to clear the FST sidecar alongside
rebuilding each TextIndex so startup cannot reload stale term-to-ID mappings
after a flush. Keep the change localized to the clear_all_contents and related
sidecar persistence handling used by the text-index feature.
In `@src/vector/persistence/manifest.rs`:
- Line 277: The pre-allocation in manifest parsing uses the unchecked header
count, which can trigger a huge allocation on corrupted input even though
checksum validation only covers payload data. Update the logic in the manifest
reader around the `count` handling and `Vec::with_capacity` so the capacity is
bounded by the maximum possible entry count derived from the payload size (or
otherwise safely capped) before allocating. Keep the existing per-entry read
guards in the manifest parsing path so corrupt data still fails gracefully
without crashing.
In `@tests/crash_recovery_vector_durability.rs`:
- Around line 1153-1157: The clustered-vectors comment is stale and contradicts
the test’s actual dimensionality. Update the documentation near the clustered
vector generation logic to match the current DIM constant used by the test, and
remove the outdated “DIM=8” rationale so the recall-test explanation stays
accurate. Use the surrounding clustered-vector generator and DIM constant
references to keep the comment aligned with the current SQ8 tie-bound setup.
---
Outside diff comments:
In `@src/shard/mod.rs`:
- Line 196: The doc comment for restore_from_persistence_v2 is stale because it
still says “+ vector recovery,” but that step is no longer part of the function.
Update the comment on restore_from_persistence_v2 to match the current recovery
flow by removing the vector recovery mention and keeping only the snapshot load
and WAL v2 replay description.
In `@src/shard/spsc_handler.rs`:
- Around line 698-710: The SPSC auto-delete handling in `spsc_handler` only
covers DEL/UNLINK, so HDEL and FLUSH commands can miss vector cleanup in
`ShardMessage::Execute`, `ExecuteSlotted`, `MultiExecute`, and
`MultiExecuteSlotted`. Extend the existing auto-delete branch alongside
`auto_delete_vectors` to also invoke the same HDEL and FLUSH cleanup helpers
used by `PipelineBatch` and `PipelineBatchSlotted`, ensuring all remaining
execute arms apply parity for key deletion and index flushing.
---
Nitpick comments:
In `@src/command/vector_search/hybrid.rs`:
- Around line 597-619: The `run_dense_knn` path is hardcoding `ef_defaulted:
false` even though it already computes `ef_search` from the AE-1 heuristic, so
please verify whether the hybrid search in `src/command/vector_search/hybrid.rs`
is intentionally opting out of per-segment min-ef behavior used by
`execute.rs::search_local_raw/filtered`. If this divergence is deliberate, add a
clear comment near the `MvccContext` construction explaining why hybrid keeps
the resolved `ef` for all segments; otherwise, change the `ef_defaulted` value
to follow the same `idx.meta.hnsw_ef_runtime == 0` logic used elsewhere. Use
`run_dense_knn` and the `MvccContext` initialization as the anchor points for
the update.
In `@tests/crash_recovery_vacuum.rs`:
- Around line 221-225: The comments in crash_recovery_vacuum test document a
real WAL write-intent durability gap but leave it as an untracked note; replace
the ad hoc BUG markers with a proper tracking reference or issue link so the gap
is explicitly owned. Update the surrounding test commentary in the
crash_recovery_vacuum scenario to reference the write-intent persistence path in
vector::mvcc::manager and wal_record instead of embedding unresolved BUG notes
directly in the test.
🪄 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: e57d0c1b-5509-476a-8a36-fec2090f3e0f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
CHANGELOG.mdCLAUDE.mdCargo.tomlbenches/sq8_adc_bench.rsdocs/commands.mddocs/vector-search-guide.mdsrc/command/vector_search/ft_info.rssrc/command/vector_search/ft_search/dispatch.rssrc/command/vector_search/ft_search/execute.rssrc/command/vector_search/ft_search/response.rssrc/command/vector_search/hybrid.rssrc/command/vector_search/hybrid_multi.rssrc/command/vector_search/recommend.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_sharded/mod.rssrc/server/conn/handler_single.rssrc/server/conn/shared.rssrc/shard/event_loop.rssrc/shard/mod.rssrc/shard/spsc_handler.rssrc/text/store.rssrc/vector/background_compact.rssrc/vector/distance/avx2.rssrc/vector/distance/avx512.rssrc/vector/distance/mod.rssrc/vector/distance/neon.rssrc/vector/f16.rssrc/vector/hnsw/search.rssrc/vector/mvcc/manager.rssrc/vector/persistence/manifest.rssrc/vector/persistence/mod.rssrc/vector/persistence/recover_v2.rssrc/vector/persistence/recovery.rssrc/vector/persistence/segment_io.rssrc/vector/persistence/wal_record.rssrc/vector/search_pool.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/crash_recovery_vacuum.rstests/crash_recovery_vector_durability.rstests/vector_flush_hdel_tombstone.rstests/vector_segment_merge.rs
💤 Files with no reviewable changes (1)
- src/vector/persistence/recovery.rs
Comment on lines
+1339
to
+1358
| // R4: HDEL of an indexed vector field tombstones it. | ||
| if !is_error && cmd.eq_ignore_ascii_case(b"HDEL") { | ||
| crate::shard::spsc_handler::auto_hdel_vectors( | ||
| &mut s.vector_store, | ||
| cmd_args, | ||
| ); | ||
| } | ||
|
|
||
| // R3: FLUSHALL/FLUSHDB clears index contents | ||
| // (FT.CREATE definitions survive). | ||
| if !is_error | ||
| && (cmd.eq_ignore_ascii_case(b"FLUSHDB") | ||
| || cmd.eq_ignore_ascii_case(b"FLUSHALL")) | ||
| { | ||
| crate::shard::spsc_handler::auto_flush_indexes( | ||
| &mut s.vector_store, | ||
| &mut s.text_store, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== handler_monoio slice ==\n'
sed -n '1318,1370p' src/server/conn/handler_monoio/mod.rs
printf '\n== auto_hdel_vectors definition ==\n'
rg -n "fn auto_hdel_vectors|auto_hdel_vectors\\(" src/shard/spsc_handler.rs -A40 -B20
printf '\n== record_vector / tombstone / undo / cross txn searches ==\n'
rg -n "record_vector|mark_deleted_for_key_in_index|tombstone|undo|cross_txn|in_cross_txn|HDEL|record_write" src -A6 -B6Repository: pilotspace/moon
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the transaction/undo-related files first.
printf '\n== files ==\n'
git ls-files 'src/**' | rg '(^src/(server/conn/handler_monoio/mod.rs|shard/spsc_handler.rs|.*txn.*|.*transaction.*|.*undo.*|.*vector.*\.rs)$|^src/.*(txn|undo|transaction).*.rs$)'
printf '\n== targeted search ==\n'
rg -n "record_vector|mark_deleted_for_key_in_index|auto_hdel_vectors|in_cross_txn|cross txn|cross-txn|abort|HDEL|tombstone" src -A8 -B8Repository: pilotspace/moon
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== auto_hdel_vectors slice ==\n'
sed -n '2210,2265p' src/shard/spsc_handler.rs
printf '\n== CrossStoreTxn vector undo definitions ==\n'
rg -n "fn record_vector|record_vector\\(|VectorIntent|vector undo|abort_cross_store_txn|record_vector_undo|tombston" src/transaction src -A20 -B10 | head -n 300
printf '\n== transaction abort implementation slice ==\n'
rg -n "abort_cross_store_txn|record_vector|VectorIntent|mark_deleted_for_key_hash|tombstoned_key_hashes" src/transaction src -A20 -B10 | head -n 300Repository: pilotspace/moon
Length of output: 42915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== auto_hdel_vectors implementation ==\n'
sed -n '2218,2258p' src/shard/spsc_handler.rs
printf '\n== CrossStoreTxn and abort slices ==\n'
sed -n '1,260p' src/transaction/mod.rs
printf '\n--- abort.rs ---\n'
sed -n '1,260p' src/transaction/abort.rsRepository: pilotspace/moon
Length of output: 22458
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== auto_hdel_vectors ==\n'
sed -n '2220,2260p' src/shard/spsc_handler.rs
printf '\n== cross-store txn/vector undo ==\n'
rg -n "record_vector\\(|tombstoned_key_hashes\\(|mark_deleted_by_key_hash\\(|abort_cross_store_txn|VectorIntent|record_delete\\(" src/transaction src/server src/vector -A8 -B8Repository: pilotspace/moon
Length of output: 42200
Track HDEL vector tombstones in TXN undo
auto_hdel_vectorsonly sees the requested field names, so a mixedHDELcan tombstone an indexed vector even when that vector field wasn’t actually removed.- The tombstone isn’t recorded in
CrossStoreTxn, soTXN.ABORTrestores the hash but leaves the vector index deleted. Add a matching undo intent or block this path inside an active cross-store txn.
🤖 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/server/conn/handler_monoio/mod.rs` around lines 1339 - 1358, The HDEL
auto-tombstone path in the monoio handler can delete indexed vectors without
recording any undo in CrossStoreTxn, so TXN.ABORT restores the hash but not the
vector index. Update the HDEL handling around auto_hdel_vectors in
handler_monoio::mod to either skip this auto-tombstone behavior while a
cross-store transaction is active or record a matching undo intent in
CrossStoreTxn so rollback can restore the vector tombstone state consistently.
Comment on lines
+1436
to
+1461
| // R4: HDEL of an indexed VECTOR field tombstones the vector | ||
| // in exactly the affected indexes (whole-key deletion is the | ||
| // DEL/UNLINK arm above; non-vector-field HDELs are no-ops). | ||
| if !matches!(response, Frame::Error(_)) | ||
| && cmd.eq_ignore_ascii_case(b"HDEL") | ||
| { | ||
| crate::shard::slice::with_shard(|s| { | ||
| crate::shard::spsc_handler::auto_hdel_vectors( | ||
| &mut s.vector_store, | ||
| cmd_args, | ||
| ); | ||
| }); | ||
| } | ||
| // R3: FLUSHALL/FLUSHDB clears vector + text index contents | ||
| // (FT.CREATE definitions survive, matching restart semantics). | ||
| if !matches!(response, Frame::Error(_)) | ||
| && (cmd.eq_ignore_ascii_case(b"FLUSHDB") | ||
| || cmd.eq_ignore_ascii_case(b"FLUSHALL")) | ||
| { | ||
| crate::shard::slice::with_shard(|s| { | ||
| crate::shard::spsc_handler::auto_flush_indexes( | ||
| &mut s.vector_store, | ||
| &mut s.text_store, | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any fan-out/broadcast handling of FLUSHALL/FLUSHDB across shards.
rg -nP -C4 '\bFLUSHALL\b|\bFLUSHDB\b' src/shard src/server --type=rust
rg -nP -C4 'fn is_multi_key_command' src --type=rustRepository: pilotspace/moon
Length of output: 9642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first.
ast-grep outline src/server/conn/handler_sharded/mod.rs --view expanded
ast-grep outline src/server/conn/shared.rs --view expanded
ast-grep outline src/shard/spsc_handler.rs --view expanded
# Read the local regions around the maintenance hooks and dispatch flow.
sed -n '1000,1125p' src/server/conn/handler_sharded/mod.rs
sed -n '1420,1485p' src/server/conn/handler_sharded/mod.rs
sed -n '2200,2245p' src/shard/spsc_handler.rs
sed -n '230,285p' src/server/conn/shared.rs
sed -n '395,470p' src/server/conn/shared.rsRepository: pilotspace/moon
Length of output: 22595
Broadcast FLUSHALL/FLUSHDB to all shards These commands are keyless here, so this hook only clears the current shard’s vector/text stores. On a sharded deployment, a flush on one connection leaves stale index contents behind on the other shards; HDEL can stay local because it is routed by 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/server/conn/handler_sharded/mod.rs` around lines 1436 - 1461,
FLUSHALL/FLUSHDB handling in `handler_sharded::auto_flush_indexes` only updates
the current shard’s stores, but these commands are keyless and must clear every
shard in a sharded deployment. Update the `FLUSHDB`/`FLUSHALL` branch in
`handler_sharded::mod` to broadcast the flush action across all shards instead
of using `with_shard` locally, while keeping `HDEL` behavior unchanged because
it is key-routed. Ensure the existing `response`/`cmd` checks still guard the
flush path and that both `vector_store` and `text_store` are cleared on each
shard.
Comment on lines
+1152
to
+1172
| } else if c.eq_ignore_ascii_case(b"HDEL") | ||
| && i < txn_results.len() | ||
| && !matches!(txn_results[i], Frame::Error(_)) | ||
| { | ||
| // R4 parity inside MULTI/EXEC. | ||
| crate::shard::spsc_handler::auto_hdel_vectors( | ||
| &mut vs.lock(), | ||
| a, | ||
| ); | ||
| } else if (c.eq_ignore_ascii_case(b"FLUSHDB") | ||
| || c.eq_ignore_ascii_case(b"FLUSHALL")) | ||
| && i < txn_results.len() | ||
| && !matches!(txn_results[i], Frame::Error(_)) | ||
| { | ||
| // R3 parity inside MULTI/EXEC (text | ||
| // store cleared via its own guard or | ||
| // the throwaway fallback store). | ||
| vs.lock().clear_all_contents(); | ||
| if let Some(ref ts) = text_store { | ||
| ts.lock().clear_all_contents(); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
FLUSHALL/FLUSHDB text-store clearing is unreachable when vector_store is None.
This whole per-command loop (including the new FLUSHDB/FLUSHALL branch at lines 1161-1173) sits inside the outer if let Some(ref vs) = vector_store { ... } guard (established above this hunk for the pre-existing HSET auto-index logic). When vector_store is None but text_store is Some(...) — a valid config (text-index enabled independently of vector search) — FLUSHALL/FLUSHDB run inside MULTI/EXEC will skip this block entirely, so ts.lock().clear_all_contents() (line 1171) never executes. Text-indexed hashes flushed inside a transaction stay searchable as ghosts, defeating the exact R3 guarantee this PR adds — and this only affects the transactional path; the non-transactional write-run (lines 2271-2296) does this correctly with independent if let checks.
🐛 Proposed fix: decouple text-store handling from the `vector_store` guard
- if let Some(ref vs) = vector_store {
- if let Frame::Array(ref txn_results) = result {
+ if vector_store.is_some() || text_store.is_some() {
+ if let Frame::Array(ref txn_results) = result {
let mut fallback_ts = crate::text::store::TextStore::new();
for (i, cmd_frame) in conn.command_queue.iter().enumerate() {
if let Some((c, a)) = extract_command(cmd_frame) {
if c.eq_ignore_ascii_case(b"HSET")
&& i < txn_results.len()
&& !matches!(txn_results[i], Frame::Error(_))
{
- if let Some(Frame::BulkString(key_bytes)) = a.first() {
- let mut store = vs.lock();
- ...
- }
+ if let Some(ref vs) = vector_store {
+ if let Some(Frame::BulkString(key_bytes)) = a.first() {
+ let mut store = vs.lock();
+ ...
+ }
+ }
} else if (c.eq_ignore_ascii_case(b"DEL") || c.eq_ignore_ascii_case(b"UNLINK"))
&& i < txn_results.len()
&& !matches!(txn_results[i], Frame::Error(_))
{
- crate::shard::spsc_handler::auto_delete_vectors(&mut vs.lock(), a);
+ if let Some(ref vs) = vector_store {
+ crate::shard::spsc_handler::auto_delete_vectors(&mut vs.lock(), a);
+ }
} else if c.eq_ignore_ascii_case(b"HDEL")
&& i < txn_results.len()
&& !matches!(txn_results[i], Frame::Error(_))
{
- crate::shard::spsc_handler::auto_hdel_vectors(&mut vs.lock(), a);
+ if let Some(ref vs) = vector_store {
+ crate::shard::spsc_handler::auto_hdel_vectors(&mut vs.lock(), a);
+ }
} else if (c.eq_ignore_ascii_case(b"FLUSHDB") || c.eq_ignore_ascii_case(b"FLUSHALL"))
&& i < txn_results.len()
&& !matches!(txn_results[i], Frame::Error(_))
{
- vs.lock().clear_all_contents();
+ if let Some(ref vs) = vector_store {
+ vs.lock().clear_all_contents();
+ }
if let Some(ref ts) = text_store {
ts.lock().clear_all_contents();
}
}
}
}
}
}Separately, auto_hdel_vectors (called at line 1157-1160) has the same requested-vs-actually-removed-field granularity gap flagged in src/server/conn/handler_monoio/mod.rs (lines 1339-1358) — not repeating the full analysis here.
🤖 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/server/conn/handler_single.rs` around lines 1152 - 1172, The
FLUSHDB/FLUSHALL transaction handling in handler_single.rs is incorrectly gated
by the vector_store check, so text_store clearing is skipped when vector_store
is None. Update the MULTI/EXEC per-command loop around the FLUSHDB/FLUSHALL
branch to handle text_store independently of vector_store, mirroring the
non-transactional write path, so ts.lock().clear_all_contents() still runs when
only text indexing is enabled.
Comment on lines
+1046
to
+1050
| if let Some(ref vdir) = vector_persist_dir { | ||
| crate::shard::slice::with_shard(|s| { | ||
| recovery_state.finish(&mut s.vector_store, vdir); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ast-grep outline src/shard/event_loop.rs --view expandedRepository: pilotspace/moon
Length of output: 223
🏁 Script executed:
rg -n "load_index_metadata_with_weights|create_index|finish\\(|sweep_unknown_index_dirs|known_index_hexes|vector_persist_dir|metas" src/shard/event_loop.rs src/shard -S
sed -n '860,1080p' src/shard/event_loop.rsRepository: pilotspace/moon
Length of output: 15340
🏁 Script executed:
fd recover_v2 src
rg -n "struct RecoveryState|fn create_index|fn finish|sweep_unknown_index_dirs|known_index_hexes|unknown `idx-|deletion probe|orphan sweep" src/vector src -SRepository: pilotspace/moon
Length of output: 250
🏁 Script executed:
ast-grep outline src/vector/persistence/recover_v2.rs --view expanded
sed -n '1,260p' src/vector/persistence/recover_v2.rsRepository: pilotspace/moon
Length of output: 13378
🏁 Script executed:
sed -n '260,540p' src/vector/persistence/recover_v2.rsRepository: pilotspace/moon
Length of output: 12087
Gate finish() on a loaded vector sidecar
When load_index_metadata_with_weights() returns None, known_index_hexes stays empty, but finish() still calls sweep_unknown_index_dirs(). That treats every idx-* dir as unknown and deletes durable vector index data after a corrupt or unreadable vector-indexes.meta. Gate the final sweep on loaded vector metadata, or distinguish “sidecar unreadable” from “no indexes exist”.
🤖 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/event_loop.rs` around lines 1046 - 1050, The recovery path in
`finish()` currently always runs `sweep_unknown_index_dirs()` through
`recovery_state.finish(...)`, even when `load_index_metadata_with_weights()`
failed and no vector sidecar metadata was actually loaded. Update
`event_loop::finish` and the `recovery_state.finish` flow so the final sweep
only happens when the vector sidecar metadata was successfully loaded, or
otherwise distinguish an unreadable `vector-indexes.meta` from a genuinely empty
index set to avoid deleting valid `idx-*` directories.
Comment on lines
+1515
to
+1542
| /// FLUSHALL/FLUSHDB parity (persistence-review R3): reset every text | ||
| /// index to an empty state (postings, term dicts, doc maps, TAG/NUMERIC | ||
| /// indexes) while KEEPING the FT.CREATE schema — mirroring restart | ||
| /// semantics. Without this, flushed hashes stayed matchable as ghost | ||
| /// documents until the next restart. | ||
| pub fn clear_all_contents(&mut self) { | ||
| #[cfg(feature = "text-index")] | ||
| { | ||
| let mut any = false; | ||
| for idx in self.indexes.values_mut() { | ||
| *idx = TextIndex::new_with_schema( | ||
| idx.name.clone(), | ||
| idx.key_prefixes.clone(), | ||
| idx.text_fields.clone(), | ||
| idx.tag_fields.clone(), | ||
| idx.numeric_fields.clone(), | ||
| idx.bm25_config, | ||
| ); | ||
| any = true; | ||
| } | ||
| if any { | ||
| self.bump_version(); | ||
| } | ||
| } | ||
| // Without the text-index feature no TextIndex constructor exists and | ||
| // no documents can have been indexed — nothing to clear. | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== file sizes ==\n'
wc -l src/text/store.rs src/vector/store.rs || true
printf '\n== outline text store ==\n'
ast-grep outline src/text/store.rs --view expanded | sed -n '1,220p'
printf '\n== outline vector store ==\n'
ast-grep outline src/vector/store.rs --view expanded | sed -n '1,220p'
printf '\n== search text store fst/persist ==\n'
rg -n "save_fst_sidecar_for_index|load_fst_sidecars|persist_dir|clear_all_contents|spawn_delete_index_persist_dir|FST" src/text/store.rs src/vector/store.rsRepository: pilotspace/moon
Length of output: 21456
🏁 Script executed:
set -euo pipefail
sed -n '1460,1565p' src/text/store.rs
printf '\n---\n'
sed -n '1760,1845p' src/vector/store.rs
printf '\n---\n'
rg -n "fn load_fst_sidecars|fn save_fst_sidecar_for_index|fst_sidecar|persist_dir|load_fst" src/text/store.rs src/vector/store.rs src/text -g '!**/target/**'Repository: pilotspace/moon
Length of output: 14891
🏁 Script executed:
set -euo pipefail
printf '\n== relevant text store slices ==\n'
sed -n '560,730p' src/text/store.rs
printf '\n---\n'
sed -n '930,1015p' src/text/store.rs
printf '\n---\n'
sed -n '1580,1650p' src/text/store.rs
printf '\n== sidecar format / load-save ==\n'
sed -n '240,360p' src/text/index_persist.rsRepository: pilotspace/moon
Length of output: 18257
🏁 Script executed:
set -euo pipefail
fd -a "fst_dict.rs" src || true
rg -n "fn expand_fuzzy|fn expand_prefix|build_fst_from_term_dict|expand_fuzzy_hashmap|expand_prefix_hashmap" src/textRepository: pilotspace/moon
Length of output: 2169
🏁 Script executed:
set -euo pipefail
printf '\n== fst_dict slice ==\n'
sed -n '1,260p' src/text/fst_dict.rs
printf '\n== fst_dict tests ==\n'
sed -n '260,430p' src/text/fst_dict.rsRepository: pilotspace/moon
Length of output: 15191
🏁 Script executed:
set -euo pipefail
sed -n '730,930p' src/text/store.rsRepository: pilotspace/moon
Length of output: 8119
Clear the FST sidecar on flush. clear_all_contents() resets the in-memory indexes, but leaving {index}.fst behind lets startup reload stale term→ID mappings into the fresh store. Once new terms are inserted, fuzzy/prefix queries can hit the wrong postings because term IDs are reused from zero after rebuild. Delete or invalidate the FST sidecar here as well.
🤖 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/text/store.rs` around lines 1515 - 1542, clear_all_contents currently
resets the in-memory TextIndex state but leaves the persisted FST sidecar
intact, so also delete or invalidate the {index}.fst data during this flush
path. Update the clear_all_contents method in TextStore to clear the FST sidecar
alongside rebuilding each TextIndex so startup cannot reload stale term-to-ID
mappings after a flush. Keep the change localized to the clear_all_contents and
related sidecar persistence handling used by the text-index feature.
| return None; | ||
| } | ||
|
|
||
| let mut entries = Vec::with_capacity(count as usize); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unbounded with_capacity from an unchecked header count can abort on corruption.
The payload checksum (lines 267-275) covers only payload, not the header's count field. A corrupt count (e.g. a single bitflip to a huge value) still passes the checksum, so Vec::with_capacity(count as usize) attempts an enormous allocation and panics/aborts — directly contradicting the tolerant-read contract ("a corrupt keymap costs a rescan of that index, never a crash", lines 233-237). The per-entry loop already fails gracefully via the read_u* guards, so only the pre-allocation needs bounding.
🛡️ Bound the pre-allocation to the largest possible entry count
- let mut entries = Vec::with_capacity(count as usize);
+ // `count` lives in the header, which the payload checksum does NOT cover, so
+ // a corrupt count must never drive an unbounded allocation. Each entry is at
+ // least 22 bytes (u64+u32+u64+u16), so cap the reservation accordingly; the
+ // per-entry read loop below still validates the true length.
+ const MIN_KEYMAP_ENTRY_LEN: usize = 8 + 4 + 8 + 2;
+ let cap = (count as usize).min(payload.len() / MIN_KEYMAP_ENTRY_LEN + 1);
+ let mut entries = Vec::with_capacity(cap);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let mut entries = Vec::with_capacity(count as usize); | |
| // `count` lives in the header, which the payload checksum does NOT cover, so | |
| // a corrupt count must never drive an unbounded allocation. Each entry is at | |
| // least 22 bytes (u64+u32+u64+u16), so cap the reservation accordingly; the | |
| // per-entry read loop below still validates the true length. | |
| const MIN_KEYMAP_ENTRY_LEN: usize = 8 + 4 + 8 + 2; | |
| let cap = (count as usize).min(payload.len() / MIN_KEYMAP_ENTRY_LEN + 1); | |
| let mut entries = Vec::with_capacity(cap); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vector/persistence/manifest.rs` at line 277, The pre-allocation in
manifest parsing uses the unchecked header count, which can trigger a huge
allocation on corrupted input even though checksum validation only covers
payload data. Update the logic in the manifest reader around the `count`
handling and `Vec::with_capacity` so the capacity is bounded by the maximum
possible entry count derived from the payload size (or otherwise safely capped)
before allocating. Keep the existing per-entry read guards in the manifest
parsing path so corrupt data still fails gracefully without crashing.
Comment on lines
+1153
to
+1157
| /// Generate `n_clusters * per_cluster` clustered vectors: cluster centers | ||
| /// are random directions in R^DIM, members are the center plus small | ||
| /// Gaussian noise. Low dimensionality (DIM=8) keeps this a meaningful | ||
| /// recall test (concentration-of-distance is a high-dimension problem — | ||
| /// see the repo gotcha on misleading random-Gaussian recall at high dims). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale comment: DIM=8 contradicts const DIM = 32. The file header (Lines 47-55) deliberately sets DIM = 32 to avoid SQ8 tie-bound ground truth, but this doc block still asserts "Low dimensionality (DIM=8)". Update it to avoid misleading future readers about the recall-test rationale.
🤖 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/crash_recovery_vector_durability.rs` around lines 1153 - 1157, The
clustered-vectors comment is stale and contradicts the test’s actual
dimensionality. Update the documentation near the clustered vector generation
logic to match the current DIM constant used by the test, and remove the
outdated “DIM=8” rationale so the recall-test explanation stays accurate. Use
the surrounding clustered-vector generator and DIM constant references to keep
the comment aligned with the current SQ8 tie-bound setup.
This was referenced Jul 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second wave of the vector-search line (stacked on #214, now rebased onto main): search hot-path SIMD + adaptive-ef performance work, keyspace-parity correctness fixes, full vector-index durability (persist + incremental recovery + kill-9 proof), a pre-existing merge recall-gate bug fix, and int8 symmetric ADC kernels — GCE-validated on both architectures.
Performance
e71bea58): NEON integer-rescale (aarch64) + F16C+FMA (x86_64) with scalar fallback; recall-gated SQ8 rerank skip.f332f481): thread-cached search scratch, Arc'd MVCC treemap snapshot, ryu score formatting.213b7b75, supersedes/retires the blanket EF-SPLITbe754eb6): a compact-time self-probe against the exact f16 sidecar certifies "trivially easy" segments to search at min-ef; hard segments keep the full beam. The blanket ef/√G split was measured to cost gaussian 5-seg R@10 0.9915 → 0.9295 and is documented as rejected.d5cc93e1): query quantized once per search (qmax=127 uniform); per-candidate stats via exact-integer SIMD — NEONvmull_s8offset-trick (SDOT is nightly-gated on rustc 1.94), AVX2 saturation-free i16-widen (VPMADDUBSW's qmax=63 clamp breached the recall gate and was dropped), AVX512-VNNIdpbusd.MOON_SQ8_INT8_ADC=0escape hatch. Recall A/B gates (top-10 overlap ≥ 0.98 vs f32) green.Correctness
eace11d3): flushes clear vector+text index contents while keeping FT.CREATE definitions;HDELof an indexed vector field tombstones exactly the affected indexes.5e8d3f47): GraphUnion merges that discard a real f16 sidecar warn + surface viaFT.INFOgraph_segments/segments_with_exact_rerank(additively merged across shards).a83edb7b, pre-existing bug):verify_merge_recallincluded the query point in HNSW results but excluded it from ground truth, structurally capping measurable recall at (k−1)/k = 0.90 — exactly the manualFT.COMPACT/VACUUM VECTORgate, so manual merges of any index ≥ 50 vectors could never pass. Now searches k+1 and drops the self-point.Durability (B1–B4)
2918f3c9): immutable segments persist at compact/merge install (staged write → fsync → atomic rename), with an atomically-written per-index manifest (collection_id / segment ids / allocator floors) and checksummed keymap snapshots, on a dedicated ordered SnapshotPool.d3effb01): startup reloads persisted segments (pinningcollection_idso the QJL rotation seed matches) and runs a dedup rescan — checksum-matched keys rebuild metadata-only (no re-encode), changed/unknown keys re-index, removed keys tombstone. Crash contract: manifests may understate, never overstate; all corruption degrades to rescan-rebuild, never wrong results. Vestigial WAL-replay vector recovery deleted.a83edb7b): 5-scenario crash suite (tests/crash_recovery_vector_durability.rs) — dedup fast path, update/delete reconcile, orphan sweep, collection_id pin through post-recovery merge, no-persistence regression guard. 3× consecutive green.Validation
cluster_*sleep-race flakes fixed by perf(persistence): local-leg group commit + BITOP/COPY/DEL/UNLINK coordinator leg durability #213 (pass in isolation).-D warnings(default +runtime-tokio,jemalloc), unsafe audit, full crash-matrix suites green.Notes for reviewers
raw_f16sidecar and int8 kernels never change stored format — SQ8 code layout (dim + 8trailer) is untouched; segments from pre-sidecar dirs degrade to ADC-only with loud accounting.author: Tin Dang
Summary by CodeRabbit
New Features
FT.INFOcoverage metrics.Bug Fixes
FLUSHALL/FLUSHDBso vector indexes clear correctly without leaving ghost results.HDELbehavior so removed vector fields are properly removed from search results.