diff --git a/src/graph/load.ts b/src/graph/load.ts index c07eb42dc..a3d50842c 100644 --- a/src/graph/load.ts +++ b/src/graph/load.ts @@ -1,23 +1,20 @@ /** - * mtime-keyed in-process cache over the two readers `ask()` calls on every + * File-metadata-keyed in-process cache over the two readers `ask()` calls on every * query: the wiring graph (`readGraph`) and the ask sidecar (`readAskIndex`). * `graft ask` re-parses these from disk on every invocation; in a long-lived * process — the MCP server, or `graft ask` invoked repeatedly in one process — * that means re-parsing the same ~tens-of-MB JSON on every tool call. * - * Keyed by `(path, mtimeMs, size)` from `statSync`, so a rebuild (`graft - * build`) is picked up on the very next call with no polling and no TTL: the - * stat is cheap relative to the parse it guards, and a changed mtime or size - * invalidates the entry. A missing file returns null and is never cached — the - * next call re-stats, so a file created after a prior miss is picked up - * immediately (no negative caching). + * Keyed by `(path, dev, ino, ctimeMs, mtimeMs, size)` from `statSync`. + * Device/inode detect an atomic replacement even when its mtime and size are + * preserved; ctime, mtime and size detect observable in-place changes. A + * missing file returns null and drops any prior entry, so a file created after + * a miss is picked up on the next call (no negative caching). * - * **Cache invalidation assumption:** This strategy assumes filesystem mtime - * resolution is finer than build cadence. This is safe because `graft build` - * rewrites the entire output file atomically, so same-size rewrites within a - * single mtime tick (which would serve stale data) are infeasible in practice. - * On APFS (macOS) mtime is nanosecond-granular and a build takes milliseconds, - * so invalidation is immediate and reliable. + * Stat metadata is not a content hash: an in-place rewrite whose entire + * observed signature remains unchanged can still look like a warm entry. + * Same-process rebuild callers therefore continue to invalidate explicitly + * rather than depending on filesystem timestamp resolution. * * Dependency-free by design: this module imports only `node:fs`, `./write.js`, * and `../ask/index-file.js`, so it can be imported from both `ask.ts` and @@ -28,9 +25,15 @@ import { readGraph, wiringPath } from "./write.js"; import { readAskIndex, askIndexPath, type AskIndex } from "../ask/index-file.js"; import type { GraphV1 } from "./types.js"; -interface CacheEntry { +interface FileStamp { + dev: number; + ino: number; + ctimeMs: number; mtimeMs: number; size: number; +} + +interface CacheEntry extends FileStamp { value: T | null; } @@ -47,10 +50,10 @@ export function __resetParseCounts(): void { __parseCount.askIndex = 0; } -function statOf(path: string): { mtimeMs: number; size: number } | null { +function statOf(path: string): FileStamp | null { try { const s = statSync(path); - return { mtimeMs: s.mtimeMs, size: s.size }; + return { dev: s.dev, ino: s.ino, ctimeMs: s.ctimeMs, mtimeMs: s.mtimeMs, size: s.size }; } catch { return null; } @@ -66,22 +69,29 @@ function loadCached( if (!st) { // Missing file: don't negatively cache, and drop any stale entry so a // subsequently-created file at the same path is re-parsed, not served - // from a cache keyed to the old (mtime, size). + // from a cache keyed to the old file identity/metadata. cache.delete(path); return null; } const cached = cache.get(path); - if (cached && cached.mtimeMs === st.mtimeMs && cached.size === st.size) { + if ( + cached && + cached.dev === st.dev && + cached.ino === st.ino && + cached.ctimeMs === st.ctimeMs && + cached.mtimeMs === st.mtimeMs && + cached.size === st.size + ) { return cached.value; } onParse(); const value = parse(); - cache.set(path, { mtimeMs: st.mtimeMs, size: st.size, value }); + cache.set(path, { ...st, value }); return value; } /** Cached `readGraph(wiringPath(outDir))` — same null-on-missing/unparseable - * semantics, re-reads only when the wiring file's `(mtimeMs, size)` changed. + * semantics, re-reads only when the wiring file's identity/metadata changed. * Returns a shared cached reference; callers must not mutate the returned graph. */ export function loadGraphCached(outDir: string): GraphV1 | null { const path = wiringPath(outDir); @@ -102,12 +112,10 @@ export function loadAskIndexCached(outDir: string): AskIndex | null { /** * Drop the cached graph + sidecar for `outDir`, forcing the next load to re-read. * - * The `(mtimeMs, size)` key above is a good invalidator for a build that happened - * in *another* process, but not for one this process just performed: the - * pre-query auto-refresh (`refresh.ts`) rewrites the graph and then immediately - * queries it, and on a filesystem with coarse mtime resolution a same-tick rewrite - * of the same size would be served from the stale entry. Callers that rebuild must - * therefore invalidate explicitly rather than trust the clock. + * The file identity/metadata key detects external atomic replacements, but + * cannot guarantee detection of an in-place rewrite with an unchanged stat + * signature. The pre-query auto-refresh (`refresh.ts`) rebuilds and immediately + * queries the graph, so callers that rebuild must still invalidate explicitly. */ export function invalidateGraphCaches(outDir: string): void { graphCache.delete(wiringPath(outDir)); diff --git a/test/graph-load-identity.test.ts b/test/graph-load-identity.test.ts new file mode 100644 index 000000000..19a231f29 --- /dev/null +++ b/test/graph-load-identity.test.ts @@ -0,0 +1,95 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, mkdirSync, renameSync, rmSync, statSync, utimesSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { loadGraphCached, loadAskIndexCached, invalidateGraphCaches, __parseCount, __resetParseCounts } from "../src/graph/load.js"; +import { writeGraph, wiringPath, readGraph } from "../src/graph/write.js"; +import { writeAskIndex, askIndexPath, readAskIndex } from "../src/ask/index-file.js"; +import type { GraphV1 } from "../src/graph/types.js"; + +const fixedTime = new Date("2023-11-14T22:13:20.000Z"); +function graph(id: string): GraphV1 { + return { version: 1, nodes: [{ id, name: id, kind: "function", path: `${id}.ts`, span: "L1-L1", signature: null, exported: true, origin: "ast", body_hash: id, summary_state: "pending", summary: null, crux: null }], edges: [] }; +} +const readers = [ + { kind: "graph", load: loadGraphCached, write: (dir: string, id: string) => writeGraph(graph(id), dir), path: wiringPath, id: (value: any) => value?.nodes[0]?.id, read: (dir: string) => readGraph(wiringPath(dir)), count: () => __parseCount.graph }, + { kind: "ask", load: loadAskIndexCached, write: (dir: string, id: string) => writeAskIndex(dir, graph(id)), path: askIndexPath, id: (value: any) => value?.docs[0]?.id, read: readAskIndex, count: () => __parseCount.askIndex }, +]; +function fixture(t: any): string { + const dir = mkdtempSync(join(tmpdir(), "graft-cache-identity-")); + t.after(() => { invalidateGraphCaches(dir); rmSync(dir, { recursive: true, force: true }); }); + return dir; +} +function stamp(path: string): void { utimesSync(path, fixedTime, fixedTime); } +function identity(path: string) { + const s = statSync(path); + return { dev: s.dev, ino: s.ino, ctimeMs: s.ctimeMs, mtimeMs: s.mtimeMs, size: s.size }; +} + +for (const r of readers) { + test(`${r.kind}: same-size same-mtime atomic rename reloads and stays warm`, (t) => { + const dir = fixture(t), replacement = fixture(t); + r.write(dir, "alpha"); stamp(r.path(dir)); __resetParseCounts(); + const first = r.load(dir); + assert.equal(r.id(first), "alpha"); assert.strictEqual(r.load(dir), first); assert.equal(r.count(), 1); + const before = identity(r.path(dir)); + r.write(replacement, "bravo"); stamp(r.path(replacement)); + renameSync(r.path(replacement), r.path(dir)); + const after = identity(r.path(dir)); + assert.equal(after.mtimeMs, before.mtimeMs); assert.equal(after.size, before.size); + assert.notEqual(after.ino, before.ino, "real replacement must have a distinct inode"); + assert.equal(r.id(r.read(dir)), "bravo", "uncached production reader must see replacement bytes"); + const second = r.load(dir); + assert.equal(r.id(second), "bravo", "cached loader must pick up atomic replacement"); + assert.notStrictEqual(second, first); assert.equal(r.count(), 2); + assert.strictEqual(r.load(dir), second); assert.equal(r.count(), 2); + }); + test(`${r.kind}: explicit invalidation forces an unchanged file to reparse`, (t) => { + const dir = fixture(t); r.write(dir, "alpha"); stamp(r.path(dir)); __resetParseCounts(); + const first = r.load(dir); assert.equal(r.id(first), "alpha"); + assert.strictEqual(r.load(dir), first); assert.equal(r.count(), 1); + invalidateGraphCaches(dir); + const second = r.load(dir); assert.deepEqual(second, first); assert.notStrictEqual(second, first); assert.equal(r.count(), 2); + }); + test(`${r.kind}: missing files do not cache a miss or retain a removed value`, (t) => { + const dir = fixture(t); __resetParseCounts(); + assert.equal(r.load(dir), null); assert.equal(r.load(dir), null); assert.equal(r.count(), 0); + r.write(dir, "alpha"); stamp(r.path(dir)); assert.equal(r.id(r.load(dir)), "alpha"); assert.equal(r.count(), 1); + rmSync(r.path(dir)); assert.equal(r.load(dir), null); assert.equal(r.count(), 1); + r.write(dir, "bravo"); stamp(r.path(dir)); assert.equal(r.id(r.load(dir)), "bravo"); assert.equal(r.count(), 2); + }); + test(`${r.kind}: malformed JSON stays null and atomic repair is read`, (t) => { + const dir = fixture(t), repair = fixture(t); r.write(repair, "alpha"); + const size = statSync(r.path(repair)).size; + mkdirSync(join(dir, r.kind === "graph" ? ".graph" : ".cache"), { recursive: true }); + writeFileSync(r.path(dir), "x".repeat(size)); stamp(r.path(dir)); __resetParseCounts(); + assert.equal(r.load(dir), null); assert.equal(r.load(dir), null); assert.equal(r.count(), 1); + stamp(r.path(repair)); renameSync(r.path(repair), r.path(dir)); + assert.equal(r.id(r.load(dir)), "alpha"); assert.equal(r.count(), 2); + }); +} + +test("root paths isolate graph and ask values with identical size/mtime", (t) => { + const parent = fixture(t), left = join(parent, "left", "graft"), right = join(parent, "right", "graft"); + t.after(() => { invalidateGraphCaches(left); invalidateGraphCaches(right); }); + __resetParseCounts(); + for (const r of readers) { + r.write(left, "alpha"); r.write(right, "bravo"); stamp(r.path(left)); stamp(r.path(right)); + const a = identity(r.path(left)), b = identity(r.path(right)); + assert.equal(a.size, b.size); assert.equal(a.mtimeMs, b.mtimeMs); + const one = r.load(left), two = r.load(right); + assert.equal(r.id(one), "alpha"); assert.equal(r.id(two), "bravo"); assert.notStrictEqual(one, two); + assert.strictEqual(r.load(left), one); assert.strictEqual(r.load(right), two); assert.equal(r.count(), 2); + invalidateGraphCaches(left); assert.equal(r.id(r.load(left)), "alpha"); + assert.strictEqual(r.load(right), two); assert.equal(r.count(), 3); + } +}); + +test("real graph/ask rebuild plus explicit invalidation needs no sleep", (t) => { + const dir = fixture(t); __resetParseCounts(); + for (const r of readers) { r.write(dir, "alpha"); stamp(r.path(dir)); assert.equal(r.id(r.load(dir)), "alpha"); } + for (const r of readers) { r.write(dir, "bravo"); stamp(r.path(dir)); } + invalidateGraphCaches(dir); + for (const r of readers) { const value = r.load(dir); assert.equal(r.id(value), "bravo"); assert.strictEqual(r.load(dir), value); assert.equal(r.count(), 2); } +});