mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-06 02:49:56 +00:00
fix(analyze): correct the numbers feeding the graph-write-collapse guard
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) <noreply@anthropic.com>
This commit is contained in:
parent
2ed6504dbd
commit
6df6fb8501
7 changed files with 221 additions and 20 deletions
|
|
@ -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`,
|
||||
|
|
|
|||
|
|
@ -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 };
|
||||
|
|
|
|||
|
|
@ -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 };
|
||||
|
|
|
|||
|
|
@ -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<string> | 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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
};
|
||||
}
|
||||
|
|
|
|||
81
gitnexus/test/unit/graph-collapse-wiring.test.ts
Normal file
81
gitnexus/test/unit/graph-collapse-wiring.test.ts
Normal file
|
|
@ -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();
|
||||
});
|
||||
});
|
||||
|
|
@ -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', () => {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue