From 0b88080b0ff55c28770ce247a4cc5bb1ae9780af Mon Sep 17 00:00:00 2001 From: gouyt13clear Date: Sat, 31 Oct 2026 20:09:09 +0800 Subject: [PATCH 1/6] fix(ivf): reject trailing bytes and report unopenable destinations IVF::load accepted any bytes after the ID section while the HNSW and SymphonyQG readers reject a payload that is not exactly the expected length. IVF::write_file set the stream exception mask before checking that the file opened, so an unwritable destination surfaced as the generic basic_ios::clear message. FlatInitializer::save/load now check the stream like FlatRaBitQInitializer; through IVF the stream already carried an exception mask, but the class is public. --- include/rabitqlib/index/ivf/initializer.hpp | 6 +++ src/index/ivf.cpp | 6 +++ .../unit/rabitqlib/index/initializer_test.cpp | 30 ++++++++++++++ tests/unit/rabitqlib/index/ivf_test.cpp | 39 +++++++++++++++++++ 4 files changed, 81 insertions(+) diff --git a/include/rabitqlib/index/ivf/initializer.hpp b/include/rabitqlib/index/ivf/initializer.hpp index d1ecdc7d..618d6731 100644 --- a/include/rabitqlib/index/ivf/initializer.hpp +++ b/include/rabitqlib/index/ivf/initializer.hpp @@ -119,6 +119,9 @@ class FlatInitializer : public Initializer { reinterpret_cast(centroids_.data()), static_cast(sizeof(float) * dim_ * num_cluster_) ); + if (!output) { + throw std::runtime_error("IVF Flat initializer cannot be written"); + } } void load(std::ifstream& input, const char* /*filename*/) override { @@ -126,6 +129,9 @@ class FlatInitializer : public Initializer { reinterpret_cast(centroids_.data()), static_cast(sizeof(float) * dim_ * num_cluster_) ); + if (!input) { + throw std::runtime_error("IVF Flat initializer cannot be read"); + } } }; diff --git a/src/index/ivf.cpp b/src/index/ivf.cpp index e906a006..3de7bb81 100644 --- a/src/index/ivf.cpp +++ b/src/index/ivf.cpp @@ -701,6 +701,9 @@ void IVF::save(const char* filename) const { void IVF::write_file(const char* filename) const { std::ofstream output(io_impl::filesystem_path(filename), std::ios::binary); + if (!output.is_open()) { + throw std::runtime_error("IVF cannot open index file for writing"); + } output.exceptions(std::ios::failbit | std::ios::badbit); const uint32_t flags = raw_reranking_ ? 1 : 0; const uint64_t stored_padded_dim = padded_dim_; @@ -950,6 +953,9 @@ void IVF::load(const char* filename) { } } } + if (remaining != 0) { + throw std::runtime_error("IVF index file has unexpected trailing data"); + } loaded.ready_ = true; input.close(); diff --git a/tests/unit/rabitqlib/index/initializer_test.cpp b/tests/unit/rabitqlib/index/initializer_test.cpp index 5abe7820..93e20555 100644 --- a/tests/unit/rabitqlib/index/initializer_test.cpp +++ b/tests/unit/rabitqlib/index/initializer_test.cpp @@ -390,5 +390,35 @@ TEST(ParallelForTest, JoinsWorkersBeforeRethrowingFunctionException) { EXPECT_TRUE(result.get()); } +TEST(IvfInitializerTest, FlatInitializerReportsStreamFailures) { + constexpr size_t kDim = 8; + constexpr size_t kCount = 3; + std::vector centroids(kCount * kDim, 1.5F); + FlatInitializer flat(kDim, kCount); + flat.add_vectors(centroids.data(), 1); + const rabitq_test::TempFile path_file("rabitq_flat_initializer.bin"); + const std::string& path = path_file.path(); + { + std::ofstream output(path, std::ios::binary); + ASSERT_NO_THROW(flat.save(output, path.c_str())); + } + { + // A stream that failed to open cannot be written. + std::ofstream unopened(path + ".missing/initializer.bin", std::ios::binary); + EXPECT_THROW(flat.save(unopened, path.c_str()), std::runtime_error); + } + { + std::ifstream input(path, std::ios::binary); + FlatInitializer restored(kDim, kCount); + ASSERT_NO_THROW(restored.load(input, path.c_str())); + } + std::filesystem::resize_file( + std::filesystem::u8path(path), sizeof(float) * kDim * kCount - 1 + ); + std::ifstream truncated(path, std::ios::binary); + FlatInitializer partial(kDim, kCount); + EXPECT_THROW(partial.load(truncated, path.c_str()), std::runtime_error); +} + } // namespace } // namespace rabitqlib::ivf diff --git a/tests/unit/rabitqlib/index/ivf_test.cpp b/tests/unit/rabitqlib/index/ivf_test.cpp index 621a9d66..0585e2ae 100644 --- a/tests/unit/rabitqlib/index/ivf_test.cpp +++ b/tests/unit/rabitqlib/index/ivf_test.cpp @@ -1548,6 +1548,45 @@ TEST(IvfInputTest, RejectsNonFiniteAndOversizedCoordinatesBeforeMutation) { ); } +TEST(IvfPersistenceTest, RejectsTrailingBytesAndReportsUnopenableDestination) { + constexpr size_t kCount = 33; + const auto points = random_rows(kCount, 911); + const auto centroids = random_rows(kDynNumClusters, 912); + std::vector assigned(kCount); + for (size_t i = 0; i < kCount; ++i) { + assigned[i] = static_cast(i % kDynNumClusters); + } + IVF index(kCount, kDynDim, kDynNumClusters, 4); + index.construct(points.data(), centroids.data(), assigned.data(), false, 1); + const rabitq_test::TempFile path_file("rabitq_ivf_trailing.index"); + const std::string& path = path_file.path(); + index.save(path.c_str()); + + IVF loaded; + ASSERT_NO_THROW(loaded.load(path.c_str())); + { + std::ofstream append(path, std::ios::binary | std::ios::app); + const std::array extra{}; + append.write(extra.data(), static_cast(extra.size())); + } + try { + loaded.load(path.c_str()); + ADD_FAILURE() << "trailing bytes were accepted"; + } catch (const std::runtime_error& error) { + EXPECT_STREQ(error.what(), "IVF index file has unexpected trailing data"); + } + // The failed load leaves the previously loaded index intact. + EXPECT_EQ(loaded.num_points(), kCount); + + const std::string missing_directory = path + ".missing/index.bin"; + try { + index.save(missing_directory.c_str()); + ADD_FAILURE() << "saving into a missing directory succeeded"; + } catch (const std::runtime_error& error) { + EXPECT_STREQ(error.what(), "IVF cannot open index file for writing"); + } +} + } // namespace } // namespace rabitqlib::ivf From 2184008ce05f103b69813ada84ab027abefb9f5c Mon Sep 17 00:00:00 2001 From: gouyt13clear Date: Sat, 31 Oct 2026 20:09:09 +0800 Subject: [PATCH 2/6] fix(symqg): reject ef_construction of zero in the C++ builder A zero search window inserted no candidates, so every row was completed by random fill without any error. The Python wrapper already rejected the value and the docs call it required positive. --- src/index/qg_builder.cpp | 3 +++ tests/unit/rabitqlib/index/qg_test.cpp | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/src/index/qg_builder.cpp b/src/index/qg_builder.cpp index dcf1bbc4..470a2f78 100644 --- a/src/index/qg_builder.cpp +++ b/src/index/qg_builder.cpp @@ -57,6 +57,9 @@ QGBuilder::QGBuilder( , dim_(qg_.dimension()) , max_degree_(qg_.degree_bound()) , seed_(seed) { + if (ef_construction == 0) { + throw std::invalid_argument("QuantizedGraph ef_construction must be positive"); + } if (cache_vectors) { cache_.emplace(); } diff --git a/tests/unit/rabitqlib/index/qg_test.cpp b/tests/unit/rabitqlib/index/qg_test.cpp index 32dae1a3..260bc1e3 100644 --- a/tests/unit/rabitqlib/index/qg_test.cpp +++ b/tests/unit/rabitqlib/index/qg_test.cpp @@ -2192,3 +2192,15 @@ TEST(QgInputTest, RejectsNonFiniteAndOversizedCoordinatesBeforeMutation) { ); } } // namespace rabitqlib::symqg + +namespace rabitqlib::symqg { +TEST(QgConstructionTest, RejectsZeroEfConstruction) { + constexpr size_t kCount = 33, kDim = 64; + const std::vector data(kCount * kDim, 0.25F); + for (auto init : {QGInitialization::Random, QGInitialization::PiPNN}) { + QuantizedGraph graph(kCount, kDim, 32); + EXPECT_THROW((QGBuilder(graph, 0, data.data(), 1, init)), std::invalid_argument); + EXPECT_EQ(graph.num_points(), 0U); + } +} +} // namespace rabitqlib::symqg From 83f0f8a92058da3e60dbe9d7598f64ebc67c1819 Mon Sep 17 00:00:00 2001 From: gouyt13clear Date: Sat, 31 Oct 2026 20:09:09 +0800 Subject: [PATCH 3/6] fix(clustering): stop reserving iteration stats for the maximum iteration count niter is a documented maximum with early stopping; reserving it up front allocated 32 bytes per possible iteration after centroids and assignments had already been resized. --- include/rabitqlib/clustering/detail/kmeans.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/include/rabitqlib/clustering/detail/kmeans.hpp b/include/rabitqlib/clustering/detail/kmeans.hpp index 26d4db11..b4603943 100644 --- a/include/rabitqlib/clustering/detail/kmeans.hpp +++ b/include/rabitqlib/clustering/detail/kmeans.hpp @@ -297,7 +297,6 @@ class LloydKMeans : public Parameters { assignments.resize(n); distances.resize(n); iteration_stats.clear(); - iteration_stats.reserve(niter); initialize_centroids(x, n); if (spherical) { normalize_centroids(); From d2069cb89479932611fa4057189553bb11c89fda Mon Sep 17 00:00:00 2001 From: gouyt13clear Date: Sat, 31 Oct 2026 20:09:09 +0800 Subject: [PATCH 4/6] build: propagate file-list failures and drop GNU-only options in the format scripts format-changed.sh read the helper's output through process substitution, so a failing helper formatted nothing and exited 0; it now captures the list and checks the helper's status like check-format.sh. The scripts also used xargs --no-run-if-empty and realpath --relative-to, which BSD userlands lack although macOS is a documented host. --- scripts/apply-format.sh | 2 +- scripts/check-format.sh | 2 +- scripts/clang-format-files.sh | 10 +++++++++- scripts/format-changed.sh | 9 ++++++++- 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/scripts/apply-format.sh b/scripts/apply-format.sh index 3397c813..b2dabc15 100755 --- a/scripts/apply-format.sh +++ b/scripts/apply-format.sh @@ -18,4 +18,4 @@ if [[ "$($formatter --version)" != *"version 23."* ]]; then fi "$repo_root/scripts/clang-format-files.sh" "$@" \ - | xargs -0 --no-run-if-empty "$formatter" -i + | xargs -0 -r "$formatter" -i diff --git a/scripts/check-format.sh b/scripts/check-format.sh index 0170fcc8..7ee5a89b 100755 --- a/scripts/check-format.sh +++ b/scripts/check-format.sh @@ -34,5 +34,5 @@ fi if ((${#files[@]} > 0)); then printf '%s\0' "${files[@]}" \ - | xargs -0 --no-run-if-empty "$formatter" --dry-run --Werror + | xargs -0 -r "$formatter" --dry-run --Werror fi diff --git a/scripts/clang-format-files.sh b/scripts/clang-format-files.sh index 278b959d..c6875529 100755 --- a/scripts/clang-format-files.sh +++ b/scripts/clang-format-files.sh @@ -13,7 +13,15 @@ else ':(exclude)include/rabitqlib/third/**' fi | while IFS= read -r -d '' file; do if [[ "$file" == /* ]]; then - file="$(realpath --relative-to="$repo_root" "$file")" + # Resolve the directory portably (no GNU realpath) and keep only + # repository paths; anything outside is skipped below. + directory="$(cd "$(dirname "$file")" 2>/dev/null && pwd -P)" || continue + file="$directory/$(basename "$file")" + if [[ "$file" == "$repo_root"/* ]]; then + file="${file#"$repo_root"/}" + else + continue + fi else file="${file#./}" fi diff --git a/scripts/format-changed.sh b/scripts/format-changed.sh index 9d283a30..b0e4f3e7 100755 --- a/scripts/format-changed.sh +++ b/scripts/format-changed.sh @@ -17,7 +17,14 @@ if [[ "$($formatter --version)" != *"version 23."* ]]; then exit 1 fi -mapfile -d '' absolute_files < <("$repo_root/scripts/clang-format-files.sh") +# Process substitution would discard the file-list helper's exit status. +file_list="$(mktemp)" +trap 'rm -f -- "$file_list"' EXIT +if ! "$repo_root/scripts/clang-format-files.sh" > "$file_list"; then + echo "error: could not list files for formatting" >&2 + exit 1 +fi +mapfile -d '' absolute_files < "$file_list" files=() for file in "${absolute_files[@]}"; do files+=("${file#"$repo_root/"}") From b272c43485ea39c14598bee3e4c5dfc23e72a75f Mon Sep 17 00:00:00 2001 From: gouyt13clear Date: Sat, 31 Oct 2026 20:09:09 +0800 Subject: [PATCH 5/6] test: fail filtered runs that select nothing and isolate temporary files per process GoogleTest exits 0 when a --gtest_filter matches no test, so a renamed suite silently removed the OpenMPBinding check and parts of the Intel SDE coverage; the CTest entry now requires its one expected test and the SDE step checks each filter pattern lists a test. The two consumer ctest runs without --no-tests=error gain it. Test temp files carry the process ID so concurrent suites in one temp directory cannot delete each other's files, and the no-SIMD HACC test asserts the generic backend ran. --- .github/workflows/test.yaml | 10 ++++++++ tests/CMakeLists.txt | 5 +++- tests/common/temp_file.hpp | 23 +++++++++++++++++-- tests/core_consumer/run.cmake | 5 ++-- tests/dispatch/fastscan_no_simd_test.cpp | 3 +++ .../index/historical_persistence_test.cpp | 6 +++-- 6 files changed, 45 insertions(+), 7 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index c18b00b1..e4b38611 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -374,6 +374,16 @@ jobs: simd_filter+=':HnswSaveTest.SuccessfulSavePreservesSearchAfterLoading' simd_filter+=':HnswPaddingTest.TailDimensionsMatchEveryAvailableSearchBackend' simd_filter+=':IvfLifecycleTest.BatchTailSurvivesAddRemoveAndReload:HnswRemoveTest.*' + # A renamed suite would silently drop out of the filter: GoogleTest exits 0 + # with zero matches, so require every pattern to select at least one test. + IFS=: read -ra patterns <<< "$simd_filter" + for pattern in "${patterns[@]}"; do + if ! build-install/tests/rabitq_tests --gtest_list_tests \ + --gtest_filter="$pattern" | grep -q '^ '; then + echo "error: no test matches the filter pattern $pattern" >&2 + exit 1 + fi + done for cpu_model in hsw icl; do RABITQ_TEST_CPU="$cpu_model" "$RUNNER_TEMP/sde/sde64" "-$cpu_model" -- \ build-install/tests/rabitq_tests --gtest_filter="$simd_filter" diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8c90ed33..fee4f15d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -102,8 +102,11 @@ if(RABITQ_BUILD_INDEX) # Binding is initialized before main; test it in its own process. add_test(NAME OpenMPBinding COMMAND rabitq_tests --gtest_filter=ThreadCount.BoundParallelForUsesRuntimePlacementAndPropagatesErrors) + # GoogleTest exits 0 when a filter matches nothing; require the one expected test. set_tests_properties(OpenMPBinding PROPERTIES - ENVIRONMENT "OMP_PROC_BIND=spread;OMP_PLACES=threads;OMP_NUM_THREADS=4") + ENVIRONMENT "OMP_PROC_BIND=spread;OMP_PLACES=threads;OMP_NUM_THREADS=4" + PASS_REGULAR_EXPRESSION "\\[ PASSED \\] 1 test\\." + FAIL_REGULAR_EXPRESSION "\\[ FAILED \\]") # Substitute CPU predicates only in this executable to exercise real cached dispatch # on a CPU without SIMD. The static library's detector object is not linked here. diff --git a/tests/common/temp_file.hpp b/tests/common/temp_file.hpp index 37cbd207..336f19ba 100644 --- a/tests/common/temp_file.hpp +++ b/tests/common/temp_file.hpp @@ -7,14 +7,33 @@ #include #include +#if defined(_WIN32) +#include +#else +#include +#endif + namespace rabitq_test { +// Prefix a temporary file name with the process ID so suites running at the same +// time in one temporary directory cannot delete or overwrite each other's files. +inline std::string process_unique(const std::string& name) { +#if defined(_WIN32) + const int pid = _getpid(); +#else + const int pid = static_cast(getpid()); +#endif + return "rabitq-" + std::to_string(pid) + "-" + name; +} + // A file under the GoogleTest temporary directory that is removed, together with an IVF // HNSW sidecar written next to it, when the test leaves its scope, including after a -// failed assertion. Leftovers from an interrupted earlier run are removed up front. +// failed assertion. Leftovers from an interrupted earlier run of this process are +// removed up front. class TempFile { public: - explicit TempFile(const std::string& name) : path_(::testing::TempDir() + name) { + explicit TempFile(const std::string& name) + : path_(::testing::TempDir() + process_unique(name)) { remove(); } ~TempFile() { remove(); } diff --git a/tests/core_consumer/run.cmake b/tests/core_consumer/run.cmake index d80bd713..33f3270a 100644 --- a/tests/core_consumer/run.cmake +++ b/tests/core_consumer/run.cmake @@ -99,7 +99,8 @@ foreach(component IN LISTS components) "-DRABITQ_SOURCE_DIR=${consumer_source}" "-DCMAKE_PREFIX_PATH=${work}/relocated") run("${CMAKE_COMMAND}" --build "${build}" --config Release --parallel 2) - run("${CMAKE_CTEST_COMMAND}" --test-dir "${build}" -C Release --output-on-failure) + run("${CMAKE_CTEST_COMMAND}" --test-dir "${build}" -C Release --output-on-failure + --no-tests=error) endforeach() foreach(build IN ITEMS producer ADD_SUBDIRECTORY FETCHCONTENT) if(EXISTS "${work}/${build}/compile_commands.json") @@ -175,5 +176,5 @@ run("${CMAKE_COMMAND}" -S "${RABITQ_SOURCE_DIR}/benchmarks/core" "-DCMAKE_PREFIX_PATH=${WORK_DIR}/core/relocated") run("${CMAKE_COMMAND}" --build "${WORK_DIR}/benchmark" --config Release --parallel 2) run("${CMAKE_CTEST_COMMAND}" --test-dir "${WORK_DIR}/benchmark" -C Release - --output-on-failure) + --output-on-failure --no-tests=error) message(STATUS "External consumers and core benchmark passed for: ${components}") diff --git a/tests/dispatch/fastscan_no_simd_test.cpp b/tests/dispatch/fastscan_no_simd_test.cpp index 39bdc1a2..71a75108 100644 --- a/tests/dispatch/fastscan_no_simd_test.cpp +++ b/tests/dispatch/fastscan_no_simd_test.cpp @@ -5,6 +5,7 @@ #include #include +#include "fastscan_backend.hpp" #include "rabitqlib/clustering/rabitqkmeans.hpp" #include "rabitqlib/fastscan/fastscan.hpp" #include "rabitqlib/fastscan/highacc_fastscan.hpp" @@ -81,6 +82,8 @@ TEST(FastScanDispatchTest, HighAccuracyUsesScalarFallbackWithoutSimd) { transfer_lut_hacc(lut.data(), kDim, high_accuracy_lut.data()); std::array result{}; accumulate_hacc(codes.data(), high_accuracy_lut.data(), result.data(), kDim); + // The AVX2 kernel produces the same sums for this layout; prove the fallback ran. + EXPECT_STREQ(detail::selected_backend().name, "generic"); for (const auto value : result) { EXPECT_EQ(value, 4000); } diff --git a/tests/unit/rabitqlib/index/historical_persistence_test.cpp b/tests/unit/rabitqlib/index/historical_persistence_test.cpp index f456acb1..e8d8156b 100644 --- a/tests/unit/rabitqlib/index/historical_persistence_test.cpp +++ b/tests/unit/rabitqlib/index/historical_persistence_test.cpp @@ -18,6 +18,7 @@ #include "rabitqlib/index/ivf/ivf.hpp" #include "rabitqlib/index/symqg/qg.hpp" #include "rabitqlib/index/types.hpp" +#include "temp_file.hpp" namespace rabitqlib { namespace { @@ -65,8 +66,9 @@ TEST_P(HistoricalPersistenceTest, LoadsSearchesResavesAndPreservesStateAfterFail expected >> reference_ids[i] >> reference_distances[i]; } ASSERT_TRUE(expected.good()); - const auto output = std::filesystem::temp_directory_path() / - ("rabitq_historical_" + GetParam().directory + "_" + name); + const auto output = + std::filesystem::temp_directory_path() / + rabitq_test::process_unique("historical_" + GetParam().directory + "_" + name); const auto compare = [&] { EXPECT_EQ(ids, reference_ids); for (size_t i = 0; i < ids.size(); ++i) { From 7d6c25463c88dcffdbd723396acea8bf83fad43a Mon Sep 17 00:00:00 2001 From: gouyt13clear Date: Sat, 31 Oct 2026 20:17:41 +0800 Subject: [PATCH 6/6] test: accept a skipped OpenMP placement test as a selected one MSVC's OpenMP runtime has no thread placement, so the test skips itself there; the CTest gate only needs to prove the filter selected it. --- tests/CMakeLists.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index fee4f15d..879b224f 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -102,10 +102,11 @@ if(RABITQ_BUILD_INDEX) # Binding is initialized before main; test it in its own process. add_test(NAME OpenMPBinding COMMAND rabitq_tests --gtest_filter=ThreadCount.BoundParallelForUsesRuntimePlacementAndPropagatesErrors) - # GoogleTest exits 0 when a filter matches nothing; require the one expected test. + # GoogleTest exits 0 when a filter matches nothing; require the one expected test + # to be selected. It skips itself on runtimes without thread placement (MSVC). set_tests_properties(OpenMPBinding PROPERTIES ENVIRONMENT "OMP_PROC_BIND=spread;OMP_PLACES=threads;OMP_NUM_THREADS=4" - PASS_REGULAR_EXPRESSION "\\[ PASSED \\] 1 test\\." + PASS_REGULAR_EXPRESSION "\\[ PASSED \\] 1 test\\.;\\[ SKIPPED \\] 1 test" FAIL_REGULAR_EXPRESSION "\\[ FAILED \\]") # Substitute CPU predicates only in this executable to exercise real cached dispatch