Skip to content

Commit d08fccd

Browse files
committed
fix(graph_buffer): merge duplicate-edge properties commutatively
confidence, strategy and via are not part of the edge dedup key (source:target:type), and the same logical edge is legitimately minted by more than one strategy carrying different values — pass_calls emits CALLS edges from both LSP resolution and registry-textual matching. cbm_gbuf_insert_edge replaced the stored blob unconditionally, and per-worker edge buffers merge in worker-slot order, so which strategy's attributes survived was a function of thread scheduling: the edge set stayed stable while its attributes flickered run to run. Replace the overwrite with a total order over the two candidates, which makes the result commutative and associative and therefore independent of arrival order: 1. higher "confidence" wins, which also enforces the intended precedence that an LSP-resolved call outranks a textual match; 2. on equal confidence, the lexicographically greater blob wins, giving a deterministic choice between otherwise equal candidates. An empty or absent incoming blob still never displaces stored properties. Adds three tests: order independence for the same pair of blobs inserted both ways, the higher-confidence winner regardless of arrival, and the existing empty-blob guard. The first two fail before this change. Signed-off-by: Andrew Studnicky <a.j.studnicky@gmail.com>
1 parent 9b9b6ff commit d08fccd

2 files changed

Lines changed: 139 additions & 2 deletions

File tree

‎src/graph_buffer/graph_buffer.c‎

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@ enum {
2222
GB_MIN_FOR_DEDUP = 2, /* need at least 2 vectors to sort+dedup */
2323
GB_DEDUP_LOOKAHEAD = 1, /* compare current with next element */
2424
};
25+
26+
/* Sentinel for an edge property blob carrying no parseable "confidence": any
27+
* blob that states one outranks a blob that does not. */
28+
#define CBM_EDGE_CONF_ABSENT (-1.0)
2529
#include "graph_buffer/graph_buffer.h"
2630
#include <yyjson/yyjson.h> // url_path extraction must match json_extract semantics
2731
#include "store/store.h"
@@ -1029,6 +1033,53 @@ void cbm_gbuf_foreach_edge(const cbm_gbuf_t *gb, cbm_gbuf_edge_visitor_fn fn, vo
10291033

10301034
/* ── Edge operations ─────────────────────────────────────────────── */
10311035

1036+
/* Read "confidence":<double> out of an edge property blob. Absent/unparseable
1037+
* reads as -1 so any edge that carries a confidence outranks one that does
1038+
* not. */
1039+
static double edge_props_confidence(const char *props_json) {
1040+
if (!props_json) {
1041+
return CBM_EDGE_CONF_ABSENT;
1042+
}
1043+
static const char conf_key[] = "\"confidence\":";
1044+
const char *p = strstr(props_json, conf_key);
1045+
if (!p) {
1046+
return CBM_EDGE_CONF_ABSENT;
1047+
}
1048+
return strtod(p + sizeof(conf_key) - SKIP_ONE, NULL);
1049+
}
1050+
1051+
/* Decide whether an incoming property blob replaces the stored one on a
1052+
* duplicate edge key.
1053+
*
1054+
* confidence/strategy/via are not part of the edge key, and the same logical
1055+
* edge is legitimately minted by more than one strategy carrying different
1056+
* values — LSP resolution and registry-textual matching both produce CALLS
1057+
* edges. Per-worker edge buffers merge in worker-slot order, so any rule that
1058+
* depends on arrival order makes the surviving attributes a function of thread
1059+
* scheduling.
1060+
*
1061+
* This rule is a total order over the two candidates, hence commutative and
1062+
* associative — the outcome is independent of arrival order:
1063+
* 1. higher "confidence" wins, which also enforces the intended precedence
1064+
* that an LSP-resolved call outranks a textual match;
1065+
* 2. on equal confidence, the lexicographically greater blob wins, giving a
1066+
* deterministic choice between otherwise equal candidates.
1067+
* An empty or absent incoming blob never displaces stored properties. */
1068+
static bool edge_props_should_replace(const char *existing_json, const char *incoming_json) {
1069+
if (!incoming_json || strcmp(incoming_json, "{}") == 0) {
1070+
return false;
1071+
}
1072+
if (!existing_json || strcmp(existing_json, "{}") == 0) {
1073+
return true;
1074+
}
1075+
double inc = edge_props_confidence(incoming_json);
1076+
double cur = edge_props_confidence(existing_json);
1077+
if (inc != cur) {
1078+
return inc > cur;
1079+
}
1080+
return strcmp(incoming_json, existing_json) > 0;
1081+
}
1082+
10321083
int64_t cbm_gbuf_insert_edge(cbm_gbuf_t *gb, int64_t source_id, int64_t target_id, const char *type,
10331084
const char *properties_json) {
10341085
if (!gb || !type) {
@@ -1041,8 +1092,7 @@ int64_t cbm_gbuf_insert_edge(cbm_gbuf_t *gb, int64_t source_id, int64_t target_i
10411092

10421093
cbm_gbuf_edge_t *existing = cbm_ht_get(gb->edge_by_key, key);
10431094
if (existing) {
1044-
/* Merge properties (just replace for now) */
1045-
if (properties_json && strcmp(properties_json, "{}") != 0) {
1095+
if (edge_props_should_replace(existing->properties_json, properties_json)) {
10461096
free(existing->properties_json);
10471097
existing->properties_json = heap_strdup(properties_json);
10481098
}

‎tests/test_graph_buffer.c‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,88 @@ TEST(gbuf_edge_dedup) {
174174
PASS();
175175
}
176176

177+
/* Properties on a deduped edge must not depend on arrival order.
178+
*
179+
* confidence/strategy/via are not part of the edge key, and the same logical
180+
* CALLS edge is legitimately minted by two strategies (LSP resolution and
181+
* registry-textual matching) carrying different values. Per-worker edge buffers
182+
* merge in worker-slot order, so an arrival-order-dependent merge makes the
183+
* stored attributes a function of thread scheduling. Insert the same pair of
184+
* blobs in both orders; the stored properties must match. */
185+
TEST(gbuf_edge_props_merge_is_order_independent) {
186+
const char *lsp = "{\"callee\":\"f\",\"confidence\":0.95,\"strategy\":\"lsp\"}";
187+
const char *txt = "{\"callee\":\"f\",\"confidence\":0.40,\"strategy\":\"registry\"}";
188+
189+
cbm_gbuf_t *fwd = cbm_gbuf_new("test", "/tmp");
190+
int64_t a1 = cbm_gbuf_upsert_node(fwd, "Function", "a", "pkg.a", "f.go", 1, 5, "{}");
191+
int64_t b1 = cbm_gbuf_upsert_node(fwd, "Function", "b", "pkg.b", "f.go", 6, 10, "{}");
192+
cbm_gbuf_insert_edge(fwd, a1, b1, "CALLS", lsp);
193+
cbm_gbuf_insert_edge(fwd, a1, b1, "CALLS", txt);
194+
195+
cbm_gbuf_t *rev = cbm_gbuf_new("test", "/tmp");
196+
int64_t a2 = cbm_gbuf_upsert_node(rev, "Function", "a", "pkg.a", "f.go", 1, 5, "{}");
197+
int64_t b2 = cbm_gbuf_upsert_node(rev, "Function", "b", "pkg.b", "f.go", 6, 10, "{}");
198+
cbm_gbuf_insert_edge(rev, a2, b2, "CALLS", txt);
199+
cbm_gbuf_insert_edge(rev, a2, b2, "CALLS", lsp);
200+
201+
const cbm_gbuf_edge_t **fe = NULL;
202+
const cbm_gbuf_edge_t **re = NULL;
203+
int fc = 0;
204+
int rc = 0;
205+
cbm_gbuf_find_edges_by_type(fwd, "CALLS", &fe, &fc);
206+
cbm_gbuf_find_edges_by_type(rev, "CALLS", &re, &rc);
207+
ASSERT_EQ(fc, 1);
208+
ASSERT_EQ(rc, 1);
209+
ASSERT_STR_EQ(fe[0]->properties_json, re[0]->properties_json);
210+
211+
cbm_gbuf_free(fwd);
212+
cbm_gbuf_free(rev);
213+
PASS();
214+
}
215+
216+
/* The order-independent winner is also the semantically right one: a
217+
* higher-confidence discovery outranks a lower-confidence one regardless of
218+
* which arrived first. */
219+
TEST(gbuf_edge_props_merge_prefers_higher_confidence) {
220+
const char *lsp = "{\"callee\":\"f\",\"confidence\":0.95,\"strategy\":\"lsp\"}";
221+
const char *txt = "{\"callee\":\"f\",\"confidence\":0.40,\"strategy\":\"registry\"}";
222+
223+
cbm_gbuf_t *gb = cbm_gbuf_new("test", "/tmp");
224+
int64_t a = cbm_gbuf_upsert_node(gb, "Function", "a", "pkg.a", "f.go", 1, 5, "{}");
225+
int64_t b = cbm_gbuf_upsert_node(gb, "Function", "b", "pkg.b", "f.go", 6, 10, "{}");
226+
cbm_gbuf_insert_edge(gb, a, b, "CALLS", lsp);
227+
cbm_gbuf_insert_edge(gb, a, b, "CALLS", txt); /* lower confidence, arrives last */
228+
229+
const cbm_gbuf_edge_t **edges = NULL;
230+
int count = 0;
231+
cbm_gbuf_find_edges_by_type(gb, "CALLS", &edges, &count);
232+
ASSERT_EQ(count, 1);
233+
ASSERT_TRUE(strstr(edges[0]->properties_json, "\"strategy\":\"lsp\"") != NULL);
234+
235+
cbm_gbuf_free(gb);
236+
PASS();
237+
}
238+
239+
/* An empty incoming blob must never displace real stored properties. */
240+
TEST(gbuf_edge_props_merge_keeps_existing_on_empty) {
241+
const char *lsp = "{\"callee\":\"f\",\"confidence\":0.95,\"strategy\":\"lsp\"}";
242+
243+
cbm_gbuf_t *gb = cbm_gbuf_new("test", "/tmp");
244+
int64_t a = cbm_gbuf_upsert_node(gb, "Function", "a", "pkg.a", "f.go", 1, 5, "{}");
245+
int64_t b = cbm_gbuf_upsert_node(gb, "Function", "b", "pkg.b", "f.go", 6, 10, "{}");
246+
cbm_gbuf_insert_edge(gb, a, b, "CALLS", lsp);
247+
cbm_gbuf_insert_edge(gb, a, b, "CALLS", "{}");
248+
249+
const cbm_gbuf_edge_t **edges = NULL;
250+
int count = 0;
251+
cbm_gbuf_find_edges_by_type(gb, "CALLS", &edges, &count);
252+
ASSERT_EQ(count, 1);
253+
ASSERT_TRUE(strstr(edges[0]->properties_json, "\"strategy\":\"lsp\"") != NULL);
254+
255+
cbm_gbuf_free(gb);
256+
PASS();
257+
}
258+
177259
/* #768: two named imports from the same specifier (same source, same target
178260
* file) must produce two distinct IMPORTS edges, keyed apart by local_name --
179261
* not collapse into one edge that silently drops whichever import lost the
@@ -1067,6 +1149,11 @@ SUITE(graph_buffer) {
10671149
RUN_TEST(gbuf_merge_into_store_preserves);
10681150
RUN_TEST(gbuf_flush_skips_orphan_edges);
10691151

1152+
/* Edge property merge determinism */
1153+
RUN_TEST(gbuf_edge_props_merge_is_order_independent);
1154+
RUN_TEST(gbuf_edge_props_merge_prefers_higher_confidence);
1155+
RUN_TEST(gbuf_edge_props_merge_keeps_existing_on_empty);
1156+
10701157
/* Shared ID tests */
10711158
RUN_TEST(gbuf_shared_ids_unique);
10721159
RUN_TEST(gbuf_shared_ids_null_fallback);

0 commit comments

Comments
 (0)