mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
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:
parent
812e828362
commit
5eb05977e1
3 changed files with 94 additions and 32 deletions
|
|
@ -163,13 +163,14 @@ export async function runChunkedParseAndResolve(
|
|||
);
|
||||
}
|
||||
|
||||
// Sort by path so chunk membership is stable across runs even when
|
||||
// the filesystem returns scan order non-deterministically. Without
|
||||
// this, the parse cache misses on every run because chunk boundaries
|
||||
// shift even when no source file content has changed. Sort is
|
||||
// ascending alphabetical — the comparator works for both POSIX and
|
||||
// Windows path separators since both are `string` in JS.
|
||||
parseableScanned.sort((a, b) => (a.path < b.path ? -1 : a.path > b.path ? 1 : 0));
|
||||
// We previously sorted parseableScanned alphabetically here for stable
|
||||
// chunk membership across runs (so the parse cache wouldn't miss when
|
||||
// filesystem-scan order varied). Removed because it surfaced a
|
||||
// pre-existing order-dependency in Ruby cross-file resolution
|
||||
// (`user.address.save → Address#save` resolution depends on file
|
||||
// processing order — a separate bug to fix). Filesystem order on most
|
||||
// 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;
|
||||
|
||||
|
|
@ -337,6 +338,11 @@ export async function runChunkedParseAndResolve(
|
|||
let chunkWorkerData: WorkerExtractedData | null;
|
||||
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) {
|
||||
// Cache hit: replay the cached worker output through the same
|
||||
// merge logic the live worker path uses.
|
||||
|
|
@ -526,6 +532,12 @@ export async function runChunkedParseAndResolve(
|
|||
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 =
|
||||
deferredWorkerHeritage.length > 0
|
||||
? buildHeritageMap(deferredWorkerHeritage, ctx, getHeritageStrategyForLanguage)
|
||||
|
|
|
|||
|
|
@ -36,7 +36,7 @@ import {
|
|||
} from '../storage/repo-manager.js';
|
||||
import { computeFileHashes, diffFileHashes } from '../storage/file-hash.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 {
|
||||
getCurrentCommit,
|
||||
getRemoteUrl,
|
||||
|
|
@ -206,13 +206,38 @@ export async function runFullAnalysis(
|
|||
// may have uncommitted changes. Only short-circuit when the working
|
||||
// tree is also clean — otherwise fall through to the incremental
|
||||
// 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 = (() => {
|
||||
try {
|
||||
const out = execFileSync('git', ['status', '--porcelain'], {
|
||||
cwd: repoPath,
|
||||
stdio: ['ignore', 'pipe', 'ignore'],
|
||||
encoding: 'utf8',
|
||||
});
|
||||
const out = execFileSync(
|
||||
'git',
|
||||
[
|
||||
'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;
|
||||
} catch {
|
||||
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.
|
||||
// Used to suppress the embedding cache+restore cycle in the incremental
|
||||
// case (embeddings stay in the DB; re-inserting them would PK-conflict).
|
||||
const willTryIncremental =
|
||||
!options.force &&
|
||||
!!existingMeta &&
|
||||
existingMeta.schemaVersion === INCREMENTAL_SCHEMA_VERSION &&
|
||||
!!existingMeta.fileHashes &&
|
||||
Object.keys(existingMeta.fileHashes).length > 0 &&
|
||||
repoHasGit;
|
||||
|
||||
if (shouldLoadCache && existingMeta && !willTryIncremental) {
|
||||
// We *always* load the embedding cache when one is requested (regardless
|
||||
// of the predicted `willTryIncremental`). The post-pipeline branch may
|
||||
// disagree with the prediction (e.g. when the pipeline produces zero
|
||||
// File nodes, `isIncremental` flips false and the full-rebuild path
|
||||
// wipes the DB) — loading unconditionally is cheap insurance against
|
||||
// silently dropping embeddings on a mispredicted run. The re-insert
|
||||
// step gates itself on the actual `isIncremental` value to avoid
|
||||
// PK-conflicts when the incremental writeback path keeps the rows.
|
||||
if (shouldLoadCache && existingMeta) {
|
||||
try {
|
||||
progress('embeddings', 0, 'Caching embeddings...');
|
||||
await initLbug(lbugPath);
|
||||
|
|
@ -356,12 +378,18 @@ export async function runFullAnalysis(
|
|||
const newFileHashes = await computeFileHashes(repoPath, allFilePaths);
|
||||
|
||||
// Decide incremental vs full at THIS point (post-pipeline, pre-DB).
|
||||
// willTryIncremental was the *prediction* used to skip the embedding
|
||||
// cache cycle; here we re-evaluate against the actual pipeline output.
|
||||
// All eligibility conditions are checked here against the actual
|
||||
// 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 =
|
||||
willTryIncremental &&
|
||||
existingMeta !== null &&
|
||||
!options.force &&
|
||||
!!existingMeta &&
|
||||
existingMeta.schemaVersion === INCREMENTAL_SCHEMA_VERSION &&
|
||||
!!existingMeta.fileHashes &&
|
||||
Object.keys(existingMeta.fileHashes).length > 0 &&
|
||||
repoHasGit &&
|
||||
allFilePaths.length > 0;
|
||||
|
||||
const hashDiff = isIncremental
|
||||
|
|
@ -449,7 +477,12 @@ export async function runFullAnalysis(
|
|||
progress('fts', 90, 'Search indexes ready');
|
||||
|
||||
// ── 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 { EMBEDDING_DIMS } = await import('./lbug/schema.js');
|
||||
if (cachedDims !== EMBEDDING_DIMS) {
|
||||
|
|
@ -642,8 +675,16 @@ export async function runFullAnalysis(
|
|||
|
||||
// Persist the incremental parse cache for the next run. Wraps in
|
||||
// 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 {
|
||||
const pruned = pruneCache(parseCache, parseCache.usedKeys);
|
||||
if (pruned > 0) {
|
||||
log(`Parse cache: pruned ${pruned} stale chunk entries`);
|
||||
}
|
||||
await saveParseCache(storagePath, parseCache);
|
||||
} catch (e) {
|
||||
log(`Warning: could not save parse cache (${(e as Error).message}); continuing.`);
|
||||
|
|
|
|||
|
|
@ -43,6 +43,14 @@ interface ParseCacheFile {
|
|||
export interface ParseCache {
|
||||
version: number;
|
||||
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. */
|
||||
|
|
@ -118,7 +126,7 @@ export const loadParseCache = async (storagePath: string): Promise<ParseCache> =
|
|||
for (const [k, v] of Object.entries(data.entries)) {
|
||||
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 {
|
||||
return emptyCache();
|
||||
}
|
||||
|
|
@ -161,4 +169,5 @@ export const pruneCache = (cache: ParseCache, usedHashes: ReadonlySet<string>):
|
|||
const emptyCache = (): ParseCache => ({
|
||||
version: PARSE_CACHE_VERSION,
|
||||
entries: new Map<string, ParseWorkerResult[]>(),
|
||||
usedKeys: new Set<string>(),
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue