Merge pull request #1526 from colbymchenry/feature/CG-35
CG-33/CG-35: converge incremental sync with a full rebuild
This commit is contained in:
Colby Mchenry
@@ -17,6 +17,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
|
||||
|
||||
### Fixes
|
||||
|
||||
- A long-lived index no longer drifts away from what a fresh `codegraph index` would produce. When a file gained or lost a symbol, references to that name in files the sync never touched kept pointing at the definition that was correct before the change, and — because nothing distinguished two same-named definitions — the winner could come down to the order files happened to be written, which differs between a full index and a sync. On this project's own repository, replaying 80 commits through `sync` left 5.7% of connections wrong; it is now 1.3%, and the wrong-answers-still-being-asserted half drops by 99.7%. Since call edges are what flow questions follow and what `codegraph_explore` ranks files by, this quietly degraded answers as an index aged, with nothing to indicate it. Syncing is unchanged in speed, and an edit that only changes a function's body does no extra work at all. Set `CODEGRAPH_NO_REBIND=1` to opt out.
|
||||
- `codegraph_explore` now concentrates its answer on the code that actually answers your question instead of spreading it across files that merely share a word with it, so more of the answer arrives in a single call. Thanks @LeDuyViet for the detailed measurements and reproduction. (#1500)
|
||||
- Files only weakly related to your question now come back as a name, symbol and line number instead of spending the answer on their source — name one of them in a follow-up `codegraph_explore` to get it back in full. (#1500)
|
||||
- A generated CRUD or protobuf layer no longer crowds out the hand-written code sitting beside it: generated files are now recognized by the `// Code generated by … DO NOT EDIT.` style banner written at the top of the file, not just by a filename that looks generated. Re-index after upgrading to pick up the new detection. (#1500)
|
||||
|
||||
@@ -0,0 +1,440 @@
|
||||
/**
|
||||
* Incremental sync must converge to a full rebuild (CG-33).
|
||||
*
|
||||
* A long-lived, auto-synced index silently diverged from a clean rebuild of the
|
||||
* identical tree: 4.3% of distinct edges wrong, in BOTH directions, on
|
||||
* codegraph's own repo. Two mechanisms, both exercised here:
|
||||
*
|
||||
* 1. Resolution binds a reference to one of the same-named definitions
|
||||
* PROJECT-WIDE, so adding or removing a definition changes the answer for
|
||||
* references in files the sync never touches. Those references resolved once
|
||||
* and their rows were deleted, so nothing revisited them — the index kept an
|
||||
* answer that was only correct against an older graph.
|
||||
* 2. When nothing disambiguated the candidates, the winner was whichever row
|
||||
* the index scan reached first — i.e. the order files were WRITTEN. A full
|
||||
* index writes in scan order; a sync appends each file as it changes, so the
|
||||
* same tree resolved differently depending on how the index was built.
|
||||
*
|
||||
* The assertions here compare the whole edge SET, never counts: the divergence
|
||||
* is bidirectional and nets out of a total (raw rows differed by 0.7% while
|
||||
* 4.3% of edges were wrong), so a count check passes on a broken index.
|
||||
*
|
||||
* ---
|
||||
*
|
||||
* THIS SUITE MUST FAIL WITH `CODEGRAPH_NO_REBIND=1` (CG-35).
|
||||
*
|
||||
* That environment variable is the kill switch on the rebind half of the fix
|
||||
* (`src/index.ts`, guarding `resurrectStaleResolutionEdges`). The convergence
|
||||
* cases below are the only coverage that half has, so the check is the suite's
|
||||
* own regression test:
|
||||
*
|
||||
* CODEGRAPH_NO_REBIND=1 npx vitest run __tests__/sync-rebuild-convergence.test.ts
|
||||
*
|
||||
* must report failures, and an unset run must be green. If you change a case
|
||||
* here, re-run both. A version of this suite passed under the kill switch
|
||||
* because `rebuildEdgeSet` was not rebuilding anything — see the note there.
|
||||
*/
|
||||
|
||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
||||
import * as fs from 'fs';
|
||||
import * as path from 'path';
|
||||
import * as os from 'os';
|
||||
import CodeGraph from '../src/index';
|
||||
import { createDatabase } from '../src/db/sqlite-adapter';
|
||||
|
||||
describe('Incremental sync converges to a full rebuild (CG-33)', () => {
|
||||
let testDir: string;
|
||||
let cg: CodeGraph;
|
||||
|
||||
const write = (rel: string, content: string) => {
|
||||
const full = path.join(testDir, rel);
|
||||
fs.mkdirSync(path.dirname(full), { recursive: true });
|
||||
fs.writeFileSync(full, content);
|
||||
};
|
||||
|
||||
/**
|
||||
* Every edge as a `source|target|kind` triple, read from the database with a
|
||||
* second read-only connection. Node ids are `sha256(filePath:kind:name:line)`,
|
||||
* so for an identical tree they are identical across a sync and a rebuild —
|
||||
* which is what makes the two sets directly comparable.
|
||||
*/
|
||||
const edgeSet = (): Set<string> => {
|
||||
const { db } = createDatabase(path.join(testDir, '.codegraph', 'codegraph.db'), { readOnly: true });
|
||||
try {
|
||||
const rows = db.prepare('SELECT source, target, kind FROM edges').all() as Array<{
|
||||
source: string;
|
||||
target: string;
|
||||
kind: string;
|
||||
}>;
|
||||
return new Set(rows.map((r) => `${r.source}|${r.target}|${r.kind}`));
|
||||
} finally {
|
||||
db.close();
|
||||
}
|
||||
};
|
||||
|
||||
/**
|
||||
* Run `fn` against a second, WRITABLE connection to the same database. Used
|
||||
* by the two rule tests below to plant edge shapes the extractor cannot
|
||||
* produce on demand — an edge from an engine older than the refName stamp,
|
||||
* and a synthesized dispatch edge.
|
||||
*/
|
||||
const withDb = <T>(fn: (db: ReturnType<typeof createDatabase>['db']) => T): T => {
|
||||
const { db } = createDatabase(path.join(testDir, '.codegraph', 'codegraph.db'));
|
||||
try {
|
||||
return fn(db);
|
||||
} finally {
|
||||
db.close();
|
||||
}
|
||||
};
|
||||
|
||||
/** Human-readable diff, so a failure names the edges instead of just a count. */
|
||||
const describeDiff = (synced: Set<string>, rebuilt: Set<string>): string => {
|
||||
const missing = [...rebuilt].filter((e) => !synced.has(e));
|
||||
const stale = [...synced].filter((e) => !rebuilt.has(e));
|
||||
return `missing from synced: ${missing.length}, stale in synced: ${stale.length}`;
|
||||
};
|
||||
|
||||
/**
|
||||
* Rebuild the index from scratch over the CURRENT tree and return its edge
|
||||
* set — the ground truth a user gets from `codegraph index`.
|
||||
*
|
||||
* It must go through `CodeGraph.recreate`, which is what the CLI's `index`
|
||||
* command does: it DELETES the database file and builds an empty one. Calling
|
||||
* `indexAll` on the live handle instead is not a rebuild at all — every file
|
||||
* hashes identical, so the store writes nothing (`nodesCreated: 0`), no
|
||||
* reference is re-created, and every existing edge survives untouched. The
|
||||
* comparison then reads the synced index against ITSELF and can never fail,
|
||||
* which is exactly how this suite passed with `CODEGRAPH_NO_REBIND=1` (CG-35).
|
||||
*/
|
||||
const rebuildEdgeSet = async (): Promise<Set<string>> => {
|
||||
// Close the live handle first: `recreate` unlinks the database file, and a
|
||||
// held handle makes that EBUSY on Windows.
|
||||
cg.destroy();
|
||||
cg = await CodeGraph.recreate(testDir);
|
||||
await cg.indexAll();
|
||||
return edgeSet();
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
testDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-cg33-'));
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cg?.destroy();
|
||||
if (fs.existsSync(testDir)) fs.rmSync(testDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
/**
|
||||
* The originating shape. `caller.ts` calls `pct` with no import, so it binds
|
||||
* by name; at index time `zeta.ts` is the only definition. A later sync adds
|
||||
* `alpha.ts`, which sorts FIRST and is therefore the rebuild's answer — but
|
||||
* `caller.ts` never changes, so nothing re-resolves it.
|
||||
*/
|
||||
it('rebinds references in UNCHANGED files when a sync adds a competing definition', async () => {
|
||||
write('src/caller.ts', `export function run(): number {\n return pct(1);\n}\n`);
|
||||
write('src/zeta.ts', `export function pct(n: number): number {\n return n;\n}\n`);
|
||||
cg = CodeGraph.initSync(testDir, { config: { include: ['**/*.ts'], exclude: [] } });
|
||||
await cg.indexAll();
|
||||
|
||||
write('src/alpha.ts', `export function pct(n: number): number {\n return n * 2;\n}\n`);
|
||||
const result = await cg.sync();
|
||||
expect(result.filesAdded).toBe(1);
|
||||
expect(result.definitionDelta).toContain('pct');
|
||||
|
||||
const synced = edgeSet();
|
||||
const rebuilt = await rebuildEdgeSet();
|
||||
expect(describeDiff(synced, rebuilt)).toBe('missing from synced: 0, stale in synced: 0');
|
||||
});
|
||||
|
||||
/**
|
||||
* The mirror direction: removing a definition narrows the candidate set too,
|
||||
* so the delta must include names the sync DROPPED, not just names it added.
|
||||
*
|
||||
* This one already converged before the fix — a removal cascades the edge
|
||||
* away and the #1240 removal path resurrects it, so the reference gets
|
||||
* re-resolved for free. It is here as a standing guard on the invariant, and
|
||||
* because the removal half of the delta has no other coverage: an
|
||||
* implementation that only sampled post-sync names would still pass every
|
||||
* other test in this file.
|
||||
*/
|
||||
it('rebinds references in UNCHANGED files when a sync removes a competing definition', async () => {
|
||||
write('src/caller.ts', `export function run(): number {\n return pct(1);\n}\n`);
|
||||
write('src/alpha.ts', `export function pct(n: number): number {\n return n;\n}\n`);
|
||||
write('src/zeta.ts', `export function pct(n: number): number {\n return n * 2;\n}\n`);
|
||||
cg = CodeGraph.initSync(testDir, { config: { include: ['**/*.ts'], exclude: [] } });
|
||||
await cg.indexAll();
|
||||
|
||||
fs.rmSync(path.join(testDir, 'src', 'alpha.ts'));
|
||||
const result = await cg.sync();
|
||||
expect(result.filesRemoved).toBe(1);
|
||||
|
||||
const synced = edgeSet();
|
||||
const rebuilt = await rebuildEdgeSet();
|
||||
expect(describeDiff(synced, rebuilt)).toBe('missing from synced: 0, stale in synced: 0');
|
||||
});
|
||||
|
||||
/**
|
||||
* The delta must be computed per FILE. Comparing one name set across the whole
|
||||
* changed batch cancels a name that is added in one changed file while another
|
||||
* changed file already defined it — which is precisely the shape a commit that
|
||||
* splits a module out has, and it was the largest residual class in the first
|
||||
* measurement of this fix.
|
||||
*/
|
||||
it('flags a name added in one changed file even when another changed file already defines it', async () => {
|
||||
write('src/caller.ts', `export function run(): number {\n return pct(1);\n}\n`);
|
||||
write('src/zeta.ts', `export function pct(n: number): number {\n return n;\n}\nexport function keep(): number {\n return 0;\n}\n`);
|
||||
cg = CodeGraph.initSync(testDir, { config: { include: ['**/*.ts'], exclude: [] } });
|
||||
await cg.indexAll();
|
||||
|
||||
// One commit: a NEW file gains `pct`, and the file that already had `pct`
|
||||
// is edited too (so a batch-wide name set would see `pct` on both sides).
|
||||
write('src/alpha.ts', `export function pct(n: number): number {\n return n * 2;\n}\n`);
|
||||
write('src/zeta.ts', `export function pct(n: number): number {\n return n + 1;\n}\nexport function keep(): number {\n return 0;\n}\n`);
|
||||
const result = await cg.sync();
|
||||
expect(result.definitionDelta).toContain('pct');
|
||||
|
||||
const synced = edgeSet();
|
||||
const rebuilt = await rebuildEdgeSet();
|
||||
expect(describeDiff(synced, rebuilt)).toBe('missing from synced: 0, stale in synced: 0');
|
||||
});
|
||||
|
||||
/**
|
||||
* The realistic case the issue was filed from: many edits driven through sync
|
||||
* one after another, the way a watcher or a `git pull` applies them. Drift
|
||||
* accumulated across syncs, so a single-edit test would not have caught it.
|
||||
*/
|
||||
it('stays converged across a sequence of adds, edits, renames and deletes', async () => {
|
||||
write('src/caller.ts', `export function run(): number {\n return pct(1) + fmt(2) + collect(3);\n}\n`);
|
||||
write('src/util/zeta.ts', `export function pct(n: number): number {\n return n;\n}\n`);
|
||||
write('src/util/omega.ts', `export function fmt(n: number): number {\n return n;\n}\n`);
|
||||
cg = CodeGraph.initSync(testDir, { config: { include: ['**/*.ts'], exclude: [] } });
|
||||
await cg.indexAll();
|
||||
|
||||
// 1. add a competing `pct` that sorts before the existing one
|
||||
write('src/util/alpha.ts', `export function pct(n: number): number {\n return n * 2;\n}\n`);
|
||||
await cg.sync();
|
||||
|
||||
// 2. body-only edit — must produce NO definition delta, so the common sync
|
||||
// pays nothing for this machinery
|
||||
write('src/util/alpha.ts', `export function pct(n: number): number {\n return n * 3;\n}\n`);
|
||||
const bodyOnly = await cg.sync();
|
||||
expect(bodyOnly.filesModified).toBe(1);
|
||||
expect(bodyOnly.definitionDelta).toBeUndefined();
|
||||
|
||||
// 3. a rename: `fmt` moves out of omega.ts into a file that sorts first
|
||||
write('src/util/omega.ts', `export function other(n: number): number {\n return n;\n}\n`);
|
||||
write('src/util/beta.ts', `export function fmt(n: number): number {\n return n;\n}\n`);
|
||||
await cg.sync();
|
||||
|
||||
// 4. a symbol appears for a reference that never resolved at all
|
||||
write('src/util/gamma.ts', `export function collect(n: number): number {\n return n;\n}\n`);
|
||||
await cg.sync();
|
||||
|
||||
// 5. delete the current `pct` winner, so the reference must fall back...
|
||||
fs.rmSync(path.join(testDir, 'src', 'util', 'alpha.ts'));
|
||||
await cg.sync();
|
||||
|
||||
// 6. ...and then a later sync introduces a new winner ahead of it again.
|
||||
// Ending here rather than on the delete matters: after the delete the
|
||||
// binding happens to land back where it started, which a broken index
|
||||
// also reaches. The final state must be one only re-resolution reaches.
|
||||
write('src/util/aaa.ts', `export function pct(n: number): number {\n return n * 5;\n}\n`);
|
||||
await cg.sync();
|
||||
|
||||
const synced = edgeSet();
|
||||
expect(synced.size).toBeGreaterThan(0);
|
||||
const rebuilt = await rebuildEdgeSet();
|
||||
expect(describeDiff(synced, rebuilt)).toBe('missing from synced: 0, stale in synced: 0');
|
||||
});
|
||||
|
||||
/**
|
||||
* The rebind pass DELETES an edge and re-inserts the reference behind it, so
|
||||
* it may only touch edges it can reconstruct. Two shapes it must leave alone,
|
||||
* both of which it would otherwise destroy permanently:
|
||||
*
|
||||
* - an edge with no `metadata.refName` — written by an engine older than the
|
||||
* stamp. Rebuilding a reference from the target's plain name would strip the
|
||||
* receiver context the original text carried (`h.greet` → `greet`);
|
||||
* - a synthesized dispatch edge (`provenance='heuristic'`), which is not
|
||||
* resolution output at all: nothing would re-create it, and the synthesizer
|
||||
* that wired it does not run again on this sync.
|
||||
*
|
||||
* Both are planted directly, since extraction cannot be asked to emit them.
|
||||
* The sync then changes the answer for `pct`, which is exactly the condition
|
||||
* that makes the pass want to re-open every edge targeting `pct`.
|
||||
*/
|
||||
it('never deletes an edge it cannot reconstruct — no refName stamp, or synthesized', async () => {
|
||||
write('src/caller.ts', `export function run(): number {\n return pct(1);\n}\n`);
|
||||
write('src/other.ts', `export function other(): number {\n return 0;\n}\n`);
|
||||
write('src/zeta.ts', `export function pct(n: number): number {\n return n;\n}\n`);
|
||||
cg = CodeGraph.initSync(testDir, { config: { include: ['**/*.ts'], exclude: [] } });
|
||||
await cg.indexAll();
|
||||
|
||||
const planted = withDb((db) => {
|
||||
const pct = db.prepare("SELECT id FROM nodes WHERE name = 'pct'").get() as { id: string };
|
||||
const other = db.prepare("SELECT id FROM nodes WHERE name = 'other'").get() as { id: string };
|
||||
|
||||
// 1. Strip the stamp off the real edge, leaving the rest of its metadata
|
||||
// intact — the shape an index built before the stamp existed has.
|
||||
db.prepare(
|
||||
`UPDATE edges SET metadata = json_remove(metadata, '$.refName')
|
||||
WHERE target = ? AND kind = 'calls'`
|
||||
).run(pct.id);
|
||||
|
||||
// 2. A synthesized edge that DOES carry a stamp, so only the provenance
|
||||
// rule can save it.
|
||||
db.prepare(
|
||||
`INSERT INTO edges (source, target, kind, metadata, line, col, provenance)
|
||||
VALUES (?, ?, 'calls', ?, 1, 0, 'heuristic')`
|
||||
).run(other.id, pct.id, JSON.stringify({ refName: 'pct', synthesizedBy: 'cg35-test' }));
|
||||
|
||||
return {
|
||||
unstamped: `${(db.prepare("SELECT source FROM edges WHERE target = ? AND provenance IS NULL AND kind = 'calls'").get(pct.id) as { source: string }).source}|${pct.id}|calls`,
|
||||
synthesized: `${other.id}|${pct.id}|calls`,
|
||||
};
|
||||
});
|
||||
|
||||
const before = edgeSet();
|
||||
expect(before.has(planted.unstamped)).toBe(true);
|
||||
expect(before.has(planted.synthesized)).toBe(true);
|
||||
|
||||
write('src/alpha.ts', `export function pct(n: number): number {\n return n * 2;\n}\n`);
|
||||
const result = await cg.sync();
|
||||
expect(result.definitionDelta).toContain('pct');
|
||||
|
||||
// Both survive: the pass considered them (their target is `pct`) and
|
||||
// declined. Drift is the acceptable outcome here; an edge that no pass can
|
||||
// ever restore is not.
|
||||
const after = edgeSet();
|
||||
expect(after.has(planted.unstamped)).toBe(true);
|
||||
expect(after.has(planted.synthesized)).toBe(true);
|
||||
});
|
||||
|
||||
/**
|
||||
* The per-name ceiling in `getResolutionEdgesByTargetName` (500 by default).
|
||||
* Above it a name is generic — `push`, `get`, `join` — one new definition
|
||||
* won't flip most of its references, and rebinding an arbitrary subset would
|
||||
* manufacture wrong edges while costing the most work. It must DECLINE the
|
||||
* name outright, and declining must be lossless.
|
||||
*
|
||||
* The rare name in the same sync is the control: it proves the pass ran and
|
||||
* that the ceiling is what spared the generic one, not a dead rebind pass.
|
||||
*/
|
||||
it('declines a name over the per-name ceiling instead of rebinding an arbitrary subset', async () => {
|
||||
// Must exceed the 500 default in getResolutionEdgesByTargetName.
|
||||
const OVER_CEILING = 501;
|
||||
const callers = Array.from(
|
||||
{ length: OVER_CEILING },
|
||||
(_, i) => `export function hot${i}(): number {\n return push(${i});\n}\n`
|
||||
).join('');
|
||||
write('src/hot.ts', callers);
|
||||
write('src/rare.ts', `export function rare(): number {\n return tug(1);\n}\n`);
|
||||
write(
|
||||
'src/zzz_defs.ts',
|
||||
`export function push(n: number): number {\n return n;\n}\nexport function tug(n: number): number {\n return n;\n}\n`
|
||||
);
|
||||
cg = CodeGraph.initSync(testDir, { config: { include: ['**/*.ts'], exclude: [] } });
|
||||
await cg.indexAll();
|
||||
|
||||
const targetsOf = (name: string): string[] =>
|
||||
withDb((db) =>
|
||||
(
|
||||
db
|
||||
.prepare(
|
||||
`SELECT t.file_path AS file FROM edges e
|
||||
JOIN nodes t ON t.id = e.target
|
||||
JOIN nodes s ON s.id = e.source
|
||||
WHERE t.name = ? AND e.kind = 'calls'`
|
||||
)
|
||||
.all(name) as Array<{ file: string }>
|
||||
).map((r) => r.file)
|
||||
);
|
||||
|
||||
expect(targetsOf('push')).toHaveLength(OVER_CEILING);
|
||||
expect(new Set(targetsOf('push'))).toEqual(new Set(['src/zzz_defs.ts']));
|
||||
expect(targetsOf('tug')).toEqual(['src/zzz_defs.ts']);
|
||||
|
||||
// One sync adds a competing definition of BOTH names, in a file that sorts
|
||||
// first and is therefore the rebuild's answer for each.
|
||||
write(
|
||||
'src/aaa.ts',
|
||||
`export function push(n: number): number {\n return n * 2;\n}\nexport function tug(n: number): number {\n return n * 2;\n}\n`
|
||||
);
|
||||
const result = await cg.sync();
|
||||
expect(result.definitionDelta).toContain('push');
|
||||
expect(result.definitionDelta).toContain('tug');
|
||||
|
||||
// `push` is untouched — every edge still there, still on the old target.
|
||||
// This is knowingly divergent from a rebuild; see "Don't chase the
|
||||
// residual" in docs/benchmarks/index-drift-cg33.md.
|
||||
const pushTargets = targetsOf('push');
|
||||
expect(pushTargets).toHaveLength(OVER_CEILING);
|
||||
expect(new Set(pushTargets)).toEqual(new Set(['src/zzz_defs.ts']));
|
||||
|
||||
// `tug` — the control — rebound.
|
||||
expect(targetsOf('tug')).toEqual(['src/aaa.ts']);
|
||||
});
|
||||
|
||||
/**
|
||||
* Guards the escape hatch itself: with the rebind pass off, the same sequence
|
||||
* must still produce a structurally sound index (no lost or orphaned edges) —
|
||||
* just a drifted one. If this ever fails, the pass is doing something the
|
||||
* kill switch cannot undo.
|
||||
*/
|
||||
it('CODEGRAPH_NO_REBIND=1 disables the pass without corrupting the index', async () => {
|
||||
write('src/caller.ts', `export function run(): number {\n return pct(1);\n}\n`);
|
||||
write('src/zeta.ts', `export function pct(n: number): number {\n return n;\n}\n`);
|
||||
cg = CodeGraph.initSync(testDir, { config: { include: ['**/*.ts'], exclude: [] } });
|
||||
await cg.indexAll();
|
||||
const before = edgeSet();
|
||||
|
||||
process.env.CODEGRAPH_NO_REBIND = '1';
|
||||
try {
|
||||
write('src/alpha.ts', `export function pct(n: number): number {\n return n * 2;\n}\n`);
|
||||
await cg.sync();
|
||||
} finally {
|
||||
delete process.env.CODEGRAPH_NO_REBIND;
|
||||
}
|
||||
|
||||
const after = edgeSet();
|
||||
// Every edge that existed before is still there — the pass is the only
|
||||
// thing that would have re-opened them, and it did not run.
|
||||
for (const edge of before) expect(after.has(edge)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Resolution's candidate order must be a property of the CODE, not of the order
|
||||
* rows were written. This is the half of CG-33 that a re-resolution pass alone
|
||||
* cannot fix: without it, re-resolving a reference against the very same graph
|
||||
* can still pick a different winner than a rebuild does.
|
||||
*/
|
||||
describe('Same-name candidate order is content-derived, not insertion-derived (CG-33)', () => {
|
||||
let testDir: string;
|
||||
let cg: CodeGraph;
|
||||
|
||||
afterEach(() => {
|
||||
cg?.destroy();
|
||||
if (fs.existsSync(testDir)) fs.rmSync(testDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it('getNodesByName orders by (file_path, start_line) even when rows were written in another order', async () => {
|
||||
testDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-cg33-order-'));
|
||||
fs.mkdirSync(path.join(testDir, 'src'), { recursive: true });
|
||||
fs.writeFileSync(path.join(testDir, 'src', 'mid.ts'), `export function pad(): void {}\nexport function dup(): number {\n return 2;\n}\n`);
|
||||
fs.writeFileSync(path.join(testDir, 'src', 'zeta.ts'), `export function dup(): number {\n return 1;\n}\n`);
|
||||
cg = CodeGraph.initSync(testDir, { config: { include: ['**/*.ts'], exclude: [] } });
|
||||
await cg.indexAll();
|
||||
|
||||
// A sync APPENDS this file's nodes, so `alpha.ts` gets the highest rowids
|
||||
// despite sorting first — exactly the divergence a full index never has,
|
||||
// and the reason candidate order cannot come from the physical row order.
|
||||
fs.writeFileSync(path.join(testDir, 'src', 'alpha.ts'), `export function dup(): number {\n return 3;\n}\n`);
|
||||
await cg.sync();
|
||||
|
||||
const keys = cg.getNodesByName('dup').map((n) => `${n.filePath}:${String(n.startLine).padStart(6, '0')}`);
|
||||
expect(keys.length).toBeGreaterThanOrEqual(3);
|
||||
expect(keys).toEqual([...keys].sort());
|
||||
expect(keys[0]).toContain('src/alpha.ts');
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,189 @@
|
||||
# Index drift: incremental sync vs. full rebuild (CG-33)
|
||||
|
||||
Measured 2026-08-06. A live, auto-sync-maintained index **does not converge** to
|
||||
a clean full rebuild of the identical working tree. On codegraph's own repo,
|
||||
**4.3% of distinct edges were wrong**, in both directions, overwhelmingly
|
||||
`calls` edges.
|
||||
|
||||
This matters because it is silent: nothing warns, nothing surfaces it, and the
|
||||
README tells users the index is never stale and there is nothing to re-run.
|
||||
Retrieval quality decays invisibly, and the user-visible symptom — an agent
|
||||
falling back to Read — reads as "codegraph isn't very good" rather than "this
|
||||
index needs rebuilding."
|
||||
|
||||
## Result
|
||||
|
||||
Subject: codegraph's own `.codegraph/codegraph.db`, long-lived and
|
||||
incrementally synced, against a full rebuild of the same tree with the same
|
||||
build. Edges compared as distinct `(source, target, kind)` triples.
|
||||
|
||||
| | count |
|
||||
|---|---|
|
||||
| distinct edge triples (rebuild) | 28,809 |
|
||||
| in rebuild but **missing** from live | **751** |
|
||||
| in live but **absent** from rebuild (stale) | **476** |
|
||||
| **total divergent** | **1,227 — 4.3%** |
|
||||
|
||||
Missing edges by kind: `calls=635`, `contains=38`, `references=34`,
|
||||
`instantiates=21`, `imports=13`, `extends=10`.
|
||||
|
||||
### Raw counts hide it
|
||||
|
||||
Raw edge **rows** were 39,845 live vs 40,122 rebuilt — a benign-looking +0.7%.
|
||||
The divergence is bidirectional, so a net-count check nets it out and reports
|
||||
almost nothing wrong. **Any drift detector must compare edge sets, not totals.**
|
||||
|
||||
### The indexer is deterministic
|
||||
|
||||
Control, rebuild vs rebuild on the same tree and build: **0 differing edges**
|
||||
(28,809 both runs). So the live-vs-rebuild delta is not run-to-run noise.
|
||||
|
||||
### It is resolution, not residue
|
||||
|
||||
Node sets are identical — `files` 501 = 501, `nodes` 10,110 = 10,110,
|
||||
heuristic edges 36 = 36 — and every integrity check is 0 on *both* indexes:
|
||||
no duplicate nodes, no orphan edges, no nodes referencing a missing file row.
|
||||
|
||||
Nothing accumulates. Cross-file **resolution** goes stale.
|
||||
|
||||
## Mechanism — two causes, both confirmed
|
||||
|
||||
`ReferenceResolver` binds a reference to one of the same-named definitions
|
||||
**project-wide**. Two things follow, and the drift needed both to be fixed.
|
||||
|
||||
**1. Scope.** Incremental sync re-resolves only the references *in* the changed
|
||||
files. Adding or removing a definition of `pct` changes the correct answer for
|
||||
every `pct(...)` reference in the repo, including references in files the sync
|
||||
never touches — and those references resolved successfully once, which *deletes*
|
||||
their `unresolved_refs` row, so nothing existed to revisit them with. (The #1240
|
||||
retry only revisits refs parked as `status='failed'`.) The index kept an answer
|
||||
that was correct against an older graph.
|
||||
|
||||
**2. Tie-break.** When nothing disambiguated the candidates, `findBestMatch`
|
||||
kept the first one, and `getNodesByName` had no `ORDER BY` — so the winner was
|
||||
decided by rowid, i.e. by the order files happened to be **written**. A full
|
||||
index writes in scan order; a sync appends each file as it changes. The same
|
||||
tree therefore resolved to different edges depending on how the index was built,
|
||||
and no amount of re-resolution could converge, because re-resolving against the
|
||||
identical graph still picked a different candidate.
|
||||
|
||||
### The fix
|
||||
|
||||
- `getNodesByName` orders by `(file_path, start_line)` — a property of the code,
|
||||
not of the write order (`src/db/queries.ts`).
|
||||
- `sync` returns a `definitionDelta`: the names whose set of definitions the sync
|
||||
changed, computed as the symmetric difference of `file\0name` pairs sampled
|
||||
before and after the store phase (`ExtractionOrchestrator.sync`).
|
||||
- For each delta name, `resurrectStaleResolutionEdges` deletes the resolution
|
||||
edges targeting a symbol of that name whose source is in an *unchanged* file,
|
||||
and re-inserts each as the reference that created it (the `metadata.refName`
|
||||
stamp). The existing orphan sweep then resolves them against the post-sync
|
||||
graph — the same input a rebuild resolves from. Kill switch:
|
||||
`CODEGRAPH_NO_REBIND=1`.
|
||||
|
||||
The delta is compared **per file**, not as one name set over the whole batch: a
|
||||
commit that adds `collect` to a new file while an unrelated changed file already
|
||||
defines `collect` cancels out of a batch-wide name set, and that miss was the
|
||||
largest residual class in the first measurement of this fix.
|
||||
|
||||
Conservative by construction, because a wrong deletion is a permanent edge loss
|
||||
while a missed rebind is only residual drift: an edge with no `refName` stamp
|
||||
(synthesized, or built by an older engine) is never touched, edges whose source
|
||||
file the sync already re-extracted are skipped, and a per-name ceiling of 500
|
||||
edges declines the generic names.
|
||||
|
||||
### Result
|
||||
|
||||
Replaying real commits of this repo through `sync` one at a time, then diffing
|
||||
against a clean rebuild of the final tree:
|
||||
|
||||
| replay | baseline (`main`) | + ORDER BY only | + rebind pass (shipped) |
|
||||
|---|---|---|---|
|
||||
| 16 commits | 48 (24 missing / 24 stale) | 20 | **0 — converged** |
|
||||
| 80 commits | 1,634 (963 / 671) | 890 | **361 (359 / 2)** |
|
||||
|
||||
The direction that actively misleads — **stale** edges the index keeps asserting
|
||||
— drops from 671 to **2** over 80 commits, a 99.7% reduction.
|
||||
|
||||
Index and sync wall-clock are unchanged (392-file repo: index 1.88–2.02s in both
|
||||
arms, single-file sync 0.183s in both). The `ORDER BY` costs 18% per *uncached*
|
||||
name lookup in a tight loop (237ms → 280ms over 10,127 lookups), which does not
|
||||
reach wall-clock because `ReferenceResolver` memoizes the lookup per name. A
|
||||
composite `(name, file_path, start_line)` index would make the sort free, but it
|
||||
would widen every node index entry with a full path string on the write-heavy
|
||||
indexing path — not worth 43ms.
|
||||
|
||||
### The residual, and why it is not chased
|
||||
|
||||
At 80 commits, 357 of the 361 remaining edges are a single pre-existing class:
|
||||
references to very generic names (`push` 260, `join` 97) that failed at index
|
||||
time and stay parked because `getRetryableFailedReferences` declines any name
|
||||
with more than 500 failed refs (1,412 for `push`, 2,346 for `join`). That
|
||||
ceiling is #1240/#999 policy, it is present on `main`, and what it declines to
|
||||
create is cross-language garbage: a TypeScript test file "calling" an R method
|
||||
named `push`, or a Rust method named `join`. **The full rebuild is the wrong one
|
||||
here** — converging would mean teaching sync to manufacture thousands of wrong
|
||||
edges. Left as is, deliberately.
|
||||
|
||||
### `codegraph status` — decided: no drift metric
|
||||
|
||||
The issue asked whether `status` should surface divergence. Decision: **no**.
|
||||
|
||||
A drift number cannot be computed without the full rebuild it would be
|
||||
recommending, so anything cheap enough to run on `status` would be an estimate —
|
||||
and an honest estimate is not available. Shipping a proxy would violate the
|
||||
product rule that a screen must not overclaim, and post-fix it would fire on the
|
||||
generic-name residual above, training users to ignore it. (`status` already
|
||||
refuses to warn on parked failed refs for the same reason: every repo with
|
||||
external-library imports has them, so the warning would be permanent noise.)
|
||||
|
||||
The check that *is* exact stays available and is documented below.
|
||||
|
||||
## Why it degrades retrieval
|
||||
|
||||
Graph mass (RWR) is **relative and normalized**, so call edges missing elsewhere
|
||||
inflate an unaffected file's share of the mass. Explore ranks files by that mass
|
||||
(`allocateExploreBudget` weights on it), so drift silently promotes files that
|
||||
should rank low.
|
||||
|
||||
Observed on a private application repo under heavy development: a generated
|
||||
ambient-types file carried graph mass **0.24750** on the drifted index vs
|
||||
**0.13119** on a clean rebuild (~1.9×), and score **49.0** vs **27.0**. On the
|
||||
drifted index it took **60.7%** of an explore envelope and starved the file the
|
||||
agent had actually named by symbol, which rendered **251 chars of a 10,970
|
||||
reservation**. After a full re-index — no code change — the same query answers
|
||||
correctly. That incident is what prompted this measurement; see CG-24.
|
||||
|
||||
Severity scales with churn and index age. codegraph's own repo shows 4.3%;
|
||||
a repo under heavier active development plausibly drifts further.
|
||||
|
||||
## Reproducing
|
||||
|
||||
`scripts/agent-eval/diff-index-drift.mjs` is read-only and diffs two indexes.
|
||||
Snapshot the live index **before** rebuilding — the original artifact for this
|
||||
investigation was destroyed by re-indexing over it:
|
||||
|
||||
```bash
|
||||
cp .codegraph/codegraph.db /tmp/live.db # snapshot FIRST
|
||||
node dist/bin/codegraph.js index . # full rebuild
|
||||
node scripts/agent-eval/diff-index-drift.mjs /tmp/live.db .codegraph/codegraph.db
|
||||
```
|
||||
|
||||
Exit code is 0 when converged, 1 when drifted. To re-confirm determinism, diff
|
||||
two consecutive rebuilds — that must report 0.
|
||||
|
||||
To reproduce the *regression* rather than measure a live index, replay real
|
||||
commits through `sync`: clone the repo, check out `HEAD~N`, index, then
|
||||
`git checkout <sha> && codegraph sync` for each commit in order, snapshot the
|
||||
database, and diff it against a rebuild of the final tree. That is what produced
|
||||
the table above, and the unit-scale version of it is
|
||||
`__tests__/sync-rebuild-convergence.test.ts`.
|
||||
|
||||
## Note on probing an index
|
||||
|
||||
The index file is `.codegraph/codegraph.db`. There is no `graph.db`. `sqlite3`
|
||||
against a mistyped path **creates an empty database** rather than failing, and
|
||||
every subsequent query then answers from an empty schema — which reads exactly
|
||||
like a stale pre-migration index. That produced a wrong root cause during this
|
||||
investigation. `diff-index-drift.mjs` checks `existsSync` before opening for
|
||||
exactly this reason.
|
||||
@@ -0,0 +1,102 @@
|
||||
#!/usr/bin/env node
|
||||
/**
|
||||
* Diff two CodeGraph indexes of the SAME tree — typically a live,
|
||||
* incrementally-synced `.codegraph/codegraph.db` against a clean full rebuild
|
||||
* of the identical working tree (CG-33).
|
||||
*
|
||||
* Non-destructive: it only reads. Rebuilding is the caller's job, so the live
|
||||
* index is never clobbered by the tool measuring it — the mistake that cost the
|
||||
* original CG-33 artifact.
|
||||
*
|
||||
* # snapshot the live index BEFORE touching it
|
||||
* cp .codegraph/codegraph.db /tmp/live.db
|
||||
* node dist/bin/codegraph.js index .
|
||||
* node scripts/agent-eval/diff-index-drift.mjs /tmp/live.db .codegraph/codegraph.db
|
||||
*
|
||||
* Edges are compared as distinct `(source, target, kind)` triples. Raw row
|
||||
* counts are NOT a drift signal: a bidirectional divergence nets out. On the
|
||||
* codegraph repo the raw counts differed by +0.7% while 4.3% of distinct edges
|
||||
* were actually wrong.
|
||||
*/
|
||||
import { DatabaseSync } from 'node:sqlite';
|
||||
import { existsSync } from 'node:fs';
|
||||
|
||||
const [livePath, rebuiltPath] = process.argv.slice(2);
|
||||
if (!livePath || !rebuiltPath) {
|
||||
console.error('usage: diff-index-drift.mjs <live.db> <rebuilt.db>');
|
||||
process.exit(2);
|
||||
}
|
||||
for (const p of [livePath, rebuiltPath]) {
|
||||
if (!existsSync(p)) {
|
||||
// node:sqlite CREATES a missing file rather than failing, which silently
|
||||
// yields an empty schema and a confident, wrong conclusion. Refuse first.
|
||||
console.error(`not found: ${p}`);
|
||||
process.exit(2);
|
||||
}
|
||||
}
|
||||
|
||||
const open = (p) => new DatabaseSync(p, { readOnly: true });
|
||||
const live = open(livePath);
|
||||
const rebuilt = open(rebuiltPath);
|
||||
|
||||
const scalar = (db, q) => db.prepare(q).get().n;
|
||||
const edgeKey = (r) => `${r.source}\u0000${r.target}\u0000${r.kind}`;
|
||||
|
||||
const liveEdges = live.prepare('select source, target, kind from edges').all();
|
||||
const rebuiltEdges = rebuilt.prepare('select source, target, kind from edges').all();
|
||||
const liveSet = new Set(liveEdges.map(edgeKey));
|
||||
const rebuiltSet = new Set(rebuiltEdges.map(edgeKey));
|
||||
|
||||
const missing = rebuiltEdges.filter((r) => !liveSet.has(edgeKey(r))); // should exist, doesn't
|
||||
const stale = liveEdges.filter((r) => !rebuiltSet.has(edgeKey(r))); // exists, shouldn't
|
||||
const divergent = missing.length + stale.length;
|
||||
|
||||
const byKind = (rows) => {
|
||||
const m = new Map();
|
||||
for (const r of rows) m.set(r.kind, (m.get(r.kind) ?? 0) + 1);
|
||||
return [...m].sort((a, b) => b[1] - a[1]).map(([k, v]) => `${k}=${v}`).join(', ') || '(none)';
|
||||
};
|
||||
|
||||
const pct = (n, d) => (d ? ((n / d) * 100).toFixed(1) : '0.0');
|
||||
|
||||
console.log(`live ${livePath}`);
|
||||
console.log(`rebuilt ${rebuiltPath}`);
|
||||
console.log('');
|
||||
console.log('counts live rebuilt');
|
||||
for (const [label, q] of [
|
||||
['files', 'select count(*) n from files'],
|
||||
['nodes', 'select count(*) n from nodes'],
|
||||
['edges (rows)', 'select count(*) n from edges'],
|
||||
// Grouped rather than `count(distinct a || b || c)`: bare concatenation has no
|
||||
// separator, so `(ab, c)` and `(a, bc)` would collapse into one.
|
||||
['edges (distinct)', 'select count(*) n from (select distinct source, target, kind from edges)'],
|
||||
['heuristic edges', "select count(*) n from edges where provenance='heuristic'"],
|
||||
]) {
|
||||
console.log(` ${label.padEnd(20)} ${String(scalar(live, q)).padEnd(9)} ${scalar(rebuilt, q)}`);
|
||||
}
|
||||
|
||||
console.log('');
|
||||
console.log('edge divergence (distinct triples)');
|
||||
console.log(` missing from live: ${missing.length} — ${byKind(missing)}`);
|
||||
console.log(` stale in live: ${stale.length} — ${byKind(stale)}`);
|
||||
console.log(` TOTAL divergent: ${divergent} (${pct(divergent, rebuiltSet.size)}% of ${rebuiltSet.size})`);
|
||||
|
||||
// Integrity checks — these separate "resolution went stale" (edges wrong, nodes
|
||||
// identical) from "residue accumulated" (duplicate/orphan rows). CG-33 is the
|
||||
// former: on the codegraph repo every check below was 0 on BOTH indexes.
|
||||
console.log('');
|
||||
console.log('integrity live rebuilt');
|
||||
for (const [label, q] of [
|
||||
['duplicate nodes', 'select count(*) n from (select file_path,name,kind,start_line from nodes group by 1,2,3,4 having count(*)>1)'],
|
||||
['orphan edges', 'select count(*) n from edges e where not exists(select 1 from nodes where id=e.source) or not exists(select 1 from nodes where id=e.target)'],
|
||||
['nodes w/ missing file row', 'select count(*) n from nodes nd where not exists(select 1 from files f where f.path=nd.file_path)'],
|
||||
]) {
|
||||
console.log(` ${label.padEnd(30)} ${String(scalar(live, q)).padEnd(6)} ${scalar(rebuilt, q)}`);
|
||||
}
|
||||
|
||||
console.log('');
|
||||
console.log(divergent === 0
|
||||
? 'CONVERGED — the synced index matches a full rebuild.'
|
||||
: `DRIFTED — ${divergent} edges differ. Rebuild-vs-rebuild is 0 (the indexer is deterministic), so this is sync divergence, not noise.`);
|
||||
|
||||
process.exitCode = divergent === 0 ? 0 : 1;
|
||||
+139
-2
@@ -1113,11 +1113,28 @@ export class QueryBuilder {
|
||||
}
|
||||
|
||||
/**
|
||||
* Get nodes by exact name match (uses idx_nodes_name index)
|
||||
* Get nodes by exact name match (uses idx_nodes_name index).
|
||||
*
|
||||
* This is resolution's candidate list, and the ORDER BY is load-bearing for
|
||||
* index correctness, not cosmetic (CG-33). When a reference names a symbol
|
||||
* that several files define and nothing disambiguates them, resolution binds
|
||||
* to the first candidate — so without an ORDER BY the winner was decided by
|
||||
* rowid, i.e. by the order files happened to be WRITTEN. A full index writes
|
||||
* them in scan order; an incremental sync appends each file as it changes, so
|
||||
* the same tree resolved to different edges depending on how the index was
|
||||
* built, and a long-lived synced index drifted away from a rebuild of itself
|
||||
* (measured at 4.3% of distinct edges, mostly `calls`).
|
||||
*
|
||||
* `(file_path, start_line)` is a property of the CODE, so both paths now pick
|
||||
* the same candidate. The sort is paid once per distinct name per resolution
|
||||
* run — ReferenceResolver memoizes this in its nameCache — and the population
|
||||
* is capped by AMBIGUOUS_NAME_CEILING (#999).
|
||||
*/
|
||||
getNodesByName(name: string): Node[] {
|
||||
if (!this.stmts.getNodesByName) {
|
||||
this.stmts.getNodesByName = this.db.prepare('SELECT * FROM nodes WHERE name = ?');
|
||||
this.stmts.getNodesByName = this.db.prepare(
|
||||
'SELECT * FROM nodes WHERE name = ? ORDER BY file_path, start_line'
|
||||
);
|
||||
}
|
||||
const rows = this.stmts.getNodesByName.all(name) as NodeRow[];
|
||||
return rows.map(rowToNode);
|
||||
@@ -2540,6 +2557,99 @@ export class QueryBuilder {
|
||||
}));
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolution edges whose TARGET symbol is named one of `names` — the edges a
|
||||
* sync must re-resolve after `names` gained or lost a definition (CG-33).
|
||||
*
|
||||
* Resolution binds a reference to a node whose name matches the reference's
|
||||
* tail, and it picks among ALL same-named definitions project-wide. So adding
|
||||
* or removing one definition of `pct` changes the answer for every `pct(...)`
|
||||
* reference in the repo — including references in files this sync never
|
||||
* touches, whose edges nothing else revisits. Those edges' current target is,
|
||||
* by that same rule, a node named `pct`, which is why the target's name is a
|
||||
* sufficient (and index-backed, via idx_nodes_name) way to find them without
|
||||
* a schema change or a scan of edge metadata.
|
||||
*
|
||||
* Returns the source file/language alongside each edge so the caller can
|
||||
* resurrect it as its original reference. Excludes `provenance='heuristic'`
|
||||
* (synthesized dispatch edges are not resolution output and carry no refName
|
||||
* stamp to resurrect from — deleting one would be a permanent loss).
|
||||
*
|
||||
* Names matching more than `perNameCeiling` edges are skipped entirely, same
|
||||
* rationale and same default as {@link getRetryableFailedReferences}: at that
|
||||
* population the name is generic (`get`, `clear`, …), one definition changing
|
||||
* won't flip most of them, and rebinding an arbitrary subset is both wasted
|
||||
* work and incoherent coverage.
|
||||
*/
|
||||
getResolutionEdgesByTargetName(
|
||||
names: string[],
|
||||
perNameCeiling: number = 500
|
||||
): Array<Edge & { edgeId: number; sourceFilePath: string; sourceLanguage: Language }> {
|
||||
if (names.length === 0) return [];
|
||||
|
||||
// Pass 1: per-name edge counts, chunked under the SQLite parameter limit.
|
||||
const keep: string[] = [];
|
||||
for (let i = 0; i < names.length; i += SQLITE_PARAM_CHUNK_SIZE) {
|
||||
const chunk = names.slice(i, i + SQLITE_PARAM_CHUNK_SIZE);
|
||||
const placeholders = chunk.map(() => '?').join(',');
|
||||
const counts = this.db
|
||||
.prepare(
|
||||
`SELECT tgt.name AS name, COUNT(*) AS count
|
||||
FROM edges e
|
||||
JOIN nodes tgt ON tgt.id = e.target
|
||||
WHERE tgt.name IN (${placeholders})
|
||||
AND (e.provenance IS NULL OR e.provenance != 'heuristic')
|
||||
GROUP BY tgt.name`
|
||||
)
|
||||
.all(...chunk) as Array<{ name: string; count: number }>;
|
||||
for (const row of counts) {
|
||||
if (row.count <= perNameCeiling) keep.push(row.name);
|
||||
}
|
||||
}
|
||||
if (keep.length === 0) return [];
|
||||
|
||||
// Pass 2: load the surviving edges with the source file context a
|
||||
// resurrection needs.
|
||||
const out: Array<Edge & { edgeId: number; sourceFilePath: string; sourceLanguage: Language }> = [];
|
||||
for (let i = 0; i < keep.length; i += SQLITE_PARAM_CHUNK_SIZE) {
|
||||
const chunk = keep.slice(i, i + SQLITE_PARAM_CHUNK_SIZE);
|
||||
const placeholders = chunk.map(() => '?').join(',');
|
||||
const rows = this.db
|
||||
.prepare(
|
||||
`SELECT e.*, src.file_path AS source_file_path, src.language AS source_language
|
||||
FROM edges e
|
||||
JOIN nodes tgt ON tgt.id = e.target
|
||||
JOIN nodes src ON src.id = e.source
|
||||
WHERE tgt.name IN (${placeholders})
|
||||
AND (e.provenance IS NULL OR e.provenance != 'heuristic')`
|
||||
)
|
||||
.all(...chunk) as Array<EdgeRow & { source_file_path: string; source_language: Language }>;
|
||||
for (const row of rows) {
|
||||
out.push({
|
||||
...rowToEdge(row),
|
||||
edgeId: row.id,
|
||||
sourceFilePath: row.source_file_path,
|
||||
sourceLanguage: row.source_language,
|
||||
});
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
/** Delete edges by primary key — the rebind pass's half of a re-resolution. */
|
||||
deleteEdgesByIds(edgeIds: number[]): number {
|
||||
if (edgeIds.length === 0) return 0;
|
||||
let changed = 0;
|
||||
this.db.transaction(() => {
|
||||
for (let i = 0; i < edgeIds.length; i += SQLITE_PARAM_CHUNK_SIZE) {
|
||||
const chunk = edgeIds.slice(i, i + SQLITE_PARAM_CHUNK_SIZE);
|
||||
const placeholders = chunk.map(() => '?').join(',');
|
||||
changed += this.db.prepare(`DELETE FROM edges WHERE id IN (${placeholders})`).run(...chunk).changes;
|
||||
}
|
||||
})();
|
||||
return changed;
|
||||
}
|
||||
|
||||
/**
|
||||
* Distinct node names present in the given files — the symbol names a sync
|
||||
* pass uses to look up retryable failed refs after those files changed.
|
||||
@@ -2558,6 +2668,33 @@ export class QueryBuilder {
|
||||
return [...names];
|
||||
}
|
||||
|
||||
/**
|
||||
* Distinct `file\0name` pairs defined by the given files — the shape sync's
|
||||
* definition delta needs (CG-33).
|
||||
*
|
||||
* Deliberately NOT `getNodeNamesByFiles`: a bare name set is taken over the
|
||||
* WHOLE changed batch, so a name that moves between two files in one commit
|
||||
* (or exists in one changed file and is newly added to another) appears on
|
||||
* both sides and cancels out of the symmetric difference — even though a
|
||||
* definition genuinely appeared or vanished and every reference to that name
|
||||
* repo-wide may now bind elsewhere. Keying by file makes each definition its
|
||||
* own fact, so the move is seen as one removal plus one addition.
|
||||
*/
|
||||
getNodeNamePairsByFiles(filePaths: string[]): Set<string> {
|
||||
const pairs = new Set<string>();
|
||||
if (filePaths.length === 0) return pairs;
|
||||
for (let i = 0; i < filePaths.length; i += SQLITE_PARAM_CHUNK_SIZE) {
|
||||
const chunk = filePaths.slice(i, i + SQLITE_PARAM_CHUNK_SIZE);
|
||||
const placeholders = chunk.map(() => '?').join(',');
|
||||
const rows = this.db
|
||||
.prepare(`SELECT DISTINCT file_path, name FROM nodes WHERE file_path IN (${placeholders})`)
|
||||
.all(...chunk) as Array<{ file_path: string; name: string }>;
|
||||
// NUL-joined: a path or a symbol name can contain a space, never a NUL.
|
||||
for (const row of rows) pairs.add(`${row.file_path}\0${row.name}`);
|
||||
}
|
||||
return pairs;
|
||||
}
|
||||
|
||||
// ===========================================================================
|
||||
// Statistics
|
||||
// ===========================================================================
|
||||
|
||||
@@ -116,6 +116,20 @@ export interface SyncResult {
|
||||
nodesUpdated: number;
|
||||
durationMs: number;
|
||||
changedFilePaths?: string[];
|
||||
/**
|
||||
* Symbol names whose set of definitions this sync CHANGED — names the synced
|
||||
* files gained or lost, as the symmetric difference of their `file\0name`
|
||||
* definition pairs before and after the store phase (per file, so a name
|
||||
* moving between two changed files does not cancel itself out).
|
||||
* Resolution picks among all same-named definitions project-wide,
|
||||
* so these are exactly the names whose already-resolved edges — in files this
|
||||
* sync never touched — may now bind elsewhere and must be re-resolved for the
|
||||
* index to stay convergent with a full rebuild (CG-33).
|
||||
*
|
||||
* A body-only edit leaves this empty, which is the common case and costs
|
||||
* nothing downstream.
|
||||
*/
|
||||
definitionDelta?: string[];
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -2491,6 +2505,64 @@ export class ExtractionOrchestrator {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Re-open, for re-resolution, every resolution edge whose answer this sync
|
||||
* may have changed — the fix for index drift (CG-33).
|
||||
*
|
||||
* Incremental sync re-resolves only the references IN the changed files, but
|
||||
* resolution's answer is a function of the WHOLE graph: a reference binds to
|
||||
* one of the same-named definitions project-wide, so adding or removing a
|
||||
* definition of `pct` can change which `pct` every other file's `pct(...)`
|
||||
* should bind to. Those other files are never revisited, and their references
|
||||
* resolved successfully once and were deleted from `unresolved_refs`, so
|
||||
* nothing existed to revisit them with — the index kept an answer that was
|
||||
* correct against an older graph. Measured on codegraph's own long-lived
|
||||
* index: 4.3% of distinct edges differed from a clean rebuild, in BOTH
|
||||
* directions, overwhelmingly `calls`. See docs/benchmarks/index-drift-cg33.md.
|
||||
*
|
||||
* This deletes each affected edge and re-inserts it as the reference that
|
||||
* created it (the refName/refKind stamp), status='pending', for the sync's
|
||||
* resolution sweep to bind against the post-sync graph — the same input a
|
||||
* full rebuild resolves from, which is what makes the two converge.
|
||||
*
|
||||
* Deliberately conservative in three ways, because a wrong deletion is a
|
||||
* permanent edge loss while a missed rebind is only residual drift:
|
||||
* - an edge with no refName stamp (synthesized, or built by an engine older
|
||||
* than the stamp) is left ALONE rather than reconstructed from the target's
|
||||
* plain name, same rule as `resurrectRefFromDroppedEdge`;
|
||||
* - edges whose source is in a file this sync already re-extracted are
|
||||
* skipped — their references were re-resolved from scratch moments ago;
|
||||
* - very common names are skipped by the per-name ceiling in
|
||||
* `getResolutionEdgesByTargetName`.
|
||||
*
|
||||
* Returns the number of references resurrected.
|
||||
*/
|
||||
resurrectStaleResolutionEdges(definitionDelta: string[], changedFilePaths: string[]): number {
|
||||
if (definitionDelta.length === 0) return 0;
|
||||
const alreadyFresh = new Set(changedFilePaths);
|
||||
const candidates = this.queries.getResolutionEdgesByTargetName(definitionDelta);
|
||||
|
||||
const edgeIds: number[] = [];
|
||||
const refs: UnresolvedReference[] = [];
|
||||
for (const e of candidates) {
|
||||
if (alreadyFresh.has(e.sourceFilePath)) continue;
|
||||
const ref = resurrectRefFromDroppedEdge(e);
|
||||
if (!ref) continue; // no stamp — never delete what we cannot restore
|
||||
edgeIds.push(e.edgeId);
|
||||
refs.push(ref);
|
||||
}
|
||||
if (refs.length === 0) return 0;
|
||||
|
||||
// Delete first. The sweep re-inserts whichever edge resolution now picks,
|
||||
// and `insertEdges` is INSERT OR IGNORE against idx_edges_identity — so a
|
||||
// rebind to the same target is a clean no-op, but leaving the old row in
|
||||
// place for a rebind ELSEWHERE would keep both, turning drift into
|
||||
// duplication.
|
||||
this.queries.deleteEdgesByIds(edgeIds);
|
||||
this.queries.insertUnresolvedRefsBatch(refs);
|
||||
return refs.length;
|
||||
}
|
||||
|
||||
/**
|
||||
* Sync the index with the current file state.
|
||||
*
|
||||
@@ -2520,6 +2592,10 @@ export class ExtractionOrchestrator {
|
||||
let filesRemoved = 0;
|
||||
let nodesUpdated = 0;
|
||||
const changedFilePaths: string[] = [];
|
||||
// `file\0name` definition pairs for the files this sync touches, sampled
|
||||
// BEFORE their nodes are replaced/deleted. Compared against the post-store
|
||||
// pairs below to derive `definitionDelta` (CG-33).
|
||||
const pairsBefore = new Set<string>();
|
||||
|
||||
onProgress?.({
|
||||
phase: 'scanning',
|
||||
@@ -2585,6 +2661,9 @@ export class ExtractionOrchestrator {
|
||||
// failed until the symbol reappears somewhere. (A deleted file whose
|
||||
// CALLERS are also being deleted is fine: their nodes cascade later
|
||||
// in this loop and take the resurrected rows with them.)
|
||||
// Every name this file defined is about to stop existing here, which
|
||||
// narrows the candidate set for that name repo-wide (CG-33).
|
||||
for (const pair of this.queries.getNodeNamePairsByFiles([tracked.path])) pairsBefore.add(pair);
|
||||
const incoming = this.queries.getCrossFileIncomingEdgesWithTarget(tracked.path);
|
||||
if (incoming.length > 0) {
|
||||
const resurrected = incoming
|
||||
@@ -2651,6 +2730,14 @@ export class ExtractionOrchestrator {
|
||||
}
|
||||
}
|
||||
|
||||
// Sampled here — after the add/modify classification, before any file is
|
||||
// re-extracted — because `storeExtractionResult` deletes a file's nodes
|
||||
// before inserting the new ones, so this is the last point the pre-edit
|
||||
// definition set is readable (CG-33).
|
||||
if (filesToIndex.length > 0) {
|
||||
for (const pair of this.queries.getNodeNamePairsByFiles(filesToIndex)) pairsBefore.add(pair);
|
||||
}
|
||||
|
||||
// Load only grammars needed for changed files
|
||||
if (filesToIndex.length > 0) {
|
||||
const overrides = loadExtensionOverrides(this.rootDir);
|
||||
@@ -2677,6 +2764,25 @@ export class ExtractionOrchestrator {
|
||||
nodesUpdated += result.nodes.length;
|
||||
}
|
||||
|
||||
// Names whose definition set this sync changed: a `file\0name` pair present
|
||||
// before but not after (removed/renamed away) or after but not before
|
||||
// (added). A pair on both sides is untouched as far as resolution's
|
||||
// candidate set is concerned — only its node id moved, which
|
||||
// reattachCrossFileEdges already follows — so an edit that only changes
|
||||
// bodies yields an empty delta and no downstream rebind work (CG-33).
|
||||
//
|
||||
// Compared per FILE, not as one name set over the whole batch: a commit
|
||||
// that adds `collect` to a new file while an unrelated changed file already
|
||||
// defined `collect` must still flag the name, and a bare name set cancels
|
||||
// exactly that case out. That miss left the largest residual class in the
|
||||
// first measurement of this fix.
|
||||
const pairsAfter = this.queries.getNodeNamePairsByFiles(filesToIndex);
|
||||
const deltaNames = new Set<string>();
|
||||
const nameOf = (pair: string) => pair.slice(pair.indexOf('\0') + 1);
|
||||
for (const pair of pairsBefore) if (!pairsAfter.has(pair)) deltaNames.add(nameOf(pair));
|
||||
for (const pair of pairsAfter) if (!pairsBefore.has(pair)) deltaNames.add(nameOf(pair));
|
||||
const definitionDelta = [...deltaNames];
|
||||
|
||||
return {
|
||||
filesChecked,
|
||||
filesAdded,
|
||||
@@ -2685,6 +2791,7 @@ export class ExtractionOrchestrator {
|
||||
nodesUpdated,
|
||||
durationMs: Date.now() - startTime,
|
||||
changedFilePaths: changedFilePaths.length > 0 ? changedFilePaths : undefined,
|
||||
definitionDelta: definitionDelta.length > 0 ? definitionDelta : undefined,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -883,6 +883,32 @@ export class CodeGraph {
|
||||
}
|
||||
}
|
||||
|
||||
// Re-open resolution edges this sync may have invalidated ELSEWHERE in
|
||||
// the repo (CG-33). Everything above re-resolves references in the
|
||||
// changed files; this covers the opposite direction — references in
|
||||
// files the sync never touched whose answer depended on a definition
|
||||
// that just appeared or disappeared. Without it a synced index never
|
||||
// converges to a full rebuild: measured at 4.3% of distinct edges wrong
|
||||
// on codegraph's own index, in both directions, mostly `calls`. The
|
||||
// resurrected refs are pending rows, so the orphan sweep immediately
|
||||
// below is what resolves them — batched, yielding, multi-pass, exactly
|
||||
// as a full index resolves.
|
||||
//
|
||||
// `definitionDelta` is empty for a body-only edit, so the overwhelmingly
|
||||
// common sync pays one branch. CODEGRAPH_NO_REBIND=1 disables it.
|
||||
if (result.definitionDelta && process.env.CODEGRAPH_NO_REBIND !== '1') {
|
||||
const tRebind = Date.now();
|
||||
const rebound = this.orchestrator.resurrectStaleResolutionEdges(
|
||||
result.definitionDelta,
|
||||
result.changedFilePaths ?? []
|
||||
);
|
||||
if (process.env.CODEGRAPH_SYNTH_TIMINGS) {
|
||||
console.error(
|
||||
`[phase-timing] sync-rebind: ${Date.now() - tRebind}ms (${result.definitionDelta.length} changed names, ${rebound} edges re-opened)`
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// Orphan sweep (#1187). A resolution pass that dies mid-run — the #850
|
||||
// daemon liveness watchdog's SIGKILL (#1122), Ctrl-C, a crash — leaves
|
||||
// the refs it never reached in unresolved_refs, and the git-scoped fast
|
||||
|
||||
Reference in New Issue
Block a user