diff --git a/crates/codegraph-core/src/db/connection.rs b/crates/codegraph-core/src/db/connection.rs index f75c34c3..12dbda1c 100644 --- a/crates/codegraph-core/src/db/connection.rs +++ b/crates/codegraph-core/src/db/connection.rs @@ -857,6 +857,14 @@ impl NativeDatabase { /// Batches are received as `serde_json::Value` and deserialized via serde so /// that `null` visibility values map to `None` instead of crashing napi's /// `Option` object conversion (#709). + /// + /// `file_hashes` is committed in its own transaction, separate from node + /// insertion (#1731) — callers that need edge-consistent hashes (i.e. the + /// standard incremental build pipeline) should pass an empty array here + /// and commit hashes themselves once resolveImports/buildEdges have + /// finished rebuilding the affected files' edges (see + /// `insertNodes.commitFileHashes` on the JS side, or + /// `insert_nodes::commit_file_hashes` for the all-Rust orchestrator). #[napi(ts_args_type = "batches: Array<{ file: string; definitions: Array<{ name: string; kind: string; line: number; endLine?: number; visibility?: string; children: Array<{ name: string; kind: string; line: number; endLine?: number; visibility?: string }> }>; exports: Array<{ name: string; kind: string; line: number }> }>, fileHashes: FileHashEntry[], removedFiles: string[]")] pub fn bulk_insert_nodes( &self, @@ -869,9 +877,16 @@ impl NativeDatabase { napi::Error::from_reason(format!("bulk_insert_nodes: invalid batches: {e}")) })?; let conn = self.conn()?; - Ok(insert_nodes::do_insert_nodes(conn, &batches, &file_hashes, &removed_files) + let insert_ok = insert_nodes::do_insert_nodes(conn, &batches, &removed_files) .inspect_err(|e| eprintln!("[NativeDatabase] bulk_insert_nodes failed: {e}")) - .is_ok()) + .is_ok(); + if !insert_ok { + return Ok(false); + } + let hashes_ok = insert_nodes::commit_file_hashes(conn, &file_hashes) + .inspect_err(|e| eprintln!("[NativeDatabase] bulk_insert_nodes hash commit failed: {e}")) + .is_ok(); + Ok(hashes_ok) } /// Bulk-insert edge rows using chunked multi-value INSERT statements. diff --git a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs index f191b9f6..ea4442e3 100644 --- a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs +++ b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs @@ -9,10 +9,13 @@ //! 2. Collect files (with gitignore + extension filter) //! 3. Detect changes (tiered: journal/mtime/hash) //! 4. Parse files in parallel (existing `parallel::parse_files_parallel`) -//! 5. Insert nodes (existing `insert_nodes::do_insert_nodes`) +//! 5. Insert nodes (existing `insert_nodes::do_insert_nodes`) — file_hashes +//! for changed files is NOT written here; see step 7 //! 6. Resolve imports (existing `resolve::resolve_imports_batch`) //! 6b. Re-parse barrel candidates (incremental only) -//! 7. Build import edges + call edges + barrel resolution +//! 7. Build import edges + call edges + barrel resolution, then commit +//! file_hashes for changed files (`insert_nodes::commit_file_hashes`) now +//! that their edges match this revision (#1731) //! 8. Structure metrics + role classification //! 9. Finalize (metadata, journal) @@ -505,13 +508,20 @@ pub fn run_pipeline( timing.parse_ms = t0.elapsed().as_secs_f64() * 1000.0; // ── Stage 5: Insert nodes ────────────────────────────────────────── + // file_hashes for these files is deliberately NOT written here — only + // node/edge-adjacent (contains, parameter_of) data plus removed-file hash + // cleanup. Committing a changed file's hash this early (before Stage 7 + // rebuilds its import/call edges) would let the hash claim "up to date" + // even if edge-building later fails or is interrupted, permanently + // desyncing file_hashes from the edges it's supposed to gate re-parsing + // on (#1731). The hash is committed at the end of Stage 7 instead, once + // edges genuinely match this revision — see `commit_file_hashes` below. let t0 = Instant::now(); let insert_batches = build_insert_batches(&file_symbols); let file_hashes = build_file_hash_entries(&parse_changes); let _ = crate::domain::graph::builder::stages::insert_nodes::do_insert_nodes( conn, &insert_batches, - &file_hashes, &change_result.removed, ); detect_changes::heal_metadata(conn, &change_result.metadata_updates); @@ -564,6 +574,15 @@ pub fn run_pipeline( build_and_insert_call_edges(conn, &file_symbols, &import_ctx, !change_result.is_full_build); reconnect_saved_reverse_dep_edges(conn, &saved_reverse_dep_edges); + + // Now that edges reflect this revision, commit file_hashes for the + // changed files (#1731). Deferred from Stage 5 — see the comment there. + if let Err(e) = crate::domain::graph::builder::stages::insert_nodes::commit_file_hashes( + conn, + &file_hashes, + ) { + eprintln!("[codegraph] commit_file_hashes failed: {e}"); + } timing.edges_ms = t0.elapsed().as_secs_f64() * 1000.0; // ── Stage 8: Structure + roles ───────────────────────────────────── diff --git a/crates/codegraph-core/src/domain/graph/builder/stages/insert_nodes.rs b/crates/codegraph-core/src/domain/graph/builder/stages/insert_nodes.rs index 69b4519a..bb81b878 100644 --- a/crates/codegraph-core/src/domain/graph/builder/stages/insert_nodes.rs +++ b/crates/codegraph-core/src/domain/graph/builder/stages/insert_nodes.rs @@ -94,17 +94,41 @@ fn query_node_ids( Ok(map) } +/// Insert nodes/children/containment edges, plus hash cleanup for removed +/// files. `file_hashes` for CHANGED files is intentionally NOT written here — +/// see [`commit_file_hashes`] below (#1731): committing a file's hash this +/// early (before import/call edges are built) would let the hash claim +/// "up to date" even if edge-building later fails or is interrupted. Deleting +/// a removed file's hash has no such coupling risk (the file has no edges to +/// keep in sync with), so `removed_files` is still handled immediately. pub(crate) fn do_insert_nodes( conn: &Connection, batches: &[InsertNodesBatch], - file_hashes: &[FileHashEntry], removed_files: &[String], ) -> rusqlite::Result<()> { let tx = conn.unchecked_transaction()?; insert_file_nodes(&tx, batches)?; let (contains_edges, param_of_edges) = insert_symbol_nodes(&tx, batches)?; upsert_node_batch(&tx, &contains_edges, ¶m_of_edges)?; - upsert_file_hashes(&tx, file_hashes, removed_files)?; + delete_removed_file_hashes(&tx, removed_files)?; + tx.commit() +} + +/// Commit `file_hashes` for changed files. Called separately from +/// [`do_insert_nodes`], strictly AFTER import/call edges have been rebuilt +/// for these files (#1731) — see that function's doc comment and the call +/// site in `pipeline.rs` Stage 7. Own transaction: node insertion and hash +/// commit are two distinct steps in the coupling this fix establishes, not +/// one atomic unit. +pub(crate) fn commit_file_hashes( + conn: &Connection, + file_hashes: &[FileHashEntry], +) -> rusqlite::Result<()> { + if file_hashes.is_empty() { + return Ok(()); + } + let tx = conn.unchecked_transaction()?; + upsert_file_hashes(&tx, file_hashes)?; tx.commit() } @@ -297,43 +321,57 @@ fn upsert_node_batch( Ok(()) } -/// Phase 4: upsert file hashes and remove hashes for deleted files. No-ops -/// gracefully when the `file_hashes` table has not been created yet (e.g. -/// during the initial schema migration). +/// Returns true iff the `file_hashes` table exists. Both halves of the old +/// combined upsert/delete function (now [`upsert_file_hashes`] and +/// [`delete_removed_file_hashes`]) no-op gracefully when it's absent — e.g. +/// during the initial schema migration. +fn has_file_hashes_table(tx: &rusqlite::Transaction) -> bool { + tx.prepare("SELECT 1 FROM sqlite_master WHERE type='table' AND name='file_hashes'") + .and_then(|mut s| s.query_row([], |_| Ok(true))) + .unwrap_or(false) +} + +/// Phase 4a: upsert file hashes for changed files. Split out from the +/// removed-files delete (below) so the two can be committed at different +/// points in the pipeline — see [`commit_file_hashes`] (#1731). fn upsert_file_hashes( tx: &rusqlite::Transaction, file_hashes: &[FileHashEntry], - removed_files: &[String], ) -> rusqlite::Result<()> { - let has_file_hashes = tx - .prepare("SELECT 1 FROM sqlite_master WHERE type='table' AND name='file_hashes'") - .and_then(|mut s| s.query_row([], |_| Ok(true))) - .unwrap_or(false); - - if !has_file_hashes { + if file_hashes.is_empty() || !has_file_hashes_table(tx) { return Ok(()); } - { - let mut upsert = tx.prepare_cached( - "INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) \ - VALUES (?1, ?2, ?3, ?4)", - )?; - for entry in file_hashes { - upsert.execute(params![ - &entry.file, - &entry.hash, - entry.mtime as i64, - entry.size as i64 - ])?; - } + let mut upsert = tx.prepare_cached( + "INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) \ + VALUES (?1, ?2, ?3, ?4)", + )?; + for entry in file_hashes { + upsert.execute(params![ + &entry.file, + &entry.hash, + entry.mtime as i64, + entry.size as i64 + ])?; } - if !removed_files.is_empty() { - let mut delete = tx.prepare_cached("DELETE FROM file_hashes WHERE file = ?1")?; - for file in removed_files { - delete.execute(params![file])?; - } + Ok(()) +} + +/// Phase 4b: remove file_hashes rows for deleted files. Safe to run +/// immediately alongside node insertion (unlike the upsert above) — a +/// removed file has no edges that need to stay in sync with its hash. +fn delete_removed_file_hashes( + tx: &rusqlite::Transaction, + removed_files: &[String], +) -> rusqlite::Result<()> { + if removed_files.is_empty() || !has_file_hashes_table(tx) { + return Ok(()); + } + + let mut delete = tx.prepare_cached("DELETE FROM file_hashes WHERE file = ?1")?; + for file in removed_files { + delete.execute(params![file])?; } Ok(()) diff --git a/src/domain/graph/builder/incremental.ts b/src/domain/graph/builder/incremental.ts index 29c17ee1..c16f0edf 100644 --- a/src/domain/graph/builder/incremental.ts +++ b/src/domain/graph/builder/incremental.ts @@ -28,7 +28,7 @@ import { resolveReceiverEdge, resolveSameClassQualifiedMethod, } from './call-resolver.js'; -import { BUILTIN_RECEIVERS, readFileSafe } from './helpers.js'; +import { BUILTIN_RECEIVERS, fileHash, fileStat, readFileSafe } from './helpers.js'; import { importNamePairs } from './import-utils.js'; // ── Local types ───────────────────────────────────────────────────────── @@ -42,6 +42,14 @@ export interface IncrementalStmts { listSymbols: { all: (...params: unknown[]) => unknown[] }; findNodeInFile: { all: (...params: unknown[]) => unknown[] }; findNodeByName: { all: (...params: unknown[]) => unknown[] }; + /** + * Upsert a `file_hashes` row: `(relPath, hash, mtime, size)`. Called only + * after a file's edges have been fully rebuilt (#1731) — see the call site + * in `rebuildFile` for why this can't happen any earlier. + */ + upsertFileHash: { run: (...params: unknown[]) => unknown }; + /** Delete a `file_hashes` row for a file removed from disk. */ + deleteFileHash: { run: (...params: unknown[]) => unknown }; } interface RebuildResult { @@ -938,6 +946,10 @@ export async function rebuildFile( if (!fs.existsSync(filePath)) { if (cache) (cache as { remove(p: string): void }).remove(filePath); + // The file no longer exists, so it has no edges to keep in sync with a + // hash — delete it immediately (mirrors the full-build removed-file path + // in insertNodes.ts, which is likewise unconditional). + stmts.deleteFileHash.run(relPath); return buildDeletionResult(relPath, oldNodes, edgesBefore, oldSymbols, diffSymbols); } @@ -982,6 +994,18 @@ export async function rebuildFile( // their edges unchanged. const totalEdgesBefore = edgesBefore + reverseDepsEdgesBefore; + // Commit file_hashes now that relPath's edges have been fully rebuilt to + // match `code` (#1731). Writing this any earlier — or not at all, as + // before this fix — would leave file_hashes stale relative to the edges + // rebuildEdgesForTargetFile just wrote, so the next full/incremental + // `codegraph build` would either redundantly reprocess an already-correct + // file (stale-hash direction) or, combined with other divergent writers, + // risk trusting a hash that doesn't actually reflect these edges. + const stat = fileStat(filePath); + if (stat) { + stmts.upsertFileHash.run(relPath, fileHash(code), stat.mtime, stat.size); + } + const symbolDiff = diffSymbols ? diffSymbols(oldSymbols, newSymbols) : null; const event = oldNodes === 0 ? 'added' : 'modified'; diff --git a/src/domain/graph/builder/pipeline.ts b/src/domain/graph/builder/pipeline.ts index 28e4eb0d..7f3bf09b 100644 --- a/src/domain/graph/builder/pipeline.ts +++ b/src/domain/graph/builder/pipeline.ts @@ -39,7 +39,7 @@ import { buildStructure } from './stages/build-structure.js'; import { collectFiles } from './stages/collect-files.js'; import { detectChanges, detectNoChanges } from './stages/detect-changes.js'; import { finalize } from './stages/finalize.js'; -import { insertNodes } from './stages/insert-nodes.js'; +import { commitFileHashes, insertNodes } from './stages/insert-nodes.js'; import { closeNativeDb, refreshJsDb, @@ -319,6 +319,14 @@ async function runPipelineStages(ctx: PipelineContext): Promise { await resolveImports(ctx); await buildEdges(ctx); + + // Commit file_hashes for changed files now that their edges have been + // rebuilt to match (#1731) — see commitFileHashes() for the rationale. + // Placed before buildStructure/runAnalyses so a failure there still + // benefits from a hash that accurately reflects the (now-consistent) + // node+edge state committed so far. + commitFileHashes(ctx); + await buildStructure(ctx); // Reopen nativeDb for feature modules (ast, cfg, complexity, dataflow). diff --git a/src/domain/graph/builder/stages/insert-nodes.ts b/src/domain/graph/builder/stages/insert-nodes.ts index c0459a2c..0d786d24 100644 --- a/src/domain/graph/builder/stages/insert-nodes.ts +++ b/src/domain/graph/builder/stages/insert-nodes.ts @@ -2,11 +2,16 @@ * Stage: insertNodes * * Batch-inserts file nodes, definitions, exports, children, and contains/parameter_of edges. - * Updates file hashes for incremental builds. * * When the native engine is available, delegates all SQLite writes to Rust via * `bulkInsertNodes` — eliminating JS↔C boundary overhead. Falls back to the * JS implementation on failure or when native is unavailable. + * + * Does NOT write file_hashes for changed files (only removed-file cleanup, + * which carries no coupling risk — see commitFileHashes below). Hashes for + * changed files are committed later, once resolveImports/buildEdges have + * finished rebuilding their edges, so a hash can never claim a file is + * "up to date" while its edges still reflect an older revision (#1731). */ import path from 'node:path'; import { performance } from 'node:perf_hooks'; @@ -19,7 +24,6 @@ import type { ExtractorOutput, FileToParse, MetadataUpdate, - SqliteStatement, } from '../../../../types.js'; import type { PipelineContext } from '../context.js'; import { @@ -218,28 +222,26 @@ export function buildFileHashes( function tryNativeInsert(ctx: PipelineContext): boolean { if (!ctx.nativeDb?.bulkInsertNodes) return false; - const { allSymbols, filesToParse, metadataUpdates, rootDir, removed } = ctx; + const { allSymbols, removed } = ctx; const batches = marshalSymbolBatches(allSymbols); - 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); - - // In native-first mode (single rusqlite connection), no WAL dance is needed. - // In dual-connection mode, checkpoint JS side before native write, then - // checkpoint native side after (#696, #709, #715, #717). + // file_hashes is intentionally NOT written here. Committing a file's hash + // this early (before resolveImports/buildEdges have run) would let the + // hash claim "up to date" even if edge-building later throws or is + // interrupted — see commitFileHashes below, called once edges are in + // place (#1731). `removed` is still passed through: deleting a removed + // file's hash has no such coupling risk (the file has no edges to keep + // in sync with). let result: boolean; if (ctx.nativeFirstProxy) { - result = ctx.nativeDb!.bulkInsertNodes(batches, fileHashes, removed); + result = ctx.nativeDb!.bulkInsertNodes(batches, [], removed); } else { try { if (ctx.db) { ctx.db.pragma('wal_checkpoint(TRUNCATE)'); } - result = ctx.nativeDb!.bulkInsertNodes(batches, fileHashes, removed); + result = ctx.nativeDb!.bulkInsertNodes(batches, [], removed); } finally { try { ctx.nativeDb?.exec('PRAGMA wal_checkpoint(TRUNCATE)'); @@ -380,36 +382,10 @@ function insertChildrenAndEdges( batchInsertEdges(db, edgeRows); } -// ── JS fallback: Phase 4 ──────────────────────────────────────────── - -function updateFileHashes( - _db: BetterSqlite3Database, - filesToParse: FileToParse[], - precomputedData: Map, - metadataUpdates: MetadataUpdate[], - rootDir: string, - upsertHash: SqliteStatement | null, -): void { - if (!upsertHash) return; - - // Iterate every collected file (#1068): files that produced zero symbols - // (empty, parser no-op, or grammar-missing optional language) still need a - // hash row, otherwise the next no-op rebuild's fast-skip pre-flight rejects. - for (const record of iterFileHashRecords( - filesToParse, - precomputedData, - metadataUpdates, - rootDir, - 'updateFileHashes', - )) { - upsertHash.run(record.file, record.hash, record.mtime, record.size); - } -} - // ── Main entry point ──────────────────────────────────────────────── export async function insertNodes(ctx: PipelineContext): Promise { - const { allSymbols, filesToParse, metadataUpdates, rootDir, removed } = ctx; + const { allSymbols, removed } = ctx; // Populate fileSymbols before any DB writes (used by later stages) for (const [relPath, symbols] of allSymbols) { @@ -423,7 +399,9 @@ export async function insertNodes(ctx: PipelineContext): Promise { try { if (tryNativeInsert(ctx)) { ctx.timing.insertMs = performance.now() - t0; - // Removed-file hash cleanup is handled inside the native call + // Removed-file hash cleanup is handled inside the native call. + // Content-changed files' hashes are committed later by + // commitFileHashes(), once their edges exist (#1731). return; } } catch (e) { @@ -431,36 +409,82 @@ export async function insertNodes(ctx: PipelineContext): Promise { } } - // JS fallback - const precomputedData = new Map(); - for (const item of filesToParse) { - if (item.relPath) precomputedData.set(item.relPath, item as PrecomputedFileData); - } - - let upsertHash: SqliteStatement | null; - try { - upsertHash = ctx.db.prepare( - 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', - ); - } catch (e) { - debug(`insertNodes: file_hashes prepare failed (table may not exist): ${toErrorMessage(e)}`); - upsertHash = null; - } - + // JS fallback — node/edge insertion only. file_hashes for changed files is + // intentionally NOT written here: see commitFileHashes() below (#1731). const insertAll = ctx.db.transaction(() => { insertDefinitionsAndExports(ctx.db, allSymbols); insertChildrenAndEdges(ctx.db, allSymbols); - updateFileHashes(ctx.db, filesToParse, precomputedData, metadataUpdates, rootDir, upsertHash); }); insertAll(); ctx.timing.insertMs = performance.now() - t0; - // Clean up removed file hashes - if (upsertHash && removed.length > 0) { - const deleteHash = ctx.db.prepare('DELETE FROM file_hashes WHERE file = ?'); - for (const relPath of removed) { - deleteHash.run(relPath); + // Clean up removed file hashes. Safe to do immediately (unlike the upsert + // path): a removed file has no edges that need to stay in sync with its hash. + if (removed.length > 0) { + try { + const deleteHash = ctx.db.prepare('DELETE FROM file_hashes WHERE file = ?'); + for (const relPath of removed) { + deleteHash.run(relPath); + } + } catch (e) { + debug( + `insertNodes: removed-file hash cleanup failed (table may not exist): ${toErrorMessage(e)}`, + ); + } + } +} + +// ── Deferred file_hashes commit ────────────────────────────────────── + +/** + * Commit `file_hashes` for every changed/parsed file, plus metadata-only + * healing entries. Called by the pipeline strictly AFTER resolveImports and + * buildEdges have finished rebuilding those files' edges. + * + * This is the fix for #1731: previously, `insertNodes` wrote file_hashes in + * the same transaction as node insertion — BEFORE resolveImports/buildEdges + * ran. Any exception, crash, or interruption between that write and the + * (separate) edge-building transaction(s) left the DB with a hash that + * claimed "up to date" while edges still reflected the previous revision (or + * were missing entirely) — and since change-detection trusts file_hashes + * exclusively, that divergence was never self-healed by later builds. + * + * Deferring the write here restores the invariant: a file's hash only ever + * advances once its edges have been rebuilt to match. If anything upstream + * throws before this point, the hash keeps its old value, so the next build + * correctly detects the file as still needing (re)processing. + */ +export function commitFileHashes(ctx: PipelineContext): void { + const { filesToParse, metadataUpdates, rootDir } = 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); + if (fileHashes.length === 0) return; + + if (ctx.engineName === 'native' && ctx.nativeDb?.healFileMetadata) { + try { + ctx.nativeDb.healFileMetadata(fileHashes); + } catch (e) { + debug(`commitFileHashes: native healFileMetadata failed: ${toErrorMessage(e)}`); } + return; + } + + try { + const upsertHash = ctx.db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ); + const commitTx = ctx.db.transaction(() => { + for (const record of fileHashes) { + upsertHash.run(record.file, record.hash, record.mtime, record.size); + } + }); + commitTx(); + } catch (e) { + debug(`commitFileHashes: file_hashes write failed (table may not exist): ${toErrorMessage(e)}`); } } diff --git a/src/domain/graph/watcher.ts b/src/domain/graph/watcher.ts index 703a4524..97b86d06 100644 --- a/src/domain/graph/watcher.ts +++ b/src/domain/graph/watcher.ts @@ -44,6 +44,10 @@ function prepareWatcherStatements(db: ReturnType): IncrementalStm "SELECT id, file, kind FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant')", ), listSymbols: db.prepare("SELECT name, kind, line FROM nodes WHERE file = ? AND kind != 'file'"), + upsertFileHash: db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ), + deleteFileHash: db.prepare('DELETE FROM file_hashes WHERE file = ?'), }; } diff --git a/tests/integration/issue-1259-watch-call-resolution.test.ts b/tests/integration/issue-1259-watch-call-resolution.test.ts index affbf8c8..78e563c3 100644 --- a/tests/integration/issue-1259-watch-call-resolution.test.ts +++ b/tests/integration/issue-1259-watch-call-resolution.test.ts @@ -99,6 +99,10 @@ function makeStmts(db: ReturnType) { "SELECT id, file, kind FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant')", ), listSymbols: db.prepare("SELECT name, kind, line FROM nodes WHERE file = ? AND kind != 'file'"), + upsertFileHash: db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ), + deleteFileHash: db.prepare('DELETE FROM file_hashes WHERE file = ?'), }; } diff --git a/tests/integration/issue-1370-incremental-caller-name.test.ts b/tests/integration/issue-1370-incremental-caller-name.test.ts index 8751d449..ecdca207 100644 --- a/tests/integration/issue-1370-incremental-caller-name.test.ts +++ b/tests/integration/issue-1370-incremental-caller-name.test.ts @@ -95,6 +95,10 @@ function makeStmts(db: ReturnType) { "SELECT id, file, kind FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant')", ), listSymbols: db.prepare("SELECT name, kind, line FROM nodes WHERE file = ? AND kind != 'file'"), + upsertFileHash: db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ), + deleteFileHash: db.prepare('DELETE FROM file_hashes WHERE file = ?'), }; } diff --git a/tests/integration/issue-1731-hash-edge-coupling.test.ts b/tests/integration/issue-1731-hash-edge-coupling.test.ts new file mode 100644 index 00000000..0fee9b45 --- /dev/null +++ b/tests/integration/issue-1731-hash-edge-coupling.test.ts @@ -0,0 +1,251 @@ +/** + * Regression test for #1731: `file_hashes` must never claim a file is + * "up to date" while its edges still reflect an older revision. + * + * Root cause: `insertNodes` committed `file_hashes` for changed files in the + * same transaction as node insertion — BEFORE `resolveImports`/`buildEdges` + * had rebuilt those files' edges (a separate, later stage/transaction). Any + * exception thrown while rebuilding edges left the DB with a hash that + * matched the file's CURRENT on-disk content while its edges still reflected + * the PREVIOUS content (or were missing entirely, since the old edges had + * already been purged). Because change-detection trusts `file_hashes` + * exclusively, that divergence was never self-healed by later builds — the + * file would be silently skipped forever, permanently showing stale/missing + * edges via `codegraph deps` / `where --file`. + * + * The fix defers the `file_hashes` commit so it only happens once + * `resolveImports`/`buildEdges` have finished rebuilding a file's edges + * (`commitFileHashes` in `insert-nodes.ts`, called from the pipeline after + * `buildEdges`). Separately, watch-mode's `rebuildFile` never wrote + * `file_hashes` at all — also fixed here, coupling the write to a + * successful edge rebuild. + * + * This file has two suites: + * 1. Fault-injects an exception inside `buildEdges` during an incremental + * build (the adversarial case that would have caught the original bug) + * and asserts the hash does not advance until edges genuinely match. + * 2. Exercises `rebuildFile` (the watch-mode path) directly and asserts it + * now keeps `file_hashes` in sync with the edges it rebuilds. + */ +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 { getNodeId as getNodeIdQuery, initSchema, openDb } from '../../src/db/index.js'; +import { fileHash } from '../../src/domain/graph/builder/helpers.js'; +import { rebuildFile } from '../../src/domain/graph/builder/incremental.js'; +import { buildGraph } from '../../src/domain/graph/builder.js'; + +// ── Fault injection for suite 1 ────────────────────────────────────────── +// A `vi.hoisted` object is used (rather than a plain module-scope `let`) so +// the mutable flag is visible both inside the hoisted `vi.mock` factory and +// in the test body that arms/disarms it. +const injection = vi.hoisted(() => ({ armed: false })); + +vi.mock('../../src/domain/graph/builder/stages/build-edges.js', async (importOriginal) => { + const mod = + await importOriginal(); + return { + ...mod, + buildEdges: async (ctx: Parameters[0]) => { + if (injection.armed) { + injection.armed = false; + throw new Error('simulated buildEdges failure (#1731 regression test)'); + } + return mod.buildEdges(ctx); + }, + }; +}); + +// ── Shared fixture helpers ──────────────────────────────────────────────── + +/** Writes a.js importing from whichever of b.js/c.js are named in `imports`. */ +function writeProject(dir: string, imports: string[]) { + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'b.js'), 'export function b() { return 1; }\n'); + fs.writeFileSync(path.join(dir, 'c.js'), 'export function c() { return 2; }\n'); + const importLines = imports.map((m) => `import { ${m} } from './${m}.js';`).join('\n'); + const body = imports.map((m) => `${m}()`).join(' + ') || '0'; + fs.writeFileSync( + path.join(dir, 'a.js'), + `${importLines}\nexport function run() { return ${body}; }\n`, + ); +} + +function readFileHashRow(dbPath: string, file: string): { hash: string } | undefined { + const db = new Database(dbPath, { readonly: true }); + try { + return db.prepare('SELECT hash FROM file_hashes WHERE file = ?').get(file) as + | { hash: string } + | undefined; + } finally { + db.close(); + } +} + +function readImportEdgeTargets(dbPath: string, file: string): string[] { + const db = new Database(dbPath, { readonly: true }); + try { + return ( + db + .prepare( + `SELECT n2.file AS tgt FROM edges e + JOIN nodes n1 ON e.source_id = n1.id + JOIN nodes n2 ON e.target_id = n2.id + WHERE n1.file = ? AND n1.kind = 'file' AND e.kind = 'imports' + ORDER BY n2.file`, + ) + .all(file) as Array<{ tgt: string }> + ).map((r) => r.tgt); + } finally { + db.close(); + } +} + +// ── Suite 1: incremental build pipeline ────────────────────────────────── + +describe('Issue #1731: file_hashes/edges coupling survives a mid-build failure', () => { + let tmpDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1731-')); + injection.armed = false; + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('does not commit file_hashes when buildEdges throws mid-incremental-build, and self-heals on retry', async () => { + writeProject(tmpDir, ['b', 'c']); + // engine: 'wasm' pins this to the JS pipeline (insertNodes/resolveImports/ + // buildEdges as separate stages) — the code path this fix changes. The + // native orchestrator runs the equivalent pipeline entirely in Rust and + // isn't reachable via a mocked JS module. + await buildGraph(tmpDir, { incremental: false, skipRegistry: true, engine: 'wasm' }); + + const dbPath = path.join(tmpDir, '.codegraph', 'graph.db'); + const originalHash = fileHash(fs.readFileSync(path.join(tmpDir, 'a.js'), 'utf-8')); + expect(readFileHashRow(dbPath, 'a.js')?.hash).toBe(originalHash); + expect(readImportEdgeTargets(dbPath, 'a.js')).toEqual(['b.js', 'c.js']); + + // Edit a.js to drop its import of c.js. + writeProject(tmpDir, ['b']); + const editedHash = fileHash(fs.readFileSync(path.join(tmpDir, 'a.js'), 'utf-8')); + expect(editedHash).not.toBe(originalHash); + + // Arm the fault injection and run the incremental build — it must reject + // partway through, after node insertion but before edges are rebuilt. + injection.armed = true; + await expect( + buildGraph(tmpDir, { incremental: true, skipRegistry: true, engine: 'wasm' }), + ).rejects.toThrow(/simulated buildEdges failure/); + + // The hash must NOT have advanced to match the edited content. If it + // had (the pre-fix behavior), the file would be permanently stuck: the + // next build would see the hash as "up to date" and skip reprocessing + // it forever, leaving its edges stale or missing. + expect(readFileHashRow(dbPath, 'a.js')?.hash).toBe(originalHash); + + // Retry without fault injection — the resulting hash/content mismatch + // must be detected so the file gets fully reprocessed. + injection.armed = false; + await buildGraph(tmpDir, { incremental: true, skipRegistry: true, engine: 'wasm' }); + + expect(readFileHashRow(dbPath, 'a.js')?.hash).toBe(editedHash); + expect(readImportEdgeTargets(dbPath, 'a.js')).toEqual(['b.js']); + }); +}); + +// ── Suite 2: watch-mode rebuildFile ─────────────────────────────────────── + +function makeStmts(db: ReturnType) { + return { + insertNode: db.prepare( + 'INSERT OR IGNORE INTO nodes (name, kind, file, line, end_line) VALUES (?, ?, ?, ?, ?)', + ), + getNodeId: { + get: (name: string, kind: string, file: string, line: number) => { + const id = getNodeIdQuery(db, name, kind, file, line); + return id != null ? { id } : undefined; + }, + }, + insertEdge: db.prepare( + 'INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) VALUES (?, ?, ?, ?, ?)', + ), + countNodes: db.prepare('SELECT COUNT(*) as c FROM nodes WHERE file = ?'), + countEdges: db.prepare( + 'SELECT COUNT(*) as c FROM edges WHERE source_id IN (SELECT id FROM nodes WHERE file = ?)', + ), + findNodeInFile: db.prepare( + "SELECT id, kind, file FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant') AND file = ?", + ), + findNodeByName: db.prepare( + "SELECT id, file, kind FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant')", + ), + listSymbols: db.prepare("SELECT name, kind, line FROM nodes WHERE file = ? AND kind != 'file'"), + upsertFileHash: db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ), + deleteFileHash: db.prepare('DELETE FROM file_hashes WHERE file = ?'), + }; +} + +describe('Issue #1731: watch-mode rebuildFile keeps file_hashes in sync with edges', () => { + let tmpDir: string; + let dbPath: string; + + beforeEach(async () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1731-watch-')); + writeProject(tmpDir, ['b', 'c']); + await buildGraph(tmpDir, { incremental: false, skipRegistry: true, engine: 'wasm' }); + dbPath = path.join(tmpDir, '.codegraph', 'graph.db'); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('updates file_hashes to match the edited content after a rebuild, not just the edges', async () => { + const originalHash = readFileHashRow(dbPath, 'a.js')?.hash; + expect(originalHash).toBeTruthy(); + + // Edit a.js to drop its import of c.js, then rebuild via the watcher's + // single-file path. + writeProject(tmpDir, ['b']); + const editedHash = fileHash(fs.readFileSync(path.join(tmpDir, 'a.js'), 'utf-8')); + expect(editedHash).not.toBe(originalHash); + + const db = openDb(dbPath); + try { + initSchema(db); + const stmts = makeStmts(db); + await rebuildFile(db, tmpDir, path.join(tmpDir, 'a.js'), stmts, { engine: 'wasm' }, null); + } finally { + db.close(); + } + + // Before the fix, rebuildFile rebuilt edges correctly but never touched + // file_hashes at all, leaving it permanently stuck at originalHash. + expect(readFileHashRow(dbPath, 'a.js')?.hash).toBe(editedHash); + expect(readImportEdgeTargets(dbPath, 'a.js')).toEqual(['b.js']); + }); + + it('removes the file_hashes row when the file is deleted', async () => { + expect(readFileHashRow(dbPath, 'a.js')).toBeTruthy(); + + fs.rmSync(path.join(tmpDir, 'a.js')); + + const db = openDb(dbPath); + try { + initSchema(db); + const stmts = makeStmts(db); + await rebuildFile(db, tmpDir, path.join(tmpDir, 'a.js'), stmts, { engine: 'wasm' }, null); + } finally { + db.close(); + } + + expect(readFileHashRow(dbPath, 'a.js')).toBeUndefined(); + }); +}); diff --git a/tests/integration/watcher-fk-embeddings.test.ts b/tests/integration/watcher-fk-embeddings.test.ts index 6d75cf55..2bac2c16 100644 --- a/tests/integration/watcher-fk-embeddings.test.ts +++ b/tests/integration/watcher-fk-embeddings.test.ts @@ -54,6 +54,10 @@ function makeStmts(db: Database.Database): Parameters[3] { "SELECT id, file FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant')", ), listSymbols: db.prepare("SELECT name, kind, line FROM nodes WHERE file = ? AND kind != 'file'"), + upsertFileHash: db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ), + deleteFileHash: db.prepare('DELETE FROM file_hashes WHERE file = ?'), } as Parameters[3]; } diff --git a/tests/integration/watcher-rebuild.test.ts b/tests/integration/watcher-rebuild.test.ts index 95c286a7..5b92d486 100644 --- a/tests/integration/watcher-rebuild.test.ts +++ b/tests/integration/watcher-rebuild.test.ts @@ -77,6 +77,10 @@ function makeStmts(db) { "SELECT id, kind, file FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant')", ), listSymbols: db.prepare("SELECT name, kind, line FROM nodes WHERE file = ? AND kind != 'file'"), + upsertFileHash: db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ), + deleteFileHash: db.prepare('DELETE FROM file_hashes WHERE file = ?'), }; }