From fea266fc5345d855e44192168efcb0e4ebe1d22d Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Tue, 7 Jul 2026 00:58:05 +0200 Subject: [PATCH 1/2] fix(gbuf): canonical collision winner honors the upsert refresh contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deterministic same-QN collision rule picked the lexicographically smallest (label, file_path, start_line, name), which silently inverted the documented upsert/merge semantics: re-upserting a QN with updated fields kept the OLD content (gbuf_upsert_updates, gbuf_upsert_same_qn_updates_all_fields, gbuf_merge_overlapping_qns all red on every platform). Replace it with a single mixed-direction total order applied in both cbm_gbuf_upsert_node and merge_update_existing: smallest file_path, then LARGEST start_line, then largest name/label. Still a pure content function of the two candidates (commutative, associative, scheduling-independent — multi-threaded runs stay byte-identical, verified by the xfs MT x4 fingerprint guard), and line-descending within a file restores the classic contract: a later definition in the same file replaces the earlier one, and a full tie is the same entity refreshed in place. Signed-off-by: Martin Vogel --- src/graph_buffer/graph_buffer.c | 111 ++++++++++++++------------------ 1 file changed, 47 insertions(+), 64 deletions(-) diff --git a/src/graph_buffer/graph_buffer.c b/src/graph_buffer/graph_buffer.c index 81b5ca75f..ca541e7d4 100644 --- a/src/graph_buffer/graph_buffer.c +++ b/src/graph_buffer/graph_buffer.c @@ -663,45 +663,35 @@ int64_t cbm_gbuf_upsert_node(cbm_gbuf_t *gb, const char *label, const char *name (strcmp(existing->label, "Project") == 0 || strcmp(existing->label, "Folder") == 0)) { return existing->id; } - /* Same-QN arrival. Two distinct cases: - * - * 1) The SAME source entity re-upserted (identity fields match) — - * refresh in place, exactly as before. - * - * 2) A DIFFERENT source entity sharing the QN. In C a struct, a - * function and a macro can all carry one name and the QN scheme - * does not encode the entity kind, so they collide here. The old - * code let the LAST arrival overwrite — under parallel extraction - * the merge order varies run to run, so WHICH entity survived - * (label/name/lines/props) flickered (xfs: Function-node count - * 4998 vs 5015 across two runs), and every order-sensitive - * consumer downstream (semantic vectors, LSH, near-threshold - * scores) inherited the flicker. Pick the survivor by a canonical - * CONTENT rule instead — lexicographically smallest - * (label, file_path, start_line, name) — so the outcome is a pure - * function of the colliding entities, not of scheduling. Exactly - * one entity is dropped, as one always was; kind-disambiguated - * QNs (the real cure) are tracked as a follow-up. */ - bool same_entity = existing->label && label && strcmp(existing->label, label) == 0 && - existing->file_path && file_path && - strcmp(existing->file_path, file_path) == 0 && - existing->start_line == start_line && existing->name && name && - strcmp(existing->name, name) == 0; - if (!same_entity) { - int c = strcmp(label ? label : "", existing->label ? existing->label : ""); - if (c == 0) { - c = strcmp(file_path ? file_path : "", - existing->file_path ? existing->file_path : ""); - } - if (c == 0) { - c = start_line - existing->start_line; - } - if (c == 0) { - c = strcmp(name ? name : "", existing->name ? existing->name : ""); - } - if (c >= 0) { - return existing->id; /* existing entity is the canonical winner */ - } + /* Same-QN arrival: distinct source entities can share a QN (C: a + * struct, a function and a macro with one name), and the same entity + * can be re-upserted with fresh content. The old code let the LAST + * arrival overwrite — under parallel extraction the merge order + * varies run to run, so WHICH entity survived flickered (xfs: + * Function-node count 4998 vs 5015 across two runs) and every + * order-sensitive consumer downstream inherited it. Pick the + * survivor by a canonical CONTENT rule instead, a pure function of + * the two candidates: smallest file_path, then LARGEST start_line, + * then largest name/label (a mixed-direction composite is still a + * total order, so the pick is commutative and scheduling-free). + * Line-descending within a file keeps the classic upsert contract — + * a later definition in the same file (macro redefinition, refresh + * of the same entity with new content) replaces the earlier one, + * deterministically, because intra-file arrival order is fixed. A + * full tie is the same entity re-upserted → refresh in place. + * Kind-disambiguated QNs (the real cure) remain a follow-up. */ + int c = strcmp(file_path ? file_path : "", existing->file_path ? existing->file_path : ""); + if (c == 0) { + c = existing->start_line - start_line; + } + if (c == 0) { + c = strcmp(existing->name ? existing->name : "", name ? name : ""); + } + if (c == 0) { + c = strcmp(existing->label ? existing->label : "", label ? label : ""); + } + if (c > 0) { + return existing->id; /* existing entity is the canonical winner */ } /* Update in-place. name/properties are strdup'd BEFORE freeing old ones * (callers may pass existing->name as an argument). label/file_path are @@ -1206,32 +1196,25 @@ static void merge_update_existing(cbm_gbuf_t *dst, cbm_gbuf_node_t *existing, (strcmp(existing->label, "Project") == 0 || strcmp(existing->label, "Folder") == 0); if (!module_on_container) { /* Canonical collision winner (determinism) — mirrors - * cbm_gbuf_upsert_node. Distinct source entities can share a QN (C: - * struct/function/macro with one name); unconditional "src wins" made - * the survivor depend on worker merge order, flickering the node set - * (and every downstream consumer) run to run. Same entity → refresh; - * different entity → keep the lexicographically smallest - * (label, file_path, start_line, name). */ - bool same_entity = existing->label && sn->label && - strcmp(existing->label, sn->label) == 0 && existing->file_path && - sn->file_path && strcmp(existing->file_path, sn->file_path) == 0 && - existing->start_line == sn->start_line && existing->name && sn->name && - strcmp(existing->name, sn->name) == 0; - bool sn_wins = true; - if (!same_entity) { - int c = strcmp(sn->label ? sn->label : "", existing->label ? existing->label : ""); - if (c == 0) { - c = strcmp(sn->file_path ? sn->file_path : "", - existing->file_path ? existing->file_path : ""); - } - if (c == 0) { - c = sn->start_line - existing->start_line; - } - if (c == 0) { - c = strcmp(sn->name ? sn->name : "", existing->name ? existing->name : ""); - } - sn_wins = c < 0; + * cbm_gbuf_upsert_node exactly. Distinct source entities can share a + * QN (C: struct/function/macro with one name); unconditional "src + * wins" made the survivor depend on worker merge order, flickering + * the node set (and every downstream consumer) run to run. Winner = + * smallest file_path, then LARGEST start_line, then largest + * name/label — one total order, commutative, scheduling-free; a full + * tie is the same entity → refresh from src. */ + int c = strcmp(sn->file_path ? sn->file_path : "", + existing->file_path ? existing->file_path : ""); + if (c == 0) { + c = existing->start_line - sn->start_line; + } + if (c == 0) { + c = strcmp(existing->name ? existing->name : "", sn->name ? sn->name : ""); + } + if (c == 0) { + c = strcmp(existing->label ? existing->label : "", sn->label ? sn->label : ""); } + bool sn_wins = c <= 0; if (sn_wins) { /* Keep the secondary indexes consistent when the surviving * label/name changes (the old code left the node listed under its From 312185ca389594d19a35b830d9e50609fb09faa6 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Tue, 7 Jul 2026 00:58:05 +0200 Subject: [PATCH 2/2] fix(test): stop budget-env poisoning from the backpressure test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cbm_mem_init is init-once (guarded by g_initialized), so the backpressure test's setenv(CBM_MEM_BUDGET_MB=1) + re-init 'restore' was a silent no-op: whichever init ran first fixed the budget for the whole process. In the full runner this test's init won, locking a 1 MB budget that leaked into every later budget consumer — mem_over_budget_low_rss red on every platform, and all subsequent pipeline tests silently running with backpressure permanently engaged. Add cbm_mem_set_budget_for_tests(): writes the budget directly without touching the init guard. The test now saves the caller-visible budget, sets 1 MB via the hook, and restores before asserting. Env dance removed. Signed-off-by: Martin Vogel --- src/foundation/mem.c | 4 ++++ src/foundation/mem.h | 9 +++++++++ tests/test_pipeline.c | 18 ++++++++++++------ 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/foundation/mem.c b/src/foundation/mem.c index 7238847f2..405a1bdc2 100644 --- a/src/foundation/mem.c +++ b/src/foundation/mem.c @@ -234,6 +234,10 @@ size_t cbm_mem_budget(void) { return g_budget; } +void cbm_mem_set_budget_for_tests(size_t bytes) { + g_budget = bytes; +} + bool cbm_mem_over_budget(void) { size_t rss = cbm_mem_rss(); check_pressure(rss); diff --git a/src/foundation/mem.h b/src/foundation/mem.h index 8c23c508a..db170465a 100644 --- a/src/foundation/mem.h +++ b/src/foundation/mem.h @@ -29,6 +29,15 @@ size_t cbm_mem_peak_rss(void); /* Total budget in bytes. */ size_t cbm_mem_budget(void); +/* TEST HOOK: overwrite the budget directly, bypassing cbm_mem_init's + * init-once guard (a setenv+re-init dance in tests is a silent no-op once + * some earlier init won the guard — the poisoned budget then leaks into + * every later budget consumer in the process). Does NOT flip the init + * guard: a later cbm_mem_init still initializes normally. Callers must + * save cbm_mem_budget() first and restore it before their assertions. + * Never call from production code. */ +void cbm_mem_set_budget_for_tests(size_t bytes); + /* Returns true if current RSS exceeds the budget. */ bool cbm_mem_over_budget(void); diff --git a/tests/test_pipeline.c b/tests/test_pipeline.c index 9d35e33d1..fc35928a7 100644 --- a/tests/test_pipeline.c +++ b/tests/test_pipeline.c @@ -6577,9 +6577,16 @@ TEST(pipeline_backpressure_futile_nap_disengages) { fclose(f); } - /* 1 MB budget: over-budget on every pull, unreclaimable by napping. */ - cbm_setenv("CBM_MEM_BUDGET_MB", "1", 1); - cbm_mem_init(0); + /* 1 MB budget: over-budget on every pull, unreclaimable by napping. + * Set via the test hook, NOT setenv + cbm_mem_init: the init-once guard + * makes any re-init keep whatever budget the FIRST in-process init + * computed. The old env dance either failed to apply the 1 MB budget + * (some earlier test's init won the guard) or applied it permanently + * (this test's init won) — the "restore" re-init was then a silent + * no-op and the 1 MB budget leaked into every later budget consumer + * in the runner (mem_over_budget_low_rss went red suite-order-wide). */ + size_t saved_budget = cbm_mem_budget(); + cbm_mem_set_budget_for_tests((size_t)1024 * 1024); ASSERT_TRUE(cbm_mem_budget() > 0); ASSERT_TRUE(cbm_mem_over_budget()); @@ -6589,9 +6596,8 @@ TEST(pipeline_backpressure_futile_nap_disengages) { int rc = cbm_pipeline_run(p); long cycles = cbm_pp_bp_nap_cycles(); - /* Restore the tests' default budget-off state BEFORE asserting. */ - cbm_unsetenv("CBM_MEM_BUDGET_MB"); - cbm_mem_init(0); + /* Restore the caller-visible budget BEFORE asserting. */ + cbm_mem_set_budget_for_tests(saved_budget); cbm_pipeline_free(p); teardown_test_repo();