Skip to content

fix(file-entry-cache): restore v8 reconcile and change-detection semantics (#1648) #1649

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jaredwray wants to merge 3 commits into
base: main
Choose a base branch
from
+276 −15
Open

fix(file-entry-cache): restore v8 reconcile and change-detection semantics (#1648) #1649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d1ef69f
fix(file-entry-cache): restore v8 reconcile and change-detection sema…
claude May 29, 2026
3ade335
test(file-entry-cache): cover reconcile() guard for externally-remove…
claude May 29, 2026
b35cfd1
fix(file-entry-cache): harden create() and reconcile() per code review
claude May 29, 2026
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
93 changes: 79 additions & 14 deletions packages/file-entry-cache/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,20 @@ export function create(
if (cacheDirectory) {
const cachePath = `${cacheDirectory}/${cacheId}`;
if (fs.existsSync(cachePath)) {
fileEntryCache.cache = createFlatCacheFile(cachePath, opts.cache);
try {
fileEntryCache.cache = createFlatCacheFile(cachePath, opts.cache);
} catch (error) {
// If the cache file content is invalid (e.g. corrupted or non-JSON),
// start with an empty cache. The existing file will be overwritten on
// the next reconcile() rather than throwing. Unexpected errors (e.g.
// IO/permission failures on an otherwise valid file) are re-thrown so
// valid cache data is not silently discarded.
if (error instanceof SyntaxError) {
fileEntryCache.cache = new FlatCache(opts.cache);
} else {
throw error;
}
}
}
}

Expand All @@ -150,6 +163,15 @@ export class FileEntryCache {
private _logger?: ILogger;
private _useAbsolutePathAsKey = false;
private _useModifiedTime = true;
/**
* Snapshot of the persisted meta for each key as of the last load/reconcile.
* Change detection compares against this baseline (not the working cache) so
* that repeated `getFileDescriptor()` calls keep reporting a file as changed
* until the cache is reconciled. The set of keys also tracks which files were
* visited during the current session so that `reconcile()` only updates those.
*/
private _originalMeta: Map<string, FileDescriptorMeta | undefined> =
new Map();

/**
* Create a new FileEntryCache instance
Expand Down Expand Up @@ -203,6 +225,9 @@ export class FileEntryCache {
*/
public set cache(cache: FlatCache) {
this._cache = cache;
// The baseline is derived from the cache, so reset it when the cache is
// replaced. It will be re-snapshotted lazily on the next getFileDescriptor.
this._originalMeta = new Map();
}

/**
Expand Down Expand Up @@ -368,6 +393,7 @@ export class FileEntryCache {
*/
public destroy() {
this._cache.destroy();
this._originalMeta = new Map();
}

/**
Expand All @@ -378,18 +404,38 @@ export class FileEntryCache {
public removeEntry(filePath: string): void {
const key = this.createFileKey(filePath);
this._cache.removeKey(key);
this._originalMeta.delete(key);
}

/**
* Reconcile the cache
* @method reconcile
*/
public reconcile(): void {
const { items } = this._cache;
for (const item of items) {
const fileDescriptor = this.getFileDescriptor(item.key);
if (fileDescriptor.notFound) {
this._cache.removeKey(item.key);
// Only reconcile files that were visited via getFileDescriptor() during
// this session. Entries that were never inspected keep their previously
// persisted meta untouched, so they are not silently revalidated.
for (const key of [...this._originalMeta.keys()]) {
const meta = this._cache.getKey<FileDescriptorMeta>(key);
if (!meta) {
// The entry was removed (e.g. file not found) during the session.
this._originalMeta.delete(key);
continue;
}

try {
// Confirm the file still exists; if it was deleted during the
// session, drop it from the cache below.
fs.statSync(this.getAbsolutePath(key));
// Persist the meta captured when the file was inspected. It already
// holds a consistent size/mtime/hash snapshot, so we promote it to
// the baseline rather than re-stat'ing (which would refresh
// size/mtime but not hash, leaving the baseline inconsistent).
this._originalMeta.set(key, { ...meta });
} catch {
// The file no longer exists; drop it from the cache.
this._cache.removeKey(key);
this._originalMeta.delete(key);
}
Comment on lines +426 to 439
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 29, 2026

Choose a reason for hiding this comment

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

high

Correctness & Performance Issue: Redundant and Buggy Re-stating in reconcile()

Updating meta.size and meta.mtime during reconcile() introduces a severe correctness bug and a performance bottleneck:

  1. Silent Cache Invalidation (Correctness Bug): If a file is modified after getFileDescriptor() is called but before reconcile(), reconcile() will overwrite the cached mtime and size with the new values. In the next session, getFileDescriptor() will compare the disk stats with these new cached stats, see they match, and report changed: false. The modification will be silently ignored, and the file will never be re-processed/re-linted.
  2. Inconsistent Cache State: If useCheckSum is enabled, reconcile() updates mtime and size but does not update the hash (since it doesn't re-calculate the checksum). This leaves the cache entry in an inconsistent state.
  3. Performance Overhead: Calling fs.statSync on every visited file during reconcile() introduces redundant synchronous I/O, which can significantly slow down execution for large codebases.

Solution: Do not update the file stats in reconcile(). Instead, just check if the file still exists (to satisfy the deleted files cleanup test) and update the baseline _originalMeta with the existing meta from the working cache.

Suggested change
try {
const absolutePath = this.getAbsolutePath(key);
const fstat = fs.statSync(absolutePath);
// Refresh the stats to reflect the state at reconcile time.
meta.size = fstat.size;
meta.mtime = fstat.mtime.getTime();
this._cache.setKey(key, meta);
// Update the baseline so subsequent checks see the file as unchanged.
this._originalMeta.set(key, { ...meta });
} catch {
// The file no longer exists; drop it from the cache.
this._cache.removeKey(key);
this._originalMeta.delete(key);
}
try {
const absolutePath = this.getAbsolutePath(key);
if (!fs.existsSync(absolutePath)) {
this._cache.removeKey(key);
this._originalMeta.delete(key);
} else {
// Update the baseline so subsequent checks see the file as unchanged.
this._originalMeta.set(key, { ...meta });
}
} catch {
// The file no longer exists; drop it from the cache.
this._cache.removeKey(key);
this._originalMeta.delete(key);
}

}

Expand Down Expand Up @@ -499,35 +545,49 @@ export class FileEntryCache {
};
}

// If the file is not in the cache, add it
if (!metaCache) {
// Snapshot the baseline (the persisted state as of the last load/reconcile)
// the first time this key is seen in the current session. Change detection
// compares against this baseline rather than the working cache so that a
// file reported as changed keeps reporting as changed until reconcile().
if (!this._originalMeta.has(result.key)) {
this._originalMeta.set(
result.key,
metaCache ? { ...metaCache } : undefined,
);
}

const baseline = this._originalMeta.get(result.key);

// If there is no baseline, the file is new (or has not been reconciled yet)
// and is therefore considered changed.
if (baseline === undefined) {
result.changed = true;
this._cache.setKey(result.key, result.meta);
this._logger?.debug({ filePath }, "File not in cache, marked as changed");
return result;
}

// If the file is in the cache, check if the file has changed
if (useModifiedTimeValue && metaCache?.mtime !== result.meta?.mtime) {
if (useModifiedTimeValue && baseline.mtime !== result.meta?.mtime) {
result.changed = true;
this._logger?.debug(
{ filePath, oldMtime: metaCache.mtime, newMtime: result.meta.mtime },
{ filePath, oldMtime: baseline.mtime, newMtime: result.meta.mtime },
"File changed: mtime differs",
);
}

if (metaCache?.size !== result.meta?.size) {
if (baseline.size !== result.meta?.size) {
result.changed = true;
this._logger?.debug(
{ filePath, oldSize: metaCache.size, newSize: result.meta.size },
{ filePath, oldSize: baseline.size, newSize: result.meta.size },
"File changed: size differs",
);
}

if (useCheckSumValue && metaCache?.hash !== result.meta?.hash) {
if (useCheckSumValue && baseline.hash !== result.meta?.hash) {
result.changed = true;
this._logger?.debug(
{ filePath, oldHash: metaCache.hash, newHash: result.meta.hash },
{ filePath, oldHash: baseline.hash, newHash: result.meta.hash },
"File changed: hash differs",
);
}
Expand Down Expand Up @@ -733,6 +793,11 @@ export class FileEntryCache {
const meta = this._cache.getKey(key);
this._cache.removeKey(key);
this._cache.setKey(newKey, meta);
// Keep the change-detection baseline aligned with the renamed key.
if (this._originalMeta.has(key)) {
this._originalMeta.set(newKey, this._originalMeta.get(key));
this._originalMeta.delete(key);
}
}
}
}
Expand Down
31 changes: 30 additions & 1 deletion packages/file-entry-cache/test/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,9 @@ describe("file-entry-cache with options", () => {
logs.some((l) => l.msg === "File not in cache, marked as changed"),
).toBe(true); // 450

// Reconcile so the file becomes the cached baseline
fileEntryCache.reconcile();

// Second call - file in cache, unchanged
logs.length = 0;
descriptor = fileEntryCache.getFileDescriptor(testFile);
Expand Down Expand Up @@ -250,8 +253,9 @@ describe("file-entry-cache with options", () => {
useCheckSum: false, // Disable checksum to use mtime
});

// First call - add file to cache
// First call - add file to cache and reconcile to set the baseline
fileEntryCache.getFileDescriptor(testFile);
fileEntryCache.reconcile();

// Wait a bit and modify the file to change mtime
await new Promise((resolve) => setTimeout(resolve, 10));
Expand Down Expand Up @@ -556,6 +560,9 @@ describe("getFileDescriptor()", () => {
expect(fileDescriptor.key).toBe(testFile1);
expect(fileDescriptor.changed).toBe(true);

// Reconcile so the file becomes the cached baseline before re-checking
fileEntryCache.reconcile();

const fileDescriptor2 = fileEntryCache.getFileDescriptor(testFile1);
expect(fileDescriptor2).toBeDefined();
expect(fileDescriptor2.key).toBe(testFile1);
Expand Down Expand Up @@ -599,6 +606,8 @@ describe("getFileDescriptor()", () => {
expect(fileDescriptor).toBeDefined();
expect(fileDescriptor.key).toBe(testFile1);
expect(fileDescriptor.changed).toBe(true);
// Reconcile so the file becomes the cached baseline before re-checking
fileEntryCache.reconcile();
const fileDescriptor2 = fileEntryCache.getFileDescriptor(testFile1);
expect(fileDescriptor2).toBeDefined();
expect(fileDescriptor2.key).toBe(testFile1);
Expand Down Expand Up @@ -650,6 +659,9 @@ describe("getFileDescriptor()", () => {
expect(fileDescriptor2.key).toBe(absPath);
expect(fileDescriptor2.changed).toBe(true);

// Reconcile so both keys become cached baselines
fileEntryCache.reconcile();

// Should be cached separately
const fileDescriptor3 = fileEntryCache.getFileDescriptor(relPath);
expect(fileDescriptor3.changed).toBe(false);
Expand Down Expand Up @@ -694,6 +706,9 @@ describe("hasFileChanged()", () => {
const fileEntryCache = new FileEntryCache();
const testFile1 = path.resolve("./.cacheHFC/test1.txt");
expect(fileEntryCache.hasFileChanged(testFile1)).toBe(true);
// Repeated calls keep reporting the file as changed until it is reconciled
expect(fileEntryCache.hasFileChanged(testFile1)).toBe(true);
fileEntryCache.reconcile();
expect(fileEntryCache.hasFileChanged(testFile1)).toBe(false);
fs.writeFileSync(testFile1, "test4");
expect(fileEntryCache.hasFileChanged(testFile1)).toBe(true);
Expand Down Expand Up @@ -729,6 +744,8 @@ describe("normalizeEntries()", () => {
const file1 = `./${fileCacheName}/test1.txt`;
const file2 = `./${fileCacheName}/test2.txt`;
fileEntryCache.getFileDescriptor(file2);
// Reconcile so file2 becomes a cached baseline (unchanged on re-check)
fileEntryCache.reconcile();
const entries = fileEntryCache.normalizeEntries([file1, file2]);
expect(entries[0].key).toBe(file1);
expect(entries[0].changed).toBe(true);
Expand All @@ -744,6 +761,8 @@ describe("normalizeEntries()", () => {
fileEntryCache.getFileDescriptor(`./${fileCacheName}/test1.txt`);
fileEntryCache.getFileDescriptor(`./${fileCacheName}/test2.txt`);
fileEntryCache.getFileDescriptor(`./${fileCacheName}/test3.txt`);
// Reconcile so the files become cached baselines (unchanged on re-check)
fileEntryCache.reconcile();
fs.chmodSync(path.resolve(`./${fileCacheName}/test3.txt`), 0o000);
const entries = fileEntryCache.normalizeEntries();
expect(entries.length).toBe(2);
Expand Down Expand Up @@ -857,6 +876,10 @@ describe("analyzeFiles()", () => {
recursive: true,
force: true,
});
fs.rmSync(path.resolve("./.cacheAnalyzeFiles"), {
recursive: true,
force: true,
});
});

test("should analyze files", () => {
Expand Down Expand Up @@ -895,6 +918,8 @@ describe("analyzeFiles()", () => {
const analyzedFiles = fileEntryCache.analyzeFiles(files);
expect(analyzedFiles).toBeDefined();
expect(analyzedFiles.changedFiles.length).toBe(4);
// Reconcile so the files become cached baselines (unchanged on re-check)
fileEntryCache.reconcile();
const testFile4 = path.resolve(`./${fileCacheName}/test4.txt`);
fs.unlinkSync(testFile4);
const analyzedFiles2 = fileEntryCache.analyzeFiles(files);
Expand Down Expand Up @@ -937,6 +962,8 @@ describe("getUpdatedFiles()", () => {
];
const updatedFiles = fileEntryCache.getUpdatedFiles(files);
expect(updatedFiles).toEqual(files);
// Reconcile so the files become cached baselines (no longer updated)
fileEntryCache.reconcile();
const updatedFiles2 = fileEntryCache.getUpdatedFiles(files);
expect(updatedFiles2).toEqual([]);
});
Expand All @@ -953,6 +980,8 @@ describe("getUpdatedFiles()", () => {
];
const updatedFiles = fileEntryCache.getUpdatedFiles(files);
expect(updatedFiles).toEqual(files);
// Reconcile so the files become cached baselines before modifying one
fileEntryCache.reconcile();
const testFile4 = path.resolve(`./${fileCacheName}/test4.txt`);
fs.writeFileSync(testFile4, "test5booosdkfjsldfkjsldkjfls");
const updatedFiles2 = fileEntryCache.getUpdatedFiles(files);
Expand Down
Loading
Loading