fix(qdrant): relocate multivector normalization guard, close review round 2

A second review round found the round-1 normalization guard (in
create_multivector_collection) was reachable only through Engine::configure,
which --skip-upload skips entirely -- so a cosine (or omitted-distance)
multivector dataset run with --skip-upload against an out-of-band-loaded
collection would bypass the guard and publish a wrong recall number: exactly
the silent-wrong-result bug the guard was added to prevent, just reachable
through a different, first-class entry point.

Fix, per 9-way agent consensus: extract the check into a new
Dataset::ensure_multivector_normalized(), called from both
read_multivector_data and read_multivector_queries -- the two functions
every entry point (configure/upload, search, ground-truth profiling, and
skip-upload) actually funnels through -- with the Qdrant-side call kept as a
fail-fast belt-and-braces on the one path that does call configure. Also
fixed the error message, which falsely claimed a declared distance even when
none was given (distance() defaults to "cosine" silently).

Also addressed, from the same round:
- Two "overflow" unit tests asserted bare is_err() but actually tripped a
  different guard than their name claimed (verified by hand); renamed and
  reasserted on the real error message.
- Added a hand-written byte-literal test pinning the .mvec on-disk format
  (mirroring sparse_reader's CSR fixture), since every existing test
  round-tripped through the same writer+reader and couldn't catch a
  coherent endianness/block-order bug.
- Added unit tests for the relocated guard (cosine/angular/omitted all
  reject, dot passes) and a live --skip-upload integration test that
  reproduces the reviewer's exact scenario end-to-end.
- README: documented that only dot/euclid are accepted for multivector
  datasets, since cosine/angular/omitted (the common case) now hard-error.

Deferred (documented, not filed as issues per agent consensus -- filing on
a real org repo needs an explicit human go-ahead): a magic-bytes/version
word for the still-unshipped .mvec format, and the three items already
deferred from round 1.

Verified live against Qdrant v1.18.2: full 29-test integration suite green.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: fcostaoliveira

README.md

@@ -917,7 +917,11 @@ neighbours.jsonl # ground truth: one JSON array of ids per query line
 ```
 
 Register it in `datasets/datasets.json` with `"type": "multivector"` and the
-per-token `vector_size`/`distance`. Ground truth for a multivector dataset MUST
+per-token `vector_size`/`distance`. **Only `dot`/`euclid` are accepted** —
+`cosine`/`angular` (and an *omitted* `distance`, which defaults to `cosine`)
+hard-error at load, since neither the reader nor the generator apply per-token
+normalization yet; this is the most likely first-attempt failure, since
+omitting `distance` is common elsewhere in this file. Ground truth for a multivector dataset MUST
 be a genuine brute-force MaxSim ranking, not a heuristic — see
 `generate_multivector`'s doc comment in `src/synthetic.rs` for why the hybrid
 generator's "planted" shortcut does not carry over. The end-to-end path

src/bin/vector_db_benchmark/dataset.rs

@@ -704,8 +704,37 @@ impl Dataset {
         self.config.dataset_type.as_deref() == Some("multivector")
     }
 
+    /// Refuse a multivector dataset whose distance requires per-token
+    /// normalization, since neither `read_multivector_data`/
+    /// `read_multivector_queries` nor `generate_multivector`'s brute-force
+    /// ground truth apply it — silently proceeding would score
+    /// normalized-in-engine vectors against un-normalized ground truth, a
+    /// silent-wrong-result bug rather than a missing feature.
+    ///
+    /// Called from BOTH `read_multivector_data` and `read_multivector_queries`
+    /// (not just the Qdrant `configure` path) because `--skip-upload` skips
+    /// `configure`/`upload` entirely — an engine-side-only guard would leave
+    /// the search and ground-truth-profiling paths unprotected, which is
+    /// exactly the entry point `--skip-upload` exists to use (#238).
+    pub fn ensure_multivector_normalized(&self) -> Result<(), String> {
+        if !self.needs_normalization() {
+            return Ok(());
+        }
+        let declared = match self.config.distance.as_deref() {
+            Some(d) => format!("declares distance '{}'", d),
+            None => "declares no distance (defaults to 'cosine')".to_string(),
+        };
+        Err(format!(
+            "multivector dataset '{}' {}, which requires per-token normalization \
+             that read_multivector_data/read_multivector_queries do not yet apply — \
+             refusing rather than silently scoring against un-normalized ground truth",
+            self.config.name, declared
+        ))
+    }
+
     /// Read multi-vector upload data from `<dir>/data.mvec`. Ids are the row indices.
     pub fn read_multivector_data(&self) -> Result<(Vec<i64>, Vec<MultiVector>), String> {
+        self.ensure_multivector_normalized()?;
         let dir = self.get_path()?;
         // Same declared-vs-actual cross-check as read_vectors/read_hybrid_data (#224).
         self.validate_vector_count()?;
@@ -723,6 +752,7 @@ impl Dataset {
     /// row-alignment guard: a short or misaligned ground truth must fail loudly
     /// rather than index out of bounds or score the wrong query.
     pub fn read_multivector_queries(&self) -> Result<(Vec<MultiVector>, Vec<Vec<i64>>), String> {
+        self.ensure_multivector_normalized()?;
         let dir = self.get_path()?;
         let queries = read_multivector_matrix(
             dir.join("queries.mvec")
@@ -1330,4 +1360,73 @@ mod tests {
         let ds = hybrid_dataset(p);
         assert!(ds.read_hybrid_data(false).is_err());
     }
+
+    /// A multivector dataset rooted at `dir`, with a configurable (possibly
+    /// omitted) `distance` — the exact axis the normalization guard branches on.
+    fn multivector_dataset(dir: &std::path::Path, distance: Option<&str>) -> Dataset {
+        let mut cfg = hybrid_dataset(dir).config;
+        cfg.name = "multivector-unit".to_string();
+        cfg.dataset_type = Some("multivector".to_string());
+        cfg.distance = distance.map(|d| d.to_string());
+        cfg.vector_count = None;
+        Dataset::new(cfg)
+    }
+
+    /// The normalization guard must fire from BOTH `read_multivector_data` and
+    /// `read_multivector_queries` — not just the Qdrant-side `configure` call —
+    /// because `--skip-upload` never calls `configure`/`upload` at all, yet
+    /// still reaches search and ground-truth profiling through these two
+    /// functions (#316 review round 2). Uses a nonexistent directory: the
+    /// guard must fire BEFORE any file access, so no fixture files are needed.
+    #[test]
+    fn multivector_normalization_guard_covers_both_read_paths() {
+        let dir = std::path::Path::new("/nonexistent/multivector-guard-test");
+        for distance in [Some("cosine"), Some("angular"), None] {
+            let ds = multivector_dataset(dir, distance);
+            let data_err = ds.read_multivector_data().unwrap_err();
+            assert!(
+                data_err.contains("per-token normalization"),
+                "distance={distance:?}: read_multivector_data got: {data_err}"
+            );
+            let queries_err = ds.read_multivector_queries().unwrap_err();
+            assert!(
+                queries_err.contains("per-token normalization"),
+                "distance={distance:?}: read_multivector_queries got: {queries_err}"
+            );
+        }
+    }
+
+    /// The error message must distinguish an EXPLICIT cosine/angular
+    /// declaration from an OMITTED `distance` (which defaults to cosine) —
+    /// `dataset.distance()` collapses both to `"cosine"`, so building the
+    /// message from it alone would claim a declaration that isn't there.
+    #[test]
+    fn multivector_normalization_error_distinguishes_declared_from_omitted_distance() {
+        let dir = std::path::Path::new("/nonexistent/multivector-guard-test");
+        let declared = multivector_dataset(dir, Some("cosine"))
+            .read_multivector_queries()
+            .unwrap_err();
+        assert!(
+            declared.contains("declares distance 'cosine'"),
+            "got: {declared}"
+        );
+        let omitted = multivector_dataset(dir, None)
+            .read_multivector_queries()
+            .unwrap_err();
+        assert!(omitted.contains("declares no distance"), "got: {omitted}");
+    }
+
+    /// A dataset that does NOT need normalization (dot/l2) must not trip the
+    /// guard — it should fail later, on the missing files, not on distance.
+    #[test]
+    fn multivector_dot_distance_does_not_trip_the_normalization_guard() {
+        let dir = std::path::Path::new("/nonexistent/multivector-guard-test");
+        let err = multivector_dataset(dir, Some("dot"))
+            .read_multivector_queries()
+            .unwrap_err();
+        assert!(
+            !err.contains("per-token normalization"),
+            "dot distance must not trip the normalization guard, got: {err}"
+        );
+    }
 }

src/bin/vector_db_benchmark/engine/qdrant.rs

@@ -622,23 +622,14 @@ impl QdrantEngine {
     /// HNSW is applied separately below via `hnsw_config_diff()`, same as the
     /// dense and hybrid paths.
     fn create_multivector_collection(&mut self, dataset: &Dataset) -> Result<(), String> {
-        // `read_multivector_data`/`read_multivector_queries` have no `normalize`
-        // parameter and `generate_multivector`'s brute-force ground truth scores
-        // raw (unnormalized) dot products unconditionally. Silently proceeding
-        // on a cosine (or omitted-distance, which defaults to cosine) dataset
-        // would score normalized-in-Qdrant vectors against un-normalized ground
-        // truth — a silent-wrong-result bug, not a missing feature. Refuse
-        // loudly until per-token normalization is actually threaded through.
-        if dataset.needs_normalization() {
-            return Err(format!(
-                "multivector dataset '{}' declares distance '{}', which requires \
-                 per-token normalization that read_multivector_data/read_multivector_queries \
-                 do not yet apply — refusing rather than silently scoring against \
-                 un-normalized ground truth",
-                dataset.config.name,
-                dataset.distance()
-            ));
-        }
+        // The authoritative check lives on `Dataset` (`ensure_multivector_normalized`)
+        // because it must also cover `read_multivector_data`/`read_multivector_queries`
+        // directly — `--skip-upload` never calls `configure` (and so never reaches
+        // this function), but search and ground-truth profiling still read via
+        // those two functions, so an engine-side-only guard would leave that path
+        // unprotected. This call is fail-fast belt-and-braces: it rejects before
+        // any Qdrant RPC, on the one path that DOES call `configure`.
+        dataset.ensure_multivector_normalized()?;
 
         let distance = dataset.distance();
         let vector_size = dataset.vector_size();

src/readers/multivector_reader.rs

@@ -206,6 +206,55 @@ mod tests {
         f
     }
 
+    /// GOLDEN test: hand-written bytes, no writer involved — mirrors
+    /// `sparse_reader::csr_row_count_decodes_a_literal_little_endian_fixture`.
+    ///
+    /// Every other test here round-trips through `write_multivector_matrix`
+    /// then `read_multivector_matrix` / `mvec_row_count`. Since the writer and
+    /// reader live in this same file and share the same assumptions, a
+    /// COHERENT bug — an endianness flip, or swapping the relative order of
+    /// the token-count block and the value block — would leave every one of
+    /// those tests green, because every party to the comparison flipped
+    /// together. `num_rows` and `dim` are given distinct values, and the two
+    /// rows distinct token counts, so a field-order or block-order swap
+    /// decodes to a wrong shape rather than accidentally matching.
+    #[test]
+    fn mvec_decodes_a_literal_little_endian_fixture() {
+        #[rustfmt::skip]
+        let bytes: &[u8] = &[
+            // num_rows = 2
+            0x02, 0x00, 0x00, 0x00,
+            // dim = 1
+            0x01, 0x00, 0x00, 0x00,
+            // token_count[0] = 1, token_count[1] = 2
+            0x01, 0x00, 0x00, 0x00,
+            0x02, 0x00, 0x00, 0x00,
+            // values, row-major: row0 = [10.0], row1 = [20.0, 30.0]
+            0x00, 0x00, 0x20, 0x41, // 10.0
+            0x00, 0x00, 0xA0, 0x41, // 20.0
+            0x00, 0x00, 0xF0, 0x41, // 30.0
+        ];
+        let f = write_tmp(bytes);
+        assert_eq!(
+            mvec_row_count(f.path().to_str().unwrap()).unwrap(),
+            2,
+            "num_rows must decode little-endian"
+        );
+        let rows = read_multivector_matrix(f.path().to_str().unwrap()).unwrap();
+        assert_eq!(
+            rows,
+            vec![
+                MultiVector {
+                    vectors: vec![vec![10.0]]
+                },
+                MultiVector {
+                    vectors: vec![vec![20.0], vec![30.0]]
+                },
+            ],
+            "header, token-count block, and value block must decode in the documented order"
+        );
+    }
+
     #[test]
     fn round_trips_multivector_matrix() {
         let rows = vec![
@@ -261,23 +310,38 @@ mod tests {
         assert!(err.contains("not a readable multivector file"), "{err}");
     }
 
+    /// `num_rows = u32::MAX` claims a token-count array far larger than the
+    /// 8-byte file could hold. `n.checked_mul(4)` itself cannot overflow here —
+    /// `u32::MAX * 4` fits comfortably in a 64-bit `usize` — so despite the
+    /// name this trips `read_u32_array`'s `max_bytes` cap, not an arithmetic
+    /// overflow. Asserting on the message (rather than bare `is_err()`) is
+    /// what makes that the test's actual claim.
     #[test]
-    fn rejects_token_count_size_overflow() {
+    fn rejects_a_declared_row_count_the_file_cannot_hold() {
         let mut b = Vec::new();
         b.extend_from_slice(&u32::MAX.to_le_bytes()); // num_rows
         b.extend_from_slice(&16u32.to_le_bytes()); // dim
         let f = write_tmp(&b);
-        assert!(read_multivector_matrix(f.path().to_str().unwrap()).is_err());
+        let err = read_multivector_matrix(f.path().to_str().unwrap()).unwrap_err();
+        assert!(err.contains("token-count bytes but file is only"), "{err}");
     }
 
+    /// `total_tokens = dim = u32::MAX` makes `total_tokens.checked_mul(dim)` ≈
+    /// 1.8446744065×10^19, which — deliberately — does NOT overflow `u64`
+    /// (max ≈1.8446744074×10^19), so that guard and the `usize::MAX` check
+    /// right after it both pass. The `Err` this test relies on actually comes
+    /// one level down, from `read_f32_array`'s own `n.checked_mul(4)`
+    /// overflowing on that same huge count. Asserting on the message is what
+    /// pins the test to the branch it actually exercises.
     #[test]
-    fn rejects_total_value_count_overflow() {
+    fn rejects_value_block_byte_size_overflow() {
         let mut b = Vec::new();
         b.extend_from_slice(&1u32.to_le_bytes()); // num_rows = 1
         b.extend_from_slice(&u32::MAX.to_le_bytes()); // dim
         b.extend_from_slice(&u32::MAX.to_le_bytes()); // token_count[0]
         let f = write_tmp(&b);
-        assert!(read_multivector_matrix(f.path().to_str().unwrap()).is_err());
+        let err = read_multivector_matrix(f.path().to_str().unwrap()).unwrap_err();
+        assert!(err.contains("multivector value size overflow"), "{err}");
     }
 
     #[test]

tests/common/mod.rs

@@ -1724,6 +1724,18 @@ pub struct MultiVectorProject {
 pub fn write_multivector_project(
     dataset_name: &str,
     engine_configs_json: &str,
+) -> MultiVectorProject {
+    write_multivector_project_with_distance(dataset_name, engine_configs_json, "dot")
+}
+
+/// Same fixture as [`write_multivector_project`], but with a caller-chosen
+/// `distance` — used to exercise the normalization guard against a
+/// cosine/angular (or omitted) multivector dataset, which this repo does not
+/// yet support (#316 review round 2).
+pub fn write_multivector_project_with_distance(
+    dataset_name: &str,
+    engine_configs_json: &str,
+    distance: &str,
 ) -> MultiVectorProject {
     const DIM: usize = 16;
     const MIN_TOKENS: usize = 4;
@@ -1752,7 +1764,7 @@ pub fn write_multivector_project(
 
     let datasets_json = serde_json::json!([{
         "name": dataset_name, "type": "multivector", "path": dataset_name,
-        "distance": "dot", "vector_size": DIM,
+        "distance": distance, "vector_size": DIM,
     }]);
     fs::write(
         root.join("datasets/datasets.json"),

tests/integration_qdrant.rs

@@ -675,6 +675,96 @@ fn test_binary_qdrant_multivector() {
     );
 }
 
+/// The normalization guard must survive `--skip-upload`, which skips
+/// `configure`/`upload` entirely and so never reaches the Qdrant-side check
+/// inside `create_multivector_collection` — yet search still reads via
+/// `Dataset::read_multivector_queries`, which is where the guard now also
+/// lives (#316 review round 2: the guard was originally placed ONLY in
+/// `create_multivector_collection`, leaving this exact entry point open).
+///
+/// Reproduces the reviewer's scenario precisely: upload once under a
+/// `dot`-distance registration (the guard doesn't fire, so a real 150-point
+/// corpus lands in Qdrant), then re-register the SAME corpus under `cosine`
+/// and rerun with `--skip-upload` — corpus reuse succeeds (the collection
+/// genuinely holds all 150 points), so the run reaches search, which must now
+/// fail on the normalization guard rather than publish a wrong recall number.
+#[test]
+fn test_binary_qdrant_multivector_skip_upload_rejects_cosine() {
+    wait_for_qdrant();
+    delete_collection();
+
+    let configs = serde_json::json!([{
+        "name": "qdrant-multivector-cosine-guard", "engine": "qdrant",
+        "connection_params": {"timeout": 60}, "collection_params": {"timeout": 60},
+        "search_params": [{"parallel": 1}], "upload_params": {"parallel": 1, "batch_size": 50}
+    }]);
+    let proj = common::write_multivector_project(
+        "multivector-cosine-guard",
+        &serde_json::to_string(&configs).unwrap(),
+    );
+
+    let run = |extra: &[&str]| -> std::process::Output {
+        let mut cmd = std::process::Command::new(binary_path());
+        cmd.args([
+            "--engines",
+            "qdrant-multivector-cosine-guard",
+            "--datasets",
+            &proj.dataset_name,
+            "--host",
+            "localhost",
+            "--skip-if-exists",
+            "false",
+        ])
+        .args(extra)
+        .env("QDRANT_GRPC_PORT", qdrant_grpc_port().to_string())
+        .env("QDRANT_REST_PORT", qdrant_rest_port().to_string())
+        .current_dir(&proj.root);
+        cmd.output().expect("run vector-db-benchmark")
+    };
+
+    // --keep-data: phase 2 needs the collection still present under
+    // --skip-upload — without it the normal end-of-run cleanup would delete
+    // it, and phase 2 would fail on "corpus is empty" instead of the guard.
+    let phase1 = run(&["--keep-data"]);
+    assert!(
+        phase1.status.success(),
+        "phase 1 (dot upload) failed:\n{}{}",
+        String::from_utf8_lossy(&phase1.stdout),
+        String::from_utf8_lossy(&phase1.stderr)
+    );
+
+    // Re-register the IDENTICAL corpus/collection under "cosine" — same
+    // dataset name and path, so data.mvec and the Qdrant collection are
+    // unchanged; only the declared distance flips.
+    let datasets_json = serde_json::json!([{
+        "name": proj.dataset_name, "type": "multivector", "path": proj.dataset_name,
+        "distance": "cosine", "vector_size": 16,
+    }]);
+    std::fs::write(
+        proj.root.join("datasets/datasets.json"),
+        serde_json::to_string_pretty(&datasets_json).unwrap(),
+    )
+    .unwrap();
+
+    let phase2 = run(&["--skip-upload", "--keep-data"]);
+    let combined = format!(
+        "{}{}",
+        String::from_utf8_lossy(&phase2.stdout),
+        String::from_utf8_lossy(&phase2.stderr)
+    );
+    assert!(
+        !phase2.status.success(),
+        "--skip-upload against a cosine-declared multivector dataset must fail.\n{combined}"
+    );
+    assert!(
+        combined.contains("per-token normalization"),
+        "failure must be the normalization guard, not something else.\n{combined}"
+    );
+
+    delete_collection();
+    std::fs::remove_dir_all(&proj.root).ok();
+}
+
 /// End-to-end HYBRID (dense + sparse) coverage WITH a negative control.
 ///
 /// The planted dataset's ground truth is recoverable ONLY by fusing both