Skip to content

fix(hnsw): clamp ef to the stored point count and validate cluster lookups - #198

Merged
gouyt13 merged 2 commits into
mainfrom
fix/hnsw-ef-clamp-cluster-lookup
Oct 31, 2026
Merged

gouyt13 merged 2 commits into
mainfrom
fix/hnsw-ef-clamp-cluster-lookup

Conversation

@gouyt13

@gouyt13 gouyt13 commented Oct 31, 2026

Copy link
Copy Markdown
Member

Resolves the remaining decided items from the audit (#195–#197).

fix(hnsw): clamp ef to the stored point count and validate cluster lookups

  • HierarchicalNSW::search_knn_sorted_with allocated a candidate pool of max(ef, k) entries, so HnswIndex.search(ef=2**40) failed with MemoryError rather than being handled; a pool can never hold more distinct points than are stored, so ef is now clamped to num_stored_points(). Results are unchanged (the C++ test compares a 2**40 window with an exhaustive one; the Python test likewise). Documented in the C++ header, the ef docstring (.pyi kept in sync) and hnsw.md.
  • cluster_id_of(PID) is public API but read the record array without checks. It now throws std::logic_error on an unbuilt index and std::invalid_argument for 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 unchecked stored_cluster_id, so the hot path is unchanged: single-thread SIFT1M search on the saved benchmark index, interleaved with the merged main binary, 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 input

Records the maintainer decision from the audit in usage.md and 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 make load allocate its stored capacity, or violate invariants that later add/remove rely on).

Verification

  • check-format.sh, ruff on the Python test and stub, git diff --check, mkdocs build --strict.
  • GCC 14 portable build: ctest --no-tests=error 441/441; Clang 23 ASan/UBSan: 441/441, no diagnostics.
  • Installed-consumer build and tests against a fresh install prefix; focused clang-tidy on the HNSW TUs, ivf*.cpp and hnsw_bindings.cpp.
  • Reinstalled package: pytest tests/python tests/test_mypy.py tests/test_pyright.py 841 passed, 2 skipped (includes the stub/runtime docstring comparison).

…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.
@github-actions github-actions Bot added the release:fixed Release category managed from the PR title label Oct 31, 2026
@gouyt13
gouyt13 merged commit bac860d into main Oct 31, 2026
40 checks passed
@gouyt13
gouyt13 deleted the fix/hnsw-ef-clamp-cluster-lookup branch October 31, 2026 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:fixed Release category managed from the PR title

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant