Skip to content

Native HNSW traversal plane: mmap graph file, off-event-loop search, opt-in dual-write (phase 1) - #2430

Draft
kriszyp wants to merge 61 commits into
mainfrom
kris/hnsw-native-plane
Draft

Native HNSW traversal plane: mmap graph file, off-event-loop search, opt-in dual-write (phase 1)#2430
kriszyp wants to merge 61 commits into
mainfrom
kris/hnsw-native-plane

Conversation

@kriszyp

@kriszyp kriszyp commented Sep 1, 2026

Copy link
Copy Markdown
Member

What this is

A native traversal plane for HNSW vector indexes: a Rust (napi-rs) module storing the graph
in a memory-mapped fixed-slot file with search off the JS event loop, plus an opt-in,
dual-write
phase-1 integration. With the nativePlane index option on an int8 cosine HNSW
index, every graph mutation the JS implementation persists is mirrored into the plane file
(host-allocated ids — the plane graph mirrors the authoritative CF graph), and search()
routes through the native module. Flag off, or native artifact absent: behavior is unchanged.

Design doc: hnsw-native-plane.md (in this diff). The crate lives in
HarperFast/hnsw and ships as
@harperfast/hnsw (Apache-2.0, prebuilds
for linux-x64/arm64, darwin-arm64, win32-x64). Harper consumes it as an exact-pinned
optional dependency; native/hnsw-plane/ is gone from this repo.

Why

Measured at 5M nodes / ef 512, ~85% of a JS search visit is object bookkeeping — not distance
math, not I/O — and a RocksDB Get per visit (~1–2 µs warm) dwarfs the ~50 ns SIMD distance
it feeds. A native loop over direct-addressed mmap slots fixes both:

1M × 768-d int8, ef 512 JS (main) native plane
search p50 7.2 ms 0.75 ms
per-visit cost 4.34 µs 0.33 µs
recall@10 (set) 0.997 0.999
concurrent n/a (event-loop bound) 6,345 QPS × 8 threads + 1,102 inserts/s writer

Calibrated Gaussian-mixture corpus (benchmarks/hnsw-scale.js methodology), brute-force
ground truth. This is the enabling dependency for same-node index slicing, moves search off
the event loop (#693, #711), and the plane's id freelist structurally fixes #2182.

Crash-recovery protocol (the hard part)

The cross-model review iterated this design through five refutations, each fixing the prior
round's weakest assumption; the sequence is worth reading as the contract's rationale:

  1. Open-time scrub of torn locks → wrong: re-armed by flush ticks; a second worker's open
    could unlink a live plane.
  2. Timeout-based lock takeover → wrong: a live writer descheduled under cgroup CFS throttling
    gets robbed; two writers splice one slot.
  3. PID-identity takeover → wrong: containerized Harper is pid 1, restarts as pid 1; pid reuse.
  4. Final protocol: per-open-handle registry tags with kernel OFD byte-range locks as
    liveness (die with the handle; immune to pid reuse/namespaces); takeover only of provably
    dead owners, sanitizing the slot (half-written payloads read as deleted, heal on rewrite);
    readers degrade after a 20 ms window; writers surface Err(Wedged) after a bounded 5 s on
    an unreclaimable lock — Harper's mirror error path then disables and rebuilds the plane.
    Non-Linux platforms never take over (bounded degradation only).

Each step is pinned by a test (tests/reopen.rs: same-pid-restart takeover, dead-writer
sanitization, live-writer-never-robbed, double-remove freelist cycle, truncated files, full
plane, odd-dims alignment; plus the concurrent torture suite that caught two real races and a
closest-keep prune orphaning bug during development).

Reviewed and accepted (decision record)

  • Mirroring runs at the indexStore.put sites, inside the transaction — rollback phantoms
    are filtered at record load; the structural fix (commit-driven mirror) is phase-2 alongside
    watermark/replay wiring. Documented in hnsw-native-plane.md §11.
  • Relaxed durability — flush barrier every 4,096 mirrored mutations (data before
    watermark, on the libuv pool); torn slots heal on touch; CF stays authoritative.
  • Crate-level Fatal TSFN — the npm loader wraps predicates (throw ⇒ batch denied, promise
    rejected); Harper's adapter guards independently.
  • Fixed 16M-node sparse reservation per index — growth (mremap) is phase-2; ids past the
    cap disable the plane with a logged error.
  • Per-query bitset copy, per-query heap allocations — measured immaterial at current
    scale; follow-ups noted.

Scope and non-goals (phase 1)

Dual-write with CF authoritative; rollback = flag off (file deleted); int8 cosine indexes
only; no slicing, no plane-primary, no binary quantization (format reserves the field).

Testing

  • Crate: tested in HarperFast/hnsw's own CI (cargo test --release on linux/macOS/Windows) — crash-window/lifecycle, upper-entry lifecycle, the
    concurrent torture suite, and the first-insert/entry-point/invalidation regressions this
    work added. No longer re-run here; see the fourth pass below.
  • Harper: unitTests/resources/vectorIndexPlane.test.js — 18 tests: dual-write parity with
    the JS path at equal ef (including update-in-place, delete, restart/reopen), predicate
    parity, throwing-filter error surfacing, sync-iteration contract, overlapping-next()
    cursor sharing, abandoned-iterable rejection handling, lazy first-enable build,
    incomplete-mirror gating, unopenable-file degradation, invalidation (in band and through the
    durable sidecar), orphaned-tombstone rebuild, disable/drop cleanup. Suite self-skips when the
    native package is absent.
  • CI: dedicated hnsw-plane job (install, prebuild load probe, parity suite); all other jobs
    unchanged.

For the human reviewer

  • The crash-recovery protocol above is the concentrated-risk area; hnsw-native-plane.md §5
    and src/seqlock.rs's
    module doc carry the full argument. That code is no longer in this diff — it is reviewed in
    its own repo.
  • resources/search.ts now supports promise-returning custom-index searches (async-only
    iterable); synchronous consumers of plane-backed results throw — one covered path, noted
    as a contract change.
  • First-enable builds run in background chunks; sub-chunk (small) graphs complete
    synchronously so readiness is immediate — the difference is only observable on large
    existing indexes.
  • smoke (linux/amd64) is red on this branch and on main at 2e65550d for the same
    reason — rocksdb-js loaded a nested msgpackr@2.0.6 in the packed image. Nothing in this
    diff touches dependency resolution.
  • The drain-latency regression asserts a wall-clock bound (best of five queries under 25 ms).
    The reviewer's remaining objection is that sustained runner contention could hold all five
    above it; every alternative that keeps the property covered is also wall-clock, so this is
    kept as-is rather than dropped — flake it once and the right answer is a longer poll
    interval behind cfg(test), not a looser threshold.
  • Three shipped-as-coded policy choices the fourth pass's review surfaced, each reversible but
    each worth an explicit yes: any plane error anywhere in the lifecycle deletes the whole
    shared file and forces every worker to rebuild from the CF, rather than failing that one
    operation; an unclassified predicate throw (a bare string or a frozen value, where tagging
    PLANE_PREDICATE_ERROR itself throws) falls through to disablePlane rather than to a
    query-scoped failure, so a legal-but-discouraged app filter can take a shared derived index
    offline; and phase 1 ships only the TSFN predicate path for filtered search, not the bitset
    fast path, which bounds what the 10–15× actually delivers for RBAC-secured deployments.
  • The review CLI printed framing-recheck: REQUIRED on round 18. It is counting rounds and
    fresh majors across this branch's whole history, and every surviving major is in the phase-1
    integration contract hnsw-native-plane.md §10/§11 records as decided — not in the
    packaging swap the fourth pass made. Re-running --mode plan would re-litigate that settled
    framing, so it was not run; flagging it here rather than silently clearing it.

Review-thread pass (dispatch)

Closes the five unresolved threads on this PR — the two crate blockers, the human blocker,
and both non-blocking suggestions. Plane format is VERSION 6; older files are rejected at
open and reindexed, which is the documented phase-1 rollback path.

  • Empty-graph inserts no longer orphan the losers.
    claim_entry_if_empty
    is a strict compare-exchange from the empty encoding, so exactly one racer roots the
    graph; set_entry_point_if_not_better cannot serve here because a not-worse install would
    put a second edgeless node over the winner and orphan everything already rooted at it. Both
    self-promotion sites go through it, and a loser joins the winner's graph, rewriting the
    upper entry its own slot already names rather than minting a second one. An insert that
    cannot resolve an entry point within its retry bound now returns Err rather than Ok for
    a node nothing points at. Regression: racing_first_inserts_all_stay_reachable — 200 fresh
    graphs, 4 barrier-synced first inserts each, asserted by id membership; it fails on round 0
    with the claim reverted.
  • Every field a concurrent reader acts on is an aligned volatile load. The vector is
    padded so neighbor and upper-id arrays are 4-aligned
    (neighbor_offset),
    and graph.rs reads flags, level, degree, scale, inv_mag, neighbor ids and upper ids
    through
    vread.
    The stored vector deliberately stays an ordinary load so cosine_int8_raw keeps
    autovectorizing — a torn vector only perturbs a distance the generation check discards.
    This does not make the access race-free under Rust's memory model (only atomics would; §10
    records that as the follow-up), but it does forbid the reload/split/sink across the seqlock
    window that lto = true, codegen-units = 1 licenses. Benchmarked, since the layout
    changed: cargo run --release --bin bench 50000 768 200 512 <path> 128 on fd5c8845
    vs this head, 8 interleaved A/B rounds on one machine — median 0.2135 → 0.212 µs/visit
    (mins 0.205 / 0.209), visits/query 1342 and recall@10 1.000 identical.
  • A dead entry point is repaired from both sides. delete_node re-elects before the
    fallible upper cleanup and while the node is still readable, so no window publishes a
    tombstoned entry point; re-election skips the node it is replacing, and one that finds no
    candidate clears the entry only while it still names that node (clear_entry_point_if),
    instead of erasing an entry a concurrent insert installed. On the read side,
    resolve_entry
    repairs an entry no writer ever will (a slot a reader sanitized after its writer died had no
    delete at all) through the O(1) previous-entry hint that promotions now record, backed — for
    when the hint is dead too — by a probe capped at 1,024 slots that walks down from the newest
    id with a stride spanning the whole range and a rotating start, never the O(high-water)
    scan that would stampede the pool thread every search runs on. Both properties are
    load-bearing. The stride, because Harper allocates node ids monotonically and never reuses
    them (Atomics.add), so a churned table has its entire low prefix tombstoned and only the
    newest ids live, while the crate's own freelist reuses ids and keeps live nodes low — a fixed
    window at either end is blind to one of those. The rotation, because a fixed start probes one
    residue class of the stride forever, so a live graph lying entirely between its samples would
    be invisible permanently rather than for one search; rotating makes stride consecutive
    repairs cover every id while each stays capped at limit. The rotation is per handle, not
    per process — a shared counter is advanced by every other plane's repairs too, which can pin
    one plane to a single residue indefinitely. It
    publishes through
    replace_entry_if, strict on the entry it observed dead: a not-worse install
    would displace a live level-0 root a concurrent first insert had just claimed, orphaning it.
    Regressions: a_wedged_upper_cleanup_still_reelects_the_entry_point,
    search_repairs_an_entry_point_no_writer_will,
    search_repairs_an_entry_point_whose_hint_is_dead_too,
    search_repairs_an_entry_point_in_a_churned_graph_whose_low_ids_are_all_dead,
    a_repair_probe_rotates_so_no_live_node_stays_between_its_samples,
    repair_probe_rotation_is_per_plane_not_per_process,
    a_repair_never_displaces_a_root_installed_while_it_ran.
  • One shared cursor per iterate(). resources/search.ts memoizes the
    iterator-creation promise plus a closed flag, so overlapping next() calls advance one
    cursor instead of each building its own over the same array; a handler on the pending
    pipeline keeps an abandoned iterable (aborted request, limit: 0) from reaching Node's
    unhandledRejection.
  • An undeletable plane is invalidated in band before the sidecar.
    invalidatePlaneFile
    calls PlaneFile::invalidate — watermark 0 plus an msync of the header page
    alone — and only then creates the .stale sidecar. The watermark makes the file read as an
    incomplete initial mirror, which planeSearchReady already refuses and
    PLANE_INCOMPLETE_REBUILD_MS already rebuilds. The barrier is synchronous on purpose: the
    sidecar is an empty file whose directory entry is never fsynced, so creating it before the
    watermark was durable would let a power loss keep the old nonzero watermark and lose the only
    marker. A whole-mapping flush gives the same ordering but cannot run inline on a multi-GB
    plane — hence a 4 KB header barrier rather than flush(0), which is sound because the data is
    being discarded and lowering the watermark is always the safe direction. The sidecar still
    follows, because another process may still be mapping the inode and can re-stamp the watermark
    from its own mirror writes. The flag-off cleanup path now marks rather than only logging, and
    both artifacts are removed with rmSync(force) so a hand-deleted plane file plus a leftover
    sidecar cannot disable the index permanently.
  • concurrent_insert_search's existing assertion was vacuous and is fixed here: its
    corpus quantized to cos ≈ 1 for every pair, so the d < 1e-3 self-query check matched any
    node. The corpus now carries a per-node signature and the assertion is by id — which is what
    makes the 1-in-60 flake this PR reported actually reproducible.

Two residuals the review left open on this delta, both recorded as decisions rather than
fixed, because each is a format or lifetime change rather than a thread-closing fix:

  • In-band invalidation is not sticky. Another worker still mapping the inode re-stamps the
    watermark at its next flush tick (its planeReady is already latched), and the .stale
    sidecar's directory entry is never fsynced — so a power loss inside that window can still
    leave a plane reading as a complete mirror. Strictly narrower than before this PR's fix (which
    had no durable barrier at all), and closing it properly means a sticky format bit, i.e. a
    rebuild. The restart cycle converges it today.
  • HnswPlane has no close(). A one-shot handle releases its mapping and its
    REGISTRY_SLOTS OFD lock only at GC; 64 concurrent live handles exhaust the registry, after
    which a handle registers with self_tag = 0 and its abandoned locks wedge writers for the
    full 5 s bound. Adding close() later means auditing every handle site.

Still open, and deliberately not taken in a thread-closing pass — all pre-existing on this
branch, all outside the five threads:

  • mirrorEntryPointCleared() mirrors before commit (HierarchicalNavigableSmallWorld.ts).
    A transaction that deletes the last indexed row and then aborts leaves the CF entry point
    intact, the plane's cleared, and that node tombstoned in the plane only. The read-side repair
    above bounds the damage — it re-elects on the first search rather than leaving the index
    blind — so the residual is one record silently absent from plane results until it is touched,
    plus a possible entry-level downgrade. §11's phantom-node acceptance does not actually cover
    this mirror site, which is the part worth a decision.
  • Native searches run on the default libuv pool, not the dedicated pool §3/§7 describes;
    four concurrent ef-512 queries can queue rocksdb-js async gets behind them.
  • clean_shutdown can never read falseflush_with_watermark writes 1 unconditionally
    and set_clean_shutdown has no callers, so openedClean() reports clean after kill -9.
    Phase-2 replay is the consumer that will trust it.
  • Upper-entry ownership is still decided outside the slot lock, so two handles
    first-writing the same level≥1 id both allocate and one entry is orphaned (one entry per
    race against a ~2M reserve); region exhaustion is silent (the node stores NO_UPPER and the
    hierarchy flattens).
  • Eligibility does not check mL/max level — the JS MAX_LEVEL (10) is not checked
    against the crate's MAX_UPPER_LEVELS (8), so a large-mL index silently drops its
    level-9/10 adjacency.
  • Plane-file identity is unlink-by-path, with no cross-worker generation check.
    disablePlane in one worker deletes the path a peer still has mapped, so that peer keeps
    mirroring into an orphaned inode until restart; a later independent error can delete a
    different peer's already-healthy rebuilt file. RocksDB stays authoritative, so this is
    derived-index staleness for one worker, not data loss. (Raised by the fourth pass's review.)
  • The predicate TSFN resolves every candidate's primary key through a synchronous CF
    decode
    , so the design doc's bitset fast path for RBAC/companion-condition filters (§7 calls
    that the dominant production shape) is not wired here — filtered nativePlane queries still
    pay the per-node CF read the plane exists to remove. (Fourth pass.)
  • The unit suite never exercises the chunked build path or a second worker. The corpus
    (1,200) stays under PLANE_BUILD_CHUNK (5,000) and the suite runs single-process
    (setMainIsWorker(true)), so the yielding builder, multi-worker attach races, and §9.4's
    kill-9/reopen/replay criterion are all unproven end to end. (Fourth pass.)
  • Interrupted-drop recovery proceeds even when neither the unlink nor the tombstone
    succeeded
    (resources/databases.ts), leaving a plane a same-name recreate could reopen and
    score against an unrelated old graph. Needs a Windows-class EBUSY plus a second independent
    fs failure. (Fourth pass.)
  • Nothing gates serving a plane after an unclean shutdown. §9's crash acceptance criterion
    is unimplemented: with clean_shutdown stuck at 1 (above), planeSearchReady serves any
    plane whose watermark is ≥ 1, so up to PLANE_FLUSH_EVERY (4,096) mirrored writes lost to a
    power cut are invisible to vector queries while the JS path still returns them. One check in
    planeSearchReady now; expensive once operators depend on post-crash availability.
  • Plane-backed searches report nodesVisited: 0 through withStats although napi.rs
    computes stats.visits and discards it, so explain reads zero visits for filtered vector
    queries — the knob operators use to size ef/filterExpansion.
  • The unit suite never empties an index and never aborts a transaction, so
    mirrorEntryPointCleared has no coverage at all; everything it proves is the happy commit
    path.
  • A JS predicate that throws reaches napi_fatal_exception — Harper's own adapter
    catches, so this is a hazard of the generic NAPI surface §10 plans to publish, not a live
    path here.
  • Also unaddressed from earlier rounds: slot_sanitizer discards a bound upper index on
    dead-owner takeover; no validity token, so a filesystem restore or an artifact-less deploy
    leaves a plane trusted; Table.clear() invalidates only the calling worker's handle;
    encodeURIComponent plane filenames collide under case folding on macOS/Windows;
    speculative.retain is O(visits × pending) exactly when verdicts lag; search_layer
    allocates two BinaryHeaps per call though SearchScratch exists to avoid it.

Second review-thread pass (dispatch)

Closes the one thread left open after the round above, plus the one blocking finding the
pre-push review raised on that fix.

  • The repair probe's stride is a ceiling division. Flooring it left stride * limit < hw
    whenever the high-water mark is not a multiple of the 1,024-slot probe limit, so every
    rotated walk stopped above the lowest hw % limit ids — a permanent blind spot rather
    than the one-search one rotation was added to close, since no offset ever reaches it. A
    graph whose entry and hint both die while its survivors sit in that prefix returns empty
    from every later search.
    stride = hw.div_ceil(limit) restores the invariant the rotation rests on,
    stride * limit >= hw. Regression:
    a_repair_probe_reaches_the_low_ids_a_floored_stride_would_never_sample
    keeps one live node inside that prefix; it fails on the parent commit and the two existing
    rotation tests now model the same ceiling stride.
  • A filtered query no longer pays 50 ms after its last verdict. The predicate drain looped
    while let Ok(..) = recv_timeout(50ms), so once the final verdict dropped outstanding to
    zero it sat out one more full timeout — on every predicated search, against a
    sub-millisecond search, on the same shared libuv pool that serves rocksdb-js gets.
    Guarding the drain on outstanding returns on the last verdict and turns a
    stray verdict into a break rather than a usize underflow. Measured from the evaluator's
    last send in
    a_predicated_search_returns_as_soon_as_the_last_verdict_lands:
    50.2 ms with the guard reverted, sub-millisecond with it.
  • Also silenced the crate's one build warning — remove_edge's level-0 branch dropped a
    Result implicitly where its level>0 sibling two lines down already did so explicitly.

Verification: cargo test --release --manifest-path native/hnsw-plane/Cargo.toml — 25 tests
green (22 reopen, 2 concurrent, 1 lib), both new tests checked to fail with their own fix
reverted; npm run build:hnsw-plane builds the napi artifact. The JS parity suite
(unitTests/resources/vectorIndexPlane.test.js) is unaffected — neither path is reachable from
it, and CI's hnsw-plane job runs it.

Third pass (dispatch): the backlog, not the threads

All eight review threads were already resolved when this pass started, and both bot rounds on
2fb3a3b7 reported no blocking issues — so it took three items off the open list above instead.
Each is a defect with one right answer rather than a design question; everything that needs a
judgment call is still listed, unchanged.

  • The contended reverse-edge merge no longer drops the edge it is adding. push then
    truncate(cap) discards the tail, and at exactly cap the tail is the new id — so a freshly
    inserted node lost its in-edge from that neighbor precisely in the contended-and-full case the
    fallback exists to serve. merge_neighbor_capped displaces an existing neighbor
    instead. Which one is arbitrary — appends push at the tail, so a list is distance-ordered only
    right after a prune — and that is the premise the pre-push review corrected in this delta: an
    arbitrary loss is the right trade against a systematic one, and choosing by distance would put
    major-faulting work under the slot lock. Regression:
    a_contended_merge_into_a_full_neighbor_list_keeps_the_edge_it_adds.
  • A refused predicate enqueue no longer holds the drain for its full deadline.
    call_with_return_value drops its callback when the queue is closing or full, so no verdict can
    arrive — yet the batch was still counted outstanding, and the tail drain sat out the whole 5 s
    DRAIN_TIMEOUT on every filtered query in flight during teardown.
    PredicatePipe::dispatch now reports whether the batch was handed off, and the
    NAPI leg returns that from the enqueue status. Regression:
    a_refused_predicate_enqueue_does_not_hold_the_drain — 5.02 s on the parent
    commit, sub-second here.
  • A per-query distance override no longer bypasses the metric. A distance: 'euclidean'
    query against a cosine index was traversed cosine-first, and rescoreResults only corrects the
    reported distances of the candidates it is handed, not which candidates the beam kept. The
    cutover now requires the query's metric to be the index's own, matching the
    dimension-mismatch precedent on the line below it, so an overridden query takes the JS path —
    asserted by parity against the same JS reference the rest of the suite uses.

Also silenced the crate's remaining build warning: the bench's build loop discarded a rejected
insert's Result, which would report build throughput for rows it never indexed and then measure
recall against a corpus the graph does not contain.

Verification: cargo test --release — 28 tests green (4 lib, 2 concurrent, 22 reopen), each new
test checked to fail with its own fix reverted; npm run build:hnsw-plane builds with no warnings;
npm run test:unit:resources 1893 passing, the plane suite 18 of them (up from 17).
npm run test:unit:main is 5163 passing / 2 failing, both environmental on the worker box and in
files this branch never touches — gitCredentials asserts on the absence of GIT_CONFIG_GLOBAL
and GIT_EDITOR, which the worker sets, and configValidator asserts a domain-socket path under
the 107-byte limit, which this worktree's path exceeds. CI's unit jobs are green on this branch.

Fourth pass (dispatch): consume the published package

The crate now lives in HarperFast/hnsw and publishes as
@harperfast/hnsw, so this pass deletes native/hnsw-plane/ (~4,800 lines of Rust, tests and
build scaffolding) and the build:hnsw-plane npm script, and points the adapter at the registry
package instead. That closes the "no prebuilds, so the flag is inert for every non-source
user" item off the open list above: 0.2.1 ships prebuilds for linux-x64/arm64-glibc,
darwin-arm64 and win32-x64, with a source build as the fallback, and it is pinned exactly —
both the root package and every platform binding — because it runs native code in-process.
It is an optional dependency, so a platform with no prebuild and no Rust toolchain
installs Harper normally and the adapter warns once and stays on the JS path.

  • Invalidation moved into the package, and with it the ordering invariant. 0.2.1's
    invalidateFile() / invalidatePlane() set a one-way header latch, zero the watermark,
    msync the header page, and only then write the .stale sidecar — fsync'd, along with its
    directory entry — and open() now refuses a plane carrying either marker. The adapter's
    hand-rolled two-step is
    one call,
    and the test asserts the outcome — both markers durable, a fresh open() throwing — rather
    than the order, which the crate now tests itself. This also closes the "in-band invalidation
    is not sticky" residual recorded in the second pass above: the latch is one-way, so a worker
    still mapping the inode can no longer re-stamp a live watermark over it, and the sidecar is
    no longer an un-fsynced empty file. The refusing open() is the one behaviour change the
    attach path had to absorb — a latched plane whose sidecar was removed by hand used to open
    at watermark 0 and wait out PLANE_INCOMPLETE_REBUILD_MS (an hour) before rebuilding, and
    now takes the throw branch of
    getPlane,
    which retries for PLANE_STALE_CREATE_MS (60 s, since the header msync leaves mtime fresh)
    and then deletes and rebuilds. Strictly faster convergence, on a path that already existed
    for a crashed create.
  • The one path the package cannot serve keeps its own durability. A plane can outlive the
    package that made it (uninstall, or a prebuild that stops loading), and a later reinstall
    would adopt it — so
    the package-absent branch
    writes the sidecar itself and fsyncs both it and the directory entry, matching what the
    package's own sidecar guarantees rather than claiming sidecar: true for a file a power cut
    could lose.
  • CI no longer re-tests the dependency's source. The job installs from the registry and
    runs the prebuild, so cargo test against a pinned source checkout would grade something
    other than the binary under test — and HarperFast/hnsw's CI already runs those tests on
    linux, macOS and Windows. What replaces it is the guard that job actually needs: a
    load probe,
    because vectorIndexPlane.test.js self-skips when the package is unavailable, and a green
    skip is indistinguishable from a green run.
  • Phase 2 now names Derived-index delivery protocol (DerivedIndexBackend): shared post-commit delivery, watermark/replay, and blob-content contract for HNSW and full-text indexes #2489's protocol. hnsw-native-plane.md §8 previously said "commit
    callback"; the delivery, watermark and retention-aware replay are the shared
    DerivedIndexBackend runtime, so the plane implements that rather than growing a second
    post-commit mechanism beside full-text indexing. §11's rollback-phantom limitation points at
    the same work. Refs Derived-index delivery protocol (DerivedIndexBackend): shared post-commit delivery, watermark/replay, and blob-content contract for HNSW and full-text indexes #2489.

The crate-side items on the open list above — default-libuv-pool searches, clean_shutdown
stuck at 1, upper-entry ownership outside the slot lock, napi_fatal_exception on a throwing
predicate (which the package's JS loader now wraps), slot_sanitizer, SearchScratch — are
no longer in this diff. They belong to HarperFast/hnsw's backlog; the Harper-side items are
unchanged and still listed.

Two things this pass fixed from its own pre-push review, both inside code it was already
touching. The job's npm run build || true was copied from the jobs that tolerate type
errors, but this one runs mocha against dist/ — so a tolerated build failure grades a stale
dist and still reports green, the same hole the load probe closes on the package side; it
builds clean on this branch, so
the job now builds strictly.
And createAndMirrorPlane's doc block sat above planeLayer0Cap, describing the wrong method.

That review ran full-scope over the whole branch rather than this delta, and everything else
it raised is phase-1 integration code this delta does not touch. Four are new to the open list
and added below; the rest were already there, or were adjudicated away as accepted-known (the
transaction-abort phantom, which §11 records) or factually wrong (two Gemini findings whose
claimed cold paths are guarded by planeEligible and by getPlane's own retry branch).

Verification: npm run build, npm run typecheck, npm run lint:required and
npm run format:write clean; npm run test:unit:resources 2131 passing / 0 failing, the
plane suite 18 of them, against the registry package with no local crate present. The CI load
probe was run locally against the installed 0.2.1 prebuild.

Refs #2489

🤖 Generated with Claude Code

Review-Coverage: authored=claude; ran=codex,gemini; declined=cursor-grok,cursor-composer,domain; rounds=19 @ 87e0fd7

Human-Review-Need: 4 @ 87e0fd7

@socket-security

socket-security Bot commented Sep 1, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​harperfast/​hnsw@​0.2.1621009993100

View full report

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a native HNSW traversal plane (hnsw-plane) written in Rust using napi-rs to move HNSW graph storage and search traversal off the JS event loop into a memory-mapped fixed-slot file. The TypeScript codebase is updated to integrate this native plane behind an opt-in nativePlane: true option, mirroring graph mutations and routing searches natively when eligible. The review feedback identifies a high-severity correctness bug and memory leak in write_node_raw where a deleted high-level node rewritten with level 0 would incorrectly inherit and leak its stale upper index, along with a redundant capacity check in the NAPI bindings.

Comment thread native/hnsw-plane/src/graph.rs Outdated
Comment thread native/hnsw-plane/src/napi.rs Outdated
Comment thread native/hnsw-plane/src/insert.rs Outdated
Comment thread native/hnsw-plane/src/graph.rs Outdated
Comment thread native/hnsw-plane/src/graph.rs Outdated
Comment thread resources/search.ts Outdated
@claude

claude Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Reviewed. One blocking issue: completeInterruptedDrop (resources/databases.ts) tombstones a stale HNSW plane file without the durable invalidate() step first, reopening the same stale-plane-adoption hazard already fixed elsewhere in this PR — see inline comment. All 8 previously-resolved threads remain genuinely fixed at this head; one non-blocking documentation suggestion also posted inline.

@cb1kenobi cb1kenobi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Barbarian reviewed fd5c884 and found 1 blocking issue. Disabling nativePlane can leave an undeletable plane trusted on a later re-enable. Tombstone the file when deletion fails so it must be rebuilt before serving searches.

Comment thread resources/indexes/HierarchicalNavigableSmallWorld.ts

@cb1kenobi cb1kenobi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Barbarian reviewed 7661fd1 and found no blocking issues. The new commit restores MADV_RANDOM at both mmap creation and reopen sites. No new blocking findings were identified; previously raised findings were not repeated.


Generated by Barber AI

kriszyp added a commit that referenced this pull request Sep 1, 2026
First-entry race: claim_entry_if_empty is a strict CAS from the empty
encoding, so exactly one racer roots the graph and every loser joins it
instead of returning an unlinked node. Both self-promotion sites go
through it; a loser reuses the upper entry its slot already names.

Concurrent reads: pad the vector so neighbor arrays are 4-aligned (and
move the upper-list pad ahead of the ids), then read every field a reader
acts on with an aligned read_volatile. The stored vector stays an
ordinary load so the dot product keeps vectorizing. VERSION 6.

Dead entry points: delete_node re-elects before the fallible upper
cleanup, and searches repair an entry no writer will through the O(1)
previous-entry hint, which promotions now record.

Async iterator: one memoized iterator per iterate() call plus a closed
flag, and a handler on the pending pipeline so an abandoned iterable
cannot raise unhandledRejection.

Stale planes: an undeletable plane is invalidated in band (watermark 0
under a durability barrier) before the .stale sidecar, the flag-off
cleanup path marks instead of only logging, and the sidecar's own
cleanup no longer permanently disables a plane whose file an operator
already removed.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Comment thread native/hnsw-plane/src/graph.rs Outdated

@cb1kenobi cb1kenobi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2fb3a3b and found no blocking issues. No confirmed blocking findings remain in the new changes. The repair-probe coverage and predicate-drain fixes are sound.


Generated by Barber AI

@cb1kenobi cb1kenobi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8dd6c16 and found no blocking issues. The new commits introduce no confirmed blocking issues on changed lines. Previously raised findings were not repeated.


Generated by Barber AI

Comment thread resources/databases.ts
if (error?.code !== 'ENOENT') {
logger.warn(`could not delete the HNSW plane file for ${columnName}; tombstoning it as stale`, error);
try {
closeSync(openSync(planeStalePathFor(planeFilePathFor(rootStore.path, columnName)), 'w'));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interrupted-drop recovery tombstones a stale plane without the durable invalidate step first

What: When the plane file can't be unlinked (the same "Windows EBUSY while still mapped" case this function already anticipates in its own comment), this catch writes the .stale sidecar directly. It never opens the plane and calls .invalidate() first — unlike HierarchicalNavigableSmallWorld.invalidatePlaneFile (called from cleanupDisabledPlane/resetDerivedStorage), which durably zeroes the watermark (a 4 KB header msync) before creating the sidecar, specifically because the sidecar's directory entry is never fsynced.

Why it matters: this is the same defect class already fixed elsewhere in this PR as a blocking issue ("Stale plane survives flag disable" on HierarchicalNavigableSmallWorld.ts:383): if a crash/power-loss lands between this sidecar write and its own durability, the sidecar is lost while the plane file keeps its old nonzero watermark. A same-name table recreated afterward can then adopt the stale plane as ready and silently serve results missing everything from before the interrupted drop.

Suggested fix: before writing the sidecar here, open the plane (if it still exists) and call .invalidate(), mirroring invalidatePlaneFile's ordering — e.g. factor that method into a standalone helper (it's currently private and instance-bound) so both call sites share one implementation instead of two independently-maintained copies of the same durability contract.

Comment thread native/hnsw-plane/src/format.rs Outdated
Ok(plane)
}

/// Force any persisted-odd seqlocks (slot + upper regions) back to even after an unclean

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion (non-blocking): this docstring ("Force any persisted-odd seqlocks... back to even after an unclean shutdown") describes a scrub pass that doesn't exist anywhere in this crate — no such function is defined, and it's attached to slot_ptr, which just computes a pointer offset. It also contradicts the design note a few lines above in open(): "No open-time repair: seqlocks persisted odd by a dead writer are taken over lazily at the contended slot... The clean-shutdown byte remains advisory metadata only." Reads like a leftover from an earlier design that scrubbed on open, before that was replaced by seqlock.rs's lazy dead-owner takeover. The opened_clean field doc above ("An unclean open has had its torn seqlocks scrubbed...") makes the same now-inaccurate claim. Worth deleting/updating both so a future reader doesn't go looking for a scrub step that was never implemented here.

kriszyp and others added 12 commits September 1, 2026 16:03
…slot file)

Design (hnsw-native-plane.md): replace the index CF with a memory-mapped
fixed-slot file as the primary graph store — the file IS the index, updated
in place per commit with bounded-lag durability (watermark + runIndexing
replay). Per-slot seqlocks, relaxed cross-slot adherence (safe under the
existing exact-rescore + MVCC record load), in-file freelist (fixes #2182
structurally), bitset + pipelined-TSFN filtering, three-phase rollout
(dual-write -> file-primary -> native insert).

Prototype (native/hnsw-plane): compilable standalone core — format,
seqlock, asymmetric int8 cosine, beam search with epoch-stamped visited
array, prototype insert, bench binary. First measurement (20K x 768-d,
ef 512, Linux): 0.440 us/visit vs 4.34 us JS baseline — 9.9x per-visit,
scalar distance, before SIMD and zero-copy reads.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sert

Lever 1: distance_to/neighbors_into read directly against the mmap under
the seqlock (no per-visit Vec allocation); full-copy read_node remains for
construction paths only.

Lever 2: explicit AVX2+FMA f32xi8 asymmetric kernel and AVX2 i8xi8
symmetric kernel (construction-time neighbor distances, recomputed since
the format drops stored per-edge distances), runtime-detected with scalar
fallback. Linux x86_64 is the performance target per design decision;
Windows may fall back to JS entirely.

Lever 3: insert ported to JS optimizeRouting parity - rank-ordered
candidate selection with indirect-route skipping and edge replacement,
per-level searchLayer construction, reverse edges with prune-to-cap-64.
Fixed a port bug where the neighbor scan broke on the first added-
connection match (JS breaks only the inner scan).

Bench: Gaussian-mixture corpus matching hnsw-scale.js calibration
(uniform-random 768-d is un-indexable per that benchmark's notes) +
brute-force recall@10. At 100K/ef512: p50 0.28ms, 0.201 us/visit
(JS 4.34), recall 1.000, build 5,583 inserts/s. Design doc updated with
cap-64, platform, and packaging decisions.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Prototype persistence for the hierarchy (slots store only the level, so
upper edges were lost on reopen and reused planes searched layer-0-only).
Atomic tmp+rename write on build completion; missing sidecar degrades to
layer-0 search. The production design remains the in-file append region.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1M cap sweep: cap-64 loses 2.2pts recall (0.975 vs 0.996; JS anchor
0.997) at equal ef and latency, and cap-128 costs +23.5% file bytes, not
2x - the 768B vector dominates the int8 slot. Binary-code v2 slots reopen
the question (+73% there). Measurement table updated: at the 1M anchor
with cap 128 the native plane is 9.6x p50 / 12.9x per-visit / 4.7x build
at JS-equal recall.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…urrency fixes

- napi feature (napi-rs v2): Plane class - create/open, insert/remove,
  async search on the libuv pool (AsyncTask, pooled scratches), searchSync,
  watermark get/set, flush (msync + upper sidecar). Harper-agnostic surface;
  pk<->id mapping and commit glue stay in the host. Build the NAPI artifact
  with --features napi --lib (the bench bin cannot link node-api symbols).
- ACORN-style bitset filter: filtered-out nodes route but are excluded from
  results; visit budget = ef * filterExpansion bounds selective filters.
- Page-grouped slot addressing when per-page waste <= 128 B (cap-128 slots:
  3/page, 64 B waste, no page straddling); packed otherwise; header-pinned.
- Concurrency fixes found by the torture test: edge RMW races (two-step
  read-then-write lost edges; now atomic via update_neighbors under the
  slot seqlock) and visited-array OOB when concurrent inserts mint ids
  past the query-start snapshot (writers panicked, wedging the run).
- tests/concurrent.rs: 4 writers x 2000 inserts against 4 readers, then
  self-query/cap/freelist-reuse verification. Passes in 0.17s.
- smoke.mjs: end-to-end through Node - PASSED on the fleet box.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
search_predicated: candidate ids batch (64/batch) to an external evaluator
while traversal keeps expanding in distance order - the search thread never
blocks on the JS event loop until the beam is done; verdicts merge in as
they arrive and gate result admission only. Visit budget ef*filterExpansion
bounds speculative overshoot; a 5s drain deadline treats missing verdicts
(predicate error, env teardown) as deny. Core is napi-free (channel-based
PredicatePipe) with a pure-Rust mock-evaluator test; the NAPI layer wires a
ThreadsafeFunction (searchWithPredicate: predicate(ids) => Uint8Array).
Smoke through Node: 18 predicate batches for one ef-128 query, no leaks.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The hierarchy moves from an in-memory map + sidecar into a fixed-entry
region of the plane file itself: per-entry seqlocks, 8-level x 32-id
entries reserved for 1/8 of max_nodes (2x the expected 1/M upper-node
rate; exhaustion degrades to missing upper links, never an error). Slots
gain S_UPPER_IDX. Removes the global RwLock every query's greedy descent
contended on, the sidecar files, and the reopen hierarchy gap. Upper
entries leak on delete (bounded by the reserve; freelist TODO). Format
VERSION bumped to 2 - v1 files reindex.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
writeNodeRaw(id, level, int8 bin, scale, invMag, layer0 ids, upper id
arrays): full node state per call with HOST-allocated ids - high-water is
raised via CAS-max, the plane allocator/freelist is bypassed, and an
existing upper entry is rewritten in place so repeated updates to a
high-level node do not leak entries. clearNode marks deleted without a
freelist push (the host owns id allocation). setEntryPoint/getEntryPoint
mirror the host's entry updates. This is the seam the Harper phase-1
integration writes through: JS keeps computing the graph, the plane
mirrors it bit-identically, rollback = flip the search flag.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Upper freelist: delete_node frees the entry (tagged CAS stack, next
pointer in the dead entry's first list bytes); write_node_raw reuses a
cleared node's entry via a flags-agnostic idx read, closing the
clearNode-then-rewrite leak in dual-write mode.

Coverage pruning: the concurrent torture test, run in a loop, exposed
orphaned nodes (~1-in-4 runs had unfindable self-queries) - closest-keep
eviction on reverse-edge overflow can strip a node's last in-edge in
dense near-duplicate clusters. Overflow eviction now prefers the most
REDUNDANT far member (some kept nearer k has d(e,k) < d(base,e), so
searches reaching k still reach e), bounded to farthest-16 x nearest-16
(~30us per overflow); falls back to plain farthest. 25/25 torture loops
green after the change.

bench: optional threads arg adds a concurrent-throughput pass (T
searchers + background writer -> aggregate QPS, per-thread p50/p99).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… QPS w/ writer)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kriszyp and others added 19 commits September 1, 2026 16:05
…tries

Two defects in `write_node_raw`, both ending with the fixed upper region
draining, `write_upper` returning NO_UPPER, hierarchy edges silently not
binding, and plane recall collapsing while the CF graph stays correct.

`upper_idx_raw` read the slot's bound index through `read_consistent`, whose
fallback is NO_UPPER. A peer worker holding that slot's lock past the 20 ms
stale window therefore made the read report "nothing bound" for a node that had
an entry, and a duplicate was minted — every contended mirror of a level>=1
node orphaned its predecessor, and on macOS `tag_is_dead` is always false, so
that path is the only one available there. The read is now `upper_idx_locked`,
taken under the slot write lock: it waits rather than guessing, and adds no
failure mode `write_node` did not already have a line later.

A node re-mirrored with no upper levels kept its old entry readable, on the
reasoning that level never shrinks. It does: the shared id counter reseeds to
largestNodeId + 1 on restart, so deleting the top ids hands them back out and
the new record redraws its level, often 0. That entry is now emptied in place.

Emptied, not freed: returning it to the shared freelist is not atomic with
publishing the slot, so a concurrent mirror that read the index first could
republish a slot pointing at an entry already handed to another node — trading
a bounded retention for cross-node corruption. Keeping it bound costs at most
one idle entry per id, which is the retention hnsw-native-plane.md §10 already
accepts.

Both are covered by tests that fail on the parent commit. The dead-owner
takeover sanitizer discards a bound index the same way and is deliberately left
alone; see the PR body.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Nothing points at an entry from `write_upper` until `write_node` publishes the
slot, so a wedged publish stranded it outside both the freelist and the graph.
`write_node_if_untouched` already frees on its own failed path; this matches it.

Also trims `upper_idx_locked`'s doc comment to the invariant.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
…edges

`write_node_if_untouched` allocates before taking the slot lock, and the `?` on
that lock returned without freeing — the same leak fixed one commit ago in
`write_node_raw`, in the function that was cited as the model for getting it
right. Nothing references a fresh entry until the write lands, so every path
that does not publish it has to free it.

Covered by a test that fails on the parent commit; it needs the 5 s writer
wedge bound to elapse, so it runs alongside the rest rather than adding to the
suite's wall clock. `write_node_raw`'s equivalent window survives only between
`upper_idx_locked` releasing and `write_node` re-acquiring and cannot be driven
deterministically — a holder that takes the lock first now wedges the read
before anything is allocated — so its guard stays defensive and untested rather
than pinned by a test that would pass on either side.

`upper_high_water` gains the accessor `id_high_water` already had, which is what
lets the test tell a reclaimed entry from a leaked one.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
…ults

The wedge and contention tests spun unbounded on the holder thread's signal, so
a holder that panicked before signalling hung the run instead of failing it,
and the wedge test discarded every write result — it could report success
without the wedge or the reuse ever happening.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Both lock-contention holders published their "held" flag on the result of
write_lock without checking it succeeded, so a wedged acquisition let the tests
run uncontended and pass.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
…y of 97bb5c9)

36cd9be29 added it; the next commit's format.rs edits came from a
worktree state that predated it and silently clobbered the function and
both call sites. Same rationale as before: densely packed hosts live in
permanent memory pressure; readahead on random re-faults taxes every
tenant with no sequential reader to protect.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
First-entry race: claim_entry_if_empty is a strict CAS from the empty
encoding, so exactly one racer roots the graph and every loser joins it
instead of returning an unlinked node. Both self-promotion sites go
through it; a loser reuses the upper entry its slot already names.

Concurrent reads: pad the vector so neighbor arrays are 4-aligned (and
move the upper-list pad ahead of the ids), then read every field a reader
acts on with an aligned read_volatile. The stored vector stays an
ordinary load so the dot product keeps vectorizing. VERSION 6.

Dead entry points: delete_node re-elects before the fallible upper
cleanup, and searches repair an entry no writer will through the O(1)
previous-entry hint, which promotions now record.

Async iterator: one memoized iterator per iterate() call plus a closed
flag, and a handler on the pending pipeline so an abandoned iterable
cannot raise unhandledRejection.

Stale planes: an undeletable plane is invalidated in band (watermark 0
under a durability barrier) before the .stale sidecar, the flag-off
cleanup path marks instead of only logging, and the sidecar's own
cleanup no longer permanently disables a plane whose file an operator
already removed.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
- delete_node re-elects while the node is still readable, so no window
  publishes a tombstoned entry point, and re-election never elects the
  node it is replacing.
- a re-election that finds no candidate clears the entry only while it
  still names the node being replaced, instead of erasing an entry a
  concurrent insert installed.
- an insert that cannot resolve an entry point within its retry bound
  returns an error rather than Ok for a node nothing points at.
- the previous-entry hint records only a live displaced node, so a
  host-mirrored post-delete re-election cannot evict a usable hint.
- the undeletable-plane invalidation stores the watermark inline and
  msyncs on the pool, off the event loop.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Invalidation ordering: PlaneFile::invalidate zeroes the watermark and
msyncs the header page alone, and the host calls it synchronously before
creating the .stale sidecar. The sidecar is an empty file whose directory
entry is never fsynced, so queuing an async whole-map flush and writing
the sidecar first let a power loss keep the old nonzero watermark and lose
the only marker — the next process then adopted a plane missing every
mutation made while mirroring was off. Skipping the data flush is sound
because the data is being discarded and lowering the watermark is the safe
direction; it is also what makes a synchronous barrier affordable on a
multi-GB plane.

Entry repair: the previous-entry hint is one slot and can itself be dead
(promote over a node, then lose both), which left every later search
returning empty. A bounded probe of the dense low id range now backs it
up, capped so a read never pays the write path's O(high-water) scan. The
repair also publishes through replace_entry_if — strict on the entry it
observed dead — because a not-worse install would displace a live level-0
root a concurrent first insert had just claimed, orphaning it.

The hint's own liveness check reads FLAG_VALID volatile like every other
field a concurrent writer mutates; it sits outside the slot seqlock, so a
retry cannot catch a tear. §10 now records the atomic-slot-payload debt
the volatile reads bound but do not discharge, which graph.rs cites.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011W6mChyPAUAoKEfxV1bSAo
The repair's last-resort probe scanned a fixed low prefix on the claim
that ids are dense from 0. They are not: Harper allocates node ids
monotonically through Atomics.add and never reuses them, so a table that
has churned has its entire low prefix tombstoned and only its newest ids
live — the probe would find nothing there and every search would return
empty forever, which is the failure the probe exists to prevent. It now
walks down from the newest id with a stride spanning the whole range, so
it assumes nothing about where the live nodes are: the crate's own
freelist does reuse ids and keeps live nodes low, and striding covers
both. Same probe budget.

probe_for_entry had also been inserted between reelect_entry_point_-
replacing's doc comment and its body, so it carried documentation for
parameters it does not have.

replace_entry_if's comment claimed more than the code: it compares the id,
not the incarnation, so under freelist reuse it can match a different node
in the same slot. That is a routing-quality window, not a lost node — the
edgeless claimer it names is structurally excluded, since
claim_entry_if_empty fires only from NO_ID.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011W6mChyPAUAoKEfxV1bSAo
A fixed start walked hw-1, hw-1-stride, ... forever, which is one residue
class of the stride. A live graph lying entirely between those samples was
not merely missed once — it was invisible to every later repair too, which
is the silent-empty-results mode the probe exists to prevent, reached by a
different route than the low-prefix assumption the previous commit fixed.

The start now rotates per call, so stride consecutive repairs cover every
id while each stays capped at the same probe budget.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011W6mChyPAUAoKEfxV1bSAo
The rotation counter was process-global, so every other plane's repairs
advanced it too: two planes repairing in turn each see offsets stepping by
two, which pins each to one residue class indefinitely — the coverage the
rotation was added to provide. It lives on the Graph handle now, so a
plane's own consecutive repairs are what rotate it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011W6mChyPAUAoKEfxV1bSAo
Floor division leaves stride * limit < hw whenever hw is not a multiple of
limit, so every rotated probe stops above the lowest hw % limit ids. That is
permanent, not per-search: no rotation offset reaches them, so a graph whose
entry and hint die while its survivors sit in that prefix returns empty from
every later search.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
… later

The tail drain looped `while let Ok(..) = recv_timeout(50ms)`, so after the
final verdict dropped `outstanding` to zero it waited out one more full
timeout. Every filtered query paid 50 ms on the pool thread — 25-50x the
search itself — and on the shared libuv pool that queues unrelated I/O too.
Guarding the drain on `outstanding` also makes a stray verdict a break
rather than a usize underflow.

Also trims the ceiling-stride comment to the invariant it rests on.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
A single wall-clock sample calls any 25 ms scheduler stall an extra receive.
The defect it guards adds the full timeout to every query, so the minimum
over a handful separates them: noise cannot hold all of them above the bound.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
None of these were open review threads — all eight were closed and the last
two bot rounds on this head were clean. They are findings earlier rounds
recorded in the PR body as open-but-not-taken, each a defect with one right
answer rather than a design question.

add_reverse_edge's contended fallback dropped the edge it was adding: a push
followed by truncate(cap) discards the tail, and at exactly cap the tail is
the new id, so the reverse edge that keeps a newly inserted node reachable
from that neighbor was lost precisely in the contended-and-full case the
fallback exists to serve. It now evicts the farthest neighbor instead (lists
are written in ascending distance).

A refused predicate enqueue was counted outstanding. call_with_return_value
drops its callback when the queue is closing or full, so no verdict can
arrive, and the tail drain then waited out its whole 5 s deadline — on every
in-flight filtered query during teardown. PredicatePipe::dispatch now reports
whether the batch was handed off.

A per-query distance override bypassed the plane cutover: a euclidean query
against a cosine index was traversed cosine-first, and rescoreResults only
corrects the reported distances of the candidates it is handed, not which
candidates the beam kept. Such a query now takes the JS path, matching the
adjacent dimension-mismatch precedent.

Regressions: a_contended_merge_into_a_full_neighbor_list_keeps_the_edge_it_adds,
a_refused_predicate_enqueue_does_not_hold_the_drain (5.02 s on the parent
commit, sub-second here), and a JS parity test asserting the overridden metric
never reaches the plane. Each checked to fail with its own fix reverted.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
…hest

The pre-push review's one finding on the delta: the merge's premise was
wrong. Neighbor lists are distance-ordered only right after a prune —
non-overflow reverse-edge appends push at the tail — so the displaced
neighbor is arbitrary, not the farthest. The behavior stands: an arbitrary
existing edge is the right thing to give up over the edge being added, whose
loss is systematic and costs a freshly inserted node its in-edge, and any
better victim needs distances this path keeps outside the lock.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
The crate's remaining build warning, and the reason it is worth silencing
rather than allowing: a benchmark that discards insert errors reports build
throughput for rows it never indexed, and then measures recall against a
corpus the graph does not contain. The concurrent-writer loop two hundred
lines down already breaks on the same error.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
@kriszyp
kriszyp force-pushed the kris/hnsw-native-plane branch from 8dd6c16 to a55adf0 Compare September 1, 2026 22:46
@kriszyp
kriszyp marked this pull request as draft September 1, 2026 22:46
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
@cb1kenobi

Copy link
Copy Markdown
Member

Reviewed a0f735d and found no blocking issues. No new blocking issues were confirmed at this commit. Existing findings were not repeated, and the new commits are rebase and formatting changes.


Generated by Barber AI

kriszyp added a commit to HarperFast/hnsw that referenced this pull request Sep 2, 2026
…ch) and the harper#2430 native delta (#1)

* Port the reviewed native delta from HarperFast/harper#2430 (7661fd1b6..a0f735d54)

Applies the native/hnsw-plane subtree diff between the commit this crate already
matched (7661fd1b6) and the PR head (a0f735d54) verbatim: first-insert claim/join,
repair probe with per-plane rotation, predicate-drain fixes, format v6 (4-aligned
neighbor and upper id arrays, volatile field reads), Plane.invalidate(), and the
accompanying tests. The design doc receives the same two hunks (slot pad, atomic
slot payloads) so its section numbers still match the citations in graph.rs.

Also corrects two stale open-time scrub claims in format.rs: open() performs no
scrub, so opened_clean is advisory and the orphaned "force persisted-odd seqlocks
back to even" doc that had attached itself to slot_ptr is gone.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017edcNKY5AgYYJmyNfxmUaK

* Package-owned plane invalidation: one-way latch, refusing open, fsync'd .stale sidecar

A plane the host cannot delete (Windows sharing violation while another process maps it)
must never be adopted later at its nonzero watermark. Harper's helper did this in JS with
three defects: the .stale sidecar was never fsynced, a temporary handle was released by the
garbage collector (on Windows that mapping is itself why the unlink failed), and both steps
swallowed their errors.

invalidatePlane(path) / plane.invalidateFile() now own it: the in-band mark is a sticky
header byte (format v7) under which watermark() reads 0 on every handle — a flushAsync
already in flight can still stamp the word but cannot revive the plane — plus the sidecar,
created with create-new semantics (a planted symlink is never followed), fsync'd with its
directory entry on POSIX. Both markers are attempted every call; the temporary handle is
dropped before the sidecar step; the call throws only when neither marker is durable,
leaving the file exactly as found. open() refuses a file carrying either marker and
create() refuses a path with a leftover sidecar, so the markers are enforced by the
package rather than by each host's attach path.

Also: CI matrix gains windows-latest (the cfg'd directory-fsync path), package.json and its
platform pins move to 0.2.0, and Cargo.lock records the libc dependency it was missing.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017edcNKY5AgYYJmyNfxmUaK

* Review round 1: equal-level re-election guard, probe futility latch, sidecar TOCTOU + no-follow reopen

- cas_entry_if_not_better aborts on an equal-level entry installed meanwhile (>=, not >):
  a stale re-election could otherwise CAS over a fresh claim_entry_if_empty winner with no
  in-edges, orphaning a node whose insert already reported success. Regression test.
- insert's bounded entry-resolution loop deletes the edgeless node it published when it
  falls out with Err(Wedged), instead of leaving a live-reading, edgeless slot for the
  repair probe or a re-election to root the graph at.
- probe_for_entry stops after `stride` consecutive empty rotations at one high-water and
  re-arms on any node write through the handle: a fully dead graph no longer pays 1024
  node reads per search forever. Unit test covers the stop and the re-arm.
- open() re-checks the sidecar after mapping and create() re-checks it before returning,
  closing the pre-map TOCTOU; an existing sidecar is re-synced through a no-follow,
  non-blocking open validated on the handle, so a marker swapped for a symlink or FIFO is
  refused rather than followed.
- smoke.mjs no longer unlinks a plane two handles still map (a Windows sharing violation).
- Contract text: a double failure deletes nothing; the in-band mark may still have landed
  in the shared mapping when its msync failed, which is the safe direction.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017edcNKY5AgYYJmyNfxmUaK

* Review round 2: probe latch keyed on a shared write epoch; a create that raced an invalidation is latched

The probe futility latch was re-armed only by writes through the same handle, so another
process reviving a fully dead graph without an entry-point update stayed invisible to this
handle until its high-water changed — a regression against always probing. The header now
carries a write epoch (v7 field, offset 96) bumped by every node write through any handle;
the latch is keyed on (high-water, epoch) and any write anywhere re-arms the probe. The unit
test revives through a second handle on the same file.

create() finding a sidecar that landed during the create now latches the finished header
before returning Err, so a lost or removed sidecar cannot turn that failed create into an
adoptable empty plane.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017edcNKY5AgYYJmyNfxmUaK

* Review round 3: epoch bumped after publish, fail-closed sidecar stat, single-flight probe, ordering test

- The write epoch is bumped after the seqlock release that publishes the slot (Release on
  the bump, Acquire on the probe's load): a probe that consumed the bump while the slot was
  still invalid could otherwise latch a plane that holds a live node.
- stale_sidecar_present treats any stat failure other than NotFound as "present": a
  durability marker must fail closed, not vanish on a transient EIO/EACCES.
- One repair probe at a time per handle; concurrent searches return empty for that call
  rather than each paying the full walk before one publishes.
- invalidate_at gained a sidecar-writer seam so a test proves the in-band mark is on the
  file before the sidecar step runs, through a temporary handle and an attached one.
- create's post-check comment states its best-effort scope.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017edcNKY5AgYYJmyNfxmUaK

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
kriszyp and others added 3 commits September 4, 2026 11:17
Co-Authored-By: OpenAI Codex <noreply@openai.com>
… in-repo crate

The Rust crate now lives in HarperFast/hnsw and ships as an Apache-2.0 npm package
with platform prebuilds, so `native/hnsw-plane/` and its `build:hnsw-plane` script
are removed and the adapter requires the exact-pinned optional dependency.

0.2.1 also moves plane invalidation into the package: `invalidateFile()` /
`invalidatePlane()` set the one-way header latch, zero the watermark, and write the
fsync'd `.stale` sidecar in that order, and `open()` now refuses a plane carrying
either marker. The ordering invariant and its power-loss rationale move with them,
so the adapter keeps only availability, fallback, and integration policy — including
a sidecar-only path for a plane that outlives the package that made it.

CI runs the published prebuild, so the job proves that prebuild loads (a failed load
would leave the suite self-skipping and green) and no longer re-runs the crate's own
tests against a pinned source checkout, which HarperFast/hnsw's CI already covers on
three platforms and which would grade something other than the binary under test.

The phase-2 plan in hnsw-native-plane.md now targets #2489's shared post-commit
`DerivedIndexBackend` delivery rather than an HNSW-specific commit callback.

Refs #2489

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ced JSDoc

The hnsw-plane job's `npm run build || true` was copied from the jobs that tolerate
type errors, but this one runs mocha against dist/ — a tolerated build failure grades
a stale dist and still reports green, which is the same hole the load probe closes on
the package side. It builds clean on this branch, so the job builds strictly.

The createAndMirrorPlane doc block sat above planeLayer0Cap, describing the wrong
method.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@socket-security

Copy link
Copy Markdown

Warning

Review the following alerts detected in dependencies.

According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.

Action Severity Alert  (click "▶" to expand/collapse)
Warn High
Publisher changed: npm @harperfast/hnsw is now published by harperdb_team

Author: harperdb_team

From: package-lock.jsonnpm/@harperfast/hnsw@0.2.1

ℹ Read more on: This package | This alert | What is unstable ownership?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: Try to reduce the number of authors you depend on to reduce the risk to malicious actors gaining access to your supply chain. Packages should remove inactive collaborators with publishing rights from packages on npm.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/@harperfast/hnsw@0.2.1. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

Warn Medium
Low adoption: npm @harperfast/hnsw

Location: Package overview

From: package-lock.jsonnpm/@harperfast/hnsw@0.2.1

ℹ Read more on: This package | This alert | What are unpopular packages?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: Unpopular packages may have less maintenance and contain other problems.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/@harperfast/hnsw@0.2.1. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HNSW: graph-size resolution is a lifetime id high-water mark — churn-heavy tables permanently over-provision build/search ef

2 participants