-
Notifications
You must be signed in to change notification settings - Fork 12
fix(incremental): prevent duplicate edges on rebuild (#979) #998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
4c9c125
fix(incremental): prevent duplicate edges on rebuild (#979)
carlos-alm 19f7651
fix(incremental): use negative edge-kind filter for barrel re-parse (…
carlos-alm 0093a02
test(incremental): assert incremental edge total matches fresh full b…
carlos-alm 62075b2
Merge branch 'main' into fix/incremental-edge-duplication-979
carlos-alm 9a9b416
Merge branch 'main' into fix/incremental-edge-duplication-979
carlos-alm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import { run } from './consumers/driver.js'; | ||
|
|
||
| export function main(inputs) { | ||
| return run(inputs); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import { processAll } from '../core/index.js'; | ||
|
|
||
| export function run(inputs) { | ||
| return processAll(inputs); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| export function clampValue(v, lo, hi) { | ||
| if (v < lo) return lo; | ||
| if (v > hi) return hi; | ||
| return v; | ||
| } | ||
|
|
||
| export function doubleValue(v) { | ||
| return v * 2; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| // Hybrid barrel: re-exports from helpers.js AND has local definitions that call helpers. | ||
| export { doubleValue } from './helpers.js'; | ||
|
|
||
| import { clampValue, doubleValue } from './helpers.js'; | ||
|
|
||
| export function processValue(v) { | ||
| return doubleValue(clampValue(v, 0, 100)); | ||
| } | ||
|
|
||
| export function processAll(values) { | ||
| return values.map((v) => processValue(v)); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| /** | ||
| * Regression test for #979: incremental rebuilds leak duplicate edges. | ||
| * | ||
| * Root cause: when `reparse_barrel_candidates` (Stage 6b, native engine) picks | ||
| * up a file imported by a reverse-dep, it used to purge only the 'imports' and | ||
| * 'reexports' edge kinds before Stage 7 re-emitted every edge kind, so every | ||
| * rebuild appended new copies of 'calls', 'receiver', 'extends', 'implements', | ||
| * 'imports-type', and 'dynamic-imports' edges. | ||
| * | ||
| * This test modifies a source file multiple times in a row and asserts: | ||
| * 1. The total edge count does not grow across incremental rebuilds. | ||
| * 2. The count of `(source_id, target_id, kind)` rows never exceeds the | ||
| * pre-existing duplicates from a fresh full build (i.e. incremental | ||
| * does not introduce new duplicates). | ||
| */ | ||
| import fs from 'node:fs'; | ||
| import os from 'node:os'; | ||
| import path from 'node:path'; | ||
| import Database from 'better-sqlite3'; | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { buildGraph } from '../../src/domain/graph/builder.js'; | ||
|
|
||
| const FIXTURE_DIR = path.join(import.meta.dirname, '..', 'fixtures', 'issue-979-hybrid-barrel'); | ||
|
|
||
| function copyDirSync(src: string, dest: string) { | ||
| fs.mkdirSync(dest, { recursive: true }); | ||
| for (const entry of fs.readdirSync(src, { withFileTypes: true })) { | ||
| const s = path.join(src, entry.name); | ||
| const d = path.join(dest, entry.name); | ||
| if (entry.isDirectory()) copyDirSync(s, d); | ||
| else fs.copyFileSync(s, d); | ||
| } | ||
| } | ||
|
|
||
| function edgeStats(dbPath: string) { | ||
| const db = new Database(dbPath, { readonly: true }); | ||
| try { | ||
| const total = (db.prepare('SELECT COUNT(*) AS c FROM edges').get() as { c: number }).c; | ||
| const duplicates = ( | ||
| db | ||
| .prepare( | ||
| `SELECT source_id, target_id, kind, COUNT(*) AS c FROM edges | ||
| GROUP BY source_id, target_id, kind HAVING c > 1`, | ||
| ) | ||
| .all() as Array<{ c: number }> | ||
| ).reduce((sum, row) => sum + row.c - 1, 0); | ||
| return { total, duplicates }; | ||
| } finally { | ||
| db.close(); | ||
| } | ||
| } | ||
|
|
||
| describe('Issue #979: incremental edges do not duplicate', () => { | ||
| it('3 incremental rebuilds produce stable edge counts with no new duplicates', async () => { | ||
| const tmpBase = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-979-')); | ||
| const fullDir = path.join(tmpBase, 'full'); | ||
| const incrDir = path.join(tmpBase, 'incr'); | ||
|
|
||
| try { | ||
| copyDirSync(FIXTURE_DIR, fullDir); | ||
| copyDirSync(FIXTURE_DIR, incrDir); | ||
|
|
||
| // Baseline full build on the incr copy so subsequent rebuilds are truly incremental. | ||
| await buildGraph(incrDir, { incremental: false, skipRegistry: true }); | ||
|
|
||
| // Apply 3 rounds of "change one file" + incremental rebuild, recording | ||
| // edge totals and duplicate counts after each rebuild. | ||
| const history: Array<{ total: number; duplicates: number }> = []; | ||
| for (let i = 0; i < 3; i++) { | ||
| fs.appendFileSync(path.join(incrDir, 'consumers', 'driver.js'), `\n// bump ${i}\n`); | ||
| await buildGraph(incrDir, { incremental: true, skipRegistry: true }); | ||
| history.push(edgeStats(path.join(incrDir, '.codegraph', 'graph.db'))); | ||
| } | ||
|
|
||
| // Mirror all 3 mutations on the full copy, then do a single clean full build. | ||
| for (let i = 0; i < 3; i++) { | ||
| fs.appendFileSync(path.join(fullDir, 'consumers', 'driver.js'), `\n// bump ${i}\n`); | ||
| } | ||
| await buildGraph(fullDir, { incremental: false, skipRegistry: true }); | ||
| const freshFull = edgeStats(path.join(fullDir, '.codegraph', 'graph.db')); | ||
|
|
||
| // Invariant 1: incremental edge count must not grow across rebuilds. | ||
| expect(history[1].total).toBe(history[0].total); | ||
| expect(history[2].total).toBe(history[0].total); | ||
|
|
||
| // Invariant 2: incremental must not introduce new duplicates beyond the | ||
| // pre-existing duplicates present in a clean full build. | ||
| expect(history[2].duplicates).toBeLessThanOrEqual(freshFull.duplicates); | ||
|
|
||
| // Invariant 3: after applying all 3 bumps, both dirs describe the same | ||
| // code, so the incremental edge total must match a clean full build. | ||
| // This catches stale edges that survive the scoped DELETE (e.g. edges | ||
| // pointing at orphaned node ids) which would not be flagged as | ||
| // (source, target, kind) duplicates. | ||
| expect(history[2].total).toBe(freshFull.total); | ||
| } finally { | ||
| fs.rmSync(tmpBase, { recursive: true, force: true }); | ||
| } | ||
| }, 60_000); | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invariant 1 proves the count stabilises across rebuild 2 and 3 relative to rebuild 1, but does not assert that rebuild 1 (
history[0]) produces the same total edge count as a clean full build over the same code state. A scenario where the first incremental rebuild leaves stale edges that happen to not be flagged as(source, target, kind)duplicates (e.g. edges pointing at a stale node id) would pass both invariants. Adding the following assertion would close the gap:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 0093a02 as
Invariant 3:expect(history[2].total).toBe(freshFull.total). This closes the gap where stale edges pointing at orphaned node ids would slip past the(source, target, kind)duplicate check. Test still passes locally.