fix(files): keep text opens working after content-cache dispose
React Strict Mode disposed the memoized content-cache owner while the provider kept reusing it, so validateContextFileOpen threw before any /api/fs/read and toasted "Failed to open file" for notes.txt. Binaries still opened because they skip the pre-read. Serve uncached reads from disposed owners, clear cache on runtime switch without deactivating, and own the cache lifecycle in an effect. Co-authored-by: Serhii Dziupin <makeittech@users.noreply.github.com>
This commit is contained in:
co-authored by
Serhii Dziupin
parent
d35cf957ac
commit
fc4db0c656
@@ -1,17 +1,32 @@
|
|||||||
import React, { type JSX, type ReactNode } from 'react';
|
import React, { type JSX, type ReactNode } from 'react';
|
||||||
import { RuntimeAPIContext } from '@/contexts/runtimeAPIContext';
|
import { RuntimeAPIContext } from '@/contexts/runtimeAPIContext';
|
||||||
import type { RuntimeAPIs } from '@/lib/api/types';
|
import type { FilesAPI, RuntimeAPIs } from '@/lib/api/types';
|
||||||
import { createContentCachedFiles } from '@/contexts/content-cache-owner';
|
import { createContentCachedFiles } from '@/contexts/content-cache-owner';
|
||||||
|
|
||||||
|
type ContentCachedFiles = ReturnType<typeof createContentCachedFiles>;
|
||||||
|
|
||||||
export function RuntimeAPIProvider({ apis, children }: { apis: RuntimeAPIs; children: ReactNode }): JSX.Element {
|
export function RuntimeAPIProvider({ apis, children }: { apis: RuntimeAPIs; children: ReactNode }): JSX.Element {
|
||||||
const cachedFiles = React.useMemo(() => createContentCachedFiles(apis.files), [apis.files]);
|
// Effect-owned lifecycle: React Strict Mode dispose+remount must create a fresh
|
||||||
React.useEffect(() => () => cachedFiles.dispose(), [cachedFiles]);
|
// owner. useMemo + dispose reused a dead owner and broke text-file opens
|
||||||
|
// (binaries skipped the pre-read, so they still appeared to work).
|
||||||
|
const [cachedOwner, setCachedOwner] = React.useState<ContentCachedFiles | null>(null);
|
||||||
|
|
||||||
|
React.useEffect(() => {
|
||||||
|
const owner = createContentCachedFiles(apis.files);
|
||||||
|
setCachedOwner(owner);
|
||||||
|
return () => {
|
||||||
|
owner.dispose();
|
||||||
|
setCachedOwner((current) => (current === owner ? null : current));
|
||||||
|
};
|
||||||
|
}, [apis.files]);
|
||||||
|
|
||||||
|
const files: FilesAPI = cachedOwner?.files ?? apis.files;
|
||||||
const cachedApis = React.useMemo<RuntimeAPIs>(
|
const cachedApis = React.useMemo<RuntimeAPIs>(
|
||||||
() => ({
|
() => ({
|
||||||
...apis,
|
...apis,
|
||||||
files: cachedFiles.files,
|
files,
|
||||||
}),
|
}),
|
||||||
[apis, cachedFiles],
|
[apis, files],
|
||||||
);
|
);
|
||||||
return <RuntimeAPIContext.Provider value={cachedApis}>{children}</RuntimeAPIContext.Provider>;
|
return <RuntimeAPIContext.Provider value={cachedApis}>{children}</RuntimeAPIContext.Provider>;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -71,4 +71,68 @@ describe("content cache owner", () => {
|
|||||||
expect(second.content).toBe("/b-2")
|
expect(second.content).toBe("/b-2")
|
||||||
owner.dispose()
|
owner.dispose()
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test("disposed owners still read through without throwing", async () => {
|
||||||
|
let reads = 0
|
||||||
|
const owner = createContentCachedFiles({
|
||||||
|
readFile: async (path: string) => ({ path, content: `value-${++reads}` }),
|
||||||
|
statFile: async () => ({ isFile: true, isDirectory: false, size: 7, mtimeMs: 1 }),
|
||||||
|
} as unknown as FilesAPI)
|
||||||
|
|
||||||
|
owner.dispose()
|
||||||
|
expect((await owner.files.readFile!("notes.txt", { optional: true, directory: "/tmp/project" })).content).toBe("value-1")
|
||||||
|
expect(reads).toBe(1)
|
||||||
|
})
|
||||||
|
|
||||||
|
test("validateContextFileOpen succeeds against a disposed cached files API", async () => {
|
||||||
|
const { validateContextFileOpen } = await import("@/lib/contextFileOpenGuard")
|
||||||
|
const owner = createContentCachedFiles({
|
||||||
|
listDirectory: async () => ({ directory: "/", entries: [] }),
|
||||||
|
readFile: async (path: string) => ({ path, content: "hello from notes\n" }),
|
||||||
|
} as unknown as FilesAPI)
|
||||||
|
|
||||||
|
owner.dispose()
|
||||||
|
expect(await validateContextFileOpen(owner.files, "/tmp/project/notes.txt", { directory: "/tmp/project" })).toEqual({
|
||||||
|
ok: true,
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
test("runtime endpoint changes clear cache but keep serving reads", async () => {
|
||||||
|
const originalWindow = globalThis.window
|
||||||
|
const events = new EventTarget()
|
||||||
|
Object.defineProperty(globalThis, "window", {
|
||||||
|
configurable: true,
|
||||||
|
value: {
|
||||||
|
addEventListener: events.addEventListener.bind(events),
|
||||||
|
removeEventListener: events.removeEventListener.bind(events),
|
||||||
|
dispatchEvent: events.dispatchEvent.bind(events),
|
||||||
|
},
|
||||||
|
})
|
||||||
|
|
||||||
|
try {
|
||||||
|
let reads = 0
|
||||||
|
const owner = createContentCachedFiles({
|
||||||
|
readFile: async (path: string) => ({ path, content: `value-${++reads}` }),
|
||||||
|
statFile: async () => ({ isFile: true, isDirectory: false, size: 7, mtimeMs: 1 }),
|
||||||
|
} as unknown as FilesAPI)
|
||||||
|
|
||||||
|
expect((await owner.files.readFile!("notes.txt")).content).toBe("value-1")
|
||||||
|
window.dispatchEvent(new CustomEvent("openchamber:runtime-endpoint-will-change", {
|
||||||
|
detail: {
|
||||||
|
apiBaseUrl: "http://127.0.0.1:3902",
|
||||||
|
previousApiBaseUrl: "http://127.0.0.1:3901",
|
||||||
|
runtimeKey: "url:http://127.0.0.1:3902",
|
||||||
|
previousRuntimeKey: "url:http://127.0.0.1:3901",
|
||||||
|
},
|
||||||
|
}))
|
||||||
|
expect((await owner.files.readFile!("notes.txt")).content).toBe("value-2")
|
||||||
|
expect(reads).toBe(2)
|
||||||
|
owner.dispose()
|
||||||
|
} finally {
|
||||||
|
Object.defineProperty(globalThis, "window", {
|
||||||
|
configurable: true,
|
||||||
|
value: originalWindow,
|
||||||
|
})
|
||||||
|
}
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -32,6 +32,10 @@ export function createContentCachedFiles(files: FilesAPI): { files: FilesAPI; di
|
|||||||
&& cached.mtimeMs === latest.mtimeMs
|
&& cached.mtimeMs === latest.mtimeMs
|
||||||
&& cached.size === latest.size
|
&& cached.size === latest.size
|
||||||
);
|
);
|
||||||
|
const clearCache = () => {
|
||||||
|
cache.clear();
|
||||||
|
totalBytes = 0;
|
||||||
|
};
|
||||||
const cacheResult = (
|
const cacheResult = (
|
||||||
key: string,
|
key: string,
|
||||||
sourcePath: string,
|
sourcePath: string,
|
||||||
@@ -61,7 +65,8 @@ export function createContentCachedFiles(files: FilesAPI): { files: FilesAPI; di
|
|||||||
const before = await files.statFile?.(path, options).catch(() => null);
|
const before = await files.statFile?.(path, options).catch(() => null);
|
||||||
const result = await files.readFile!(path, options);
|
const result = await files.readFile!(path, options);
|
||||||
const after = await files.statFile?.(path, options).catch(() => null);
|
const after = await files.statFile?.(path, options).catch(() => null);
|
||||||
if (!active) throw new Error('File read invalidated by runtime change');
|
// Disposed mid-read: still return the bytes we fetched; do not cache.
|
||||||
|
if (!active) return result;
|
||||||
if (capturedGeneration !== generation) return cachedReadFile!(path, options);
|
if (capturedGeneration !== generation) return cachedReadFile!(path, options);
|
||||||
const stable = before && after && before.isFile && after.isFile
|
const stable = before && after && before.isFile && after.isFile
|
||||||
&& before.mtimeMs !== undefined && after.mtimeMs !== undefined
|
&& before.mtimeMs !== undefined && after.mtimeMs !== undefined
|
||||||
@@ -72,14 +77,17 @@ export function createContentCachedFiles(files: FilesAPI): { files: FilesAPI; di
|
|||||||
const cachedReadFile: FilesAPI['readFile'] = files.readFile
|
const cachedReadFile: FilesAPI['readFile'] = files.readFile
|
||||||
? async (path, options) => {
|
? async (path, options) => {
|
||||||
await mutationBarrier;
|
await mutationBarrier;
|
||||||
if (!active) throw new Error('File cache owner disposed');
|
// Disposed owners must keep serving reads. React Strict Mode can dispose a
|
||||||
|
// memoized owner that the provider still holds; throwing here surfaces as
|
||||||
|
// "Failed to open file" with no /api/fs/read request for text opens.
|
||||||
|
if (!active) return files.readFile!(path, options);
|
||||||
const capturedGeneration = generation;
|
const capturedGeneration = generation;
|
||||||
if (options?.allowOutsideWorkspace) return files.readFile!(path, options);
|
if (options?.allowOutsideWorkspace) return files.readFile!(path, options);
|
||||||
const key = cacheKey(path, options);
|
const key = cacheKey(path, options);
|
||||||
const hit = cache.get(key);
|
const hit = cache.get(key);
|
||||||
if (!hit) return readFresh(key, path, options, capturedGeneration);
|
if (!hit) return readFresh(key, path, options, capturedGeneration);
|
||||||
const latest = await files.statFile?.(path, options).catch(() => null);
|
const latest = await files.statFile?.(path, options).catch(() => null);
|
||||||
if (!active) throw new Error('File read invalidated by runtime change');
|
if (!active) return files.readFile!(path, options);
|
||||||
if (capturedGeneration !== generation) return cachedReadFile!(path, options);
|
if (capturedGeneration !== generation) return cachedReadFile!(path, options);
|
||||||
if (!latest || !metadataMatches(hit, latest)) {
|
if (!latest || !metadataMatches(hit, latest)) {
|
||||||
removeEntry(key);
|
removeEntry(key);
|
||||||
@@ -116,18 +124,18 @@ export function createContentCachedFiles(files: FilesAPI): { files: FilesAPI; di
|
|||||||
};
|
};
|
||||||
const unsubscribeRuntime = subscribeRuntimeEndpointWillChange((detail) => {
|
const unsubscribeRuntime = subscribeRuntimeEndpointWillChange((detail) => {
|
||||||
if (detail.runtimeKey === detail.previousRuntimeKey) return;
|
if (detail.runtimeKey === detail.previousRuntimeKey) return;
|
||||||
active = false;
|
// Invalidate cached content for the previous runtime, but keep serving reads.
|
||||||
|
// `apis.files` is typically stable across endpoint switches, so permanently
|
||||||
|
// deactivating this owner would break every subsequent text-file open.
|
||||||
generation += 1;
|
generation += 1;
|
||||||
cache.clear();
|
clearCache();
|
||||||
totalBytes = 0;
|
|
||||||
});
|
});
|
||||||
return {
|
return {
|
||||||
files: cachedFiles,
|
files: cachedFiles,
|
||||||
dispose: () => {
|
dispose: () => {
|
||||||
active = false;
|
active = false;
|
||||||
generation += 1;
|
generation += 1;
|
||||||
cache.clear();
|
clearCache();
|
||||||
totalBytes = 0;
|
|
||||||
unsubscribeRuntime();
|
unsubscribeRuntime();
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|||||||
Reference in New Issue
Block a user