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/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(); 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/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/"}") 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/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/CMakeLists.txt b/tests/CMakeLists.txt index 8c90ed33..879b224f 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -102,8 +102,12 @@ 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 + # 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") + ENVIRONMENT "OMP_PROC_BIND=spread;OMP_PLACES=threads;OMP_NUM_THREADS=4" + 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 # 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) { 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 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