diff --git a/CHANGELOG.md b/CHANGELOG.md index 0dfc0e9..fb61a44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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) diff --git a/__tests__/sync-rebuild-convergence.test.ts b/__tests__/sync-rebuild-convergence.test.ts new file mode 100644 index 0000000..78ce4dd --- /dev/null +++ b/__tests__/sync-rebuild-convergence.test.ts @@ -0,0 +1,271 @@ +/** + * 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. + */ + +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 => { + 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(); + } + }; + + /** Human-readable diff, so a failure names the edges instead of just a count. */ + const describeDiff = (synced: Set, rebuilt: Set): 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. `indexAll` recreates the database file, so this is the same ground + * truth a user gets from `codegraph index`. + */ + const rebuildEdgeSet = async (): Promise> => { + 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'); + }); + + /** + * 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'); + }); +}); diff --git a/docs/benchmarks/index-drift-cg33.md b/docs/benchmarks/index-drift-cg33.md index 548e793..58fdce4 100644 --- a/docs/benchmarks/index-drift-cg33.md +++ b/docs/benchmarks/index-drift-cg33.md @@ -46,19 +46,98 @@ no duplicate nodes, no orphan edges, no nodes referencing a missing file row. Nothing accumulates. Cross-file **resolution** goes stale. -## Likely mechanism +## Mechanism — two causes, both confirmed -`ReferenceResolver` resolves calls and imports by name-matching and the import -graph across the **whole** project. Incremental sync re-parses and re-resolves -only the changed file, so: +`ReferenceResolver` binds a reference to one of the same-named definitions +**project-wide**. Two things follow, and the drift needed both to be fixed. -- edges from *other* files into changed symbols are never recomputed → stale - edges retained (the 476); -- edges that should newly form from unchanged files into changed symbols are - never created → missing edges (the 751). +**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. -Start in `src/sync/` and `src/resolution/` — specifically what scope is -re-resolved on a single-file change. +**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 @@ -93,6 +172,13 @@ node scripts/agent-eval/diff-index-drift.mjs /tmp/live.db .codegraph/codegraph.d 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 && 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` diff --git a/src/db/queries.ts b/src/db/queries.ts index bad4a26..a0ab11f 100644 --- a/src/db/queries.ts +++ b/src/db/queries.ts @@ -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); @@ -2445,6 +2462,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 { + 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 = []; + 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; + 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. @@ -2463,6 +2573,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 { + const pairs = new Set(); + 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 // =========================================================================== diff --git a/src/extraction/index.ts b/src/extraction/index.ts index 4e4af4a..22108d1 100644 --- a/src/extraction/index.ts +++ b/src/extraction/index.ts @@ -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(); 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(); + 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, }; } diff --git a/src/index.ts b/src/index.ts index 86dae6c..e8d5377 100644 --- a/src/index.ts +++ b/src/index.ts @@ -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