fix(graph): bound non-square CSR OOB read, honor max_depth, loud-error path-tracking, fix HNSW recall + deterministic sort (review 2.9)

- MEMORY SAFETY: 8 traverse algorithms (pagerank/connected/louvain/degree/
  topsort/cluster/betweenness/closeness) read OOB when a relation is
  non-square (fwd.n_nodes != rev.n_nodes) -> graph_require_square guard,
  loud 'domain' error (ASAN-confirmed real SEGV without it).
- Dijkstra/A* max_depth was accepted then ignored (search ran unbounded)
  -> now honored (both track a hop counter); default 255 keeps full search.
- var-expand path_tracking ('track-path') was accepted then ignored (only
  endpoints returned) -> loud 'nyi'; real path return is a large feature.
- HNSW: at neighbor saturation the back-link was silently dropped (prune was
  dead because M_max_l == M_keep), degrading recall -> proper 'select M
  closest' replacement; removed the dead prune_neighbors.
- CSR edge sort had no .row tie-break -> unstable rowmap for parallel edges;
  added .row tie-break for deterministic output.
- review claim STALE: HNSW xorshift RNG (fixed seed) is intended determinism,
  not a bug -- left unchanged.

Tests: non-square OOB bounded (ASAN), max_depth honored, CSR tie-break
determinism, ANN recall@1 under forced saturation; re-baselined the
var-expand track + HNSW tied-top-1 assertions. Clean-rebuild suite
3487/3489 0-failed, ASAN/UBSan + -Werror clean.

Author: singaraiona

src/ops/graph_builtin.c

@@ -742,6 +742,12 @@ ray_t* ray_graph_var_expand_fn(ray_t** args, int64_t n) {
             return ray_error("type", "graph.var-expand: track-path must be a boolean or integer, got %s", ray_type_name(args[5]->type));
         track = (args[5]->type == -RAY_BOOL) ? (args[5]->b8 != 0)
                                               : (atom_to_i64(args[5]) != 0);
+        /* Path tracking (returning the full intermediate node sequence per
+         * reached endpoint) is not implemented: the BFS only records
+         * (start, end, depth) endpoints, not parent chains.  Rather than
+         * silently ignore the flag (a lying API), reject it loudly. */
+        if (track)
+            return ray_error("nyi", "graph.var-expand: track-path is not supported (only start/end/depth endpoints are returned); pass track-path=false");
     }
 
     ray_graph_t* g = ray_graph_new(NULL);

src/ops/traverse.c

@@ -27,6 +27,23 @@
  * Graph execution functions
  * ============================================================================ */
 
+/* Precondition for algorithms that iterate node ids in [0, fwd.n_nodes) while
+ * dereferencing the reverse CSR at the same index (e.g. rev_off[i+1]).  A
+ * relation built with distinct src/dst node counts (ray_rel_from_edges with
+ * n_src_nodes != n_dst_nodes) has fwd.n_nodes != rev.n_nodes; indexing the
+ * smaller CSR's offset array with the larger bound reads out of bounds.
+ *
+ * Returns NULL when the relation is square (safe), otherwise a loud error
+ * naming the op.  Callers must propagate the returned error. */
+static ray_t* graph_require_square(const ray_rel_t* rel, const char* op_name) {
+    if (rel->fwd.n_nodes != rel->rev.n_nodes)
+        return ray_error("domain",
+            "%s: requires a square relation (forward and reverse node counts "
+            "must match), got fwd=%lld rev=%lld",
+            op_name, (long long)rel->fwd.n_nodes, (long long)rel->rev.n_nodes);
+    return NULL;
+}
+
 /* exec_expand_factorized: emit factorized output for expand+group fusion.
  * Returns a table with _src (unique sources) and _count (degree per source).
  * This avoids materializing the full (src, dst) cross-product. */
@@ -293,6 +310,11 @@ ray_t* exec_var_expand(ray_graph_t* g, ray_op_t* op, ray_t* start_vec) {
     uint8_t direction = ext->graph.direction;
     uint8_t min_depth = ext->graph.min_depth;
     uint8_t max_depth = ext->graph.max_depth;
+    /* path_tracking is accepted by the op constructor but unimplemented: the
+     * BFS records (start, end, depth) endpoints only, not parent chains.
+     * Surface a loud error instead of silently dropping the request. */
+    if (ext->graph.path_tracking)
+        return ray_error("nyi", "graph.var-expand: path tracking is not supported (only start/end/depth endpoints are returned)");
     ray_csr_t* csr_fwd = &rel->fwd;
     ray_csr_t* csr_rev = &rel->rev;
     /* For direction==2 (both), use fwd for n_nodes bound but expand both */
@@ -651,6 +673,7 @@ ray_t* exec_pagerank(ray_graph_t* g, ray_op_t* op) {
     double damping  = ext->graph.damping;
 
     if (n <= 0) return ray_error("length", "graph.pagerank: graph must have at least one node, got %lld", (long long)n);
+    { ray_t* sq = graph_require_square(rel, "graph.pagerank"); if (sq) return sq; }
 
     /* Arena for all scratch memory — freed in one shot */
     ray_scratch_arena_t arena;
@@ -752,6 +775,7 @@ ray_t* exec_connected_comp(ray_graph_t* g, ray_op_t* op) {
 
     int64_t n = rel->fwd.n_nodes;
     if (n <= 0) return ray_error("length", "graph.connected: graph must have at least one node, got %lld", (long long)n);
+    { ray_t* sq = graph_require_square(rel, "graph.connected"); if (sq) return sq; }
 
     /* Arena for all scratch memory — freed in one shot */
     ray_scratch_arena_t arena;
@@ -939,6 +963,7 @@ ray_t* exec_dijkstra(ray_graph_t* g, ray_op_t* op,
 
     int64_t n = rel->fwd.n_nodes;
     int64_t m = rel->fwd.n_edges;
+    uint8_t max_depth = ext->graph.max_depth;
     int64_t src_id = ray_is_atom(src_val) ? src_val->i64 : ((int64_t*)ray_data(src_val))[0];
     int64_t dst_id = !dst_val ? -1 : ray_is_atom(dst_val) ? dst_val->i64 : ((int64_t*)ray_data(dst_val))[0];
 
@@ -1000,6 +1025,11 @@ ray_t* exec_dijkstra(ray_graph_t* g, ray_op_t* op,
 
         if (u == dst_id) break;  /* early exit if destination reached */
 
+        /* Honor max-depth (hop bound): do not relax beyond max_depth hops
+         * from the source.  depth[] is the natural hop counter.  A path
+         * needing more than max_depth hops is simply not discovered. */
+        if (depth[u] >= max_depth) continue;
+
         for (int64_t j = fwd_off[u]; j < fwd_off[u + 1]; j++) {
             int64_t v = fwd_tgt[j];
             int64_t edge_row = fwd_row[j];
@@ -1179,6 +1209,7 @@ ray_t* exec_louvain(ray_graph_t* g, ray_op_t* op) {
     uint16_t max_iter = ext->graph.max_iter;
 
     if (n <= 0) return ray_error("length", "graph.louvain: graph must have at least one node, got %lld", (long long)n);
+    { ray_t* sq = graph_require_square(rel, "graph.louvain"); if (sq) return sq; }
 
     /* Arena for all scratch memory — freed in one shot */
     ray_scratch_arena_t arena;
@@ -1331,6 +1362,7 @@ ray_t* exec_degree_cent(ray_graph_t* g, ray_op_t* op) {
 
     int64_t n = rel->fwd.n_nodes;
     if (n <= 0) return ray_error("length", "graph.degree: graph must have at least one node, got %lld", (long long)n);
+    { ray_t* sq = graph_require_square(rel, "graph.degree"); if (sq) return sq; }
 
     int64_t* fwd_off = (int64_t*)ray_data(rel->fwd.offsets);
     int64_t* rev_off = (int64_t*)ray_data(rel->rev.offsets);
@@ -1397,6 +1429,7 @@ ray_t* exec_topsort(ray_graph_t* g, ray_op_t* op) {
 
     int64_t n = rel->fwd.n_nodes;
     if (n <= 0) return ray_error("length", "graph.topsort: graph must have at least one node, got %lld", (long long)n);
+    { ray_t* sq = graph_require_square(rel, "graph.topsort"); if (sq) return sq; }
 
     int64_t* fwd_off = (int64_t*)ray_data(rel->fwd.offsets);
     int64_t* fwd_tgt = (int64_t*)ray_data(rel->fwd.targets);
@@ -1490,6 +1523,7 @@ ray_t* exec_cluster_coeff(ray_graph_t* g, ray_op_t* op) {
 
     int64_t n = rel->fwd.n_nodes;
     if (n <= 0) return ray_error("length", "graph.cluster: graph must have at least one node, got %lld", (long long)n);
+    { ray_t* sq = graph_require_square(rel, "graph.cluster"); if (sq) return sq; }
 
     ray_scratch_arena_t arena;
     ray_scratch_arena_init(&arena);
@@ -1592,6 +1626,7 @@ ray_t* exec_betweenness(ray_graph_t* g, ray_op_t* op) {
 
     int64_t n = rel->fwd.n_nodes;
     if (n <= 0) return ray_error("length", "graph.betweenness: graph must have at least one node, got %lld", (long long)n);
+    { ray_t* sq = graph_require_square(rel, "graph.betweenness"); if (sq) return sq; }
     uint16_t sample = ext->graph.max_iter;
     int64_t n_sources = (sample > 0 && (int64_t)sample < n) ? (int64_t)sample : n;
 
@@ -1778,6 +1813,7 @@ ray_t* exec_closeness(ray_graph_t* g, ray_op_t* op) {
 
     int64_t n = rel->fwd.n_nodes;
     if (n <= 0) return ray_error("length", "graph.closeness: graph must have at least one node, got %lld", (long long)n);
+    { ray_t* sq = graph_require_square(rel, "graph.closeness"); if (sq) return sq; }
     uint16_t sample = ext->graph.max_iter;
     int64_t n_sources = (sample > 0 && (int64_t)sample < n) ? (int64_t)sample : n;
 
@@ -2224,6 +2260,7 @@ ray_t* exec_astar(ray_graph_t* g, ray_op_t* op,
 
     int64_t n = rel->fwd.n_nodes;
     int64_t m = rel->fwd.n_edges;
+    uint8_t max_depth = ext->graph.max_depth;
     int64_t src_id = src_val->i64;
     int64_t dst_id = dst_val->i64;
 
@@ -2282,6 +2319,9 @@ ray_t* exec_astar(ray_graph_t* g, ray_op_t* op,
 
         if (u == dst_id) break;
 
+        /* Honor max-depth (hop bound): stop expanding past max_depth hops. */
+        if (depth_a[u] >= max_depth) continue;
+
         for (int64_t j = fwd_off[u]; j < fwd_off[u + 1]; j++) {
             int64_t v = fwd_tgt[j];
             int64_t edge_row = fwd_row[j];

src/store/csr.c

@@ -44,25 +44,35 @@ typedef struct {
     int64_t row;  /* original row index for rowmap */
 } edge_pair_t;
 
-/* Comparison by src then dst */
+/* Comparison by src then dst, with .row as a final tie-break.
+ *
+ * qsort is not a stable sort: for parallel edges (equal src AND equal dst)
+ * the relative order of their original rows would otherwise be platform- and
+ * input-dependent, making rowmap (and any golden test or MST that consumes it)
+ * non-deterministic.  The .row tie-break pins a total order. */
 static int cmp_edge_by_src(const void* a, const void* b) {
     const edge_pair_t* ea = (const edge_pair_t*)a;
     const edge_pair_t* eb = (const edge_pair_t*)b;
     if (ea->src < eb->src) return -1;
     if (ea->src > eb->src) return 1;
     if (ea->dst < eb->dst) return -1;
     if (ea->dst > eb->dst) return 1;
+    if (ea->row < eb->row) return -1;
+    if (ea->row > eb->row) return 1;
     return 0;
 }
 
-/* Comparison by dst then src (for reverse CSR) */
+/* Comparison by dst then src (for reverse CSR), with .row as a final tie-break
+ * for determinism (see cmp_edge_by_src). */
 static int cmp_edge_by_dst(const void* a, const void* b) {
     const edge_pair_t* ea = (const edge_pair_t*)a;
     const edge_pair_t* eb = (const edge_pair_t*)b;
     if (ea->dst < eb->dst) return -1;
     if (ea->dst > eb->dst) return 1;
     if (ea->src < eb->src) return -1;
     if (ea->src > eb->src) return 1;
+    if (ea->row < eb->row) return -1;
+    if (ea->row > eb->row) return 1;
     return 0;
 }
 

src/store/hnsw.c

@@ -393,27 +393,45 @@ static int64_t hnsw_greedy_closest(const ray_hnsw_t* idx, const float* query,
 }
 
 /* --------------------------------------------------------------------------
- * Neighbor pruning: keep M closest neighbors (simple selection)
- * -------------------------------------------------------------------------- */
-
-static void prune_neighbors(const ray_hnsw_t* idx, int64_t node_id,
-                              int64_t* nb, int64_t M_max, int64_t M_keep) {
-    /* Count current neighbors */
+ * Neighbor selection: keep M closest neighbors (simple selection)
+ * --------------------------------------------------------------------------
+ *
+ * Connect new_id into node_id's neighbor list, applying "keep M closest"
+ * selection when the list is already full.
+ *
+ * Background: M_keep == M_max in this build (layer M_max equals the connect
+ * budget M / M_max0), so the old prune_neighbors() guard `count <= M_keep`
+ * was always true and prune was a no-op — meaning a saturated list silently
+ * dropped every back-link, degrading recall.  Here we instead consider the
+ * combined set {existing neighbors} ∪ {new_id}, rank by distance to node_id,
+ * and keep the M_keep closest.  If new_id is farther than all M current
+ * neighbors it is correctly not added (standard HNSW heuristic); otherwise it
+ * displaces the current farthest.
+ *
+ * Returns true if new_id ended up in the list, false if it was farther than
+ * all kept neighbors (a legitimate, non-silent drop). */
+static bool hnsw_connect_neighbor(const ray_hnsw_t* idx, int64_t node_id,
+                                   int64_t* nb, int64_t M_max, int64_t M_keep,
+                                   int64_t new_id) {
+    /* Fast path: room available (or already present). */
+    if (add_neighbor(nb, M_max, new_id)) return true;
+
+    /* List is full.  Build combined candidate set and keep M_keep closest. */
     int64_t count = count_neighbors(nb, M_max);
-    if (count <= M_keep) return;
+    int64_t total = count + 1;
+    hnsw_cand_t* ranked = (hnsw_cand_t*)ray_sys_alloc((size_t)total * sizeof(hnsw_cand_t));
+    if (!ranked) return false; /* OOM: leave list unchanged (no corruption) */
 
-    /* Compute distances from node to each neighbor */
     const float* vec = idx->vectors + node_id * idx->dim;
-    hnsw_cand_t* ranked = (hnsw_cand_t*)ray_sys_alloc((size_t)count * sizeof(hnsw_cand_t));
-    if (!ranked) return;
-
     for (int64_t i = 0; i < count; i++) {
         ranked[i].id = nb[i];
         ranked[i].dist = hnsw_dist(idx, vec, idx->vectors + nb[i] * idx->dim);
     }
+    ranked[count].id = new_id;
+    ranked[count].dist = hnsw_dist(idx, vec, idx->vectors + new_id * idx->dim);
 
-    /* Sort by distance ascending */
-    for (int64_t i = 1; i < count; i++) {
+    /* Insertion sort by distance ascending (lists are small, <= M_max+1). */
+    for (int64_t i = 1; i < total; i++) {
         hnsw_cand_t key = ranked[i];
         int64_t j = i - 1;
         while (j >= 0 && ranked[j].dist > key.dist) {
@@ -423,12 +441,19 @@ static void prune_neighbors(const ray_hnsw_t* idx, int64_t node_id,
         ranked[j + 1] = key;
     }
 
-    /* Keep M_keep closest */
+    bool kept = false;
+    int64_t keep = (M_keep < M_max) ? M_keep : M_max;
     for (int64_t i = 0; i < M_max; i++) {
-        nb[i] = (i < M_keep) ? ranked[i].id : -1;
+        if (i < keep && i < total) {
+            nb[i] = ranked[i].id;
+            if (ranked[i].id == new_id) kept = true;
+        } else {
+            nb[i] = -1;
+        }
     }
 
     ray_sys_free(ranked);
+    return kept;
 }
 
 /* --------------------------------------------------------------------------
@@ -560,11 +585,10 @@ ray_hnsw_t* ray_hnsw_build(const float* vectors, int64_t n_nodes, int32_t dim,
                 if (nb_local < 0) continue;
 
                 int64_t* their_nb = &layer->neighbors[nb_local * M_max_l];
-                if (!add_neighbor(their_nb, M_max_l, i)) {
-                    /* Neighbor list full — prune to make room, then add i */
-                    prune_neighbors(idx, nb_id, their_nb, M_max_l, M_keep);
-                    add_neighbor(their_nb, M_max_l, i);
-                }
+                /* Insert i into nb_id's list, displacing the farthest neighbor
+                 * when full (keep the M_keep closest).  Previously a full list
+                 * silently dropped the back-link, degrading recall. */
+                hnsw_connect_neighbor(idx, nb_id, their_nb, M_max_l, M_keep, i);
             }
 
             /* Update ep for next lower layer */

test/rfl/graph/graph_advanced.rfl

@@ -182,10 +182,12 @@
 ;; The reverse-CSR predecessors of sink node 5 are exactly {3,4}.
 (at VeRev '_end)                                 -- [3 4]
 
-;; track-path = true accepts the boolean form without changing schema
-;; (the underlying op records paths internally; the result table is the
-;; same {_start,_end,_depth} shape per the wrapper).
-(>= (count (.graph.var-expand G 0 1 2 0 true)) 4) -- true
+;; track-path = true is rejected loudly: path tracking is unimplemented
+;; (only {_start,_end,_depth} endpoints are returned), so rather than
+;; silently ignore the flag the wrapper returns a 'nyi error.
+(.graph.var-expand G 0 1 2 0 true)               !- nyi
+;; track-path = false still succeeds.
+(>= (count (.graph.var-expand G 0 1 2 0 false)) 4) -- true
 
 ;; ====================================================================
 ;; (.graph.betweenness h [sample])

test/rfl/graph/graph_builtin_branches.rfl

@@ -474,10 +474,10 @@
 ;; non-bool/non-int track -> type error (line 741-742)
 (.graph.var-expand Uw 0 1 2 0 "bad")                     !- type
 
-;; track as integer (1) -> exercises atom_to_i64 path (line 744)
-(>= (count (.graph.var-expand Uw 0 1 2 0 1)) 1)         -- true
+;; track as integer (1) -> truthy -> path tracking unsupported -> nyi error
+(.graph.var-expand Uw 0 1 2 0 1)                        !- nyi
 
-;; track as integer (0) -> exercises atom_to_i64 path (line 744)
+;; track as integer (0) -> falsy -> exercises atom_to_i64 path, succeeds
 (>= (count (.graph.var-expand Uw 0 1 2 0 0)) 1)         -- true
 
 ;; ====================================================================

test/rfl/graph/traverse_branch_cov.rfl

@@ -228,9 +228,10 @@
 (>= (sum (at PrFC '_rank)) 0.99)                     -- true
 (<= (sum (at PrFC '_rank)) 1.01)                     -- true
 
-;; (5) var-expand track-path integer-true 6-arg form (schema unchanged).
-(>= (count (.graph.var-expand FC 0 1 2 2 1)) 1)     -- true
-;; track-path integer-false also accepted.
+;; (5) var-expand track-path integer-true 6-arg form: path tracking is
+;;     unimplemented, so a truthy track-path flag is rejected loudly (nyi).
+(.graph.var-expand FC 0 1 2 2 1)                    !- nyi
+;; track-path integer-false is accepted (no tracking requested).
 (>= (count (.graph.var-expand FC 0 1 2 2 0)) 1)     -- true
 
 ;; =====================================================================