Repository navigation
fix(hnsw): clamp ef to the stored point count and validate cluster lookups - #198
Merged
Merged
Conversation
…okups search allocated a candidate pool of max(ef, k) entries, so HnswIndex.search(ef=2**40) failed with MemoryError instead of a validation error although no pool can hold more distinct points than are stored; ef is now clamped to the stored point count, which does not change results. cluster_id_of(PID) is public API but read the record array unchecked; it now throws std::logic_error on an unbuilt index and std::invalid_argument for an internal ID at or beyond the stored count, while the search and construction paths keep an unchecked private accessor.
Loaders validate structure to report corrupt or mismatched files, not to defend against crafted ones: a file's stored capacity is allocated as written and later add/remove calls rely on invariants the writer guaranteed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves the remaining decided items from the audit (#195–#197).
fix(hnsw): clampefto the stored point count and validate cluster lookupsHierarchicalNSW::search_knn_sorted_withallocated a candidate pool ofmax(ef, k)entries, soHnswIndex.search(ef=2**40)failed withMemoryErrorrather than being handled; a pool can never hold more distinct points than are stored, soefis now clamped tonum_stored_points(). Results are unchanged (the C++ test compares a2**40window with an exhaustive one; the Python test likewise). Documented in the C++ header, theefdocstring (.pyikept in sync) andhnsw.md.cluster_id_of(PID)is public API but read the record array without checks. It now throwsstd::logic_erroron an unbuilt index andstd::invalid_argumentfor an internal ID>= num_stored_points(); the comment states that internal IDs coincide with labels only for points added in order. The search estimators and construction keep a private uncheckedstored_cluster_id, so the hot path is unchanged: single-thread SIFT1M search on the saved benchmark index, interleaved with the mergedmainbinary, 2 passes × 2 rounds: ef=128 5575/5479 · 5850/5789 (main) vs 5771/5373 · 5433/5724 (this PR); ef=256 3413/3414 · 3492/3507 vs 3469/3432 · 3476/3507; recall@10 identical. (Absolute numbers are lower than in fix: validate advanced encoder preconditions and bound HNSW prefetch look-ahead #197 because sanitizer and Python suites were running concurrently.)docs: index files are trusted inputRecords the maintainer decision from the audit in
usage.mdand the "Change persistence" recipe: loaders validate structure so corrupt or mismatched files are reported, but a crafted file is not a supported input (it can makeloadallocate its stored capacity, or violate invariants that lateradd/removerely on).Verification
check-format.sh, ruff on the Python test and stub,git diff --check,mkdocs build --strict.ctest --no-tests=error441/441; Clang 23 ASan/UBSan: 441/441, no diagnostics.ivf*.cppandhnsw_bindings.cpp.pytest tests/python tests/test_mypy.py tests/test_pyright.py841 passed, 2 skipped (includes the stub/runtime docstring comparison).