fix(files): stop file viewer reload loop from sub-ms mtime jitter (#1489) (#2297)

* fix(files): guard file polling races - Ignore sub-millisecond mtime jitter on a same-size file so an unchanged open file no longer loops through reload and flickers. - Swap externally changed text content into the open editor in place instead of clearing the loaded path and showing the load spinner. - Read content only after metadata changed, confirm it with a second read, and skip the swap when the file is unchanged, the buffer is dirty, or a newer local write landed. - Keep the stat baseline unchanged when a poll cannot observe content so a failed read is retried rather than treated as unchanged. - Fall back to a full reload for images, PDFs, binaries, and files above the content-poll byte limit, and when a poll returns binary content. - Serialize polls and dispose the poller on unmount, file switch, and directory change. - Add a `fresh` file read option that bypasses the content cache and the HTTP cache. * fix(files): invalidate stale polls after diagram saves --------- Co-authored-by: Bohdan Triapitsyn <artmore@protonmail.com>
2026-09-05 18:31:35 +03:00
parent a005215458
commit 7ea24e3c50
10 changed files with 397 additions and 31 deletions
@@ -26,6 +26,8 @@ import { CodeMirrorEditor } from '@/components/ui/CodeMirrorEditor';
 import { GoToLineDialog } from './GoToLineDialog';
 import { MarkdownPreviewSearch } from './MarkdownPreviewSearch';
 import { PreviewToggleButton } from './PreviewToggleButton';
+import { createFileContentPoller } from './fileContentPoller';
+import { hasFileStatChanged } from './fileStatChange';
 import { JsonTreeView } from '@/components/ui/JsonTreeView';
 import { SimpleMarkdownRenderer } from '@/components/chat/MarkdownRenderer';
 import { languageByExtension, loadLanguageByExtension } from '@/lib/codemirror/languageByExtension';
@@ -314,6 +316,7 @@ const isFileMissingError = (error: unknown): boolean => {
 };

 const MAX_VIEW_CHARS = 200_000;
+const MAX_CONTENT_POLL_BYTES = 200_000;
 type FileLineEnding = '\n' | '\r\n';

 // Fast cache key for pierre's line/highlight caches: content-derived (not a
@@ -931,6 +934,8 @@ export const FilesView: React.FC<FilesViewProps> = ({ mode = 'full' }) => {
  const pendingDrawioPreviewFrameRef = React.useRef<number | null>(null);
  const diagramEditorRef = React.useRef<React.ComponentRef<typeof DiagramEditor>>(null);
  const lastLoadedFileStatRef = React.useRef<FileStatSnapshot | null>(null);
+  const lastLoadedFileContentRef = React.useRef('');
+  const lastLoadedFileRevisionRef = React.useRef(0);
  const activeFileLoadIdRef = React.useRef(0);
  const loadingFilePathRef = React.useRef<string | null>(null);
  const [fileContentRevision, setFileContentRevision] = React.useState(0);
@@ -1573,10 +1578,16 @@ export const FilesView: React.FC<FilesViewProps> = ({ mode = 'full' }) => {
    };
  }, [currentDirectory, debouncedSearchQuery, searchFiles, showHidden, showGitignored]);

-  const readFile = React.useCallback(async (path: string): Promise<string> => {
+  // `fresh` bypasses the content cache and HTTP cache so external-change polling
+  // compares against the file on disk rather than a cached copy.
+  const readFile = React.useCallback(async (path: string, cacheOptions?: { fresh?: boolean }): Promise<string> => {
    const options = await resolveFileReadOptions(path);
    if (files.readFile) {
-      const result = await files.readFile(path, { ...options, directory: root || undefined });
+      const result = await files.readFile(path, {
+        ...options,
+        directory: root || undefined,
+        ...cacheOptions,
+      });
      return result.content ?? '';
    }

@@ -1590,7 +1601,10 @@ export const FilesView: React.FC<FilesViewProps> = ({ mode = 'full' }) => {
    if (root) {
      params.set('directory', root);
    }
-    const response = await runtimeFetch(`/api/fs/read?${params.toString()}`);
+    const response = await runtimeFetch(
+      `/api/fs/read?${params.toString()}`,
+      cacheOptions?.fresh ? { cache: 'no-store' } : undefined,
+    );
    if (!response.ok) {
      const error = await response.json().catch(() => ({ error: response.statusText }));
      throw new Error((error as { error?: string }).error || t('filesView.error.readFileFailed'));
@@ -1690,6 +1704,8 @@ export const FilesView: React.FC<FilesViewProps> = ({ mode = 'full' }) => {
        return false;
      }
      setFileContent(draftContent);
+      lastLoadedFileContentRef.current = contentToWrite;
+      lastLoadedFileRevisionRef.current += 1;
      if (root && isPathWithinRoot(selectedFile.path, root)) {
        const relativePath = getDisplayPath(root, selectedFile.path);
        if (relativePath) {
@@ -1819,6 +1835,19 @@ export const FilesView: React.FC<FilesViewProps> = ({ mode = 'full' }) => {
    },
  });

+  const applyLoadedTextContent = React.useCallback((content: string) => {
+    const editorContent = normalizeEditorLineEndings(content);
+    lastLoadedFileContentRef.current = content;
+    lastLoadedFileRevisionRef.current += 1;
+    setLoadedFileLineEnding(detectFileLineEnding(content));
+    setFileContent(editorContent);
+    diagramXmlRef.current = editorContent;
+    diagramSavedXmlRef.current = editorContent;
+    setDraftContent(editorContent.length > MAX_VIEW_CHARS
+      ? `${editorContent.slice(0, MAX_VIEW_CHARS)}\n\n… truncated …`
+      : editorContent);
+  }, []);
+
  const loadSelectedFile = React.useCallback(async (node: FileNode) => {
    const loadId = activeFileLoadIdRef.current + 1;
    activeFileLoadIdRef.current = loadId;
@@ -1896,14 +1925,7 @@ export const FilesView: React.FC<FilesViewProps> = ({ mode = 'full' }) => {
          setLoadedFilePath(node.path);
          return;
        }
-        const editorContent = normalizeEditorLineEndings(content);
-        setLoadedFileLineEnding(detectFileLineEnding(content));
-        setFileContent(editorContent);
-        diagramXmlRef.current = editorContent;
-        diagramSavedXmlRef.current = editorContent;
-        setDraftContent(editorContent.length > MAX_VIEW_CHARS
-          ? `${editorContent.slice(0, MAX_VIEW_CHARS)}\n\n… truncated …`
-          : editorContent);
+        applyLoadedTextContent(content);
        setLoadedFilePath(node.path);
        void readFileStat(node.path)
          .then((stat) => {
@@ -1970,7 +1992,7 @@ export const FilesView: React.FC<FilesViewProps> = ({ mode = 'full' }) => {
          setFileLoading(false);
        }
      });
-  }, [expandPaths, isMobile, loadDirectory, readFile, readFileStat, removeOpenPathsByPrefix, resolveFileReadOptions, root, runtime.isDesktop, searchQuery, setSelectedPath, t]);
+  }, [applyLoadedTextContent, expandPaths, isMobile, loadDirectory, readFile, readFileStat, removeOpenPathsByPrefix, resolveFileReadOptions, root, runtime.isDesktop, searchQuery, setSelectedPath, t]);

  const ensurePathVisible = React.useCallback(async (targetPath: string, includeTarget: boolean) => {
    if (!root) {
@@ -2082,38 +2104,71 @@ export const FilesView: React.FC<FilesViewProps> = ({ mode = 'full' }) => {
    setFileContentRevision((revision) => revision + 1);
  }), [selectedFile?.path]);

-  // Poll open file for external changes.
-  // When a change is detected, reset loadedFilePath so the effect above
-  // triggers a single reload — no double-load.
+  // Poll open file for external changes. Metadata is compared first so an
+  // unchanged file never reads content, and a changed text file swaps content
+  // in place; only other files fall back to a full reload.
  React.useEffect(() => {
    if (!selectedFile?.path || loadedFilePath !== selectedFile.path) {
      return;
    }

+    const selectedPath = selectedFile.path;
+    // draw.io preview edits live in the XML refs, not the draft buffer, so an
+    // in-place content swap has to treat them as unsaved too.
+    const hasUnsavedChanges = () => isDirtyRef.current || (
+      isDrawioFile(selectedPath) && diagramXmlRef.current !== diagramSavedXmlRef.current
+    );
+    // Same exclusions as `isTextFile`: `isBinaryFile` covers PDFs, `isImageFile` covers SVG.
+    const contentPoller = !isBinaryFile(selectedPath) && !isImageFile(selectedPath) && !contentDetectedBinary
+      ? createFileContentPoller({
+          readContent: () => readFile(selectedPath, { fresh: true }),
+          getLoadedContent: () => lastLoadedFileContentRef.current,
+          getLoadedRevision: () => lastLoadedFileRevisionRef.current,
+          isDirty: hasUnsavedChanges,
+          applyContent: (content) => {
+            // An external write can turn a text file binary; reload so the
+            // binary guards run instead of pasting binary into the editor.
+            if (looksLikeBinaryText(content)) {
+              setLoadedFilePath(null);
+              return;
+            }
+            applyLoadedTextContent(content);
+          },
+          maxBytes: MAX_CONTENT_POLL_BYTES,
+        })
+      : null;
+
    let cancelled = false;
+    let polling = false;
    const interval = window.setInterval(() => {
-      if (document.hidden) {
+      if (document.hidden || polling) {
        return;
      }

-      void readFileStat(selectedFile.path)
-        .then((latestStat) => {
+      polling = true;
+      void readFileStat(selectedPath)
+        .then(async (latestStat) => {
          if (cancelled || !latestStat) {
            return;
          }

          const previousStat = lastLoadedFileStatRef.current;
-          if (!previousStat || previousStat.path !== selectedFile.path) {
+          if (!previousStat || previousStat.path !== selectedPath) {
            lastLoadedFileStatRef.current = latestStat;
            return;
          }

-          const changedByMtime = latestStat.mtimeMs !== undefined
-            && previousStat.mtimeMs !== undefined
-            && latestStat.mtimeMs !== previousStat.mtimeMs;
-          const changedBySize = latestStat.size !== previousStat.size;
+          if (!hasFileStatChanged(previousStat, latestStat)) {
+            return;
+          }

-          if (!changedByMtime && !changedBySize) {
+          if (contentPoller && latestStat.size <= MAX_CONTENT_POLL_BYTES) {
+            // Only an observed read retires the change; a dirty buffer or a
+            // failed read leaves the baseline so the next tick retries.
+            const observed = await contentPoller.poll(latestStat.size);
+            if (observed && !cancelled) {
+              lastLoadedFileStatRef.current = latestStat;
+            }
            return;
          }

@@ -2125,14 +2180,18 @@ export const FilesView: React.FC<FilesViewProps> = ({ mode = 'full' }) => {
          // Reset loadedFilePath so the effect above triggers a single reload.
          setLoadedFilePath(null);
        })
-        .catch(() => {});
+        .catch(() => {})
+        .finally(() => {
+          polling = false;
+        });
    }, 2000);

    return () => {
      cancelled = true;
+      contentPoller?.dispose();
      window.clearInterval(interval);
    };
-  }, [loadedFilePath, readFileStat, selectedFile?.path]);
+  }, [applyLoadedTextContent, contentDetectedBinary, loadedFilePath, readFile, readFileStat, selectedFile?.path]);

  const discardAndContinue = React.useCallback(() => {
    const nextFile = pendingSelectFileRef.current;
@@ -2595,15 +2654,13 @@ export const FilesView: React.FC<FilesViewProps> = ({ mode = 'full' }) => {
      return false;
    }

-    diagramXmlRef.current = xml;
-    diagramSavedXmlRef.current = xml;
-    setDraftContent(xml);
+    applyLoadedTextContent(xml);
    const stat = await readFileStat(path).catch(() => null);
    if (stat) {
      lastLoadedFileStatRef.current = stat;
    }
    return true;
-  }, [files, readFileStat, t]);
+  }, [applyLoadedTextContent, files, readFileStat, t]);

  React.useEffect(() => {
    return () => {
@@ -0,0 +1,200 @@
+import { describe, expect, test } from 'bun:test';
+
+import { createFileContentPoller } from './fileContentPoller';
+
+const MAX_BYTES = 200_000;
+
+const deferred = <T>() => {
+  let resolve!: (value: T) => void;
+  const promise = new Promise<T>((done) => { resolve = done; });
+  return { promise, resolve };
+};
+
+describe('createFileContentPoller', () => {
+  test('does not reload unchanged content across repeated metadata false positives (issue #1489)', async () => {
+    let reads = 0;
+    const applied: string[] = [];
+    const poller = createFileContentPoller({
+      readContent: async () => {
+        reads += 1;
+        return 'same content';
+      },
+      getLoadedContent: () => 'same content',
+      getLoadedRevision: () => 0,
+      isDirty: () => false,
+      applyContent: (content) => applied.push(content),
+      maxBytes: MAX_BYTES,
+    });
+
+    expect(await poller.poll(12)).toBe(true);
+    expect(await poller.poll(12)).toBe(true);
+
+    expect(reads).toBe(2);
+    expect(applied).toEqual([]);
+  });
+
+  test('reports a failed read as unobserved instead of unchanged', async () => {
+    const applied: string[] = [];
+    const poller = createFileContentPoller({
+      readContent: async () => { throw new Error('read failed'); },
+      getLoadedContent: () => 'before',
+      getLoadedRevision: () => 0,
+      isDirty: () => false,
+      applyContent: (content) => applied.push(content),
+      maxBytes: MAX_BYTES,
+    });
+
+    expect(await poller.poll(6)).toBe(false);
+    expect(applied).toEqual([]);
+  });
+
+  test('reports a dirty buffer as unobserved', async () => {
+    let reads = 0;
+    const poller = createFileContentPoller({
+      readContent: async () => {
+        reads += 1;
+        return 'external edit';
+      },
+      getLoadedContent: () => 'before',
+      getLoadedRevision: () => 0,
+      isDirty: () => true,
+      applyContent: () => undefined,
+      maxBytes: MAX_BYTES,
+    });
+
+    expect(await poller.poll(6)).toBe(false);
+    expect(reads).toBe(0);
+  });
+
+  test('reloads a same-size edit even when metadata cannot distinguish it', async () => {
+    let reads = 0;
+    const applied: string[] = [];
+    const poller = createFileContentPoller({
+      readContent: async () => {
+        reads += 1;
+        return 'abd';
+      },
+      getLoadedContent: () => 'abc',
+      getLoadedRevision: () => 0,
+      isDirty: () => false,
+      applyContent: (content) => applied.push(content),
+      maxBytes: MAX_BYTES,
+    });
+
+    expect(await poller.poll(3)).toBe(true);
+
+    expect(reads).toBe(2);
+    expect(applied).toEqual(['abd']);
+  });
+
+  test('does not overwrite a buffer that becomes dirty while polling', async () => {
+    const read = deferred<string>();
+    let dirty = false;
+    const applied: string[] = [];
+    const poller = createFileContentPoller({
+      readContent: () => read.promise,
+      getLoadedContent: () => 'before',
+      getLoadedRevision: () => 0,
+      isDirty: () => dirty,
+      applyContent: (content) => applied.push(content),
+      maxBytes: MAX_BYTES,
+    });
+
+    const polling = poller.poll(20);
+    dirty = true;
+    read.resolve('external edit');
+    await polling;
+
+    expect(applied).toEqual([]);
+  });
+
+  test('does not apply a transient read that changes during polling', async () => {
+    const reads = ['partial', 'settled', 'settled', 'settled'];
+    const applied: string[] = [];
+    const poller = createFileContentPoller({
+      readContent: async () => reads.shift() ?? 'settled',
+      getLoadedContent: () => 'before',
+      getLoadedRevision: () => 0,
+      isDirty: () => false,
+      applyContent: (content) => applied.push(content),
+      maxBytes: MAX_BYTES,
+    });
+
+    await poller.poll(7);
+    expect(applied).toEqual([]);
+
+    expect(await poller.poll(7)).toBe(true);
+    expect(applied).toEqual(['settled']);
+  });
+
+  test('allows only one read in flight and ignores a disposed poll', async () => {
+    const read = deferred<string>();
+    let reads = 0;
+    const applied: string[] = [];
+    const poller = createFileContentPoller({
+      readContent: () => {
+        reads += 1;
+        return read.promise;
+      },
+      getLoadedContent: () => 'before',
+      getLoadedRevision: () => 0,
+      isDirty: () => false,
+      applyContent: (content) => applied.push(content),
+      maxBytes: MAX_BYTES,
+    });
+
+    const first = poller.poll(5);
+    await poller.poll(5);
+    poller.dispose();
+    read.resolve('after');
+    await first;
+
+    expect(reads).toBe(1);
+    expect(applied).toEqual([]);
+  });
+
+  test('does not overwrite after an ABA save during the confirmation read', async () => {
+    const firstRead = deferred<string>();
+    const secondRead = deferred<string>();
+    let reads = 0;
+    let loadedRevision = 0;
+    const applied: string[] = [];
+    const poller = createFileContentPoller({
+      readContent: () => ++reads === 1 ? firstRead.promise : secondRead.promise,
+      getLoadedContent: () => 'before',
+      getLoadedRevision: () => loadedRevision,
+      isDirty: () => false,
+      applyContent: (content) => applied.push(content),
+      maxBytes: MAX_BYTES,
+    });
+
+    const polling = poller.poll(20);
+    firstRead.resolve('external edit');
+    await Promise.resolve();
+    loadedRevision += 1;
+    secondRead.resolve('external edit');
+    await polling;
+
+    expect(reads).toBe(2);
+    expect(applied).toEqual([]);
+  });
+
+  test('does not read content above the polling byte limit', async () => {
+    let reads = 0;
+    const poller = createFileContentPoller({
+      readContent: async () => {
+        reads += 1;
+        return 'content';
+      },
+      getLoadedContent: () => 'before',
+      getLoadedRevision: () => 0,
+      isDirty: () => false,
+      applyContent: () => undefined,
+      maxBytes: MAX_BYTES,
+    });
+
+    expect(await poller.poll(MAX_BYTES + 1)).toBe(false);
+
+    expect(reads).toBe(0);
+  });
+});
@@ -0,0 +1,43 @@
+type FileContentPollerOptions = {
+  readContent: () => Promise<string>;
+  getLoadedContent: () => string;
+  getLoadedRevision: () => number;
+  isDirty: () => boolean;
+  applyContent: (content: string) => void;
+  maxBytes: number;
+};
+
+export const createFileContentPoller = (options: FileContentPollerOptions) => {
+  let active = true;
+  let polling = false;
+
+  return {
+    /** Resolves true only when the poll observed the file's current content. */
+    poll: async (size: number): Promise<boolean> => {
+      if (!active || polling || options.isDirty() || size > options.maxBytes) return false;
+      polling = true;
+      const loadedContent = options.getLoadedContent();
+      const loadedRevision = options.getLoadedRevision();
+      try {
+        const content = await options.readContent();
+        if (!active || options.isDirty() || loadedRevision !== options.getLoadedRevision()) return false;
+        if (content === loadedContent) return true;
+
+        const confirmedContent = await options.readContent();
+        if (!active || options.isDirty() || confirmedContent !== content || loadedRevision !== options.getLoadedRevision()) {
+          return false;
+        }
+        options.applyContent(content);
+        return true;
+      } catch {
+        // A failed read is not proof the file is unchanged; the next poll retries.
+        return false;
+      } finally {
+        polling = false;
+      }
+    },
+    dispose: () => {
+      active = false;
+    },
+  };
+};
@@ -0,0 +1,17 @@
+import { describe, expect, test } from 'bun:test';
+
+import { hasFileStatChanged } from './fileStatChange';
+
+describe('hasFileStatChanged', () => {
+  test('ignores sub-millisecond mtime jitter on an unchanged file (issue #1489)', () => {
+    expect(hasFileStatChanged(
+      { size: 100, mtimeMs: 1700000000123.456 },
+      { size: 100, mtimeMs: 1700000000123.4561 },
+    )).toBe(false);
+  });
+
+  test('detects size changes and meaningful mtime changes', () => {
+    expect(hasFileStatChanged({ size: 100, mtimeMs: 1 }, { size: 101, mtimeMs: 1 })).toBe(true);
+    expect(hasFileStatChanged({ size: 100, mtimeMs: 1 }, { size: 100, mtimeMs: 2 })).toBe(true);
+  });
+});
@@ -0,0 +1,16 @@
+type FileStatChangeInput = {
+  size: number;
+  mtimeMs?: number;
+};
+
+// Some filesystems report sub-millisecond mtime jitter for unchanged files.
+const MIN_MTIME_CHANGE_MS = 1;
+
+export const hasFileStatChanged = (
+  previous: FileStatChangeInput,
+  latest: FileStatChangeInput,
+): boolean => latest.size !== previous.size || (
+  latest.mtimeMs !== undefined
+  && previous.mtimeMs !== undefined
+  && Math.abs(latest.mtimeMs - previous.mtimeMs) >= MIN_MTIME_CHANGE_MS
+);
@@ -36,6 +36,25 @@ describe("content cache owner", () => {
    owner.dispose()
  })

+  test("bypasses metadata cache for an authoritative read", async () => {
+    let content = "old"
+    let reads = 0
+    const owner = createContentCachedFiles({
+      readFile: async (path: string) => {
+        reads += 1
+        return { path, content }
+      },
+      statFile: async () => ({ isFile: true, isDirectory: false, size: 3, mtimeMs: 1 }),
+    } as unknown as FilesAPI)
+
+    await owner.files.readFile!("file.ts")
+    content = "new"
+    expect((await owner.files.readFile!("file.ts")).content).toBe("old")
+    expect((await owner.files.readFile!("file.ts", { fresh: true })).content).toBe("new")
+    expect(reads).toBe(2)
+    owner.dispose()
+  })
+
  test("retries a read that overlaps a write", async () => {
    const firstRead = deferred<{ path: string; content: string }>()
    let content = "old"
@@ -88,6 +88,10 @@ export function createContentCachedFiles(files: FilesAPI): { files: FilesAPI; di
        const capturedGeneration = generation;
        if (options?.allowOutsideWorkspace) return files.readFile!(path, options);
        const key = cacheKey(path, options);
+        if (options?.fresh) {
+          removeEntry(key);
+          return files.readFile!(path, options);
+        }
        const hit = cache.get(key);
        if (!hit) return readFresh(key, path, options, capturedGeneration);
        const latest = await files.statFile?.(path, options).catch(() => null);
@@ -655,6 +655,7 @@ interface FileReadOptions {
  outsideFileGrant?: string;
  optional?: boolean;
  directory?: string;
+  fresh?: boolean;
 }

 export interface FilesAPI {
@@ -77,6 +77,15 @@ describe('createWebFilesAPI', () => {
      cache: 'default',
      headers: { 'x-opencode-directory': '/worktree-a' },
    });
+
+    runtimeFetchMock.mockResolvedValueOnce(new Response('fresh content'));
+    await api.readFile?.('/worktree-b/file.txt', { directory: '/worktree-a', fresh: true });
+
+    expect(runtimeFetchMock).toHaveBeenLastCalledWith('/api/fs/read', {
+      query: new URLSearchParams({ path: '/worktree-b/file.txt' }),
+      cache: 'no-store',
+      headers: { 'x-opencode-directory': '/worktree-a' },
+    });
  });

  it('sends the workspace directory header for downloads', async () => {
@@ -197,7 +197,7 @@ export const createWebFilesAPI = ({ getDirectory }: WebFilesAPIOptions): FilesAP
    }
    const response = await runtimeFetch('/api/fs/read', {
      query: params,
-      cache: options?.optional ? 'no-store' : 'default',
+      cache: options?.optional || options?.fresh ? 'no-store' : 'default',
      headers: directoryHeaders(getDirectory, options?.directory),
    });