From 7846d953e61ef3cc6c54e4abf5f0961d6855cc4b Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Mon, 17 Aug 2026 05:54:55 -0600 Subject: [PATCH] fix(builder): do not commit a file's hash when its parse produced no data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same failure mode as #2435 (a file_hashes row that outlives the data it describes), but on the codegraph build --incremental (scoped, and more generally any) build path rather than codegraph watch, and independent of that fix. handleScopedBuild purges a changed file's nodes/edges before parsing runs, deliberately leaving its OLD file_hashes row alone (the #1731 deferred-commit design: a hash only advances once the data it describes has been rebuilt to match). commitFileHashes/build_file_hash_entries then built their hash lists from the changed-file set directly, with no check that each file actually produced data — so a file whose extraction failed outright (worker crash, unreadable, unsupported/missing grammar) still got its hash committed as if the (missing) new state matched disk, silently and permanently hiding the loss from every later incremental build. Both engines shared this bug: the JS orchestration pipeline (commitFileHashes) via ctx.filesToParse, and the fully-native Rust pipeline (NativeDatabase::build_graph -> run_pipeline) via its own build_file_hash_entries, called independently as the primary native build path (JS runPipelineStages is only the fallback when native build_graph is unavailable or throws). Fix: skip a changed file's hash entry when it has no corresponding entry in fileSymbols/file_symbols — the reliable signal that extraction actually ran and produced (possibly empty) output, as opposed to no entry at all meaning it never ran. This preserves #1068's distinct guarantee (a file that parses successfully but legitimately produces zero symbols DOES get an entry, with empty definitions/exports, and must still get its hash committed or the no-op fast-skip pre-flight would reject it as missing). The purge itself is not deferred (a much larger change, since the purge and the parse live in different pipeline stages) — the surviving stale hash is what forces the next incremental build to reprocess the file and recover the data. docs check acknowledged Impact: 3 functions changed, 6 affected --- .../src/domain/graph/builder/pipeline.rs | 71 ++++++- .../graph/builder/stages/insert-nodes.ts | 33 ++- ...coped-build-failed-parse-data-loss.test.ts | 199 ++++++++++++++++++ 3 files changed, 300 insertions(+), 3 deletions(-) create mode 100644 tests/integration/issue-2441-scoped-build-failed-parse-data-loss.test.ts 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); + }); +});