Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/domain/graph/builder/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,12 +222,18 @@ export function fileHash(content: string): string {
}

/**
* Stat a file, returning { mtimeMs, size } or null on error.
* 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.
*/
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 };
const s = fs.statSync(filePath, { bigint: true });
return { mtime: Number(s.mtimeNs / 1_000_000n), size: Number(s.size) };
} catch {
return null;
}
Expand Down
2 changes: 1 addition & 1 deletion src/domain/graph/builder/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -938,7 +938,7 @@ async function backfillNativeDroppedFiles(ctx: PipelineContext): Promise<void> {
}
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);
}
Expand Down
12 changes: 6 additions & 6 deletions src/domain/graph/builder/stages/detect-changes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ interface FileHashRow {
}

interface FileStat {
mtimeMs: number;
mtime: number;
size: number;
}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -663,7 +663,7 @@ export async function detectChanges(ctx: PipelineContext): Promise<void> {
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
Expand All @@ -674,7 +674,7 @@ export async function detectChanges(ctx: PipelineContext): Promise<void> {
.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);
Expand Down
8 changes: 4 additions & 4 deletions src/domain/graph/builder/stages/insert-nodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand All @@ -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 });
}
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
Expand Down
38 changes: 28 additions & 10 deletions tests/builder/detect-changes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import path from 'node:path';
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,
Expand Down Expand Up @@ -62,12 +63,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'));
const stat = fs.statSync(path.join(dir, 'a.js'), { bigint: true });
db.prepare('INSERT INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)').run(
'a.js',
hash,
Math.floor(stat.mtimeMs),
stat.size,
Number(stat.mtimeNs / 1_000_000n),
Number(stat.size),
);

// Write journal header so journal check doesn't confuse things
Expand Down Expand Up @@ -158,15 +159,16 @@ describe('detectNoChanges fast-skip', () => {
relPath: string,
filePath: string,
): { mtime: number; size: number } {
const stat = fs.statSync(filePath);
const mtime = Math.floor(stat.mtimeMs);
const stat = fs.statSync(filePath, { bigint: true });
const mtime = Number(stat.mtimeNs / 1_000_000n);
const size = Number(stat.size);
db.prepare('INSERT INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)').run(
relPath,
'deadbeef',
mtime,
stat.size,
size,
);
return { mtime, size: stat.size };
return { mtime, size };
}

it('returns false when file_hashes is empty (first build)', () => {
Expand Down Expand Up @@ -221,12 +223,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);
const stat = fs.statSync(file, { bigint: true });
db.prepare('INSERT INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)').run(
'a.js',
'deadbeef',
Math.floor(stat.mtimeMs) + 1000, // skewed mtime
stat.size,
Number(stat.mtimeNs / 1_000_000n) + 1000, // skewed mtime
Number(stat.size),
);

expect(detectNoChanges(db, [file], dir)).toBe(false);
Expand Down Expand Up @@ -276,4 +278,20 @@ 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`. Reverting to `Math.floor(stat.mtimeMs)`
// re-introduces #1075: at large epoch values the f64 `mtimeMs` rounds, so a
// Rust-written `file_hashes.mtime` reads back 1ms ahead in JS and busts the
// fast-skip path on every native→JS handoff.
it('fileStat mtime matches Rust Duration::as_millis() truncation (#1075)', () => {
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 });
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Regression test only catches the bug probabilistically

The test comment says "Reverting to Math.floor(stat.mtimeMs) re-introduces the bug visibly," but that's only true when the freshly-created file's mtimeNs happens to land near a millisecond boundary where f64 rounding flips. At the current epoch (~1.748 × 10¹⁸ ns), the f64 ULP is ~256 ns, so only values with a sub-ms remainder in the ~[999744 ns, 1000000 ns) window trigger the discrepancy — roughly a 0.026% chance per run. In the common case the test passes even with Math.floor.

A deterministic version would use fs.utimesSync to stamp a known-bad mtime (one where Math.floor(Number(badNs) / 1e6) !== Number(badNs / 1_000_000n)) and assert the two values diverge, then confirm fileStat returns the BigInt-truncated value. As written the test is still useful as a sanity check, but the regression-guard claim in the comment overstates it.

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1a44aa4 — replaced the probabilistic test with a deterministic vi.spyOn(fs, 'statSync') stub that injects a hand-picked mtimeNs (1748400000000999808n) where the BigInt path lands on N and the f64 path lands on N+1. Verified locally by reverting fileStat() to Math.floor(stat.mtimeMs) — the test now fails deterministically (1748400000000 vs 1748400000001) instead of mostly passing. Kept the original fresh-file-on-disk variant alongside as a sanity check that the helper still works against real filesystem stats.

});
Loading