From 40a55e77881545b8efa15a335bdeef8bdd6036d0 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 12:24:20 +0000 Subject: [PATCH 1/2] fix(metadata): cache a degraded list() result AS degraded, on a 2s TTL (#5184) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MetadataManager.list()` memoized a known-partial result exactly like a complete one: same 30s `LIST_CACHE_TTL_MS`, no marker on the entry. So the single `error` line #5108 introduced covered a 30s window in which the failing loader was never asked again — no retry, no second signal — and recovery went unnoticed (and `reportLoaderReadRecovered` unlogged) for up to another 30s after the store healed. Not caching degraded reads was rejected on evidence, not on principle: the knex/SQLite single-connection deadlock the `listCache` comment was built for is still reachable on the current driver stack (`DatabaseLoader._find()` does not thread the caller's transaction; driver-sql still models SQLite as a single-connection pool via `activeTransactions` / `assertBareKnexSafe`, the latter a no-op in production; plugin-audit's `captureBefore` threads the transaction by hand for the same reason). Skipping the cache would trade one 30s silent window for a 60s stall per call. - `listCache` entries carry `degraded`, set when a loader threw while the result was assembled. Read through the single `readCachedList()` helper so the flag and its TTL are applied in one place and stay visible to any future consumer. - Degraded entries expire after `DEGRADED_LIST_CACHE_TTL_MS` (2s) instead of 30s: the in-transaction burst is still absorbed, the silent window shrinks 15×, recovery is reported within seconds. - Complete reads are unchanged (cached, not degraded, 30s). - The outage message now names the degraded TTL as the retry interval. Also replaces the field comment's claim that the cache kept "only positive (non-empty) hits or repeated hits with a stable miss signature" — a policy no code ever implemented — with a description of what `cacheListResult()` really does, pinned by a test that an empty complete read IS cached. Internal caching policy only; `IMetadataService` and the public exports are untouched. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Pbu27iNUfQCHeuS551Rqo7 --- .changeset/degraded-list-cache-policy.md | 58 ++++ ...tadata-manager-degraded-list-cache.test.ts | 289 ++++++++++++++++++ packages/metadata/src/metadata-manager.ts | 130 +++++++- 3 files changed, 464 insertions(+), 13 deletions(-) create mode 100644 .changeset/degraded-list-cache-policy.md create mode 100644 packages/metadata/src/metadata-manager-degraded-list-cache.test.ts diff --git a/.changeset/degraded-list-cache-policy.md b/.changeset/degraded-list-cache-policy.md new file mode 100644 index 0000000000..c45427a814 --- /dev/null +++ b/.changeset/degraded-list-cache-policy.md @@ -0,0 +1,58 @@ +--- +"@objectstack/metadata": patch +--- + +fix(metadata): a known-partial `list()` result is cached as degraded, on a 2s TTL instead of 30s (#5184) + +Since #5108 a loader that cannot read its store throws rather than answering +`[]`, so `MetadataManager.list()` catches, reports the outage once at `error`, +and keeps serving what the reachable loaders hold. That best-effort posture is +deliberate. What was not deliberate is what happened on the next line: the +known-short result went into `listCache` on the same 30s TTL as a complete read, +with nothing on the entry to say it was partial. + +The consequences were all invisible from outside. That one `error` line covered a +**30s window in which the failing loader was never asked again** — no retry, no +second signal, the manager simply re-served a set it already knew was short. When +the store came back, nothing noticed for up to another 30s, so #5108's recovery +line (`reportLoaderReadRecovered`) arrived that late too. And because the entry +carried no marker, no consumer of the cache — including that once-only report — +could tell a partial answer from a complete one. + +Not caching degraded reads at all was considered and rejected on evidence. The +`listCache` field comment records why the cache exists: security middleware +calling `list('permission')` from inside a user-initiated DB transaction, where +`DatabaseLoader`'s `engine.find('sys_metadata', …)` tries to take a second knex +connection while the transaction holds SQLite's only one, and knex waits out +`acquireConnectionTimeout` (60s). That hazard was re-verified against the current +driver stack and is still live — `DatabaseLoader._find()` still does not thread +the caller's transaction, `driver-sql` still models SQLite as a +single-connection pool (`activeTransactions`, `assertBareKnexSafe`, the latter a +dev/test guard that no-ops in production), and `plugin-audit` still threads the +transaction by hand for the same reason. Skipping the cache would have traded one +30s silent window for a fresh 60s stall per call. + +So the entry is still cached, but as what it is: + +- `listCache` entries carry a `degraded` flag, set when at least one loader threw + while the result was being assembled. It lives on the entry rather than in a + side table, so every reader can distinguish a complete answer from a partial + one; entries are read through a single `readCachedList()` helper that applies + the flag and its TTL in one place. +- A degraded entry expires after **2s** (`DEGRADED_LIST_CACHE_TTL_MS`) instead of + 30s. The burst of repeated lookups inside one transaction is still absorbed — + those are milliseconds apart — while the window in which a known-short set is + served without re-asking anyone shrinks 15×, and recovery is noticed (and + logged) within seconds of the store healing. +- A complete read is unchanged: cached, not degraded, 30s TTL. +- The outage message now names the degraded TTL as the retry interval, since it + previously promised the 30s one. + +Also closes a `declared ≠ enforced` defect in the same field's comment: it claimed +the cache kept "only positive (non-empty) hits or repeated hits with a stable miss +signature". No such condition ever existed in `cacheListResult()`. The comment now +describes the policy the code actually implements, and the behaviour it claims +(an empty complete read *is* cached) is pinned by a test. + +Internal caching policy only — no change to the `IMetadataService` contract or to +any public export. diff --git a/packages/metadata/src/metadata-manager-degraded-list-cache.test.ts b/packages/metadata/src/metadata-manager-degraded-list-cache.test.ts new file mode 100644 index 0000000000..e4c9a8a7f1 --- /dev/null +++ b/packages/metadata/src/metadata-manager-degraded-list-cache.test.ts @@ -0,0 +1,289 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5184 — a KNOWN-PARTIAL `list()` result must not be memoized as if it were a + * complete one. + * + * After #5108 a loader that cannot read its store throws instead of answering + * `[]`, so `MetadataManager.list()` catches, logs one line at `error`, and + * assembles the result from whatever the remaining loaders hold. That + * best-effort posture is deliberate. What was not deliberate is the next line: + * the known-short result went into `listCache` on the SAME 30s TTL as a + * complete read. One `error` line therefore covered a 30s window in which the + * loader was never asked again — no retry, no second signal — and after the + * store healed it took up to another 30s before anyone noticed, delaying + * `reportLoaderReadRecovered` by the same amount. The entry also carried no + * marker, so no consumer of the cache could tell a partial answer from a + * complete one. + * + * **Why the entry is still cached at all.** The obvious fix — don't cache a + * degraded read — was rejected, and re-verified before it was rejected. The + * `listCache` field comment records why the cache exists: security middleware + * calling `list('permission')` from inside a user-initiated DB transaction, + * where `DatabaseLoader`'s `engine.find('sys_metadata', …)` tries to acquire a + * second knex connection while the transaction holds SQLite's only one, and + * knex waits out `acquireConnectionTimeout` (60s). That hazard is live on the + * current stack: `DatabaseLoader._find()` still does not thread the caller's + * transaction, and `driver-sql` still models SQLite as a single-connection pool + * (`activeTransactions`, `assertBareKnexSafe` — a dev/test guard that no-ops in + * production, so production still eats the timeout), which is why + * `plugin-audit`'s `captureBefore` threads the transaction by hand. Refusing to + * cache degraded reads would swap one 30s silent window for a 60s stall *per + * call*. So the policy is: cache it, but mark it `degraded` and expire it on a + * far shorter TTL. + * + * These tests pin all three halves of that: the flag exists on the entry, the + * degraded TTL is short, and the healthy TTL is untouched. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import type { IDataDriver } from '@objectstack/spec/contracts'; +import { MetadataManager } from './metadata-manager.js'; +import { DatabaseLoader } from './loaders/database-loader.js'; +import { MemoryLoader } from './loaders/memory-loader.js'; + +// Stable logger mock — the recovery timing assertions read what was logged. +const logger = vi.hoisted(() => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), +})); + +vi.mock('@objectstack/core', () => ({ + createLogger: () => logger, +})); + +/** Mirror of the manager's private `ListCacheEntry` (deliberately not exported). */ +type ListCacheEntry = { ts: number; items: unknown[]; degraded: boolean }; + +/** Peek at the private list cache — the shape under test is internal by design. */ +const peekEntry = (mgr: MetadataManager, type: string): ListCacheEntry | undefined => + (mgr as unknown as { listCache: Map }).listCache.get(type); + +/** The two TTLs, read off the class so the test cannot drift from the policy. */ +const ttls = () => { + const c = MetadataManager as unknown as { + LIST_CACHE_TTL_MS: number; + DEGRADED_LIST_CACHE_TTL_MS: number; + }; + return { healthy: c.LIST_CACHE_TTL_MS, degraded: c.DEGRADED_LIST_CACHE_TTL_MS }; +}; + +const connectionReset = () => + Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }); + +/** + * A `sys_metadata` store that fails every read until `heal()`, then serves one + * `permission` row. Minimal on purpose: `loadMany()` only reaches `syncSchema` + * and `find`. + */ +function healableStore() { + let broken = true; + const find = vi.fn(async (): Promise[]> => { + if (broken) throw connectionReset(); + return [ + { + id: 'r1', + name: 'from_db', + type: 'permission', + metadata: JSON.stringify({ name: 'from_db' }), + }, + ]; + }); + const driver = { + name: 'mock', + version: '1.0.0', + supports: {}, + connect: async () => {}, + disconnect: async () => {}, + syncSchema: async () => {}, + find, + } as unknown as IDataDriver; + + return { + driver, + find, + heal: () => { + broken = false; + }, + }; +} + +/** The issue's repro, assembled: registry item + a DatabaseLoader over a dead store. */ +function managerOverBrokenStore() { + const store = healableStore(); + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + // `cache: { enabled: false }` keeps the loader's OWN LRU out of the picture; + // the memoization under test is the manager's. + manager.registerLoader(new DatabaseLoader({ driver: store.driver, cache: { enabled: false } })); + manager.registerInMemory('permission', 'from_code', { name: 'from_code' }); + return { manager, store }; +} + +const names = (items: unknown[]): string[] => + (items as { name: string }[]).map((i) => i.name).sort(); + +/** Everything logged at `info` so far, joined — `registerLoader` also logs here. */ +const infoLines = (): string => logger.info.mock.calls.map((c) => c[0]).join('\n'); + +beforeEach(() => { + logger.error.mockClear(); + logger.info.mockClear(); + logger.warn.mockClear(); + vi.useFakeTimers(); +}); + +afterEach(() => { + vi.useRealTimers(); +}); + +describe('#5184 — a degraded list() result is cached AS degraded', () => { + it('marks the entry `degraded` instead of storing it like a complete read', async () => { + const { manager } = managerOverBrokenStore(); + + expect(names(await manager.list('permission'))).toEqual(['from_code']); + + const entry = peekEntry(manager, 'permission'); + expect(entry).toBeDefined(); + // Still cached — that is what keeps the knex path from re-burning 60s. + expect(entry!.items).toHaveLength(1); + // …but no longer indistinguishable from a complete answer. + expect(entry!.degraded).toBe(true); + }); + + it('a complete read is cached as NOT degraded', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [new MemoryLoader()] }); + manager.registerInMemory('permission', 'from_code', { name: 'from_code' }); + + await manager.list('permission'); + + expect(peekEntry(manager, 'permission')!.degraded).toBe(false); + expect(logger.error).not.toHaveBeenCalled(); + }); + + it('degraded entries expire far sooner than complete ones', () => { + const { healthy, degraded } = ttls(); + expect(healthy).toBe(30_000); + // The ruling's 1–2s band, and an order of magnitude below the healthy TTL. + expect(degraded).toBeGreaterThanOrEqual(1_000); + expect(degraded).toBeLessThanOrEqual(2_000); + expect(degraded * 10).toBeLessThanOrEqual(healthy); + }); +}); + +describe('#5184 — the issue repro: a healed store is not shadowed by the degraded entry', () => { + it('re-asks the loader once the degraded TTL lapses, and serves the healed set', async () => { + const { manager, store } = managerOverBrokenStore(); + + // 1. Outage: one error line, best-effort result, degraded entry cached. + expect(names(await manager.list('permission'))).toEqual(['from_code']); + expect(logger.error).toHaveBeenCalledTimes(1); + const callsAfterFirstList = store.find.mock.calls.length; + + // 2. Inside the degraded window the cache still absorbs the lookups — + // the whole reason the entry is cached at all. + expect(names(await manager.list('permission'))).toEqual(['from_code']); + expect(store.find.mock.calls.length).toBe(callsAfterFirstList); + + // 3. Storage recovers. + store.heal(); + + // 4. Before #5184 this stayed stale for the rest of a 30s window. It no + // longer does: just past the degraded TTL the loader is asked again. + vi.advanceTimersByTime(ttls().degraded + 1); + expect(names(await manager.list('permission'))).toEqual(['from_code', 'from_db']); + expect(store.find.mock.calls.length).toBeGreaterThan(callsAfterFirstList); + }); + + it('the recovery line lands within seconds of the heal, not up to 30s later', async () => { + const { manager, store } = managerOverBrokenStore(); + + await manager.list('permission'); + expect(logger.error).toHaveBeenCalledTimes(1); + expect(infoLines()).not.toMatch(/readable again/i); + + const healedAt = Date.now(); + store.heal(); + + vi.advanceTimersByTime(ttls().degraded + 1); + await manager.list('permission'); + + // Well inside the OLD 30s window — under the previous policy nothing + // would have re-read the loader yet, so `reportLoaderReadRecovered` + // could not have fired. + expect(Date.now() - healedAt).toBeLessThan(ttls().healthy); + + expect(infoLines()).toMatch(/readable again/i); + // Still exactly one outage line — the faster retry must not become log spam. + expect(logger.error).toHaveBeenCalledTimes(1); + }); + + it('a second outage after recovery is reported again, and re-marked degraded', async () => { + const { manager } = managerOverBrokenStore(); + + await manager.list('permission'); + expect(peekEntry(manager, 'permission')!.degraded).toBe(true); + expect(logger.error).toHaveBeenCalledTimes(1); + + // Let the degraded entry lapse against a store that is STILL broken: + // the rewritten entry must be degraded again, not age into a "complete" + // one just because it was re-read. + vi.advanceTimersByTime(ttls().degraded + 1); + await manager.list('permission'); + expect(peekEntry(manager, 'permission')!.degraded).toBe(true); + // Once per outage episode, not once per read. + expect(logger.error).toHaveBeenCalledTimes(1); + }); +}); + +describe('#5184 — the healthy TTL is untouched', () => { + it('a complete read is still served from cache for the full 30s', async () => { + const memory = new MemoryLoader(); + await memory.save('permission', 'stored', { name: 'stored' }); + const manager = new MetadataManager({ formats: ['json'], loaders: [memory] }); + const loadMany = vi.spyOn(memory, 'loadMany'); + + expect(names(await manager.list('permission'))).toEqual(['stored']); + expect(loadMany).toHaveBeenCalledTimes(1); + + // Past the degraded TTL, nowhere near the healthy one. + vi.advanceTimersByTime(ttls().degraded * 3); + await manager.list('permission'); + expect(loadMany).toHaveBeenCalledTimes(1); + + // Just short of 30s — still cached. + vi.advanceTimersByTime(ttls().healthy - ttls().degraded * 3 - 1); + await manager.list('permission'); + expect(loadMany).toHaveBeenCalledTimes(1); + + // Past 30s — re-read, exactly as before. + vi.advanceTimersByTime(2); + await manager.list('permission'); + expect(loadMany).toHaveBeenCalledTimes(2); + }); +}); + +describe('#5184 — 现象二: the comment now describes the code', () => { + /** + * The old field comment promised "we only cache positive (non-empty) hits + * or repeated hits with a stable miss signature". No such condition ever + * existed. Rather than re-assert prose, pin the behaviour the replacement + * comment claims: an empty result IS cached, unconditionally. + */ + it('an empty complete read is cached too — there is no non-empty condition', async () => { + const memory = new MemoryLoader(); + const manager = new MetadataManager({ formats: ['json'], loaders: [memory] }); + const loadMany = vi.spyOn(memory, 'loadMany'); + + expect(await manager.list('permission')).toEqual([]); + const entry = peekEntry(manager, 'permission'); + expect(entry).toBeDefined(); + expect(entry!.items).toEqual([]); + expect(entry!.degraded).toBe(false); + + // And it is served from cache, not re-read. + await manager.list('permission'); + expect(loadMany).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index 9d7881224c..91c1c01786 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -125,6 +125,28 @@ export interface ClusterMetadataChangedPayload { event: MetadataWatchEvent; } +/** + * [#5184] One entry of {@link MetadataManager}'s short-TTL `list()` cache. + * + * Deliberately NOT exported: this is an internal caching-policy detail, not + * part of the `IMetadataService` contract. See the `listCache` field comment + * for the policy this shape encodes. + */ +interface ListCacheEntry { + /** When the entry was written (`Date.now()`). */ + ts: number; + /** The `list()` result being memoized. */ + items: unknown[]; + /** + * True when at least one loader threw while `items` was being assembled, so + * this answer is known to be a partial view of what is declared. Degraded + * entries expire after `DEGRADED_LIST_CACHE_TTL_MS` instead of + * `LIST_CACHE_TTL_MS`, and any future consumer of the cache can branch on + * this rather than having to guess. + */ + degraded: boolean; +} + export interface MetadataManagerOptions extends MetadataManagerConfig { loaders?: MetadataLoader[]; /** Optional IDataDriver instance. When provided alongside config.datasource, auto-configures DatabaseLoader. */ @@ -164,10 +186,54 @@ export class MetadataManager implements IMetadataService { // (60s) before returning []. The cache absorbs the repeated lookups so the // loader is only hit once per TTL window. // + // [#5184] That hazard is NOT historical — it was re-verified on the current + // driver stack before this policy was chosen. `DatabaseLoader._find()` still + // issues `engine.find('sys_metadata', …)` without threading the caller's + // transaction, and `driver-sql` still treats SQLite as a single-connection + // pool (`activeTransactions`, `assertBareKnexSafe` — the latter a dev/test + // guard that is a no-op in production, so production still waits the timeout + // out). `plugin-audit`'s `captureBefore` threads the transaction by hand for + // exactly this reason. Hence the policy below keeps caching degraded reads + // rather than skipping them: "don't cache a degraded read" would trade one + // 30s silent window for a fresh 60s stall per call. + // + // [#5184] WHAT IS ACTUALLY CACHED, AND FOR HOW LONG — this paragraph is the + // contract, and it describes `cacheListResult()` / `readCachedList()` below. + // (An earlier version of this comment claimed the cache kept "only positive + // (non-empty) hits or repeated hits with a stable miss signature". No such + // condition ever existed in the code. Comment is contract; a comment that + // describes a policy nothing implements is a declared ≠ enforced defect in + // its own right, so it is replaced rather than patched.) + // + // • EVERY completed `list()` is cached, empty results included. There is + // no non-empty test and no "miss signature" concept. + // • An entry assembled while at least one loader THREW is a known-partial + // answer: it is cached with `degraded: true` and expires after + // `DEGRADED_LIST_CACHE_TTL_MS`, not `LIST_CACHE_TTL_MS`. So the burst of + // repeated lookups the knex path above depends on is still absorbed, + // while the window in which the manager serves a known-short set without + // re-asking anyone shrinks from 30s to ~2s. Recovery is therefore also + // noticed (and `reportLoaderReadRecovered` logged) within ~2s of storage + // healing instead of up to 30s later. + // • `degraded` lives ON the entry, not in a side table, so every consumer + // of the cache can tell a complete answer from a partial one. Read + // entries through `readCachedList()` rather than `listCache.get()`, so + // the flag and its TTL are applied in one place. + // // Invalidated on every `register()` / `unregister()` to keep CRUD writes // visible to subsequent reads. - private listCache = new Map(); + private listCache = new Map(); private static readonly LIST_CACHE_TTL_MS = 30_000; + /** + * [#5184] TTL for an entry produced by a degraded read (≥1 loader threw). + * + * Deliberately at the top of the 1–2s band: the point of keeping degraded + * results cached at all is to absorb a burst of `list()` calls issued from + * inside one open transaction, and those bursts are milliseconds apart but + * can be spread by per-row work. Two seconds covers that while still being + * 15× shorter than the healthy TTL. + */ + private static readonly DEGRADED_LIST_CACHE_TTL_MS = 2_000; // [#5108] Loader names whose read failure has already been reported at // `error` by `list()`. AGENTS.md → "Degradation log levels": say it once, at @@ -539,12 +605,11 @@ export class MetadataManager implements IMetadataService { * List all metadata items of a given type */ async list(type: string): Promise { - // Short-TTL cache: see field comment on `listCache`. Skip when called - // from tests / hot reloads that rely on always-fresh reads — we only - // cache positive (non-empty) hits or repeated hits with a stable miss - // signature. - const cached = this.listCache.get(type); - if (cached && Date.now() - cached.ts < MetadataManager.LIST_CACHE_TTL_MS) { + // Short-TTL cache: see the field comment on `listCache` for what is + // cached and for how long. Every completed read is memoized; a read that + // lost a loader is memoized as `degraded` and expires ~15× sooner. + const cached = this.readCachedList(type); + if (cached) { return cached.items; } @@ -558,7 +623,11 @@ export class MetadataManager implements IMetadataService { } } - // From loaders (deduplicate) + // From loaders (deduplicate). [#5184] `degraded` records whether this + // particular read lost a loader, so the memoized answer carries the fact + // that it is known-partial instead of being indistinguishable from a + // complete one. + let degraded = false; for (const loader of this.loaders.values()) { try { const loaderItems = await loader.loadMany(type); @@ -570,12 +639,13 @@ export class MetadataManager implements IMetadataService { } this.reportLoaderReadRecovered(loader.contract.name); } catch (e) { + degraded = true; this.reportLoaderReadFailure(loader.contract.name, type, e); } } const result = Array.from(items.values()); - this.cacheListResult(type, result); + this.cacheListResult(type, result, degraded); return result; } @@ -601,6 +671,14 @@ export class MetadataManager implements IMetadataService { * Said **once** per loader, and un-said on recovery, because `list()` is a * hot path — one line per outage, not one per read. * + * [#5184] The once-only guard carries more weight than it used to: a + * degraded `list()` result is now memoized for `DEGRADED_LIST_CACHE_TTL_MS` + * rather than `LIST_CACHE_TTL_MS`, so during an outage the loader is + * re-asked (and this method re-entered) roughly every 2s instead of every + * 30s. That is the point — the outage stops being a 30s silent window and + * recovery is noticed within seconds — and it costs nothing in log volume + * precisely because `loaderReadFailureReported` still speaks only once. + * * Deliberately does NOT rethrow: `list()` is the best-effort listing seam and * must keep serving what the reachable loaders hold. The strict counterpart * for callers whose answer is a security decision is `listForIndex()` (no @@ -617,8 +695,9 @@ export class MetadataManager implements IMetadataService { `Consumers that gate on a declared set (permissions, sharing rules, policies, api endpoints) will read the ` + `declarations this loader holds as "never declared" — which grants or locks out depending on the consumer, silently either way. ` + `Fix: check the datasource behind \`${loaderName}\` — connection, credentials, and that its metadata table exists. ` + - `The read is retried on the next list once the ${MetadataManager.LIST_CACHE_TTL_MS}ms list cache lapses, so a transient ` + - `cause recovers on its own and the recovery is logged.`, + `The read is retried on the next list once the ${MetadataManager.DEGRADED_LIST_CACHE_TTL_MS}ms degraded-result list cache lapses ` + + `(a known-partial listing is memoized far more briefly than a complete one — #5184), so a transient cause recovers on its ` + + `own within seconds and the recovery is logged.`, error instanceof Error ? error : undefined, { loader: loaderName, type, error }, ); @@ -632,8 +711,33 @@ export class MetadataManager implements IMetadataService { ); } - private cacheListResult(type: string, items: unknown[]): void { - this.listCache.set(type, { ts: Date.now(), items }); + /** + * Memoize a completed {@link list} result. + * + * [#5184] `degraded` is not optional at the call site by accident — it is the + * one thing this cache used to throw away. A result assembled while a loader + * was unreadable is stored, but stored *as* what it is, so it expires on the + * degraded TTL and any reader can tell it apart from a complete answer. + */ + private cacheListResult(type: string, items: unknown[], degraded: boolean): void { + this.listCache.set(type, { ts: Date.now(), items, degraded }); + } + + /** + * Read a still-fresh {@link listCache} entry, or `undefined` when there is + * none / it has expired. + * + * [#5184] The single place the TTL policy is applied, so "a degraded entry + * expires sooner" cannot be forgotten by a second reader. Returns the whole + * entry rather than just `items` so callers keep access to `degraded`. + */ + private readCachedList(type: string): ListCacheEntry | undefined { + const cached = this.listCache.get(type); + if (!cached) return undefined; + const ttl = cached.degraded + ? MetadataManager.DEGRADED_LIST_CACHE_TTL_MS + : MetadataManager.LIST_CACHE_TTL_MS; + return Date.now() - cached.ts < ttl ? cached : undefined; } /** Internal helper: drop the cached `list()` result for a type. */ From 47a6bb82935b8b33a136294603cf4eaef2a145a0 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 12:27:37 +0000 Subject: [PATCH 2/2] docs(metadata): qualify the listCache "once per TTL window" claim (#5253) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sentence is true for sequential callers only — nothing is written until a read completes, so concurrent callers all miss and each walk every loader. Filed as #5253; noting it here so the comment this PR just turned into a contract does not carry a fresh overclaim. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Pbu27iNUfQCHeuS551Rqo7 --- packages/metadata/src/metadata-manager.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index 91c1c01786..a190b2fb5b 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -184,7 +184,10 @@ export class MetadataManager implements IMetadataService { // acquire a fresh knex connection while the transaction is still holding // SQLite's single connection — knex waits the full `acquireConnectionTimeout` // (60s) before returning []. The cache absorbs the repeated lookups so the - // loader is only hit once per TTL window. + // loader is only hit once per TTL window — for SEQUENTIAL callers. Nothing is + // written until a read completes, so calls issued concurrently with the first + // one all miss and each walk every loader (#5253); read that sentence as a + // statement about repeated lookups, not a concurrency guarantee. // // [#5184] That hazard is NOT historical — it was re-verified on the current // driver stack before this policy was chosen. `DatabaseLoader._find()` still