fix(files): refresh open file content after external changes (#967)
* fix(files): refresh open file content after external edits Previously, opening a file in the Files view and then editing it externally (e.g. via CLI or another editor) would show stale content. Even closing and reopening the file returned cached content — a full page reload was required. Root causes: 1. The in-memory readFile cache used path-only hits, with no metadata validation. External edits were invisible until the cache was evicted. 2. No polling mechanism existed to detect external changes to the open file. Fix: - Add mtimeMs to statFile across all runtimes (web, VS Code, desktop). - Cache layer (RuntimeAPIProvider): validate cache hits against current stat metadata (mtimeMs + size). On miss, use stat→read→stat to avoid TOCTOU. - UI layer (FilesView): poll the open file every 2s; on detected change, set loadedFilePath=null to trigger the existing load effect once (no double reload). Skip polling when tab is hidden or editor has unsaved changes. - After save, refresh the stat ref so the next poll doesn't see a spurious change from the save itself. Addresses review feedback from PR #827 (double reload + TOCTOU). * fix(files): address P2 review findings - readFreshFile retry now uses stat→read→stat to maintain TOCTOU protection during the retry path (not just the initial read). - Replace isDirty in polling effect deps with isDirtyRef to avoid unnecessary interval teardown/restart on every edit/save cycle.
This commit is contained in:
@@ -11,28 +11,90 @@ import {
|
||||
|
||||
/** Wrap a FilesAPI with an in-memory LRU content cache. */
|
||||
function withContentCache(files: FilesAPI): FilesAPI {
|
||||
const cache = new Map<string, { content: string; path: string }>();
|
||||
const cache = new Map<string, { content: string; path: string; size?: number; mtimeMs?: number }>();
|
||||
|
||||
/** Whether cached metadata still matches the file on disk. */
|
||||
const statMatches = (
|
||||
cached: { size?: number; mtimeMs?: number },
|
||||
latest: { isFile: boolean; size: number; mtimeMs?: number },
|
||||
): boolean => {
|
||||
if (!latest.isFile) return false;
|
||||
// If mtimeMs is available on both sides, it is the strongest signal.
|
||||
if (cached.mtimeMs !== undefined && latest.mtimeMs !== undefined) {
|
||||
return cached.mtimeMs === latest.mtimeMs && cached.size === latest.size;
|
||||
}
|
||||
return cached.size === latest.size;
|
||||
};
|
||||
|
||||
const syncCacheEntry = (
|
||||
path: string,
|
||||
result: { content: string; path: string },
|
||||
stat?: { isFile: boolean; size: number; mtimeMs?: number } | null,
|
||||
): { content: string; path: string } => {
|
||||
const bytes = approxStringBytes(result.content);
|
||||
cache.set(path, {
|
||||
...result,
|
||||
size: stat?.isFile ? stat.size : undefined,
|
||||
mtimeMs: stat?.isFile ? stat.mtimeMs : undefined,
|
||||
});
|
||||
setContentBytes(path, bytes);
|
||||
|
||||
const keep = new Set<string>();
|
||||
evictContentLru(keep, (evictPath) => {
|
||||
cache.delete(evictPath);
|
||||
});
|
||||
|
||||
return result;
|
||||
};
|
||||
|
||||
const readFreshFile = async (path: string): Promise<{ content: string; path: string }> => {
|
||||
// stat → read → stat to avoid TOCTOU:
|
||||
// if the file changes between read and either stat, metadata won't match and we retry.
|
||||
const statBefore = await files.statFile?.(path).catch(() => null);
|
||||
|
||||
const result = await files.readFile!(path);
|
||||
|
||||
const statAfter = await files.statFile?.(path).catch(() => null);
|
||||
|
||||
// If both stats are available and agree, the read was atomic with respect to file changes.
|
||||
if (statBefore && statAfter && statBefore.isFile && statAfter.isFile) {
|
||||
if (statBefore.size === statAfter.size && statBefore.mtimeMs === statAfter.mtimeMs) {
|
||||
return syncCacheEntry(path, result, statAfter);
|
||||
}
|
||||
// File changed during read — discard and re-read once.
|
||||
const retryStatBefore = await files.statFile?.(path).catch(() => null);
|
||||
const retry = await files.readFile!(path);
|
||||
const retryStat = await files.statFile?.(path).catch(() => null);
|
||||
// Accept retry only if file was stable across the read.
|
||||
if (retryStatBefore && retryStat && retryStatBefore.isFile && retryStat.isFile
|
||||
&& retryStatBefore.size === retryStat.size && retryStatBefore.mtimeMs === retryStat.mtimeMs) {
|
||||
return syncCacheEntry(path, retry, retryStat);
|
||||
}
|
||||
// Best-effort: file was still changing, cache what we got. Next hit will re-validate.
|
||||
return syncCacheEntry(path, retry, retryStat);
|
||||
}
|
||||
|
||||
return syncCacheEntry(path, result, statAfter ?? statBefore);
|
||||
};
|
||||
|
||||
const cachedReadFile: FilesAPI['readFile'] = files.readFile
|
||||
? async (path: string) => {
|
||||
const hit = cache.get(path);
|
||||
if (hit) {
|
||||
// Validate cached entry is still fresh
|
||||
if (files.statFile) {
|
||||
const latest = await files.statFile(path).catch(() => null);
|
||||
if (latest && !statMatches(hit, latest)) {
|
||||
cache.delete(path);
|
||||
removeContentBytes(path);
|
||||
return readFreshFile(path);
|
||||
}
|
||||
}
|
||||
touchContentLru(path);
|
||||
return hit;
|
||||
return { content: hit.content, path: hit.path };
|
||||
}
|
||||
|
||||
const result = await files.readFile!(path);
|
||||
const bytes = approxStringBytes(result.content);
|
||||
cache.set(path, result);
|
||||
setContentBytes(path, bytes);
|
||||
|
||||
// Evict if over limits
|
||||
const keep = new Set<string>();
|
||||
evictContentLru(keep, (evictPath) => {
|
||||
cache.delete(evictPath);
|
||||
});
|
||||
|
||||
return result;
|
||||
return readFreshFile(path);
|
||||
}
|
||||
: undefined;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user