From 27d9831ab85698647710b8e9acc45cb63d38143e Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Fri, 31 Jul 2026 18:13:35 +0200 Subject: [PATCH] fix(cypher): never fabricate an OPTIONAL "no match" after the budget fills process_edges and expand_var_length carried the expansion budget in the LOOP condition (`ei < edge_count && *new_count < max_new`). Once new_count reached max_new they stopped iterating entirely, so match_count was never incremented for the remaining sources -- even though those sources had real neighbours. expand_pattern_rels' OPTIONAL fallback is ungated (it has a reserved slot per binding since the #1177 sizing fix), so it saw match_count == 0 and emitted an unbound row for every such source. The result is a false negative that reads as a positive assertion: MATCH (f:Function) OPTIONAL MATCH (f)-[:CALLS]->(c) WHERE c IS NULL reports a function as having no callers when it has callers, once an earlier function in the same scan saturated the budget. That is a dead-code query telling a user that live code is dead. The budget caps MATERIALISATION, never detection. Both loops now iterate the full candidate set, increment match_count whenever a real neighbour passes the filters, and gate only the write into new_bindings. The bound proof from #1177 is unchanged -- writes are still capped at max_new and fallbacks are still at most one per binding -- so allocation safety does not depend on this change. Cost: a saturated source still performs its node lookups and filter checks, the same ones the unsaturated path performs, and materialises nothing. Reported by @SEPURI-SAI-KRISHNA while working on the sibling bound-terminal path in #1378; this is the same defect in expand_pattern_rels, which was already live on main rather than introduced there. Signed-off-by: Martin Vogel --- src/cypher/cypher.c | 41 +++++++++++++++------- tests/test_cypher.c | 83 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 13 deletions(-) diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 2840672f0..db0c46976 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -3082,7 +3082,15 @@ static void process_edges(cbm_store_t *store, cbm_edge_t *edges, int edge_count, * the result of dead-code queries and produced wrong rows (#627). */ cbm_node_t *bound_to = binding_get(b, to_var); int64_t bound_to_id = bound_to ? bound_to->id : 0; - for (int ei = 0; ei < edge_count && *new_count < max_new; ei++) { + /* The budget caps MATERIALISATION, not detection: `match_count` must stay + * truthful even after `new_count` hits `max_new`. Gating the loop itself + * (`ei < edge_count && *new_count < max_new`) stopped fetching entirely, so + * a saturated source reported `match_count == 0` despite having neighbours — + * and the OPTIONAL fallback in expand_pattern_rels then emitted an unbound + * "no match" row for it. `WHERE x IS NULL` reads those rows as "nothing + * points here", i.e. it reported live code as dead. Losing rows is + * recoverable; asserting a match does not exist when it does is not. */ + for (int ei = 0; ei < edge_count; ei++) { int64_t tid = inbound ? edges[ei].source_id : edges[ei].target_id; if (bound_to && tid != bound_to_id) { continue; /* edge does not reach the already-bound terminal node */ @@ -3099,15 +3107,17 @@ static void process_edges(cbm_store_t *store, cbm_edge_t *edges, int edge_count, node_fields_free(&found); continue; } - binding_t nb = {0}; - binding_copy(&nb, b); - binding_set(&nb, to_var, &found); - if (rel_var) { - binding_set_edge(&nb, rel_var, &edges[ei]); + (*match_count)++; /* a real neighbour exists, budget or not */ + if (*new_count < max_new) { + binding_t nb = {0}; + binding_copy(&nb, b); + binding_set(&nb, to_var, &found); + if (rel_var) { + binding_set_edge(&nb, rel_var, &edges[ei]); + } + new_bindings[(*new_count)++] = nb; } node_fields_free(&found); - new_bindings[(*new_count)++] = nb; - (*match_count)++; } } @@ -3141,7 +3151,10 @@ static void expand_var_length(cbm_store_t *store, cbm_rel_pattern_t *rel, cbm_traverse_result_t tr = {0}; const char *dir = rel->direction ? rel->direction : "outbound"; cbm_store_bfs(store, src->id, dir, rel->types, rel->type_count, max_depth, CBM_PERCENT, &tr); - for (int v = 0; v < tr.visited_count && *new_count < max_new; v++) { + /* Same contract as process_edges: the budget caps materialisation, never + * detection, so a saturated source cannot report match_count == 0 and get a + * fabricated OPTIONAL "no match" row. */ + for (int v = 0; v < tr.visited_count; v++) { cbm_node_hop_t *hop = &tr.visited[v]; if (hop->hop < rel->min_hops) { continue; @@ -3152,11 +3165,13 @@ static void expand_var_length(cbm_store_t *store, cbm_rel_pattern_t *rel, if (!check_inline_props(&hop->node, target_node->props, target_node->prop_count, store)) { continue; } - binding_t nb = {0}; - binding_copy(&nb, b); - binding_set(&nb, to_var, &hop->node); - new_bindings[(*new_count)++] = nb; (*match_count)++; + if (*new_count < max_new) { + binding_t nb = {0}; + binding_copy(&nb, b); + binding_set(&nb, to_var, &hop->node); + new_bindings[(*new_count)++] = nb; + } } cbm_store_traverse_free(&tr); } diff --git a/tests/test_cypher.c b/tests/test_cypher.c index adb52a98f..73a0455ec 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -580,6 +580,88 @@ TEST(cypher_exec_optional_rel_saturated_no_overflow) { PASS(); } +/* Reproduce-first: after the expansion budget is exhausted, OPTIONAL MATCH must + * not FABRICATE a "no match" row for a source that genuinely has matches. + * + * process_edges / expand_var_length used to carry the budget in the LOOP + * condition (`ei < edge_count && *new_count < max_new`), so once new_count hit + * max_new they stopped iterating entirely and never incremented match_count — + * even though neighbours existed. expand_pattern_rels' ungated fallback then saw + * match_count == 0 and emitted an unbound row. `WHERE b IS NULL` reads that as + * "nothing points here", so a dead-code query reported LIVE code as dead. + * + * Shape: A saturates the budget, B genuinely has no callees, C has 5. Only B may + * appear with an unbound b. Asserting on C specifically is what discriminates — + * the pre-fix code emits C with an empty b, and a `row_count` check would not + * notice. Deterministic: insertion order fixes the scan order (rowid), and every + * count is exact. */ +TEST(cypher_exec_optional_saturated_does_not_fabricate_no_match) { + cbm_store_t *s = cbm_store_open_memory(); + cbm_store_upsert_project(s, "test", "/tmp/test"); + + /* 3 Function nodes → scan_count = 3; max_rows = 3 → bind_cap = 3 and + * max_new = 30. A alone exceeds that, so B and C are reached with the + * budget already spent — the regime that produced the fabrication. */ + cbm_node_t a = {.project = "test", .label = "Function", .name = "A", .qualified_name = "test.A"}; + cbm_node_t b = {.project = "test", .label = "Function", .name = "B", .qualified_name = "test.B"}; + cbm_node_t c = {.project = "test", .label = "Function", .name = "C", .qualified_name = "test.C"}; + int64_t a_id = cbm_store_upsert_node(s, &a); + (void)cbm_store_upsert_node(s, &b); /* B: no outgoing CALLS at all */ + int64_t c_id = cbm_store_upsert_node(s, &c); + ASSERT_GT(a_id, 0); + ASSERT_GT(c_id, 0); + + /* Callees are label Var so they do not inflate scan_count/bind_cap. */ + for (int i = 0; i < 40; i++) { + char nm[32]; + char qn[48]; + snprintf(nm, sizeof(nm), "acallee%d", i); + snprintf(qn, sizeof(qn), "test.acallee%d", i); + cbm_node_t t = {.project = "test", .label = "Var", .name = nm, .qualified_name = qn}; + int64_t tid = cbm_store_upsert_node(s, &t); + cbm_edge_t e = {.project = "test", .source_id = a_id, .target_id = tid, .type = "CALLS"}; + cbm_store_insert_edge(s, &e); + } + for (int i = 0; i < 5; i++) { + char nm[32]; + char qn[48]; + snprintf(nm, sizeof(nm), "ccallee%d", i); + snprintf(qn, sizeof(qn), "test.ccallee%d", i); + cbm_node_t t = {.project = "test", .label = "Var", .name = nm, .qualified_name = qn}; + int64_t tid = cbm_store_upsert_node(s, &t); + cbm_edge_t e = {.project = "test", .source_id = c_id, .target_id = tid, .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 (f)-[:CALLS]->(g) WHERE g IS NULL RETURN f.name", + "test", 3, &r); + ASSERT_EQ(rc, 0); + + /* Positive control: B genuinely has no callees, so the query must still find + * it. Without this a "C is absent" assertion could pass on an empty result. */ + bool saw_b = false; + bool saw_c = false; + for (int i = 0; i < r.row_count; i++) { + const char *name = r.rows[i][0]; + if (name && strcmp(name, "B") == 0) { + saw_b = true; + } + if (name && strcmp(name, "C") == 0) { + saw_c = true; + } + } + ASSERT_TRUE(saw_b); + /* The discriminator: C has 5 callees, so claiming it has none is a + * fabrication. Pre-fix this is exactly what the saturated path emitted. */ + ASSERT_FALSE(saw_c); + + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + /* Arithmetic-boundary companion to the zero-label overflow above: the node * cross-join sizes its buffer from bind_count * extra_count. As a plain int that * product wraps past INT_MAX to a negative/garbage malloc size (the large-graph @@ -3491,6 +3573,7 @@ SUITE(cypher) { RUN_TEST(cypher_exec_optional_empty_label_no_overflow); RUN_TEST(cypher_cross_join_alloc_rejects_overflow); RUN_TEST(cypher_exec_optional_rel_saturated_no_overflow); + RUN_TEST(cypher_exec_optional_saturated_does_not_fabricate_no_match); RUN_TEST(cypher_exec_optional_rel_leaf_fallback_survives); RUN_TEST(cypher_issue240_labels_function); RUN_TEST(cypher_issue237_distinct_order_limit);