From d5a3890de05edb93546290b8e25698de7ca26c3c Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Thu, 7 May 2026 21:14:03 -0600 Subject: [PATCH 1/2] revert: "fix(native): read mtime via BigInt nanoseconds to match Rust truncation (#1079)" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 35758809f8a47866b0a89f3f27d14e5cabd7cbfd. #1079 paid a real perf cost (BigInt-mode statSync on every file in change detection) for what is in practice a no-op: at large epoch values, JS Math.floor(stat.mtimeMs) can read N+1 where Rust wrote N, busting the fast-skip optimization. But fast-skip only gates whether to re-hash the file — when it misses, the hash is recomputed from contents and matches what's stored, so the graph is byte-identical either way. The fix changes optimization hit rate, not correctness. The 0.026% miss-rate window the fix targeted does not justify the constant-cost BigInt overhead applied to 100% of stat calls on the change-detection hot path. Post-merge benchmark gate confirmed the regression: no-op rebuild went from +70% (warning) to +108% (fail). Reopens #1075. The right path is documenting that fast-skip is best-effort (the on-disk hash is the source of truth for graph correctness) rather than chasing exact integer parity with Rust. --- src/domain/graph/builder/helpers.ts | 14 +--- src/domain/graph/builder/pipeline.ts | 2 +- .../graph/builder/stages/detect-changes.ts | 12 +-- .../graph/builder/stages/insert-nodes.ts | 8 +- tests/builder/detect-changes.test.ts | 76 +++---------------- 5 files changed, 26 insertions(+), 86 deletions(-) diff --git a/src/domain/graph/builder/helpers.ts b/src/domain/graph/builder/helpers.ts index 1be562aeb..7ab16b63b 100644 --- a/src/domain/graph/builder/helpers.ts +++ b/src/domain/graph/builder/helpers.ts @@ -222,18 +222,12 @@ export function fileHash(content: string): string { } /** - * Stat a file, returning integer-truncated mtime in ms (and size). - * - * Reads via BigInt nanoseconds and truncates with integer math so the value - * matches Rust's `Duration::as_millis() as i64` exactly. `Math.floor(stat.mtimeMs)` - * cannot be substituted: at large epoch values the f64 `mtimeMs` rounds, so a - * Rust-written `file_hashes.mtime` of N can read back as N+1 in JS and bust the - * fast-skip path on every native→JS handoff. + * Stat a file, returning { mtimeMs, size } or null on error. */ -export function fileStat(filePath: string): { mtime: number; size: number } | null { +export function fileStat(filePath: string): { mtimeMs: number; size: number } | null { try { - const s = fs.statSync(filePath, { bigint: true }); - return { mtime: Number(s.mtimeNs / 1_000_000n), size: Number(s.size) }; + const s = fs.statSync(filePath); + return { mtimeMs: s.mtimeMs, size: s.size }; } catch { return null; } diff --git a/src/domain/graph/builder/pipeline.ts b/src/domain/graph/builder/pipeline.ts index 33df73450..55096e060 100644 --- a/src/domain/graph/builder/pipeline.ts +++ b/src/domain/graph/builder/pipeline.ts @@ -938,7 +938,7 @@ async function backfillNativeDroppedFiles(ctx: PipelineContext): Promise { } if (code === null) continue; const stat = fileStat(absPath); - const mtime = stat ? stat.mtime : 0; + const mtime = stat ? Math.floor(stat.mtimeMs) : 0; const size = stat ? stat.size : 0; upsertHash.run(relPath, fileHash(code), mtime, size); } diff --git a/src/domain/graph/builder/stages/detect-changes.ts b/src/domain/graph/builder/stages/detect-changes.ts index cc51155dc..6abf224dd 100644 --- a/src/domain/graph/builder/stages/detect-changes.ts +++ b/src/domain/graph/builder/stages/detect-changes.ts @@ -27,7 +27,7 @@ interface FileHashRow { } interface FileStat { - mtime: number; + mtimeMs: number; size: number; } @@ -182,7 +182,7 @@ function mtimeAndHashTiers( if (!stat) continue; const storedMtime = record.mtime || 0; const storedSize = record.size || 0; - if (storedSize > 0 && stat.mtime === storedMtime && stat.size === storedSize) { + if (storedSize > 0 && Math.floor(stat.mtimeMs) === storedMtime && stat.size === storedSize) { skipped.push(relPath); continue; } @@ -596,9 +596,9 @@ export function detectNoChanges( log(`false: stored size <= 0 for ${relPath} (stored=${record.size})`); return false; } - if (stat.mtime !== storedMtime || stat.size !== storedSize) { + if (Math.floor(stat.mtimeMs) !== storedMtime || stat.size !== storedSize) { log( - `false: mtime/size diff for ${relPath}: stat=${stat.mtime}/${stat.size} stored=${storedMtime}/${storedSize}`, + `false: mtime/size diff for ${relPath}: stat=${Math.floor(stat.mtimeMs)}/${stat.size} stored=${storedMtime}/${storedSize} (mtimeMs=${stat.mtimeMs})`, ); return false; } @@ -663,7 +663,7 @@ export async function detectChanges(ctx: PipelineContext): Promise { relPath: c.relPath, content: c.content, hash: c.hash, - stat: c.stat ? { mtime: c.stat.mtime, size: c.stat.size } : undefined, + stat: c.stat ? { mtime: Math.floor(c.stat.mtimeMs), size: c.stat.size } : undefined, _reverseDepOnly: c._reverseDepOnly, })); ctx.metadataUpdates = increResult.changed @@ -674,7 +674,7 @@ export async function detectChanges(ctx: PipelineContext): Promise { .map((c) => ({ relPath: c.relPath, hash: c.hash, - stat: { mtime: c.stat.mtime, size: c.stat.size }, + stat: { mtime: Math.floor(c.stat.mtimeMs), size: c.stat.size }, })); if (!ctx.isFullBuild && ctx.parseChanges.length === 0 && ctx.removed.length === 0) { const ranAnalysis = await runPendingAnalysis(ctx); diff --git a/src/domain/graph/builder/stages/insert-nodes.ts b/src/domain/graph/builder/stages/insert-nodes.ts index 064dea953..339b11237 100644 --- a/src/domain/graph/builder/stages/insert-nodes.ts +++ b/src/domain/graph/builder/stages/insert-nodes.ts @@ -128,7 +128,7 @@ export function buildFileHashes( size = precomputed.stat.size; } else { const rawStat = fileStat(path.join(rootDir, relPath)); - mtime = rawStat ? rawStat.mtime : 0; + mtime = rawStat ? Math.floor(rawStat.mtimeMs) : 0; size = rawStat ? rawStat.size : 0; } fileHashes.push({ file: relPath, hash: precomputed.hash, mtime, size }); @@ -143,7 +143,7 @@ export function buildFileHashes( } if (code !== null) { const stat = fileStat(absPath); - const mtime = stat ? stat.mtime : 0; + const mtime = stat ? Math.floor(stat.mtimeMs) : 0; const size = stat ? stat.size : 0; fileHashes.push({ file: relPath, hash: fileHash(code), mtime, size }); } @@ -365,7 +365,7 @@ function updateFileHashes( size = precomputed.stat.size; } else { const rawStat = fileStat(path.join(rootDir, relPath)); - mtime = rawStat ? rawStat.mtime : 0; + mtime = rawStat ? Math.floor(rawStat.mtimeMs) : 0; size = rawStat ? rawStat.size : 0; } upsertHash.run(relPath, precomputed.hash, mtime, size); @@ -380,7 +380,7 @@ function updateFileHashes( } if (code !== null) { const stat = fileStat(absPath); - const mtime = stat ? stat.mtime : 0; + const mtime = stat ? Math.floor(stat.mtimeMs) : 0; const size = stat ? stat.size : 0; upsertHash.run(relPath, fileHash(code), mtime, size); } diff --git a/tests/builder/detect-changes.test.ts b/tests/builder/detect-changes.test.ts index 6d0bfba5f..7d798ef16 100644 --- a/tests/builder/detect-changes.test.ts +++ b/tests/builder/detect-changes.test.ts @@ -4,10 +4,9 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; -import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { closeDb, initSchema, openDb } from '../../src/db/index.js'; import { PipelineContext } from '../../src/domain/graph/builder/context.js'; -import { fileStat } from '../../src/domain/graph/builder/helpers.js'; import { detectChanges, detectNoChanges, @@ -63,12 +62,12 @@ describe('detectChanges stage', () => { const content = fs.readFileSync(path.join(dir, 'a.js'), 'utf-8'); const { createHash } = await import('node:crypto'); const hash = createHash('md5').update(content).digest('hex'); - const stat = fs.statSync(path.join(dir, 'a.js'), { bigint: true }); + const stat = fs.statSync(path.join(dir, 'a.js')); db.prepare('INSERT INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)').run( 'a.js', hash, - Number(stat.mtimeNs / 1_000_000n), - Number(stat.size), + Math.floor(stat.mtimeMs), + stat.size, ); // Write journal header so journal check doesn't confuse things @@ -159,16 +158,15 @@ describe('detectNoChanges fast-skip', () => { relPath: string, filePath: string, ): { mtime: number; size: number } { - const stat = fs.statSync(filePath, { bigint: true }); - const mtime = Number(stat.mtimeNs / 1_000_000n); - const size = Number(stat.size); + const stat = fs.statSync(filePath); + const mtime = Math.floor(stat.mtimeMs); db.prepare('INSERT INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)').run( relPath, 'deadbeef', mtime, - size, + stat.size, ); - return { mtime, size }; + return { mtime, size: stat.size }; } it('returns false when file_hashes is empty (first build)', () => { @@ -223,12 +221,12 @@ describe('detectNoChanges fast-skip', () => { const db = openDb(path.join(dbDir, 'graph.db')); initSchema(db); const file = seedFile(dir, 'a.js', 'export const a = 1;'); - const stat = fs.statSync(file, { bigint: true }); + const stat = fs.statSync(file); db.prepare('INSERT INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)').run( 'a.js', 'deadbeef', - Number(stat.mtimeNs / 1_000_000n) + 1000, // skewed mtime - Number(stat.size), + Math.floor(stat.mtimeMs) + 1000, // skewed mtime + stat.size, ); expect(detectNoChanges(db, [file], dir)).toBe(false); @@ -278,56 +276,4 @@ describe('detectNoChanges fast-skip', () => { closeDb(db); fs.rmSync(dir, { recursive: true, force: true }); }); - - // Pins down the BigInt-nanosecond truncation the helper uses to match Rust's - // `Duration::as_millis() as i64`. We can't trigger the f64 ULP rounding bug - // with a freshly-created file (the failure window is ~256 ns out of every ms, - // ~0.026% of values), so instead we stub `fs.statSync` to return a hand-picked - // BigInt `mtimeNs` whose f64-mtimeMs path diverges from the BigInt path: - // ns = 1748400000000999808n (≈ 2025-05-28 epoch ns) - // BigInt: Number(ns / 1_000_000n) === 1748400000000 - // f64 (broken): Math.floor(Number(ns) / 1e6) === 1748400000001 - // Reverting `fileStat` to `Math.floor(stat.mtimeMs)` would flip the result to - // N+1 and fail the assertion deterministically — re-introducing #1075 (the - // Rust-written `file_hashes.mtime` of N reading back as N+1 in JS, busting - // the fast-skip path on every native→JS handoff). - describe('fileStat #1075 mtime truncation', () => { - afterEach(() => { - vi.restoreAllMocks(); - }); - - it('matches Rust Duration::as_millis() truncation at f64-rounding boundary', () => { - // Hand-picked epoch ns where Number(ns)/1e6 rounds up across a ms boundary. - const badMtimeNs = 1748400000000999808n; - const truncatedMs = 1748400000000; - const roundedMs = 1748400000001; - - // Sanity: confirm the chosen value actually triggers the divergence; if a - // future Node.js release changes f64 rounding, this baseline assertion - // catches it before we trust the spy-based test below. - expect(Number(badMtimeNs / 1_000_000n)).toBe(truncatedMs); - expect(Math.floor(Number(badMtimeNs) / 1e6)).toBe(roundedMs); - - const stubStats = { - mtimeNs: badMtimeNs, - mtimeMs: Number(badMtimeNs) / 1e6, - size: 42n, - } as unknown as fs.BigIntStats; - vi.spyOn(fs, 'statSync').mockReturnValue(stubStats); - - // BigInt path must win: N, not N+1. - expect(fileStat('/fake/path.js')?.mtime).toBe(truncatedMs); - }); - - it('returns the BigInt-truncated mtime for a real file on disk', () => { - const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-fileStat-trunc-')); - const file = seedFile(dir, 'a.js', 'export const a = 1;'); - - const big = fs.statSync(file, { bigint: true }); - const expected = Number(big.mtimeNs / 1_000_000n); - expect(fileStat(file)?.mtime).toBe(expected); - - fs.rmSync(dir, { recursive: true, force: true }); - }); - }); }); From bf3f4b45040f8e79e7679a57715de860e04dacc0 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Thu, 7 May 2026 21:37:31 -0600 Subject: [PATCH 2/2] refactor(builder): floor mtime inside fileStat helper (#1080) Encapsulate `Math.floor(stat.mtimeMs)` inside the `fileStat` helper so every consumer of the integer DB column gets a pre-floored value by default. Eliminates the risk that a future call site reads `stat.mtimeMs` and stores it un-floored, which would silently write a non-integer (or rounded-up integer) into the DB and cause spurious fast-skip misses on the next build. All six existing call sites simplified from `Math.floor(stat.mtimeMs)` to `stat.mtime`. Behaviour unchanged. Addresses Greptile P2 feedback on the revert PR. --- src/domain/graph/builder/helpers.ts | 11 ++++++++--- src/domain/graph/builder/pipeline.ts | 2 +- src/domain/graph/builder/stages/detect-changes.ts | 12 ++++++------ src/domain/graph/builder/stages/insert-nodes.ts | 12 ++++++------ 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/src/domain/graph/builder/helpers.ts b/src/domain/graph/builder/helpers.ts index 7ab16b63b..c6cbd4845 100644 --- a/src/domain/graph/builder/helpers.ts +++ b/src/domain/graph/builder/helpers.ts @@ -222,12 +222,17 @@ export function fileHash(content: string): string { } /** - * Stat a file, returning { mtimeMs, size } or null on error. + * Stat a file, returning { mtime, size } or null on error. + * + * `mtime` is `Math.floor(stat.mtimeMs)` so it matches the integer column + * stored in the DB. Floor-once-here keeps every consumer honest: storing or + * comparing a non-floored `mtimeMs` against the integer DB column would cause + * spurious fast-skip misses on the next build. */ -export function fileStat(filePath: string): { mtimeMs: number; size: number } | null { +export function fileStat(filePath: string): { mtime: number; size: number } | null { try { const s = fs.statSync(filePath); - return { mtimeMs: s.mtimeMs, size: s.size }; + return { mtime: Math.floor(s.mtimeMs), size: s.size }; } catch { return null; } diff --git a/src/domain/graph/builder/pipeline.ts b/src/domain/graph/builder/pipeline.ts index 55096e060..33df73450 100644 --- a/src/domain/graph/builder/pipeline.ts +++ b/src/domain/graph/builder/pipeline.ts @@ -938,7 +938,7 @@ async function backfillNativeDroppedFiles(ctx: PipelineContext): Promise { } if (code === null) continue; const stat = fileStat(absPath); - const mtime = stat ? Math.floor(stat.mtimeMs) : 0; + const mtime = stat ? stat.mtime : 0; const size = stat ? stat.size : 0; upsertHash.run(relPath, fileHash(code), mtime, size); } diff --git a/src/domain/graph/builder/stages/detect-changes.ts b/src/domain/graph/builder/stages/detect-changes.ts index 6abf224dd..cc51155dc 100644 --- a/src/domain/graph/builder/stages/detect-changes.ts +++ b/src/domain/graph/builder/stages/detect-changes.ts @@ -27,7 +27,7 @@ interface FileHashRow { } interface FileStat { - mtimeMs: number; + mtime: number; size: number; } @@ -182,7 +182,7 @@ function mtimeAndHashTiers( if (!stat) continue; const storedMtime = record.mtime || 0; const storedSize = record.size || 0; - if (storedSize > 0 && Math.floor(stat.mtimeMs) === storedMtime && stat.size === storedSize) { + if (storedSize > 0 && stat.mtime === storedMtime && stat.size === storedSize) { skipped.push(relPath); continue; } @@ -596,9 +596,9 @@ export function detectNoChanges( log(`false: stored size <= 0 for ${relPath} (stored=${record.size})`); return false; } - if (Math.floor(stat.mtimeMs) !== storedMtime || stat.size !== storedSize) { + if (stat.mtime !== storedMtime || stat.size !== storedSize) { log( - `false: mtime/size diff for ${relPath}: stat=${Math.floor(stat.mtimeMs)}/${stat.size} stored=${storedMtime}/${storedSize} (mtimeMs=${stat.mtimeMs})`, + `false: mtime/size diff for ${relPath}: stat=${stat.mtime}/${stat.size} stored=${storedMtime}/${storedSize}`, ); return false; } @@ -663,7 +663,7 @@ export async function detectChanges(ctx: PipelineContext): Promise { relPath: c.relPath, content: c.content, hash: c.hash, - stat: c.stat ? { mtime: Math.floor(c.stat.mtimeMs), size: c.stat.size } : undefined, + stat: c.stat ? { mtime: c.stat.mtime, size: c.stat.size } : undefined, _reverseDepOnly: c._reverseDepOnly, })); ctx.metadataUpdates = increResult.changed @@ -674,7 +674,7 @@ export async function detectChanges(ctx: PipelineContext): Promise { .map((c) => ({ relPath: c.relPath, hash: c.hash, - stat: { mtime: Math.floor(c.stat.mtimeMs), size: c.stat.size }, + stat: { mtime: c.stat.mtime, size: c.stat.size }, })); if (!ctx.isFullBuild && ctx.parseChanges.length === 0 && ctx.removed.length === 0) { const ranAnalysis = await runPendingAnalysis(ctx); diff --git a/src/domain/graph/builder/stages/insert-nodes.ts b/src/domain/graph/builder/stages/insert-nodes.ts index 339b11237..88e403ec9 100644 --- a/src/domain/graph/builder/stages/insert-nodes.ts +++ b/src/domain/graph/builder/stages/insert-nodes.ts @@ -128,7 +128,7 @@ export function buildFileHashes( size = precomputed.stat.size; } else { const rawStat = fileStat(path.join(rootDir, relPath)); - mtime = rawStat ? Math.floor(rawStat.mtimeMs) : 0; + mtime = rawStat ? rawStat.mtime : 0; size = rawStat ? rawStat.size : 0; } fileHashes.push({ file: relPath, hash: precomputed.hash, mtime, size }); @@ -143,7 +143,7 @@ export function buildFileHashes( } if (code !== null) { const stat = fileStat(absPath); - const mtime = stat ? Math.floor(stat.mtimeMs) : 0; + const mtime = stat ? stat.mtime : 0; const size = stat ? stat.size : 0; fileHashes.push({ file: relPath, hash: fileHash(code), mtime, size }); } @@ -152,7 +152,7 @@ export function buildFileHashes( // Also include metadata-only updates (self-heal mtime/size without re-parse) for (const item of metadataUpdates) { - const mtime = item.stat ? Math.floor(item.stat.mtime) : 0; + const mtime = item.stat ? item.stat.mtime : 0; const size = item.stat ? item.stat.size : 0; fileHashes.push({ file: item.relPath, hash: item.hash, mtime, size }); } @@ -365,7 +365,7 @@ function updateFileHashes( size = precomputed.stat.size; } else { const rawStat = fileStat(path.join(rootDir, relPath)); - mtime = rawStat ? Math.floor(rawStat.mtimeMs) : 0; + mtime = rawStat ? rawStat.mtime : 0; size = rawStat ? rawStat.size : 0; } upsertHash.run(relPath, precomputed.hash, mtime, size); @@ -380,7 +380,7 @@ function updateFileHashes( } if (code !== null) { const stat = fileStat(absPath); - const mtime = stat ? Math.floor(stat.mtimeMs) : 0; + const mtime = stat ? stat.mtime : 0; const size = stat ? stat.size : 0; upsertHash.run(relPath, fileHash(code), mtime, size); } @@ -389,7 +389,7 @@ function updateFileHashes( // Also update metadata-only entries (self-heal mtime/size without re-parse) for (const item of metadataUpdates) { - const mtime = item.stat ? Math.floor(item.stat.mtime) : 0; + const mtime = item.stat ? item.stat.mtime : 0; const size = item.stat ? item.stat.size : 0; upsertHash.run(item.relPath, item.hash, mtime, size); }