diff --git a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs index b2813740f..b1f12748f 100644 --- a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs +++ b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs @@ -719,7 +719,7 @@ pub fn run_pipeline( // than a "successful" build with missing data (#1827). let t0 = Instant::now(); let insert_batches = build_insert_batches(&file_symbols); - let file_hashes = build_file_hash_entries(&parse_changes); + let file_hashes = build_file_hash_entries(&parse_changes, &file_symbols); crate::domain::graph::builder::stages::insert_nodes::do_insert_nodes( conn, &insert_batches, @@ -1356,11 +1356,26 @@ fn build_insert_batches( /// For full builds, `detect_changes` returns `hash: None` because it skips /// reading file content. In that case we read and hash each file here so /// that `file_hashes` is populated for subsequent incremental builds. +/// +/// A changed file with no entry in `file_symbols` means extraction failed +/// outright (worker panic recovery, unreadable, unsupported/missing +/// grammar) — as opposed to a file that parsed successfully but +/// legitimately produced zero symbols, which DOES get an entry (with empty +/// `definitions`/`exports`; `parse_files_parallel`'s `filter_map` only +/// drops files where parsing itself never produced a tree). Committing a +/// hash for the former would mark it "up to date" relative to graph data +/// that was never written, permanently hiding the loss from every later +/// incremental build (issue #2441) — skip it instead, so the next build +/// still sees it as changed and reprocesses it. Mirrors +/// `iterFileHashRecords`'s `parsedRelPaths` check in +/// `src/domain/graph/builder/stages/insert-nodes.ts`. fn build_file_hash_entries( changed: &[&detect_changes::ChangedFile], + file_symbols: &BTreeMap, ) -> Vec { changed .iter() + .filter(|c| file_symbols.contains_key(&c.rel_path)) .filter_map(|c| { let hash = match c.hash.as_ref() { Some(h) => h.clone(), @@ -3054,4 +3069,58 @@ mod tests { Some("User") ); } + + fn changed_file(rel_path: &str, hash: &str) -> detect_changes::ChangedFile { + detect_changes::ChangedFile { + abs_path: format!("/repo/{rel_path}"), + rel_path: rel_path.to_string(), + content: None, + hash: Some(hash.to_string()), + mtime: 1000, + size: 10, + metadata_only: false, + reverse_dep_only: false, + } + } + + // Issue #2441: a changed file whose extraction failed outright (worker + // panic recovery, unreadable, unsupported/missing grammar) has no entry + // in file_symbols at all — must not get a committed hash, or the next + // incremental build wrongly believes its (missing) graph data is up to + // date with the file's new content, permanently hiding the loss. + #[test] + fn skips_a_changed_file_with_no_file_symbols_entry() { + let ok = changed_file("a.js", "hash-a"); + let failed = changed_file("b.js", "hash-b"); + let changed: Vec<&detect_changes::ChangedFile> = vec![&ok, &failed]; + + let mut file_symbols = BTreeMap::new(); + file_symbols.insert("a.js".to_string(), FileSymbols::new("a.js".to_string())); + // "b.js" intentionally has no entry — simulates a parse failure. + + let entries = build_file_hash_entries(&changed, &file_symbols); + let files: Vec<&str> = entries.iter().map(|e| e.file.as_str()).collect(); + assert_eq!(files, vec!["a.js"]); + } + + // A file that parsed successfully but legitimately produced zero symbols + // (empty file, parser no-op) DOES get a file_symbols entry (with empty + // definitions/exports) — it must still get a committed hash, or the + // no-op fast-skip pre-flight on the next rebuild would reject it as + // "missing from file_hashes" and force a full rebuild. + #[test] + fn still_includes_a_changed_file_that_parsed_with_zero_symbols() { + let empty = changed_file("empty.js", "hash-empty"); + let changed: Vec<&detect_changes::ChangedFile> = vec![&empty]; + + let mut file_symbols = BTreeMap::new(); + file_symbols.insert( + "empty.js".to_string(), + FileSymbols::new("empty.js".to_string()), + ); + + let entries = build_file_hash_entries(&changed, &file_symbols); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].file, "empty.js"); + } } diff --git a/src/domain/graph/builder/stages/insert-nodes.ts b/src/domain/graph/builder/stages/insert-nodes.ts index d98ddd97e..0328bc88a 100644 --- a/src/domain/graph/builder/stages/insert-nodes.ts +++ b/src/domain/graph/builder/stages/insert-nodes.ts @@ -165,6 +165,7 @@ function* iterFileHashRecords( metadataUpdates: MetadataUpdate[], rootDir: string, caller: string, + parsedRelPaths?: ReadonlySet, ): Generator { const seen = new Set(); @@ -176,6 +177,19 @@ function* iterFileHashRecords( const precomputed = precomputedData.get(relPath); if (precomputed?._reverseDepOnly) continue; + // #2441: a file that was actually scheduled for parsing (not a + // reverse-dep-only refresh, handled above) but produced no entry in + // fileSymbols means extraction failed outright — a crash, an unreadable + // file, or an unsupported/missing grammar — as opposed to a file that + // parsed successfully but legitimately produced zero symbols (#1068), + // which DOES get a fileSymbols entry (with empty definitions/exports). + // Committing this file's hash would mark it "up to date" relative to + // content whose graph data was never written, permanently hiding the + // loss from every later incremental build. Only enforced when the + // caller actually tracked parse outcomes (`parsedRelPaths` provided) — + // direct/synthetic callers keep the original unconditional behavior. + if (parsedRelPaths && !parsedRelPaths.has(relPath)) continue; + const record = resolveHashFromPrecomputed( relPath, precomputed ?? ({} as PrecomputedFileData), @@ -204,6 +218,13 @@ function* iterFileHashRecords( * `file_hashes`, which permanently breaks the JS-side fast-skip pre-flight on * any subsequent no-op rebuild (#1068). * + * `parsedRelPaths`, when provided, additionally excludes a file that was + * scheduled for parsing but has no entry there at all — a genuine parse + * failure rather than a legitimate empty result (#2441; see the check inside + * `iterFileHashRecords`). Omitted by direct/synthetic callers (e.g. this + * function's own unit tests), which don't track parse outcomes and keep the + * original unconditional #1068 behavior. + * * Exported for unit testing. */ export function buildFileHashes( @@ -211,6 +232,7 @@ export function buildFileHashes( precomputedData: Map, metadataUpdates: MetadataUpdate[], rootDir: string, + parsedRelPaths?: ReadonlySet, ): FileHashRecord[] { return [ ...iterFileHashRecords( @@ -219,6 +241,7 @@ export function buildFileHashes( metadataUpdates, rootDir, 'buildFileHashes', + parsedRelPaths, ), ]; } @@ -478,13 +501,19 @@ export async function insertNodes(ctx: PipelineContext): Promise { * correctly detects the file as still needing (re)processing. */ export function commitFileHashes(ctx: PipelineContext): void { - const { filesToParse, metadataUpdates, rootDir } = ctx; + const { filesToParse, metadataUpdates, rootDir, fileSymbols } = ctx; const precomputedData = new Map(); for (const item of filesToParse) { if (item.relPath) precomputedData.set(item.relPath, item as PrecomputedFileData); } - const fileHashes = buildFileHashes(filesToParse, precomputedData, metadataUpdates, rootDir); + const fileHashes = buildFileHashes( + filesToParse, + precomputedData, + metadataUpdates, + rootDir, + new Set(fileSymbols.keys()), + ); if (fileHashes.length === 0) return; if (ctx.engineName === 'native' && ctx.nativeDb?.healFileMetadata) { diff --git a/tests/integration/issue-2441-scoped-build-failed-parse-data-loss.test.ts b/tests/integration/issue-2441-scoped-build-failed-parse-data-loss.test.ts new file mode 100644 index 000000000..67180d6d0 --- /dev/null +++ b/tests/integration/issue-2441-scoped-build-failed-parse-data-loss.test.ts @@ -0,0 +1,199 @@ +/** + * Regression test for #2441: a scoped incremental build (`codegraph build + * --incremental` with an explicit `scope`) that fails to PARSE a changed + * file still committed a `file_hashes` row matching that file's new on-disk + * content — permanently hiding the data loss from every later incremental + * build. Same failure mode as #2435 (a `file_hashes` row that outlives the + * data it describes), but pre-existing and independent of that fix, which + * only protects `rebuildFile` (the `codegraph watch` path). + * + * Root cause: `handleScopedBuild` purges a changed file's nodes/edges + * BEFORE parsing runs — but deliberately leaves its OLD `file_hashes` row + * alone (deferred-commit design from #1731: a hash only ever advances once + * the data it describes has been rebuilt to match). `parseFiles` + * (`domain/graph/builder/stages/parse-files.ts`) then parses the changed + * set; a file whose extraction fails outright (worker crash, unreadable, + * unsupported/missing grammar) simply gets no entry in + * `ctx.allSymbols`/`ctx.fileSymbols` — but `commitFileHashes` + * (`domain/graph/builder/stages/insert-nodes.ts`) built its hash list from + * `ctx.filesToParse` directly, with no check that the file actually + * produced data, so the file's hash still got overwritten with a value + * matching its NEW on-disk content, even though the (missing) new state was + * never written. + * + * Unlike #2435, the purge itself is NOT deferred here (a much larger change + * given the purge and the parse live in different pipeline stages) — the + * fix only withholds the file_hashes commit, so the OLD (pre-edit) hash + * survives the failed build. Since it no longer matches the file's actual + * current content, the next incremental build correctly detects the file as + * still changed and reprocesses it, recovering the data. The read-failure + * half of this bug class doesn't apply on the scoped-build path: change + * detection reads each candidate up front and drops unreadable files from + * the changed set before the purge ever runs. + */ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import Database from 'better-sqlite3'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { buildGraph } from '../../src/domain/graph/builder.js'; +import { getWasmWorkerPool } from '../../src/domain/wasm-worker-pool.js'; + +function writeProject(dir: string, { callsTarget = true }: { callsTarget?: boolean } = {}) { + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'target.js'), 'export function target() { return 1; }\n'); + fs.writeFileSync( + path.join(dir, 'caller.js'), + callsTarget + ? "import { target } from './target.js';\nexport function run() { return target(); }\n" + : 'export function run() { return 0; }\n', + ); +} + +function withDb(dbPath: string, fn: (db: Database.Database) => T): T { + const db = new Database(dbPath, { readonly: true }); + try { + return fn(db); + } finally { + db.close(); + } +} + +function readSymbolNames(dbPath: string, file: string): string[] { + return withDb(dbPath, (db) => + ( + db + .prepare("SELECT name FROM nodes WHERE file = ? AND kind != 'file' ORDER BY name") + .all(file) as Array<{ name: string }> + ).map((r) => r.name), + ); +} + +function readIncomingCalls(dbPath: string, targetFile: string, targetName: string): string[] { + return withDb(dbPath, (db) => + ( + db + .prepare( + `SELECT n_src.file AS file, n_src.name AS name FROM edges e + JOIN nodes n_src ON e.source_id = n_src.id + JOIN nodes n_tgt ON e.target_id = n_tgt.id + WHERE e.kind = 'calls' AND n_tgt.file = ? AND n_tgt.name = ? + ORDER BY n_src.file, n_src.name`, + ) + .all(targetFile, targetName) as Array<{ file: string; name: string }> + ).map((r) => `${r.file}:${r.name}`), + ); +} + +function hasFileHashRow(dbPath: string, file: string): boolean { + return withDb( + dbPath, + (db) => db.prepare('SELECT 1 FROM file_hashes WHERE file = ?').get(file) !== undefined, + ); +} + +function readFileHash(dbPath: string, file: string): string | undefined { + return withDb( + dbPath, + (db) => + ( + db.prepare('SELECT hash FROM file_hashes WHERE file = ?').get(file) as + | { hash: string } + | undefined + )?.hash, + ); +} + +describe('Issue #2441: a scoped build that fails to parse a changed file', () => { + let tmpDir: string; + let dbPath: string; + + beforeEach(async () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-2441-')); + writeProject(tmpDir); + await buildGraph(tmpDir, { incremental: false, skipRegistry: true, engine: 'wasm' }); + dbPath = path.join(tmpDir, '.codegraph', 'graph.db'); + + expect(readSymbolNames(dbPath, 'caller.js')).toContain('run'); + expect(readIncomingCalls(dbPath, 'target.js', 'target')).toEqual(['caller.js:run']); + expect(hasFileHashRow(dbPath, 'caller.js')).toBe(true); + }); + + afterEach(() => { + vi.restoreAllMocks(); + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('does not commit a hash for the file, so a later scoped build recovers it', async () => { + const originalHash = readFileHash(dbPath, 'caller.js'); + expect(originalHash).toBeDefined(); + + const callerAbs = path.join(tmpDir, 'caller.js'); + // Change the file's on-disk content so change detection classifies it as + // changed, then force ITS parse specifically to fail (worker crash / + // soft error) — `WasmWorkerPool.parse` returning null is exactly what + // `parseFilesWasm` treats as "this file produced no output", the same + // signal a real crash or unsupported grammar would produce. + fs.writeFileSync( + callerAbs, + "import { target } from './target.js';\nexport function run() { return target() + 1; }\n", + ); + + const pool = getWasmWorkerPool(); + const actualParse = pool.parse.bind(pool); + vi.spyOn(pool, 'parse').mockImplementation(async (filePath, code, opts) => { + if (filePath === callerAbs) return null; + return actualParse(filePath, code, opts); + }); + + await buildGraph(tmpDir, { + incremental: true, + skipRegistry: true, + engine: 'wasm', + scope: ['caller.js'], + }); + + vi.restoreAllMocks(); + + // The purge already ran before the failed parse — caller.js's old data + // is gone. That half of the current behavior is unchanged by this fix + // (the issue's own "stronger" lazy-purge option is a separate, larger + // change) — what matters is that the file is NOT silently marked up to + // date with nothing to show for it: the row survives unchanged (#1731's + // deferred-commit design left it alone before the purge ever ran), so it + // no longer matches the file's actual current content. + expect(readSymbolNames(dbPath, 'caller.js')).toEqual([]); + expect(readFileHash(dbPath, 'caller.js')).toBe(originalHash); + + // A subsequent scoped build, with parsing succeeding normally this + // time, must actually reprocess the file rather than fast-skipping it + // as "unchanged" — proving the data loss is recoverable, not permanent. + await buildGraph(tmpDir, { + incremental: true, + skipRegistry: true, + engine: 'wasm', + scope: ['caller.js'], + }); + + expect(readSymbolNames(dbPath, 'caller.js')).toContain('run'); + expect(readIncomingCalls(dbPath, 'target.js', 'target')).toEqual(['caller.js:run']); + expect(readFileHash(dbPath, 'caller.js')).not.toBe(originalHash); + }); + + it('still commits the hash on a successful scoped build', async () => { + // Counterpart sanity check: the fix must not withhold the hash from a + // file that parsed fine — only from one that genuinely produced nothing. + writeProject(tmpDir, { callsTarget: false }); + + await buildGraph(tmpDir, { + incremental: true, + skipRegistry: true, + engine: 'wasm', + scope: ['caller.js'], + }); + + expect(readSymbolNames(dbPath, 'caller.js')).toContain('run'); + expect(readIncomingCalls(dbPath, 'target.js', 'target')).toEqual([]); + expect(hasFileHashRow(dbPath, 'caller.js')).toBe(true); + }); +});