Sync commits Feat/vector engine and fix critical issues

[TinDang97]

#27

Summary by CodeRabbit

• New Features

  • Added broader end-to-end vector search tests and moon-only consistency checks.

• Bug Fixes / Stability

  • DIM parameter validated to 1–65536.
  • Safer initialization paths and deterministic error messages to avoid undefined behavior.
  • Quantization and vector indexing hardened with fallbacks.
  • Refined auto-delete logic to avoid removing keys on field-only operations.

• Known Limitations

  • FILTER is not supported in multi-shard vector searches.

[TinDang97]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[TinDang97]

@claude review

[claude]

Claude finished @TinDang97's task in 15m 14s —— View job

---

Code Review

Tasks:

• Gather context
• Read key files
• Analyze issues
• Post review feedback

---

This is a large, technically ambitious PR adding a complete vector search engine (HNSW + IVF + TurboQuant). The algorithmic work is impressive, but there are several critical correctness/safety issues and rule violations that should be addressed before merging.

---

Critical Bugs

1. Deadlock: Lock Order Inversion (DB ↔ VectorStore)

This is the most serious issue. There are two concurrent paths that acquire the same two locks in opposite orders:

Local write path (handler_sharded.rs:1355-1379): DB write lock → vector_store lock

let mut guard = shard_databases.write_db(shard_id, db_idx);  // DB lock acquired
// ... dispatch ...
let mut vs = shard_databases.vector_store(shard_id);          // vector_store lock acquired WHILE DB still held

Shard event loop (event_loop.rs:424): vector_store lock → DB write lock

&mut *shard_databases.vector_store(shard_id),  // vector_store lock acquired
// drain_spsc_shared → write_db() inside spsc_handler.rs:204

Under concurrent cross-shard traffic, these two paths can deadlock any shard. The fix is to drop guard before acquiring vector_store, or establish a global lock order and enforce it everywhere. Fix this →

2. Undefined Behavior When distance::init() Not Called

src/vector/distance/mod.rs:157 and src/vector/distance/fastscan.rs:56 both use unwrap_unchecked() protected only by debug_assert!:

unsafe { DISTANCE_TABLE.get().unwrap_unchecked() }  // UB in release if not initialized

Any test or benchmark that uses vector code without going through main() will trigger UB in release builds. Use get_or_init(...) or OnceLock::get_or_init to make initialization automatic and safe. Fix this →

3. Unbounded DIM — DoS via OOM/Panic

ft_create only validates DIM > 0 then calls padded_dimension(dim) which uses next_power_of_two(). For dim > 2^31, this overflows and panics. For very large dims, it triggers massive allocations. The docs say 1–65536 is the intended range; enforce it in code. Fix this →

4. FILTER Silently Ignored in Multi-Shard Mode

handler_sharded.rs:1261 parses the filter expression but discards it (_filter) — it is never forwarded to scatter_vector_search_remote. Queries return different results in single-shard vs multi-shard mode with the same FILTER clause. Either propagate the filter through ShardMessage::VectorSearch or return an explicit error for filtered multi-shard queries. Fix this →

5. DEL k1 k2 Only Marks First Key as Deleted

handler_sharded.rs:1375 calls cmd_args.first() for DEL/UNLINK, so only the first key's vectors are removed. The SPSC path (spsc_handler.rs:290-294) correctly iterates all keys, creating an inconsistency. Fix this →

6. HDEL on Any Field Drops Vector from Index

Both spsc_handler.rs:285-288 and handler_sharded.rs:1373-1378 mark a key's vector as deleted on any HDEL, even when the deleted field is unrelated to the vector (e.g., HDEL doc:1 title). The fix requires checking whether the deleted fields include the index's source_field for that key's matching indexes. Fix this →

---

Rule Violations (CLAUDE.md)

7. unwrap() in Library Code

Multiple violations of the no-unwrap() rule outside tests:

• hnsw/search.rs:290: sub_table.unwrap() — panics if use_subcent is true but sub_centroid_table is None
• tq_adc.rs:596,676: partial_cmp(&b.0).unwrap() — panics on NaN distances. Use f32::total_cmp instead.
• codebook.rs:141,156: panic!("unsupported bit width: {bits}") — should return Result<Vec<f32>, Error>
• collection.rs:284-304: assert_eq! + try_into().unwrap() in codebook_16() / codebook_boundaries_15() — should return Option<&[f32; N]>

8. Hot-Path Allocations in src/command/

ft_info() at vector_search.rs:307-344 uses format!(), to_string(), and clone() — all prohibited in src/command/:

format!("{}", idx.meta.hnsw_ef_runtime)   // heap alloc
"auto".to_string()                          // heap alloc
format!("{:?}", idx.meta.quantization)      // heap alloc
Frame::BulkString(idx.meta.name.clone())    // clone

Use itoa for integer formatting and static Bytes for string constants.

Also: format!("{}", r.distance) in result building (line ~562) and to_string() at line 1191.

9. Instant::now() on the Command Path

ft_search() at vector_search.rs:402-405 calls std::time::Instant::now() + elapsed() on every search. This adds syscall overhead on the hot path. Use the shard-cached timestamp instead.

10. vector_search.rs Exceeds 1500 Lines

The file is 1526 lines — over the project limit. Split into submodules: parsing (parse.rs), execution (exec.rs), response building (response.rs), and tests.

11. FT.* Commands Missing from Test Scripts

scripts/test-consistency.sh and scripts/test-commands.sh contain no FT.* coverage. Per CLAUDE.md, new commands require entries in both scripts.

12. CUDA FWHT Kernel Not Wired to build.rs

src/vector/gpu/fwht_kernel.rs documents that the .cu kernel exists at src/gpu/kernels/turbo_quant_wht.cu but is not compiled via build.rs and always returns CudaNotAvailable. Per CLAUDE.md, kernels must be built via build.rs when --features gpu-cuda is enabled.

---

Minor Observations

• src/vector/segment/holder.rs (825 lines) and src/vector/segment/mutable.rs (792 lines) are approaching the limit and will likely need splitting as they grow.
• The VectorStore uses Arc<Mutex<>> in server/listener.rs per the Qodo review; the CLAUDE.md coding rules prefer parking_lot::Mutex.
• search.rs:740 has the same partial_cmp(...).unwrap() issue inside a test function — while tests are exempt from the no-unwrap rule, using f32::total_cmp would be more robust.

---

Summary: The vector engine implementation is architecturally sound, but the deadlock risk (issue #1) and UB (issue #2) are blocking issues. The HDEL/DEL correctness bugs (#5, #6) and silent FILTER discard (#4) will cause silent data integrity problems. The unwrap() violations should be systematically addressed. I'd recommend fixing the critical bugs before merging.

[coderabbitai]

Actionable comments posted: 11

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@scripts/test-commands.sh`:
- Around line 682-698: The test blocks updating PASS/FAIL for the vector
assertions do not increment TOTAL, causing the final summary to be wrong; update
the script so each assertion that modifies PASS or FAIL also increments TOTAL
(e.g., before or immediately after each conditional that updates PASS/FAIL for
the FT.INFO check, the FT.SEARCH check, and the FT.INFO after drop check),
ensuring TOTAL is incremented alongside PASS/FAIL for the checks involving
FT.INFO, FT.SEARCH, and FT.INFO after drop (symbols: TOTAL, PASS, FAIL, and the
FT-related checks/assert_moon usage).
- Around line 685-691: The test uses positional FT.SEARCH and command
substitution which strips null bytes; change the test to call FT.SEARCH using
the server's expected PARAMS form so extract_param_blob() is exercised: build
the 16‑byte binary vector into a temporary file with printf '\x...' (to preserve
null bytes), then invoke mcli FT.SEARCH with a query that references a parameter
name (e.g. "'@embedding:$p'") followed by PARAMS <count> <param-name> `@tempfile`
KNN 2 so the server reads the raw blob from the file instead of losing null
bytes via "$(…)" substitution.

In `@scripts/test-consistency.sh`:
- Around line 499-500: The grep -q call is causing the script to exit under set
-e before the PASS/FAIL accounting runs; replace the two-line pattern using $?
with a proper if/then wrapper that runs grep in the conditional (e.g., if echo
"$FT_INFO" | grep -q "vecidx"; then PASS=$((PASS + 1)); else FAIL=$((FAIL + 1));
echo "  FAIL: FT.INFO should show vecidx"; fi) so the grep exit status is
handled by the conditional instead of triggering set -e; update the block that
references FT_INFO, PASS, and FAIL accordingly.
- Around line 494-495: The two redis-cli HSET lines using bash command
substitution with printf containing NUL bytes (e.g., the lines with redis-cli -p
"$PORT_RUST" HSET vec:1 embedding "$(printf '\x00\x00\x80\x3f...')") corrupt
binary data because Bash truncates at NUL; replace each of these with a
redis-cli -x invocation that reads binary from stdin and generate the binary
payload with a short Python one-liner (via python -c writing struct.pack bytes
to stdout) piped into redis-cli -x HSET vec:1 embedding -p "$PORT_RUST" (and
similarly for vec:2), and make the same substitution for the identical pattern
in the other script (the line that matches the redis-cli ... HSET vec:*
embedding "$(printf ...)") so binary FLOAT32 blobs are preserved.

In `@src/command/vector_search/tests.rs`:
- Around line 636-680: The test test_vector_metrics_increment_decrement is flaky
because shared atomics VECTOR_INDEXES and VECTOR_SEARCH_TOTAL are modified by
other tests; wrap the metric snapshot+operation+assertion sequences with a
process-wide mutex to serialize access. Add a static global lock (e.g., a
once_cell::sync::Lazy< std::sync::Mutex<()> > METRICS_LOCK) in the tests module
and acquire METRICS_LOCK.lock().unwrap() around the FT.CREATE section
(before_create → ft_create → after_create → assert), around the FT.SEARCH
section (before_search → ft_search → after_search → assert), and around the
FT.DROPINDEX section (before_drop → ft_dropindex → after_drop → assert) so the
before/after snapshots cannot be disturbed by other tests; keep ft_create,
ft_search, ft_dropindex calls and the VECTOR_INDEXES/VECTOR_SEARCH_TOTAL loads
as-is.
- Around line 241-289: Add a duplicate of the existing FT.SEARCH tests that
explicitly forces the scalar distance backend before running the same
assertions: for each test (e.g. test_ft_search_dimension_mismatch and
test_ft_search_empty_index) call the scalar-backend initializer (for example
crate::vector::distance::init_scalar() or
crate::vector::distance::set_backend(Backend::Scalar) /
crate::vector::distance::force_scalar(true) depending on your API) instead of
crate::vector::distance::init(), then run ft_search(&mut store, &search_args)
and assert the same results; ensure the new tests reference ft_search and
crate::vector::distance so both SIMD and scalar paths are covered.

In `@src/server/conn/handler_monoio.rs`:
- Around line 1419-1437: The monoio handler is incorrectly rejecting FILTER and
always calling scatter_vector_search_remote even when num_shards == 1; update
the match branch that handles Ok((index_name, query_blob, k, filter)) so that
when num_shards == 1 it calls the local ft_search(...) path (the same way
handler_sharded.rs does) and preserves FILTER handling, otherwise keep the
existing scatter_vector_search_remote(...) call; use parse_ft_search_args,
ft_search, scatter_vector_search_remote, shard_databases, dispatch_tx and
spsc_notifiers to locate and implement the conditional routing.

In `@src/vector/distance/fastscan.rs`:
- Around line 55-57: Replace the panic-on-missing-initialization in
fastscan_dispatch by using FASTSCAN_DISPATCH.get_or_init(...) so the dispatch is
initialized automatically; implement the closure to detect AVX2 (cfg target_arch
= "x86_64" + is_x86_feature_detected!("avx2")) and return a FastScanDispatch
with scan_block calling fastscan_block_avx2 when available, otherwise return a
FastScanDispatch with scan_block = fastscan_block_scalar; update the
fastscan_dispatch function to return the reference from get_or_init and remove
the expect()/panic path and any safety precondition related to init_fastscan().

In `@src/vector/distance/mod.rs`:
- Around line 149-151: The table() accessor currently calls
DISTANCE_TABLE.get().expect(...), which can panic; replace that path with a
non-panicking access by using DISTANCE_TABLE.get_or_init(...) (or simply
DISTANCE_TABLE.get_or_init(|| /* previously initialized value or call to init
*/) and return the reference it yields) so library code no longer uses expect();
update the table() function to call get_or_init on the OnceLock<DistanceTable>
named DISTANCE_TABLE to retrieve the DistanceTable instance safely (matching
other uses in this module).

In `@src/vector/turbo_quant/codebook.rs`:
- Around line 141-144: The match arm that currently returns Vec::new() for
unsupported centroid bit widths should fail fast instead: change the function to
return a Result (e.g., Result<Vec<CentroidType>, Error>) and replace the
fallback arm(s) that return Vec::new() with an Err containing a descriptive
error like "unsupported centroid bit width {bits}" (do the same for the other
occurrence at the second match), update all successful branches to return
Ok(...), and propagate the Result to callers; mirror the stricter behavior used
by code_bytes_per_vector so configuration/programming errors are surfaced rather
than silently producing empty codebooks.

In `@src/vector/turbo_quant/collection.rs`:
- Around line 284-291: The current fallback returns a ZERO array when
self.codebook_boundaries.as_slice().try_into() fails, which hides metadata
corruption; change this to fail fast by removing the silent ZERO fallback and
instead return an Err (or propagate a Result) with a clear error message (e.g.,
"invalid codebook_boundaries length for 4-bit quantization") or panic with that
message if changing the signature isn’t feasible; apply the same change to the
other occurrence around lines 299-306 so any invalid
codebook_lengths/codebook_boundaries on methods using self.codebook_boundaries
and try_into are reported immediately rather than producing all-zero
quantization metadata.

🪄 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: f072b5bf-a9e3-4336-8f43-24fcb2016361

📥 Commits

Reviewing files that changed from the base of the PR and between 65224d1 and 7dee89f.

📒 Files selected for processing (13)

• scripts/test-commands.sh
• scripts/test-consistency.sh
• src/command/vector_search/mod.rs
• src/command/vector_search/tests.rs
• src/server/conn/handler_monoio.rs
• src/server/conn/handler_sharded.rs
• src/shard/spsc_handler.rs
• src/vector/distance/fastscan.rs
• src/vector/distance/mod.rs
• src/vector/hnsw/search.rs
• src/vector/turbo_quant/codebook.rs
• src/vector/turbo_quant/collection.rs
• src/vector/turbo_quant/tq_adc.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major

❓ Verification inconclusive

Script executed:

#!/bin/bash
set -euo pipefail

echo "Current vector smoke-test snippet:"
sed -n '685,691p' scripts/test-commands.sh

echo
echo "Current FT.SEARCH parser in the server:"
sed -n '660,699p' src/command/vector_search/mod.rs

echo
blob="$(printf '\x00\x00\x80\x3f\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00')"
printf 'bytes preserved by command substitution: %s\n' "$(printf '%s' "$blob" | wc -c)"

Repository: pilotspace/moon

---

Repository: pilotspace/moon
Exit code: 0

stdout:

Current vector smoke-test snippet:
    # Insert vectors via HSET (auto-indexed)
    mcli HSET doc:1 embedding "$(printf '\x00\x00\x80\x3f\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00')" >/dev/null 2>&1
    mcli HSET doc:2 embedding "$(printf '\x00\x00\x00\x00\x00\x00\x80\x3f\x00\x00\x00\x00\x00\x00\x00\x00')" >/dev/null 2>&1

    # FT.SEARCH — basic vector search
    FT_SEARCH=$(mcli FT.SEARCH myidx 4 "$(printf '\x00\x00\x80\x3f\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00')" KNN 2 2>&1)
    if echo "$FT_SEARCH" | grep -q "doc:"; then PASS=$((PASS + 1)); echo "  PASS: FT.SEARCH returns results"; else FAIL=$((FAIL + 1)); echo "  FAIL: FT.SEARCH returns results"; fi

Current FT.SEARCH parser in the server:
pub fn parse_ft_search_args(
    args: &[Frame],
) -> Result<(Bytes, Bytes, usize, Option<FilterExpr>), Frame> {
    if args.len() < 2 {
        return Err(Frame::Error(Bytes::from_static(
            b"ERR wrong number of arguments for 'FT.SEARCH' command",
        )));
    }

    let index_name = match extract_bulk(&args[0]) {
        Some(b) => b,
        None => return Err(Frame::Error(Bytes::from_static(b"ERR invalid index name"))),
    };

    let query_str = match extract_bulk(&args[1]) {
        Some(b) => b,
        None => return Err(Frame::Error(Bytes::from_static(b"ERR invalid query"))),
    };

    let (k, param_name) = match parse_knn_query(&query_str) {
        Some(parsed) => parsed,
        None => {
            return Err(Frame::Error(Bytes::from_static(
                b"ERR invalid KNN query syntax",
            )));
        }
    };

    let query_blob = match extract_param_blob(args, &param_name) {
        Some(blob) => blob,
        None => {
            return Err(Frame::Error(Bytes::from_static(
                b"ERR query vector parameter not found in PARAMS",
            )));
        }
    };

    let filter = parse_filter_clause(args);
    Ok((index_name, query_blob, k, filter))
}

bytes preserved by command substitution: 2

stderr:

/bin/bash: line 12: warning: command substitution: ignored null byte in input

---

Vector smoke test uses incompatible command format and loses binary data.

The server's FT.SEARCH parser (lines 660–699 in src/command/vector_search/mod.rs) expects the format:

FT.SEARCH <index> '<query>' PARAMS <count> <key> <vector-blob>

where the query string is parsed to extract a parameter name, then the vector blob is retrieved via extract_param_blob().

The test at lines 685–691 uses the incompatible positional form:

FT.SEARCH myidx 4 "$(printf '...')" KNN 2

Additionally, command substitution strips null bytes from the binary payload. The bash warning confirms this: ignored null byte in input, and the payload is reduced from 16 bytes to 2 bytes before being sent to the server.

The test cannot exercise the intended code path and will fail to parse correctly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/test-commands.sh` around lines 685 - 691, The test uses positional
FT.SEARCH and command substitution which strips null bytes; change the test to
call FT.SEARCH using the server's expected PARAMS form so extract_param_blob()
is exercised: build the 16‑byte binary vector into a temporary file with printf
'\x...' (to preserve null bytes), then invoke mcli FT.SEARCH with a query that
references a parameter name (e.g. "'@embedding:$p'") followed by PARAMS <count>
<param-name> `@tempfile` KNN 2 so the server reads the raw blob from the file
instead of losing null bytes via "$(…)" substitution.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Search coverage still misses the scalar backend.

These FT.SEARCH tests only call crate::vector::distance::init(), so x86 CI will exercise the runtime-selected SIMD path and leave the scalar fallback untested. Please run at least one end-to-end search assertion with the scalar backend forced on.
Based on learnings: Applies to **/*.rs : Add unit tests for both SIMD and scalar paths

Also applies to: 360-444, 599-623

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/command/vector_search/tests.rs` around lines 241 - 289, Add a duplicate
of the existing FT.SEARCH tests that explicitly forces the scalar distance
backend before running the same assertions: for each test (e.g.
test_ft_search_dimension_mismatch and test_ft_search_empty_index) call the
scalar-backend initializer (for example crate::vector::distance::init_scalar()
or crate::vector::distance::set_backend(Backend::Scalar) /
crate::vector::distance::force_scalar(true) depending on your API) instead of
crate::vector::distance::init(), then run ft_search(&mut store, &search_args)
and assert the same results; ensure the new tests reference ft_search and
crate::vector::distance so both SIMD and scalar paths are covered.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the file and the specific lines mentioned
cat -n src/vector/distance/fastscan.rs | head -70

Repository: pilotspace/moon

Length of output: 3214

---

🏁 Script executed:

# Let's also search for FASTSCAN_DISPATCH definition to understand its type
rg "FASTSCAN_DISPATCH" -B 3 -A 3

Repository: pilotspace/moon

Length of output: 1485

---

🏁 Script executed:

# Check if get_or_init is available and what the type signature looks like
rg "get_or_init" src/

Repository: pilotspace/moon

Length of output: 252

---

🏁 Script executed:

# Let's also check if there are other .expect() calls in the library code to understand the pattern
rg "\.expect\(" src/ --type rs | head -20

Repository: pilotspace/moon

Length of output: 85

---

Replace .expect() with get_or_init() for automatic initialization.

This code violates the library guideline against .expect() in non-test code. Instead of panicking on missing initialization, call get_or_init() with the scalar fallback as default:

pub fn fastscan_dispatch() -> &'static FastScanDispatch {
    FASTSCAN_DISPATCH.get_or_init(|| {
        #[cfg(target_arch = "x86_64")]
        {
            if is_x86_feature_detected!("avx2") {
                return FastScanDispatch {
                    scan_block: |codes, lut, dim_half, results| {
                        unsafe { fastscan_block_avx2(codes, lut, dim_half, results) }
                    },
                };
            }
        }
        FastScanDispatch {
            scan_block: fastscan_block_scalar,
        }
    })
}

This eliminates the panic path, removes the safety contract requirement, and follows the guideline: "No unwrap() or expect() in library code outside tests; use pattern matching or if let."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/vector/distance/fastscan.rs` around lines 55 - 57, Replace the
panic-on-missing-initialization in fastscan_dispatch by using
FASTSCAN_DISPATCH.get_or_init(...) so the dispatch is initialized automatically;
implement the closure to detect AVX2 (cfg target_arch = "x86_64" +
is_x86_feature_detected!("avx2")) and return a FastScanDispatch with scan_block
calling fastscan_block_avx2 when available, otherwise return a FastScanDispatch
with scan_block = fastscan_block_scalar; update the fastscan_dispatch function
to return the reference from get_or_init and remove the expect()/panic path and
any safety precondition related to init_fastscan().

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n src/vector/distance/mod.rs | sed -n '140,160p'

Repository: pilotspace/moon

Length of output: 811

---

🏁 Script executed:

# First, let's understand the file structure and find DISTANCE_TABLE definition
rg -n "DISTANCE_TABLE" src/vector/distance/mod.rs -A 3 -B 3

Repository: pilotspace/moon

Length of output: 767

---

🏁 Script executed:

# Check the full context around lines 149-151
head -n 160 src/vector/distance/mod.rs | tail -n 25 | cat -n

Repository: pilotspace/moon

Length of output: 894

---

Replace .expect() with .get_or_init() to maintain library code compliance.

The table() function uses .expect() in library code, violating the guideline requiring non-panicking access patterns. Switch to get_or_init() since it's already available on OnceLock<DistanceTable> and used elsewhere in the module.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/vector/distance/mod.rs` around lines 149 - 151, The table() accessor
currently calls DISTANCE_TABLE.get().expect(...), which can panic; replace that
path with a non-panicking access by using DISTANCE_TABLE.get_or_init(...) (or
simply DISTANCE_TABLE.get_or_init(|| /* previously initialized value or call to
init */) and return the reference it yields) so library code no longer uses
expect(); update the table() function to call get_or_init on the
OnceLock<DistanceTable> named DISTANCE_TABLE to retrieve the DistanceTable
instance safely (matching other uses in this module).

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

src/vector/turbo_quant/collection.rs (1)

220-233: ⚠️ Potential issue | 🟠 Major

Silent failure in constructor creates unusable metadata that passes checksum verification.

When scaled_centroids_n or scaled_boundaries_n fails, the constructor logs an error but continues building metadata with an empty codebook. This metadata:

1. Passes verify_checksum() (checksum computed on invalid state)
2. Causes downstream codebook_16() / codebook_boundaries_15() to return zero arrays
3. Produces silently wrong search results per the relevant code snippets

The constructor should propagate errors rather than create a metadata object that appears valid but is fundamentally broken.

🔧 Suggested approach: make constructor fallible

-    pub fn with_build_mode(
+    pub fn with_build_mode(
         collection_id: u64,
         dimension: u32,
         metric: DistanceMetric,
         quantization: QuantizationConfig,
         seed: u64,
         build_mode: BuildMode,
-    ) -> Self {
+    ) -> Result<Self, &'static str> {
         // ... sign flip generation unchanged ...

         let mut meta = Self {
             // ...
             codebook: if quantization.is_turbo_quant() {
-                scaled_centroids_n(padded, quantization.bits()).unwrap_or_else(|e| {
-                    tracing::error!("failed to compute codebook centroids: {e}");
-                    Vec::new()
-                })
+                scaled_centroids_n(padded, quantization.bits())?
             } else {
                 Vec::new()
             },
             codebook_boundaries: if quantization.is_turbo_quant() {
-                scaled_boundaries_n(padded, quantization.bits()).unwrap_or_else(|e| {
-                    tracing::error!("failed to compute codebook boundaries: {e}");
-                    Vec::new()
-                })
+                scaled_boundaries_n(padded, quantization.bits())?
             } else {
                 Vec::new()
             },
             // ...
         };
         meta.metadata_checksum = meta.compute_checksum();
-        meta
+        Ok(meta)
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/vector/turbo_quant/collection.rs` around lines 220 - 233, The constructor
currently swallows errors from scaled_centroids_n and scaled_boundaries_n and
substitutes empty Vecs, producing invalid metadata that still passes
verify_checksum; change the constructor (the function building the struct that
calls scaled_centroids_n and scaled_boundaries_n) to be fallible and propagate
those errors (return a Result) instead of logging and returning Vec::new(), so
failures bubble up to callers; update call sites to handle the Result and ensure
downstream callers like codebook_16 and codebook_boundaries_15 never receive
empty/invalid codebooks.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/vector/turbo_quant/collection.rs`:
- Around line 303-306: The docstring for the accessor (the method that returns
the codebook as a `&[f32; 16]`, e.g., codebook_as_array) is misleading—update it
to state clearly that this accessor is intended only for 4-bit quantization
implementations (TurboQuant4 / TurboQuantProd4) and that returning a zeroed
array is an error fallback for invariant violations rather than expected
behavior; also add a defensive check (debug_assert! or unreachable! / panic) in
codebook_as_array to catch non-4-bit usage in debug builds so callers are
notified of the invariant breach.

---

Duplicate comments:
In `@src/vector/turbo_quant/collection.rs`:
- Around line 220-233: The constructor currently swallows errors from
scaled_centroids_n and scaled_boundaries_n and substitutes empty Vecs, producing
invalid metadata that still passes verify_checksum; change the constructor (the
function building the struct that calls scaled_centroids_n and
scaled_boundaries_n) to be fallible and propagate those errors (return a Result)
instead of logging and returning Vec::new(), so failures bubble up to callers;
update call sites to handle the Result and ensure downstream callers like
codebook_16 and codebook_boundaries_15 never receive empty/invalid codebooks.

🪄 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: 7e723bc7-090d-47d5-9470-02d02a58be45

📥 Commits

Reviewing files that changed from the base of the PR and between 7dee89f and c74e379.

📒 Files selected for processing (11)

• scripts/test-commands.sh
• scripts/test-consistency.sh
• src/command/vector_search/tests.rs
• src/server/conn/handler_monoio.rs
• src/vector/distance/fastscan.rs
• src/vector/distance/mod.rs
• src/vector/turbo_quant/codebook.rs
• src/vector/turbo_quant/collection.rs
• src/vector/turbo_quant/encoder.rs
• src/vector/turbo_quant/inner_product.rs
• src/vector/turbo_quant/tq_adc.rs

✅ Files skipped from review due to trivial changes (3)

• scripts/test-consistency.sh
• src/vector/distance/fastscan.rs
• src/command/vector_search/tests.rs

🚧 Files skipped from review as they are similar to previous changes (4)

• src/vector/distance/mod.rs
• src/server/conn/handler_monoio.rs
• scripts/test-commands.sh
• src/vector/turbo_quant/codebook.rs

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/vector/turbo_quant/collection.rs`:
- Around line 220-233: CollectionMetadata is being constructed with empty
codebook and codebook_boundaries when scaled_centroids_n or scaled_boundaries_n
fail, producing an invalid collection; change the constructor to fail instead of
swallowing errors: when quantization.is_turbo_quant() and
scaled_centroids_n(...) or scaled_boundaries_n(...) returns Err, propagate that
error (or panic) so construction returns Result<Self, CollectionMetadataError>
(or panics) rather than inserting Vec::new(); update the constructor signature
and all call sites that create CollectionMetadata (and any tests) to handle the
Result, and ensure callers that rely on codebook_16() / codebook_boundaries_15()
won’t see silent all-zero data.

🪄 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: 1b66c7ab-5118-4955-9fe4-92315b479fbd

📥 Commits

Reviewing files that changed from the base of the PR and between c74e379 and 777be2b.

📒 Files selected for processing (1)

• src/vector/turbo_quant/collection.rs

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/command/vector_search/mod.rs (1)

421-424: ⚠️ Potential issue | 🟡 Minor

Latency metric removal leaves INFO command reporting stale data.

Per the code change, per-search latency tracking was removed—search_local_filtered() no longer calls record_search_latency(). However, src/command/connection.rs:174 still reads and exposes VECTOR_SEARCH_LATENCY_US in the INFO output. This will report either zero or a stale value from before this change.

Consider either:

1. Removing the metric from the INFO output and deleting VECTOR_SEARCH_LATENCY_US / record_search_latency() from src/vector/metrics.rs, or
2. Retaining latency tracking if this metric is valuable for observability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/command/vector_search/mod.rs` around lines 421 - 424, The INFO output is
still reading VECTOR_SEARCH_LATENCY_US even though search_local_filtered no
longer records per-search latency; either remove the stale metric usage or
restore latency recording. Option A: delete VECTOR_SEARCH_LATENCY_US and
record_search_latency() from src/vector/metrics.rs and remove references to that
metric in the INFO handler in connection.rs (stop exposing
VECTOR_SEARCH_LATENCY_US). Option B: reintroduce latency recording by calling
record_search_latency(...) inside search_local_filtered (or any wrapper that
runs each search) so VECTOR_SEARCH_LATENCY_US remains valid; update
search_local_filtered and ensure metrics::record_search_latency is imported and
called with the measured duration. Use the symbols search_local_filtered,
record_search_latency, and VECTOR_SEARCH_LATENCY_US to locate and change the
code.

♻️ Duplicate comments (2)

src/vector/turbo_quant/collection.rs (1)

289-332: ⚠️ Potential issue | 🟠 Major

The zero-filled fallback still masks corrupted metadata.

src/vector/hnsw/search.rs, src/vector/segment/mutable.rs, and src/vector/persistence/segment_io.rs all consume these accessors as valid 4-bit tables. Returning [0.0; N] here silently degrades scoring/encoding instead of surfacing the invariant break. Please fail the operation rather than substitute zeros.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/vector/turbo_quant/collection.rs` around lines 289 - 332, The accessors
codebook_boundaries_15 and codebook_16 currently log an error and return
zero-filled fallbacks which masks corrupted metadata; instead, make them fail
hard when the slice -> array conversion fails by replacing the Err(_) branches
with a panic (or unreachable!) that includes the actual len and a clear message
about violated construction invariants (remove the static ZERO fallback). Keep
the existing debug_asserts but ensure the panic is used in release builds so
callers like code in hnsw::search, segment::mutable, and persistence::segment_io
observe the failure rather than proceeding with zeros; update the doc comments
to state that these functions will panic if invariants are violated.

src/vector/turbo_quant/codebook.rs (1)

193-196: ⚠️ Potential issue | 🟠 Major

Don't use 0 as the unsupported-bit fallback here.

This turns an invariant failure into bad layout math. CollectionMetadata::code_bytes_per_vector() forwards self.quantization.bits(), so a valid QuantizationConfig::Sq8 metadata object now reports 0 bytes, and src/vector/turbo_quant/tq_adc.rs uses that size to compute bytes_per_code and slice the buffer. Please fail fast or return a Result here instead of a sentinel length.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/vector/turbo_quant/codebook.rs` around lines 193 - 196, The match arm
that returns 0 for unsupported bit widths must not return a sentinel 0; change
it to fail fast by replacing the "0" fallback with a panic (or a Result::Err)
that includes the unsupported bit width so callers (e.g.,
CollectionMetadata::code_bytes_per_vector() and consumers in
turbo_quant::tq_adc.rs) never get a bogus length; update the code in codebook.rs
where the match occurs to call panic!("unsupported bit width for
code_bytes_per_vector: {}", bits) (or change the surrounding function to return
Result and propagate an Err with that message) so the invalid
QuantizationConfig::Sq8 path fails immediately.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/vector/turbo_quant/collection.rs`:
- Around line 220-231: The code currently calls scaled_centroids_n(...)
.expect(...) and scaled_boundaries_n(...) .expect(...); replace each .expect()
with explicit pattern matching: call scaled_centroids_n(padded,
quantization.bits()) and match the Result — on Ok(v) return v for the codebook
field, on Err(e) call unreachable!() (or unreachable!("codebook centroids:
invalid bit width: {}", quantization.bits())) to express the invariant; do the
same for scaled_boundaries_n(...) used for codebook_boundaries, referencing
quantization.is_turbo_quant(), scaled_centroids_n, and scaled_boundaries_n to
locate the spots.

In `@src/vector/turbo_quant/tq_adc.rs`:
- Around line 320-333: The per-vector logging and f32::MAX sentinel in the
tq_adc hot path must be replaced by a one-time validation and abort: move
validation of centroid slice length (e.g., exactly 16 centroids for 4-bit) and
supported bit widths out of the inner branch in tq_l2_adc_scaled()/tq_adc.rs and
into the caller (brute_force_tq_adc_multibit()), perform these checks once
before scanning candidates, and return an error/abort the search if invariants
fail; then remove the per-vector tracing::error + f32::MAX fallback in the match
arms so the hot path no longer emits logs or produces sentinel distances. Ensure
you reference and validate the same symbols used in the diff (centroids, bits,
tq_l2_adc_scaled, brute_force_tq_adc_multibit) and propagate a non-OK result
instead of continuing.

---

Outside diff comments:
In `@src/command/vector_search/mod.rs`:
- Around line 421-424: The INFO output is still reading VECTOR_SEARCH_LATENCY_US
even though search_local_filtered no longer records per-search latency; either
remove the stale metric usage or restore latency recording. Option A: delete
VECTOR_SEARCH_LATENCY_US and record_search_latency() from src/vector/metrics.rs
and remove references to that metric in the INFO handler in connection.rs (stop
exposing VECTOR_SEARCH_LATENCY_US). Option B: reintroduce latency recording by
calling record_search_latency(...) inside search_local_filtered (or any wrapper
that runs each search) so VECTOR_SEARCH_LATENCY_US remains valid; update
search_local_filtered and ensure metrics::record_search_latency is imported and
called with the measured duration. Use the symbols search_local_filtered,
record_search_latency, and VECTOR_SEARCH_LATENCY_US to locate and change the
code.

---

Duplicate comments:
In `@src/vector/turbo_quant/codebook.rs`:
- Around line 193-196: The match arm that returns 0 for unsupported bit widths
must not return a sentinel 0; change it to fail fast by replacing the "0"
fallback with a panic (or a Result::Err) that includes the unsupported bit width
so callers (e.g., CollectionMetadata::code_bytes_per_vector() and consumers in
turbo_quant::tq_adc.rs) never get a bogus length; update the code in codebook.rs
where the match occurs to call panic!("unsupported bit width for
code_bytes_per_vector: {}", bits) (or change the surrounding function to return
Result and propagate an Err with that message) so the invalid
QuantizationConfig::Sq8 path fails immediately.

In `@src/vector/turbo_quant/collection.rs`:
- Around line 289-332: The accessors codebook_boundaries_15 and codebook_16
currently log an error and return zero-filled fallbacks which masks corrupted
metadata; instead, make them fail hard when the slice -> array conversion fails
by replacing the Err(_) branches with a panic (or unreachable!) that includes
the actual len and a clear message about violated construction invariants
(remove the static ZERO fallback). Keep the existing debug_asserts but ensure
the panic is used in release builds so callers like code in hnsw::search,
segment::mutable, and persistence::segment_io observe the failure rather than
proceeding with zeros; update the doc comments to state that these functions
will panic if invariants are violated.

🪄 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: b72a43bf-84bc-4a49-b127-1ff5ad81f8c0

📥 Commits

Reviewing files that changed from the base of the PR and between 777be2b and 6054c64.

📒 Files selected for processing (4)

• src/command/vector_search/mod.rs
• src/vector/turbo_quant/codebook.rs
• src/vector/turbo_quant/collection.rs
• src/vector/turbo_quant/tq_adc.rs

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Replace .expect() with an explicit invariant branch.

The fail-fast behavior is fine, but src/**/*.rs explicitly disallows unwrap()/expect() in library code. Use match + unreachable! (or return a typed error) so the invariant stays explicit without violating the repo rule.

♻️ Suggested adjustment

             codebook: if quantization.is_turbo_quant() {
-                scaled_centroids_n(padded, quantization.bits())
-                    .expect("codebook centroids: invalid bit width is a programming bug")
+                match scaled_centroids_n(padded, quantization.bits()) {
+                    Ok(v) => v,
+                    Err(_) => unreachable!("codebook centroids: invalid bit width is a programming bug"),
+                }
             } else {
                 // SQ8 doesn't use codebooks -- store empty Vec
                 Vec::new()
             },
             codebook_boundaries: if quantization.is_turbo_quant() {
-                scaled_boundaries_n(padded, quantization.bits())
-                    .expect("codebook boundaries: invalid bit width is a programming bug")
+                match scaled_boundaries_n(padded, quantization.bits()) {
+                    Ok(v) => v,
+                    Err(_) => unreachable!("codebook boundaries: invalid bit width is a programming bug"),
+                }
             } else {
                 Vec::new()
             },

As per coding guidelines, src/**/*.rs: No unwrap() or expect() in library code outside tests; use pattern matching or if let.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/vector/turbo_quant/collection.rs` around lines 220 - 231, The code
currently calls scaled_centroids_n(...) .expect(...) and
scaled_boundaries_n(...) .expect(...); replace each .expect() with explicit
pattern matching: call scaled_centroids_n(padded, quantization.bits()) and match
the Result — on Ok(v) return v for the codebook field, on Err(e) call
unreachable!() (or unreachable!("codebook centroids: invalid bit width: {}",
quantization.bits())) to express the invariant; do the same for
scaled_boundaries_n(...) used for codebook_boundaries, referencing
quantization.is_turbo_quant(), scaled_centroids_n, and scaled_boundaries_n to
locate the spots.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't log-and-continue inside the per-vector ADC hot path.

brute_force_tq_adc_multibit() calls this once per candidate. A bad centroid slice or unsupported bits will emit one error per vector and still return a heap of f32::MAX sentinels, which means arbitrary top-k output plus log spam. Validate these invariants once before the scan and abort the whole search instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/vector/turbo_quant/tq_adc.rs` around lines 320 - 333, The per-vector
logging and f32::MAX sentinel in the tq_adc hot path must be replaced by a
one-time validation and abort: move validation of centroid slice length (e.g.,
exactly 16 centroids for 4-bit) and supported bit widths out of the inner branch
in tq_l2_adc_scaled()/tq_adc.rs and into the caller
(brute_force_tq_adc_multibit()), perform these checks once before scanning
candidates, and return an error/abort the search if invariants fail; then remove
the per-vector tracing::error + f32::MAX fallback in the match arms so the hot
path no longer emits logs or produces sentinel distances. Ensure you reference
and validate the same symbols used in the diff (centroids, bits,
tq_l2_adc_scaled, brute_force_tq_adc_multibit) and propagate a non-OK result
instead of continuing.