trending/codebase-memory-mcp
1
0
Fork 0
mirror of https://github.com/DeusData/codebase-memory-mcp.git synced 2026-11-12 09:47:20 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(cypher): expansion materializes every matched row — the cap falsified aggregates

Browse Source 
Second mechanism of #1196, exposed by the reporter's v0.10.6 retest: every
relationship-expansion site capped its output buffer at bind_cap*10, so edges
past the cap were silently dropped BEFORE WHERE and aggregation ever ran.
count() then reported the scanned prefix as if it were a fact — 9,360 of
13,691 DEFINES field-measured at --max-rows 1000 — and a LABEL on the source
did not protect you (the label workaround only ever fixed source
enumeration, which #1323 already made exact). max_rows is an output-row
limit per the public header; projection already enforces it.

All five capped sites now share one growable append (geometric growth,
size_t sizing): the per-hop expansion, its fixed/variable-length helpers and
process_edges, the bound-terminal driver, and the cross-join outer buffer.
Only allocation failure stops materialisation; match_count stays truthful
either way, so the #627 OPTIONAL contracts hold (a saturated buffer can no
longer exist, and the fallback rows share the same append). The #601
deadline still bounds pathological time, and hop caps keep bounding depth —
this removes only the silent row-dropping.

Regression test: 2 labeled sources x 30 edges with max_rows=2 — the old cap
returned count=20; ground truth 60 now holds, and the list form returns
exactly max_rows rows. Proven RED before and RED again on revert; cypher
181/181 and mcp suites green, including every OPTIONAL/#627 semantics test.

Fixes #1196 (together with #1323, which fixed the unlabeled source scan).

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This commit is contained in:
Martin Vogel
2026-08-18 11:19:32 +02:00
parent 07cac7d6b3
commit 58ef9f19ac
2 changed files with 164 additions and 85 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/cypher/cypher.c
+77 -66
View File
@@ -25,6 +25,7 @@ enum {
CYP_MAX_VARS = 16, /* max Cypher variables in a query */
CYP_MAX_EDGE_VARS = 8, /* max edge variables */
CYP_GROWTH_10 = 10, /* binding growth factor */
CYP_GROWTH_2 = 2, /* geometric buffer growth (#1196) */
CYP_CHAR_IDX1 = 1, /* second character index (e.g. op[1]) */
CYP_EBUF_MASK = 7,
CYP_NODE_COLS = 4, /* columns per node var: name, qn, label, file */
@@ -3101,10 +3102,33 @@ static void scan_pattern_nodes(cbm_store_t *store, const char *project, cbm_node
/* Process edges: look up target node, filter by label/props, add binding.
* `inbound` controls which end of the edge is the target id. */
/* #1196 (second mechanism): a hop's output buffer must hold EVERY matched
* row. max_rows is an OUTPUT-row limit — projection already enforces it — and
* WHERE/aggregation run after expansion, so any cap here silently falsifies
* results: field-measured, count() reported 9,360 of 13,691 DEFINES because
* the old bind_cap*10 ceiling dropped edges before aggregation ever saw them,
* and a labeled source did not save you. The buffer now grows geometrically;
* only allocation failure stops materialisation (the #601 deadline still
* bounds time), and match_count stays truthful either way (#627 contract). */
static bool binding_out_append(binding_t **rows, int *count, int *cap, binding_t *nb) {
if (*count == *cap) {
int next = *cap > 0 ? *cap * CYP_GROWTH_2 : CYP_GROWTH_10;
binding_t *grown = realloc(*rows, (size_t)next * sizeof(binding_t));
if (!grown) {
binding_free(nb);
return false;
}
*rows = grown;
*cap = next;
}
(*rows)[(*count)++] = *nb;
return true;
}
static void process_edges(cbm_store_t *store, cbm_edge_t *edges, int edge_count, bool inbound,
const cbm_node_pattern_t *target_node, binding_t *b, const char *to_var,
const char *rel_var, binding_t *new_bindings, int *new_count, int max_new,
int *match_count) {
const char *rel_var, binding_t **new_bindings, int *new_count,
int *new_cap, int *match_count) {
/* When the terminal node variable is ALREADY bound (e.g. the second pattern
* `(c)-[:CALLS]->(f)` where `f` came from an earlier MATCH), we must FILTER
* to edges that actually reach the bound node — not overwrite the caller's
@@ -3137,16 +3161,14 @@ static void process_edges(cbm_store_t *store, cbm_edge_t *edges, int edge_count,
node_fields_free(&found);
continue;
}
(*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;
(*match_count)++; /* a real neighbour exists, OOM or not */
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]);
}
(void)binding_out_append(new_bindings, new_count, new_cap, &nb);
node_fields_free(&found);
}
}
@@ -3161,8 +3183,8 @@ static _Thread_local int g_cypher_depth_clamped = 0;
static void expand_var_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
cbm_node_pattern_t *target_node, binding_t *b, cbm_node_t *src,
const char *to_var, binding_t *new_bindings, int *new_count,
int max_new, int *match_count) {
const char *to_var, binding_t **new_bindings, int *new_count,
int *new_cap, int *match_count) {
/* Clamp BOTH the explicit (`*1..N`) and unbounded (`*`, `*..m`) forms to the
* engine ceiling: an explicit N above the cap was previously honoured
* verbatim, driving cbm_store_bfs to an unbounded hop count (#887). WARN on
@@ -3201,12 +3223,10 @@ static void expand_var_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
continue;
}
(*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;
}
binding_t nb = {0};
binding_copy(&nb, b);
binding_set(&nb, to_var, &hop->node);
(void)binding_out_append(new_bindings, new_count, new_cap, &nb);
}
cbm_store_traverse_free(&tr);
}
@@ -3214,8 +3234,8 @@ static void expand_var_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
/* Expand fixed-length (1-hop) relationship edges */
static void expand_fixed_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
cbm_node_pattern_t *target_node, binding_t *b, cbm_node_t *src,
const char *to_var, binding_t *new_bindings, int *new_count,
int max_new, int *match_count) {
const char *to_var, binding_t **new_bindings, int *new_count,
int *new_cap, int *match_count) {
bool is_inbound = rel->direction && strcmp(rel->direction, "inbound") == 0;
bool is_any = rel->direction && strcmp(rel->direction, "any") == 0;
const char *rel_var = rel->variable;
@@ -3232,7 +3252,7 @@ static void expand_fixed_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
&edge_count);
}
process_edges(store, edges, edge_count, is_inbound, target_node, b, to_var, rel_var,
new_bindings, new_count, max_new, match_count);
new_bindings, new_count, new_cap, match_count);
cbm_store_free_edges(edges, edge_count);
}
if (is_any) {
@@ -3242,7 +3262,7 @@ static void expand_fixed_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
cbm_store_find_edges_by_target_type(store, src->id, rel->types[ti], &edges,
&edge_count);
process_edges(store, edges, edge_count, true, target_node, b, to_var, rel_var,
new_bindings, new_count, max_new, match_count);
new_bindings, new_count, new_cap, match_count);
cbm_store_free_edges(edges, edge_count);
}
}
@@ -3255,21 +3275,21 @@ static void expand_fixed_length(cbm_store_t *store, cbm_rel_pattern_t *rel,
cbm_store_find_edges_by_source(store, src->id, &edges, &edge_count);
}
process_edges(store, edges, edge_count, is_inbound, target_node, b, to_var, rel_var,
new_bindings, new_count, max_new, match_count);
new_bindings, new_count, new_cap, match_count);
cbm_store_free_edges(edges, edge_count);
if (is_any) {
edges = NULL;
edge_count = 0;
cbm_store_find_edges_by_target(store, src->id, &edges, &edge_count);
process_edges(store, edges, edge_count, true, target_node, b, to_var, rel_var,
new_bindings, new_count, max_new, match_count);
new_bindings, new_count, new_cap, match_count);
cbm_store_free_edges(edges, edge_count);
}
}
}
static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_t **bindings,
int *bind_count, const int *bind_cap, const char **var_name,
int *bind_count, int *bind_cap, const char **var_name,
bool is_optional) {
for (int ri = 0; ri < pat->rel_count; ri++) {
/* #601: stop expanding further hops once the wall-clock budget is spent
@@ -3283,16 +3303,14 @@ static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_
bool is_variable_length = (rel->min_hops != SKIP_ONE || rel->max_hops != SKIP_ONE);
/* Size this hop's output for BOTH writers without dropping any row: the
* expansion helpers emit at most max_new = bind_cap*10 rows (they stop at
* max_new), 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 "+ 1" sizing fit only a SINGLE fallback row after a
* saturated expansion; a second one ran off the end (heap OOB, CWE-787). */
size_t alloc_n = (size_t)*bind_cap * (size_t)CYP_GROWTH_10 + (size_t)*bind_count;
binding_t *new_bindings = malloc(alloc_n * sizeof(binding_t));
/* #1196: the hop's output buffer GROWS to hold every matched row —
* the old bind_cap*10 ceiling silently dropped edges before WHERE and
* aggregation, falsifying counts. binding_out_append handles growth
* and the OPTIONAL fallback shares it, so no writer can run off the
* end and no row class is dropped (the old fixed sizing had exactly
* those two failure modes, CWE-787 and the OPTIONAL data loss). */
int new_cap = *bind_count > 0 ? *bind_count : CYP_GROWTH_10;
binding_t *new_bindings = malloc((size_t)new_cap * sizeof(binding_t));
if (!new_bindings) {
return; /* OOM: leave existing bindings untouched rather than corrupt */
}
@@ -3310,24 +3328,22 @@ static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_
int match_count = 0;
int max_new = *bind_cap * CYP_GROWTH_10;
if (is_variable_length) {
expand_var_length(store, rel, target_node, b, src, to_var, new_bindings, &new_count,
max_new, &match_count);
expand_var_length(store, rel, target_node, b, src, to_var, &new_bindings,
&new_count, &new_cap, &match_count);
} else {
expand_fixed_length(store, rel, target_node, b, src, to_var, new_bindings,
&new_count, max_new, &match_count);
expand_fixed_length(store, rel, target_node, b, src, to_var, &new_bindings,
&new_count, &new_cap, &match_count);
}
/* OPTIONAL MATCH: no expansion for this source, so keep the binding
* with the target unbound (projection renders it ""). The buffer is
* sized max_new + *bind_count precisely so every such fallback row has
* a slot — no guard needed, and no OPTIONAL no-match row is dropped. */
* with the target unbound (projection renders it ""). The shared
* growable append gives every fallback row a slot. */
if (is_optional && match_count == 0) {
binding_t nb = {0};
binding_copy(&nb, b);
/* Don't set to_var — it remains unbound; projection returns "" */
new_bindings[new_count++] = nb;
(void)binding_out_append(&new_bindings, &new_count, &new_cap, &nb);
}
}
@@ -3337,6 +3353,7 @@ static void expand_pattern_rels(cbm_store_t *store, cbm_pattern_t *pat, binding_
free(*bindings);
*bindings = new_bindings;
*bind_count = new_count;
*bind_cap = new_cap;
*var_name = to_var;
}
}
@@ -4577,8 +4594,8 @@ static void cross_join_with_rels(cbm_store_t *store, cbm_pattern_t *patn, bindin
* graphs (e.g. an unbound `c` scanned against ~29 K `f` bindings), wrapping
* the int product negative and yielding a tiny/garbage malloc → heap OOB
* write → SIGSEGV/SIGABRT (#627). */
size_t alloc_n = (size_t)*bind_count * (size_t)extra_count * (size_t)CYP_GROWTH_10 + SKIP_ONE;
binding_t *new_bindings = malloc(alloc_n * sizeof(binding_t));
int new_cap = *bind_count > 0 && extra_count > 0 ? *bind_count : SKIP_ONE;
binding_t *new_bindings = malloc((size_t)new_cap * sizeof(binding_t));
if (!new_bindings) {
return; /* OOM: leave existing bindings untouched rather than corrupt */
}
@@ -4595,7 +4612,7 @@ static void cross_join_with_rels(cbm_store_t *store, cbm_pattern_t *patn, bindin
const char *tv = nvar;
expand_pattern_rels(store, patn, &tmp, &tc, &tcap, &tv, opt);
for (int ti = 0; ti < tc; ti++) {
new_bindings[new_count++] = tmp[ti];
(void)binding_out_append(&new_bindings, &new_count, &new_cap, &tmp[ti]);
}
free(tmp);
}
@@ -4647,13 +4664,12 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn,
* 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));
int new_cap = *bind_count > 0 ? *bind_count : CYP_GROWTH_10;
binding_t *new_bindings = malloc((size_t)new_cap * sizeof(binding_t));
if (!new_bindings) {
return;
}
int new_count = 0;
int max_new = *bind_count * CYP_GROWTH_10;
for (int bi = 0; bi < *bind_count; bi++) {
binding_t *b = &(*bindings)[bi];
@@ -4694,15 +4710,13 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn,
continue;
}
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;
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]);
}
(void)binding_out_append(&new_bindings, &new_count, &new_cap, &nb);
node_fields_free(&found);
}
cbm_store_free_edges(edges, edge_count);
@@ -4711,15 +4725,12 @@ static void expand_from_bound_terminal(cbm_store_t *store, cbm_pattern_t *patn,
if (opt && match_count == 0) {
/* GENUINELY no matching neighbour: 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. */
* means the terminal really has none. Keep the row with start_var left
* UNBOUND so `WHERE <start> IS NULL` correctly identifies the no-edge
* case; the shared growable append gives every fallback row a slot. */
binding_t nb = {0};
binding_copy(&nb, b);
new_bindings[new_count++] = nb;
(void)binding_out_append(&new_bindings, &new_count, &new_cap, &nb);
}
}