runtime: publish the vector index instead of guarding it
El SDK CI - dev / build-and-test (pull_request) Failing after 10m59s
El SDK CI - dev / build-and-test (pull_request) Failing after 10m59s
The crash (SIGTRAP in engram_activate -> eg_vindex_sync -> vindex_insert -> _realloc) had three read paths mutating five process-global statics. engram_activate, eg_knn_for_node (whose own comment says "No writes.") and engram_geo_reify_run_json all called eg_vindex_sync, which frees the index, reallocs the seen-map and inserts — on a read. Three moves, in decreasing order of how much they dissolve: 1. Misfiled scratch is not shared state. visited/visit_epoch/visited_cap were never owned by the index; they are one traversal's local, hoisted into struct VIndex as an allocation optimisation. They want neither a lock nor a capability nor a pool — just to go back in the call frame. Two concurrent READS stomped each other purely because of this. 2. const IS the capability. Once the scratch leaves the struct, search reads and nothing else, so vindex_search takes a const VIndex*. That is exactly what a capability-pointer ABI would have bought — a read path physically cannot call vindex_insert, enforced by the compiler on every future caller — for one qualifier instead of an ABI swept across hundreds of builtins. 3. What survives is publication, not ownership. HNSW insert is NOT an append: it rewires the neighbour links of already-existing elements and reallocs elems[], so the store's append-only property does not transfer to the index derived from it. eg_vindex_sync therefore splits into eg_vindex_maintain (exclusive, sole mutator) and eg_vindex_view (shared, returns const VIndex*). A read path may demand that a current snapshot exist — a request to the owner, not a mutation by the reader. Write-side owner: eg_vindex_note_embedded hooks the embedding-ASSIGNMENT sites rather than the append sites, because a node with no embedding cannot be in a vector index — embedding assignment is the event that owns index membership. One O(log n) insert, no O(node_count) presence scan. This also retires the "STALENESS (honest tradeoff)" note where a lazily-embedded older node stayed invisible to route_nearest/autoconnect until a full rebuild (the embed-gap #20 shape). Evidence. The existing harness conflated two hazards, which is why fixing half of it read as failure. Split into four: single (3000 vec, ASan+UBSan) clean -> clean readers (4 readers, no writer, TSan) RACE -> clean unsynchronized (writer+reader, bare) race -> race, expected forever published (owner + 4 readers) n/a -> clean, 3000/3000 landed RESULT: PASS. recall@10 = 0.9365 at ef_search=128 (gate >= 0.90); determinism byte-identical across two independent builds. The unsynchronized half is now permanently expected to race, deliberately: it is the executable proof that the boundary must live above the data structure, not inside it. fb32d15's guard is KEPT, correcting this design's own section 5. Measured, it guards TWO structures and only one was converted here: g->nodes/g->edges are realloc'd in place (el_runtime.c:7618,7629) and engram_activate_inner's embed-backfill writes n->emb through exactly such a borrowed pointer. Deleting the guard reintroduces a measured 11171->9579 edge loss. Its comment is narrowed to the RAM graph and the deletion precondition named. That corrects the ordering claim too: the residual is not one ABI that dissolves everything at once, it is a PROPERTY applied per structure. Residues evaporate in the order the property is applied, and a residue whose structure has not been converted must be left standing.
This commit is contained in:
@@ -1,16 +1,31 @@
|
||||
#!/usr/bin/env bash
|
||||
# run_vindex_concurrency_tests.sh — regression harness for the 2026-08-16 soul crash.
|
||||
#
|
||||
# Runs two halves. The PAIR is the point: the ASan control localises the defect to
|
||||
# concurrency rather than to HNSW logic. See test_vindex_concurrency.c for the full
|
||||
# story (SIGSEGV at ASCII address "gramNode", heap corruption in xzm_realloc, etc).
|
||||
# Four halves. The SET is the point: it separates two hazards the original two-half
|
||||
# version conflated, and which have fixes in different files.
|
||||
#
|
||||
# 1. single — ASan+UBSan, one thread. MUST be clean. Always a hard failure.
|
||||
# 2. concurrent — TSan, writer + reader on one index. Currently EXPECTED to report a
|
||||
# race at visited_reset, because VIndex still owns its visited[] +
|
||||
# visit_epoch scratch. Once that moves to a per-query checkout pool
|
||||
# this must go clean; flip EXPECT_RACE=0 then and it becomes a real
|
||||
# regression gate.
|
||||
# 1. single ASan+UBSan, one thread. MUST be clean. Hard failure.
|
||||
#
|
||||
# 2. readers TSan, N readers, NO writer. Hazard (a): the visited set used
|
||||
# to live on the index, so two pure READS stamped each other's
|
||||
# epoch. Fixed in engram_vindex.c (frame-owned VVisit +
|
||||
# `const VIndex*` search). MUST be clean. Hard failure.
|
||||
#
|
||||
# 3. unsynchronized TSan, writer + reader on a BARE index. Hazard (b): in-place
|
||||
# HNSW insert rewires existing elements' neighbour lists and
|
||||
# reallocs elems[]. EXPECTED TO RACE, PERMANENTLY. This is not
|
||||
# a bug to fix inside engram_vindex.c — it is the executable
|
||||
# proof that a publication boundary must exist above it.
|
||||
# Not a failure. If it ever goes CLEAN, the test stopped
|
||||
# interleaving and half 4 is no longer meaningful either.
|
||||
#
|
||||
# 4. published TSan, owner + N readers through a publication boundary
|
||||
# (rwlock: readers shared, owner exclusive) mirroring
|
||||
# eg_vindex_view / eg_vindex_maintain in lang/runtime/el_runtime.c.
|
||||
# MUST be clean, and all inserts must land. Hard failure.
|
||||
#
|
||||
# See test_vindex_concurrency.c for the full story (SIGSEGV at ASCII address
|
||||
# "gramNode", heap corruption in xzm_realloc, etc).
|
||||
#
|
||||
# usage: run_vindex_concurrency_tests.sh
|
||||
set -uo pipefail
|
||||
@@ -23,12 +38,9 @@ trap 'rm -rf "$WORK"' EXIT
|
||||
SRC="$HERE/test_vindex_concurrency.c"
|
||||
VINDEX="$RUNTIME/engram_vindex.c"
|
||||
|
||||
# Flip to 0 once the visited set is per-query; the concurrent half then becomes a gate.
|
||||
EXPECT_RACE="${EXPECT_RACE:-1}"
|
||||
|
||||
fail=0
|
||||
|
||||
echo "== [1/2] single-threaded control under AddressSanitizer =="
|
||||
echo "== [1/4] single-threaded control under AddressSanitizer =="
|
||||
cc -std=c11 -g -O1 -fsanitize=address,undefined -fno-omit-frame-pointer \
|
||||
-I"$RUNTIME" -o "$WORK/single" "$SRC" "$VINDEX" -lm || { echo "BUILD FAILED"; exit 2; }
|
||||
if ASAN_OPTIONS=detect_leaks=0 "$WORK/single" single; then
|
||||
@@ -40,30 +52,54 @@ else
|
||||
fail=1
|
||||
fi
|
||||
|
||||
echo
|
||||
echo "== [2/2] concurrent writer+reader under ThreadSanitizer =="
|
||||
cc -std=c11 -g -O1 -fsanitize=thread -fno-omit-frame-pointer \
|
||||
-I"$RUNTIME" -o "$WORK/conc" "$SRC" "$VINDEX" -lm || { echo "BUILD FAILED"; exit 2; }
|
||||
|
||||
tsan_log="$WORK/tsan.log"
|
||||
TSAN_OPTIONS="halt_on_error=0" "$WORK/conc" concurrent >"$tsan_log" 2>&1
|
||||
if grep -q "ThreadSanitizer: data race" "$tsan_log"; then
|
||||
echo " -> RACE DETECTED:"
|
||||
grep -m1 -A6 "ThreadSanitizer: data race" "$tsan_log" | sed 's/^/ /'
|
||||
if [ "$EXPECT_RACE" = "1" ]; then
|
||||
echo " -> EXPECTED (VIndex still owns the shared visited set). Not a failure yet."
|
||||
echo " Fix = per-query visited buffer (hnswlib VisitedListPool style), then"
|
||||
echo " re-run with EXPECT_RACE=0."
|
||||
else
|
||||
echo " -> REGRESSION: the visited set was supposed to be per-query."
|
||||
fail=1
|
||||
fi
|
||||
# run_tsan <mode> <logfile>; echoes nothing, sets $tsan_raced
|
||||
run_tsan() {
|
||||
TSAN_OPTIONS="halt_on_error=0" "$WORK/conc" "$1" >"$2" 2>&1
|
||||
tsan_rc=$?
|
||||
if grep -q "ThreadSanitizer: data race" "$2"; then tsan_raced=1; else tsan_raced=0; fi
|
||||
}
|
||||
|
||||
echo
|
||||
echo "== [2/4] concurrent READERS, no writer (visited-set gate) =="
|
||||
run_tsan readers "$WORK/readers.log"
|
||||
if [ "$tsan_raced" = "1" ]; then
|
||||
echo " -> REGRESSION: two concurrent reads still race."
|
||||
grep -m1 -A6 "ThreadSanitizer: data race" "$WORK/readers.log" | sed 's/^/ /'
|
||||
echo " The visited set was supposed to be owned by the call frame."
|
||||
fail=1
|
||||
else
|
||||
echo " -> clean"
|
||||
if [ "$EXPECT_RACE" = "1" ]; then
|
||||
echo " -> NOTE: no race reported, but EXPECT_RACE=1. Either the fix landed"
|
||||
echo " (set EXPECT_RACE=0) or the test did not actually interleave."
|
||||
fi
|
||||
echo " -> clean (concurrent reads are safe)"
|
||||
fi
|
||||
|
||||
echo
|
||||
echo "== [3/4] writer+reader on a BARE index (expected-race probe) =="
|
||||
run_tsan unsynchronized "$WORK/unsync.log"
|
||||
if [ "$tsan_raced" = "1" ]; then
|
||||
echo " -> RACE DETECTED, as expected:"
|
||||
grep -m1 -A4 "ThreadSanitizer: data race" "$WORK/unsync.log" | sed 's/^/ /'
|
||||
echo " In-place HNSW insert mutates existing elements. Not fixable inside"
|
||||
echo " engram_vindex.c — this is why the publication boundary exists."
|
||||
else
|
||||
echo " -> NOTE: no race reported. The probe did not interleave; half 4's"
|
||||
echo " clean result proves less than it should. Investigate."
|
||||
fi
|
||||
|
||||
echo
|
||||
echo "== [4/4] owner+readers through the publication boundary (boundary gate) =="
|
||||
run_tsan published "$WORK/pub.log"
|
||||
if [ "$tsan_raced" = "1" ]; then
|
||||
echo " -> REGRESSION: the publication boundary did not serialize the owner."
|
||||
grep -m1 -A6 "ThreadSanitizer: data race" "$WORK/pub.log" | sed 's/^/ /'
|
||||
fail=1
|
||||
elif [ "$tsan_rc" != "0" ]; then
|
||||
echo " -> FAIL: boundary clean under TSan but the run failed:"
|
||||
tail -3 "$WORK/pub.log" | sed 's/^/ /'
|
||||
fail=1
|
||||
else
|
||||
echo " -> clean (readers project concurrently; the owner's inserts all landed)"
|
||||
fi
|
||||
|
||||
echo
|
||||
|
||||
@@ -15,22 +15,45 @@
|
||||
* Neither hnswlib nor FAISS puts the visited set on the index: hnswlib checks one
|
||||
* out of a VisitedListPool per query, FAISS uses a thread_local VisitedTable.
|
||||
*
|
||||
* THIS TEST HAS TWO HALVES, and they must BOTH be run — the pair is what localises
|
||||
* the bug to concurrency rather than to HNSW logic:
|
||||
* THE ORIGINAL `concurrent` HALF CONFLATED TWO DISTINCT HAZARDS (2026-08-16). It ran
|
||||
* a writer against a reader on one bare index, so it could not tell apart:
|
||||
*
|
||||
* single Insert N clustered vectors on ONE thread and search. Build with ASan.
|
||||
* This is the CONTROL. It must stay clean. When this passes and `concurrent`
|
||||
* fails, the defect is concurrency, not an out-of-bounds/logic error in the
|
||||
* graph code. (On 2026-08-16 this control cleared all 13,820 real dim-768
|
||||
* store vectors under ASan, which DISPROVED an inspection-derived hypothesis
|
||||
* about an out-of-bounds reverse-link write at engram_vindex.c:340.)
|
||||
* (a) READ/READ corruption — two searches stamping each other's visited epoch.
|
||||
* A defect INSIDE engram_vindex.c, fixable there, and now fixed: the visited
|
||||
* set moved to the call frame and vindex_search takes a `const VIndex*`.
|
||||
*
|
||||
* concurrent One writer thread inserting while a reader thread searches the SAME
|
||||
* index. Build with TSan. Until the visited set moves off the index struct
|
||||
* this is EXPECTED TO REPORT A RACE at engram_vindex.c visited_reset —
|
||||
* that is the bug, reproduced. Once a per-query visited buffer lands
|
||||
* (see backlog: "Move VIndex visited-set off the index struct"), this must
|
||||
* become clean, and THAT is the regression this file guards.
|
||||
* (b) WRITE/READ corruption — vindex_insert rewires the neighbour lists of
|
||||
* EXISTING elements and reallocs elems[], so an insert is a mutation of the
|
||||
* whole structure. This is NOT fixable inside engram_vindex.c at any price:
|
||||
* it is inherent to in-place HNSW. It requires a publication boundary ABOVE
|
||||
* the data structure (el_runtime.c: eg_vindex_view / eg_vindex_maintain).
|
||||
*
|
||||
* Conflating them made the suite unfailable-then-unpassable: fixing (a) left (b)
|
||||
* still racing, which reads as "the fix did not work" when in fact a different,
|
||||
* correctly-located fix is what (b) needs. So the halves are now separate:
|
||||
*
|
||||
* single N clustered vectors, ONE thread, ASan. The CONTROL. Must always
|
||||
* be clean. When this passes and a concurrent half fails, the defect
|
||||
* is concurrency, not an out-of-bounds/logic error in the graph code.
|
||||
* (On 2026-08-16 this control cleared all 13,820 real dim-768 store
|
||||
* vectors under ASan, which DISPROVED an inspection-derived hypothesis
|
||||
* about an out-of-bounds reverse-link write at engram_vindex.c:340.)
|
||||
*
|
||||
* readers N reader threads, NO writer, one shared index, TSan. This is
|
||||
* hazard (a) in isolation. It RACED before the visited set moved off
|
||||
* the index struct and must be CLEAN now. Hard gate.
|
||||
*
|
||||
* unsynchronized writer + reader on a bare index, TSan. Hazard (b) in isolation.
|
||||
* EXPECTED TO RACE, permanently — it is the executable proof that
|
||||
* the index cannot be made safe from the inside, and therefore that
|
||||
* the publication boundary in el_runtime.c has to exist. If this
|
||||
* ever goes clean, the test stopped interleaving; do not celebrate.
|
||||
*
|
||||
* published writer + readers through a publication boundary that mirrors
|
||||
* eg_vindex_view / eg_vindex_maintain (rwlock: readers shared,
|
||||
* the single owner exclusive), TSan. Must be CLEAN. Hard gate.
|
||||
* This is what proves the shape of the runtime fix, in the same
|
||||
* process, rather than asserting it.
|
||||
*
|
||||
* Absence of a crash does NOT mean absence of a race — always read the sanitizer
|
||||
* verdict, never just the exit code.
|
||||
@@ -104,23 +127,108 @@ static int run_single(void) {
|
||||
return 0;
|
||||
}
|
||||
|
||||
static int run_concurrent(void) {
|
||||
printf("[concurrent] 1 writer + 1 reader on ONE shared index (TSan probe)\n");
|
||||
/* Hazard (b) in isolation: writer + reader on a BARE index, no boundary. */
|
||||
static int run_unsynchronized(void) {
|
||||
printf("[unsynchronized] 1 writer + 1 reader on a BARE index (TSan probe)\n");
|
||||
printf("[unsynchronized] a race here is EXPECTED and PERMANENT — in-place HNSW\n");
|
||||
printf("[unsynchronized] insert rewires existing elements. This is the proof that\n");
|
||||
printf("[unsynchronized] the publication boundary must live ABOVE engram_vindex.c.\n");
|
||||
g_ix = vindex_create(DIM, 0, 0);
|
||||
if (!g_ix) { fprintf(stderr, "[concurrent] vindex_create failed\n"); return 1; }
|
||||
if (!g_ix) { fprintf(stderr, "[unsynchronized] vindex_create failed\n"); return 1; }
|
||||
for (int i = 0; i < SEED_N; i++)
|
||||
(void)vindex_insert(g_ix, (uint64_t)i, g_vecs + (size_t)i * DIM);
|
||||
|
||||
pthread_t w, r;
|
||||
if (pthread_create(&w, NULL, writer_fn, NULL) ||
|
||||
pthread_create(&r, NULL, reader_fn, NULL)) {
|
||||
fprintf(stderr, "[concurrent] pthread_create failed\n"); return 1;
|
||||
fprintf(stderr, "[unsynchronized] pthread_create failed\n"); return 1;
|
||||
}
|
||||
pthread_join(w, NULL);
|
||||
pthread_join(r, NULL);
|
||||
vindex_free(g_ix); g_ix = NULL;
|
||||
printf("[concurrent] completed — CHECK THE SANITIZER VERDICT, not this line.\n");
|
||||
printf("[concurrent] a clean TSan run here is the actual pass condition.\n");
|
||||
printf("[unsynchronized] completed — CHECK THE SANITIZER VERDICT, not this line.\n");
|
||||
return 0;
|
||||
}
|
||||
|
||||
/* ── hazard (a) in isolation: concurrent READS only ───────────────────────────
|
||||
* This is what the frame-owned visited set fixes. Before that change, two
|
||||
* vindex_search calls on one index wrote each other's epoch stamp; TSan reported
|
||||
* the race at visited_reset and the traversal then walked bogus element indices. */
|
||||
#define NREADERS 4
|
||||
|
||||
static int run_readers(void) {
|
||||
printf("[readers] %d concurrent readers, NO writer, one shared index (TSan)\n", NREADERS);
|
||||
printf("[readers] this is the visited-set regression gate — must be CLEAN.\n");
|
||||
g_ix = vindex_create(DIM, 0, 0);
|
||||
if (!g_ix) { fprintf(stderr, "[readers] vindex_create failed\n"); return 1; }
|
||||
for (int i = 0; i < NVEC; i++)
|
||||
(void)vindex_insert(g_ix, (uint64_t)i, g_vecs + (size_t)i * DIM);
|
||||
|
||||
pthread_t t[NREADERS];
|
||||
for (int i = 0; i < NREADERS; i++)
|
||||
if (pthread_create(&t[i], NULL, reader_fn, NULL)) {
|
||||
fprintf(stderr, "[readers] pthread_create failed\n"); return 1;
|
||||
}
|
||||
for (int i = 0; i < NREADERS; i++) pthread_join(t[i], NULL);
|
||||
vindex_free(g_ix); g_ix = NULL;
|
||||
printf("[readers] completed — CHECK THE SANITIZER VERDICT, not this line.\n");
|
||||
return 0;
|
||||
}
|
||||
|
||||
/* ── the publication boundary, mirroring el_runtime.c ─────────────────────────
|
||||
* Readers take the boundary SHARED and hold it across the whole search; the one
|
||||
* owner takes it EXCLUSIVE to extend. Same shape as eg_vindex_view /
|
||||
* eg_vindex_maintain. Note the reader's index pointer is `const VIndex*` — the
|
||||
* compiler, not this comment, is what stops a reader inserting. */
|
||||
static pthread_rwlock_t g_pub = PTHREAD_RWLOCK_INITIALIZER;
|
||||
|
||||
static void* pub_writer_fn(void* arg) {
|
||||
(void)arg;
|
||||
for (int i = SEED_N; i < NVEC; i++) {
|
||||
pthread_rwlock_wrlock(&g_pub);
|
||||
(void)vindex_insert(g_ix, (uint64_t)i, g_vecs + (size_t)i * DIM);
|
||||
pthread_rwlock_unlock(&g_pub);
|
||||
}
|
||||
return NULL;
|
||||
}
|
||||
|
||||
static void* pub_reader_fn(void* arg) {
|
||||
(void)arg;
|
||||
uint64_t ids[8]; float ds[8];
|
||||
for (int i = 0; i < 5000; i++) {
|
||||
pthread_rwlock_rdlock(&g_pub);
|
||||
const VIndex* view = g_ix; /* immutable view */
|
||||
(void)vindex_search(view, g_vecs + (size_t)(i % NVEC) * DIM, 8, 0, ids, ds);
|
||||
pthread_rwlock_unlock(&g_pub);
|
||||
}
|
||||
return NULL;
|
||||
}
|
||||
|
||||
static int run_published(void) {
|
||||
printf("[published] 1 owner + %d readers through a publication boundary (TSan)\n", NREADERS);
|
||||
printf("[published] this is the eg_vindex_view/eg_vindex_maintain gate — must be CLEAN.\n");
|
||||
g_ix = vindex_create(DIM, 0, 0);
|
||||
if (!g_ix) { fprintf(stderr, "[published] vindex_create failed\n"); return 1; }
|
||||
for (int i = 0; i < SEED_N; i++)
|
||||
(void)vindex_insert(g_ix, (uint64_t)i, g_vecs + (size_t)i * DIM);
|
||||
|
||||
pthread_t w, r[NREADERS];
|
||||
if (pthread_create(&w, NULL, pub_writer_fn, NULL)) {
|
||||
fprintf(stderr, "[published] pthread_create failed\n"); return 1;
|
||||
}
|
||||
for (int i = 0; i < NREADERS; i++)
|
||||
if (pthread_create(&r[i], NULL, pub_reader_fn, NULL)) {
|
||||
fprintf(stderr, "[published] pthread_create failed\n"); return 1;
|
||||
}
|
||||
pthread_join(w, NULL);
|
||||
for (int i = 0; i < NREADERS; i++) pthread_join(r[i], NULL);
|
||||
if (vindex_size(g_ix) != (size_t)NVEC) {
|
||||
fprintf(stderr, "[published] size %zu != %d — the owner lost inserts\n",
|
||||
vindex_size(g_ix), NVEC);
|
||||
vindex_free(g_ix); g_ix = NULL; return 1;
|
||||
}
|
||||
vindex_free(g_ix); g_ix = NULL;
|
||||
printf("[published] all %d inserts landed; CHECK THE SANITIZER VERDICT too.\n", NVEC);
|
||||
return 0;
|
||||
}
|
||||
|
||||
@@ -128,9 +236,16 @@ int main(int argc, char** argv) {
|
||||
const char* mode = (argc > 1) ? argv[1] : "single";
|
||||
fill_vectors();
|
||||
int rc;
|
||||
if (!strcmp(mode, "single")) rc = run_single();
|
||||
else if (!strcmp(mode, "concurrent")) rc = run_concurrent();
|
||||
else { fprintf(stderr, "usage: %s [single|concurrent]\n", argv[0]); rc = 2; }
|
||||
if (!strcmp(mode, "single")) rc = run_single();
|
||||
else if (!strcmp(mode, "readers")) rc = run_readers();
|
||||
else if (!strcmp(mode, "unsynchronized")) rc = run_unsynchronized();
|
||||
else if (!strcmp(mode, "published")) rc = run_published();
|
||||
/* back-compat: the pre-split name meant the bare writer+reader probe. */
|
||||
else if (!strcmp(mode, "concurrent")) rc = run_unsynchronized();
|
||||
else {
|
||||
fprintf(stderr, "usage: %s [single|readers|unsynchronized|published]\n", argv[0]);
|
||||
rc = 2;
|
||||
}
|
||||
free(g_vecs);
|
||||
return rc;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user