fix(cypher): make expand_from_bound_terminal OPTIONAL fallback lossless

Signed-off-by: SEPURI-SAI-KRISHNA <saik20533@gmail.com>

Author: SEPURI-SAI-KRISHNA

src/cypher/cypher.c

@@ -4549,21 +4549,43 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn,
     bool scan_targets =
         !rel_inbound; /* (start)->(term): start = edge source = scan term's inbound */
 
-    size_t alloc_n = (size_t)*bind_count * (size_t)CYP_GROWTH_10 + SKIP_ONE;
+    /* Size this hop's output for BOTH writers without dropping any row: the
+     * materialised expansion is capped at max_new = *bind_count * 10 rows (only
+     * the WRITE is gated on max_new; for OPTIONAL, match DETECTION below is not,
+     * so a full buffer never hides a real neighbour), and the OPTIONAL fallback
+     * emits at most one row per source (<= *bind_count). A source either matches
+     * (feeds the expansion) or takes the fallback, never both, so the two counts
+     * are additive and bounded by max_new + *bind_count. Computed in size_t so
+     * the product cannot overflow.
+     * The previous "+ SKIP_ONE" sizing tied max_new to the whole buffer, so once
+     * the expansion saturated it the fallback guard silently dropped every later
+     * OPTIONAL no-match row (a data-loss bug, not an OOB — the fallback stayed
+     * in-bounds behind that guard) — exactly the rows
+     * `OPTIONAL MATCH ... WHERE <start> IS NULL` is meant to surface. */
+    size_t alloc_n = (size_t)*bind_count * (size_t)CYP_GROWTH_10 + (size_t)*bind_count;
     binding_t *new_bindings = malloc(alloc_n * sizeof(binding_t));
     if (!new_bindings) {
         return;
     }
     int new_count = 0;
-    int max_new = (int)alloc_n;
+    int max_new = *bind_count * CYP_GROWTH_10;
 
-    for (int bi = 0; bi < *bind_count && new_count < max_new; bi++) {
+    for (int bi = 0; bi < *bind_count; bi++) {
         binding_t *b = &(*bindings)[bi];
         cbm_node_t *term = binding_get(b, patn->nodes[1].variable ? patn->nodes[1].variable : "");
         int match_count = 0;
         if (term) {
+            /* Only the WRITE is gated on the ceiling (below). For OPTIONAL, match
+             * DETECTION is not: `|| opt` keeps the scan running past a full buffer,
+             * so match_count counts every real neighbour. If detection stopped at
+             * max_new too, a saturated buffer would leave match_count at 0 for a
+             * terminal that actually has callers, and the fallback below would
+             * fabricate an UNBOUND "dead code" row for live code — worse than
+             * dropping a row. Non-OPTIONAL never reads match_count, so its guard
+             * reduces to `new_count < max_new` and keeps the cheap early-exit. */
             for (int ti = 0;
-                 ti < (rel->type_count > 0 ? rel->type_count : 1) && new_count < max_new; ti++) {
+                 ti < (rel->type_count > 0 ? rel->type_count : 1) && (new_count < max_new || opt);
+                 ti++) {
                 cbm_edge_t *edges = NULL;
                 int edge_count = 0;
                 if (rel->type_count > 0) {
@@ -4579,7 +4601,7 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn,
                 } else {
                     cbm_store_find_edges_by_source(store, term->id, &edges, &edge_count);
                 }
-                for (int ei = 0; ei < edge_count && new_count < max_new; ei++) {
+                for (int ei = 0; ei < edge_count && (new_count < max_new || opt); ei++) {
                     int64_t sid = scan_targets ? edges[ei].source_id : edges[ei].target_id;
                     cbm_node_t found = {0};
                     if (cbm_store_find_node_by_id(store, sid, &found) != CBM_STORE_OK) {
@@ -4589,22 +4611,30 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn,
                         node_fields_free(&found);
                         continue;
                     }
-                    binding_t nb = {0};
-                    binding_copy(&nb, b);
-                    binding_set(&nb, start_var, &found);
-                    if (rel->variable) {
-                        binding_set_edge(&nb, rel->variable, &edges[ei]);
+                    match_count++;
+                    if (new_count < max_new) {
+                        binding_t nb = {0};
+                        binding_copy(&nb, b);
+                        binding_set(&nb, start_var, &found);
+                        if (rel->variable) {
+                            binding_set_edge(&nb, rel->variable, &edges[ei]);
+                        }
+                        new_bindings[new_count++] = nb;
                     }
                     node_fields_free(&found);
-                    new_bindings[new_count++] = nb;
-                    match_count++;
                 }
                 cbm_store_free_edges(edges, edge_count);
             }
         }
-        if (opt && match_count == 0 && new_count < max_new) {
-            /* No matching neighbour: keep the row with start_var left UNBOUND so
-             * `WHERE <start> IS NULL` correctly identifies the no-edge case. */
+        if (opt && match_count == 0) {
+            /* GENUINELY no matching neighbour: for OPTIONAL the scan above runs
+             * unconditionally, so match_count is the true neighbour count and
+             * match_count == 0 here means the terminal really has none — not merely
+             * that the buffer filled first. Keep the row with start_var left UNBOUND
+             * so `WHERE <start> IS NULL` correctly identifies the no-edge case. The
+             * buffer is sized max_new + *bind_count precisely so every fallback row
+             * has a slot — no guard needed, and no OPTIONAL no-match row is dropped
+             * even after the expansion has saturated max_new. */
             binding_t nb = {0};
             binding_copy(&nb, b);
             new_bindings[new_count++] = nb;

tests/test_cypher.c

@@ -675,6 +675,154 @@ TEST(cypher_exec_optional_rel_leaf_fallback_survives) {
     PASS();
 }
 
+/* Sibling of the leaf-fallback test, but for the BOUND-TERMINAL expansion path
+ * (expand_from_bound_terminal): the OPTIONAL start var is unbound and the
+ * terminal is bound, e.g. `MATCH (f) OPTIONAL MATCH (c)-[:CALLS]->(f)`. That
+ * function sized its hop buffer bind_count*10 + 1 and gated the OPTIONAL
+ * fallback on `new_count < max_new`, so once one terminal's expansion saturated
+ * the buffer, every LATER terminal's no-match row was silently dropped — the
+ * rows `WHERE c IS NULL` is meant to surface. Not an overflow (the guard kept
+ * the write in bounds) but real data loss. Lossless sizing (bind_count*10 +
+ * bind_count, fallback ungated) preserves them. */
+TEST(cypher_exec_bound_terminal_optional_fallback_survives) {
+    cbm_store_t *s = cbm_store_open_memory();
+    cbm_store_upsert_project(s, "test", "/tmp/test");
+
+    /* 2 Function nodes (hub + leaf) → bind_count = 2, max_new = 20. The hub has
+     * 21 incoming CALLS edges (> max_new), so its expansion saturates the write
+     * buffer; the leaf has none, so it must still yield its OPTIONAL no-match
+     * row. Under the original "+ SKIP_ONE" sizing that row was dropped once the
+     * buffer filled; the lossless sizing preserves it regardless of the order in
+     * which the (unordered) label scan visits the two terminals. */
+    cbm_node_t hub = {
+        .project = "test", .label = "Function", .name = "hub", .qualified_name = "test.hub"};
+    int64_t hub_id = cbm_store_upsert_node(s, &hub);
+    cbm_node_t leaf = {
+        .project = "test", .label = "Function", .name = "leaf", .qualified_name = "test.leaf"};
+    cbm_store_upsert_node(s, &leaf);
+
+    /* Callers are non-Function so they do not inflate bind_count. Each CALLS the
+     * hub (source = caller, target = hub), so from the bound terminal `hub` the
+     * expansion binds the unbound start `c` to each caller. */
+    for (int i = 0; i < 21; i++) {
+        char nm[32];
+        char qn[48];
+        snprintf(nm, sizeof(nm), "caller%02d", i);
+        snprintf(qn, sizeof(qn), "test.caller%02d", i);
+        cbm_node_t caller = {.project = "test", .label = "Var", .name = nm, .qualified_name = qn};
+        int64_t cid = cbm_store_upsert_node(s, &caller);
+        cbm_edge_t e = {.project = "test", .source_id = cid, .target_id = hub_id, .type = "CALLS"};
+        cbm_store_insert_edge(s, &e);
+    }
+
+    /* max_rows 0 → the 100000 result ceiling, so the output LIMIT does not hide
+     * the leaf row; we are testing the hop buffer, not the output cap. */
+    cbm_cypher_result_t r = {0};
+    int rc = cbm_cypher_execute(
+        s, "MATCH (f:Function) OPTIONAL MATCH (c)-[:CALLS]->(f) RETURN f.name, c.name", "test", 0,
+        &r);
+    ASSERT_EQ(rc, 0);
+    ASSERT_EQ(r.col_count, 2);
+
+    /* The leaf terminal has no incoming CALLS edge, so it must yield one row with
+     * the start var `c` unbound (""). Before the fix this row was dropped once the
+     * hub saturated the buffer. Also confirm the hub still expanded to bound rows. */
+    bool leaf_fallback = false;
+    bool hub_expanded = false;
+    for (int i = 0; i < r.row_count; i++) {
+        const char *f = r.rows[i][0];
+        const char *c = r.rows[i][1];
+        if (strcmp(f, "leaf") == 0 && c[0] == '\0') {
+            leaf_fallback = true;
+        }
+        if (strcmp(f, "hub") == 0 && c[0] != '\0') {
+            hub_expanded = true;
+        }
+    }
+    ASSERT_TRUE(leaf_fallback); /* the bound-terminal OPTIONAL no-match row survived */
+    ASSERT_TRUE(hub_expanded);  /* the expansion still produced bound rows */
+
+    cbm_cypher_result_free(&r);
+    cbm_store_close(s);
+    PASS();
+}
+
+/* Discriminating companion to the test above: preserving OPTIONAL no-match rows
+ * under saturation must NOT be bought by fabricating them. If match detection is
+ * gated on the same ceiling as the write, then once one terminal fills the
+ * buffer, another terminal that genuinely HAS callers is never scanned, its
+ * match_count stays 0, and the fallback invents an unbound "dead code" row for it
+ * — reporting live code as dead, which is worse than dropping a row.
+ *
+ * The construction is deliberately order-independent: BOTH hubs have enough
+ * callers to saturate the buffer on their own, so whichever the scan visits
+ * second is guaranteed to be processed after saturation. Under the gated variant
+ * that second hub is fabricated as dead; the assertion "no hub with callers is
+ * dead" then fails no matter which order `find_nodes_by_label` returns (its query
+ * has no ORDER BY, so the test must not depend on one). The fix makes it pass. */
+TEST(cypher_exec_bound_terminal_saturation_no_false_deadcode) {
+    cbm_store_t *s = cbm_store_open_memory();
+    cbm_store_upsert_project(s, "test", "/tmp/test");
+
+    /* 3 Function terminals → bind_count = 3, max_new = 30. */
+    cbm_node_t hubA = {
+        .project = "test", .label = "Function", .name = "hubA", .qualified_name = "test.hubA"};
+    int64_t hubA_id = cbm_store_upsert_node(s, &hubA);
+    cbm_node_t hubB = {
+        .project = "test", .label = "Function", .name = "hubB", .qualified_name = "test.hubB"};
+    int64_t hubB_id = cbm_store_upsert_node(s, &hubB);
+    cbm_node_t leaf = {
+        .project = "test", .label = "Function", .name = "leaf", .qualified_name = "test.leaf"};
+    cbm_store_upsert_node(s, &leaf);
+
+    /* BOTH hubs get 35 callers (> max_new = 30), so either one saturates the write
+     * buffer by itself; leaf gets none. Callers are non-Function so they don't
+     * inflate bind_count. */
+    for (int i = 0; i < 70; i++) {
+        char nm[32];
+        char qn[48];
+        snprintf(nm, sizeof(nm), "caller%02d", i);
+        snprintf(qn, sizeof(qn), "test.caller%02d", i);
+        cbm_node_t caller = {.project = "test", .label = "Var", .name = nm, .qualified_name = qn};
+        int64_t cid = cbm_store_upsert_node(s, &caller);
+        int64_t tgt = i < 35 ? hubA_id : hubB_id; /* 35 -> hubA, 35 -> hubB */
+        cbm_edge_t e = {.project = "test", .source_id = cid, .target_id = tgt, .type = "CALLS"};
+        cbm_store_insert_edge(s, &e);
+    }
+
+    cbm_cypher_result_t r = {0};
+    int rc = cbm_cypher_execute(
+        s, "MATCH (f:Function) OPTIONAL MATCH (c)-[:CALLS]->(f) RETURN f.name, c.name", "test", 0,
+        &r);
+    ASSERT_EQ(rc, 0);
+    ASSERT_EQ(r.col_count, 2);
+
+    bool hub_expanded = false;      /* sanity: the buffer really did fill from a hub */
+    bool hub_false_deadcode = false; /* the bug: a hub with callers invented as dead */
+    bool leaf_deadcode = false;     /* the lossless property: genuine dead code kept */
+    for (int i = 0; i < r.row_count; i++) {
+        const char *f = r.rows[i][0];
+        const char *c = r.rows[i][1];
+        bool is_hub = strcmp(f, "hubA") == 0 || strcmp(f, "hubB") == 0;
+        if (is_hub && c[0] != '\0') {
+            hub_expanded = true;
+        }
+        if (is_hub && c[0] == '\0') {
+            hub_false_deadcode = true;
+        }
+        if (strcmp(f, "leaf") == 0 && c[0] == '\0') {
+            leaf_deadcode = true;
+        }
+    }
+    ASSERT_TRUE(hub_expanded);
+    ASSERT_FALSE(hub_false_deadcode); /* live code with callers must never appear as dead */
+    ASSERT_TRUE(leaf_deadcode);       /* genuine no-match row still survives saturation */
+
+    cbm_cypher_result_free(&r);
+    cbm_store_close(s);
+    PASS();
+}
+
 TEST(cypher_exec_where_eq) {
     cbm_store_t *s = setup_cypher_store();
     cbm_cypher_result_t r = {0};
@@ -3425,6 +3573,8 @@ SUITE(cypher) {
     RUN_TEST(cypher_cross_join_alloc_rejects_overflow);
     RUN_TEST(cypher_exec_optional_rel_saturated_no_overflow);
     RUN_TEST(cypher_exec_optional_rel_leaf_fallback_survives);
+    RUN_TEST(cypher_exec_bound_terminal_optional_fallback_survives);
+    RUN_TEST(cypher_exec_bound_terminal_saturation_no_false_deadcode);
     RUN_TEST(cypher_issue240_labels_function);
     RUN_TEST(cypher_issue237_distinct_order_limit);
     RUN_TEST(cypher_issue873_distinct_order_limit_dedupes_before_limit);