fix(hnsw): restore multi-threaded build; vendor USearch #735 fix - #25163
Conversation
fix Concurrent USearch add() could orphan HNSW graph nodes (a vector stored but never linked, so search() couldn't reach it -> flaky recall@1). MatrixOne matrixorigin#24849 worked around it by forcing single-threaded builds everywhere. The root cause is fixed upstream-style in our vendored libusearch (two-pass add: form ALL forward links before ANY reverse link, so a node is never reachable as a descent seed while a lower level is still empty), so the workaround is no longer needed. - thirdparties/usearch-2.25.3.tar.gz: patched index.hpp with the matrixorigin#735 fix (pristine v2.25.3 source, only index.hpp/test.cpp changed; CMakeLists still march=native so the Makefile's sed applies as before). - build.go: drop the hardcoded `nthread := 1`; restore the real concurrency estimate (GetConcurrency / GetConcurrencyForBuild from nworker/ThreadsBuild). - sync.go: CDC/sync paths use GetConcurrencyForBuild directly. - types.go: remove the GetConcurrencyForSingleThreadBuild stopgap. - zz_orphan_test.go: enable TestZZBuildOrphan as a regression guard — 30x 8-thread builds with the BVT t2 params (M 64, EF_CONSTRUCTION/SEARCH 200), rotating insertion order each run to mimic `load data ... parallel 'true'`; asserts 0 orphans. Auto-skips without the SIFT fixture (~5s when present). Validated: zz_orphan_test 0/30 multi-threaded (was ~1/30 pre-fix); 1M wiki_all HNSW build clean (recall@10 82% at M=8); vector_hnsw_async t2 BVT 30/30. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix Concurrent USearch add() could orphan HNSW graph nodes (a vector stored but never linked, so search() couldn't reach it -> flaky recall@1). MatrixOne matrixorigin#24849 worked around it by forcing single-threaded builds everywhere. The root cause is fixed upstream-style in our vendored libusearch (two-pass add: form ALL forward links before ANY reverse link, so a node is never reachable as a descent seed while a lower level is still empty), so the workaround is no longer needed. - thirdparties/usearch-2.25.3.tar.gz: patched index.hpp with the matrixorigin#735 fix (pristine v2.25.3 source, only index.hpp/test.cpp changed; CMakeLists still march=native so the Makefile's sed applies as before). - build.go: drop the hardcoded `nthread := 1`; restore the real concurrency estimate (GetConcurrency / GetConcurrencyForBuild from nworker/ThreadsBuild). - sync.go: CDC/sync paths use GetConcurrencyForBuild directly. - types.go: remove the GetConcurrencyForSingleThreadBuild stopgap. - zz_orphan_test.go: enable TestZZBuildOrphan as a regression guard — 30x 8-thread builds with the BVT t2 params (M 64, EF_CONSTRUCTION/SEARCH 200), rotating insertion order each run to mimic `load data ... parallel 'true'`; asserts 0 orphans. Auto-skips without the SIFT fixture (~5s when present). - vector_hnsw_async.sql/.result: bump t2's post-build wait sleep(20)->sleep(30) so the async index is reliably visible before the NN query (the build's model becomes searchable a beat after sleep(20) under load — a visibility/timing flake, not an orphan). Validated: zz_orphan_test 0/30 multi-threaded (was ~1/30 pre-fix); 1M wiki_all HNSW build clean (recall@10 82% at M=8); vector_hnsw_async 5/5 at 100%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o usearch_build_fix
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
…o usearch_build_fix
|
Queued — the merge queue status continues in this comment ↓. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
I re-checked this from the correctness / unhappy-path angle. The vendored USearch #735 patch looks directionally right, and the focused orphan test passes locally, but restoring multi-threaded HnswBuild re-enables two MatrixOne-side lifecycle bugs that are not covered by the new tests.
Blocker 1: async worker errors can be lost at finalization, and later producers can also hang.
Add() only polls err_chan before enqueueing (pkg/vectorindex/hnsw/build.go:158-168). If a worker fails while processing the last queued item, that Add() already returned nil. ToInsertSql() then only calls CloseAndWait() and never drains/joins err_chan (pkg/vectorindex/hnsw/build.go:271-284), so the build can be finalized as successful even though a worker returned an error. The same channel lifecycle is also risky for unhappy paths such as context cancellation: workers can exit, but a producer may still block on h.add_chan <- ... because the send does not select on worker error or context cancellation.
Please make finalization return the joined worker error, e.g. have CloseAndWait()/ToInsertSql() drain err_chan after wg.Wait(), and make the enqueue path avoid blocking forever once workers have failed or the context is cancelled. A focused regression should cover a multi-threaded build where the final queued vector fails (dimension mismatch or cancelled context) and ToInsertSql() must return that error.
Blocker 2: capacity rollover can save/destroy an index while other workers still have in-flight adds assigned to it.
In the multi-thread path, getIndexForAddSync() reserves a slot under h.mutex, but the actual idx.Add() happens after the lock is released (pkg/vectorindex/hnsw/build.go:178-239). When another worker crosses IndexCapacity, it receives the previous index as save_idx, calls SaveToFile() outside the lock, and SaveToFile() saves then destroys idx.Index (pkg/vectorindex/hnsw/model.go:144-224). That can race with earlier workers that already reserved slots in the old index but have not finished idx.Add() yet. The result can be an incomplete persisted HNSW file, usearch index is nil, or native use-after-destroy behavior depending on the interleaving.
Please bound the old-index lifecycle before saving it: track per-index in-flight adds / generation completion, or otherwise ensure all adds assigned to an index have completed before SaveToFile() can save and destroy that index. A regression with small IndexCapacity, nthread > 1, and enough concurrent adds to force rollover should verify all keys survive and worker errors are surfaced.
Local checks I ran:
git diff --check origin/main...HEAD
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/vectorindex/hnsw -run 'TestBuildMultiWorker|TestZZBuildOrphan' -count=1Both passed, but they do not cover the two lifecycle cases above.
…ent build Restoring multi-threaded HnswBuild re-enabled two lifecycle races: Blocker 1 — lost worker errors + producer hang. A worker failing on the last queued vector failed after Add() already returned nil, and finalization never drained err_chan, so a corrupt build reported success. Add()'s enqueue was also an unconditional blocking send, so a full buffer blocked the producer forever once workers died. Replace the poll-once err_chan with a first-error record (workerErr) plus a `stopped` channel closed on the first failure or context cancellation. Add() now selects on the send vs <-stopped (can't block once workers are gone, surfaces the error); CloseAndWait() returns the recorded error; ToInsertSql()/Destroy() propagate it up through the existing hnsw_create error path. Blocker 2 — rollover saves/destroys an index with in-flight adds. A worker crossing IndexCapacity received the previous index as save_idx and called SaveToFile() (which saves AND destroys idx.Index) outside the lock, racing peer workers still doing idx.Add() on it (use-after-destroy / partial save; observed as "usearch index is nil"). Add a per-index in-flight WaitGroup on HnswModel: reserve the slot under the same lock that decides rollover (getIndexForAdd), release after the add, and Wait() it before SaveToFile(). The rolled-over index gets no new adds, so the wait converges; the crossing worker's own add targets the new index, so no self-deadlock. Regressions (both fail on the old code, pass with the fix, run under -race): - TestBuildMultiWorkerLastItemError: dim mismatch on the final queued vector must surface from ToInsertSql(). - TestBuildMultiWorkerRollover: small IndexCapacity + 8 workers + 1000 adds forcing ~50 rollovers; all keys survive and finalization succeeds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head from the multi-threaded build / unhappy-path angle. The two lifecycle blockers I raised on the previous revision look fixed now.
What changed in this head:
- Worker failures are now recorded with
recordWorkerErr, producers can stop oncestoppedis closed, andToInsertSql()returns the recorded worker error afterCloseAndWait(). This covers the “last queued item fails after Add returned nil” case. - Rollover now reserves an
inflightslot on the assigned index underHnswBuild.mutex, and waits for the rolled-over index's in-flight adds beforeSaveToFile()saves/destroys it. That addresses the save/destroy vs concurrentidx.Add()race. - Focused regressions were added for both cases: final worker error propagation and small-capacity multi-worker rollover.
Local checks:
git diff --check origin/main...HEAD
git merge-tree --write-tree HEAD origin/main
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/vectorindex/hnsw -run 'TestBuildMultiWorker|TestBuildMultiWorkerLastItemError|TestBuildMultiWorkerRollover|TestZZBuildOrphan' -count=1
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test -race ./pkg/vectorindex/hnsw -run 'TestBuildMultiWorkerRollover|TestBuildMultiWorkerLastItemError' -count=1
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/sql/colexec/table_function -run 'TestHnswCreate|TestHnswCreateF64' -count=1All passed locally. LGTM.
Merge Queue Status
This pull request spent 1 hour 10 minutes 26 seconds in the queue, including 1 hour 10 minutes 1 second running CI. Required conditions to merge
|
5 participants
Concurrent USearch add() could orphan HNSW graph nodes (a vector stored but never linked, so search() couldn't reach it -> flaky recall@1). MatrixOne #24849 worked around it by forcing single-threaded builds everywhere. The root cause is fixed upstream-style in our vendored libusearch (two-pass add: form ALL forward links before ANY reverse link, so a node is never reachable as a descent seed while a lower level is still empty), so the workaround is no longer needed.
nthread := 1; restore the real concurrency estimate (GetConcurrency / GetConcurrencyForBuild from nworker/ThreadsBuild).load data ... parallel 'true'; asserts 0 orphans. Auto-skips without the SIFT fixture (~5s when present).Validated: zz_orphan_test 0/30 multi-threaded (was ~1/30 pre-fix); 1M wiki_all HNSW build clean (recall@10 82% at M=8); vector_hnsw_async t2 BVT 30/30.
What type of PR is this?
Which issue(s) this PR fixes:
issue #24977
What this PR does / why we need it:
patch the fix with usearch PR: unum-cloud/USearch#772
add unit test to test the same bvt case flaky test to make sure no orphan in final index.
revert the change build thread = 1