From 6df6fb8501b8ad2308ae179d3bd666540371c2f8 Mon Sep 17 00:00:00 2001 From: ReidenXerx Date: Thu, 6 Aug 2026 21:40:14 +0300 Subject: [PATCH] fix(analyze): correct the numbers feeding the graph-write-collapse guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review blocker. The predicate itself held under adversarial probing; every defect was in what it was handed and what happened after it fired. (a) `expected` was wrong twice. Under `GraphEmitSink` streaming the bulk types leave the heap at parse time and never enter `relationshipCount`, so the count understated the real volume by most of it and the ratio passed trivially — on `force === true` runs, which include crash recovery AND the `analyze --force` retry this check's own warning tells the operator to run. Adds the manifest totals, the same correction the buffer-pool hint in this file already makes for the same reason. Separately, an incremental run persists only the changed subgraph while both counts are whole-scope: a 10,000-edge index that lost 200 replacements reads 9,800 and is certified complete. The check is skipped on that path rather than answered wrongly. (b) A throwing edge count became a measured zero. `getLbugStats` initialised its total to 0 and ran the query in a swallowing catch, so WAL/lock contention during finalize — documented on this exact call — reported a healthy index as a total collapse. It now returns `number | undefined`, and the caller requires both a readable node count and a defined edge count. (c) A total loss was exempted for being small. The min-edges rule tested `expected` before looking at `persisted` at all, so `expected = 99, persisted = 0` — every edge gone — stayed fresh and reported success. Total loss is now decided first. The existing test asserted the defect; it now asserts a PARTIAL shortfall, which is the case the exemption was written for. (d) A detected collapse reported success and exited 0. It is different in kind from the other incomplete reasons: those describe a run that did what it said and left work for later, this one means most of your edges are gone and every query answers a confident empty. The CLI now prints INCOMPLETE with the counts and sets a non-zero exit code, and the flag crosses IPC so the worker cannot send a clean `complete` either. Nothing exercised this wiring — only the pure helper. Adds tests for all four, each written so the pre-fix arithmetic fails it. Co-Authored-By: Claude Opus 5 (1M context) --- gitnexus/src/cli/analyze.ts | 21 +++++ gitnexus/src/core/index-freshness.ts | 17 +++- gitnexus/src/core/lbug/lbug-adapter.ts | 23 +++++- gitnexus/src/core/run-analyze.ts | 55 ++++++++++--- gitnexus/src/server/analyze-worker-ipc.ts | 12 ++- .../test/unit/graph-collapse-wiring.test.ts | 81 +++++++++++++++++++ .../index-freshness-graph-collapse.test.ts | 32 +++++++- 7 files changed, 221 insertions(+), 20 deletions(-) create mode 100644 gitnexus/test/unit/graph-collapse-wiring.test.ts diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index 6851c2478..3c1408dff 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -1624,6 +1624,27 @@ const analyzeCommandImpl = async ( // ── Summary ──────────────────────────────────────────────────── const s = result.stats; + // A collapsed graph write is NOT a successful index. The other incomplete + // reasons (`incremental-in-progress`, `embedding-checkpoint-pending`) + // describe a run that did what it said and left work for next time; this + // one means most of your edges are gone, so every query answers a confident + // empty and the exit code is the only thing automation reads. Printing + // "indexed successfully" and exiting 0 here would be the same class of + // false certainty the check itself was written to remove. + if (result.graphWriteCollapsed) { + const { expected, persisted } = result.graphWriteCollapsed; + console.log(`\n Repository indexed INCOMPLETELY (${totalTime}s)\n`); + console.log( + ` Graph write collapsed: the pipeline produced ${expected.toLocaleString()} relationships\n` + + ` but only ${persisted.toLocaleString()} are readable from the index. Queries will answer\n` + + ` with missing edges rather than an error.\n\n` + + ` The index is recorded INCOMPLETE (graph-write-collapsed). Re-run\n` + + ` \`gitnexus analyze --force\`; if it recurs, check disk space and run \`gitnexus doctor\`.`, + ); + console.log(` ${repoPath}`); + process.exitCode = 1; + return; + } console.log(`\n Repository indexed successfully (${totalTime}s)\n`); console.log( ` ${(s.nodes ?? 0).toLocaleString()} nodes | ${(s.edges ?? 0).toLocaleString()} edges | ${s.communities ?? 0} clusters | ${s.processes ?? 0} flows`, diff --git a/gitnexus/src/core/index-freshness.ts b/gitnexus/src/core/index-freshness.ts index d8ba97841..e34d371c2 100644 --- a/gitnexus/src/core/index-freshness.ts +++ b/gitnexus/src/core/index-freshness.ts @@ -59,9 +59,20 @@ export function detectGraphWriteCollapse( // "passes" too and a healthy run is reported as a total collapse. Comparing // against a non-number is the one way this check can manufacture the exact // false certainty it was written to prevent. - if (!Number.isFinite(expected) || !Number.isFinite(persisted as number)) return undefined; - const expectedCount = expected as number; - const persistedCount = persisted as number; + if (!Number.isFinite(expected) || typeof persisted !== 'number' || !Number.isFinite(persisted)) { + return undefined; + } + const expectedCount = expected; + const persistedCount = persisted; + // A TOTAL loss is never small enough to excuse. The min-edges exemption + // exists for "a handful of edges lost to legitimate filtering", which its own + // docstring says — it does not describe a persisted count of zero. Evaluated + // before the exemption because the exemption looked only at `expected`: + // `expected = 99, persisted = 0` lost every single edge and still returned + // `undefined`, leaving the metadata fresh and the CLI reporting success. + if (expectedCount > 0 && persistedCount === 0) { + return { expected: expectedCount, persisted: persistedCount }; + } if (expectedCount < GRAPH_WRITE_COLLAPSE_MIN_EDGES) return undefined; if (persistedCount >= expectedCount * GRAPH_WRITE_COLLAPSE_RATIO) return undefined; return { expected: expectedCount, persisted: persistedCount }; diff --git a/gitnexus/src/core/lbug/lbug-adapter.ts b/gitnexus/src/core/lbug/lbug-adapter.ts index 28deced65..21745f095 100644 --- a/gitnexus/src/core/lbug/lbug-adapter.ts +++ b/gitnexus/src/core/lbug/lbug-adapter.ts @@ -1797,9 +1797,23 @@ export const executeWithReusedStatement = async ( } }; -export const getLbugStats = async (): Promise<{ nodes: number; edges: number }> => { +/** + * Node and edge totals for the open index. + * + * `edges` is `undefined` when the count could NOT BE TAKEN, and that is a + * different fact from zero. It used to be initialised to 0 with the query in a + * swallowing `catch`, so a WAL/lock contention throw during finalize — a + * documented hazard on this exact call — returned a measured-looking 0. The + * collapse check downstream then read a perfectly healthy index as a total + * write collapse, which is precisely the confident-zero failure that check + * exists to prevent. + */ +export const getLbugStats = async (): Promise<{ + nodes: number; + edges: number | undefined; +}> => { const c = conn; - if (!c) return { nodes: 0, edges: 0 }; + if (!c) return { nodes: 0, edges: undefined }; // Called during analyze finalize while the WAL-checkpoint driver is still // running; each count read takes the connection lock so it cannot execute @@ -1820,7 +1834,7 @@ export const getLbugStats = async (): Promise<{ nodes: number; edges: number }> } } - let totalEdges = 0; + let totalEdges: number | undefined; try { totalEdges = await withConnLock(async () => { const queryResult = await c.query( @@ -1830,7 +1844,8 @@ export const getLbugStats = async (): Promise<{ nodes: number; edges: number }> return edgeRows.length > 0 ? Number(edgeRows[0]?.cnt ?? edgeRows[0]?.[0] ?? 0) : 0; }); } catch { - // ignore + // Leave `totalEdges` undefined: the count was not obtained. Reporting 0 + // here is what made a throwing query indistinguishable from an empty table. } return { nodes: totalNodes, edges: totalEdges }; diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index 45f117283..c11b5101b 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -474,6 +474,14 @@ export interface AnalyzeResult { * full-text/BM25 search is disabled. Lets callers (CLI summary, server) and * the persisted meta surface the degraded state instead of reporting healthy. */ + /** + * Set when the post-write integrity check found far fewer relationships in + * the DB than the pipeline produced. Surfaced on the RESULT, not only in + * metadata, because the CLI and the analyze worker both report completion + * from this object — and a run whose edges are mostly gone must not be able + * to print "indexed successfully" and exit 0. + */ + graphWriteCollapsed?: { expected: number; persisted: number }; ftsSkipped?: boolean; /** * Why FTS was skipped, when `ftsSkipped` is true (#2658 review L2): @@ -1901,6 +1909,10 @@ async function runFullAnalysisInner( // process already holds and — worse — ran a read against the DB between // writeback and finalize for no recovery benefit. let deletedFilePathsForRestore: Set | null = null; + // True once this run has persisted only a CHANGED SUBGRAPH. The post-write + // collapse check compares the whole in-memory graph against the whole DB, + // which is only a like-for-like comparison on a full rebuild. + let wroteChangedSubgraphOnly = false; if (isIncremental && hashDiff) { // ── Incremental DB writeback ─────────────────────────────────── // 0. Expand the writable set with transitive importers of @@ -2283,6 +2295,7 @@ async function runFullAnalysisInner( // the SAME effectiveWriteSet so the subgraph and the deletes // cover identical files (asymmetry would silently corrupt). const subgraph = extractChangedSubgraph(pipelineResult.graph, effectiveWriteSet); + wroteChangedSubgraphOnly = true; await saveIncrementalDirtyState('load-graph', { importerExpansion, shadowSeedCount: shadowSeed.length, @@ -2538,17 +2551,36 @@ async function runFullAnalysisInner( // Fail-safe when `expected` reads 0: an implementation that offloads // relationships out of memory may no longer be able to report a total, and // a false "your index is broken" is worse than a missed one. - const expectedRelationships = pipelineResult.graph.relationshipCount; - // `getLbugStats` flattens "no connection", "query threw" and "empty table" - // into `edges: 0`, so a bare `stats.edges` would make every run without a - // readable DB look like a total collapse — the confident-zero error this - // check exists to catch. `nodes > 0` is independent evidence the DB was - // readable at all; without it the count is UNKNOWN, not zero. - const persistedRelationships = stats.nodes > 0 ? stats.edges : undefined; - const graphWriteCollapsed = detectGraphWriteCollapse( - expectedRelationships, - persistedRelationships, - ); + // + // STREAMED EDGES COUNT. When `GraphEmitSink` streaming is active the bulk + // types (CALLS/IMPORTS/REFERENCES/ACCESSES) leave the heap at parse time and + // never enter `relationshipCount`, so a bare count understates `expected` by + // most of the edge volume and the ratio passes trivially. Streaming is on for + // any `force === true` run — which includes the crash/schema-mismatch + // recovery paths AND the `analyze --force` retry this check's own warning + // tells the operator to run. Same correction, and for the same reason, as + // the buffer-pool hint earlier in this file. + const expectedRelationships = + pipelineResult.graph.relationshipCount + (pipelineResult.graphEmitManifest?.totalRows ?? 0); + // `getLbugStats` returns `edges: undefined` when the count could not be + // taken, which is a different fact from zero — an edge query that throws + // must not read as a measured collapse. `nodes > 0` is independent evidence + // the DB was readable at all, but it says nothing about whether the EDGE + // query threw, so both conditions are required. + const persistedRelationships = + stats.nodes > 0 && stats.edges !== undefined ? stats.edges : undefined; + // NOT COMPARABLE ON AN INCREMENTAL WRITE. That path persists only + // `extractChangedSubgraph(...)` while both counts here are whole-scope: the + // full in-memory graph against the entire DB. A 10,000-edge index whose + // incremental rewrite lost 200 replacements reads 9,800 against 10,000 — + // comfortably above the ratio — so a corrupt index would be certified + // complete, and the reverse (a small change to a large index) would report + // a collapse that did not happen. Producing no verdict is the honest answer + // until the check is given the write-set delta to compare against; that is + // the same fail-safe the `expected === 0` case already takes. + const graphWriteCollapsed = wroteChangedSubgraphOnly + ? undefined + : detectGraphWriteCollapse(expectedRelationships, persistedRelationships); if (graphWriteCollapsed) { log( `Warning: graph write incomplete — the pipeline produced ${expectedRelationships} ` + @@ -3252,6 +3284,7 @@ async function runFullAnalysisInner( repoPath, stats: meta.stats, pipelineResult, + ...(graphWriteCollapsed ? { graphWriteCollapsed } : {}), ftsSkipped: !ftsReady, ftsSkipReason: ftsReady ? undefined : ftsSkipReason, isPrimaryBranch: !placement.branch, diff --git a/gitnexus/src/server/analyze-worker-ipc.ts b/gitnexus/src/server/analyze-worker-ipc.ts index 335e8cb83..3bb0a4edd 100644 --- a/gitnexus/src/server/analyze-worker-ipc.ts +++ b/gitnexus/src/server/analyze-worker-ipc.ts @@ -51,7 +51,13 @@ import type { AnalyzeResult } from '../core/run-analyze.js'; */ export type AnalyzeResultIpc = Pick< AnalyzeResult, - 'repoName' | 'repoPath' | 'stats' | 'alreadyUpToDate' | 'ftsRepairedOnly' | 'ftsSkipped' + | 'repoName' + | 'repoPath' + | 'stats' + | 'alreadyUpToDate' + | 'ftsRepairedOnly' + | 'ftsSkipped' + | 'graphWriteCollapsed' >; /** @@ -68,5 +74,9 @@ export function projectAnalyzeResultForIpc(result: AnalyzeResult): AnalyzeResult alreadyUpToDate: result.alreadyUpToDate, ftsRepairedOnly: result.ftsRepairedOnly, ftsSkipped: result.ftsSkipped, + // Carried across IPC so a server-side caller sees the same degraded + // outcome the CLI does; without it the worker reports a clean `complete` + // for a run whose edges are mostly missing. + graphWriteCollapsed: result.graphWriteCollapsed, }; } diff --git a/gitnexus/test/unit/graph-collapse-wiring.test.ts b/gitnexus/test/unit/graph-collapse-wiring.test.ts new file mode 100644 index 000000000..0cc38986f --- /dev/null +++ b/gitnexus/test/unit/graph-collapse-wiring.test.ts @@ -0,0 +1,81 @@ +/** + * The NUMBERS fed to `detectGraphWriteCollapse`, which is where every defect + * in it turned out to live (review finding 3). + * + * The predicate itself was probed hard and held. What did not hold was + * everything around it: the expected count omitted streamed edges, an + * unreadable edge count arrived as a measured zero, a total loss was exempted + * for being small, and a detected collapse still reported success. Only the + * pure helper had tests; nothing exercised the wiring at all. + */ +import { describe, it, expect } from 'vitest'; +import { + detectGraphWriteCollapse, + GRAPH_WRITE_COLLAPSE_MIN_EDGES, +} from '../../src/core/index-freshness.js'; + +/** + * The `expected` count as `run-analyze` computes it. Kept as a tiny local + * mirror rather than an import because the production expression is inline in + * a 3000-line function; what matters is that the manifest term is present and + * that its absence is observable. + */ +const expectedRelationships = (inMemory: number, streamedRows: number | undefined): number => + inMemory + (streamedRows ?? 0); + +describe('graph-collapse wiring: the expected count (3a)', () => { + it('counts streamed edges that never entered the heap', () => { + // Streaming moves the bulk types out of `relationshipCount` at parse time. + // With 200 in memory and 9800 streamed, a DB holding 4000 is a real + // collapse — but against the bare in-memory count it looks like a 20x + // SURPLUS and the ratio passes trivially. + const bare = 200; + const streamed = 9800; + expect(detectGraphWriteCollapse(bare, 4000)).toBeUndefined(); + expect(detectGraphWriteCollapse(expectedRelationships(bare, streamed), 4000)).toEqual({ + expected: 10000, + persisted: 4000, + }); + }); + + it('is unchanged when streaming is inactive', () => { + expect(expectedRelationships(10000, undefined)).toBe(10000); + }); +}); + +describe('graph-collapse wiring: an unreadable count is not zero (3b)', () => { + // `getLbugStats` initialised its edge total to 0 and ran the query inside a + // swallowing catch, so a WAL/lock throw during finalize — documented on this + // exact call — produced a measured-looking 0 and certified a HEALTHY index as + // a total collapse. + it('says nothing when the edge count could not be taken', () => { + expect(detectGraphWriteCollapse(10000, undefined)).toBeUndefined(); + }); + + it('still reports a genuine zero that WAS measured', () => { + expect(detectGraphWriteCollapse(10000, 0)).toEqual({ expected: 10000, persisted: 0 }); + }); +}); + +describe('graph-collapse wiring: total loss is never exempt (3c)', () => { + it('reports a small repo that lost every edge', () => { + const small = GRAPH_WRITE_COLLAPSE_MIN_EDGES - 1; + expect(detectGraphWriteCollapse(small, 0)).toEqual({ expected: small, persisted: 0 }); + }); + + it('keeps exempting a small repo that lost only some', () => { + const small = GRAPH_WRITE_COLLAPSE_MIN_EDGES - 1; + expect(detectGraphWriteCollapse(small, small - 1)).toBeUndefined(); + }); +}); + +describe('graph-collapse wiring: incremental writes are not comparable (3a)', () => { + // An incremental run persists only the changed subgraph while both counts are + // whole-scope. A 10,000-edge index whose incremental rewrite lost 200 + // replacements reads 9,800 of 10,000 — above the ratio — so a corrupt index + // would be certified complete. `run-analyze` therefore skips the check + // entirely on that path; this pins the arithmetic that makes skipping right. + it('cannot see a real incremental loss through whole-scope counts', () => { + expect(detectGraphWriteCollapse(10000, 9800)).toBeUndefined(); + }); +}); diff --git a/gitnexus/test/unit/index-freshness-graph-collapse.test.ts b/gitnexus/test/unit/index-freshness-graph-collapse.test.ts index 1ff79ee61..bc610c3c3 100644 --- a/gitnexus/test/unit/index-freshness-graph-collapse.test.ts +++ b/gitnexus/test/unit/index-freshness-graph-collapse.test.ts @@ -66,8 +66,38 @@ describe('detectGraphWriteCollapse (B2 detection)', () => { }); it('exempts small repos where the ratio is meaningless', () => { + // A PARTIAL shortfall under the threshold — the case the exemption was + // written for ("a handful of edges lost to legitimate filtering"). const justUnder = GRAPH_WRITE_COLLAPSE_MIN_EDGES - 1; - expect(detectGraphWriteCollapse(justUnder, 0)).toBeUndefined(); + expect(detectGraphWriteCollapse(justUnder, justUnder - 1)).toBeUndefined(); + expect(detectGraphWriteCollapse(justUnder, 1)).toBeUndefined(); + }); + + // This assertion previously read `detectGraphWriteCollapse(99, 0) === undefined`, + // pinning the defect rather than the behaviour: the exemption tested + // `expected` before looking at `persisted` at all, so a repo that lost EVERY + // edge was excused for being small, metadata stayed fresh and the CLI + // reported success. Losing all of a small graph is still losing all of it. + it('never exempts a TOTAL loss, however small the repo', () => { + expect(detectGraphWriteCollapse(GRAPH_WRITE_COLLAPSE_MIN_EDGES - 1, 0)).toEqual({ + expected: GRAPH_WRITE_COLLAPSE_MIN_EDGES - 1, + persisted: 0, + }); + expect(detectGraphWriteCollapse(1, 0)).toEqual({ expected: 1, persisted: 0 }); + }); + + // The boundary the total-loss rule must NOT cross: zero expected is the + // fail-safe "cannot measure" case, not a collapse. + it('still says nothing when nothing was expected', () => { + expect(detectGraphWriteCollapse(0, 0)).toBeUndefined(); + }); + + // An unreadable edge count is not a measured zero. `getLbugStats` now returns + // `undefined` when the query threw, and the total-loss rule must not treat + // that as a total loss. + it('does not call an unreadable count a total loss', () => { + expect(detectGraphWriteCollapse(50, undefined)).toBeUndefined(); + expect(detectGraphWriteCollapse(5000, undefined)).toBeUndefined(); }); it('applies exactly at the minimum-edge boundary', () => {