fix(hnsw): six data-integrity fixes for the HNSW vector index (5.1 GA) - #1234
Merged
Conversation
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
|
Reviewed; no blockers found. The integration-test helper fix (commit a864069) fully addresses the prior blocker: |
1. Non-finite vector guard: reject NaN/Infinity before touching the graph. NaN poisons bisectInsert (arr[mid].distance <= NaN always false), pinning the candidate to rank 1 of every future search. Also maps non-finite exact distances to Infinity in search.ts int8 rerank path. 2. Entry-point replacement scan: pass transaction to getRange, skip deleted node (previously re-elected, leaving dangling entry point and empty searches), verify candidate before electing. Also handles orphan-is-EP in needsReindexing path. 3. Update-cleanup sweep levels: on UPDATE, remove reverse edge only at level l where old connection existed; keep 0..l sweep only for DELETE. Prevents destruction of reverse edges re-added by addConnection on every re-embed. 4. Backfill resume idempotency: when nodeId exists but no existingVector supplied, load stored node and treat as update. Prevents dangling reverse edges and wrong levels on crash-recovery re-feed by runIndexing. 5. Structural-change rebuild checkpoint (databases.ts): reset lastIndexedKey when index options structurally change so runIndexing clears and rebuilds. Pure crash-recovery (same options) still preserves the checkpoint. 6a. Delete path removes safeKey->nodeId mapping to keep key count accurate. 6b. le comparator uses <=, not <, at the distance threshold. Tests: 10 new unit tests in unitTests/resources/vectorIndex.test.js using the mock-store harness. Baseline 659/21; after 669/21 (+10 passing, zero new failures). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
heskew
force-pushed
the
fix/hnsw-integrity-5.1
branch
from
June 10, 2026 21:03
3b7e071 to
251e5b7
Compare
…, thresholds, reindex Four tests through the full stack (real RocksDB, real Table.search(), schema-driven HNSW deployment via component API) guard each of the six data-integrity fixes in commit 251e5b7: 1. delete-entry-point: 50 records, bulk-delete 40 including EP, all survivors reachable. Guards fix #2 (EP replacement scan + transaction + skip-deleted). 2. update-churn: 30 records × 5 re-embed rounds, all records still findable by their final vector. Guards fix #3 (UPDATE sweeps only level l). 3. threshold queries: 2-D vectors at known exact cosine distances verify that le(boundary) is inclusive and lt(boundary) is exclusive. Guards fix #6b (le comparator uses <= not <). 4. reindex backfill: populate table without HNSW, add index, poll until search works, assert all 40 pre-existing records reachable; post-backfill update and delete must behave correctly. Guards fixes #4 and #5. Interrupted-backfill-then-restart is explicitly deferred: 40-record backfill completes in milliseconds so a SIGKILL race would be non-deterministic. That scenario is covered by the unit tests in unitTests/resources/vectorIndex.test.js. Vector search is exercised via the HTTP QUERY method so the body reaches Table.search() without the mapCondition stripping done by search_by_conditions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…side distances for boundaries supertest has no API for non-standard HTTP verbs, so the QUERY helper never executed; replaced with fetch. Threshold boundaries now come from the server's own $distance values: vectors are stored as float32, so float64-predicted boundaries differ at ~1e-8 and exact-boundary assertions can't hold. le(0) now exercises a true zero threshold through the real query path. All 4 integration tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kriszyp
approved these changes
Jun 10, 2026
heskew
marked this pull request as ready for review
June 10, 2026 23:25
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
11 tasks
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
2 participants
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.
Summary
Six verified data-integrity bugs in the HNSW vector index, found by a multipass review of the 5.1 models/vector subsystem. All six corrupt or degrade persisted graph state, which is why they're targeted at 5.1 before the non-beta release — the damage survives a later patch.
index()now rejects vectors with NaN/Infinity components (theembedHook.tscomment claimed this guard existed; it didn't — a NaN vector becomes rank 1 of every search that reaches it becausebisectInsertwith NaN always lands at position 0). The int8 rerank insearch.tsmaps non-finite exact distances toInfinityso they sort last.@embedre-embed path), reverse edges are now removed only at the level where the old connection existed, instead of sweeping0..land destroying edges just re-added/preserved. Deletes keep the full sweep. Stops gradual reachability erosion under normal write traffic.existingVector(whatrunIndexingdoes after a crash/restart) now loads the stored node and treats the call as an update (int8 vectors dequantized, mirroring the existing orphan-reindex path), instead of assigning a fresh random level and orphaning old reverse edges.databases.ts) — adistance/M/quantizationchange now clearslastIndexedKeyso the rebuild starts clean; previously an interrupted backfill + option change produced one graph built under two configurations. Search-only changes (efConstructionSearch) and pure crash-resume still preserve the checkpoint.safeKey→nodeIdmapping (stops unbounded growth and inflatedautoScaleEfcounts);lethreshold queries are inclusive (and a threshold of0now filters instead of being skipped as falsy — relevant for exact-match anddotProductqueries).11 new unit tests in
unitTests/resources/vectorIndex.test.js(mock-store harness).Where to put attention
integrationTests/server/vector-index-integrity.test.ts(4 tests, passing) exercises the transaction-sensitive fixes through the full stack — bulk-delete including the entry point, 5 rounds of re-embed churn,le/ltthresholds incl. a truele(0)(boundaries taken from server-side$distance, since vectors store as float32), and HNSW backfill over a populated table via schema change.databases.tscheckpoint-reset condition, thesearch.tsnon-finite rerank mapping, and the interrupted-backfill-then-restart scenario (the backfill completes in milliseconds at test scale, so a kill race is non-deterministic) have no dedicated tests. Flagged by the cross-model review; calling it out rather than papering over it.existingVectoris the dequantized approximate float — same approximation the existing delete/orphan-reindex path already uses.Cross-model review: Gemini findings adjudicated and addressed (the
le 0falsy-skip was real and is fixed+tested; two claimed blockers — nodeId-0 falsiness and Buffer/Int8Array dequantize — were verified not real: node ids start at 1 by construction and the dequantize cast is the established pattern on main). Codex leg was unavailable in this environment (repeated session interruptions); noting for transparency.Part of the 5.1 GA readiness work: relates to #1235 (PR 1 of 2). Generated by an LLM (Claude, Fable 5).
🤖 Generated with Claude Code