fix(cypher): make expand_from_bound_terminal OPTIONAL fallback lossless

[SEPURI-SAI-KRISHNA]

What does this PR do?

Makes the OPTIONAL no-match fallback in expand_from_bound_terminal
(src/cypher/cypher.c) lossless and free of false positives — the follow-up
called out in the merge note on #1177.

That function drives a single-relationship OPTIONAL MATCH from its already-bound
terminal node — e.g. MATCH (f:Function) OPTIONAL MATCH (c)-[:CALLS]->(f), where f
is bound and the start c is not. It sized its hop-output buffer bind_count*10 + 1
and gated the no-match fallback write on new_count < max_new, with max_new equal to
the whole buffer. Once one terminal's expansion saturated the buffer, every later
terminal's no-match row was silently dropped — exactly the rows
OPTIONAL MATCH ... WHERE c IS NULL is meant to surface. Not a memory-safety bug (the
guard kept the write in bounds), but real data loss under saturation, and the same
shape as the fallback issue fixed for the sibling expand_pattern_rels in #1177.

Fix, in two parts:

1. Lossless sizing. Size the buffer (size_t)*bind_count * CYP_GROWTH_10 + (size_t)*bind_count. The materialised expansion is capped at max_new = *bind_count * 10; 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; operands <= INT_MAX, so no overflow.

2. Decouple match detection from materialisation (so the fallback never fabricates a
   false result). For OPTIONAL the ti/ei scan loops are no longer gated on the ceiling
   (... && (new_count < max_new || opt)); only the write is
   (if (new_count < max_new)). So match_count is the true neighbour count, and
   match_count == 0 at the fallback means the terminal genuinely has none — not "the
   buffer filled before we looked." Non-OPTIONAL never reads match_count, so its guard
   reduces to new_count < max_new and keeps the original early-exit.

Behaviour: no OPTIONAL no-match row is dropped; no live terminal is ever mislabelled dead;
a saturated-but-matching terminal is truncated at the per-hop ceiling (bounded success,
unchanged).

Tests (reproduce-first, RED→GREEN under ASan+UBSan):

• cypher_exec_bound_terminal_optional_fallback_survives — a hub with more incoming
  CALLS edges than max_new saturates the buffer; a callerless leaf's OPTIONAL no-match
  row must still survive. Fails under the old + 1 sizing.
• cypher_exec_bound_terminal_saturation_no_false_deadcode — two hubs, each able to
  saturate the buffer alone (so the construction is order-independent; the label-scan query
  has no ORDER BY), plus a genuinely callerless leaf. Asserts no hub with callers ever
  appears as an unbound (c IS NULL) row while the leaf still does. Fails against
  gated detection, passes only once detection is decoupled from the write.

Checklist

• Every commit is signed off (git commit -s) — required, CI rejects
  unsigned commits (DCO, see CONTRIBUTING.md)
• Tests pass locally (make -f Makefile.cbm test)
• Lint passes (make -f Makefile.cbm lint-ci)
• New behavior is covered by a test (reproduce-first for bug fixes)

_{Notes: cypher suite 168 passed under ASan+UBSan. The full suite shows 10 failures that
are pre-existing and unrelated to this change (CLI install/uninstall/binary-publish tests
that need a real install target, a subprocess-timeout timing test, and the vendored
integrity check) — the cypher module is green. clang-format/cppcheck were not available in
my local environment; the change mirrors the sibling expand_pattern_rels formatting and
stays under 100 columns, so I am relying on CI for the formal lint gate.}

[github-actions]

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

[DeusData]

Thank you for picking this up so quickly — and the bug you are fixing is real. I verified it on current main: cypher.c:4558 sizes bind_count*10 + 1, :4564 ties max_new to the whole buffer, and both the outer loop (:4566) and the fallback (:4611) are gated on new_count < max_new, so once one terminal's expansion saturates the buffer every later terminal's no-match row is silently dropped. Your sizing proof is sound, the allocation is safe, the scope is exactly two files, and the security pass is clean.

But I have to hold it on one finding, because in the regime it targets it now produces a worse answer than main does.

Removing the two guards makes every binding reachable, which is the goal. The problem is that new_count < max_new still gates the ti loop (:4576-4577) and the ei loop (:4593). So once new_count == max_new, the remaining terminals never fetch their edges at all — match_count stays 0 — and the now-ungated fallback at :4616 emits an unbound row for each of them regardless of whether they actually have matching neighbours.

Worked through: three Function terminals, bind_count = 3, max_new = 30. A has 40 callers and saturates. B has 0 callers and gets a correct fallback row. C has 5 callers, its loop is skipped, match_count is 0, and it gets a fallback row too. The late WHERE c IS NULL filter at :4749 then keeps exactly those rows — so

MATCH (f:Function) OPTIONAL MATCH (c)-[:CALLS]->(f) WHERE c IS NULL

reports C as dead code when it has five callers. On main, C simply did not appear.

That is the trade I cannot take: missing rows become fabricated rows, and on the flagship dead-code query a false positive is the one a user acts on destructively. Under-reporting is bad; confidently telling someone live code is dead is worse.

The fix is three lines, and it keeps everything you have already proved. Decouple "did this terminal match" from "was there budget to materialise it" — count the match unconditionally, gate only the write:

for (int ei = 0; ei < edge_count; ei++) {
    /* ... find node, label filter (unchanged) ... */
    match_count++;                          /* a real neighbour exists, budget or not */
    if (new_count < max_new) {              /* only the materialisation is capped */
        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);
}

Drop new_count < max_new from the ti loop condition too. Your bound proof is unchanged — writes are still gated by max_new, fallbacks are still at most one per binding. The cost is that the saturated tail still does its edge lookups, which are the same lookups the unsaturated path already does, and materialises nothing.

One thing about the test, said carefully because it is a good test. It genuinely binds and it is genuinely RED on main — I traced it: hub is driven first by rowid order, fills the buffer at new_count = 21, the outer guard ends the loop, leaf is never visited, and your ASSERT_TRUE(leaf_fallback) fails. The assertions are specific rather than row_count > 0. All good.

The gap is that it is green through the path the finding condemns. With your fix, by the time leaf is reached new_count == max_new, so its edges are never queried and the asserted row comes from the saturation-blind fallback. The test would pass identically if leaf had 500 callers. It pins "a row with empty c appears for leaf", not "leaf's no-match was determined correctly".

A test that discriminates — green on main, RED on this PR as written, green under the fix above — is: insert three Functions in order, A with 40 incoming CALLS, B with 0, C with 5, then

MATCH (f:Function) OPTIONAL MATCH (c)-[:CALLS]->(f) RETURN f.name, c.name

and assert no row has f == "C" with an empty c. With max_new = 30, A saturates, B falls back correctly, and C is the fabricated row.

Two things that are ours, not yours. expand_pattern_rels has the same fabrication today — its fallback has been ungated since before #1177, and its outer loop never had a budget guard — so merging yours as-is would make the two paths consistently wrong rather than introducing something new. And the underlying question of what the expansion ceiling should mean when it is hit is a design call I have taken to the maintainer rather than deciding in review.

This is your fourth fix in this engine after #1173, #1176 and #1177, and the standard has been high in all four. Send the match_count change plus the discriminating test and I will pick it straight back up.

[SEPURI-SAI-KRISHNA]

Thanks for the precise diagnosis. Updated so match detection is independent of the write ceiling, detection and materialisation are now decoupled.

• The ti and ei loops no longer stop at max_new for OPTIONAL (... && (new_count < max_new || opt)), so every real neighbour is counted regardless of buffer state. Only the write is gated (if (new_count < max_new)). match_count == 0 at the fallback therefore means the terminal genuinely has no neighbour, not that the buffer filled first. Non-OPTIONAL never reads match_count, so its guard reduces to new_count < max_new and keeps the original early-exit.
• A saturated-but-matching terminal is truncated at the ceiling (bounded, as before); a live terminal is never surfaced as dead, and no genuine no-match row is dropped. Buffer sizing is unchanged (max_new + *bind_count) and still overflow-free.

New test cypher_exec_bound_terminal_saturation_no_false_deadcode: two hubs, each with enough callers to saturate the buffer on its own, so the case holds whichever order the label scan visits them (that query has no ORDER BY), plus a genuinely callerless leaf. It asserts no hub with callers appears as an unbound (c IS NULL) row while the leaf still does; it passes with the decoupled detection and fails without it. Full cypher suite green under ASan+UBSan.