Native HNSW index: file-primary mmap graph as a backend on the shared derived-index runtime - #2430
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed the full diff at head |
cb1kenobi
left a comment
cb1kenobi
left a comment
There was a problem hiding this comment.
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
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>
| 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')); |
There was a problem hiding this comment.
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.
| Ok(plane) | ||
| } | ||
|
|
||
| /// Force any persisted-odd seqlocks (slot + upper regions) back to even after an unclean |
There was a problem hiding this comment.
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.
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>
8dd6c16 to
a55adf0
Compare
|
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. — |
…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>
|
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.
|
kriszyp
changed the title
e01f2bb to
dd3bb39
Compare
dd3bb39 to
1615a53
Compare
kriszyp
changed the title
1615a53 to
65f9902
Compare
4cad607 to
b2f0e49
Compare
`resources/DerivedIndexBackend.ts`, this branch's own delivery runtime, is deleted. The file-primary HNSW index is now a `DerivedIndexBackend` on the shared runtime from #2548: - `resources/indexes/hnswDerivedIndex.ts` adds `HnswDerivedIndexBackend` — `deliver()` queues, an applier drains in 5 ms slices, the plane's `msync` is the barrier, and the one cursor vector is published after the pending mappings under `Symbol.for('derived-index-cursor')`; `reset(epoch)` removes the cursor before the file and the mappings — and `attachDerivedIndexes`, which registers a table's post-commit indexes with one runtime per database on every worker. - The commit path only validates (a malformed vector is still the client's 400); nothing is staged on the transaction and the `aftercommit` targets argument is gone. Writer backpressure is the runtime's lag policy (`maxLagMilliseconds`, 30 s default on a `nativePlane` attribute). - Search readiness is the runtime's shared record on every worker, not the per-worker `isIndexing`; a failed search detaches and asks the owner for a rebuild instead of unlinking a file a peer may already have replaced. - `vectorIndexPlane.test.js` is ported: 22 passing against the real native package, including the two-worker, worker-death, reload-marker, retention and lost-file cases. The ingest bench's repeated-key shape now measures 265 native applies for 1,000 commits over 50 keys. `hnsw-native-plane.md` §8, §10, §11 and §13 record the outcome; six of the §10 open items are closed by the shared runtime. Refs #2489 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
65f9902 to
5ddefc9
Compare
| 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')); |
There was a problem hiding this comment.
Still open from the previous round: this recovery path's tombstone write has neither the in-band invalidate step nor the fsync durability that the two sibling call sites in this PR already got fixed with (cleanupDisabledPlane/resetDerivedStorage, both via invalidatePlaneFile). If a crash lands between this write and its own durability, the sidecar is lost while the plane keeps its old nonzero watermark — a same-name table recreated afterward can adopt it as ready and silently serve results missing everything before the interrupted drop.
This file already imports planeFilePathFor/planeStalePathFor from ./indexes/hnswPlaneBinding.ts; that module also exports invalidatePlaneFile(filePath), which performs the durable zero-watermark-then-fsynced-sidecar sequence in one call. Swapping the manual closeSync(openSync(...)) here for invalidatePlaneFile(planeFilePathFor(rootStore.path, columnName)) would close this gap with no new code.
| "cpu": [ | ||
| "arm64" | ||
| ], | ||
| "libc": [ |
There was a problem hiding this comment.
The libc discriminator was silently dropped from this and 11 other pre-existing optional packages in this lockfile update: @harperfast/rocksdb-js-linux-{arm64,x64}-{glibc,musl} (4 packages) and @oxlint/binding-linux-arm64-{gnu,musl}, -ppc64-gnu, -riscv64-{gnu,musl}, -s390x-gnu, -x64-{gnu,musl} (8 packages). version/resolved/integrity are byte-identical to base for all of them, and neither package.json nor lockfileVersion changed — so this isn't an intentional bump, just a lockfile-regen artifact (a different npm client version rewriting these blocks).
This is the same regression pattern as harper#1992: without the libc discriminator npm can't tell glibc-only from musl-only binaries apart on install, so both variants can end up installed instead of just the one matching the runtime, inflating install footprint. The new @harperfast/hnsw-linux-*-glibc entries this PR adds don't carry libc either, so any regeneration fix should cover those too.
Suggested fix: regenerate the lockfile with the npm version this repo standardizes on (or manually restore the libc fields) so only the correct optional binaries get installed.
Every Unit Test run on main since #2430 merged aborts on Node 22 (`Aborted (core dumped)`, exit 134) right after the plane suite terminates a worker thread and the next test calls `resetDatabases()`. Node 24 and 26 pass. Root cause is in rocksdb-js, reproducible with no Harper code: a worker calls `tryLock(key, callback)` while another thread holds the lock, the worker is `terminate()`d, and the holder calls `unlock()`. `DBDescriptor::lockReleaseByKey` then calls the queued callback's thread-safe function for an env that no longer exists; on Node 22 that aborts the process, on Node 24 it returns `napi_closing` and is skipped. Lock callbacks have no per-env cleanup (the park-timeout registry does: `releaseByEnv`). The shared runtime made this deterministic because every non-owner worker keeps a standing lock waiter, and `manageThreads.js` terminates workers on restart, so this is a production path on Node 22 — including for `recordLock.ts`, which uses the same primitive. The runner now takes the lock with no callback. Successors are woken by the releasing owner's `notify()` on the readiness buffer (whose listeners survive a dead env; proven by the same repro), by commit wakes, and by a 5 s retry timer that covers an owner that died without releasing. The releasing runner ignores the one notification it caused so an idle release does not re-acquire itself. A lock attempt that throws still backs off and ignores commit wakes meanwhile. The Stage 1 fake store now models `notify()` the way the native binding does; the test for a synchronous unlock callback goes with the callback. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 participants
A file-primary native HNSW vector index, as a backend on the shared derived-index runtime from #2567. The memory-mapped
.hnswfile (@harperfast/hnsw0.2.1, exact-pinned optional dependency) is the only home of graph nodes, adjacency, the entry point and the id allocator; RocksDB keeps the primary records, thepk ↔ nodeIdmappings and one durable cursor vector. This PR is stacked on #2567 and shows only the HNSW part; it is that runtime's first consumer.The durable design — file format, concurrency, durability, search path, how the index sits on the runtime, what
nativePlane: truerequires and does not promise — is the § Native HNSW plane section of the rootDESIGN.md. This description holds the transient part: decisions, measurements, verification.Why
Measured at 5M nodes / ef 512, ~85% of a JS search visit is object bookkeeping and a RocksDB
Getper visit (~1–2 µs warm) dwarfs the ~50 ns SIMD distance it feeds:What changed in this revision
The branch's own delivery runtime (
resources/DerivedIndexBackend.ts, 486 lines) is deleted. In its place:resources/indexes/hnswDerivedIndex.ts—HnswDerivedIndexBackend:deliver()queues and returns accepted, an applier drains in 5 mssetImmediateslices,plane.flushAsync()is the barrier, then the pending mappings are published, then the batch'sthroughvector is written as the one durable cursor (Symbol.for('derived-index-cursor')). Application pauses while a barrier is in flight so the barrier publishes exactly what it covers.reset(epoch)removes the cursor before the file and the mappings.attachDerivedIndexes(Table)registers a table's post-commit indexes with one runtime per database, on every worker.HierarchicalNavigableSmallWorld.ts— the commit path only validates (a malformed vector is still the client's 400); nothing is staged on the transaction. Search readiness is the runtime's shared record on every worker (planeSearchReady), notisIndexing; a failed search detaches and asks the owner for a rebuild instead of unlinking a file a peer may already have replaced. A vector the plane cannot hold at apply time is skipped and counted, never a rebuild.maxLagMilliseconds(30 s default on anativePlaneattribute, settable) → retryable 503DERIVED_INDEX_LAGGINGon local user writes to the table; canonical-source applies, replay and replication notifications are never shed.RocksTransactionLogStore.aftercommitno longer carries staged targets.Six open items from the earlier design record are closed by the shared runtime: shared cross-worker readiness, a new origin's first write forcing a rebuild, per-key coalescing (the ingest bench's repeated-key shape now measures 265 native applies for 1,000 commits over 50 keys; it was 1,000), undecodable audit headers, and the whole-retention-window replay after a rebuild. An interior corrupt frame inside the committed prefix still fails the attempt closed — a log that cannot be read to its tail cannot be replayed from any anchor.
What
nativePlane: truerequiresUnchanged: explicit
audit: trueon the table (the transaction log is the recovery source; inheriting the global setting is rejected), RocksDB,M=16/efConstruction=200/ int8 cosine,@harperfast/hnswloadable (absence = 503, not degraded). Not promised: a single total order across concurrent CRDT/source-resolution arrivals, byte-identical graphs across nodes or rebuilds, in-place format upgrades.Decisions
DerivedIndexBackend.tsimplemented the same protocol; the measured backend share of the write-and-index wall (95–97%) put the per-transaction bookkeeping the two runtimes differed on inside the remaining 3–5%, so it could not justify keeping both.commitFinished()advanceslastCommittedPositionto the earliest still-uncommitted write, so the committed tail is already a contiguous-prefix boundary and needs no storage-layer change.DESIGN.md.maxLagMillisecondssheds the same writes on time behind, which is the quantity retention actually bounds.Measurements
Per-visit cost at 5M nodes / ef 512, 768-d int8: 4.34 µs per warm JS visit, of which the int8 cosine is 0.43 µs and a cold msgpackr node decode 5.57 µs; native per-visit 0.33 µs. 1M × 768-d: search p50 7.2 → 0.75 ms, recall@10 0.997 → 0.999. Package in isolation (N = 10,000): insert 208 µs (128-d) / 315 µs (384-d) / 835 µs (1536-d). Ingest bench on this branch (384-d, graph ≈3,000): foreground put 0.090 ms (11,078/s),
applyDerivedValue0.365 ms/call,flushDerived8.96 ms/call, event loop max 11.8 ms while draining; repeated-key shape 265 applies for 1,000 commits over 50 keys. Rebuild insertion floor: 4,684/s at 100k against a 1,000/s CI gate; 1,242/s at 1M from the crate anchor.Verification
unitTests/resources/vectorIndexPlane.test.js— 22 passing against the real native package: mappings-only CF; deterministic recall; aborted transaction publishes nothing; unrelated field change schedules no insert; predicates and full-stack rescoring; wrong-dimension write/query are 400s; f32-range rejection; async result contract; two workers into one native file; a committed write replayed after its worker dies before delivery; whole-table reload marker rebuilds (live and on a replacement worker); recovery acrossresetDatabases; a cursor outside retention rebuilds from primary records; an empty index answers no results; a populated index that lost its file answers 503; drop removes the file.test:unit:resourceson this branch: 2480 passing, 32 pending, 0 failing.hnswDerivedIngest.bench.jsruns; thehnsw-planeCI job still requires the native binding to load rather than accepting a skip.npm run build,tsc --noEmit,lint:required, prettier: clean.Restacking exposed one runtime defect, fixed in #2567: a log that has never written a file reports
oldestSequenceNumber: 0, which the runtime read as a retention gap on every brand-new database.Refs #2489, #693, #711, #895, #2182
🤖 Generated with Claude Code