fix(incremental): bugbot review + CI test failures

Bugbot (PR #1479):
- Medium: pruneCache was exported but never called -> cache grew
  unbounded. Wire pruneCache into run-analyze before saveParseCache,
  using a transient usedKeys Set on ParseCache that the parse phase
  populates as it processes chunks.
- Low: willTryIncremental (pre-pipeline) and isIncremental
  (post-pipeline) could desync, silently dropping embeddings on
  mispredicted runs. Removed the prediction; the embedding cache
  now loads unconditionally when shouldLoadCache is true. The
  re-insert step gates on the actual isIncremental value to avoid
  PK-conflicts when the incremental-writeback path keeps DB rows.

CI test failures:
- cli-e2e #1169 + run-analyze.test.ts #1233: my dirty-tree gate on
  the lastCommit==HEAD early-return saw GitNexus's own auto-generated
  outputs (.claude/, .cursor/, AGENTS.md, CLAUDE.md) as dirty,
  perpetually defeating the up-to-date fast path. Extended the
  pathspec exclusion to cover all auto-gen outputs, not just
  .gitnexus/.
- ruby field-type disambig: my chunk-stability sort exposed a
  pre-existing order-dependency in Ruby cross-file resolution
  (`user.address.save -> Address#save` only resolves correctly when
  user.rb parses before address.rb in some configurations). Removed
  the sort. Filesystem ordering is stable enough in practice that
  the parse cache still hits the common case; the pre-existing
  fragility is left for a separate fix.
- pipeline-graph-golden: regenerated. Seeded Leiden RNG produces a
  partition different from the previous Math.random snapshot.
- staleness `parallel calls` was a CI timing flake; passes locally.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
abhigyanpatwari 2026-05-10 18:10:52 +05:30
parent 812e828362
commit 5eb05977e1
3 changed files with 94 additions and 32 deletions

View file

@ -163,13 +163,14 @@ export async function runChunkedParseAndResolve(
); );
} }
// Sort by path so chunk membership is stable across runs even when // We previously sorted parseableScanned alphabetically here for stable
// the filesystem returns scan order non-deterministically. Without // chunk membership across runs (so the parse cache wouldn't miss when
// this, the parse cache misses on every run because chunk boundaries // filesystem-scan order varied). Removed because it surfaced a
// shift even when no source file content has changed. Sort is // pre-existing order-dependency in Ruby cross-file resolution
// ascending alphabetical — the comparator works for both POSIX and // (`user.address.save → Address#save` resolution depends on file
// Windows path separators since both are `string` in JS. // processing order — a separate bug to fix). Filesystem order on most
parseableScanned.sort((a, b) => (a.path < b.path ? -1 : a.path > b.path ? 1 : 0)); // platforms is stable enough in practice that the cache still hits the
// common case; runs where it doesn't simply pay a cold-parse cost.
const totalParseable = parseableScanned.length; const totalParseable = parseableScanned.length;
@ -337,6 +338,11 @@ export async function runChunkedParseAndResolve(
let chunkWorkerData: WorkerExtractedData | null; let chunkWorkerData: WorkerExtractedData | null;
const cachedRaw = chunkHash ? parseCache!.entries.get(chunkHash) : undefined; const cachedRaw = chunkHash ? parseCache!.entries.get(chunkHash) : undefined;
// Track every chunk hash we touched so the orchestrator can
// prune stale entries (chunks whose composition no longer
// corresponds to a live chunk in the current scan) before saving.
if (parseCache && chunkHash) parseCache.usedKeys.add(chunkHash);
if (cachedRaw && cachedRaw.length > 0) { if (cachedRaw && cachedRaw.length > 0) {
// Cache hit: replay the cached worker output through the same // Cache hit: replay the cached worker output through the same
// merge logic the live worker path uses. // merge logic the live worker path uses.
@ -526,6 +532,12 @@ export async function runChunkedParseAndResolve(
astCache.clear(); astCache.clear();
} }
if (isDev && parseCache && (chunkCacheHits > 0 || chunkCacheMisses > 0)) {
logger.info(
`📦 parse-cache summary: ${chunkCacheHits} chunk hit(s), ${chunkCacheMisses} miss(es) across ${numChunks} chunk(s)`,
);
}
const fullWorkerHeritageMap = const fullWorkerHeritageMap =
deferredWorkerHeritage.length > 0 deferredWorkerHeritage.length > 0
? buildHeritageMap(deferredWorkerHeritage, ctx, getHeritageStrategyForLanguage) ? buildHeritageMap(deferredWorkerHeritage, ctx, getHeritageStrategyForLanguage)

View file

@ -36,7 +36,7 @@ import {
} from '../storage/repo-manager.js'; } from '../storage/repo-manager.js';
import { computeFileHashes, diffFileHashes } from '../storage/file-hash.js'; import { computeFileHashes, diffFileHashes } from '../storage/file-hash.js';
import { extractChangedSubgraph } from './incremental/subgraph-extract.js'; import { extractChangedSubgraph } from './incremental/subgraph-extract.js';
import { loadParseCache, saveParseCache } from '../storage/parse-cache.js'; import { loadParseCache, saveParseCache, pruneCache } from '../storage/parse-cache.js';
import { import {
getCurrentCommit, getCurrentCommit,
getRemoteUrl, getRemoteUrl,
@ -206,13 +206,38 @@ export async function runFullAnalysis(
// may have uncommitted changes. Only short-circuit when the working // may have uncommitted changes. Only short-circuit when the working
// tree is also clean — otherwise fall through to the incremental // tree is also clean — otherwise fall through to the incremental
// path which will hash-diff and update only changed files. // path which will hash-diff and update only changed files.
//
// We exclude paths that GitNexus itself writes during analyze:
// .gitnexus/ — db / parse cache / meta.json
// .claude/, .cursor/ — auto-generated agent skill files
// AGENTS.md, CLAUDE.md — auto-updated stats blocks
// Counting them as dirty would perpetually defeat the up-to-date
// fast path because the previous analyze just wrote them
// (regression vs PR #1233 behavior).
const dirty = (() => { const dirty = (() => {
try { try {
const out = execFileSync('git', ['status', '--porcelain'], { const out = execFileSync(
cwd: repoPath, 'git',
stdio: ['ignore', 'pipe', 'ignore'], [
encoding: 'utf8', 'status',
}); '--porcelain',
'--',
'.',
':(exclude).gitnexus',
':(exclude).gitnexus/**',
':(exclude).claude',
':(exclude).claude/**',
':(exclude).cursor',
':(exclude).cursor/**',
':(exclude)AGENTS.md',
':(exclude)CLAUDE.md',
],
{
cwd: repoPath,
stdio: ['ignore', 'pipe', 'ignore'],
encoding: 'utf8',
},
);
return out.trim().length > 0; return out.trim().length > 0;
} catch { } catch {
return true; // conservative on git failure return true; // conservative on git failure
@ -281,18 +306,15 @@ export async function runFullAnalysis(
); );
} }
// Predict whether this run will use the incremental DB-writeback path. // We *always* load the embedding cache when one is requested (regardless
// Used to suppress the embedding cache+restore cycle in the incremental // of the predicted `willTryIncremental`). The post-pipeline branch may
// case (embeddings stay in the DB; re-inserting them would PK-conflict). // disagree with the prediction (e.g. when the pipeline produces zero
const willTryIncremental = // File nodes, `isIncremental` flips false and the full-rebuild path
!options.force && // wipes the DB) — loading unconditionally is cheap insurance against
!!existingMeta && // silently dropping embeddings on a mispredicted run. The re-insert
existingMeta.schemaVersion === INCREMENTAL_SCHEMA_VERSION && // step gates itself on the actual `isIncremental` value to avoid
!!existingMeta.fileHashes && // PK-conflicts when the incremental writeback path keeps the rows.
Object.keys(existingMeta.fileHashes).length > 0 && if (shouldLoadCache && existingMeta) {
repoHasGit;
if (shouldLoadCache && existingMeta && !willTryIncremental) {
try { try {
progress('embeddings', 0, 'Caching embeddings...'); progress('embeddings', 0, 'Caching embeddings...');
await initLbug(lbugPath); await initLbug(lbugPath);
@ -356,12 +378,18 @@ export async function runFullAnalysis(
const newFileHashes = await computeFileHashes(repoPath, allFilePaths); const newFileHashes = await computeFileHashes(repoPath, allFilePaths);
// Decide incremental vs full at THIS point (post-pipeline, pre-DB). // Decide incremental vs full at THIS point (post-pipeline, pre-DB).
// willTryIncremental was the *prediction* used to skip the embedding // All eligibility conditions are checked here against the actual
// cache cycle; here we re-evaluate against the actual pipeline output. // pipeline output — no separate pre-pipeline prediction to desync from
// (Bugbot review on PR #1479: a prediction that flipped post-pipeline
// could skip the embedding cache load and then take the full-rebuild
// path, silently losing embeddings).
const isIncremental = const isIncremental =
willTryIncremental && !options.force &&
existingMeta !== null && !!existingMeta &&
existingMeta.schemaVersion === INCREMENTAL_SCHEMA_VERSION &&
!!existingMeta.fileHashes && !!existingMeta.fileHashes &&
Object.keys(existingMeta.fileHashes).length > 0 &&
repoHasGit &&
allFilePaths.length > 0; allFilePaths.length > 0;
const hashDiff = isIncremental const hashDiff = isIncremental
@ -449,7 +477,12 @@ export async function runFullAnalysis(
progress('fts', 90, 'Search indexes ready'); progress('fts', 90, 'Search indexes ready');
// ── Phase 3.5: Re-insert cached embeddings ──────────────────────── // ── Phase 3.5: Re-insert cached embeddings ────────────────────────
if (cachedEmbeddings.length > 0) { // Skipped on the incremental path because that path keeps the
// existing DB rows in place (re-inserting cached vectors over
// surviving rows would PK-conflict). On the full-rebuild path,
// the DB was wiped, so re-inserting the cache is the mechanism
// that preserves embeddings across the rebuild.
if (cachedEmbeddings.length > 0 && !isIncremental) {
const cachedDims = cachedEmbeddings[0].embedding.length; const cachedDims = cachedEmbeddings[0].embedding.length;
const { EMBEDDING_DIMS } = await import('./lbug/schema.js'); const { EMBEDDING_DIMS } = await import('./lbug/schema.js');
if (cachedDims !== EMBEDDING_DIMS) { if (cachedDims !== EMBEDDING_DIMS) {
@ -642,8 +675,16 @@ export async function runFullAnalysis(
// Persist the incremental parse cache for the next run. Wraps in // Persist the incremental parse cache for the next run. Wraps in
// try/catch so a cache-write failure never breaks an otherwise // try/catch so a cache-write failure never breaks an otherwise
// successful indexing run. // successful indexing run. Prune stale chunk-hash entries first so
// the cache file size stays bounded across runs (chunks whose
// composition no longer matches anything in the current scan are
// dead weight; the parse phase populates `usedKeys` as it processes
// chunks).
try { try {
const pruned = pruneCache(parseCache, parseCache.usedKeys);
if (pruned > 0) {
log(`Parse cache: pruned ${pruned} stale chunk entries`);
}
await saveParseCache(storagePath, parseCache); await saveParseCache(storagePath, parseCache);
} catch (e) { } catch (e) {
log(`Warning: could not save parse cache (${(e as Error).message}); continuing.`); log(`Warning: could not save parse cache (${(e as Error).message}); continuing.`);

View file

@ -43,6 +43,14 @@ interface ParseCacheFile {
export interface ParseCache { export interface ParseCache {
version: number; version: number;
entries: Map<string, ParseWorkerResult[]>; entries: Map<string, ParseWorkerResult[]>;
/**
* Hashes referenced (hit OR miss-and-stored) by the current run.
* The parse phase populates this as it processes chunks; the orchestrator
* uses it as input to `pruneCache` before saving so entries that no
* longer correspond to any chunk in the current scan are discarded.
* Transient — never serialized to disk.
*/
usedKeys: Set<string>;
} }
/** SHA-256 hex of a single string or buffer. */ /** SHA-256 hex of a single string or buffer. */
@ -118,7 +126,7 @@ export const loadParseCache = async (storagePath: string): Promise<ParseCache> =
for (const [k, v] of Object.entries(data.entries)) { for (const [k, v] of Object.entries(data.entries)) {
if (Array.isArray(v)) entries.set(k, v as ParseWorkerResult[]); if (Array.isArray(v)) entries.set(k, v as ParseWorkerResult[]);
} }
return { version: PARSE_CACHE_VERSION, entries }; return { version: PARSE_CACHE_VERSION, entries, usedKeys: new Set<string>() };
} catch { } catch {
return emptyCache(); return emptyCache();
} }
@ -161,4 +169,5 @@ export const pruneCache = (cache: ParseCache, usedHashes: ReadonlySet<string>):
const emptyCache = (): ParseCache => ({ const emptyCache = (): ParseCache => ({
version: PARSE_CACHE_VERSION, version: PARSE_CACHE_VERSION,
entries: new Map<string, ParseWorkerResult[]>(), entries: new Map<string, ParseWorkerResult[]>(),
usedKeys: new Set<string>(),
}); });