From 5d5e61c314b7d31af579190d849e9bb266c6ef5c Mon Sep 17 00:00:00 2001 From: Ilia Date: Mon, 14 Sep 2026 13:45:16 +0300 Subject: [PATCH] Prepare v0.10.0-rc.2: fix rc.1 review blockers Cherry-pick of the review fixes onto v0.10.0-rc.1. Package as PEP 440 0.10.0rc2. Do not publish as stable 0.10.0. Fail closed on unopened stream loads without clearing a live index, restore const searchKnnCloserFirst and addPoint(..., int level), stop CMake from wiping caller CMAKE_CXX_FLAGS, and make StatusOr/NoExceptions stream I/O avoid extra copies and throws. --- CMakeLists.txt | 35 +++---- README.md | 7 +- hnswlib/bruteforce.h | 34 ++++--- hnswlib/hnswalg.h | 118 ++++++++++++++++------- hnswlib/hnswlib.h | 136 ++++++++++++++++++++++----- setup.py | 3 +- tests/cpp/no_exceptions_api_test.cpp | 91 ++++++++++++++++++ 7 files changed, 338 insertions(+), 86 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5cfc8a6d6..d8987431a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -151,19 +151,21 @@ else() set(DISABLE_EXCEPTIONS_FLAGS -fno-exceptions) endif() -# Turn on assertions in the RelWithDebInfo build type. -foreach(NDEBUG_FLAG_STR IN ITEMS "/DNDEBUG" "/D NDEBUG" "-DNDEBUG") - string(REPLACE "${NDEBUG_FLAG_STR}" "" CMAKE_CXX_FLAGS_RELWITHDEBINFO - "${CMAKE_CXX_FLAGS_RELWITHDEBINFO}") -endforeach() -string(STRIP "${CMAKE_CXX_FLAGS_RELWITHDEBINFO}" CMAKE_CXX_FLAGS_RELWITHDEBINFO) -set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO}" - CACHE STRING "Flags for RelWithDebInfo configuration." FORCE) - -# Start with an empty value of CMAKE_CXX_FLAGS, not the value from the cache. -# It will not override any "default" flags -- those will come from -# per-build-type variables (CMAKE_CXX_FLAGS_${CMAKE_BUILD_TYPE}). -set(CMAKE_CXX_FLAGS "") +# RelWithDebInfo assertions apply only when this repo is the top-level +# project. Forcing the cache from an add_subdirectory() would mutate the +# parent build. +if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME) + foreach(NDEBUG_FLAG_STR IN ITEMS "/DNDEBUG" "/D NDEBUG" "-DNDEBUG") + string(REPLACE "${NDEBUG_FLAG_STR}" "" CMAKE_CXX_FLAGS_RELWITHDEBINFO + "${CMAKE_CXX_FLAGS_RELWITHDEBINFO}") + endforeach() + string(STRIP "${CMAKE_CXX_FLAGS_RELWITHDEBINFO}" CMAKE_CXX_FLAGS_RELWITHDEBINFO) + set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO}" + CACHE STRING "Flags for RelWithDebInfo configuration." FORCE) +endif() + +# Do not wipe or FORCE-write CMAKE_CXX_FLAGS. That erased caller-supplied +# flags even when HNSWLIB_EXAMPLES=OFF (add_subdirectory consumers). if(HNSWLIB_EXAMPLES) message("Building examples and tests") @@ -274,7 +276,6 @@ if(HNSWLIB_EXAMPLES) endforeach() endif() -# Persist CMAKE_CXX_FLAGS in the cache for debuggability. -string(STRIP "${CMAKE_CXX_FLAGS}" CMAKE_CXX_FLAGS) -set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}" - CACHE STRING "Flags used by the CXX compiler during all build types." FORCE) +# Do not persist CMAKE_CXX_FLAGS into the CMake cache. A FORCE write from +# this directory would overwrite the parent project's (or the user's +# -DCMAKE_CXX_FLAGS=) value, including when examples are disabled. diff --git a/README.md b/README.md index b5ea2f15f..f22dc0708 100644 --- a/README.md +++ b/README.md @@ -3,8 +3,13 @@ Header-only C++ HNSW implementation with python bindings, insertions and updates **NEWS:** -**version 0.10.0** +**version 0.10.0rc2** (release candidate — not a stable PyPI upload) +* Package version is `0.10.0rc2` (PEP 440); do not upload as final `0.10.0` yet. Supersedes v0.10.0-rc.1. +* Stream `loadIndexNoExceptions` fails closed on an unopened/failed input without clearing a live index +* `searchKnnCloserFirst` is `const` again; `addPoint(..., int level)` is restored so integer levels are not treated as `replace_deleted` +* CMake no longer wipes caller `CMAKE_CXX_FLAGS` (including `add_subdirectory` / examples-off) +* `StatusOr` stores T inline and moves the result; `*NoExceptions` stream writes return `Status` instead of throwing on write failure * Optional no-exceptions C++ API: `*NoExceptions` methods return `Status` / `StatusOr` so the headers can be compiled with `-fno-exceptions` (`-DHNSWLIB_ENABLE_EXCEPTIONS=OFF`). Throwing methods remain the default. (#619, #678) by [@michaelbautin](https://github.com/michaelbautin) * Stream `saveIndex` / `loadIndex` over `std::ostream` / `std::istream`, plus `getInternalIdByLabel` * CI covers exceptions on/off, Clang / GCC / MSVC, and ASAN / UBSAN diff --git a/hnswlib/bruteforce.h b/hnswlib/bruteforce.h index a0b41aec5..5a236a2cf 100644 --- a/hnswlib/bruteforce.h +++ b/hnswlib/bruteforce.h @@ -132,28 +132,40 @@ class BruteforceSearch : public AlgorithmInterface { Status saveIndexNoExceptions(std::ostream &output) { - writeBinaryPOD(output, maxelements_); - writeBinaryPOD(output, size_per_element_); - writeBinaryPOD(output, cur_element_count); - if (!output.good()) { - return Status("Failed writing index metadata"); - } + StreamExceptionsOff guard(output); + return invokeWithoutStreamThrow([&]() -> Status { + if (!output) { + return Status("Cannot save index: output stream is not open or in a failed state"); + } + writeBinaryPOD(output, maxelements_); + writeBinaryPOD(output, size_per_element_); + writeBinaryPOD(output, cur_element_count); + if (!output.good()) { + return Status("Failed writing index metadata"); + } - output.write(data_, maxelements_ * size_per_element_); - if (!output.good()) { - return Status("Failed writing vector data"); - } - return OkStatus(); + output.write(data_, maxelements_ * size_per_element_); + if (!output.good()) { + return Status("Failed writing vector data"); + } + return OkStatus(); + }); } Status saveIndexNoExceptions(const std::string &location) override { std::ofstream output(location, std::ios::binary); + if (!output.is_open()) { + return Status("Cannot save index: failed to open output file"); + } return saveIndexNoExceptions(output); } void loadIndex(std::istream &input, SpaceInterface *s) { + if (!input) { + HNSWLIB_THROW_RUNTIME_ERROR("Cannot load index: input stream is not open or not readable"); + } readBinaryPOD(input, maxelements_); readBinaryPOD(input, size_per_element_); readBinaryPOD(input, cur_element_count); diff --git a/hnswlib/hnswalg.h b/hnswlib/hnswalg.h index 641738f7f..6d8f73745 100644 --- a/hnswlib/hnswalg.h +++ b/hnswlib/hnswalg.h @@ -722,45 +722,54 @@ class HierarchicalNSW : public AlgorithmInterface { } Status saveIndexNoExceptions(std::ostream &output) { - writeBinaryPOD(output, offsetLevel0_); - writeBinaryPOD(output, max_elements_); - writeBinaryPOD(output, cur_element_count); - writeBinaryPOD(output, size_data_per_element_); - writeBinaryPOD(output, label_offset_); - writeBinaryPOD(output, offsetData_); - writeBinaryPOD(output, maxlevel_); - writeBinaryPOD(output, enterpoint_node_); - writeBinaryPOD(output, maxM_); - - writeBinaryPOD(output, maxM0_); - writeBinaryPOD(output, M_); - writeBinaryPOD(output, mult_); - writeBinaryPOD(output, ef_construction_); - - if (!output.good()) { - return Status("Failed writing index metadata"); - } - - output.write(data_level0_memory_, cur_element_count * size_data_per_element_); - if (!output.good()) { - return Status("Failed writing level 0 memory block"); - } + StreamExceptionsOff guard(output); + return invokeWithoutStreamThrow([&]() -> Status { + if (!output) { + return Status("Cannot save index: output stream is not open or in a failed state"); + } + writeBinaryPOD(output, offsetLevel0_); + writeBinaryPOD(output, max_elements_); + writeBinaryPOD(output, cur_element_count); + writeBinaryPOD(output, size_data_per_element_); + writeBinaryPOD(output, label_offset_); + writeBinaryPOD(output, offsetData_); + writeBinaryPOD(output, maxlevel_); + writeBinaryPOD(output, enterpoint_node_); + writeBinaryPOD(output, maxM_); + + writeBinaryPOD(output, maxM0_); + writeBinaryPOD(output, M_); + writeBinaryPOD(output, mult_); + writeBinaryPOD(output, ef_construction_); - for (size_t i = 0; i < cur_element_count; i++) { - unsigned int linkListSize = element_levels_[i] > 0 ? size_links_per_element_ * element_levels_[i] : 0; - writeBinaryPOD(output, linkListSize); - if (linkListSize) { - output.write(linkLists_[i], linkListSize); + if (!output.good()) { + return Status("Failed writing index metadata"); } + + output.write(data_level0_memory_, cur_element_count * size_data_per_element_); if (!output.good()) { - return Status("Failed writing link list elements"); + return Status("Failed writing level 0 memory block"); } - } - return OkStatus(); + + for (size_t i = 0; i < cur_element_count; i++) { + unsigned int linkListSize = element_levels_[i] > 0 ? size_links_per_element_ * element_levels_[i] : 0; + writeBinaryPOD(output, linkListSize); + if (linkListSize) { + output.write(linkLists_[i], linkListSize); + } + if (!output.good()) { + return Status("Failed writing link list elements"); + } + } + return OkStatus(); + }); } Status saveIndexNoExceptions(const std::string &location) override { std::ofstream output(location, std::ios::binary); + if (!output.is_open()) { + return Status("Cannot save index: failed to open output file"); + } return saveIndexNoExceptions(output); } @@ -772,11 +781,31 @@ class HierarchicalNSW : public AlgorithmInterface { } Status loadIndexNoExceptions(std::istream &input, SpaceInterface *s, size_t max_elements_i = 0) { - clear(); - // get file size: + // Must not destroy the live index until the stream is known to be readable. + // An unopened / failed stream used to seek to -1, skip the empty-index + // corruption loop, and return OkStatus() after clear(). + if (!input) { + return Status("Cannot load index: input stream is not open or not readable"); + } + + StreamExceptionsOff guard(input); + return invokeWithoutStreamThrow([&]() -> Status { + // Default-constructed ifstreams are often still good() on libc++. + // If we cannot peek a byte, there is no index to load — leave the + // live index untouched. + if (input.peek() == std::char_traits::eof()) { + return Status("Cannot load index: input stream is not open or not readable"); + } + + // get file size before mutating the in-memory index: input.seekg(0, input.end); std::streampos total_filesize = input.tellg(); input.seekg(0, input.beg); + if (!input || total_filesize < std::streampos(0)) { + return Status("Cannot load index: failed to determine stream size"); + } + + clear(); readBinaryPOD(input, offsetLevel0_); readBinaryPOD(input, max_elements_); @@ -797,6 +826,9 @@ class HierarchicalNSW : public AlgorithmInterface { readBinaryPOD(input, M_); readBinaryPOD(input, mult_); readBinaryPOD(input, ef_construction_); + if (!input) { + return Status("Cannot load index: failed to read index header"); + } data_size_ = s->get_data_size(); fstdistfunc_ = s->get_dist_func(); @@ -870,6 +902,7 @@ class HierarchicalNSW : public AlgorithmInterface { } return OkStatus(); + }); } @@ -898,7 +931,7 @@ class HierarchicalNSW : public AlgorithmInterface { if (!result.ok()) { HNSWLIB_THROW_RUNTIME_ERROR(result.status().message()); } - return std::move(result.value()); + return std::move(result).value(); } template @@ -1057,6 +1090,21 @@ class HierarchicalNSW : public AlgorithmInterface { return OkStatus(); } + // Keep the bool addPoint from AlgorithmInterface visible. Without this, + // the integer-level overload below would hide it and `addPoint(p, id, 1)` + // would bind to replace_deleted=true. + using AlgorithmInterface::addPoint; + + // Historic overload: insert at an explicit graph level. + // `addPoint(data, label, 1)` must not become replace_deleted=true. + tableint addPoint(const void *data_point, labeltype label, int level) { + auto result = addPointWithLevel(data_point, label, level); + if (!result.ok()) { + HNSWLIB_THROW_RUNTIME_ERROR(result.status().message()); + } + return std::move(result).value(); + } + Status updatePoint(const void *dataPoint, tableint internalId, float updateNeighborProbability) { // update the feature vector associated with existing point with new vector @@ -1434,7 +1482,7 @@ class HierarchicalNSW : public AlgorithmInterface { if (!result.ok()) { HNSWLIB_THROW_RUNTIME_ERROR(result.status().message()); } - return std::move(result.value()); + return std::move(result).value(); } StatusOr diff --git a/hnswlib/hnswlib.h b/hnswlib/hnswlib.h index 570e876bf..ff36772b8 100644 --- a/hnswlib/hnswlib.h +++ b/hnswlib/hnswlib.h @@ -125,6 +125,8 @@ static bool AVX512Capable() { #include #include #include +#include +#include #include #include #include @@ -171,40 +173,97 @@ class HNSWLIB_NODISCARD Status { inline Status OkStatus() { return Status(); } +// Result-or-error container. T is stored inline (no extra heap allocation +// for the wrapper itself) and is move-constructed from the success path so +// search / getDataByLabel do not copy the full result. T is not constructed +// on error paths. template class StatusOr { public: - // Default constructor - StatusOr() : status_(), value_() {} + StatusOr() : status_("StatusOr has no value"), has_value_(false) {} - // Constructor with a value - StatusOr(T value) : status_(), value_(value) {} + StatusOr(T value) : status_(), has_value_(false) { + new (storage()) T(std::move(value)); + has_value_ = true; + } - // Constructor with an error status - StatusOr(const char* error) : status_(error), value_() {} - StatusOr(Status status) : status_(status), value_() {} + StatusOr(const char* error) + : status_(error ? error : "StatusOr has no value"), has_value_(false) {} - // Returns true if the status is OK. - bool ok() const { return status_.ok(); } + StatusOr(Status status) + : status_(status.ok() ? Status("StatusOr has no value") : status), + has_value_(false) {} - // Returns the value if the status is OK, undefined behavior otherwise. - T&& value() { - return std::move(value_); + StatusOr(const StatusOr& other) + : status_(other.status_), has_value_(false) { + if (other.has_value_) { + new (storage()) T(*other.storage()); + has_value_ = true; + } } - const T& value() const { - return value_; + StatusOr(StatusOr&& other) + : status_(other.status_), has_value_(false) { + if (other.has_value_) { + new (storage()) T(std::move(*other.storage())); + has_value_ = true; + } } - T operator*() const { - return value(); + StatusOr& operator=(const StatusOr& other) { + if (this == &other) { + return *this; + } + destroy(); + status_ = other.status_; + if (other.has_value_) { + new (storage()) T(*other.storage()); + has_value_ = true; + } + return *this; } - Status status() const { return status_; } + StatusOr& operator=(StatusOr&& other) { + if (this == &other) { + return *this; + } + destroy(); + status_ = other.status_; + if (other.has_value_) { + new (storage()) T(std::move(*other.storage())); + has_value_ = true; + } + return *this; + } + + ~StatusOr() { destroy(); } + + bool ok() const { return status_.ok() && has_value_; } + + T& value() & { return *storage(); } + const T& value() const & { return *storage(); } + T&& value() && { return std::move(*storage()); } + + const T& operator*() const { return value(); } + + Status status() const { + return ok() ? OkStatus() : (status_.ok() ? Status("StatusOr has no value") : status_); + } private: + T* storage() { return reinterpret_cast(storage_); } + const T* storage() const { return reinterpret_cast(storage_); } + + void destroy() { + if (has_value_) { + storage()->~T(); + has_value_ = false; + } + } + Status status_; - T value_; + alignas(T) unsigned char storage_[sizeof(T)]; + bool has_value_; }; typedef size_t labeltype; @@ -252,6 +311,41 @@ static void readBinaryPOD(std::istream &in, T &podRef) { in.read((char *) &podRef, sizeof(T)); } +// Temporarily disable iostream exceptions so *NoExceptions I/O reports +// failures via Status instead of throwing std::ios_base::failure. +class StreamExceptionsOff { + public: + explicit StreamExceptionsOff(std::ios& stream) + : stream_(stream), old_(stream.exceptions()) { + stream_.exceptions(std::ios::goodbit); + } + + ~StreamExceptionsOff() { + // Restoring a mask that includes failbit/badbit throws if those bits + // are already set. Only restore when that would be safe. + if (!stream_.fail()) { + stream_.exceptions(old_); + } + } + + private: + std::ios& stream_; + std::ios::iostate old_; +}; + +template +Status invokeWithoutStreamThrow(Fn&& fn) { +#if defined(__EXCEPTIONS) || _HAS_EXCEPTIONS == 1 + try { + return fn(); + } catch (const std::ios_base::failure&) { + return Status("Stream I/O failed"); + } +#else + return fn(); +#endif +} + template using DISTFUNC = MTYPE(*)(const void *, const void *, const void *); @@ -299,7 +393,7 @@ class AlgorithmInterface { if (!result.ok()) { HNSWLIB_THROW_RUNTIME_ERROR(result.status().message()); } - return std::move(result.value()); + return std::move(result).value(); } virtual StatusOr searchKnnNoExceptions( @@ -311,13 +405,13 @@ class AlgorithmInterface { virtual DistanceLabelVector searchKnnCloserFirst( const void* query_data, size_t k, - BaseFilterFunctor* isIdAllowed = nullptr) { + BaseFilterFunctor* isIdAllowed = nullptr) const { auto result = searchKnnCloserFirstNoExceptions(query_data, k, isIdAllowed); if (!result.ok()) { HNSWLIB_THROW_RUNTIME_ERROR(result.status().message()); } - return std::move(result.value()); + return std::move(result).value(); } virtual StatusOr searchKnnCloserFirstNoExceptions( diff --git a/setup.py b/setup.py index e7915c118..546de52ef 100644 --- a/setup.py +++ b/setup.py @@ -8,7 +8,8 @@ from setuptools import Extension, setup from setuptools.command.build_ext import build_ext -__version__ = '0.10.0' +# PEP 440 RC. Do not drop the suffix until the stable PyPI release. +__version__ = '0.10.0rc2' include_dirs = [ diff --git a/tests/cpp/no_exceptions_api_test.cpp b/tests/cpp/no_exceptions_api_test.cpp index 83826d84c..dec3522a1 100644 --- a/tests/cpp/no_exceptions_api_test.cpp +++ b/tests/cpp/no_exceptions_api_test.cpp @@ -1,5 +1,9 @@ #include #include +#include +#include +#include +#include #include #include @@ -44,10 +48,97 @@ void testBruteforceSaveIndexNoExceptionsDoesNotThrow() { std::remove(path); } +void testLoadIndexNoExceptionsDoesNotClearOnUnopenedStream() { + const int dim = 4; + std::vector a(dim, 1.0f); + + hnswlib::L2Space space(dim); + hnswlib::HierarchicalNSW index(&space, 8); + assert(index.addPointNoExceptions(a.data(), 1).ok()); + assert(index.getCurrentElementCount() == 1); + + std::ifstream missing("hnswlib_rc_review_missing_index.bin", std::ios::binary); + assert(!missing.is_open()); + hnswlib::Status status = index.loadIndexNoExceptions(missing, &space); + assert(!status.ok()); + assert(index.getCurrentElementCount() == 1); + + // Default-constructed streams are often still good() on libc++; the loader + // must not treat "never opened" as an empty successful index. + std::ifstream never_opened; + hnswlib::Status status_unopened = index.loadIndexNoExceptions(never_opened, &space); + assert(!status_unopened.ok()); + assert(index.getCurrentElementCount() == 1); + + std::vector restored = index.getDataByLabel(1); + assert(restored.size() == static_cast(dim)); + assert(restored[0] == 1.0f); +} + +class FailingBuf : public std::streambuf { + protected: + int overflow(int) override { return traits_type::eof(); } + std::streamsize xsputn(const char*, std::streamsize) override { return 0; } +}; + +void testSaveIndexNoExceptionsDoesNotThrowOnWriteFailure() { + const int dim = 4; + std::vector a(dim, 1.0f); + + hnswlib::L2Space space(dim); + hnswlib::HierarchicalNSW index(&space, 8); + assert(index.addPointNoExceptions(a.data(), 1).ok()); + + FailingBuf buf; + std::ostream out(&buf); +#if defined(__EXCEPTIONS) || _HAS_EXCEPTIONS == 1 + // The review case: write failure with iostream exceptions enabled must + // still return Status, not throw. Skipped when the TU is built with + // -fno-exceptions because enabling the mask would abort instead. + out.exceptions(std::ios::failbit | std::ios::badbit); +#endif + + hnswlib::Status status = index.saveIndexNoExceptions(out); + assert(!status.ok()); +} + +void testAddPointIntegerLevelIsNotReplaceDeleted() { + const int dim = 4; + std::vector a(dim, 1.0f); + std::vector b(dim, 2.0f); + + hnswlib::L2Space space(dim); + hnswlib::HierarchicalNSW index( + &space, /*max_elements=*/8, /*M=*/16, /*ef_construction=*/16, + /*random_seed=*/100, /*allow_replace_deleted=*/false); + index.addPoint(a.data(), 1); + // Integer 3 is a graph level. Binding it to bool replace_deleted would + // throw because replacement is disabled. + index.addPoint(b.data(), 2, 3); + assert(index.getCurrentElementCount() == 2); +} + +void testSearchKnnCloserFirstIsConst() { + const int dim = 4; + std::vector a(dim, 1.0f); + + hnswlib::L2Space space(dim); + hnswlib::HierarchicalNSW index(&space, 8); + index.addPoint(a.data(), 1); + const hnswlib::HierarchicalNSW& cref = index; + auto res = cref.searchKnnCloserFirst(a.data(), 1); + assert(res.size() == 1); + assert(res[0].second == 1); +} + } // namespace int main() { testAddPointReportsCapacityErrorWhenReplaceHasNoVacancy(); testBruteforceSaveIndexNoExceptionsDoesNotThrow(); + testLoadIndexNoExceptionsDoesNotClearOnUnopenedStream(); + testSaveIndexNoExceptionsDoesNotThrowOnWriteFailure(); + testAddPointIntegerLevelIsNotReplaceDeleted(); + testSearchKnnCloserFirstIsConst(); return 0; }