[cpegeric]

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

Fixes #30660

What this PR does / why we need it:

After an HNSW index is built, a DELETE followed by an INSERT can make other live rows unreachable through the index. In the issue's repro (1000 rows on a line, delete id 100, insert a far row) the index returns 901 rows, with ids 1–99 missing. This persists after the new model is published and after a restart; only FORCE_SYNC (a full rebuild) restores it.

Root cause (usearch, not ISCP). The CDC path is correct: HnswSync.sequentialUpdate applies Remove(100) then Add(2001). The same two calls on bare usearch, with no SQL or ISCP involved, give the same 901 result. In usearch, remove() only tombstones the node and queues its slot. The next add() reuses that slot, and index_gt::update() rewrites the node in place at the new vector's position, clearing its neighbour lists without repairing anything (upstream leaves a TODO there). Whatever was reachable only through the removed node is cut off. Points on a line make the removed node the only bridge, so the loss is a whole range; on realistic data the same mechanism causes partial recall loss. Full trace: #30660 (comment)

Fix. The vendored usearch is upgraded from usearch-2.25.3.tar.gz to usearch-2.26.5.tar.gz, built with git archive (same layout as before) from cpegeric/usearch matrixone_20261022. That branch is upstream v2.26.5 plus the existing concurrent_add (#735) fix plus fix_delete_20261022, which cherry-picks cleanly onto upstream for its own upstream PR. Upstream v2.26.5 still has this bug: the new line-reachability test fails on unmodified v2.26.5.

Upgrading to 2.26.5 also brings upstream #769, which rehashes tombstones in the key-lookup table. In v2.25.3, remove() could spin at 100% CPU under heavy delete/insert churn.

Upstream #777 changes Reserve(n): it now allocates exactly n slots, where it used to round up to the key-lookup table's power-of-two size (about +37%, e.g. Reserve(1000) gave 1365). MO always reserves its real capacity: MaxCapacity on build, which the build path never exceeds, and IndexCapacity on a sync load, which defaults to 1,000,000. Updates reuse the slot their own Remove freed. So no production path depended on that extra room. Only TestModel, which loaded with IndexCapacity 0, relied on it, and it now sets a real capacity.

• Repair: before a removed node's slot is reused, update() relinks the node's old neighbours on every level. Each neighbour drops its link to the reused slot and re-picks its neighbours, using the existing heuristic, from its own list plus the removed node's other neighbours. Nodes are locked one at a time, as in form_reverse_links_. The snapshot uses a reused per-thread buffer, and out-of-memory is reported as an error.
• Concurrency: index_dense_gt::add_() serializes adds that reuse a slot, because the in-place rewrite is not safe against concurrent rewrites of neighbouring nodes. Fresh adds and searches stay concurrent. MO's CDC update path is already sequential.

No memory growth: removed slots are still reused. Apart from the usearch upgrade, the only MO change is the TestModel capacity in its test setup.

Tests

Validated on 2.26.5:

• Unit tests: all 43 pkg/vectorindex/... packages pass.
• BVT: vector_hnsw_delete_insert passes 44/44. pessimistic_transaction/vector has 0 failures, vector passes 3301/3301, and branch_hnsw_async_refresh, fulltext2_hnsw_clone, index_cache_size_bounds, clone and branch_multi_index_restore_init have 0 failures.

The results below were first measured on the 2.25.3-based tarball:

• Go: new TestRemoveAddKeepsGraphReachable (pkg/vectorindex/hnsw) goes through HnswModel: remove 100, remove 500, and save→load then remove, each followed by a far add, asserting every live key is returned. All three fail on the previous tarball (901 / 501 rows) and pass with this one. All pkg/vectorindex/hnsw/... and pkg/vectorindex/usearchex packages pass.
• usearch C++ (cpp/test.cpp in the fork): the remove + far add on a line test for slot32_t and uint40_t fails without the fix. Remove/add churn rounds, single- and multi-threaded, assert every live key finds itself in an exhaustive search. On the merged branch, these and the Data lost after restart server #735 concurrent-add reachability test pass 10/10 runs with internal assertions enabled.
• BVT: new pessimistic_transaction/vector/vector_hnsw_delete_insert. It mirrors the issue (single-threaded build + FORCE_SYNC, DELETE id 100 + far INSERT, then DELETE id 500 + another far INSERT) and waits for the published model's checksum to change before querying. With this tarball: 44/44, and the index returns all 1000 rows, every id below the deleted one, and the true nearest row. On the previous tarball the same case returns 901 / 502 rows, none of the ids below the deleted one, and id 101 as the nearest row to [1,1,2]. The pessimistic_transaction/vector (703 pass, 0 failed) and vector suites stay green. The one suite-only failure, in IVF vector_filtered_search_knobs, passes 51/51 when run alone and doesn't involve HNSW.
• Live SQL, using the issue's exact repro after waiting for the async model to be published: the index returns 1000/1000 for query vectors [1,1,2], [777.2,1,2] and [1000,1,2], and 1000/1000 in the delete-id-500 variant (was 501).

🤖 Generated with Claude Code

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[XuPeng-SH]

mo-bot review · mo-bot-local · done

Commit: 489e38cd3f8da31af3d745bc35f3464b706b3b90

Review completed. Read the assessment.

[XuPeng-SH]

mo-bot review · mo-bot-local · review

Commit: 489e38cd3f8da31af3d745bc35f3464b706b3b90

Assessment: request_changes

Reviewed all seven changed repository files and 48 changed files inside the vendored archive. Repairing reachability in the native graph owner is a coherent design, and focused probes confirm the reported fix. However, the upgrade introduces an allocation-failure path during reserved-capacity churn that aborts the process.

Model: gpt-6-astra / xhigh

Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1

• P2 thirdparties/Makefile:32: Repeated DELETE/new-key INSERT operations trigger the new tombstone rehash in usearch-2.26.5; allocation of the replacement hash table fails.
  Impact: std::allocator throws through noexcept try_reserve, causing SIGABRT instead of returning an index-update error. Because HnswModel.Add invokes this native path, allocator rejection can terminate the CN. Handle rehash failure before committing the insertion and propagate it through Add; merely returning false is insufficient because the insertion callback currently ignores try_emplace's result.
  Evidence: Inside the selected archive, include/usearch/index_plugins.hpp:4303 introduces the churn-triggered rehash; allocation at line 3992 occurs inside noexcept try_reserve. A public-C-API probe reserves 128 entries, fills them, then performs remove/new-key-add rounds while rejecting the 4160-byte hash allocation. Candidate receipt checks/1792673191665326807-3.json records SIGABRT (-6); base control checks/1792673205892858231-3.json completes 100 rounds without that allocation. oom-trace.log and receipt checks/1792674039073797676-3.json confirm usearch_add → index_dense::add_ → flat_hash_multi_set::try_reserve. This finding concerns the newly introduced churn allocation, not preexisting initial-reserve behavior.

Validation:

• Artifacts are retained under /home/xupeng/.local/state/mo-bot/jobs/56d8cf9310231b17c5b9a596/attempt-1. review-audit.json correlates 23 terminal receipts with commands, source fingerprints, exit status and logs. Final audit receipt 1792674238133858201-4.json passed; source remains unchanged.

• Isolation preflight passed. Candidate and base native libraries were built through their exact repository Makefiles; archive and build-input hashes were independently reconciled in native-inputs.json.

• The original reachability probe returned 901 keys on base and all 1000 on candidate. Candidate also passed 40 deterministic graphs × 100 reuse operations and a four-writer mixed fresh/reused-slot probe.

• Cross-version persistence passed: base save, candidate load/repair/save, and reload by both versions retained all expected keys.

• Five paired native microbenchmarks measured median candidate/base elapsed ratios of 1.20× for build, 1.14× for search and 2.76× for reuse updates. Repair adds work and serialization to reuse; these measurements do not establish SQL workload latency.

• Frozen evidence reports 20 successful CI checks, including UT, native builds and pessimistic multi-CN BVT. local_validation is empty; author test-count claims were not treated as independently reproduced results.

Limitations:

• Local behavioral checks exercised native APIs, not a running SQL server. No local Go suite, SQL BVT or sanitizer run was performed.

• Allocation failure was injected; no SQL-level OOM or CN crash was induced. Functional concurrency probes do not establish race freedom.

• Raw CI/author execution logs and the standalone linked issue body were not supplied; issue behavior was assessed from the frozen PR description.

[aptend]

Deep-reviewed exact head 489e38cd3f8da31af3d745bc35f3464b706b3b90 against merge-base 6cbe375b49696021d72778baa292a66287f080bd, including all seven repository files, the 48 changed files inside the archive, historical review/comments, and the native Go/CDC/save/load consumers. One independently reproduced P2 blocker remains (inline).

The reachability fix works: the same base-generated 1,000-point model returns 901 keys after remove/add on base and all 1,000 on this head. Candidate repair/save and reload by both versions preserve the exact live key set. Local Go regression passed all three cases. Selected native regressions passed for 32/40-bit slots, churn, one/four-writer reuse, compact, load/view, truncated input, and RNG reserve continuity.

Reused verified exact-head CI 39761813910: HNSW owning package -race checkpoint exited 0; non-race coverage producer exited 0 with no failed events; SCA passed. The new SQL case actually ran in the PROXY compose job: 44/44, no failures/ignored/abnormal cases. Native builds used each revision's repository Makefile in separate detached worktrees.

Unhappy-path check	Result
Q1 ownership	Model/context/native-buffer destruction traced; normal failures have owners.
Q2 termination	No new lock-order cycle found; allocation failure instead reaches process termination (blocker).
Q3 boundedness	Repair buffers are degree/context-bounded; rehash retains old/new tables only during replacement.

Limits: the crash proof is deterministic allocator rejection through the public C API, not a real SQL OOM or live CN crash. Native concurrency checks are functional, not TSAN proof. An initial whole-vendored-test harness hit an unrelated StringZilla API mismatch; only the successfully compiled selected-test harness is counted. Evidence retained locally under /tmp/review30823-m5l6DB.

[cpegeric]

@XuPeng-SH: the P2 from your review (rehash allocation failure aborting the CN) is the same finding as @aptend's inline comment. Details in the reply there: #30823 (comment)

Fixed in 54d80ef534. usearch-2.26.5.tar.gz is now built from cpegeric/usearch matrixone_20261023 (732fc64), which adds fix_rehash_oom_20261023 on top of the #735 and #30660 fixes. Upstream PR: unum-cloud/USearch#822.

• try_reserve catches the std::bad_alloc and returns false. The old table, its mapping and the free slot are unchanged.
• try_emplace, when the churn rehash can't allocate, still inserts into a free or deleted slot. It fails only when every slot holds a live key, which can't happen here because the key table is reserved for the index's member capacity. Tombstones are reclaimed by the next rehash that can allocate.
• index_dense_gt::add_ no longer ignores try_emplace's result. A failed mapping returns failed("Failed to map the key to its slot"), which usearch_add and HnswModel.Add surface as an index-update error.

The two cases are covered separately, as you asked:

• Allocation rejection: the new test_slot_lookup_rehash_failure runs 1,024 erase/emplace rounds on a 128-entry table with an allocator that throws. It checks that the reserve fails, every insert succeeds, all live keys are found and erased ones aren't, and a later rehash that can allocate succeeds. Without the fix it aborts (exit 134).
• Successful churn: covered by the existing test_slot_lookup_churn.

Results:

• the full usearch test_cpp suite passes on the fix branch and on matrixone_20261023;
• all 42 pkg/vectorindex/... packages pass;
• the pessimistic_transaction/vector and vector BVT suites are at 100%, including vector_hnsw_delete_insert at 44/44;
• upstream main is merged (fd5a2507ab).

[XuPeng-SH]

mo-bot review · mo-bot-local · superseded

Commit: 54d80ef534ab54ed4d4a69639f1f9402261bd832

"interrupted"

[XuPeng-SH]

Re-reviewed the updated exact head 54d80ef534ab54ed4d4a69639f1f9402261bd832 against the prior allocation-failure finding. The follow-up change closes that path: try_reserve catches allocator exceptions and preserves the old table; try_emplace can still consume the freed/tombstoned slot when rehash allocation fails; index_dense_gt::add_ now propagates a failed key mapping instead of silently succeeding. The new upstream test exercises repeated erase/emplace under throwing allocation and checks live/deleted lookups, with later successful rehash; the separate churn test remains. Exact-head GitHub checks report UT, coverage, shared build, native build, Dockerfile, and pessimistic/PROXY BVT successful. I did not rerun them locally; this workspace has no MatrixOne checkout or Go/native harness. I found no remaining actionable blocker in the updated source.