Hardening Allocations & Concurrency - #764
Merged
Merged
Conversation
A freshly-`make`d dense index inherited `index_gt`'s `{0, 0}` thread
limits. The wrapper paths `load_from_stream` and `view` then sized
`available_threads_` and `cast_buffer_` from those zero limits, so the
first `search` threw "No available threads to lock".
Reserve `index_limits_t{}` inside `make` itself so the returned index is
immediately usable. The new fourth parameter accepts an explicit
`index_limits_t`; pass `{unreserved}` (a `std::defer_lock`-style tag) to
skip the upfront allocation when the next call will overwrite everything
anyway. Floor `std::thread::hardware_concurrency()` at 1 inside
`index_limits_t`'s default ctor so hosts where the runtime returns 0 don't
land in the same trap. `load_from_stream` and `view` use the new
`index_limits_t::with_thread_defaults()` helper to fill in zero thread
counts inherited from a prior `unreserved` index.
Drop the now-redundant post-`make` reserves in the Rust and C bindings
and the `hc==0` fallback in the Java binding - they are centralized.
Co-authored-by: Mikhail Chichvarin <6496186+desertfury@users.noreply.github.com>
Co-authored-by: Ash Vardanian <1983160+ashvardanian@users.noreply.github.com>
Co-authored-by: Mikhail Chichvarin <desertfury@nebius.com>
`index_dense_gt::change_metric` reassigned `metric_` in place but left
`cast_buffer_` sized by the previous metric's `bytes_per_vector()`. A
follow-up `add` or `search` then wrote
`metric_.bytes_per_vector() * thread_id` past the start of the buffer,
overrunning the slot belonging to the next thread - silent corruption,
ASan-detectable.
Add `try_change_metric` as the fallible variant, paired with a throwing
`change_metric` - mirroring `try_reserve` / `reserve`. The new helper
re-allocates `cast_buffer_` if the new metric needs more bytes per
vector and rebuilds the cast dispatch table when the scalar kind also
changes.
Wire the new contract through every binding that exposes the API:
* C ABI: `usearch_change_metric` and `usearch_change_metric_kind`
route through `try_change_metric` and populate `*error` on OOM
instead of letting a C++ exception escape the boundary.
* Rust: cxx bridge declarations gain `-> Result<()>` so a thrown
`std::runtime_error` is caught and surfaced as `cxx::Exception`.
`Index::change_metric_kind` and the `VectorType::change_metric`
trait impls propagate the result.
* Python: pybind11 already translates `std::runtime_error` to a
Python `RuntimeError`, no wrapper change needed.
`flat_hash_multi_set_gt::equal_iterator_gt::operator++` could spin forever
when every slot in the hash table was either live or tombstoned. The
`do { ... } while (true)` loop only exited on an empty slot or a match,
and tombstones accumulate monotonically without compaction - so a
long-running workload that adds and removes through the table eventually
loses every empty slot, and the next `remove()` call would peg a CPU
core indefinitely.
Replace the unbounded loop with a `for` bounded by `capacity_slots_`.
After a saturated probe the iterator becomes `end()` and the surrounding
`std::distance` / `for` over `equal_range` terminates normally. No new
branches; the probe upper bound moves from an unreachable `while (true)`
into the `for` header.
Closes #753
Relates to #726
`usearch_change_threads_add` and `usearch_change_threads_search` called `try_reserve(limits)` and discarded its boolean return. On allocation failure they returned silently, leaving the caller convinced the change had succeeded while the underlying thread pool was unchanged or - worse - half-resized. Subsequent `add`/`search` calls would then either run on a stale pool or fail with a misleading downstream error. Populate `*error` when `try_reserve` returns false. Behavior of the success path is unchanged.
`usearch_metadata` and `usearch_metadata_buffer` wrote to `*error` on a failed read of the index header, then continued executing - reading `result.head` fields whose contents are unspecified after the failure and overwriting the caller's `options` struct with whatever happened to be there. Callers that conscientiously checked `*error` afterwards would still see corrupted `options`. Return from the failure branch instead of falling through. No other changes to the success path.
On `index_dense_t::make` failure, `usearch_init` set `*error` and then continued to `new index_dense_t(std::move(state.index))`, returning a pointer to a default-constructed (non-functional) index. Callers who checked `*error` could still receive a non-NULL pointer and invoke subsequent C-ABI calls on it - all undefined behavior because the underlying `typed_` pointer is null and no metric is set. Return `NULL` inside the existing failure branch and switch the heap allocation to `new (std::nothrow)` so an out-of-memory condition there also surfaces through `*error` instead of throwing across the C-ABI boundary. No public signature changes.
`Napi::Error::...ThrowAsJavaScriptException()` only schedules the JS-level exception - C++ control flow continues. The previous `Remove` implementation kept iterating after a failed `native_->remove(...)`, re-reading `result.error` after its inner buffer had already been `.release()`d and writing past-end values into `results[i]`. The next turn of the JavaScript event loop would then see both the scheduled exception and a return value, with undefined behavior in between. Return `env.Null()` from the failure branch so the loop stops at the first error, leaving the exception as the sole observable outcome. The `Count` and `Contains` siblings don't need the same treatment - their inner calls return primitives rather than a result struct with an error.
`ring_gt::try_push` was written as `return push(value); return true;` - ill-formed C++ because the inner `push` returns `void`. It compiled only because `try_push` happens to be uninstantiated in the current build; the first downstream user of the method would trip on a compile error. Drop the unreachable `return true;`, call `push(value)` as a statement, and return `true` after it. Matches the standard "try X / X-or-throw" pair style used elsewhere in the file.
Python's Global Interpreter Lock was being held across every multi-threaded C++ entry point - bulk `add`, `search`, brute-force `exact_search`, `cluster_many_brute_force`, `cluster`, `join`, `compact`, `remove(compact=True)`, and `merge_paths` - so other Python threads couldn't make progress while the executor was running its worker pool. Throughput-bound workloads that interleave a USearch call with any other Python-driven I/O or compute saw no parallelism at all. Make `progress_t::operator()` self-acquire the GIL with `gil_scoped_acquire` (reference-counted, no-op when the GIL is already held) so it stays safe to invoke from any thread; release the GIL with `gil_scoped_release` around each long-running C++ call; and acquire it briefly inside worker lambdas only around `PyErr_CheckSignals()`. The `progress_func_t` typedef is `std::function`, not `py::function`, so its destructor on GIL-released threads is benign.
After the previous commit released the GIL around long C++ operations, the contract is: another Python library running on the same or a different Python thread can do useful work concurrently, while the `Index` API itself is still single-threaded from Python's perspective. Add five focused tests that assert this contract end-to-end. A background Python thread increments a counter in a tight loop while the main thread runs a long `add`/`search`; the counter must advance by at least a conservative floor during the call, otherwise the GIL was held. Progress callbacks fire from a C++ worker that must reacquire the GIL to invoke the Python callable, mutate a list, and return a bool; returning `False` must terminate the operation cleanly as a Python `RuntimeError` rather than UB. Tested locally against Python 3.10, 3.13, and free-threaded 3.14t (`Py_GIL_DISABLED=1`); all five pass on each.
The documented contract is one Python thread per `Index` at a time - the native `index_dense_t` carries per-worker `cast_buffer_` slots indexed by an executor-local `thread_idx` the binding picks for each call, so two Python threads spawning their own executors collide on those slots. After releasing the GIL around long C++ ops in the previous commit, accidental concurrent access from two Python threads went from "silently incorrect" to "segfault". Defensive enforcement is the right behavior. Add a `unique_ptr<std::mutex>` to `dense_index_py_t` and `dense_indexes_py_t` (held via `unique_ptr` so the wrapper stays move-constructible for pybind11's factories). Each binding entry point that releases the GIL now acquires the per-index mutex first; the order is GIL-release-then-lock so a Python thread waiting on the mutex does not hold the GIL - otherwise the current owner's worker thread would block forever in `gil_scoped_acquire` when invoking the progress callback. Hoist `try_reserve` into the locked region in every site so concurrent callers can't race on it either. Heavy ops covered: bulk add/search, multi-shard search, cluster, join, compact, isolate from `remove(compact=True)`, multi-shard merge. Extend `test_gil_release.py` with `test_concurrent_access_serializes_safely` that spawns six Python threads (adders with disjoint key ranges, searchers, lock-free getters) on a single index and asserts every key landed, no thread errors, no crash. Validated under both stock CPython 3.14 and free-threaded 3.14t (`Py_GIL_DISABLED=1`); existing 608-test suite passes unchanged.
`index_dense_gt::clear()` documents that it zeros `size()` while keeping `capacity()`. The previous implementation called `vectors_lookup_.reset()`, freeing the per-slot pointer table entirely, so a subsequent `add()` would dereference a null entry and segfault before any caller could re-reserve. Zero the entries in place instead - the pointers they held were stale the moment `vectors_tape_allocator_.reset()` ran on the next line, but the buffer itself stays sized so future `add()` calls land in valid memory. Co-authored-by: Mikhail Chichvarin <6496186+desertfury@users.noreply.github.com> Co-authored-by: Ash Vardanian <1983160+ashvardanian@users.noreply.github.com> Co-authored-by: Mikhail Chichvarin <desertfury@nebius.com>
In a multi-vector index, `rename(from, from)` would keep removing and re-inserting entries under the same key, so the lookup loop could continue finding the freshly inserted slot forever. Co-authored-by: Mikhail Chichvarin <6496186+desertfury@users.noreply.github.com> Co-authored-by: Mikhail Chichvarin <desertfury@nebius.com> Co-authored-by: Ash Vardanian <1983160+ashvardanian@users.noreply.github.com>
Reject missing metrics before replacing the active metric, and mirror the same validation in the callback-based C entry point. The existing `try_change_metric` path still handles cast-buffer growth failures separately. Co-authored-by: Mikhail Chichvarin <desertfury@nebius.com> Co-authored-by: Mikhail Chichvarin <6496186+desertfury@users.noreply.github.com> Co-authored-by: Ash Vardanian <1983160+ashvardanian@users.noreply.github.com>
The integer-quantizing casts normalize input vectors by their magnitude. Zero or NaN magnitudes produced NaN scaled values, and casting those values to `int8_t` or `uint8_t` is UB. Co-authored-by: Mikhail Chichvarin <desertfury@nebius.com> Co-authored-by: Ash Vardanian <1983160+ashvardanian@users.noreply.github.com> Co-authored-by: Mikhail Chichvarin <6496186+desertfury@users.noreply.github.com>
`index_dense_gt::try_reserve` reused the keyed lookup capacity as the member limit after reserving. The lookup reported physical hash slots, so later thread-only reserves fed that larger number back into the hash-table growth path and kept expanding the table. Co-authored-by: Mikhail Chichvarin <desertfury@nebius.com> Co-authored-by: Ash Vardanian <1983160+ashvardanian@users.noreply.github.com> Co-authored-by: Mikhail Chichvarin <6496186+desertfury@users.noreply.github.com>
Add checked size arithmetic helpers and wire them into allocation, reservation, serialization, and dense metadata paths that previously relied on unchecked size_t math. Co-authored-by: Mikhail Chichvarin <desertfury@nebius.com> Co-authored-by: Ash Vardanian <1983160+ashvardanian@users.noreply.github.com> Co-authored-by: Mikhail Chichvarin <6496186+desertfury@users.noreply.github.com>
After a failed load resets the underlying graph, the dense wrapper can remain mutable while its per-thread context buffers are empty. The next operation could then acquire no worker thread and index into empty per-thread storage. Co-authored-by: Mikhail Chichvarin <desertfury@nebius.com> Co-authored-by: Ash Vardanian <1983160+ashvardanian@users.noreply.github.com> Co-authored-by: Mikhail Chichvarin <6496186+desertfury@users.noreply.github.com>
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.
No description provided.