diff --git a/.changeset/fs-watch-invalidates-list-cache.md b/.changeset/fs-watch-invalidates-list-cache.md new file mode 100644 index 0000000000..f4ccc6a39f --- /dev/null +++ b/.changeset/fs-watch-invalidates-list-cache.md @@ -0,0 +1,46 @@ +--- +"@objectstack/metadata": patch +--- + +fix(metadata): 文件系统改动同样失效本节点的 `listCache`/`registry`,不再只叫醒 watcher (#5218) + +`NodeMetadataManager.handleFileEvent()` 在 chokidar 报告 `add` / `change` / +`unlink` 之后只做两件事:重新 `load()` 一次文件内容,然后 `notifyWatchers()`。 +它既不碰 `listCache` 也不碰 `registry` —— 而 `load()` 是纯读路径(它委托给 +`loadDiagnosed`,后者只遍历 loader),两个缓存都不写。 + +后果是**同一个 manager 的两个读接口互相矛盾**。手改 `rootDir` 下的 +`view/.json` 之后: + +- `get(type, name)` 是新的 —— 它穿透到 `FilesystemLoader`; +- `list(type)` 在 `LIST_CACHE_TTL_MS`(30 秒)窗口内继续返回改动前的清单 —— + REST `/api/v1/metadata/:type`、Studio 左栏、`listViews()` 等一切走 `list()` + 的读都受影响。 + +更糟的是被这次事件叫醒的消费者(Studio HMR/SSE 流、ObjectQL SchemaRegistry +桥)正是通过回头拉 `list()` 来响应的,于是这次唤醒**递回了它自己刚刚宣告已失效 +的那份数据**。 + +这与 #5109(集群对端写入不失效本节点缓存)是同一形状、不同触发源,因此复用该 +修复落地的 `invalidateForForeignWrite(type, name)`(可见性由 `private` 放宽为 +`protected`):文件改动正是「不是经由本 manager 写接口发生的写入」,没有任何东西 +替它刷新过缓存,delete-而非-预填 的语义也正好对上 —— 穿透回 loader 读到的就是 +文件的真相。 + +两点与基类其余写路径一致的约束: + +- **先失效,再通知**(`register` / `unregister` / `applyRepoEvent` / 集群订阅者 + 都是这个次序),使 watcher 不可能同时观察到事件与事件前的缓存; +- **registry 条目一并删除**,不只是列表缓存。FS 加载的条目本来就不进 registry, + 通常无可删;但当同名条目此前被 `register()` / `registerInMemory()` 写过时, + 它在 `get()` 和 `list()` 中都会**遮蔽** loader,只删列表缓存会让那份陈旧副本 + 一直应答下去。 + +命中面主要是开发期:`MetadataPlugin` 默认 `watch: true`,在 +`bootstrap: 'artifact-only'` 下被强制关闭,`standalone-stack` 显式传 +`watch: false`。因此 artifact 模式的 `os dev` 与 standalone 不受影响,非 artifact +的默认 `MetadataPlugin` 装配受影响。 + +`type === 'api'` 的行为不变:端点索引此前已由 #5089 装的 `subscribe('api', …)` +那条缝覆盖,本次改动把 `invalidateListCache` 那条缝也接上,两条缝对称。 +`EndpointMatcher.invalidate()` 是两次赋 `undefined`,重复失效幂等。 diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index c69369c4bb..9d7881224c 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -2036,14 +2036,18 @@ export class MetadataManager implements IMetadataService { * we did not perform ourselves has just invalidated, so the next read falls * through to the source of truth. * - * The two callers are the manager's two *foreign-write* seams — the - * repository watch loop ({@link applyRepoEvent}) and the cluster peer replay - * in {@link attachClusterPubSub}. Both learn about a write that landed - * somewhere else (the repo head; another node's `sys_metadata`) and hold - * caches that the write silently aged out. Local writes do not come through - * here: `register()` / `unregister()` / `registerInMemory()` update the - * registry to the value they just wrote and call `invalidateListCache()` - * themselves. + * The callers are the manager's *foreign-write* seams — the repository watch + * loop ({@link applyRepoEvent}), the cluster peer replay in + * {@link attachClusterPubSub}, and — since #5218 — `NodeMetadataManager`'s + * chokidar handler, which is why this is `protected` rather than `private`. + * All three learn about a write that landed somewhere else (the repo head; + * another node's `sys_metadata`; an editor writing `rootDir/view/x.json`) and + * hold caches that the write silently aged out. A file event qualifies on + * exactly the definition that matters here: it did not come through this + * manager's write API, so nothing has updated the caches on its behalf. + * Local writes do not come through here: `register()` / `unregister()` / + * `registerInMemory()` update the registry to the value they just wrote and + * call `invalidateListCache()` themselves. * * **Delete, do not pre-fill.** Even when the event carries a body we drop the * registry entry rather than writing the body into it: the body reaching us @@ -2052,7 +2056,9 @@ export class MetadataManager implements IMetadataService { * a definition we did not load. Lazy invalidation is the safer default — * `get()` then falls through to the loaders / repository, which is where the * truth is. (This paragraph is the rationale `applyRepoEvent` carried since - * ADR-0008 PR-6; #5109 extended the same choice to the cluster path.) + * ADR-0008 PR-6; #5109 extended the same choice to the cluster path, #5218 to + * the filesystem watcher — where "the truth" is the file chokidar just + * reported, served by the `FilesystemLoader` the registry entry was shadowing.) * * `name` is optional because `MetadataWatchEvent.name` is: a nameless event * cannot address a registry entry, so it invalidates the list cache only. @@ -2060,7 +2066,7 @@ export class MetadataManager implements IMetadataService { * artefacts (code-owned datasources, ADR-0015 Addendum) that no loader can * restore — an unrecoverable loss in exchange for a guess. */ - private invalidateForForeignWrite(type: string, name?: string): void { + protected invalidateForForeignWrite(type: string, name?: string): void { if (name) { const typeStore = this.registry.get(type); if (typeStore) { diff --git a/packages/metadata/src/node-metadata-manager-fs-invalidation.test.ts b/packages/metadata/src/node-metadata-manager-fs-invalidation.test.ts new file mode 100644 index 0000000000..d7f11fc39d --- /dev/null +++ b/packages/metadata/src/node-metadata-manager-fs-invalidation.test.ts @@ -0,0 +1,341 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5218 — a filesystem change must invalidate THIS node's caches, not just wake + * its watchers. + * + * `NodeMetadataManager.handleFileEvent()` used to do exactly two things when + * chokidar reported `add` / `change` / `unlink`: re-`load()` the file (a pure + * read — it delegates to `loadDiagnosed`, which only walks the loaders and + * writes neither cache) and `notifyWatchers()`. It never touched `listCache` + * or `registry`. So after editing `rootDir/view/x.json` the manager's two read + * surfaces contradicted each other for up to `LIST_CACHE_TTL_MS` (30s): + * `get()` returned the new definition because it falls through to the + * FilesystemLoader, while `list()` — REST `/api/v1/metadata/:type`, the Studio + * left rail, `listViews()` — kept serving the pre-change set. The HMR/SSE + * consumers the event woke answer it by re-reading through `list()`, so the + * wake-up handed back precisely the stale data it was announcing. + * + * Same defect shape as #5109 (a cluster peer's write) with a different + * trigger, and it is fixed by reusing that fix's helper — + * `invalidateForForeignWrite`, widened `private` → `protected`. A file event + * is a foreign write on the definition that matters: it did not come through + * this manager's write API, so nothing refreshed the caches on its behalf. + * + * **Test approach.** These drive `handleFileEvent` directly with a synthetic + * event rather than waiting on real chokidar — everything else is real (real + * files in a real tmpdir, the real default `FilesystemLoader`, the real + * `list()` / `get()` / `matchEndpoint` read paths), so only the notification + * is synthesized. `startWatching` polls at `interval: 1000`, which would make + * every case here a multi-second wait for no added coverage of the seam under + * test. The one thing a synthetic event cannot prove — that chokidar's + * callbacks actually reach `handleFileEvent` — is pinned by the end-to-end + * case at the bottom, which uses a real watcher. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { NodeMetadataManager } from './node-metadata-manager.js'; +import type { MetadataManager } from './metadata-manager.js'; + +vi.mock('@objectstack/core', () => ({ + createLogger: () => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }), +})); + +let rootDir: string; +const managers: NodeMetadataManager[] = []; + +beforeEach(async () => { + rootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'os-5218-')); +}); + +afterEach(async () => { + for (const mgr of managers.splice(0)) await mgr.stopWatching(); + await fs.rm(rootDir, { recursive: true, force: true }); +}); + +/** Write `rootDir//.json`, returning the absolute path. */ +async function writeMetadataFile( + type: string, + name: string, + data: Record, +): Promise { + const dir = path.join(rootDir, type); + await fs.mkdir(dir, { recursive: true }); + const filePath = path.join(dir, `${name}.json`); + await fs.writeFile(filePath, JSON.stringify(data), 'utf-8'); + return filePath; +} + +function makeManager(watch = false): NodeMetadataManager { + // No `loaders`, so the constructor installs the default FilesystemLoader + // over `rootDir` — the real read path this issue is about. + const mgr = new NodeMetadataManager({ rootDir, watch, formats: ['json'] }); + managers.push(mgr); + return mgr; +} + +/** + * Deliver a chokidar-shaped event straight to the handler under test. See the + * "Test approach" note above for why this is preferred over a real watcher. + */ +function fireFileEvent( + mgr: NodeMetadataManager, + eventType: 'added' | 'changed' | 'deleted', + filePath: string, +): Promise { + return ( + mgr as unknown as { + handleFileEvent(t: 'added' | 'changed' | 'deleted', p: string): Promise; + } + ).handleFileEvent(eventType, filePath); +} + +/** Read the private list cache without waiting on any async seam. */ +const cachedTypes = (mgr: MetadataManager): string[] => + Array.from((mgr as unknown as { listCache: Map }).listCache.keys()); + +const view = (name: string, title = name) => ({ name, title, object: 'account' }); + +const viewNames = (items: unknown[]): string[] => + (items as { name: string }[]).map((v) => v.name).sort(); + +describe('#5218 — an FS change invalidates the local listCache', () => { + it('a pre-warmed list() sees a new file without waiting out the 30s TTL', async () => { + // The issue's repro, verbatim. + await writeMetadataFile('view', 'v_a', view('v_a')); + const mgr = makeManager(); + + expect(viewNames(await mgr.list('view'))).toEqual(['v_a']); // pre-warm + + const added = await writeMetadataFile('view', 'v_b', view('v_b')); + await fireFileEvent(mgr, 'added', added); + + // Same tick, no timers: the 30s TTL has not lapsed. Before the fix this + // returned ['v_a'] for another 30 seconds. + expect(viewNames(await mgr.list('view'))).toEqual(['v_a', 'v_b']); + }); + + it('stops get() and list() contradicting each other', async () => { + const filePath = await writeMetadataFile('view', 'v_a', view('v_a', 'old')); + const mgr = makeManager(); + await mgr.list('view'); // pre-warm + + await fs.writeFile(filePath, JSON.stringify(view('v_a', 'new')), 'utf-8'); + await fireFileEvent(mgr, 'changed', filePath); + + // `get()` was ALWAYS right — it falls through to the loader. The bug + // was that `list()` disagreed with it, which is what this pins. + expect((await mgr.get('view', 'v_a') as { title: string }).title).toBe('new'); + expect((await mgr.list('view') as { title: string }[])[0].title).toBe('new'); + }); + + it('drops a deleted file from a pre-warmed list()', async () => { + const filePath = await writeMetadataFile('view', 'v_doomed', view('v_doomed')); + const mgr = makeManager(); + expect(viewNames(await mgr.list('view'))).toEqual(['v_doomed']); + + await fs.rm(filePath); + await fireFileEvent(mgr, 'deleted', filePath); + + expect(await mgr.list('view')).toEqual([]); + }); + + it('invalidates BEFORE notifying — a watcher never sees the pre-event cache', async () => { + await writeMetadataFile('view', 'v_a', view('v_a')); + const mgr = makeManager(); + await mgr.list('view'); + expect(cachedTypes(mgr)).toEqual(['view']); + + // `notifyWatchersLocal` is synchronous, so this callback runs INSIDE + // the notify. Ordering the invalidation after it would leave a window + // in which the event and the stale cache are observable together — + // every `await` in a request handler is such a window. + const cacheSeenByWatcher: string[][] = []; + mgr.subscribe('view', () => { cacheSeenByWatcher.push(cachedTypes(mgr)); }); + + const added = await writeMetadataFile('view', 'v_b', view('v_b')); + await fireFileEvent(mgr, 'added', added); + + expect(cacheSeenByWatcher).toEqual([[]]); + }); + + it('a watcher that re-reads via list() on the wake-up gets the post-change set', async () => { + await writeMetadataFile('view', 'v_a', view('v_a')); + const mgr = makeManager(); + await mgr.list('view'); + + // The Studio HMR SSE stream and the ObjectQL SchemaRegistry bridge both + // react to the event by re-reading. That re-read is what used to + // contradict the notification it was answering. + let seenByWatcher: string[] = []; + const observed = new Promise((resolve) => { + mgr.subscribe('view', () => { + void mgr.list('view').then((items) => { + seenByWatcher = viewNames(items); + resolve(); + }); + }); + }); + + const added = await writeMetadataFile('view', 'v_b', view('v_b')); + await fireFileEvent(mgr, 'added', added); + await observed; + + expect(seenByWatcher).toEqual(['v_a', 'v_b']); + }); + + it('drops a shadowing registry entry so reads fall through to the file', async () => { + // FS-loaded items never enter the registry, so usually there is nothing + // to delete. But `register()` writes one, and it SHADOWS the loader in + // both `get()` and `list()` (registry is merged first). Dropping only + // the list cache would leave this stale copy answering forever. + const filePath = await writeMetadataFile('view', 'v_shared', view('v_shared', 'from-file')); + const mgr = makeManager(); + // FilesystemLoader is read-only at runtime, so this stays registry-only + // — exactly the shadowing shape. + await mgr.register('view', 'v_shared', view('v_shared', 'from-registry')); + expect((await mgr.get('view', 'v_shared') as { title: string }).title).toBe('from-registry'); + + await fs.writeFile(filePath, JSON.stringify(view('v_shared', 'edited-on-disk')), 'utf-8'); + await fireFileEvent(mgr, 'changed', filePath); + + // Deleted, never pre-filled from the event's `data` — the answer comes + // from the loader, which is where the truth is. + expect((await mgr.get('view', 'v_shared') as { title: string }).title).toBe('edited-on-disk'); + expect((await mgr.list('view') as { title: string }[])[0].title).toBe('edited-on-disk'); + }); + + it('keeps in-memory-only entries of OTHER names intact', async () => { + // `invalidateForForeignWrite` deletes one name, never the whole type + // store: `registerInMemory` artefacts (code-owned datasources, ADR-0015 + // Addendum) exist in no loader, so evicting one is unrecoverable. + const mgr = makeManager(); + mgr.registerInMemory('datasource', 'crm_db', { name: 'crm_db', origin: 'code' }); + await mgr.list('datasource'); + + const added = await writeMetadataFile('datasource', 'other_db', { name: 'other_db' }); + await fireFileEvent(mgr, 'added', added); + + expect(await mgr.get('datasource', 'crm_db')).toMatchObject({ name: 'crm_db' }); + expect(viewNames(await mgr.list('datasource'))).toEqual(['crm_db', 'other_db']); + }); + + it('still invalidates when the changed file cannot be parsed', async () => { + // An unparseable file is a real change to the stored set — `loadMany` + // skips it, so the cached list is stale either way and must go. + // + // Note the handler does NOT take its `catch` here: `load()` delegates + // to `loadDiagnosed`, which absorbs a loader throw into + // `{ data: null, degraded: true }` and returns `null` rather than + // rethrowing. So the early-return guards nothing in this case and the + // event is announced with `data: null`. That announcement is its own + // (pre-existing, out-of-scope) problem — filed separately; this test + // pins only that the invalidation happens. + await writeMetadataFile('view', 'v_a', view('v_a')); + const mgr = makeManager(); + await mgr.list('view'); + expect(cachedTypes(mgr)).toEqual(['view']); + + const broken = path.join(rootDir, 'view', 'v_broken.json'); + await fs.writeFile(broken, '{ not json', 'utf-8'); + await fireFileEvent(mgr, 'added', broken); + + expect(cachedTypes(mgr)).toEqual([]); + // The good sibling still lists; the unparseable file simply is not there. + expect(viewNames(await mgr.list('view'))).toEqual(['v_a']); + }); + + it('ignores paths that are not metadata files', async () => { + await writeMetadataFile('view', 'v_a', view('v_a')); + const mgr = makeManager(); + await mgr.list('view'); + + // Fewer than 2 path segments below rootDir — no type to invalidate. + await fireFileEvent(mgr, 'changed', path.join(rootDir, 'README.md')); + + expect(cachedTypes(mgr)).toEqual(['view']); + }); +}); + +/** + * The `api` seam the issue asked to confirm — and it confirms a NON-defect, so + * read this block as a guard rather than a regression pin. + * + * Before this fix, an `api` file event still invalidated the endpoint index, + * through one of that index's two seams: the `subscribe('api', …)` watcher the + * base constructor registers (#5089). The other seam, `invalidateListCache`, + * was empty on this path — an asymmetry with every other write path in the + * manager, which the issue flagged as "worth confirming" precisely because it + * was not itself a bug. This case passes on both sides of the change; what it + * pins is that closing the second seam does not break the first. Per #5089's + * note the overlap is free: `EndpointMatcher.invalidate()` is two assignments + * to `undefined`, so invalidating twice per event is idempotent. + */ +describe('#5218 — the `api` path invalidates the endpoint index idempotently', () => { + const endpoint = (name: string, urlPath: string) => ({ + name, + path: urlPath, + method: 'GET', + type: 'object_operation', + target: 'showcase_task', + objectParams: { object: 'showcase_task', operation: 'find' }, + }); + + it('a new api file becomes matchable, and double invalidation is harmless', async () => { + await writeMetadataFile('api', 'list_tasks', endpoint('list_tasks', '/api/v1/apps/showcase/tasks')); + const mgr = makeManager(); + + // Build the index, so a stale one would be observable. + expect(await mgr.matchEndpoint({ method: 'GET', path: '/api/v1/apps/showcase/tasks' })) + .toMatchObject({ endpoint: { name: 'list_tasks' } }); + + const added = await writeMetadataFile('api', 'list_users', endpoint('list_users', '/api/v1/apps/showcase/users')); + await fireFileEvent(mgr, 'added', added); + + // Both seams fired for this one event (invalidateListCache + the + // watcher). Idempotent: the index rebuilt once and is correct. + expect(await mgr.matchEndpoint({ method: 'GET', path: '/api/v1/apps/showcase/users' })) + .toMatchObject({ endpoint: { name: 'list_users' } }); + expect(await mgr.matchEndpoint({ method: 'GET', path: '/api/v1/apps/showcase/tasks' })) + .toMatchObject({ endpoint: { name: 'list_tasks' } }); + }); +}); + +/** + * End-to-end with a REAL chokidar watcher — the one thing the synthetic events + * above cannot show is that the watcher's callbacks actually reach + * `handleFileEvent`. Polling at `interval: 1000` makes this slow, so it is + * deliberately a single case rather than the house style for this file. + */ +describe('#5218 — end-to-end through the real chokidar watcher', () => { + it('a real file write invalidates a pre-warmed list()', async () => { + await writeMetadataFile('view', 'v_a', view('v_a')); + const mgr = makeManager(true); + + // Wait for chokidar's initial scan. `ignoreInitial: true` means a file + // created BEFORE `ready` is folded into the baseline and never raises + // `add` — writing without this wait is a race that fails as a 30s + // timeout. Subscribed synchronously after construction, so the event + // cannot have been missed. + const watcher = (mgr as unknown as { watcher: { once(e: string, cb: () => void): unknown } }).watcher; + await new Promise((resolve) => { watcher.once('ready', () => resolve()); }); + + expect(viewNames(await mgr.list('view'))).toEqual(['v_a']); // pre-warm + + const announced = new Promise((resolve) => { + mgr.subscribe('view', () => resolve()); + }); + await writeMetadataFile('view', 'v_b', view('v_b')); + await announced; + + expect(viewNames(await mgr.list('view'))).toEqual(['v_a', 'v_b']); + }, 30_000); +}); diff --git a/packages/metadata/src/node-metadata-manager.ts b/packages/metadata/src/node-metadata-manager.ts index 8089b3628f..a9e2807f50 100644 --- a/packages/metadata/src/node-metadata-manager.ts +++ b/packages/metadata/src/node-metadata-manager.ts @@ -101,10 +101,6 @@ export class NodeMetadataManager extends MetadataManager { const fileName = parts[parts.length - 1]; const name = path.basename(fileName, path.extname(fileName)); - // We can't access private watchCallbacks from parent. - // We need a protected method to trigger watch event or access it. - // OPTION: Add a method `triggerWatchEvent` to MetadataManager - let data: any = undefined; if (eventType !== 'deleted') { try { @@ -127,6 +123,37 @@ export class NodeMetadataManager extends MetadataManager { timestamp: new Date().toISOString(), }; + // [#5218] Invalidate BEFORE announcing. A file event is a *foreign write* + // in the precise sense {@link MetadataManager.invalidateForForeignWrite} + // means: it did not come through this manager's write API, so — unlike + // `register()` / `unregister()` — nothing has refreshed the caches on its + // behalf. `load()` above is a pure read (it delegates to `loadDiagnosed`, + // which only walks the loaders), so before this call the handler left both + // `listCache` and `registry` holding the pre-change state. + // + // Without it, editing `rootDir/view/x.json` left the two read surfaces + // contradicting each other for up to LIST_CACHE_TTL_MS (30s): `get()` saw + // the new file because it falls through to the FilesystemLoader, while + // `list()` — REST `/api/v1/metadata/:type`, the Studio left rail, + // `listViews()` — kept serving the pre-change set. Worse, the HMR/SSE + // consumers woken by the event answer it by re-reading through `list()`, + // so the wake-up handed back exactly the stale data it was announcing. + // Same defect shape as #5109 (cluster peer) with a different trigger; this + // reuses that fix's helper rather than re-deriving it. + // + // Ordering is the same discipline every other write path in the base class + // keeps (`register` / `unregister` / `applyRepoEvent` / the cluster + // subscriber all invalidate, then announce): a watcher must never be able + // to observe the event and the pre-event cache at the same time. + // + // The registry entry goes too, not just the list cache. FS-loaded items + // never enter the registry, so there is usually nothing to delete — but + // when a same-named entry was previously written by `register()` / + // `registerInMemory()` it SHADOWS the loader in both `get()` and `list()`, + // and dropping the list cache alone would leave that stale copy answering + // forever. Deleted, never pre-filled from `data`, per the helper's contract. + this.invalidateForForeignWrite(type, name); + this.notifyWatchers(type, event); } }