HNSW Index: Link and unlink neighbours

[villagedeb]

Implement bidirectional neighbour linking and unlinking using incoming link flags, including overflow handling.

Document the lock hierarchy and link protocol, and add the LevelStore operation mutex. Remove UnlinkOrphans; orphaned slots are now reported by incoming_neighbours().

Add scratch buffers for link walks and update MockGraph and tests.

[tomas-villagesql]

I've tested the entire PR stack end to end. It produces reasonable recall numbers. Furthermore I've done 2 different Claude reviews, one with context and one standalone without context. First verdict, approve as is. Second approve as is. Detailed comments from the latter:

VillageSQL Self-Review — PR #48 "HNSW Index: Link and unlink neighbours"

Base: deb/index/16 (stacked PR, single commit 8729a59, clean merge-base). Reviewed the PR diff, not the local worktree (worktree HEAD ba083e3 is an unrelated branch with later graph.cc changes that are not part of this PR — I extracted the PR's graph.cc from origin/deb/index/17 to review accurately).

Deletion audit — clean

All removals are the intended flip side of the change: the UnlinkOrphans enum and the two-mode unlink_neighbours(..., UnlinkOrphans, ...) signature are replaced by a single-pass unlink_neighbours + incoming_neighbours(), exactly as the PR describes. The removed using UnlinkOrphans in graph_ops_impl.h, the mock's enum, and the unlink_no:/unlink_yes: call-log strings all follow. The removed graph_ops_impl.h:NN-NN line-reference comments in the tests are a genuine improvement (see below). No upstream logic, bounds check, or invariant was silently dropped.

Correctness — verified, no bugs found

I traced the load-bearing invariants:

• update() "slots strictly increasing" contract holds on all four m_link_slots-carrying updates (demote, primary-free, overflow-compact, overflow-free). Compaction always preserves ascending order and the write index never overtakes the read index.
• Scratch-buffer aliasing: unlink_neighbours iterates m_node_buf_1/m_incoming_buf_1 while unlink_neighbour uses m_node_buf_2 and find_overflow_link uses m_incoming_buf_2 — no buffer is clobbered mid-walk. overflow_capacity() = min(MAX_OVERFLOW_LEN, M) ≤ M ≤ 2M = mmax0, so every capacity <= m__buf.size() assert holds.
• Flag-clear on withheld (overflowed) links in link_neighbours_locked is correct: node's outgoing edge to the neighbour is real regardless of whether the reciprocal landed directly or in the neighbour's overflow chain.
• MtrCtx destructor commits (per the existing graph.cc:371 comment); every new early-return true after a fetch has no prior write in that mtr to lose, and write paths commit explicitly before returning.

Findings (all minor)

1. graph.cc:963,1036 — assert-gated invariants that a release build drops (nit / consider)
   The assert(!new_neighbours.empty()) added in graph_ops_impl.h:321 and the several assert(capacity <= m_ctx.m_incoming_buf_1.size()) guards are the only thing protecting later unchecked indexing/spans. These are legitimate internal invariants and match the file's existing heavy assert usage, so this is not a defect — just flagging that with -DNDEBUG they vanish. No action needed unless you want a hard guard on the buffer-capacity ones; consistent with existing style to leave as-is.

2. graph_ops_impl.h test comments — good altitude fix, verify one claim
   The tests replaced brittle graph_ops_impl.h:77-80 line-number references with concept-level descriptions ("per the invariant insert() documents around its create_node() call"). This is exactly the right direction per the comment-altitude rule. One to double-check: the rewritten comment at test_insert_upgrade_recheck… asserts "insert() deliberately keeps the X lock rather than downgrading in that case" — confirm that phrasing still matches what insert() actually does now, since it's no longer anchored to a line.

3. Comment style — consistent
   No /* */ comments, no section separators, no #include-line comments, no line-number refs in new code, no "Phase N" narrative. The one TODO(villagesql-indexing) (at unlink_neighbour, on distinguishing a freed record from a genuine fetch failure) uses a tag consistent with the existing visible() TODO in the same file.

Checklist items with no issues

• ABI (no abi/** headers touched), THD/PSI lifecycle (no thread teardown), string/schema-name hygiene (none), const std::string& params (none — the new API uses const Node&/NID/std::vector& throughout), narrowing casts (static_cast<uint16_t> on slot indices bounded well below 65535), new includes (all match new usage; correctly added to graph_ops_impl.h), diff hygiene (no unrelated reformatting/renames/reordering).

Verdict: No blocking issues. The lock-hierarchy documentation, the bidirectional-link protocol comment, and the per-function contracts are unusually thorough and internally consistent. The two items above are optional polish, not fixes.

[robgolebiowski-vsql]

LGTM